[PATCH v2 1/4] cmd: bind: Add unbind command with driver filter

[Miquel Raynal]

Hi Marek,

marex at denx.de wrote on Sun, 23 Jul 2023 23:45:55 +0200:

> On 7/23/23 19:49, Miquel Raynal wrote:
> > Hi Marek,  
> 
> Hi,
> 
> > marex at denx.de wrote on Mon, 17 Jul 2023 13:21:34 +0200:
> >   
> >> Extend the driver core to perform lookup by both OF node and driver
> >> bound to the node. Use this to look up specific device instances to
> >> unbind from nodes in the unbind command. One example where this is
> >> needed is USB peripheral controller, which may have multiple gadget
> >> drivers bound to it. The unbind command has to select that specific
> >> gadget driver instance to unbind from the controller, not unbind the
> >> controller driver itself from the controller.
> >>
> >> USB ethernet gadget usage looks as follows with this change. Notice
> >> the extra 'usb_ether' addition in the 'unbind' command at the end.
> >> "
> >> bind /soc/usb-otg at 49000000 usb_ether  
> > 
> > I don't really get why this is needed? Yes, having proper bind and
> > unbind methods and having them called internally is relevant, but when
> > you have a single OTG controller, why is this needed?  
> 
> Because you do need to bind the usb_ether driver to a controller.

Right now it seems that binding is kind of performed in the context of
a command execution (and then everything is cleaned up). I also think
it is not optimal, but I would like to avoid breaking common commands
like you propose. At the *very least* the error messages should be very
clear about what has changed and what the user needs to do now in order
to workaround it. But the best would be to avoid user interaction as
much as possible. So why not load the first driver needed and expect
the user the run the correct unbind/bind commands if it wants to
change?

> I suspect you are trying to point in the direction of usb_ether_init() , right ? That is bogus and should be removed, because that is hard-coding one specific (ethernet) function to an OTG controller.

Well, hardcoding is bad, but setting a default is often useful and much
more user friendly, IMHO.

> 
> > It basically
> > breaks the CLI  
> 
> Can you elaborate on this ?
> 
> How does calling a 'bind' command breaks CLI ?

I've attempted a bisect between U-Boot 2018 and 2023, it was already
very painful. But if I try again in 2024 and need to cope with the new
rules added in 2023 for binding drivers which were not needed until,
then it becomes even more painful. So yes, it breaks the CLI. Before:
=> tftp xxx
=> fastboot xxx
just worked, after you patchset these simple operations won't work
anymore.

> > , making bisects more painful and all updates just fail.  
> 
> This is just utter nonsense, sorry.

Well, I am a U-Boot user, and I expect this change to break
a good amount of boot scripts after an update.

> >> setenv ethact usb_ether
> >> setenv loadaddr 0xc2000000
> >> setenv ipaddr 10.0.0.2
> >> setenv serverip 10.0.0.1
> >> setenv netmask 255.255.255.0
> >> tftpboot 0xc2000000 10.0.0.1:test.file
> >> unbind /soc/usb-otg at 49000000 usb_ether
> >> "
> >>
> >> Signed-off-by: Marek Vasut <marex at denx.de>
> >> ---
> >> Cc: Kevin Hilman <khilman at baylibre.com>
> >> Cc: Lukasz Majewski <lukma at denx.de>
> >> Cc: Marek Vasut <marex at denx.de>
> >> Cc: Simon Glass <sjg at chromium.org>  
> > 
> > I've tested the whole series, unfortunately is does not work on
> > AM335x/BBBW:
> > 
> > * Any recovery attempted using the network will now fail in
> >    the SPL, where, AFAIK, there is no way to manually bind:
> > 
> > U-Boot SPL 2023.07-00806-gac80e6de9cf (Jul 23 2023 - 19:45:51 +0200)
> > Trying to boot from USB eth
> > Could not get PHY for eth_cpsw: addr 0
> > eth0: eth_cpswusing musb-hdrc, OUT ep1out IN ep1in STATUS ep2in
> > MAC de:ad:be:ef:00:01
> > HOST MAC de:ad:be:ef:00:00
> > RNDIS ready
> > , eth1: usb_ether  
> 
> It seems the RNDIS adapter is actually ready based on the above ^ ?
> 
> > * The bind command was not available on my default configuration,
> >    making it even difficult for people unaware that this command is
> >    now required to fix their common commands.  
> 
> It has been required ever since, it's just that many ignored the need to move over to DM and depended on board-specific hackage to set up their USB ethernet. Now that hackage broke, time to switch to the proper way.

