Re: [PATCH] usb: dwc2: gadget: Update for new usb_endpoint_maxp()

[John Youn]

On 11/9/2016 12:02 AM, Felipe Balbi wrote:
> 
> Hi,
> 
> John Youn  writes:
>>> John Youn  writes:
> John Youn  writes:
 @@ -1812,17 +1812,17 @@ static u32 dwc2_hsotg_ep0_mps(unsigned int mps)
   * @hsotg: The driver state.
   * @ep: The index number of the endpoint
   * @mps: The maximum packet size in bytes
 + * @mc: The multicount value
   *
   * Configure the maximum packet size for the given endpoint, updating
   * the hardware control registers to reflect this.
   */
  static void dwc2_hsotg_set_ep_maxpacket(struct dwc2_hsotg *hsotg,
 -  unsigned int ep, unsigned int mps, unsigned int 
 dir_in)
 +  unsigned int ep, unsigned int 
 mps,
 +  unsigned int mc, unsigned int 
 dir_in)
>>>
>>> this has an odd set of arguments. You pass the ep index, mps, direction
>>> and mult value, when you could just pass hsotg_ep and descriptor 
>>> instead.
>>
>> Yes looks like we can do some simplification here. And you probably
>> don't need to pass a descriptor either since it must be set in the
>> usb_ep before enable.
>>
>> However this is also called in some contexts where a descriptor is not
>> available (initialization and ep0). So we have to think about this a
>> bit.
>>
>> I think dwc3 can make similar simplification on the
>> __dwc3_gadget_ep_enable().
>
> __dwc3_gadget_ep_enable() takes the actual descriptors as arguments:
>
> static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep,
>   const struct usb_endpoint_descriptor *desc,
>   const struct usb_ss_ep_comp_descriptor *comp_desc,
>   bool modify, bool restore)
>
> I fail to see how much simpler we can make this. Perhaps we can turn
> bool and restore into a single argument if we use a bitfield instead of
> a bool.
>

 Hi Felipe,

 I mean that there shouldn't be any need to pass in descriptors with
 usb_ep since the ones in usb_ep should be set properly from ep enable
 to ep disable.
> 
> Oh, I see what you mean now :-)
> 
 I think that's the case anyways.
