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 <ad...@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 <z...@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 <ad...@hifiphile.com>' != 'Signed-off-by: Zixun LI
<z...@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.s...@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
