| Message ID | 20230621031358.20462-1-teik.heng.chong@intel.com |
|---|---|
| State | Accepted |
| Commit | 9c9454ac2e4ffd9a8b30744329029f1676d2e7be |
| Delegated to: | Marek Vasut |
| Headers | show |
| Series | [v3,1/1] usb: dwc2: Fix the write to W1C fields in HPRT register | expand |
On 6/21/23 05:13, teik.heng.chong@intel.com wrote: > From: Teik Heng Chong <teik.heng.chong@intel.com> > > Fix the write to the HPRT register which treat W1C fields > as if they were mere RW. This leads to unintended clearing of such fields > > This bug was found during the testing on Simics model. Referring to > specification DesignWare Cores USB 2.0 Hi-Speed On-The-Go (OTG) > Databook (3.30a)"5.3.4.8 Host Port Control and Status Register (HPRT)", the > HPRT.PrtPwr is cleared by this mistake. In the Linux driver (contrary to > U-Boot), HPRT is always read using dwc2_read_hprt0 helper function which > clears W1C bits. So after write back those bits are zeroes. > > Signed-off-by: Teik Heng Chong <teik.heng.chong@intel.com> > > --- > > V2->V3 > - update commit message > --- > drivers/usb/host/dwc2.c | 34 ++++++++-------------------------- > drivers/usb/host/dwc2.h | 4 ++++ > 2 files changed, 12 insertions(+), 26 deletions(-) > > diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c > index 23060fc369..9818f9be94 100644 > --- a/drivers/usb/host/dwc2.c > +++ b/drivers/usb/host/dwc2.c > @@ -315,9 +315,7 @@ static void dwc_otg_core_host_init(struct udevice *dev, > > /* Turn on the vbus power. */ > if (readl(®s->gintsts) & DWC2_GINTSTS_CURMODE_HOST) { > - hprt0 = readl(®s->hprt0); > - hprt0 &= ~(DWC2_HPRT0_PRTENA | DWC2_HPRT0_PRTCONNDET); > - hprt0 &= ~(DWC2_HPRT0_PRTENCHNG | DWC2_HPRT0_PRTOVRCURRCHNG); > + hprt0 = readl(®s->hprt0) & ~DWC2_HPRT0_W1C_MASK; > if (!(hprt0 & DWC2_HPRT0_PRTPWR)) { > hprt0 |= DWC2_HPRT0_PRTPWR; > writel(hprt0, ®s->hprt0); > @@ -748,7 +746,7 @@ static int dwc_otg_submit_rh_msg_out(struct dwc2_priv *priv, > case (USB_REQ_CLEAR_FEATURE << 8) | USB_RECIP_OTHER | USB_TYPE_CLASS: > switch (wValue) { > case USB_PORT_FEAT_C_CONNECTION: > - setbits_le32(®s->hprt0, DWC2_HPRT0_PRTCONNDET); > + clrsetbits_le32(®s->hprt0, DWC2_HPRT0_W1C_MASK, DWC2_HPRT0_PRTCONNDET); > break; > } > break; > @@ -759,21 +757,13 @@ static int dwc_otg_submit_rh_msg_out(struct dwc2_priv *priv, > break; > > case USB_PORT_FEAT_RESET: > - clrsetbits_le32(®s->hprt0, DWC2_HPRT0_PRTENA | > - DWC2_HPRT0_PRTCONNDET | > - DWC2_HPRT0_PRTENCHNG | > - DWC2_HPRT0_PRTOVRCURRCHNG, > - DWC2_HPRT0_PRTRST); > + clrsetbits_le32(®s->hprt0, DWC2_HPRT0_W1C_MASK, DWC2_HPRT0_PRTRST); > mdelay(50); > - clrbits_le32(®s->hprt0, DWC2_HPRT0_PRTRST); > + clrbits_le32(®s->hprt0, DWC2_HPRT0_W1C_MASK | DWC2_HPRT0_PRTRST); > break; > > case USB_PORT_FEAT_POWER: > - clrsetbits_le32(®s->hprt0, DWC2_HPRT0_PRTENA | > - DWC2_HPRT0_PRTCONNDET | > - DWC2_HPRT0_PRTENCHNG | > - DWC2_HPRT0_PRTOVRCURRCHNG, > - DWC2_HPRT0_PRTRST); > + clrsetbits_le32(®s->hprt0, DWC2_HPRT0_W1C_MASK, DWC2_HPRT0_PRTRST); > break; > > case USB_PORT_FEAT_ENABLE: > @@ -1213,14 +1203,9 @@ static int dwc2_init_common(struct udevice *dev, struct dwc2_priv *priv) > dwc_otg_core_host_init(dev, regs); > } > > - clrsetbits_le32(®s->hprt0, DWC2_HPRT0_PRTENA | > - DWC2_HPRT0_PRTCONNDET | DWC2_HPRT0_PRTENCHNG | > - DWC2_HPRT0_PRTOVRCURRCHNG, > - DWC2_HPRT0_PRTRST); > + clrsetbits_le32(®s->hprt0, DWC2_HPRT0_W1C_MASK, DWC2_HPRT0_PRTRST); > mdelay(50); > - clrbits_le32(®s->hprt0, DWC2_HPRT0_PRTENA | DWC2_HPRT0_PRTCONNDET | > - DWC2_HPRT0_PRTENCHNG | DWC2_HPRT0_PRTOVRCURRCHNG | > - DWC2_HPRT0_PRTRST); > + clrbits_le32(®s->hprt0, DWC2_HPRT0_W1C_MASK | DWC2_HPRT0_PRTRST); > > for (i = 0; i < MAX_DEVICE; i++) { > for (j = 0; j < MAX_ENDPOINT; j++) { > @@ -1246,10 +1231,7 @@ static int dwc2_init_common(struct udevice *dev, struct dwc2_priv *priv) > static void dwc2_uninit_common(struct dwc2_core_regs *regs) > { > /* Put everything in reset. */ > - clrsetbits_le32(®s->hprt0, DWC2_HPRT0_PRTENA | > - DWC2_HPRT0_PRTCONNDET | DWC2_HPRT0_PRTENCHNG | > - DWC2_HPRT0_PRTOVRCURRCHNG, > - DWC2_HPRT0_PRTRST); > + clrsetbits_le32(®s->hprt0, DWC2_HPRT0_W1C_MASK, DWC2_HPRT0_PRTRST); > } > > #if !CONFIG_IS_ENABLED(DM_USB) > diff --git a/drivers/usb/host/dwc2.h b/drivers/usb/host/dwc2.h > index a6f562fe60..6f022e33a1 100644 > --- a/drivers/usb/host/dwc2.h > +++ b/drivers/usb/host/dwc2.h > @@ -543,6 +543,10 @@ struct dwc2_core_regs { > #define DWC2_HPRT0_PRTSPD_LOW (2 << 17) > #define DWC2_HPRT0_PRTSPD_MASK (0x3 << 17) > #define DWC2_HPRT0_PRTSPD_OFFSET 17 > +#define DWC2_HPRT0_W1C_MASK (DWC2_HPRT0_PRTCONNDET | \ > + DWC2_HPRT0_PRTENA | \ > + DWC2_HPRT0_PRTENCHNG | \ > + DWC2_HPRT0_PRTOVRCURRCHNG) > #define DWC2_HAINT_CH0 (1 << 0) > #define DWC2_HAINT_CH0_OFFSET 0 > #define DWC2_HAINT_CH1 (1 << 1) Applied to usb/master, thanks.
> -----Original Message----- > From: Marek Vasut <marex@denx.de> > Sent: Wednesday, 21 June, 2023 7:16 PM > To: Chong, Teik Heng <teik.heng.chong@intel.com>; u-boot@lists.denx.de > Cc: Jagan Teki <jagan@amarulasolutions.com>; Vignesh R <vigneshr@ti.com>; > Simon <simon.k.r.goldschmidt@gmail.com>; Kris <kris.chaplin@intel.com>; > Chee, Tien Fong <tien.fong.chee@intel.com>; Hea, Kok Kiang > <kok.kiang.hea@intel.com>; Lokanathan, Raaj <raaj.lokanathan@intel.com>; > Maniyam, Dinesh <dinesh.maniyam@intel.com>; Ng, Boon Khai > <boon.khai.ng@intel.com>; Yuslaimi, Alif Zakuan > <alif.zakuan.yuslaimi@intel.com>; Zamri, Muhammad Hazim Izzat > <muhammad.hazim.izzat.zamri@intel.com>; Lim, Jit Loon > <jit.loon.lim@intel.com>; Tang, Sieu Mun <sieu.mun.tang@intel.com> > Subject: Re: [PATCH v3 1/1] usb: dwc2: Fix the write to W1C fields in HPRT > register > > On 6/21/23 05:13, teik.heng.chong@intel.com wrote: > > From: Teik Heng Chong <teik.heng.chong@intel.com> > > > > Fix the write to the HPRT register which treat W1C fields as if they > > were mere RW. This leads to unintended clearing of such fields > > > > This bug was found during the testing on Simics model. Referring to > > specification DesignWare Cores USB 2.0 Hi-Speed On-The-Go (OTG) > > Databook (3.30a)"5.3.4.8 Host Port Control and Status Register > > (HPRT)", the HPRT.PrtPwr is cleared by this mistake. In the Linux > > driver (contrary to U-Boot), HPRT is always read using dwc2_read_hprt0 > > helper function which clears W1C bits. So after write back those bits are > zeroes. > > > > Signed-off-by: Teik Heng Chong <teik.heng.chong@intel.com> > > > > --- > > > > V2->V3 > > - update commit message > > --- > > drivers/usb/host/dwc2.c | 34 ++++++++-------------------------- > > drivers/usb/host/dwc2.h | 4 ++++ > > 2 files changed, 12 insertions(+), 26 deletions(-) > > > > diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c index > > 23060fc369..9818f9be94 100644 > > --- a/drivers/usb/host/dwc2.c > > +++ b/drivers/usb/host/dwc2.c > > @@ -315,9 +315,7 @@ static void dwc_otg_core_host_init(struct udevice > > *dev, > > > > /* Turn on the vbus power. */ > > if (readl(®s->gintsts) & DWC2_GINTSTS_CURMODE_HOST) { > > - hprt0 = readl(®s->hprt0); > > - hprt0 &= ~(DWC2_HPRT0_PRTENA | > DWC2_HPRT0_PRTCONNDET); > > - hprt0 &= ~(DWC2_HPRT0_PRTENCHNG | > DWC2_HPRT0_PRTOVRCURRCHNG); > > + hprt0 = readl(®s->hprt0) & ~DWC2_HPRT0_W1C_MASK; > > if (!(hprt0 & DWC2_HPRT0_PRTPWR)) { > > hprt0 |= DWC2_HPRT0_PRTPWR; > > writel(hprt0, ®s->hprt0); > > @@ -748,7 +746,7 @@ static int dwc_otg_submit_rh_msg_out(struct > dwc2_priv *priv, > > case (USB_REQ_CLEAR_FEATURE << 8) | USB_RECIP_OTHER | > USB_TYPE_CLASS: > > switch (wValue) { > > case USB_PORT_FEAT_C_CONNECTION: > > - setbits_le32(®s->hprt0, > DWC2_HPRT0_PRTCONNDET); > > + clrsetbits_le32(®s->hprt0, > DWC2_HPRT0_W1C_MASK, > > +DWC2_HPRT0_PRTCONNDET); > > break; > > } > > break; > > @@ -759,21 +757,13 @@ static int dwc_otg_submit_rh_msg_out(struct > dwc2_priv *priv, > > break; > > > > case USB_PORT_FEAT_RESET: > > - clrsetbits_le32(®s->hprt0, DWC2_HPRT0_PRTENA | > > - DWC2_HPRT0_PRTCONNDET | > > - DWC2_HPRT0_PRTENCHNG | > > - DWC2_HPRT0_PRTOVRCURRCHNG, > > - DWC2_HPRT0_PRTRST); > > + clrsetbits_le32(®s->hprt0, > DWC2_HPRT0_W1C_MASK, > > +DWC2_HPRT0_PRTRST); > > mdelay(50); > > - clrbits_le32(®s->hprt0, DWC2_HPRT0_PRTRST); > > + clrbits_le32(®s->hprt0, DWC2_HPRT0_W1C_MASK | > > +DWC2_HPRT0_PRTRST); > > break; > > > > case USB_PORT_FEAT_POWER: > > - clrsetbits_le32(®s->hprt0, DWC2_HPRT0_PRTENA | > > - DWC2_HPRT0_PRTCONNDET | > > - DWC2_HPRT0_PRTENCHNG | > > - DWC2_HPRT0_PRTOVRCURRCHNG, > > - DWC2_HPRT0_PRTRST); > > + clrsetbits_le32(®s->hprt0, > DWC2_HPRT0_W1C_MASK, > > +DWC2_HPRT0_PRTRST); > > break; > > > > case USB_PORT_FEAT_ENABLE: > > @@ -1213,14 +1203,9 @@ static int dwc2_init_common(struct udevice *dev, > struct dwc2_priv *priv) > > dwc_otg_core_host_init(dev, regs); > > } > > > > - clrsetbits_le32(®s->hprt0, DWC2_HPRT0_PRTENA | > > - DWC2_HPRT0_PRTCONNDET | > DWC2_HPRT0_PRTENCHNG | > > - DWC2_HPRT0_PRTOVRCURRCHNG, > > - DWC2_HPRT0_PRTRST); > > + clrsetbits_le32(®s->hprt0, DWC2_HPRT0_W1C_MASK, > > +DWC2_HPRT0_PRTRST); > > mdelay(50); > > - clrbits_le32(®s->hprt0, DWC2_HPRT0_PRTENA | > DWC2_HPRT0_PRTCONNDET | > > - DWC2_HPRT0_PRTENCHNG | > DWC2_HPRT0_PRTOVRCURRCHNG | > > - DWC2_HPRT0_PRTRST); > > + clrbits_le32(®s->hprt0, DWC2_HPRT0_W1C_MASK | > DWC2_HPRT0_PRTRST); > > > > for (i = 0; i < MAX_DEVICE; i++) { > > for (j = 0; j < MAX_ENDPOINT; j++) { @@ -1246,10 +1231,7 @@ > static > > int dwc2_init_common(struct udevice *dev, struct dwc2_priv *priv) > > static void dwc2_uninit_common(struct dwc2_core_regs *regs) > > { > > /* Put everything in reset. */ > > - clrsetbits_le32(®s->hprt0, DWC2_HPRT0_PRTENA | > > - DWC2_HPRT0_PRTCONNDET | > DWC2_HPRT0_PRTENCHNG | > > - DWC2_HPRT0_PRTOVRCURRCHNG, > > - DWC2_HPRT0_PRTRST); > > + clrsetbits_le32(®s->hprt0, DWC2_HPRT0_W1C_MASK, > > +DWC2_HPRT0_PRTRST); > > } > > > > #if !CONFIG_IS_ENABLED(DM_USB) > > diff --git a/drivers/usb/host/dwc2.h b/drivers/usb/host/dwc2.h index > > a6f562fe60..6f022e33a1 100644 > > --- a/drivers/usb/host/dwc2.h > > +++ b/drivers/usb/host/dwc2.h > > @@ -543,6 +543,10 @@ struct dwc2_core_regs { > > #define DWC2_HPRT0_PRTSPD_LOW (2 << 17) > > #define DWC2_HPRT0_PRTSPD_MASK (0x3 > << 17) > > #define DWC2_HPRT0_PRTSPD_OFFSET 17 > > +#define DWC2_HPRT0_W1C_MASK > (DWC2_HPRT0_PRTCONNDET | \ > > + > DWC2_HPRT0_PRTENA | \ > > + > DWC2_HPRT0_PRTENCHNG | \ > > + > DWC2_HPRT0_PRTOVRCURRCHNG) > > #define DWC2_HAINT_CH0 (1 << 0) > > #define DWC2_HAINT_CH0_OFFSET 0 > > #define DWC2_HAINT_CH1 (1 << 1) > > Applied to usb/master, thanks. Thank you.
diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c index 23060fc369..9818f9be94 100644 --- a/drivers/usb/host/dwc2.c +++ b/drivers/usb/host/dwc2.c @@ -315,9 +315,7 @@ static void dwc_otg_core_host_init(struct udevice *dev, /* Turn on the vbus power. */ if (readl(®s->gintsts) & DWC2_GINTSTS_CURMODE_HOST) { - hprt0 = readl(®s->hprt0); - hprt0 &= ~(DWC2_HPRT0_PRTENA | DWC2_HPRT0_PRTCONNDET); - hprt0 &= ~(DWC2_HPRT0_PRTENCHNG | DWC2_HPRT0_PRTOVRCURRCHNG); + hprt0 = readl(®s->hprt0) & ~DWC2_HPRT0_W1C_MASK; if (!(hprt0 & DWC2_HPRT0_PRTPWR)) { hprt0 |= DWC2_HPRT0_PRTPWR; writel(hprt0, ®s->hprt0); @@ -748,7 +746,7 @@ static int dwc_otg_submit_rh_msg_out(struct dwc2_priv *priv, case (USB_REQ_CLEAR_FEATURE << 8) | USB_RECIP_OTHER | USB_TYPE_CLASS: switch (wValue) { case USB_PORT_FEAT_C_CONNECTION: - setbits_le32(®s->hprt0, DWC2_HPRT0_PRTCONNDET); + clrsetbits_le32(®s->hprt0, DWC2_HPRT0_W1C_MASK, DWC2_HPRT0_PRTCONNDET); break; } break; @@ -759,21 +757,13 @@ static int dwc_otg_submit_rh_msg_out(struct dwc2_priv *priv, break; case USB_PORT_FEAT_RESET: - clrsetbits_le32(®s->hprt0, DWC2_HPRT0_PRTENA | - DWC2_HPRT0_PRTCONNDET | - DWC2_HPRT0_PRTENCHNG | - DWC2_HPRT0_PRTOVRCURRCHNG, - DWC2_HPRT0_PRTRST); + clrsetbits_le32(®s->hprt0, DWC2_HPRT0_W1C_MASK, DWC2_HPRT0_PRTRST); mdelay(50); - clrbits_le32(®s->hprt0, DWC2_HPRT0_PRTRST); + clrbits_le32(®s->hprt0, DWC2_HPRT0_W1C_MASK | DWC2_HPRT0_PRTRST); break; case USB_PORT_FEAT_POWER: - clrsetbits_le32(®s->hprt0, DWC2_HPRT0_PRTENA | - DWC2_HPRT0_PRTCONNDET | - DWC2_HPRT0_PRTENCHNG | - DWC2_HPRT0_PRTOVRCURRCHNG, - DWC2_HPRT0_PRTRST); + clrsetbits_le32(®s->hprt0, DWC2_HPRT0_W1C_MASK, DWC2_HPRT0_PRTRST); break; case USB_PORT_FEAT_ENABLE: @@ -1213,14 +1203,9 @@ static int dwc2_init_common(struct udevice *dev, struct dwc2_priv *priv) dwc_otg_core_host_init(dev, regs); } - clrsetbits_le32(®s->hprt0, DWC2_HPRT0_PRTENA | - DWC2_HPRT0_PRTCONNDET | DWC2_HPRT0_PRTENCHNG | - DWC2_HPRT0_PRTOVRCURRCHNG, - DWC2_HPRT0_PRTRST); + clrsetbits_le32(®s->hprt0, DWC2_HPRT0_W1C_MASK, DWC2_HPRT0_PRTRST); mdelay(50); - clrbits_le32(®s->hprt0, DWC2_HPRT0_PRTENA | DWC2_HPRT0_PRTCONNDET | - DWC2_HPRT0_PRTENCHNG | DWC2_HPRT0_PRTOVRCURRCHNG | - DWC2_HPRT0_PRTRST); + clrbits_le32(®s->hprt0, DWC2_HPRT0_W1C_MASK | DWC2_HPRT0_PRTRST); for (i = 0; i < MAX_DEVICE; i++) { for (j = 0; j < MAX_ENDPOINT; j++) { @@ -1246,10 +1231,7 @@ static int dwc2_init_common(struct udevice *dev, struct dwc2_priv *priv) static void dwc2_uninit_common(struct dwc2_core_regs *regs) { /* Put everything in reset. */ - clrsetbits_le32(®s->hprt0, DWC2_HPRT0_PRTENA | - DWC2_HPRT0_PRTCONNDET | DWC2_HPRT0_PRTENCHNG | - DWC2_HPRT0_PRTOVRCURRCHNG, - DWC2_HPRT0_PRTRST); + clrsetbits_le32(®s->hprt0, DWC2_HPRT0_W1C_MASK, DWC2_HPRT0_PRTRST); } #if !CONFIG_IS_ENABLED(DM_USB) diff --git a/drivers/usb/host/dwc2.h b/drivers/usb/host/dwc2.h index a6f562fe60..6f022e33a1 100644 --- a/drivers/usb/host/dwc2.h +++ b/drivers/usb/host/dwc2.h @@ -543,6 +543,10 @@ struct dwc2_core_regs { #define DWC2_HPRT0_PRTSPD_LOW (2 << 17) #define DWC2_HPRT0_PRTSPD_MASK (0x3 << 17) #define DWC2_HPRT0_PRTSPD_OFFSET 17 +#define DWC2_HPRT0_W1C_MASK (DWC2_HPRT0_PRTCONNDET | \ + DWC2_HPRT0_PRTENA | \ + DWC2_HPRT0_PRTENCHNG | \ + DWC2_HPRT0_PRTOVRCURRCHNG) #define DWC2_HAINT_CH0 (1 << 0) #define DWC2_HAINT_CH0_OFFSET 0 #define DWC2_HAINT_CH1 (1 << 1)