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(-)

--
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/d94c0ee1de89e2fbd3cc5bea6351dbaa9462c84f

--
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/d94c0ee1de89e2fbd3cc5bea6351dbaa9462c84f
>
> --
> 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.