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

From: Rob Herring <robh@kernel.org>
To: Felipe Balbi <balbi@kernel.org>
Cc: Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
	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: Thu, 19 Dec 2019 16:17:04 -0600	[thread overview]
Message-ID: <20191219221704.GA29965@bogus> (raw)
In-Reply-To: <87d0cswvs9.fsf@kernel.org>

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