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

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi,

marex@denx.de wrote on Fri, 4 Aug 2023 17:05:01 +0200:

> On 8/4/23 17:01, Tom Rini wrote:
> > On Fri, Aug 04, 2023 at 04:42:13PM +0200, Marek Vasut wrote:  
> >> On 8/4/23 09:00, Miquel Raynal wrote:  
> >>> Hi Marek,
> >>>
> >>> marex@denx.de wrote on Wed,  2 Aug 2023 14:46:54 +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@49000000 usb_ether
> >>>> 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@49000000 usb_ether  
> >>>
> >>> This extra parameter does not seem to work on the BBBW. I cannot select
> >>> the "usb_ether" driver like you do.
> >>>
> >>> Good news though, I am now able to use fastboot, but it is not
> >>> straightforward:
> >>>
> >>> Here is my sequence right after the boot (reducing the dm tree output
> >>> to the usb nodes for clarity):
> >>>  
> >>> => dm tree  
> >>>    misc          0  [ + ]   ti-musb-wrapper       |   |-- usb@47400000
> >>>    usb           0  [ + ]   ti-musb-peripheral    |   |   |-- usb@47401000
> >>>    ethernet      1  [ + ]   usb_ether             |   |   |   `-- usb_ether
> >>>    bootdev       3  [   ]   eth_bootdev           |   |   |       `-- usb_ether.bootdev
> >>>    usb           0  [   ]   ti-musb-host          |   |   `-- usb@47401800  
> >>> => fastboot usb 0  
> >>> couldn't find an available UDC
> >>> g_dnl_register: failed!, error: -19  
> >>
> >> That is expected and not a bug, since the beagle explicitly binds USB
> >> ethernet to MUSB gadget in board file, which is legacy deprecated way.  
> > 
> > So, we should do away with, probably all of arch_misc_init() in
> > arch/arm/mach-omap2/am33xx/board.c for the non-SPL case.  
> 
> Yes
> 
> >>> exit not allowed from main input shell.  
> >>> => unbind /ocp/usb@47400000/usb@47401000 usb_ether  
> >>
> >> Does
> >>  
> >> => unbind ethernet 0  
> >>
> >> work ?
> >>
> >> If so, 1/4 in this series can be skipped altogether.
> >>
> >> You likely won't even need the rebinding of ti-musb-peripheral anymore.
> >>  
> >>> Cannot find a device with path /ocp/usb@47400000/usb@47401000  
> >>> => unbind /ocp/usb@47400000/usb@47401000
> >>> => dm tree  
> >>>    misc          0  [ + ]   ti-musb-wrapper       |   |-- usb@47400000
> >>>    usb           0  [   ]   ti-musb-host          |   |   `-- usb@47401800  
> >>> => fastboot usb 0
> >>> => bind /ocp/usb@47400000/usb@47401000 ti-musb-peripheral
> >>> => dm tree  
> >>>    misc          0  [ + ]   ti-musb-wrapper       |   |-- usb@47400000
> >>>    usb           0  [   ]   ti-musb-host          |   |   |-- usb@47401800
> >>>    usb           0  [   ]   ti-musb-peripheral    |   |   `-- usb@47401000  
> >>> => fastboot usb 0  
> >>> musb-hdrc: peripheral reset irq lost!
> >>> # works! (the irq-related line above as always been there)
> >>>
> >>> So now, how do we make this process easy/understandable?  
> >>
> >> What would be your proposal ? 

At least I would appreciate:
- to select CMD_BIND "by default" when relevant
- to make the fastboot error more readable for the regular user

If you want to get rid of all the legacy code, I am not opposed,
really, but that must not be the user who is responsible for
understanding what changed in the "core". It must be very easily
accessible to understand that now:
- manual binding/unbinding is needed
- which driver must be used when

> > Well, what's needed / is it possible to get to the point where we don't
> > _need_ to call bind/unbind for each of these cases? Is there something
> > we're supposed to be setting in the DT that we aren't?  
> 
> You do need to unbind the ethernet before using fastboot, always, with the 'unbind ethernet 0', you dont need the peripheral unbind/rebind part, so it should behave like before.

And for my own understanding, why don't we need to bind a fastboot
gadget?

Thanks,
Miquèl