diff mbox series

[RFC,13/14] usb: devicetree: dwc3: Add property to disable mult TRB fetch

Message ID b791f032edb8e6a739c342dbd0d2d5faa66ddfb8.1576118671.git.thinhn@synopsys.com (mailing list archive)
State New, archived
Headers show
Series usb: dwc3: Introduce DWC_usb32 | expand

Commit Message

Thinh Nguyen Dec. 12, 2019, 2:50 a.m. UTC
DWC_usb32 has a feature where it can issue multiple TRB fetch requests.
Add a new property to limit and only do only single TRB fetch request.

Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
---
 Documentation/devicetree/bindings/usb/dwc3.txt | 2 ++
 1 file changed, 2 insertions(+)

Comments

Felipe Balbi Dec. 12, 2019, 8:19 a.m. UTC | #1
Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
> DWC_usb32 has a feature where it can issue multiple TRB fetch requests.
> Add a new property to limit and only do only single TRB fetch request.
>
> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
> ---
>  Documentation/devicetree/bindings/usb/dwc3.txt | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
> index ff35fa6de2eb..29d6f9b1fc70 100644
> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> @@ -108,6 +108,8 @@ Optional properties:
>   - snps,num-trb-prefetch: max value to do TRBs cache for DWC_usb32. The value
>  			can be from 1 to DWC_USB32_CACHE_TRBS_PER_TRANSFER.
>  			Default value is DWC_USB32_CACHE_TRBS_PER_TRANSFER.
> + - snps,dis-mult-trb-fetch: set to issue only single TRB fetch request in
> +			DWC_usb32.

two questions:

- how is this different from passing 1 to the previous DT binding
- do we know of anybody having issues with multi-trb prefetch?
Thinh Nguyen Dec. 12, 2019, 10:28 p.m. UTC | #2
Hi,

Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>> DWC_usb32 has a feature where it can issue multiple TRB fetch requests.
>> Add a new property to limit and only do only single TRB fetch request.
>>
>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
>> ---
>>   Documentation/devicetree/bindings/usb/dwc3.txt | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>> index ff35fa6de2eb..29d6f9b1fc70 100644
>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>> @@ -108,6 +108,8 @@ Optional properties:
>>    - snps,num-trb-prefetch: max value to do TRBs cache for DWC_usb32. The value
>>   			can be from 1 to DWC_USB32_CACHE_TRBS_PER_TRANSFER.
>>   			Default value is DWC_USB32_CACHE_TRBS_PER_TRANSFER.
>> + - snps,dis-mult-trb-fetch: set to issue only single TRB fetch request in
>> +			DWC_usb32.
> two questions:
>
> - how is this different from passing 1 to the previous DT binding

The previous DT binding is related to the number TRBs to cache while 
this one is related to whether the controller will send multiple 
(internal) fetch commands to fetch the TRBs.

> - do we know of anybody having issues with multi-trb prefetch?

No, we added this for various internal tests.

BR,
Thinh
Felipe Balbi Dec. 13, 2019, 7:04 a.m. UTC | #3
Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>> DWC_usb32 has a feature where it can issue multiple TRB fetch requests.
>>> Add a new property to limit and only do only single TRB fetch request.
>>>
>>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
>>> ---
>>>   Documentation/devicetree/bindings/usb/dwc3.txt | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>>> index ff35fa6de2eb..29d6f9b1fc70 100644
>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>>> @@ -108,6 +108,8 @@ Optional properties:
>>>    - snps,num-trb-prefetch: max value to do TRBs cache for DWC_usb32. The value
>>>   			can be from 1 to DWC_USB32_CACHE_TRBS_PER_TRANSFER.
>>>   			Default value is DWC_USB32_CACHE_TRBS_PER_TRANSFER.
>>> + - snps,dis-mult-trb-fetch: set to issue only single TRB fetch request in
>>> +			DWC_usb32.
>> two questions:
>>
>> - how is this different from passing 1 to the previous DT binding
>
> The previous DT binding is related to the number TRBs to cache while 
> this one is related to whether the controller will send multiple 
> (internal) fetch commands to fetch the TRBs.
>
>> - do we know of anybody having issues with multi-trb prefetch?
>
> No, we added this for various internal tests.

