Re: [PATCH v2 1/1] usb: dwc2: Fix the write to W1C fields in HPRT register

Marek Vasut Tue, 20 Jun 2023 18:20:02 -0700

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(&regs->gintsts) & DWC2_GINTSTS_CURMODE_HOST) {
-               hprt0 = readl(&regs->hprt0);
-               hprt0 &= ~(DWC2_HPRT0_PRTENA | DWC2_HPRT0_PRTCONNDET);
-               hprt0 &= ~(DWC2_HPRT0_PRTENCHNG | DWC2_HPRT0_PRTOVRCURRCHNG);
+               hprt0 = readl(&regs->hprt0) & ~DWC2_HPRT0_W1C_MASK;
                if (!(hprt0 & DWC2_HPRT0_PRTPWR)) {
                        hprt0 |= DWC2_HPRT0_PRTPWR;
                        writel(hprt0, &regs->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(&regs->hprt0, DWC2_HPRT0_PRTCONNDET);
+                       clrsetbits_le32(&regs->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(&regs->hprt0, DWC2_HPRT0_PRTENA |
-                                       DWC2_HPRT0_PRTCONNDET |
-                                       DWC2_HPRT0_PRTENCHNG |
-                                       DWC2_HPRT0_PRTOVRCURRCHNG,
-                                       DWC2_HPRT0_PRTRST);
+                       clrsetbits_le32(&regs->hprt0, DWC2_HPRT0_W1C_MASK,
+DWC2_HPRT0_PRTRST);
                        mdelay(50);
-                       clrbits_le32(&regs->hprt0, DWC2_HPRT0_PRTRST);
+                       clrbits_le32(&regs->hprt0, DWC2_HPRT0_W1C_MASK |
+DWC2_HPRT0_PRTRST);
                        break;

case USB_PORT_FEAT_POWER:

-                       clrsetbits_le32(&regs->hprt0, DWC2_HPRT0_PRTENA |
-                                       DWC2_HPRT0_PRTCONNDET |
-                                       DWC2_HPRT0_PRTENCHNG |
-                                       DWC2_HPRT0_PRTOVRCURRCHNG,
-                                       DWC2_HPRT0_PRTRST);
+                       clrsetbits_le32(&regs->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(&regs->hprt0, DWC2_HPRT0_PRTENA |

-                       DWC2_HPRT0_PRTCONNDET | DWC2_HPRT0_PRTENCHNG |
-                       DWC2_HPRT0_PRTOVRCURRCHNG,
-                       DWC2_HPRT0_PRTRST);
+       clrsetbits_le32(&regs->hprt0, DWC2_HPRT0_W1C_MASK,
+DWC2_HPRT0_PRTRST);
        mdelay(50);
-       clrbits_le32(&regs->hprt0, DWC2_HPRT0_PRTENA | DWC2_HPRT0_PRTCONNDET |
-                    DWC2_HPRT0_PRTENCHNG | DWC2_HPRT0_PRTOVRCURRCHNG |
-                    DWC2_HPRT0_PRTRST);
+       clrbits_le32(&regs->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(&regs->hprt0, DWC2_HPRT0_PRTENA |
-                       DWC2_HPRT0_PRTCONNDET | DWC2_HPRT0_PRTENCHNG |
-                       DWC2_HPRT0_PRTOVRCURRCHNG,
-                       DWC2_HPRT0_PRTRST);
+       clrsetbits_le32(&regs->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 noobjection.

Re: [PATCH v2 1/1] usb: dwc2: Fix the write to W1C fields in HPRT register

Reply via email to