[PATCH v4 1/4] cmd: bind: Add unbind command with driver filter
Tom Rini
trini at konsulko.com
Fri Aug 4 18:15:15 CEST 2023
On Fri, Aug 04, 2023 at 06:00:12PM +0200, Miquel Raynal wrote:
> Hi Marek,
>
> marex at denx.de wrote on Fri, 4 Aug 2023 17:40:07 +0200:
>
> > On 8/4/23 17:12, Miquel Raynal wrote:
> > > Hi,
> > >
> > > marex at 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 at 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 at 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 at 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 at 47400000
> > >>>>> usb 0 [ + ] ti-musb-peripheral | | |-- usb at 47401000
> > >>>>> ethernet 1 [ + ] usb_ether | | | `-- usb_ether
> > >>>>> bootdev 3 [ ] eth_bootdev | | | `-- usb_ether.bootdev
> > >>>>> usb 0 [ ] ti-musb-host | | `-- usb at 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 at 47400000/usb at 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.
> >
> > Did you test this yet ?
>
> Not yet, I'll send you the feedback once done.
>
> > >>>>> Cannot find a device with path /ocp/usb at 47400000/usb at 47401000
> > >>>>> => unbind /ocp/usb at 47400000/usb at 47401000
> > >>>>> => dm tree
> > >>>>> misc 0 [ + ] ti-musb-wrapper | |-- usb at 47400000
> > >>>>> usb 0 [ ] ti-musb-host | | `-- usb at 47401800
> > >>>>> => fastboot usb 0
> > >>>>> => bind /ocp/usb at 47400000/usb at 47401000 ti-musb-peripheral
> > >>>>> => dm tree
> > >>>>> misc 0 [ + ] ti-musb-wrapper | |-- usb at 47400000
> > >>>>> usb 0 [ ] ti-musb-host | | |-- usb at 47401800
> > >>>>> usb 0 [ ] ti-musb-peripheral | | `-- usb at 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
> >
> > Since with this 'unbind ethernet 0' this is orthogonal to this series, send separate patches, thanks.
>
> This is not orthogonal, I am sorry.
>
> version X:
> - tftp works "out of the box"
> - fastboot works "out of the box"
> version X+1:
> - tftp works "out of the box"
> - fastboot returns an obscure error
>
> 1/ If we now *need* the bind/unbind commands, the series must take care
> of it.
> 2/ Without proper error message you just break fastboot for most
> regular users (basically everyone but few U-Boot devs).
>
> > > 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
> >
> > When my spare time permits.
>
> I understand. But I strongly disagree with this approach. We want to
> make U-Boot better. I am fine with all internal changes you wish, even
> if it breaks the CLI at some point because it is more accurate and
> drops a ton of legacy code. Again, this is okay as long as the CLI
> tells the user what changed in the behavior. So when I run
> tftp/dhcp/ping/whatever, if you don't want to automatically bind the
> ethernet gadget I'm okay, but just tell the user "Please make sure the
> Ethernet gadget is bound, eg: unbind xxx; bind yyy" (possibly giving
> automatic shortcuts to avoid the pain of finding the path of the
> controller). And when fastboot fails, same idea.
>
> Could you please consider enhancing these parts as well?
>
> > >>> 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?
> >
> > The fastboot command does that internally .
>
> This is not visible with dm tree and I did not find how to bind the
> fastboot gadget manually.
>
> This makes the CLI clearly uneven and the internal code badly different
> between gadgets/commands. Why can't we have them both
> autoloaded/unloaded like before? I think I missed something which
> explains the rationale behind this series.
They aren't both auto-loaded currently. We have a legacy call,
usb_ether_init(), in a few cases, so that gadget mode ethernet starts.
But this leads to the ref counting problems you encountered and
re-posted the rejected series for.
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20230804/928fa2d6/attachment.sig>
More information about the U-Boot
mailing list