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

[Chong, Teik Heng]

> -----Original Message-----
> From: Marek Vasut <marex at denx.de>
> Sent: Wednesday, 21 June, 2023 7:16 PM
> To: Chong, Teik Heng <teik.heng.chong at intel.com>; u-boot at lists.denx.de
> Cc: Jagan Teki <jagan at amarulasolutions.com>; Vignesh R <vigneshr at ti.com>;
> Simon <simon.k.r.goldschmidt at gmail.com>; Kris <kris.chaplin at intel.com>;
> Chee, Tien Fong <tien.fong.chee at intel.com>; Hea, Kok Kiang
> <kok.kiang.hea at intel.com>; Lokanathan, Raaj <raaj.lokanathan at intel.com>;
> Maniyam, Dinesh <dinesh.maniyam at intel.com>; Ng, Boon Khai
> <boon.khai.ng at intel.com>; Yuslaimi, Alif Zakuan
> <alif.zakuan.yuslaimi at intel.com>; Zamri, Muhammad Hazim Izzat
> <muhammad.hazim.izzat.zamri at intel.com>; Lim, Jit Loon
> <jit.loon.lim at intel.com>; Tang, Sieu Mun <sieu.mun.tang at 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 at intel.com wrote:
> > From: Teik Heng Chong <teik.heng.chong at 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 at 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(&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)
> 
> Applied to usb/master, thanks.

Thank you.