drivers/usb/cdns3/cdns3-gadget.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)
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);
--
2.43.0
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.
© 2016 - 2026 Red Hat, Inc.