[PATCH v4 0/7] usb: gadget: atmel: Code refactor and DM_USB_GADGET support

[PATCH v4 0/7] usb: gadget: atmel: Code refactor and DM_USB_GADGET support

[Zixun LI]

Changes in v4:
- Release clocks if probe failed
- Add missing endpoint data free
- Addressed comments

Changes in v3:
- Separate code refactor into individual commits
- Extract the controller point from udevice private data in DM functions

Changes in v2:
- Fix null pointer deference in driver unbinding
- Separate code refactor into 2 parts
- Remove dead code

Changes in v1:
- Based on [PATCH RFC] usb: gadget: atmel: Add DM_USB_GADGET support:
  https://lists.denx.de/pipermail/u-boot/2024-July/559503.html
- Addressed comments, moved the refactoring to a preparatory patch.

Zixun LI (7):
  usb: gadget: atmel: Sort includes
  usb: gadget: atmel: Replace printf() and pr_err() by log_err()
  usb: gadget: atmel: Fix typo in usb gadget driver register and
    unregister
  usb: gadget: atmel: Move usba_udc_pdata() with other static functions
  usb: gadget: atmel: Rename atmel_usba_start()/_stop() to
    usba_udc_enable()/_disable()
  usb: gadget: atmel: Add attach/detach support
  usb: gadget: atmel: Add DM_USB_GADGET support

 drivers/usb/gadget/atmel_usba_udc.c | 256 +++++++++++++++++++++++-----
 drivers/usb/gadget/atmel_usba_udc.h |   3 +
 include/linux/usb/atmel_usba_udc.h  |   2 +
 3 files changed, 214 insertions(+), 47 deletions(-)

--
2.45.2

[PATCH v4 1/7] usb: gadget: atmel: Sort includes

[Zixun LI]

Sort includes in alphabetical order.

Signed-off-by: Zixun LI <admin@hifiphile.com>
---
 drivers/usb/gadget/atmel_usba_udc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/atmel_usba_udc.c b/drivers/usb/gadget/atmel_usba_udc.c
index f99553df8d..5f78251fdb 100644
--- a/drivers/usb/gadget/atmel_usba_udc.c
+++ b/drivers/usb/gadget/atmel_usba_udc.c
@@ -7,16 +7,16 @@
  *			   Bo Shen <voice.shen@atmel.com>
  */
 
-#include <linux/bitops.h>
-#include <linux/errno.h>
+#include <malloc.h>
 #include <asm/gpio.h>
 #include <asm/hardware.h>
+#include <linux/bitops.h>
+#include <linux/errno.h>
 #include <linux/list.h>
 #include <linux/printk.h>
 #include <linux/usb/ch9.h>
 #include <linux/usb/gadget.h>
 #include <linux/usb/atmel_usba_udc.h>
-#include <malloc.h>
 
 #include "atmel_usba_udc.h"
 
-- 
2.45.2

[PATCH v4 2/7] usb: gadget: atmel: Replace printf() and pr_err() by log_err()

[Zixun LI]

To have a uniform printing function, also drop linux/printk.h as no
longer used.

Signed-off-by: Zixun LI <admin@hifiphile.com>
---
 drivers/usb/gadget/atmel_usba_udc.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/gadget/atmel_usba_udc.c b/drivers/usb/gadget/atmel_usba_udc.c
index 5f78251fdb..4641638412 100644
--- a/drivers/usb/gadget/atmel_usba_udc.c
+++ b/drivers/usb/gadget/atmel_usba_udc.c
@@ -7,13 +7,13 @@
  *			   Bo Shen <voice.shen@atmel.com>
  */
 
+#include <log.h>
 #include <malloc.h>
 #include <asm/gpio.h>
 #include <asm/hardware.h>
 #include <linux/bitops.h>
 #include <linux/errno.h>
 #include <linux/list.h>
-#include <linux/printk.h>
 #include <linux/usb/ch9.h>
 #include <linux/usb/gadget.h>
 #include <linux/usb/atmel_usba_udc.h>
@@ -1204,12 +1204,12 @@ int usb_gadget_register_driver(struct usb_gadget_driver *driver)
 	int ret;
 
 	if (!driver || !driver->bind || !driver->setup) {
-		printf("bad paramter\n");
+		log_err("bad paramter\n");
 		return -EINVAL;
 	}
 
 	if (udc->driver) {
-		printf("UDC already has a gadget driver\n");
+		log_err("UDC already has a gadget driver\n");
 		return -EBUSY;
 	}
 
@@ -1219,7 +1219,7 @@ int usb_gadget_register_driver(struct usb_gadget_driver *driver)
 
 	ret = driver->bind(&udc->gadget);
 	if (ret) {
-		pr_err("driver->bind() returned %d\n", ret);
+		log_err("driver->bind() returned %d\n", ret);
 		udc->driver = NULL;
 	}
 
