Hi Tim,
On Wed, 16 Feb 2022 at 17:03, Tim Harvey <thar...@gateworks.com> wrote:
>
> On Wed, Feb 16, 2022 at 12:23 AM Patrice CHOTARD
> <patrice.chot...@foss.st.com> wrote:
> >
> > Hi Tim
> >
> > On 2/15/22 19:28, Tim Harvey wrote:
> > > On Tue, Feb 15, 2022 at 9:36 AM Patrice CHOTARD
> > > <patrice.chot...@foss.st.com> wrote:
> > >>
> > >> Hi Tim
> > >>
> > >> On 2/15/22 17:09, Tim Harvey wrote:
> > >>> On Tue, Feb 15, 2022 at 5:29 AM Patrice CHOTARD
> > >>> <patrice.chot...@foss.st.com> wrote:
> > >>>>
> > >>>> Hi Tim
> > >>>>
> > >>>> On 2/10/22 18:04, Tim Harvey wrote:
> > >>>>> Greetings,
> > >>>>>
> > >>>>> I'm trying to understand how to use the U-Boot bind command to bind
> > >>>>> the usb_ether driver to the usb class to register a USB ethernet
> > >>>>> gadget network device as referenced in:
> > >>>>> commit 02291d83fdaaf ("doc: add bind/unbind command documentation")
> > >>>>> commit 49c752c93a78 ("cmd: Add bind/unbind commands to bind a device
> > >>>>> to a driver from the command line")
> > >>>>>
> > >>>>
> > >>>> For example, i made some trial on STM32MP1 platform:
> > >>>>
> > >>>> At boot, we got :
> > >>>>
> > >>>> STM32MP> dm tree
> > >>>> Class Index Probed Driver Name
> > >>>> -----------------------------------------------------------
> > >>>> root 0 [ + ] root_driver root_driver
> > >>>> firmware 0 [ ] psci |-- psci
> > >>>> sysreset 0 [ ] psci-sysreset | `-- psci-sysreset
> > >>>> .....
> > >>>> blk 0 [ + ] mmc_blk | | `--
> > >>>> m...@58005000.blk
> > >>>> ethernet 0 [ + ] eth_eqos | |--
> > >>>> ethernet@5800a000
> > >>>> eth_phy_ge 0 [ + ] eth_phy_generic_drv | | `--
> > >>>> ethernet-phy@0
> > >>>> usb 0 [ ] ehci_generic | |-- usb@5800d000
> > >>>> video 0 [ ] stm32_display | |--
> > >>>> display-controller@5a001000
> > >>>> .....
> > >>>>
> > >>>>
> > >>>> As you can see, there is already an ethernet interface used.
> > >>>> We unbind the ethernet interface before binding the usb_ether gadget
> > >>>> to the usb class.
> > >>>> First unbind the generic ethernet phy (eth_phy_generic_drv) and the
> > >>>> ethernet driver
> > >>>> (eth_eqos).
> > >>>>
> > >>>>
> > >>>> STM32MP> unbind eth_phy_generic 0
> > >>>> STM32MP> unbind ethernet 0
> > >>>> STM32MP> dm tree
> > >>>> Class Index Probed Driver Name
> > >>>> -----------------------------------------------------------
> > >>>> root 0 [ + ] root_driver root_driver
> > >>>> firmware 0 [ ] psci |-- psci
> > >>>> sysreset 0 [ ] psci-sysreset | `-- psci-sysreset
> > >>>> ....
> > >>>> blk 0 [ + ] mmc_blk | | `--
> > >>>> m...@58005000.blk
> > >>>> usb 0 [ ] ehci_generic | |-- usb@5800d000
> > >>>> video 0 [ ] stm32_display | |--
> > >>>> display-controller@5a001000
> > >>>> ....
> > >>>>
> > >>>> Ethernet and phy driver are both unbinded.
> > >>>> Now we can bind the usb_eher to the usb class
> > >>>>
> > >>>> STM32MP> bind usb 0 usb_ether
> > >>>> STM32MP> dm tree
> > >>>> Class Index Probed Driver Name
> > >>>> -----------------------------------------------------------
> > >>>> root 0 [ + ] root_driver root_driver
> > >>>> firmware 0 [ ] psci |-- psci
> > >>>> sysreset 0 [ ] psci-sysreset | `-- psci-sysreset
> > >>>> ....
> > >>>> blk 0 [ + ] mmc_blk | | `--
> > >>>> m...@58005000.blk
> > >>>> usb 0 [ ] ehci_generic | |-- usb@5800d000
> > >>>> ethernet 0 [ ] usb_ether | | `-- usb_ether
> > >>>> video 0 [ ] stm32_display | |--
> > >>>> display-controller@5a001000
> > >>>> ....
> > >>>>
> > >>>> usb_ether is now binded.
> > >>>> As example, if you can then use some ethernet command as dhcp or ping :
> > >>>>
> > >>>> STM32MP> dhcp
> > >>>> using dwc2-udc, OUT ep2out-bulk IN ep1in-bulk STATUS ep3in-int
> > >>>> MAC de:ad:be:ef:00:01
> > >>>> HOST MAC de:ad:be:ef:00:00
> > >>>> RNDIS ready
> > >>>> high speed config #2: 2 mA, Ethernet Gadget, using RNDIS
> > >>>> USB RNDIS network up!
> > >>>> BOOTP broadcast 1
> > >>>>
> > >>>>> I have enabled:
> > >>>>> CONFIG_DM_USB=y
> > >>>>> CONFIG_USB_GADGET=y
> > >>>>> CONFIG_USB_ETHER=y
> > >>>>>
> > >>>> In my case i enabled also CONFIG_USB_ETH_RNDIS=y
> > >>>>
> > >>>
> > >>> Patrice,
> > >>>
> > >>> In my case when I try to bind to usb_ether the device can not be found
> > >>> (as it is never registered in the first place):
> > >>> Ventana > unbind ethernet 0
> > >>> Ventana > bind usb 0 usb_ether
> > >>> Cannot find device 0 of class usb
> > >>
> > >> weird, because below, in the dm tree output, we can see :
> > >>
> > >>> usb 0 [ ] ehci_mx6 | | |-- usb@2184000
> > >>> usb 1 [ ] ehci_mx6 | | |-- usb@2184200
> > >>
> > >> so it should find a usb class device .....
> > >>
> > >
> > > Patrice,
> > >
> > > I added some debugging and found that 'bind usb 0 usb_ether' does the
> > > following:
> > > bind_by_class_index(class="usb" index=0 drv="usb_ether")
> > > uclass_get_by_name(name="usb")
> > > uclass_get_by_name_len(name="usb" len=3)
> > > ^^^ does a strncmp with name=usb and len=3 so it matches the first
> > > driver with a class name starting with 'usb' and in this case matches
> > > usb_mass_storage instead of 'usb'
> >
> > Good catch ;-)
> >
> > >
> > > Simon, this behavior comes from commit 4b030177b660 ("dm: core: Allow
> > > finding children / uclasses by partial name"). Why would
> > > device_find_child_by_name() want to use a substring match? I suppose
> > > if there is a valid reason for this such as your commit logs describe
> > > then those functions should directly call uclass_get_by_name_len() and
> > > uclass_get_by_name() should be doing a full name match correct?
> > >
> > > Patrice, if I make uclass_get_by_name match the full string with the
> > > below patch then 'bind usb 0 usb_ether' indeed works as planned:
> > >
> > > diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c
> > > index 2578803b7a4d..33dbca544574 100644
> > > --- a/drivers/core/uclass.c
> > > +++ b/drivers/core/uclass.c
> > > @@ -196,7 +196,16 @@ enum uclass_id uclass_get_by_name_len(const char
> > > *name, int len)
> > >
> > > enum uclass_id uclass_get_by_name(const char *name)
> > > {
> > > - return uclass_get_by_name_len(name, strlen(name));
> > > + int i;
> > > +
> > > + for (i = 0; i < UCLASS_COUNT; i++) {
> > > + struct uclass_driver *uc_drv = lists_uclass_lookup(i);
> > > +
> > > + if (uc_drv && !strcmp(uc_drv->name, name))
> > > + return i;
> > > + }
> > > +
> > > + return UCLASS_INVALID;
> > > }
> > >
> > > int dev_get_uclass_index(struct udevice *dev, struct uclass **ucp)
> > >
> > > I don't see the need to unbind 'ethernet' as you can select the active
> > > network device with 'ethact':
> > >
> > > Ventana > net list
> > > eth0 : ethernet@2188000 00:d0:12:f3:f2:f5 active
> > > Ventana > bind usb 0 usb_ether
> > > Ventana > net list
> > > eth0 : ethernet@2188000 00:d0:12:f3:f2:f5 active
> > > eth1 : usb_ether 00:00:00:00:00:00
> > > Ventana > setenv ethact eth1
> >
> >
> > Thanks for the tips, i didn't know it.
> >
> > > Ventana > ping 192.168.1.146
> > > ^^^ but now we run into the issue Heiko discovered where the
> > > usb_gadget_register_driver() call from ether.c ends up removing usb
> > > and usb_ether and without his hack to return from device-remove if
> > > device is usb_ether we hang:
> > >
> > > @@ -201,12 +202,16 @@ int device_remove(struct udevice *dev, uint flags)
> > > const struct driver *drv;
> > > int ret;
> > >
> > > +printf("%s %s\n", __func__, dev->name);
> > > if (!dev)
> > > return -EINVAL;
> > >
> > > if (!(dev_get_flags(dev) & DM_FLAG_ACTIVATED))
> > > return 0;
> > >
> > > + if (!strncmp(dev->name, "usb_ether", 8))
> > > + return 0;
> > > +
> > > /*
> > > * If the child returns EKEYREJECTED, continue. It just means
> > > that it
> > > * didn't match the flags.
> > >
> > >
>
> Simon,
>
> Now that we have identified a fix needed to uclass_get_by_name to fix
> the dm bind command, can you comment on the other issue we have run
> into trying to use usb_ether?
>
> Here's an example with some debugging:
> Ventana > net list
> device_probe ethernet@2188000 flags=0x1043
> eth0 : ethernet@2188000 00:d0:12:f3:f2:f5 active
> Ventana > bind usb 0 usb_ether
> Ventana > net list
> eth0 : ethernet@2188000 00:d0:12:f3:f2:f5 active
> eth1 : usb_ether 00:00:00:00:00:00
> Ventana > setenv ethact eth1
> Ventana > ping 192.168.1.146
> device_probe ethernet@2188000 flags=0x1043
> device_probe usb_ether flags=0x4a
> device_probe usb@2184000 flags=0x1042
> device_probe bus@2100000 flags=0x1051
> device_probe usbotggrp flags=0x40
> device_probe iomuxc@20e0000 flags=0x1041
> device_probe iomuxc@20e0000 flags=0x1041
> device_probe regulator-usb-otg-vbus flags=0x52
> device_probe gpio@20a4000 flags=0x1043
> device_probe root_driver flags=0x1041
> device_probe iomuxc@20e0000 flags=0x1041
> usb_eth_probe usb_ether
> usb_eth_start
> usb_setup_ehci_gadget
> usb_setup_ehci_gadget removing old device 'usb@2184000'
> device_remove usb@2184000
> device_remove usb_ether
> usb_eth_stop
> usb_setup_ehci_gadget probing 'usb@2184000'
> device_probe usb@2184000 flags=0x42
> device_probe bus@2100000 flags=0x1051
> device_probe usbotggrp flags=0x1041
> device_probe regulator-usb-otg-vbus flags=0x1053
> usb_setup_ehci_gadget done
> ^^^ hangs here - notice usb controller and the usb_ether child were
> removed then usb controller probed again (but not usb_ether)
>
> fbeceb260232 ("dm: usb: Allow setting up a USB controller as a
> device/gadget") adds the usb_setup_ehci_gadget() function which
> removes the old USB controller device (and its usb_ether child) then
> probes only the USB controller leaving the usb_ether device un-probed.
> The commit log makes it sound like something isn't complete:
>
> Some controllers support OTG (on-the-go) where they can operate as either
> host or device. The gadget layer in U-Boot supports this.
>
> While this layer does not interact with driver model, we can provide a
> function which sets up the controller in the correct way. This way the
> code
> at least builds (although it likely will not work).
>
> I'm not clear why the USB controller (and children) need to be removed
> here. If I comment out the removal and re-probe of the controller
> usb_ether then works fine:
>
> diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
> index 27e2fc6fcd36..0882641b51b0 100644
> --- a/drivers/usb/host/usb-uclass.c
> +++ b/drivers/usb/host/usb-uclass.c
> @@ -399,6 +399,7 @@ int usb_setup_ehci_gadget(struct ehci_ctrl **ctlrp)
> ret = uclass_find_first_device(UCLASS_USB, &dev);
> if (ret)
> return ret;
> +#if 0
> ret = device_remove(dev, DM_REMOVE_NORMAL);
> if (ret)
> return ret;
> @@ -408,6 +409,7 @@ int usb_setup_ehci_gadget(struct ehci_ctrl **ctlrp)
> ret = device_probe(dev);
> if (ret)
> return ret;
> +#endif
> *ctlrp = dev_get_priv(dev);
>
> return 0;
>
> Why was it necessary to remove the USB controller and children and reprobe?
+Marek Vasut who may know more
I suspect that this is a bit of a hack to get the device running after
it is swtiched from host to device mode?
The gadget side of things should really move to driver model.
Regards,
SImon