>>>
>>> __dwc3_gadget_ep_enable() is the one assigning the descriptor to the ep:
>>>
>>> static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep,
>>> const struct usb_endpoint_descriptor *desc,
>>> const struct usb_ss_ep_comp_descriptor *comp_desc,
>>> bool modify, bool restore)
>>> {
>>> struct dwc3 *dwc = dep->dwc;
>>> u32 reg;
>>> int ret;
>>>
>>> if (!(dep->flags & DWC3_EP_ENABLED)) {
>>> ret = dwc3_gadget_start_config(dwc, dep);
>>> if (ret)
>>> return ret;
>>> }
>>>
>>> ret = dwc3_gadget_set_ep_config(dwc, dep, desc, comp_desc, modify,
>>> restore);
>>> if (ret)
>>> return ret;
>>>
>>> if (!(dep->flags & DWC3_EP_ENABLED)) {
>>> struct dwc3_trb *trb_st_hw;
>>> struct dwc3_trb *trb_link;
>>>
>>> dep->endpoint.desc = desc;
>>> dep->comp_desc = comp_desc;
>>>
>>> [...]
>>>
>>
>> Right, but that's redundant for non-ep0 case. And the EP0 case can be
>> handled globally elsewhere.
>>
>> The usb_ep_enable() API function takes only the usb_ep and requires
>> that the descriptors are set in the usb_ep beforehand. So 'desc'
>> argument in the callback function is probably not required either.
>>
>> I think this is correct to make it consistent. Since we don't pass the
> 
> makes sense to me :-)
> 
>> SS comp descriptor, and we don't want to keep passing more descriptors
>> every time a new standard descriptor is added, such as the SSP ISO EP
>> comp descriptor for 3.1.
>>
>> See below for a proposed change to see what I mean (not tested).
>>
>> Regards,
>> John
>>
>>
>> >8
>>
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 2322863..b9903c6 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -539,7 +539,6 @@ struct dwc3_ep {
>>  
>>  struct dwc3_trb *trb_pool;
>>  dma_addr_t  trb_pool_dma;
>> -const struct usb_ss_ep_comp_descriptor *comp_desc;
>>  struct dwc3 *dwc;
>>  
>>  u32 saved_state;
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index e47cba5..0fc55e89 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -576,13 +576,12 @@ static int dwc3_gadget_set_xfer_resource(struct dwc3 
>> *dwc, struct dwc3_ep *dep)
>>   * Caller should take care of locking
>>   */
>>  static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep,
>> -

Re: [PATCH] usb: dwc2: gadget: Update for new usb_endpoint_maxp()

[Felipe Balbi]

Hi,

John Youn  writes:
>> John Youn  writes:
 John Youn  writes:
>>> @@ -1812,17 +1812,17 @@ static u32 dwc2_hsotg_ep0_mps(unsigned int mps)
>>>   * @hsotg: The driver state.
>>>   * @ep: The index number of the endpoint
>>>   * @mps: The maximum packet size in bytes
>>> + * @mc: The multicount value
>>>   *
>>>   * Configure the maximum packet size for the given endpoint, updating
>>>   * the hardware control registers to reflect this.
>>>   */
>>>  static void dwc2_hsotg_set_ep_maxpacket(struct dwc2_hsotg *hsotg,
>>> -   unsigned int ep, unsigned int mps, unsigned int 
>>> dir_in)
>>> +   unsigned int ep, unsigned int 
>>> mps,
>>> +   unsigned int mc, unsigned int 
>>> dir_in)
>>
>> this has an odd set of arguments. You pass the ep index, mps, direction
>> and mult value, when you could just pass hsotg_ep and descriptor instead.
>
> Yes looks like we can do some simplification here. And you probably
> don't need to pass a descriptor either since it must be set in the
> usb_ep before enable.
>
> However this is also called in some contexts where a descriptor is not
> available (initialization and ep0). So we have to think about this a
> bit.
>
> I think dwc3 can make similar simplification on the
> __dwc3_gadget_ep_enable().

 __dwc3_gadget_ep_enable() takes the actual descriptors as arguments:

 static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep,
const struct usb_endpoint_descriptor *desc,
const struct usb_ss_ep_comp_descriptor *comp_desc,
bool modify, bool restore)

 I fail to see how much simpler we can make this. Perhaps we can turn
 bool and restore into a single argument if we use a bitfield instead of
 a bool.

>>>
>>> Hi Felipe,
>>>
>>> I mean that there shouldn't be any need to pass in descriptors with
>>> usb_ep since the ones in usb_ep should be set properly from ep enable
>>> to ep disable.

Oh, I see what you mean now :-)