@@ -1231,7 +1231,7 @@ int usb_gadget_unregister_driver(struct usb_gadget_driver *driver)
 	struct usba_udc *udc = &controller;
 
 	if (!driver || !driver->unbind || !driver->disconnect) {
-		pr_err("bad paramter\n");
+		log_err("bad paramter\n");
 		return -EINVAL;
 	}
 
@@ -1252,7 +1252,7 @@ static struct usba_ep *usba_udc_pdata(struct usba_platform_data *pdata,
 
 	eps = malloc(sizeof(struct usba_ep) * pdata->num_ep);
 	if (!eps) {
-		pr_err("failed to alloc eps\n");
+		log_err("failed to alloc eps\n");
 		return NULL;
 	}
 
-- 
2.45.2

[PATCH v4 3/7] usb: gadget: atmel: Fix typo in usb gadget driver register and unregister

[Zixun LI]

Replace "paramter" by "parameter".

Signed-off-by: Zixun LI <admin@hifiphile.com>
---
 drivers/usb/gadget/atmel_usba_udc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/atmel_usba_udc.c b/drivers/usb/gadget/atmel_usba_udc.c
index 4641638412..2e3d8f36a2 100644
--- a/drivers/usb/gadget/atmel_usba_udc.c
+++ b/drivers/usb/gadget/atmel_usba_udc.c
@@ -1204,7 +1204,7 @@ int usb_gadget_register_driver(struct usb_gadget_driver *driver)
 	int ret;
 
 	if (!driver || !driver->bind || !driver->setup) {
-		log_err("bad paramter\n");
+		log_err("bad parameter\n");
 		return -EINVAL;
 	}
 
@@ -1231,7 +1231,7 @@ int usb_gadget_unregister_driver(struct usb_gadget_driver *driver)
 	struct usba_udc *udc = &controller;
 
 	if (!driver || !driver->unbind || !driver->disconnect) {
-		log_err("bad paramter\n");
+		log_err("bad parameter\n");
 		return -EINVAL;
 	}
 
-- 
2.45.2

[PATCH v4 4/7] usb: gadget: atmel: Move usba_udc_pdata() with other static functions

[Zixun LI]

To make all static functions in the top, no functional change.

Signed-off-by: Zixun LI <admin@hifiphile.com>
---
 drivers/usb/gadget/atmel_usba_udc.c | 80 ++++++++++++++---------------
 1 file changed, 40 insertions(+), 40 deletions(-)

diff --git a/drivers/usb/gadget/atmel_usba_udc.c b/drivers/usb/gadget/atmel_usba_udc.c
index 2e3d8f36a2..a3d24501ba 100644
--- a/drivers/usb/gadget/atmel_usba_udc.c
+++ b/drivers/usb/gadget/atmel_usba_udc.c
@@ -1179,6 +1179,46 @@ static int atmel_usba_stop(struct usba_udc *udc)
 	return 0;
 }
 
+static struct usba_ep *usba_udc_pdata(struct usba_platform_data *pdata,
+				      struct usba_udc *udc)
+{
+	struct usba_ep *eps;
+	int i;
+
+	eps = malloc(sizeof(struct usba_ep) * pdata->num_ep);
+	if (!eps) {
+		log_err("failed to alloc eps\n");
+		return NULL;
+	}
+
+	udc->gadget.ep0 = &eps[0].ep;
+
+	INIT_LIST_HEAD(&udc->gadget.ep_list);
+	INIT_LIST_HEAD(&eps[0].ep.ep_list);
+
+	for (i = 0; i < pdata->num_ep; i++) {
+		struct usba_ep *ep = &eps[i];
+
+		ep->ep_regs = udc->regs + USBA_EPT_BASE(i);
+		ep->dma_regs = udc->regs + USBA_DMA_BASE(i);
+		ep->fifo = udc->fifo + USBA_FIFO_BASE(i);
+		ep->ep.ops = &usba_ep_ops;
+		ep->ep.name = pdata->ep[i].name;
+		ep->ep.maxpacket = pdata->ep[i].fifo_size;
+		ep->fifo_size = ep->ep.maxpacket;
+		ep->udc = udc;
+		INIT_LIST_HEAD(&ep->queue);
+		ep->nr_banks = pdata->ep[i].nr_banks;
+		ep->index = pdata->ep[i].index;
+		ep->can_dma = pdata->ep[i].can_dma;
+		ep->can_isoc = pdata->ep[i].can_isoc;
+		if (i)
+			list_add_tail(&ep->ep.ep_list, &udc->gadget.ep_list);
+	};
+
+	return eps;
+}
+
 static struct usba_udc controller = {
 	.regs = (unsigned *)ATMEL_BASE_UDPHS,
 	.fifo = (unsigned *)ATMEL_BASE_UDPHS_FIFO,
@@ -1244,46 +1284,6 @@ int usb_gadget_unregister_driver(struct usb_gadget_driver *driver)
 	return 0;
 }
 
-static struct usba_ep *usba_udc_pdata(struct usba_platform_data *pdata,
-				      struct usba_udc *udc)
-{
-	struct usba_ep *eps;
-	int i;
-
-	eps = malloc(sizeof(struct usba_ep) * pdata->num_ep);
-	if (!eps) {
-		log_err("failed to alloc eps\n");
-		return NULL;
-	}
-
-	udc->gadget.ep0 = &eps[0].ep;
-
-	INIT_LIST_HEAD(&udc->gadget.ep_list);
-	INIT_LIST_HEAD(&eps[0].ep.ep_list);
-
-	for (i = 0; i < pdata->num_ep; i++) {
-		struct usba_ep *ep = &eps[i];
-
-		ep->ep_regs = udc->regs + USBA_EPT_BASE(i);
-		ep->dma_regs = udc->regs + USBA_DMA_BASE(i);
-		ep->fifo = udc->fifo + USBA_FIFO_BASE(i);
-		ep->ep.ops = &usba_ep_ops;
-		ep->ep.name = pdata->ep[i].name;
-		ep->ep.maxpacket = pdata->ep[i].fifo_size;
-		ep->fifo_size = ep->ep.maxpacket;
-		ep->udc = udc;
-		INIT_LIST_HEAD(&ep->queue);
-		ep->nr_banks = pdata->ep[i].nr_banks;
-		ep->index = pdata->ep[i].index;
-		ep->can_dma = pdata->ep[i].can_dma;
-		ep->can_isoc = pdata->ep[i].can_isoc;
-		if (i)
-			list_add_tail(&ep->ep.ep_list, &udc->gadget.ep_list);
-	};
-
-	return eps;
-}
-
 int usba_udc_probe(struct usba_platform_data *pdata)
 {
 	struct usba_udc *udc;
-- 
2.45.2

[PATCH v4 5/7] usb: gadget: atmel: Rename atmel_usba_start()/_stop() to usba_udc_enable()/_disable()

[Zixun LI]

Rename atmel_usba_start() / atmel_usba_stop() to usba_udc_enable()
/ usba_udc_disable(), remove atmel_ prefix to be inline with other
functions. Also avoid confusion with DM start() / stop() functions.

Signed-off-by: Zixun LI <admin@hifiphile.com>
---
 drivers/usb/gadget/atmel_usba_udc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/gadget/atmel_usba_udc.c b/drivers/usb/gadget/atmel_usba_udc.c
index a3d24501ba..ea9ad7585e 100644
--- a/drivers/usb/gadget/atmel_usba_udc.c
+++ b/drivers/usb/gadget/atmel_usba_udc.c
@@ -1153,7 +1153,7 @@ static int usba_udc_irq(struct usba_udc *udc)
 	return 0;
 }
 
-static int atmel_usba_start(struct usba_udc *udc)
+static int usba_udc_enable(struct usba_udc *udc)
 {
 	udc->devstatus = 1 << USB_DEVICE_SELF_POWERED;
 
@@ -1168,7 +1168,7 @@ static int atmel_usba_start(struct usba_udc *udc)
 	return 0;
 }
 
-static int atmel_usba_stop(struct usba_udc *udc)
+static int usba_udc_disable(struct usba_udc *udc)
 {
 	udc->gadget.speed = USB_SPEED_UNKNOWN;
 	reset_all_endpoints(udc);
@@ -1253,7 +1253,7 @@ int usb_gadget_register_driver(struct usb_gadget_driver *driver)
 		return -EBUSY;
 	}
 
-	atmel_usba_start(udc);
+	usba_udc_enable(udc);
 
 	udc->driver = driver;
 
@@ -1279,7 +1279,7 @@ int usb_gadget_unregister_driver(struct usb_gadget_driver *driver)
 	driver->unbind(&udc->gadget);
 	udc->driver = NULL;
 
-	atmel_usba_stop(udc);
+	usba_udc_disable(udc);
 
 	return 0;
 }
-- 
2.45.2

[PATCH v4 6/7] usb: gadget: atmel: Add attach/detach support

[Zixun LI]

Add controller attach/detach support by using
usb_gadget_ops.pullup() function.

Signed-off-by: Zixun LI <admin@hifiphile.com>
---
 drivers/usb/gadget/atmel_usba_udc.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/usb/gadget/atmel_usba_udc.c b/drivers/usb/gadget/atmel_usba_udc.c
index ea9ad7585e..a7b96449f8 100644
--- a/drivers/usb/gadget/atmel_usba_udc.c
+++ b/drivers/usb/gadget/atmel_usba_udc.c
@@ -506,10 +506,28 @@ usba_udc_set_selfpowered(struct usb_gadget *gadget, int is_selfpowered)
 	return 0;
 }
 
