All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] Align SiFive PDMA behavior to real hardware
@ 2021-12-28  0:52 Jim Shu
  2021-12-28  0:52 ` [PATCH 1/2] hw/dma: sifive_pdma: support high 32-bit access of 64-bit register Jim Shu
  2021-12-28  0:52 ` [PATCH 2/2] hw/dma: sifive_pdma: permit 4/8-byte access size of PDMA registers Jim Shu
  0 siblings, 2 replies; 15+ messages in thread
From: Jim Shu @ 2021-12-28  0:52 UTC (permalink / raw)
  To: Alistair.Francis, bin.meng, palmer, frank.chang, qemu-riscv, qemu-devel
  Cc: Jim Shu

HiFive Unmatched PDMA supports high/low 32-bit access of 64-bit
register, but QEMU emulation support low part access now. Enhance QEMU
emulation to support high 32-bit access. 

Also, permit 4/8-byte valid access in PDMA as we have verified 32/64-bit
accesses of PDMA registers are supported.

Jim Shu (2):
  hw/dma: sifive_pdma: support high 32-bit access of 64-bit register
  hw/dma: sifive_pdma: permit 4/8-byte access size of PDMA registers

 hw/dma/sifive_pdma.c | 178 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 156 insertions(+), 22 deletions(-)

-- 
2.25.1



^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/2] hw/dma: sifive_pdma: support high 32-bit access of 64-bit register
  2021-12-28  0:52 [PATCH 0/2] Align SiFive PDMA behavior to real hardware Jim Shu
@ 2021-12-28  0:52 ` Jim Shu
  2021-12-28 10:33   ` Philippe Mathieu-Daudé
                     ` (2 more replies)
  2021-12-28  0:52 ` [PATCH 2/2] hw/dma: sifive_pdma: permit 4/8-byte access size of PDMA registers Jim Shu
  1 sibling, 3 replies; 15+ messages in thread
From: Jim Shu @ 2021-12-28  0:52 UTC (permalink / raw)
  To: Alistair.Francis, bin.meng, palmer, frank.chang, qemu-riscv, qemu-devel
  Cc: Jim Shu

Real PDMA support high 32-bit read/write memory access of 64-bit
register.

The following result is PDMA tested in U-Boot on Unmatched board:

1. Real PDMA is allowed high 32-bit read/write to 64-bit register.
=> mw.l 0x3000000 0x0                      <= Disclaim channel 0
=> mw.l 0x3000000 0x1                      <= Claim channel 0
=> mw.l 0x3000010 0x80000000               <= Write low 32-bit NextDest (NextDest = 0x280000000)
=> mw.l 0x3000014 0x2                      <= Write high 32-bit NextDest
=> md.l 0x3000010 1                        <= Dump low 32-bit NextDest
03000010: 80000000
=> md.l 0x3000014 1                        <= Dump high 32-bit NextDest
03000014: 00000002
=> mw.l 0x3000018 0x80001000               <= Write low 32-bit NextSrc (NextSrc = 0x280001000)
=> mw.l 0x300001c 0x2                      <= Write high 32-bit NextSrc
=> md.l 0x3000018 1                        <= Dump low 32-bit NextSrc
03000010: 80001000
=> md.l 0x300001c 1                        <= Dump high 32-bit NextSrc
03000014: 00000002

2. PDMA transfer from 0x280001000 to 0x280000000 is OK.
=> mw.q 0x3000008 0x4                      <= NextBytes = 4
=> mw.l 0x3000004 0x22000000               <= wsize = rsize = 2 (2^2 = 4 bytes)
=> mw.l 0x280000000 0x87654321             <= Fill test data to dst
=> mw.l 0x280001000 0x12345678             <= Fill test data to src
=> md.l 0x280000000 1; md.l 0x280001000 1  <= Dump src/dst memory contents
280000000: 87654321                              !Ce.
280001000: 12345678                              xV4.
=> md.l 0x3000000 8                        <= Dump PDMA status
03000000: 00000001 22000000 00000004 00000000    ......."........
03000010: 80000000 00000002 80001000 00000002    ................
=> mw.l 0x3000000 0x3                      <= Set channel 0 run and claim bits
=> md.l 0x3000000 8                        <= Dump PDMA status
03000000: 40000001 22000000 00000004 00000000    ...@..."........
03000010: 80000000 00000002 80001000 00000002    ................
=> md.l 0x280000000 1; md.l 0x280001000 1  <= Dump src/dst memory contents
280000000: 12345678                               xV4.
280001000: 12345678                               xV4.

Signed-off-by: Jim Shu <jim.shu@sifive.com>
Reviewed-by: Frank Chang <frank.chang@sifive.com>
---
 hw/dma/sifive_pdma.c | 174 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 152 insertions(+), 22 deletions(-)

diff --git a/hw/dma/sifive_pdma.c b/hw/dma/sifive_pdma.c
index 85fe34f5f3..b8b198ab4e 100644
--- a/hw/dma/sifive_pdma.c
+++ b/hw/dma/sifive_pdma.c
@@ -177,18 +177,44 @@ static inline void sifive_pdma_update_irq(SiFivePDMAState *s, int ch)
     s->chan[ch].state = DMA_CHAN_STATE_IDLE;
 }
 
-static uint64_t sifive_pdma_read(void *opaque, hwaddr offset, unsigned size)
+static uint64_t sifive_pdma_readq(SiFivePDMAState *s, int ch, hwaddr offset)
 {
-    SiFivePDMAState *s = opaque;
-    int ch = SIFIVE_PDMA_CHAN_NO(offset);
     uint64_t val = 0;
 
-    if (ch >= SIFIVE_PDMA_CHANS) {
-        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid channel no %d\n",
-                      __func__, ch);
-        return 0;
+    offset &= 0xfff;
+    switch (offset) {
+    case DMA_NEXT_BYTES:
+        val = s->chan[ch].next_bytes;
+        break;
+    case DMA_NEXT_DST:
+        val = s->chan[ch].next_dst;
+        break;
+    case DMA_NEXT_SRC:
+        val = s->chan[ch].next_src;
+        break;
+    case DMA_EXEC_BYTES:
+        val = s->chan[ch].exec_bytes;
+        break;
+    case DMA_EXEC_DST:
+        val = s->chan[ch].exec_dst;
+        break;
+    case DMA_EXEC_SRC:
+        val = s->chan[ch].exec_src;
+        break;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Unexpected 64-bit access to 0x%" HWADDR_PRIX "\n",
+                      __func__, offset);
+        break;
     }
 
+    return val;
+}
+
+static uint32_t sifive_pdma_readl(SiFivePDMAState *s, int ch, hwaddr offset)
+{
+    uint32_t val = 0;
+
     offset &= 0xfff;
     switch (offset) {
     case DMA_CONTROL:
@@ -198,28 +224,47 @@ static uint64_t sifive_pdma_read(void *opaque, hwaddr offset, unsigned size)
         val = s->chan[ch].next_config;
         break;
     case DMA_NEXT_BYTES:
-        val = s->chan[ch].next_bytes;
+        val = extract64(s->chan[ch].next_bytes, 0, 32);
+        break;
+    case DMA_NEXT_BYTES + 4:
+        val = extract64(s->chan[ch].next_bytes, 32, 32);
         break;
     case DMA_NEXT_DST:
-        val = s->chan[ch].next_dst;
+        val = extract64(s->chan[ch].next_dst, 0, 32);
+        break;
+    case DMA_NEXT_DST + 4:
+        val = extract64(s->chan[ch].next_dst, 32, 32);
         break;
     case DMA_NEXT_SRC:
-        val = s->chan[ch].next_src;
+        val = extract64(s->chan[ch].next_src, 0, 32);
+        break;
+    case DMA_NEXT_SRC + 4:
+        val = extract64(s->chan[ch].next_src, 32, 32);
         break;
     case DMA_EXEC_CONFIG:
         val = s->chan[ch].exec_config;
         break;
     case DMA_EXEC_BYTES:
-        val = s->chan[ch].exec_bytes;
+        val = extract64(s->chan[ch].exec_bytes, 0, 32);
+        break;
+    case DMA_EXEC_BYTES + 4:
+        val = extract64(s->chan[ch].exec_bytes, 32, 32);
         break;
     case DMA_EXEC_DST:
-        val = s->chan[ch].exec_dst;
+        val = extract64(s->chan[ch].exec_dst, 0, 32);
+        break;
+    case DMA_EXEC_DST + 4:
+        val = extract64(s->chan[ch].exec_dst, 32, 32);
         break;
     case DMA_EXEC_SRC:
-        val = s->chan[ch].exec_src;
+        val = extract64(s->chan[ch].exec_src, 0, 32);
+        break;
+    case DMA_EXEC_SRC + 4:
+        val = extract64(s->chan[ch].exec_src, 32, 32);
         break;
     default:
-        qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIX "\n",
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Unexpected 32-bit access to 0x%" HWADDR_PRIX "\n",
                       __func__, offset);
         break;
     }
@@ -227,19 +272,66 @@ static uint64_t sifive_pdma_read(void *opaque, hwaddr offset, unsigned size)
     return val;
 }
 
-static void sifive_pdma_write(void *opaque, hwaddr offset,
-                              uint64_t value, unsigned size)
+static uint64_t sifive_pdma_read(void *opaque, hwaddr offset, unsigned size)
 {
     SiFivePDMAState *s = opaque;
     int ch = SIFIVE_PDMA_CHAN_NO(offset);
-    bool claimed, run;
+    uint64_t val = 0;
 
     if (ch >= SIFIVE_PDMA_CHANS) {
         qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid channel no %d\n",
                       __func__, ch);
-        return;
+        return 0;
+    }
+
+    switch (size) {
+    case 8:
+        val = sifive_pdma_readq(s, ch, offset);
+        break;
+    case 4:
+        val = sifive_pdma_readl(s, ch, offset);
+        break;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid read size %u to PDMA\n",
+                      __func__, size);
+        return 0;
     }
 
+    return val;
+}
+
+static void sifive_pdma_writeq(SiFivePDMAState *s, int ch,
+                               hwaddr offset, uint64_t value)
+{
+    offset &= 0xfff;
+    switch (offset) {
+    case DMA_NEXT_BYTES:
+        s->chan[ch].next_bytes = value;
+        break;
+    case DMA_NEXT_DST:
+        s->chan[ch].next_dst = value;
+        break;
+    case DMA_NEXT_SRC:
+        s->chan[ch].next_src = value;
+        break;
+    case DMA_EXEC_BYTES:
+    case DMA_EXEC_DST:
+    case DMA_EXEC_SRC:
+        /* these are read-only registers */
+        break;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Unexpected 64-bit access to 0x%" HWADDR_PRIX "\n",
+                      __func__, offset);
+        break;
+    }
+}
+
+static void sifive_pdma_writel(SiFivePDMAState *s, int ch,
+                               hwaddr offset, uint32_t value)
+{
+    bool claimed, run;
+
     offset &= 0xfff;
     switch (offset) {
     case DMA_CONTROL:
@@ -282,13 +374,24 @@ static void sifive_pdma_write(void *opaque, hwaddr offset,
         s->chan[ch].next_config = value;
         break;
     case DMA_NEXT_BYTES:
-        s->chan[ch].next_bytes = value;
+        s->chan[ch].next_bytes =
+            deposit64(s->chan[ch].next_bytes, 0, 32, value);
+        break;
+    case DMA_NEXT_BYTES + 4:
+        s->chan[ch].next_bytes =
+            deposit64(s->chan[ch].next_bytes, 32, 32, value);
         break;
     case DMA_NEXT_DST:
-        s->chan[ch].next_dst = value;
+        s->chan[ch].next_dst = deposit64(s->chan[ch].next_dst, 0, 32, value);
+        break;
+    case DMA_NEXT_DST + 4:
+        s->chan[ch].next_dst = deposit64(s->chan[ch].next_dst, 32, 32, value);
         break;
     case DMA_NEXT_SRC:
-        s->chan[ch].next_src = value;
+        s->chan[ch].next_src = deposit64(s->chan[ch].next_src, 0, 32, value);
+        break;
+    case DMA_NEXT_SRC + 4:
+        s->chan[ch].next_src = deposit64(s->chan[ch].next_src, 32, 32, value);
         break;
     case DMA_EXEC_CONFIG:
     case DMA_EXEC_BYTES:
@@ -297,12 +400,39 @@ static void sifive_pdma_write(void *opaque, hwaddr offset,
         /* these are read-only registers */
         break;
     default:
-        qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIX "\n",
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Unexpected 32-bit access to 0x%" HWADDR_PRIX "\n",
                       __func__, offset);
         break;
     }
 }
 
+static void sifive_pdma_write(void *opaque, hwaddr offset,
+                              uint64_t value, unsigned size)
+{
+    SiFivePDMAState *s = opaque;
+    int ch = SIFIVE_PDMA_CHAN_NO(offset);
+
+    if (ch >= SIFIVE_PDMA_CHANS) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid channel no %d\n",
+                      __func__, ch);
+        return;
+    }
+
+    switch (size) {
+    case 8:
+        sifive_pdma_writeq(s, ch, offset, value);
+        break;
+    case 4:
+        sifive_pdma_writel(s, ch, offset, (uint32_t) value);
+        break;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid write size %u to PDMA\n",
+                      __func__, size);
+        break;
+    }
+}
+
 static const MemoryRegionOps sifive_pdma_ops = {
     .read = sifive_pdma_read,
     .write = sifive_pdma_write,
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 2/2] hw/dma: sifive_pdma: permit 4/8-byte access size of PDMA registers
  2021-12-28  0:52 [PATCH 0/2] Align SiFive PDMA behavior to real hardware Jim Shu
  2021-12-28  0:52 ` [PATCH 1/2] hw/dma: sifive_pdma: support high 32-bit access of 64-bit register Jim Shu
@ 2021-12-28  0:52 ` Jim Shu
  2021-12-28 10:35   ` Philippe Mathieu-Daudé
                     ` (2 more replies)
  1 sibling, 3 replies; 15+ messages in thread
From: Jim Shu @ 2021-12-28  0:52 UTC (permalink / raw)
  To: Alistair.Francis, bin.meng, palmer, frank.chang, qemu-riscv, qemu-devel
  Cc: Jim Shu

It's obvious that PDMA support 64-bit access of 64-bit registers, and
in previous commit, we confirm that PDMA support 32-bit access of both
32/64-bit registers. Thus, we configure 32/64-bit memory access of
PDMA registers as valid in general.

Signed-off-by: Jim Shu <jim.shu@sifive.com>
Reviewed-by: Frank Chang <frank.chang@sifive.com>
---
 hw/dma/sifive_pdma.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/dma/sifive_pdma.c b/hw/dma/sifive_pdma.c
