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

[Jerry Huang]

> -----Original Message-----
> From: Jerry Huang
> Sent: Wednesday, January 04, 2017 10:25 AM
> To: 'Rob Herring' <robh@xxxxxxxxxx>
> Cc: balbi@xxxxxxxxxx; mark.rutland@xxxxxxx; catalin.marinas@xxxxxxx;
> will.deacon@xxxxxxx; linux@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> linux-usb@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx
> Subject: RE: [PATCH v3 2/3] USB3/DWC3: Add property "snps, incr-burst-
> type-adjustment" for INCR burst type
> 
> Hi, Rob,
> 
> > -----Original Message-----
> > From: Rob Herring [mailto:robh@xxxxxxxxxx]
> > Sent: Wednesday, January 04, 2017 5:24 AM
> > To: Jerry Huang <jerry.huang@xxxxxxx>
> > Cc: balbi@xxxxxxxxxx; mark.rutland@xxxxxxx; catalin.marinas@xxxxxxx;
> > will.deacon@xxxxxxx; linux@xxxxxxxxxxxxxxx;
> > devicetree@xxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx;
> > linux-kernel@xxxxxxxxxxxxxxx; linux-arm- kernel@xxxxxxxxxxxxxxxxxxx
> > Subject: Re: [PATCH v3 2/3] USB3/DWC3: Add property "snps, incr-burst-
> > type-adjustment" for INCR burst type
> >
> > On Thu, Dec 22, 2016 at 8:52 PM, Jerry Huang <jerry.huang@xxxxxxx>
> wrote:
> > > Hi, Rob,
> > >> -----Original Message-----
> > >> From: Rob Herring [mailto:robh@xxxxxxxxxx]
> > >> Sent: Friday, December 23, 2016 2:45 AM
> > >> To: Jerry Huang <jerry.huang@xxxxxxx>
> > >> Cc: balbi@xxxxxxxxxx; mark.rutland@xxxxxxx;
> > >> catalin.marinas@xxxxxxx; will.deacon@xxxxxxx;
> > >> linux@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> > >> linux-usb@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-arm-
> > >> kernel@xxxxxxxxxxxxxxxxxxx
> > >> 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@xxxxxxx>
> > >> > ---
> > >> > 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?