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

From: Cristian.Birsan
Date: Fri Nov 22 2019 - 11:02:51 EST

Next message: Rob Herring: "Re: [PATCH 1/2] dt-bindings: iio: adc: ad7292: Update SPDX identifier"
Previous message: Borislav Petkov: "Re: [PATCH v3 09/19] x86/cpu: Clear VMX feature flag if VMX is not fully enabled"
In reply to: Gregory CLEMENT: "Re: [PATCH 1/3] usb: gadget: udc: atmel: Don't use DT to configure end point"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 11/22/19 4:50 PM, Gregory CLEMENT wrote:

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

Hi Gregory,

Thank you for sending the patch. It looks good to me.

I checked it briefly on sama5d2 with the in kernel cdc-acm.

>
> Thanks,
>
> Gregory
>
>
>>
>> Signed-off-by: Gregory CLEMENT <gregory.clement@xxxxxxxxxxx>
Acked-by: Cristian Birsan <cristian.birsan@xxxxxxxxxxxxx>

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

Next message: Rob Herring: "Re: [PATCH 1/2] dt-bindings: iio: adc: ad7292: Update SPDX identifier"
Previous message: Borislav Petkov: "Re: [PATCH v3 09/19] x86/cpu: Clear VMX feature flag if VMX is not fully enabled"
In reply to: Gregory CLEMENT: "Re: [PATCH 1/3] usb: gadget: udc: atmel: Don't use DT to configure end point"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]