* [PATCH] usb: gadget: dwc2_udc_otg: Fix dwc2_gadget_start()
@ 2021-02-10 14:17 Patrice Chotard
2021-02-10 14:26 ` Marek Vasut
0 siblings, 1 reply; 8+ messages in thread
From: Patrice Chotard @ 2021-02-10 14:17 UTC (permalink / raw)
To: u-boot
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)
--
2.17.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH] usb: gadget: dwc2_udc_otg: Fix dwc2_gadget_start()
2021-02-10 14:17 [PATCH] usb: gadget: dwc2_udc_otg: Fix dwc2_gadget_start() Patrice Chotard
@ 2021-02-10 14:26 ` Marek Vasut
2021-02-11 9:58 ` Patrice CHOTARD
0 siblings, 1 reply; 8+ messages in thread
From: Marek Vasut @ 2021-02-10 14:26 UTC (permalink / raw)
To: u-boot
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)
>
[...]
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] usb: gadget: dwc2_udc_otg: Fix dwc2_gadget_start()
2021-02-10 14:26 ` Marek Vasut
@ 2021-02-11 9:58 ` Patrice CHOTARD
2021-02-11 11:26 ` Marek Vasut
0 siblings, 1 reply; 8+ messages in thread
From: Patrice CHOTARD @ 2021-02-11 9:58 UTC (permalink / raw)
To: u-boot
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)
>>
>
>
> [...]
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] usb: gadget: dwc2_udc_otg: Fix dwc2_gadget_start()
2021-02-11 9:58 ` Patrice CHOTARD
@ 2021-02-11 11:26 ` Marek Vasut
2021-02-16 17:32 ` Patrice CHOTARD
0 siblings, 1 reply; 8+ messages in thread
From: Marek Vasut @ 2021-02-11 11:26 UTC (permalink / raw)
To: u-boot
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 ?
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] usb: gadget: dwc2_udc_otg: Fix dwc2_gadget_start()
2021-02-11 11:26 ` Marek Vasut
@ 2021-02-16 17:32 ` Patrice CHOTARD
2021-02-16 21:15 ` Marek Vasut
0 siblings, 1 reply; 8+ messages in thread
From: Patrice CHOTARD @ 2021-02-16 17:32 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] usb: gadget: dwc2_udc_otg: Fix dwc2_gadget_start()
2021-02-16 17:32 ` Patrice CHOTARD
@ 2021-02-16 21:15 ` Marek Vasut
2021-02-17 8:57 ` Patrice CHOTARD
0 siblings, 1 reply; 8+ messages in thread
From: Marek Vasut @ 2021-02-16 21:15 UTC (permalink / raw)
To: u-boot
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 ?
> !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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] usb: gadget: dwc2_udc_otg: Fix dwc2_gadget_start()
2021-02-16 21:15 ` Marek Vasut
@ 2021-02-17 8:57 ` Patrice CHOTARD
2021-02-17 10:02 ` Marek Vasut
0 siblings, 1 reply; 8+ messages in thread
From: Patrice CHOTARD @ 2021-02-17 8:57 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] usb: gadget: dwc2_udc_otg: Fix dwc2_gadget_start()
2021-02-17 8:57 ` Patrice CHOTARD
@ 2021-02-17 10:02 ` Marek Vasut
0 siblings, 0 replies; 8+ messages in thread
From: Marek Vasut @ 2021-02-17 10:02 UTC (permalink / raw)
To: u-boot
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 ?
[...]
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-02-17 10:02 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-10 14:17 [PATCH] usb: gadget: dwc2_udc_otg: Fix dwc2_gadget_start() Patrice Chotard
2021-02-10 14:26 ` Marek Vasut
2021-02-11 9:58 ` Patrice CHOTARD
2021-02-11 11:26 ` Marek Vasut
2021-02-16 17:32 ` Patrice CHOTARD
2021-02-16 21:15 ` Marek Vasut
2021-02-17 8:57 ` Patrice CHOTARD
2021-02-17 10:02 ` Marek Vasut
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.