diff mbox series

usb: cdns3: gadget: Fix g_audio use case when connected to Super-Speed host

Message ID 20191029151514.28495-1-rogerq@ti.com
State Superseded
Headers show
Series usb: cdns3: gadget: Fix g_audio use case when connected to Super-Speed host | expand

Commit Message

Roger Quadros Oct. 29, 2019, 3:15 p.m. UTC 
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(-)

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Comments

Peter Chen Oct. 30, 2019, 6:36 a.m. UTC | #1 
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
Roger Quadros Oct. 30, 2019, 8:44 a.m. UTC | #2 
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
Peter Chen Oct. 30, 2019, 8:56 a.m. UTC | #3 
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
Felipe Balbi Oct. 30, 2019, 11:46 a.m. UTC | #4 
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
Greg KH Oct. 30, 2019, 11:51 a.m. UTC | #5 
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 :(
diff mbox series

Patch

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: