Re: [PATCH 3/3] usb: gadget: default U1/U2 exit latencies to maximum

[Felipe Balbi]

Hi,

John Youn  writes:

[...]

 module parameters also feel like a big, big hammer to hit a tiny nail
 head. I don't want to add any module parameters for stuff like this. And
 since you've been pushing for them for a while, it only shows that
 you're only concerned about your use case ;-)
>>>
>>> Maybe so, but module params are the easiest, workable solution. It
>> 
>> It might be easy to implement but it becomes a pain as time
>> passes. I can give you a concrete example (using device properties to
>> illustrate):
>> 
>> We introduced way back a property for platform code to tell us that
>> $this dwc3 instance needed the driver to resize FIFOs. The only reason
>> for this was because OMAP5 ES1.0 had default FIFO sizes which were less
>> than a full bulk USB 3.0 packet (< 1024 bytes) so we couldn't receive
>> any USB 3.0 packets.
>> 
>> Documentation was clear that this property was only needed if default
>> FIFO sizes were bad and yet several bindings started using it
>> blindly. Granted, this is one occasion where it really didn't cause
>> problems to resize FIFO, but now imagine what happens when we introduce
>> several module parameters and people start using it without really
>> knowing what they're for. We will start getting "bug reports" because
>> someone passed a e.g. "number_of_endpoints=0" parameter to dwc3 and the
>> driver didn't allocate any EP structure, or something silly like that.
>
> Sure, that's a problem. But you have the same issue with DT bindings
> and users setting the wrong things there. I've also had these issues
> for dwc2.

fair enough :-)

> And there are potentially several things we may want to control that
> should have no exposure to the user... but we can discuss that when we
> get to it :)

oh oh

>>> doesn't affect any other modules other than dwc3-pci, and they will
>>> only touch certain already-defined DT bindings. So in terms of
>>> maintainability, the code is all in one place, in one module, and it
>>> should be stable since the bindings are already defined as part of the
>>> ABI of dwc3.ko.
>> 
>> dwc3-pci is also used by non-FPGA platforms :-) I really don't want to
>> introduce module parameters and I really think debugfs can be used here
>> for your case. Just expose all parameters you want to dwc3-pci's debugfs
>> (needs to be created) and blacklist dwc3.ko so it doesn't load
>> automatically.
>> 
>> Then, load dwc3-pci, mount debugfs, set all parameters you need and
>> manually modprobe dwc3.ko. No module parameters, debugfs is usually not
>> shipping in products, and we can still wrap dwc3-pci's debugfs creation
>> with a #ifdef DWC3_FPGA_PROTOTYPE_EXTRAS or something along those lines.
>
> Ok that should work.
>
> Do you think it is worth it to create a glue driver just for HAPS with
> those settings adjustable only there?

it's probably a good idea, then there's even less probability of this
leaking into real systems.

-- 
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 3/3] usb: gadget: default U1/U2 exit latencies to maximum

[John Youn]

On 9/16/2016 12:55 AM, Felipe Balbi wrote:
> 
> Hi John,
> 
> John Youn  writes:
>>> John Youn  writes:
>> John Youn  writes:
>>> On 8/31/2016 2:38 AM, Felipe Balbi wrote:
 If we don't know what are the actual U1/U2 exit
 latencies from the UDC, we're better off using
 maximum latencies as default. This should avoid
 any problems with too frequent U1/U2 entry.

 Signed-off-by: Felipe Balbi 
 ---
  include/linux/usb/gadget.h | 12 ++--
  1 file changed, 10 insertions(+), 2 deletions(-)

 diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
 index 3667d667cab1..20cb590c658e 100644
 --- a/include/linux/usb/gadget.h
 +++ b/include/linux/usb/gadget.h
 @@ -276,11 +276,19 @@ static inline void usb_ep_fifo_flush(struct 
 usb_ep *ep)
  
  
 /*-*/
  
 +/**
 + * struct usb_dcd_config_params - Default U1/U2 exit latencies
 + * @bU1DevExitLat: U1 Exit Latency in us
 + * @bU2DevExitLat: U2 Exit Latency in us
 + *
 + * Note that we will be setting U1/U2 exit latencies to their maximum
 + * by default if no value is passed by the UDC Driver.
 + */
  struct usb_dcd_config_params {
__u8  bU1DevExitLat;/* U1 Device exit Latency */
 -#define USB_DEFAULT_U1_DEV_EXIT_LAT   0x01/* Less then 1 microsec 
 */
 +#define USB_DEFAULT_U1_DEV_EXIT_LAT   10 /* us */
__le16 wU2DevExitLat;   /* U2 Device exit Latency */
 -#define USB_DEFAULT_U2_DEV_EXIT_LAT   0x1F4   /* Less then 500 
 microsec */
 +#define USB_DEFAULT_U2_DEV_EXIT_LAT   2047 /* us */
  };
  
  