index b8b198ab4e..731fcdcf89 100644
--- a/hw/dma/sifive_pdma.c
+++ b/hw/dma/sifive_pdma.c
@@ -441,6 +441,10 @@ static const MemoryRegionOps sifive_pdma_ops = {
     .impl = {
         .min_access_size = 4,
         .max_access_size = 8,
+    },
+    .valid = {
+        .min_access_size = 4,
+        .max_access_size = 8,
     }
 };
 
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] hw/dma: sifive_pdma: support high 32-bit access of 64-bit register
  2021-12-28  0:52 ` [PATCH 1/2] hw/dma: sifive_pdma: support high 32-bit access of 64-bit register Jim Shu
@ 2021-12-28 10:33   ` Philippe Mathieu-Daudé
  2022-01-03 22:45     ` Alistair Francis
  2022-01-04  2:55     ` Bin Meng
  2 siblings, 0 replies; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-12-28 10:33 UTC (permalink / raw)
  To: Jim Shu, Alistair.Francis, bin.meng, palmer, frank.chang,
	qemu-riscv, qemu-devel

Hi Jim and Frank,

On 12/28/21 01:52, Jim Shu wrote:
> Real PDMA support high 32-bit read/write memory access of 64-bit
> register.
> 
> The following result is PDMA tested in U-Boot on Unmatched board:
> 
> 1. Real PDMA is allowed high 32-bit read/write to 64-bit register.
> => mw.l 0x3000000 0x0                      <= Disclaim channel 0
> => mw.l 0x3000000 0x1                      <= Claim channel 0
> => mw.l 0x3000010 0x80000000               <= Write low 32-bit NextDest (NextDest = 0x280000000)
> => mw.l 0x3000014 0x2                      <= Write high 32-bit NextDest
> => md.l 0x3000010 1                        <= Dump low 32-bit NextDest
> 03000010: 80000000
> => md.l 0x3000014 1                        <= Dump high 32-bit NextDest
> 03000014: 00000002
> => mw.l 0x3000018 0x80001000               <= Write low 32-bit NextSrc (NextSrc = 0x280001000)
> => mw.l 0x300001c 0x2                      <= Write high 32-bit NextSrc
> => md.l 0x3000018 1                        <= Dump low 32-bit NextSrc
> 03000010: 80001000
> => md.l 0x300001c 1                        <= Dump high 32-bit NextSrc
> 03000014: 00000002
> 
> 2. PDMA transfer from 0x280001000 to 0x280000000 is OK.
> => mw.q 0x3000008 0x4                      <= NextBytes = 4
> => mw.l 0x3000004 0x22000000               <= wsize = rsize = 2 (2^2 = 4 bytes)
> => mw.l 0x280000000 0x87654321             <= Fill test data to dst
> => mw.l 0x280001000 0x12345678             <= Fill test data to src
> => md.l 0x280000000 1; md.l 0x280001000 1  <= Dump src/dst memory contents
> 280000000: 87654321                              !Ce.
> 280001000: 12345678                              xV4.
> => md.l 0x3000000 8                        <= Dump PDMA status
> 03000000: 00000001 22000000 00000004 00000000    ......."........
> 03000010: 80000000 00000002 80001000 00000002    ................
> => mw.l 0x3000000 0x3                      <= Set channel 0 run and claim bits
> => md.l 0x3000000 8                        <= Dump PDMA status
> 03000000: 40000001 22000000 00000004 00000000    ...@..."........
> 03000010: 80000000 00000002 80001000 00000002    ................
> => md.l 0x280000000 1; md.l 0x280001000 1  <= Dump src/dst memory contents
> 280000000: 12345678                               xV4.
> 280001000: 12345678                               xV4.
> 
> Signed-off-by: Jim Shu <jim.shu@sifive.com>
> Reviewed-by: Frank Chang <frank.chang@sifive.com>
> ---
>  hw/dma/sifive_pdma.c | 174 +++++++++++++++++++++++++++++++++++++------
>  1 file changed, 152 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/dma/sifive_pdma.c b/hw/dma/sifive_pdma.c
> index 85fe34f5f3..b8b198ab4e 100644
> --- a/hw/dma/sifive_pdma.c
> +++ b/hw/dma/sifive_pdma.c
> @@ -177,18 +177,44 @@ static inline void sifive_pdma_update_irq(SiFivePDMAState *s, int ch)
>      s->chan[ch].state = DMA_CHAN_STATE_IDLE;
>  }
>  
> -static uint64_t sifive_pdma_read(void *opaque, hwaddr offset, unsigned size)
> +static uint64_t sifive_pdma_readq(SiFivePDMAState *s, int ch, hwaddr offset)
>  {
> -    SiFivePDMAState *s = opaque;
> -    int ch = SIFIVE_PDMA_CHAN_NO(offset);
>      uint64_t val = 0;
>  
> -    if (ch >= SIFIVE_PDMA_CHANS) {
> -        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid channel no %d\n",
> -                      __func__, ch);
> -        return 0;
> +    offset &= 0xfff;
> +    switch (offset) {
> +    case DMA_NEXT_BYTES:
> +        val = s->chan[ch].next_bytes;
> +        break;
> +    case DMA_NEXT_DST:
> +        val = s->chan[ch].next_dst;
> +        break;
> +    case DMA_NEXT_SRC:
> +        val = s->chan[ch].next_src;
> +        break;
> +    case DMA_EXEC_BYTES:
> +        val = s->chan[ch].exec_bytes;
> +        break;
> +    case DMA_EXEC_DST:
> +        val = s->chan[ch].exec_dst;
> +        break;
> +    case DMA_EXEC_SRC:
> +        val = s->chan[ch].exec_src;
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Unexpected 64-bit access to 0x%" HWADDR_PRIX "\n",
> +                      __func__, offset);
> +        break;
>      }
>  
> +    return val;
> +}
> +
> +static uint32_t sifive_pdma_readl(SiFivePDMAState *s, int ch, hwaddr offset)
> +{
> +    uint32_t val = 0;
> +
>      offset &= 0xfff;
>      switch (offset) {
>      case DMA_CONTROL:
> @@ -198,28 +224,47 @@ static uint64_t sifive_pdma_read(void *opaque, hwaddr offset, unsigned size)
>          val = s->chan[ch].next_config;
>          break;
>      case DMA_NEXT_BYTES:
> -        val = s->chan[ch].next_bytes;
> +        val = extract64(s->chan[ch].next_bytes, 0, 32);
> +        break;
> +    case DMA_NEXT_BYTES + 4:
> +        val = extract64(s->chan[ch].next_bytes, 32, 32);
>          break;
>      case DMA_NEXT_DST:
> -        val = s->chan[ch].next_dst;
> +        val = extract64(s->chan[ch].next_dst, 0, 32);
> +        break;
> +    case DMA_NEXT_DST + 4:
> +        val = extract64(s->chan[ch].next_dst, 32, 32);
>          break;
>      case DMA_NEXT_SRC:
> -        val = s->chan[ch].next_src;
> +        val = extract64(s->chan[ch].next_src, 0, 32);
> +        break;
> +    case DMA_NEXT_SRC + 4:
> +        val = extract64(s->chan[ch].next_src, 32, 32);
>          break;
>      case DMA_EXEC_CONFIG:
>          val = s->chan[ch].exec_config;
>          break;
>      case DMA_EXEC_BYTES:
> -        val = s->chan[ch].exec_bytes;
> +        val = extract64(s->chan[ch].exec_bytes, 0, 32);
> +        break;
> +    case DMA_EXEC_BYTES + 4:
> +        val = extract64(s->chan[ch].exec_bytes, 32, 32);
>          break;
>      case DMA_EXEC_DST:
> -        val = s->chan[ch].exec_dst;
> +        val = extract64(s->chan[ch].exec_dst, 0, 32);
> +        break;
> +    case DMA_EXEC_DST + 4:
> +        val = extract64(s->chan[ch].exec_dst, 32, 32);
>          break;
>      case DMA_EXEC_SRC:
> -        val = s->chan[ch].exec_src;
> +        val = extract64(s->chan[ch].exec_src, 0, 32);
> +        break;
> +    case DMA_EXEC_SRC + 4:
> +        val = extract64(s->chan[ch].exec_src, 32, 32);
>          break;
>      default:
> -        qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIX "\n",
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Unexpected 32-bit access to 0x%" HWADDR_PRIX "\n",
>                        __func__, offset);
>          break;
>      }
> @@ -227,19 +272,66 @@ static uint64_t sifive_pdma_read(void *opaque, hwaddr offset, unsigned size)
>      return val;
>  }
>  
> -static void sifive_pdma_write(void *opaque, hwaddr offset,
> -                              uint64_t value, unsigned size)
> +static uint64_t sifive_pdma_read(void *opaque, hwaddr offset, unsigned size)
>  {
>      SiFivePDMAState *s = opaque;
>      int ch = SIFIVE_PDMA_CHAN_NO(offset);
> -    bool claimed, run;
> +    uint64_t val = 0;
>  
>      if (ch >= SIFIVE_PDMA_CHANS) {
>          qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid channel no %d\n",
>                        __func__, ch);
> -        return;
> +        return 0;
> +    }
> +
> +    switch (size) {
> +    case 8:
> +        val = sifive_pdma_readq(s, ch, offset);
> +        break;
> +    case 4:
> +        val = sifive_pdma_readl(s, ch, offset);
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid read size %u to PDMA\n",
> +                      __func__, size);
> +        return 0;
>      }
>  
> +    return val;
> +}
> +
> +static void sifive_pdma_writeq(SiFivePDMAState *s, int ch,
> +                               hwaddr offset, uint64_t value)
> +{
> +    offset &= 0xfff;
> +    switch (offset) {
> +    case DMA_NEXT_BYTES:
> +        s->chan[ch].next_bytes = value;
> +        break;
> +    case DMA_NEXT_DST:
> +        s->chan[ch].next_dst = value;
> +        break;
> +    case DMA_NEXT_SRC:
> +        s->chan[ch].next_src = value;
> +        break;
> +    case DMA_EXEC_BYTES:
> +    case DMA_EXEC_DST:
> +    case DMA_EXEC_SRC:
> +        /* these are read-only registers */
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Unexpected 64-bit access to 0x%" HWADDR_PRIX "\n",
> +                      __func__, offset);
> +        break;
> +    }
> +}
> +
> +static void sifive_pdma_writel(SiFivePDMAState *s, int ch,
> +                               hwaddr offset, uint32_t value)
> +{
> +    bool claimed, run;
> +
>      offset &= 0xfff;
>      switch (offset) {
>      case DMA_CONTROL:
> @@ -282,13 +374,24 @@ static void sifive_pdma_write(void *opaque, hwaddr offset,
>          s->chan[ch].next_config = value;
>          break;
>      case DMA_NEXT_BYTES:
> -        s->chan[ch].next_bytes = value;
> +        s->chan[ch].next_bytes =
> +            deposit64(s->chan[ch].next_bytes, 0, 32, value);
> +        break;
> +    case DMA_NEXT_BYTES + 4:
> +        s->chan[ch].next_bytes =
> +            deposit64(s->chan[ch].next_bytes, 32, 32, value);
>          break;
>      case DMA_NEXT_DST:
> -        s->chan[ch].next_dst = value;
> +        s->chan[ch].next_dst = deposit64(s->chan[ch].next_dst, 0, 32, value);
> +        break;
> +    case DMA_NEXT_DST + 4:
> +        s->chan[ch].next_dst = deposit64(s->chan[ch].next_dst, 32, 32, value);
>          break;
>      case DMA_NEXT_SRC:
> -        s->chan[ch].next_src = value;
> +        s->chan[ch].next_src = deposit64(s->chan[ch].next_src, 0, 32, value);
> +        break;
> +    case DMA_NEXT_SRC + 4:
> +        s->chan[ch].next_src = deposit64(s->chan[ch].next_src, 32, 32, value);
>          break;
>      case DMA_EXEC_CONFIG:
>      case DMA_EXEC_BYTES:
> @@ -297,12 +400,39 @@ static void sifive_pdma_write(void *opaque, hwaddr offset,
>          /* these are read-only registers */
>          break;
>      default:
> -        qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIX "\n",
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Unexpected 32-bit access to 0x%" HWADDR_PRIX "\n",
>                        __func__, offset);
>          break;
>      }
>  }
>  
> +static void sifive_pdma_write(void *opaque, hwaddr offset,
> +                              uint64_t value, unsigned size)
> +{
> +    SiFivePDMAState *s = opaque;
> +    int ch = SIFIVE_PDMA_CHAN_NO(offset);
> +
> +    if (ch >= SIFIVE_PDMA_CHANS) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid channel no %d\n",
> +                      __func__, ch);
> +        return;
> +    }
> +
> +    switch (size) {
> +    case 8:
> +        sifive_pdma_writeq(s, ch, offset, value);
> +        break;
> +    case 4:
> +        sifive_pdma_writel(s, ch, offset, (uint32_t) value);
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid write size %u to PDMA\n",
> +                      __func__, size);
> +        break;
> +    }
> +}
> +
>  static const MemoryRegionOps sifive_pdma_ops = {
>      .read = sifive_pdma_read,
>      .write = sifive_pdma_write,

Would this work instead?

-- >8 --
diff --git a/hw/dma/sifive_pdma.c b/hw/dma/sifive_pdma.c
index 85fe34f5f31..8f879de6f60 100644
--- a/hw/dma/sifive_pdma.c
+++ b/hw/dma/sifive_pdma.c
@@ -308,10 +308,14 @@ static const MemoryRegionOps sifive_pdma_ops = {
     .write = sifive_pdma_write,
     .endianness = DEVICE_LITTLE_ENDIAN,
     /* there are 32-bit and 64-bit wide registers */
-    .impl = {
+    .valid = {
         .min_access_size = 4,
         .max_access_size = 8,
     }
+    .impl = {
+        .min_access_size = 8,
+        .max_access_size = 8,
+    }
 };

---



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] hw/dma: sifive_pdma: permit 4/8-byte access size of PDMA registers
  2021-12-28  0:52 ` [PATCH 2/2] hw/dma: sifive_pdma: permit 4/8-byte access size of PDMA registers Jim Shu
@ 2021-12-28 10:35   ` Philippe Mathieu-Daudé
  2022-01-03 22:46     ` Alistair Francis
  2022-01-04  2:56     ` Bin Meng
  2 siblings, 0 replies; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-12-28 10:35 UTC (permalink / raw)
  To: Jim Shu, Alistair.Francis, bin.meng, palmer, frank.chang,
	qemu-riscv, qemu-devel

On 12/28/21 01:52, Jim Shu wrote:
> It's obvious that PDMA support 64-bit access of 64-bit registers, and
> in previous commit, we confirm that PDMA support 32-bit access of both
> 32/64-bit registers. Thus, we configure 32/64-bit memory access of
> PDMA registers as valid in general.
> 
> Signed-off-by: Jim Shu <jim.shu@sifive.com>
> Reviewed-by: Frank Chang <frank.chang@sifive.com>
> ---
>  hw/dma/sifive_pdma.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/dma/sifive_pdma.c b/hw/dma/sifive_pdma.c
> index b8b198ab4e..731fcdcf89 100644
> --- a/hw/dma/sifive_pdma.c
> +++ b/hw/dma/sifive_pdma.c
> @@ -441,6 +441,10 @@ static const MemoryRegionOps sifive_pdma_ops = {
>      .impl = {
>          .min_access_size = 4,
>          .max_access_size = 8,
> +    },
> +    .valid = {
> +        .min_access_size = 4,
> +        .max_access_size = 8,
>      }
>  };
>  

Eh now I see this patch. It is correct, so:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

But I think the previous patch could be simpler.



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] hw/dma: sifive_pdma: support high 32-bit access of 64-bit register
  2021-12-28  0:52 ` [PATCH 1/2] hw/dma: sifive_pdma: support high 32-bit access of 64-bit register Jim Shu
@ 2022-01-03 22:45     ` Alistair Francis
  2022-01-03 22:45     ` Alistair Francis
  2022-01-04  2:55     ` Bin Meng
  2 siblings, 0 replies; 15+ messages in thread
