[PATCH v1 0/5] dma/xlnx-zdma: Bug fixes

[PATCH v1 0/5] dma/xlnx-zdma: Bug fixes

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Hi,

This series fixes a couple of bugs we've ran into with some
proprietary test code and drivers using the Xilinx zDMA.

Cheers,
Edgar

Edgar E. Iglesias (5):
  dma/xlnx-zdma: Remove comment
  dma/xlnx-zdma: Populate DBG0.CMN_BUF_FREE
  dma/xlnx-zdma: Clear DMA_DONE when halting
  dma/xlnx-zdma: Advance the descriptor address when stopping
  dma/xlnx-zdma: Reorg to fix CUR_DSCR

 hw/dma/xlnx-zdma.c | 56 ++++++++++++++++++++++++----------------------
 1 file changed, 29 insertions(+), 27 deletions(-)

-- 
2.20.1

[PATCH v1 1/5] dma/xlnx-zdma: Remove comment

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Remove comment.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 hw/dma/xlnx-zdma.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/dma/xlnx-zdma.c b/hw/dma/xlnx-zdma.c
index 2d9c0a0d5e..a6c5b2304a 100644
--- a/hw/dma/xlnx-zdma.c
+++ b/hw/dma/xlnx-zdma.c
@@ -511,7 +511,6 @@ static void zdma_process_descr(XlnxZDMA *s)
         zdma_src_done(s);
     }
 
