[U-Boot] [PATCH v3 7/7] cmd: Add bind/unbind commands to bind a device to a driver from the command line

[Michal Simek]

On 17.7.2018 05:44, Simon Glass wrote:
> Hi Michal,
> 
> On 16 July 2018 at 02:33, Michal Simek <michal.simek at xilinx.com> wrote:
>> On 11.7.2018 22:13, Simon Glass wrote:
>>> Hi,
>>>
>>> On 11 July 2018 at 07:40, Tom Rini <trini at konsulko.com> wrote:
>>>>
>>>> On Wed, Jul 11, 2018 at 03:31:39PM +0200, Michal Simek wrote:
>>>>> On 11.7.2018 14:46, Tom Rini wrote:
>>>>>> On Wed, Jul 11, 2018 at 07:57:13AM +0200, Michal Simek wrote:
>>>>>>> On 10.7.2018 18:40, Tom Rini wrote:
>>>>>>>> On Mon, Jul 09, 2018 at 11:59:57AM -0500, Joe Hershberger wrote:
>>>>>>>>> On Mon, Jul 9, 2018 at 9:43 AM, Tom Rini <trini at konsulko.com> wrote:
>>>>>>>>>> On Mon, Jul 09, 2018 at 08:19:44AM +0200, Michal Simek wrote:
>>>>>>>>>>> On 30.6.2018 06:19, Simon Glass wrote:
>>>>>>>>>>>> On 27 June 2018 at 07:13, Michal Simek <michal.simek at xilinx.com> wrote:
>>>>>>>>>>>>> On 22.6.2018 14:25, Jean-Jacques Hiblot wrote:
>>>>>>>>>>>>>> In some cases it can be useful to be able to bind a device to a driver from
>>>>>>>>>>>>>> the command line.
>>>>>>>>>>>>>> The obvious example is for versatile devices such as USB gadget.
>>>>>>>>>>>>>> Another use case is when the devices are not yet ready at startup and
>>>>>>>>>>>>>> require some setup before the drivers are bound (ex: FPGA which bitsream is
>>>>>>>>>>>>>> fetched from a mass storage or ethernet)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> usage example:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> bind usb_dev_generic 0 usb_ether
>>>>>>>>>>>>>> unbind usb_dev_generic 0 usb_ether
>>>>>>>>>>>>>> or
>>>>>>>>>>>>>> unbind eth 1
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> bind /ocp/omap_dwc3 at 48380000/usb at 48390000 usb_ether
>>>>>>>>>>>>>> unbind /ocp/omap_dwc3 at 48380000/usb at 48390000
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot at ti.com>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Changes in v3:
>>>>>>>>>>>>>> - factorize code based on comments from ML
>>>>>>>>>>>>>> - remove the devices before unbinding them
>>>>>>>>>>>>>> - use device_find_global_by_ofnode() to get a device by its node.
>>>>>>>>>>>>>> - Added tests
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Changes in v2:
>>>>>>>>>>>>>> - Make the bind/unbind command generic, not specific to usb device.
>>>>>>>>>>>>>> - Update the API to be able to bind/unbind based on DTS node path
>>>>>>>>>>>>>> - Add a Kconfig option to select the bind/unbind commands
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>  arch/sandbox/dts/test.dts  |  11 ++
>>>>>>>>>>>>>>  cmd/Kconfig                |   9 ++
>>>>>>>>>>>>>>  cmd/Makefile               |   1 +
>>>>>>>>>>>>>>  cmd/bind.c                 | 255 +++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>>>>>>>  configs/sandbox_defconfig  |   1 +
>>>>>>>>>>>>>>  test/py/tests/test_bind.py | 178 +++++++++++++++++++++++++++++++
>>>>>>>>>>>>>>  6 files changed, 455 insertions(+)
>>>>>>>>>>>>>>  create mode 100644 cmd/bind.c
>>>>>>>>>>>>>>  create mode 100644 test/py/tests/test_bind.py
>>>>>>>>>>>>
>>>>>>>>>>>> Reviewed-by: Simon Glass <sjg at chromium.org>
>>>>>>>>>>>>
>>>>>>>>>>>> Nice work
>>>>>>>>>>>>
>>>>>>>>>>>> [...]
>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> I have tested bind/unbind with dwc3 on zynqmp for ethernet gadget and it
>>>>>>>>>>>>> is working fine for me.
>>>>>>>>>>>>> I have also tried to use bind/unbind for gpio zynqmp driver and it is
>>>>>>>>>>>>> also working fine.
>>>>>>>>>>>>>
>>>>>>>>>>>>> It means
>>>>>>>>>>>>> Tested-by: Michal Simek <michal.simek at xilinx.com>
>>>>>>>>>>>>>
>>>>>>>>>>>>> I would prefer if these commands are called as dm bind and dm unbind
>>>>>>>>>>>>> instead of simply bind/unbind which should also fit to dm command
>>>>>>>>>>>>> description "dm - Driver model low level access".
>>>>>>>>>>>>
>>>>>>>>>>>> Yes i can see the point. But after thinking about it, maybe it is best
>>>>>>>>>>>> as it is? After all driver model is fundamental to U-Boot and it's not
>>>>>>>>>>>> clear what else we might bind/unbind.
>>>>>>>>>>>>
>>>>>>>>>>>> I'd like to get other opinions here, too.
>>>>>>>>>>>
>>>>>>>>>>> Tom/Marek: Any opinion?
>>>>>>>>>>
>>>>>>>>>> I think dm bind/unbind makes sense, yes.  "bind" and "unbind" are pretty
>>>>>>>>>> generic terms and making it clear it's part of working "inside" of DM to
>>>>>>>>>> hook/unhook things by making it a sub-command of dm sounds good.
>>>>>>>>>> Thanks!
>>>>>>>>>
>>>>>>>>> I agree with Simon here. I think bind and unbind won't have any
>>>>>>>>> plausible other meaning in U-Boot and DM is core to U-Boot
>>>>>>>>> functionality in the new world. I think it would be OK to have "dm
>>>>>>>>> bind" alias to "bind" if that's a point of confusion, but having it
>>>>>>>>> top-level seems good to me.
>>>>>>>>
>>>>>>>> They're still very generic words for something that's part of working
>>>>>>>> under the dm framework.  That said, looking at test/dm/cmd_dm.c and that
>>>>>>>> it's supposed to be only for test/debug type work, yes, OK, I'll change
>>>>>>>> my mind.
>>>>>>>
>>>>>>> I would expect that almost everybody will enable CMD_DM where symbol is
>>>>>>> in cmd/Kconfig but implementation in test/dm/cmd_dm.c (it is a question
>>>>>>> if this is the right place for this file).
>>>>>>
>>>>>> It might well really belong as cmd/dm.c, but content wise it's debug
>>>>>> information that is also useful in the bind/unbind case (so you know
>>>>>> where U-Boot sees things as being at).
>>>>>
>>>>> Then we should really not enable it by default for all boards with DM on.
>>>>>
>>>>>  640 config CMD_DM
>>>>>  641         bool "dm - Access to driver model information"
>>>>>  642         depends on DM
>>>>>  643         default y
>>>>
>>>> Simon?
>>>
>>> I'm OK with having it disabled by default - it is for debugging after all.
>>
>> Does it make sense to simply remove that line? At least I would like to
>> keep it enable for microblaze/zynq/zynqmp boards but not sure about
>> others. Or simply update every defconfig and enable it there to have
>> zero diff?
> 
> That seems like a hassle.
> 
> One option is to change each arch to 'imply CMD_DM' and then people
> can change that down the track as they want to disable it?

I have sent patches for that. Please check.

Thanks,
Michal