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

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_register_driver() 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 | 250 ++++++++++++++++++++++------ drivers/usb/gadget/atmel_usba_udc.h | 3 + include/linux/usb/atmel_usba_udc.h | 2 + 3 files changed, 208 insertions(+), 47 deletions(-)
-- 2.45.2

Sort includes in alphabetical order.
Signed-off-by: Zixun LI zli@ogga.fr --- 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"

On 7/23/24 3:18 PM, Zixun LI wrote:
Sort includes in alphabetical order.
Reviewed-by: Marek Vasut marex@denx.de
Thanks !

Hi Zixun,
Thank you for the patch.
On mar., juil. 23, 2024 at 15:18, Zixun LI admin@hifiphile.com wrote:
Sort includes in alphabetical order.
Signed-off-by: Zixun LI zli@ogga.fr
Reviewed-by: Mattijs Korpershoek mkorpershoek@baylibre.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

To have a uniform printing function, also drop linux/printk.h as no longer used.
Signed-off-by: Zixun LI zli@ogga.fr --- 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..83fdc36870 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 parameter\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; }

On 7/23/24 3:18 PM, Zixun LI wrote:
To have a uniform printing function, also drop linux/printk.h as no longer used.
Reviewed-by: Marek Vasut marex@denx.de
Thanks !

Hi Zixun,
Thank you for the patch.
On mar., juil. 23, 2024 at 15:18, Zixun LI admin@hifiphile.com wrote:
To have a uniform printing function, also drop linux/printk.h as no longer used.
Signed-off-by: Zixun LI zli@ogga.fr
Checkpatch complains here:
checkpatch.pl: 164: WARNING: From:/Signed-off-by: email address mismatch: 'From: Zixun LI admin@hifiphile.com' != 'Signed-off-by: Zixun LI zli@ogga.fr'
Please make sure that the commiter matches the Signed-off-by. This can be done with git commit --reset-author or --author="name <email>"
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..83fdc36870 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");
return -EBUSY; }log_err("UDC already has a gadget driver\n");
@@ -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);
udc->driver = NULL; }log_err("driver->bind() returned %d\n", ret);
@@ -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 parameter\n");
Here we change both: 1. pr_err() to log_err() 2. fix paramter -> parameter
2. should be done in patch 3/7 instead of here.
With the above fixed:
Reviewed-by: Mattijs Korpershoek mkorpershoek@baylibre.com
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");
return NULL; }log_err("failed to alloc eps\n");
-- 2.45.2

Replace "paramter" by "parameter".
Signed-off-by: Zixun LI zli@ogga.fr --- drivers/usb/gadget/atmel_usba_udc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/atmel_usba_udc.c b/drivers/usb/gadget/atmel_usba_udc.c index 83fdc36870..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; }

On 7/23/24 3:18 PM, Zixun LI wrote:
Replace "paramter" by "parameter".
Signed-off-by: Zixun LI zli@ogga.fr
Reviewed-by: Marek Vasut marex@denx.de
This is really MUCH better and MUCH easier to review, thank you!

Hi Zixun,
Thank you for the patch.
On mar., juil. 23, 2024 at 15:18, Zixun LI admin@hifiphile.com wrote:
Replace "paramter" by "parameter".
Signed-off-by: Zixun LI zli@ogga.fr
Same remark on checkpatch warning:
WARNING: From:/Signed-off-by: email address mismatch: 'From: Zixun LI admin@hifiphile.com' != 'Signed-off-by: Zixun LI zli@ogga.fr'
drivers/usb/gadget/atmel_usba_udc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/atmel_usba_udc.c b/drivers/usb/gadget/atmel_usba_udc.c index 83fdc36870..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");
return -EINVAL; }log_err("bad parameter\n");
-- 2.45.2

To make all static functions in the top, no functional change.
Signed-off-by: Zixun LI zli@ogga.fr --- 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;

On 7/23/24 3:18 PM, Zixun LI wrote:
To make all static functions in the top, no functional change.
Reviewed-by: Marek Vasut marex@denx.de
Thanks !

Hi Zixun,
Thank you for the patch.
On mar., juil. 23, 2024 at 15:18, Zixun LI admin@hifiphile.com wrote:
To make all static functions in the top, no functional change.
Signed-off-by: Zixun LI zli@ogga.fr
Reviewed-by: Mattijs Korpershoek mkorpershoek@baylibre.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

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 zli@ogga.fr --- 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; }

On 7/23/24 3:18 PM, Zixun LI wrote:
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.
Reviewed-by: Marek Vasut marex@denx.de
Thanks !

Hi Zixun,
Thank you for the patch.
On mar., juil. 23, 2024 at 15:18, Zixun LI admin@hifiphile.com wrote:
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 zli@ogga.fr
Reviewed-by: Mattijs Korpershoek mkorpershoek@baylibre.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

