[U-Boot] [RFC PATCH] WIP: Simplified device model implementation and demo

[Pavel Herrmann]

Hello

On Wednesday 24 of April 2013 08:53:09 Simon Glass wrote:
> From: Pavel Herrmann <morpheus.ibis at 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 at denx.de>
>    Pavel Herrmann <morpheus.ibis at gmail.com>
>    Viktor Křivák <viktor.krivak at gmail.com>
>    Tomas Hlavacek <tmshlvck at 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 at 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