[U-Boot-Users] Imminent u-boot-fdt pull request

[U-Boot-Users] Imminent u-boot-fdt pull request

[Jerry Van Baren]

Dear all,

Please review the u-boot-fdt changes.  I intend to request Wolfgang pull 
the pending changes soon.  All of the changes (except the last trivial 
patch "For fdt_find_node_by_path(), handle the root path properly.") 
have been previously published to the list.

The following changes are pending:

libfdt: Conditionally compile based on CONFIG_OF_LIBFDT
<http://www.denx.de/cgi-bin/gitweb.cgi?p=u-boot/u-boot-fdt.git;a=commit;h=6b495962ba7b2b43904a87265e182faed033582c>

Improve error messages, more informative.
<http://www.denx.de/cgi-bin/gitweb.cgi?p=u-boot/u-boot-fdt.git;a=commit;h=5a4c80ceaf8ff29a169b9a76065a7023fbfda5b9>

Minor code clean up.
<http://www.denx.de/cgi-bin/gitweb.cgi?p=u-boot/u-boot-fdt.git;a=commit;h=1959d478d18434ae12d725b4ace1bc86e63b5ad9>

Improve fdt move length handling.
<http://www.denx.de/cgi-bin/gitweb.cgi?p=u-boot/u-boot-fdt.git;a=commit;h=4b034226a31a78f2fd619ae09c0e34c7c04348c3>

Fix bugs in the CONFIG_OF_LIBFDT
<http://www.denx.de/cgi-bin/gitweb.cgi?p=u-boot/u-boot-fdt.git;a=commit;h=3c3652ec50a13711c844222389cd207d52517a00>

Reorganize and fix problems (returns) in the bootm command.
<http://www.denx.de/cgi-bin/gitweb.cgi?p=u-boot/u-boot-fdt.git;a=commit;h=7450dcd7c3040f7d523a4153f90ca649527f804e>

FDT command improvements.
<http://www.denx.de/cgi-bin/gitweb.cgi?p=u-boot/u-boot-fdt.git;a=commit;h=f2d8680ac9760072e5ca9e0f021c757aa3a67535>

Fix cmd_fdt line lengths, refactor code.
<http://www.denx.de/cgi-bin/gitweb.cgi?p=u-boot/u-boot-fdt.git;a=commit;h=792f0989bf90961ba2644ccc718cf09d338706b8>

Replace fdt_node_offset() with fdt_find_node_by_path().
<http://www.denx.de/cgi-bin/gitweb.cgi?p=u-boot/u-boot-fdt.git;a=commit;h=5e136a1a853f7c9567e428320422da6b2b91c640>

Add fdt_find_node_by_type() and fdt_find_compatible_node() to LIBFDT
<http://www.denx.de/cgi-bin/gitweb.cgi?p=u-boot/u-boot-fdt.git;a=commit;h=efe49ab24e87857a0bd2961c8269b5006e3af93a>

For fdt_find_node_by_path(), handle the root path properly.
<http://www.denx.de/cgi-bin/gitweb.cgi?p=u-boot/u-boot-fdt.git;a=commit;h=cd87336beaca12aa8cb6a72bf19d4c5d0f9cfd3b>

Best regards,
gvb

[U-Boot-Users] Imminent u-boot-fdt pull request

[Kim Phillips]

On Tue, 22 May 2007 00:06:06 -0400
Jerry Van Baren <gvb.uboot@gmail.com> wrote:

