[U-Boot] [RFC PATCH v2 2/6] fdt: Add support for embedded device tree (CONFIG_OF_EMBED)

[Simon Glass]

Hi Jason,

On Wed, Sep 14, 2011 at 1:16 PM, Jason <u-boot at lakedaemon.net> wrote:
> On Wed, Sep 14, 2011 at 01:05:58PM -0700, Simon Glass wrote:
>> On Wed, Sep 14, 2011 at 12:50 PM, Jason <u-boot at lakedaemon.net> wrote:
>> > On Wed, Sep 14, 2011 at 10:45:59AM -0700, Simon Glass wrote:
>>
>> [snip]
>> >> > 5.) run, then 'date' fails like so:
>> >> >
>> >> > find_alias_node: rtc0
>> >> > fdt_decode_next_alias failed.
>> >> > Error decoding fdt for mvrtc.
>> >> > ## Get date failed
>> >>
>> >> I don't actually see an alias in your fdt. And actually I left it out
>> >> of mine, so that is understandable. For i2c I have:
>> >>
>> >> ...
>> >>       aliases {
>> >>               i2c0 = "/i2c at 0x7000d000";
>> >>               i2c1 = "/i2c at 0x7000c000";
>> >>               i2c2 = "/i2c at 0x7000c400";
>> >>               i2c3 = "/i2c at 0x7000c500";
>> >>       };
>> >
>> > That worked!
>> >
>> > Marvell>> date
>> > find_alias_node: rtc0
>> > Date: 2011-09-14 (Wednesday)    Time: 14:04:54
>> > Marvell>>
>> >
>>
>> Great!
>>
>> >> So I think you need to add a /alias node and try again. I can submit a
>> >> new patch set with this and a couple of other things I want to change,
>> >> but it would be good if you can get to the end first, in case you find
>> >> other problems.
>> >
>> > I'll clean up what I have and post it RFC.
>>
>> OK good
>>
>> >> > I had the remove your fdt_decode_i2c() and clock.h include.  The clock.h
>> >> > include seems to be specific to the tegra2 and doesn't exist for
>> >> > kirkwood.
>> >>
>> >> Yes that's right, it is just an example at this stage, and the idea of
>> >> a periph_id is specific to Tegra at present. Patches 5 and 6 are just
>> >> an example to show how to use it in code.
>> >
>> > Ok, I'll drop those from my branch to make a cleaner example.
>>
>> Yes, ideally I would like to keep SOC-specific things out of it at
>> this stage, but I was asked for an example and had to choose
>> something! My hope is that we can have the base patch and then a
>> couple of architecture patches.
>
> Yes, I don't like putting fdt_decode_i2c() or fdt_decode_rtc() in
> common/fdt_decode.c ...  The current implementation of fdt...i2c() is
> arch specific, and fdt...rtc() I know will only work for the simple
> integrated rtc with two registers.

This is one of the things to resolve. I think we need an fdt_decode to
lighten the load of finding aliases, decoding addresses and the like,
but a bigger question is whether we want the various i2c drivers to
share decode logic. It will help make the drivers more similar, but
clearly means that they have to follow the lowest common denominator.
This is a bit like CONFIG_ works at present - and we are replacing the
CONFIG_ items. For i2c the configs might be CONFIG_SYS_I2C_SPEED and
the controller address (and maybe CONFIG_SYS_I2C_SLAVE). But we don't
deal with particular i2c config like pinmux settings etc. which are
not common across a lot of boards. So fdt_decode would deal with these
few common settings and leave specific drivers to do the rest.

But if people are happy with the idea of fdt decode code bleeding more
into each driver, then fdt_decode just becomes a low-level helper
library, and does not have specific functions for decoding an i2c
node, a uart, etc. That is perhaps more pure - my main concerns with
this are uptake (too hard to swtich a board over to fdt) and
consistency (everyone will use their own way of doing the same thing -
and we have enough of that in U-Boot already :-)

Regards,
Simon

>
> Let me rework it to the appropriate place and then I'll post.  I'll also
> remove the fdt...i2c().
>
> thx,
>
> Jason.
>