>>> I think that's the case anyways.
>> 
>> __dwc3_gadget_ep_enable() is the one assigning the descriptor to the ep:
>> 
>> static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep,
>>  const struct usb_endpoint_descriptor *desc,
>>  const struct usb_ss_ep_comp_descriptor *comp_desc,
>>  bool modify, bool restore)
>> {
>>  struct dwc3 *dwc = dep->dwc;
>>  u32 reg;
>>  int ret;
>> 
>>  if (!(dep->flags & DWC3_EP_ENABLED)) {
>>  ret = dwc3_gadget_start_config(dwc, dep);
>>  if (ret)
>>  return ret;
>>  }
>> 
>>  ret = dwc3_gadget_set_ep_config(dwc, dep, desc, comp_desc, modify,
>>  restore);
>>  if (ret)
>>  return ret;
>> 
>>  if (!(dep->flags & DWC3_EP_ENABLED)) {
>>  struct dwc3_trb *trb_st_hw;
>>  struct dwc3_trb *trb_link;
>> 
>>  dep->endpoint.desc = desc;
>>  dep->comp_desc = comp_desc;
>> 
>> [...]
>> 
>
> Right, but that's redundant for non-ep0 case. And the EP0 case can be
> handled globally elsewhere.
>
> The usb_ep_enable() API function takes only the usb_ep and requires
> that the descriptors are set in the usb_ep beforehand. So 'desc'
> argument in the callback function is probably not required either.
>
> I think this is correct to make it consistent. Since we don't pass the

makes sense to me :-)

> SS comp descriptor, and we don't want to keep passing more descriptors
> every time a new standard descriptor is added, such as the SSP ISO EP
> comp descriptor for 3.1.
>
> See below for a proposed change to see what I mean (not tested).
>
> Regards,
> John
>
>
> >8
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 2322863..b9903c6 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -539,7 +539,6 @@ struct dwc3_ep {
>  
>   struct dwc3_trb *trb_pool;
>   dma_addr_t  trb_pool_dma;
> - const struct usb_ss_ep_comp_descriptor *comp_desc;
>   struct dwc3 *dwc;
>  
>   u32 saved_state;
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index e47cba5..0fc55e89 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -576,13 +576,12 @@ static int dwc3_gadget_set_xfer_resource(struct dwc3 
> *dwc, struct dwc3_ep *dep)
>   * Caller should take care of locking
>   */
>  static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep,
> - const struct usb_endpoint_descriptor *desc,
> - const struct usb_ss_ep_comp_descriptor *comp_desc,
>   bool modify, bool restore)
>  {

Re: [PATCH] usb: dwc2: gadget: Update for new usb_endpoint_maxp()

[John Youn]

On 11/8/2016 2:48 AM, Felipe Balbi wrote:
> 
> Hi,
> 
> John Youn  writes:
>>> John Youn  writes:
>> @@ -1812,17 +1812,17 @@ static u32 dwc2_hsotg_ep0_mps(unsigned int mps)
>>   * @hsotg: The driver state.
>>   * @ep: The index number of the endpoint
>>   * @mps: The maximum packet size in bytes
>> + * @mc: The multicount value
>>   *
>>   * Configure the maximum packet size for the given endpoint, updating
>>   * the hardware control registers to reflect this.
>>   */
>>  static void dwc2_hsotg_set_ep_maxpacket(struct dwc2_hsotg *hsotg,
>> -unsigned int ep, unsigned int mps, unsigned int 
>> dir_in)
>> +unsigned int ep, unsigned int 
>> mps,
>> +unsigned int mc, unsigned int 
>> dir_in)
>
> this has an odd set of arguments. You pass the ep index, mps, direction
> and mult value, when you could just pass hsotg_ep and descriptor instead.

 Yes looks like we can do some simplification here. And you probably
 don't need to pass a descriptor either since it must be set in the
 usb_ep before enable.

 However this is also called in some contexts where a descriptor is not
 available (initialization and ep0). So we have to think about this a
 bit.

 I think dwc3 can make similar simplification on the
 __dwc3_gadget_ep_enable().
>>>
>>> __dwc3_gadget_ep_enable() takes the actual descriptors as arguments:
>>>
>>> static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep,
>>> const struct usb_endpoint_descriptor *desc,
>>> const struct usb_ss_ep_comp_descriptor *comp_desc,
>>> bool modify, bool restore)
>>>
>>> I fail to see how much simpler we can make this. Perhaps we can turn
>>> bool and restore into a single argument if we use a bitfield instead of
>>> a bool.
>>>
>>
>> Hi Felipe,
>>
>> I mean that there shouldn't be any need to pass in descriptors with
>> usb_ep since the ones in usb_ep should be set properly from ep enable
>> to ep disable.
>>
>> I think that's the case anyways.
> 
> __dwc3_gadget_ep_enable() is the one assigning the descriptor to the ep:
> 
> static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep,
>   const struct usb_endpoint_descriptor *desc,
>   const struct usb_ss_ep_comp_descriptor *comp_desc,
>   bool modify, bool restore)
> {
>   struct dwc3 *dwc = dep->dwc;
>   u32 reg;
>   int ret;
> 
>   if (!(dep->flags & DWC3_EP_ENABLED)) {
>   ret = dwc3_gadget_start_config(dwc, dep);
>   if (ret)
>   return ret;
>   }
> 
>   ret = dwc3_gadget_set_ep_config(dwc, dep, desc, comp_desc, modify,
>   restore);
>   if (ret)
>   return ret;
> 
>   if (!(dep->flags & DWC3_EP_ENABLED)) {
>   struct dwc3_trb *trb_st_hw;
>   struct dwc3_trb *trb_link;
> 
>   dep->endpoint.desc = desc;
>   dep->comp_desc = comp_desc;
> 
> [...]
> 

Right, but that's redundant for non-ep0 case. And the EP0 case can be
handled globally elsewhere.

The usb_ep_enable() API function takes only the usb_ep and requires
that the descriptors are set in the usb_ep beforehand. So 'desc'
argument in the callback function is probably not required either.

I think this is correct to make it consistent. Since we don't pass the
SS comp descriptor, and we don't want to keep passing more descriptors
every time a new standard descriptor is added, such as the SSP ISO EP
comp descriptor for 3.1.

See below for a proposed change to see what I mean (not tested).

Regards,
John


>8

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 2322863..b9903c6 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -539,7 +539,6 @@ struct dwc3_ep {
 
struct dwc3_trb *trb_pool;
dma_addr_t  trb_pool_dma;
-   const struct usb_ss_ep_comp_descriptor *comp_desc;
struct dwc3 *dwc;
 
u32 saved_state;
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index e47cba5..0fc55e89 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -576,13 +576,12 @@ static int dwc3_gadget_set_xfer_resource(struct dwc3 
*dwc, struct dwc3_ep *dep)
  * Caller should take care of locking
  */
 static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep,
-   const struct usb_endpoint_descriptor *desc,
-   const struct usb_ss_ep_comp_descriptor *comp_desc,
bool modify, bool restore)
 {
struct dwc3 *dwc = dep->dwc;
u32 reg;
int ret;
+   struct usb_ep   *ep;
 
if 

Re: [PATCH] usb: dwc2: gadget: Update for new usb_endpoint_maxp()

[Felipe Balbi]

Hi,

John Youn  writes:
>> John Youn  writes:
> @@ -1812,17 +1812,17 @@ static u32 dwc2_hsotg_ep0_mps(unsigned int mps)
>   * @hsotg: The driver state.
>   * @ep: The index number of the endpoint
>   * @mps: The maximum packet size in bytes
> + * @mc: The multicount value
>   *
>   * Configure the maximum packet size for the given endpoint, updating
>   * the hardware control registers to reflect this.
>   */
>  static void dwc2_hsotg_set_ep_maxpacket(struct dwc2_hsotg *hsotg,
> - unsigned int ep, unsigned int mps, unsigned int dir_in)
> + unsigned int ep, unsigned int mps,
> + unsigned int mc, unsigned int dir_in)

 this has an odd set of arguments. You pass the ep index, mps, direction
 and mult value, when you could just pass hsotg_ep and descriptor instead.
>>>
>>> Yes looks like we can do some simplification here. And you probably
>>> don't need to pass a descriptor either since it must be set in the
>>> usb_ep before enable.
>>>
>>> However this is also called in some contexts where a descriptor is not
>>> available (initialization and ep0). So we have to think about this a
>>> bit.
>>>
>>> I think dwc3 can make similar simplification on the
>>> __dwc3_gadget_ep_enable().
>> 
>> __dwc3_gadget_ep_enable() takes the actual descriptors as arguments:
>> 
>> static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep,
>>  const struct usb_endpoint_descriptor *desc,
>>  const struct usb_ss_ep_comp_descriptor *comp_desc,
>>  bool modify, bool restore)
>> 
>> I fail to see how much simpler we can make this. Perhaps we can turn
>> bool and restore into a single argument if we use a bitfield instead of
>> a bool.
>> 
>
> Hi Felipe,
>
> I mean that there shouldn't be any need to pass in descriptors with
> usb_ep since the ones in usb_ep should be set properly from ep enable
> to ep disable.
>
> I think that's the case anyways.

__dwc3_gadget_ep_enable() is the one assigning the descriptor to the ep:

static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep,
const struct usb_endpoint_descriptor *desc,
const struct usb_ss_ep_comp_descriptor *comp_desc,
bool modify, bool restore)
{
struct dwc3 *dwc = dep->dwc;
u32 reg;
int ret;

if (!(dep->flags & DWC3_EP_ENABLED)) {
ret = dwc3_gadget_start_config(dwc, dep);
if (ret)
return ret;
}

ret = dwc3_gadget_set_ep_config(dwc, dep, desc, comp_desc, modify,
restore);
if (ret)
return ret;

if (!(dep->flags & DWC3_EP_ENABLED)) {
struct dwc3_trb *trb_st_hw;
struct dwc3_trb *trb_link;

dep->endpoint.desc = desc;
dep->comp_desc = comp_desc;

[...]

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc2: gadget: Update for new usb_endpoint_maxp()

[John Youn]

On 11/6/2016 11:32 PM, Felipe Balbi wrote:
> 
> Hi,
> 
> John Youn  writes:
 @@ -1812,17 +1812,17 @@ static u32 dwc2_hsotg_ep0_mps(unsigned int mps)
   * @hsotg: The driver state.
   * @ep: The index number of the endpoint
   * @mps: The maximum packet size in bytes
 + * @mc: The multicount value
   *
   * Configure the maximum packet size for the given endpoint, updating
   * the hardware control registers to reflect this.
   */
  static void dwc2_hsotg_set_ep_maxpacket(struct dwc2_hsotg *hsotg,
 -  unsigned int ep, unsigned int mps, unsigned int dir_in)
 +  unsigned int ep, unsigned int mps,
 +  unsigned int mc, unsigned int dir_in)
>>>
>>> this has an odd set of arguments. You pass the ep index, mps, direction
>>> and mult value, when you could just pass hsotg_ep and descriptor instead.
>>
>> Yes looks like we can do some simplification here. And you probably
>> don't need to pass a descriptor either since it must be set in the
>> usb_ep before enable.
>>
>> However this is also called in some contexts where a descriptor is not
>> available (initialization and ep0). So we have to think about this a
>> bit.
>>
>> I think dwc3 can make similar simplification on the
>> __dwc3_gadget_ep_enable().
> 
> __dwc3_gadget_ep_enable() takes the actual descriptors as arguments:
> 
> static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep,
>   const struct usb_endpoint_descriptor *desc,
>   const struct usb_ss_ep_comp_descriptor *comp_desc,
>   bool modify, bool restore)
> 
> I fail to see how much simpler we can make this. Perhaps we can turn
> bool and restore into a single argument if we use a bitfield instead of
> a bool.
> 

