RE: [PATCH v3 2/3] USB3/DWC3: Add property "snps, incr-burst-type-adjustment" for INCR burst type

From: Jerry Huang <jerry.huang@nxp.com>
To: Felipe Balbi <balbi@kernel.org>, Rob Herring <robh@kernel.org>
Cc: "mark.rutland@arm.com" <mark.rutland@arm.com>,
	"catalin.marinas@arm.com" <catalin.marinas@arm.com>,
	"will.deacon@arm.com" <will.deacon@arm.com>,
	"linux@armlinux.org.uk" <linux@armlinux.org.uk>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>
Subject: RE: [PATCH v3 2/3] USB3/DWC3: Add property "snps, incr-burst-type-adjustment" for INCR burst type
Date: Tue, 17 Jan 2017 10:37:17 +0000	[thread overview]
Message-ID: <DB5PR0401MB1813548526D3C8D34B9CCE52FE7C0@DB5PR0401MB1813.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <87tw8zo0c0.fsf@linux.intel.com>

> -----Original Message-----
> From: Felipe Balbi [mailto:balbi@kernel.org]
> Sent: Monday, January 16, 2017 4:50 PM
> To: Jerry Huang <jerry.huang@nxp.com>; Rob Herring <robh@kernel.org>
> Cc: mark.rutland@arm.com; catalin.marinas@arm.com;
> will.deacon@arm.com; linux@armlinux.org.uk; devicetree@vger.kernel.org;
> linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org
> Subject: RE: [PATCH v3 2/3] USB3/DWC3: Add property "snps, incr-burst-
> type-adjustment" for INCR burst type
> 
> 
> Hi,
> 
> Jerry Huang <jerry.huang@nxp.com> writes:
> >> > On Thu, Dec 22, 2016 at 8:52 PM, Jerry Huang <jerry.huang@nxp.com>
> >> wrote:
> >> > > Hi, Rob,
> >> > >> -----Original Message-----
> >> > >> From: Rob Herring [mailto:robh@kernel.org]
> >> > >> Sent: Friday, December 23, 2016 2:45 AM
> >> > >> To: Jerry Huang <jerry.huang@nxp.com>
> >> > >> Cc: balbi@kernel.org; mark.rutland@arm.com;
> >> > >> catalin.marinas@arm.com; will.deacon@arm.com;
> >> > >> linux@armlinux.org.uk; devicetree@vger.kernel.org;
> >> > >> linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org;
> >> > >> linux-arm- kernel@lists.infradead.org
> >> > >> Subject: Re: [PATCH v3 2/3] USB3/DWC3: Add property "snps,
> >> > >> incr-burst- type-adjustment" for INCR burst type
> >> > >>
> >> > >> On Mon, Dec 19, 2016 at 05:25:53PM +0800, Changming Huang wrote:
> >> > >> > New property "snps,incr-burst-type-adjustment = <x>, <y>" for
> >> > >> > USB3.0
> >> > >> DWC3.
> >> > >> > Field "x": 1/0 - undefined length INCR burst type enable or
> >> > >> > not; Field
> >> > >> > "y": INCR4/INCR8/INCR16/INCR32/INCR64/INCR128/INCR256 burst
> >> type.
> >> > >> >
> >> > >> > While enabling undefined length INCR burst type and INCR16
> >> > >> > burst type, get better write performance on NXP Layerscape
> platform:
> >> > >> > around 3% improvement (from 364MB/s to 375MB/s).
> >> > >> >
> >> > >> > Signed-off-by: Changming Huang <jerry.huang@nxp.com>
> >> > >> > ---
> >> > >> > Changes in v3:
> >> > >> >   - add new property for INCR burst in usb node.
> >> > >> >
> >> > >> >  Documentation/devicetree/bindings/usb/dwc3.txt |    5 +++++
> >> > >> >  arch/arm/boot/dts/ls1021a.dtsi                 |    1 +
> >> > >> >  arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi |    3 +++
> >> > >> >  arch/arm64/boot/dts/freescale/fsl-ls2080a.dtsi |    2 ++
> >> > >> >  4 files changed, 11 insertions(+)
> >> > >> >
> >> > >> > diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt
> >> > >> > b/Documentation/devicetree/bindings/usb/dwc3.txt
> >> > >> > index e3e6983..8c405a3 100644
> >> > >> > --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> >> > >> > +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> >> > >> > @@ -55,6 +55,10 @@ Optional properties:
> >> > >> >     fladj_30mhz_sdbnd signal is invalid or incorrect.
> >> > >> >
> >> > >> >   - <DEPRECATED> tx-fifo-resize: determines if the FIFO *has*
> >> > >> > to be
> >> > >> reallocated.
> >> > >> > + - snps,incr-burst-type-adjustment: Value for INCR burst type
> >> > >> > + of
> >> > >> GSBUSCFG0
> >> > >> > +   register, undefined length INCR burst type enable and INCRx
> type.
> >> > >> > +   First field is for undefined length INCR burst type enable or not.
> >> > >> > +   Second field is for largest INCRx type enabled.
> >> > >>
> >> > >> Why do you need the first field? Is the 2nd field used if the 1st is 0?
> >> > >> If not, then just use the presence of the property to enable or not.
> >> > > The first field is one switch.
> >> > > When it is 1, means undefined length INCR burst type enabled, we
> >> > > can use
> >> > any length less than or equal to the largest-enabled burst length
> >> > of INCR4/8/16/32/64/128/256.
> >> > > When it is zero, means INCRx burst mode enabled, we can use one
> >> > > fixed
> >> > burst length of 1/4/8/16/32/64/128/256 byte.
> >> > > So, the 2nd field is used if the 1st is 0, we need to select one
> >> > > largest burst
> >> > length the USB controller can support.
> >> > > If we don't want to change the value of this register (use the
> >> > > default value),
> >> > we don't need to add this property to usb node.
> >> >
> >> > Just make this a single value with 0 meaning INCR and 4/8/16/etc
> >> > being
> >> INCRx.
> >> Maybe, I didn't describe it clearly.
> >> According to DWC3 spec, the value "0" of field INCRBrstEna means
> >> INCRx burst mode, 1 means INCR burst mode.
> >> Regardless of the value of INCRBrstEna [bit0], we need to modify the
> >> other field bit[1,2,3,4,5,6,7] to one INCR burst type  for the platform
> supported.
> >> Ad you mentioned, if we just use a single value with 0 meaning INCR
> >> and 4/8/16/etc being INCRx.
> >> I understand totally that when it is none-zero, we can use it for
> >> INCR burst mode.
> >> Then, when it is 0, how to select the INCRx value?
> >>
> >> So, I think we still need two vaue to specify INCRBrstEna and INCRx
> >> burst type.
> > Hi, Balbi,
> > It seems there is no feedback for my comment, so these patches can be
> accepted?
> 
> probably not, we need to really understand what information we need so it
> can be described properly. The last thing we want is unnecessary DT
> properties.
> 
> It seems to me that we can extrapolate INCRBrstEna based on which burst
> modes are enabled. If only 0 is passed, then that bit should be 1, if 0 and any
> other size is passed, then that bit should be 0, no?
Hi, Balbi,
Below is the definition for this property,
snps,incr-burst-type-adjustment = <x>, <y>
x: Undefined Length INCR Burst Type Enable (INCRBrstEna)
    0 - INCRX burst mode (not enable INCRBrstEna)
    1 - INCR (undefined length) burst mode (enable INCRBrstEna)
y: the burst length

1> if x = 0: means INCRBrstEna not enabled, set bit0 to zero (or clear it) , we select one of INCR1/INCR4/INCR8/INCR16/INCR32/INCR64/INCR128/INCR256 (pass this value through "y")to set the fix burst length controller supported.
For example:
snps,incr-burst-type-adjustment = <0>, <16>
driver will set bit0 to zero and set bit3 to 1 (INCR16 Burst Type Enabled), controller will use INCR16 (with 16 bytes) to transfer data.

2> if x = 1: means INCRBrstEna enabled, we select one of INCR4/INCR8/INCR16/INCR32/INCR64/INCR128/INCR256 (pass this value through "y") to set the burst length, and controller will use any length less than or equal to that we selected.
For example:
snps,incr-burst-type-adjustment = <1>, <32>
driver will set bit0 to 1 and set bit4 to 1 (INCR32 Burst Type Enabled), controller will use any burst length less than (such as 4, 8, 16 byte) or equal to 32 byte to transfer data.

Therefore, I think this two fileds are needed. Do you think about it?