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

[Tom Rini]

On Fri, Aug 04, 2023 at 08:01:56PM +0200, Miquel Raynal wrote:
> Hi Tom,
> 
> trini at konsulko.com wrote on Fri, 4 Aug 2023 13:20:29 -0400:
> 
> > On Fri, Aug 04, 2023 at 07:01:46PM +0200, Miquel Raynal wrote:
> > > Hi Tom,
> > >   
> > > > > > >>> 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.  
> > > 
> > > Ok, thanks for the additional details.
> > > 
> > > I don't understand why fastboot autoloads the correct gadget driver if
> > > there is none bound, while all network commands just fail to do that if
> > > we don't make the usb_ether_init() call manually.  
> > 
> > Because "fastboot" or "dfu" are both being told (as part of their call)
> > "find usb gadget controller number X".  That's doing the bind.  Calling
> > usb_ether_init just takes the first controller and that's that (and so
> > could be / is wrong depending on the platform).
> 
> This makes total sense, thanks for pointing it out.
> 
> > > I also don't understand why I need to unbind the ethernet gadget but I
> > > cannot bind the fastboot gadget.  
> > 
> > You can't bind fastboot while ethernet is still configured.
> 
> I guess "before this series", the ethernet would not be kept
> configured, because I could use both fastboot and ethernet without
> caring about which driver had to be bound. And that's maybe what lead to
> the bug reports also. So you want to get rid of this. Do I get the
> situation right now?

We're getting rid of this path because it leads to failures, yes.

> >  It's in
> > use.  And we aren't a full blown operating system with the logic to have
> > multiple end points and devices configured and exposed.  I mean heck, we
> > don't keep the gadget interface up between network calls so on the host
> > side you need to re-configure the interface (or have something that's
> > bringing it up there again each time).  Which is why DFU or fastboot or
> > other "treat USB like USB" options tend to be more popular than "treat
> > USB like ethernet" work flows.
> 
> Yes of course.
> 
> > > My underlying question is: can we have a single approach for all
> > > drivers, or is it too complex today? Could it be possible, when we
> > > perform these autoloads, to look up the registered gadget and
> > > potentially unbind the one already in place before?  
> > 
> > I would invite you to look in to this, yes.
> 
> Sounds reasonably complex now, with the reasoning you made above.
> 
> >  No one objects to enhancing
> > the code, but the "functionality" you see on am33xx is as you've also
> > seen very broken in other ways, which is why it's not used virtually
> > anywhere else and instead the "bind" command is.  For example, if you
> > wanted to do this workflow on the new beagle devices, that's DWC3 and
> > where the "bind" command came from :)
> 
> Again to be very clear, while I felt misunderstood at the beginning and
> did not accept Marek's series because it was replacing a data abort
> with a non-functional setup, I now get a better understanding of the
> problem (I believe) and, as said before, I am always in favor of
> writing better code, easier to maintain, clearer to review. I am in
> favor of such change. I just want the user life to be eased when this
> happens if we break the CLI. So if you think the right approach is to
> get rid of the usb_ether_init() call, alright, let's drop it off. But
> we should not let the users in the dark by providing proper doc or
> error messages which should compensate the extra step now required.

I think it would be good if you posted a patch to update
doc/board/ti/am335x_evm.rst to explain how to use the various gadget
device cases.

-- 
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/efd4852f/attachment.sig>