Re: [PATCH v1 1/3] hw/usb/aspeed-udc: Add ASPEED UDC device controller

On 7/9/26 23:03, Philippe Mathieu-Daudé wrote:
> Hi Jamin,
> 
> On 3/7/26 09:43, Jamin Lin wrote:
>> The AST2600 has a USB 2.0 Device Controller (UDC) at 0x1e6a2000 with one
>> control endpoint and four programmable endpoints, driven by the Linux
>> "aspeed_udc" gadget driver.
>>
>> Add the controller as a sysbus (system) device: the MMIO register map
>> described with the registerfields macros, the interrupt line and the
>> soft reset. This is only the register/system side.
>>
>> Note: this "device controller" is the system-bus device (TYPE_ASPEED_UDC).
>> It is not the gadget USB device (TYPE_ASPEED_UDC_DEV) that a host
>> controller enumerates, which is added in the next patch.
>>
>> Signed-off-by: Jamin Lin <[email protected]>
>> ---
>>   hw/arm/Kconfig              |   1 +
>>   hw/usb/Kconfig              |   4 +
>>   hw/usb/aspeed-udc.c         | 264 ++++++++++++++++++++++++++++++++++++
>>   hw/usb/meson.build          |   1 +
>>   hw/usb/trace-events         |   7 +
>>   include/hw/usb/aspeed-udc.h |  54 ++++++++
>>   6 files changed, 331 insertions(+)
>>   create mode 100644 hw/usb/aspeed-udc.c
>>   create mode 100644 include/hw/usb/aspeed-udc.h
> 
> 
>> +static void aspeed_udc_realize(DeviceState *dev, Error **errp)
>> +{
>> +    AspeedUDCState *s = ASPEED_UDC(dev);
>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>> +    int i;
>> +
>> +    s->regs = g_new0(uint32_t, ASPEED_UDC_NR_REGS);
>> +
>> +    memory_region_init(&s->iomem, OBJECT(s), TYPE_ASPEED_UDC,
>> +                       ASPEED_UDC_REG_SIZE);
>> +
>> +    /* Root/global registers occupy the low part of the window */
>> +    memory_region_init_io(&s->reg_mr, OBJECT(s), &aspeed_udc_ops, s,
>> +                          TYPE_ASPEED_UDC ".regs", ASPEED_UDC_NR_REGS << 2);
>> +    memory_region_add_subregion(&s->iomem, 0, &s->reg_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);
>> +
>> +        s->ep[i].index = i;
>> +        s->ep[i].regs = g_new0(uint32_t, ASPEED_UDC_EP_NR_REGS);
> 
> Why allocate a fixed size? Otherwise LGTM.

I agree. The rest looks good.

Thanks,

C.


> 
>> +        memory_region_init_io(&s->ep[i].mr, OBJECT(s), &aspeed_udc_ep_ops,
>> +                              &s->ep[i], name, ASPEED_UDC_EP_NR_REGS << 2);
>> +        memory_region_add_subregion(&s->iomem, ASPEED_UDC_EP_REG_BASE +
>> +                                    i * ASPEED_UDC_EP_REG_SIZE, &s->ep[i].mr);
>> +    }
>> +
>> +    sysbus_init_mmio(sbd, &s->iomem);
>> +    sysbus_init_irq(sbd, &s->irq);
>> +}
> 
>> +static void aspeed_udc_unrealize(DeviceState *dev)
>> +{
>> +    AspeedUDCState *s = ASPEED_UDC(dev);
>> +    int i;
>> +
>> +    for (i = 0; i < ASPEED_UDC_NUM_EP; i++) {
>> +        g_free(s->ep[i].regs);
>> +    }
>> +    g_free(s->regs);
>> +}
> 
>> +/*
>> + * EP0 (control) is served through the root registers (UDC_EP0_*), so only
>> + * the 4 programmable endpoints get their own register bank / ep[] entry.
>> + */
>> +#define ASPEED_UDC_NUM_EP       4
>> +/* 32-bit registers per programmable endpoint */
>> +#define ASPEED_UDC_EP_NR_REGS   4
>> +
>> +/*
>> + * The root/global register block spans 0x000...0x087: the SETUP data buffer
>> + * ends at 0x84. Size the backing array to cover the whole block.
>> + */
>> +#define ASPEED_UDC_NR_REGS      (0x88 >> 2)
>> +
>> +/* MMIO window: root registers below EP_REG_BASE, then the per-EP banks */
>> +#define ASPEED_UDC_REG_SIZE     0x300
>> +#define ASPEED_UDC_EP_REG_BASE  0x200
>> +#define ASPEED_UDC_EP_REG_SIZE  0x10
>> +
>> +typedef struct AspeedUDCEP {
>> +    MemoryRegion mr;
>> +    int index;
>> +    uint32_t *regs;
>> +} AspeedUDCEP;
>> +
>> +struct AspeedUDCState {
>> +    SysBusDevice parent_obj;
>> +
>> +    /* container: root registers + per-endpoint banks */
>> +    MemoryRegion iomem;
>> +    MemoryRegion reg_mr;
>> +    qemu_irq irq;
>> +    uint32_t *regs;
>> +    AspeedUDCEP ep[ASPEED_UDC_NUM_EP];
>> +};
>> +
>> +#endif /* HW_USB_ASPEED_UDC_H */
>