[PATCH v2 00/11] hw/usb/aspeed-udc: Add AST1030 UDC support

[PATCH v2 00/11] hw/usb/aspeed-udc: Add AST1030 UDC support

[Jamin Lin]

This series adds USB Device Controller (UDC) support for the ASPEED
AST1030 and extends the existing ASPEED UDC model with the transfer
handling required by the AST1030.

The v1 series included both the usbredir server transport and the ASPEED
AST1030 UDC support. Following review feedback, the two parts have been
split into separate series. The usbredir server transport is now sent
separately, while this series contains the ASPEED AST1030 UDC changes.

Dependency
==========

The AST1030 UDC implementation itself does not depend on the usbredir
server transport series. However, the functional test in this series
does depend on it, because the AST1030 has no USB host controller and
requires usb-redir-server to export the emulated USB gadget to a USB
host controller in another QEMU instance.

The functional test therefore depends on:

[PATCH v2 0/8] hw/usb: Add a usbredir server transport
https://patchwork.kernel.org/project/qemu-devel/cover/20261005032022.3980903-1-jamin_lin@aspeedtech.com/

Background
==========

This work is part of a larger plan to model USB device-side support on
ASPEED BMC/BIC SoCs, which has three goals:

1. Model the ASPEED UDC (AST2600 / AST1030). The AST2600 also has USB host
   (EHCI) controllers, so that work targets the AST2600 UDC: its gadget can
   be attached to the SoC's own EHCI bus, letting the guest enumerate its
   own gadget and exercise the UDC end-to-end.  [Done]
   https://lore.kernel.org/qemu-devel/20260902021542.3194812-1-jamin_lin@aspeedtech.com/

2. AST1030 UDC. The AST1030 has no USB host controller, so testing its UDC
   needs a second QEMU instance. The UDC gadget is redirected out of the
   guest with libusbredir and attached to another QEMU that runs a USB host
   (a VMM, or an AST2600 / AST2700 guest).  [this series]

3. ASPEED vHub, as a longer-term goal towards BMC KVM / Virtual Media
   support in QEMU.  [future]

This series implements goal 2.

AST1030 gadget to an AST2700 host
=================================

        QEMU #1  -M ast1030-evb              QEMU #2  -M ast2700a2-evb
  +-----------------------------+      +-----------------------------+
  | Zephyr   usb_dc_aspeed      |      | Linux    ehci-platform      |
  +--------------+--------------+      +--------------+--------------+
                 | MMIO                               | MMIO
  +--------------v--------------+      +--------------v--------------+
  | aspeed.udc                  |      | EHCI3        (usb-bus.3)    |
  +--------------+--------------+      +--------------+--------------+
                 | USBPacket                          | USBPacket
  +--------------v--------------+      +--------------v--------------+
  | aspeed.udc-gadget           |      | usb-redir                   |
  +--------------+--------------+      +--------------+--------------+
                 | USBPacket                          |
  +--------------v--------------+                     |
  | usb-redir-server            |                     |
  +--------------+--------------+                     |
                 |                                    |
                 +=========== unix socket ============+
                           usbredir protocol

Testing
=======

The AST1030 UDC was tested with a Zephyr CDC-ACM gadget. The AST1030
side exports the gadget through usb-redir-server:

$ qemu-system-arm -M ast1030-evb -nographic
-kernel zephyr.bin
-chardev socket,id=u,path=/tmp/usbredir.sock,server=on,wait=off
-device usb-redir-server,id=udcredir,chardev=u
-device aspeed.udc-gadget,bus=udcredir.0,udc=/machine/soc/udc

uart:~$ usb enable
uart:~$ usb print 1

The gadget is attached to EHCI3 of an AST2700 guest through the
usb-redir client:

$ qemu-system-aarch64 -M ast2700a2-evb -nographic
-drive file=image-bmc,if=mtd,format=raw -snapshot
-net nic -net user
-chardev socket,id=r,path=/tmp/usbredir.sock
-device usb-redir,chardev=r,bus=usb-bus.3

The AST2700 guest enumerates the AST1030 gadget:

root@ast2700-default:~# lsusb
Bus 001 Device 001: ID 1d6b:0002 Linux 6.18.36 ehci_hcd EHCI Host Controller
Bus 001 Device 002: ID 2245:1030 ZEPHYR Zephyr DFU sample

Data transfer was also checked:

2700 -> 1030 
echo '1234567890' > /dev/ttyACM0
1030 console: Print Data: 0x31 0x32 ... 0x30

A functional test is added to exercise the AST1030 UDC through this
usbredir setup.

v1:
  1. Make the SETUP buffer byte addressable
  2. Support single-stage IN transfers
  3. Clear EP_POOL_ACK when no endpoint ack is left
  4. Mask the reserved bits on register writes
  5. Use the endpoint max packet size for OUT transfers
  6. Test the usbredir server transport
  7. Add the USB device controller for AST1030
  
v2:
  1. Split the usbredir server transport into a separate series as
     suggested during review.
  2. Keep this series focused on the ASPEED UDC changes and AST1030
     UDC support.
  3. Add the AST1030 functional test using the usbredir server transport.

Jamin Lin (11):
  hw/usb/aspeed-udc: Make the SETUP buffer byte addressable
  hw/usb/aspeed-udc: Mask the reserved bits on register writes
  hw/usb/aspeed-udc: Clear EP_POOL_ACK when no endpoint ack is left
  hw/usb/aspeed-udc: Rename the transfer helpers after their DMA mode
  hw/usb/aspeed-udc: Store the EP_DMA_STS write first
  hw/usb/aspeed-udc: Pick the EP_DMA_STS path by direction
  hw/usb/aspeed-udc: Support single-stage IN transfers
  hw/usb/aspeed-udc: Use the endpoint max packet size for OUT transfers
  hw/usb/aspeed-udc: Add trace events for the transfer paths
  hw/arm/aspeed_ast10x0: Add the USB device controller
  tests/functional/arm/test_aspeed_ast1030: Test the usbredir server
    transport

 include/hw/arm/aspeed_soc.h                 |   1 +
 include/hw/usb/aspeed-udc.h                 |  13 +-
 hw/arm/aspeed_ast10x0.c                     |  17 +-
 hw/usb/aspeed-udc.c                         | 290 ++++++++++++++++----
 hw/usb/trace-events                         |   9 +
 tests/functional/arm/meson.build            |   1 +
 tests/functional/arm/test_aspeed_ast1030.py |  52 ++++
 7 files changed, 330 insertions(+), 53 deletions(-)

-- 
2.43.0

[PATCH v2 01/11] hw/usb/aspeed-udc: Make the SETUP buffer byte addressable

[Jamin Lin]

The controller registers need a 4 byte access, but the 8 bytes at 0x80
are not registers. They are a SRAM buffer holding the EP0 SETUP packet.
A read or a write that is not 4 bytes returned the wrong data, so the
driver saw a broken SETUP packet.

Introduce a separate SETUP MMIO region that handles 1, 2 and 4 byte
reads and writes.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 include/hw/usb/aspeed-udc.h | 11 ++++++--
 hw/usb/aspeed-udc.c         | 56 +++++++++++++++++++++++++++++++++----
 hw/usb/trace-events         |  2 ++
 3 files changed, 61 insertions(+), 8 deletions(-)

diff --git a/include/hw/usb/aspeed-udc.h b/include/hw/usb/aspeed-udc.h
index 7701c1aa34..42916c0bd6 100644
--- a/include/hw/usb/aspeed-udc.h
+++ b/include/hw/usb/aspeed-udc.h
@@ -24,11 +24,14 @@ OBJECT_DECLARE_SIMPLE_TYPE(AspeedUDCState, ASPEED_UDC)
 OBJECT_DECLARE_SIMPLE_TYPE(AspeedUDCGadget, ASPEED_UDC_GADGET)
 
 /*
- * Register map: root/global block at 0x000 - 0x087, then one 0x10 byte bank
- * per programmable endpoint from 0x200.
+ * Register map: root/global block at 0x000 - 0x07f, the SETUP buffer at
+ * 0x080 - 0x087, then one 0x10 byte bank per programmable endpoint from
+ * 0x200.
  */
 #define ASPEED_UDC_MEM_SIZE     0x300
-#define ASPEED_UDC_ROOT_NR_REGS (0x88 >> 2)
+#define ASPEED_UDC_ROOT_NR_REGS (0x80 >> 2)
+#define ASPEED_UDC_SETUP_BASE   0x80
+#define ASPEED_UDC_SETUP_SIZE   0x08
 #define ASPEED_UDC_EP_REG_BASE  0x200
 #define ASPEED_UDC_EP_NR_REGS   (0x10 >> 2)
 
@@ -62,9 +65,11 @@ struct AspeedUDCState {
 
     MemoryRegion udc_container;
     MemoryRegion root_mr;
+    MemoryRegion setup_mr;
     MemoryRegion *dram_mr;
     AddressSpace dram_as;
     uint32_t regs[ASPEED_UDC_ROOT_NR_REGS];
+    uint8_t setup_buf[ASPEED_UDC_SETUP_SIZE];
     AspeedUDCEP ep[ASPEED_UDC_NUM_EP];
     qemu_irq irq;
 
diff --git a/hw/usb/aspeed-udc.c b/hw/usb/aspeed-udc.c
index 4c90b4f4c2..355512822b 100644
--- a/hw/usb/aspeed-udc.c
+++ b/hw/usb/aspeed-udc.c
@@ -58,9 +58,6 @@ REG32(UDC_EP0_CTRL, 0x30)
     FIELD(UDC_EP0_CTRL, STALL,              0, 1)
 REG32(UDC_EP0_DATA_BUFF, 0x34)
     FIELD(UDC_EP0_DATA_BUFF, BASE_ADDR,     0, 31)
-/* EP0 SETUP packet buffer: SETUP0 = bytes 0...3, SETUP1 = bytes 4...7 */
-REG32(UDC_SETUP0, 0x80)
-REG32(UDC_SETUP1, 0x84)
 
 /* Per programmable-endpoint registers (offset from the EP register base) */
 REG32(EP_CONFIG, 0x00)
@@ -363,6 +360,45 @@ static const MemoryRegionOps aspeed_udc_ops = {
     },
 };
 