From: Alistair Francis @ 2022-01-03 22:45 UTC (permalink / raw)
  To: Jim Shu
  Cc: open list:RISC-V, Frank Chang, Bin Meng,
	qemu-devel@nongnu.org Developers, Palmer Dabbelt,
	Alistair Francis

On Tue, Dec 28, 2021 at 10:54 AM Jim Shu <jim.shu@sifive.com> wrote:
>
> Real PDMA support high 32-bit read/write memory access of 64-bit
> register.
>
> The following result is PDMA tested in U-Boot on Unmatched board:
>
> 1. Real PDMA is allowed high 32-bit read/write to 64-bit register.
> => mw.l 0x3000000 0x0                      <= Disclaim channel 0
> => mw.l 0x3000000 0x1                      <= Claim channel 0
> => mw.l 0x3000010 0x80000000               <= Write low 32-bit NextDest (NextDest = 0x280000000)
> => mw.l 0x3000014 0x2                      <= Write high 32-bit NextDest
> => md.l 0x3000010 1                        <= Dump low 32-bit NextDest
> 03000010: 80000000
> => md.l 0x3000014 1                        <= Dump high 32-bit NextDest
> 03000014: 00000002
> => mw.l 0x3000018 0x80001000               <= Write low 32-bit NextSrc (NextSrc = 0x280001000)
> => mw.l 0x300001c 0x2                      <= Write high 32-bit NextSrc
> => md.l 0x3000018 1                        <= Dump low 32-bit NextSrc
> 03000010: 80001000
> => md.l 0x300001c 1                        <= Dump high 32-bit NextSrc
> 03000014: 00000002
>
> 2. PDMA transfer from 0x280001000 to 0x280000000 is OK.
> => mw.q 0x3000008 0x4                      <= NextBytes = 4
> => mw.l 0x3000004 0x22000000               <= wsize = rsize = 2 (2^2 = 4 bytes)
> => mw.l 0x280000000 0x87654321             <= Fill test data to dst
> => mw.l 0x280001000 0x12345678             <= Fill test data to src
> => md.l 0x280000000 1; md.l 0x280001000 1  <= Dump src/dst memory contents
> 280000000: 87654321                              !Ce.
> 280001000: 12345678                              xV4.
> => md.l 0x3000000 8                        <= Dump PDMA status
> 03000000: 00000001 22000000 00000004 00000000    ......."........
> 03000010: 80000000 00000002 80001000 00000002    ................
> => mw.l 0x3000000 0x3                      <= Set channel 0 run and claim bits
> => md.l 0x3000000 8                        <= Dump PDMA status
> 03000000: 40000001 22000000 00000004 00000000    ...@..."........
> 03000010: 80000000 00000002 80001000 00000002    ................
> => md.l 0x280000000 1; md.l 0x280001000 1  <= Dump src/dst memory contents
> 280000000: 12345678                               xV4.
> 280001000: 12345678                               xV4.
>
> Signed-off-by: Jim Shu <jim.shu@sifive.com>
> Reviewed-by: Frank Chang <frank.chang@sifive.com>

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

Alistair

