On Sat, Apr 12, 2025 at 07:39:57PM +0000, Sam Day 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 <m...@samcday.com>
Thanks a lot for working on this. Seems to work without problems on my
dragonboard410c. Just some minor nits below to reduce code duplication.
> ---
> drivers/usb/gadget/ci_udc.c | 84
> ++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 83 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c
> index
> 4bff75da759ded6716641713fbe47f6ba79386dc..179f8540da30c693d11d772aacc00cc37f7669c5
> 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,18 @@ 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 = {
> @@ -927,7 +938,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;
> @@ -1072,6 +1083,71 @@ static int ci_udc_probe(void)
> return 0;
> }
>
> +#if CONFIG_IS_ENABLED(DM_USB_GADGET)
> +static int ci_udc_generic_probe(struct udevice *dev)
> +{
> + int ret;
> +#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
...
> [...]
> +static int ci_udc_generic_remove(struct udevice *dev)
> +{
> + usb_del_gadget_udc(&controller.gadget);
> +
> + if (IS_ENABLED(CONFIG_DM_USB)) {
> + usb_remove_ehci_gadget(&controller.ctrl);
> + } else {
> + usb_lowlevel_stop(0);
> + controller.ctrl = NULL;
> + }
This style is nice for compile testing. But in the probe() function
above you're still using the #if/#else pre-processor style. Can it use
if (IS_ENABLED(...)) too?
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;
This seems to compile at least.
If yes, can you make the same cleanup for the old !DM_USB_GADGET case,
so the two code paths are consistent?
> +
> + 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_start(struct usb_gadget *g,
> + struct usb_gadget_driver *driver)
> +{
> + controller.driver = driver;
> + return 0;
> +}
> +
> +static int ci_udc_gadget_stop(struct usb_gadget *g)
> +{
> + controller.driver = NULL;
> + udc_disconnect();
> +
> + ci_ep_free_request(&controller.ep[0].ep, &controller.ep0_req->req);
> + free(controller.items_mem);
> + free(controller.epts);
> +
> + return 0;
> +}
I wonder if you could just define the start and stop function outside
the #if blocks and call them from usb_gadget_unregister_driver()
and usb_gadget_register_driver(). This would avoid duplicating the code,
reducing the risk that someone forgets to update one of those when
making changes.
The same could be also said for probe()/remove(), i.e. moving the
if (IS_ENABLED(CONFIG_DM_USB)) {
usb_remove_ehci_gadget(&controller.ctrl);
} else {
usb_lowlevel_stop(0);
controller.ctrl = NULL;
}
block to a separate function and calling that from both variants. Not
sure if that would also be worth it?
Thanks,
Stephan
