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

[Marek Vasut]

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 ?

>>>>> 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.

> 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.

>>> 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 .