| Message ID | 20250206125943.786949-1-s-vadapalli@ti.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | usb: cdns3: exit cdns3_gadget_udc_start if FAST_REG_ACCESS cannot be set | expand |
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 > > To address this, the fast register access feature can be enabled by > setting PUSB_PWR_FST_REG_ACCESS bit of the USB_PWR register, which > allows quick access by software. Software is expected to poll on > PUSB_PWR_FST_REG_ACCESS_STAT to ensure that fast register access has > been enabled by the controller. Attempting to access any of the > aforementioned registers after setting PUSB_PWR_FST_REG_ACCESS but > before PUSB_PWR_FST_REG_ACCESS_STAT has been set will result in > undefined behavior and potentially result in system hang. > > Hence, poll on PUSB_PWR_FST_REG_ACCESS_STAT before proceeding with > gadget configuration, and exit if it cannot be enabled. > > Fixes: b5148d946f45 ("usb: cdns3: gadget: set fast access bit") > Cc: <stable@vger.kernel.org> > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> > --- > > Hello, > > This patch is based on commit > 92514ef226f5 Merge tag 'for-6.14-rc1-tag' of git://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux > of Mainline Linux. > > Regards, > Siddharth. > > drivers/usb/cdns3/cdns3-gadget.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/cdns3/cdns3-gadget.c b/drivers/usb/cdns3/cdns3-gadget.c > index fd1beb10bba7..b62691944272 100644 > --- a/drivers/usb/cdns3/cdns3-gadget.c > +++ b/drivers/usb/cdns3/cdns3-gadget.c > @@ -2971,8 +2971,6 @@ static void cdns3_gadget_config(struct cdns3_device *priv_dev) > /* enable generic interrupt*/ > writel(USB_IEN_INIT, ®s->usb_ien); > writel(USB_CONF_CLK2OFFDS | USB_CONF_L1DS, ®s->usb_conf); > - /* keep Fast Access bit */ > - writel(PUSB_PWR_FST_REG_ACCESS, &priv_dev->regs->usb_pwr); > > cdns3_configure_dmult(priv_dev, NULL); > } > @@ -2990,6 +2988,8 @@ 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; > + u32 reg; > > spin_lock_irqsave(&priv_dev->lock, flags); > priv_dev->gadget_driver = driver; > @@ -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? Peter
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 > > > > To address this, the fast register access feature can be enabled by > > setting PUSB_PWR_FST_REG_ACCESS bit of the USB_PWR register, which > > allows quick access by software. Software is expected to poll on > > PUSB_PWR_FST_REG_ACCESS_STAT to ensure that fast register access has > > been enabled by the controller. Attempting to access any of the > > aforementioned registers after setting PUSB_PWR_FST_REG_ACCESS but > > before PUSB_PWR_FST_REG_ACCESS_STAT has been set will result in > > undefined behavior and potentially result in system hang. > > > > Hence, poll on PUSB_PWR_FST_REG_ACCESS_STAT before proceeding with > > gadget configuration, and exit if it cannot be enabled. > > > > Fixes: b5148d946f45 ("usb: cdns3: gadget: set fast access bit") > > Cc: <stable@vger.kernel.org> > > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> > > --- > > > > Hello, > > > > This patch is based on commit > > 92514ef226f5 Merge tag 'for-6.14-rc1-tag' of git://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux > > of Mainline Linux. > > > > Regards, > > Siddharth. > > > > drivers/usb/cdns3/cdns3-gadget.c | 18 ++++++++++++++++-- > > 1 file changed, 16 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/usb/cdns3/cdns3-gadget.c b/drivers/usb/cdns3/cdns3-gadget.c > > index fd1beb10bba7..b62691944272 100644 > > --- a/drivers/usb/cdns3/cdns3-gadget.c > > +++ b/drivers/usb/cdns3/cdns3-gadget.c > > @@ -2971,8 +2971,6 @@ static void cdns3_gadget_config(struct cdns3_device *priv_dev) > > /* enable generic interrupt*/ > > writel(USB_IEN_INIT, ®s->usb_ien); > > writel(USB_CONF_CLK2OFFDS | USB_CONF_L1DS, ®s->usb_conf); > > - /* keep Fast Access bit */ > > - writel(PUSB_PWR_FST_REG_ACCESS, &priv_dev->regs->usb_pwr); > > > > cdns3_configure_dmult(priv_dev, NULL); > > } > > @@ -2990,6 +2988,8 @@ 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; > > + u32 reg; > > > > spin_lock_irqsave(&priv_dev->lock, flags); > > priv_dev->gadget_driver = driver; > > @@ -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. Regards, Siddharth.
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 > > > > > > To address this, the fast register access feature can be enabled by > > > setting PUSB_PWR_FST_REG_ACCESS bit of the USB_PWR register, which > > > allows quick access by software. Software is expected to poll on > > > PUSB_PWR_FST_REG_ACCESS_STAT to ensure that fast register access has > > > been enabled by the controller. Attempting to access any of the > > > aforementioned registers after setting PUSB_PWR_FST_REG_ACCESS but > > > before PUSB_PWR_FST_REG_ACCESS_STAT has been set will result in > > > undefined behavior and potentially result in system hang. > > > > > > Hence, poll on PUSB_PWR_FST_REG_ACCESS_STAT before proceeding with > > > gadget configuration, and exit if it cannot be enabled. > > > > > > Fixes: b5148d946f45 ("usb: cdns3: gadget: set fast access bit") > > > Cc: <stable@vger.kernel.org> > > > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> > > > --- > > > > > > Hello, > > > > > > This patch is based on commit > > > 92514ef226f5 Merge tag 'for-6.14-rc1-tag' of git://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux > > > of Mainline Linux. > > > > > > Regards, > > > Siddharth. > > > > > > drivers/usb/cdns3/cdns3-gadget.c | 18 ++++++++++++++++-- > > > 1 file changed, 16 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/usb/cdns3/cdns3-gadget.c b/drivers/usb/cdns3/cdns3-gadget.c > > > index fd1beb10bba7..b62691944272 100644 > > > --- a/drivers/usb/cdns3/cdns3-gadget.c > > > +++ b/drivers/usb/cdns3/cdns3-gadget.c > > > @@ -2971,8 +2971,6 @@ static void cdns3_gadget_config(struct cdns3_device *priv_dev) > > > /* enable generic interrupt*/ > > > writel(USB_IEN_INIT, ®s->usb_ien); > > > writel(USB_CONF_CLK2OFFDS | USB_CONF_L1DS, ®s->usb_conf); > > > - /* keep Fast Access bit */ > > > - writel(PUSB_PWR_FST_REG_ACCESS, &priv_dev->regs->usb_pwr); > > > > > > cdns3_configure_dmult(priv_dev, NULL); > > > } > > > @@ -2990,6 +2988,8 @@ 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; > > > + u32 reg; > > > > > > spin_lock_irqsave(&priv_dev->lock, flags); > > > priv_dev->gadget_driver = driver; > > > @@ -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? Regards, Peter
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, ®s->usb_ien); writel(USB_CONF_CLK2OFFDS | USB_CONF_L1DS, ®s->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.
diff --git a/drivers/usb/cdns3/cdns3-gadget.c b/drivers/usb/cdns3/cdns3-gadget.c index fd1beb10bba7..b62691944272 100644 --- a/drivers/usb/cdns3/cdns3-gadget.c +++ b/drivers/usb/cdns3/cdns3-gadget.c @@ -2971,8 +2971,6 @@ static void cdns3_gadget_config(struct cdns3_device *priv_dev) /* enable generic interrupt*/ writel(USB_IEN_INIT, ®s->usb_ien); writel(USB_CONF_CLK2OFFDS | USB_CONF_L1DS, ®s->usb_conf); - /* keep Fast Access bit */ - writel(PUSB_PWR_FST_REG_ACCESS, &priv_dev->regs->usb_pwr); cdns3_configure_dmult(priv_dev, NULL); } @@ -2990,6 +2988,8 @@ 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; + u32 reg; spin_lock_irqsave(&priv_dev->lock, flags); priv_dev->gadget_driver = driver; @@ -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);
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 To address this, the fast register access feature can be enabled by setting PUSB_PWR_FST_REG_ACCESS bit of the USB_PWR register, which allows quick access by software. Software is expected to poll on PUSB_PWR_FST_REG_ACCESS_STAT to ensure that fast register access has been enabled by the controller. Attempting to access any of the aforementioned registers after setting PUSB_PWR_FST_REG_ACCESS but before PUSB_PWR_FST_REG_ACCESS_STAT has been set will result in undefined behavior and potentially result in system hang. Hence, poll on PUSB_PWR_FST_REG_ACCESS_STAT before proceeding with gadget configuration, and exit if it cannot be enabled. Fixes: b5148d946f45 ("usb: cdns3: gadget: set fast access bit") Cc: <stable@vger.kernel.org> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> --- Hello, This patch is based on commit 92514ef226f5 Merge tag 'for-6.14-rc1-tag' of git://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux of Mainline Linux. Regards, Siddharth. drivers/usb/cdns3/cdns3-gadget.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)