| Message ID | 20191029151514.28495-1-rogerq@ti.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: cdns3: gadget: Fix g_audio use case when connected to Super-Speed host | expand |
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>
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> >
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.
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; [...]
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 --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:
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(-)