We really a better way for you guys to have your test coverage enabled
with upstream kernel. I wonder if DT guys would accept a set of bindings
marked as "for testing purposes". In any case, we really need to enable
Silicon Validation with upstream kernel.
Thinh Nguyen Dec. 13, 2019, 8:10 p.m. UTC | #4
Hi,

Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>>> DWC_usb32 has a feature where it can issue multiple TRB fetch requests.
>>>> Add a new property to limit and only do only single TRB fetch request.
>>>>
>>>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
>>>> ---
>>>>    Documentation/devicetree/bindings/usb/dwc3.txt | 2 ++
>>>>    1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>> index ff35fa6de2eb..29d6f9b1fc70 100644
>>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>> @@ -108,6 +108,8 @@ Optional properties:
>>>>     - snps,num-trb-prefetch: max value to do TRBs cache for DWC_usb32. The value
>>>>    			can be from 1 to DWC_USB32_CACHE_TRBS_PER_TRANSFER.
>>>>    			Default value is DWC_USB32_CACHE_TRBS_PER_TRANSFER.
>>>> + - snps,dis-mult-trb-fetch: set to issue only single TRB fetch request in
>>>> +			DWC_usb32.
>>> two questions:
>>>
>>> - how is this different from passing 1 to the previous DT binding
>> The previous DT binding is related to the number TRBs to cache while
>> this one is related to whether the controller will send multiple
>> (internal) fetch commands to fetch the TRBs.
>>
>>> - do we know of anybody having issues with multi-trb prefetch?
>> No, we added this for various internal tests.
> We really a better way for you guys to have your test coverage enabled
> with upstream kernel. I wonder if DT guys would accept a set of bindings
> marked as "for testing purposes". In any case, we really need to enable
> Silicon Validation with upstream kernel.
>

That would be great! If there's a sensible way to do so, we're open to 
suggestions.

Thanks,
Thinh
Rob Herring (Arm) Dec. 19, 2019, 10:17 p.m. UTC | #5
On Fri, Dec 13, 2019 at 09:04:54AM +0200, Felipe Balbi wrote:
> 
> Hi,
> 
> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
> >> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
> >>> DWC_usb32 has a feature where it can issue multiple TRB fetch requests.
> >>> Add a new property to limit and only do only single TRB fetch request.
> >>>
> >>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
> >>> ---
> >>>   Documentation/devicetree/bindings/usb/dwc3.txt | 2 ++
> >>>   1 file changed, 2 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
> >>> index ff35fa6de2eb..29d6f9b1fc70 100644
> >>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> >>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> >>> @@ -108,6 +108,8 @@ Optional properties:
> >>>    - snps,num-trb-prefetch: max value to do TRBs cache for DWC_usb32. The value
> >>>   			can be from 1 to DWC_USB32_CACHE_TRBS_PER_TRANSFER.
> >>>   			Default value is DWC_USB32_CACHE_TRBS_PER_TRANSFER.
> >>> + - snps,dis-mult-trb-fetch: set to issue only single TRB fetch request in
> >>> +			DWC_usb32.
> >> two questions:
> >>
> >> - how is this different from passing 1 to the previous DT binding
> >
> > The previous DT binding is related to the number TRBs to cache while 
> > this one is related to whether the controller will send multiple 
> > (internal) fetch commands to fetch the TRBs.
> >
> >> - do we know of anybody having issues with multi-trb prefetch?
> >
> > No, we added this for various internal tests.
> 
> We really a better way for you guys to have your test coverage enabled
> with upstream kernel. I wonder if DT guys would accept a set of bindings
> marked as "for testing purposes". In any case, we really need to enable
> Silicon Validation with upstream kernel.

Well, anything would be better than the endless stream of new 
properties. Include 'test-mode' in the property names would be fine I 
guess.

However, why do they need to be in DT rather than module params or 
debugfs settings? Changing at runtime or reloading the module is a 
better experience than editting a DT and rebooting.

Rob
Thinh Nguyen Dec. 19, 2019, 10:51 p.m. UTC | #6
Hi,