Hi Felipe,

I mean that there shouldn't be any need to pass in descriptors with
usb_ep since the ones in usb_ep should be set properly from ep enable
to ep disable.

I think that's the case anyways.

Regards,
John
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: dwc2: gadget: Update for new usb_endpoint_maxp()

[Felipe Balbi]

Hi,

John Youn  writes:
>>> @@ -1812,17 +1812,17 @@ static u32 dwc2_hsotg_ep0_mps(unsigned int mps)
>>>   * @hsotg: The driver state.
>>>   * @ep: The index number of the endpoint
>>>   * @mps: The maximum packet size in bytes
>>> + * @mc: The multicount value
>>>   *
>>>   * Configure the maximum packet size for the given endpoint, updating
>>>   * the hardware control registers to reflect this.
>>>   */
>>>  static void dwc2_hsotg_set_ep_maxpacket(struct dwc2_hsotg *hsotg,
>>> -   unsigned int ep, unsigned int mps, unsigned int dir_in)
>>> +   unsigned int ep, unsigned int mps,
>>> +   unsigned int mc, unsigned int dir_in)
>> 
>> this has an odd set of arguments. You pass the ep index, mps, direction
>> and mult value, when you could just pass hsotg_ep and descriptor instead.
>
> Yes looks like we can do some simplification here. And you probably
> don't need to pass a descriptor either since it must be set in the
> usb_ep before enable.
>
> However this is also called in some contexts where a descriptor is not
> available (initialization and ep0). So we have to think about this a
> bit.
>
> I think dwc3 can make similar simplification on the
> __dwc3_gadget_ep_enable().

