Re: [PATCH v3 2/2] usb: gadget: Add Renesas RZ/N1 USBF controller support

Romain Gantois <[email protected]> Wed, 19 Aug 2026 10:20:35 +0200
Newsgroups	gmane.comp.boot-loaders.u-boot.general,gmane.comp.boot-loaders.u-boot
Message-ID	<[email protected]>
Hi Mattijs,

Sorry for the late reply. :/

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.

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

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

Thanks,

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
signature.asc (application/pgp-signature, 833 B)