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@xxxxxxxxxx>
> Sent: Saturday, June 18, 2022 4:18 AM
> To: Piyush Mehta <piyush.mehta@xxxxxxxxxx>
> Cc: gregkh@xxxxxxxxxxxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx;
> balbi@xxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; michal.simek@xxxxxxxxxx; git@xxxxxxxxxx;
> sivadur@xxxxxxxxxx
> 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@xxxxxxxxxx>
> > ---
> >  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
> >
> >