Re: [PATCH linux-next v2 1/1] usb: gadget: udc: atmel: Update endpoint allocation scheme

[Felipe Balbi]

Hi,

Cristian Birsan  writes:
>>> +   /* It might be a little bit late to set this */
>> 
>> Is it too late or not? This comment is frightening... We may need some
>> feedback from the USB guys here...
>>
>
> If someone from USB can comment a bit on this topic I will be grateful.

it is not too late :-) Just look at the implementation of
usb_ep_autoconfig_ss(). That said, I haven't considered if gadget
drivers might want this value to be valid before calling
usb_ep_autoconfig_ss(). I can't see why they would, but still; just a
little warning.

struct usb_ep *usb_ep_autoconfig_ss(
struct usb_gadget   *gadget,
struct usb_endpoint_descriptor  *desc,
struct usb_ss_ep_comp_descriptor *ep_comp
)
{
struct usb_ep   *ep;
u8  type;

type = desc->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK;

if (gadget->ops->match_ep) {
ep = gadget->ops->match_ep(gadget, desc, ep_comp);
if (ep)
goto found_ep;
}

/* Second, look at endpoints until an unclaimed one looks usable */
list_for_each_entry (ep, >ep_list, ep_list) {
if (usb_gadget_ep_match_desc(gadget, ep, desc, ep_comp))
goto found_ep;
}

/* Fail */
return NULL;
found_ep:

/*
 * If the protocol driver hasn't yet decided on wMaxPacketSize
 * and wants to know the maximum possible, provide the info.
 */
if (desc->wMaxPacketSize == 0)
desc->wMaxPacketSize = cpu_to_le16(ep->maxpacket_limit);

/* report address */
desc->bEndpointAddress &= USB_DIR_IN;
if (isdigit(ep->name[2])) {
u8 num = simple_strtoul(>name[2], NULL, 10);
desc->bEndpointAddress |= num;
} else if (desc->bEndpointAddress & USB_DIR_IN) {
if (++gadget->in_epnum > 15)
return NULL;
desc->bEndpointAddress = USB_DIR_IN | gadget->in_epnum;
} else {
if (++gadget->out_epnum > 15)
return NULL;
desc->bEndpointAddress |= gadget->out_epnum;
}

/* report (variable) full speed bulk maxpacket */
if ((type == USB_ENDPOINT_XFER_BULK) && !ep_comp) {
int size = ep->maxpacket_limit;

/* min() doesn't work on bitfields with gcc-3.5 */
if (size > 64)
size = 64;
desc->wMaxPacketSize = cpu_to_le16(size);
}

ep->address = desc->bEndpointAddress;
ep->desc = NULL;
ep->comp_desc = NULL;
ep->claimed = true;
return ep;
}

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH linux-next v2 1/1] usb: gadget: udc: atmel: Update endpoint allocation scheme

[Felipe Balbi]

Hi,

Cristian Birsan  writes:
>>> +   /* It might be a little bit late to set this */
>> 
>> Is it too late or not? This comment is frightening... We may need some
>> feedback from the USB guys here...
>>
>
> If someone from USB can comment a bit on this topic I will be grateful.

it is not too late :-) Just look at the implementation of
usb_ep_autoconfig_ss(). That said, I haven't considered if gadget
drivers might want this value to be valid before calling
usb_ep_autoconfig_ss(). I can't see why they would, but still; just a
little warning.

struct usb_ep *usb_ep_autoconfig_ss(
struct usb_gadget   *gadget,
struct usb_endpoint_descriptor  *desc,
struct usb_ss_ep_comp_descriptor *ep_comp
)
{
struct usb_ep   *ep;
u8  type;

type = desc->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK;

if (gadget->ops->match_ep) {
ep = gadget->ops->match_ep(gadget, desc, ep_comp);
if (ep)
goto found_ep;
}

/* Second, look at endpoints until an unclaimed one looks usable */
list_for_each_entry (ep, >ep_list, ep_list) {
if (usb_gadget_ep_match_desc(gadget, ep, desc, ep_comp))
goto found_ep;
}

/* Fail */
return NULL;
found_ep:

/*
 * If the protocol driver hasn't yet decided on wMaxPacketSize
 * and wants to know the maximum possible, provide the info.
 */
if (desc->wMaxPacketSize == 0)
desc->wMaxPacketSize = cpu_to_le16(ep->maxpacket_limit);

/* report address */
desc->bEndpointAddress &= USB_DIR_IN;
if (isdigit(ep->name[2])) {
u8 num = simple_strtoul(>name[2], NULL, 10);
desc->bEndpointAddress |= num;
} else if (desc->bEndpointAddress & USB_DIR_IN) {
if (++gadget->in_epnum > 15)
return NULL;
desc->bEndpointAddress = USB_DIR_IN | gadget->in_epnum;
} else {
if (++gadget->out_epnum > 15)
return NULL;
desc->bEndpointAddress |= gadget->out_epnum;
}

/* report (variable) full speed bulk maxpacket */
if ((type == USB_ENDPOINT_XFER_BULK) && !ep_comp) {
int size = ep->maxpacket_limit;

/* min() doesn't work on bitfields with gcc-3.5 */
if (size > 64)
size = 64;
desc->wMaxPacketSize = cpu_to_le16(size);
}

ep->address = desc->bEndpointAddress;
ep->desc = NULL;
ep->comp_desc = NULL;
ep->claimed = true;
return ep;
}

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH linux-next v2 1/1] usb: gadget: udc: atmel: Update endpoint allocation scheme

[Nicolas Ferre]

