Hi Romain, Thank you for the patch. I've diffed this against the linux driver, and have some comments below.
On Thu, Jul 23, 2026 at 11:09, Romain Gantois <[email protected]> wrote: > From: Herve Codina <[email protected]> > > Add support for the Renesas USBF controller. This is an USB2.0 UDC > controller available in the RZ/N1 SoC. > > This driver was originally written by Hervé Codina for the Linux kernel. > > Linux revision this was ported from: > > 74851fbb6d64 ("usb: gadget: renesas_usbf: Handle devm_pm_runtime_enable() > errors") > > The following features were removed to reduce the memory footprint of the > driver, and to avoid including features not relevant to U-Boot: > > - Suspend/resume > - Handling of endpoints > 2 > - Dynamic detection of endpoint DMA capability (DMA capability is > predetermined for each endpoint anyway). > > Signed-off-by: Herve Codina <[email protected]> > Co-developed-by: Romain Gantois (Schneider Electric) > <[email protected]> > Signed-off-by: Romain Gantois (Schneider Electric) > <[email protected]> > Tested-by: Ralph Siemsen <[email protected]> > --- > MAINTAINERS | 5 + > drivers/usb/gadget/Kconfig | 9 + > drivers/usb/gadget/Makefile | 1 + > drivers/usb/gadget/renesas_usbf.c | 3118 > +++++++++++++++++++++++++++++++++++++ > 4 files changed, 3133 insertions(+) > [...] > +}; > + > +#define USBF_SINGLE_BUFFER 0 > +#define USBF_DOUBLE_BUFFER 1 > +#define USBF_EP_INFO(_name, _base_addr, _is_double, _maxpacket_limit) \ > + { \ > + .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/[email protected]/ * https://lore.kernel.org/u-boot/[email protected]/ 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. > + > +/* This table is computed from the recommended values provided in the SOC > + * datasheet. The buffer type (single/double) and the endpoint type cannot > + * be changed. The mapping in internal RAM (base_addr and number of words) > + * for each endpoints depends on the max packet size and the buffer type. > + */ > +static const struct usbf_ep_info usbf_ep_info[USBF_NUM_ENDPOINTS] = { > + /* ep0: buf @0x0000 64 bytes, fixed 32 words */ > + [0] = USBF_EP_INFO("ep0-ctrl", > + 0x0000, USBF_SINGLE_BUFFER, USBF_EP0_MAX_PCKT_SIZE), > + /* ep1: buf @0x0020, 2 buffers 512 bytes -> (512 * 2 / 4) words */ > + [1] = USBF_EP_INFO("ep1-bulk", > + 0x0020, USBF_DOUBLE_BUFFER, 512), > + /* ep2: buf @0x0120, 2 buffers 512 bytes -> (512 * 2 / 4) words */ > + [2] = USBF_EP_INFO("ep2-bulk", > + 0x0120, USBF_DOUBLE_BUFFER, 512), > +}; [..] > + > + spin_unlock_irqrestore(&udc->lock, flags); > +} > + > +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? > + 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) > + 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. > + 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. > + > + spin_lock_irqsave(&udc->lock, flags); > + > + /* hook up the driver */ > + udc->driver = driver; > + > + /* Enable VBUS interrupt */ > + usbf_reg_writel(udc, USBF_REG_AHBBINTEN, USBF_SYS_VBUS_INTEN); > + > + spin_unlock_irqrestore(&udc->lock, flags); > + > + return 0; > +} > + > +static int usbf_udc_stop(struct usb_gadget *gadget) > +{ > + struct usbf_udc *udc = container_of(gadget, struct usbf_udc, gadget); > + unsigned long flags; > + > + spin_lock_irqsave(&udc->lock, flags); > + > + /* Disable VBUS interrupt */ > + usbf_reg_writel(udc, USBF_REG_AHBBINTEN, 0); > + > + udc->driver = NULL; > + > + spin_unlock_irqrestore(&udc->lock, flags); Same here > + > + return 0; > +} > + > +static int usbf_get_frame(struct usb_gadget *gadget) > +{ > + struct usbf_udc *udc = container_of(gadget, struct usbf_udc, gadget); > + > + return USBF_USB_GET_FRAME(usbf_reg_readl(udc, USBF_REG_USB_ADDRESS)); > +} > + [...] > + > +static struct usb_gadget_ops usbf_gadget_ops = { > + .get_frame = usbf_get_frame, > + .pullup = usbf_pullup, > + .udc_start = usbf_udc_start, > + .udc_stop = usbf_udc_stop, > +}; > + > +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? > + > + ctrl = usbf_ep_reg_readl(epn, USBF_REG_EPN_CONTROL); > + > + if ((ctrl & USBF_EPN_MODE_MASK) != USBF_EPN_MODE_BULK) { > + dev_err(epn->udc->dev, "ep%u unknown type\n", epn->id); > + return -EINVAL; > + } > + > + if (!(ctrl & USBF_EPN_BUF_TYPE_DOUBLE)) { > + dev_err(epn->udc->dev, > + "ep%u buffer mismatch, single expected\n", > + epn->id); > + return -EINVAL; > + } > + > + return 0; > +} > + > +static int usbf_probe(struct udevice *dev) > +{ > + struct usbf_udc *udc; > + struct usbf_ep *ep; > + unsigned int i; > + int ret; > + > + udc = dev_get_priv(dev); > + udc->dev = dev; > + spin_lock_init(&udc->lock); > + > + udc->regs = dev_read_addr_ptr(dev); > + if (!udc->regs) > + return -EINVAL; > + > + /* Resetting the PLL is handled via the clock driver as it has common > + * registers with USB Host > + */ > + usbf_reg_bitclr(udc, USBF_REG_EPCTR, USBF_SYS_EPC_RST); > + > + /* modify in register gadget process */ > + udc->gadget.speed = USB_SPEED_FULL; > + udc->gadget.max_speed = USB_SPEED_HIGH; > + udc->gadget.is_dualspeed = 1; > + udc->gadget.ops = &usbf_gadget_ops; > + > + 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. > + > + if (!(usbf_reg_readl(udc, USBF_REG_USBSSCONF) & > + USBF_SYS_EP_AVAILABLE(i))) { > + continue; > + } > + > + INIT_LIST_HEAD(&ep->queue); > + > + ep->id = i;
