> -----Original Message-----
> From: Marek Vasut <ma...@denx.de>
> Sent: Wednesday, 21 June, 2023 9:20 AM
> To: Chong, Teik Heng <teik.heng.ch...@intel.com>; u-boot@lists.denx.de
> Cc: Jagan Teki <ja...@amarulasolutions.com>; Vignesh R
> <vigne...@ti.com>; Simon <simon.k.r.goldschm...@gmail.com>; Kris
> <kris.chap...@intel.com>; Chee, Tien Fong <tien.fong.c...@intel.com>; Hea,
> Kok Kiang <kok.kiang....@intel.com>; Lokanathan, Raaj
> <raaj.lokanat...@intel.com>; Maniyam, Dinesh
> <dinesh.mani...@intel.com>; Ng, Boon Khai <boon.khai...@intel.com>;
> Yuslaimi, Alif Zakuan <alif.zakuan.yusla...@intel.com>; Zamri, Muhammad
> Hazim Izzat <muhammad.hazim.izzat.za...@intel.com>; Lim, Jit Loon
> <jit.loon....@intel.com>; Tang, Sieu Mun <sieu.mun.t...@intel.com>;
> Patrice CHOTARD <patrice.chot...@foss.st.com>; Patrick DELAUNAY
> <patrick.delau...@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 <ma...@denx.de>
> > Sent: Wednesday, 21 June, 2023 5:38 AM
> > To: Chong, Teik Heng <teik.heng.ch...@intel.com>; u-boot@lists.denx.de
> > Cc: Jagan Teki <ja...@amarulasolutions.com>; Vignesh R
> > <vigne...@ti.com>; Simon <simon.k.r.goldschm...@gmail.com>; Kris
> > <kris.chap...@intel.com>; Chee, Tien Fong <tien.fong.c...@intel.com>;
> > Hea, Kok Kiang <kok.kiang....@intel.com>; Lokanathan, Raaj
> > <raaj.lokanat...@intel.com>; Maniyam, Dinesh
> > <dinesh.mani...@intel.com>; Ng, Boon Khai <boon.khai...@intel.com>;
> > Yuslaimi, Alif Zakuan <alif.zakuan.yusla...@intel.com>; Zamri,
> > Muhammad Hazim Izzat <muhammad.hazim.izzat.za...@intel.com>; Lim,
> Jit
> > Loon <jit.loon....@intel.com>; Tang, Sieu Mun
> > <sieu.mun.t...@intel.com>; Patrice CHOTARD
> > <patrice.chot...@foss.st.com>; Patrick DELAUNAY
> > <patrick.delau...@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.ch...@intel.com wrote:
> >> From: Teik Heng Chong <teik.heng.ch...@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.ch...@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)
> >
> > +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.
