[PATCH v3 1/1] usb: dwc2: Fix the write to W1C fields in HPRT register
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)
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;@@ -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;@@ -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)
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;@@ -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.
participants (3)
-
Chong, Teik Heng -
Marek Vasut -
teik.heng.chong＠intel.com