[PATCH v2 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
Signed-off-by: Teik Heng Chong teik.heng.chong@intel.com
---
V1->V2 - update subject tags to usb:dwc2 --- 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/20/23 15:52, 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
Signed-off-by: Teik Heng Chong teik.heng.chong@intel.com
V1->V2
- update subject tags to usb:dwc2
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)
+CC ST
Is there an actual bug this is solving ? Details please ?
-----Original Message----- From: Marek Vasut marex@denx.de Sent: Wednesday, 21 June, 2023 5:38 AM 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; Patrice CHOTARD patrice.chotard@foss.st.com; Patrick DELAUNAY patrick.delaunay@foss.st.com Subject: Re: [PATCH v2 1/1] usb: dwc2: Fix the write to W1C fields in HPRT register
On 6/20/23 15:52, 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
Signed-off-by: Teik Heng Chong teik.heng.chong@intel.com
V1->V2
- update subject tags to usb:dwc2
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)
+CC ST
Is there an actual bug this is solving ? Details please ?
This bug was found during the testing on Simics model. We read the 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)" and verified what every write to HPRT is intended to do in U-Boot dw2 driver. Then, we realized HPRT.PrtPwr is cleared by this mistake. Furthermore, we compared U-Boot driver source code and the Linux driver source code (which handles HPRT correctly). In the Linux driver (contrary to U-Boot), HPRT is always read using dwc2_read_hprt0 helper function https://elixir.bootlin.com/linux/latest/source/drivers/usb/dwc2/hcd.h#L462 which clears W1C bits. So after write back those bits are zeroes.
On 6/21/23 02:57, Chong, Teik Heng wrote:
-----Original Message----- From: Marek Vasut marex@denx.de Sent: Wednesday, 21 June, 2023 5:38 AM 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; Patrice CHOTARD patrice.chotard@foss.st.com; Patrick DELAUNAY patrick.delaunay@foss.st.com Subject: Re: [PATCH v2 1/1] usb: dwc2: Fix the write to W1C fields in HPRT register
On 6/20/23 15:52, 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
Signed-off-by: Teik Heng Chong teik.heng.chong@intel.com
V1->V2
- update subject tags to usb:dwc2
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)+CC ST
Is there an actual bug this is solving ? Details please ?
This bug was found during the testing on Simics model. We read the 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)" and verified what every write to HPRT is intended to do in U-Boot dw2 driver. Then, we realized HPRT.PrtPwr is cleared by this mistake. Furthermore, we compared U-Boot driver source code and the Linux driver source code (which handles HPRT correctly). In the Linux driver (contrary to U-Boot), HPRT is always read using dwc2_read_hprt0 helper function https://elixir.bootlin.com/linux/latest/source/drivers/usb/dwc2/hcd.h#L462 which clears W1C bits. So after write back those bits are zeroes.
Please do add this to the commit message, I'll pick V3 if ST has no objection.
-----Original Message----- From: Marek Vasut marex@denx.de Sent: Wednesday, 21 June, 2023 9:20 AM 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; Patrice CHOTARD patrice.chotard@foss.st.com; Patrick DELAUNAY patrick.delaunay@foss.st.com Subject: Re: [PATCH v2 1/1] usb: dwc2: Fix the write to W1C fields in HPRT register
On 6/21/23 02:57, Chong, Teik Heng wrote:
-----Original Message----- From: Marek Vasut marex@denx.de Sent: Wednesday, 21 June, 2023 5:38 AM 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; Patrice CHOTARD patrice.chotard@foss.st.com; Patrick DELAUNAY patrick.delaunay@foss.st.com Subject: Re: [PATCH v2 1/1] usb: dwc2: Fix the write to W1C fields in HPRT register
On 6/20/23 15:52, 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
Signed-off-by: Teik Heng Chong teik.heng.chong@intel.com
V1->V2
- update subject tags to usb:dwc2
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 <<
#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 <<
#define DWC2_HAINT_CH0_OFFSET 0 #define DWC2_HAINT_CH1 (1 <<
+CC ST
Is there an actual bug this is solving ? Details please ?
This bug was found during the testing on Simics model. We read the
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)" and verified what every write to HPRT is intended to do in U-Boot dw2 driver. Then, we realized HPRT.PrtPwr is cleared by this mistake.
Furthermore, we compared U-Boot driver source code and the Linux driver
source code (which handles HPRT correctly). In the Linux driver (contrary to U-Boot), HPRT is always read using dwc2_read_hprt0 helper function https://elixir.bootlin.com/linux/latest/source/drivers/usb/dwc2/hcd.h#L462 which clears W1C bits. So after write back those bits are zeroes.
Please do add this to the commit message, I'll pick V3 if ST has no objection.
Ok I will add this.
On 6/21/23 04:55, Chong, Teik Heng wrote:
-----Original Message----- From: Marek Vasut marex@denx.de Sent: Wednesday, 21 June, 2023 9:20 AM 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; Patrice CHOTARD patrice.chotard@foss.st.com; Patrick DELAUNAY patrick.delaunay@foss.st.com Subject: Re: [PATCH v2 1/1] usb: dwc2: Fix the write to W1C fields in HPRT register
On 6/21/23 02:57, Chong, Teik Heng wrote:
-----Original Message----- From: Marek Vasut marex@denx.de Sent: Wednesday, 21 June, 2023 5:38 AM 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; Patrice CHOTARD patrice.chotard@foss.st.com; Patrick DELAUNAY patrick.delaunay@foss.st.com Subject: Re: [PATCH v2 1/1] usb: dwc2: Fix the write to W1C fields in HPRT register
On 6/20/23 15:52, 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
Signed-off-by: Teik Heng Chong teik.heng.chong@intel.com
V1->V2
- update subject tags to usb:dwc2
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 <<
#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 <<
#define DWC2_HAINT_CH0_OFFSET 0 #define DWC2_HAINT_CH1 (1 <<
+CC ST
Is there an actual bug this is solving ? Details please ?
This bug was found during the testing on Simics model. We read the
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)" and verified what every write to HPRT is intended to do in U-Boot dw2 driver. Then, we realized HPRT.PrtPwr is cleared by this mistake.
Furthermore, we compared U-Boot driver source code and the Linux driver
source code (which handles HPRT correctly). In the Linux driver (contrary to U-Boot), HPRT is always read using dwc2_read_hprt0 helper function https://elixir.bootlin.com/linux/latest/source/drivers/usb/dwc2/hcd.h#L462 which clears W1C bits. So after write back those bits are zeroes.
Please do add this to the commit message, I'll pick V3 if ST has no objection.
Ok I will add this.
Thanks
participants (3)
-
Chong, Teik Heng -
Marek Vasut -
teik.heng.chong＠intel.com