RE: [PATCH v4 3/3] USB3/DWC3: Enable undefined length INCR burst type
> -Original Message- > From: Felipe Balbi [mailto:ba...@kernel.org] > Sent: Friday, March 10, 2017 7:27 PM > To: Jerry Huang ; robh...@kernel.org; > mark.rutl...@arm.com; catalin.mari...@arm.com > Cc: linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; > devicet...@vger.kernel.org; linux-arm-ker...@lists.infradead.org; Rajesh > Bhagat > Subject: RE: [PATCH v4 3/3] USB3/DWC3: Enable undefined length INCR burst > type > > > Hi, > > Jerry Huang writes: > >> >> -- > >> >> 1.7.9.5 > >> > Hi, Balbi and all guys, > >> > Any comment for these patches? Can they be accepted? > >> > >> Rob had comments which you didn't reply yet. I cannot take this > >> patchset yet ;-) > >> > > Balbi, > > > > I look into his mail again, which was based v3, and I replied it. > > > > He had different understanding for undefined length burst mode. > > > > It seems he think for this mode, just setting bit[0] (INCRBrstEna) and > > don't need to set other field. > > > > However, according to the DWC USB3.0 controller databook, when it is > > undefined length INCR burst mode, we still need to set one max burst > > type, such as INCR8, which means controller will use any length less > > than or equal to this INCR8. > > Rob, do you agree with the patch now? > > -- > balbi Hi, Balbi, Any comment for these patches? Or any chance to merge them?
RE: [PATCH v4 3/3] USB3/DWC3: Enable undefined length INCR burst type
> -Original Message- > From: Jerry Huang > Sent: Friday, February 10, 2017 11:30 PM > To: 'Felipe Balbi' ; robh...@kernel.org; > mark.rutl...@arm.com; catalin.mari...@arm.com > Cc: linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; > devicet...@vger.kernel.org; linux-arm-ker...@lists.infradead.org; Rajesh > Bhagat > Subject: RE: [PATCH v4 3/3] USB3/DWC3: Enable undefined length INCR burst > type > > > >> -- > > >> 1.7.9.5 > > > Hi, Balbi and all guys, > > > Any comment for these patches? Can they be accepted? > > > > Rob had comments which you didn't reply yet. I cannot take this > > patchset yet ;-) > > > Balbi, > I look into his mail again, which was based v3, and I replied it. > He had different understanding for undefined length burst mode. > It seems he think for this mode, just setting bit[0] (INCRBrstEna) and don't > need to set other field. > However, according to the DWC USB3.0 controller databook, when it is > undefined length INCR burst mode, we still need to set one max burst type, > such as INCR8, which means controller will use any length less than or equal > to this INCR8. Any comment for it? Ten days passed away again :)
RE: [PATCH v4 3/3] USB3/DWC3: Enable undefined length INCR burst type
> >> -- > >> 1.7.9.5 > > Hi, Balbi and all guys, > > Any comment for these patches? Can they be accepted? > > Rob had comments which you didn't reply yet. I cannot take this patchset > yet ;-) > Balbi, I look into his mail again, which was based v3, and I replied it. He had different understanding for undefined length burst mode. It seems he think for this mode, just setting bit[0] (INCRBrstEna) and don't need to set other field. However, according to the DWC USB3.0 controller databook, when it is undefined length INCR burst mode, we still need to set one max burst type, such as INCR8, which means controller will use any length less than or equal to this INCR8.
RE: [PATCH v3 2/3] USB3/DWC3: Add property "snps, incr-burst-type-adjustment" for INCR burst type
> >> >> 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? > > What I mean is: > - burst disabled Yes, I understand it. > 0 - INCR burst. INCR is undefined length burst IIRC. When INCR is undefined length, we need to set the max INCR type, too, such as when setting INCR16, controller can use any length less than or equal to the 16 byte. > 4/8/16/etc. - INCR4/INCR8/INCR16/etc. I understand it, too. > > What case does this not cover? According to Balbi's suggestion, I changed the property like below, you can see the detail in v4 (sent by 1/18/2017) Property "snps,incr-burst-type-adjustment = , ..." for USB3.0 DWC3. When Just one value means INCRx mode with fix burst type. When more than one value, means undefined length burst mode, USB controller can use the length less than or equal to the largest enabled burst length.
RE: [PATCH v4 3/3] USB3/DWC3: Enable undefined length INCR burst type
> -Original Message-
> From: Changming Huang [mailto:jerry.hu...@nxp.com]
> Sent: Wednesday, January 18, 2017 4:12 PM
> To: ba...@kernel.org; robh...@kernel.org; mark.rutl...@arm.com;
> catalin.mari...@arm.com
> Cc: linux-...@vger.kernel.org; linux-kernel@vger.kernel.org;
> devicet...@vger.kernel.org; linux-arm-ker...@lists.infradead.org; Jerry
> Huang ; Rajesh Bhagat
> Subject: [PATCH v4 3/3] USB3/DWC3: Enable undefined length INCR burst
> type
>
> Enable the undefined length INCR burst type and set INCRx.
> Different platform may has the different burst size type.
> In order to get best performance, we need to tune the burst size to one
> special value, instead of the default value.
>
> Signed-off-by: Changming Huang
> Signed-off-by: Rajesh Bhagat
> ---
> Changes in v4:
> - Modify the codes according to the definition of this property.
> Changes in v3:
> - add new property for INCR burst in usb node to reset GSBUSCFG0.
> Changes in v2:
> - split patch
> - create one new function to handle soc bus configuration register.
>
> drivers/usb/dwc3/core.c | 83
> +++
> drivers/usb/dwc3/core.h |7
> 2 files changed, 90 insertions(+)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index
> 369bab1..446aec3 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -650,6 +650,87 @@ static void dwc3_core_setup_global_control(struct
> dwc3 *dwc)
> dwc3_writel(dwc->regs, DWC3_GCTL, reg); }
>
> +/* set global soc bus configuration registers */ static void
> +dwc3_set_soc_bus_cfg(struct dwc3 *dwc) {
> + struct device *dev = dwc->dev;
> + u32 *vals;
> + u32 cfg;
> + int ntype;
> + int ret;
> + int i;
> +
> + cfg = dwc3_readl(dwc->regs, DWC3_GSBUSCFG0);
> +
> + /*
> + * Handle property "snps,incr-burst-type-adjustment".
> + * Get the number of value from this property:
> + * result <= 0, means this property is not supported.
> + * result = 1, means INCRx burst mode supported.
> + * result > 1, means undefined length burst mode supported.
> + */
> + ntype = device_property_read_u32_array(dev,
> + "snps,incr-burst-type-adjustment", NULL, 0);
> + if (ntype > 0) {
> + vals = kcalloc(ntype, sizeof(u32), GFP_KERNEL);
> + if (!vals) {
> + dev_err(dev, "Error to get memory\n");
> + return;
> + }
> + /* Get INCR burst type, and parse it */
> + ret = device_property_read_u32_array(dev,
> + "snps,incr-burst-type-adjustment", vals, ntype);
> + if (ret) {
> + dev_err(dev, "Error to get property\n");
> + return;
> + }
> + *(dwc->incrx_type + 1) = vals[0];
> + if (ntype > 1) {
> + *dwc->incrx_type = 1;
> + for (i = 1; i < ntype; i++) {
> + if (vals[i] > *(dwc->incrx_type + 1))
> + *(dwc->incrx_type + 1) = vals[i];
> + }
> + } else
> + *dwc->incrx_type = 0;
> +
> + /* Enable Undefined Length INCR Burst and Enable INCRx
> Burst */
> + cfg &= ~DWC3_GSBUSCFG0_INCRBRST_MASK;
> + if (*dwc->incrx_type)
> + cfg |= DWC3_GSBUSCFG0_INCRBRSTENA;
> + switch (*(dwc->incrx_type + 1)) {
> + case 256:
> + cfg |= DWC3_GSBUSCFG0_INCR256BRSTENA;
> + break;
> + case 128:
> + cfg |= DWC3_GSBUSCFG0_INCR128BRSTENA;
> + break;
> + case 64:
> + cfg |= DWC3_GSBUSCFG0_INCR64BRSTENA;
> + break;
> + case 32:
> + cfg |= DWC3_GSBUSCFG0_INCR32BRSTENA;
> + break;
> + case 16:
> + cfg |= DWC3_GSBUSCFG0_INCR16BRSTENA;
> + break;
> + case 8:
> + cfg |= DWC3_GSBUSCFG0_INCR8BRSTENA;
> + break;
> + case 4:
> + cfg |= DWC3_GSBUSCFG0_INCR4BRSTENA;
> + break;
> + case 1:
> + break;
> + default:
> + dev_err(dev, "Invalid property\n");
> +
RE: [PATCH v3 2/3] USB3/DWC3: Add property "snps, incr-burst-type-adjustment" for INCR burst type
> -Original Message- > From: Felipe Balbi [mailto:ba...@kernel.org] > Sent: Tuesday, January 17, 2017 6:45 PM > To: Jerry Huang ; Rob Herring > Cc: mark.rutl...@arm.com; catalin.mari...@arm.com; > will.dea...@arm.com; li...@armlinux.org.uk; devicet...@vger.kernel.org; > linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm- > ker...@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 writes: > > > > >> >> 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: 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? > > no, I don't think two values are needed, because first value can be > extrapolated from the second. Something like this: > > snps,incr-burst-type-adjustment = <4>, <8>, <16>, <32>; > > This is basically telling us that we can support anything in this list. So > INCRBrstEna should be set to 1. > > If DT, on the other hand, says: > > snps,incr-burst-type-adjustment = <32>; > > this means that we can only support INCR32, so INCRBrstEna should be > cleared to 0. Got it, I will try this mode. Thanks a lot, Balbi,
RE: [PATCH v3 2/3] USB3/DWC3: Add property "snps, incr-burst-type-adjustment" for INCR burst type
> -Original Message- > From: Felipe Balbi [mailto:ba...@kernel.org] > Sent: Monday, January 16, 2017 4:50 PM > To: Jerry Huang ; Rob Herring > Cc: mark.rutl...@arm.com; catalin.mari...@arm.com; > will.dea...@arm.com; li...@armlinux.org.uk; devicet...@vger.kernel.org; > linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm- > ker...@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 writes: > >> > On Thu, Dec 22, 2016 at 8:52 PM, Jerry Huang > >> wrote: > >> > > Hi, Rob, > >> > >> -Original Message- > >> > >> From: Rob Herring [mailto:r...@kernel.org] > >> > >> Sent: Friday, December 23, 2016 2:45 AM > >> > >> To: Jerry Huang > >> > >> Cc: ba...@kernel.org; mark.rutl...@arm.com; > >> > >> catalin.mari...@arm.com; will.dea...@arm.com; > >> > >> li...@armlinux.org.uk; devicet...@vger.kernel.org; > >> > >> linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; > >> > >> linux-arm- ker...@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 = , " 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 > >> > >> > --- > >> > >> > 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. > >> > >> > > >> > >> > - 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
RE: [PATCH v3 2/3] USB3/DWC3: Add property "snps, incr-burst-type-adjustment" for INCR burst type
> -Original Message- > From: Jerry Huang > Sent: Wednesday, January 04, 2017 10:25 AM > To: 'Rob Herring' > Cc: ba...@kernel.org; mark.rutl...@arm.com; catalin.mari...@arm.com; > will.dea...@arm.com; li...@armlinux.org.uk; devicet...@vger.kernel.org; > linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm- > ker...@lists.infradead.org > 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:r...@kernel.org] > > Sent: Wednesday, January 04, 2017 5:24 AM > > To: Jerry Huang > > Cc: ba...@kernel.org; mark.rutl...@arm.com; catalin.mari...@arm.com; > > will.dea...@arm.com; li...@armlinux.org.uk; > > devicet...@vger.kernel.org; linux-...@vger.kernel.org; > > linux-kernel@vger.kernel.org; linux-arm- ker...@lists.infradead.org > > 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 > wrote: > > > Hi, Rob, > > >> -Original Message- > > >> From: Rob Herring [mailto:r...@kernel.org] > > >> Sent: Friday, December 23, 2016 2:45 AM > > >> To: Jerry Huang > > >> Cc: ba...@kernel.org; mark.rutl...@arm.com; > > >> catalin.mari...@arm.com; will.dea...@arm.com; > > >> li...@armlinux.org.uk; devicet...@vger.kernel.org; > > >> linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm- > > >> ker...@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 = , " 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 > > >> > --- > > >> > 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. > > >> > > > >> > - 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
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:r...@kernel.org] > Sent: Wednesday, January 04, 2017 5:24 AM > To: Jerry Huang > Cc: ba...@kernel.org; mark.rutl...@arm.com; catalin.mari...@arm.com; > will.dea...@arm.com; li...@armlinux.org.uk; devicet...@vger.kernel.org; > linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm- > ker...@lists.infradead.org > 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 wrote: > > Hi, Rob, > >> -Original Message- > >> From: Rob Herring [mailto:r...@kernel.org] > >> Sent: Friday, December 23, 2016 2:45 AM > >> To: Jerry Huang > >> Cc: ba...@kernel.org; mark.rutl...@arm.com; catalin.mari...@arm.com; > >> will.dea...@arm.com; li...@armlinux.org.uk; > >> devicet...@vger.kernel.org; linux-...@vger.kernel.org; > >> linux-kernel@vger.kernel.org; linux-arm- ker...@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 = , " 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 > >> > --- > >> > 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. > >> > > >> > - 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.
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:r...@kernel.org] > Sent: Friday, December 23, 2016 2:45 AM > To: Jerry Huang > Cc: ba...@kernel.org; mark.rutl...@arm.com; catalin.mari...@arm.com; > will.dea...@arm.com; li...@armlinux.org.uk; devicet...@vger.kernel.org; > linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm- > ker...@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 = , " 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 > > --- > > 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. > > > > - 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.
RE: [PATCH] USB3/DWC3: Enable undefined length INCR burst type
Hi, Balbi, > -Original Message- > From: Felipe Balbi [mailto:ba...@kernel.org] > Sent: Saturday, December 17, 2016 1:02 AM > To: Jerry Huang ; gre...@linuxfoundation.org > Cc: linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; Rajesh Bhagat > > Subject: RE: [PATCH] USB3/DWC3: Enable undefined length INCR burst type > > > Hi, > > Jerry Huang writes: > > Hi, Balbi, > >> -Original Message- > >> From: Felipe Balbi [mailto:ba...@kernel.org] > >> Sent: Friday, December 16, 2016 7:44 PM > >> To: Jerry Huang ; gre...@linuxfoundation.org > >> Cc: linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; Rajesh > >> Bhagat > >> Subject: RE: [PATCH] USB3/DWC3: Enable undefined length INCR burst > >> type > >> > >> > >> Hi, > >> > >> Jerry Huang 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 = , " 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/300.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 = 0x0700 > >> > >> 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 majord...@vger.kernel.org More > majordomo > > info at http://vger.kernel.org/majordomo-info.html > > -- > balbi
RE: [PATCH] USB3/DWC3: Enable undefined length INCR burst type
Hi, Balbi, > -Original Message- > From: Felipe Balbi [mailto:ba...@kernel.org] > Sent: Friday, December 16, 2016 7:44 PM > To: Jerry Huang ; gre...@linuxfoundation.org > Cc: linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; Rajesh Bhagat > > Subject: RE: [PATCH] USB3/DWC3: Enable undefined length INCR burst type > > > Hi, > > Jerry Huang 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. > >> 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/300.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. 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. > > GSBUSCFG1 = 0x0700 > > 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
RE: [PATCH] USB3/DWC3: Enable undefined length INCR burst type
> -Original Message- > From: Felipe Balbi [mailto:ba...@kernel.org] > Sent: Friday, December 16, 2016 5:17 PM > To: Jerry Huang ; Jerry Huang > ; gre...@linuxfoundation.org > Cc: linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; Rajesh Bhagat > > Subject: RE: [PATCH] USB3/DWC3: Enable undefined length INCR burst type > > > Hi, > > Jerry Huang writes: > >> -Original Message- > >> From: Changming Huang [mailto:jerry.hu...@nxp.com] > >> Sent: Tuesday, December 13, 2016 5:06 PM > >> To: ba...@kernel.org; gre...@linuxfoundation.org > >> Cc: linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; Jerry > >> Huang ; Rajesh Bhagat > >> Subject: [PATCH] USB3/DWC3: Enable undefined length INCR 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 > >> Signed-off-by: Rajesh Bhagat > >> --- > >> drivers/usb/dwc3/core.c |6 ++ > >> drivers/usb/dwc3/core.h | 13 + > >> 2 files changed, 19 insertions(+) > >> > >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index > >> fea4469..0e11891 100644 > >> --- a/drivers/usb/dwc3/core.c > >> +++ b/drivers/usb/dwc3/core.c > >> @@ -621,6 +621,12 @@ static int dwc3_core_init(struct dwc3 *dwc) > >>goto err0; > >>} > >> > >> + /* Enable Undefined Length INCR Burst Type and Enable INCR16 > >> Burst */ > >> + reg = dwc3_readl(dwc->regs, DWC3_GSBUSCFG0); > >> + reg &= ~DWC3_GSBUSCFG0_INCRBRSTMASK; > >> + reg |= DWC3_GSBUSCFG0_INCR16BRSTENA | > >> DWC3_GSBUSCFG0_INCRBRSTENA; > >> + dwc3_writel(dwc->regs, DWC3_GSBUSCFG0, reg); > >> + > >>/* > >> * Write Linux Version Code to our GUID register so it's easy to figure > >> * out which kernel version a bug was found. > >> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index > >> 6b60e42..8bfdb77 100644 > >> --- a/drivers/usb/dwc3/core.h > >> +++ b/drivers/usb/dwc3/core.h > >> @@ -156,6 +156,19 @@ > >> > >> /* Bit fields */ > >> > >> +/* Global SoC Bus Configuration Register 0 */ > >> +#define DWC3_GSBUSCFG0_DATABIGEND (1 << 11) > >> +#define DWC3_GSBUSCFG0_DESCBIGEND (1 << 10) > >> +#define DWC3_GSBUSCFG0_INCR256BRSTENA (1 << 7) > >> +#define DWC3_GSBUSCFG0_INCR128BRSTENA (1 << 6) > >> +#define DWC3_GSBUSCFG0_INCR64BRSTENA (1 << 5) > >> +#define DWC3_GSBUSCFG0_INCR32BRSTENA (1 << 4) > >> +#define DWC3_GSBUSCFG0_INCR16BRSTENA (1 << 3) > >> +#define DWC3_GSBUSCFG0_INCR8BRSTENA (1 << 2) > >> +#define DWC3_GSBUSCFG0_INCR4BRSTENA (1 << 1) > >> +#define DWC3_GSBUSCFG0_INCRBRSTENA(1 << 0) > >> +#define DWC3_GSBUSCFG0_INCRBRSTMASK 0xff > >> + > >> /* Global Debug Queue/FIFO Space Available Register */ > >> #define DWC3_GDBGFIFOSPACE_NUM(n) ((n) & 0x1f) > >> #define DWC3_GDBGFIFOSPACE_TYPE(n)(((n) << 5) & 0x1e0) > >> -- > > I will split this patch to two, one is for the performance tune, the > > other for macro definition in header file. > > 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? > 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/300.usb3# cat regdump GSBUSCFG0 = 0x00100080 GSBUSCFG1 = 0x0700 GTXTHRCFG = 0x GRXTHRCFG = 0x GCTL = 0x30c11004 GEVTEN = 0x GSTS = 0x3e81 GUCTL1 = 0x018a GSNPSID = 0x5533280a GGPIO = 0x GUID = 0x00040900 GUCTL = 0x02008010 GBUSERRADDR0 = 0x GBUSERRADDR1 = 0x GPRTBIMAP0 = 0x GPRTBIMAP1 = 0x GHWPARAMS0 = 0x4020400a GHWPARAMS1 = 0x81e0c93b GHWPARAMS2 = 0x0130280a GHWPARAMS3 = 0x04108485 GHWPARAMS4 = 0x47822004 GHWPARAMS5 = 0x04204108 GHWPARAMS6 = 0x09049c20 GHWPARAMS7 =
RE: [PATCH] USB3/DWC3: Enable undefined length INCR burst type
> -Original Message- > From: Changming Huang [mailto:jerry.hu...@nxp.com] > Sent: Tuesday, December 13, 2016 5:06 PM > To: ba...@kernel.org; gre...@linuxfoundation.org > Cc: linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; Jerry Huang > ; Rajesh Bhagat > Subject: [PATCH] USB3/DWC3: Enable undefined length INCR 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 > Signed-off-by: Rajesh Bhagat > --- > drivers/usb/dwc3/core.c |6 ++ > drivers/usb/dwc3/core.h | 13 + > 2 files changed, 19 insertions(+) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index > fea4469..0e11891 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -621,6 +621,12 @@ static int dwc3_core_init(struct dwc3 *dwc) > goto err0; > } > > + /* Enable Undefined Length INCR Burst Type and Enable INCR16 > Burst */ > + reg = dwc3_readl(dwc->regs, DWC3_GSBUSCFG0); > + reg &= ~DWC3_GSBUSCFG0_INCRBRSTMASK; > + reg |= DWC3_GSBUSCFG0_INCR16BRSTENA | > DWC3_GSBUSCFG0_INCRBRSTENA; > + dwc3_writel(dwc->regs, DWC3_GSBUSCFG0, reg); > + > /* >* Write Linux Version Code to our GUID register so it's easy to figure >* out which kernel version a bug was found. > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index > 6b60e42..8bfdb77 100644 > --- a/drivers/usb/dwc3/core.h > +++ b/drivers/usb/dwc3/core.h > @@ -156,6 +156,19 @@ > > /* Bit fields */ > > +/* Global SoC Bus Configuration Register 0 */ > +#define DWC3_GSBUSCFG0_DATABIGEND(1 << 11) > +#define DWC3_GSBUSCFG0_DESCBIGEND(1 << 10) > +#define DWC3_GSBUSCFG0_INCR256BRSTENA(1 << 7) > +#define DWC3_GSBUSCFG0_INCR128BRSTENA(1 << 6) > +#define DWC3_GSBUSCFG0_INCR64BRSTENA (1 << 5) > +#define DWC3_GSBUSCFG0_INCR32BRSTENA (1 << 4) > +#define DWC3_GSBUSCFG0_INCR16BRSTENA (1 << 3) > +#define DWC3_GSBUSCFG0_INCR8BRSTENA (1 << 2) > +#define DWC3_GSBUSCFG0_INCR4BRSTENA (1 << 1) > +#define DWC3_GSBUSCFG0_INCRBRSTENA (1 << 0) > +#define DWC3_GSBUSCFG0_INCRBRSTMASK 0xff > + > /* Global Debug Queue/FIFO Space Available Register */ > #define DWC3_GDBGFIFOSPACE_NUM(n)((n) & 0x1f) > #define DWC3_GDBGFIFOSPACE_TYPE(n) (((n) << 5) & 0x1e0) > -- I will split this patch to two, one is for the performance tune, the other for macro definition in header file.
RE: [PATCH] arm/dts: ls1021a: Add dma-coherent property to usb3 node
> -Original Message-
> From: Changming Huang [mailto:jerry.hu...@nxp.com]
> Sent: Wednesday, November 23, 2016 3:15 PM
> To: robh...@kernel.org; mark.rutl...@arm.com; li...@armlinux.org.uk
> Cc: devicet...@vger.kernel.org; linux-arm-ker...@lists.infradead.org; linux-
> ker...@vger.kernel.org; Jerry Huang ; Rajesh
> Bhagat
> Subject: [PATCH] arm/dts: ls1021a: Add dma-coherent property to usb3 node
>
> This sets dma ops as coherent for usb 3.0 platform device
>
> Signed-off-by: Changming Huang
> Signed-off-by: Rajesh Bhagat
> ---
> arch/arm/boot/dts/ls1021a.dtsi |1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm/boot/dts/ls1021a.dtsi b/arch/arm/boot/dts/ls1021a.dtsi
> index 368e219..81fb4d9 100644
> --- a/arch/arm/boot/dts/ls1021a.dtsi
> +++ b/arch/arm/boot/dts/ls1021a.dtsi
> @@ -627,6 +627,7 @@
> dr_mode = "host";
> snps,quirk-frame-length-adjustment = <0x20>;
> snps,dis_rxdet_inp3_quirk;
> + dma-coherent;
> };
>
> pcie@340 {
> --
How about this property for usb? Any comment for it?
RE: [PATCH v3] fsl/usb: Workarourd for USB erratum-A005697
Thanks a lot, Alan.
That is my fault, will fix it and send one new version.
Best Regards
Jerry Huang
-Original Message-
From: Alan Stern [mailto:st...@rowland.harvard.edu]
Sent: Monday, November 28, 2016 11:58 PM
To: Jerry Huang
Cc: gre...@linuxfoundation.org; Ramneek Mehresh ;
julia.law...@lip6.fr; Sriram Dash ;
linux-...@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] fsl/usb: Workarourd for USB erratum-A005697
On Mon, 28 Nov 2016, Changming Huang wrote:
> The EHCI specification states the following in the SUSP bit description:
> In the Suspend state, the port is sensitive to resume detection.
> Note that the bit status does not change until the port is suspended
> and that there may be a delay in suspending a port if there is a
> transaction currently in progress on the USB.
>
> However, in NXP USBDR controller, the PORTSCx[SUSP] bit changes
> immediately when the application sets it and not when the port is actually
> suspended.
>
> So the application must wait for at least 10 milliseconds after a port
> indicates that it is suspended, to make sure this port has entered
> suspended state before initiating this port resume using the Force
> Port Resume bit. This bit is for NXP controller, not EHCI compatible.
>
> Signed-off-by: Changming Huang
> Signed-off-by: Ramneek Mehresh
> ---
> Changes in v3:
> - add 10ms delay in function ehci_hub_control
> - fix typos
> Changes in v2:
> - move sleep out of spin-lock
> - add more comment for this workaround
> --- a/drivers/usb/host/ehci-hub.c
> +++ b/drivers/usb/host/ehci-hub.c
> @@ -310,6 +310,14 @@ static int ehci_bus_suspend (struct usb_hcd *hcd)
> }
> spin_unlock_irq(&ehci->lock);
>
> + if (changed && ehci_has_fsl_susp_errata(ehci))
> + /*
> + * Wait for at least 10 millisecondes to ensure the controller
> + * enter the suspend status before initiating a port resume
> + * using the Force Port Resume bit (Not-EHCI compatible).
> + */
> + usleep_range(1, 2);
> +
> if ((changed && ehci->has_tdi_phy_lpm) || fs_idle_delay) {
> /*
>* Wait for HCD to enter low-power mode or for the bus @@
> -1200,6
> +1208,9 @@ int ehci_hub_control(
> wIndex, (temp1 & HOSTPC_PHCD) ?
> "succeeded" : "failed");
> }
> + if (ehci_has_fsl_susp_errata(ehci))
> + /* 10ms for HCD enter suspend */
> + usleep_range(1, 2);
> set_bit(wIndex, &ehci->suspended_ports);
> break;
Just like before, you have to release the spinlock before sleeping.
Look at the code 10 lines above this.
Alan Stern
RE: [PATCH v2] fsl/usb: Workarourd for USB erratum-A005697
Thanks a lot, Alan,
I will send the v3 with your suggestion.
Best Regards
Jerry Huang
-Original Message-
From: Alan Stern [mailto:st...@rowland.harvard.edu]
Sent: Friday, November 25, 2016 11:14 PM
To: Jerry Huang
Cc: gre...@linuxfoundation.org; Ramneek Mehresh ;
julia.law...@lip6.fr; Sriram Dash ;
linux-...@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] fsl/usb: Workarourd for USB erratum-A005697
On Fri, 25 Nov 2016, Changming Huang wrote:
> The EHCI specification states the following in the SUSP bit description:
> In the Suspend state, the port is senstive to resume detection.
> Note that the bit status does not change untile the port is suspended
> and that there may be a delay in susupending a port if there is a
> transaction currently in progress on the USB.
>
> However, in NXP USBDR controller, the PORTSCx[SUSP] bit changes
> immediately when the application sets it and not when the port is actually
> suspended.
>
> So the application must wait for at least 10 milliseconds after a port
> indicates that it is suspended, to make sure this port has entered
> suspended state before initiating this port resume using the Force
> Port Resume bit. This bit is for NXP controller, not EHCI compatible.
>
> Signed-off-by: Changming Huang
> Signed-off-by: Ramneek Mehresh
> ---
> Change in v2:
> - move sleep out of spin-lock and add more comment for this workaround
This patch is incomplete.
> --- a/drivers/usb/host/ehci-hub.c
> +++ b/drivers/usb/host/ehci-hub.c
> @@ -310,6 +310,13 @@ static int ehci_bus_suspend (struct usb_hcd *hcd)
> }
> spin_unlock_irq(&ehci->lock);
>
> + if (changed && ehci_has_fsl_susp_errata(ehci))
> + /* Wait for at least 10 millisecondes to ensure the controller
> + * enter the suspend status before initiating a port resume
> + * using the Fore Port Resume bit (Not-EHCI compatible).
> + */
The proper style for multi-line comments is:
/*
* Wait for ...
* ...
*/
Also, "Force" is misspelled.
> + usleep_range(1, 2);
> +
> if ((changed && ehci->has_tdi_phy_lpm) || fs_idle_delay) {
> /*
>* Wait for HCD to enter low-power mode or for the bus
The patch does not change the code around line 1190 in the original
file:
/* After above check the port must be connected.
* Set appropriate bit thus could put phy into low power
* mode if we have tdi_phy_lpm feature
*/
temp &= ~PORT_WKCONN_E;
temp |= PORT_WKDISC_E | PORT_WKOC_E;
ehci_writel(ehci, temp | PORT_SUSPEND, status_reg);
This code sets the PORT_SUSPEND bit but does not have a 10-ms delay.
You need to add a delay here.
Alan Stern
RE: [PATCH v2] fsl/usb: Workarourd for USB erratum-A005697
Thanks a lot for you look into this patch, I will fixed these typos.
Best Regards
Jerry Huang
-Original Message-
From: Sergei Shtylyov [mailto:sergei.shtyl...@cogentembedded.com]
Sent: Friday, November 25, 2016 9:12 PM
To: Jerry Huang ; st...@rowland.harvard.edu;
gre...@linuxfoundation.org
Cc: Ramneek Mehresh ; julia.law...@lip6.fr; Sriram
Dash ; linux-...@vger.kernel.org;
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] fsl/usb: Workarourd for USB erratum-A005697
Hello.
On 11/25/2016 06:24 AM, Changming Huang wrote:
> The EHCI specification states the following in the SUSP bit description:
> In the Suspend state, the port is senstive to resume detection.
Sensitive.
> Note that the bit status does not change untile the port is suspended
> and
Until.
> that there may be a delay in susupending a port if there is a
> transaction
Suspending.
> currently in progress on the USB.
>
> However, in NXP USBDR controller, the PORTSCx[SUSP] bit changes
> immediately when the application sets it and not when the port is actually
> suspended.
>
> So the application must wait for at least 10 milliseconds after a port
> indicates that it is suspended, to make sure this port has entered
> suspended state before initiating this port resume using the Force
> Port Resume bit. This bit is for NXP controller, not EHCI compatible.
>
> Signed-off-by: Changming Huang
> Signed-off-by: Ramneek Mehresh
[...]
> diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
> index 74f62d6..81e2310 100644
> --- a/drivers/usb/host/ehci-hub.c
> +++ b/drivers/usb/host/ehci-hub.c
> @@ -310,6 +310,13 @@ static int ehci_bus_suspend (struct usb_hcd *hcd)
> }
> spin_unlock_irq(&ehci->lock);
>
> + if (changed && ehci_has_fsl_susp_errata(ehci))
> + /* Wait for at least 10 millisecondes to ensure the controller
Milliseconds.
> + * enter the suspend status before initiating a port resume
> + * using the Fore Port Resume bit (Not-EHCI compatible).
Maybe force?
s/Not/non/ also.
> + */
> + usleep_range(1, 2);
> +
> if ((changed && ehci->has_tdi_phy_lpm) || fs_idle_delay) {
> /*
>* Wait for HCD to enter low-power mode or for the bus
[...]
> @@ -703,10 +704,15 @@ struct ehci_tt { #if defined(CONFIG_PPC_85xx)
> /* Some Freescale processors have an erratum (USB A-005275) in which
> * incoming packets get corrupted in HS mode
> + * Some Freescale processors have an erratum (USB A-005697) in which
> + * we need to wait for 10ms for bus to fo into suspend mode after
Fo?
[...]
MBR, Sergei
RE: [PATCH] fsl/usb: Workarourd for USB erratum-A005697
Hi, Alan, Maybe other USB controller does not need this delay. However, our silicon errata point out, in the USBDR controller, the PORTCx[SUSP] bit changes immediately when the application sets it and not when the port is actually suspended, so the application must wait for at least 10 milliseconds after a port indicates that it is suspended to ensure the port has entered SUSPEND status before initiating this port resume using the Force Port Resume (which is one bit for NXP silicon, not-EHCI compatible). I will add more comment to understand why we need this delay in next version. Best Regards Jerry Huang -Original Message- From: Alan Stern [mailto:st...@rowland.harvard.edu] Sent: Friday, November 25, 2016 12:54 AM To: Sriram Dash Cc: Jerry Huang ; gre...@linuxfoundation.org; linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; julia.law...@lip6.fr; Ramneek Mehresh ; Suresh Gupta Subject: RE: [PATCH] fsl/usb: Workarourd for USB erratum-A005697 On Thu, 24 Nov 2016, Sriram Dash wrote: > >From: Changming Huang [mailto:jerry.hu...@nxp.com] As per USB > >specification, in the Suspend state, the status bit does not change > >until the port is suspended. However, there may be a delay in > >suspending a port if there is a transaction currently in progress on the bus. > > > >In the USBDR controller, the PORTSCx[SUSP] bit changes immediately > >when the application sets it and not when the port is actually > >suspended > > > >Workaround for this issue involves waiting for a minimum of 10ms to > >allow the controller to go into SUSPEND state before proceeding ahead The USB core guarantees that there won't be any data transactions in progress when a root hub is suspended. There might possibly be an SOF transaction, but that doesn't take more than a few microseconds at most. Certainly nowhere near 10 ms! Given that we already perform a 150-us delay, is this change really needed? Alan Stern
RE: [PATCH] fsl/usb: Workarourd for USB erratum-A005697
Thanks, Sriram,
It is better to move this delay out of spin-lock.
Best Regards
Jerry Huang
-Original Message-
From: Sriram Dash
Sent: Thursday, November 24, 2016 7:17 PM
To: Jerry Huang ; st...@rowland.harvard.edu;
gre...@linuxfoundation.org
Cc: linux-...@vger.kernel.org; linux-kernel@vger.kernel.org;
julia.law...@lip6.fr; Jerry Huang ; Ramneek Mehresh
; Suresh Gupta
Subject: RE: [PATCH] fsl/usb: Workarourd for USB erratum-A005697
>From: Changming Huang [mailto:jerry.hu...@nxp.com] As per USB
>specification, in the Suspend state, the status bit does not change
>until the port is suspended. However, there may be a delay in
>suspending a port if there is a transaction currently in progress on the bus.
>
>In the USBDR controller, the PORTSCx[SUSP] bit changes immediately when
>the application sets it and not when the port is actually suspended
>
>Workaround for this issue involves waiting for a minimum of 10ms to
>allow the controller to go into SUSPEND state before proceeding ahead
>
>Signed-off-by: Changming Huang
>Signed-off-by: Ramneek Mehresh
>---
> drivers/usb/host/ehci-fsl.c |3 +++
> drivers/usb/host/ehci-hub.c |2 ++
> drivers/usb/host/ehci.h |6 ++
> drivers/usb/host/fsl-mph-dr-of.c |2 ++
> include/linux/fsl_devices.h |1 +
> 5 files changed, 14 insertions(+)
>
>diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
>index 9f5ffb6..91701cc 100644
>--- a/drivers/usb/host/ehci-fsl.c
>+++ b/drivers/usb/host/ehci-fsl.c
>@@ -286,6 +286,9 @@ static int ehci_fsl_usb_setup(struct ehci_hcd *ehci)
> if (pdata->has_fsl_erratum_a005275 == 1)
> ehci->has_fsl_hs_errata = 1;
>
>+ if (pdata->has_fsl_erratum_a005697 == 1)
>+ ehci->has_fsl_susp_errata = 1;
>+
> if ((pdata->operating_mode == FSL_USB2_DR_HOST) ||
> (pdata->operating_mode == FSL_USB2_DR_OTG))
> if (ehci_fsl_setup_phy(hcd, pdata->phy_mode, 0)) diff --git
>a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c index
>74f62d6..86d154e 100644
>--- a/drivers/usb/host/ehci-hub.c
>+++ b/drivers/usb/host/ehci-hub.c
>@@ -305,6 +305,8 @@ static int ehci_bus_suspend (struct usb_hcd *hcd)
> USB_PORT_STAT_HIGH_SPEED)
> fs_idle_delay = true;
> ehci_writel(ehci, t2, reg);
>+ if (ehci_has_fsl_susp_errata(ehci))
>+ mdelay(10);
Hi Jerry,
Move the delay out of the spin lock. Other than that, it looks fine to me.
> changed = 1;
> }
> }
>diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h index
>3f3b74a..7706e4a 100644
>--- a/drivers/usb/host/ehci.h
>+++ b/drivers/usb/host/ehci.h
>@@ -219,6 +219,7 @@ struct ehci_hcd { /* one per controller */
> unsignedno_selective_suspend:1;
> unsignedhas_fsl_port_bug:1; /* FreeScale */
> unsignedhas_fsl_hs_errata:1;/* Freescale HS quirk */
>+ unsignedhas_fsl_susp_errata:1; /*Freescale SUSP quirk*/
> unsignedbig_endian_mmio:1;
> unsignedbig_endian_desc:1;
> unsignedbig_endian_capbase:1;
>@@ -703,10 +704,15 @@ struct ehci_tt {
> #if defined(CONFIG_PPC_85xx)
> /* Some Freescale processors have an erratum (USB A-005275) in which
> * incoming packets get corrupted in HS mode
>+ * Some Freescale processors have an erratum (USB A-005697) in which
>+ * we need to wait for 10ms for bus to fo into suspend mode after
>+ * setting SUSP bit
> */
> #define ehci_has_fsl_hs_errata(e) ((e)->has_fsl_hs_errata)
>+#define ehci_has_fsl_susp_errata(e) ((e)->has_fsl_susp_errata)
> #else
> #define ehci_has_fsl_hs_errata(e) (0)
>+#define ehci_has_fsl_susp_errata(e) (0)
> #endif
>
> /*
>diff --git a/drivers/usb/host/fsl-mph-dr-of.c
>b/drivers/usb/host/fsl-mph-dr-of.c
>index f07ccb2..e90ddb5 100644
>--- a/drivers/usb/host/fsl-mph-dr-of.c
>+++ b/drivers/usb/host/fsl-mph-dr-of.c
>@@ -226,6 +226,8 @@ static int fsl_usb2_mph_dr_of_probe(struct
>platform_device *ofdev)
> of_property_read_bool(np, "fsl,usb-erratum-a007792");
> pdata->has_fsl_erratum_a005275 =
> of_property_read_bool(np, "fsl,usb-erratum-a005275");
>+ pdata->has_fsl_erratum_a005697 =
>+ of_property_read_bool(np, "fsl,usb_erratum-a005697");
>
> /*
>* Determine whether phy_clk_valid needs to be checked diff --git
>a/include/linux/fsl_de
