Hi Romain, On Wed, Aug 19, 2026 at 10:20, Romain Gantois <[email protected]> wrote:
> Hi Mattijs, > > Sorry for the late reply. :/ No worries > > On Friday, 24 July 2026 09:46:07 CEST Mattijs Korpershoek wrote: > ... >> > + .name = _name, \ >> > + .base_addr = _base_addr, \ >> > + .is_double = _is_double, \ >> > + .maxpacket_limit = _maxpacket_limit, \ >> > + } >> >> When comparing with the linux driver, I've noticed that we dropped the >> .caps field here. >> This is fine for now, but at some point we will be updating the UDC core >> in U-Boot, and capabilities might be required. >> >> See the following for some background: >> * >> https://lore.kernel.org/all/20260703-usb-prep-dwc3-sync-v1-2-1352bc238c9b@k >> ernel.org/ * >> https://lore.kernel.org/u-boot/20260629084507.3254232-1-jens.wiklander@lina >> ro.org/ >> >> To be clear: I don't think you need to put endpoint capabilities back in >> the driver right now, but keep this in mind once we merge the UDC core >> upgrade. >> > > ACK! > >> > + > ... >> > +static void usbf_ahb_epc_irq(struct usbf_udc *udc) >> > +{ >> > + unsigned long flags; >> > + struct usbf_ep *epn; >> > + u32 sysbint, sysben; >> > + void (*ep_action)(struct usbf_ep *epn); >> > + int i; >> > + >> > + spin_lock_irqsave(&udc->lock, flags); >> > + >> > + /* Read and ack interrupts */ >> > + sysbint = usbf_reg_readl(udc, USBF_REG_AHBBINT); >> > + sysben = usbf_reg_readl(udc, USBF_REG_AHBBINTEN); >> > + sysbint &= sysben; >> >> In the linux driver, we don't read USBF_REG_AHBBINTEN. Can you explain >> why this is needed here? >> > > This is due to the fact that U-Boot calls this handler in a polling manner. > In > Linux, when we disable bridge interrupts e.g. during DMA transfer operations, > we can be sure that the handler won't get called, since the AHBBINTEN > register > masks interrupts. > > However in U-Boot, the contents of AHBBINTEN do not prevent the handler from > being called since we're continuously calling it in polling mode. We've > actually ran into bugs because of this while testing initial versions of the > driver port. Therefore, we mask AHBBINT with the contents of AHBBINTEN to > prevent bridge events from being serviced when we don't want them to. Ok, since this is a deliberate (and necessary) change because we operate in polling mode, can we please document this in a comment in the driver? That will make it more clear when others read/diff the code against linux. > >> > + usbf_reg_writel(udc, USBF_REG_AHBBINT, sysbint); >> > + >> > + if ((sysbint & USBF_SYS_VBUS_INT) == USBF_SYS_VBUS_INT) { >> > + if (usbf_reg_readl(udc, USBF_REG_EPCTR) & USBF_SYS_VBUS_LEVEL) > { >> > + g_dnl_clear_detach(); >> >> Why do we need to depend on g_dnl_*() ? >> This is the only gadget driver that does this. It seems wrong (g_dnl* is >> usually called from higher up (in u-boot commands or function drivers) >> > > The purpose of this is to signal to function drivers that the VBUS status has > changed (presumably because of a cable removal) and that they should stop > polling the UDC interrupt handler and exit cleanly. This seems like a correct > use of this mechanism to me, but indeed I don't know why other UDC drivers > don't use it this way. I see. Why can't we implement the .vbus_session operation then? See at91_udc.c for example. > >> > + spin_unlock(&udc->lock); >> > + usb_gadget_set_state(&udc->gadget, USB_STATE_POWERED); >> > + spin_lock(&udc->lock); >> > + } else { >> > + g_dnl_trigger_detach(); >> >> The linux driver has some dev_dbg() statements here. Why are they >> removed? >> If we want to clean up dev_dbg(), why don't we remove the others in this >> function as well? >> >> For consistency, I'd prefer for them to stay, please. >> > > That's an oversight on my part, I'll put them back. > >> > + spin_unlock(&udc->lock); >> > + usb_gadget_set_state(&udc->gadget, >> > + USB_STATE_NOTATTACHED); >> > + spin_lock(&udc->lock); >> > + } >> > + } >> > + >> > + for (i = 1; i < ARRAY_SIZE(udc->ep); i++) { >> > + if (sysbint & USBF_SYS_DMA_ENDINT_EPN(i)) { >> > + epn = &udc->ep[i]; >> > + dev_dbg(epn->udc->dev, >> > + "ep%u handle DMA complete. action=%ps\n", >> > + epn->id, epn->bridge_on_dma_end); >> > + ep_action = epn->bridge_on_dma_end; >> > + if (ep_action) { >> > + epn->bridge_on_dma_end = NULL; >> > + ep_action(epn); >> > + } >> > + } >> > + } >> > + >> > + spin_unlock_irqrestore(&udc->lock, flags); >> > +} >> > + >> > +static int usbf_udc_start(struct usb_gadget *gadget, >> > + struct usb_gadget_driver *driver) >> > +{ >> > + struct usbf_udc *udc = container_of(gadget, struct usbf_udc, gadget); >> > + unsigned long flags; >> >> Missing dev_info() from linux here. Please keep it or justify why it >> absolutely needs to be removed. >> > > Ditto, oversight on my part. > >> > + >> > + spin_lock_irqsave(&udc->lock, flags); >> > + >> > + /* hook up the driver */ > ... >> > + >> > +static int usbf_epn_check(struct usbf_ep *epn) >> > +{ >> > + u32 ctrl; >> >> Why can't we keep the same error handling as linux, which contains a >> dev_dbg() statement with some information about the endpoint? >> > > You're right, we can probably keep that dev_dbg(). > >> > + >> > + ctrl = usbf_ep_reg_readl(epn, USBF_REG_EPN_CONTROL); >> > + > ... >> > + udc->gadget.name = dev->driver->name; >> > + udc->gadget.ep0 = &udc->ep[0].ep; >> > + >> > + INIT_LIST_HEAD(&udc->gadget.ep_list); >> > + /* we have a canned request structure to allow sending packets as > reply >> > + * to get_status requests >> > + */ >> > + INIT_LIST_HEAD(&udc->setup_reply.queue); >> > + >> > + for (i = 0; i < ARRAY_SIZE(udc->ep); i++) { >> > + ep = &udc->ep[i]; >> > + >> > + ep->disabled = 1; >> >> Why has the ep->disabled been moved to a an earler place compared with >> linux? >> >> In linux it's lower, part with the other assignments. > > When "ep->disabled = 1" is placed after the endpoint availability check, this > means that we can end up with endpoint structs referring to nonexistent > hardware which have "disabled" set to zero. In U-Boot this is problematic > because function drivers can call usb_ep_queue() on such an endpoint struct, > causing reads of nonexistent registers. > > In Linux, usb_ep_queue() checks if "ep->enabled" and "ep->address" are valid > before calling into the UDC driver, whereas in U-Boot it does not. Therefore, > this extra precaution is needed in U-Boot. Ok, that makes sense. Thanks > > Thanks, > > -- > Romain Gantois, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
