[PATCH] usb: gadget: dwc2_udc_otg: Fix dwc2_gadget_start()
Since commit 8745b9ebccae ("usb: gadget: add super speed support") ums was no more functional on platform which use dwc2_udc_otg driver.
Remove the speed test in dwc2_gadget_start() to fix it. Tested on stm32mp157c-ev1 board.
Fixes: c791c8431c34 ("usb: dwc2: convert driver to DM_USB_GADGET")
Signed-off-by: Patrice Chotard patrice.chotard@foss.st.com ---
drivers/usb/gadget/dwc2_udc_otg.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/gadget/dwc2_udc_otg.c b/drivers/usb/gadget/dwc2_udc_otg.c index e3871e381e..4f3d761eb1 100644 --- a/drivers/usb/gadget/dwc2_udc_otg.c +++ b/drivers/usb/gadget/dwc2_udc_otg.c @@ -248,10 +248,7 @@ int usb_gadget_register_driver(struct usb_gadget_driver *driver)
debug_cond(DEBUG_SETUP != 0, "%s: %s\n", __func__, "no name");
- if (!driver - || (driver->speed != USB_SPEED_FULL - && driver->speed != USB_SPEED_HIGH) - || !driver->bind || !driver->disconnect || !driver->setup) + if (!driver || !driver->bind || !driver->disconnect || !driver->setup) return -EINVAL; if (!dev) return -ENODEV; @@ -320,10 +317,7 @@ static int dwc2_gadget_start(struct usb_gadget *g,
debug_cond(DEBUG_SETUP != 0, "%s: %s\n", __func__, "no name");
- if (!driver || - (driver->speed != USB_SPEED_FULL && - driver->speed != USB_SPEED_HIGH) || - !driver->bind || !driver->disconnect || !driver->setup) + if (!driver || !driver->bind || !driver->disconnect || !driver->setup) return -EINVAL;
if (!dev)
On 2/10/21 3:17 PM, Patrice Chotard wrote:
Since commit 8745b9ebccae ("usb: gadget: add super speed support") ums was no more functional on platform which use dwc2_udc_otg driver.
Remove the speed test in dwc2_gadget_start() to fix it. Tested on stm32mp157c-ev1 board.
Isn't the speed check correct though ?
What is really going on when this fails ?
Fixes: c791c8431c34 ("usb: dwc2: convert driver to DM_USB_GADGET")
Signed-off-by: Patrice Chotard patrice.chotard@foss.st.com
drivers/usb/gadget/dwc2_udc_otg.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/gadget/dwc2_udc_otg.c b/drivers/usb/gadget/dwc2_udc_otg.c index e3871e381e..4f3d761eb1 100644 --- a/drivers/usb/gadget/dwc2_udc_otg.c +++ b/drivers/usb/gadget/dwc2_udc_otg.c @@ -248,10 +248,7 @@ int usb_gadget_register_driver(struct usb_gadget_driver *driver)
debug_cond(DEBUG_SETUP != 0, "%s: %s\n", __func__, "no name");
- if (!driver
|| (driver->speed != USB_SPEED_FULL&& driver->speed != USB_SPEED_HIGH)|| !driver->bind || !driver->disconnect || !driver->setup)
- if (!driver || !driver->bind || !driver->disconnect || !driver->setup) return -EINVAL; if (!dev) return -ENODEV;
@@ -320,10 +317,7 @@ static int dwc2_gadget_start(struct usb_gadget *g,
debug_cond(DEBUG_SETUP != 0, "%s: %s\n", __func__, "no name");
- if (!driver ||
(driver->speed != USB_SPEED_FULL &&driver->speed != USB_SPEED_HIGH) ||!driver->bind || !driver->disconnect || !driver->setup)
if (!driver || !driver->bind || !driver->disconnect || !driver->setup) return -EINVAL;
if (!dev)
[...]
Hi Marek
On 2/10/21 3:26 PM, Marek Vasut wrote:
On 2/10/21 3:17 PM, Patrice Chotard wrote:
Since commit 8745b9ebccae ("usb: gadget: add super speed support") ums was no more functional on platform which use dwc2_udc_otg driver.
Remove the speed test in dwc2_gadget_start() to fix it. Tested on stm32mp157c-ev1 board.
Isn't the speed check correct though ?
I am not sure this speed test is needed.
What is really going on when this fails ?
Since 8745b9ebccae ("usb: gadget: add super speed support"), driver->speed is now set to USB_SPEED_SUPER in drivers/usb/gadget/composite.c
and this forbids dwc2_udc_otg.c to be registered.
Patrice
Fixes: c791c8431c34 ("usb: dwc2: convert driver to DM_USB_GADGET")
Signed-off-by: Patrice Chotard patrice.chotard@foss.st.com
drivers/usb/gadget/dwc2_udc_otg.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/gadget/dwc2_udc_otg.c b/drivers/usb/gadget/dwc2_udc_otg.c index e3871e381e..4f3d761eb1 100644 --- a/drivers/usb/gadget/dwc2_udc_otg.c +++ b/drivers/usb/gadget/dwc2_udc_otg.c @@ -248,10 +248,7 @@ int usb_gadget_register_driver(struct usb_gadget_driver *driver) debug_cond(DEBUG_SETUP != 0, "%s: %s\n", __func__, "no name"); - if (!driver - || (driver->speed != USB_SPEED_FULL - && driver->speed != USB_SPEED_HIGH) - || !driver->bind || !driver->disconnect || !driver->setup) + if (!driver || !driver->bind || !driver->disconnect || !driver->setup) return -EINVAL; if (!dev) return -ENODEV; @@ -320,10 +317,7 @@ static int dwc2_gadget_start(struct usb_gadget *g, debug_cond(DEBUG_SETUP != 0, "%s: %s\n", __func__, "no name"); - if (!driver || - (driver->speed != USB_SPEED_FULL && - driver->speed != USB_SPEED_HIGH) || - !driver->bind || !driver->disconnect || !driver->setup) + if (!driver || !driver->bind || !driver->disconnect || !driver->setup) return -EINVAL; if (!dev)
[...]
On 2/11/21 10:58 AM, Patrice CHOTARD wrote:
Hi Marek
On 2/10/21 3:26 PM, Marek Vasut wrote:
On 2/10/21 3:17 PM, Patrice Chotard wrote:
Since commit 8745b9ebccae ("usb: gadget: add super speed support") ums was no more functional on platform which use dwc2_udc_otg driver.
Remove the speed test in dwc2_gadget_start() to fix it. Tested on stm32mp157c-ev1 board.
Isn't the speed check correct though ?
I am not sure this speed test is needed.
What is really going on when this fails ?
Since 8745b9ebccae ("usb: gadget: add super speed support"), driver->speed is now set to USB_SPEED_SUPER in drivers/usb/gadget/composite.c
and this forbids dwc2_udc_otg.c to be registered.
That sounds like a bug in the USB gadget/otg core , no ?
Hi Marek
On 2/11/21 12:26 PM, Marek Vasut wrote:
On 2/11/21 10:58 AM, Patrice CHOTARD wrote:
Hi Marek
On 2/10/21 3:26 PM, Marek Vasut wrote:
On 2/10/21 3:17 PM, Patrice Chotard wrote:
Since commit 8745b9ebccae ("usb: gadget: add super speed support") ums was no more functional on platform which use dwc2_udc_otg driver.
Remove the speed test in dwc2_gadget_start() to fix it. Tested on stm32mp157c-ev1 board.
Isn't the speed check correct though ?
I am not sure this speed test is needed.
What is really going on when this fails ?
Since 8745b9ebccae ("usb: gadget: add super speed support"), driver->speed is now set to USB_SPEED_SUPER in drivers/usb/gadget/composite.c
and this forbids dwc2_udc_otg.c to be registered.
That sounds like a bug in the USB gadget/otg core , no ?
After analysis, if i correctly understood, the speed test done in both usb_gadget_register_driver() and in dwc2_gadget_start() in dwc2_udc_otg.c is too restrictive and accepts only gadget driver with max speed equal to USB_SPEED_FULL or USB_SPEED_HIGH.
So all gadget driver with max speed set to higher speed than USB_SPEED_HIGH (USB_SPEED_SUPER in case of composite gadget driver) are not allowed, which is wrong.
This test should check that the gadget driver max speed is higher or equal to the min speed supported by dwc2_udc_otg driver, ie USB_SPEED_FULL.
So the test should be :
@@ -247,12 +247,11 @@ int usb_gadget_register_driver(struct usb_gadget_driver *driver) unsigned long flags = 0;
debug_cond(DEBUG_SETUP != 0, "%s: %s\n", __func__, "no name");
if (!driver - || (driver->speed != USB_SPEED_FULL - && driver->speed != USB_SPEED_HIGH) + || driver->speed < USB_SPEED_FULL || !driver->bind || !driver->disconnect || !driver->setup) return -EINVAL; if (!dev) return -ENODEV; if (dev->driver) @@ -319,12 +318,11 @@ static int dwc2_gadget_start(struct usb_gadget *g, struct dwc2_udc *dev = the_controller;
debug_cond(DEBUG_SETUP != 0, "%s: %s\n", __func__, "no name");
if (!driver || - (driver->speed != USB_SPEED_FULL && - driver->speed != USB_SPEED_HIGH) || + driver->speed < USB_SPEED_FULL || !driver->bind || !driver->disconnect || !driver->setup) return -EINVAL;
if (!dev)
I you are agree, i will send a v2 with this.
Patrice
On 2/16/21 6:32 PM, Patrice CHOTARD wrote:
Hi Marek
Hi,
On 2/11/21 12:26 PM, Marek Vasut wrote:
On 2/11/21 10:58 AM, Patrice CHOTARD wrote:
Hi Marek
On 2/10/21 3:26 PM, Marek Vasut wrote:
On 2/10/21 3:17 PM, Patrice Chotard wrote:
Since commit 8745b9ebccae ("usb: gadget: add super speed support") ums was no more functional on platform which use dwc2_udc_otg driver.
Remove the speed test in dwc2_gadget_start() to fix it. Tested on stm32mp157c-ev1 board.
Isn't the speed check correct though ?
I am not sure this speed test is needed.
What is really going on when this fails ?
Since 8745b9ebccae ("usb: gadget: add super speed support"), driver->speed is now set to USB_SPEED_SUPER in drivers/usb/gadget/composite.c
and this forbids dwc2_udc_otg.c to be registered.
That sounds like a bug in the USB gadget/otg core , no ?
After analysis, if i correctly understood, the speed test done in both usb_gadget_register_driver() and in dwc2_gadget_start() in dwc2_udc_otg.c is too restrictive and accepts only gadget driver with max speed equal to USB_SPEED_FULL or USB_SPEED_HIGH.
That should be fine for the DWC2 IP, which is LS/FS/HS.
So all gadget driver with max speed set to higher speed than USB_SPEED_HIGH (USB_SPEED_SUPER in case of composite gadget driver) are not allowed, which is wrong.
This test should check that the gadget driver max speed is higher or equal to the min speed supported by dwc2_udc_otg driver, ie USB_SPEED_FULL.
So the test should be :
@@ -247,12 +247,11 @@ int usb_gadget_register_driver(struct usb_gadget_driver *driver) unsigned long flags = 0;
debug_cond(DEBUG_SETUP != 0, "%s: %s\n", __func__, "no name");
if (!driver
|| (driver->speed != USB_SPEED_FULL&& driver->speed != USB_SPEED_HIGH)
|| driver->speed < USB_SPEED_FULL || !driver->bind || !driver->disconnect || !driver->setup)@@ -319,12 +318,11 @@ static int dwc2_gadget_start(struct usb_gadget *g, struct dwc2_udc *dev = the_controller;
debug_cond(DEBUG_SETUP != 0, "%s: %s\n", __func__, "no name");
if (!driver ||
(driver->speed != USB_SPEED_FULL &&driver->speed != USB_SPEED_HIGH) ||
driver->speed < USB_SPEED_FULL ||
The DWC2 can't operate in LS gadget mode , i.e. emulate some old keyboard ? Maybe what you want is driver->speed > USB_SPEED_HIGH instead ?
!driver->bind || !driver->disconnect || !driver->setup) return -EINVAL;if (!dev)
I you are agree, i will send a v2 with this.
Yes please. But you really want to get AB/RB from Lukasz, since he does the gadget stuff.
Hi Marek
On 2/16/21 10:15 PM, Marek Vasut wrote:
On 2/16/21 6:32 PM, Patrice CHOTARD wrote:
Hi Marek
Hi,
On 2/11/21 12:26 PM, Marek Vasut wrote:
On 2/11/21 10:58 AM, Patrice CHOTARD wrote:
Hi Marek
On 2/10/21 3:26 PM, Marek Vasut wrote:
On 2/10/21 3:17 PM, Patrice Chotard wrote:
Since commit 8745b9ebccae ("usb: gadget: add super speed support") ums was no more functional on platform which use dwc2_udc_otg driver.
Remove the speed test in dwc2_gadget_start() to fix it. Tested on stm32mp157c-ev1 board.
Isn't the speed check correct though ?
I am not sure this speed test is needed.
What is really going on when this fails ?
Since 8745b9ebccae ("usb: gadget: add super speed support"), driver->speed is now set to USB_SPEED_SUPER in drivers/usb/gadget/composite.c
and this forbids dwc2_udc_otg.c to be registered.
That sounds like a bug in the USB gadget/otg core , no ?
After analysis, if i correctly understood, the speed test done in both usb_gadget_register_driver() and in dwc2_gadget_start() in dwc2_udc_otg.c is too restrictive and accepts only gadget driver with max speed equal to USB_SPEED_FULL or USB_SPEED_HIGH.
That should be fine for the DWC2 IP, which is LS/FS/HS.
So all gadget driver with max speed set to higher speed than USB_SPEED_HIGH (USB_SPEED_SUPER in case of composite gadget driver) are not allowed, which is wrong.
This test should check that the gadget driver max speed is higher or equal to the min speed supported by dwc2_udc_otg driver, ie USB_SPEED_FULL.
So the test should be :
@@ -247,12 +247,11 @@ int usb_gadget_register_driver(struct usb_gadget_driver *driver) unsigned long flags = 0; debug_cond(DEBUG_SETUP != 0, "%s: %s\n", __func__, "no name"); if (!driver - || (driver->speed != USB_SPEED_FULL - && driver->speed != USB_SPEED_HIGH) + || driver->speed < USB_SPEED_FULL || !driver->bind || !driver->disconnect || !driver->setup) return -EINVAL; if (!dev) return -ENODEV; if (dev->driver) @@ -319,12 +318,11 @@ static int dwc2_gadget_start(struct usb_gadget *g, struct dwc2_udc *dev = the_controller; debug_cond(DEBUG_SETUP != 0, "%s: %s\n", __func__, "no name"); if (!driver || - (driver->speed != USB_SPEED_FULL && - driver->speed != USB_SPEED_HIGH) || + driver->speed < USB_SPEED_FULL ||
The DWC2 can't operate in LS gadget mode , i.e. emulate some old keyboard ? Maybe what you want is driver->speed > USB_SPEED_HIGH instead ?
The test is correct, in case driver->speed is lower than FS, we return -EINVAL. All others speed FS/HS/SS and higher are allowed.
!driver->bind || !driver->disconnect || !driver->setup) return -EINVAL; if (!dev)
I you are agree, i will send a v2 with this.
Yes please. But you really want to get AB/RB from Lukasz, since he does the gadget stuff.
Ok, i will add Lukasz for the V2 review.
Thanks Patrice
On 2/17/21 9:57 AM, Patrice CHOTARD wrote:
Hi Marek
Hi,
[...]
@@ -319,12 +318,11 @@ static int dwc2_gadget_start(struct usb_gadget *g, struct dwc2_udc *dev = the_controller; debug_cond(DEBUG_SETUP != 0, "%s: %s\n", __func__, "no name"); if (!driver || - (driver->speed != USB_SPEED_FULL && - driver->speed != USB_SPEED_HIGH) || + driver->speed < USB_SPEED_FULL ||
The DWC2 can't operate in LS gadget mode , i.e. emulate some old keyboard ? Maybe what you want is driver->speed > USB_SPEED_HIGH instead ?
The test is correct, in case driver->speed is lower than FS, we return -EINVAL. All others speed FS/HS/SS and higher are allowed.
What's the problem with LS ?
[...]
participants (3)
-
Marek Vasut -
Patrice CHOTARD -
Patrice Chotard