Re: [PATCH RFC] usb: gadget: atmel: Add DM_USB_GADGET support

On 7/20/24 12:35 AM, Zixun Li wrote:

Add driver model support by using the uclass UCLASS_USB_GADGET_GENERIC.

Disable local usb_gadget_register_driver()/usb_gadget_unregister_driver() implementation which is implemented in udc-core.c when DM_USB_GADGET is enabled.

Compared to Linux driver sam9x60 and sama7g5 DT bindings are not included as they are not supported by the driver yet.

Signed-off-by: Zixun LI zli@ogga.fr

+CC Mattijs

---

drivers/usb/gadget/atmel_usba_udc.c | 196 ++++++++++++++++++++++++---- 1 file changed, 169 insertions(+), 27 deletions(-)

diff --git a/drivers/usb/gadget/atmel_usba_udc.c b/drivers/usb/gadget/atmel_usba_udc.c index f99553df8d..40e8b895a0 100644 --- a/drivers/usb/gadget/atmel_usba_udc.c +++ b/drivers/usb/gadget/atmel_usba_udc.c @@ -17,9 +17,19 @@ #include <linux/usb/gadget.h> #include <linux/usb/atmel_usba_udc.h> #include <malloc.h> +#include <clk.h> +#include <dm.h> +#include <dm/device_compat.h> +#include <dm/devres.h>

Keep the list sorted

+#if CONFIG_IS_ENABLED(DM_USB_GADGET) +#include <mach/atmel_usba_udc.h> +#endif /* CONFIG_IS_ENABLED(DM_USB_GADGET) */

#include "atmel_usba_udc.h"

+static struct usba_udc *controller;

• static int vbus_is_present(struct usba_udc *udc) { /* No Vbus detection: Assume always present */

@@ -506,12 +516,6 @@ usba_udc_set_selfpowered(struct usb_gadget *gadget, int is_selfpowered) return 0; }

-static const struct usb_gadget_ops usba_udc_ops = {

• .get_frame = usba_udc_get_frame,
• .wakeup = usba_udc_wakeup,
• .set_selfpowered = usba_udc_set_selfpowered,

-};

• static struct usb_endpoint_descriptor usba_ep0_desc = { .bLength = USB_DT_ENDPOINT_SIZE, .bDescriptorType = USB_DT_ENDPOINT,

@@ -1153,7 +1157,7 @@ static int usba_udc_irq(struct usba_udc *udc) return 0; }

-static int atmel_usba_start(struct usba_udc *udc) +static int usba_enable(struct usba_udc *udc)

Please factor out such rename (if it is even necessary) into a separate preparatory patch, it makes the review easier.

{ udc->devstatus = 1 << USB_DEVICE_SELF_POWERED;

@@ -1168,7 +1172,7 @@ static int atmel_usba_start(struct usba_udc *udc) return 0; }

-static int atmel_usba_stop(struct usba_udc *udc) +static int usba_disable(struct usba_udc *udc) { udc->gadget.speed = USB_SPEED_UNKNOWN; reset_all_endpoints(udc); @@ -1179,28 +1183,29 @@ static int atmel_usba_stop(struct usba_udc *udc) return 0; }

-static struct usba_udc controller = {

• .regs = (unsigned *)ATMEL_BASE_UDPHS,
• .fifo = (unsigned *)ATMEL_BASE_UDPHS_FIFO,
• .gadget = {

• .ops		= &usba_udc_ops,
  

• .ep_list	= LIST_HEAD_INIT(controller.gadget.ep_list),
  

• .speed		= USB_SPEED_HIGH,
  

• .is_dualspeed	= 1,
  

• .name		= "atmel_usba_udc",
  

• },

-}; +static int usba_gadget_pullup(struct usb_gadget *g, int is_on) +{

• struct usba_udc *udc = controller;
• u32 ctrl;
• ctrl = usba_readl(udc, CTRL);

if (is_on) ctrl &= ~USBA_DETACH; else ctrl |= USBA_DETACH;

usba_writel(udc, ctrl);

Should help readability.

If this is moved code, do this in a preparatory patch please.

• usba_writel(udc, CTRL, is_on ? ctrl & ~USBA_DETACH : ctrl | USBA_DETACH);
• return 0;

+}

int dm_usb_gadget_handle_interrupts(struct udevice *dev) {

• struct usba_udc *udc = &controller;

• struct usba_udc *udc = controller;

  return usba_udc_irq(udc); }

+#if !CONFIG_IS_ENABLED(DM_USB_GADGET) int usb_gadget_register_driver(struct usb_gadget_driver *driver) {

• struct usba_udc *udc = &controller;

• struct usba_udc *udc = controller; int ret;

  if (!driver || !driver->bind || !driver->setup) {

@@ -1213,7 +1218,7 @@ int usb_gadget_register_driver(struct usb_gadget_driver *driver) return -EBUSY; }

• atmel_usba_start(udc);

• usba_enable(udc);

Should be in preparatory patch.

udc->driver = driver;

@@ -1228,7 +1233,7 @@ int usb_gadget_register_driver(struct usb_gadget_driver *driver)

