[PATCH 0/2] usb: dwc3: core: Enable GUCTL1 bit 10 for fixing crc error after resume

[PATCH 0/2] usb: dwc3: core: Enable GUCTL1 bit 10 for fixing crc error after resume

[Piyush Mehta]

This patch of the series does the following:
- Add a new DT "snps,enable_guctl1_resume_quirk" quirk
- Enable GUCTL1 bit 10 for fixing crc error after resume bug
  When this bit is set to '1', the ULPI opmode will be changed
  to 'normal' along with HS terminations after EOR.
  This option is to support certain legacy ULPI PHYs.

Piyush Mehta (2):
  dt-bindings: usb: snps,dwc3: Add 'snps,enable_guctl1_resume_quirk'
    quirk
  usb: dwc3: core: Enable GUCTL1 bit 10 for fixing crc error after
    resume bug

 .../devicetree/bindings/usb/snps,dwc3.yaml       |  6 ++++++
 drivers/usb/dwc3/core.c                          | 16 ++++++++++++++++
 drivers/usb/dwc3/core.h                          |  6 ++++++
 3 files changed, 28 insertions(+)

-- 
2.17.1

[PATCH 1/2] dt-bindings: usb: snps,dwc3: Add 'snps,enable_guctl1_resume_quirk' quirk

[Piyush Mehta]

Add a new DT quirk to dwc3 core to resolved issue of CRC failed error.
On the resume path, U3/U2 exit controller fails to send proper CRC
checksum in CRC5 field. As result Transaction Error is generated.
Enabling bit 10 of GUCTL1 will correct this problem.

When this bit is set to '1', the UTMI/ULPI opmode will be changed to
"normal" along with HS terminations after EOR.
This option is to support certain legacy UTMI/ULPI PHYs.

Signed-off-by: Piyush Mehta <piyush.mehta@xilinx.com>
---
 Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
index d41265ba8ce2..36fa87df57a9 100644
--- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
+++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
@@ -234,6 +234,12 @@ properties:
       avoid -EPROTO errors with usbhid on some devices (Hikey 970).
     type: boolean
 
+  snps,enable_guctl1_resume_quirk:
+    description:
+      Set if we enable quirk for fixing improper crc generation after resume
+      from suspend.
+    type: boolean
+
   snps,is-utmi-l1-suspend:
     description:
       True when DWC3 asserts output signal utmi_l1_suspend_n, false when
-- 
2.17.1

[PATCH 2/2] usb: dwc3: core: Enable GUCTL1 bit 10 for fixing crc error after resume bug

[Piyush Mehta]

When configured in HOST mode, after issuing U3/L2 exit controller fails
to send proper CRC checksum in CRC5 field. Because of this behavior
Transaction Error is generated, resulting in reset and re-enumeration of
usb device attached. Enabling chicken bit 10 of GUCTL1 will correct this
problem.

When this bit is set to '1', the UTMI/ULPI opmode will be changed to
"normal" along with HS terminations after EOR. This option is to support
certain legacy UTMI/ULPI PHYs.

Signed-off-by: Piyush Mehta <piyush.mehta@xilinx.com>
---
 drivers/usb/dwc3/core.c | 16 ++++++++++++++++
 drivers/usb/dwc3/core.h |  6 ++++++
 2 files changed, 22 insertions(+)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index e027c0420dc3..8afc025390d2 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1140,6 +1140,20 @@ static int dwc3_core_init(struct dwc3 *dwc)
 		dwc3_writel(dwc->regs, DWC3_GUCTL2, reg);
 	}
 
+	/*
+	 * When configured in HOST mode, after issuing U3/L2 exit controller
+	 * fails to send proper CRC checksum in CRC5 feild. Because of this
+	 * behaviour Transaction Error is generated, resulting in reset and
+	 * re-enumeration of usb device attached. Enabling bit 10 of GUCTL1
+	 * will correct this problem. This option is to support certain
+	 * legacy ULPI PHYs.
+	 */
+	if (dwc->enable_guctl1_resume_quirk) {
+		reg = dwc3_readl(dwc->regs, DWC3_GUCTL1);
+		reg |= DWC3_GUCTL1_RESUME_QUIRK;
+		dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
+	}
+
 	if (!DWC3_VER_IS_PRIOR(DWC3, 250A)) {
 		reg = dwc3_readl(dwc->regs, DWC3_GUCTL1);
 
@@ -1483,6 +1497,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
 				"snps,dis-del-phy-power-chg-quirk");
 	dwc->dis_tx_ipgap_linecheck_quirk = device_property_read_bool(dev,
 				"snps,dis-tx-ipgap-linecheck-quirk");
+	dwc->enable_guctl1_resume_quirk = device_property_read_bool(dev,
+				"snps,enable_guctl1_resume_quirk");
 	dwc->parkmode_disable_ss_quirk = device_property_read_bool(dev,
 				"snps,parkmode-disable-ss-quirk");
 
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 81c486b3941c..e386209f0e1b 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -397,6 +397,9 @@
 #define DWC3_GUCTL_REFCLKPER_MASK		0xffc00000
 #define DWC3_GUCTL_REFCLKPER_SEL		22
 
+/* Global User Control Register 1 */
+#define DWC3_GUCTL1_RESUME_QUIRK		BIT(10)
+
 /* Global User Control Register 2 */
 #define DWC3_GUCTL2_RST_ACTBITLATER		BIT(14)
 
@@ -1093,6 +1096,8 @@ struct dwc3_scratchpad_array {
  *			change quirk.
  * @dis_tx_ipgap_linecheck_quirk: set if we disable u2mac linestate
  *			check during HS transmit.
+ * @enable_guctl1_resume_quirk: Set if we enable quirk for fixing improper crc
+ *			generation after resume from suspend.
  * @parkmode_disable_ss_quirk: set if we need to disable all SuperSpeed
  *			instances in park mode.
  * @tx_de_emphasis_quirk: set if we enable Tx de-emphasis quirk
@@ -1308,6 +1313,7 @@ struct dwc3 {
 	unsigned		dis_u2_freeclk_exists_quirk:1;
 	unsigned		dis_del_phy_power_chg_quirk:1;
 	unsigned		dis_tx_ipgap_linecheck_quirk:1;
+	unsigned		enable_guctl1_resume_quirk:1;
 	unsigned		parkmode_disable_ss_quirk:1;
 
 	unsigned		tx_de_emphasis_quirk:1;
-- 
2.17.1

Re: [PATCH 1/2] dt-bindings: usb: snps,dwc3: Add 'snps,enable_guctl1_resume_quirk' quirk

[Krzysztof Kozlowski]

On 13/06/2022 05:47, Piyush Mehta wrote:
> Add a new DT quirk to dwc3 core to resolved issue of CRC failed error.
> On the resume path, U3/U2 exit controller fails to send proper CRC
> checksum in CRC5 field. As result Transaction Error is generated.
> Enabling bit 10 of GUCTL1 will correct this problem.
> 
> When this bit is set to '1', the UTMI/ULPI opmode will be changed to
> "normal" along with HS terminations after EOR.
> This option is to support certain legacy UTMI/ULPI PHYs.
> 
> Signed-off-by: Piyush Mehta <piyush.mehta@xilinx.com>
> ---
>  Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> index d41265ba8ce2..36fa87df57a9 100644
> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> @@ -234,6 +234,12 @@ properties:
>        avoid -EPROTO errors with usbhid on some devices (Hikey 970).
>      type: boolean
>  
> +  snps,enable_guctl1_resume_quirk:

No underscores in properties, use hyphens.

> +    description:
> +      Set if we enable quirk for fixing improper crc generation after resume
> +      from suspend.

Please describe actual issue, hardware property, not driver behavior. In
the description and property name. This could be something like
"snps,missing-src-after-resume" (or anything better).

Best regards,
Krzysztof

Re: [PATCH 2/2] usb: dwc3: core: Enable GUCTL1 bit 10 for fixing crc error after resume bug

[Rob Herring]

On Mon, Jun 13, 2022 at 06:17:03PM +0530, Piyush Mehta wrote:
> When configured in HOST mode, after issuing U3/L2 exit controller fails
> to send proper CRC checksum in CRC5 field. Because of this behavior
> Transaction Error is generated, resulting in reset and re-enumeration of
> usb device attached. Enabling chicken bit 10 of GUCTL1 will correct this
> problem.
> 
> When this bit is set to '1', the UTMI/ULPI opmode will be changed to
> "normal" along with HS terminations after EOR. This option is to support
> certain legacy UTMI/ULPI PHYs.
> 
> Signed-off-by: Piyush Mehta <piyush.mehta@xilinx.com>
> ---
>  drivers/usb/dwc3/core.c | 16 ++++++++++++++++
>  drivers/usb/dwc3/core.h |  6 ++++++
>  2 files changed, 22 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index e027c0420dc3..8afc025390d2 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1140,6 +1140,20 @@ static int dwc3_core_init(struct dwc3 *dwc)
>  		dwc3_writel(dwc->regs, DWC3_GUCTL2, reg);
>  	}
>  
> +	/*
> +	 * When configured in HOST mode, after issuing U3/L2 exit controller
> +	 * fails to send proper CRC checksum in CRC5 feild. Because of this
> +	 * behaviour Transaction Error is generated, resulting in reset and
> +	 * re-enumeration of usb device attached. Enabling bit 10 of GUCTL1
> +	 * will correct this problem. This option is to support certain
> +	 * legacy ULPI PHYs.
> +	 */
> +	if (dwc->enable_guctl1_resume_quirk) {

What's the downside to just always doing this?

> +		reg = dwc3_readl(dwc->regs, DWC3_GUCTL1);
> +		reg |= DWC3_GUCTL1_RESUME_QUIRK;
> +		dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
> +	}
> +
>  	if (!DWC3_VER_IS_PRIOR(DWC3, 250A)) {
>  		reg = dwc3_readl(dwc->regs, DWC3_GUCTL1);
>  
> @@ -1483,6 +1497,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>  				"snps,dis-del-phy-power-chg-quirk");
>  	dwc->dis_tx_ipgap_linecheck_quirk = device_property_read_bool(dev,
>  				"snps,dis-tx-ipgap-linecheck-quirk");
> +	dwc->enable_guctl1_resume_quirk = device_property_read_bool(dev,
> +				"snps,enable_guctl1_resume_quirk");
>  	dwc->parkmode_disable_ss_quirk = device_property_read_bool(dev,
>  				"snps,parkmode-disable-ss-quirk");
>  
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 81c486b3941c..e386209f0e1b 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -397,6 +397,9 @@
>  #define DWC3_GUCTL_REFCLKPER_MASK		0xffc00000
>  #define DWC3_GUCTL_REFCLKPER_SEL		22
>  
> +/* Global User Control Register 1 */
> +#define DWC3_GUCTL1_RESUME_QUIRK		BIT(10)
> +
>  /* Global User Control Register 2 */
>  #define DWC3_GUCTL2_RST_ACTBITLATER		BIT(14)
>  
> @@ -1093,6 +1096,8 @@ struct dwc3_scratchpad_array {
>   *			change quirk.
>   * @dis_tx_ipgap_linecheck_quirk: set if we disable u2mac linestate
>   *			check during HS transmit.
> + * @enable_guctl1_resume_quirk: Set if we enable quirk for fixing improper crc
> + *			generation after resume from suspend.
>   * @parkmode_disable_ss_quirk: set if we need to disable all SuperSpeed
>   *			instances in park mode.
>   * @tx_de_emphasis_quirk: set if we enable Tx de-emphasis quirk
> @@ -1308,6 +1313,7 @@ struct dwc3 {
>  	unsigned		dis_u2_freeclk_exists_quirk:1;
>  	unsigned		dis_del_phy_power_chg_quirk:1;
>  	unsigned		dis_tx_ipgap_linecheck_quirk:1;
> +	unsigned		enable_guctl1_resume_quirk:1;
>  	unsigned		parkmode_disable_ss_quirk:1;
>  
>  	unsigned		tx_de_emphasis_quirk:1;
> -- 
> 2.17.1
> 
> 

Re: [PATCH 0/2] usb: dwc3: core: Enable GUCTL1 bit 10 for fixing crc error after resume

[Michael Grzeschik]

[-- Attachment #1: Type: text/plain, Size: 1731 bytes --]

Hi Piyush!

On Mon, Jun 13, 2022 at 06:17:01PM +0530, Piyush Mehta wrote:
>This patch of the series does the following:
>- Add a new DT "snps,enable_guctl1_resume_quirk" quirk
>- Enable GUCTL1 bit 10 for fixing crc error after resume bug
>  When this bit is set to '1', the ULPI opmode will be changed
>  to 'normal' along with HS terminations after EOR.
>  This option is to support certain legacy ULPI PHYs.
>
>Piyush Mehta (2):
>  dt-bindings: usb: snps,dwc3: Add 'snps,enable_guctl1_resume_quirk'
>    quirk
>  usb: dwc3: core: Enable GUCTL1 bit 10 for fixing crc error after
>    resume bug
>
> .../devicetree/bindings/usb/snps,dwc3.yaml       |  6 ++++++
> drivers/usb/dwc3/core.c                          | 16 ++++++++++++++++
> drivers/usb/dwc3/core.h                          |  6 ++++++
> 3 files changed, 28 insertions(+)

I found your series and am wondering if you are planning to send a v2 of
it? It would really help to see this mainline.

The Xilinx Register Reference states BIT 10 as

RESUME_TERMSEL_XCVRSEL_UNIFY

which seems to be more meaningful than GUCTL1_RESUME_QUIRK. It would
probably make sense to work this in for v2.

The Documentation is also refering more than just opmode to be 0
during EOR. (termsel, xcvrsel, opmode).

https://www.xilinx.com/htmldocs/registers/ug1087/ug1087-zynq-ultrascale-registers.html#usb3_xhci___guctl1.html

Regards,
Michael

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH 2/2] usb: dwc3: core: Enable GUCTL1 bit 10 for fixing crc error after resume bug

[Mehta, Piyush]

Hello @Rob Herring,

Please find my inline comments below with tag[Piyush]

Regards,
Piyush Mehta

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Saturday, June 18, 2022 4:18 AM
> To: Piyush Mehta <piyush.mehta@xilinx.com>
> Cc: gregkh@linuxfoundation.org; krzysztof.kozlowski+dt@linaro.org;
> balbi@kernel.org; linux-usb@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; michal.simek@xilinx.com; git@xilinx.com;
> sivadur@xilinx.com
> Subject: Re: [PATCH 2/2] usb: dwc3: core: Enable GUCTL1 bit 10 for fixing crc
> error after resume bug
> 
> CAUTION: This message has originated from an External Source. Please use
> proper judgment and caution when opening attachments, clicking links, or
> responding to this email.
> 
> 
> On Mon, Jun 13, 2022 at 06:17:03PM +0530, Piyush Mehta wrote:
> > When configured in HOST mode, after issuing U3/L2 exit controller
> > fails to send proper CRC checksum in CRC5 field. Because of this
> > behavior Transaction Error is generated, resulting in reset and
> > re-enumeration of usb device attached. Enabling chicken bit 10 of
> > GUCTL1 will correct this problem.
> >
> > When this bit is set to '1', the UTMI/ULPI opmode will be changed to
> > "normal" along with HS terminations after EOR. This option is to
> > support certain legacy UTMI/ULPI PHYs.
> >
> > Signed-off-by: Piyush Mehta <piyush.mehta@xilinx.com>
> > ---
> >  drivers/usb/dwc3/core.c | 16 ++++++++++++++++
> > drivers/usb/dwc3/core.h |  6 ++++++
> >  2 files changed, 22 insertions(+)
> >
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index
> > e027c0420dc3..8afc025390d2 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -1140,6 +1140,20 @@ static int dwc3_core_init(struct dwc3 *dwc)
> >               dwc3_writel(dwc->regs, DWC3_GUCTL2, reg);
> >       }
> >
> > +     /*
> > +      * When configured in HOST mode, after issuing U3/L2 exit controller
> > +      * fails to send proper CRC checksum in CRC5 feild. Because of this
> > +      * behaviour Transaction Error is generated, resulting in reset and
> > +      * re-enumeration of usb device attached. Enabling bit 10 of GUCTL1
> > +      * will correct this problem. This option is to support certain
> > +      * legacy ULPI PHYs.
> > +      */
> > +     if (dwc->enable_guctl1_resume_quirk) {
> 
> What's the downside to just always doing this?
[Piyush]

This bit is global user control register in host mode and is for USB 2.0 opmode behavior in HS Resume.
- When this bit is set to '1', the ULPI opmode will be changed to 'normal' along with HS terminations after EOR. This option is to support certain legacy ULPI PHYs.
- When this bit is set to '0', the ULPI opmode will be changed to 'normal' 2us after HS terminations change after EOR. This is the default behavior.

Normally this bit is set 0. If the customer PHY requires the opmode behavior other way, like changing it along with term/xcvr select signals, then they can set this bit to 1. Fyi , this is not based on any spec reference, rather just based on the PHY implementation by different vendors.

So as a conclusion, this bit is specific to phy requirement and as moreover related vendor specific.

> 
> > +             reg = dwc3_readl(dwc->regs, DWC3_GUCTL1);
> > +             reg |= DWC3_GUCTL1_RESUME_QUIRK;
> > +             dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
> > +     }
> > +
> >       if (!DWC3_VER_IS_PRIOR(DWC3, 250A)) {
> >               reg = dwc3_readl(dwc->regs, DWC3_GUCTL1);
> >
> > @@ -1483,6 +1497,8 @@ static void dwc3_get_properties(struct dwc3
> *dwc)
> >                               "snps,dis-del-phy-power-chg-quirk");
> >       dwc->dis_tx_ipgap_linecheck_quirk = device_property_read_bool(dev,
> >                               "snps,dis-tx-ipgap-linecheck-quirk");
> > +     dwc->enable_guctl1_resume_quirk = device_property_read_bool(dev,
> > +                             "snps,enable_guctl1_resume_quirk");
> >       dwc->parkmode_disable_ss_quirk = device_property_read_bool(dev,
> >                               "snps,parkmode-disable-ss-quirk");
> >
> > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index
> > 81c486b3941c..e386209f0e1b 100644
> > --- a/drivers/usb/dwc3/core.h
> > +++ b/drivers/usb/dwc3/core.h
> > @@ -397,6 +397,9 @@
> >  #define DWC3_GUCTL_REFCLKPER_MASK            0xffc00000
> >  #define DWC3_GUCTL_REFCLKPER_SEL             22
> >
> > +/* Global User Control Register 1 */
> > +#define DWC3_GUCTL1_RESUME_QUIRK             BIT(10)
> > +
> >  /* Global User Control Register 2 */
> >  #define DWC3_GUCTL2_RST_ACTBITLATER          BIT(14)
> >
> > @@ -1093,6 +1096,8 @@ struct dwc3_scratchpad_array {
> >   *                   change quirk.
> >   * @dis_tx_ipgap_linecheck_quirk: set if we disable u2mac linestate
> >   *                   check during HS transmit.
> > + * @enable_guctl1_resume_quirk: Set if we enable quirk for fixing
> improper crc
> > + *                   generation after resume from suspend.
> >   * @parkmode_disable_ss_quirk: set if we need to disable all SuperSpeed
> >   *                   instances in park mode.
> >   * @tx_de_emphasis_quirk: set if we enable Tx de-emphasis quirk @@
> > -1308,6 +1313,7 @@ struct dwc3 {
> >       unsigned                dis_u2_freeclk_exists_quirk:1;
> >       unsigned                dis_del_phy_power_chg_quirk:1;
> >       unsigned                dis_tx_ipgap_linecheck_quirk:1;
> > +     unsigned                enable_guctl1_resume_quirk:1;
> >       unsigned                parkmode_disable_ss_quirk:1;
> >
> >       unsigned                tx_de_emphasis_quirk:1;
> > --
> > 2.17.1
> >
> >