>>>
>>> Hi Felipe,
>>>
>>> Speaking of this, how would you feel about adding module parameters in
>>> the dwc3-pci to set these? And we also have several more settings in
>>> our internal tree that we need for IP validation and debugging.
>>>
>>> As you know the IP is highly configurable, and we do much of the
>>> testing against these configurations via software settings. And our
>>> validation platform is not a typical platform. Basically we have a
>>> single platform, but the instantiated IP and PHY could be configured
>>> in any way so it could need different values passed in for
>>> testing. Like the U1/U2 exit latencies, LPM NYET threshold, HIRD,
>>> USB2/3 SUSPHY, LPM sleep mode, GFLADJ, etc.
>>>
>>> And some things that are automatically detected we need to restrict or
>>> disable to get full coverage. Such as disabling U1/U2 and LPM, maximum
>>> speed, dr_mode, NUMP, RX thresholding, RX thresholding packet count.
>>>
>>> I know module parameters are typically frowned upon so do you
>>> recommend another approach?
>>
>> I completely understand the situation. Module parameters are, well,
>> rather unsophisticated. FPGA validation is, however, a 'peculiar' use
>> case.
>>
>> I want to avoid module parameters as much as possible, but apart from
>> module parameters, the only thing I can think of is a specific
>> sub-directory on debugfs which only gets compile if EXPERT &&
>> DWC3_VALIDATION_MODE or whatever.
>>
>> Would debugfs work for you? The only problem is for things which get
>> discovered during probe()
>
> Yes that's the problem, otherwise I'd be fine with debugfs. Almost
> everything we need is initialized on probe. I thought of maybe
> refactoring the dwc3 code so that this doesn't have to happen on probe
> and we can trigger a "module reset" or something. But that is not
> exactly clean either.
>
> Regards,
> John
>

 Does it seem reasonable to add module params to the PCI driver for
 this use case? At least until/if there is some better solution. We can
 guard it with a config option if necessary.
>>>
>>> module parameters are user-space ABI. Once added, we can't easily
>>> remove. I've been considering if kprobes could be used to change stuff
>>> like this.
>>>
>>> module parameters also feel like a big, big hammer to hit a tiny nail
>>> head. I don't want to add any module parameters for stuff like this. And
>>> since you've been pushing for them for a while, it only shows that
>>> you're only concerned about your use case ;-)
>>
>> Maybe so, but module params are the easiest, workable solution. It
> 
> It might be easy to implement but it becomes a pain as time
> passes. I can give you a concrete example (using device properties to
> illustrate):
> 
> We introduced way back a property for platform code to tell us that
> $this 

Re: [PATCH 3/3] usb: gadget: default U1/U2 exit latencies to maximum

[Felipe Balbi]

Hi John,

John Youn  writes:
>> John Youn  writes:
> John Youn  writes:
>> On 8/31/2016 2:38 AM, Felipe Balbi wrote:
>>> If we don't know what are the actual U1/U2 exit
>>> latencies from the UDC, we're better off using
>>> maximum latencies as default. This should avoid
>>> any problems with too frequent U1/U2 entry.
>>>
>>> Signed-off-by: Felipe Balbi 
>>> ---
>>>  include/linux/usb/gadget.h | 12 ++--
>>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>>> index 3667d667cab1..20cb590c658e 100644
>>> --- a/include/linux/usb/gadget.h
>>> +++ b/include/linux/usb/gadget.h
>>> @@ -276,11 +276,19 @@ static inline void usb_ep_fifo_flush(struct 
>>> usb_ep *ep)
>>>  
>>>  
>>> /*-*/
>>>  
>>> +/**
>>> + * struct usb_dcd_config_params - Default U1/U2 exit latencies
>>> + * @bU1DevExitLat: U1 Exit Latency in us
>>> + * @bU2DevExitLat: U2 Exit Latency in us
>>> + *
>>> + * Note that we will be setting U1/U2 exit latencies to their maximum
>>> + * by default if no value is passed by the UDC Driver.
>>> + */
>>>  struct usb_dcd_config_params {
>>> __u8  bU1DevExitLat;/* U1 Device exit Latency */
>>> -#define USB_DEFAULT_U1_DEV_EXIT_LAT0x01/* Less then 1 microsec 
>>> */
>>> +#define USB_DEFAULT_U1_DEV_EXIT_LAT10 /* us */
>>> __le16 wU2DevExitLat;   /* U2 Device exit Latency */
>>> -#define USB_DEFAULT_U2_DEV_EXIT_LAT0x1F4   /* Less then 500 
>>> microsec */
>>> +#define USB_DEFAULT_U2_DEV_EXIT_LAT2047 /* us */
>>>  };
>>>  
>>>  
>>>
>>
>> Hi Felipe,
>>
>> Speaking of this, how would you feel about adding module parameters in
>> the dwc3-pci to set these? And we also have several more settings in
>> our internal tree that we need for IP validation and debugging.
>>
>> As you know the IP is highly configurable, and we do much of the
>> testing against these configurations via software settings. And our
>> validation platform is not a typical platform. Basically we have a
>> single platform, but the instantiated IP and PHY could be configured
>> in any way so it could need different values passed in for
>> testing. Like the U1/U2 exit latencies, LPM NYET threshold, HIRD,
>> USB2/3 SUSPHY, LPM sleep mode, GFLADJ, etc.
>>
>> And some things that are automatically detected we need to restrict or
>> disable to get full coverage. Such as disabling U1/U2 and LPM, maximum
>> speed, dr_mode, NUMP, RX thresholding, RX thresholding packet count.
>>
>> I know module parameters are typically frowned upon so do you
>> recommend another approach?
>
> I completely understand the situation. Module parameters are, well,
> rather unsophisticated. FPGA validation is, however, a 'peculiar' use
> case.
>
> I want to avoid module parameters as much as possible, but apart from
> module parameters, the only thing I can think of is a specific
> sub-directory on debugfs which only gets compile if EXPERT &&
> DWC3_VALIDATION_MODE or whatever.
>
> Would debugfs work for you? The only problem is for things which get
> discovered during probe()

 Yes that's the problem, otherwise I'd be fine with debugfs. Almost
 everything we need is initialized on probe. I thought of maybe
 refactoring the dwc3 code so that this doesn't have to happen on probe
 and we can trigger a "module reset" or something. But that is not
 exactly clean either.

 Regards,
 John

