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

Marek Vasut marex at denx.de
Wed Jun 21 05:46:15 CEST 2023 

On 6/21/23 04:55, Chong, Teik Heng wrote:
>> -----Original Message-----
>> From: Marek Vasut <marex at denx.de>
>> Sent: Wednesday, 21 June, 2023 9:20 AM
>> 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>;
>> Patrice CHOTARD <patrice.chotard at foss.st.com>; Patrick DELAUNAY
>> <patrick.delaunay at 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 at denx.de>
>>> Sent: Wednesday, 21 June, 2023 5:38 AM
>>> 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>; Patrice CHOTARD
>>> <patrice.chotard at foss.st.com>; Patrick DELAUNAY
>>> <patrick.delaunay at 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 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
>>>>
>>>> Signed-off-by: Teik Heng Chong <teik.heng.chong at 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(&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 no objection.
> 
> Ok I will add this.

Thanks

More information about the U-Boot mailing list