__dwc3_gadget_ep_enable() takes the actual descriptors as arguments:

static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep,
const struct usb_endpoint_descriptor *desc,
const struct usb_ss_ep_comp_descriptor *comp_desc,
bool modify, bool restore)

I fail to see how much simpler we can make this. Perhaps we can turn
bool and restore into a single argument if we use a bitfield instead of
a bool.

-- 
balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: dwc2: gadget: Update for new usb_endpoint_maxp()

[John Youn]

On 11/1/2016 4:14 AM, Felipe Balbi wrote:
> 
> Hi,
> 
> John Youn  writes:
>> From: Vardan Mikayelyan 
>>
>> Update the dwc2 driver for the new behavior of the usb_endpoint_maxp()
>> and also use the new usb_endpoint_maxp_mult() helper function.
>>
>> This commit fixes failures in high-badwith ISOC transfer tests.
>>
>> Signed-off-by: Vardan Mikayelyan 
>> Signed-off-by: John Youn 
>> ---
>>  drivers/usb/dwc2/gadget.c | 38 --
>>  1 file changed, 20 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
>> index 8a7fd73..a505bbf 100644
>> --- a/drivers/usb/dwc2/gadget.c
>> +++ b/drivers/usb/dwc2/gadget.c
>> @@ -1812,17 +1812,17 @@ static u32 dwc2_hsotg_ep0_mps(unsigned int mps)
>>   * @hsotg: The driver state.
>>   * @ep: The index number of the endpoint
>>   * @mps: The maximum packet size in bytes
>> + * @mc: The multicount value
>>   *
>>   * Configure the maximum packet size for the given endpoint, updating
>>   * the hardware control registers to reflect this.
>>   */
>>  static void dwc2_hsotg_set_ep_maxpacket(struct dwc2_hsotg *hsotg,
>> -unsigned int ep, unsigned int mps, unsigned int dir_in)
>> +unsigned int ep, unsigned int mps,
>> +unsigned int mc, unsigned int dir_in)
> 
> this has an odd set of arguments. You pass the ep index, mps, direction
> and mult value, when you could just pass hsotg_ep and descriptor instead.
> 
> Anyway, this is in my testing/next now.
> 

