Hi Sam, Thank you for the patch. Sorry for the delays in the review for this one.
On Wed, May 27, 2026 at 00:37, Sam Day <[email protected]> wrote: > When CONFIG_DM_USB_GADGET is enabled, a UCLASS_USB_GADGET_GENERIC driver > will be defined that wraps the ChipIdea UDC operations. The > (dm_)?usb_gadget_.* symbols will no longer be defined (as these are now > handled by the UDC uclass). > > If CONFIG_DM_USB_GADGET is not enabled, this driver behaves as it > previously did. > > This new driver does not declare any compatibles of its own. It requires > a glue driver to bind it. The ehci_msm driver will be updated in the > following commit to do exactly that. > > Signed-off-by: Sam Day <[email protected]> > --- > drivers/usb/gadget/Kconfig | 1 - > drivers/usb/gadget/ci_udc.c | 118 > ++++++++++++++++++++++++++++++++++++-------- > 2 files changed, 97 insertions(+), 22 deletions(-) > > diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig > index 5390878254a..6ba558cc292 100644 > --- a/drivers/usb/gadget/Kconfig > +++ b/drivers/usb/gadget/Kconfig > @@ -153,7 +153,6 @@ config USB_GADGET_OS_DESCRIPTORS > > config CI_UDC > bool "ChipIdea device controller" > - depends on !DM_USB_GADGET > select USB_GADGET_DUALSPEED > help > Say Y here to enable device controller functionality of the > diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c > index 4729570c525..42028e11d59 100644 > --- a/drivers/usb/gadget/ci_udc.c > +++ b/drivers/usb/gadget/ci_udc.c > @@ -10,6 +10,7 @@ > #include <command.h> > #include <config.h> > #include <cpu_func.h> > +#include <dm.h> > #include <net.h> > #include <malloc.h> > #include <wait_bit.h> > @@ -94,8 +95,17 @@ static struct usb_request * > ci_ep_alloc_request(struct usb_ep *ep, unsigned int gfp_flags); > static void ci_ep_free_request(struct usb_ep *ep, struct usb_request *_req); > > +#if CONFIG_IS_ENABLED(DM_USB_GADGET) > +static int ci_udc_gadget_start(struct usb_gadget *g, struct > usb_gadget_driver *driver); > +static int ci_udc_gadget_stop(struct usb_gadget *g); > +#endif > + > static const struct usb_gadget_ops ci_udc_ops = { > .pullup = ci_pullup, > +#if CONFIG_IS_ENABLED(DM_USB_GADGET) > + .udc_start = ci_udc_gadget_start, > + .udc_stop = ci_udc_gadget_stop, > +#endif > }; > > static const struct usb_ep_ops ci_ep_ops = { > @@ -978,7 +988,7 @@ void udc_irq(void) > } > } > > -int dm_usb_gadget_handle_interrupts(struct udevice *dev) > +static int ci_udc_handle_interrupts(struct udevice *dev) > { > struct ci_udc *udc = (struct ci_udc *)controller.ctrl->hcor; > u32 value; > @@ -1037,7 +1047,7 @@ static int ci_pullup(struct usb_gadget *gadget, int > is_on) > static int ci_udc_probe(void) > { > struct ept_queue_head *head; > - int i; > + int i, ret; > > const int num = 2 * NUM_ENDPOINTS; > > @@ -1046,6 +1056,14 @@ static int ci_udc_probe(void) > const int eplist_raw_sz = num * sizeof(struct ept_queue_head); > const int eplist_sz = roundup(eplist_raw_sz, ARCH_DMA_MINALIGN); > > + if (IS_ENABLED(CONFIG_DM_USB)) > + ret = usb_setup_ehci_gadget(&controller.ctrl); > + else > + ret = usb_lowlevel_init(0, USB_INIT_DEVICE, (void > **)&controller.ctrl); > + > + if (ret) > + return ret; > + What happens if later down in ci_udc_probe() we fail to finish the function sucessfully? For example, memalign(ILIST_ALIGN, ILIST_SZ) can return -ENOMEM. Doesn't that leaves us with an usb that's halfway configured? Shouldn't we add some error handling / do some cleanup in that case? > /* The QH list must be aligned to 4096 bytes. */ > controller.epts = memalign(eplist_align, eplist_sz); > if (!controller.epts) > @@ -1123,6 +1141,73 @@ static int ci_udc_probe(void) > return 0; > } > > +static void ci_udc_remove(void) > +{ > + if (IS_ENABLED(CONFIG_DM_USB)) { > + usb_remove_ehci_gadget(&controller.ctrl); > + } else { > + usb_lowlevel_stop(0); > + controller.ctrl = NULL; > + } > +} > + > +static void ci_udc_stop(void) > +{ > + ci_ep_free_request(&controller.ep[0].ep, &controller.ep0_req->req); > + free(controller.items_mem); > + free(controller.epts); It feels a bit weird to free the endpoint request in stop(). Shouldn't we do this in the remove() function to match the ci_ep_alloc_request() which is done in ci_udc_probe()? > +} > + > +static int ci_udc_gadget_start(struct usb_gadget *g, struct > usb_gadget_driver *driver) > +{ > + controller.driver = driver; > + return 0; > +} Nitpick: could we please move this next to ci_udc_gadget_stop() ? I know this would force you to write add controller.driver = driver in usb_gadget_register_driver() but I don't this it's a problem given that we do something similar in usb_gadget_unregister_driver(). > + > +#if CONFIG_IS_ENABLED(DM_USB_GADGET) > +static int ci_udc_generic_probe(struct udevice *dev) > +{ > + int ret = ci_udc_probe(); > + > + if (ret) > + return ret; > + > + return usb_add_gadget_udc((struct device *)dev, &controller.gadget); > +} > + > +static int ci_udc_generic_remove(struct udevice *dev) > +{ > + usb_del_gadget_udc(&controller.gadget); > + > + ci_udc_remove(); > + > + return 0; > +} > + > +static const struct usb_gadget_generic_ops ci_udc_generic_ops = { > + .handle_interrupts = ci_udc_handle_interrupts, > +}; > + > +U_BOOT_DRIVER(ci_udc_generic) = { > + .name = "ci-udc", > + .id = UCLASS_USB_GADGET_GENERIC, > + .ops = &ci_udc_generic_ops, > + .probe = ci_udc_generic_probe, > + .remove = ci_udc_generic_remove, > +}; > + > +static int ci_udc_gadget_stop(struct usb_gadget *g) > +{ > + /* Avoid calling disconnect() twice; the UDC uclass already did it. */ There is no disconnect in udc-uclass.c? Did you mean "UDC core" here instead of "UDC uclass" ? > + controller.driver = NULL; > + udc_disconnect(); > + > + ci_udc_stop(); > + > + return 0; > +} > + > +#else > int usb_gadget_register_driver(struct usb_gadget_driver *driver) > { > int ret; > @@ -1132,14 +1217,6 @@ int usb_gadget_register_driver(struct > usb_gadget_driver *driver) > if (!driver->bind || !driver->setup || !driver->disconnect) > return -EINVAL; > > -#if CONFIG_IS_ENABLED(DM_USB) > - ret = usb_setup_ehci_gadget(&controller.ctrl); > -#else > - ret = usb_lowlevel_init(0, USB_INIT_DEVICE, (void **)&controller.ctrl); > -#endif > - if (ret) > - return ret; > - > ret = ci_udc_probe(); > if (ret) { > DBG("udc probe failed, returned %d\n", ret); > @@ -1151,7 +1228,8 @@ int usb_gadget_register_driver(struct usb_gadget_driver > *driver) > DBG("driver->bind() returned %d\n", ret); > return ret; > } > - controller.driver = driver; > + > + ci_udc_gadget_start(&controller.gadget, driver); > > return 0; > } > @@ -1163,20 +1241,18 @@ int usb_gadget_unregister_driver(struct > usb_gadget_driver *driver) > driver->unbind(&controller.gadget); > controller.driver = NULL; > > - ci_ep_free_request(&controller.ep[0].ep, &controller.ep0_req->req); > - free(controller.items_mem); > - free(controller.epts); > - > -#if CONFIG_IS_ENABLED(DM_USB) > - usb_remove_ehci_gadget(&controller.ctrl); > -#else > - usb_lowlevel_stop(0); > - controller.ctrl = NULL; > -#endif > + ci_udc_stop(); > + ci_udc_remove(); > > return 0; > } > > +int dm_usb_gadget_handle_interrupts(struct udevice *dev) > +{ > + return ci_udc_handle_interrupts(dev); > +} > +#endif > + > bool dfu_usb_get_reset(void) > { > struct ci_udc *udc = (struct ci_udc *)controller.ctrl->hcor; > > -- > 2.54.0
