[PATCH] usb: gadget: ether: Handle gadget driver registration in start and stop
Revert part of 718f1d41 to move usb_gadget_register_driver()/usb_gadget_unregister_driver() back to usb_eth_start()/usb_eth_stop().
usb_gadget_register_driver() will initialize the USB controller which enters ready to connect state with pull-up resistor enabled.
From the host's point of view, a device is ready to be enumerated.
However, since dm_usb_gadget_handle_interrupts() is only called when ethernet function is started, at this stage USB events are not managed, host's enumeration attempts will fail and resulting error like: usb 1-1: new high-speed USB device number 50 using xhci_hcd usb 1-1: device descriptor read/64, error -110 usb 1-1: device descriptor read/64, error -110 usb usb1-port1: attempt power cycle
With this patch the USB controller will only be initialized when ethernet function is used, in which case USB controller events are handled, so the host won't see an unresponsive device.
Signed-off-by: Zixun LI admin@hifiphile.com --- drivers/usb/gadget/ether.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c index b7b7bacb00..ed55f12662 100644 --- a/drivers/usb/gadget/ether.c +++ b/drivers/usb/gadget/ether.c @@ -2277,6 +2277,9 @@ static int usb_eth_start(struct udevice *udev) packet_received = 0; packet_sent = 0;
+ if (usb_gadget_register_driver(&priv->eth_driver) < 0) + goto fail; + gadget = dev->gadget; usb_gadget_connect(gadget);
@@ -2398,6 +2401,8 @@ static void usb_eth_stop(struct udevice *udev) dm_usb_gadget_handle_interrupts(udev->parent); dev->network_started = 0; } + + usb_gadget_unregister_driver(&priv->eth_driver); }
static int usb_eth_recv(struct udevice *dev, int flags, uchar **packetp) @@ -2503,15 +2508,6 @@ static int usb_eth_probe(struct udevice *dev) priv->eth_driver.disconnect = eth_disconnect; priv->eth_driver.suspend = eth_suspend; priv->eth_driver.resume = eth_resume; - return usb_gadget_register_driver(&priv->eth_driver); -} - -static int usb_eth_remove(struct udevice *dev) -{ - struct ether_priv *priv = dev_get_priv(dev); - - usb_gadget_unregister_driver(&priv->eth_driver); - return 0; }
@@ -2526,7 +2522,6 @@ U_BOOT_DRIVER(eth_usb) = { .name = "usb_ether", .id = UCLASS_ETH, .probe = usb_eth_probe, - .remove = usb_eth_remove, .unbind = usb_eth_unbind, .ops = &usb_eth_ops, .priv_auto = sizeof(struct ether_priv), -- 2.45.2
Hi Zixun,
Thank you for the patch.
On ven., juil. 26, 2024 at 10:31, Zixun LI admin@hifiphile.com wrote:
Revert part of 718f1d41 to move usb_gadget_register_driver()/usb_gadget_unregister_driver() back to usb_eth_start()/usb_eth_stop().
usb_gadget_register_driver() will initialize the USB controller which enters ready to connect state with pull-up resistor enabled.
From the host's point of view, a device is ready to be enumerated. However, since dm_usb_gadget_handle_interrupts() is only called when ethernet function is started, at this stage USB events are not managed, host's enumeration attempts will fail and resulting error like: usb 1-1: new high-speed USB device number 50 using xhci_hcd usb 1-1: device descriptor read/64, error -110 usb 1-1: device descriptor read/64, error -110 usb usb1-port1: attempt power cycle
With this patch the USB controller will only be initialized when ethernet function is used, in which case USB controller events are handled, so the host won't see an unresponsive device.
Signed-off-by: Zixun LI admin@hifiphile.com
Reviewed-by: Mattijs Korpershoek mkorpershoek@baylibre.com
I'd like to test this on my end as well. Could you please give some details on how this has been tested?
A sequence of U-Boot commands would be helpful, for example.
drivers/usb/gadget/ether.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c index b7b7bacb00..ed55f12662 100644 --- a/drivers/usb/gadget/ether.c +++ b/drivers/usb/gadget/ether.c @@ -2277,6 +2277,9 @@ static int usb_eth_start(struct udevice *udev) packet_received = 0; packet_sent = 0;
- if (usb_gadget_register_driver(&priv->eth_driver) < 0)
goto fail;- gadget = dev->gadget; usb_gadget_connect(gadget);
@@ -2398,6 +2401,8 @@ static void usb_eth_stop(struct udevice *udev) dm_usb_gadget_handle_interrupts(udev->parent); dev->network_started = 0; }
- usb_gadget_unregister_driver(&priv->eth_driver);
}
static int usb_eth_recv(struct udevice *dev, int flags, uchar **packetp) @@ -2503,15 +2508,6 @@ static int usb_eth_probe(struct udevice *dev) priv->eth_driver.disconnect = eth_disconnect; priv->eth_driver.suspend = eth_suspend; priv->eth_driver.resume = eth_resume;
- return usb_gadget_register_driver(&priv->eth_driver);
-}
-static int usb_eth_remove(struct udevice *dev) -{
- struct ether_priv *priv = dev_get_priv(dev);
- usb_gadget_unregister_driver(&priv->eth_driver);
- return 0;
}
@@ -2526,7 +2522,6 @@ U_BOOT_DRIVER(eth_usb) = { .name = "usb_ether", .id = UCLASS_ETH, .probe = usb_eth_probe,
- .remove = usb_eth_remove, .unbind = usb_eth_unbind, .ops = &usb_eth_ops, .priv_auto = sizeof(struct ether_priv),
-- 2.45.2
Hi Mattijs,
On Tue, Aug 6, 2024 at 4:00 PM Mattijs Korpershoek mkorpershoek@baylibre.com wrote:
I'd like to test this on my end as well. Could you please give some details on how this has been tested?
A sequence of U-Boot commands would be helpful, for example.
My tests are done on a custom ATMEL SAM9G25 board powered by USB Gadget port.
Gadget is enabled in the DT: &usb2 { status = "okay"; };
usb_ether enabled in board late init: int board_late_init(void) { #ifdef CONFIG_USB_ETHER usb_ether_init(); #endif return 0; }
Without this patch the host will try to enumerate the USB device once U-Boot is loaded and result in the error I mentioned.
With this patch USB is connected only when ethernet command, like dhcp is run, then disconnect when it's finished: usb 1-1: new high-speed USB device number 91 using xhci_hcd usb 1-1: New USB device found, idVendor=0000, idProduct=0000, bcdDevice= 3.17 usb 1-1: New USB device strings: Mfr=1, Product=2, SerialNumber=0 usb 1-1: Product: Ethernet Gadget usb 1-1: Manufacturer: U-Boot cdc_ether 1-1:1.0 usb0: register 'cdc_ether' at usb-0000:04:00.3-1, CDC Ethernet Device, de:ad:be:ef:00:00 cdc_ether 1-1:1.0 enp4s0f3u1: renamed from usb0 gadget0: port 1(enp4s0f3u1) entered blocking state gadget0: port 1(enp4s0f3u1) entered disabled state cdc_ether 1-1:1.0 enp4s0f3u1: entered allmulticast mode cdc_ether 1-1:1.0 enp4s0f3u1: entered promiscuous mode gadget0: port 1(enp4s0f3u1) entered blocking state gadget0: port 1(enp4s0f3u1) entered forwarding state usb 1-1: USB disconnect, device number 91
Hi Zixun,
On mar., août 06, 2024 at 22:28, Zixun LI admin@hifiphile.com wrote:
Hi Mattijs,
On Tue, Aug 6, 2024 at 4:00 PM Mattijs Korpershoek mkorpershoek@baylibre.com wrote:
I'd like to test this on my end as well. Could you please give some details on how this has been tested?
A sequence of U-Boot commands would be helpful, for example.
My tests are done on a custom ATMEL SAM9G25 board powered by USB Gadget port.
Gadget is enabled in the DT: &usb2 { status = "okay"; };
usb_ether enabled in board late init: int board_late_init(void) { #ifdef CONFIG_USB_ETHER usb_ether_init(); #endif return 0; }
Without this patch the host will try to enumerate the USB device once U-Boot is loaded and result in the error I mentioned.
Thank you for the details. I could reproduce the issue on Khadas VIM3 board and I could also test that your patch fixes the issue.
Tested-by: Mattijs Korpershoek mkorpershoek@baylibre.com
With this patch USB is connected only when ethernet command, like dhcp is run, then disconnect when it's finished: usb 1-1: new high-speed USB device number 91 using xhci_hcd usb 1-1: New USB device found, idVendor=0000, idProduct=0000, bcdDevice= 3.17 usb 1-1: New USB device strings: Mfr=1, Product=2, SerialNumber=0 usb 1-1: Product: Ethernet Gadget usb 1-1: Manufacturer: U-Boot cdc_ether 1-1:1.0 usb0: register 'cdc_ether' at usb-0000:04:00.3-1, CDC Ethernet Device, de:ad:be:ef:00:00 cdc_ether 1-1:1.0 enp4s0f3u1: renamed from usb0 gadget0: port 1(enp4s0f3u1) entered blocking state gadget0: port 1(enp4s0f3u1) entered disabled state cdc_ether 1-1:1.0 enp4s0f3u1: entered allmulticast mode cdc_ether 1-1:1.0 enp4s0f3u1: entered promiscuous mode gadget0: port 1(enp4s0f3u1) entered blocking state gadget0: port 1(enp4s0f3u1) entered forwarding state usb 1-1: USB disconnect, device number 91
Hi,
On Fri, 26 Jul 2024 10:31:00 +0200, Zixun LI wrote:
Revert part of 718f1d41 to move usb_gadget_register_driver()/usb_gadget_unregister_driver() back to usb_eth_start()/usb_eth_stop().
usb_gadget_register_driver() will initialize the USB controller which enters ready to connect state with pull-up resistor enabled.
[...]
Thanks, Applied to https://source.denx.de/u-boot/custodians/u-boot-dfu (u-boot-dfu)
[1/1] usb: gadget: ether: Handle gadget driver registration in start and stop https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/d94c0ee1de89e2f...
-- Mattijs
Hi Zixun,
On mer., août 07, 2024 at 08:38, Mattijs Korpershoek mkorpershoek@baylibre.com wrote:
Hi,
On Fri, 26 Jul 2024 10:31:00 +0200, Zixun LI wrote:
Revert part of 718f1d41 to move usb_gadget_register_driver()/usb_gadget_unregister_driver() back to usb_eth_start()/usb_eth_stop().
usb_gadget_register_driver() will initialize the USB controller which enters ready to connect state with pull-up resistor enabled.
[...]
Thanks, Applied to https://source.denx.de/u-boot/custodians/u-boot-dfu (u-boot-dfu)
[1/1] usb: gadget: ether: Handle gadget driver registration in start and stop https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/d94c0ee1de89e2f...
-- Mattijs
There has been some ongoing discussion on IRC about this patch: https://libera.irclog.whitequark.org/u-boot/2024-08-20
Marek, who has quite some knowledge about the USB stack in U-Boot (more than myself) suggested that using usb_ether_init() should not be used anymore.
Instead, to enable usb ethernet, we should manually bind the UDC driver to the usb_ether gadget.
For example, on Khadas VIM3 board, this can be done with:
=> bind /soc/usb@ffe09000/usb@ff400000 usb_ether
Use "dm tree" to find the node path for your UDC.
Then, I can enable Ethernet Gadget with:
=> setenv ethact usb_ether
And only when using it, I will see enumerations:
=> dhcp
[437548.488938] usb 1-1: New USB device found, idVendor=1b8e, idProduct=fada, bcdDevice=7e.8a [437548.488950] usb 1-1: New USB device strings: Mfr=1, Product=2, SerialNumber=0 [437548.488957] usb 1-1: Product: RNDIS/Ethernet Gadget [437548.488963] usb 1-1: Manufacturer: U-Boot [437548.634417] usbcore: registered new interface driver cdc_ether [437548.640519] rndis_host 1-1:2.0 usb0: register 'rndis_host' at usb-0000:00:14.0-1, RNDIS device, de:ad:be:ef:00:00 [437548.640588] usbcore: registered new interface driver rndis_host [437548.694343] rndis_host 1-1:2.0 enp0s20f0u1c2: renamed from usb0
So, with this method, I can't reproduce the problem that this patch was initially try to solve.
Could you please try using the bind method instead of the (deprecated) usb_ether_init() ?
Instead, to enable usb ethernet, we should manually bind the UDC driver to the usb_ether gadget.
For example, on Khadas VIM3 board, this can be done with:
=> bind /soc/usb@ffe09000/usb@ff400000 usb_ether
It would be great if this could be done like ums where the UDC index is given rather than having to figure out the magic DT path incantation :D
=> ums 0 scsi 0,1,2,3,4,5
Use "dm tree" to find the node path for your UDC.
On 8/20/24 7:11 PM, Caleb Connolly wrote:
Instead, to enable usb ethernet, we should manually bind the UDC driver to the usb_ether gadget.
For example, on Khadas VIM3 board, this can be done with:
=> bind /soc/usb@ffe09000/usb@ff400000 usb_ether
It would be great if this could be done like ums where the UDC index is given rather than having to figure out the magic DT path incantation :D
=> ums 0 scsi 0,1,2,3,4,5
Yeah ... I wonder if we want an 'udc' command ?
On mar., août 20, 2024 at 19:12, Marek Vasut marex@denx.de wrote:
On 8/20/24 7:11 PM, Caleb Connolly wrote:
Instead, to enable usb ethernet, we should manually bind the UDC driver to the usb_ether gadget.
For example, on Khadas VIM3 board, this can be done with:
=> bind /soc/usb@ffe09000/usb@ff400000 usb_ether
It would be great if this could be done like ums where the UDC index is given rather than having to figure out the magic DT path incantation :D
=> ums 0 scsi 0,1,2,3,4,5
Agreed, but we don't want to have one U-Boot command per gadget function.
Yeah ... I wonder if we want an 'udc' command ?
This seems like a nice idea. I will give it some more thoughts and look into prototyping something.
participants (4)
-
Caleb Connolly -
Marek Vasut -
Mattijs Korpershoek -
Zixun LI