> ---
>  hw/dma/sifive_pdma.c | 174 +++++++++++++++++++++++++++++++++++++------
>  1 file changed, 152 insertions(+), 22 deletions(-)
>
> diff --git a/hw/dma/sifive_pdma.c b/hw/dma/sifive_pdma.c
> index 85fe34f5f3..b8b198ab4e 100644
> --- a/hw/dma/sifive_pdma.c
> +++ b/hw/dma/sifive_pdma.c
> @@ -177,18 +177,44 @@ static inline void sifive_pdma_update_irq(SiFivePDMAState *s, int ch)
>      s->chan[ch].state = DMA_CHAN_STATE_IDLE;
>  }
>
> -static uint64_t sifive_pdma_read(void *opaque, hwaddr offset, unsigned size)
> +static uint64_t sifive_pdma_readq(SiFivePDMAState *s, int ch, hwaddr offset)
>  {
> -    SiFivePDMAState *s = opaque;
> -    int ch = SIFIVE_PDMA_CHAN_NO(offset);
>      uint64_t val = 0;
>
> -    if (ch >= SIFIVE_PDMA_CHANS) {
> -        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid channel no %d\n",
> -                      __func__, ch);
> -        return 0;
> +    offset &= 0xfff;
> +    switch (offset) {
> +    case DMA_NEXT_BYTES:
> +        val = s->chan[ch].next_bytes;
> +        break;
> +    case DMA_NEXT_DST:
> +        val = s->chan[ch].next_dst;
> +        break;
> +    case DMA_NEXT_SRC:
> +        val = s->chan[ch].next_src;
> +        break;
> +    case DMA_EXEC_BYTES:
> +        val = s->chan[ch].exec_bytes;
> +        break;
> +    case DMA_EXEC_DST:
> +        val = s->chan[ch].exec_dst;
> +        break;
> +    case DMA_EXEC_SRC:
> +        val = s->chan[ch].exec_src;
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Unexpected 64-bit access to 0x%" HWADDR_PRIX "\n",
> +                      __func__, offset);
> +        break;
>      }
>
> +    return val;
> +}
> +
> +static uint32_t sifive_pdma_readl(SiFivePDMAState *s, int ch, hwaddr offset)
> +{
> +    uint32_t val = 0;
> +
>      offset &= 0xfff;
>      switch (offset) {
>      case DMA_CONTROL:
> @@ -198,28 +224,47 @@ static uint64_t sifive_pdma_read(void *opaque, hwaddr offset, unsigned size)
>          val = s->chan[ch].next_config;
>          break;
>      case DMA_NEXT_BYTES:
> -        val = s->chan[ch].next_bytes;
> +        val = extract64(s->chan[ch].next_bytes, 0, 32);
> +        break;
> +    case DMA_NEXT_BYTES + 4:
> +        val = extract64(s->chan[ch].next_bytes, 32, 32);
>          break;
>      case DMA_NEXT_DST:
> -        val = s->chan[ch].next_dst;
> +        val = extract64(s->chan[ch].next_dst, 0, 32);
> +        break;
> +    case DMA_NEXT_DST + 4:
> +        val = extract64(s->chan[ch].next_dst, 32, 32);
>          break;
>      case DMA_NEXT_SRC:
> -        val = s->chan[ch].next_src;
> +        val = extract64(s->chan[ch].next_src, 0, 32);
> +        break;
> +    case DMA_NEXT_SRC + 4:
> +        val = extract64(s->chan[ch].next_src, 32, 32);
>          break;
>      case DMA_EXEC_CONFIG:
>          val = s->chan[ch].exec_config;
>          break;
>      case DMA_EXEC_BYTES:
> -        val = s->chan[ch].exec_bytes;
> +        val = extract64(s->chan[ch].exec_bytes, 0, 32);
> +        break;
> +    case DMA_EXEC_BYTES + 4:
> +        val = extract64(s->chan[ch].exec_bytes, 32, 32);
>          break;
>      case DMA_EXEC_DST:
> -        val = s->chan[ch].exec_dst;
> +        val = extract64(s->chan[ch].exec_dst, 0, 32);
> +        break;
> +    case DMA_EXEC_DST + 4:
> +        val = extract64(s->chan[ch].exec_dst, 32, 32);
>          break;
>      case DMA_EXEC_SRC:
> -        val = s->chan[ch].exec_src;
> +        val = extract64(s->chan[ch].exec_src, 0, 32);
> +        break;
> +    case DMA_EXEC_SRC + 4:
> +        val = extract64(s->chan[ch].exec_src, 32, 32);
>          break;
>      default:
> -        qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIX "\n",
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Unexpected 32-bit access to 0x%" HWADDR_PRIX "\n",
>                        __func__, offset);
>          break;
>      }
> @@ -227,19 +272,66 @@ static uint64_t sifive_pdma_read(void *opaque, hwaddr offset, unsigned size)
>      return val;
>  }
>
> -static void sifive_pdma_write(void *opaque, hwaddr offset,
> -                              uint64_t value, unsigned size)
> +static uint64_t sifive_pdma_read(void *opaque, hwaddr offset, unsigned size)
>  {
>      SiFivePDMAState *s = opaque;
>      int ch = SIFIVE_PDMA_CHAN_NO(offset);
> -    bool claimed, run;
> +    uint64_t val = 0;
>
>      if (ch >= SIFIVE_PDMA_CHANS) {
>          qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid channel no %d\n",
>                        __func__, ch);
> -        return;
> +        return 0;
> +    }
> +
> +    switch (size) {
> +    case 8:
> +        val = sifive_pdma_readq(s, ch, offset);
> +        break;
> +    case 4:
> +        val = sifive_pdma_readl(s, ch, offset);
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid read size %u to PDMA\n",
> +                      __func__, size);
> +        return 0;
>      }
>
> +    return val;
> +}
> +
> +static void sifive_pdma_writeq(SiFivePDMAState *s, int ch,
> +                               hwaddr offset, uint64_t value)
> +{
> +    offset &= 0xfff;
> +    switch (offset) {
> +    case DMA_NEXT_BYTES:
> +        s->chan[ch].next_bytes = value;
> +        break;
> +    case DMA_NEXT_DST:
> +        s->chan[ch].next_dst = value;
> +        break;
> +    case DMA_NEXT_SRC:
> +        s->chan[ch].next_src = value;
> +        break;
> +    case DMA_EXEC_BYTES:
> +    case DMA_EXEC_DST:
> +    case DMA_EXEC_SRC:
> +        /* these are read-only registers */
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Unexpected 64-bit access to 0x%" HWADDR_PRIX "\n",
> +                      __func__, offset);
> +        break;
> +    }
> +}
> +
> +static void sifive_pdma_writel(SiFivePDMAState *s, int ch,
> +                               hwaddr offset, uint32_t value)
> +{
> +    bool claimed, run;
> +
>      offset &= 0xfff;
>      switch (offset) {
>      case DMA_CONTROL:
> @@ -282,13 +374,24 @@ static void sifive_pdma_write(void *opaque, hwaddr offset,
>          s->chan[ch].next_config = value;
>          break;
>      case DMA_NEXT_BYTES:
> -        s->chan[ch].next_bytes = value;
> +        s->chan[ch].next_bytes =
> +            deposit64(s->chan[ch].next_bytes, 0, 32, value);
> +        break;
> +    case DMA_NEXT_BYTES + 4:
> +        s->chan[ch].next_bytes =
> +            deposit64(s->chan[ch].next_bytes, 32, 32, value);
>          break;
>      case DMA_NEXT_DST:
> -        s->chan[ch].next_dst = value;
> +        s->chan[ch].next_dst = deposit64(s->chan[ch].next_dst, 0, 32, value);
> +        break;
> +    case DMA_NEXT_DST + 4:
> +        s->chan[ch].next_dst = deposit64(s->chan[ch].next_dst, 32, 32, value);
>          break;
>      case DMA_NEXT_SRC:
> -        s->chan[ch].next_src = value;
> +        s->chan[ch].next_src = deposit64(s->chan[ch].next_src, 0, 32, value);
> +        break;
> +    case DMA_NEXT_SRC + 4:
> +        s->chan[ch].next_src = deposit64(s->chan[ch].next_src, 32, 32, value);
>          break;
>      case DMA_EXEC_CONFIG:
>      case DMA_EXEC_BYTES:
> @@ -297,12 +400,39 @@ static void sifive_pdma_write(void *opaque, hwaddr offset,
>          /* these are read-only registers */
>          break;
>      default:
> -        qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIX "\n",
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Unexpected 32-bit access to 0x%" HWADDR_PRIX "\n",
>                        __func__, offset);
>          break;
>      }
>  }
>
> +static void sifive_pdma_write(void *opaque, hwaddr offset,
> +                              uint64_t value, unsigned size)
> +{
> +    SiFivePDMAState *s = opaque;
> +    int ch = SIFIVE_PDMA_CHAN_NO(offset);
> +
> +    if (ch >= SIFIVE_PDMA_CHANS) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid channel no %d\n",
> +                      __func__, ch);
> +        return;
> +    }
> +
> +    switch (size) {
> +    case 8:
> +        sifive_pdma_writeq(s, ch, offset, value);
> +        break;
> +    case 4:
> +        sifive_pdma_writel(s, ch, offset, (uint32_t) value);
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid write size %u to PDMA\n",
> +                      __func__, size);
> +        break;
> +    }
> +}
> +
>  static const MemoryRegionOps sifive_pdma_ops = {
>      .read = sifive_pdma_read,
>      .write = sifive_pdma_write,
> --
> 2.25.1
>
>


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] hw/dma: sifive_pdma: support high 32-bit access of 64-bit register
@ 2022-01-03 22:45     ` Alistair Francis
  0 siblings, 0 replies; 15+ messages in thread
From: Alistair Francis @ 2022-01-03 22:45 UTC (permalink / raw)
  To: Jim Shu
  Cc: Alistair Francis, Bin Meng, Palmer Dabbelt, Frank Chang,
	open list:RISC-V, qemu-devel@nongnu.org Developers

On Tue, Dec 28, 2021 at 10:54 AM Jim Shu <jim.shu@sifive.com> wrote:
>
> Real PDMA support high 32-bit read/write memory access of 64-bit
> register.
>
> The following result is PDMA tested in U-Boot on Unmatched board:
>
> 1. Real PDMA is allowed high 32-bit read/write to 64-bit register.
> => mw.l 0x3000000 0x0                      <= Disclaim channel 0
> => mw.l 0x3000000 0x1                      <= Claim channel 0
> => mw.l 0x3000010 0x80000000               <= Write low 32-bit NextDest (NextDest = 0x280000000)
> => mw.l 0x3000014 0x2                      <= Write high 32-bit NextDest
> => md.l 0x3000010 1                        <= Dump low 32-bit NextDest
> 03000010: 80000000
> => md.l 0x3000014 1                        <= Dump high 32-bit NextDest
> 03000014: 00000002
> => mw.l 0x3000018 0x80001000               <= Write low 32-bit NextSrc (NextSrc = 0x280001000)
> => mw.l 0x300001c 0x2                      <= Write high 32-bit NextSrc
> => md.l 0x3000018 1                        <= Dump low 32-bit NextSrc
> 03000010: 80001000
> => md.l 0x300001c 1                        <= Dump high 32-bit NextSrc
> 03000014: 00000002
>
> 2. PDMA transfer from 0x280001000 to 0x280000000 is OK.
> => mw.q 0x3000008 0x4                      <= NextBytes = 4
> => mw.l 0x3000004 0x22000000               <= wsize = rsize = 2 (2^2 = 4 bytes)
> => mw.l 0x280000000 0x87654321             <= Fill test data to dst
> => mw.l 0x280001000 0x12345678             <= Fill test data to src
> => md.l 0x280000000 1; md.l 0x280001000 1  <= Dump src/dst memory contents
> 280000000: 87654321                              !Ce.
> 280001000: 12345678                              xV4.
> => md.l 0x3000000 8                        <= Dump PDMA status
> 03000000: 00000001 22000000 00000004 00000000    ......."........
> 03000010: 80000000 00000002 80001000 00000002    ................
> => mw.l 0x3000000 0x3                      <= Set channel 0 run and claim bits
> => md.l 0x3000000 8                        <= Dump PDMA status
> 03000000: 40000001 22000000 00000004 00000000    ...@..."........
> 03000010: 80000000 00000002 80001000 00000002    ................
> => md.l 0x280000000 1; md.l 0x280001000 1  <= Dump src/dst memory contents
> 280000000: 12345678                               xV4.
> 280001000: 12345678                               xV4.
>
> Signed-off-by: Jim Shu <jim.shu@sifive.com>
> Reviewed-by: Frank Chang <frank.chang@sifive.com>

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

Alistair

> ---
>  hw/dma/sifive_pdma.c | 174 +++++++++++++++++++++++++++++++++++++------
>  1 file changed, 152 insertions(+), 22 deletions(-)
>
> diff --git a/hw/dma/sifive_pdma.c b/hw/dma/sifive_pdma.c
> index 85fe34f5f3..b8b198ab4e 100644
> --- a/hw/dma/sifive_pdma.c
> +++ b/hw/dma/sifive_pdma.c
> @@ -177,18 +177,44 @@ static inline void sifive_pdma_update_irq(SiFivePDMAState *s, int ch)
>      s->chan[ch].state = DMA_CHAN_STATE_IDLE;
>  }
>
> -static uint64_t sifive_pdma_read(void *opaque, hwaddr offset, unsigned size)
> +static uint64_t sifive_pdma_readq(SiFivePDMAState *s, int ch, hwaddr offset)
>  {
> -    SiFivePDMAState *s = opaque;
> -    int ch = SIFIVE_PDMA_CHAN_NO(offset);
>      uint64_t val = 0;
>
> -    if (ch >= SIFIVE_PDMA_CHANS) {
> -        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid channel no %d\n",
> -                      __func__, ch);
> -        return 0;
> +    offset &= 0xfff;
> +    switch (offset) {
> +    case DMA_NEXT_BYTES:
> +        val = s->chan[ch].next_bytes;
> +        break;
> +    case DMA_NEXT_DST:
> +        val = s->chan[ch].next_dst;
> +        break;
> +    case DMA_NEXT_SRC:
> +        val = s->chan[ch].next_src;
> +        break;
> +    case DMA_EXEC_BYTES:
> +        val = s->chan[ch].exec_bytes;
> +        break;
> +    case DMA_EXEC_DST:
> +        val = s->chan[ch].exec_dst;
> +        break;
> +    case DMA_EXEC_SRC:
> +        val = s->chan[ch].exec_src;
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Unexpected 64-bit access to 0x%" HWADDR_PRIX "\n",
> +                      __func__, offset);
> +        break;
>      }
>
> +    return val;
> +}
> +
> +static uint32_t sifive_pdma_readl(SiFivePDMAState *s, int ch, hwaddr offset)
> +{
> +    uint32_t val = 0;
> +
>      offset &= 0xfff;
>      switch (offset) {
>      case DMA_CONTROL:
> @@ -198,28 +224,47 @@ static uint64_t sifive_pdma_read(void *opaque, hwaddr offset, unsigned size)
>          val = s->chan[ch].next_config;
>          break;
>      case DMA_NEXT_BYTES:
> -        val = s->chan[ch].next_bytes;
> +        val = extract64(s->chan[ch].next_bytes, 0, 32);
> +        break;
> +    case DMA_NEXT_BYTES + 4:
> +        val = extract64(s->chan[ch].next_bytes, 32, 32);
>          break;
>      case DMA_NEXT_DST:
> -        val = s->chan[ch].next_dst;
> +        val = extract64(s->chan[ch].next_dst, 0, 32);
> +        break;
> +    case DMA_NEXT_DST + 4:
> +        val = extract64(s->chan[ch].next_dst, 32, 32);
>          break;
>      case DMA_NEXT_SRC:
> -        val = s->chan[ch].next_src;
> +        val = extract64(s->chan[ch].next_src, 0, 32);
> +        break;
> +    case DMA_NEXT_SRC + 4:
> +        val = extract64(s->chan[ch].next_src, 32, 32);
>          break;
>      case DMA_EXEC_CONFIG:
>          val = s->chan[ch].exec_config;
>          break;
>      case DMA_EXEC_BYTES:
> -        val = s->chan[ch].exec_bytes;
> +        val = extract64(s->chan[ch].exec_bytes, 0, 32);
> +        break;
> +    case DMA_EXEC_BYTES + 4:
> +        val = extract64(s->chan[ch].exec_bytes, 32, 32);
>          break;
>      case DMA_EXEC_DST:
> -        val = s->chan[ch].exec_dst;
> +        val = extract64(s->chan[ch].exec_dst, 0, 32);
> +        break;
> +    case DMA_EXEC_DST + 4:
> +        val = extract64(s->chan[ch].exec_dst, 32, 32);
>          break;
>      case DMA_EXEC_SRC:
> -        val = s->chan[ch].exec_src;
> +        val = extract64(s->chan[ch].exec_src, 0, 32);
> +        break;
> +    case DMA_EXEC_SRC + 4:
> +        val = extract64(s->chan[ch].exec_src, 32, 32);
>          break;
>      default:
> -        qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIX "\n",
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Unexpected 32-bit access to 0x%" HWADDR_PRIX "\n",
>                        __func__, offset);
>          break;
>      }
> @@ -227,19 +272,66 @@ static uint64_t sifive_pdma_read(void *opaque, hwaddr offset, unsigned size)
>      return val;
>  }
>
> -static void sifive_pdma_write(void *opaque, hwaddr offset,
> -                              uint64_t value, unsigned size)
> +static uint64_t sifive_pdma_read(void *opaque, hwaddr offset, unsigned size)
>  {
>      SiFivePDMAState *s = opaque;
>      int ch = SIFIVE_PDMA_CHAN_NO(offset);
> -    bool claimed, run;
> +    uint64_t val = 0;
>
>      if (ch >= SIFIVE_PDMA_CHANS) {
>          qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid channel no %d\n",
>                        __func__, ch);
> -        return;
> +        return 0;
> +    }
> +
> +    switch (size) {
> +    case 8:
> +        val = sifive_pdma_readq(s, ch, offset);
> +        break;
> +    case 4:
> +        val = sifive_pdma_readl(s, ch, offset);
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid read size %u to PDMA\n",
> +                      __func__, size);
> +        return 0;
>      }
>
> +    return val;
> +}
> +
> +static void sifive_pdma_writeq(SiFivePDMAState *s, int ch,
> +                               hwaddr offset, uint64_t value)
> +{
> +    offset &= 0xfff;
> +    switch (offset) {
> +    case DMA_NEXT_BYTES:
> +        s->chan[ch].next_bytes = value;
> +        break;
> +    case DMA_NEXT_DST:
> +        s->chan[ch].next_dst = value;
> +        break;
> +    case DMA_NEXT_SRC:
> +        s->chan[ch].next_src = value;
> +        break;
> +    case DMA_EXEC_BYTES:
> +    case DMA_EXEC_DST:
> +    case DMA_EXEC_SRC:
> +        /* these are read-only registers */
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Unexpected 64-bit access to 0x%" HWADDR_PRIX "\n",
> +                      __func__, offset);
> +        break;
> +    }
> +}
> +
> +static void sifive_pdma_writel(SiFivePDMAState *s, int ch,
> +                               hwaddr offset, uint32_t value)
> +{
> +    bool claimed, run;
> +
>      offset &= 0xfff;
>      switch (offset) {
>      case DMA_CONTROL:
> @@ -282,13 +374,24 @@ static void sifive_pdma_write(void *opaque, hwaddr offset,
>          s->chan[ch].next_config = value;
>          break;
>      case DMA_NEXT_BYTES:
> -        s->chan[ch].next_bytes = value;
> +        s->chan[ch].next_bytes =
> +            deposit64(s->chan[ch].next_bytes, 0, 32, value);
> +        break;
> +    case DMA_NEXT_BYTES + 4:
> +        s->chan[ch].next_bytes =
> +            deposit64(s->chan[ch].next_bytes, 32, 32, value);
>          break;
>      case DMA_NEXT_DST:
> -        s->chan[ch].next_dst = value;
> +        s->chan[ch].next_dst = deposit64(s->chan[ch].next_dst, 0, 32, value);
> +        break;
> +    case DMA_NEXT_DST + 4:
> +        s->chan[ch].next_dst = deposit64(s->chan[ch].next_dst, 32, 32, value);
>          break;
>      case DMA_NEXT_SRC:
> -        s->chan[ch].next_src = value;
> +        s->chan[ch].next_src = deposit64(s->chan[ch].next_src, 0, 32, value);
> +        break;
> +    case DMA_NEXT_SRC + 4:
> +        s->chan[ch].next_src = deposit64(s->chan[ch].next_src, 32, 32, value);
>          break;
>      case DMA_EXEC_CONFIG:
>      case DMA_EXEC_BYTES:
> @@ -297,12 +400,39 @@ static void sifive_pdma_write(void *opaque, hwaddr offset,
>          /* these are read-only registers */
>          break;
>      default:
> -        qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIX "\n",
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Unexpected 32-bit access to 0x%" HWADDR_PRIX "\n",
>                        __func__, offset);
>          break;
>      }
>  }
>
> +static void sifive_pdma_write(void *opaque, hwaddr offset,
> +                              uint64_t value, unsigned size)
> +{
> +    SiFivePDMAState *s = opaque;
> +    int ch = SIFIVE_PDMA_CHAN_NO(offset);
> +
> +    if (ch >= SIFIVE_PDMA_CHANS) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid channel no %d\n",
> +                      __func__, ch);
> +        return;
> +    }
> +
> +    switch (size) {
> +    case 8:
> +        sifive_pdma_writeq(s, ch, offset, value);
> +        break;
> +    case 4:
> +        sifive_pdma_writel(s, ch, offset, (uint32_t) value);
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid write size %u to PDMA\n",
> +                      __func__, size);
> +        break;
> +    }
> +}
> +
>  static const MemoryRegionOps sifive_pdma_ops = {
>      .read = sifive_pdma_read,
>      .write = sifive_pdma_write,
> --
> 2.25.1
>
>


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] hw/dma: sifive_pdma: permit 4/8-byte access size of PDMA registers
  2021-12-28  0:52 ` [PATCH 2/2] hw/dma: sifive_pdma: permit 4/8-byte access size of PDMA registers Jim Shu
@ 2022-01-03 22:46     ` Alistair Francis
  2022-01-03 22:46     ` Alistair Francis
  2022-01-04  2:56     ` Bin Meng
  2 siblings, 0 replies; 15+ messages in thread
From: Alistair Francis @ 2022-01-03 22:46 UTC (permalink / raw)
  To: Jim Shu
  Cc: open list:RISC-V, Frank Chang, Bin Meng,
	qemu-devel@nongnu.org Developers, Palmer Dabbelt,
	Alistair Francis

On Tue, Dec 28, 2021 at 10:54 AM Jim Shu <jim.shu@sifive.com> wrote:
>
> It's obvious that PDMA support 64-bit access of 64-bit registers, and
> in previous commit, we confirm that PDMA support 32-bit access of both
> 32/64-bit registers. Thus, we configure 32/64-bit memory access of
> PDMA registers as valid in general.
>
> Signed-off-by: Jim Shu <jim.shu@sifive.com>
> Reviewed-by: Frank Chang <frank.chang@sifive.com>

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

Alistair

> ---
>  hw/dma/sifive_pdma.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/hw/dma/sifive_pdma.c b/hw/dma/sifive_pdma.c
> index b8b198ab4e..731fcdcf89 100644
> --- a/hw/dma/sifive_pdma.c
> +++ b/hw/dma/sifive_pdma.c
> @@ -441,6 +441,10 @@ static const MemoryRegionOps sifive_pdma_ops = {
>      .impl = {
>          .min_access_size = 4,
>          .max_access_size = 8,
> +    },
> +    .valid = {
> +        .min_access_size = 4,
> +        .max_access_size = 8,
>      }
>  };
>
> --
> 2.25.1
>
>


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] hw/dma: sifive_pdma: permit 4/8-byte access size of PDMA registers
@ 2022-01-03 22:46     ` Alistair Francis
  0 siblings, 0 replies; 15+ messages in thread
From: Alistair Francis @ 2022-01-03 22:46 UTC (permalink / raw)
  To: Jim Shu
  Cc: Alistair Francis, Bin Meng, Palmer Dabbelt, Frank Chang,
	open list:RISC-V, qemu-devel@nongnu.org Developers

On Tue, Dec 28, 2021 at 10:54 AM Jim Shu <jim.shu@sifive.com> wrote:
>
> It's obvious that PDMA support 64-bit access of 64-bit registers, and
> in previous commit, we confirm that PDMA support 32-bit access of both
> 32/64-bit registers. Thus, we configure 32/64-bit memory access of
> PDMA registers as valid in general.
>
> Signed-off-by: Jim Shu <jim.shu@sifive.com>
> Reviewed-by: Frank Chang <frank.chang@sifive.com>

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

Alistair

> ---
>  hw/dma/sifive_pdma.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/hw/dma/sifive_pdma.c b/hw/dma/sifive_pdma.c
> index b8b198ab4e..731fcdcf89 100644
> --- a/hw/dma/sifive_pdma.c
> +++ b/hw/dma/sifive_pdma.c
> @@ -441,6 +441,10 @@ static const MemoryRegionOps sifive_pdma_ops = {
>      .impl = {
>          .min_access_size = 4,
>          .max_access_size = 8,
> +    },
> +    .valid = {
> +        .min_access_size = 4,
> +        .max_access_size = 8,
>      }
>  };
>
> --
> 2.25.1
>
>


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] hw/dma: sifive_pdma: support high 32-bit access of 64-bit register
  2021-12-28  0:52 ` [PATCH 1/2] hw/dma: sifive_pdma: support high 32-bit access of 64-bit register Jim Shu
@ 2022-01-04  2:55     ` Bin Meng
  2022-01-03 22:45     ` Alistair Francis
  2022-01-04  2:55     ` Bin Meng
  2 siblings, 0 replies; 15+ messages in thread
