-----Original Message-----
From: Marek Vasut <[email protected]>
Sent: Wednesday, 21 June, 2023 9:20 AM
To: Chong, Teik Heng <[email protected]>; [email protected]
Cc: Jagan Teki <[email protected]>; Vignesh R
<[email protected]>; Simon <[email protected]>; Kris
<[email protected]>; Chee, Tien Fong <[email protected]>; Hea,
Kok Kiang <[email protected]>; Lokanathan, Raaj
<[email protected]>; Maniyam, Dinesh
<[email protected]>; Ng, Boon Khai <[email protected]>;
Yuslaimi, Alif Zakuan <[email protected]>; Zamri, Muhammad
Hazim Izzat <[email protected]>; Lim, Jit Loon
<[email protected]>; Tang, Sieu Mun <[email protected]>;
Patrice CHOTARD <[email protected]>; Patrick DELAUNAY
<[email protected]>
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 <[email protected]>
Sent: Wednesday, 21 June, 2023 5:38 AM
To: Chong, Teik Heng <[email protected]>; [email protected]
Cc: Jagan Teki <[email protected]>; Vignesh R
<[email protected]>; Simon <[email protected]>; Kris
<[email protected]>; Chee, Tien Fong <[email protected]>;
Hea, Kok Kiang <[email protected]>; Lokanathan, Raaj
<[email protected]>; Maniyam, Dinesh
<[email protected]>; Ng, Boon Khai <[email protected]>;
Yuslaimi, Alif Zakuan <[email protected]>; Zamri,
Muhammad Hazim Izzat <[email protected]>; Lim,
Jit
Loon <[email protected]>; Tang, Sieu Mun
<[email protected]>; Patrice CHOTARD
<[email protected]>; Patrick DELAUNAY
<[email protected]>
Subject: Re: [PATCH v2 1/1] usb: dwc2: Fix the write to W1C fields in
HPRT register
On 6/20/23 15:52, [email protected] wrote:
From: Teik Heng Chong <[email protected]>
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 <[email protected]>
---
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)
+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.
