Re: [PATCH 1/3] usb: gadget: udc: atmel: Don't use DT to configure end point

[Gregory CLEMENT <gregory.clement@bootlin.com>]

Hello,

> The endpoint configuration used to be stored in the device tree,
> however the configuration depend on the "version" of the controller
> itself.
>
> This information is already documented by the compatible string. It
> then possible to just rely on the compatible string and completely
> remove the full ep configuration done in the device tree as it was
> already the case for all the other USB device controller.

Do you have any feedback about this patch ?

The binding being reviewed is there any chance that this patch will be
merged?

Thanks,

Gregory


>
> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
> ---
>  drivers/usb/gadget/udc/atmel_usba_udc.c | 112 +++++++++++++++---------
>  drivers/usb/gadget/udc/atmel_usba_udc.h |  12 +++
>  2 files changed, 84 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
> index 86ffc8307864..2db833caeb09 100644
> --- a/drivers/usb/gadget/udc/atmel_usba_udc.c
> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
> @@ -2040,10 +2040,56 @@ static const struct usba_udc_errata at91sam9g45_errata = {
>  	.pulse_bias = at91sam9g45_pulse_bias,
>  };
>  
> +static const struct usba_ep_config ep_config_sam9[] __initconst = {
> +	{ .nr_banks = 1 },				/* ep 0 */
> +	{ .nr_banks = 2, .can_dma = 1, .can_isoc = 1 },	/* ep 1 */
> +	{ .nr_banks = 2, .can_dma = 1, .can_isoc = 1 },	/* ep 2 */
> +	{ .nr_banks = 3, .can_dma = 1 },		/* ep 3 */
> +	{ .nr_banks = 3, .can_dma = 1 },		/* ep 4 */
> +	{ .nr_banks = 3, .can_dma = 1, .can_isoc = 1 },	/* ep 5 */
> +	{ .nr_banks = 3, .can_dma = 1, .can_isoc = 1 },	/* ep 6 */
> +};
> +
> +static const struct usba_ep_config ep_config_sama5[] __initconst = {
> +	{ .nr_banks = 1 },				/* ep 0 */
> +	{ .nr_banks = 3, .can_dma = 1, .can_isoc = 1 },	/* ep 1 */
> +	{ .nr_banks = 3, .can_dma = 1, .can_isoc = 1 },	/* ep 2 */
> +	{ .nr_banks = 2, .can_dma = 1, .can_isoc = 1 },	/* ep 3 */
> +	{ .nr_banks = 2, .can_dma = 1, .can_isoc = 1 },	/* ep 4 */
> +	{ .nr_banks = 2, .can_dma = 1, .can_isoc = 1 },	/* ep 5 */
> +	{ .nr_banks = 2, .can_dma = 1, .can_isoc = 1 },	/* ep 6 */
> +	{ .nr_banks = 2, .can_dma = 1, .can_isoc = 1 },	/* ep 7 */
> +	{ .nr_banks = 2, .can_isoc = 1 },		/* ep 8 */
> +	{ .nr_banks = 2, .can_isoc = 1 },		/* ep 9 */
> +	{ .nr_banks = 2, .can_isoc = 1 },		/* ep 10 */
> +	{ .nr_banks = 2, .can_isoc = 1 },		/* ep 11 */
> +	{ .nr_banks = 2, .can_isoc = 1 },		/* ep 12 */
> +	{ .nr_banks = 2, .can_isoc = 1 },		/* ep 13 */
> +	{ .nr_banks = 2, .can_isoc = 1 },		/* ep 14 */
> +	{ .nr_banks = 2, .can_isoc = 1 },		/* ep 15 */
> +};
> +
> +static const struct usba_udc_config udc_at91sam9rl_cfg = {
> +	.errata = &at91sam9rl_errata,
> +	.config = ep_config_sam9,
> +	.num_ep = ARRAY_SIZE(ep_config_sam9),
> +};
> +
> +static const struct usba_udc_config udc_at91sam9g45_cfg = {
> +	.errata = &at91sam9g45_errata,
> +	.config = ep_config_sam9,
> +	.num_ep = ARRAY_SIZE(ep_config_sam9),
> +};
> +
> +static const struct usba_udc_config udc_sama5d3_cfg = {
> +	.config = ep_config_sama5,
> +	.num_ep = ARRAY_SIZE(ep_config_sama5),
> +};
> +
>  static const struct of_device_id atmel_udc_dt_ids[] = {
> -	{ .compatible = "atmel,at91sam9rl-udc", .data = &at91sam9rl_errata },
> -	{ .compatible = "atmel,at91sam9g45-udc", .data = &at91sam9g45_errata },
> -	{ .compatible = "atmel,sama5d3-udc" },
> +	{ .compatible = "atmel,at91sam9rl-udc", .data = &udc_at91sam9rl_cfg },
> +	{ .compatible = "atmel,at91sam9g45-udc", .data = &udc_at91sam9g45_cfg },
> +	{ .compatible = "atmel,sama5d3-udc", .data = &udc_sama5d3_cfg },
>  	{ /* sentinel */ }
>  };
>  
> @@ -2052,18 +2098,19 @@ MODULE_DEVICE_TABLE(of, atmel_udc_dt_ids);
>  static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev,
>  						    struct usba_udc *udc)
>  {
> -	u32 val;
>  	struct device_node *np = pdev->dev.of_node;
>  	const struct of_device_id *match;
>  	struct device_node *pp;
>  	int i, ret;
>  	struct usba_ep *eps, *ep;
> +	const struct usba_udc_config *udc_config;
>  
>  	match = of_match_node(atmel_udc_dt_ids, np);
>  	if (!match)
>  		return ERR_PTR(-EINVAL);
>  
> -	udc->errata = match->data;
> +	udc_config = match->data;
> +	udc->errata = udc_config->errata;
>  	udc->pmc = syscon_regmap_lookup_by_compatible("atmel,at91sam9g45-pmc");
>  	if (IS_ERR(udc->pmc))
>  		udc->pmc = syscon_regmap_lookup_by_compatible("atmel,at91sam9rl-pmc");
> @@ -2079,8 +2126,7 @@ static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev,
>  
>  	if (fifo_mode == 0) {
>  		pp = NULL;
> -		while ((pp = of_get_next_child(np, pp)))
> -			udc->num_ep++;
> +		udc->num_ep = udc_config->num_ep;
>  		udc->configured_ep = 1;
>  	} else {
>  		udc->num_ep = usba_config_fifo_table(udc);
> @@ -2097,52 +2143,38 @@ static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev,
>  
>  	pp = NULL;
>  	i = 0;
> -	while ((pp = of_get_next_child(np, pp)) && i < udc->num_ep) {
> +	while (i < udc->num_ep) {
> +		const struct usba_ep_config *ep_cfg = &udc_config->config[i];
> +
>  		ep = &eps[i];
>  
> -		ret = of_property_read_u32(pp, "reg", &val);
> -		if (ret) {
> -			dev_err(&pdev->dev, "of_probe: reg error(%d)\n", ret);
> -			goto err;
> -		}
> -		ep->index = fifo_mode ? udc->fifo_cfg[i].hw_ep_num : val;
> +		ep->index = fifo_mode ? udc->fifo_cfg[i].hw_ep_num : i;
> +
> +		/* Only the first EP is 64 bytes */
> +		if (ep->index == 0)
> +			ep->fifo_size = 64;
> +		else
> +			ep->fifo_size = 1024;
>  
> -		ret = of_property_read_u32(pp, "atmel,fifo-size", &val);
> -		if (ret) {
> -			dev_err(&pdev->dev, "of_probe: fifo-size error(%d)\n", ret);
> -			goto err;
> -		}
>  		if (fifo_mode) {
> -			if (val < udc->fifo_cfg[i].fifo_size) {
> +			if (ep->fifo_size < udc->fifo_cfg[i].fifo_size)
>  				dev_warn(&pdev->dev,
> -					 "Using max fifo-size value from DT\n");
> -				ep->fifo_size = val;
> -			} else {
> +					 "Using default max fifo-size value\n");
> +			else
>  				ep->fifo_size = udc->fifo_cfg[i].fifo_size;
> -			}
> -		} else {
> -			ep->fifo_size = val;
>  		}
>  
> -		ret = of_property_read_u32(pp, "atmel,nb-banks", &val);
> -		if (ret) {
> -			dev_err(&pdev->dev, "of_probe: nb-banks error(%d)\n", ret);
> -			goto err;
> -		}
> +		ep->nr_banks = ep_cfg->nr_banks;
>  		if (fifo_mode) {
> -			if (val < udc->fifo_cfg[i].nr_banks) {
> +			if (ep->nr_banks < udc->fifo_cfg[i].nr_banks)
>  				dev_warn(&pdev->dev,
> -					 "Using max nb-banks value from DT\n");
> -				ep->nr_banks = val;
> -			} else {
> +					 "Using default max nb-banks value\n");
> +			else
>  				ep->nr_banks = udc->fifo_cfg[i].nr_banks;
> -			}
> -		} else {
> -			ep->nr_banks = val;
>  		}
>  
> -		ep->can_dma = of_property_read_bool(pp, "atmel,can-dma");
> -		ep->can_isoc = of_property_read_bool(pp, "atmel,can-isoc");
> +		ep->can_dma = ep_cfg->can_dma;
> +		ep->can_isoc = ep_cfg->can_isoc;
>  
>  		sprintf(ep->name, "ep%d", ep->index);
>  		ep->ep.name = ep->name;
> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.h b/drivers/usb/gadget/udc/atmel_usba_udc.h
> index a0225e4543d4..48e332439ed5 100644
> --- a/drivers/usb/gadget/udc/atmel_usba_udc.h
> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.h
> @@ -290,6 +290,12 @@ struct usba_ep {
>  #endif
>  };
>  
> +struct usba_ep_config {
> +	u8					nr_banks;
> +	unsigned int				can_dma:1;
> +	unsigned int				can_isoc:1;
> +};
> +
>  struct usba_request {
>  	struct usb_request			req;
>  	struct list_head			queue;
> @@ -307,6 +313,12 @@ struct usba_udc_errata {
>  	void (*pulse_bias)(struct usba_udc *udc);
>  };
>  
> +struct usba_udc_config {
> +	const struct usba_udc_errata *errata;
> +	const struct usba_ep_config *config;
> +	const int num_ep;
> +};
> +
>  struct usba_udc {
>  	/* Protect hw registers from concurrent modifications */
>  	spinlock_t lock;
> -- 
> 2.24.0.rc1
>

-- 
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com