Re: [PATCH] usb: cdns3: exit cdns3_gadget_udc_start if FAST_REG_ACCESS cannot be set

[Siddharth Vadapalli <s-vadapalli@xxxxxx>]

On Sat, Feb 08, 2025 at 01:39:44PM +0800, Peter Chen wrote:
> On 25-02-07 11:12:50, Siddharth Vadapalli wrote:
> > On Fri, Feb 07, 2025 at 10:25:23AM +0800, Peter Chen wrote:
> > 
> > Hello Peter,
> > 
> > > On 25-02-06 18:29:36, Siddharth Vadapalli wrote:
> > > > When the device is in a low power state, access to the following
> > > > registers takes a long time:
> > > > - EP_CFG
> > > > - EP_TRADDR
> > > > - EP_CMD
> > > > - EP_SEL
> > > > - EP_STS
> > > > - USB_CONF

[...]

> > > > @@ -2997,6 +2997,20 @@ static int cdns3_gadget_udc_start(struct usb_gadget *gadget,
> > > >  	/* limit speed if necessary */
> > > >  	max_speed = min(driver->max_speed, gadget->max_speed);
> > > >  
> > > > +	/*  keep Fast Access bit */
> > > > +	writel(PUSB_PWR_FST_REG_ACCESS, &priv_dev->regs->usb_pwr);
> > > > +	reg = readl(&priv_dev->regs->usb_pwr);
> > > > +	if (!(reg & PUSB_PWR_FST_REG_ACCESS_STAT)) {
> > > > +		ret = readl_poll_timeout_atomic(&priv_dev->regs->usb_pwr, reg,
> > > > +						(reg & PUSB_PWR_FST_REG_ACCESS_STAT),
> > > > +						10, 1000);
> > > > +		if (ret) {
> > > > +			dev_err(priv_dev->dev, "Failed to enable fast access\n");
> > > > +			spin_unlock_irqrestore(&priv_dev->lock, flags);
> > > > +			return ret;
> > > > +		}
> > > > +	}
> > > > +
> > > >  	switch (max_speed) {
> > > >  	case USB_SPEED_FULL:
> > > >  		writel(USB_CONF_SFORCE_FS, &priv_dev->regs->usb_conf);
> > > 
> > > Hi Siddharth,
> > > 
> > > Would you please keep this change at cdns3_gadget_config in case the
> > > controller is power lost during the system suspend?
> > 
> > I did think of that initially, but the problem with doing so is that we
> > are already accessing USB_CONF above in the "switch(max_speed)" section.
> > The PUSB_PWR_FST_REG_ACCESS bit needs to be set before accessing any of:
> > - EP_CFG
> > - EP_TRADDR
> > - EP_CMD
> > - EP_SEL
> > - EP_STS
> > - USB_CONF
> > 
> > Please let me know if you have an alternate suggestion to address the
> > above.
> > 
> 
> How about move cdns3_gadget_config at the beginning of function
> cdns3_gadget_udc_start, and add your changes at cdns3_gadget_config?

Yes, I could do that, but I will still move the following section within
cdns3_gadget_config() function to the top of that function:

	/*  keep Fast Access bit */
	writel(PUSB_PWR_FST_REG_ACCESS, &priv_dev->regs->usb_pwr);

Additionally, I will update cdns3_gadget_config() to return an error
code if Fast Access bit cannot be set. The diff corresponding to this is:
--------------------------------------------------------------------------------------------
diff --git a/drivers/usb/cdns3/cdns3-gadget.c b/drivers/usb/cdns3/cdns3-gadget.c
index fd1beb10bba7..67d8b0805ba0 100644
--- a/drivers/usb/cdns3/cdns3-gadget.c
+++ b/drivers/usb/cdns3/cdns3-gadget.c
@@ -2935,10 +2935,24 @@ static int cdns3_gadget_pullup(struct usb_gadget *gadget, int is_on)
 	return 0;
 }
 
-static void cdns3_gadget_config(struct cdns3_device *priv_dev)
+static int cdns3_gadget_config(struct cdns3_device *priv_dev)
 {
 	struct cdns3_usb_regs __iomem *regs = priv_dev->regs;
 	u32 reg;
+	int ret;
+
+	/*  keep Fast Access bit */
+	writel(PUSB_PWR_FST_REG_ACCESS, &priv_dev->regs->usb_pwr);
+	reg = readl(&priv_dev->regs->usb_pwr);
+	if (!(reg & PUSB_PWR_FST_REG_ACCESS_STAT)) {
+		ret = readl_poll_timeout_atomic(&priv_dev->regs->usb_pwr, reg,
+						(reg & PUSB_PWR_FST_REG_ACCESS_STAT),
+						10, 1000);
+		if (ret) {
+			dev_err(priv_dev->dev, "enabling fast access timed out\n");
+			return ret;
+		}
+	}
 
 	cdns3_ep0_config(priv_dev);
 
@@ -2971,10 +2985,10 @@ static void cdns3_gadget_config(struct cdns3_device *priv_dev)
 	/* enable generic interrupt*/
 	writel(USB_IEN_INIT, &regs->usb_ien);
 	writel(USB_CONF_CLK2OFFDS | USB_CONF_L1DS, &regs->usb_conf);
-	/*  keep Fast Access bit */
-	writel(PUSB_PWR_FST_REG_ACCESS, &priv_dev->regs->usb_pwr);
 
 	cdns3_configure_dmult(priv_dev, NULL);
+
+	return 0;
 }
 
 /**
@@ -2990,10 +3004,15 @@ static int cdns3_gadget_udc_start(struct usb_gadget *gadget,
 	struct cdns3_device *priv_dev = gadget_to_cdns3_device(gadget);
 	unsigned long flags;
 	enum usb_device_speed max_speed = driver->max_speed;
+	int ret;
 
 	spin_lock_irqsave(&priv_dev->lock, flags);
 	priv_dev->gadget_driver = driver;
 
+	ret = cdns3_gadget_config(priv_dev);
+	if (ret)
+		return ret;
+
 	/* limit speed if necessary */
 	max_speed = min(driver->max_speed, gadget->max_speed);
 
@@ -3018,7 +3037,6 @@ static int cdns3_gadget_udc_start(struct usb_gadget *gadget,
 		break;
 	}
 
-	cdns3_gadget_config(priv_dev);
 	spin_unlock_irqrestore(&priv_dev->lock, flags);
 	return 0;
 }
@@ -3471,11 +3489,15 @@ __must_hold(&cdns->lock)
 static int cdns3_gadget_resume(struct cdns *cdns, bool hibernated)
 {
 	struct cdns3_device *priv_dev = cdns->gadget_dev;
+	int ret;
 
 	if (!priv_dev->gadget_driver)
 		return 0;
 
-	cdns3_gadget_config(priv_dev);
+	ret = cdns3_gadget_config(priv_dev);
+	if (ret)
+		return ret;
+
 	if (hibernated)
 		writel(USB_CONF_DEVEN, &priv_dev->regs->usb_conf);
--------------------------------------------------------------------------------------------

Regards,
Siddharth.