Hi Felipe,

There's a problem with this patch. Can you drop this from your
testing/next.

We'll send you a fixed version next week.

Regards,
John
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: dwc2: gadget: Update for new usb_endpoint_maxp()

[John Youn]

On 11/1/2016 4:14 AM, Felipe Balbi wrote:
> 
> Hi,
> 
> John Youn  writes:
>> From: Vardan Mikayelyan 
>>
>> Update the dwc2 driver for the new behavior of the usb_endpoint_maxp()
>> and also use the new usb_endpoint_maxp_mult() helper function.
>>
>> This commit fixes failures in high-badwith ISOC transfer tests.
>>
>> Signed-off-by: Vardan Mikayelyan 
>> Signed-off-by: John Youn 
>> ---
>>  drivers/usb/dwc2/gadget.c | 38 --
>>  1 file changed, 20 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
>> index 8a7fd73..a505bbf 100644
>> --- a/drivers/usb/dwc2/gadget.c
>> +++ b/drivers/usb/dwc2/gadget.c
>> @@ -1812,17 +1812,17 @@ static u32 dwc2_hsotg_ep0_mps(unsigned int mps)
>>   * @hsotg: The driver state.
>>   * @ep: The index number of the endpoint
>>   * @mps: The maximum packet size in bytes
>> + * @mc: The multicount value
>>   *
>>   * Configure the maximum packet size for the given endpoint, updating
>>   * the hardware control registers to reflect this.
>>   */
>>  static void dwc2_hsotg_set_ep_maxpacket(struct dwc2_hsotg *hsotg,
>> -unsigned int ep, unsigned int mps, unsigned int dir_in)
>> +unsigned int ep, unsigned int mps,
>> +unsigned int mc, unsigned int dir_in)
> 
> this has an odd set of arguments. You pass the ep index, mps, direction
> and mult value, when you could just pass hsotg_ep and descriptor instead.

Yes looks like we can do some simplification here. And you probably
don't need to pass a descriptor either since it must be set in the
usb_ep before enable.

However this is also called in some contexts where a descriptor is not
available (initialization and ep0). So we have to think about this a
bit.

I think dwc3 can make similar simplification on the
__dwc3_gadget_ep_enable().

> 
> Anyway, this is in my testing/next now.
> 

Ok thanks.

Regards,
John


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: dwc2: gadget: Update for new usb_endpoint_maxp()

[Felipe Balbi]