Add controller attach/detach support by using usb_gadget_ops.pullup() method.
Signed-off-by: Zixun LI zli@ogga.fr --- 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 = {

On 7/23/24 3:18 PM, Zixun LI wrote:
Add controller attach/detach support by using usb_gadget_ops.pullup() method.
It is called 'function' in C, not 'method'. I think 'method' is C++ or maybe Java ?
In any case:
Reviewed-by: Marek Vasut marex@denx.de
Thanks !

On mar., juil. 23, 2024 at 15:36, Marek Vasut marex@denx.de wrote:
On 7/23/24 3:18 PM, Zixun LI wrote:
Add controller attach/detach support by using usb_gadget_ops.pullup() method.
It is called 'function' in C, not 'method'. I think 'method' is C++ or maybe Java ?
Agreed, 'function' is more appropriate wording.
Zixun, if you need to respin a v4 please consider using 'function'.
Otherwise I can fix it up while applying !
In any case:
Reviewed-by: Marek Vasut marex@denx.de
Thanks !

Hi Zixun,
Thank you for the patch.
On mar., juil. 23, 2024 at 15:18, Zixun LI admin@hifiphile.com wrote:
Add controller attach/detach support by using usb_gadget_ops.pullup() method.
Signed-off-by: Zixun LI zli@ogga.fr
Reviewed-by: Mattijs Korpershoek mkorpershoek@baylibre.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

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 zli@ogga.fr --- drivers/usb/gadget/atmel_usba_udc.c | 138 ++++++++++++++++++++++++++++ drivers/usb/gadget/atmel_usba_udc.h | 3 + include/linux/usb/atmel_usba_udc.h | 2 + 3 files changed, 143 insertions(+)
diff --git a/drivers/usb/gadget/atmel_usba_udc.c b/drivers/usb/gadget/atmel_usba_udc.c index a7b96449f8..b7b2e5196b 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,124 @@ 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; + + ret = usba_udc_clk_init(dev, &priv->clks); + if (ret) + return 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; + + udc->gadget.ops = &usba_udc_ops; + udc->gadget.speed = USB_SPEED_HIGH, + udc->gadget.is_dualspeed = 1, + udc->gadget.name = "atmel_usba_udc", + + udc->usba_ep = usba_udc_pdata(&pdata, udc); + + udc->driver = NULL; + + ret = usb_add_gadget_udc((struct device *)dev, &udc->gadget); + + 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); + + 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 */

Hi Zixun,
Thank you for the patch and doing this very welcome DM_USB_GADGET addition :)
On mar., juil. 23, 2024 at 15:18, 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 zli@ogga.fr
checkpatch.pl seems to complain about Signed-off-by vs commiter here as well:
checkpatch.pl: 324: WARNING: From:/Signed-off-by: email address mismatch: 'From: Zixun LI admin@hifiphile.com' != 'Signed-off-by: Zixun LI zli@ogga.fr'
drivers/usb/gadget/atmel_usba_udc.c | 138 ++++++++++++++++++++++++++++ drivers/usb/gadget/atmel_usba_udc.h | 3 + include/linux/usb/atmel_usba_udc.h | 2 + 3 files changed, 143 insertions(+)
diff --git a/drivers/usb/gadget/atmel_usba_udc.c b/drivers/usb/gadget/atmel_usba_udc.c index a7b96449f8..b7b2e5196b 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,124 @@ 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;
Nitpick: no need for extra blank line here:
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);
Same
- 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;
- ret = usba_udc_clk_init(dev, &priv->clks);
- if (ret)
return ret;
- udc->fifo = (void __iomem *)dev_remap_addr_index(dev, FIFO_IOMEM_ID);
- if (!udc->fifo)
I think that at this points &priv->clks are valid and enabled. Therefore, we should disable/release them (as being done in the remove)
return -EINVAL;
- udc->regs = (void __iomem *)dev_remap_addr_index(dev, CTRL_IOMEM_ID);
- if (!udc->regs)
Same here
return -EINVAL;
- udc->gadget.ops = &usba_udc_ops;
- udc->gadget.speed = USB_SPEED_HIGH,
- udc->gadget.is_dualspeed = 1,
- udc->gadget.name = "atmel_usba_udc",
- udc->usba_ep = usba_udc_pdata(&pdata, udc);
- udc->driver = NULL;
Why do we assign the driver to NULL ? Maybe this deserves a comment in the code?
- ret = usb_add_gadget_udc((struct device *)dev, &udc->gadget);
- 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);
- 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

Hi Mattijs,
On Wed, Jul 24, 2024 at 12:12 PM Mattijs Korpershoek mkorpershoek@baylibre.com wrote:
Checkpatch complains here:
checkpatch.pl: 164: WARNING: From:/Signed-off-by: email address mismatch: 'From: Zixun LI admin@hifiphile.com' != 'Signed-off-by: Zixun LI zli@ogga.fr'
Please make sure that the commiter matches the Signed-off-by. This can be done with git commit --reset-author or --author="name <email>"
Will fix all Signed-off-by in v4.
Here we change both:
pr_err() to log_err()
fix paramter -> parameter
should be done in patch 3/7 instead of here.
With the above fixed:
Will fix in v4.
Agreed, 'function' is more appropriate wording. Zixun, if you need to respin a v4 please consider using 'function'. Otherwise I can fix it up while applying !
Will fix in v4.
+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;
ret = usba_udc_clk_init(dev, &priv->clks);
if (ret)
return ret;
udc->fifo = (void __iomem *)dev_remap_addr_index(dev, FIFO_IOMEM_ID);
if (!udc->fifo)
I think that at this points &priv->clks are valid and enabled. Therefore, we should disable/release them (as being done in the remove)
return -EINVAL;
udc->regs = (void __iomem *)dev_remap_addr_index(dev, CTRL_IOMEM_ID);
if (!udc->regs)
Same here
Good catch, I copied the dwc2 driver who has the same issue, will fix in v4.
Also spotted udc->usba_ep allocated in usba_udc_pdata() is not freed.
I'll change probe() and remove() functions as follow:
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(priv->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); }
participants (4)
-
admin LI
-
Marek Vasut
-
Mattijs Korpershoek
-
Zixun LI