>>>
>>> Does it seem reasonable to add module params to the PCI driver for
>>> this use case? At least until/if there is some better solution. We can
>>> guard it with a config option if necessary.
>> 
>> module parameters are user-space ABI. Once added, we can't easily
>> remove. I've been considering if kprobes could be used to change stuff
>> like this.
>> 
>> module parameters also feel like a big, big hammer to hit a tiny nail
>> head. I don't want to add any module parameters for stuff like this. And
>> since you've been pushing for them for a while, it only shows that
>> you're only concerned about your use case ;-)
>
> Maybe so, but module params are the easiest, workable solution. It

It might be easy to implement but it becomes a pain as time
passes. I can give you a concrete example (using device properties to
illustrate):

We introduced way back a property for platform code to tell us that
$this dwc3 instance needed the driver to resize FIFOs. The only reason
for this was because OMAP5 ES1.0 had default FIFO sizes which were less
than a full bulk USB 3.0 

Re: [PATCH 3/3] usb: gadget: default U1/U2 exit latencies to maximum

[John Youn]

On 9/14/2016 11:53 PM, Felipe Balbi wrote:
> 
> Hi,
> 
> John Youn  writes:
 John Youn  writes:
> On 8/31/2016 2:38 AM, Felipe Balbi wrote:
>> If we don't know what are the actual U1/U2 exit
>> latencies from the UDC, we're better off using
>> maximum latencies as default. This should avoid
>> any problems with too frequent U1/U2 entry.
>>
>> Signed-off-by: Felipe Balbi 
>> ---
>>  include/linux/usb/gadget.h | 12 ++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>> index 3667d667cab1..20cb590c658e 100644
>> --- a/include/linux/usb/gadget.h
>> +++ b/include/linux/usb/gadget.h
>> @@ -276,11 +276,19 @@ static inline void usb_ep_fifo_flush(struct usb_ep 
>> *ep)
>>  
>>  
>> /*-*/
>>  
>> +/**
>> + * struct usb_dcd_config_params - Default U1/U2 exit latencies
>> + * @bU1DevExitLat: U1 Exit Latency in us
>> + * @bU2DevExitLat: U2 Exit Latency in us
>> + *
>> + * Note that we will be setting U1/U2 exit latencies to their maximum
>> + * by default if no value is passed by the UDC Driver.
>> + */
>>  struct usb_dcd_config_params {
>>  __u8  bU1DevExitLat;/* U1 Device exit Latency */
>> -#define USB_DEFAULT_U1_DEV_EXIT_LAT 0x01/* Less then 1 microsec 
>> */
>> +#define USB_DEFAULT_U1_DEV_EXIT_LAT 10 /* us */
>>  __le16 wU2DevExitLat;   /* U2 Device exit Latency */
>> -#define USB_DEFAULT_U2_DEV_EXIT_LAT 0x1F4   /* Less then 500 
>> microsec */
>> +#define USB_DEFAULT_U2_DEV_EXIT_LAT 2047 /* us */
>>  };
>>  
>>  
>>
>
> Hi Felipe,
>
> Speaking of this, how would you feel about adding module parameters in
> the dwc3-pci to set these? And we also have several more settings in
> our internal tree that we need for IP validation and debugging.
>
> As you know the IP is highly configurable, and we do much of the
> testing against these configurations via software settings. And our
> validation platform is not a typical platform. Basically we have a
> single platform, but the instantiated IP and PHY could be configured
> in any way so it could need different values passed in for
> testing. Like the U1/U2 exit latencies, LPM NYET threshold, HIRD,
> USB2/3 SUSPHY, LPM sleep mode, GFLADJ, etc.
>
> And some things that are automatically detected we need to restrict or
> disable to get full coverage. Such as disabling U1/U2 and LPM, maximum
> speed, dr_mode, NUMP, RX thresholding, RX thresholding packet count.
>
> I know module parameters are typically frowned upon so do you
> recommend another approach?

 I completely understand the situation. Module parameters are, well,
 rather unsophisticated. FPGA validation is, however, a 'peculiar' use
 case.

 I want to avoid module parameters as much as possible, but apart from
 module parameters, the only thing I can think of is a specific
 sub-directory on debugfs which only gets compile if EXPERT &&
 DWC3_VALIDATION_MODE or whatever.

 Would debugfs work for you? The only problem is for things which get
 discovered during probe()