From: Bin Meng @ 2022-01-04  2:55 UTC (permalink / raw)
  To: Jim Shu
  Cc: open list:RISC-V, Frank Chang, Bin Meng,
	qemu-devel@nongnu.org Developers, Palmer Dabbelt,
	Alistair Francis

Hi Jim,

On Tue, Dec 28, 2021 at 8:53 AM Jim Shu <jim.shu@sifive.com> wrote:
>
> Real PDMA support high 32-bit read/write memory access of 64-bit

%s/support/supports

> register.
>
> The following result is PDMA tested in U-Boot on Unmatched board:
>
> 1. Real PDMA is allowed high 32-bit read/write to 64-bit register.

%s/is allowed/allows

> => mw.l 0x3000000 0x0                      <= Disclaim channel 0
> => mw.l 0x3000000 0x1                      <= Claim channel 0
> => mw.l 0x3000010 0x80000000               <= Write low 32-bit NextDest (NextDest = 0x280000000)
> => mw.l 0x3000014 0x2                      <= Write high 32-bit NextDest
> => md.l 0x3000010 1                        <= Dump low 32-bit NextDest
> 03000010: 80000000
> => md.l 0x3000014 1                        <= Dump high 32-bit NextDest
> 03000014: 00000002
> => mw.l 0x3000018 0x80001000               <= Write low 32-bit NextSrc (NextSrc = 0x280001000)
> => mw.l 0x300001c 0x2                      <= Write high 32-bit NextSrc
> => md.l 0x3000018 1                        <= Dump low 32-bit NextSrc
> 03000010: 80001000
> => md.l 0x300001c 1                        <= Dump high 32-bit NextSrc
> 03000014: 00000002
>
> 2. PDMA transfer from 0x280001000 to 0x280000000 is OK.
> => mw.q 0x3000008 0x4                      <= NextBytes = 4
> => mw.l 0x3000004 0x22000000               <= wsize = rsize = 2 (2^2 = 4 bytes)
> => mw.l 0x280000000 0x87654321             <= Fill test data to dst
> => mw.l 0x280001000 0x12345678             <= Fill test data to src
> => md.l 0x280000000 1; md.l 0x280001000 1  <= Dump src/dst memory contents
> 280000000: 87654321                              !Ce.
> 280001000: 12345678                              xV4.
> => md.l 0x3000000 8                        <= Dump PDMA status
> 03000000: 00000001 22000000 00000004 00000000    ......."........
> 03000010: 80000000 00000002 80001000 00000002    ................
> => mw.l 0x3000000 0x3                      <= Set channel 0 run and claim bits
> => md.l 0x3000000 8                        <= Dump PDMA status
> 03000000: 40000001 22000000 00000004 00000000    ...@..."........
> 03000010: 80000000 00000002 80001000 00000002    ................
> => md.l 0x280000000 1; md.l 0x280001000 1  <= Dump src/dst memory contents
> 280000000: 12345678                               xV4.
> 280001000: 12345678                               xV4.
>
> Signed-off-by: Jim Shu <jim.shu@sifive.com>
> Reviewed-by: Frank Chang <frank.chang@sifive.com>
> ---
>  hw/dma/sifive_pdma.c | 174 +++++++++++++++++++++++++++++++++++++------
>  1 file changed, 152 insertions(+), 22 deletions(-)
>
> diff --git a/hw/dma/sifive_pdma.c b/hw/dma/sifive_pdma.c
> index 85fe34f5f3..b8b198ab4e 100644
> --- a/hw/dma/sifive_pdma.c
> +++ b/hw/dma/sifive_pdma.c
> @@ -177,18 +177,44 @@ static inline void sifive_pdma_update_irq(SiFivePDMAState *s, int ch)
>      s->chan[ch].state = DMA_CHAN_STATE_IDLE;
>  }
>
> -static uint64_t sifive_pdma_read(void *opaque, hwaddr offset, unsigned size)
> +static uint64_t sifive_pdma_readq(SiFivePDMAState *s, int ch, hwaddr offset)
>  {
> -    SiFivePDMAState *s = opaque;
> -    int ch = SIFIVE_PDMA_CHAN_NO(offset);
>      uint64_t val = 0;
>
> -    if (ch >= SIFIVE_PDMA_CHANS) {
> -        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid channel no %d\n",
> -                      __func__, ch);
> -        return 0;
> +    offset &= 0xfff;
> +    switch (offset) {
> +    case DMA_NEXT_BYTES:
> +        val = s->chan[ch].next_bytes;
> +        break;
> +    case DMA_NEXT_DST:
> +        val = s->chan[ch].next_dst;
> +        break;
> +    case DMA_NEXT_SRC:
> +        val = s->chan[ch].next_src;
> +        break;
> +    case DMA_EXEC_BYTES:
> +        val = s->chan[ch].exec_bytes;
> +        break;
> +    case DMA_EXEC_DST:
> +        val = s->chan[ch].exec_dst;
> +        break;
> +    case DMA_EXEC_SRC:
> +        val = s->chan[ch].exec_src;
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Unexpected 64-bit access to 0x%" HWADDR_PRIX "\n",
> +                      __func__, offset);
> +        break;
>      }
>
> +    return val;
> +}
> +
> +static uint32_t sifive_pdma_readl(SiFivePDMAState *s, int ch, hwaddr offset)
> +{
> +    uint32_t val = 0;
> +
>      offset &= 0xfff;
>      switch (offset) {
>      case DMA_CONTROL:
> @@ -198,28 +224,47 @@ static uint64_t sifive_pdma_read(void *opaque, hwaddr offset, unsigned size)
>          val = s->chan[ch].next_config;
>          break;
>      case DMA_NEXT_BYTES:
> -        val = s->chan[ch].next_bytes;
> +        val = extract64(s->chan[ch].next_bytes, 0, 32);
> +        break;
> +    case DMA_NEXT_BYTES + 4:
> +        val = extract64(s->chan[ch].next_bytes, 32, 32);
>          break;
>      case DMA_NEXT_DST:
> -        val = s->chan[ch].next_dst;
> +        val = extract64(s->chan[ch].next_dst, 0, 32);
> +        break;
> +    case DMA_NEXT_DST + 4:
> +        val = extract64(s->chan[ch].next_dst, 32, 32);
>          break;
>      case DMA_NEXT_SRC:
> -        val = s->chan[ch].next_src;
> +        val = extract64(s->chan[ch].next_src, 0, 32);
> +        break;
> +    case DMA_NEXT_SRC + 4:
> +        val = extract64(s->chan[ch].next_src, 32, 32);
>          break;
>      case DMA_EXEC_CONFIG:
>          val = s->chan[ch].exec_config;
>          break;
>      case DMA_EXEC_BYTES:
> -        val = s->chan[ch].exec_bytes;
> +        val = extract64(s->chan[ch].exec_bytes, 0, 32);
> +        break;
> +    case DMA_EXEC_BYTES + 4:
> +        val = extract64(s->chan[ch].exec_bytes, 32, 32);
>          break;
>      case DMA_EXEC_DST:
> -        val = s->chan[ch].exec_dst;
> +        val = extract64(s->chan[ch].exec_dst, 0, 32);
> +        break;
> +    case DMA_EXEC_DST + 4:
> +        val = extract64(s->chan[ch].exec_dst, 32, 32);
>          break;
>      case DMA_EXEC_SRC:
> -        val = s->chan[ch].exec_src;
> +        val = extract64(s->chan[ch].exec_src, 0, 32);
> +        break;
> +    case DMA_EXEC_SRC + 4:
> +        val = extract64(s->chan[ch].exec_src, 32, 32);
>          break;
>      default:
> -        qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIX "\n",
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Unexpected 32-bit access to 0x%" HWADDR_PRIX "\n",
>                        __func__, offset);
>          break;
>      }
> @@ -227,19 +272,66 @@ static uint64_t sifive_pdma_read(void *opaque, hwaddr offset, unsigned size)
>      return val;
>  }
>
> -static void sifive_pdma_write(void *opaque, hwaddr offset,
> -                              uint64_t value, unsigned size)
> +static uint64_t sifive_pdma_read(void *opaque, hwaddr offset, unsigned size)
>  {
>      SiFivePDMAState *s = opaque;
>      int ch = SIFIVE_PDMA_CHAN_NO(offset);
> -    bool claimed, run;
> +    uint64_t val = 0;
>
>      if (ch >= SIFIVE_PDMA_CHANS) {
>          qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid channel no %d\n",
>                        __func__, ch);
> -        return;
> +        return 0;
> +    }
> +
> +    switch (size) {
> +    case 8:
> +        val = sifive_pdma_readq(s, ch, offset);
> +        break;
> +    case 4:
> +        val = sifive_pdma_readl(s, ch, offset);
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid read size %u to PDMA\n",
> +                      __func__, size);
> +        return 0;
>      }
>
> +    return val;
> +}
> +
> +static void sifive_pdma_writeq(SiFivePDMAState *s, int ch,
> +                               hwaddr offset, uint64_t value)
> +{
> +    offset &= 0xfff;
> +    switch (offset) {
> +    case DMA_NEXT_BYTES:
> +        s->chan[ch].next_bytes = value;
> +        break;
> +    case DMA_NEXT_DST:
> +        s->chan[ch].next_dst = value;
> +        break;
> +    case DMA_NEXT_SRC:
> +        s->chan[ch].next_src = value;
> +        break;
> +    case DMA_EXEC_BYTES:
> +    case DMA_EXEC_DST:
> +    case DMA_EXEC_SRC:
> +        /* these are read-only registers */
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Unexpected 64-bit access to 0x%" HWADDR_PRIX "\n",
> +                      __func__, offset);
> +        break;
> +    }
> +}
> +
> +static void sifive_pdma_writel(SiFivePDMAState *s, int ch,
> +                               hwaddr offset, uint32_t value)
> +{
> +    bool claimed, run;
> +
>      offset &= 0xfff;
>      switch (offset) {
>      case DMA_CONTROL:
> @@ -282,13 +374,24 @@ static void sifive_pdma_write(void *opaque, hwaddr offset,
>          s->chan[ch].next_config = value;
>          break;
>      case DMA_NEXT_BYTES:
> -        s->chan[ch].next_bytes = value;
> +        s->chan[ch].next_bytes =
> +            deposit64(s->chan[ch].next_bytes, 0, 32, value);
> +        break;
> +    case DMA_NEXT_BYTES + 4:
> +        s->chan[ch].next_bytes =
> +            deposit64(s->chan[ch].next_bytes, 32, 32, value);
>          break;
>      case DMA_NEXT_DST:
> -        s->chan[ch].next_dst = value;
> +        s->chan[ch].next_dst = deposit64(s->chan[ch].next_dst, 0, 32, value);
> +        break;
> +    case DMA_NEXT_DST + 4:
> +        s->chan[ch].next_dst = deposit64(s->chan[ch].next_dst, 32, 32, value);
>          break;
>      case DMA_NEXT_SRC:
> -        s->chan[ch].next_src = value;
> +        s->chan[ch].next_src = deposit64(s->chan[ch].next_src, 0, 32, value);
> +        break;
> +    case DMA_NEXT_SRC + 4:
> +        s->chan[ch].next_src = deposit64(s->chan[ch].next_src, 32, 32, value);
>          break;
>      case DMA_EXEC_CONFIG:
>      case DMA_EXEC_BYTES:

The high 32-bit of exec_bytes, exec_dst and exec_src should be added
to the "read-only" break branch, instead of falling through to the
default branch.

> @@ -297,12 +400,39 @@ static void sifive_pdma_write(void *opaque, hwaddr offset,
>          /* these are read-only registers */
>          break;
>      default:
> -        qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIX "\n",
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Unexpected 32-bit access to 0x%" HWADDR_PRIX "\n",
>                        __func__, offset);
>          break;
>      }
>  }
>
> +static void sifive_pdma_write(void *opaque, hwaddr offset,
> +                              uint64_t value, unsigned size)
> +{
> +    SiFivePDMAState *s = opaque;
> +    int ch = SIFIVE_PDMA_CHAN_NO(offset);
> +
> +    if (ch >= SIFIVE_PDMA_CHANS) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid channel no %d\n",
> +                      __func__, ch);
> +        return;
> +    }
> +
> +    switch (size) {
> +    case 8:
> +        sifive_pdma_writeq(s, ch, offset, value);
> +        break;
> +    case 4:
> +        sifive_pdma_writel(s, ch, offset, (uint32_t) value);
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid write size %u to PDMA\n",
> +                      __func__, size);
> +        break;
> +    }
> +}
> +
>  static const MemoryRegionOps sifive_pdma_ops = {
>      .read = sifive_pdma_read,
>      .write = sifive_pdma_write,
> --

Otherwise, tested with U-Boot sifive_unleashed_defconfig,

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
Tested-by: Bin Meng <bmeng.cn@gmail.com>


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] hw/dma: sifive_pdma: support high 32-bit access of 64-bit register
@ 2022-01-04  2:55     ` Bin Meng
  0 siblings, 0 replies; 15+ messages in thread
From: Bin Meng @ 2022-01-04  2:55 UTC (permalink / raw)
  To: Jim Shu
  Cc: Alistair Francis, Bin Meng, Palmer Dabbelt, Frank Chang,
	open list:RISC-V, qemu-devel@nongnu.org Developers

Hi Jim,

On Tue, Dec 28, 2021 at 8:53 AM Jim Shu <jim.shu@sifive.com> wrote:
>
> Real PDMA support high 32-bit read/write memory access of 64-bit

%s/support/supports

> register.
>
> The following result is PDMA tested in U-Boot on Unmatched board:
>
> 1. Real PDMA is allowed high 32-bit read/write to 64-bit register.

%s/is allowed/allows

> => mw.l 0x3000000 0x0                      <= Disclaim channel 0
> => mw.l 0x3000000 0x1                      <= Claim channel 0
> => mw.l 0x3000010 0x80000000               <= Write low 32-bit NextDest (NextDest = 0x280000000)
> => mw.l 0x3000014 0x2                      <= Write high 32-bit NextDest
> => md.l 0x3000010 1                        <= Dump low 32-bit NextDest
> 03000010: 80000000
> => md.l 0x3000014 1                        <= Dump high 32-bit NextDest
> 03000014: 00000002
> => mw.l 0x3000018 0x80001000               <= Write low 32-bit NextSrc (NextSrc = 0x280001000)
> => mw.l 0x300001c 0x2                      <= Write high 32-bit NextSrc
> => md.l 0x3000018 1                        <= Dump low 32-bit NextSrc
> 03000010: 80001000
> => md.l 0x300001c 1                        <= Dump high 32-bit NextSrc
> 03000014: 00000002
>
> 2. PDMA transfer from 0x280001000 to 0x280000000 is OK.
> => mw.q 0x3000008 0x4                      <= NextBytes = 4
> => mw.l 0x3000004 0x22000000               <= wsize = rsize = 2 (2^2 = 4 bytes)
> => mw.l 0x280000000 0x87654321             <= Fill test data to dst
> => mw.l 0x280001000 0x12345678             <= Fill test data to src
> => md.l 0x280000000 1; md.l 0x280001000 1  <= Dump src/dst memory contents
> 280000000: 87654321                              !Ce.
> 280001000: 12345678                              xV4.
> => md.l 0x3000000 8                        <= Dump PDMA status
> 03000000: 00000001 22000000 00000004 00000000    ......."........
> 03000010: 80000000 00000002 80001000 00000002    ................
> => mw.l 0x3000000 0x3                      <= Set channel 0 run and claim bits
> => md.l 0x3000000 8                        <= Dump PDMA status
> 03000000: 40000001 22000000 00000004 00000000    ...@..."........
> 03000010: 80000000 00000002 80001000 00000002    ................
> => md.l 0x280000000 1; md.l 0x280001000 1  <= Dump src/dst memory contents
> 280000000: 12345678                               xV4.
> 280001000: 12345678                               xV4.
>
> Signed-off-by: Jim Shu <jim.shu@sifive.com>
> Reviewed-by: Frank Chang <frank.chang@sifive.com>
> ---
>  hw/dma/sifive_pdma.c | 174 +++++++++++++++++++++++++++++++++++++------
>  1 file changed, 152 insertions(+), 22 deletions(-)
>
> diff --git a/hw/dma/sifive_pdma.c b/hw/dma/sifive_pdma.c
> index 85fe34f5f3..b8b198ab4e 100644
> --- a/hw/dma/sifive_pdma.c
> +++ b/hw/dma/sifive_pdma.c
> @@ -177,18 +177,44 @@ static inline void sifive_pdma_update_irq(SiFivePDMAState *s, int ch)
>      s->chan[ch].state = DMA_CHAN_STATE_IDLE;
>  }
>
> -static uint64_t sifive_pdma_read(void *opaque, hwaddr offset, unsigned size)
> +static uint64_t sifive_pdma_readq(SiFivePDMAState *s, int ch, hwaddr offset)
>  {
> -    SiFivePDMAState *s = opaque;
> -    int ch = SIFIVE_PDMA_CHAN_NO(offset);
>      uint64_t val = 0;
>
> -    if (ch >= SIFIVE_PDMA_CHANS) {
> -        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid channel no %d\n",
> -                      __func__, ch);
> -        return 0;
> +    offset &= 0xfff;
> +    switch (offset) {
> +    case DMA_NEXT_BYTES:
> +        val = s->chan[ch].next_bytes;
> +        break;
> +    case DMA_NEXT_DST:
> +        val = s->chan[ch].next_dst;
> +        break;
> +    case DMA_NEXT_SRC:
> +        val = s->chan[ch].next_src;
> +        break;
> +    case DMA_EXEC_BYTES:
> +        val = s->chan[ch].exec_bytes;
> +        break;
> +    case DMA_EXEC_DST:
> +        val = s->chan[ch].exec_dst;
> +        break;
> +    case DMA_EXEC_SRC:
> +        val = s->chan[ch].exec_src;
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Unexpected 64-bit access to 0x%" HWADDR_PRIX "\n",
> +                      __func__, offset);
> +        break;
>      }
>
> +    return val;
> +}
> +
> +static uint32_t sifive_pdma_readl(SiFivePDMAState *s, int ch, hwaddr offset)
> +{
> +    uint32_t val = 0;
> +
>      offset &= 0xfff;
>      switch (offset) {
>      case DMA_CONTROL:
> @@ -198,28 +224,47 @@ static uint64_t sifive_pdma_read(void *opaque, hwaddr offset, unsigned size)
>          val = s->chan[ch].next_config;
>          break;
>      case DMA_NEXT_BYTES:
> -        val = s->chan[ch].next_bytes;
> +        val = extract64(s->chan[ch].next_bytes, 0, 32);
> +        break;
> +    case DMA_NEXT_BYTES + 4:
> +        val = extract64(s->chan[ch].next_bytes, 32, 32);
>          break;
>      case DMA_NEXT_DST:
> -        val = s->chan[ch].next_dst;
> +        val = extract64(s->chan[ch].next_dst, 0, 32);
> +        break;
> +    case DMA_NEXT_DST + 4:
> +        val = extract64(s->chan[ch].next_dst, 32, 32);
>          break;
>      case DMA_NEXT_SRC:
> -        val = s->chan[ch].next_src;
> +        val = extract64(s->chan[ch].next_src, 0, 32);
> +        break;
> +    case DMA_NEXT_SRC + 4:
> +        val = extract64(s->chan[ch].next_src, 32, 32);
>          break;
>      case DMA_EXEC_CONFIG:
>          val = s->chan[ch].exec_config;
>          break;
>      case DMA_EXEC_BYTES:
> -        val = s->chan[ch].exec_bytes;
> +        val = extract64(s->chan[ch].exec_bytes, 0, 32);
> +        break;
> +    case DMA_EXEC_BYTES + 4:
> +        val = extract64(s->chan[ch].exec_bytes, 32, 32);
>          break;
>      case DMA_EXEC_DST:
> -        val = s->chan[ch].exec_dst;
> +        val = extract64(s->chan[ch].exec_dst, 0, 32);
> +        break;
> +    case DMA_EXEC_DST + 4:
> +        val = extract64(s->chan[ch].exec_dst, 32, 32);
>          break;
>      case DMA_EXEC_SRC:
> -        val = s->chan[ch].exec_src;
> +        val = extract64(s->chan[ch].exec_src, 0, 32);
> +        break;
> +    case DMA_EXEC_SRC + 4:
> +        val = extract64(s->chan[ch].exec_src, 32, 32);
>          break;
>      default:
> -        qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIX "\n",
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Unexpected 32-bit access to 0x%" HWADDR_PRIX "\n",
>                        __func__, offset);
>          break;
>      }
> @@ -227,19 +272,66 @@ static uint64_t sifive_pdma_read(void *opaque, hwaddr offset, unsigned size)
>      return val;
>  }
>
> -static void sifive_pdma_write(void *opaque, hwaddr offset,
> -                              uint64_t value, unsigned size)
> +static uint64_t sifive_pdma_read(void *opaque, hwaddr offset, unsigned size)
>  {
>      SiFivePDMAState *s = opaque;
>      int ch = SIFIVE_PDMA_CHAN_NO(offset);
> -    bool claimed, run;
> +    uint64_t val = 0;
>
>      if (ch >= SIFIVE_PDMA_CHANS) {
>          qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid channel no %d\n",
>                        __func__, ch);
> -        return;
> +        return 0;
> +    }
> +
> +    switch (size) {
> +    case 8:
> +        val = sifive_pdma_readq(s, ch, offset);
> +        break;
> +    case 4:
> +        val = sifive_pdma_readl(s, ch, offset);
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid read size %u to PDMA\n",
> +                      __func__, size);
> +        return 0;
>      }
>
> +    return val;
> +}
> +
> +static void sifive_pdma_writeq(SiFivePDMAState *s, int ch,
> +                               hwaddr offset, uint64_t value)
> +{
> +    offset &= 0xfff;
> +    switch (offset) {
> +    case DMA_NEXT_BYTES:
> +        s->chan[ch].next_bytes = value;
> +        break;
> +    case DMA_NEXT_DST:
> +        s->chan[ch].next_dst = value;
> +        break;
> +    case DMA_NEXT_SRC:
> +        s->chan[ch].next_src = value;
> +        break;
> +    case DMA_EXEC_BYTES:
> +    case DMA_EXEC_DST:
> +    case DMA_EXEC_SRC:
> +        /* these are read-only registers */
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Unexpected 64-bit access to 0x%" HWADDR_PRIX "\n",
> +                      __func__, offset);
> +        break;
> +    }
> +}
> +
> +static void sifive_pdma_writel(SiFivePDMAState *s, int ch,
> +                               hwaddr offset, uint32_t value)
> +{
> +    bool claimed, run;
> +
>      offset &= 0xfff;
>      switch (offset) {
>      case DMA_CONTROL:
> @@ -282,13 +374,24 @@ static void sifive_pdma_write(void *opaque, hwaddr offset,
>          s->chan[ch].next_config = value;
>          break;
>      case DMA_NEXT_BYTES:
> -        s->chan[ch].next_bytes = value;
> +        s->chan[ch].next_bytes =
> +            deposit64(s->chan[ch].next_bytes, 0, 32, value);
> +        break;
> +    case DMA_NEXT_BYTES + 4:
> +        s->chan[ch].next_bytes =
> +            deposit64(s->chan[ch].next_bytes, 32, 32, value);
>          break;
>      case DMA_NEXT_DST:
> -        s->chan[ch].next_dst = value;
> +        s->chan[ch].next_dst = deposit64(s->chan[ch].next_dst, 0, 32, value);
> +        break;
> +    case DMA_NEXT_DST + 4:
> +        s->chan[ch].next_dst = deposit64(s->chan[ch].next_dst, 32, 32, value);
>          break;
>      case DMA_NEXT_SRC:
> -        s->chan[ch].next_src = value;
> +        s->chan[ch].next_src = deposit64(s->chan[ch].next_src, 0, 32, value);
> +        break;
> +    case DMA_NEXT_SRC + 4:
> +        s->chan[ch].next_src = deposit64(s->chan[ch].next_src, 32, 32, value);
>          break;
>      case DMA_EXEC_CONFIG:
>      case DMA_EXEC_BYTES:

The high 32-bit of exec_bytes, exec_dst and exec_src should be added
to the "read-only" break branch, instead of falling through to the
default branch.

> @@ -297,12 +400,39 @@ static void sifive_pdma_write(void *opaque, hwaddr offset,
>          /* these are read-only registers */
>          break;
>      default:
> -        qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIX "\n",
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Unexpected 32-bit access to 0x%" HWADDR_PRIX "\n",
>                        __func__, offset);
>          break;
>      }
>  }
>
> +static void sifive_pdma_write(void *opaque, hwaddr offset,
> +                              uint64_t value, unsigned size)
> +{
> +    SiFivePDMAState *s = opaque;
> +    int ch = SIFIVE_PDMA_CHAN_NO(offset);
> +
> +    if (ch >= SIFIVE_PDMA_CHANS) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid channel no %d\n",
> +                      __func__, ch);
> +        return;
> +    }
> +
> +    switch (size) {
> +    case 8:
> +        sifive_pdma_writeq(s, ch, offset, value);
> +        break;
> +    case 4:
> +        sifive_pdma_writel(s, ch, offset, (uint32_t) value);
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid write size %u to PDMA\n",
> +                      __func__, size);
> +        break;
> +    }
> +}
> +
>  static const MemoryRegionOps sifive_pdma_ops = {
>      .read = sifive_pdma_read,
>      .write = sifive_pdma_write,
> --

Otherwise, tested with U-Boot sifive_unleashed_defconfig,

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
Tested-by: Bin Meng <bmeng.cn@gmail.com>


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] hw/dma: sifive_pdma: permit 4/8-byte access size of PDMA registers
  2021-12-28  0:52 ` [PATCH 2/2] hw/dma: sifive_pdma: permit 4/8-byte access size of PDMA registers Jim Shu
