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

[Marek Vasut]

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 at 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