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

[Simon Glass]

Hi Peter,

On Wed, Apr 24, 2013 at 9:51 AM, Pavel Herrmann <morpheus.ibis at gmail.com> wrote:
> 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!

Thanks for the comments - it has taken me a bit of time to digest...

>
> 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'm pleased it all worked out well.

>
> 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.

Yes I am beginning to think that we should drop relocation for now,
and add it back when we find a problem that it solves.

>
> I agree with the naming change, we started wth the instances+cores, and then
> stuck with it (and added some more).

OK good.

>
> 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).

OK I see. I don't think there is any problem with storing flags or the
sibling list node info in the driver data. In fact the whole struct
device can be considered to be private to the driver model, except the
private data.

I wonder if we should allow drivers to embed struct device in their
own structure as Linux does? Then it might avoid private data
altogether in the common case. Perhaps driver model could be given a
size value, which is the size of the entire structure including struct
device, and it could automatically allocate this for you?

>
> 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).

It is allocated for the driver automatically before probe() is
callled, and freed after remove() returns.

> 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)

I suppose we can check the address range? Or perhaps add a flag to
indicate whether the device was created pre-relocation.

>
> 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.

Here I was referring to the need to declare platform_data static data
in your board file. With FDT it should not be needed, since the driver
can convert the FDT node into platform_data for you, and then use
that.

For dynamic devices, they probably don't have platform_data anyway,
but if they do, then I think the FDT can still be used.

It may be true that eliminating platform_data is a step too far in
some cases, but I think it is possible.

>
> 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.

Yes I need to tidy that up.

>
> 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),

To take your second point first, I think that devices which implement
multiple uclasses should be considered multi-function-devices (MFD).
These then to sit in a uclass of their own. Then, they can allocate
separate individual devices for each uclass. This seems cleaner than
dealing with a device in multiple uclasses. It also mirrors how this
is done in Linux.

Re the uclass information, that is true. We need a way for the uclass
code to store information about each device in a uclass. I will have a
think about how to do that, perhaps another private data pointer?

>
> 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).
>

OK, there are a few things like that which could do with work.

> 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).

OK - I did compare the code against the final commit in that branch.
There certainly were changes, but not many to the core code, so I
decided it did not matter for now.

>
> 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.

What sort of operation do you mean here? I would probably hold that
when a driver is being probed, it can't use driver model calls to
itself from within its probe function. If it needs to, it can call its
own functions directly from probe. For example, if it wants to read
from the device, ti can call its local read() function instead of
trying to call the driver model 'read' handler for itself. The effect
should be the same but it will be much less confusing.

But perhaps I have missed something here.

>
> Thats it for now, I will spend some time over your code in near future, to
> provide more comments

Thanks again for your comments and insight. I am hoping to find the
time to respin this again soon.

Regards,
Simon

>
> Pavel Herrmann