Hi,

John Youn  writes:
> From: Vardan Mikayelyan 
>
> Update the dwc2 driver for the new behavior of the usb_endpoint_maxp()
> and also use the new usb_endpoint_maxp_mult() helper function.
>
> This commit fixes failures in high-badwith ISOC transfer tests.
>
> Signed-off-by: Vardan Mikayelyan 
> Signed-off-by: John Youn 
> ---
>  drivers/usb/dwc2/gadget.c | 38 --
>  1 file changed, 20 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 8a7fd73..a505bbf 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -1812,17 +1812,17 @@ static u32 dwc2_hsotg_ep0_mps(unsigned int mps)
>   * @hsotg: The driver state.
>   * @ep: The index number of the endpoint
>   * @mps: The maximum packet size in bytes
> + * @mc: The multicount value
>   *
>   * Configure the maximum packet size for the given endpoint, updating
>   * the hardware control registers to reflect this.
>   */
>  static void dwc2_hsotg_set_ep_maxpacket(struct dwc2_hsotg *hsotg,
> - unsigned int ep, unsigned int mps, unsigned int dir_in)
> + unsigned int ep, unsigned int mps,
> + unsigned int mc, unsigned int dir_in)

this has an odd set of arguments. You pass the ep index, mps, direction
and mult value, when you could just pass hsotg_ep and descriptor instead.

Anyway, this is in my testing/next now.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc2: gadget: Update for new usb_endpoint_maxp()

[kbuild test robot]

Hi Vardan,

