* Re: [PATCH] usb: dwc2: fix multiplier handling in endpoint setup
2023-01-10 15:17 [PATCH] usb: dwc2: fix multiplier handling in endpoint setup Michael Grzeschik
@ 2023-01-10 15:23 ` Ahmad Fatoum
2023-01-10 15:34 ` Michael Grzeschik
2023-01-10 15:51 ` Jules Maselbas
2023-01-10 17:33 ` Ahmad Fatoum
2 siblings, 1 reply; 6+ messages in thread
From: Ahmad Fatoum @ 2023-01-10 15:23 UTC (permalink / raw)
To: Michael Grzeschik, barebox
Hello Michael,
On 10.01.23 16:17, Michael Grzeschik wrote:
> If the multiplier is 0 in the descriptor bitfield the multiplier count
> is one. For calculating the multiplier count the extra function
> usb_endpoint_maxp_mult should be used. Rework the dwc2 driver to use it
> and make multi packages work.
>
> While at it, we also remove the USB_EP_MAXP_MULT and
> USB_ENDPOINT_MAXP_MASK macros that would stay left unused and are
> defined in include/usb/ch9.ch anyway.
I assume this is a fix and should go into master?
Could you shortly describe the symptoms?
Cheers,
Ahmad
>
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
> drivers/usb/dwc2/gadget.c | 9 +--------
> 1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 7070485410..5a72ba795b 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -14,13 +14,6 @@
> #define spin_lock_irqsave(lock, flags) (void)(flags)
> #define spin_unlock_irqrestore(lock, flags) (void)(flags)
>
> -#ifndef USB_ENDPOINT_MAXP_MASK
> -#define USB_ENDPOINT_MAXP_MASK 0x07ff
> -#endif
> -#ifndef USB_EP_MAXP_MULT
> -#define USB_EP_MAXP_MULT(m) (((m) & 0x1800) >> 11)
> -#endif
> -
> static void kill_all_requests(struct dwc2 *, struct dwc2_ep *, int);
>
> static inline struct dwc2_ep *index_to_ep(struct dwc2 *dwc2,
> @@ -484,7 +477,7 @@ static int dwc2_ep_enable(struct usb_ep *ep,
>
> ep_type = desc->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK;
> mps = usb_endpoint_maxp(desc) & USB_ENDPOINT_MAXP_MASK;
> - mc = USB_EP_MAXP_MULT(usb_endpoint_maxp(desc));
> + mc = usb_endpoint_maxp_mult(desc);
>
> /* note, we handle this here instead of dwc2_set_ep_maxpacket */
> epctrl_reg = dir_in ? DIEPCTL(index) : DOEPCTL(index);
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: dwc2: fix multiplier handling in endpoint setup
2023-01-10 15:23 ` Ahmad Fatoum
@ 2023-01-10 15:34 ` Michael Grzeschik
0 siblings, 0 replies; 6+ messages in thread
From: Michael Grzeschik @ 2023-01-10 15:34 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
[-- Attachment #1: Type: text/plain, Size: 2891 bytes --]
On Tue, Jan 10, 2023 at 04:23:11PM +0100, Ahmad Fatoum wrote:
>Hello Michael,
>
>On 10.01.23 16:17, Michael Grzeschik wrote:
>> If the multiplier is 0 in the descriptor bitfield the multiplier count
>> is one. For calculating the multiplier count the extra function
>> usb_endpoint_maxp_mult should be used. Rework the dwc2 driver to use it
>> and make multi packages work.
>>
>> While at it, we also remove the USB_EP_MAXP_MULT and
>> USB_ENDPOINT_MAXP_MASK macros that would stay left unused and are
>> defined in include/usb/ch9.ch anyway.
>
>I assume this is a fix and should go into master?
Yes. The earlier the better.
>Could you shortly describe the symptoms?
When setting up an payload (bulk, iso, ...) endpoint with the
multiplier of 1 (usual use case) the hs_ep->mc is set to 0, the
dedicated fifo setup in ep_enable is not calculating the values right
since the "size = hs_ep->ep.maxpacket * hs_ep->mc" is wrong.
>Cheers,
>Ahmad
>
>>
>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>> ---
>> drivers/usb/dwc2/gadget.c | 9 +--------
>> 1 file changed, 1 insertion(+), 8 deletions(-)
>>
>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
>> index 7070485410..5a72ba795b 100644
>> --- a/drivers/usb/dwc2/gadget.c
>> +++ b/drivers/usb/dwc2/gadget.c
>> @@ -14,13 +14,6 @@
>> #define spin_lock_irqsave(lock, flags) (void)(flags)
>> #define spin_unlock_irqrestore(lock, flags) (void)(flags)
>>
>> -#ifndef USB_ENDPOINT_MAXP_MASK
>> -#define USB_ENDPOINT_MAXP_MASK 0x07ff
>> -#endif
>> -#ifndef USB_EP_MAXP_MULT
>> -#define USB_EP_MAXP_MULT(m) (((m) & 0x1800) >> 11)
>> -#endif
>> -
>> static void kill_all_requests(struct dwc2 *, struct dwc2_ep *, int);
>>
>> static inline struct dwc2_ep *index_to_ep(struct dwc2 *dwc2,
>> @@ -484,7 +477,7 @@ static int dwc2_ep_enable(struct usb_ep *ep,
>>
>> ep_type = desc->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK;
>> mps = usb_endpoint_maxp(desc) & USB_ENDPOINT_MAXP_MASK;
>> - mc = USB_EP_MAXP_MULT(usb_endpoint_maxp(desc));
>> + mc = usb_endpoint_maxp_mult(desc);
>>
>> /* note, we handle this here instead of dwc2_set_ep_maxpacket */
>> epctrl_reg = dir_in ? DIEPCTL(index) : DOEPCTL(index);
>
>--
>Pengutronix e.K. | |
>Steuerwalder Str. 21 | http://www.pengutronix.de/ |
>31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
>Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: dwc2: fix multiplier handling in endpoint setup
2023-01-10 15:17 [PATCH] usb: dwc2: fix multiplier handling in endpoint setup Michael Grzeschik
2023-01-10 15:23 ` Ahmad Fatoum
@ 2023-01-10 15:51 ` Jules Maselbas
2023-01-10 17:33 ` Ahmad Fatoum
2 siblings, 0 replies; 6+ messages in thread
From: Jules Maselbas @ 2023-01-10 15:51 UTC (permalink / raw)
To: Michael Grzeschik; +Cc: barebox
On Tue, Jan 10, 2023 at 04:17:38PM +0100, Michael Grzeschik wrote:
> If the multiplier is 0 in the descriptor bitfield the multiplier count
> is one. For calculating the multiplier count the extra function
> usb_endpoint_maxp_mult should be used. Rework the dwc2 driver to use it
> and make multi packages work.
Yes, the driver seems to expect mc to be in the range of 1 to 3
included (see dwc2_set_ep_maxpacket in driver/usb/dwc2/gadget.c)
This is aslo the range accepted by dwc2 hardware.
However the .mc fields isn't used at all, it should be written
in the epsize register (see macro DXEPTSIZ_MC)
> While at it, we also remove the USB_EP_MAXP_MULT and
> USB_ENDPOINT_MAXP_MASK macros that would stay left unused and are
> defined in include/usb/ch9.ch anyway.
Indeed, this is can be removed.
I've tested this patch on one board and it worked just find.
Tested-by: Jules Maselbas <jmaselbas@kalray.eu>
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
> drivers/usb/dwc2/gadget.c | 9 +--------
> 1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 7070485410..5a72ba795b 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -14,13 +14,6 @@
> #define spin_lock_irqsave(lock, flags) (void)(flags)
> #define spin_unlock_irqrestore(lock, flags) (void)(flags)
>
> -#ifndef USB_ENDPOINT_MAXP_MASK
> -#define USB_ENDPOINT_MAXP_MASK 0x07ff
> -#endif
> -#ifndef USB_EP_MAXP_MULT
> -#define USB_EP_MAXP_MULT(m) (((m) & 0x1800) >> 11)
> -#endif
> -
> static void kill_all_requests(struct dwc2 *, struct dwc2_ep *, int);
>
> static inline struct dwc2_ep *index_to_ep(struct dwc2 *dwc2,
> @@ -484,7 +477,7 @@ static int dwc2_ep_enable(struct usb_ep *ep,
>
> ep_type = desc->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK;
> mps = usb_endpoint_maxp(desc) & USB_ENDPOINT_MAXP_MASK;
> - mc = USB_EP_MAXP_MULT(usb_endpoint_maxp(desc));
> + mc = usb_endpoint_maxp_mult(desc);
>
> /* note, we handle this here instead of dwc2_set_ep_maxpacket */
> epctrl_reg = dir_in ? DIEPCTL(index) : DOEPCTL(index);
> --
> 2.30.2
>
>
>
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: dwc2: fix multiplier handling in endpoint setup
2023-01-10 15:17 [PATCH] usb: dwc2: fix multiplier handling in endpoint setup Michael Grzeschik
2023-01-10 15:23 ` Ahmad Fatoum
2023-01-10 15:51 ` Jules Maselbas
@ 2023-01-10 17:33 ` Ahmad Fatoum
2023-01-24 20:04 ` Michael Grzeschik
2 siblings, 1 reply; 6+ messages in thread
From: Ahmad Fatoum @ 2023-01-10 17:33 UTC (permalink / raw)
To: Michael Grzeschik, barebox
Hello Michael,
On 10.01.23 16:17, Michael Grzeschik wrote:
> If the multiplier is 0 in the descriptor bitfield the multiplier count
> is one. For calculating the multiplier count the extra function
> usb_endpoint_maxp_mult should be used. Rework the dwc2 driver to use it
> and make multi packages work.
>
> While at it, we also remove the USB_EP_MAXP_MULT and
> USB_ENDPOINT_MAXP_MASK macros that would stay left unused and are
> defined in include/usb/ch9.ch anyway.
This breaks fastboot for me on the STM32MP157-based LXA TAC:
│barebox@Linux Automation Test Automation Controller (TAC):/ usbgadget -A ''
│udc0: registering UDC driver [g_multi]
│multi_bind: creating Fastboot function
│g_multi usbgadget: Multifunction Composite Gadget
│g_multi usbgadget: g_multi ready
│dwc2 49000000.usb-otg@49000000.of: bound driver g_multi
│barebox@Linux Automation Test Automation Controller (TAC):/ dwc2 49000000.usb-otg@49000000.of: new address 7
│g_multi usbgadget: high-speed config #1: Multifunction Composite Gadget
│ERROR: dwc2 49000000.usb-otg@49000000.of: dwc2_ep_enable: No suitable fifo found
│ERROR: fastboot: failed to enable in ep: Out of memory
Could you look into it?
Cheers,
Ahmad
>
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
> drivers/usb/dwc2/gadget.c | 9 +--------
> 1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 7070485410..5a72ba795b 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -14,13 +14,6 @@
> #define spin_lock_irqsave(lock, flags) (void)(flags)
> #define spin_unlock_irqrestore(lock, flags) (void)(flags)
>
> -#ifndef USB_ENDPOINT_MAXP_MASK
> -#define USB_ENDPOINT_MAXP_MASK 0x07ff
> -#endif
> -#ifndef USB_EP_MAXP_MULT
> -#define USB_EP_MAXP_MULT(m) (((m) & 0x1800) >> 11)
> -#endif
> -
> static void kill_all_requests(struct dwc2 *, struct dwc2_ep *, int);
>
> static inline struct dwc2_ep *index_to_ep(struct dwc2 *dwc2,
> @@ -484,7 +477,7 @@ static int dwc2_ep_enable(struct usb_ep *ep,
>
> ep_type = desc->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK;
> mps = usb_endpoint_maxp(desc) & USB_ENDPOINT_MAXP_MASK;
> - mc = USB_EP_MAXP_MULT(usb_endpoint_maxp(desc));
> + mc = usb_endpoint_maxp_mult(desc);
>
> /* note, we handle this here instead of dwc2_set_ep_maxpacket */
> epctrl_reg = dir_in ? DIEPCTL(index) : DOEPCTL(index);
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: dwc2: fix multiplier handling in endpoint setup
2023-01-10 17:33 ` Ahmad Fatoum
@ 2023-01-24 20:04 ` Michael Grzeschik
0 siblings, 0 replies; 6+ messages in thread
From: Michael Grzeschik @ 2023-01-24 20:04 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
[-- Attachment #1: Type: text/plain, Size: 3746 bytes --]
On Tue, Jan 10, 2023 at 06:33:25PM +0100, Ahmad Fatoum wrote:
>Hello Michael,
>
>On 10.01.23 16:17, Michael Grzeschik wrote:
>> If the multiplier is 0 in the descriptor bitfield the multiplier count
>> is one. For calculating the multiplier count the extra function
>> usb_endpoint_maxp_mult should be used. Rework the dwc2 driver to use it
>> and make multi packages work.
>>
>> While at it, we also remove the USB_EP_MAXP_MULT and
>> USB_ENDPOINT_MAXP_MASK macros that would stay left unused and are
>> defined in include/usb/ch9.ch anyway.
>
>This breaks fastboot for me on the STM32MP157-based LXA TAC:
>
> │barebox@Linux Automation Test Automation Controller (TAC):/ usbgadget -A ''
> │udc0: registering UDC driver [g_multi]
> │multi_bind: creating Fastboot function
> │g_multi usbgadget: Multifunction Composite Gadget
> │g_multi usbgadget: g_multi ready
> │dwc2 49000000.usb-otg@49000000.of: bound driver g_multi
> │barebox@Linux Automation Test Automation Controller (TAC):/ dwc2 49000000.usb-otg@49000000.of: new address 7
> │g_multi usbgadget: high-speed config #1: Multifunction Composite Gadget
> │ERROR: dwc2 49000000.usb-otg@49000000.of: dwc2_ep_enable: No suitable fifo found
> │ERROR: fastboot: failed to enable in ep: Out of memory
>
>Could you look into it?
Yes! We found that the fifo handling on stm32mp1 is not right.
There is a patch for the kernel in discussion.
https://lore.kernel.org/linux-arm-kernel/20230112112013.1086787-1-u.kleine-koenig@pengutronix.de/
I just send v2 of this patch with a fix for the fifo
size handling in the local dts of barebox.
https://lore.barebox.org/barebox/20230124200114.3160585-1-m.grzeschik@pengutronix.de/T/#m4440a84c96b72a77d47047c8958788fe475b7440
Can you test the series?
Thanks,
Michael
>>
>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>> ---
>> drivers/usb/dwc2/gadget.c | 9 +--------
>> 1 file changed, 1 insertion(+), 8 deletions(-)
>>
>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
>> index 7070485410..5a72ba795b 100644
>> --- a/drivers/usb/dwc2/gadget.c
>> +++ b/drivers/usb/dwc2/gadget.c
>> @@ -14,13 +14,6 @@
>> #define spin_lock_irqsave(lock, flags) (void)(flags)
>> #define spin_unlock_irqrestore(lock, flags) (void)(flags)
>>
>> -#ifndef USB_ENDPOINT_MAXP_MASK
>> -#define USB_ENDPOINT_MAXP_MASK 0x07ff
>> -#endif
>> -#ifndef USB_EP_MAXP_MULT
>> -#define USB_EP_MAXP_MULT(m) (((m) & 0x1800) >> 11)
>> -#endif
>> -
>> static void kill_all_requests(struct dwc2 *, struct dwc2_ep *, int);
>>
>> static inline struct dwc2_ep *index_to_ep(struct dwc2 *dwc2,
>> @@ -484,7 +477,7 @@ static int dwc2_ep_enable(struct usb_ep *ep,
>>
>> ep_type = desc->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK;
>> mps = usb_endpoint_maxp(desc) & USB_ENDPOINT_MAXP_MASK;
>> - mc = USB_EP_MAXP_MULT(usb_endpoint_maxp(desc));
>> + mc = usb_endpoint_maxp_mult(desc);
>>
>> /* note, we handle this here instead of dwc2_set_ep_maxpacket */
>> epctrl_reg = dir_in ? DIEPCTL(index) : DOEPCTL(index);
>
>--
>Pengutronix e.K. | |
>Steuerwalder Str. 21 | http://www.pengutronix.de/ |
>31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
>Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread