| Message ID | 20220613124703.4493-1-piyush.mehta@xilinx.com |
|---|---|
| Headers | show |
| Series | usb: dwc3: core: Enable GUCTL1 bit 10 for fixing crc error after resume | expand |
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 > >
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
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 > > > >
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(+)