>>>
>>> Yes that's the problem, otherwise I'd be fine with debugfs. Almost
>>> everything we need is initialized on probe. I thought of maybe
>>> refactoring the dwc3 code so that this doesn't have to happen on probe
>>> and we can trigger a "module reset" or something. But that is not
>>> exactly clean either.
>>>
>>> Regards,
>>> John
>>>
>>
>> Does it seem reasonable to add module params to the PCI driver for
>> this use case? At least until/if there is some better solution. We can
>> guard it with a config option if necessary.
> 
> module parameters are user-space ABI. Once added, we can't easily
> remove. I've been considering if kprobes could be used to change stuff
> like this.
> 
> module parameters also feel like a big, big hammer to hit a tiny nail
> head. I don't want to add any module parameters for stuff like this. And
> since you've been pushing for them for a while, it only shows that
> you're only concerned about your use case ;-)

Maybe so, but module params are the easiest, workable solution. It
doesn't affect any other modules other than dwc3-pci, and they will
only touch certain already-defined DT bindings. So in terms of
maintainability, the code is all in one place, in one module, and it
should be stable since the bindings are already defined as part of the
ABI of dwc3.ko.

> 
> Remember that module parameters are 'global', every instance of
> dwc3-pci.ko/dwc3.ko will behave with the same set of module parameters.
> 
> Sorry, but I have to say 'no' again. 

Re: [PATCH 3/3] usb: gadget: default U1/U2 exit latencies to maximum

[Felipe Balbi]

Hi,

John Youn  writes:
>>> John Youn  writes:
 On 8/31/2016 2:38 AM, Felipe Balbi wrote:
> If we don't know what are the actual U1/U2 exit
> latencies from the UDC, we're better off using
> maximum latencies as default. This should avoid
> any problems with too frequent U1/U2 entry.
>
> Signed-off-by: Felipe Balbi 
> ---
>  include/linux/usb/gadget.h | 12 ++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index 3667d667cab1..20cb590c658e 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -276,11 +276,19 @@ static inline void usb_ep_fifo_flush(struct usb_ep 
> *ep)
>  
>  
> /*-*/
>  
> +/**
> + * struct usb_dcd_config_params - Default U1/U2 exit latencies
> + * @bU1DevExitLat: U1 Exit Latency in us
> + * @bU2DevExitLat: U2 Exit Latency in us
> + *
> + * Note that we will be setting U1/U2 exit latencies to their maximum
> + * by default if no value is passed by the UDC Driver.
> + */
>  struct usb_dcd_config_params {
>   __u8  bU1DevExitLat;/* U1 Device exit Latency */
> -#define USB_DEFAULT_U1_DEV_EXIT_LAT  0x01/* Less then 1 microsec 
> */
> +#define USB_DEFAULT_U1_DEV_EXIT_LAT  10 /* us */
>   __le16 wU2DevExitLat;   /* U2 Device exit Latency */
> -#define USB_DEFAULT_U2_DEV_EXIT_LAT  0x1F4   /* Less then 500 
> microsec */
> +#define USB_DEFAULT_U2_DEV_EXIT_LAT  2047 /* us */
>  };
>  
>  
>

 Hi Felipe,

 Speaking of this, how would you feel about adding module parameters in
 the dwc3-pci to set these? And we also have several more settings in
 our internal tree that we need for IP validation and debugging.

 As you know the IP is highly configurable, and we do much of the
 testing against these configurations via software settings. And our
 validation platform is not a typical platform. Basically we have a
 single platform, but the instantiated IP and PHY could be configured
 in any way so it could need different values passed in for
 testing. Like the U1/U2 exit latencies, LPM NYET threshold, HIRD,
 USB2/3 SUSPHY, LPM sleep mode, GFLADJ, etc.

 And some things that are automatically detected we need to restrict or
 disable to get full coverage. Such as disabling U1/U2 and LPM, maximum
 speed, dr_mode, NUMP, RX thresholding, RX thresholding packet count.

 I know module parameters are typically frowned upon so do you
 recommend another approach?
>>>
>>> I completely understand the situation. Module parameters are, well,
>>> rather unsophisticated. FPGA validation is, however, a 'peculiar' use
>>> case.
>>>
>>> I want to avoid module parameters as much as possible, but apart from
>>> module parameters, the only thing I can think of is a specific
>>> sub-directory on debugfs which only gets compile if EXPERT &&
>>> DWC3_VALIDATION_MODE or whatever.
>>>
>>> Would debugfs work for you? The only problem is for things which get
>>> discovered during probe()
>> 
>> Yes that's the problem, otherwise I'd be fine with debugfs. Almost
>> everything we need is initialized on probe. I thought of maybe
>> refactoring the dwc3 code so that this doesn't have to happen on probe
>> and we can trigger a "module reset" or something. But that is not
>> exactly clean either.
>> 
>> Regards,
>> John
>> 
>
> Does it seem reasonable to add module params to the PCI driver for
> this use case? At least until/if there is some better solution. We can
> guard it with a config option if necessary.

