Re: [PATCH] usb: cdns3: gadget: Fix g_audio use case when connected to Super-Speed host

[Peter Chen <peter.chen@nxp.com>]

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