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

[Marek Vasut]

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.