-    /* Load next descriptor.  */
     if (ptype == PT_REG || src_cmd == CMD_STOP) {
         ARRAY_FIELD_DP32(s->regs, ZDMA_CH_CTRL2, EN, 0);
         zdma_set_state(s, DISABLED);
-- 
2.20.1

[PATCH v1 2/5] dma/xlnx-zdma: Populate DBG0.CMN_BUF_FREE

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Populate DBG0.CMN_BUF_FREE so that SW can see some free space.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 hw/dma/xlnx-zdma.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/dma/xlnx-zdma.c b/hw/dma/xlnx-zdma.c
index a6c5b2304a..6a4699757a 100644
--- a/hw/dma/xlnx-zdma.c
+++ b/hw/dma/xlnx-zdma.c
@@ -680,6 +680,12 @@ static RegisterAccessInfo zdma_regs_info[] = {
     },{ .name = "ZDMA_CH_DBG0",  .addr = A_ZDMA_CH_DBG0,
         .rsvd = 0xfffffe00,
         .ro = 0x1ff,
+
+        /*
+         * There's SW out there that will check the debug regs for free space.
+         * Claim that we always have 0x100 free.
+         */
+        .reset = 0x100
     },{ .name = "ZDMA_CH_DBG1",  .addr = A_ZDMA_CH_DBG1,
         .rsvd = 0xfffffe00,
         .ro = 0x1ff,
-- 
2.20.1

[PATCH v1 3/5] dma/xlnx-zdma: Clear DMA_DONE when halting

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Clear DMA_DONE when halting the DMA channel.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 hw/dma/xlnx-zdma.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/dma/xlnx-zdma.c b/hw/dma/xlnx-zdma.c
index 6a4699757a..dd893bc420 100644
--- a/hw/dma/xlnx-zdma.c
+++ b/hw/dma/xlnx-zdma.c
@@ -520,6 +520,7 @@ static void zdma_process_descr(XlnxZDMA *s)
     if (src_cmd == CMD_HALT) {
         zdma_set_state(s, PAUSED);
         ARRAY_FIELD_DP32(s->regs, ZDMA_CH_ISR, DMA_PAUSE, 1);
+        ARRAY_FIELD_DP32(s->regs, ZDMA_CH_ISR, DMA_DONE, false);
         zdma_ch_imr_update_irq(s);
         return;
     }
-- 
2.20.1

[PATCH v1 4/5] dma/xlnx-zdma: Advance the descriptor address when stopping

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Advance the descriptor address when stopping the channel.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 hw/dma/xlnx-zdma.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/dma/xlnx-zdma.c b/hw/dma/xlnx-zdma.c
index dd893bc420..e856d233f2 100644
--- a/hw/dma/xlnx-zdma.c
+++ b/hw/dma/xlnx-zdma.c
@@ -514,7 +514,6 @@ static void zdma_process_descr(XlnxZDMA *s)
     if (ptype == PT_REG || src_cmd == CMD_STOP) {
         ARRAY_FIELD_DP32(s->regs, ZDMA_CH_CTRL2, EN, 0);
         zdma_set_state(s, DISABLED);
-        return;
     }
 
     if (src_cmd == CMD_HALT) {
-- 
2.20.1

[PATCH v1 5/5] dma/xlnx-zdma: Reorg to fix CUR_DSCR

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Reorganize the descriptor handling so that CUR_DSCR always
points to the next descriptor to be processed.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 hw/dma/xlnx-zdma.c | 47 ++++++++++++++++++++++------------------------
 1 file changed, 22 insertions(+), 25 deletions(-)

diff --git a/hw/dma/xlnx-zdma.c b/hw/dma/xlnx-zdma.c
index e856d233f2..1c45367f3c 100644
--- a/hw/dma/xlnx-zdma.c
+++ b/hw/dma/xlnx-zdma.c
@@ -333,10 +333,28 @@ static void zdma_load_src_descriptor(XlnxZDMA *s)
     }
 }
 
+static void zdma_update_descr_addr(XlnxZDMA *s, bool type,
+                                   unsigned int basereg)
+{
+    uint64_t addr, next;
+
+    if (type == DTYPE_LINEAR) {
+        addr = zdma_get_regaddr64(s, basereg);
+        next = addr + sizeof(s->dsc_dst);
+    } else {
+        addr = zdma_get_regaddr64(s, basereg);
+        addr += sizeof(s->dsc_dst);
+        address_space_read(s->dma_as, addr, s->attr, (void *) &next, 8);
+    }
+
+    zdma_put_regaddr64(s, basereg, next);
+}
+
 static void zdma_load_dst_descriptor(XlnxZDMA *s)
 {
     uint64_t dst_addr;
     unsigned int ptype = ARRAY_FIELD_EX32(s->regs, ZDMA_CH_CTRL0, POINT_TYPE);
+    bool dst_type;
 
     if (ptype == PT_REG) {
         memcpy(&s->dsc_dst, &s->regs[R_ZDMA_CH_DST_DSCR_WORD0],
@@ -349,24 +367,10 @@ static void zdma_load_dst_descriptor(XlnxZDMA *s)
     if (!zdma_load_descriptor(s, dst_addr, &s->dsc_dst)) {
         ARRAY_FIELD_DP32(s->regs, ZDMA_CH_ISR, AXI_RD_DST_DSCR, true);
     }
-}
-
-static uint64_t zdma_update_descr_addr(XlnxZDMA *s, bool type,
-                                       unsigned int basereg)
-{
-    uint64_t addr, next;
 
-    if (type == DTYPE_LINEAR) {
-        next = zdma_get_regaddr64(s, basereg);
-        next += sizeof(s->dsc_dst);
-        zdma_put_regaddr64(s, basereg, next);
-    } else {
-        addr = zdma_get_regaddr64(s, basereg);
-        addr += sizeof(s->dsc_dst);
-        address_space_read(s->dma_as, addr, s->attr, &next, 8);
-        zdma_put_regaddr64(s, basereg, next);
-    }
-    return next;
+    /* Advance the descriptor pointer.  */
+    dst_type = FIELD_EX32(s->dsc_dst.words[3], ZDMA_CH_DST_DSCR_WORD3, TYPE);
+    zdma_update_descr_addr(s, dst_type, R_ZDMA_CH_DST_CUR_DSCR_LSB);
 }
 
 static void zdma_write_dst(XlnxZDMA *s, uint8_t *buf, uint32_t len)
@@ -387,14 +391,7 @@ static void zdma_write_dst(XlnxZDMA *s, uint8_t *buf, uint32_t len)
         dst_size = FIELD_EX32(s->dsc_dst.words[2], ZDMA_CH_DST_DSCR_WORD2,
                               SIZE);
         if (dst_size == 0 && ptype == PT_MEM) {
-            uint64_t next;
-            bool dst_type = FIELD_EX32(s->dsc_dst.words[3],
-                                       ZDMA_CH_DST_DSCR_WORD3,
-                                       TYPE);
-
-            next = zdma_update_descr_addr(s, dst_type,
-                                          R_ZDMA_CH_DST_CUR_DSCR_LSB);
-            zdma_load_descriptor(s, next, &s->dsc_dst);
+            zdma_load_dst_descriptor(s);
             dst_size = FIELD_EX32(s->dsc_dst.words[2], ZDMA_CH_DST_DSCR_WORD2,
                                   SIZE);
         }
-- 
2.20.1

Re: [PATCH v1 1/5] dma/xlnx-zdma: Remove comment

[Alistair Francis]

On Thu, Apr 2, 2020 at 6:49 AM Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
>
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Remove comment.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/dma/xlnx-zdma.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/hw/dma/xlnx-zdma.c b/hw/dma/xlnx-zdma.c
> index 2d9c0a0d5e..a6c5b2304a 100644
> --- a/hw/dma/xlnx-zdma.c
> +++ b/hw/dma/xlnx-zdma.c
> @@ -511,7 +511,6 @@ static void zdma_process_descr(XlnxZDMA *s)
>          zdma_src_done(s);
>      }
>
> -    /* Load next descriptor.  */
>      if (ptype == PT_REG || src_cmd == CMD_STOP) {
>          ARRAY_FIELD_DP32(s->regs, ZDMA_CH_CTRL2, EN, 0);
>          zdma_set_state(s, DISABLED);
> --
> 2.20.1
>
>

Re: [PATCH v1 2/5] dma/xlnx-zdma: Populate DBG0.CMN_BUF_FREE

[Alistair Francis]

On Thu, Apr 2, 2020 at 6:46 AM Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
>
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Populate DBG0.CMN_BUF_FREE so that SW can see some free space.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/dma/xlnx-zdma.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/hw/dma/xlnx-zdma.c b/hw/dma/xlnx-zdma.c
> index a6c5b2304a..6a4699757a 100644
> --- a/hw/dma/xlnx-zdma.c
> +++ b/hw/dma/xlnx-zdma.c
> @@ -680,6 +680,12 @@ static RegisterAccessInfo zdma_regs_info[] = {
>      },{ .name = "ZDMA_CH_DBG0",  .addr = A_ZDMA_CH_DBG0,
>          .rsvd = 0xfffffe00,
>          .ro = 0x1ff,
> +
> +        /*
> +         * There's SW out there that will check the debug regs for free space.
> +         * Claim that we always have 0x100 free.
> +         */
> +        .reset = 0x100
>      },{ .name = "ZDMA_CH_DBG1",  .addr = A_ZDMA_CH_DBG1,
>          .rsvd = 0xfffffe00,
>          .ro = 0x1ff,
> --
> 2.20.1
>
>

Re: [PATCH v1 3/5] dma/xlnx-zdma: Clear DMA_DONE when halting

[Alistair Francis]

On Thu, Apr 2, 2020 at 6:50 AM Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
>
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Clear DMA_DONE when halting the DMA channel.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Acked-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/dma/xlnx-zdma.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/dma/xlnx-zdma.c b/hw/dma/xlnx-zdma.c
> index 6a4699757a..dd893bc420 100644
> --- a/hw/dma/xlnx-zdma.c
> +++ b/hw/dma/xlnx-zdma.c
> @@ -520,6 +520,7 @@ static void zdma_process_descr(XlnxZDMA *s)
>      if (src_cmd == CMD_HALT) {
>          zdma_set_state(s, PAUSED);
>          ARRAY_FIELD_DP32(s->regs, ZDMA_CH_ISR, DMA_PAUSE, 1);
> +        ARRAY_FIELD_DP32(s->regs, ZDMA_CH_ISR, DMA_DONE, false);
>          zdma_ch_imr_update_irq(s);
>          return;
>      }
> --
> 2.20.1
>
>

Re: [PATCH v1 4/5] dma/xlnx-zdma: Advance the descriptor address when stopping

[Alistair Francis]

On Thu, Apr 2, 2020 at 6:47 AM Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
>
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Advance the descriptor address when stopping the channel.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Acked-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/dma/xlnx-zdma.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/hw/dma/xlnx-zdma.c b/hw/dma/xlnx-zdma.c
> index dd893bc420..e856d233f2 100644
> --- a/hw/dma/xlnx-zdma.c
> +++ b/hw/dma/xlnx-zdma.c
> @@ -514,7 +514,6 @@ static void zdma_process_descr(XlnxZDMA *s)
>      if (ptype == PT_REG || src_cmd == CMD_STOP) {
>          ARRAY_FIELD_DP32(s->regs, ZDMA_CH_CTRL2, EN, 0);
>          zdma_set_state(s, DISABLED);
> -        return;
>      }
>
>      if (src_cmd == CMD_HALT) {
> --
> 2.20.1
>
>

Re: [PATCH v1 5/5] dma/xlnx-zdma: Reorg to fix CUR_DSCR

[Alistair Francis]

On Thu, Apr 2, 2020 at 6:50 AM Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
>
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Reorganize the descriptor handling so that CUR_DSCR always
> points to the next descriptor to be processed.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/dma/xlnx-zdma.c | 47 ++++++++++++++++++++++------------------------
>  1 file changed, 22 insertions(+), 25 deletions(-)
>
> diff --git a/hw/dma/xlnx-zdma.c b/hw/dma/xlnx-zdma.c
> index e856d233f2..1c45367f3c 100644
> --- a/hw/dma/xlnx-zdma.c
> +++ b/hw/dma/xlnx-zdma.c
> @@ -333,10 +333,28 @@ static void zdma_load_src_descriptor(XlnxZDMA *s)
>      }
>  }
>
> +static void zdma_update_descr_addr(XlnxZDMA *s, bool type,
> +                                   unsigned int basereg)
> +{
> +    uint64_t addr, next;
> +
> +    if (type == DTYPE_LINEAR) {
> +        addr = zdma_get_regaddr64(s, basereg);
> +        next = addr + sizeof(s->dsc_dst);
> +    } else {
> +        addr = zdma_get_regaddr64(s, basereg);
> +        addr += sizeof(s->dsc_dst);
> +        address_space_read(s->dma_as, addr, s->attr, (void *) &next, 8);
> +    }
> +
> +    zdma_put_regaddr64(s, basereg, next);
> +}
> +
>  static void zdma_load_dst_descriptor(XlnxZDMA *s)
>  {
>      uint64_t dst_addr;
>      unsigned int ptype = ARRAY_FIELD_EX32(s->regs, ZDMA_CH_CTRL0, POINT_TYPE);
> +    bool dst_type;
>
>      if (ptype == PT_REG) {
>          memcpy(&s->dsc_dst, &s->regs[R_ZDMA_CH_DST_DSCR_WORD0],
> @@ -349,24 +367,10 @@ static void zdma_load_dst_descriptor(XlnxZDMA *s)
>      if (!zdma_load_descriptor(s, dst_addr, &s->dsc_dst)) {
>          ARRAY_FIELD_DP32(s->regs, ZDMA_CH_ISR, AXI_RD_DST_DSCR, true);
>      }
> -}
> -
> -static uint64_t zdma_update_descr_addr(XlnxZDMA *s, bool type,
> -                                       unsigned int basereg)
> -{
> -    uint64_t addr, next;
>
> -    if (type == DTYPE_LINEAR) {
> -        next = zdma_get_regaddr64(s, basereg);
> -        next += sizeof(s->dsc_dst);
> -        zdma_put_regaddr64(s, basereg, next);
> -    } else {
> -        addr = zdma_get_regaddr64(s, basereg);
> -        addr += sizeof(s->dsc_dst);
> -        address_space_read(s->dma_as, addr, s->attr, &next, 8);
> -        zdma_put_regaddr64(s, basereg, next);
> -    }
> -    return next;
> +    /* Advance the descriptor pointer.  */
> +    dst_type = FIELD_EX32(s->dsc_dst.words[3], ZDMA_CH_DST_DSCR_WORD3, TYPE);
> +    zdma_update_descr_addr(s, dst_type, R_ZDMA_CH_DST_CUR_DSCR_LSB);
>  }
>
>  static void zdma_write_dst(XlnxZDMA *s, uint8_t *buf, uint32_t len)
> @@ -387,14 +391,7 @@ static void zdma_write_dst(XlnxZDMA *s, uint8_t *buf, uint32_t len)
>          dst_size = FIELD_EX32(s->dsc_dst.words[2], ZDMA_CH_DST_DSCR_WORD2,
>                                SIZE);
>          if (dst_size == 0 && ptype == PT_MEM) {
> -            uint64_t next;
> -            bool dst_type = FIELD_EX32(s->dsc_dst.words[3],
> -                                       ZDMA_CH_DST_DSCR_WORD3,
> -                                       TYPE);
> -
> -            next = zdma_update_descr_addr(s, dst_type,
> -                                          R_ZDMA_CH_DST_CUR_DSCR_LSB);
> -            zdma_load_descriptor(s, next, &s->dsc_dst);
> +            zdma_load_dst_descriptor(s);
>              dst_size = FIELD_EX32(s->dsc_dst.words[2], ZDMA_CH_DST_DSCR_WORD2,
>                                    SIZE);
>          }
> --
> 2.20.1
>
>

Re: [PATCH v1 1/5] dma/xlnx-zdma: Remove comment

[Francisco Iglesias]

On [2020 Apr 02] Thu 15:47:17, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> 
> Remove comment.
> 
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com>

> ---
>  hw/dma/xlnx-zdma.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/hw/dma/xlnx-zdma.c b/hw/dma/xlnx-zdma.c
> index 2d9c0a0d5e..a6c5b2304a 100644
> --- a/hw/dma/xlnx-zdma.c
> +++ b/hw/dma/xlnx-zdma.c
> @@ -511,7 +511,6 @@ static void zdma_process_descr(XlnxZDMA *s)
>          zdma_src_done(s);
>      }
>  
> -    /* Load next descriptor.  */
>      if (ptype == PT_REG || src_cmd == CMD_STOP) {
>          ARRAY_FIELD_DP32(s->regs, ZDMA_CH_CTRL2, EN, 0);
>          zdma_set_state(s, DISABLED);
> -- 
> 2.20.1
> 

Re: [PATCH v1 2/5] dma/xlnx-zdma: Populate DBG0.CMN_BUF_FREE

[Francisco Iglesias]

On [2020 Apr 02] Thu 15:47:18, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> 
> Populate DBG0.CMN_BUF_FREE so that SW can see some free space.
> 
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com>

> ---
>  hw/dma/xlnx-zdma.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/hw/dma/xlnx-zdma.c b/hw/dma/xlnx-zdma.c
> index a6c5b2304a..6a4699757a 100644
> --- a/hw/dma/xlnx-zdma.c
> +++ b/hw/dma/xlnx-zdma.c
> @@ -680,6 +680,12 @@ static RegisterAccessInfo zdma_regs_info[] = {
>      },{ .name = "ZDMA_CH_DBG0",  .addr = A_ZDMA_CH_DBG0,
>          .rsvd = 0xfffffe00,
>          .ro = 0x1ff,
> +
> +        /*
> +         * There's SW out there that will check the debug regs for free space.
> +         * Claim that we always have 0x100 free.
> +         */
> +        .reset = 0x100
>      },{ .name = "ZDMA_CH_DBG1",  .addr = A_ZDMA_CH_DBG1,
>          .rsvd = 0xfffffe00,
>          .ro = 0x1ff,
> -- 
> 2.20.1
> 

Re: [PATCH v1 3/5] dma/xlnx-zdma: Clear DMA_DONE when halting

[Francisco Iglesias]

On [2020 Apr 02] Thu 15:47:19, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> 
> Clear DMA_DONE when halting the DMA channel.
> 
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com>

> ---
>  hw/dma/xlnx-zdma.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/dma/xlnx-zdma.c b/hw/dma/xlnx-zdma.c
> index 6a4699757a..dd893bc420 100644
> --- a/hw/dma/xlnx-zdma.c
> +++ b/hw/dma/xlnx-zdma.c
> @@ -520,6 +520,7 @@ static void zdma_process_descr(XlnxZDMA *s)
>      if (src_cmd == CMD_HALT) {
>          zdma_set_state(s, PAUSED);
>          ARRAY_FIELD_DP32(s->regs, ZDMA_CH_ISR, DMA_PAUSE, 1);
> +        ARRAY_FIELD_DP32(s->regs, ZDMA_CH_ISR, DMA_DONE, false);
>          zdma_ch_imr_update_irq(s);
>          return;
>      }
> -- 
> 2.20.1
> 

Re: [PATCH v1 4/5] dma/xlnx-zdma: Advance the descriptor address when stopping

[Francisco Iglesias]

On [2020 Apr 02] Thu 15:47:20, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> 
> Advance the descriptor address when stopping the channel.
> 
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com>

> ---
>  hw/dma/xlnx-zdma.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/hw/dma/xlnx-zdma.c b/hw/dma/xlnx-zdma.c
> index dd893bc420..e856d233f2 100644
> --- a/hw/dma/xlnx-zdma.c
> +++ b/hw/dma/xlnx-zdma.c
> @@ -514,7 +514,6 @@ static void zdma_process_descr(XlnxZDMA *s)
>      if (ptype == PT_REG || src_cmd == CMD_STOP) {
>          ARRAY_FIELD_DP32(s->regs, ZDMA_CH_CTRL2, EN, 0);
>          zdma_set_state(s, DISABLED);
> -        return;
>      }
>  
>      if (src_cmd == CMD_HALT) {
> -- 
> 2.20.1
> 

Re: [PATCH v1 5/5] dma/xlnx-zdma: Reorg to fix CUR_DSCR

[Francisco Iglesias]

On [2020 Apr 02] Thu 15:47:21, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> 
> Reorganize the descriptor handling so that CUR_DSCR always
> points to the next descriptor to be processed.
> 
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com>

> ---
>  hw/dma/xlnx-zdma.c | 47 ++++++++++++++++++++++------------------------
>  1 file changed, 22 insertions(+), 25 deletions(-)
> 
> diff --git a/hw/dma/xlnx-zdma.c b/hw/dma/xlnx-zdma.c
> index e856d233f2..1c45367f3c 100644
> --- a/hw/dma/xlnx-zdma.c
> +++ b/hw/dma/xlnx-zdma.c
> @@ -333,10 +333,28 @@ static void zdma_load_src_descriptor(XlnxZDMA *s)
>      }
>  }
>  
> +static void zdma_update_descr_addr(XlnxZDMA *s, bool type,
> +                                   unsigned int basereg)
> +{
> +    uint64_t addr, next;
> +
> +    if (type == DTYPE_LINEAR) {
> +        addr = zdma_get_regaddr64(s, basereg);
> +        next = addr + sizeof(s->dsc_dst);
> +    } else {
> +        addr = zdma_get_regaddr64(s, basereg);
> +        addr += sizeof(s->dsc_dst);
> +        address_space_read(s->dma_as, addr, s->attr, (void *) &next, 8);
> +    }
> +
> +    zdma_put_regaddr64(s, basereg, next);
> +}
> +
>  static void zdma_load_dst_descriptor(XlnxZDMA *s)
>  {
>      uint64_t dst_addr;
>      unsigned int ptype = ARRAY_FIELD_EX32(s->regs, ZDMA_CH_CTRL0, POINT_TYPE);
> +    bool dst_type;
>  
>      if (ptype == PT_REG) {
>          memcpy(&s->dsc_dst, &s->regs[R_ZDMA_CH_DST_DSCR_WORD0],
> @@ -349,24 +367,10 @@ static void zdma_load_dst_descriptor(XlnxZDMA *s)
>      if (!zdma_load_descriptor(s, dst_addr, &s->dsc_dst)) {
>          ARRAY_FIELD_DP32(s->regs, ZDMA_CH_ISR, AXI_RD_DST_DSCR, true);
>      }
> -}
> -
> -static uint64_t zdma_update_descr_addr(XlnxZDMA *s, bool type,
> -                                       unsigned int basereg)
> -{
> -    uint64_t addr, next;
>  
> -    if (type == DTYPE_LINEAR) {
> -        next = zdma_get_regaddr64(s, basereg);
> -        next += sizeof(s->dsc_dst);
> -        zdma_put_regaddr64(s, basereg, next);
> -    } else {
> -        addr = zdma_get_regaddr64(s, basereg);
> -        addr += sizeof(s->dsc_dst);
> -        address_space_read(s->dma_as, addr, s->attr, &next, 8);
> -        zdma_put_regaddr64(s, basereg, next);
> -    }
> -    return next;
> +    /* Advance the descriptor pointer.  */
> +    dst_type = FIELD_EX32(s->dsc_dst.words[3], ZDMA_CH_DST_DSCR_WORD3, TYPE);
> +    zdma_update_descr_addr(s, dst_type, R_ZDMA_CH_DST_CUR_DSCR_LSB);
>  }
>  
>  static void zdma_write_dst(XlnxZDMA *s, uint8_t *buf, uint32_t len)
> @@ -387,14 +391,7 @@ static void zdma_write_dst(XlnxZDMA *s, uint8_t *buf, uint32_t len)
>          dst_size = FIELD_EX32(s->dsc_dst.words[2], ZDMA_CH_DST_DSCR_WORD2,
>                                SIZE);
>          if (dst_size == 0 && ptype == PT_MEM) {
> -            uint64_t next;
> -            bool dst_type = FIELD_EX32(s->dsc_dst.words[3],
> -                                       ZDMA_CH_DST_DSCR_WORD3,
> -                                       TYPE);
> -
> -            next = zdma_update_descr_addr(s, dst_type,
> -                                          R_ZDMA_CH_DST_CUR_DSCR_LSB);
> -            zdma_load_descriptor(s, next, &s->dsc_dst);
> +            zdma_load_dst_descriptor(s);
>              dst_size = FIELD_EX32(s->dsc_dst.words[2], ZDMA_CH_DST_DSCR_WORD2,
>                                    SIZE);
>          }
> -- 
> 2.20.1
> 

Re: [PATCH v1 5/5] dma/xlnx-zdma: Reorg to fix CUR_DSCR

[Peter Maydell]

On Thu, 2 Apr 2020 at 14:46, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
>
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Reorganize the descriptor handling so that CUR_DSCR always
> points to the next descriptor to be processed.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---

This is just moved-code in this patch so I think it's
a separate issue, but it looks like you have an endianness
bug here:

> +static void zdma_update_descr_addr(XlnxZDMA *s, bool type,
> +                                   unsigned int basereg)
> +{
> +    uint64_t addr, next;
> +
> +    if (type == DTYPE_LINEAR) {
> +        addr = zdma_get_regaddr64(s, basereg);
> +        next = addr + sizeof(s->dsc_dst);
> +    } else {
> +        addr = zdma_get_regaddr64(s, basereg);
> +        addr += sizeof(s->dsc_dst);
> +        address_space_read(s->dma_as, addr, s->attr, (void *) &next, 8);

This reads 8 bytes into the uint64_t 'next', which means
that the value from C's point of view will be different
for big-endian and little-endian hosts. You probably wanted
address_space_ldq_le(), assuming the h/w always does
little-endian reads and that these get_regaddr64 and
put_regaddr64 functions work with host-endian integers.

There's a similar problem elsewhere in the device where
it does this:
    address_space_read(s->dma_as, addr, s->attr, buf, sizeof(XlnxZDMADescr));
and assumes the guest structure is the same layout and
endianness as the host struct XlnxDMADescr.

I'm not a huge fan of defining host C structs to match
guest data structures, but if you want to go that way
you ought to (a) be byteswapping the contents appropriately
and (b) have a compile-time assert that the size of the
struct is what you think it is and the compiler hasn't
inserted any helpful extra padding.

thanks
-- PMM

Re: [PATCH v1 0/5] dma/xlnx-zdma: Bug fixes

[Peter Maydell]

On Thu, 2 Apr 2020 at 14:46, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
>
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Hi,
>
> This series fixes a couple of bugs we've ran into with some
> proprietary test code and drivers using the Xilinx zDMA.

Thanks; as these are bugfixes I've applied them to target-arm.next
for 5.0. I'd appreciate it if you could take a look at the
endianness bugs I mention in a reply to patch 5 (probably
that is 5.1 work).

-- PMM

Re: [PATCH v1 0/5] dma/xlnx-zdma: Bug fixes

[Edgar E. Iglesias]

On Fri, Apr 03, 2020 at 07:53:24PM +0100, Peter Maydell wrote:
> On Thu, 2 Apr 2020 at 14:46, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> >
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> > Hi,
> >
> > This series fixes a couple of bugs we've ran into with some
> > proprietary test code and drivers using the Xilinx zDMA.
> 
> Thanks; as these are bugfixes I've applied them to target-arm.next
> for 5.0. I'd appreciate it if you could take a look at the
> endianness bugs I mention in a reply to patch 5 (probably
> that is 5.1 work).


Thanks for pointing those out!

I've just posted fixes for the endianness related descriptor loading
bugs but left some of the C struct issues for 5.1.

Cheers,
Edgar