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 ?