@ 2022-01-04  2:56     ` Bin Meng
  2022-01-03 22:46     ` Alistair Francis
  2022-01-04  2:56     ` Bin Meng
  2 siblings, 0 replies; 15+ messages in thread
From: Bin Meng @ 2022-01-04  2:56 UTC (permalink / raw)
  To: Jim Shu
  Cc: open list:RISC-V, Frank Chang, Bin Meng,
	qemu-devel@nongnu.org Developers, Palmer Dabbelt,
	Alistair Francis

On Tue, Dec 28, 2021 at 8:53 AM Jim Shu <jim.shu@sifive.com> wrote:
>
> It's obvious that PDMA support 64-bit access of 64-bit registers, and

%s/support/supports

> in previous commit, we confirm that PDMA support 32-bit access of both

%s/support/supports

> 32/64-bit registers. Thus, we configure 32/64-bit memory access of
> PDMA registers as valid in general.
>
> Signed-off-by: Jim Shu <jim.shu@sifive.com>
> Reviewed-by: Frank Chang <frank.chang@sifive.com>
> ---
>  hw/dma/sifive_pdma.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/hw/dma/sifive_pdma.c b/hw/dma/sifive_pdma.c
> index b8b198ab4e..731fcdcf89 100644
> --- a/hw/dma/sifive_pdma.c
> +++ b/hw/dma/sifive_pdma.c
> @@ -441,6 +441,10 @@ static const MemoryRegionOps sifive_pdma_ops = {
>      .impl = {
>          .min_access_size = 4,
>          .max_access_size = 8,
> +    },
> +    .valid = {
> +        .min_access_size = 4,
> +        .max_access_size = 8,
>      }
>  };
>

Otherwise,

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
Tested-by: Bin Meng <bmeng.cn@gmail.com>


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] hw/dma: sifive_pdma: permit 4/8-byte access size of PDMA registers
@ 2022-01-04  2:56     ` Bin Meng
  0 siblings, 0 replies; 15+ messages in thread
From: Bin Meng @ 2022-01-04  2:56 UTC (permalink / raw)
  To: Jim Shu
  Cc: Alistair Francis, Bin Meng, Palmer Dabbelt, Frank Chang,
	open list:RISC-V, qemu-devel@nongnu.org Developers

On Tue, Dec 28, 2021 at 8:53 AM Jim Shu <jim.shu@sifive.com> wrote:
>
> It's obvious that PDMA support 64-bit access of 64-bit registers, and

%s/support/supports

> in previous commit, we confirm that PDMA support 32-bit access of both

%s/support/supports

> 32/64-bit registers. Thus, we configure 32/64-bit memory access of
> PDMA registers as valid in general.
>
> Signed-off-by: Jim Shu <jim.shu@sifive.com>
> Reviewed-by: Frank Chang <frank.chang@sifive.com>
> ---
>  hw/dma/sifive_pdma.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/hw/dma/sifive_pdma.c b/hw/dma/sifive_pdma.c
> index b8b198ab4e..731fcdcf89 100644
> --- a/hw/dma/sifive_pdma.c
> +++ b/hw/dma/sifive_pdma.c
> @@ -441,6 +441,10 @@ static const MemoryRegionOps sifive_pdma_ops = {
>      .impl = {
>          .min_access_size = 4,
>          .max_access_size = 8,
> +    },
> +    .valid = {
> +        .min_access_size = 4,
> +        .max_access_size = 8,
>      }
>  };
>