+static uint64_t aspeed_udc_setup_read(void *opaque, hwaddr offset,
+                                      unsigned size)
+{
+    AspeedUDCState *s = ASPEED_UDC(opaque);
+    uint64_t value = 0;
+    int i;
+
+    for (i = 0; i < size; i++) {
+        value |= (uint64_t)s->setup_buf[offset + i] << (8 * i);
+    }
+
+    trace_aspeed_udc_setup_read(offset, size, value);
+
+    return value;
+}
+
+static void aspeed_udc_setup_write(void *opaque, hwaddr offset,
+                                   uint64_t value, unsigned size)
+{
+    AspeedUDCState *s = ASPEED_UDC(opaque);
+    int i;
+
+    trace_aspeed_udc_setup_write(offset, size, value);
+
+    for (i = 0; i < size; i++) {
+        s->setup_buf[offset + i] = value >> (8 * i);
+    }
+}
+
+static const MemoryRegionOps aspeed_udc_setup_ops = {
+    .read = aspeed_udc_setup_read,
+    .write = aspeed_udc_setup_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .valid = {
+        .min_access_size = 1,
+        .max_access_size = 4,
+    },
+};
+
 /*
  * Copy len bytes from guest memory at addr into the IN packet, going through
  * a bounce buffer one buf-full at a time. Returns false on DMA failure.
@@ -708,6 +744,7 @@ static void aspeed_udc_reset_hold(Object *obj, ResetType type)
     int i;
 
     memset(s->regs, 0, sizeof(s->regs));
+    memset(s->setup_buf, 0, sizeof(s->setup_buf));
     for (i = 0; i < ASPEED_UDC_NUM_EP; i++) {
         memset(s->ep[i].regs, 0, sizeof(s->ep[i].regs));
         s->ep[i].pkt = NULL;
@@ -754,6 +791,12 @@ static void aspeed_udc_realize(DeviceState *dev, Error **errp)
                           ASPEED_UDC_ROOT_NR_REGS << 2);
     memory_region_add_subregion(&s->udc_container, 0, &s->root_mr);
 
+    memory_region_init_io(&s->setup_mr, OBJECT(s), &aspeed_udc_setup_ops,
+                          s, TYPE_ASPEED_UDC ".setup",
+                          ASPEED_UDC_SETUP_SIZE);
+    memory_region_add_subregion(&s->udc_container,
+                                ASPEED_UDC_SETUP_BASE, &s->setup_mr);
+
     /* Each programmable endpoint has its own register bank */
     for (i = 0; i < ASPEED_UDC_NUM_EP; i++) {
         g_autofree char *name = g_strdup_printf(TYPE_ASPEED_UDC ".ep%d", i);
@@ -918,8 +961,11 @@ static void aspeed_udc_gadget_handle_control(USBDevice *udev, USBPacket *p,
      * Reconstruct the 8-byte SETUP packet into the SETUP data buffer where
      * the guest gadget driver reads it from.
      */
-    s->regs[R_UDC_SETUP0] = type | (req << 8) | ((value & 0xffff) << 16);
-    s->regs[R_UDC_SETUP1] = (index & 0xffff) | ((length & 0xffff) << 16);
+    s->setup_buf[0] = type;
+    s->setup_buf[1] = req;
+    stw_le_p(&s->setup_buf[2], value);
+    stw_le_p(&s->setup_buf[4], index);
+    stw_le_p(&s->setup_buf[6], length);
 
     /* A new SETUP clears the EP0 STALL condition */
     s->regs[R_UDC_EP0_CTRL] &= ~R_UDC_EP0_CTRL_STALL_MASK;
diff --git a/hw/usb/trace-events b/hw/usb/trace-events
index 80ead23358..aa260dacad 100644
--- a/hw/usb/trace-events
+++ b/hw/usb/trace-events
@@ -381,6 +381,8 @@ canokey_unrealize(void)
 # aspeed-udc.c
 aspeed_udc_read(uint64_t offset, uint32_t value) "offset 0x%" PRIx64 " value 0x%x"
 aspeed_udc_write(uint64_t offset, uint32_t value) "offset 0x%" PRIx64 " value 0x%x"
+aspeed_udc_setup_read(uint64_t offset, unsigned size, uint64_t value) "offset 0x%" PRIx64 " size %u value 0x%" PRIx64
+aspeed_udc_setup_write(uint64_t offset, unsigned size, uint64_t value) "offset 0x%" PRIx64 " size %u value 0x%" PRIx64
 aspeed_udc_ep_read(int ep, uint64_t offset, uint32_t value) "ep %d, offset 0x%" PRIx64 " value 0x%x"
 aspeed_udc_ep_write(int ep, uint64_t offset, uint32_t value) "ep %d, offset 0x%" PRIx64 " value 0x%x"
 aspeed_udc_pullup(int on, int attached) "on %d, attached %d"
-- 
2.43.0

[PATCH v2 02/11] hw/usb/aspeed-udc: Mask the reserved bits on register writes

[Jamin Lin]

Writes to the interrupt registers were not masked, so a guest could set
a reserved bit and read it back. The hardware returns 0 for those.

Mask each one to the bits the datasheet marks read/write:

  UDC_IER         17, 16, 8:6, 4:0
  UDC_ISR         8:6, 4:0
  UDC_EP_ACK_IER  3:0
  UDC_EP_NAK_IER  3:0
  UDC_EP_ACK_ISR  3:0
  UDC_EP_NAK_ISR  3:0

UDC_ISR bits 17 and 16 summarise the endpoint pool. They are read only,
so a write cannot clear them.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/usb/aspeed-udc.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/hw/usb/aspeed-udc.c b/hw/usb/aspeed-udc.c
index 355512822b..95fe823534 100644
--- a/hw/usb/aspeed-udc.c
+++ b/hw/usb/aspeed-udc.c
@@ -319,14 +319,24 @@ static void aspeed_udc_write(void *opaque, hwaddr offset, uint64_t data,
         s->regs[R_UDC_FUNC_CTRL] = val;
         break;
     case R_UDC_IER:
+        val &= 0x000301df;
+        s->regs[reg] = val;
+        aspeed_udc_update_irq(s);
+        break;
+    case R_UDC_ISR:
+        val &= 0x000001df;
+        s->regs[reg] &= ~val;
+        aspeed_udc_update_irq(s);
+        break;
     case R_UDC_EP_ACK_IER:
     case R_UDC_EP_NAK_IER:
+        val &= 0x0000000f;
         s->regs[reg] = val;
         aspeed_udc_update_irq(s);
         break;
-    case R_UDC_ISR:
     case R_UDC_EP_ACK_ISR:
     case R_UDC_EP_NAK_ISR:
+        val &= 0x0000000f;
         s->regs[reg] &= ~val;
         aspeed_udc_update_irq(s);
         break;
-- 
2.43.0

[PATCH v2 03/11] hw/usb/aspeed-udc: Clear EP_POOL_ACK when no endpoint ack is left

[Jamin Lin]

UDC_ISR.EP_POOL_ACK says that at least one endpoint in UDC_EP_ACK_ISR
has an ack. It is read only, so a write cannot clear it.

Clear it when bits 3:0 of UDC_EP_ACK_ISR are all 0, which means no
programmable endpoint has an ack.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/usb/aspeed-udc.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/usb/aspeed-udc.c b/hw/usb/aspeed-udc.c
index 95fe823534..bdbd1a78b8 100644
--- a/hw/usb/aspeed-udc.c
+++ b/hw/usb/aspeed-udc.c
@@ -335,6 +335,13 @@ static void aspeed_udc_write(void *opaque, hwaddr offset, uint64_t data,
         aspeed_udc_update_irq(s);
         break;
     case R_UDC_EP_ACK_ISR:
+        val &= 0x0000000f;
+        s->regs[reg] &= ~val;
+        if (!s->regs[R_UDC_EP_ACK_ISR]) {
+            s->regs[R_UDC_ISR] &= ~R_UDC_ISR_EP_POOL_ACK_MASK;
+        }
+        aspeed_udc_update_irq(s);
+        break;
     case R_UDC_EP_NAK_ISR:
         val &= 0x0000000f;
         s->regs[reg] &= ~val;
-- 
2.43.0

[PATCH v2 04/11] hw/usb/aspeed-udc: Rename the transfer helpers after their DMA mode

[Jamin Lin]

The controller can run an endpoint in descriptor list mode or in single
stage mode. Put the mode in the helper names. That makes the code
easier to read.

No functional change.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/usb/aspeed-udc.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/hw/usb/aspeed-udc.c b/hw/usb/aspeed-udc.c
index bdbd1a78b8..b43f077c2d 100644
--- a/hw/usb/aspeed-udc.c
+++ b/hw/usb/aspeed-udc.c
@@ -459,8 +459,8 @@ static bool aspeed_udc_ep_copy_to_pkt(AspeedUDCState *s, int ep, uint32_t addr,
  * This function raises the endpoint ACK by itself when the ring becomes empty
  * or when a descriptor asks for an interrupt.
  */
-static AspeedUDCXferResult aspeed_udc_ep_xfer_in(AspeedUDCState *s, int ep,
-                                                 USBPacket *p)
+static AspeedUDCXferResult aspeed_udc_ep_xfer_in_desc(AspeedUDCState *s,
+                                                      int ep, USBPacket *p)
 {
     QEMUIOVector *pktiov = p->combined ? &p->combined->iov : &p->iov;
     AspeedUDCEP *e = &s->ep[ep];
@@ -568,15 +568,15 @@ static AspeedUDCXferResult aspeed_udc_ep_xfer_in(AspeedUDCState *s, int ep,
 
 /*
  * OUT transfer: receive data from the host by copying its OUT packet into the
- * buffer the guest gadget driver set up (single-stage mode).
+ * buffer the guest gadget driver set up.
  *
  * A host packet can be bigger than one buffer, so we copy at most PKT_SIZE
  * bytes per call, continuing from where the last call stopped
  * (p->actual_length). The caller keeps the packet parked until it is fully
  * copied.
  */
-static AspeedUDCXferResult aspeed_udc_ep_xfer_out(AspeedUDCState *s, int ep,
-                                                  USBPacket *p)
+static AspeedUDCXferResult aspeed_udc_ep_xfer_out_single(AspeedUDCState *s,
+                                                         int ep, USBPacket *p)
 {
     AspeedUDCEP *e = &s->ep[ep];
     uint32_t chunk = FIELD_EX32(e->regs[R_EP_DMA_STS], EP_DMA_STS, PKT_SIZE);
@@ -619,7 +619,7 @@ static AspeedUDCXferResult aspeed_udc_ep_xfer_out(AspeedUDCState *s, int ep,
  * If the request needs more data than was queued, keep it parked and wait for
  * the next kick.
  */
-static void aspeed_udc_ep_in_kick(AspeedUDCState *s, int ep, uint32_t val)
+static void aspeed_udc_ep_in_kick_desc(AspeedUDCState *s, int ep, uint32_t val)
 {
     AspeedUDCEP *e = &s->ep[ep];
     uint32_t cur_rptr = FIELD_EX32(e->regs[R_EP_DMA_STS], EP_DMA_STS, RPTR);
@@ -649,7 +649,7 @@ static void aspeed_udc_ep_in_kick(AspeedUDCState *s, int ep, uint32_t val)
         return;
     }
 
-    switch (aspeed_udc_ep_xfer_in(s, ep, p)) {
+    switch (aspeed_udc_ep_xfer_in_desc(s, ep, p)) {
     case ASPEED_UDC_XFER_DONE:
         e->pkt = NULL;
         p->status = USB_RET_SUCCESS;
@@ -671,7 +671,7 @@ static void aspeed_udc_ep_in_kick(AspeedUDCState *s, int ep, uint32_t val)
  * buffer before), copy its data into the buffer now and finish it. If the
  * packet has more data than fits, keep it parked and wait for the next buffer.
  */
-static void aspeed_udc_ep_out_kick(AspeedUDCState *s, int ep)
+static void aspeed_udc_ep_out_kick_single(AspeedUDCState *s, int ep)
 {
     AspeedUDCEP *e = &s->ep[ep];
     USBPacket *p = e->pkt;
@@ -681,7 +681,7 @@ static void aspeed_udc_ep_out_kick(AspeedUDCState *s, int ep)
         return;
     }
 
-    switch (aspeed_udc_ep_xfer_out(s, ep, p)) {
+    switch (aspeed_udc_ep_xfer_out_single(s, ep, p)) {
     case ASPEED_UDC_XFER_DONE:
         e->pkt = NULL;
         p->status = USB_RET_SUCCESS;
@@ -727,11 +727,11 @@ static void aspeed_udc_ep_write(void *opaque, hwaddr offset, uint64_t data,
         val &= 0x77ffffff;
         if (FIELD_EX32(e->regs[R_EP_DMA_CTRL], EP_DMA_CTRL, DESC_OP_EN)) {
             /* IN, descriptor-list mode */
-            aspeed_udc_ep_in_kick(s, e->index, val);
+            aspeed_udc_ep_in_kick_desc(s, e->index, val);
         } else {
             /* OUT, single-stage mode */
             e->regs[reg] = val;
-            aspeed_udc_ep_out_kick(s, e->index);
+            aspeed_udc_ep_out_kick_single(s, e->index);
         }
         break;
     default:
@@ -895,7 +895,7 @@ static void aspeed_udc_ep_data_in(AspeedUDCState *s, int ep, USBPacket *p)
         /*
          * No IN data is queued yet. Save the packet and return ASYNC
          * instead of NAK. A NAK would make the host retry slowly.
-         * aspeed_udc_ep_in_kick() serves and completes this packet later,
+         * aspeed_udc_ep_in_kick_desc() serves and completes this packet later,
          * once the guest gadget driver queues descriptors.
          */
         e->pkt = p;
@@ -903,7 +903,7 @@ static void aspeed_udc_ep_data_in(AspeedUDCState *s, int ep, USBPacket *p)
         return;
     }
 
-    switch (aspeed_udc_ep_xfer_in(s, ep, p)) {
+    switch (aspeed_udc_ep_xfer_in_desc(s, ep, p)) {
     case ASPEED_UDC_XFER_DONE:
         p->status = USB_RET_SUCCESS;
         break;
@@ -931,7 +931,7 @@ static void aspeed_udc_ep_data_out(AspeedUDCState *s, int ep, USBPacket *p)
          * No OUT buffer is ready yet. Save the packet and return ASYNC
          * instead of NAK. Writing now could use an old buffer address and
          * lose the data (for example a mass-storage CBW). A NAK would make
-         * the host retry slowly. aspeed_udc_ep_out_kick() delivers this
+         * the host retry slowly. aspeed_udc_ep_out_kick_single() delivers this
          * packet later, once the guest gadget driver sets up a buffer.
          */
         e->pkt = p;
@@ -939,7 +939,7 @@ static void aspeed_udc_ep_data_out(AspeedUDCState *s, int ep, USBPacket *p)
         return;
     }
 
-    switch (aspeed_udc_ep_xfer_out(s, ep, p)) {
+    switch (aspeed_udc_ep_xfer_out_single(s, ep, p)) {
     case ASPEED_UDC_XFER_DONE:
         p->status = USB_RET_SUCCESS;
         break;
-- 
2.43.0

[PATCH v2 05/11] hw/usb/aspeed-udc: Store the EP_DMA_STS write first

[Jamin Lin]

The other helpers run after EP_DMA_STS is written. The descriptor mode
kick did not: it had to run before the write, because it stored the
value itself.

Write the register first in every case, then call the helper. The order
is now the same everywhere, so a later change cannot go wrong on it,
and the code is easier to maintain.

No functional change.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/usb/aspeed-udc.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/hw/usb/aspeed-udc.c b/hw/usb/aspeed-udc.c
index b43f077c2d..4572914e16 100644
--- a/hw/usb/aspeed-udc.c
+++ b/hw/usb/aspeed-udc.c
@@ -619,12 +619,13 @@ static AspeedUDCXferResult aspeed_udc_ep_xfer_out_single(AspeedUDCState *s,
  * If the request needs more data than was queued, keep it parked and wait for
  * the next kick.
  */
-static void aspeed_udc_ep_in_kick_desc(AspeedUDCState *s, int ep, uint32_t val)
+static void aspeed_udc_ep_in_kick_desc(AspeedUDCState *s, int ep,
+                                       uint32_t old_val)
 {
     AspeedUDCEP *e = &s->ep[ep];
-    uint32_t cur_rptr = FIELD_EX32(e->regs[R_EP_DMA_STS], EP_DMA_STS, RPTR);
-    uint32_t new_rptr = FIELD_EX32(val, EP_DMA_STS, RPTR);
-    uint32_t new_wptr = FIELD_EX32(val, EP_DMA_STS, WPTR);
+    uint32_t new_rptr = FIELD_EX32(e->regs[R_EP_DMA_STS], EP_DMA_STS, RPTR);
+    uint32_t new_wptr = FIELD_EX32(e->regs[R_EP_DMA_STS], EP_DMA_STS, WPTR);
+    uint32_t cur_rptr = FIELD_EX32(old_val, EP_DMA_STS, RPTR);
     USBPacket *p = e->pkt;
 
     /*
@@ -641,8 +642,9 @@ static void aspeed_udc_ep_in_kick_desc(AspeedUDCState *s, int ep, uint32_t val)
         cur_rptr = new_rptr;
         e->desc_off = 0;
     }
-    /* store the guest's write, but keep our own read pointer */
-    e->regs[R_EP_DMA_STS] = FIELD_DP32(val, EP_DMA_STS, RPTR, cur_rptr);
+    /* keep our own read pointer */
+    e->regs[R_EP_DMA_STS] = FIELD_DP32(e->regs[R_EP_DMA_STS], EP_DMA_STS,
+                                       RPTR, cur_rptr);
 
     /* nothing to do unless an IN packet is waiting and the ring has data */
     if (!p || cur_rptr == new_wptr) {
@@ -715,6 +717,7 @@ static void aspeed_udc_ep_write(void *opaque, hwaddr offset, uint64_t data,
     AspeedUDCEP *e = opaque;
     AspeedUDCState *s = container_of(e - e->index, AspeedUDCState, ep[0]);
     uint32_t reg = offset >> 2;
+    uint32_t old_val = e->regs[reg];
     uint32_t val = data;
 
     trace_aspeed_udc_ep_write(e->index, offset, val);
@@ -725,12 +728,12 @@ static void aspeed_udc_ep_write(void *opaque, hwaddr offset, uint64_t data,
         break;
     case R_EP_DMA_STS:
         val &= 0x77ffffff;
+        e->regs[reg] = val;
         if (FIELD_EX32(e->regs[R_EP_DMA_CTRL], EP_DMA_CTRL, DESC_OP_EN)) {
             /* IN, descriptor-list mode */
-            aspeed_udc_ep_in_kick_desc(s, e->index, val);
+            aspeed_udc_ep_in_kick_desc(s, e->index, old_val);
         } else {
             /* OUT, single-stage mode */
-            e->regs[reg] = val;
             aspeed_udc_ep_out_kick_single(s, e->index);
         }
         break;
-- 
2.43.0

[PATCH v2 06/11] hw/usb/aspeed-udc: Pick the EP_DMA_STS path by direction

[Jamin Lin]

DESC_OP_EN is the DMA mode. It does not tell the direction. The write
handler used it as the direction anyway: set meant IN, clear meant OUT.

Both directions support both modes. So take the direction from
EP_CONFIG.DIR_OUT first, then take the mode from DESC_OP_EN.

The model does not support OUT descriptor mode. Log it as not
implemented. It does not support IN single stage mode yet either, so
that path does nothing until the next patch.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/usb/aspeed-udc.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/hw/usb/aspeed-udc.c b/hw/usb/aspeed-udc.c
index 4572914e16..c48e2c5f1b 100644
--- a/hw/usb/aspeed-udc.c
+++ b/hw/usb/aspeed-udc.c
@@ -729,12 +729,17 @@ static void aspeed_udc_ep_write(void *opaque, hwaddr offset, uint64_t data,
     case R_EP_DMA_STS:
         val &= 0x77ffffff;
         e->regs[reg] = val;
-        if (FIELD_EX32(e->regs[R_EP_DMA_CTRL], EP_DMA_CTRL, DESC_OP_EN)) {
-            /* IN, descriptor-list mode */
-            aspeed_udc_ep_in_kick_desc(s, e->index, old_val);
-        } else {
-            /* OUT, single-stage mode */
+        if (FIELD_EX32(e->regs[R_EP_CONFIG], EP_CONFIG, DIR_OUT)) {
+            if (FIELD_EX32(e->regs[R_EP_DMA_CTRL], EP_DMA_CTRL, DESC_OP_EN)) {
+                qemu_log_mask(LOG_UNIMP, "%s: ep %d OUT descriptor mode is not"
+                              " implemented\n", __func__, e->index);
+                break;
+            }
             aspeed_udc_ep_out_kick_single(s, e->index);
+        } else {
+            if (FIELD_EX32(e->regs[R_EP_DMA_CTRL], EP_DMA_CTRL, DESC_OP_EN)) {
+                aspeed_udc_ep_in_kick_desc(s, e->index, old_val);
+            }
         }
         break;
     default:
-- 
2.43.0

[PATCH v2 07/11] hw/usb/aspeed-udc: Support single-stage IN transfers

[Jamin Lin]

The controller can send an IN endpoint's data from a descriptor ring or
from a single buffer. Only the descriptor ring was modelled, so an IN
endpoint in single stage mode sent nothing.

Add the path: one buffer at EP_DMA_BUFF, its length in PKT_SIZE, queued
by setting the write pointer. How to tell whether anything is queued
also depends on the mode. A descriptor ring is empty when its two
pointers are equal. A single stage buffer is queued by the write
pointer alone.

The buffer can hold more than the host asks for in one request, so keep
an offset into it. The rest goes out when the host asks again, and the
guest is told only once all of it has gone.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 include/hw/usb/aspeed-udc.h |   2 +
 hw/usb/aspeed-udc.c         | 108 ++++++++++++++++++++++++++++++++++--
 2 files changed, 106 insertions(+), 4 deletions(-)

diff --git a/include/hw/usb/aspeed-udc.h b/include/hw/usb/aspeed-udc.h
index 42916c0bd6..6665609501 100644
--- a/include/hw/usb/aspeed-udc.h
+++ b/include/hw/usb/aspeed-udc.h
@@ -53,6 +53,8 @@ typedef struct AspeedUDCEP {
     USBPacket *pkt;
     /* bytes of the current IN descriptor already served */
     uint32_t desc_off;
+    /* bytes of the single-stage IN buffer already sent */
+    uint32_t single_buf_off;
 } AspeedUDCEP;
 
 struct AspeedUDCGadget {
diff --git a/hw/usb/aspeed-udc.c b/hw/usb/aspeed-udc.c
index c48e2c5f1b..dcced1b8fb 100644
--- a/hw/usb/aspeed-udc.c
+++ b/hw/usb/aspeed-udc.c
@@ -566,6 +566,60 @@ static AspeedUDCXferResult aspeed_udc_ep_xfer_in_desc(AspeedUDCState *s,
     return done ? ASPEED_UDC_XFER_DONE : ASPEED_UDC_XFER_MORE;
 }
 
+/*
+ * IN transfer: the guest gadget driver put one buffer in EP_DMA_BUFF, its
+ * length in PKT_SIZE, and kicked it with the write pointer. Copy that
+ * buffer into the host's IN packet.
+ *
+ * The buffer can hold more than the host asked for. Copy what fits and
+ * remember how far we got in single_buf_off, so the rest goes out when
+ * the host asks again.
+ *
+ * A buffer smaller than the packet makes a short packet, and in USB a
+ * short packet ends the transfer.
+ */
+static AspeedUDCXferResult aspeed_udc_ep_xfer_in_single(AspeedUDCState *s,
+                                                        int ep, USBPacket *p)
+{
+    QEMUIOVector *pktiov = p->combined ? &p->combined->iov : &p->iov;
+    AspeedUDCEP *e = &s->ep[ep];
+    uint32_t buf_len = FIELD_EX32(e->regs[R_EP_DMA_STS], EP_DMA_STS, PKT_SIZE);
+    uint32_t left = buf_len > e->single_buf_off ?
+                    buf_len - e->single_buf_off : 0;
+    uint32_t pkt_space = pktiov->size > (uint32_t)p->actual_length ?
+                         pktiov->size - (uint32_t)p->actual_length : 0;
+    uint32_t addr = e->regs[R_EP_DMA_BUFF];
+    uint32_t len = MIN(left, pkt_space);
+
+    if (len && !aspeed_udc_ep_copy_to_pkt(s, ep, addr + e->single_buf_off,
+                                          len, p)) {
+        return ASPEED_UDC_XFER_ERROR;
+    }
+
+    e->single_buf_off += len;
+
+    /*
+     * The packet is full, but the buffer is not finished. Complete the
+     * packet. Do not ACK, and leave WPTR set: the guest still owns the
+     * buffer, and the next IN request sends the rest.
+     */
+    if (e->single_buf_off < buf_len) {
+        return ASPEED_UDC_XFER_DONE;
+    }
+
+    /* All of the buffer went out. Retire it and tell the driver. */
+    e->single_buf_off = 0;
+    e->regs[R_EP_DMA_STS] = FIELD_DP32(e->regs[R_EP_DMA_STS], EP_DMA_STS,
+                                       PKT_SIZE, buf_len);
+    e->regs[R_EP_DMA_STS] = FIELD_DP32(e->regs[R_EP_DMA_STS], EP_DMA_STS,
+                                       WPTR, 0);
+    e->regs[R_EP_DMA_CTRL] = FIELD_DP32(e->regs[R_EP_DMA_CTRL], EP_DMA_CTRL,
+                                        PROC_STS, EP_DMA_CTRL_STS_TX_IDLE);
+    aspeed_udc_raise_ep_ack(s, ep);
+
+    return ASPEED_UDC_XFER_DONE;
+}
+
 /*
  * OUT transfer: receive data from the host by copying its OUT packet into the
  * buffer the guest gadget driver set up.
@@ -667,6 +721,40 @@ static void aspeed_udc_ep_in_kick_desc(AspeedUDCState *s, int ep,
     }
 }
 
+/*
+ * IN kick: the guest gadget driver wrote EP_DMA_STS to queue one buffer to
+ * send to the host. If a host IN request is already waiting (parked because
+ * there was no data before), send the buffer now and finish the request.
+ */
+static void aspeed_udc_ep_in_kick_single(AspeedUDCState *s, int ep)
+{
+    AspeedUDCEP *e = &s->ep[ep];
+    USBPacket *p = e->pkt;
+
+    /* Do nothing if no host request is waiting, or no buffer was queued. */
+    if (!p || !FIELD_EX32(e->regs[R_EP_DMA_STS], EP_DMA_STS, WPTR)) {
+        return;
+    }
+
+    /* A kick serves a newly queued buffer, so start from its beginning. */
+    e->single_buf_off = 0;
+
+    switch (aspeed_udc_ep_xfer_in_single(s, ep, p)) {
+    case ASPEED_UDC_XFER_DONE:
+        e->pkt = NULL;
+        p->status = USB_RET_SUCCESS;
+        usb_packet_complete(USB_DEVICE(s->usbgadget), p);
+        break;
+    case ASPEED_UDC_XFER_ERROR:
+        e->pkt = NULL;
+        p->status = USB_RET_IOERROR;
+        usb_packet_complete(USB_DEVICE(s->usbgadget), p);
+        break;
+    case ASPEED_UDC_XFER_MORE:
+        break;
+    }
+}
+
 /*
  * OUT kick: the guest gadget driver wrote EP_DMA_STS to give us a buffer for
  * OUT data. If an OUT packet is already waiting (parked because there was no
@@ -739,6 +827,8 @@ static void aspeed_udc_ep_write(void *opaque, hwaddr offset, uint64_t data,
         } else {
             if (FIELD_EX32(e->regs[R_EP_DMA_CTRL], EP_DMA_CTRL, DESC_OP_EN)) {
                 aspeed_udc_ep_in_kick_desc(s, e->index, old_val);
+            } else {
+                aspeed_udc_ep_in_kick_single(s, e->index);
             }
         }
         break;
@@ -774,6 +864,7 @@ static void aspeed_udc_reset_hold(Object *obj, ResetType type)
         memset(s->ep[i].regs, 0, sizeof(s->ep[i].regs));
         s->ep[i].pkt = NULL;
         s->ep[i].desc_off = 0;
+        s->ep[i].single_buf_off = 0;
     }
 
     /* Device-reset default: root, DMA and EP-pool soft-reset bits set */
@@ -896,22 +987,31 @@ static int aspeed_udc_find_ep(AspeedUDCState *s, int ep_nr, bool is_out)
 static void aspeed_udc_ep_data_in(AspeedUDCState *s, int ep, USBPacket *p)
 {
     AspeedUDCEP *e = &s->ep[ep];
+    bool is_desc = FIELD_EX32(e->regs[R_EP_DMA_CTRL], EP_DMA_CTRL, DESC_OP_EN);
     uint32_t rptr = FIELD_EX32(e->regs[R_EP_DMA_STS], EP_DMA_STS, RPTR);
     uint32_t wptr = FIELD_EX32(e->regs[R_EP_DMA_STS], EP_DMA_STS, WPTR);
+    AspeedUDCXferResult res;
 
-    if (rptr == wptr) {
+    /*
+     * In descriptor mode the ring is empty when the pointers meet;
+     * in single-stage mode one buffer is queued by setting the write pointer.
+     */
+    if (is_desc ? (rptr == wptr) : (wptr == 0)) {
         /*
          * No IN data is queued yet. Save the packet and return ASYNC
          * instead of NAK. A NAK would make the host retry slowly.
-         * aspeed_udc_ep_in_kick_desc() serves and completes this packet later,
-         * once the guest gadget driver queues descriptors.
+         * The kick from the guest gadget driver serves and completes this
+         * packet later, once it queues something to send.
          */
         e->pkt = p;
         p->status = USB_RET_ASYNC;
         return;
     }
 
-    switch (aspeed_udc_ep_xfer_in_desc(s, ep, p)) {
+    res = is_desc ? aspeed_udc_ep_xfer_in_desc(s, ep, p)
+                  : aspeed_udc_ep_xfer_in_single(s, ep, p);
+
+    switch (res) {
     case ASPEED_UDC_XFER_DONE:
         p->status = USB_RET_SUCCESS;
         break;
-- 
2.43.0

[PATCH v2 08/11] hw/usb/aspeed-udc: Use the endpoint max packet size for OUT transfers

[Jamin Lin]

In single mode EP_DMA_STS.PKT_SIZE on an OUT endpoint is where the
controller reports the length it received. It is not a buffer size. The
model took the copy length from it, so a zero in that field meant no
data was copied at all.

One maximum packet is the most that can arrive, so copy at most
EP_CONFIG.MAX_PKT bytes per call instead. That field cannot hold 1024,
so a value of 0 there means 1024 bytes. The IN path decoded that by
hand; move it into one helper and use it from both paths.

Take the packet size from the combined packet when there is one, as the
IN paths do. A combined packet holds more than p->iov says, so reading
p->iov.size cut the transfer short.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/usb/aspeed-udc.c | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/hw/usb/aspeed-udc.c b/hw/usb/aspeed-udc.c
index dcced1b8fb..09f5913aef 100644
--- a/hw/usb/aspeed-udc.c
+++ b/hw/usb/aspeed-udc.c
@@ -416,6 +416,13 @@ static const MemoryRegionOps aspeed_udc_setup_ops = {
     },
 };
 
+static uint32_t aspeed_udc_ep_get_max_packet(AspeedUDCEP *e)
+{
+    uint32_t mps = FIELD_EX32(e->regs[R_EP_CONFIG], EP_CONFIG, MAX_PKT);
+
+    return mps ? mps : ASPEED_UDC_EP_MAXPKT;
+}
+
 /*
  * Copy len bytes from guest memory at addr into the IN packet, going through
  * a bounce buffer one buf-full at a time. Returns false on DMA failure.
@@ -464,10 +471,10 @@ static AspeedUDCXferResult aspeed_udc_ep_xfer_in_desc(AspeedUDCState *s,
 {
     QEMUIOVector *pktiov = p->combined ? &p->combined->iov : &p->iov;
     AspeedUDCEP *e = &s->ep[ep];
-    uint32_t mps = FIELD_EX32(e->regs[R_EP_CONFIG], EP_CONFIG, MAX_PKT);
     uint32_t wptr = FIELD_EX32(e->regs[R_EP_DMA_STS], EP_DMA_STS, WPTR);
     uint32_t rptr = FIELD_EX32(e->regs[R_EP_DMA_STS], EP_DMA_STS, RPTR);
     uint32_t desc_base = e->regs[R_EP_DMA_BUFF];
+    uint32_t mps = aspeed_udc_ep_get_max_packet(e);
     uint32_t desc_addr;
     uint32_t remaining;
     uint32_t desc_ctrl;
@@ -480,11 +487,6 @@ static AspeedUDCXferResult aspeed_udc_ep_xfer_in_desc(AspeedUDCState *s,
     bool done = false;
     bool ack = false;
 
-    if (mps == 0) {
-        /* a MAX_PKT field of 0 means the maximum packet size */
-        mps = ASPEED_UDC_EP_MAXPKT;
-    }
-
     trace_aspeed_udc_ep_data_in(ep, rptr, wptr, pktiov->size);
 
     /* walk the queued descriptors, filling the packet */
@@ -624,19 +626,21 @@ static AspeedUDCXferResult aspeed_udc_ep_xfer_in_single(AspeedUDCState *s,
  * OUT transfer: receive data from the host by copying its OUT packet into the
  * buffer the guest gadget driver set up.
  *
- * A host packet can be bigger than one buffer, so we copy at most PKT_SIZE
- * bytes per call, continuing from where the last call stopped
- * (p->actual_length). The caller keeps the packet parked until it is fully
- * copied.
+ * In single mode PKT_SIZE is where the controller reports the length it
+ * received, so it is not a buffer size. One maximum packet is the most that
+ * can arrive, so copy at most that much per call, continuing from where the
+ * last call stopped (p->actual_length). The caller keeps the packet parked
+ * until it is fully copied.
  */
 static AspeedUDCXferResult aspeed_udc_ep_xfer_out_single(AspeedUDCState *s,
                                                          int ep, USBPacket *p)
 {
+    QEMUIOVector *pktiov = p->combined ? &p->combined->iov : &p->iov;
     AspeedUDCEP *e = &s->ep[ep];
-    uint32_t chunk = FIELD_EX32(e->regs[R_EP_DMA_STS], EP_DMA_STS, PKT_SIZE);
-    uint32_t remaining = p->iov.size - (uint32_t)p->actual_length;
+    uint32_t remaining = pktiov->size > (uint32_t)p->actual_length ?
+                         pktiov->size - (uint32_t)p->actual_length : 0;
+    uint32_t len = MIN(remaining, aspeed_udc_ep_get_max_packet(e));
     uint32_t data_buf_addr = e->regs[R_EP_DMA_BUFF];
-    uint32_t len = MIN(remaining, chunk);
     g_autofree uint8_t *buf = g_malloc(len);
 
     if (data_buf_addr && len) {
@@ -659,7 +663,7 @@ static AspeedUDCXferResult aspeed_udc_ep_xfer_out_single(AspeedUDCState *s,
                                         PROC_STS, EP_DMA_CTRL_STS_RX_IDLE);
     aspeed_udc_raise_ep_ack(s, ep);
 
-    if ((uint32_t)p->actual_length >= p->iov.size) {
+    if ((uint32_t)p->actual_length >= pktiov->size) {
         return ASPEED_UDC_XFER_DONE;
     }
 
-- 
2.43.0

[PATCH v2 09/11] hw/usb/aspeed-udc: Add trace events for the transfer paths

[Jamin Lin]

The register traces show what the guest wrote, but not which transfer
path the write chose, how much each call moved, or when the host gave up
on a packet.

Add one event per path, so the name says which path ran and a single
path can be enabled on its own: a kick, a transfer result, and a
cancelled packet.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/usb/aspeed-udc.c | 23 +++++++++++++++++++++++
 hw/usb/trace-events |  7 +++++++
 2 files changed, 30 insertions(+)

diff --git a/hw/usb/aspeed-udc.c b/hw/usb/aspeed-udc.c
index 09f5913aef..1c99e45d9a 100644
--- a/hw/usb/aspeed-udc.c
+++ b/hw/usb/aspeed-udc.c
@@ -565,6 +565,9 @@ static AspeedUDCXferResult aspeed_udc_ep_xfer_in_desc(AspeedUDCState *s,
         aspeed_udc_raise_ep_ack(s, ep);
     }
 
+    trace_aspeed_udc_ep_xfer_in_desc(ep, (uint32_t)p->actual_length,
+                                     done ? "done" : "more");
+
     return done ? ASPEED_UDC_XFER_DONE : ASPEED_UDC_XFER_MORE;
 }
 
@@ -606,6 +609,9 @@ static AspeedUDCXferResult aspeed_udc_ep_xfer_in_single(AspeedUDCState *s,
      * buffer, and the next IN request sends the rest.
      */
     if (e->single_buf_off < buf_len) {
+        trace_aspeed_udc_ep_xfer_in_single(ep,
+                                           (uint32_t)p->actual_length,
+                                           "buffer left");
         return ASPEED_UDC_XFER_DONE;
     }
 
@@ -619,6 +625,9 @@ static AspeedUDCXferResult aspeed_udc_ep_xfer_in_single(AspeedUDCState *s,
                                         PROC_STS, EP_DMA_CTRL_STS_TX_IDLE);
     aspeed_udc_raise_ep_ack(s, ep);
 
+    trace_aspeed_udc_ep_xfer_in_single(ep, (uint32_t)p->actual_length,
+                                       "done");
+
     return ASPEED_UDC_XFER_DONE;
 }
 
@@ -664,9 +673,15 @@ static AspeedUDCXferResult aspeed_udc_ep_xfer_out_single(AspeedUDCState *s,
     aspeed_udc_raise_ep_ack(s, ep);
 
     if ((uint32_t)p->actual_length >= pktiov->size) {
+        trace_aspeed_udc_ep_xfer_out_single(ep,
+                                            (uint32_t)p->actual_length,
+                                            "done");
         return ASPEED_UDC_XFER_DONE;
     }
 
+    trace_aspeed_udc_ep_xfer_out_single(ep, (uint32_t)p->actual_length,
+                                        "more");
+
     return ASPEED_UDC_XFER_MORE;
 }
 
@@ -686,6 +701,8 @@ static void aspeed_udc_ep_in_kick_desc(AspeedUDCState *s, int ep,
     uint32_t cur_rptr = FIELD_EX32(old_val, EP_DMA_STS, RPTR);
     USBPacket *p = e->pkt;
 
+    trace_aspeed_udc_ep_in_kick_desc(ep);
+
     /*
      * A normal kick only sets the write pointer and leaves the read-pointer
      * field 0 (the read pointer is ours to advance). The guest resets the ring
@@ -735,6 +752,8 @@ static void aspeed_udc_ep_in_kick_single(AspeedUDCState *s, int ep)
     AspeedUDCEP *e = &s->ep[ep];
     USBPacket *p = e->pkt;
 
+    trace_aspeed_udc_ep_in_kick_single(ep);
+
     /* Do nothing if no host request is waiting, or no buffer was queued. */
     if (!p || !FIELD_EX32(e->regs[R_EP_DMA_STS], EP_DMA_STS, WPTR)) {
         return;
@@ -770,6 +789,8 @@ static void aspeed_udc_ep_out_kick_single(AspeedUDCState *s, int ep)
     AspeedUDCEP *e = &s->ep[ep];
     USBPacket *p = e->pkt;
 
+    trace_aspeed_udc_ep_out_kick_single(ep);
+
     /* nothing to do unless an OUT packet is waiting and a buffer is ready */
     if (!p || !FIELD_EX32(e->regs[R_EP_DMA_STS], EP_DMA_STS, WPTR)) {
         return;
@@ -1155,6 +1176,8 @@ static void aspeed_udc_gadget_cancel_packet(USBDevice *udev, USBPacket *p)
     AspeedUDCState *s = ASPEED_UDC_GADGET(udev)->udc;
     int i;
 
+    trace_aspeed_udc_cancel_packet(p->ep->nr);
+
     if (s->ep0_packet == p) {
         s->ep0_packet = NULL;
     }
diff --git a/hw/usb/trace-events b/hw/usb/trace-events
index aa260dacad..661011b07e 100644
--- a/hw/usb/trace-events
+++ b/hw/usb/trace-events
@@ -395,3 +395,10 @@ aspeed_udc_handle_data(int ep_nr, const char *dir, uint32_t iov, int ep_idx) "ep
 aspeed_udc_ep_data_in(unsigned ep, uint32_t rptr, uint32_t wptr, uint32_t iov) "ep %u, rptr %u, wptr %u, iov %u"
 aspeed_udc_ep_data_out(unsigned ep, uint32_t wptr, uint32_t avail, uint32_t iov) "ep %u, wptr %u, avail %u, iov %u"
 aspeed_udc_ep_ack(unsigned ep) "ep %u"
+aspeed_udc_ep_in_kick_desc(int ep) "ep %d"
+aspeed_udc_ep_in_kick_single(int ep) "ep %d"
+aspeed_udc_ep_out_kick_single(int ep) "ep %d"
+aspeed_udc_ep_xfer_in_desc(int ep, uint32_t sent, const char *res) "ep %d, sent %u, %s"
+aspeed_udc_ep_xfer_in_single(int ep, uint32_t sent, const char *res) "ep %d, sent %u, %s"
+aspeed_udc_ep_xfer_out_single(int ep, uint32_t received, const char *res) "ep %d, received %u, %s"
+aspeed_udc_cancel_packet(int ep) "ep %d"
-- 
2.43.0

[PATCH v2 10/11] hw/arm/aspeed_ast10x0: Add the USB device controller

[Jamin Lin]

Add the USB UDC device at 0x7E6A2000 with IRQ 9. The AST10x0 UDC
controller is compatible with the AST2600 one, so reuse TYPE_ASPEED_UDC.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 include/hw/arm/aspeed_soc.h |  1 +
 hw/arm/aspeed_ast10x0.c     | 17 +++++++++++++----
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index 38226d9b80..4459188c57 100644
--- a/include/hw/arm/aspeed_soc.h
+++ b/include/hw/arm/aspeed_soc.h
@@ -167,6 +167,7 @@ struct Aspeed10x0SoCState {
     AspeedSoCState parent;
 
     ARMv7MState armv7m;
+    AspeedUDCState udc;
 };
 
 #define TYPE_ASPEED10X0_SOC "aspeed10x0-soc"
diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
index aeb5a4423d..81f2d78a45 100644
--- a/hw/arm/aspeed_ast10x0.c
+++ b/hw/arm/aspeed_ast10x0.c
@@ -162,10 +162,11 @@ static void aspeed_soc_ast10x0_init(Object *obj, const char *socname)
     snprintf(typename, sizeof(typename), "aspeed.hace-%s", socname);
     object_initialize_child(obj, "hace", &s->hace, typename);
 
+    object_initialize_child(obj, "udc", &a->udc, TYPE_ASPEED_UDC);
+
     object_initialize_child(obj, "iomem", &s->iomem, TYPE_UNIMPLEMENTED_DEVICE);
     object_initialize_child(obj, "pwm", &s->pwm, TYPE_UNIMPLEMENTED_DEVICE);
     object_initialize_child(obj, "espi", &s->espi, TYPE_UNIMPLEMENTED_DEVICE);
-    object_initialize_child(obj, "udc", &s->udc, TYPE_UNIMPLEMENTED_DEVICE);
     object_initialize_child(obj, "sgpiom", &s->sgpiom,
                             TYPE_UNIMPLEMENTED_DEVICE);
     object_initialize_child(obj, "jtag[0]", &s->jtag[0],
@@ -394,6 +395,17 @@ static bool aspeed_soc_ast10x0_realize(Aspeed10x0SoCState *a, Error **errp)
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->gpio), 0,
                        aspeed_soc_ast1030_get_irq(s, ASPEED_DEV_GPIO));
 
+    /* UDC - USB 2.0 Device Controller */
+    object_property_set_link(OBJECT(&a->udc), "dram",
+                             OBJECT(&s->sram[0]), &error_abort);
+    if (!sysbus_realize(SYS_BUS_DEVICE(&a->udc), errp)) {
+        return false;
+    }
+    aspeed_mmio_map(s->memory, SYS_BUS_DEVICE(&a->udc), 0,
+                    sc->memmap[ASPEED_DEV_UDC]);
+    sysbus_connect_irq(SYS_BUS_DEVICE(&a->udc), 0,
+                       aspeed_soc_ast1030_get_irq(s, ASPEED_DEV_UDC));
+
     aspeed_mmio_map_unimplemented(s->memory, SYS_BUS_DEVICE(&s->pwm),
                                   "aspeed.pwm",
                                   sc->memmap[ASPEED_DEV_PWM], 0x100);
@@ -402,9 +414,6 @@ static bool aspeed_soc_ast10x0_realize(Aspeed10x0SoCState *a, Error **errp)
                                   "aspeed.espi",
                                   sc->memmap[ASPEED_DEV_ESPI], 0x800);
 
-    aspeed_mmio_map_unimplemented(s->memory, SYS_BUS_DEVICE(&s->udc),
-                                  "aspeed.udc",
-                                  sc->memmap[ASPEED_DEV_UDC], 0x1000);
     aspeed_mmio_map_unimplemented(s->memory, SYS_BUS_DEVICE(&s->sgpiom),
                                   "aspeed.sgpiom",
                                   sc->memmap[ASPEED_DEV_SGPIOM0], 0x100);
-- 
2.43.0

[PATCH v2 11/11] tests/functional/arm/test_aspeed_ast1030: Test the usbredir server transport

[Jamin Lin]

Export the AST1030 UDC gadget with usb-redir-server and import it on an
AST2600 with usb-redir, then check that the AST2600 enumerates it. That
covers the announce, control and enumeration paths of the transport, and
the UDC model behind it.

The two machines have to come from the same QEMU binary, because a
functional test gets only one. ast1030-evb and ast2600-evb are both in
qemu-system-arm.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 tests/functional/arm/meson.build            |  1 +
 tests/functional/arm/test_aspeed_ast1030.py | 52 +++++++++++++++++++++
 2 files changed, 53 insertions(+)

diff --git a/tests/functional/arm/meson.build b/tests/functional/arm/meson.build
index 61b00932db..cfbd376b0d 100644
--- a/tests/functional/arm/meson.build
+++ b/tests/functional/arm/meson.build
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0-or-later
 
 test_arm_timeouts = {
+  'aspeed_ast1030' : 720,
   'aspeed_palmetto' : 120,
   'aspeed_romulus' : 120,
   'aspeed_witherspoon' : 120,
diff --git a/tests/functional/arm/test_aspeed_ast1030.py b/tests/functional/arm/test_aspeed_ast1030.py
index 83a96ec322..d1d059c83e 100755
--- a/tests/functional/arm/test_aspeed_ast1030.py
+++ b/tests/functional/arm/test_aspeed_ast1030.py
@@ -6,6 +6,8 @@
 #
 # SPDX-License-Identifier: GPL-2.0-or-later
 
+import os
+
 from aspeed import AspeedTest
 from qemu_test import Asset, exec_command_and_wait_for_pattern
 
@@ -68,6 +70,56 @@ def test_arm_ast1030_zephyros_1_07(self):
                 'kernel uptime',
         ]: exec_command_and_wait_for_pattern(self, shell_cmd, "uart:~$")
 
+    ASSET_SDK_V1103_AST2600 = Asset(
+        'https://github.com/AspeedTech-BMC/openbmc/releases/download/v11.03/ast2600-default-image.tar.gz',
+        '47e3656a14bf7a4de28d3dfbf48bc2325443bc42d270f3bc82646f92f6dea165')
+
+    def test_arm_ast1030_usbredir_to_ast2600(self):
+        self.require_device('usb-redir-server')
+        self.require_device('usb-redir')
+        self.set_machine('ast2600-evb')
+        self.set_machine('ast1030-evb')
+
+        ast1030_kernel_file = self.archive_extract(
+            self.ASSET_ZEPHYR_3_08, member="ast1030-evb-demo/zephyr.elf")
+
+        ast2600_image_file = self.archive_extract(
+            self.ASSET_SDK_V1103_AST2600,
+            member="ast2600-default-image/image-bmc")
+        sock = os.path.join(self.socket_dir().name, 'usbredir.sock')
+
+        udc = self.get_vm(name='udc')
+        udc.set_console()
+        udc.add_args('-kernel', ast1030_kernel_file, '-nographic',
+                     '-chardev',
+                     f'socket,id=usbredir0,path={sock},server=on,wait=off',
+                     '-device',
+                     'usb-redir-server,id=udcredir,chardev=usbredir0',
+                     '-device', 'aspeed.udc-gadget,bus=udcredir.0,'
+                                'udc=/machine/soc/udc')
+        udc.launch()
+        self.wait_for_console_pattern('Booting Zephyr OS', vm=udc)
+        exec_command_and_wait_for_pattern(self, 'usb enable', 'uart:~$',
+                                          vm=udc)
+
+        host = self.get_vm(name='host')
+        host.set_machine('ast2600-evb')
+        host.set_console()
+        host.add_args('-drive',
+                      f'file={ast2600_image_file},if=mtd,format=raw',
+                      '-snapshot',
+                      '-chardev', f'socket,id=usbredir0,path={sock}',
+                      '-device', 'usb-redir,chardev=usbredir0,bus=usb-bus.1')
+        host.launch()
+        self.wait_for_console_pattern('Starting kernel ...', vm=host)
+        self.wait_for_console_pattern('login:', vm=host)
+        exec_command_and_wait_for_pattern(self, 'root', 'Password:', vm=host)
+        exec_command_and_wait_for_pattern(self, '0penBmc',
+                                          'root@ast2600-default:~#', vm=host)
+        exec_command_and_wait_for_pattern(self, 'lsusb',
+                                          'ZEPHYR Zephyr DFU sample',
+                                          vm=host)
+
     def test_arm_ast1030_otp_blockdev_device(self):
         self.vm.set_machine("ast1030-evb")
 
-- 
2.43.0

Re: [PATCH v2 01/11] hw/usb/aspeed-udc: Make the SETUP buffer byte addressable

[Cédric Le Goater]

On 10/5/26 08:44, Jamin Lin wrote:
> The controller registers need a 4 byte access, but the 8 bytes at 0x80
> are not registers. They are a SRAM buffer holding the EP0 SETUP packet.
> A read or a write that is not 4 bytes returned the wrong data, so the
> driver saw a broken SETUP packet.
> 
> Introduce a separate SETUP MMIO region that handles 1, 2 and 4 byte
> reads and writes.
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> ---
>   include/hw/usb/aspeed-udc.h | 11 ++++++--
>   hw/usb/aspeed-udc.c         | 56 +++++++++++++++++++++++++++++++++----
>   hw/usb/trace-events         |  2 ++
>   3 files changed, 61 insertions(+), 8 deletions(-)
> 
> diff --git a/include/hw/usb/aspeed-udc.h b/include/hw/usb/aspeed-udc.h
> index 7701c1aa34..42916c0bd6 100644
> --- a/include/hw/usb/aspeed-udc.h
> +++ b/include/hw/usb/aspeed-udc.h
> @@ -24,11 +24,14 @@ OBJECT_DECLARE_SIMPLE_TYPE(AspeedUDCState, ASPEED_UDC)
>   OBJECT_DECLARE_SIMPLE_TYPE(AspeedUDCGadget, ASPEED_UDC_GADGET)
>   
>   /*
> - * Register map: root/global block at 0x000 - 0x087, then one 0x10 byte bank
> - * per programmable endpoint from 0x200.
> + * Register map: root/global block at 0x000 - 0x07f, the SETUP buffer at
> + * 0x080 - 0x087, then one 0x10 byte bank per programmable endpoint from
> + * 0x200.
>    */
>   #define ASPEED_UDC_MEM_SIZE     0x300
> -#define ASPEED_UDC_ROOT_NR_REGS (0x88 >> 2)
> +#define ASPEED_UDC_ROOT_NR_REGS (0x80 >> 2)
> +#define ASPEED_UDC_SETUP_BASE   0x80
> +#define ASPEED_UDC_SETUP_SIZE   0x08
>   #define ASPEED_UDC_EP_REG_BASE  0x200
>   #define ASPEED_UDC_EP_NR_REGS   (0x10 >> 2)
>   
> @@ -62,9 +65,11 @@ struct AspeedUDCState {
>   
>       MemoryRegion udc_container;
>       MemoryRegion root_mr;
> +    MemoryRegion setup_mr;
>       MemoryRegion *dram_mr;
>       AddressSpace dram_as;
>       uint32_t regs[ASPEED_UDC_ROOT_NR_REGS];
> +    uint8_t setup_buf[ASPEED_UDC_SETUP_SIZE];
>       AspeedUDCEP ep[ASPEED_UDC_NUM_EP];
>       qemu_irq irq;
>   
> diff --git a/hw/usb/aspeed-udc.c b/hw/usb/aspeed-udc.c
> index 4c90b4f4c2..355512822b 100644
> --- a/hw/usb/aspeed-udc.c
> +++ b/hw/usb/aspeed-udc.c
> @@ -58,9 +58,6 @@ REG32(UDC_EP0_CTRL, 0x30)
>       FIELD(UDC_EP0_CTRL, STALL,              0, 1)
>   REG32(UDC_EP0_DATA_BUFF, 0x34)
>       FIELD(UDC_EP0_DATA_BUFF, BASE_ADDR,     0, 31)
> -/* EP0 SETUP packet buffer: SETUP0 = bytes 0...3, SETUP1 = bytes 4...7 */
> -REG32(UDC_SETUP0, 0x80)
> -REG32(UDC_SETUP1, 0x84)
>   
>   /* Per programmable-endpoint registers (offset from the EP register base) */
>   REG32(EP_CONFIG, 0x00)
> @@ -363,6 +360,45 @@ static const MemoryRegionOps aspeed_udc_ops = {
>       },
>   };
>   
> +static uint64_t aspeed_udc_setup_read(void *opaque, hwaddr offset,
> +                                      unsigned size)
> +{
> +    AspeedUDCState *s = ASPEED_UDC(opaque);
> +    uint64_t value = 0;
> +    int i;
> +
> +    for (i = 0; i < size; i++) {
> +        value |= (uint64_t)s->setup_buf[offset + i] << (8 * i);
> +    }
> +
> +    trace_aspeed_udc_setup_read(offset, size, value);
> +
> +    return value;
> +}
> +
> +static void aspeed_udc_setup_write(void *opaque, hwaddr offset,
> +                                   uint64_t value, unsigned size)
> +{
> +    AspeedUDCState *s = ASPEED_UDC(opaque);
> +    int i;
> +
> +    trace_aspeed_udc_setup_write(offset, size, value);
> +
> +    for (i = 0; i < size; i++) {
> +        s->setup_buf[offset + i] = value >> (8 * i);
> +    }
> +}
> +
> +static const MemoryRegionOps aspeed_udc_setup_ops = {
> +    .read = aspeed_udc_setup_read,
> +    .write = aspeed_udc_setup_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .valid = {
> +        .min_access_size = 1,
> +        .max_access_size = 4,
> +    },
> +};
> +
>   /*
>    * Copy len bytes from guest memory at addr into the IN packet, going through
>    * a bounce buffer one buf-full at a time. Returns false on DMA failure.
> @@ -708,6 +744,7 @@ static void aspeed_udc_reset_hold(Object *obj, ResetType type)
>       int i;
>   
>       memset(s->regs, 0, sizeof(s->regs));
> +    memset(s->setup_buf, 0, sizeof(s->setup_buf));
>       for (i = 0; i < ASPEED_UDC_NUM_EP; i++) {
>           memset(s->ep[i].regs, 0, sizeof(s->ep[i].regs));
>           s->ep[i].pkt = NULL;
> @@ -754,6 +791,12 @@ static void aspeed_udc_realize(DeviceState *dev, Error **errp)
>                             ASPEED_UDC_ROOT_NR_REGS << 2);
>       memory_region_add_subregion(&s->udc_container, 0, &s->root_mr);
>   
> +    memory_region_init_io(&s->setup_mr, OBJECT(s), &aspeed_udc_setup_ops,
> +                          s, TYPE_ASPEED_UDC ".setup",
> +                          ASPEED_UDC_SETUP_SIZE);
> +    memory_region_add_subregion(&s->udc_container,
> +                                ASPEED_UDC_SETUP_BASE, &s->setup_mr);
> +
>       /* Each programmable endpoint has its own register bank */
>       for (i = 0; i < ASPEED_UDC_NUM_EP; i++) {
>           g_autofree char *name = g_strdup_printf(TYPE_ASPEED_UDC ".ep%d", i);
> @@ -918,8 +961,11 @@ static void aspeed_udc_gadget_handle_control(USBDevice *udev, USBPacket *p,
>        * Reconstruct the 8-byte SETUP packet into the SETUP data buffer where
>        * the guest gadget driver reads it from.
>        */
> -    s->regs[R_UDC_SETUP0] = type | (req << 8) | ((value & 0xffff) << 16);
> -    s->regs[R_UDC_SETUP1] = (index & 0xffff) | ((length & 0xffff) << 16);
> +    s->setup_buf[0] = type;
> +    s->setup_buf[1] = req;
> +    stw_le_p(&s->setup_buf[2], value);
> +    stw_le_p(&s->setup_buf[4], index);
> +    stw_le_p(&s->setup_buf[6], length);
>   
>       /* A new SETUP clears the EP0 STALL condition */
>       s->regs[R_UDC_EP0_CTRL] &= ~R_UDC_EP0_CTRL_STALL_MASK;
> diff --git a/hw/usb/trace-events b/hw/usb/trace-events
> index 80ead23358..aa260dacad 100644
> --- a/hw/usb/trace-events
> +++ b/hw/usb/trace-events
> @@ -381,6 +381,8 @@ canokey_unrealize(void)
>   # aspeed-udc.c
>   aspeed_udc_read(uint64_t offset, uint32_t value) "offset 0x%" PRIx64 " value 0x%x"
>   aspeed_udc_write(uint64_t offset, uint32_t value) "offset 0x%" PRIx64 " value 0x%x"
> +aspeed_udc_setup_read(uint64_t offset, unsigned size, uint64_t value) "offset 0x%" PRIx64 " size %u value 0x%" PRIx64
> +aspeed_udc_setup_write(uint64_t offset, unsigned size, uint64_t value) "offset 0x%" PRIx64 " size %u value 0x%" PRIx64
>   aspeed_udc_ep_read(int ep, uint64_t offset, uint32_t value) "ep %d, offset 0x%" PRIx64 " value 0x%x"
>   aspeed_udc_ep_write(int ep, uint64_t offset, uint32_t value) "ep %d, offset 0x%" PRIx64 " value 0x%x"
>   aspeed_udc_pullup(int on, int attached) "on %d, attached %d"


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.

Re: [PATCH v2 02/11] hw/usb/aspeed-udc: Mask the reserved bits on register writes

[Cédric Le Goater]

On 10/5/26 08:44, Jamin Lin wrote:
> Writes to the interrupt registers were not masked, so a guest could set
> a reserved bit and read it back. The hardware returns 0 for those.
> 
> Mask each one to the bits the datasheet marks read/write:
> 
>    UDC_IER         17, 16, 8:6, 4:0
>    UDC_ISR         8:6, 4:0
>    UDC_EP_ACK_IER  3:0
>    UDC_EP_NAK_IER  3:0
>    UDC_EP_ACK_ISR  3:0
>    UDC_EP_NAK_ISR  3:0
> 
> UDC_ISR bits 17 and 16 summarise the endpoint pool. They are read only,
> so a write cannot clear them.
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> ---
>   hw/usb/aspeed-udc.c | 12 +++++++++++-
>   1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/usb/aspeed-udc.c b/hw/usb/aspeed-udc.c
> index 355512822b..95fe823534 100644
> --- a/hw/usb/aspeed-udc.c
> +++ b/hw/usb/aspeed-udc.c
> @@ -319,14 +319,24 @@ static void aspeed_udc_write(void *opaque, hwaddr offset, uint64_t data,
>           s->regs[R_UDC_FUNC_CTRL] = val;
>           break;
>       case R_UDC_IER:
> +        val &= 0x000301df;
> +        s->regs[reg] = val;
> +        aspeed_udc_update_irq(s);
> +        break;
> +    case R_UDC_ISR:
> +        val &= 0x000001df;
> +        s->regs[reg] &= ~val;
> +        aspeed_udc_update_irq(s);
> +        break;
>       case R_UDC_EP_ACK_IER:
>       case R_UDC_EP_NAK_IER:
> +        val &= 0x0000000f;
>           s->regs[reg] = val;
>           aspeed_udc_update_irq(s);
>           break;
> -    case R_UDC_ISR:
>       case R_UDC_EP_ACK_ISR:
>       case R_UDC_EP_NAK_ISR:
> +        val &= 0x0000000f;
>           s->regs[reg] &= ~val;
>           aspeed_udc_update_irq(s);
>           break;

Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.

Re: [PATCH v2 03/11] hw/usb/aspeed-udc: Clear EP_POOL_ACK when no endpoint ack is left

[Cédric Le Goater]

On 10/5/26 08:44, Jamin Lin wrote:
> UDC_ISR.EP_POOL_ACK says that at least one endpoint in UDC_EP_ACK_ISR
> has an ack. It is read only, so a write cannot clear it.
> 
> Clear it when bits 3:0 of UDC_EP_ACK_ISR are all 0, which means no
> programmable endpoint has an ack.
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> ---
>   hw/usb/aspeed-udc.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/hw/usb/aspeed-udc.c b/hw/usb/aspeed-udc.c
> index 95fe823534..bdbd1a78b8 100644
> --- a/hw/usb/aspeed-udc.c
> +++ b/hw/usb/aspeed-udc.c
> @@ -335,6 +335,13 @@ static void aspeed_udc_write(void *opaque, hwaddr offset, uint64_t data,
>           aspeed_udc_update_irq(s);
>           break;
>       case R_UDC_EP_ACK_ISR:
> +        val &= 0x0000000f;
> +        s->regs[reg] &= ~val;
> +        if (!s->regs[R_UDC_EP_ACK_ISR]) {
> +            s->regs[R_UDC_ISR] &= ~R_UDC_ISR_EP_POOL_ACK_MASK;
> +        }
> +        aspeed_udc_update_irq(s);
> +        break;
>       case R_UDC_EP_NAK_ISR:
>           val &= 0x0000000f;
>           s->regs[reg] &= ~val;

Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.

Re: [PATCH v2 04/11] hw/usb/aspeed-udc: Rename the transfer helpers after their DMA mode

[Cédric Le Goater]

On 10/5/26 08:44, Jamin Lin wrote:
> The controller can run an endpoint in descriptor list mode or in single
> stage mode. Put the mode in the helper names. That makes the code
> easier to read.
> 
> No functional change.
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> ---
>   hw/usb/aspeed-udc.c | 30 +++++++++++++++---------------
>   1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/usb/aspeed-udc.c b/hw/usb/aspeed-udc.c
> index bdbd1a78b8..b43f077c2d 100644
> --- a/hw/usb/aspeed-udc.c
> +++ b/hw/usb/aspeed-udc.c
> @@ -459,8 +459,8 @@ static bool aspeed_udc_ep_copy_to_pkt(AspeedUDCState *s, int ep, uint32_t addr,
>    * This function raises the endpoint ACK by itself when the ring becomes empty
>    * or when a descriptor asks for an interrupt.
>    */
> -static AspeedUDCXferResult aspeed_udc_ep_xfer_in(AspeedUDCState *s, int ep,
> -                                                 USBPacket *p)
> +static AspeedUDCXferResult aspeed_udc_ep_xfer_in_desc(AspeedUDCState *s,
> +                                                      int ep, USBPacket *p)
>   {
>       QEMUIOVector *pktiov = p->combined ? &p->combined->iov : &p->iov;
>       AspeedUDCEP *e = &s->ep[ep];
> @@ -568,15 +568,15 @@ static AspeedUDCXferResult aspeed_udc_ep_xfer_in(AspeedUDCState *s, int ep,
>   
>   /*
>    * OUT transfer: receive data from the host by copying its OUT packet into the
> - * buffer the guest gadget driver set up (single-stage mode).
> + * buffer the guest gadget driver set up.
>    *
>    * A host packet can be bigger than one buffer, so we copy at most PKT_SIZE
>    * bytes per call, continuing from where the last call stopped
>    * (p->actual_length). The caller keeps the packet parked until it is fully
>    * copied.
>    */
> -static AspeedUDCXferResult aspeed_udc_ep_xfer_out(AspeedUDCState *s, int ep,
> -                                                  USBPacket *p)
> +static AspeedUDCXferResult aspeed_udc_ep_xfer_out_single(AspeedUDCState *s,
> +                                                         int ep, USBPacket *p)
>   {
>       AspeedUDCEP *e = &s->ep[ep];
>       uint32_t chunk = FIELD_EX32(e->regs[R_EP_DMA_STS], EP_DMA_STS, PKT_SIZE);
> @@ -619,7 +619,7 @@ static AspeedUDCXferResult aspeed_udc_ep_xfer_out(AspeedUDCState *s, int ep,
>    * If the request needs more data than was queued, keep it parked and wait for
>    * the next kick.
>    */
> -static void aspeed_udc_ep_in_kick(AspeedUDCState *s, int ep, uint32_t val)
> +static void aspeed_udc_ep_in_kick_desc(AspeedUDCState *s, int ep, uint32_t val)
>   {
>       AspeedUDCEP *e = &s->ep[ep];
>       uint32_t cur_rptr = FIELD_EX32(e->regs[R_EP_DMA_STS], EP_DMA_STS, RPTR);
> @@ -649,7 +649,7 @@ static void aspeed_udc_ep_in_kick(AspeedUDCState *s, int ep, uint32_t val)
>           return;
>       }
>   
> -    switch (aspeed_udc_ep_xfer_in(s, ep, p)) {
> +    switch (aspeed_udc_ep_xfer_in_desc(s, ep, p)) {
>       case ASPEED_UDC_XFER_DONE:
>           e->pkt = NULL;
>           p->status = USB_RET_SUCCESS;
> @@ -671,7 +671,7 @@ static void aspeed_udc_ep_in_kick(AspeedUDCState *s, int ep, uint32_t val)
>    * buffer before), copy its data into the buffer now and finish it. If the
>    * packet has more data than fits, keep it parked and wait for the next buffer.
>    */
> -static void aspeed_udc_ep_out_kick(AspeedUDCState *s, int ep)
> +static void aspeed_udc_ep_out_kick_single(AspeedUDCState *s, int ep)
>   {
>       AspeedUDCEP *e = &s->ep[ep];
>       USBPacket *p = e->pkt;
> @@ -681,7 +681,7 @@ static void aspeed_udc_ep_out_kick(AspeedUDCState *s, int ep)
>           return;
>       }
>   
> -    switch (aspeed_udc_ep_xfer_out(s, ep, p)) {
> +    switch (aspeed_udc_ep_xfer_out_single(s, ep, p)) {
>       case ASPEED_UDC_XFER_DONE:
>           e->pkt = NULL;
>           p->status = USB_RET_SUCCESS;
> @@ -727,11 +727,11 @@ static void aspeed_udc_ep_write(void *opaque, hwaddr offset, uint64_t data,
>           val &= 0x77ffffff;
>           if (FIELD_EX32(e->regs[R_EP_DMA_CTRL], EP_DMA_CTRL, DESC_OP_EN)) {
>               /* IN, descriptor-list mode */
> -            aspeed_udc_ep_in_kick(s, e->index, val);
> +            aspeed_udc_ep_in_kick_desc(s, e->index, val);
>           } else {
>               /* OUT, single-stage mode */
>               e->regs[reg] = val;
> -            aspeed_udc_ep_out_kick(s, e->index);
> +            aspeed_udc_ep_out_kick_single(s, e->index);
>           }
>           break;
>       default:
> @@ -895,7 +895,7 @@ static void aspeed_udc_ep_data_in(AspeedUDCState *s, int ep, USBPacket *p)
>           /*
>            * No IN data is queued yet. Save the packet and return ASYNC
>            * instead of NAK. A NAK would make the host retry slowly.
> -         * aspeed_udc_ep_in_kick() serves and completes this packet later,
> +         * aspeed_udc_ep_in_kick_desc() serves and completes this packet later,
>            * once the guest gadget driver queues descriptors.
>            */
>           e->pkt = p;
> @@ -903,7 +903,7 @@ static void aspeed_udc_ep_data_in(AspeedUDCState *s, int ep, USBPacket *p)
>           return;
>       }
>   
> -    switch (aspeed_udc_ep_xfer_in(s, ep, p)) {
> +    switch (aspeed_udc_ep_xfer_in_desc(s, ep, p)) {
>       case ASPEED_UDC_XFER_DONE:
>           p->status = USB_RET_SUCCESS;
>           break;
> @@ -931,7 +931,7 @@ static void aspeed_udc_ep_data_out(AspeedUDCState *s, int ep, USBPacket *p)
>            * No OUT buffer is ready yet. Save the packet and return ASYNC
>            * instead of NAK. Writing now could use an old buffer address and
>            * lose the data (for example a mass-storage CBW). A NAK would make
> -         * the host retry slowly. aspeed_udc_ep_out_kick() delivers this
> +         * the host retry slowly. aspeed_udc_ep_out_kick_single() delivers this
>            * packet later, once the guest gadget driver sets up a buffer.
>            */
>           e->pkt = p;
> @@ -939,7 +939,7 @@ static void aspeed_udc_ep_data_out(AspeedUDCState *s, int ep, USBPacket *p)
>           return;
>       }
>   
> -    switch (aspeed_udc_ep_xfer_out(s, ep, p)) {
> +    switch (aspeed_udc_ep_xfer_out_single(s, ep, p)) {
>       case ASPEED_UDC_XFER_DONE:
>           p->status = USB_RET_SUCCESS;
>           break;

Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.

Re: [PATCH v2 05/11] hw/usb/aspeed-udc: Store the EP_DMA_STS write first

[Cédric Le Goater]

On 10/5/26 08:44, Jamin Lin wrote:
> The other helpers run after EP_DMA_STS is written. The descriptor mode
> kick did not: it had to run before the write, because it stored the
> value itself.
> 
> Write the register first in every case, then call the helper. The order
> is now the same everywhere, so a later change cannot go wrong on it,
> and the code is easier to maintain.
> 
> No functional change.
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> ---
>   hw/usb/aspeed-udc.c | 19 +++++++++++--------
>   1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/usb/aspeed-udc.c b/hw/usb/aspeed-udc.c
> index b43f077c2d..4572914e16 100644
> --- a/hw/usb/aspeed-udc.c
> +++ b/hw/usb/aspeed-udc.c
> @@ -619,12 +619,13 @@ static AspeedUDCXferResult aspeed_udc_ep_xfer_out_single(AspeedUDCState *s,
>    * If the request needs more data than was queued, keep it parked and wait for
>    * the next kick.
>    */
> -static void aspeed_udc_ep_in_kick_desc(AspeedUDCState *s, int ep, uint32_t val)
> +static void aspeed_udc_ep_in_kick_desc(AspeedUDCState *s, int ep,
> +                                       uint32_t old_val)
>   {
>       AspeedUDCEP *e = &s->ep[ep];
> -    uint32_t cur_rptr = FIELD_EX32(e->regs[R_EP_DMA_STS], EP_DMA_STS, RPTR);
> -    uint32_t new_rptr = FIELD_EX32(val, EP_DMA_STS, RPTR);
> -    uint32_t new_wptr = FIELD_EX32(val, EP_DMA_STS, WPTR);
> +    uint32_t new_rptr = FIELD_EX32(e->regs[R_EP_DMA_STS], EP_DMA_STS, RPTR);
> +    uint32_t new_wptr = FIELD_EX32(e->regs[R_EP_DMA_STS], EP_DMA_STS, WPTR);
> +    uint32_t cur_rptr = FIELD_EX32(old_val, EP_DMA_STS, RPTR);
>       USBPacket *p = e->pkt;
>   
>       /*
> @@ -641,8 +642,9 @@ static void aspeed_udc_ep_in_kick_desc(AspeedUDCState *s, int ep, uint32_t val)
>           cur_rptr = new_rptr;
>           e->desc_off = 0;
>       }
> -    /* store the guest's write, but keep our own read pointer */
> -    e->regs[R_EP_DMA_STS] = FIELD_DP32(val, EP_DMA_STS, RPTR, cur_rptr);
> +    /* keep our own read pointer */
> +    e->regs[R_EP_DMA_STS] = FIELD_DP32(e->regs[R_EP_DMA_STS], EP_DMA_STS,
> +                                       RPTR, cur_rptr);
>   
>       /* nothing to do unless an IN packet is waiting and the ring has data */
>       if (!p || cur_rptr == new_wptr) {
> @@ -715,6 +717,7 @@ static void aspeed_udc_ep_write(void *opaque, hwaddr offset, uint64_t data,
>       AspeedUDCEP *e = opaque;
>       AspeedUDCState *s = container_of(e - e->index, AspeedUDCState, ep[0]);
>       uint32_t reg = offset >> 2;
> +    uint32_t old_val = e->regs[reg];
>       uint32_t val = data;
>   
>       trace_aspeed_udc_ep_write(e->index, offset, val);
> @@ -725,12 +728,12 @@ static void aspeed_udc_ep_write(void *opaque, hwaddr offset, uint64_t data,
>           break;
>       case R_EP_DMA_STS:
>           val &= 0x77ffffff;
> +        e->regs[reg] = val;
>           if (FIELD_EX32(e->regs[R_EP_DMA_CTRL], EP_DMA_CTRL, DESC_OP_EN)) {
>               /* IN, descriptor-list mode */
> -            aspeed_udc_ep_in_kick_desc(s, e->index, val);
> +            aspeed_udc_ep_in_kick_desc(s, e->index, old_val);
>           } else {
>               /* OUT, single-stage mode */
> -            e->regs[reg] = val;
>               aspeed_udc_ep_out_kick_single(s, e->index);
>           }
>           break;

Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.

Re: [PATCH v2 10/11] hw/arm/aspeed_ast10x0: Add the USB device controller

[Cédric Le Goater]

On 10/5/26 08:44, Jamin Lin wrote:
> Add the USB UDC device at 0x7E6A2000 with IRQ 9. The AST10x0 UDC
> controller is compatible with the AST2600 one, so reuse TYPE_ASPEED_UDC.
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> ---
>   include/hw/arm/aspeed_soc.h |  1 +
>   hw/arm/aspeed_ast10x0.c     | 17 +++++++++++++----
>   2 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
> index 38226d9b80..4459188c57 100644
> --- a/include/hw/arm/aspeed_soc.h
> +++ b/include/hw/arm/aspeed_soc.h
> @@ -167,6 +167,7 @@ struct Aspeed10x0SoCState {
>       AspeedSoCState parent;
>   
>       ARMv7MState armv7m;
> +    AspeedUDCState udc;
>   };
>   
>   #define TYPE_ASPEED10X0_SOC "aspeed10x0-soc"
> diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
> index aeb5a4423d..81f2d78a45 100644
> --- a/hw/arm/aspeed_ast10x0.c
> +++ b/hw/arm/aspeed_ast10x0.c
> @@ -162,10 +162,11 @@ static void aspeed_soc_ast10x0_init(Object *obj, const char *socname)
>       snprintf(typename, sizeof(typename), "aspeed.hace-%s", socname);
>       object_initialize_child(obj, "hace", &s->hace, typename);
>   
> +    object_initialize_child(obj, "udc", &a->udc, TYPE_ASPEED_UDC);
> +
>       object_initialize_child(obj, "iomem", &s->iomem, TYPE_UNIMPLEMENTED_DEVICE);
>       object_initialize_child(obj, "pwm", &s->pwm, TYPE_UNIMPLEMENTED_DEVICE);
>       object_initialize_child(obj, "espi", &s->espi, TYPE_UNIMPLEMENTED_DEVICE);
> -    object_initialize_child(obj, "udc", &s->udc, TYPE_UNIMPLEMENTED_DEVICE);
>       object_initialize_child(obj, "sgpiom", &s->sgpiom,
>                               TYPE_UNIMPLEMENTED_DEVICE);
>       object_initialize_child(obj, "jtag[0]", &s->jtag[0],
> @@ -394,6 +395,17 @@ static bool aspeed_soc_ast10x0_realize(Aspeed10x0SoCState *a, Error **errp)
>       sysbus_connect_irq(SYS_BUS_DEVICE(&s->gpio), 0,
>                          aspeed_soc_ast1030_get_irq(s, ASPEED_DEV_GPIO));
>   
> +    /* UDC - USB 2.0 Device Controller */
> +    object_property_set_link(OBJECT(&a->udc), "dram",
> +                             OBJECT(&s->sram[0]), &error_abort);
> +    if (!sysbus_realize(SYS_BUS_DEVICE(&a->udc), errp)) {
> +        return false;
> +    }
> +    aspeed_mmio_map(s->memory, SYS_BUS_DEVICE(&a->udc), 0,
> +                    sc->memmap[ASPEED_DEV_UDC]);
> +    sysbus_connect_irq(SYS_BUS_DEVICE(&a->udc), 0,
> +                       aspeed_soc_ast1030_get_irq(s, ASPEED_DEV_UDC));
> +
>       aspeed_mmio_map_unimplemented(s->memory, SYS_BUS_DEVICE(&s->pwm),
>                                     "aspeed.pwm",
>                                     sc->memmap[ASPEED_DEV_PWM], 0x100);
> @@ -402,9 +414,6 @@ static bool aspeed_soc_ast10x0_realize(Aspeed10x0SoCState *a, Error **errp)
>                                     "aspeed.espi",
>                                     sc->memmap[ASPEED_DEV_ESPI], 0x800);
>   
> -    aspeed_mmio_map_unimplemented(s->memory, SYS_BUS_DEVICE(&s->udc),
> -                                  "aspeed.udc",
> -                                  sc->memmap[ASPEED_DEV_UDC], 0x1000);
>       aspeed_mmio_map_unimplemented(s->memory, SYS_BUS_DEVICE(&s->sgpiom),
>                                     "aspeed.sgpiom",
>                                     sc->memmap[ASPEED_DEV_SGPIOM0], 0x100);

Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.

Re: [PATCH v2 09/11] hw/usb/aspeed-udc: Add trace events for the transfer paths

[Cédric Le Goater]

On 10/5/26 08:44, Jamin Lin wrote:
> The register traces show what the guest wrote, but not which transfer
> path the write chose, how much each call moved, or when the host gave up
> on a packet.
> 
> Add one event per path, so the name says which path ran and a single
> path can be enabled on its own: a kick, a transfer result, and a
> cancelled packet.
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> ---
>   hw/usb/aspeed-udc.c | 23 +++++++++++++++++++++++
>   hw/usb/trace-events |  7 +++++++
>   2 files changed, 30 insertions(+)
> 
> diff --git a/hw/usb/aspeed-udc.c b/hw/usb/aspeed-udc.c
> index 09f5913aef..1c99e45d9a 100644
> --- a/hw/usb/aspeed-udc.c
> +++ b/hw/usb/aspeed-udc.c
> @@ -565,6 +565,9 @@ static AspeedUDCXferResult aspeed_udc_ep_xfer_in_desc(AspeedUDCState *s,
>           aspeed_udc_raise_ep_ack(s, ep);
>       }
>   
> +    trace_aspeed_udc_ep_xfer_in_desc(ep, (uint32_t)p->actual_length,
> +                                     done ? "done" : "more");
> +
>       return done ? ASPEED_UDC_XFER_DONE : ASPEED_UDC_XFER_MORE;
>   }
>   
> @@ -606,6 +609,9 @@ static AspeedUDCXferResult aspeed_udc_ep_xfer_in_single(AspeedUDCState *s,
>        * buffer, and the next IN request sends the rest.
>        */
>       if (e->single_buf_off < buf_len) {
> +        trace_aspeed_udc_ep_xfer_in_single(ep,
> +                                           (uint32_t)p->actual_length,
> +                                           "buffer left");
>           return ASPEED_UDC_XFER_DONE;
>       }
>   
> @@ -619,6 +625,9 @@ static AspeedUDCXferResult aspeed_udc_ep_xfer_in_single(AspeedUDCState *s,
>                                           PROC_STS, EP_DMA_CTRL_STS_TX_IDLE);
>       aspeed_udc_raise_ep_ack(s, ep);
>   
> +    trace_aspeed_udc_ep_xfer_in_single(ep, (uint32_t)p->actual_length,
> +                                       "done");
> +
>       return ASPEED_UDC_XFER_DONE;
>   }
>   
> @@ -664,9 +673,15 @@ static AspeedUDCXferResult aspeed_udc_ep_xfer_out_single(AspeedUDCState *s,
>       aspeed_udc_raise_ep_ack(s, ep);
>   
>       if ((uint32_t)p->actual_length >= pktiov->size) {
> +        trace_aspeed_udc_ep_xfer_out_single(ep,
> +                                            (uint32_t)p->actual_length,
> +                                            "done");
>           return ASPEED_UDC_XFER_DONE;
>       }
>   
> +    trace_aspeed_udc_ep_xfer_out_single(ep, (uint32_t)p->actual_length,
> +                                        "more");
> +
>       return ASPEED_UDC_XFER_MORE;
>   }
>   
> @@ -686,6 +701,8 @@ static void aspeed_udc_ep_in_kick_desc(AspeedUDCState *s, int ep,
>       uint32_t cur_rptr = FIELD_EX32(old_val, EP_DMA_STS, RPTR);
>       USBPacket *p = e->pkt;
>   
> +    trace_aspeed_udc_ep_in_kick_desc(ep);
> +
>       /*
>        * A normal kick only sets the write pointer and leaves the read-pointer
>        * field 0 (the read pointer is ours to advance). The guest resets the ring
> @@ -735,6 +752,8 @@ static void aspeed_udc_ep_in_kick_single(AspeedUDCState *s, int ep)
>       AspeedUDCEP *e = &s->ep[ep];
>       USBPacket *p = e->pkt;
>   
> +    trace_aspeed_udc_ep_in_kick_single(ep);
> +
>       /* Do nothing if no host request is waiting, or no buffer was queued. */
>       if (!p || !FIELD_EX32(e->regs[R_EP_DMA_STS], EP_DMA_STS, WPTR)) {
>           return;
> @@ -770,6 +789,8 @@ static void aspeed_udc_ep_out_kick_single(AspeedUDCState *s, int ep)
>       AspeedUDCEP *e = &s->ep[ep];
>       USBPacket *p = e->pkt;
>   
> +    trace_aspeed_udc_ep_out_kick_single(ep);
> +
>       /* nothing to do unless an OUT packet is waiting and a buffer is ready */
>       if (!p || !FIELD_EX32(e->regs[R_EP_DMA_STS], EP_DMA_STS, WPTR)) {
>           return;
> @@ -1155,6 +1176,8 @@ static void aspeed_udc_gadget_cancel_packet(USBDevice *udev, USBPacket *p)
>       AspeedUDCState *s = ASPEED_UDC_GADGET(udev)->udc;
>       int i;
>   
> +    trace_aspeed_udc_cancel_packet(p->ep->nr);
> +
>       if (s->ep0_packet == p) {
>           s->ep0_packet = NULL;
>       }
> diff --git a/hw/usb/trace-events b/hw/usb/trace-events
> index aa260dacad..661011b07e 100644
> --- a/hw/usb/trace-events
> +++ b/hw/usb/trace-events
> @@ -395,3 +395,10 @@ aspeed_udc_handle_data(int ep_nr, const char *dir, uint32_t iov, int ep_idx) "ep
>   aspeed_udc_ep_data_in(unsigned ep, uint32_t rptr, uint32_t wptr, uint32_t iov) "ep %u, rptr %u, wptr %u, iov %u"
>   aspeed_udc_ep_data_out(unsigned ep, uint32_t wptr, uint32_t avail, uint32_t iov) "ep %u, wptr %u, avail %u, iov %u"
>   aspeed_udc_ep_ack(unsigned ep) "ep %u"
> +aspeed_udc_ep_in_kick_desc(int ep) "ep %d"
> +aspeed_udc_ep_in_kick_single(int ep) "ep %d"
> +aspeed_udc_ep_out_kick_single(int ep) "ep %d"
> +aspeed_udc_ep_xfer_in_desc(int ep, uint32_t sent, const char *res) "ep %d, sent %u, %s"
> +aspeed_udc_ep_xfer_in_single(int ep, uint32_t sent, const char *res) "ep %d, sent %u, %s"
> +aspeed_udc_ep_xfer_out_single(int ep, uint32_t received, const char *res) "ep %d, received %u, %s"
> +aspeed_udc_cancel_packet(int ep) "ep %d"

Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.

Re: [PATCH v2 06/11] hw/usb/aspeed-udc: Pick the EP_DMA_STS path by direction

[Cédric Le Goater]

On 10/5/26 08:44, Jamin Lin wrote:
> DESC_OP_EN is the DMA mode. It does not tell the direction. The write
> handler used it as the direction anyway: set meant IN, clear meant OUT.
> 
> Both directions support both modes. So take the direction from
> EP_CONFIG.DIR_OUT first, then take the mode from DESC_OP_EN.
> 
> The model does not support OUT descriptor mode. Log it as not
> implemented. It does not support IN single stage mode yet either, so
> that path does nothing until the next patch.
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> ---
>   hw/usb/aspeed-udc.c | 15 ++++++++++-----
>   1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/usb/aspeed-udc.c b/hw/usb/aspeed-udc.c
> index 4572914e16..c48e2c5f1b 100644
> --- a/hw/usb/aspeed-udc.c
> +++ b/hw/usb/aspeed-udc.c
> @@ -729,12 +729,17 @@ static void aspeed_udc_ep_write(void *opaque, hwaddr offset, uint64_t data,
>       case R_EP_DMA_STS:
>           val &= 0x77ffffff;
>           e->regs[reg] = val;
> -        if (FIELD_EX32(e->regs[R_EP_DMA_CTRL], EP_DMA_CTRL, DESC_OP_EN)) {
> -            /* IN, descriptor-list mode */
> -            aspeed_udc_ep_in_kick_desc(s, e->index, old_val);
> -        } else {
> -            /* OUT, single-stage mode */
> +        if (FIELD_EX32(e->regs[R_EP_CONFIG], EP_CONFIG, DIR_OUT)) {
> +            if (FIELD_EX32(e->regs[R_EP_DMA_CTRL], EP_DMA_CTRL, DESC_OP_EN)) {
> +                qemu_log_mask(LOG_UNIMP, "%s: ep %d OUT descriptor mode is not"
> +                              " implemented\n", __func__, e->index);
> +                break;
> +            }
>               aspeed_udc_ep_out_kick_single(s, e->index);
> +        } else {
> +            if (FIELD_EX32(e->regs[R_EP_DMA_CTRL], EP_DMA_CTRL, DESC_OP_EN)) {
> +                aspeed_udc_ep_in_kick_desc(s, e->index, old_val);
> +            }
>           }
>           break;
>       default:

Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.