Hi Hans, On 21 July 2015 at 13:52, Hans de Goede <hdego...@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 <hdego...@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. I think it can be made to work for now, but perhaps we should port the keyboard drivers to DM? > >> 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, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