module parameters are user-space ABI. Once added, we can't easily
remove. I've been considering if kprobes could be used to change stuff
like this.

module parameters also feel like a big, big hammer to hit a tiny nail
head. I don't want to add any module parameters for stuff like this. And
since you've been pushing for them for a while, it only shows that
you're only concerned about your use case ;-)

Remember that module parameters are 'global', every instance of
dwc3-pci.ko/dwc3.ko will behave with the same set of module parameters.

Sorry, but I have to say 'no' again. module parameters are not an
option; and I'd strongly suggest you don't do it for dwc2 either because
it'll make maintaining the driver a lot more difficult.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 3/3] usb: gadget: default U1/U2 exit latencies to maximum

[John Youn]

On 8/31/2016 1:37 PM, John Youn wrote:
> On 8/31/2016 12:47 PM, Felipe Balbi wrote:
>>
>> Hi John,
>>
>> John Youn  writes:
>>> On 8/31/2016 2:38 AM, Felipe Balbi wrote:
 If we don't know what are the actual U1/U2 exit
 latencies from the UDC, we're better off using
 maximum latencies as default. This should avoid
 any problems with too frequent U1/U2 entry.

 Signed-off-by: Felipe Balbi 
 ---
  include/linux/usb/gadget.h | 12 ++--
  1 file changed, 10 insertions(+), 2 deletions(-)

 diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
 index 3667d667cab1..20cb590c658e 100644
 --- a/include/linux/usb/gadget.h
 +++ b/include/linux/usb/gadget.h
 @@ -276,11 +276,19 @@ static inline void usb_ep_fifo_flush(struct usb_ep 
 *ep)
  
  
 /*-*/
  
 +/**
 + * struct usb_dcd_config_params - Default U1/U2 exit latencies
 + * @bU1DevExitLat: U1 Exit Latency in us
 + * @bU2DevExitLat: U2 Exit Latency in us
 + *
 + * Note that we will be setting U1/U2 exit latencies to their maximum
 + * by default if no value is passed by the UDC Driver.
 + */
  struct usb_dcd_config_params {
__u8  bU1DevExitLat;/* U1 Device exit Latency */
 -#define USB_DEFAULT_U1_DEV_EXIT_LAT   0x01/* Less then 1 microsec 
 */
 +#define USB_DEFAULT_U1_DEV_EXIT_LAT   10 /* us */
__le16 wU2DevExitLat;   /* U2 Device exit Latency */
 -#define USB_DEFAULT_U2_DEV_EXIT_LAT   0x1F4   /* Less then 500 
 microsec */
 +#define USB_DEFAULT_U2_DEV_EXIT_LAT   2047 /* us */
  };
  
  

>>>
>>> Hi Felipe,
>>>
>>> Speaking of this, how would you feel about adding module parameters in
>>> the dwc3-pci to set these? And we also have several more settings in
>>> our internal tree that we need for IP validation and debugging.
>>>
>>> As you know the IP is highly configurable, and we do much of the
>>> testing against these configurations via software settings. And our
>>> validation platform is not a typical platform. Basically we have a
>>> single platform, but the instantiated IP and PHY could be configured
>>> in any way so it could need different values passed in for
>>> testing. Like the U1/U2 exit latencies, LPM NYET threshold, HIRD,
>>> USB2/3 SUSPHY, LPM sleep mode, GFLADJ, etc.
>>>
>>> And some things that are automatically detected we need to restrict or
>>> disable to get full coverage. Such as disabling U1/U2 and LPM, maximum
>>> speed, dr_mode, NUMP, RX thresholding, RX thresholding packet count.
>>>
>>> I know module parameters are typically frowned upon so do you
>>> recommend another approach?
>>
>> I completely understand the situation. Module parameters are, well,
>> rather unsophisticated. FPGA validation is, however, a 'peculiar' use
>> case.
>>
>> I want to avoid module parameters as much as possible, but apart from
>> module parameters, the only thing I can think of is a specific
>> sub-directory on debugfs which only gets compile if EXPERT &&
>> DWC3_VALIDATION_MODE or whatever.
>>
>> Would debugfs work for you? The only problem is for things which get
>> discovered during probe()
> 
> Yes that's the problem, otherwise I'd be fine with debugfs. Almost
> everything we need is initialized on probe. I thought of maybe
> refactoring the dwc3 code so that this doesn't have to happen on probe
> and we can trigger a "module reset" or something. But that is not
> exactly clean either.
> 
> Regards,
> John
> 

Does it seem reasonable to add module params to the PCI driver for
this use case? At least until/if there is some better solution. We can
guard it with a config option if necessary.

We want to do a similar thing with dwc2 as well.

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 3/3] usb: gadget: default U1/U2 exit latencies to maximum

[John Youn]

