Hello
On Wednesday 24 of April 2013 08:53:09 Simon Glass wrote:
From: Pavel Herrmann morpheus.ibis@gmail.com
** Please note that this is very early code. I am sending out an RFC to get comments. This is not a commit message. This will only build on sandbox and all you can do it try the 'demo' command. I am most interested in comments about how to optimise the implementation to minimise impact on existing drivers, minimise code bloat and make use of existing knowledge users may have another other driver model implementations (e.g. Linux).
This patch adds a very simple drive model implementation. It is taken from the driver model code developed by:
Marek Vasut marex@denx.de Pavel Herrmann morpheus.ibis@gmail.com Viktor Křivák viktor.krivak@gmail.com Tomas Hlavacek tmshlvck@gmail.com
and possible others?
Please see doc/driver-model/README.txt for details of how to run this and what to look for.
You can find a test version of the code used here in branch dm2 at:
http://git.denx.de/u-boot-x86.git
(Branch dm contains the original implementation)
This patch requires sandbox generic board support and other enhancements.
Signed-off-by: Simon Glass sjg@chromium.org
Thanks for building on our design!
What actually happened (for those who were wondering) is that we sucesfully defended the university project part of this (which required us to have a working build, that did not necessarily adhere to what would be applicable to U-Boot upstream, so we dropped of the map for some time), and then went into "rethink and upgrade" mode of operations.
I can see that you have already implemented some ot he changes I had in mind (keeping size of private data as a part of the driver, so relocation can be done semi-automatically). I actually had a little more automation in mind (specifically the calls to core_replace, or uclass_replace in your naming scheme, and core_remove), which would mean that for a driver with a flat private data structure (which would be most of them), no relocation hook would be needed. Of course, we could arguably remove relocation as a whole, and use DM only after the relocation has taken place.
I agree with the naming change, we started wth the instances+cores, and then stuck with it (and added some more).
as for (some of) your changes, i would have a few comments:
the driver_instance wrapper around instance was created so that the driver itself doesn't have access to its status flags (currently only the activated flag) and siblings, as it should need them, and definitely should ever modify them. There was no other level of indirection, because the struct instance was embedded in struct driver_instance (eg. not a pointer).
I dont see where/if you actually do anything with the private_data_size (didnt read the whole yet), but i certianly didnt find it used for smarter relocations (see above). There is one more problem with the DM structures management, which is not entirely obvious - there is currently no way to tell whether platform_data is in static memory or not (and therefore if it should be relocated/freed. this would of course require to remember the size somewhere). this will arise when you start handling dynamically generated devices (disks, USB)
As for removal of platform data, this is not possible. You can load them from FDT for "static" devices, but it will still be required in dynamic devices (again, port number for a disk, USB descriptor number for an USB device). Also relocation/deallocation of platform data is not currently handled in DM code, which causes a slight memory leak.
driver_activate() should not be called by the user (or bootup routine) at any point, instead it should be called by the classes when/before proxying the call to the driver specified implementation. We had it in our bootup code only to show that it is possible to do selectively.
having a list_head for uclass membership in the struct device is not really a good idea, for two reasons. the biggest one is that some of the classes will need to keep some information about the devices, that is not usable outside the class (id/name for a disk, mapping to global linear numbering for GPIO...). This kind of information is used in queries "give me a device with specific properties", instead of the generic "give me Nth device of this class", The other problem is that it disallows the device to support multiple classes (this could probably be worked around by having virtual child devices for this case) - an exapmle that would use this would be a RAM disk, which you would want to still behave as a normal disk, but be tracked by a separate class (so you can resize/delete/whatever it),
I like how you cache the driver structure for each device, instead of looking it up every time. I cannot recall why we didnt use this approach, but i would point out that the name is present/used so we can have runtime loadable drivers sometime in the future (so the symbol would not resolve at link time).
I see that you are not building on the latest version from DM branch. there were a few changes in the DM mechanism there (the one I see now is the USB driver match function, which is useless as defined before we actually implemented USB).
One more thing that is not possible in current DM - sometimes, we would like to call an operation on ourselves as a part of .probe(), which would create an infinite loop in with driver_activate(). an example of this is when probing a bus controller, you probably want to scan the bus.
Thats it for now, I will spend some time over your code in near future, to provide more comments
Pavel Herrmann