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

-----Original Message-----
From: Felipe Balbi [mailto:balbi at kernel.org]
Sent: Monday, January 16, 2017 4:50 PM
To: Jerry Huang <redacted>; Rob Herring <robh@kernel.org>
Cc: mark.rutland at arm.com; catalin.marinas at arm.com;
will.deacon at arm.com; linux at armlinux.org.uk; devicetree at vger.kernel.org;
linux-usb at vger.kernel.org; linux-kernel at vger.kernel.org; linux-arm-
kernel at 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 [off-list ref] writes:

quoted

quoted

quoted

On Thu, Dec 22, 2016 at 8:52 PM, Jerry Huang [off-list ref]

wrote:

quoted

quoted

Hi, Rob,

quoted

-----Original Message-----
From: Rob Herring [mailto:robh at kernel.org]
Sent: Friday, December 23, 2016 2:45 AM
To: Jerry Huang <redacted>
Cc: balbi at kernel.org; mark.rutland at arm.com;
catalin.marinas at arm.com; will.deacon at arm.com;
linux at armlinux.org.uk; devicetree at vger.kernel.org;
linux-usb at vger.kernel.org; linux-kernel at vger.kernel.org;
linux-arm- kernel at 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:

quoted

New property "snps,incr-burst-type-adjustment = <x>, <y>" for
USB3.0

DWC3.

quoted

Field "x": 1/0 - undefined length INCR burst type enable or
not; Field
"y": INCR4/INCR8/INCR16/INCR32/INCR64/INCR128/INCR256 burst

type.

quoted

quoted

quoted

quoted

While enabling undefined length INCR burst type and INCR16
burst type, get better write performance on NXP Layerscape

platform:

quoted

quoted

quoted

quoted

quoted

quoted

around 3% improvement (from 364MB/s to 375MB/s).

Signed-off-by: Changming Huang <redacted>
---
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.

quoted

+ - snps,incr-burst-type-adjustment: Value for INCR burst type
+ of

GSBUSCFG0

quoted

+   register, undefined length INCR burst type enable and INCRx

type.

quoted

quoted

quoted

quoted

quoted

quoted

+   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.

quoted

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.

quoted

So, the 2nd field is used if the 1st is 0, we need to select one
largest burst

length the USB controller can support.

quoted

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.

quoted

quoted

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?