Imminent u-boot-fdt pull request

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

Quoted 4 lines · On Tue, 22 May 2007 00:06:06 -0400 Jerry Van Baren <[email protected]> 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!

Quoted 4 lines · On Tue, 22 May 2007 00:06:06 -0400 Jerry Van Baren <[email protected]> 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

Quoted 5 lines · Kim Phillips

Quoted 2 lines · On Tue, 22 May 2007 00:06:06 -0400 Jerry Van Baren <[email protected]> wrote:

Dear all,

Quoted 6 lines

Quoted 2 lines

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

Quoted 10 lines · On Fri, 25 May 2007 07:54:47 -0400 Jerry Van Baren…

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 <[email protected]>
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 <[email protected]>

Kim

Quoted 4 lines · On Fri, 25 May 2007 11:27:47 +0200 Wolfgang Grandegger <[email protected]>…

Quoted 3 lines · Kim Phillips

On Tue, 22 May 2007 00:06:06 -0400
Jerry Van Baren <[email protected]> wrote:

Quoted 8 lines

Quoted 7 lines

Quoted 4 lines

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

Quoted 2 lines

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

Quoted 4 lines · On Fri, 25 May 2007 11:27:47 +0200 Wolfgang Grandegger <[email protected]>…

Quoted 3 lines · Kim Phillips

On Tue, 22 May 2007 00:06:06 -0400
Jerry Van Baren <[email protected]> wrote:

Quoted 8 lines

Quoted 7 lines

Quoted 4 lines

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

Quoted 2 lines

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

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

On Fri, 25 May 2007 12:56:13 -0400
Jerry Van Baren <[email protected]> wrote:

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

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

Quoted 10 lines · Jerry Van Baren

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

Quoted 5 lines

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.

Quoted 7 lines · Jerry Van Baren

Quoted 5 lines

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

Quoted 8 lines

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

"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?

Quoted 2 lines · Kim Phillips

On Fri, 25 May 2007 12:56:13 -0400
Jerry Van Baren <[email protected]> wrote:

[snip]

Quoted 5 lines

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.

Quoted 22 lines · On Fri, 25 May 2007 13:33:49 -0400 Jerry Van Baren…

Quoted 2 lines · Kim Phillips

On Fri, 25 May 2007 12:56:13 -0400
Jerry Van Baren <[email protected]> wrote:

[snip]

Quoted 5 lines

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.

Quoted 8 lines

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