+static int usba_udc_pullup(struct usb_gadget *gadget, int is_on)
+{
+	struct usba_udc *udc = to_usba_udc(gadget);
+	u32 ctrl;
+
+	ctrl = usba_readl(udc, CTRL);
+
+	if (is_on)
+		ctrl &= ~USBA_DETACH;
+	else
+		ctrl |= USBA_DETACH;
+
+	usba_writel(udc, CTRL, ctrl);
+
+	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,
+	.pullup			= usba_udc_pullup,
 };
 
 static struct usb_endpoint_descriptor usba_ep0_desc = {
-- 
2.45.2

[PATCH v4 7/7] usb: gadget: atmel: Add DM_USB_GADGET support

[Zixun LI]

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.

Replace dm_usb_gadget_handle_interrupts() with handle_interrupts ops
when DM_USB_GADGET is enabled.

Disable legacy struct usba_udc controller as controller point is extracted
from udevice private data with DM.

Disable legacy usba_udc_probe() to avoid conflict with DM when it's
enabled.

Compared to Linux driver only supported devices' DT bindings are included
(sorted as Linux driver)

Signed-off-by: Zixun LI <admin@hifiphile.com>
---
 drivers/usb/gadget/atmel_usba_udc.c | 144 ++++++++++++++++++++++++++++
 drivers/usb/gadget/atmel_usba_udc.h |   3 +
 include/linux/usb/atmel_usba_udc.h  |   2 +
 3 files changed, 149 insertions(+)

diff --git a/drivers/usb/gadget/atmel_usba_udc.c b/drivers/usb/gadget/atmel_usba_udc.c
index a7b96449f8..a77037a709 100644
--- a/drivers/usb/gadget/atmel_usba_udc.c
+++ b/drivers/usb/gadget/atmel_usba_udc.c
@@ -7,10 +7,14 @@
  *			   Bo Shen <voice.shen@atmel.com>
  */
 
+#include <clk.h>
+#include <dm.h>
 #include <log.h>
 #include <malloc.h>
 #include <asm/gpio.h>
 #include <asm/hardware.h>
+#include <dm/device_compat.h>
+#include <dm/devres.h>
 #include <linux/bitops.h>
 #include <linux/errno.h>
 #include <linux/list.h>
@@ -18,6 +22,14 @@
 #include <linux/usb/gadget.h>
 #include <linux/usb/atmel_usba_udc.h>
 
+#if CONFIG_IS_ENABLED(DM_USB_GADGET)
+#include <mach/atmel_usba_udc.h>
+
+static int usba_udc_start(struct usb_gadget *gadget,
+			  struct usb_gadget_driver *driver);
+static int usba_udc_stop(struct usb_gadget *gadget);
+#endif /* CONFIG_IS_ENABLED(DM_USB_GADGET) */
+
 #include "atmel_usba_udc.h"
 
 static int vbus_is_present(struct usba_udc *udc)
@@ -528,6 +540,10 @@ static const struct usb_gadget_ops usba_udc_ops = {
 	.wakeup			= usba_udc_wakeup,
 	.set_selfpowered	= usba_udc_set_selfpowered,
 	.pullup			= usba_udc_pullup,
+#if CONFIG_IS_ENABLED(DM_USB_GADGET)
+	.udc_start		= usba_udc_start,
+	.udc_stop		= usba_udc_stop,
+#endif
 };
 
 static struct usb_endpoint_descriptor usba_ep0_desc = {
@@ -1237,6 +1253,7 @@ static struct usba_ep *usba_udc_pdata(struct usba_platform_data *pdata,
 	return eps;
 }
 
+#if !CONFIG_IS_ENABLED(DM_USB_GADGET)
 static struct usba_udc controller = {
 	.regs = (unsigned *)ATMEL_BASE_UDPHS,
 	.fifo = (unsigned *)ATMEL_BASE_UDPHS_FIFO,
@@ -1312,3 +1329,130 @@ int usba_udc_probe(struct usba_platform_data *pdata)
 
 	return 0;
 }
+
+#else /* !CONFIG_IS_ENABLED(DM_USB_GADGET) */
+struct usba_priv_data {
+	struct clk_bulk		clks;
+	struct usba_udc		udc;
+};
+
+static int usba_udc_start(struct usb_gadget *gadget,
+			  struct usb_gadget_driver *driver)
+{
+	struct usba_udc *udc = to_usba_udc(gadget);
+
+	usba_udc_enable(udc);
+
+	udc->driver = driver;
+	return 0;
+}
+
+static int usba_udc_stop(struct usb_gadget *gadget)
+{
+	struct usba_udc *udc = to_usba_udc(gadget);
+
+	udc->driver = NULL;
+
+	usba_udc_disable(udc);
+	return 0;
+}
+
+static int usba_udc_clk_init(struct udevice *dev, struct clk_bulk *clks)
+{
+	int ret;
+
+	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 usba_udc_probe(struct udevice *dev)
+{
+	struct usba_priv_data *priv = dev_get_priv(dev);
+	struct usba_udc *udc = &priv->udc;
+	int ret;
+
+	udc->fifo = (void __iomem *)dev_remap_addr_index(dev, FIFO_IOMEM_ID);
+	if (!udc->fifo)
+		return -EINVAL;
+
+	udc->regs = (void __iomem *)dev_remap_addr_index(dev, CTRL_IOMEM_ID);
+	if (!udc->regs)
+		return -EINVAL;
+
+	ret = usba_udc_clk_init(dev, &priv->clks);
+	if (ret)
+		return ret;
+
+	udc->usba_ep = usba_udc_pdata(&pdata, udc);
+
+	udc->gadget.ops = &usba_udc_ops;
+	udc->gadget.speed = USB_SPEED_HIGH,
+	udc->gadget.is_dualspeed = 1,
+	udc->gadget.name = "atmel_usba_udc",
+
+	ret = usb_add_gadget_udc((struct device *)dev, &udc->gadget);
+	if (ret)
+		goto err;
+
+	return 0;
+err:
+	free(udc->usba_ep);
+
+	clk_release_bulk(&priv->clks);
+
+	return ret;
+}
+
+static int usba_udc_remove(struct udevice *dev)
+{
+	struct usba_priv_data *priv = dev_get_priv(dev);
+
+	usb_del_gadget_udc(&priv->udc.gadget);
+
+	free(priv->udc.usba_ep);
+
+	clk_release_bulk(&priv->clks);
+
+	return dm_scan_fdt_dev(dev);
+}
+
+static int usba_udc_handle_interrupts(struct udevice *dev)
+{
+	struct usba_priv_data *priv = dev_get_priv(dev);
+
+	return usba_udc_irq(&priv->udc);
+}
+
+static const struct usb_gadget_generic_ops usba_udc_gadget_ops = {
+	.handle_interrupts	= usba_udc_handle_interrupts,
+};
+
+static const struct udevice_id usba_udc_ids[] = {
+	{ .compatible = "atmel,at91sam9rl-udc" },
+	{ .compatible = "atmel,at91sam9g45-udc" },
+	{ .compatible = "atmel,sama5d3-udc" },
+	{}
+};
+
+U_BOOT_DRIVER(atmel_usba_udc) = {
+	.name	= "atmel_usba_udc",
+	.id	= UCLASS_USB_GADGET_GENERIC,
+	.of_match = usba_udc_ids,
+	.ops = &usba_udc_gadget_ops,
+	.probe = usba_udc_probe,
+	.remove = usba_udc_remove,
+	.priv_auto = sizeof(struct usba_priv_data),
+};
+#endif /* !CONFIG_IS_ENABLED(DM_USB_GADGET) */
diff --git a/drivers/usb/gadget/atmel_usba_udc.h b/drivers/usb/gadget/atmel_usba_udc.h
index f6cb48c1cf..7f5e98f6c4 100644
--- a/drivers/usb/gadget/atmel_usba_udc.h
+++ b/drivers/usb/gadget/atmel_usba_udc.h
@@ -211,6 +211,9 @@
 #define EP0_EPT_SIZE		USBA_EPT_SIZE_64
 #define EP0_NR_BANKS		1
 
+#define FIFO_IOMEM_ID	0
+#define CTRL_IOMEM_ID	1
+
 #define DBG_ERR		0x0001	/* report all error returns */
 #define DBG_HW		0x0002	/* debug hardware initialization */
 #define DBG_GADGET	0x0004	/* calls to/from gadget driver */
diff --git a/include/linux/usb/atmel_usba_udc.h b/include/linux/usb/atmel_usba_udc.h
index c1c810759c..37c4f21849 100644
--- a/include/linux/usb/atmel_usba_udc.h
+++ b/include/linux/usb/atmel_usba_udc.h
@@ -20,6 +20,8 @@ struct usba_platform_data {
 	struct usba_ep_data	*ep;
 };
 
+#if !CONFIG_IS_ENABLED(DM_USB_GADGET)
 extern int usba_udc_probe(struct usba_platform_data *pdata);
+#endif
 
 #endif /* __LINUX_USB_USBA_H */
-- 
2.45.2

Re: [PATCH v4 0/7] usb: gadget: atmel: Code refactor and DM_USB_GADGET support

[Marek Vasut]

On 7/25/24 5:31 PM, Zixun LI wrote:
> Changes in v4:
> - Release clocks if probe failed
> - Add missing endpoint data free
> - Addressed comments

Please collect any AB/RB/TB tags that were provided in v3 too.

Re: [PATCH v4 0/7] usb: gadget: atmel: Code refactor and DM_USB_GADGET support

[Zixun LI]

On Thu, Jul 25, 2024 at 8:24 PM Marek Vasut <marex@denx.de> wrote:
>
> On 7/25/24 5:31 PM, Zixun LI wrote:
> > Changes in v4:
> > - Release clocks if probe failed
> > - Add missing endpoint data free
> > - Addressed comments
>
> Please collect any AB/RB/TB tags that were provided in v3 too.

Sorry didn't know that, should I spin a v5 ?

Re: [PATCH v4 0/7] usb: gadget: atmel: Code refactor and DM_USB_GADGET support

[Marek Vasut]

On 7/25/24 8:48 PM, Zixun LI wrote:
> On Thu, Jul 25, 2024 at 8:24 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 7/25/24 5:31 PM, Zixun LI wrote:
>>> Changes in v4:
>>> - Release clocks if probe failed
>>> - Add missing endpoint data free
>>> - Addressed comments
>>
>> Please collect any AB/RB/TB tags that were provided in v3 too.
> 
> Sorry didn't know that, should I spin a v5 ?

No worries, wait a few days for more feedback (if any), and then collect 
them for V5 and send V5.

Re: [PATCH v4 0/7] usb: gadget: atmel: Code refactor and DM_USB_GADGET support

[Mattijs Korpershoek]

Hi,

On jeu., juil. 25, 2024 at 20:57, Marek Vasut <marex@denx.de> wrote:

> On 7/25/24 8:48 PM, Zixun LI wrote:
>> On Thu, Jul 25, 2024 at 8:24 PM Marek Vasut <marex@denx.de> wrote:
>>>
>>> On 7/25/24 5:31 PM, Zixun LI wrote:
>>>> Changes in v4:
>>>> - Release clocks if probe failed
>>>> - Add missing endpoint data free
>>>> - Addressed comments
>>>
>>> Please collect any AB/RB/TB tags that were provided in v3 too.
>>
>> Sorry didn't know that, should I spin a v5 ?
>
> No worries, wait a few days for more feedback (if any), and then collect
> them for V5 and send V5.

Indeed, it's important to collect the tags to make sure that the
reviewers get credit for their time reviewing.

Zixun, make sure to collect them for all future contributions.

Fortunately, b4 (https://b4.docs.kernel.org/en/latest/index.html) can
collect the tags:

$ b4 shazam -s -l --check 20240725153204.358925-1-admin@hifiphile.com

[...]

  ✓ [PATCH v4 1/7] usb: gadget: atmel: Sort includes
    + Reviewed-by: Marek Vasut <marex@denx.de> (✓ DKIM/denx.de)
    + Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> (✓ DKIM/baylibre-com.20230601.gappssmtp.com)
    + Link: https://lore.kernel.org/r/20240725153204.358925-2-admin@hifiphile.com
    + Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
    ● checkpatch.pl: passed all checks

So, for this series, 2 cases can happen:

1. No other review comments are found. A v5 won't be required.
   I will collect the tags with b4 when applying to the u-boot-dfu tree

2. New review comments require the series to be modified. A v5 is
   required. In that case, make sure to collect all tags yourself.

Please wait a few days for others to get time to chime in this patch
series.

Thank you for your contribution !

Mattijs

Re: [PATCH v4 7/7] usb: gadget: atmel: Add DM_USB_GADGET support

[Mattijs Korpershoek]

Hi Zixun,

Thank you for the patch.

On jeu., juil. 25, 2024 at 17:32, Zixun LI <admin@hifiphile.com> 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.
>
> Replace dm_usb_gadget_handle_interrupts() with handle_interrupts ops
> when DM_USB_GADGET is enabled.
>
> Disable legacy struct usba_udc controller as controller point is extracted
> from udevice private data with DM.
>
> Disable legacy usba_udc_probe() to avoid conflict with DM when it's
> enabled.
>
> Compared to Linux driver only supported devices' DT bindings are included
> (sorted as Linux driver)
>
> Signed-off-by: Zixun LI <admin@hifiphile.com>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

> ---
>  drivers/usb/gadget/atmel_usba_udc.c | 144 ++++++++++++++++++++++++++++
>  drivers/usb/gadget/atmel_usba_udc.h |   3 +
>  include/linux/usb/atmel_usba_udc.h  |   2 +
>  3 files changed, 149 insertions(+)
>
> diff --git a/drivers/usb/gadget/atmel_usba_udc.c b/drivers/usb/gadget/atmel_usba_udc.c
> index a7b96449f8..a77037a709 100644
> --- a/drivers/usb/gadget/atmel_usba_udc.c
> +++ b/drivers/usb/gadget/atmel_usba_udc.c
> @@ -7,10 +7,14 @@
>   *			   Bo Shen <voice.shen@atmel.com>
>   */
>  
> +#include <clk.h>
> +#include <dm.h>
>  #include <log.h>
>  #include <malloc.h>
>  #include <asm/gpio.h>
>  #include <asm/hardware.h>
> +#include <dm/device_compat.h>
> +#include <dm/devres.h>
>  #include <linux/bitops.h>
>  #include <linux/errno.h>
>  #include <linux/list.h>
> @@ -18,6 +22,14 @@
>  #include <linux/usb/gadget.h>
>  #include <linux/usb/atmel_usba_udc.h>
>  
> +#if CONFIG_IS_ENABLED(DM_USB_GADGET)
> +#include <mach/atmel_usba_udc.h>
> +
> +static int usba_udc_start(struct usb_gadget *gadget,
> +			  struct usb_gadget_driver *driver);
> +static int usba_udc_stop(struct usb_gadget *gadget);
> +#endif /* CONFIG_IS_ENABLED(DM_USB_GADGET) */
> +
>  #include "atmel_usba_udc.h"
>  
>  static int vbus_is_present(struct usba_udc *udc)
> @@ -528,6 +540,10 @@ static const struct usb_gadget_ops usba_udc_ops = {
>  	.wakeup			= usba_udc_wakeup,
>  	.set_selfpowered	= usba_udc_set_selfpowered,
>  	.pullup			= usba_udc_pullup,
> +#if CONFIG_IS_ENABLED(DM_USB_GADGET)
> +	.udc_start		= usba_udc_start,
> +	.udc_stop		= usba_udc_stop,
> +#endif
>  };
>  
>  static struct usb_endpoint_descriptor usba_ep0_desc = {
> @@ -1237,6 +1253,7 @@ static struct usba_ep *usba_udc_pdata(struct usba_platform_data *pdata,
>  	return eps;
>  }
>  
> +#if !CONFIG_IS_ENABLED(DM_USB_GADGET)
>  static struct usba_udc controller = {
>  	.regs = (unsigned *)ATMEL_BASE_UDPHS,
>  	.fifo = (unsigned *)ATMEL_BASE_UDPHS_FIFO,
> @@ -1312,3 +1329,130 @@ int usba_udc_probe(struct usba_platform_data *pdata)
>  
>  	return 0;
>  }
> +
> +#else /* !CONFIG_IS_ENABLED(DM_USB_GADGET) */
> +struct usba_priv_data {
> +	struct clk_bulk		clks;
> +	struct usba_udc		udc;
> +};
> +
> +static int usba_udc_start(struct usb_gadget *gadget,
> +			  struct usb_gadget_driver *driver)
> +{
> +	struct usba_udc *udc = to_usba_udc(gadget);
> +
> +	usba_udc_enable(udc);
> +
> +	udc->driver = driver;
> +	return 0;
> +}
> +
> +static int usba_udc_stop(struct usb_gadget *gadget)
> +{
> +	struct usba_udc *udc = to_usba_udc(gadget);
> +
> +	udc->driver = NULL;
> +
> +	usba_udc_disable(udc);
> +	return 0;
> +}
> +
> +static int usba_udc_clk_init(struct udevice *dev, struct clk_bulk *clks)
> +{
> +	int ret;
> +
> +	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 usba_udc_probe(struct udevice *dev)
> +{
> +	struct usba_priv_data *priv = dev_get_priv(dev);
> +	struct usba_udc *udc = &priv->udc;
> +	int ret;
> +
> +	udc->fifo = (void __iomem *)dev_remap_addr_index(dev, FIFO_IOMEM_ID);
> +	if (!udc->fifo)
> +		return -EINVAL;
> +
> +	udc->regs = (void __iomem *)dev_remap_addr_index(dev, CTRL_IOMEM_ID);
> +	if (!udc->regs)
> +		return -EINVAL;
> +
> +	ret = usba_udc_clk_init(dev, &priv->clks);
> +	if (ret)
> +		return ret;
> +
> +	udc->usba_ep = usba_udc_pdata(&pdata, udc);
> +
> +	udc->gadget.ops = &usba_udc_ops;
> +	udc->gadget.speed = USB_SPEED_HIGH,
> +	udc->gadget.is_dualspeed = 1,
> +	udc->gadget.name = "atmel_usba_udc",
> +
> +	ret = usb_add_gadget_udc((struct device *)dev, &udc->gadget);
> +	if (ret)
> +		goto err;
> +
> +	return 0;
> +err:
> +	free(udc->usba_ep);
> +
> +	clk_release_bulk(&priv->clks);
> +
> +	return ret;
> +}
> +
> +static int usba_udc_remove(struct udevice *dev)
> +{
> +	struct usba_priv_data *priv = dev_get_priv(dev);
> +
> +	usb_del_gadget_udc(&priv->udc.gadget);
> +
> +	free(priv->udc.usba_ep);
> +
> +	clk_release_bulk(&priv->clks);
> +
> +	return dm_scan_fdt_dev(dev);
> +}
> +
> +static int usba_udc_handle_interrupts(struct udevice *dev)
> +{
> +	struct usba_priv_data *priv = dev_get_priv(dev);
> +
> +	return usba_udc_irq(&priv->udc);
> +}
> +
> +static const struct usb_gadget_generic_ops usba_udc_gadget_ops = {
> +	.handle_interrupts	= usba_udc_handle_interrupts,
> +};
> +
> +static const struct udevice_id usba_udc_ids[] = {
> +	{ .compatible = "atmel,at91sam9rl-udc" },
> +	{ .compatible = "atmel,at91sam9g45-udc" },
> +	{ .compatible = "atmel,sama5d3-udc" },
> +	{}
> +};
> +
> +U_BOOT_DRIVER(atmel_usba_udc) = {
> +	.name	= "atmel_usba_udc",
> +	.id	= UCLASS_USB_GADGET_GENERIC,
> +	.of_match = usba_udc_ids,
> +	.ops = &usba_udc_gadget_ops,
> +	.probe = usba_udc_probe,
> +	.remove = usba_udc_remove,
> +	.priv_auto = sizeof(struct usba_priv_data),
> +};
> +#endif /* !CONFIG_IS_ENABLED(DM_USB_GADGET) */
> diff --git a/drivers/usb/gadget/atmel_usba_udc.h b/drivers/usb/gadget/atmel_usba_udc.h
> index f6cb48c1cf..7f5e98f6c4 100644
> --- a/drivers/usb/gadget/atmel_usba_udc.h
> +++ b/drivers/usb/gadget/atmel_usba_udc.h
> @@ -211,6 +211,9 @@
>  #define EP0_EPT_SIZE		USBA_EPT_SIZE_64
>  #define EP0_NR_BANKS		1
>  
> +#define FIFO_IOMEM_ID	0
> +#define CTRL_IOMEM_ID	1
> +
>  #define DBG_ERR		0x0001	/* report all error returns */
>  #define DBG_HW		0x0002	/* debug hardware initialization */
>  #define DBG_GADGET	0x0004	/* calls to/from gadget driver */
> diff --git a/include/linux/usb/atmel_usba_udc.h b/include/linux/usb/atmel_usba_udc.h
> index c1c810759c..37c4f21849 100644
> --- a/include/linux/usb/atmel_usba_udc.h
> +++ b/include/linux/usb/atmel_usba_udc.h
> @@ -20,6 +20,8 @@ struct usba_platform_data {
>  	struct usba_ep_data	*ep;
>  };
>  
> +#if !CONFIG_IS_ENABLED(DM_USB_GADGET)
>  extern int usba_udc_probe(struct usba_platform_data *pdata);
> +#endif
>  
>  #endif /* __LINUX_USB_USBA_H */
> -- 
> 2.45.2

Re: [PATCH v4 0/7] usb: gadget: atmel: Code refactor and DM_USB_GADGET support

[Mattijs Korpershoek]

Hi,

On Thu, 25 Jul 2024 17:31:54 +0200, Zixun LI wrote:
> Changes in v4:
> - Release clocks if probe failed
> - Add missing endpoint data free
> - Addressed comments
> 
> Changes in v3:
> - Separate code refactor into individual commits
> - Extract the controller point from udevice private data in DM functions
> 
> [...]

Thanks, Applied to https://source.denx.de/u-boot/custodians/u-boot-dfu (u-boot-dfu-next)

[1/7] usb: gadget: atmel: Sort includes
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/9cfee49022e2f22ca7074d423bd23a610ed652f4
[2/7] usb: gadget: atmel: Replace printf() and pr_err() by log_err()
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/9ec3b70b21d9f825a4dedbf609cf76ffe37deb55
[3/7] usb: gadget: atmel: Fix typo in usb gadget driver register and unregister
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/dbbacf19d6310b5b255662ad87c07eb34b73d39d
[4/7] usb: gadget: atmel: Move usba_udc_pdata() with other static functions
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/9b97a354bdc0afc788b9e99ceb9d03a696e5f749
[5/7] usb: gadget: atmel: Rename atmel_usba_start()/_stop() to usba_udc_enable()/_disable()
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/7a448c8f384022a2585e8439c2a182ada3e4aab2
[6/7] usb: gadget: atmel: Add attach/detach support
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/ef5e1d1f97623f8c549c6fffd481bb120df12dcb
[7/7] usb: gadget: atmel: Add DM_USB_GADGET support
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/d6376f7ed823569acd8bb0fdfa501c040cfec36f

--
Mattijs