Le 27/01/2017 à 14:47, Cristian Birsan a écrit :
> I will send a fixup patch with updates based on the comments received from 
> Nicolas.
> 
> On 01/26/2017 05:02 PM, Nicolas Ferre wrote:
>> Le 23/01/2017 à 15:45, cristian.bir...@microchip.com a écrit :
>>> From: Cristian Birsan 
>>>
>>> Update atmel udc driver with a new enpoint allocation scheme. The data
>>> sheet requires that all endpoints are allocated in order.
>>
>> Pieces of information from the cover letter could have been added here.
>>
> 
> Ack. I'll remove the cover letter.
> 
>>> Signed-off-by: Cristian Birsan 
>>> ---
>>>  drivers/usb/gadget/udc/Kconfig  |  14 ++
>>>  drivers/usb/gadget/udc/atmel_usba_udc.c | 236 
>>> +++-
>>>  drivers/usb/gadget/udc/atmel_usba_udc.h |  10 +-
>>>  3 files changed, 227 insertions(+), 33 deletions(-)
>>
>> It can be good to run ./scripts/checkpatch.pl --strict as well. It would
>> have told you about some of the alignment and braces issues. I ran it as
>> some the code has looked weird to me once or twice...
>>
>>
> 
> Ack.
> 
>>> diff --git a/drivers/usb/gadget/udc/Kconfig b/drivers/usb/gadget/udc/Kconfig
>>> index 658b8da..4b69f28 100644
>>> --- a/drivers/usb/gadget/udc/Kconfig
>>> +++ b/drivers/usb/gadget/udc/Kconfig
>>> @@ -60,6 +60,20 @@ config USB_ATMEL_USBA
>>>   USBA is the integrated high-speed USB Device controller on
>>>   the AT32AP700x, some AT91SAM9 and AT91CAP9 processors from Atmel.
>>>  
>>> + The fifo_mode parameter is used to select endpoint allocation mode.
>>> + fifo_mode = 0 is used to let the driver autoconfigure the endpoints.
>>> + In this case 2 banks are allocated for isochronous endpoints and
>>
>> You mean that 2 banks can do isochronous xfers if needed, but they can
>> do other types as well, right? So maybe rephrase the sentence.
>>
> 
> For fifo_mode = 0 (autoconfigure case) the driver allocates 2 banks only for
> isochronous endpoints as required by the reference manual. For other endpoints
> it will allocate only 1 bank because it cannot know in advance the number of
> endpoints and the size of them, especially if the configfs is used to create
> the gadget. The idea is to support as many endpoints as we can in this mode.

You know that I know this. It was just a remark about the wording of
your sentence. "are allocated for isochronous" can be turned into "are
allocated so that they could be use as isochronous endpoints". Not a big
deal though.