> Dear all,
> 
> Please review the u-boot-fdt changes.  I intend to request Wolfgang pull 
> the pending changes soon.  All of the changes (except the last trivial 

I have properties being placed at the wrong level, timebase- and
bus-frequencies not being updated, there are multiple copies of the
code, and, it is hard to read - I saw a line length of 139!

Please work on your CodingStyle.  I haven't finished debugging here.

Kim

[U-Boot-Users] Imminent u-boot-fdt pull request

[Wolfgang Grandegger]

Kim Phillips wrote:
> On Tue, 22 May 2007 00:06:06 -0400
> Jerry Van Baren <gvb.uboot@gmail.com> wrote:
> 
>> Dear all,
>>
>> Please review the u-boot-fdt changes.  I intend to request Wolfgang pull 
>> the pending changes soon.  All of the changes (except the last trivial 
> 
> I have properties being placed at the wrong level, timebase- and
> bus-frequencies not being updated, there are multiple copies of the
> code, and, it is hard to read - I saw a line length of 139!

Could you be more specific, please? Preferably by adding comments to the 
patch. And are these problems related to the patch?

Thanks,

Wolfgang.

[U-Boot-Users] Imminent u-boot-fdt pull request

[Kim Phillips]

On Fri, 25 May 2007 11:27:47 +0200
Wolfgang Grandegger <wg@grandegger.com> wrote:

> Kim Phillips wrote:
> > On Tue, 22 May 2007 00:06:06 -0400
> > Jerry Van Baren <gvb.uboot@gmail.com> wrote:
> > 
> >> Dear all,
> >>
> >> Please review the u-boot-fdt changes.  I intend to request Wolfgang pull 
> >> the pending changes soon.  All of the changes (except the last trivial 
> > 
> > I have properties being placed at the wrong level, timebase- and
> > bus-frequencies not being updated, there are multiple copies of the
> > code, and, it is hard to read - I saw a line length of 139!
> 
> Could you be more specific, please? Preferably by adding comments to the 

sorry, my comments were not specific, it's because the nature of the
problem as I see it is general, and I decided to send a quick note to
save Wolfgang from them ;)

> patch. And are these problems related to the patch?
> 

This is current top-of-fdt-tree behaviour:

=> fdt addr $fdtaddr
=> fdt chosen
=> fdt print /cpus
cpus {
        clock-frequency=<1f78a400>;            // should be one level down
        #address-cells=<00000001>;
        #size-cells=<00000000>;
        PowerPC,8360 at 0 {
                device_type="cpu";
                reg=<00000000>;
                d-cache-line-size=<00000020>;
                i-cache-line-size=<00000020>;
                d-cache-size=<00008000>;
                i-cache-size=<00008000>;
                timebase-frequency=<00000000>;  // not updated
                bus-frequency=<00000000>;       // not updated
                clock-frequency=<00000000>;     // not updated, see above
                32-bit;
        };
};

I wouldn't call this quality material.

I'll post (specific) patches once I get a kernel booting, unless someone
beats me to it.

Kim

[U-Boot-Users] Imminent u-boot-fdt pull request

[Jerry Van Baren]

Kim Phillips wrote:
> On Fri, 25 May 2007 11:27:47 +0200
> Wolfgang Grandegger <wg@grandegger.com> wrote:
> 
>> Kim Phillips wrote:
>>> On Tue, 22 May 2007 00:06:06 -0400
>>> Jerry Van Baren <gvb.uboot@gmail.com> wrote:
>>>
>>>> Dear all,
>>>>
>>>> Please review the u-boot-fdt changes.  I intend to request Wolfgang pull 
>>>> the pending changes soon.  All of the changes (except the last trivial 
>>> I have properties being placed at the wrong level, timebase- and
>>> bus-frequencies not being updated, there are multiple copies of the
>>> code, and, it is hard to read - I saw a line length of 139!
>> Could you be more specific, please? Preferably by adding comments to the 
> 
> sorry, my comments were not specific, it's because the nature of the
> problem as I see it is general, and I decided to send a quick note to
> save Wolfgang from them ;)
> 
>> patch. And are these problems related to the patch?
>>
> 
> This is current top-of-fdt-tree behaviour:
> 
> => fdt addr $fdtaddr
> => fdt chosen
> => fdt print /cpus
> cpus {
>         clock-frequency=<1f78a400>;            // should be one level down
>         #address-cells=<00000001>;
>         #size-cells=<00000000>;
>         PowerPC,8360 at 0 {
>                 device_type="cpu";
>                 reg=<00000000>;
>                 d-cache-line-size=<00000020>;
>                 i-cache-line-size=<00000020>;
>                 d-cache-size=<00008000>;
>                 i-cache-size=<00008000>;
>                 timebase-frequency=<00000000>;  // not updated
>                 bus-frequency=<00000000>;       // not updated
>                 clock-frequency=<00000000>;     // not updated, see above
>                 32-bit;
>         };
> };
> 
> I wouldn't call this quality material.
> 
> I'll post (specific) patches once I get a kernel booting, unless someone
> beats me to it.
> 
> Kim

Hi Kim,

FWIIW, that is in the (infamous) common/fdt_support.c.  It was adapted 
from ft_build.c.  It looks like I lost a OF_CPU in the constructed 
string when I was adapting.

Original code:
         p = ft_get_prop(blob, "/cpus/" OF_CPU "/clock-frequency", &len);

Comments:
1) It looks like you are missing the OF_CPU definition, or I'm not 
pulling in the right .h file

2) Some of the code is in fdt_support.c fdt_chosen().  The stuff messing 
with CPU-specific values does NOT belong in fdt_chosen(), it should go 
in cpu/mpc83xx/cpu.c

