Hi Cédric, Philippe
> Subject: 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.
>
>
Thanks for the review and suggestion.
Will fix it.
Jamin
> >
> >> + 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 */
> >