int usb_gadget_unregister_driver(struct usb_gadget_driver *driver) {

• struct usba_udc *udc = &controller;

• struct usba_udc *udc = controller;

Should be in one of preparatory patches.

if (!driver || !driver->unbind || !driver->disconnect) { pr_err("bad paramter\n"); @@ -1239,11 +1244,54 @@ int usb_gadget_unregister_driver(struct usb_gadget_driver *driver) driver->unbind(&udc->gadget); udc->driver = NULL;

• atmel_usba_stop(udc);

• usba_disable(udc);
• return 0;

+} +#else /* !CONFIG_IS_ENABLED(DM_USB_GADGET) */

+static int usba_gadget_start(struct usb_gadget *g,

• 	     struct usb_gadget_driver *driver)
  

+{

• struct usba_udc *udc = controller;
• if (!driver || !driver->bind || !driver->setup) {

• printf("bad parameter\n");
  

Use log_ or dev_err

• return -EINVAL;
  

• }
• if (udc->driver) {

• printf("UDC already has a gadget driver\n");
  

• return -EBUSY;
  

• }
• usba_enable(udc);
• udc->driver = driver;
• return 0;

+}

+static int usba_gadget_stop(struct usb_gadget *g) +{

• struct usba_udc *udc = controller;

• if (!udc)

• return -ENODEV;
  

• if (!udc->driver)

• return -EINVAL;
  

• udc->driver->disconnect(&udc->gadget);

• udc->driver = NULL;

• usba_disable(udc);

  return 0; }

+#endif /* !CONFIG_IS_ENABLED(DM_USB_GADGET) */

• static struct usba_ep *usba_udc_pdata(struct usba_platform_data *pdata, struct usba_udc *udc) {

@@ -1284,13 +1332,107 @@ static struct usba_ep *usba_udc_pdata(struct usba_platform_data *pdata, return eps; }

+static const struct usb_gadget_ops usba_udc_ops = {

• .get_frame = usba_udc_get_frame,
• .wakeup = usba_udc_wakeup,
• .set_selfpowered = usba_udc_set_selfpowered,
• .pullup = usba_gadget_pullup,

+#if CONFIG_IS_ENABLED(DM_USB_GADGET)

• .udc_start = usba_gadget_start,
• .udc_stop = usba_gadget_stop,

+#endif +};

+static struct usba_udc udc_controller = {

• .regs = (unsigned int *)ATMEL_BASE_UDPHS,
• .fifo = (unsigned int *)ATMEL_BASE_UDPHS_FIFO,
• .gadget = {

• .ops		= &usba_udc_ops,
  

• .ep_list	= LIST_HEAD_INIT(udc_controller.gadget.ep_list),
  

• .speed		= USB_SPEED_HIGH,
  

• .is_dualspeed	= 1,
  

• .name		= "atmel_usba_udc",
  

• },

+};

• int usba_udc_probe(struct usba_platform_data *pdata) {

• struct usba_udc *udc;

• controller = &udc_controller;
• controller->usba_ep = usba_udc_pdata(pdata, controller);
• return 0;

+}

+#if CONFIG_IS_ENABLED(DM_USB_GADGET) +struct usba_priv_data {

• struct clk_bulk clks;

+};

• udc = &controller;

+static int atmel_usba_udc_clk_init(struct udevice *dev, struct clk_bulk *clks) +{

• int ret;

• udc->usba_ep = usba_udc_pdata(pdata, udc);

• ret = clk_get_bulk(dev, clks);

• if (ret == -ENOSYS)

• return 0;
  

• if (ret)

• return ret;
  

• ret = clk_enable_bulk(clks);

• if (ret) {

• clk_release_bulk(clks);
  

• return ret;
  

• }

  return 0; }

+static int atmel_usba_udc_probe(struct udevice *dev) +{

• struct usba_priv_data *priv = dev_get_priv(dev);
• int ret;
• ret = atmel_usba_udc_clk_init(dev, &priv->clks);
• if (ret)

• return ret;
  

• ret = usba_udc_probe(&pdata);
• if (ret)

• return ret;
  

• controller->driver = 0;
• ret = usb_add_gadget_udc((struct device *)dev, &controller->gadget);
• return ret;

+}

+static int atmel_usba_udc_remove(struct udevice *dev) +{

• struct usba_priv_data *priv = dev_get_priv(dev);
• usb_del_gadget_udc(&controller->gadget);
• clk_release_bulk(&priv->clks);
• return dm_scan_fdt_dev(dev);

+}

+static const struct udevice_id atmel_usba_udc_ids[] = {

• { .compatible = "atmel,at91sam9rl-udc" },
• { .compatible = "atmel,at91sam9g45-udc" },
• { .compatible = "atmel,sama5d3-udc" },

Keep the list sorted.

• {}

+};

+U_BOOT_DRIVER(ateml_usba_udc) = {

• .name = "ateml_usba_udc",
• .id = UCLASS_USB_GADGET_GENERIC,
• .of_match = atmel_usba_udc_ids,
• .probe = atmel_usba_udc_probe,
• .remove = atmel_usba_udc_remove,
• .priv_auto = sizeof(struct usba_priv_data),

+}; +#endif /* CONFIG_IS_ENABLED(DM_USB_GADGET) */

Looks pretty good otherwise, thanks !

Don't forget to CC Mattijs on V2