On 9/14/2016 11:53 PM, Felipe Balbi wrote:
>
> Hi,
>
> John Youn <John.Youn@xxxxxxxxxxxx> writes:
>>>> John Youn <John.Youn@xxxxxxxxxxxx> 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 <felipe.balbi@xxxxxxxxxxxxxxx>
>>>>>> ---
>>>>>> 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. 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.
>
Well you never said 'no' in the first place. The thread just died
before any decision was made either way. Hence why I bring it up
again.
As I mentioned previously, one other thing I can think of is to rework
the platform probe to take out the core and parameter initialization
to be called from other contexts. What do you think about this?
In any case, whatever the ultimate solution, I will follow the same in
dwc2.
Regards,
John
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