Otherwise,

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
Tested-by: Bin Meng <bmeng.cn@gmail.com>


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] hw/dma: sifive_pdma: support high 32-bit access of 64-bit register
  2022-01-04  2:55     ` Bin Meng
@ 2022-01-04  5:53       ` Jim Shu
  -1 siblings, 0 replies; 15+ messages in thread
From: Jim Shu @ 2022-01-04  5:53 UTC (permalink / raw)
  To: Bin Meng
  Cc: open list:RISC-V, Frank Chang, Bin Meng,
	qemu-devel@nongnu.org Developers, Palmer Dabbelt,
	Alistair Francis

[-- Attachment #1: Type: text/plain, Size: 12195 bytes --]

Hi Bin,

Thanks for the review.
I will fix the commit log and the behavior of writing high 32-bit of RO
registers in v2 patch.


Thanks,
Jim Shu

On Tue, Jan 4, 2022 at 10:55 AM Bin Meng <bmeng.cn@gmail.com> wrote:

> Hi Jim,
>
> On Tue, Dec 28, 2021 at 8:53 AM Jim Shu <jim.shu@sifive.com> wrote:
> >
> > Real PDMA support high 32-bit read/write memory access of 64-bit
>
> %s/support/supports
>
> > register.
> >
> > The following result is PDMA tested in U-Boot on Unmatched board:
> >
> > 1. Real PDMA is allowed high 32-bit read/write to 64-bit register.
>
> %s/is allowed/allows
>
> > => mw.l 0x3000000 0x0                      <= Disclaim channel 0
> > => mw.l 0x3000000 0x1                      <= Claim channel 0
> > => mw.l 0x3000010 0x80000000               <= Write low 32-bit NextDest
> (NextDest = 0x280000000)
> > => mw.l 0x3000014 0x2                      <= Write high 32-bit NextDest
> > => md.l 0x3000010 1                        <= Dump low 32-bit NextDest
> > 03000010: 80000000
> > => md.l 0x3000014 1                        <= Dump high 32-bit NextDest
> > 03000014: 00000002
> > => mw.l 0x3000018 0x80001000               <= Write low 32-bit NextSrc
> (NextSrc = 0x280001000)
> > => mw.l 0x300001c 0x2                      <= Write high 32-bit NextSrc
> > => md.l 0x3000018 1                        <= Dump low 32-bit NextSrc
> > 03000010: 80001000
> > => md.l 0x300001c 1                        <= Dump high 32-bit NextSrc
> > 03000014: 00000002
> >
> > 2. PDMA transfer from 0x280001000 to 0x280000000 is OK.
> > => mw.q 0x3000008 0x4                      <= NextBytes = 4
> > => mw.l 0x3000004 0x22000000               <= wsize = rsize = 2 (2^2 = 4
> bytes)
> > => mw.l 0x280000000 0x87654321             <= Fill test data to dst
> > => mw.l 0x280001000 0x12345678             <= Fill test data to src
> > => md.l 0x280000000 1; md.l 0x280001000 1  <= Dump src/dst memory
> contents
> > 280000000: 87654321                              !Ce.
> > 280001000: 12345678                              xV4.
> > => md.l 0x3000000 8                        <= Dump PDMA status
> > 03000000: 00000001 22000000 00000004 00000000    ......."........
> > 03000010: 80000000 00000002 80001000 00000002    ................
> > => mw.l 0x3000000 0x3                      <= Set channel 0 run and
> claim bits
> > => md.l 0x3000000 8                        <= Dump PDMA status
> > 03000000: 40000001 22000000 00000004 00000000    ...@..."........
> > 03000010: 80000000 00000002 80001000 00000002    ................
> > => md.l 0x280000000 1; md.l 0x280001000 1  <= Dump src/dst memory
> contents
> > 280000000: 12345678                               xV4.
> > 280001000: 12345678                               xV4.
> >
> > Signed-off-by: Jim Shu <jim.shu@sifive.com>
> > Reviewed-by: Frank Chang <frank.chang@sifive.com>
> > ---
> >  hw/dma/sifive_pdma.c | 174 +++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 152 insertions(+), 22 deletions(-)
> >
> > diff --git a/hw/dma/sifive_pdma.c b/hw/dma/sifive_pdma.c
> > index 85fe34f5f3..b8b198ab4e 100644
> > --- a/hw/dma/sifive_pdma.c
> > +++ b/hw/dma/sifive_pdma.c
> > @@ -177,18 +177,44 @@ static inline void
> sifive_pdma_update_irq(SiFivePDMAState *s, int ch)
> >      s->chan[ch].state = DMA_CHAN_STATE_IDLE;
> >  }
> >
> > -static uint64_t sifive_pdma_read(void *opaque, hwaddr offset, unsigned
> size)
> > +static uint64_t sifive_pdma_readq(SiFivePDMAState *s, int ch, hwaddr
> offset)
> >  {
> > -    SiFivePDMAState *s = opaque;
> > -    int ch = SIFIVE_PDMA_CHAN_NO(offset);
> >      uint64_t val = 0;
> >
> > -    if (ch >= SIFIVE_PDMA_CHANS) {
> > -        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid channel no %d\n",
> > -                      __func__, ch);
> > -        return 0;
> > +    offset &= 0xfff;
> > +    switch (offset) {
> > +    case DMA_NEXT_BYTES:
> > +        val = s->chan[ch].next_bytes;
> > +        break;
> > +    case DMA_NEXT_DST:
> > +        val = s->chan[ch].next_dst;
> > +        break;
> > +    case DMA_NEXT_SRC:
> > +        val = s->chan[ch].next_src;
> > +        break;
> > +    case DMA_EXEC_BYTES:
> > +        val = s->chan[ch].exec_bytes;
> > +        break;
> > +    case DMA_EXEC_DST:
> > +        val = s->chan[ch].exec_dst;
> > +        break;
> > +    case DMA_EXEC_SRC:
> > +        val = s->chan[ch].exec_src;
> > +        break;
> > +    default:
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: Unexpected 64-bit access to 0x%" HWADDR_PRIX
> "\n",
> > +                      __func__, offset);
> > +        break;
> >      }
> >
> > +    return val;
> > +}
> > +
> > +static uint32_t sifive_pdma_readl(SiFivePDMAState *s, int ch, hwaddr
> offset)
> > +{
> > +    uint32_t val = 0;
> > +
> >      offset &= 0xfff;
> >      switch (offset) {
> >      case DMA_CONTROL:
> > @@ -198,28 +224,47 @@ static uint64_t sifive_pdma_read(void *opaque,
> hwaddr offset, unsigned size)
> >          val = s->chan[ch].next_config;
> >          break;
> >      case DMA_NEXT_BYTES:
> > -        val = s->chan[ch].next_bytes;
> > +        val = extract64(s->chan[ch].next_bytes, 0, 32);
> > +        break;
> > +    case DMA_NEXT_BYTES + 4:
> > +        val = extract64(s->chan[ch].next_bytes, 32, 32);
> >          break;
> >      case DMA_NEXT_DST:
> > -        val = s->chan[ch].next_dst;
> > +        val = extract64(s->chan[ch].next_dst, 0, 32);
> > +        break;
> > +    case DMA_NEXT_DST + 4:
> > +        val = extract64(s->chan[ch].next_dst, 32, 32);
> >          break;
> >      case DMA_NEXT_SRC:
> > -        val = s->chan[ch].next_src;
> > +        val = extract64(s->chan[ch].next_src, 0, 32);
> > +        break;
> > +    case DMA_NEXT_SRC + 4:
> > +        val = extract64(s->chan[ch].next_src, 32, 32);
> >          break;
> >      case DMA_EXEC_CONFIG:
> >          val = s->chan[ch].exec_config;
> >          break;
> >      case DMA_EXEC_BYTES:
> > -        val = s->chan[ch].exec_bytes;
> > +        val = extract64(s->chan[ch].exec_bytes, 0, 32);
> > +        break;
> > +    case DMA_EXEC_BYTES + 4:
> > +        val = extract64(s->chan[ch].exec_bytes, 32, 32);
> >          break;
> >      case DMA_EXEC_DST:
> > -        val = s->chan[ch].exec_dst;
> > +        val = extract64(s->chan[ch].exec_dst, 0, 32);
> > +        break;
> > +    case DMA_EXEC_DST + 4:
> > +        val = extract64(s->chan[ch].exec_dst, 32, 32);
> >          break;
> >      case DMA_EXEC_SRC:
> > -        val = s->chan[ch].exec_src;
> > +        val = extract64(s->chan[ch].exec_src, 0, 32);
> > +        break;
> > +    case DMA_EXEC_SRC + 4:
> > +        val = extract64(s->chan[ch].exec_src, 32, 32);
> >          break;
> >      default:
> > -        qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIX
> "\n",
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: Unexpected 32-bit access to 0x%" HWADDR_PRIX
> "\n",
> >                        __func__, offset);
> >          break;
> >      }
> > @@ -227,19 +272,66 @@ static uint64_t sifive_pdma_read(void *opaque,
> hwaddr offset, unsigned size)
> >      return val;
> >  }
> >
> > -static void sifive_pdma_write(void *opaque, hwaddr offset,
> > -                              uint64_t value, unsigned size)
> > +static uint64_t sifive_pdma_read(void *opaque, hwaddr offset, unsigned
> size)
> >  {
> >      SiFivePDMAState *s = opaque;
> >      int ch = SIFIVE_PDMA_CHAN_NO(offset);
> > -    bool claimed, run;
> > +    uint64_t val = 0;
> >
> >      if (ch >= SIFIVE_PDMA_CHANS) {
> >          qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid channel no %d\n",
> >                        __func__, ch);
> > -        return;
> > +        return 0;
> > +    }
> > +
> > +    switch (size) {
> > +    case 8:
> > +        val = sifive_pdma_readq(s, ch, offset);
> > +        break;
> > +    case 4:
> > +        val = sifive_pdma_readl(s, ch, offset);
> > +        break;
> > +    default:
> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid read size %u to
> PDMA\n",
> > +                      __func__, size);
> > +        return 0;
> >      }
> >
> > +    return val;
> > +}
> > +
> > +static void sifive_pdma_writeq(SiFivePDMAState *s, int ch,
> > +                               hwaddr offset, uint64_t value)
> > +{
> > +    offset &= 0xfff;
> > +    switch (offset) {
> > +    case DMA_NEXT_BYTES:
> > +        s->chan[ch].next_bytes = value;
> > +        break;
> > +    case DMA_NEXT_DST:
> > +        s->chan[ch].next_dst = value;
> > +        break;
> > +    case DMA_NEXT_SRC:
> > +        s->chan[ch].next_src = value;
> > +        break;
> > +    case DMA_EXEC_BYTES:
> > +    case DMA_EXEC_DST:
> > +    case DMA_EXEC_SRC:
> > +        /* these are read-only registers */
> > +        break;
> > +    default:
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: Unexpected 64-bit access to 0x%" HWADDR_PRIX
> "\n",
> > +                      __func__, offset);
> > +        break;
> > +    }
> > +}
> > +
> > +static void sifive_pdma_writel(SiFivePDMAState *s, int ch,
> > +                               hwaddr offset, uint32_t value)
> > +{
> > +    bool claimed, run;
> > +
> >      offset &= 0xfff;
> >      switch (offset) {
> >      case DMA_CONTROL:
> > @@ -282,13 +374,24 @@ static void sifive_pdma_write(void *opaque, hwaddr
> offset,
> >          s->chan[ch].next_config = value;
> >          break;
> >      case DMA_NEXT_BYTES:
> > -        s->chan[ch].next_bytes = value;
> > +        s->chan[ch].next_bytes =
> > +            deposit64(s->chan[ch].next_bytes, 0, 32, value);
> > +        break;
> > +    case DMA_NEXT_BYTES + 4:
> > +        s->chan[ch].next_bytes =
> > +            deposit64(s->chan[ch].next_bytes, 32, 32, value);
> >          break;
> >      case DMA_NEXT_DST:
> > -        s->chan[ch].next_dst = value;
> > +        s->chan[ch].next_dst = deposit64(s->chan[ch].next_dst, 0, 32,
> value);
> > +        break;
> > +    case DMA_NEXT_DST + 4:
> > +        s->chan[ch].next_dst = deposit64(s->chan[ch].next_dst, 32, 32,
> value);
> >          break;
> >      case DMA_NEXT_SRC:
> > -        s->chan[ch].next_src = value;
> > +        s->chan[ch].next_src = deposit64(s->chan[ch].next_src, 0, 32,
> value);
> > +        break;
> > +    case DMA_NEXT_SRC + 4:
> > +        s->chan[ch].next_src = deposit64(s->chan[ch].next_src, 32, 32,
> value);
> >          break;
> >      case DMA_EXEC_CONFIG:
> >      case DMA_EXEC_BYTES:
>
> The high 32-bit of exec_bytes, exec_dst and exec_src should be added
> to the "read-only" break branch, instead of falling through to the
> default branch.
>
> > @@ -297,12 +400,39 @@ static void sifive_pdma_write(void *opaque, hwaddr
> offset,
> >          /* these are read-only registers */
> >          break;
> >      default:
> > -        qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIX
> "\n",
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: Unexpected 32-bit access to 0x%" HWADDR_PRIX
> "\n",
> >                        __func__, offset);
> >          break;
> >      }
> >  }
> >
> > +static void sifive_pdma_write(void *opaque, hwaddr offset,
> > +                              uint64_t value, unsigned size)
> > +{
> > +    SiFivePDMAState *s = opaque;
> > +    int ch = SIFIVE_PDMA_CHAN_NO(offset);
> > +
> > +    if (ch >= SIFIVE_PDMA_CHANS) {
> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid channel no %d\n",
> > +                      __func__, ch);
> > +        return;
> > +    }
> > +
> > +    switch (size) {
> > +    case 8:
> > +        sifive_pdma_writeq(s, ch, offset, value);
> > +        break;
> > +    case 4:
> > +        sifive_pdma_writel(s, ch, offset, (uint32_t) value);
> > +        break;
> > +    default:
> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid write size %u to
> PDMA\n",
> > +                      __func__, size);
> > +        break;
> > +    }
> > +}
> > +
> >  static const MemoryRegionOps sifive_pdma_ops = {
> >      .read = sifive_pdma_read,
> >      .write = sifive_pdma_write,
> > --
>
> Otherwise, tested with U-Boot sifive_unleashed_defconfig,
>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> Tested-by: Bin Meng <bmeng.cn@gmail.com>
>

[-- Attachment #2: Type: text/html, Size: 16197 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] hw/dma: sifive_pdma: support high 32-bit access of 64-bit register
@ 2022-01-04  5:53       ` Jim Shu
  0 siblings, 0 replies; 15+ messages in thread
From: Jim Shu @ 2022-01-04  5:53 UTC (permalink / raw)
  To: Bin Meng
  Cc: Alistair Francis, Bin Meng, Palmer Dabbelt, Frank Chang,
	open list:RISC-V, qemu-devel@nongnu.org Developers

[-- Attachment #1: Type: text/plain, Size: 12195 bytes --]

Hi Bin,

Thanks for the review.
I will fix the commit log and the behavior of writing high 32-bit of RO
registers in v2 patch.


Thanks,
Jim Shu

On Tue, Jan 4, 2022 at 10:55 AM Bin Meng <bmeng.cn@gmail.com> wrote:

> Hi Jim,
>
> On Tue, Dec 28, 2021 at 8:53 AM Jim Shu <jim.shu@sifive.com> wrote:
> >
> > Real PDMA support high 32-bit read/write memory access of 64-bit
>
> %s/support/supports
>
> > register.
> >
> > The following result is PDMA tested in U-Boot on Unmatched board:
> >
> > 1. Real PDMA is allowed high 32-bit read/write to 64-bit register.
>
> %s/is allowed/allows
>
> > => mw.l 0x3000000 0x0                      <= Disclaim channel 0
> > => mw.l 0x3000000 0x1                      <= Claim channel 0
> > => mw.l 0x3000010 0x80000000               <= Write low 32-bit NextDest
> (NextDest = 0x280000000)
> > => mw.l 0x3000014 0x2                      <= Write high 32-bit NextDest
> > => md.l 0x3000010 1                        <= Dump low 32-bit NextDest
> > 03000010: 80000000
> > => md.l 0x3000014 1                        <= Dump high 32-bit NextDest
> > 03000014: 00000002
> > => mw.l 0x3000018 0x80001000               <= Write low 32-bit NextSrc
> (NextSrc = 0x280001000)
> > => mw.l 0x300001c 0x2                      <= Write high 32-bit NextSrc
> > => md.l 0x3000018 1                        <= Dump low 32-bit NextSrc
> > 03000010: 80001000
> > => md.l 0x300001c 1                        <= Dump high 32-bit NextSrc
> > 03000014: 00000002
> >
> > 2. PDMA transfer from 0x280001000 to 0x280000000 is OK.
> > => mw.q 0x3000008 0x4                      <= NextBytes = 4
> > => mw.l 0x3000004 0x22000000               <= wsize = rsize = 2 (2^2 = 4
> bytes)
> > => mw.l 0x280000000 0x87654321             <= Fill test data to dst
> > => mw.l 0x280001000 0x12345678             <= Fill test data to src
> > => md.l 0x280000000 1; md.l 0x280001000 1  <= Dump src/dst memory
> contents
> > 280000000: 87654321                              !Ce.
> > 280001000: 12345678                              xV4.
> > => md.l 0x3000000 8                        <= Dump PDMA status
> > 03000000: 00000001 22000000 00000004 00000000    ......."........
> > 03000010: 80000000 00000002 80001000 00000002    ................
> > => mw.l 0x3000000 0x3                      <= Set channel 0 run and
> claim bits
> > => md.l 0x3000000 8                        <= Dump PDMA status
> > 03000000: 40000001 22000000 00000004 00000000    ...@..."........
> > 03000010: 80000000 00000002 80001000 00000002    ................
> > => md.l 0x280000000 1; md.l 0x280001000 1  <= Dump src/dst memory
> contents
> > 280000000: 12345678                               xV4.
> > 280001000: 12345678                               xV4.
> >
> > Signed-off-by: Jim Shu <jim.shu@sifive.com>
> > Reviewed-by: Frank Chang <frank.chang@sifive.com>
> > ---
> >  hw/dma/sifive_pdma.c | 174 +++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 152 insertions(+), 22 deletions(-)
> >
> > diff --git a/hw/dma/sifive_pdma.c b/hw/dma/sifive_pdma.c
> > index 85fe34f5f3..b8b198ab4e 100644
> > --- a/hw/dma/sifive_pdma.c
> > +++ b/hw/dma/sifive_pdma.c
> > @@ -177,18 +177,44 @@ static inline void
> sifive_pdma_update_irq(SiFivePDMAState *s, int ch)
> >      s->chan[ch].state = DMA_CHAN_STATE_IDLE;
> >  }
> >
> > -static uint64_t sifive_pdma_read(void *opaque, hwaddr offset, unsigned
> size)
> > +static uint64_t sifive_pdma_readq(SiFivePDMAState *s, int ch, hwaddr
> offset)
> >  {
> > -    SiFivePDMAState *s = opaque;
> > -    int ch = SIFIVE_PDMA_CHAN_NO(offset);
> >      uint64_t val = 0;
> >
> > -    if (ch >= SIFIVE_PDMA_CHANS) {
> > -        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid channel no %d\n",
> > -                      __func__, ch);
> > -        return 0;
> > +    offset &= 0xfff;
> > +    switch (offset) {
> > +    case DMA_NEXT_BYTES:
> > +        val = s->chan[ch].next_bytes;
> > +        break;
> > +    case DMA_NEXT_DST:
> > +        val = s->chan[ch].next_dst;
> > +        break;
> > +    case DMA_NEXT_SRC:
> > +        val = s->chan[ch].next_src;
> > +        break;
> > +    case DMA_EXEC_BYTES:
> > +        val = s->chan[ch].exec_bytes;
> > +        break;
> > +    case DMA_EXEC_DST:
> > +        val = s->chan[ch].exec_dst;
> > +        break;
> > +    case DMA_EXEC_SRC:
> > +        val = s->chan[ch].exec_src;
> > +        break;
> > +    default:
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: Unexpected 64-bit access to 0x%" HWADDR_PRIX
> "\n",
> > +                      __func__, offset);
> > +        break;
> >      }
> >
> > +    return val;
> > +}
> > +
> > +static uint32_t sifive_pdma_readl(SiFivePDMAState *s, int ch, hwaddr
> offset)
> > +{
> > +    uint32_t val = 0;
> > +
> >      offset &= 0xfff;
> >      switch (offset) {
> >      case DMA_CONTROL:
> > @@ -198,28 +224,47 @@ static uint64_t sifive_pdma_read(void *opaque,
> hwaddr offset, unsigned size)
> >          val = s->chan[ch].next_config;
> >          break;
> >      case DMA_NEXT_BYTES:
> > -        val = s->chan[ch].next_bytes;
> > +        val = extract64(s->chan[ch].next_bytes, 0, 32);
> > +        break;
> > +    case DMA_NEXT_BYTES + 4:
> > +        val = extract64(s->chan[ch].next_bytes, 32, 32);
> >          break;
> >      case DMA_NEXT_DST:
> > -        val = s->chan[ch].next_dst;
> > +        val = extract64(s->chan[ch].next_dst, 0, 32);
> > +        break;
> > +    case DMA_NEXT_DST + 4:
> > +        val = extract64(s->chan[ch].next_dst, 32, 32);
> >          break;
> >      case DMA_NEXT_SRC:
> > -        val = s->chan[ch].next_src;
> > +        val = extract64(s->chan[ch].next_src, 0, 32);
> > +        break;
> > +    case DMA_NEXT_SRC + 4:
> > +        val = extract64(s->chan[ch].next_src, 32, 32);
> >          break;
> >      case DMA_EXEC_CONFIG:
> >          val = s->chan[ch].exec_config;
> >          break;
> >      case DMA_EXEC_BYTES:
> > -        val = s->chan[ch].exec_bytes;
> > +        val = extract64(s->chan[ch].exec_bytes, 0, 32);
> > +        break;
> > +    case DMA_EXEC_BYTES + 4:
> > +        val = extract64(s->chan[ch].exec_bytes, 32, 32);
> >          break;
> >      case DMA_EXEC_DST:
> > -        val = s->chan[ch].exec_dst;
> > +        val = extract64(s->chan[ch].exec_dst, 0, 32);
> > +        break;
> > +    case DMA_EXEC_DST + 4:
> > +        val = extract64(s->chan[ch].exec_dst, 32, 32);
> >          break;
> >      case DMA_EXEC_SRC:
> > -        val = s->chan[ch].exec_src;
> > +        val = extract64(s->chan[ch].exec_src, 0, 32);
> > +        break;
> > +    case DMA_EXEC_SRC + 4:
> > +        val = extract64(s->chan[ch].exec_src, 32, 32);
> >          break;
> >      default:
> > -        qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIX
> "\n",
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: Unexpected 32-bit access to 0x%" HWADDR_PRIX
> "\n",
> >                        __func__, offset);
> >          break;
> >      }
> > @@ -227,19 +272,66 @@ static uint64_t sifive_pdma_read(void *opaque,
> hwaddr offset, unsigned size)
> >      return val;
> >  }
> >
> > -static void sifive_pdma_write(void *opaque, hwaddr offset,
> > -                              uint64_t value, unsigned size)
> > +static uint64_t sifive_pdma_read(void *opaque, hwaddr offset, unsigned
> size)
> >  {
> >      SiFivePDMAState *s = opaque;
> >      int ch = SIFIVE_PDMA_CHAN_NO(offset);
> > -    bool claimed, run;
> > +    uint64_t val = 0;
> >
> >      if (ch >= SIFIVE_PDMA_CHANS) {
> >          qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid channel no %d\n",
> >                        __func__, ch);
> > -        return;
> > +        return 0;
> > +    }
> > +
> > +    switch (size) {
> > +    case 8:
> > +        val = sifive_pdma_readq(s, ch, offset);
> > +        break;
> > +    case 4:
> > +        val = sifive_pdma_readl(s, ch, offset);
> > +        break;
> > +    default:
> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid read size %u to
> PDMA\n",
> > +                      __func__, size);
> > +        return 0;
> >      }
> >
> > +    return val;
> > +}
> > +
> > +static void sifive_pdma_writeq(SiFivePDMAState *s, int ch,
> > +                               hwaddr offset, uint64_t value)
> > +{
> > +    offset &= 0xfff;
> > +    switch (offset) {
> > +    case DMA_NEXT_BYTES:
> > +        s->chan[ch].next_bytes = value;
> > +        break;
> > +    case DMA_NEXT_DST:
> > +        s->chan[ch].next_dst = value;
> > +        break;
> > +    case DMA_NEXT_SRC:
> > +        s->chan[ch].next_src = value;
> > +        break;
> > +    case DMA_EXEC_BYTES:
> > +    case DMA_EXEC_DST:
> > +    case DMA_EXEC_SRC:
> > +        /* these are read-only registers */
> > +        break;
> > +    default:
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: Unexpected 64-bit access to 0x%" HWADDR_PRIX
> "\n",
> > +                      __func__, offset);
> > +        break;
> > +    }
> > +}
> > +
> > +static void sifive_pdma_writel(SiFivePDMAState *s, int ch,
> > +                               hwaddr offset, uint32_t value)
> > +{
> > +    bool claimed, run;
> > +
> >      offset &= 0xfff;
> >      switch (offset) {
> >      case DMA_CONTROL:
> > @@ -282,13 +374,24 @@ static void sifive_pdma_write(void *opaque, hwaddr
> offset,
> >          s->chan[ch].next_config = value;
> >          break;
> >      case DMA_NEXT_BYTES:
> > -        s->chan[ch].next_bytes = value;
> > +        s->chan[ch].next_bytes =
> > +            deposit64(s->chan[ch].next_bytes, 0, 32, value);
> > +        break;
> > +    case DMA_NEXT_BYTES + 4:
> > +        s->chan[ch].next_bytes =
> > +            deposit64(s->chan[ch].next_bytes, 32, 32, value);
> >          break;
> >      case DMA_NEXT_DST:
> > -        s->chan[ch].next_dst = value;
> > +        s->chan[ch].next_dst = deposit64(s->chan[ch].next_dst, 0, 32,
> value);
> > +        break;
> > +    case DMA_NEXT_DST + 4:
> > +        s->chan[ch].next_dst = deposit64(s->chan[ch].next_dst, 32, 32,
> value);
> >          break;
> >      case DMA_NEXT_SRC:
> > -        s->chan[ch].next_src = value;
> > +        s->chan[ch].next_src = deposit64(s->chan[ch].next_src, 0, 32,
> value);
> > +        break;
> > +    case DMA_NEXT_SRC + 4:
> > +        s->chan[ch].next_src = deposit64(s->chan[ch].next_src, 32, 32,
> value);
> >          break;
> >      case DMA_EXEC_CONFIG:
> >      case DMA_EXEC_BYTES:
>
> The high 32-bit of exec_bytes, exec_dst and exec_src should be added
> to the "read-only" break branch, instead of falling through to the
> default branch.
>
> > @@ -297,12 +400,39 @@ static void sifive_pdma_write(void *opaque, hwaddr
> offset,
> >          /* these are read-only registers */
> >          break;
> >      default:
> > -        qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIX
> "\n",
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: Unexpected 32-bit access to 0x%" HWADDR_PRIX
> "\n",
> >                        __func__, offset);
> >          break;
> >      }
> >  }
> >
> > +static void sifive_pdma_write(void *opaque, hwaddr offset,
> > +                              uint64_t value, unsigned size)
> > +{
> > +    SiFivePDMAState *s = opaque;
> > +    int ch = SIFIVE_PDMA_CHAN_NO(offset);
> > +
> > +    if (ch >= SIFIVE_PDMA_CHANS) {
> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid channel no %d\n",
> > +                      __func__, ch);
> > +        return;
> > +    }
> > +
> > +    switch (size) {
> > +    case 8:
> > +        sifive_pdma_writeq(s, ch, offset, value);
> > +        break;
> > +    case 4:
> > +        sifive_pdma_writel(s, ch, offset, (uint32_t) value);
> > +        break;
> > +    default:
> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid write size %u to
> PDMA\n",
> > +                      __func__, size);
> > +        break;
> > +    }
> > +}
> > +
> >  static const MemoryRegionOps sifive_pdma_ops = {
> >      .read = sifive_pdma_read,
> >      .write = sifive_pdma_write,
> > --
>
> Otherwise, tested with U-Boot sifive_unleashed_defconfig,
>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> Tested-by: Bin Meng <bmeng.cn@gmail.com>
>

[-- Attachment #2: Type: text/html, Size: 16197 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2022-01-04  5:56 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-28  0:52 [PATCH 0/2] Align SiFive PDMA behavior to real hardware Jim Shu
2021-12-28  0:52 ` [PATCH 1/2] hw/dma: sifive_pdma: support high 32-bit access of 64-bit register Jim Shu
2021-12-28 10:33   ` Philippe Mathieu-Daudé
2022-01-03 22:45   ` Alistair Francis
2022-01-03 22:45     ` Alistair Francis
2022-01-04  2:55   ` Bin Meng
2022-01-04  2:55     ` Bin Meng
2022-01-04  5:53     ` Jim Shu
2022-01-04  5:53       ` Jim Shu
2021-12-28  0:52 ` [PATCH 2/2] hw/dma: sifive_pdma: permit 4/8-byte access size of PDMA registers Jim Shu
2021-12-28 10:35   ` Philippe Mathieu-Daudé
2022-01-03 22:46   ` Alistair Francis
2022-01-03 22:46     ` Alistair Francis
2022-01-04  2:56   ` Bin Meng
2022-01-04  2:56     ` Bin Meng

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.