On 8/31/2016 12:47 PM, Felipe Balbi wrote:
> 
> Hi John,
> 
> John Youn  writes:
>> On 8/31/2016 2:38 AM, Felipe Balbi wrote:
>>> If we don't know what are the actual U1/U2 exit
>>> latencies from the UDC, we're better off using
>>> maximum latencies as default. This should avoid
>>> any problems with too frequent U1/U2 entry.
>>>
>>> Signed-off-by: Felipe Balbi 
>>> ---
>>>  include/linux/usb/gadget.h | 12 ++--
>>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>>> index 3667d667cab1..20cb590c658e 100644
>>> --- a/include/linux/usb/gadget.h
>>> +++ b/include/linux/usb/gadget.h
>>> @@ -276,11 +276,19 @@ static inline void usb_ep_fifo_flush(struct usb_ep 
>>> *ep)
>>>  
>>>  
>>> /*-*/
>>>  
>>> +/**
>>> + * struct usb_dcd_config_params - Default U1/U2 exit latencies
>>> + * @bU1DevExitLat: U1 Exit Latency in us
>>> + * @bU2DevExitLat: U2 Exit Latency in us
>>> + *
>>> + * Note that we will be setting U1/U2 exit latencies to their maximum
>>> + * by default if no value is passed by the UDC Driver.
>>> + */
>>>  struct usb_dcd_config_params {
>>> __u8  bU1DevExitLat;/* U1 Device exit Latency */
>>> -#define USB_DEFAULT_U1_DEV_EXIT_LAT0x01/* Less then 1 microsec 
>>> */
>>> +#define USB_DEFAULT_U1_DEV_EXIT_LAT10 /* us */
>>> __le16 wU2DevExitLat;   /* U2 Device exit Latency */
>>> -#define USB_DEFAULT_U2_DEV_EXIT_LAT0x1F4   /* Less then 500 
>>> microsec */
>>> +#define USB_DEFAULT_U2_DEV_EXIT_LAT2047 /* us */
>>>  };
>>>  
>>>  
>>>
>>
>> Hi Felipe,
>>
>> Speaking of this, how would you feel about adding module parameters in
>> the dwc3-pci to set these? And we also have several more settings in
>> our internal tree that we need for IP validation and debugging.
>>
>> As you know the IP is highly configurable, and we do much of the
>> testing against these configurations via software settings. And our
>> validation platform is not a typical platform. Basically we have a
>> single platform, but the instantiated IP and PHY could be configured
>> in any way so it could need different values passed in for
>> testing. Like the U1/U2 exit latencies, LPM NYET threshold, HIRD,
>> USB2/3 SUSPHY, LPM sleep mode, GFLADJ, etc.
>>
>> And some things that are automatically detected we need to restrict or
>> disable to get full coverage. Such as disabling U1/U2 and LPM, maximum
>> speed, dr_mode, NUMP, RX thresholding, RX thresholding packet count.
>>
>> I know module parameters are typically frowned upon so do you
>> recommend another approach?
> 
> I completely understand the situation. Module parameters are, well,
> rather unsophisticated. FPGA validation is, however, a 'peculiar' use
> case.
> 
> I want to avoid module parameters as much as possible, but apart from
> module parameters, the only thing I can think of is a specific
> sub-directory on debugfs which only gets compile if EXPERT &&
> DWC3_VALIDATION_MODE or whatever.
> 
> Would debugfs work for you? The only problem is for things which get
> discovered during probe()

Yes that's the problem, otherwise I'd be fine with debugfs. Almost
everything we need is initialized on probe. I thought of maybe
refactoring the dwc3 code so that this doesn't have to happen on probe
and we can trigger a "module reset" or something. But that is not
exactly clean either.

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 3/3] usb: gadget: default U1/U2 exit latencies to maximum

[Alan Stern]

On Wed, 31 Aug 2016, Felipe Balbi wrote:

> Hi John,
> 
> John Youn  writes:
> > On 8/31/2016 2:38 AM, Felipe Balbi wrote:
> >> If we don't know what are the actual U1/U2 exit
> >> latencies from the UDC, we're better off using
> >> maximum latencies as default. This should avoid
> >> any problems with too frequent U1/U2 entry.
> >> 
> >> Signed-off-by: Felipe Balbi 
> >> ---
> >>  include/linux/usb/gadget.h | 12 ++--
> >>  1 file changed, 10 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> >> index 3667d667cab1..20cb590c658e 100644
> >> --- a/include/linux/usb/gadget.h
> >> +++ b/include/linux/usb/gadget.h
> >> @@ -276,11 +276,19 @@ static inline void usb_ep_fifo_flush(struct usb_ep 
> >> *ep)
> >>  
> >>  
> >> /*-*/
> >>  
> >> +/**
> >> + * struct usb_dcd_config_params - Default U1/U2 exit latencies
> >> + * @bU1DevExitLat: U1 Exit Latency in us
> >> + * @bU2DevExitLat: U2 Exit Latency in us
> >> + *
> >> + * Note that we will be setting U1/U2 exit latencies to their maximum
> >> + * by default if no value is passed by the UDC Driver.
> >> + */
> >>  struct usb_dcd_config_params {
> >>__u8  bU1DevExitLat;/* U1 Device exit Latency */
> >> -#define USB_DEFAULT_U1_DEV_EXIT_LAT   0x01/* Less then 1 microsec 
> >> */
> >> +#define USB_DEFAULT_U1_DEV_EXIT_LAT   10 /* us */
> >>__le16 wU2DevExitLat;   /* U2 Device exit Latency */
> >> -#define USB_DEFAULT_U2_DEV_EXIT_LAT   0x1F4   /* Less then 500 
> >> microsec */
> >> +#define USB_DEFAULT_U2_DEV_EXIT_LAT   2047 /* us */
> >>  };
> >>  
> >>  
> >> 
> >
> > Hi Felipe,
> >
> > Speaking of this, how would you feel about adding module parameters in
> > the dwc3-pci to set these? And we also have several more settings in
> > our internal tree that we need for IP validation and debugging.
> >
> > As you know the IP is highly configurable, and we do much of the
> > testing against these configurations via software settings. And our
> > validation platform is not a typical platform. Basically we have a
> > single platform, but the instantiated IP and PHY could be configured
> > in any way so it could need different values passed in for
> > testing. Like the U1/U2 exit latencies, LPM NYET threshold, HIRD,
> > USB2/3 SUSPHY, LPM sleep mode, GFLADJ, etc.
> >
> > And some things that are automatically detected we need to restrict or
> > disable to get full coverage. Such as disabling U1/U2 and LPM, maximum
> > speed, dr_mode, NUMP, RX thresholding, RX thresholding packet count.
> >
> > I know module parameters are typically frowned upon so do you
> > recommend another approach?
> 
> I completely understand the situation. Module parameters are, well,
> rather unsophisticated. FPGA validation is, however, a 'peculiar' use
> case.
> 
> I want to avoid module parameters as much as possible, but apart from
> module parameters, the only thing I can think of is a specific
> sub-directory on debugfs which only gets compile if EXPERT &&
> DWC3_VALIDATION_MODE or whatever.
> 
> Would debugfs work for you? The only problem is for things which get
> discovered during probe()
> 
> It's an odd situation; whatever we do, I'd still like to be able to
> connect two HAPS DX to the same desktop (as PCIe cards) and validate
> both with different parameters.
> 
> I'm open for suggestions which don't involve module parameters :-)
> 
> Greg? Alan? Anybody?

debugfs seems like a good compromise.  At least it does offer the 
ability to set different values for different controllers.  The 
disadvantage is that you can't set the values right at probe time.  For 
testing that doesn't matter, but it might be important for actual use.

(Come to think of it, I don't know of any reasonable way to make
different values available for different controllers at probe time.  
Specifying a controller ID or address along with the values in a module
parameter is not reasonable.)

Alan Stern

--
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 3/3] usb: gadget: default U1/U2 exit latencies to maximum

[Felipe Balbi]

Hi John,

John Youn  writes:
> On 8/31/2016 2:38 AM, Felipe Balbi wrote:
>> If we don't know what are the actual U1/U2 exit
>> latencies from the UDC, we're better off using
>> maximum latencies as default. This should avoid
>> any problems with too frequent U1/U2 entry.
>> 
>> Signed-off-by: Felipe Balbi 
>> ---
>>  include/linux/usb/gadget.h | 12 ++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>> 
>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>> index 3667d667cab1..20cb590c658e 100644
>> --- a/include/linux/usb/gadget.h
>> +++ b/include/linux/usb/gadget.h
>> @@ -276,11 +276,19 @@ static inline void usb_ep_fifo_flush(struct usb_ep *ep)
>>  
>>  
>> /*-*/
>>  
>> +/**
>> + * struct usb_dcd_config_params - Default U1/U2 exit latencies
>> + * @bU1DevExitLat: U1 Exit Latency in us
>> + * @bU2DevExitLat: U2 Exit Latency in us
>> + *
>> + * Note that we will be setting U1/U2 exit latencies to their maximum
>> + * by default if no value is passed by the UDC Driver.
>> + */
>>  struct usb_dcd_config_params {
>>  __u8  bU1DevExitLat;/* U1 Device exit Latency */
>> -#define USB_DEFAULT_U1_DEV_EXIT_LAT 0x01/* Less then 1 microsec */
>> +#define USB_DEFAULT_U1_DEV_EXIT_LAT 10 /* us */
>>  __le16 wU2DevExitLat;   /* U2 Device exit Latency */
>> -#define USB_DEFAULT_U2_DEV_EXIT_LAT 0x1F4   /* Less then 500 microsec */
>> +#define USB_DEFAULT_U2_DEV_EXIT_LAT 2047 /* us */
>>  };
>>  
>>  
>> 
>
> Hi Felipe,
>
> Speaking of this, how would you feel about adding module parameters in
> the dwc3-pci to set these? And we also have several more settings in
> our internal tree that we need for IP validation and debugging.
>
> As you know the IP is highly configurable, and we do much of the
> testing against these configurations via software settings. And our
> validation platform is not a typical platform. Basically we have a
> single platform, but the instantiated IP and PHY could be configured
> in any way so it could need different values passed in for
> testing. Like the U1/U2 exit latencies, LPM NYET threshold, HIRD,
> USB2/3 SUSPHY, LPM sleep mode, GFLADJ, etc.
>
> And some things that are automatically detected we need to restrict or
> disable to get full coverage. Such as disabling U1/U2 and LPM, maximum
> speed, dr_mode, NUMP, RX thresholding, RX thresholding packet count.
>
> I know module parameters are typically frowned upon so do you
> recommend another approach?