>>> + only one bank is allocated for the rest of the endpoints.
>>> +
>>> + fifo_mode = 1 is a generic maximum fifo size (1024 bytes) 
>>> configuration
>>> + allowing the usage of ep1 - ep6
>>> +
>>> + fifo_mode = 2 is a generic performance maximum fifo size (1024 bytes)
>>> + configuration allowing the usage of ep1 - ep3
>>> +
>>> + fifo_mode = 3 is a balanced performance configuration allowing the
>>> + the usage of ep1 - ep8
>>> +
>>>  config USB_BCM63XX_UDC
>>> tristate "Broadcom BCM63xx Peripheral Controller"
>>> depends on BCM63XX
>>> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c 
>>> b/drivers/usb/gadget/udc/atmel_usba_udc.c
>>> index 12c7687..11bbce2 100644
>>> --- a/drivers/usb/gadget/udc/atmel_usba_udc.c
>>> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
>>> @@ -20,6 +20,7 @@
>>>  #include 
>>>  #include 
>>>  #include 
>>> +#include 
>>>  #include 
>>>  #include 
>>>  #include 
>>> @@ -318,6 +319,91 @@ static inline void usba_cleanup_debugfs(struct 
>>> usba_udc *udc)
>>>  }
>>>  #endif
>>>  
>>> +static ushort fifo_mode;
>>> +
>>> +/* "modprobe ... fifo_mode=1" etc */
>>
>> No need for this comment.
>>
>  
> Ack. I'll remove it.
> 
>>> +module_param(fifo_mode, ushort, 0x0);
>>> +MODULE_PARM_DESC(fifo_mode, "Endpoint configuration mode");
>>> +
>>> +/* mode 0 - uses autoconfig */
>>> +
>>> +/* mode 1 - fits in 8KB, generic max fifo configuration */
>>> +static struct usba_fifo_cfg mode_1_cfg[] = {
>>> +{ .hw_ep_num = 0, .fifo_size = 64, .nr_banks = 1, },
>>> +{ .hw_ep_num = 1, .fifo_size = 1024,   .nr_banks = 2, },
>>> +{ .hw_ep_num = 2, .fifo_size = 1024,   .nr_banks = 1, },
>>> +{ .hw_ep_num = 3, .fifo_size = 1024,   .nr_banks = 1, },
>>> +{ .hw_ep_num = 4, .fifo_size = 1024,   .nr_banks = 1, },
>>> +{ .hw_ep_num = 5, .fifo_size = 1024,   .nr_banks = 1, },
>>> +{ .hw_ep_num = 6, .fifo_size = 1024,   .nr_banks = 1, },
>>> +};
>>> +
>>> +/* mode 2 - fits in 8KB, performance max fifo configuration */
>>> +static struct usba_fifo_cfg mode_2_cfg[] = {
>>> +{ .hw_ep_num = 0, .fifo_size = 64, .nr_banks = 1, },
>>> +{ .hw_ep_num = 1, .fifo_size = 1024,   .nr_banks = 3, },
>>> +{ .hw_ep_num = 2, .fifo_size = 1024,   .nr_banks = 2, },
>>> +{ .hw_ep_num = 3, .fifo_size = 1024,   .nr_banks = 2, },
>>> +};
>>> +
>>> +/* mode 3 - fits in 8KB, mixed fifo configuration */
>>> +static struct usba_fifo_cfg mode_3_cfg[] = {
>>> +{ .hw_ep_num = 0, 

Re: [PATCH linux-next v2 1/1] usb: gadget: udc: atmel: Update endpoint allocation scheme

[Nicolas Ferre]

Le 27/01/2017 à 14:47, Cristian Birsan a écrit :
> I will send a fixup patch with updates based on the comments received from 
> Nicolas.
> 
> On 01/26/2017 05:02 PM, Nicolas Ferre wrote:
>> Le 23/01/2017 à 15:45, cristian.bir...@microchip.com a écrit :
>>> From: Cristian Birsan 
>>>
>>> Update atmel udc driver with a new enpoint allocation scheme. The data
>>> sheet requires that all endpoints are allocated in order.
>>
>> Pieces of information from the cover letter could have been added here.
>>
> 
> Ack. I'll remove the cover letter.
> 
>>> Signed-off-by: Cristian Birsan 
>>> ---
>>>  drivers/usb/gadget/udc/Kconfig  |  14 ++
>>>  drivers/usb/gadget/udc/atmel_usba_udc.c | 236 
>>> +++-
>>>  drivers/usb/gadget/udc/atmel_usba_udc.h |  10 +-
>>>  3 files changed, 227 insertions(+), 33 deletions(-)
>>
>> It can be good to run ./scripts/checkpatch.pl --strict as well. It would
>> have told you about some of the alignment and braces issues. I ran it as
>> some the code has looked weird to me once or twice...
>>
>>
> 
> Ack.
> 
>>> diff --git a/drivers/usb/gadget/udc/Kconfig b/drivers/usb/gadget/udc/Kconfig
>>> index 658b8da..4b69f28 100644
>>> --- a/drivers/usb/gadget/udc/Kconfig
>>> +++ b/drivers/usb/gadget/udc/Kconfig
>>> @@ -60,6 +60,20 @@ config USB_ATMEL_USBA
>>>   USBA is the integrated high-speed USB Device controller on
>>>   the AT32AP700x, some AT91SAM9 and AT91CAP9 processors from Atmel.
>>>  
>>> + The fifo_mode parameter is used to select endpoint allocation mode.
>>> + fifo_mode = 0 is used to let the driver autoconfigure the endpoints.
>>> + In this case 2 banks are allocated for isochronous endpoints and
>>
>> You mean that 2 banks can do isochronous xfers if needed, but they can
>> do other types as well, right? So maybe rephrase the sentence.
>>
> 
> For fifo_mode = 0 (autoconfigure case) the driver allocates 2 banks only for
> isochronous endpoints as required by the reference manual. For other endpoints
> it will allocate only 1 bank because it cannot know in advance the number of
> endpoints and the size of them, especially if the configfs is used to create
> the gadget. The idea is to support as many endpoints as we can in this mode.

You know that I know this. It was just a remark about the wording of
your sentence. "are allocated for isochronous" can be turned into "are
allocated so that they could be use as isochronous endpoints". Not a big
deal though.


>>> + only one bank is allocated for the rest of the endpoints.
>>> +
>>> + fifo_mode = 1 is a generic maximum fifo size (1024 bytes) 
>>> configuration
>>> + allowing the usage of ep1 - ep6
>>> +
>>> + fifo_mode = 2 is a generic performance maximum fifo size (1024 bytes)
>>> + configuration allowing the usage of ep1 - ep3
>>> +
>>> + fifo_mode = 3 is a balanced performance configuration allowing the
>>> + the usage of ep1 - ep8
>>> +
>>>  config USB_BCM63XX_UDC
>>> tristate "Broadcom BCM63xx Peripheral Controller"
>>> depends on BCM63XX
>>> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c 
>>> b/drivers/usb/gadget/udc/atmel_usba_udc.c
>>> index 12c7687..11bbce2 100644
>>> --- a/drivers/usb/gadget/udc/atmel_usba_udc.c
>>> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
>>> @@ -20,6 +20,7 @@
>>>  #include 
>>>  #include 
>>>  #include 
>>> +#include 
>>>  #include 
>>>  #include 
>>>  #include 
>>> @@ -318,6 +319,91 @@ static inline void usba_cleanup_debugfs(struct 
>>> usba_udc *udc)
>>>  }
>>>  #endif
>>>  
>>> +static ushort fifo_mode;
>>> +
>>> +/* "modprobe ... fifo_mode=1" etc */
>>
>> No need for this comment.
>>
>  
> Ack. I'll remove it.
> 
>>> +module_param(fifo_mode, ushort, 0x0);
>>> +MODULE_PARM_DESC(fifo_mode, "Endpoint configuration mode");
>>> +
>>> +/* mode 0 - uses autoconfig */
>>> +
>>> +/* mode 1 - fits in 8KB, generic max fifo configuration */
>>> +static struct usba_fifo_cfg mode_1_cfg[] = {
>>> +{ .hw_ep_num = 0, .fifo_size = 64, .nr_banks = 1, },
>>> +{ .hw_ep_num = 1, .fifo_size = 1024,   .nr_banks = 2, },
>>> +{ .hw_ep_num = 2, .fifo_size = 1024,   .nr_banks = 1, },
>>> +{ .hw_ep_num = 3, .fifo_size = 1024,   .nr_banks = 1, },
>>> +{ .hw_ep_num = 4, .fifo_size = 1024,   .nr_banks = 1, },
>>> +{ .hw_ep_num = 5, .fifo_size = 1024,   .nr_banks = 1, },
>>> +{ .hw_ep_num = 6, .fifo_size = 1024,   .nr_banks = 1, },
>>> +};
>>> +
>>> +/* mode 2 - fits in 8KB, performance max fifo configuration */
>>> +static struct usba_fifo_cfg mode_2_cfg[] = {
>>> +{ .hw_ep_num = 0, .fifo_size = 64, .nr_banks = 1, },
>>> +{ .hw_ep_num = 1, .fifo_size = 1024,   .nr_banks = 3, },
>>> +{ .hw_ep_num = 2, .fifo_size = 1024,   .nr_banks = 2, },
>>> +{ .hw_ep_num = 3, .fifo_size = 1024,   .nr_banks = 2, },
>>> +};
>>> +
>>> +/* mode 3 - fits in 8KB, mixed fifo configuration */
>>> +static struct usba_fifo_cfg mode_3_cfg[] = {
>>> +{ .hw_ep_num = 0, .fifo_size = 64, .nr_banks = 1, },
>>> +{ .hw_ep_num = 1, 

Re: [PATCH linux-next v2 1/1] usb: gadget: udc: atmel: Update endpoint allocation scheme

[Cristian Birsan]

I will send a fixup patch with updates based on the comments received from 
Nicolas.

On 01/26/2017 05:02 PM, Nicolas Ferre wrote:
> Le 23/01/2017 à 15:45, cristian.bir...@microchip.com a écrit :
>> From: Cristian Birsan 
>>
>> Update atmel udc driver with a new enpoint allocation scheme. The data
>> sheet requires that all endpoints are allocated in order.
> 
> Pieces of information from the cover letter could have been added here.
>

Ack. I'll remove the cover letter.

>> Signed-off-by: Cristian Birsan 
>> ---
>>  drivers/usb/gadget/udc/Kconfig  |  14 ++
>>  drivers/usb/gadget/udc/atmel_usba_udc.c | 236 
>> +++-
>>  drivers/usb/gadget/udc/atmel_usba_udc.h |  10 +-
>>  3 files changed, 227 insertions(+), 33 deletions(-)
> 
> It can be good to run ./scripts/checkpatch.pl --strict as well. It would
> have told you about some of the alignment and braces issues. I ran it as
> some the code has looked weird to me once or twice...
> 
> 

Ack.

>> diff --git a/drivers/usb/gadget/udc/Kconfig b/drivers/usb/gadget/udc/Kconfig
>> index 658b8da..4b69f28 100644
>> --- a/drivers/usb/gadget/udc/Kconfig
>> +++ b/drivers/usb/gadget/udc/Kconfig
>> @@ -60,6 +60,20 @@ config USB_ATMEL_USBA
>>USBA is the integrated high-speed USB Device controller on
>>the AT32AP700x, some AT91SAM9 and AT91CAP9 processors from Atmel.
>>  
>> +  The fifo_mode parameter is used to select endpoint allocation mode.
>> +  fifo_mode = 0 is used to let the driver autoconfigure the endpoints.
>> +  In this case 2 banks are allocated for isochronous endpoints and
> 
> You mean that 2 banks can do isochronous xfers if needed, but they can
> do other types as well, right? So maybe rephrase the sentence.
> 

For fifo_mode = 0 (autoconfigure case) the driver allocates 2 banks only for
isochronous endpoints as required by the reference manual. For other endpoints
it will allocate only 1 bank because it cannot know in advance the number of
endpoints and the size of them, especially if the configfs is used to create
the gadget. The idea is to support as many endpoints as we can in this mode.

>> +  only one bank is allocated for the rest of the endpoints.
>> +
>> +  fifo_mode = 1 is a generic maximum fifo size (1024 bytes) 
>> configuration
>> +  allowing the usage of ep1 - ep6
>> +
>> +  fifo_mode = 2 is a generic performance maximum fifo size (1024 bytes)
>> +  configuration allowing the usage of ep1 - ep3
>> +
>> +  fifo_mode = 3 is a balanced performance configuration allowing the
>> +  the usage of ep1 - ep8
>> +
>>  config USB_BCM63XX_UDC
>>  tristate "Broadcom BCM63xx Peripheral Controller"
>>  depends on BCM63XX
>> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c 
>> b/drivers/usb/gadget/udc/atmel_usba_udc.c
>> index 12c7687..11bbce2 100644
>> --- a/drivers/usb/gadget/udc/atmel_usba_udc.c
>> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
>> @@ -20,6 +20,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>  #include 
>>  #include 
>>  #include 
>> @@ -318,6 +319,91 @@ static inline void usba_cleanup_debugfs(struct usba_udc 
>> *udc)
>>  }
>>  #endif
>>  
>> +static ushort fifo_mode;
>> +
>> +/* "modprobe ... fifo_mode=1" etc */
> 
> No need for this comment.
>
 
Ack. I'll remove it.

>> +module_param(fifo_mode, ushort, 0x0);
>> +MODULE_PARM_DESC(fifo_mode, "Endpoint configuration mode");
>> +
>> +/* mode 0 - uses autoconfig */
>> +
>> +/* mode 1 - fits in 8KB, generic max fifo configuration */
>> +static struct usba_fifo_cfg mode_1_cfg[] = {
>> +{ .hw_ep_num = 0, .fifo_size = 64,  .nr_banks = 1, },
>> +{ .hw_ep_num = 1, .fifo_size = 1024,.nr_banks = 2, },
>> +{ .hw_ep_num = 2, .fifo_size = 1024,.nr_banks = 1, },
>> +{ .hw_ep_num = 3, .fifo_size = 1024,.nr_banks = 1, },
>> +{ .hw_ep_num = 4, .fifo_size = 1024,.nr_banks = 1, },
>> +{ .hw_ep_num = 5, .fifo_size = 1024,.nr_banks = 1, },
>> +{ .hw_ep_num = 6, .fifo_size = 1024,.nr_banks = 1, },
>> +};
>> +
>> +/* mode 2 - fits in 8KB, performance max fifo configuration */
>> +static struct usba_fifo_cfg mode_2_cfg[] = {
>> +{ .hw_ep_num = 0, .fifo_size = 64,  .nr_banks = 1, },
>> +{ .hw_ep_num = 1, .fifo_size = 1024,.nr_banks = 3, },
>> +{ .hw_ep_num = 2, .fifo_size = 1024,.nr_banks = 2, },
>> +{ .hw_ep_num = 3, .fifo_size = 1024,.nr_banks = 2, },
>> +};
>> +
>> +/* mode 3 - fits in 8KB, mixed fifo configuration */
>> +static struct usba_fifo_cfg mode_3_cfg[] = {
>> +{ .hw_ep_num = 0, .fifo_size = 64,  .nr_banks = 1, },
>> +{ .hw_ep_num = 1, .fifo_size = 1024,.nr_banks = 2, },
>> +{ .hw_ep_num = 2, .fifo_size = 512, .nr_banks = 2, },
>> +{ .hw_ep_num = 3, .fifo_size = 512, .nr_banks = 2, },
>> +{ .hw_ep_num = 4, .fifo_size = 512, .nr_banks = 2, },
>> +{ .hw_ep_num = 5, .fifo_size = 512, .nr_banks = 2, },
>> +{ .hw_ep_num = 6, .fifo_size = 512, .nr_banks 

Re: [PATCH linux-next v2 1/1] usb: gadget: udc: atmel: Update endpoint allocation scheme

[Cristian Birsan]

I will send a fixup patch with updates based on the comments received from 
Nicolas.

On 01/26/2017 05:02 PM, Nicolas Ferre wrote:
> Le 23/01/2017 à 15:45, cristian.bir...@microchip.com a écrit :
>> From: Cristian Birsan 
>>
>> Update atmel udc driver with a new enpoint allocation scheme. The data
>> sheet requires that all endpoints are allocated in order.
> 
> Pieces of information from the cover letter could have been added here.
>

Ack. I'll remove the cover letter.

>> Signed-off-by: Cristian Birsan 
>> ---
>>  drivers/usb/gadget/udc/Kconfig  |  14 ++
>>  drivers/usb/gadget/udc/atmel_usba_udc.c | 236 
>> +++-
>>  drivers/usb/gadget/udc/atmel_usba_udc.h |  10 +-
>>  3 files changed, 227 insertions(+), 33 deletions(-)
> 
> It can be good to run ./scripts/checkpatch.pl --strict as well. It would
> have told you about some of the alignment and braces issues. I ran it as
> some the code has looked weird to me once or twice...
> 
> 

Ack.

>> diff --git a/drivers/usb/gadget/udc/Kconfig b/drivers/usb/gadget/udc/Kconfig
>> index 658b8da..4b69f28 100644
>> --- a/drivers/usb/gadget/udc/Kconfig
>> +++ b/drivers/usb/gadget/udc/Kconfig
>> @@ -60,6 +60,20 @@ config USB_ATMEL_USBA
>>USBA is the integrated high-speed USB Device controller on
>>the AT32AP700x, some AT91SAM9 and AT91CAP9 processors from Atmel.
>>  
>> +  The fifo_mode parameter is used to select endpoint allocation mode.
>> +  fifo_mode = 0 is used to let the driver autoconfigure the endpoints.
>> +  In this case 2 banks are allocated for isochronous endpoints and
> 
> You mean that 2 banks can do isochronous xfers if needed, but they can
> do other types as well, right? So maybe rephrase the sentence.
> 

For fifo_mode = 0 (autoconfigure case) the driver allocates 2 banks only for
isochronous endpoints as required by the reference manual. For other endpoints
it will allocate only 1 bank because it cannot know in advance the number of
endpoints and the size of them, especially if the configfs is used to create
the gadget. The idea is to support as many endpoints as we can in this mode.

>> +  only one bank is allocated for the rest of the endpoints.
>> +
>> +  fifo_mode = 1 is a generic maximum fifo size (1024 bytes) 
>> configuration
>> +  allowing the usage of ep1 - ep6
>> +
>> +  fifo_mode = 2 is a generic performance maximum fifo size (1024 bytes)
>> +  configuration allowing the usage of ep1 - ep3
>> +
>> +  fifo_mode = 3 is a balanced performance configuration allowing the
>> +  the usage of ep1 - ep8
>> +
>>  config USB_BCM63XX_UDC
>>  tristate "Broadcom BCM63xx Peripheral Controller"
>>  depends on BCM63XX
>> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c 
>> b/drivers/usb/gadget/udc/atmel_usba_udc.c
>> index 12c7687..11bbce2 100644
>> --- a/drivers/usb/gadget/udc/atmel_usba_udc.c
>> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
>> @@ -20,6 +20,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>  #include 
>>  #include 
>>  #include 
>> @@ -318,6 +319,91 @@ static inline void usba_cleanup_debugfs(struct usba_udc 
>> *udc)
>>  }
>>  #endif
>>  
>> +static ushort fifo_mode;
>> +
>> +/* "modprobe ... fifo_mode=1" etc */
> 
> No need for this comment.
>
 
Ack. I'll remove it.

>> +module_param(fifo_mode, ushort, 0x0);
>> +MODULE_PARM_DESC(fifo_mode, "Endpoint configuration mode");
>> +
>> +/* mode 0 - uses autoconfig */
>> +
>> +/* mode 1 - fits in 8KB, generic max fifo configuration */
>> +static struct usba_fifo_cfg mode_1_cfg[] = {
>> +{ .hw_ep_num = 0, .fifo_size = 64,  .nr_banks = 1, },
>> +{ .hw_ep_num = 1, .fifo_size = 1024,.nr_banks = 2, },
>> +{ .hw_ep_num = 2, .fifo_size = 1024,.nr_banks = 1, },
>> +{ .hw_ep_num = 3, .fifo_size = 1024,.nr_banks = 1, },
>> +{ .hw_ep_num = 4, .fifo_size = 1024,.nr_banks = 1, },
>> +{ .hw_ep_num = 5, .fifo_size = 1024,.nr_banks = 1, },
>> +{ .hw_ep_num = 6, .fifo_size = 1024,.nr_banks = 1, },
>> +};
>> +
>> +/* mode 2 - fits in 8KB, performance max fifo configuration */
>> +static struct usba_fifo_cfg mode_2_cfg[] = {
>> +{ .hw_ep_num = 0, .fifo_size = 64,  .nr_banks = 1, },
>> +{ .hw_ep_num = 1, .fifo_size = 1024,.nr_banks = 3, },
>> +{ .hw_ep_num = 2, .fifo_size = 1024,.nr_banks = 2, },
>> +{ .hw_ep_num = 3, .fifo_size = 1024,.nr_banks = 2, },
>> +};
>> +
>> +/* mode 3 - fits in 8KB, mixed fifo configuration */
>> +static struct usba_fifo_cfg mode_3_cfg[] = {
>> +{ .hw_ep_num = 0, .fifo_size = 64,  .nr_banks = 1, },
>> +{ .hw_ep_num = 1, .fifo_size = 1024,.nr_banks = 2, },
>> +{ .hw_ep_num = 2, .fifo_size = 512, .nr_banks = 2, },
>> +{ .hw_ep_num = 3, .fifo_size = 512, .nr_banks = 2, },
>> +{ .hw_ep_num = 4, .fifo_size = 512, .nr_banks = 2, },
>> +{ .hw_ep_num = 5, .fifo_size = 512, .nr_banks = 2, },
>> +{ .hw_ep_num = 6, .fifo_size = 512, .nr_banks = 2, },
>> +};
>> +
>> +/* mode 4 - fits in 8KB, custom fifo 

Re: [PATCH linux-next v2 1/1] usb: gadget: udc: atmel: Update endpoint allocation scheme

[Felipe Balbi]

Nicolas Ferre  writes:

> Le 23/01/2017 à 15:45, cristian.bir...@microchip.com a écrit :
>> From: Cristian Birsan 
>> 
>> Update atmel udc driver with a new enpoint allocation scheme. The data
>> sheet requires that all endpoints are allocated in order.
>
> Pieces of information from the cover letter could have been added here.

yup, that's what I did:

commit 741d2558bf0aa8da9c0834ad43e1b9a1b16aa515
Author: Cristian Birsan 
Date:   Mon Jan 23 16:45:59 2017 +0200

usb: gadget: udc: atmel: Update endpoint allocation scheme

This patch updates the usb endpoint allocation scheme for atmel usba
driver to make sure all endpoints are allocated in order. This
requirement comes from the datasheet of the controller.

The allocation scheme is decided by fifo_mode parameter. For fifo_mode =
0 the driver tries to autoconfigure the endpoints fifo size. All other
modes contain fixed configurations optimized for different purposes. The
idea is somehow similar with the approach used on musb driver.

Signed-off-by: Cristian Birsan 
Signed-off-by: Felipe Balbi 

But seems like we will need a fixup patch during v4.11-rc?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH linux-next v2 1/1] usb: gadget: udc: atmel: Update endpoint allocation scheme

[Felipe Balbi]

Nicolas Ferre  writes:

> Le 23/01/2017 à 15:45, cristian.bir...@microchip.com a écrit :
>> From: Cristian Birsan 
>> 
>> Update atmel udc driver with a new enpoint allocation scheme. The data
>> sheet requires that all endpoints are allocated in order.
>
> Pieces of information from the cover letter could have been added here.

yup, that's what I did:

commit 741d2558bf0aa8da9c0834ad43e1b9a1b16aa515
Author: Cristian Birsan 
Date:   Mon Jan 23 16:45:59 2017 +0200

usb: gadget: udc: atmel: Update endpoint allocation scheme

This patch updates the usb endpoint allocation scheme for atmel usba
driver to make sure all endpoints are allocated in order. This
requirement comes from the datasheet of the controller.

The allocation scheme is decided by fifo_mode parameter. For fifo_mode =
0 the driver tries to autoconfigure the endpoints fifo size. All other
modes contain fixed configurations optimized for different purposes. The
idea is somehow similar with the approach used on musb driver.

Signed-off-by: Cristian Birsan 
Signed-off-by: Felipe Balbi 

But seems like we will need a fixup patch during v4.11-rc?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH linux-next v2 1/1] usb: gadget: udc: atmel: Update endpoint allocation scheme

[Nicolas Ferre]

Le 23/01/2017 à 15:45, cristian.bir...@microchip.com a écrit :
> From: Cristian Birsan 
> 
> Update atmel udc driver with a new enpoint allocation scheme. The data
> sheet requires that all endpoints are allocated in order.

Pieces of information from the cover letter could have been added here.

> Signed-off-by: Cristian Birsan 
> ---
>  drivers/usb/gadget/udc/Kconfig  |  14 ++
>  drivers/usb/gadget/udc/atmel_usba_udc.c | 236 
> +++-
>  drivers/usb/gadget/udc/atmel_usba_udc.h |  10 +-
>  3 files changed, 227 insertions(+), 33 deletions(-)

It can be good to run ./scripts/checkpatch.pl --strict as well. It would
have told you about some of the alignment and braces issues. I ran it as
some the code has looked weird to me once or twice...


> diff --git a/drivers/usb/gadget/udc/Kconfig b/drivers/usb/gadget/udc/Kconfig
> index 658b8da..4b69f28 100644
> --- a/drivers/usb/gadget/udc/Kconfig
> +++ b/drivers/usb/gadget/udc/Kconfig
> @@ -60,6 +60,20 @@ config USB_ATMEL_USBA
> USBA is the integrated high-speed USB Device controller on
> the AT32AP700x, some AT91SAM9 and AT91CAP9 processors from Atmel.
>  
> +   The fifo_mode parameter is used to select endpoint allocation mode.
> +   fifo_mode = 0 is used to let the driver autoconfigure the endpoints.
> +   In this case 2 banks are allocated for isochronous endpoints and

You mean that 2 banks can do isochronous xfers if needed, but they can
do other types as well, right? So maybe rephrase the sentence.

> +   only one bank is allocated for the rest of the endpoints.
> +
> +   fifo_mode = 1 is a generic maximum fifo size (1024 bytes) 
> configuration
> +   allowing the usage of ep1 - ep6
> +
> +   fifo_mode = 2 is a generic performance maximum fifo size (1024 bytes)
> +   configuration allowing the usage of ep1 - ep3
> +
> +   fifo_mode = 3 is a balanced performance configuration allowing the
> +   the usage of ep1 - ep8
> +
>  config USB_BCM63XX_UDC
>   tristate "Broadcom BCM63xx Peripheral Controller"
>   depends on BCM63XX
> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c 
> b/drivers/usb/gadget/udc/atmel_usba_udc.c
> index 12c7687..11bbce2 100644
> --- a/drivers/usb/gadget/udc/atmel_usba_udc.c
> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
> @@ -20,6 +20,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -318,6 +319,91 @@ static inline void usba_cleanup_debugfs(struct usba_udc 
> *udc)
>  }
>  #endif
>  
> +static ushort fifo_mode;
> +
> +/* "modprobe ... fifo_mode=1" etc */

No need for this comment.

> +module_param(fifo_mode, ushort, 0x0);
> +MODULE_PARM_DESC(fifo_mode, "Endpoint configuration mode");
> +
> +/* mode 0 - uses autoconfig */
> +
> +/* mode 1 - fits in 8KB, generic max fifo configuration */
> +static struct usba_fifo_cfg mode_1_cfg[] = {
> +{ .hw_ep_num = 0, .fifo_size = 64,   .nr_banks = 1, },
> +{ .hw_ep_num = 1, .fifo_size = 1024, .nr_banks = 2, },
> +{ .hw_ep_num = 2, .fifo_size = 1024, .nr_banks = 1, },
> +{ .hw_ep_num = 3, .fifo_size = 1024, .nr_banks = 1, },
> +{ .hw_ep_num = 4, .fifo_size = 1024, .nr_banks = 1, },
> +{ .hw_ep_num = 5, .fifo_size = 1024, .nr_banks = 1, },
> +{ .hw_ep_num = 6, .fifo_size = 1024, .nr_banks = 1, },
> +};
> +
> +/* mode 2 - fits in 8KB, performance max fifo configuration */
> +static struct usba_fifo_cfg mode_2_cfg[] = {
> +{ .hw_ep_num = 0, .fifo_size = 64,   .nr_banks = 1, },
> +{ .hw_ep_num = 1, .fifo_size = 1024, .nr_banks = 3, },
> +{ .hw_ep_num = 2, .fifo_size = 1024, .nr_banks = 2, },
> +{ .hw_ep_num = 3, .fifo_size = 1024, .nr_banks = 2, },
> +};
> +
> +/* mode 3 - fits in 8KB, mixed fifo configuration */
> +static struct usba_fifo_cfg mode_3_cfg[] = {
> +{ .hw_ep_num = 0, .fifo_size = 64,   .nr_banks = 1, },
> +{ .hw_ep_num = 1, .fifo_size = 1024, .nr_banks = 2, },
> +{ .hw_ep_num = 2, .fifo_size = 512,  .nr_banks = 2, },
> +{ .hw_ep_num = 3, .fifo_size = 512,  .nr_banks = 2, },
> +{ .hw_ep_num = 4, .fifo_size = 512,  .nr_banks = 2, },
> +{ .hw_ep_num = 5, .fifo_size = 512,  .nr_banks = 2, },
> +{ .hw_ep_num = 6, .fifo_size = 512,  .nr_banks = 2, },
> +};
> +
> +/* mode 4 - fits in 8KB, custom fifo configuration */
> +static struct usba_fifo_cfg mode_4_cfg[] = {
> +{ .hw_ep_num = 0, .fifo_size = 64,   .nr_banks = 1, },
> +{ .hw_ep_num = 1, .fifo_size = 512,  .nr_banks = 2, },
> +{ .hw_ep_num = 2, .fifo_size = 512,  .nr_banks = 2, },
> +{ .hw_ep_num = 3, .fifo_size = 8,.nr_banks = 2, },
> +{ .hw_ep_num = 4, .fifo_size = 512,  .nr_banks = 2, },
> +{ .hw_ep_num = 5, .fifo_size = 512,  .nr_banks = 2, },
> +{ .hw_ep_num = 6, .fifo_size = 16,   .nr_banks = 2, },
> +{ .hw_ep_num = 7, .fifo_size = 8,.nr_banks = 2, },
> +{ .hw_ep_num = 8, .fifo_size = 8,.nr_banks = 2, },
> +};
> +/* Add additional configurations here */
> +
> +int usba_config_fifo_table(struct usba_udc *udc)

Isn't 

Re: [PATCH linux-next v2 1/1] usb: gadget: udc: atmel: Update endpoint allocation scheme

[Nicolas Ferre]

Le 23/01/2017 à 15:45, cristian.bir...@microchip.com a écrit :
> From: Cristian Birsan 
> 
> Update atmel udc driver with a new enpoint allocation scheme. The data
> sheet requires that all endpoints are allocated in order.

Pieces of information from the cover letter could have been added here.

> Signed-off-by: Cristian Birsan 
> ---
>  drivers/usb/gadget/udc/Kconfig  |  14 ++
>  drivers/usb/gadget/udc/atmel_usba_udc.c | 236 
> +++-
>  drivers/usb/gadget/udc/atmel_usba_udc.h |  10 +-
>  3 files changed, 227 insertions(+), 33 deletions(-)

It can be good to run ./scripts/checkpatch.pl --strict as well. It would
have told you about some of the alignment and braces issues. I ran it as
some the code has looked weird to me once or twice...


> diff --git a/drivers/usb/gadget/udc/Kconfig b/drivers/usb/gadget/udc/Kconfig
> index 658b8da..4b69f28 100644
> --- a/drivers/usb/gadget/udc/Kconfig
> +++ b/drivers/usb/gadget/udc/Kconfig
> @@ -60,6 +60,20 @@ config USB_ATMEL_USBA
> USBA is the integrated high-speed USB Device controller on
> the AT32AP700x, some AT91SAM9 and AT91CAP9 processors from Atmel.
>  
> +   The fifo_mode parameter is used to select endpoint allocation mode.
> +   fifo_mode = 0 is used to let the driver autoconfigure the endpoints.
> +   In this case 2 banks are allocated for isochronous endpoints and

You mean that 2 banks can do isochronous xfers if needed, but they can
do other types as well, right? So maybe rephrase the sentence.

> +   only one bank is allocated for the rest of the endpoints.
> +
> +   fifo_mode = 1 is a generic maximum fifo size (1024 bytes) 
> configuration
> +   allowing the usage of ep1 - ep6
> +
> +   fifo_mode = 2 is a generic performance maximum fifo size (1024 bytes)
> +   configuration allowing the usage of ep1 - ep3
> +
> +   fifo_mode = 3 is a balanced performance configuration allowing the
> +   the usage of ep1 - ep8
> +
>  config USB_BCM63XX_UDC
>   tristate "Broadcom BCM63xx Peripheral Controller"
>   depends on BCM63XX
> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c 
> b/drivers/usb/gadget/udc/atmel_usba_udc.c
> index 12c7687..11bbce2 100644
> --- a/drivers/usb/gadget/udc/atmel_usba_udc.c
> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
> @@ -20,6 +20,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -318,6 +319,91 @@ static inline void usba_cleanup_debugfs(struct usba_udc 
> *udc)
>  }
>  #endif
>  
> +static ushort fifo_mode;
> +
> +/* "modprobe ... fifo_mode=1" etc */

No need for this comment.

> +module_param(fifo_mode, ushort, 0x0);
> +MODULE_PARM_DESC(fifo_mode, "Endpoint configuration mode");
> +
> +/* mode 0 - uses autoconfig */
> +
> +/* mode 1 - fits in 8KB, generic max fifo configuration */
> +static struct usba_fifo_cfg mode_1_cfg[] = {
> +{ .hw_ep_num = 0, .fifo_size = 64,   .nr_banks = 1, },
> +{ .hw_ep_num = 1, .fifo_size = 1024, .nr_banks = 2, },
> +{ .hw_ep_num = 2, .fifo_size = 1024, .nr_banks = 1, },
> +{ .hw_ep_num = 3, .fifo_size = 1024, .nr_banks = 1, },
> +{ .hw_ep_num = 4, .fifo_size = 1024, .nr_banks = 1, },
> +{ .hw_ep_num = 5, .fifo_size = 1024, .nr_banks = 1, },
> +{ .hw_ep_num = 6, .fifo_size = 1024, .nr_banks = 1, },
> +};
> +
> +/* mode 2 - fits in 8KB, performance max fifo configuration */
> +static struct usba_fifo_cfg mode_2_cfg[] = {
> +{ .hw_ep_num = 0, .fifo_size = 64,   .nr_banks = 1, },
> +{ .hw_ep_num = 1, .fifo_size = 1024, .nr_banks = 3, },
> +{ .hw_ep_num = 2, .fifo_size = 1024, .nr_banks = 2, },
> +{ .hw_ep_num = 3, .fifo_size = 1024, .nr_banks = 2, },
> +};
> +
> +/* mode 3 - fits in 8KB, mixed fifo configuration */
> +static struct usba_fifo_cfg mode_3_cfg[] = {
> +{ .hw_ep_num = 0, .fifo_size = 64,   .nr_banks = 1, },
> +{ .hw_ep_num = 1, .fifo_size = 1024, .nr_banks = 2, },
> +{ .hw_ep_num = 2, .fifo_size = 512,  .nr_banks = 2, },
> +{ .hw_ep_num = 3, .fifo_size = 512,  .nr_banks = 2, },
> +{ .hw_ep_num = 4, .fifo_size = 512,  .nr_banks = 2, },
> +{ .hw_ep_num = 5, .fifo_size = 512,  .nr_banks = 2, },
> +{ .hw_ep_num = 6, .fifo_size = 512,  .nr_banks = 2, },
> +};
> +
> +/* mode 4 - fits in 8KB, custom fifo configuration */
> +static struct usba_fifo_cfg mode_4_cfg[] = {
> +{ .hw_ep_num = 0, .fifo_size = 64,   .nr_banks = 1, },
> +{ .hw_ep_num = 1, .fifo_size = 512,  .nr_banks = 2, },
> +{ .hw_ep_num = 2, .fifo_size = 512,  .nr_banks = 2, },
> +{ .hw_ep_num = 3, .fifo_size = 8,.nr_banks = 2, },
> +{ .hw_ep_num = 4, .fifo_size = 512,  .nr_banks = 2, },
> +{ .hw_ep_num = 5, .fifo_size = 512,  .nr_banks = 2, },
> +{ .hw_ep_num = 6, .fifo_size = 16,   .nr_banks = 2, },
> +{ .hw_ep_num = 7, .fifo_size = 8,.nr_banks = 2, },
> +{ .hw_ep_num = 8, .fifo_size = 8,.nr_banks = 2, },
> +};
> +/* Add additional configurations here */
> +
> +int usba_config_fifo_table(struct usba_udc *udc)

Isn't is static?

> +{
> + int n;
> +
> + switch (fifo_mode)