[PATCH 1/3] usb: udc: ci: support USB gadget driver model

[Sam Day]

Heya Stephan,

On Wed Apr 16, 2025 at 8:51 PM CEST, Stephan Gerhold wrote:
> 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 <me at 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.

\0/ Thanks for the review and testing!

>
>> ---
>>  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?

Ah, I had converted a few of these when checkpatch reprimanded me. Guess
I missed this one - oops!

>
> 	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?

Thanks, I've done this in v2.

>
>> +
>> +	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.

So, not quite unfortunately. In the case of ci_udc_gadget_stop(), the
controller.driver = NULL happening before calling into udc_disconnect()
is important: the generic UDC uclass code has already called
driver->disconnect() and calling it again is a Very Bad Thing to do. So
we can't just delegate usb_gadget_unregister_driver to
ci_udc_gadget_stop outright.

In v2 I've tried to unify the codepaths as much as possible though,
both to avoid duplication and the risks of future patches making
incomplete 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?

Good call, these ones are less problematic, and I've done that in v2.

Rock on \m/,
-Sam

>
> Thanks,
> Stephan