On Sat, Apr 19, 2025 at 09:32:04AM +0000, Sam Day wrote:
> 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.
Hello Sam,
Are you going to send v2 soon? I am just asking as I had something similar
in the pipeline.
Regrads,
Wojtek
>
> Rock on \m/,
> -Sam
>
> >
> > Thanks,
> > Stephan
>
>