It breaks because there is something wrong elsewhere. It's not at all
the fault of the board hack (if any, I have no idea, I'll trust your
judgment regarding the BBB support). My series fixes a real problem in
the current code. The fact that it won't be visible after your series
is almost anecdotic... until it triggers again for whatever reason.

> > * Any command that expects the usb_ether driver will now fail badly
> >    even after the bind call:
> >   
> > => bind /ocp/usb at 47400000/usb at 47401000 usb_ether
> > => fastboot usb 0  
> > couldn't find an available UDC  
> 
> You already have the ethernet function bound to the UDC and now you are trying to bind another function, fastboot, right ? This is likely the problem. Unbind the ethernet first, then use fastboot.
> 
> > g_dnl_register: failed!, error: -19
> > exit not allowed from main input shell.  
> > => tftp 0x81000000 zImage  
> > dev_get_priv: null device
> > data abort
> > pc : [<9ffa04ba>]          lr : [<9ff86d5f>]
> > reloc pc : [<8083b4ba>]    lr : [<80821d5f>]
> > sp : 9df2f920  ip : 00000000     fp : 00000003
> > r10: 9df4cf48  r9 : 9df44ea0     r8 : 9ffec33c
> > r7 : 00004a53  r6 : 00000000     r5 : 00000bb8  r4 : 9df4d3c0
> > r3 : 9ff97653  r2 : fff1102c     r1 : 00000000  r0 : 00000000
> > Flags: nzcv  IRQs off  FIQs on  Mode SVC_32 (T)
> > Code: 9ffd b508 f7e6 fc4b (6801) 2000
> > Resetting CPU ...
> > 
> > Is there anything I missed?  
> 
> Please see above.

Sorry for disturbing you with the fastboot command. Can you advise a
couple of commands to test fastboot then? I don't find the relevant
driver name to use.

Here is a boot without anything else than a tftp, no change:

U-Boot SPL 2023.07-00806-gac80e6de9cf (Jul 23 2023 - 19:45:51 +0200)
Trying to boot from MMC2


U-Boot 2023.07-00806-gac80e6de9cf (Jul 23 2023 - 19:45:51 +0200)

CPU  : AM335X-GP rev 2.1
Model: TI AM335x BeagleBone Black
DRAM:  512 MiB
Core:  160 devices, 18 uclasses, devicetree: separate
WDT:   Started wdt at 44e35000 with servicing every 1000ms (60s timeout)
NAND:  0 MiB
MMC:   OMAP SD/MMC: 0, OMAP SD/MMC: 1
Loading Environment from FAT... Unable to read "uboot.env" from mmc1:1... 
<ethaddr> not set. Validating first E-fuse MAC
Net:   Could not get PHY for ethernet at 4a100000: addr 0
eth2: ethernet at 4a100000using musb-hdrc, OUT ep1out IN ep1in STATUS ep2in
MAC de:ad:be:ef:00:01
HOST MAC de:ad:be:ef:00:00
RNDIS ready
, eth3: usb_ether
=> bind /ocp/usb at 47400000/usb at 47401000 usb_ether
=> tftp 0x81000000 zImage
dev_get_priv: null device
data abort
pc : [<9ffa04ba>]          lr : [<9ff86d5f>]
reloc pc : [<8083b4ba>]    lr : [<80821d5f>]
sp : 9df2f920  ip : 00000000     fp : 00000003
r10: 9df4cf48  r9 : 9df44ea0     r8 : 9ffec33c
r7 : 0000538c  r6 : 00000000     r5 : 00000bb8  r4 : 9df4d3c0
r3 : 9ff97653  r2 : fff69249     r1 : 00000000  r0 : 00000000
Flags: nzcv  IRQs off  FIQs on  Mode SVC_32 (T)
Code: 9ffd b508 f7e6 fc4b (6801) 2000 
Resetting CPU ...


Thanks,
Miquèl