3) /cpu/PowerPC,8360 at 0/bus-frequency is properly set in 
cpu/mpc83xx/cpu.c ft_cpu_setup().  The table of nodes and properties 
correctly has
     "/cpus/" OF_CPU,
     "bus-frequency",
which seems to indicate that I don't have the right header pulled in to 
define OF_CPU.  The defines from point #2

Screwed up code in fdt_chosen()

#ifdef OF_TBCLK
         nodeoffset = fdt_find_node_by_path (fdt, "/cpus/" OF_CPU 
"/timebase-frequency");
         if (nodeoffset >= 0) {
                 clock = cpu_to_be32(OF_TBCLK);
                 err = fdt_setprop(fdt, nodeoffset, "clock-frequency", 
&clock, 4);


The code in question is in

[U-Boot-Users] Imminent u-boot-fdt pull request

[Kim Phillips]

On Fri, 25 May 2007 12:56:13 -0400
Jerry Van Baren <gerald.vanbaren@smiths-aerospace.com> wrote:

> FWIIW, that is in the (infamous) common/fdt_support.c.  It was adapted 
> from ft_build.c.  It looks like I lost a OF_CPU in the constructed 
> string when I was adapting.
> 
> Original code:
>          p = ft_get_prop(blob, "/cpus/" OF_CPU "/clock-frequency", &len);
> 
> Comments:
> 1) It looks like you are missing the OF_CPU definition, or I'm not 
> pulling in the right .h file
> 

I didn't do anything; as I said before, this is your u-boot-fdt.git
top-of-tree.

> 2) Some of the code is in fdt_support.c fdt_chosen().  The stuff messing 
> with CPU-specific values does NOT belong in fdt_chosen(), it should go 
> in cpu/mpc83xx/cpu.c

not sure about that; why replicate code in the cpu/*/cpu.c files? 
Can't fdt_chosen() update a property only if it exists?  i.e., the cpu
dependency would be implied by the device tree, the best place for it
IMO.

> 3) /cpu/PowerPC,8360 at 0/bus-frequency is properly set in 
> cpu/mpc83xx/cpu.c ft_cpu_setup().  The table of nodes and properties 
> correctly has
>      "/cpus/" OF_CPU,
>      "bus-frequency",
> which seems to indicate that I don't have the right header pulled in to 
> define OF_CPU.  The defines from point #2
> 

precisely.

> Screwed up code in fdt_chosen()
> 
> #ifdef OF_TBCLK
>          nodeoffset = fdt_find_node_by_path (fdt, "/cpus/" OF_CPU 
> "/timebase-frequency");
>          if (nodeoffset >= 0) {
>                  clock = cpu_to_be32(OF_TBCLK);
>                  err = fdt_setprop(fdt, nodeoffset, "clock-frequency", 
> &clock, 4);
> 

noticed that sloppiness.

> 
> The code in question is in

that is the question.

please do not ask Wolfgang to pull patches to files in mpc83xx and
related directories (cpu.c, board configs, etc.) from your tree; they
should go through the mpc83xx tree from now on.

Kim

[U-Boot-Users] Imminent u-boot-fdt pull request

[Jerry Van Baren]

Kim Phillips wrote:
> On Fri, 25 May 2007 12:56:13 -0400
> Jerry Van Baren <gerald.vanbaren@smiths-aerospace.com> wrote:

[snip]

> please do not ask Wolfgang to pull patches to files in mpc83xx and
> related directories (cpu.c, board configs, etc.) from your tree; they
> should go through the mpc83xx tree from now on.
> 
> Kim

In retrospect, that was impolite and I apologize for the crossover. :-(

The 83xx changes were necessary to make the bootm using libfdt work (I 
have a 8360 board).  If I don't push the necessary changes through 
u-boot-fdt, u-boot-fdt will be impossible to use until you add the 
necessary changes in the 83xx repo (and possibly vice versa).  In 
addition, Wolfgang would have to coordinate his pulls into the main repo 
or it would break.

The cross coupling is unfortunate, but we have to have one example of a 
CPU/board and libfdt coupled in order to test the libfdt changes.

I would prefer to fix the problems noted (which really are not as bad as 
they look IMHO) to make one CPU+board+libfdt example work and push those 
changes to Wolfgang via the u-boot-fdt repo.  If you are opposed to 
this, I can back out the 83xx changes and push just libfdt stuff to 
Wolfgang and supply you with the set of 83xx patches which you can push 
through your repo, but it will tough for others to use/test since they 
will get only half a picture from each repo.

Best regards,
gvb

[U-Boot-Users] Imminent u-boot-fdt pull request

[Kim Phillips]

On Fri, 25 May 2007 13:33:49 -0400
Jerry Van Baren <gerald.vanbaren@smiths-aerospace.com> wrote:

> Kim Phillips wrote:
> > On Fri, 25 May 2007 12:56:13 -0400
> > Jerry Van Baren <gerald.vanbaren@smiths-aerospace.com> wrote:
> 
> [snip]
> 
> > please do not ask Wolfgang to pull patches to files in mpc83xx and
> > related directories (cpu.c, board configs, etc.) from your tree; they
> > should go through the mpc83xx tree from now on.
> > 
> > Kim
> 
> In retrospect, that was impolite and I apologize for the crossover. :-(
> 
> The 83xx changes were necessary to make the bootm using libfdt work (I 
> have a 8360 board).  If I don't push the necessary changes through 
> u-boot-fdt, u-boot-fdt will be impossible to use until you add the 
> necessary changes in the 83xx repo (and possibly vice versa).  In 
> addition, Wolfgang would have to coordinate his pulls into the main repo 
> or it would break.
> 
> The cross coupling is unfortunate, but we have to have one example of a 
> CPU/board and libfdt coupled in order to test the libfdt changes.
> 
> I would prefer to fix the problems noted (which really are not as bad as 
> they look IMHO) to make one CPU+board+libfdt example work and push those 
> changes to Wolfgang via the u-boot-fdt repo.  If you are opposed to 
> this, I can back out the 83xx changes and push just libfdt stuff to 
> Wolfgang and supply you with the set of 83xx patches which you can push 
> through your repo, but it will tough for others to use/test since they 
> will get only half a picture from each repo.
> 

nothing would have broken if LIBFDT were not turned on by default.

the only reason nothing seemed broken in the first place was the
coincidence that the board you developed on is one of a handful of
boards with valid (non-zero) default -frequency values in its dts (in
Linux).

anyone can just as easily pull the mpc83xx git tree on top of the fdt
tree, or vice-versa, as clone Wolfgang's stable or testing tree.
Wolfgang doesn't ever have to do anything except respond to our pull
requests.  You and I can coordinate pull requests if you think it's
necessary (I don't think it will be in the long term).

I don't see any reason why any mpc83xx specific patches should bypass
the mpc83xx tree, and any libfdt specific patches bypass the fdt tree,
for that matter. If you want, you can put your mpc83xx specific patches
on a mpc83xx branch on your tree, and ask me to pull from your tree,
after having posted the patches on u-boot-users, if you think it's more
convenient.

Let's concentrate on getting things fixed; I'm not going to post a patch
to turn off LIBFDT on the MPC8360EMDS unless Wolfgang informs me of an
imminent release or something.  In fact, the reason this came about was
because I'm adding a new board, and I thought it be good if it used
libfdt.  I like libfdt, but it needs to work, be readable, and be easy
to use.

btw, I'll try to be more prompt with my testing in the future; please
be more thorough with yours.

Thanks,

Kim

[U-Boot-Users] Imminent u-boot-fdt pull request

[Jerry Van Baren]

Kim Phillips wrote:
> On Fri, 25 May 2007 13:33:49 -0400
> Jerry Van Baren <gerald.vanbaren@smiths-aerospace.com> wrote:
> 
>> Kim Phillips wrote:
>>> On Fri, 25 May 2007 12:56:13 -0400
>>> Jerry Van Baren <gerald.vanbaren@smiths-aerospace.com> wrote:
>> [snip]
>>
>>> please do not ask Wolfgang to pull patches to files in mpc83xx and
>>> related directories (cpu.c, board configs, etc.) from your tree; they
>>> should go through the mpc83xx tree from now on.
>>>
>>> Kim
>> In retrospect, that was impolite and I apologize for the crossover. :-(
>>
>> The 83xx changes were necessary to make the bootm using libfdt work (I 
>> have a 8360 board).  If I don't push the necessary changes through 
>> u-boot-fdt, u-boot-fdt will be impossible to use until you add the 
>> necessary changes in the 83xx repo (and possibly vice versa).  In 
>> addition, Wolfgang would have to coordinate his pulls into the main repo 
>> or it would break.
>>
>> The cross coupling is unfortunate, but we have to have one example of a 
>> CPU/board and libfdt coupled in order to test the libfdt changes.
>>
>> I would prefer to fix the problems noted (which really are not as bad as 
>> they look IMHO) to make one CPU+board+libfdt example work and push those 
>> changes to Wolfgang via the u-boot-fdt repo.  If you are opposed to 
>> this, I can back out the 83xx changes and push just libfdt stuff to 
>> Wolfgang and supply you with the set of 83xx patches which you can push 
>> through your repo, but it will tough for others to use/test since they 
>> will get only half a picture from each repo.
>>
> 
> nothing would have broken if LIBFDT were not turned on by default.

Yup, I screwed that up in my first pull request to Wolfgang. :-/

> the only reason nothing seemed broken in the first place was the
> coincidence that the board you developed on is one of a handful of
> boards with valid (non-zero) default -frequency values in its dts (in
> Linux).
> 
> anyone can just as easily pull the mpc83xx git tree on top of the fdt
> tree, or vice-versa, as clone Wolfgang's stable or testing tree.
> Wolfgang doesn't ever have to do anything except respond to our pull
> requests.  You and I can coordinate pull requests if you think it's
> necessary (I don't think it will be in the long term).

I agree with this in the long term.  I screwed up enabling LIBFDT by 
default and got carried away making bootm work... but those are excuses.

> I don't see any reason why any mpc83xx specific patches should bypass
> the mpc83xx tree, and any libfdt specific patches bypass the fdt tree,
> for that matter. If you want, you can put your mpc83xx specific patches
> on a mpc83xx branch on your tree, and ask me to pull from your tree,
> after having posted the patches on u-boot-users, if you think it's more
> convenient.
 >
> Let's concentrate on getting things fixed; I'm not going to post a patch
> to turn off LIBFDT on the MPC8360EMDS unless Wolfgang informs me of an
> imminent release or something.  In fact, the reason this came about was
> because I'm adding a new board, and I thought it be good if it used
> libfdt.  I like libfdt, but it needs to work, be readable, and be easy
> to use.

Just quibbling, I consider libfdt/* and cmd_fdt.c to be good quality. 
The cmd_bootm.c and associated code (fdt_chosen(), fdt_env(), 
fdt_bd_t(), ft_cpu_setup(), and ft_pci_setup()) is what is rough.

Part of that is my inattention to detail (hacking without understanding 
the code first), part of that is my lack of "big picture" understanding 
of what the blob contents /should/ look like, and part of that is my 
small sample size (one) that just happens to work. :-(

> btw, I'll try to be more prompt with my testing in the future; please
> be more thorough with yours.

Fair enough.

> Thanks,
> Kim

Thanks in turn,
gvb

[U-Boot-Users] Imminent u-boot-fdt pull request

[Scott Wood]

Jerry Van Baren wrote:
> FWIIW, that is in the (infamous) common/fdt_support.c.  It was adapted 
> from ft_build.c.  It looks like I lost a OF_CPU in the constructed 
> string when I was adapting.
> 
> Original code:
>          p = ft_get_prop(blob, "/cpus/" OF_CPU "/clock-frequency", &len);
> 
> Comments:
> 1) It looks like you are missing the OF_CPU definition, or I'm not 
> pulling in the right .h file

What about doing a search for device_type = "cpu", rather than looking 
for a specific CPU name?  The Linux bootwrapper does it this way.  It 
also has the benefit of working when there's more than one CPU.

-Scott

[U-Boot-Users] HUSH local variables visibility in u-boot code.

[Leonid]

"Global" u-boot variables, defined by setenv commands may be seen from
code using getenv() function and may be also set from code via setenv()
function.

Do similar functions exist for HUSH local variables?

Thanks,

Leonid.

[U-Boot-Users] HUSH local variables visibility in u-boot code.

[Wolfgang Denk]

In message <406A31B117F2734987636D6CCC93EE3C01887495@ehost011-3.exch011.intermedia.net> you wrote:
> "Global" u-boot variables, defined by setenv commands may be seen from
> code using getenv() function and may be also set from code via setenv()
> function.
> 
> Do similar functions exist for HUSH local variables?

Only in the scope of the hush code itself.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
If the facts don't fit the theory, change the facts.
                                                   -- Albert Einstein

[U-Boot-Users] Imminent u-boot-fdt pull request

[Jerry Van Baren]

Scott Wood wrote:
> Jerry Van Baren wrote:
>> FWIIW, that is in the (infamous) common/fdt_support.c.  It was adapted 
>> from ft_build.c.  It looks like I lost a OF_CPU in the constructed 
>> string when I was adapting.
>>
>> Original code:
>>          p = ft_get_prop(blob, "/cpus/" OF_CPU "/clock-frequency", &len);
>>
>> Comments:
>> 1) It looks like you are missing the OF_CPU definition, or I'm not 
>> pulling in the right .h file
> 
> What about doing a search for device_type = "cpu", rather than looking 
> for a specific CPU name?  The Linux bootwrapper does it this way.  It 
> also has the benefit of working when there's more than one CPU.
> 
> -Scott

I deeply regret the confusion I caused by pushing the "Send" when I 
meant to push "Write" to start a new, unrelated, message.  My updated 
re-send, as you probably have figured out by now, is more coherent.

OF_CPU is defined in the board-specific header file, e.g. 
configs/MPC8360EMDS.h, pulled in by config.h.  Since I am missing the 
#include <config.h> in the two files being discussed, OF_CPU is 
undefined and the string concatenation trick makes it silently disappear.

Best regards,
gvb

[U-Boot-Users] Imminent u-boot-fdt pull request

[Scott Wood]

Jerry Van Baren wrote:
>> What about doing a search for device_type = "cpu", rather than looking 
>> for a specific CPU name?  The Linux bootwrapper does it this way.  It 
>> also has the benefit of working when there's more than one CPU.
>>
>> -Scott
> 
> 
> I deeply regret the confusion I caused by pushing the "Send" when I 
> meant to push "Write" to start a new, unrelated, message.  My updated 
> re-send, as you probably have figured out by now, is more coherent.
> 
> OF_CPU is defined in the board-specific header file, e.g. 
> configs/MPC8360EMDS.h, pulled in by config.h.  Since I am missing the 
> #include <config.h> in the two files being discussed, OF_CPU is 
> undefined and the string concatenation trick makes it silently disappear.

Sure... I was just suggesting a more robust method, that doesn't rely on 
the board config file to define the CPU name.

-Scott

[U-Boot-Users] Imminent u-boot-fdt pull request

[Jerry Van Baren]

Scott Wood wrote:
> Jerry Van Baren wrote:
>>> What about doing a search for device_type = "cpu", rather than 
>>> looking for a specific CPU name?  The Linux bootwrapper does it this 
>>> way.  It also has the benefit of working when there's more than one CPU.
>>>
>>> -Scott
>>
>>
>> I deeply regret the confusion I caused by pushing the "Send" when I 
>> meant to push "Write" to start a new, unrelated, message.  My updated 
>> re-send, as you probably have figured out by now, is more coherent.
>>
>> OF_CPU is defined in the board-specific header file, e.g. 
>> configs/MPC8360EMDS.h, pulled in by config.h.  Since I am missing the 
>> #include <config.h> in the two files being discussed, OF_CPU is 
>> undefined and the string concatenation trick makes it silently disappear.
> 
> Sure... I was just suggesting a more robust method, that doesn't rely on 
> the board config file to define the CPU name.
> 
> -Scott

Ah, now I follow.  That is worth considering.  That would make some 
assumptions too, just different ones.

Of the top of my head...
* All CPUs are clocked the same (unlikely to be a problem)
* That the CPU actually _needs_ the fixups (currently they all get the 
fixup so it isn't currently a problem)

As long as it works for the bootwrapper, it would work for u-boot.  The 
two are imperfect subsets of each other, however.

Thanks,
gvb

[U-Boot-Users] Imminent u-boot-fdt pull request

[Jerry Van Baren]

Kim Phillips wrote:
> On Fri, 25 May 2007 11:27:47 +0200
> Wolfgang Grandegger <wg@grandegger.com> wrote:
> 
>> Kim Phillips wrote:
>>> On Tue, 22 May 2007 00:06:06 -0400
>>> Jerry Van Baren <gvb.uboot@gmail.com> wrote:
>>>
>>>> Dear all,
>>>>
>>>> Please review the u-boot-fdt changes.  I intend to request Wolfgang pull 
>>>> the pending changes soon.  All of the changes (except the last trivial 
>>> I have properties being placed at the wrong level, timebase- and
>>> bus-frequencies not being updated, there are multiple copies of the
>>> code, and, it is hard to read - I saw a line length of 139!
>> Could you be more specific, please? Preferably by adding comments to the 
> 
> sorry, my comments were not specific, it's because the nature of the
> problem as I see it is general, and I decided to send a quick note to
> save Wolfgang from them ;)
> 
>> patch. And are these problems related to the patch?
>>
> 
> This is current top-of-fdt-tree behaviour:
> 
> => fdt addr $fdtaddr
> => fdt chosen
> => fdt print /cpus
> cpus {
>         clock-frequency=<1f78a400>;            // should be one level down
>         #address-cells=<00000001>;
>         #size-cells=<00000000>;
>         PowerPC,8360 at 0 {
>                 device_type="cpu";
>                 reg=<00000000>;
>                 d-cache-line-size=<00000020>;
>                 i-cache-line-size=<00000020>;
>                 d-cache-size=<00008000>;
>                 i-cache-size=<00008000>;
>                 timebase-frequency=<00000000>;  // not updated
>                 bus-frequency=<00000000>;       // not updated
>                 clock-frequency=<00000000>;     // not updated, see above
>                 32-bit;
>         };
> };
> 
> I wouldn't call this quality material.
> 
> I'll post (specific) patches once I get a kernel booting, unless someone
> beats me to it.
> 
> Kim

(Aaargh, hit the wrong button.  Sorry for the line noise.)

Hi Kim,

FWIIW, that is in the (infamous) common/fdt_support.c.  It was adapted
from ft_build.c.  It looks like I lost a OF_CPU in the constructed
string when I was adapting.

Original code:
         p = ft_get_prop(blob, "/cpus/" OF_CPU "/clock-frequency", &len);

Comments:
1) Hmm, the OF_CPU definition comes from include/config.h and I'm not 
pulling it in.  Grrrrr.

2) Some of the code is in fdt_support.c fdt_chosen().  The stuff messing
with CPU-specific values does NOT belong in fdt_chosen(), it should go
in cpu/mpc83xx/cpu.c

3) /cpu/PowerPC,8360 at 0/bus-frequency is properly set in
cpu/mpc83xx/cpu.c ft_cpu_setup().  The table of nodes and properties
correctly has
     "/cpus/" OF_CPU,
     "bus-frequency",
Ooops, another config.h missing.  Double grrrrr.  Also, the CPU-specific 
fixups from fdt_chosen() (point #2) should be moved here.

I'll clean this up unless someone beats me to it.  :-/

Best regards,
gvb

[U-Boot-Users] Imminent u-boot-fdt pull request

[Jerry Van Baren]

Kim Phillips wrote:
> On Tue, 22 May 2007 00:06:06 -0400
> Jerry Van Baren <gvb.uboot@gmail.com> wrote:
> 
>> Dear all,
>>
>> Please review the u-boot-fdt changes.  I intend to request Wolfgang pull 
>> the pending changes soon.  All of the changes (except the last trivial 
> 
> I have properties being placed at the wrong level, timebase- and
> bus-frequencies not being updated, there are multiple copies of the
> code, and, it is hard to read - I saw a line length of 139!
> 
> Please work on your CodingStyle.  I haven't finished debugging here.
> 
> Kim

Hi Kim,

Thanks for the feedback, I'll check things.  Pointers to files (and 
lines) would be helpful.  :-/

WRT line lengths, Wolfgang dinged me on them so I created a cleanup 
patch.  As a result, one patch has line length violations and a later 
patch fixes them.  This may be related to your line length and multiple 
copy comment.

Thanks,
gvb

[U-Boot-Users] Imminent u-boot-fdt pull request

[Kim Phillips]

On Fri, 25 May 2007 07:54:47 -0400
Jerry Van Baren <gerald.vanbaren@smiths-aerospace.com> wrote:

> Thanks for the feedback, I'll check things.  Pointers to files (and 
> lines) would be helpful.  :-/
> 
> WRT line lengths, Wolfgang dinged me on them so I created a cleanup 
> patch.  As a result, one patch has line length violations and a later 
> patch fixes them.  This may be related to your line length and multiple 
> copy comment.
> 
> Thanks,
> gvb

$ wc -L common/fdt_support.c
    139 common/fdt_support.c

introduced by:

commit 792f0989bf90961ba2644ccc718cf09d338706b8
Author: Gerald Van Baren <vanbaren@cideas.com>
Date:   Wed May 16 22:39:59 2007 -0400

    Fix cmd_fdt line lengths, refactor code.

    Break lines that were greater than 80 characters in length.
    Move the fdt print and property parsing code to separate static functions
      to reduce coding clutter in the fdt_cmd handling body.

    Signed-off-by: Gerald Van Baren <vanbaren@cideas.com>

Kim

[U-Boot-Users] Imminent u-boot-fdt pull request

[Jerry Van Baren]

Kim Phillips wrote:
> On Fri, 25 May 2007 07:54:47 -0400
> Jerry Van Baren <gerald.vanbaren@smiths-aerospace.com> wrote:
> 
>> Thanks for the feedback, I'll check things.  Pointers to files (and 
>> lines) would be helpful.  :-/
>>
>> WRT line lengths, Wolfgang dinged me on them so I created a cleanup 
>> patch.  As a result, one patch has line length violations and a later 
>> patch fixes them.  This may be related to your line length and multiple 
>> copy comment.
>>
>> Thanks,
>> gvb
> 
> $ wc -L common/fdt_support.c
>     139 common/fdt_support.c
> 
> introduced by:
> 
> commit 792f0989bf90961ba2644ccc718cf09d338706b8
> Author: Gerald Van Baren <vanbaren@cideas.com>
> Date:   Wed May 16 22:39:59 2007 -0400
> 
>     Fix cmd_fdt line lengths, refactor code.
> 
>     Break lines that were greater than 80 characters in length.
>     Move the fdt print and property parsing code to separate static functions
>       to reduce coding clutter in the fdt_cmd handling body.
> 
>     Signed-off-by: Gerald Van Baren <vanbaren@cideas.com>
> 
> Kim

Hmm, looks like I did a half-a$$ job there.  Looking at the patch, I 
only cleaned up cmd_fdt and missed cleaning up the long lines in 
fdt_+support.c.

Sorry for the sloppiness, I'll fix that.

Best regards,
gvb