On 03/11/2019 10:17, Pawel Laszczak wrote:
Hi,
Hi,
Roger Quadros <rogerq@xxxxxx> writes:
Hi,
On 31/10/2019 10:55, Felipe Balbi wrote:
Hi,
Roger Quadros <rogerq@xxxxxx> 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@xxxxxx>
Acked-by: Peter Chen <peter.chen@xxxxxxx>
---
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
