[U-Boot] USB and unbinding

[Hans de Goede]

Hi,

On 22-07-15 05:48, Simon Glass wrote:
> Hi Hans,
>
> On 21 July 2015 at 13:52, Hans de Goede <hdegoede at redhat.com> wrote:
>> Hi,
>>
>> On 07/20/2015 05:49 PM, Simon Glass wrote:
>>>
>>> Hi Hans,
>>>
>>>
>>> On 20 July 2015 at 09:31, Hans de Goede <hdegoede at redhat.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 20-07-15 04:23, Simon Glass wrote:
>>>>>
>>>>>
>>>>> Hi Hans,
>>>>>
>>>>> I've been thinking about the USB unbinding code. I know that I agreed
>>>>> to go with it, but in retrospect I think that was a mistake.
>>>>>
>>>>> I believe we should separate out the unbinding and make it an option,
>>>>> so that it is not required in order to use USB. In effect this makes
>>>>> one of driver model's design goals (the option to drop unused code)
>>>>> useless since USB is a common interface.
>>>>>
>>>>> If I recall the only problem the lack of unbinding caused was that the
>>>>> keyboard driver broke. I suspect it broke in a way that can be fixed.
>>>>> In fact I recently converted usb_ether to driver model and I'm willing
>>>>> to do the keyboard side also.
>>>>>
>>>>> I'd like the USB code to function with or without the unbinding (i.e.
>>>>> it uses it if there). What do you think?
>>>>
>>>>
>>>>
>>>> I strongly believe that unbinding is the proper thing todo for usb since
>>>> it is a hotplug bus.
>>>>
>>>> IMHO the way the usb_find_emul_child() function was used before to re-use
>>>> udevice-s after e.g. a "usb reset" was an ugly hack which just happened
>>>> to
>>>> work, but it in no way reflects reality.
>>>>
>>>> More importantly we need unbind support to properly stop usb controllers
>>>> when
>>>> booting the OS, so that they are not DMA-ing to/from their scratch-ram
>>>> area
>>>> in DRAM when the main OS boots, so not having unbind support combined
>>>> with
>>>> USB really is a no no.
>>>>
>>>> This is why I suggested to simply select the unbind Kconfig when USB is
>>>> selected in Kconfig.
>>>
>>>
>>> I think you are referring to remove(), not unbind().
>>
>>
>> Right I mean that the remove callback *must* be called on usb_stop to avoid
>> the usb controller dma-ing over random DRAM when the OS starts.
>
> OK.
>
>>
>>> Although we might
>>> consider spiting them so we have a DM_DEVICE_REMOVE and a separate
>>> DM_DEVICE_UNBIND.
>>>
>>>>
>>>> The actual unbind core code is not that big, so I believe that the best
>>>> solution is to always build the core if either DM_DEVICE_REMOVE *or*
>>>> DM_USB is selected, and non USB drivers can leave out their unbind
>>>> code if DM_DEVICE_REMOVE is not set, that should still give us most of
>>>> the size savings without needing to do ugly hacks for USB.
>>>
>>>
>>> My main objection is that we tie USB such that it *will not work*
>>> unless we support unbinding. I'm fine with it being recommended, but
>>> core driver model features should be independent of subsystems. This
>>> also seems quite unnecessary. Re your common about the 'ugly hack that
>>> just happened to work', in principle we can just keep on creating new
>>> devices and ignore the old ones.
>>
>>
>> That will still cause problems with code addressing the usb-devices
>> by index, as the old devices will still be there.
>>
>>> That's the idea behind not supporting
>>> unbinding. There should be no problem with this approach.
>>
>>
>> This approach will only work if find_child_devnum is fixed to search
>> backwards through the childs list, so that it will check the newly
>> added nodes first.
>
> Or that it just ignores the nodes that aren't active. Anyway that
> function is a hang-over from the old code. It makes no sense to
> enumerate the devices when you can just look up the data and find
> them.

Right, walking over the tree is still necessary for the "usb tree"
command though.

> I think it can be made to work for now, but perhaps we should
> port the keyboard drivers to DM?

I agree that atleast the usb-keyb. driver should be ported to use DM
style binding like the usb-storage driver.

I've been thinking a bit about this, currently the driver will only
bind to the first available usb hid intf with a boot - keyb subclass.
If we move to DM binding we should support multiple keyboards, but
the current stdio.c code deals poorly with this, so I think that
it would be best to keep a list of usb-keyb-devices in common/usb_kbd.c
and register a stdio device for this when the first usb-keyb-device
gets registered (so the list is empty) and unregister it when the last
one gets removed.

All usb-keyboards would then feed keypresses into a single FIFO
(usb_kbd_buffer + usb_[in|out]_pointer in the current code), from
which the single stdio device would feed.

Note that the unregistering bit requires DM_DEVICE_REMOVE, we can do
without this though and just keep the stdio-device around with an
always empty keyevent fifo.

>>> So I'd like to adjust the USB code so that it still works without
>>> unbinding, even if it is not optimal. I think that is the right thing
>>> to do in this case.
>>
>>
>> As said, the remove callback of usb-host drivers *must* always be called,
>> other then that if you can make things work without unbind that is
>> fine with me.
>
> OK thanks, will give it a crack.

Regards,

Hans