RE: [PATCH] USB3/DWC3: Enable undefined length INCR burst type

[Jerry Huang]

Hi, Balbi,

> -----Original Message-----
> From: Felipe Balbi [mailto:balbi@xxxxxxxxxx]
> Sent: Saturday, December 17, 2016 1:02 AM
> To: Jerry Huang <jerry.huang@xxxxxxx>; gregkh@xxxxxxxxxxxxxxxxxxx
> Cc: linux-usb@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Rajesh Bhagat
> <rajesh.bhagat@xxxxxxx>
> Subject: RE: [PATCH] USB3/DWC3: Enable undefined length INCR burst type
> 
> 
> Hi,
> 
> Jerry Huang <jerry.huang@xxxxxxx> writes:
> > Hi, Balbi,
> >> -----Original Message-----
> >> From: Felipe Balbi [mailto:balbi@xxxxxxxxxx]
> >> Sent: Friday, December 16, 2016 7:44 PM
> >> To: Jerry Huang <jerry.huang@xxxxxxx>; gregkh@xxxxxxxxxxxxxxxxxxx
> >> Cc: linux-usb@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Rajesh
> >> Bhagat <rajesh.bhagat@xxxxxxx>
> >> Subject: RE: [PATCH] USB3/DWC3: Enable undefined length INCR burst
> >> type
> >>
> >>
> >> Hi,
> >>
> >> Jerry Huang <jerry.huang@xxxxxxx> writes:
> >> >> there's no need for that. This patch is in good format. I do have
> >> >> a question,
> >> >> however: how do you know this will work for all users? Burst size
> >> >> is a function of how wide the interconnect where dwc3 is attached to,
> is.
> >> > So I need to generate one new property in usb node to identify my
> >> platform?
> >>
> >> Well, we probably need a property to be passed, yes. But let's go
> >> through it all first :-)
> >
> > I think "snps,quirk-frame-length-adjustment" is one good reference,
> > which can pass the required value to driver from DTS file.
> 
> that's not for burst increment, though.
I created one new property " snps,incr-burst-type-adjustment = <x>, <y>" to identify it from usb node, and will send out the v3 patch.

> >> >> You could very well be degrading performance for some users here.
> >> >> Can you send me the result of the following commands *without*
> >> >> this patch
> >> applied?
> >> >>
> >> >> # mkdir -p /d
> >> >> # mount -t debugfs none /d
> >> >> # cat /d/*dwc3*/regdump
> >> >>
> >> > Below is the regdump:
> >> > root@ls1043ardb:/d/3000000.usb3# cat regdump
> >> > GSBUSCFG0 = 0x00100080
> >>
> >> so you already have INCR256 here. There's one note in the databook
> >> which just caught my attention. It states the following:
> >>
> >> 	"Undefined burst length has priority over all other burst lenghts."
> >>
> >> This means that setting both INCR16 and undefined INCR is unnecessary.
> > When bit0 = 1 (Undefined Length INCR Burst Type Enable), which means:
> >  1: INCR (undefined length) burst mode
> > - AHB configurations: HBURST uses SINGLE or INCR of any length less
> > than or equal to the largest-enabled burst length of
> > INCR4/8/16/32/64/128/256.
> > - AXI configurations: ARLEN/AWLEN uses any length less than or equal
> > to the largest-enabled burst length of INCR4/8/16/32/64/128/256.
> 
> interesting, it doesn't describe what happens to OCP or PCI.
Yes, just mention AHB and AXI from DWC superspeed USB3.0 controller databook (Version 2.50a, November 2012, maybe it is too old version for this IP).

> > So, after enable undefined length INCR burst and enable INCR16,
> > controller will use less than or equal to 16byte.
> >
> >> Only Undefined INCR will be taken into consideration. Can you check
> >> with your HW engineers what's the largest burst the interconnect is
> >> supposed to support?
> > I will check it with IP designer.
> 
> cool, thanks :-)
Have confirmed with hardware engineer,  the maximum INCR burst size of NXP platform is 16 byte

> >> > GSBUSCFG1 = 0x00000700
> >>
> >> 8 AXI pipelined requests
> >>
> >> > GSNPSID = 0x5533280a
> >>
> >> 2.80a cool :-)
> >>
> >> I'll check these settings on my platform as well and see if there's
> >> any setting which would improve transfer speed. This is a very good
> >> idea, btw, but we need to be careful about how to play with it.
> >>
> >> --
> >> balbi
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-usb"
> > in the body of a message to majordomo@xxxxxxxxxxxxxxx More
> majordomo
> > info at  http://vger.kernel.org/majordomo-info.html
> 
> --
> balbi