* [PATCH] usb: cdns3: gadget: Fix g_audio use case when connected to Super-Speed host
@ 2019-10-29 15:15 Roger Quadros
2019-10-30 6:36 ` Peter Chen
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: Roger Quadros @ 2019-10-29 15:15 UTC (permalink / raw)
To: felipe.balbi, gregkh
Cc: pawell, peter.chen, nsekhar, kurahul, linux-usb, linux-kernel,
Roger Quadros
Take into account gadget driver's speed limit when programming
controller speed.
Signed-off-by: Roger Quadros <rogerq@ti.com>
---
Hi Greg,
Please apply this for -rc.
Without this, g_audio is broken on cdns3 USB controller is
connected to a Super-Speed host.
cheers,
-roger
drivers/usb/cdns3/gadget.c | 31 ++++++++++++++++++++++++++-----
1 file changed, 26 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
index 40dad4e8d0dc..1c724c20d468 100644
--- a/drivers/usb/cdns3/gadget.c
+++ b/drivers/usb/cdns3/gadget.c
@@ -2338,9 +2338,35 @@ 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;
spin_lock_irqsave(&priv_dev->lock, flags);
priv_dev->gadget_driver = driver;
+
+ /* limit speed if necessary */
+ max_speed = min(driver->max_speed, gadget->max_speed);
+
+ switch (max_speed) {
+ case USB_SPEED_FULL:
+ writel(USB_CONF_SFORCE_FS, &priv_dev->regs->usb_conf);
+ writel(USB_CONF_USB3DIS, &priv_dev->regs->usb_conf);
+ break;
+ case USB_SPEED_HIGH:
+ writel(USB_CONF_USB3DIS, &priv_dev->regs->usb_conf);
+ break;
+ case USB_SPEED_SUPER:
+ break;
+ default:
+ dev_err(priv_dev->dev,
+ "invalid maximum_speed parameter %d\n",
+ max_speed);
+ /* fall through */
+ case USB_SPEED_UNKNOWN:
+ /* default to superspeed */
+ max_speed = USB_SPEED_SUPER;
+ break;
+ }
+
cdns3_gadget_config(priv_dev);
spin_unlock_irqrestore(&priv_dev->lock, flags);
return 0;
@@ -2570,12 +2596,7 @@ static int cdns3_gadget_start(struct cdns3 *cdns)
/* Check the maximum_speed parameter */
switch (max_speed) {
case USB_SPEED_FULL:
- writel(USB_CONF_SFORCE_FS, &priv_dev->regs->usb_conf);
- writel(USB_CONF_USB3DIS, &priv_dev->regs->usb_conf);
- break;
case USB_SPEED_HIGH:
- writel(USB_CONF_USB3DIS, &priv_dev->regs->usb_conf);
- break;
case USB_SPEED_SUPER:
break;
default:
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] usb: cdns3: gadget: Fix g_audio use case when connected to Super-Speed host
2019-10-29 15:15 [PATCH] usb: cdns3: gadget: Fix g_audio use case when connected to Super-Speed host Roger Quadros
@ 2019-10-30 6:36 ` Peter Chen
2019-10-30 8:44 ` Roger Quadros
2019-10-30 11:46 ` Felipe Balbi
2019-10-30 12:16 ` [PATCH v2] " Roger Quadros
2 siblings, 1 reply; 17+ messages in thread
From: Peter Chen @ 2019-10-30 6:36 UTC (permalink / raw)
To: Roger Quadros
Cc: felipe.balbi, gregkh, pawell, nsekhar, kurahul, linux-usb, linux-kernel
On 19-10-29 17:15:14, Roger Quadros wrote:
> Take into account gadget driver's speed limit when programming
> controller speed.
>
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
> Hi Greg,
>
> Please apply this for -rc.
> Without this, g_audio is broken on cdns3 USB controller is
> connected to a Super-Speed host.
>
> cheers,
> -roger
>
> drivers/usb/cdns3/gadget.c | 31 ++++++++++++++++++++++++++-----
> 1 file changed, 26 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
> index 40dad4e8d0dc..1c724c20d468 100644
> --- a/drivers/usb/cdns3/gadget.c
> +++ b/drivers/usb/cdns3/gadget.c
> @@ -2338,9 +2338,35 @@ 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;
>
> spin_lock_irqsave(&priv_dev->lock, flags);
> priv_dev->gadget_driver = driver;
> +
> + /* limit speed if necessary */
> + max_speed = min(driver->max_speed, gadget->max_speed);
> +
> + switch (max_speed) {
> + case USB_SPEED_FULL:
> + writel(USB_CONF_SFORCE_FS, &priv_dev->regs->usb_conf);
> + writel(USB_CONF_USB3DIS, &priv_dev->regs->usb_conf);
> + break;
> + case USB_SPEED_HIGH:
> + writel(USB_CONF_USB3DIS, &priv_dev->regs->usb_conf);
> + break;
> + case USB_SPEED_SUPER:
> + break;
> + default:
> + dev_err(priv_dev->dev,
> + "invalid maximum_speed parameter %d\n",
> + max_speed);
> + /* fall through */
> + case USB_SPEED_UNKNOWN:
> + /* default to superspeed */
> + max_speed = USB_SPEED_SUPER;
> + break;
> + }
> +
> cdns3_gadget_config(priv_dev);
> spin_unlock_irqrestore(&priv_dev->lock, flags);
> return 0;
> @@ -2570,12 +2596,7 @@ static int cdns3_gadget_start(struct cdns3 *cdns)
> /* Check the maximum_speed parameter */
> switch (max_speed) {
> case USB_SPEED_FULL:
> - writel(USB_CONF_SFORCE_FS, &priv_dev->regs->usb_conf);
> - writel(USB_CONF_USB3DIS, &priv_dev->regs->usb_conf);
> - break;
> case USB_SPEED_HIGH:
> - writel(USB_CONF_USB3DIS, &priv_dev->regs->usb_conf);
> - break;
> case USB_SPEED_SUPER:
> break;
> default:
Just a small comment:
You could delete switch-case at cdns3_gadget_start, and just use
if() statement, eg:
max_speed = usb_get_maximum_speed(cdns->dev);
if (max_speed == USB_SPEED_UNKNOWN)
max_speed = USB_SPEED_SUPER;
Otherwise:
Acked-by: Peter Chen <peter.chen@nxp.com>
--
Thanks,
Peter Chen
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] usb: cdns3: gadget: Fix g_audio use case when connected to Super-Speed host
2019-10-30 6:36 ` Peter Chen
@ 2019-10-30 8:44 ` Roger Quadros
2019-10-30 8:56 ` Peter Chen
0 siblings, 1 reply; 17+ messages in thread
From: Roger Quadros @ 2019-10-30 8:44 UTC (permalink / raw)
To: Peter Chen
Cc: felipe.balbi, gregkh, pawell, nsekhar, kurahul, linux-usb, linux-kernel
On 30/10/2019 08:36, Peter Chen wrote:
> On 19-10-29 17:15:14, Roger Quadros wrote:
>> Take into account gadget driver's speed limit when programming
>> controller speed.
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>> Hi Greg,
>>
>> Please apply this for -rc.
>> Without this, g_audio is broken on cdns3 USB controller is
>> connected to a Super-Speed host.
>>
>> cheers,
>> -roger
>>
>> drivers/usb/cdns3/gadget.c | 31 ++++++++++++++++++++++++++-----
>> 1 file changed, 26 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
>> index 40dad4e8d0dc..1c724c20d468 100644
>> --- a/drivers/usb/cdns3/gadget.c
>> +++ b/drivers/usb/cdns3/gadget.c
>> @@ -2338,9 +2338,35 @@ 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;
>>
>> spin_lock_irqsave(&priv_dev->lock, flags);
>> priv_dev->gadget_driver = driver;
>> +
>> + /* limit speed if necessary */
>> + max_speed = min(driver->max_speed, gadget->max_speed);
>> +
>> + switch (max_speed) {
>> + case USB_SPEED_FULL:
>> + writel(USB_CONF_SFORCE_FS, &priv_dev->regs->usb_conf);
>> + writel(USB_CONF_USB3DIS, &priv_dev->regs->usb_conf);
>> + break;
>> + case USB_SPEED_HIGH:
>> + writel(USB_CONF_USB3DIS, &priv_dev->regs->usb_conf);
>> + break;
>> + case USB_SPEED_SUPER:
>> + break;
>> + default:
>> + dev_err(priv_dev->dev,
>> + "invalid maximum_speed parameter %d\n",
>> + max_speed);
>> + /* fall through */
>> + case USB_SPEED_UNKNOWN:
>> + /* default to superspeed */
>> + max_speed = USB_SPEED_SUPER;
>> + break;
>> + }
>> +
>> cdns3_gadget_config(priv_dev);
>> spin_unlock_irqrestore(&priv_dev->lock, flags);
>> return 0;
>> @@ -2570,12 +2596,7 @@ static int cdns3_gadget_start(struct cdns3 *cdns)
>> /* Check the maximum_speed parameter */
>> switch (max_speed) {
>> case USB_SPEED_FULL:
>> - writel(USB_CONF_SFORCE_FS, &priv_dev->regs->usb_conf);
>> - writel(USB_CONF_USB3DIS, &priv_dev->regs->usb_conf);
>> - break;
>> case USB_SPEED_HIGH:
>> - writel(USB_CONF_USB3DIS, &priv_dev->regs->usb_conf);
>> - break;
>> case USB_SPEED_SUPER:
>> break;
>> default:
>
> Just a small comment:
>
> You could delete switch-case at cdns3_gadget_start, and just use
> if() statement, eg:
>
> max_speed = usb_get_maximum_speed(cdns->dev);
> if (max_speed == USB_SPEED_UNKNOWN)
> max_speed = USB_SPEED_SUPER;
But then it will not take care of bailing out for USB_SPEED_WIRELESS,
USB_SPEED_SUPER_PLUS and any future speeds.
>
> Otherwise:
>
> Acked-by: Peter Chen <peter.chen@nxp.com>
>
--
cheers,
-roger
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] usb: cdns3: gadget: Fix g_audio use case when connected to Super-Speed host
2019-10-30 8:44 ` Roger Quadros
@ 2019-10-30 8:56 ` Peter Chen
0 siblings, 0 replies; 17+ messages in thread
From: Peter Chen @ 2019-10-30 8:56 UTC (permalink / raw)
To: Roger Quadros
Cc: felipe.balbi, gregkh, pawell, nsekhar, kurahul, linux-usb, linux-kernel
On 19-10-30 10:44:10, Roger Quadros wrote:
>
>
> On 30/10/2019 08:36, Peter Chen wrote:
> > On 19-10-29 17:15:14, Roger Quadros wrote:
> > > Take into account gadget driver's speed limit when programming
> > > controller speed.
> > >
> > > Signed-off-by: Roger Quadros <rogerq@ti.com>
> > > ---
> > > Hi Greg,
> > >
> > > Please apply this for -rc.
> > > Without this, g_audio is broken on cdns3 USB controller is
> > > connected to a Super-Speed host.
> > >
> > > cheers,
> > > -roger
> > >
> > > drivers/usb/cdns3/gadget.c | 31 ++++++++++++++++++++++++++-----
> > > 1 file changed, 26 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
> > > index 40dad4e8d0dc..1c724c20d468 100644
> > > --- a/drivers/usb/cdns3/gadget.c
> > > +++ b/drivers/usb/cdns3/gadget.c
> > > @@ -2338,9 +2338,35 @@ 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;
> > > spin_lock_irqsave(&priv_dev->lock, flags);
> > > priv_dev->gadget_driver = driver;
> > > +
> > > + /* limit speed if necessary */
> > > + max_speed = min(driver->max_speed, gadget->max_speed);
> > > +
> > > + switch (max_speed) {
> > > + case USB_SPEED_FULL:
> > > + writel(USB_CONF_SFORCE_FS, &priv_dev->regs->usb_conf);
> > > + writel(USB_CONF_USB3DIS, &priv_dev->regs->usb_conf);
> > > + break;
> > > + case USB_SPEED_HIGH:
> > > + writel(USB_CONF_USB3DIS, &priv_dev->regs->usb_conf);
> > > + break;
> > > + case USB_SPEED_SUPER:
> > > + break;
> > > + default:
> > > + dev_err(priv_dev->dev,
> > > + "invalid maximum_speed parameter %d\n",
> > > + max_speed);
> > > + /* fall through */
> > > + case USB_SPEED_UNKNOWN:
> > > + /* default to superspeed */
> > > + max_speed = USB_SPEED_SUPER;
> > > + break;
> > > + }
> > > +
> > > cdns3_gadget_config(priv_dev);
> > > spin_unlock_irqrestore(&priv_dev->lock, flags);
> > > return 0;
> > > @@ -2570,12 +2596,7 @@ static int cdns3_gadget_start(struct cdns3 *cdns)
> > > /* Check the maximum_speed parameter */
> > > switch (max_speed) {
> > > case USB_SPEED_FULL:
> > > - writel(USB_CONF_SFORCE_FS, &priv_dev->regs->usb_conf);
> > > - writel(USB_CONF_USB3DIS, &priv_dev->regs->usb_conf);
> > > - break;
> > > case USB_SPEED_HIGH:
> > > - writel(USB_CONF_USB3DIS, &priv_dev->regs->usb_conf);
> > > - break;
> > > case USB_SPEED_SUPER:
> > > break;
> > > default:
> >
> > Just a small comment:
> >
> > You could delete switch-case at cdns3_gadget_start, and just use
> > if() statement, eg:
> >
> > max_speed = usb_get_maximum_speed(cdns->dev);
> > if (max_speed == USB_SPEED_UNKNOWN)
> > max_speed = USB_SPEED_SUPER;
>
> But then it will not take care of bailing out for USB_SPEED_WIRELESS,
> USB_SPEED_SUPER_PLUS and any future speeds.
This IP only supports FS/HS/SS. It doesn't a big issue, if you would
like to keep the code like your patch, it is also OK.
--
Thanks,
Peter Chen
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] usb: cdns3: gadget: Fix g_audio use case when connected to Super-Speed host
2019-10-29 15:15 [PATCH] usb: cdns3: gadget: Fix g_audio use case when connected to Super-Speed host Roger Quadros
2019-10-30 6:36 ` Peter Chen
@ 2019-10-30 11:46 ` Felipe Balbi
2019-10-30 11:51 ` Greg KH
2019-10-30 12:16 ` [PATCH v2] " Roger Quadros
2 siblings, 1 reply; 17+ messages in thread
From: Felipe Balbi @ 2019-10-30 11:46 UTC (permalink / raw)
To: Roger Quadros, gregkh
Cc: pawell, peter.chen, nsekhar, kurahul, linux-usb, linux-kernel,
Roger Quadros
Hi,
Roger Quadros <rogerq@ti.com> writes:
> Take into account gadget driver's speed limit when programming
> controller speed.
>
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
> Hi Greg,
>
> Please apply this for -rc.
if you want this in -rc, you should have a Fixes line there.
> Without this, g_audio is broken on cdns3 USB controller is
> connected to a Super-Speed host.
>
> cheers,
> -roger
>
> drivers/usb/cdns3/gadget.c | 31 ++++++++++++++++++++++++++-----
> 1 file changed, 26 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
> index 40dad4e8d0dc..1c724c20d468 100644
> --- a/drivers/usb/cdns3/gadget.c
> +++ b/drivers/usb/cdns3/gadget.c
> @@ -2338,9 +2338,35 @@ 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;
>
> spin_lock_irqsave(&priv_dev->lock, flags);
> priv_dev->gadget_driver = driver;
> +
> + /* limit speed if necessary */
> + max_speed = min(driver->max_speed, gadget->max_speed);
> +
> + switch (max_speed) {
> + case USB_SPEED_FULL:
> + writel(USB_CONF_SFORCE_FS, &priv_dev->regs->usb_conf);
> + writel(USB_CONF_USB3DIS, &priv_dev->regs->usb_conf);
> + break;
> + case USB_SPEED_HIGH:
> + writel(USB_CONF_USB3DIS, &priv_dev->regs->usb_conf);
> + break;
seems like this can be simplified a little:
switch (max_speed) {
case USB_SPEED_FULL:
writel(USB_CONF_SFORCE_FS, &priv_dev->regs->usb_conf);
/* fallthrough */
case USB_SPEED_HIGH:
writel(USB_CONF_USB3DIS, &priv_dev->regs->usb_conf);
/* fallthrough */
case USB_SPEED_SUPER:
break;
[...]
--
balbi
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] usb: cdns3: gadget: Fix g_audio use case when connected to Super-Speed host
2019-10-30 11:46 ` Felipe Balbi
@ 2019-10-30 11:51 ` Greg KH
0 siblings, 0 replies; 17+ messages in thread
From: Greg KH @ 2019-10-30 11:51 UTC (permalink / raw)
To: Felipe Balbi
Cc: Roger Quadros, pawell, peter.chen, nsekhar, kurahul, linux-usb,
linux-kernel
On Wed, Oct 30, 2019 at 01:46:07PM +0200, Felipe Balbi wrote:
>
> Hi,
>
> Roger Quadros <rogerq@ti.com> writes:
>
> > Take into account gadget driver's speed limit when programming
> > controller speed.
> >
> > Signed-off-by: Roger Quadros <rogerq@ti.com>
> > ---
> > Hi Greg,
> >
> > Please apply this for -rc.
>
> if you want this in -rc, you should have a Fixes line there.
I agree, I can't take this for -rc as-is :(
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2] usb: cdns3: gadget: Fix g_audio use case when connected to Super-Speed host
2019-10-29 15:15 [PATCH] usb: cdns3: gadget: Fix g_audio use case when connected to Super-Speed host Roger Quadros
2019-10-30 6:36 ` Peter Chen
2019-10-30 11:46 ` Felipe Balbi
@ 2019-10-30 12:16 ` Roger Quadros
2019-10-30 13:30 ` Greg KH
2019-10-31 8:55 ` Felipe Balbi
2 siblings, 2 replies; 17+ messages in thread
From: Roger Quadros @ 2019-10-30 12:16 UTC (permalink / raw)
To: felipe.balbi, gregkh
Cc: pawell, peter.chen, nsekhar, kurahul, linux-usb, linux-kernel,
Roger Quadros
Take into account gadget driver's speed limit when programming
controller speed.
Fixes: commit 7733f6c32e36 ("usb: cdns3: Add Cadence USB3 DRD Driver")
Signed-off-by: Roger Quadros <rogerq@ti.com>
Acked-by: Peter Chen <peter.chen@nxp.com>
---
Changelog:
v2
- Add Fixes line
drivers/usb/cdns3/gadget.c | 31 ++++++++++++++++++++++++++-----
1 file changed, 26 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
index 40dad4e8d0dc..1c724c20d468 100644
--- a/drivers/usb/cdns3/gadget.c
+++ b/drivers/usb/cdns3/gadget.c
@@ -2338,9 +2338,35 @@ 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;
spin_lock_irqsave(&priv_dev->lock, flags);
priv_dev->gadget_driver = driver;
+
+ /* limit speed if necessary */
+ max_speed = min(driver->max_speed, gadget->max_speed);
+
+ switch (max_speed) {
+ case USB_SPEED_FULL:
+ writel(USB_CONF_SFORCE_FS, &priv_dev->regs->usb_conf);
+ writel(USB_CONF_USB3DIS, &priv_dev->regs->usb_conf);
+ break;
+ case USB_SPEED_HIGH:
+ writel(USB_CONF_USB3DIS, &priv_dev->regs->usb_conf);
+ break;
+ case USB_SPEED_SUPER:
+ break;
+ default:
+ dev_err(priv_dev->dev,
+ "invalid maximum_speed parameter %d\n",
+ max_speed);
+ /* fall through */
+ case USB_SPEED_UNKNOWN:
+ /* default to superspeed */
+ max_speed = USB_SPEED_SUPER;
+ break;
+ }
+
cdns3_gadget_config(priv_dev);
spin_unlock_irqrestore(&priv_dev->lock, flags);
return 0;
@@ -2570,12 +2596,7 @@ static int cdns3_gadget_start(struct cdns3 *cdns)
/* Check the maximum_speed parameter */
switch (max_speed) {
case USB_SPEED_FULL:
- writel(USB_CONF_SFORCE_FS, &priv_dev->regs->usb_conf);
- writel(USB_CONF_USB3DIS, &priv_dev->regs->usb_conf);
- break;
case USB_SPEED_HIGH:
- writel(USB_CONF_USB3DIS, &priv_dev->regs->usb_conf);
- break;
case USB_SPEED_SUPER:
break;
default:
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2] usb: cdns3: gadget: Fix g_audio use case when connected to Super-Speed host
2019-10-30 12:16 ` [PATCH v2] " Roger Quadros
@ 2019-10-30 13:30 ` Greg KH
2019-10-30 14:20 ` Roger Quadros
2019-10-31 8:55 ` Felipe Balbi
1 sibling, 1 reply; 17+ messages in thread
From: Greg KH @ 2019-10-30 13:30 UTC (permalink / raw)
To: Roger Quadros
Cc: felipe.balbi, pawell, peter.chen, nsekhar, kurahul, linux-usb,
linux-kernel
On Wed, Oct 30, 2019 at 02:16:07PM +0200, Roger Quadros wrote:
> Take into account gadget driver's speed limit when programming
> controller speed.
>
> Fixes: commit 7733f6c32e36 ("usb: cdns3: Add Cadence USB3 DRD Driver")
No need for "commit", doesn't the documentation say the correct format?
I haven't looked in a while...
I can edit it out this time...
greg k-h
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] usb: cdns3: gadget: Fix g_audio use case when connected to Super-Speed host
2019-10-30 13:30 ` Greg KH
@ 2019-10-30 14:20 ` Roger Quadros
0 siblings, 0 replies; 17+ messages in thread
From: Roger Quadros @ 2019-10-30 14:20 UTC (permalink / raw)
To: Greg KH
Cc: felipe.balbi, pawell, peter.chen, nsekhar, kurahul, linux-usb,
linux-kernel
On 30/10/2019 15:30, Greg KH wrote:
> On Wed, Oct 30, 2019 at 02:16:07PM +0200, Roger Quadros wrote:
>> Take into account gadget driver's speed limit when programming
>> controller speed.
>>
>> Fixes: commit 7733f6c32e36 ("usb: cdns3: Add Cadence USB3 DRD Driver")
>
> No need for "commit", doesn't the documentation say the correct format?
> I haven't looked in a while...
>
Sorry, my bad.
> I can edit it out this time...
Thanks!
>
> greg k-h
>
--
cheers,
-roger
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] usb: cdns3: gadget: Fix g_audio use case when connected to Super-Speed host
2019-10-30 12:16 ` [PATCH v2] " Roger Quadros
2019-10-30 13:30 ` Greg KH
@ 2019-10-31 8:55 ` Felipe Balbi
2019-10-31 10:35 ` Roger Quadros
1 sibling, 1 reply; 17+ messages in thread
From: Felipe Balbi @ 2019-10-31 8:55 UTC (permalink / raw)
To: Roger Quadros, gregkh
Cc: pawell, peter.chen, nsekhar, kurahul, linux-usb, linux-kernel,
Roger Quadros
Hi,
Roger Quadros <rogerq@ti.com> writes:
> Take into account gadget driver's speed limit when programming
> controller speed.
>
> Fixes: commit 7733f6c32e36 ("usb: cdns3: Add Cadence USB3 DRD Driver")
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> Acked-by: Peter Chen <peter.chen@nxp.com>
> ---
>
> Changelog:
> v2
> - Add Fixes line
>
> drivers/usb/cdns3/gadget.c | 31 ++++++++++++++++++++++++++-----
> 1 file changed, 26 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
> index 40dad4e8d0dc..1c724c20d468 100644
> --- a/drivers/usb/cdns3/gadget.c
> +++ b/drivers/usb/cdns3/gadget.c
> @@ -2338,9 +2338,35 @@ 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;
>
> spin_lock_irqsave(&priv_dev->lock, flags);
> priv_dev->gadget_driver = driver;
> +
> + /* limit speed if necessary */
> + max_speed = min(driver->max_speed, gadget->max_speed);
> +
> + switch (max_speed) {
> + case USB_SPEED_FULL:
> + writel(USB_CONF_SFORCE_FS, &priv_dev->regs->usb_conf);
> + writel(USB_CONF_USB3DIS, &priv_dev->regs->usb_conf);
> + break;
> + case USB_SPEED_HIGH:
> + writel(USB_CONF_USB3DIS, &priv_dev->regs->usb_conf);
> + break;
> + case USB_SPEED_SUPER:
> + break;
> + default:
> + dev_err(priv_dev->dev,
> + "invalid maximum_speed parameter %d\n",
> + max_speed);
> + /* fall through */
> + case USB_SPEED_UNKNOWN:
> + /* default to superspeed */
> + max_speed = USB_SPEED_SUPER;
> + break;
> + }
I had suggested some simplification for this case statement.
--
balbi
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] usb: cdns3: gadget: Fix g_audio use case when connected to Super-Speed host
2019-10-31 8:55 ` Felipe Balbi
@ 2019-10-31 10:35 ` Roger Quadros
2019-10-31 10:55 ` Felipe Balbi
0 siblings, 1 reply; 17+ messages in thread
From: Roger Quadros @ 2019-10-31 10:35 UTC (permalink / raw)
To: Felipe Balbi, gregkh, pawell
Cc: peter.chen, nsekhar, kurahul, linux-usb, linux-kernel
Hi,
On 31/10/2019 10:55, Felipe Balbi wrote:
>
> Hi,
>
> Roger Quadros <rogerq@ti.com> writes:
>
>> Take into account gadget driver's speed limit when programming
>> controller speed.
>>
>> Fixes: commit 7733f6c32e36 ("usb: cdns3: Add Cadence USB3 DRD Driver")
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> Acked-by: Peter Chen <peter.chen@nxp.com>
>> ---
>>
>> Changelog:
>> v2
>> - Add Fixes line
>>
>> drivers/usb/cdns3/gadget.c | 31 ++++++++++++++++++++++++++-----
>> 1 file changed, 26 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
>> index 40dad4e8d0dc..1c724c20d468 100644
>> --- a/drivers/usb/cdns3/gadget.c
>> +++ b/drivers/usb/cdns3/gadget.c
>> @@ -2338,9 +2338,35 @@ 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;
>>
>> spin_lock_irqsave(&priv_dev->lock, flags);
>> priv_dev->gadget_driver = driver;
>> +
>> + /* limit speed if necessary */
>> + max_speed = min(driver->max_speed, gadget->max_speed);
>> +
>> + switch (max_speed) {
>> + case USB_SPEED_FULL:
>> + writel(USB_CONF_SFORCE_FS, &priv_dev->regs->usb_conf);
>> + writel(USB_CONF_USB3DIS, &priv_dev->regs->usb_conf);
>> + break;
>> + case USB_SPEED_HIGH:
>> + writel(USB_CONF_USB3DIS, &priv_dev->regs->usb_conf);
>> + break;
>> + case USB_SPEED_SUPER:
>> + break;
>> + default:
>> + dev_err(priv_dev->dev,
>> + "invalid maximum_speed parameter %d\n",
>> + max_speed);
>> + /* fall through */
>> + case USB_SPEED_UNKNOWN:
>> + /* default to superspeed */
>> + max_speed = USB_SPEED_SUPER;
>> + break;
>> + }
>
> I had suggested some simplification for this case statement.
>
oops, looks like Greg picked this already.
During more tests today I just observed that this patch causes
the following regression.
Connect EVM to Super-Speed host
Load g_audio. (this enumerates as HS which is fine)
unload g_audio
load g_zero (this enumerates at HS instead of SS).
This is because the speed limit that we set doesn't get cleared.
Now the bits are write only and there is a way to undo USB_CONF_SFORCE_FS
by writing USB_CONF_CFORCE_FS, however there is no corresponding bit
to clear USB_CONF_USB3DIS. Only way seems to be USB_CFG_SWRST which
is a bit harsh IMO.
Pawel,
could you please confirm what is the best way to undo the high-speed
limit?
--
cheers,
-roger
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] usb: cdns3: gadget: Fix g_audio use case when connected to Super-Speed host
2019-10-31 10:35 ` Roger Quadros
@ 2019-10-31 10:55 ` Felipe Balbi
2019-10-31 11:02 ` Roger Quadros
0 siblings, 1 reply; 17+ messages in thread
From: Felipe Balbi @ 2019-10-31 10:55 UTC (permalink / raw)
To: Roger Quadros, gregkh, pawell
Cc: peter.chen, nsekhar, kurahul, linux-usb, linux-kernel
Hi,
Roger Quadros <rogerq@ti.com> writes:
> Hi,
>
> On 31/10/2019 10:55, Felipe Balbi wrote:
>>
>> Hi,
>>
>> Roger Quadros <rogerq@ti.com> writes:
>>
>>> Take into account gadget driver's speed limit when programming
>>> controller speed.
>>>
>>> Fixes: commit 7733f6c32e36 ("usb: cdns3: Add Cadence USB3 DRD Driver")
>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>> Acked-by: Peter Chen <peter.chen@nxp.com>
>>> ---
>>>
>>> Changelog:
>>> v2
>>> - Add Fixes line
>>>
>>> drivers/usb/cdns3/gadget.c | 31 ++++++++++++++++++++++++++-----
>>> 1 file changed, 26 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
>>> index 40dad4e8d0dc..1c724c20d468 100644
>>> --- a/drivers/usb/cdns3/gadget.c
>>> +++ b/drivers/usb/cdns3/gadget.c
>>> @@ -2338,9 +2338,35 @@ 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;
>>>
>>> spin_lock_irqsave(&priv_dev->lock, flags);
>>> priv_dev->gadget_driver = driver;
>>> +
>>> + /* limit speed if necessary */
>>> + max_speed = min(driver->max_speed, gadget->max_speed);
>>> +
>>> + switch (max_speed) {
>>> + case USB_SPEED_FULL:
>>> + writel(USB_CONF_SFORCE_FS, &priv_dev->regs->usb_conf);
so this forces the controller to FS
>>> + writel(USB_CONF_USB3DIS, &priv_dev->regs->usb_conf);
and this disconnects in superspeed? What is this supposed to do?
>>> + break;
>>> + case USB_SPEED_HIGH:
>>> + writel(USB_CONF_USB3DIS, &priv_dev->regs->usb_conf);
>>> + break;
>>> + case USB_SPEED_SUPER:
>>> + break;
>>> + default:
>>> + dev_err(priv_dev->dev,
>>> + "invalid maximum_speed parameter %d\n",
>>> + max_speed);
>>> + /* fall through */
>>> + case USB_SPEED_UNKNOWN:
>>> + /* default to superspeed */
>>> + max_speed = USB_SPEED_SUPER;
>>> + break;
>>> + }
>>
>> I had suggested some simplification for this case statement.
>>
>
> oops, looks like Greg picked this already.
>
> During more tests today I just observed that this patch causes
> the following regression.
>
> Connect EVM to Super-Speed host
> Load g_audio. (this enumerates as HS which is fine)
> unload g_audio
> load g_zero (this enumerates at HS instead of SS).
>
> This is because the speed limit that we set doesn't get cleared.
>
> Now the bits are write only and there is a way to undo USB_CONF_SFORCE_FS
> by writing USB_CONF_CFORCE_FS, however there is no corresponding bit
> to clear USB_CONF_USB3DIS. Only way seems to be USB_CFG_SWRST which
> is a bit harsh IMO.
Isn't bit 0 enough?
/* Reset USB device configuration. */
#define USB_CONF_CFGRST BIT(0)
Also, now that I look at this more carefully, you should move that code
to udc_set_speed().
--
balbi
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] usb: cdns3: gadget: Fix g_audio use case when connected to Super-Speed host
2019-10-31 10:55 ` Felipe Balbi
@ 2019-10-31 11:02 ` Roger Quadros
2019-10-31 11:08 ` Felipe Balbi
2019-11-03 8:17 ` Pawel Laszczak
0 siblings, 2 replies; 17+ messages in thread
From: Roger Quadros @ 2019-10-31 11:02 UTC (permalink / raw)
To: Felipe Balbi, gregkh, pawell
Cc: peter.chen, nsekhar, kurahul, linux-usb, linux-kernel
On 31/10/2019 12:55, Felipe Balbi wrote:
>
> Hi,
>
> Roger Quadros <rogerq@ti.com> writes:
>
>> Hi,
>>
>> On 31/10/2019 10:55, Felipe Balbi wrote:
>>>
>>> Hi,
>>>
>>> Roger Quadros <rogerq@ti.com> writes:
>>>
>>>> Take into account gadget driver's speed limit when programming
>>>> controller speed.
>>>>
>>>> Fixes: commit 7733f6c32e36 ("usb: cdns3: Add Cadence USB3 DRD Driver")
>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>> Acked-by: Peter Chen <peter.chen@nxp.com>
>>>> ---
>>>>
>>>> Changelog:
>>>> v2
>>>> - Add Fixes line
>>>>
>>>> drivers/usb/cdns3/gadget.c | 31 ++++++++++++++++++++++++++-----
>>>> 1 file changed, 26 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
>>>> index 40dad4e8d0dc..1c724c20d468 100644
>>>> --- a/drivers/usb/cdns3/gadget.c
>>>> +++ b/drivers/usb/cdns3/gadget.c
>>>> @@ -2338,9 +2338,35 @@ 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;
>>>>
>>>> spin_lock_irqsave(&priv_dev->lock, flags);
>>>> priv_dev->gadget_driver = driver;
>>>> +
>>>> + /* limit speed if necessary */
>>>> + max_speed = min(driver->max_speed, gadget->max_speed);
>>>> +
>>>> + switch (max_speed) {
>>>> + case USB_SPEED_FULL:
>>>> + writel(USB_CONF_SFORCE_FS, &priv_dev->regs->usb_conf);
>
> so this forces the controller to FS
Right.
>
>>>> + writel(USB_CONF_USB3DIS, &priv_dev->regs->usb_conf);
>
> and this disconnects in superspeed? What is this supposed to do?
>
It says "Disconnect USB device in SuperSpeed".
We were asked to set that bit to limit it to HS.
>>>> + break;
>>>> + case USB_SPEED_HIGH:
>>>> + writel(USB_CONF_USB3DIS, &priv_dev->regs->usb_conf);
>>>> + break;
>>>> + case USB_SPEED_SUPER:
>>>> + break;
>>>> + default:
>>>> + dev_err(priv_dev->dev,
>>>> + "invalid maximum_speed parameter %d\n",
>>>> + max_speed);
>>>> + /* fall through */
>>>> + case USB_SPEED_UNKNOWN:
>>>> + /* default to superspeed */
>>>> + max_speed = USB_SPEED_SUPER;
>>>> + break;
>>>> + }
>>>
>>> I had suggested some simplification for this case statement.
>>>
>>
>> oops, looks like Greg picked this already.
>>
>> During more tests today I just observed that this patch causes
>> the following regression.
>>
>> Connect EVM to Super-Speed host
>> Load g_audio. (this enumerates as HS which is fine)
>> unload g_audio
>> load g_zero (this enumerates at HS instead of SS).
>>
>> This is because the speed limit that we set doesn't get cleared.
>>
>> Now the bits are write only and there is a way to undo USB_CONF_SFORCE_FS
>> by writing USB_CONF_CFORCE_FS, however there is no corresponding bit
>> to clear USB_CONF_USB3DIS. Only way seems to be USB_CFG_SWRST which
>> is a bit harsh IMO.
>
> Isn't bit 0 enough?
>
> /* Reset USB device configuration. */
> #define USB_CONF_CFGRST BIT(0)
Probably not, as explanation of USB3DIS bit says,
"To connect disconnected device, CPU performs
software reset (CFG.SWRST)." which is bit 7. "Device software reset.
But I'll let Pawel comment on this.
>
> Also, now that I look at this more carefully, you should move that code
> to udc_set_speed().
>
Agreed. I'll revise the implementation to move it to udc_set_speed()
once I know how to undo the USB3DIS.
--
cheers,
-roger
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] usb: cdns3: gadget: Fix g_audio use case when connected to Super-Speed host
2019-10-31 11:02 ` Roger Quadros
@ 2019-10-31 11:08 ` Felipe Balbi
2019-11-03 8:17 ` Pawel Laszczak
1 sibling, 0 replies; 17+ messages in thread
From: Felipe Balbi @ 2019-10-31 11:08 UTC (permalink / raw)
To: Roger Quadros, gregkh, pawell
Cc: peter.chen, nsekhar, kurahul, linux-usb, linux-kernel
Hi,
Roger Quadros <rogerq@ti.com> writes:
>>>>> Take into account gadget driver's speed limit when programming
>>>>> controller speed.
>>>>>
>>>>> Fixes: commit 7733f6c32e36 ("usb: cdns3: Add Cadence USB3 DRD Driver")
>>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>>> Acked-by: Peter Chen <peter.chen@nxp.com>
>>>>> ---
>>>>>
>>>>> Changelog:
>>>>> v2
>>>>> - Add Fixes line
>>>>>
>>>>> drivers/usb/cdns3/gadget.c | 31 ++++++++++++++++++++++++++-----
>>>>> 1 file changed, 26 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
>>>>> index 40dad4e8d0dc..1c724c20d468 100644
>>>>> --- a/drivers/usb/cdns3/gadget.c
>>>>> +++ b/drivers/usb/cdns3/gadget.c
>>>>> @@ -2338,9 +2338,35 @@ 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;
>>>>>
>>>>> spin_lock_irqsave(&priv_dev->lock, flags);
>>>>> priv_dev->gadget_driver = driver;
>>>>> +
>>>>> + /* limit speed if necessary */
>>>>> + max_speed = min(driver->max_speed, gadget->max_speed);
>>>>> +
>>>>> + switch (max_speed) {
>>>>> + case USB_SPEED_FULL:
>>>>> + writel(USB_CONF_SFORCE_FS, &priv_dev->regs->usb_conf);
>>
>> so this forces the controller to FS
>
> Right.
>
>>
>>>>> + writel(USB_CONF_USB3DIS, &priv_dev->regs->usb_conf);
>>
>> and this disconnects in superspeed? What is this supposed to do?
>>
>
> It says "Disconnect USB device in SuperSpeed".
>
> We were asked to set that bit to limit it to HS.
I wonder if by "Disconnect USB device in SuperSpeed" means tristating
the PIPE3 interface, therefore forcing RX.Detect to fail.
>>>>> + break;
>>>>> + case USB_SPEED_HIGH:
>>>>> + writel(USB_CONF_USB3DIS, &priv_dev->regs->usb_conf);
>>>>> + break;
>>>>> + case USB_SPEED_SUPER:
>>>>> + break;
>>>>> + default:
>>>>> + dev_err(priv_dev->dev,
>>>>> + "invalid maximum_speed parameter %d\n",
>>>>> + max_speed);
>>>>> + /* fall through */
>>>>> + case USB_SPEED_UNKNOWN:
>>>>> + /* default to superspeed */
>>>>> + max_speed = USB_SPEED_SUPER;
>>>>> + break;
>>>>> + }
>>>>
>>>> I had suggested some simplification for this case statement.
>>>>
>>>
>>> oops, looks like Greg picked this already.
>>>
>>> During more tests today I just observed that this patch causes
>>> the following regression.
>>>
>>> Connect EVM to Super-Speed host
>>> Load g_audio. (this enumerates as HS which is fine)
>>> unload g_audio
>>> load g_zero (this enumerates at HS instead of SS).
>>>
>>> This is because the speed limit that we set doesn't get cleared.
>>>
>>> Now the bits are write only and there is a way to undo USB_CONF_SFORCE_FS
>>> by writing USB_CONF_CFORCE_FS, however there is no corresponding bit
>>> to clear USB_CONF_USB3DIS. Only way seems to be USB_CFG_SWRST which
>>> is a bit harsh IMO.
>>
>> Isn't bit 0 enough?
>>
>> /* Reset USB device configuration. */
>> #define USB_CONF_CFGRST BIT(0)
>
> Probably not, as explanation of USB3DIS bit says,
> "To connect disconnected device, CPU performs
> software reset (CFG.SWRST)." which is bit 7. "Device software reset.
ouch
> But I'll let Pawel comment on this.
ok
>> Also, now that I look at this more carefully, you should move that code
>> to udc_set_speed().
>>
>
> Agreed. I'll revise the implementation to move it to udc_set_speed()
> once I know how to undo the USB3DIS.
Sure
--
balbi
^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH v2] usb: cdns3: gadget: Fix g_audio use case when connected to Super-Speed host
2019-10-31 11:02 ` Roger Quadros
2019-10-31 11:08 ` Felipe Balbi
@ 2019-11-03 8:17 ` Pawel Laszczak
2019-11-04 9:18 ` Roger Quadros
1 sibling, 1 reply; 17+ messages in thread
From: Pawel Laszczak @ 2019-11-03 8:17 UTC (permalink / raw)
To: Roger Quadros, Felipe Balbi, gregkh
Cc: peter.chen, nsekhar, Rahul Kumar, linux-usb, linux-kernel
Hi,
>>
>> Hi,
>>
>> Roger Quadros <rogerq@ti.com> writes:
>>
>>> Hi,
>>>
>>> On 31/10/2019 10:55, Felipe Balbi wrote:
>>>>
>>>> Hi,
>>>>
>>>> Roger Quadros <rogerq@ti.com> writes:
>>>>
>>>>> Take into account gadget driver's speed limit when programming
>>>>> controller speed.
>>>>>
>>>>> Fixes: commit 7733f6c32e36 ("usb: cdns3: Add Cadence USB3 DRD Driver")
>>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>>> Acked-by: Peter Chen <peter.chen@nxp.com>
>>>>> ---
>>>>>
>>>>> Changelog:
>>>>> v2
>>>>> - Add Fixes line
>>>>>
>>>>> drivers/usb/cdns3/gadget.c | 31 ++++++++++++++++++++++++++-----
>>>>> 1 file changed, 26 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
>>>>> index 40dad4e8d0dc..1c724c20d468 100644
>>>>> --- a/drivers/usb/cdns3/gadget.c
>>>>> +++ b/drivers/usb/cdns3/gadget.c
>>>>> @@ -2338,9 +2338,35 @@ 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;
>>>>>
>>>>> spin_lock_irqsave(&priv_dev->lock, flags);
>>>>> priv_dev->gadget_driver = driver;
>>>>> +
>>>>> + /* limit speed if necessary */
>>>>> + max_speed = min(driver->max_speed, gadget->max_speed);
>>>>> +
>>>>> + switch (max_speed) {
>>>>> + case USB_SPEED_FULL:
>>>>> + writel(USB_CONF_SFORCE_FS, &priv_dev->regs->usb_conf);
>>
>> so this forces the controller to FS
>
>Right.
>
>>
>>>>> + writel(USB_CONF_USB3DIS, &priv_dev->regs->usb_conf);
>>
>> and this disconnects in superspeed? What is this supposed to do?
>>
>
>It says "Disconnect USB device in SuperSpeed".
>
>We were asked to set that bit to limit it to HS.
>
>>>>> + break;
>>>>> + case USB_SPEED_HIGH:
>>>>> + writel(USB_CONF_USB3DIS, &priv_dev->regs->usb_conf);
>>>>> + break;
>>>>> + case USB_SPEED_SUPER:
>>>>> + break;
>>>>> + default:
>>>>> + dev_err(priv_dev->dev,
>>>>> + "invalid maximum_speed parameter %d\n",
>>>>> + max_speed);
>>>>> + /* fall through */
>>>>> + case USB_SPEED_UNKNOWN:
>>>>> + /* default to superspeed */
>>>>> + max_speed = USB_SPEED_SUPER;
>>>>> + break;
>>>>> + }
>>>>
>>>> I had suggested some simplification for this case statement.
>>>>
>>>
>>> oops, looks like Greg picked this already.
>>>
>>> During more tests today I just observed that this patch causes
>>> the following regression.
>>>
>>> Connect EVM to Super-Speed host
>>> Load g_audio. (this enumerates as HS which is fine)
>>> unload g_audio
>>> load g_zero (this enumerates at HS instead of SS).
>>>
>>> This is because the speed limit that we set doesn't get cleared.
>>>
>>> Now the bits are write only and there is a way to undo USB_CONF_SFORCE_FS
>>> by writing USB_CONF_CFORCE_FS, however there is no corresponding bit
>>> to clear USB_CONF_USB3DIS. Only way seems to be USB_CFG_SWRST which
>>> is a bit harsh IMO.
>>
>> Isn't bit 0 enough?
>>
>> /* Reset USB device configuration. */
>> #define USB_CONF_CFGRST BIT(0)
>
>Probably not, as explanation of USB3DIS bit says,
>"To connect disconnected device, CPU performs
>software reset (CFG.SWRST)." which is bit 7. "Device software reset.
>
>But I'll let Pawel comment on this.
Yes it's the only way to return back to SS mode.
The USB speed is detecting during connecting.
CFG.SWRST make reconnect.
Maybe it would be better to add SS support in g_audio function.
>>
>> Also, now that I look at this more carefully, you should move that code
>> to udc_set_speed().
>>
>
>Agreed. I'll revise the implementation to move it to udc_set_speed()
>once I know how to undo the USB3DIS.
>
>--
>cheers,
>-roger
>Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
>Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] usb: cdns3: gadget: Fix g_audio use case when connected to Super-Speed host
2019-11-03 8:17 ` Pawel Laszczak
@ 2019-11-04 9:18 ` Roger Quadros
2019-11-04 15:16 ` Pawel Laszczak
0 siblings, 1 reply; 17+ messages in thread
From: Roger Quadros @ 2019-11-04 9:18 UTC (permalink / raw)
To: Pawel Laszczak, Felipe Balbi, gregkh
Cc: peter.chen, nsekhar, Rahul Kumar, linux-usb, linux-kernel
On 03/11/2019 10:17, Pawel Laszczak wrote:
> Hi,
>
>>>
>>> Hi,
>>>
>>> Roger Quadros <rogerq@ti.com> writes:
>>>
>>>> Hi,
>>>>
>>>> On 31/10/2019 10:55, Felipe Balbi wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> Roger Quadros <rogerq@ti.com> writes:
>>>>>
>>>>>> Take into account gadget driver's speed limit when programming
>>>>>> controller speed.
>>>>>>
>>>>>> Fixes: commit 7733f6c32e36 ("usb: cdns3: Add Cadence USB3 DRD Driver")
>>>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>>>> Acked-by: Peter Chen <peter.chen@nxp.com>
>>>>>> ---
>>>>>>
>>>>>> Changelog:
>>>>>> v2
>>>>>> - Add Fixes line
>>>>>>
>>>>>> drivers/usb/cdns3/gadget.c | 31 ++++++++++++++++++++++++++-----
>>>>>> 1 file changed, 26 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
>>>>>> index 40dad4e8d0dc..1c724c20d468 100644
>>>>>> --- a/drivers/usb/cdns3/gadget.c
>>>>>> +++ b/drivers/usb/cdns3/gadget.c
>>>>>> @@ -2338,9 +2338,35 @@ 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;
>>>>>>
>>>>>> spin_lock_irqsave(&priv_dev->lock, flags);
>>>>>> priv_dev->gadget_driver = driver;
>>>>>> +
>>>>>> + /* limit speed if necessary */
>>>>>> + max_speed = min(driver->max_speed, gadget->max_speed);
>>>>>> +
>>>>>> + switch (max_speed) {
>>>>>> + case USB_SPEED_FULL:
>>>>>> + writel(USB_CONF_SFORCE_FS, &priv_dev->regs->usb_conf);
>>>
>>> so this forces the controller to FS
>>
>> Right.
>>
>>>
>>>>>> + writel(USB_CONF_USB3DIS, &priv_dev->regs->usb_conf);
>>>
>>> and this disconnects in superspeed? What is this supposed to do?
>>>
>>
>> It says "Disconnect USB device in SuperSpeed".
>>
>> We were asked to set that bit to limit it to HS.
>>
>>>>>> + break;
>>>>>> + case USB_SPEED_HIGH:
>>>>>> + writel(USB_CONF_USB3DIS, &priv_dev->regs->usb_conf);
>>>>>> + break;
>>>>>> + case USB_SPEED_SUPER:
>>>>>> + break;
>>>>>> + default:
>>>>>> + dev_err(priv_dev->dev,
>>>>>> + "invalid maximum_speed parameter %d\n",
>>>>>> + max_speed);
>>>>>> + /* fall through */
>>>>>> + case USB_SPEED_UNKNOWN:
>>>>>> + /* default to superspeed */
>>>>>> + max_speed = USB_SPEED_SUPER;
>>>>>> + break;
>>>>>> + }
>>>>>
>>>>> I had suggested some simplification for this case statement.
>>>>>
>>>>
>>>> oops, looks like Greg picked this already.
>>>>
>>>> During more tests today I just observed that this patch causes
>>>> the following regression.
>>>>
>>>> Connect EVM to Super-Speed host
>>>> Load g_audio. (this enumerates as HS which is fine)
>>>> unload g_audio
>>>> load g_zero (this enumerates at HS instead of SS).
>>>>
>>>> This is because the speed limit that we set doesn't get cleared.
>>>>
>>>> Now the bits are write only and there is a way to undo USB_CONF_SFORCE_FS
>>>> by writing USB_CONF_CFORCE_FS, however there is no corresponding bit
>>>> to clear USB_CONF_USB3DIS. Only way seems to be USB_CFG_SWRST which
>>>> is a bit harsh IMO.
>>>
>>> Isn't bit 0 enough?
>>>
>>> /* Reset USB device configuration. */
>>> #define USB_CONF_CFGRST BIT(0)
>>
>> Probably not, as explanation of USB3DIS bit says,
>> "To connect disconnected device, CPU performs
>> software reset (CFG.SWRST)." which is bit 7. "Device software reset.
>>
>> But I'll let Pawel comment on this.
>
> Yes it's the only way to return back to SS mode.
> The USB speed is detecting during connecting.
> CFG.SWRST make reconnect.
Can this be done as part of udc_start or udc_stop?
>
> Maybe it would be better to add SS support in g_audio function.
Not every gadget needs SS, so this is not the right way IMO.
>
>>>
>>> Also, now that I look at this more carefully, you should move that code
>>> to udc_set_speed().
>>>
>>
>> Agreed. I'll revise the implementation to move it to udc_set_speed()
>> once I know how to undo the USB3DIS.
>>
--
cheers,
-roger
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH v2] usb: cdns3: gadget: Fix g_audio use case when connected to Super-Speed host
2019-11-04 9:18 ` Roger Quadros
@ 2019-11-04 15:16 ` Pawel Laszczak
0 siblings, 0 replies; 17+ messages in thread
From: Pawel Laszczak @ 2019-11-04 15:16 UTC (permalink / raw)
To: Roger Quadros, Felipe Balbi, gregkh
Cc: peter.chen, nsekhar, Rahul Kumar, linux-usb, linux-kernel
Hi,
>
>On 03/11/2019 10:17, Pawel Laszczak wrote:
>> Hi,
>>
>>>>
>>>> Hi,
>>>>
>>>> Roger Quadros <rogerq@ti.com> writes:
>>>>
>>>>> Hi,
>>>>>
>>>>> On 31/10/2019 10:55, Felipe Balbi wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> Roger Quadros <rogerq@ti.com> writes:
>>>>>>
>>>>>>> Take into account gadget driver's speed limit when programming
>>>>>>> controller speed.
>>>>>>>
>>>>>>> Fixes: commit 7733f6c32e36 ("usb: cdns3: Add Cadence USB3 DRD Driver")
>>>>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>>>>> Acked-by: Peter Chen <peter.chen@nxp.com>
>>>>>>> ---
>>>>>>>
>>>>>>> Changelog:
>>>>>>> v2
>>>>>>> - Add Fixes line
>>>>>>>
>>>>>>> drivers/usb/cdns3/gadget.c | 31 ++++++++++++++++++++++++++-----
>>>>>>> 1 file changed, 26 insertions(+), 5 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
>>>>>>> index 40dad4e8d0dc..1c724c20d468 100644
>>>>>>> --- a/drivers/usb/cdns3/gadget.c
>>>>>>> +++ b/drivers/usb/cdns3/gadget.c
>>>>>>> @@ -2338,9 +2338,35 @@ 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;
>>>>>>>
>>>>>>> spin_lock_irqsave(&priv_dev->lock, flags);
>>>>>>> priv_dev->gadget_driver = driver;
>>>>>>> +
>>>>>>> + /* limit speed if necessary */
>>>>>>> + max_speed = min(driver->max_speed, gadget->max_speed);
>>>>>>> +
>>>>>>> + switch (max_speed) {
>>>>>>> + case USB_SPEED_FULL:
>>>>>>> + writel(USB_CONF_SFORCE_FS, &priv_dev->regs->usb_conf);
>>>>
>>>> so this forces the controller to FS
>>>
>>> Right.
>>>
>>>>
>>>>>>> + writel(USB_CONF_USB3DIS, &priv_dev->regs->usb_conf);
>>>>
>>>> and this disconnects in superspeed? What is this supposed to do?
>>>>
>>>
>>> It says "Disconnect USB device in SuperSpeed".
>>>
>>> We were asked to set that bit to limit it to HS.
>>>
>>>>>>> + break;
>>>>>>> + case USB_SPEED_HIGH:
>>>>>>> + writel(USB_CONF_USB3DIS, &priv_dev->regs->usb_conf);
>>>>>>> + break;
>>>>>>> + case USB_SPEED_SUPER:
>>>>>>> + break;
>>>>>>> + default:
>>>>>>> + dev_err(priv_dev->dev,
>>>>>>> + "invalid maximum_speed parameter %d\n",
>>>>>>> + max_speed);
>>>>>>> + /* fall through */
>>>>>>> + case USB_SPEED_UNKNOWN:
>>>>>>> + /* default to superspeed */
>>>>>>> + max_speed = USB_SPEED_SUPER;
>>>>>>> + break;
>>>>>>> + }
>>>>>>
>>>>>> I had suggested some simplification for this case statement.
>>>>>>
>>>>>
>>>>> oops, looks like Greg picked this already.
>>>>>
>>>>> During more tests today I just observed that this patch causes
>>>>> the following regression.
>>>>>
>>>>> Connect EVM to Super-Speed host
>>>>> Load g_audio. (this enumerates as HS which is fine)
>>>>> unload g_audio
>>>>> load g_zero (this enumerates at HS instead of SS).
>>>>>
>>>>> This is because the speed limit that we set doesn't get cleared.
>>>>>
>>>>> Now the bits are write only and there is a way to undo USB_CONF_SFORCE_FS
>>>>> by writing USB_CONF_CFORCE_FS, however there is no corresponding bit
>>>>> to clear USB_CONF_USB3DIS. Only way seems to be USB_CFG_SWRST which
>>>>> is a bit harsh IMO.
>>>>
>>>> Isn't bit 0 enough?
>>>>
>>>> /* Reset USB device configuration. */
>>>> #define USB_CONF_CFGRST BIT(0)
>>>
>>> Probably not, as explanation of USB3DIS bit says,
>>> "To connect disconnected device, CPU performs
>>> software reset (CFG.SWRST)." which is bit 7. "Device software reset.
>>>
>>> But I'll let Pawel comment on this.
>>
>> Yes it's the only way to return back to SS mode.
>> The USB speed is detecting during connecting.
>> CFG.SWRST make reconnect.
>
>Can this be done as part of udc_start or udc_stop?
I think that it should work in both cases, however because it finish with connection
so probably the better will be udc_start function.
>
>>
>> Maybe it would be better to add SS support in g_audio function.
>
>Not every gadget needs SS, so this is not the right way IMO.
>
>>
>>>>
>>>> Also, now that I look at this more carefully, you should move that code
>>>> to udc_set_speed().
>>>>
>>>
>>> Agreed. I'll revise the implementation to move it to udc_set_speed()
>>> once I know how to undo the USB3DIS.
>>>
>
>
>--
>cheers,
>-roger
>Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
>Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2019-11-04 15:16 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-29 15:15 [PATCH] usb: cdns3: gadget: Fix g_audio use case when connected to Super-Speed host Roger Quadros
2019-10-30 6:36 ` Peter Chen
2019-10-30 8:44 ` Roger Quadros
2019-10-30 8:56 ` Peter Chen
2019-10-30 11:46 ` Felipe Balbi
2019-10-30 11:51 ` Greg KH
2019-10-30 12:16 ` [PATCH v2] " Roger Quadros
2019-10-30 13:30 ` Greg KH
2019-10-30 14:20 ` Roger Quadros
2019-10-31 8:55 ` Felipe Balbi
2019-10-31 10:35 ` Roger Quadros
2019-10-31 10:55 ` Felipe Balbi
2019-10-31 11:02 ` Roger Quadros
2019-10-31 11:08 ` Felipe Balbi
2019-11-03 8:17 ` Pawel Laszczak
2019-11-04 9:18 ` Roger Quadros
2019-11-04 15:16 ` Pawel Laszczak
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).