I completely understand the situation. Module parameters are, well,
rather unsophisticated. FPGA validation is, however, a 'peculiar' use
case.

I want to avoid module parameters as much as possible, but apart from
module parameters, the only thing I can think of is a specific
sub-directory on debugfs which only gets compile if EXPERT &&
DWC3_VALIDATION_MODE or whatever.

Would debugfs work for you? The only problem is for things which get
discovered during probe()

It's an odd situation; whatever we do, I'd still like to be able to
connect two HAPS DX to the same desktop (as PCIe cards) and validate
both with different parameters.

I'm open for suggestions which don't involve module parameters :-)

Greg? Alan? Anybody?

-- 
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 3/3] usb: gadget: default U1/U2 exit latencies to maximum

[John Youn]

On 8/31/2016 2:38 AM, Felipe Balbi wrote:
> If we don't know what are the actual U1/U2 exit
> latencies from the UDC, we're better off using
> maximum latencies as default. This should avoid
> any problems with too frequent U1/U2 entry.
> 
> Signed-off-by: Felipe Balbi 
> ---
>  include/linux/usb/gadget.h | 12 ++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index 3667d667cab1..20cb590c658e 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -276,11 +276,19 @@ static inline void usb_ep_fifo_flush(struct usb_ep *ep)
>  
>  /*-*/
>  
> +/**
> + * struct usb_dcd_config_params - Default U1/U2 exit latencies
> + * @bU1DevExitLat: U1 Exit Latency in us
> + * @bU2DevExitLat: U2 Exit Latency in us
> + *
> + * Note that we will be setting U1/U2 exit latencies to their maximum
> + * by default if no value is passed by the UDC Driver.
> + */
>  struct usb_dcd_config_params {
>   __u8  bU1DevExitLat;/* U1 Device exit Latency */
> -#define USB_DEFAULT_U1_DEV_EXIT_LAT  0x01/* Less then 1 microsec */
> +#define USB_DEFAULT_U1_DEV_EXIT_LAT  10 /* us */
>   __le16 wU2DevExitLat;   /* U2 Device exit Latency */
> -#define USB_DEFAULT_U2_DEV_EXIT_LAT  0x1F4   /* Less then 500 microsec */
> +#define USB_DEFAULT_U2_DEV_EXIT_LAT  2047 /* us */
>  };
>  
>  
> 

Hi Felipe,

Speaking of this, how would you feel about adding module parameters in
the dwc3-pci to set these? And we also have several more settings in
our internal tree that we need for IP validation and debugging.

As you know the IP is highly configurable, and we do much of the
testing against these configurations via software settings. And our
validation platform is not a typical platform. Basically we have a
single platform, but the instantiated IP and PHY could be configured
in any way so it could need different values passed in for
testing. Like the U1/U2 exit latencies, LPM NYET threshold, HIRD,
USB2/3 SUSPHY, LPM sleep mode, GFLADJ, etc.

And some things that are automatically detected we need to restrict or
disable to get full coverage. Such as disabling U1/U2 and LPM, maximum
speed, dr_mode, NUMP, RX thresholding, RX thresholding packet count.

I know module parameters are typically frowned upon so do you
recommend another approach?

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

[PATCH 3/3] usb: gadget: default U1/U2 exit latencies to maximum

[Felipe Balbi]

If we don't know what are the actual U1/U2 exit
latencies from the UDC, we're better off using
maximum latencies as default. This should avoid
any problems with too frequent U1/U2 entry.

Signed-off-by: Felipe Balbi 
---
 include/linux/usb/gadget.h | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 3667d667cab1..20cb590c658e 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -276,11 +276,19 @@ static inline void usb_ep_fifo_flush(struct usb_ep *ep)
 
 /*-*/
 
+/**
+ * struct usb_dcd_config_params - Default U1/U2 exit latencies
+ * @bU1DevExitLat: U1 Exit Latency in us
+ * @bU2DevExitLat: U2 Exit Latency in us
+ *
+ * Note that we will be setting U1/U2 exit latencies to their maximum
+ * by default if no value is passed by the UDC Driver.
+ */
 struct usb_dcd_config_params {
__u8  bU1DevExitLat;/* U1 Device exit Latency */
-#define USB_DEFAULT_U1_DEV_EXIT_LAT0x01/* Less then 1 microsec */
+#define USB_DEFAULT_U1_DEV_EXIT_LAT10 /* us */
__le16 wU2DevExitLat;   /* U2 Device exit Latency */
-#define USB_DEFAULT_U2_DEV_EXIT_LAT0x1F4   /* Less then 500 microsec */
+#define USB_DEFAULT_U2_DEV_EXIT_LAT2047 /* us */
 };
 
 
-- 
2.9.1

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