Rob Herring wrote:
> On Fri, Dec 13, 2019 at 09:04:54AM +0200, Felipe Balbi wrote:
>> Hi,
>>
>> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>>> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>>>> DWC_usb32 has a feature where it can issue multiple TRB fetch requests.
>>>>> Add a new property to limit and only do only single TRB fetch request.
>>>>>
>>>>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
>>>>> ---
>>>>>    Documentation/devicetree/bindings/usb/dwc3.txt | 2 ++
>>>>>    1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>> index ff35fa6de2eb..29d6f9b1fc70 100644
>>>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>> @@ -108,6 +108,8 @@ Optional properties:
>>>>>     - snps,num-trb-prefetch: max value to do TRBs cache for DWC_usb32. The value
>>>>>    			can be from 1 to DWC_USB32_CACHE_TRBS_PER_TRANSFER.
>>>>>    			Default value is DWC_USB32_CACHE_TRBS_PER_TRANSFER.
>>>>> + - snps,dis-mult-trb-fetch: set to issue only single TRB fetch request in
>>>>> +			DWC_usb32.
>>>> two questions:
>>>>
>>>> - how is this different from passing 1 to the previous DT binding
>>> The previous DT binding is related to the number TRBs to cache while
>>> this one is related to whether the controller will send multiple
>>> (internal) fetch commands to fetch the TRBs.
>>>
>>>> - do we know of anybody having issues with multi-trb prefetch?
>>> No, we added this for various internal tests.
>> We really a better way for you guys to have your test coverage enabled
>> with upstream kernel. I wonder if DT guys would accept a set of bindings
>> marked as "for testing purposes". In any case, we really need to enable
>> Silicon Validation with upstream kernel.
> Well, anything would be better than the endless stream of new
> properties. Include 'test-mode' in the property names would be fine I
> guess.
>

Generally the properties are for some quirks or configurations that the 
controller must use to work properly and not for debugging purposes. 
Unfortunately, this list can be long..

> However, why do they need to be in DT rather than module params or
> debugfs settings? Changing at runtime or reloading the module is a
> better experience than editting a DT and rebooting.
>
> Rob

Internally, our tool can debug different properties as if they are 
module parameters. They can be changed at runtime. Setting properties is 
one of the options which we can configure without tampering much of the 
existing code.

BR,
Thinh
Rob Herring (Arm) Dec. 20, 2019, 10:11 p.m. UTC | #7
On Thu, Dec 19, 2019 at 3:52 PM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
>
> Hi,
>
> Rob Herring wrote:
> > On Fri, Dec 13, 2019 at 09:04:54AM +0200, Felipe Balbi wrote:
> >> Hi,
> >>
> >> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
> >>>> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
> >>>>> DWC_usb32 has a feature where it can issue multiple TRB fetch requests.
> >>>>> Add a new property to limit and only do only single TRB fetch request.
> >>>>>
> >>>>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
> >>>>> ---
> >>>>>    Documentation/devicetree/bindings/usb/dwc3.txt | 2 ++
> >>>>>    1 file changed, 2 insertions(+)
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
> >>>>> index ff35fa6de2eb..29d6f9b1fc70 100644
> >>>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> >>>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> >>>>> @@ -108,6 +108,8 @@ Optional properties:
> >>>>>     - snps,num-trb-prefetch: max value to do TRBs cache for DWC_usb32. The value
> >>>>>                           can be from 1 to DWC_USB32_CACHE_TRBS_PER_TRANSFER.
> >>>>>                           Default value is DWC_USB32_CACHE_TRBS_PER_TRANSFER.
> >>>>> + - snps,dis-mult-trb-fetch: set to issue only single TRB fetch request in
> >>>>> +                 DWC_usb32.
> >>>> two questions:
> >>>>
> >>>> - how is this different from passing 1 to the previous DT binding
> >>> The previous DT binding is related to the number TRBs to cache while
> >>> this one is related to whether the controller will send multiple
> >>> (internal) fetch commands to fetch the TRBs.
> >>>
> >>>> - do we know of anybody having issues with multi-trb prefetch?
> >>> No, we added this for various internal tests.
> >> We really a better way for you guys to have your test coverage enabled
> >> with upstream kernel. I wonder if DT guys would accept a set of bindings
> >> marked as "for testing purposes". In any case, we really need to enable
> >> Silicon Validation with upstream kernel.
> > Well, anything would be better than the endless stream of new
> > properties. Include 'test-mode' in the property names would be fine I
> > guess.
> >
>
> Generally the properties are for some quirks or configurations that the
> controller must use to work properly and not for debugging purposes.
> Unfortunately, this list can be long..

quirks or testing? Now I'm confused, which is it?

I'm sure I've said this before (for DWC3), but it is better to have a
compatible string for each implementation (usually an SoC) so you can
address new quirks without a dtb change and continually adding new
properties.