[auto build test ERROR on balbi-usb/next]
[also build test ERROR on v4.9-rc3 next-20161028]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]
[Suggest to use git(>=2.9.0) format-patch --base= (or --base=auto for 
convenience) to record what (public, well-known) commit your patch series was 
built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:
https://github.com/0day-ci/linux/commits/John-Youn/usb-dwc2-gadget-Update-for-new-usb_endpoint_maxp/20161101-052727
base:   https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git next
config: x86_64-randconfig-x014-201644 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/usb/dwc2/gadget.c: In function 'dwc2_hsotg_ep_enable':
>> drivers/usb/dwc2/gadget.c:2979:7: error: implicit declaration of function 
>> 'usb_endpoint_maxp_mult' [-Werror=implicit-function-declaration]
 mc = usb_endpoint_maxp_mult(desc);
  ^~
   cc1: some warnings being treated as errors

vim +/usb_endpoint_maxp_mult +2979 drivers/usb/dwc2/gadget.c

  2973  if (dir_in != hs_ep->dir_in) {
  2974  dev_err(hsotg->dev, "%s: direction mismatch!\n", 
__func__);
  2975  return -EINVAL;
  2976  }
  2977  
  2978  mps = usb_endpoint_maxp(desc);
> 2979  mc = usb_endpoint_maxp_mult(desc);
  2980  
  2981  /* note, we handle this here instead of 
dwc2_hsotg_set_ep_maxpacket */
  2982  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

[PATCH] usb: dwc2: gadget: Update for new usb_endpoint_maxp()

[John Youn]

From: Vardan Mikayelyan 

Update the dwc2 driver for the new behavior of the usb_endpoint_maxp()
and also use the new usb_endpoint_maxp_mult() helper function.

This commit fixes failures in high-badwith ISOC transfer tests.

Signed-off-by: Vardan Mikayelyan 
Signed-off-by: John Youn 
---
 drivers/usb/dwc2/gadget.c | 38 --
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 8a7fd73..a505bbf 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -1812,17 +1812,17 @@ static u32 dwc2_hsotg_ep0_mps(unsigned int mps)
  * @hsotg: The driver state.
  * @ep: The index number of the endpoint
  * @mps: The maximum packet size in bytes
+ * @mc: The multicount value
  *
  * Configure the maximum packet size for the given endpoint, updating
  * the hardware control registers to reflect this.
  */
 static void dwc2_hsotg_set_ep_maxpacket(struct dwc2_hsotg *hsotg,
-   unsigned int ep, unsigned int mps, unsigned int dir_in)
+   unsigned int ep, unsigned int mps,
+   unsigned int mc, unsigned int dir_in)
 {
struct dwc2_hsotg_ep *hs_ep;
void __iomem *regs = hsotg->regs;
-   u32 mpsval;
-   u32 mcval;
u32 reg;
 
hs_ep = index_to_ep(hsotg, ep, dir_in);
@@ -1831,31 +1831,29 @@ static void dwc2_hsotg_set_ep_maxpacket(struct 
dwc2_hsotg *hsotg,
 
if (ep == 0) {
/* EP0 is a special case */
-   mpsval = dwc2_hsotg_ep0_mps(mps);
-   if (mpsval > 3)
+   mps = dwc2_hsotg_ep0_mps(mps);
+   if (mps > 3)
goto bad_mps;
hs_ep->ep.maxpacket = mps;
hs_ep->mc = 1;
} else {
-   mpsval = mps & DXEPCTL_MPS_MASK;
-   if (mpsval > 1024)
+   if (mps > 1024)
goto bad_mps;
-   mcval = ((mps >> 11) & 0x3) + 1;
-   hs_ep->mc = mcval;
-   if (mcval > 3)
+   hs_ep->mc = mc;
+   if (mc > 3)
goto bad_mps;
-   hs_ep->ep.maxpacket = mpsval;
+   hs_ep->ep.maxpacket = mps;
}
 
if (dir_in) {
reg = dwc2_readl(regs + DIEPCTL(ep));
reg &= ~DXEPCTL_MPS_MASK;
-   reg |= mpsval;
+   reg |= mps;
dwc2_writel(reg, regs + DIEPCTL(ep));
} else {
reg = dwc2_readl(regs + DOEPCTL(ep));
reg &= ~DXEPCTL_MPS_MASK;
-   reg |= mpsval;
+   reg |= mps;
dwc2_writel(reg, regs + DOEPCTL(ep));
}
 
@@ -2390,13 +2388,15 @@ static void dwc2_hsotg_irq_enumdone(struct dwc2_hsotg 
*hsotg)
if (ep0_mps) {
int i;
/* Initialize ep0 for both in and out directions */
-   dwc2_hsotg_set_ep_maxpacket(hsotg, 0, ep0_mps, 1);
-   dwc2_hsotg_set_ep_maxpacket(hsotg, 0, ep0_mps, 0);
+   dwc2_hsotg_set_ep_maxpacket(hsotg, 0, ep0_mps, 0, 1);
+   dwc2_hsotg_set_ep_maxpacket(hsotg, 0, ep0_mps, 0, 0);
for (i = 1; i < hsotg->num_of_eps; i++) {
if (hsotg->eps_in[i])
-   dwc2_hsotg_set_ep_maxpacket(hsotg, i, ep_mps, 
1);
+   dwc2_hsotg_set_ep_maxpacket(hsotg, i, ep_mps,
+   0, 1);
if (hsotg->eps_out[i])
-   dwc2_hsotg_set_ep_maxpacket(hsotg, i, ep_mps, 
0);
+   dwc2_hsotg_set_ep_maxpacket(hsotg, i, ep_mps,
+   0, 0);
}
}
 
@@ -2952,6 +2952,7 @@ static int dwc2_hsotg_ep_enable(struct usb_ep *ep,
u32 epctrl_reg;
u32 epctrl;
u32 mps;
+   u32 mc;
u32 mask;
unsigned int dir_in;
unsigned int i, val, size;
@@ -2975,6 +2976,7 @@ static int dwc2_hsotg_ep_enable(struct usb_ep *ep,
}
 
mps = usb_endpoint_maxp(desc);
+   mc = usb_endpoint_maxp_mult(desc);
 
/* note, we handle this here instead of dwc2_hsotg_set_ep_maxpacket */
 
@@ -2996,7 +2998,7 @@ static int dwc2_hsotg_ep_enable(struct usb_ep *ep,
epctrl |= DXEPCTL_USBACTEP;
 
/* update the endpoint state */
-   dwc2_hsotg_set_ep_maxpacket(hsotg, hs_ep->index, mps, dir_in);
+   dwc2_hsotg_set_ep_maxpacket(hsotg, hs_ep->index, mps, mc, dir_in);
 
/* default, set to non-periodic */
hs_ep->isochronous = 0;
-- 
2.10.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at