diff mbox series

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

Message ID 20230620135243.32090-1-teik.heng.chong@intel.com
State Superseded
Delegated to: Marek Vasut
Headers show
Series [v2,1/1] usb: dwc2: Fix the write to W1C fields in HPRT register | expand

Commit Message

Chong, Teik Heng June 20, 2023, 1:52 p.m. UTC 
From: Teik Heng Chong <teik.heng.chong@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@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(-)

Comments

Marek Vasut June 20, 2023, 9:38 p.m. UTC | #1 
On 6/20/23 15:52, teik.heng.chong@intel.com wrote:
> From: Teik Heng Chong <teik.heng.chong@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@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 ?
Chong, Teik Heng June 21, 2023, 12:57 a.m. UTC | #2 
-----Original Message-----
From: Marek Vasut <marex@denx.de> 
Sent: Wednesday, 21 June, 2023 5:38 AM
To: Chong, Teik Heng <teik.heng.chong@intel.com>; u-boot@lists.denx.de
Cc: Jagan Teki <jagan@amarulasolutions.com>; Vignesh R <vigneshr@ti.com>; Simon <simon.k.r.goldschmidt@gmail.com>; Kris <kris.chaplin@intel.com>; Chee, Tien Fong <tien.fong.chee@intel.com>; Hea, Kok Kiang <kok.kiang.hea@intel.com>; Lokanathan, Raaj <raaj.lokanathan@intel.com>; Maniyam, Dinesh <dinesh.maniyam@intel.com>; Ng, Boon Khai <boon.khai.ng@intel.com>; Yuslaimi, Alif Zakuan <alif.zakuan.yuslaimi@intel.com>; Zamri, Muhammad Hazim Izzat <muhammad.hazim.izzat.zamri@intel.com>; Lim, Jit Loon <jit.loon.lim@intel.com>; Tang, Sieu Mun <sieu.mun.tang@intel.com>; Patrice CHOTARD <patrice.chotard@foss.st.com>; Patrick DELAUNAY <patrick.delaunay@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@intel.com wrote:
> From: Teik Heng Chong <teik.heng.chong@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@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.
Marek Vasut June 21, 2023, 1:19 a.m. UTC | #3 
On 6/21/23 02:57, Chong, Teik Heng wrote:
> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: Wednesday, 21 June, 2023 5:38 AM
> To: Chong, Teik Heng <teik.heng.chong@intel.com>; u-boot@lists.denx.de
> Cc: Jagan Teki <jagan@amarulasolutions.com>; Vignesh R <vigneshr@ti.com>; Simon <simon.k.r.goldschmidt@gmail.com>; Kris <kris.chaplin@intel.com>; Chee, Tien Fong <tien.fong.chee@intel.com>; Hea, Kok Kiang <kok.kiang.hea@intel.com>; Lokanathan, Raaj <raaj.lokanathan@intel.com>; Maniyam, Dinesh <dinesh.maniyam@intel.com>; Ng, Boon Khai <boon.khai.ng@intel.com>; Yuslaimi, Alif Zakuan <alif.zakuan.yuslaimi@intel.com>; Zamri, Muhammad Hazim Izzat <muhammad.hazim.izzat.zamri@intel.com>; Lim, Jit Loon <jit.loon.lim@intel.com>; Tang, Sieu Mun <sieu.mun.tang@intel.com>; Patrice CHOTARD <patrice.chotard@foss.st.com>; Patrick DELAUNAY <patrick.delaunay@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@intel.com wrote:
>> From: Teik Heng Chong <teik.heng.chong@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@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.
Chong, Teik Heng June 21, 2023, 2:55 a.m. UTC | #4 
> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: Wednesday, 21 June, 2023 9:20 AM
> To: Chong, Teik Heng <teik.heng.chong@intel.com>; u-boot@lists.denx.de
> Cc: Jagan Teki <jagan@amarulasolutions.com>; Vignesh R
> <vigneshr@ti.com>; Simon <simon.k.r.goldschmidt@gmail.com>; Kris
> <kris.chaplin@intel.com>; Chee, Tien Fong <tien.fong.chee@intel.com>; Hea,
> Kok Kiang <kok.kiang.hea@intel.com>; Lokanathan, Raaj
> <raaj.lokanathan@intel.com>; Maniyam, Dinesh
> <dinesh.maniyam@intel.com>; Ng, Boon Khai <boon.khai.ng@intel.com>;
> Yuslaimi, Alif Zakuan <alif.zakuan.yuslaimi@intel.com>; Zamri, Muhammad
> Hazim Izzat <muhammad.hazim.izzat.zamri@intel.com>; Lim, Jit Loon
> <jit.loon.lim@intel.com>; Tang, Sieu Mun <sieu.mun.tang@intel.com>;
> Patrice CHOTARD <patrice.chotard@foss.st.com>; Patrick DELAUNAY
> <patrick.delaunay@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@denx.de>
> > Sent: Wednesday, 21 June, 2023 5:38 AM
> > To: Chong, Teik Heng <teik.heng.chong@intel.com>; u-boot@lists.denx.de
> > Cc: Jagan Teki <jagan@amarulasolutions.com>; Vignesh R
> > <vigneshr@ti.com>; Simon <simon.k.r.goldschmidt@gmail.com>; Kris
> > <kris.chaplin@intel.com>; Chee, Tien Fong <tien.fong.chee@intel.com>;
> > Hea, Kok Kiang <kok.kiang.hea@intel.com>; Lokanathan, Raaj
> > <raaj.lokanathan@intel.com>; Maniyam, Dinesh
> > <dinesh.maniyam@intel.com>; Ng, Boon Khai <boon.khai.ng@intel.com>;
> > Yuslaimi, Alif Zakuan <alif.zakuan.yuslaimi@intel.com>; Zamri,
> > Muhammad Hazim Izzat <muhammad.hazim.izzat.zamri@intel.com>; Lim,
> Jit
> > Loon <jit.loon.lim@intel.com>; Tang, Sieu Mun
> > <sieu.mun.tang@intel.com>; Patrice CHOTARD
> > <patrice.chotard@foss.st.com>; Patrick DELAUNAY
> > <patrick.delaunay@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@intel.com wrote:
> >> From: Teik Heng Chong <teik.heng.chong@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@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.
Marek Vasut June 21, 2023, 3:46 a.m. UTC | #5 
On 6/21/23 04:55, Chong, Teik Heng wrote:
>> -----Original Message-----
>> From: Marek Vasut <marex@denx.de>
>> Sent: Wednesday, 21 June, 2023 9:20 AM
>> To: Chong, Teik Heng <teik.heng.chong@intel.com>; u-boot@lists.denx.de
>> Cc: Jagan Teki <jagan@amarulasolutions.com>; Vignesh R
>> <vigneshr@ti.com>; Simon <simon.k.r.goldschmidt@gmail.com>; Kris
>> <kris.chaplin@intel.com>; Chee, Tien Fong <tien.fong.chee@intel.com>; Hea,
>> Kok Kiang <kok.kiang.hea@intel.com>; Lokanathan, Raaj
>> <raaj.lokanathan@intel.com>; Maniyam, Dinesh
>> <dinesh.maniyam@intel.com>; Ng, Boon Khai <boon.khai.ng@intel.com>;
>> Yuslaimi, Alif Zakuan <alif.zakuan.yuslaimi@intel.com>; Zamri, Muhammad
>> Hazim Izzat <muhammad.hazim.izzat.zamri@intel.com>; Lim, Jit Loon
>> <jit.loon.lim@intel.com>; Tang, Sieu Mun <sieu.mun.tang@intel.com>;
>> Patrice CHOTARD <patrice.chotard@foss.st.com>; Patrick DELAUNAY
>> <patrick.delaunay@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@denx.de>
>>> Sent: Wednesday, 21 June, 2023 5:38 AM
>>> To: Chong, Teik Heng <teik.heng.chong@intel.com>; u-boot@lists.denx.de
>>> Cc: Jagan Teki <jagan@amarulasolutions.com>; Vignesh R
>>> <vigneshr@ti.com>; Simon <simon.k.r.goldschmidt@gmail.com>; Kris
>>> <kris.chaplin@intel.com>; Chee, Tien Fong <tien.fong.chee@intel.com>;
>>> Hea, Kok Kiang <kok.kiang.hea@intel.com>; Lokanathan, Raaj
>>> <raaj.lokanathan@intel.com>; Maniyam, Dinesh
>>> <dinesh.maniyam@intel.com>; Ng, Boon Khai <boon.khai.ng@intel.com>;
>>> Yuslaimi, Alif Zakuan <alif.zakuan.yuslaimi@intel.com>; Zamri,
>>> Muhammad Hazim Izzat <muhammad.hazim.izzat.zamri@intel.com>; Lim,
>> Jit
>>> Loon <jit.loon.lim@intel.com>; Tang, Sieu Mun
>>> <sieu.mun.tang@intel.com>; Patrice CHOTARD
>>> <patrice.chotard@foss.st.com>; Patrick DELAUNAY
>>> <patrick.delaunay@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@intel.com wrote:
>>>> From: Teik Heng Chong <teik.heng.chong@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@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
diff mbox series

Patch

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)