Rob
Thinh Nguyen Dec. 20, 2019, 11:52 p.m. UTC | #8
Hi,

Rob Herring wrote:
> On Thu, Dec 19, 2019 at 3:52 PM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
>> Hi,
>>
>> Rob Herring wrote:
>>> On Fri, Dec 13, 2019 at 09:04:54AM +0200, Felipe Balbi wrote:
>>>> Hi,
>>>>
>>>> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>>>>> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>>>>>> DWC_usb32 has a feature where it can issue multiple TRB fetch requests.
>>>>>>> Add a new property to limit and only do only single TRB fetch request.
>>>>>>>
>>>>>>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
>>>>>>> ---
>>>>>>>     Documentation/devicetree/bindings/usb/dwc3.txt | 2 ++
>>>>>>>     1 file changed, 2 insertions(+)
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>>>> index ff35fa6de2eb..29d6f9b1fc70 100644
>>>>>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>>>> @@ -108,6 +108,8 @@ Optional properties:
>>>>>>>      - snps,num-trb-prefetch: max value to do TRBs cache for DWC_usb32. The value
>>>>>>>                            can be from 1 to DWC_USB32_CACHE_TRBS_PER_TRANSFER.
>>>>>>>                            Default value is DWC_USB32_CACHE_TRBS_PER_TRANSFER.
>>>>>>> + - snps,dis-mult-trb-fetch: set to issue only single TRB fetch request in
>>>>>>> +                 DWC_usb32.
>>>>>> two questions:
>>>>>>
>>>>>> - how is this different from passing 1 to the previous DT binding
>>>>> The previous DT binding is related to the number TRBs to cache while
>>>>> this one is related to whether the controller will send multiple
>>>>> (internal) fetch commands to fetch the TRBs.
>>>>>
>>>>>> - do we know of anybody having issues with multi-trb prefetch?
>>>>> No, we added this for various internal tests.
>>>> We really a better way for you guys to have your test coverage enabled
>>>> with upstream kernel. I wonder if DT guys would accept a set of bindings
>>>> marked as "for testing purposes". In any case, we really need to enable
>>>> Silicon Validation with upstream kernel.
>>> Well, anything would be better than the endless stream of new
>>> properties. Include 'test-mode' in the property names would be fine I
>>> guess.
>>>
>> Generally the properties are for some quirks or configurations that the
>> controller must use to work properly and not for debugging purposes.
>> Unfortunately, this list can be long..
> quirks or testing? Now I'm confused, which is it?

I was referring to the existing properties, they are necessary for a 
working device. Nothing "extra" solely for debugging purposes.

With the exception for these couple properties related to TRB fetching 
in this RFC series, they are for testing only. Regardless, I agreed to 
Felipe previously that we can remove them.


> I'm sure I've said this before (for DWC3), but it is better to have a
> compatible string for each implementation (usually an SoC) so you can
> address new quirks without a dtb change and continually adding new
> properties.
>
> Rob

Yes, you mentioned it before. It may work on SoC, but it won't work for 
PCI devices. We can't use VID:PID either, because the HAPS devices only 
have a set of VID:PID.

Please refer back to a couple discussions back:
1) https://www.spinics.net/lists/linux-usb/msg164494.html

This one is related to cadence's, but I think it's relevant:
2) https://www.spinics.net/lists/linux-usb/msg175199.html

There maybe more discussions previously, but I don't have the history of 
all the emails.

BR,
Thinh
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
index ff35fa6de2eb..29d6f9b1fc70 100644
--- a/Documentation/devicetree/bindings/usb/dwc3.txt
+++ b/Documentation/devicetree/bindings/usb/dwc3.txt
@@ -108,6 +108,8 @@  Optional properties:
  - snps,num-trb-prefetch: max value to do TRBs cache for DWC_usb32. The value
 			can be from 1 to DWC_USB32_CACHE_TRBS_PER_TRANSFER.
 			Default value is DWC_USB32_CACHE_TRBS_PER_TRANSFER.
+ - snps,dis-mult-trb-fetch: set to issue only single TRB fetch request in
+			DWC_usb32.
 
  - <DEPRECATED> tx-fifo-resize: determines if the FIFO *has* to be reallocated.
  - snps,incr-burst-type-adjustment: Value for INCR burst type of GSBUSCFG0