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

From: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
To: Rob Herring <robh@kernel.org>, Thinh Nguyen <Thinh.Nguyen@synopsys.com>
Cc: Felipe Balbi <balbi@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	John Youn <John.Youn@synopsys.com>
Subject: Re: [RFC PATCH 13/14] usb: devicetree: dwc3: Add property to disable mult TRB fetch
Date: Fri, 20 Dec 2019 23:52:10 +0000	[thread overview]
Message-ID: <75877f65-0b2c-fa71-22df-1199a5463814@synopsys.com> (raw)
In-Reply-To: <CAL_JsqKrRSk0NhqZ5tcus23rWfBzHcxBs8ZR6qv2qR4z_NrD_A@mail.gmail.com>

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