[U-Boot-Users] [PATCH 2/3] fdt: Fixup compile error and add a new OF manipulation option

[U-Boot-Users] [PATCH 2/3] fdt: Fixup compile error and add a new OF manipulation option

[Bryan O'Donoghue]

Greetings.

This patch fixes up a compile error that crept with with debug switched on.
Introduces CONFIG_OF_CHOSEN_UPDATE - which is useful if you have a /chosen
entry in the dts - which doesn't contain a bootargs entry - in which case you'd
want u-boot's version of this.

Signed-off-by: Bryan O'Donoghue <bodonoghue@codehermit.ie>
---

diff --git a/README b/README
index 26f93c2..bc7a6a4 100644
--- a/README
+++ b/README
@@ -375,6 +375,11 @@ The following options need to be configured:
 		This define fills in the correct boot cpu in the boot
 		param header, the default value is zero if undefined.
 
+		CONFIG_OF_CHOSEN_UPDATE
+
+		This define adds or updates a bootargs field to the /chosen
+		entry.
+
 - Serial Ports:
 		CFG_PL010_SERIAL
 
diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c
index 9546729..c729f52 100644
--- a/common/cmd_bootm.c
+++ b/common/cmd_bootm.c
@@ -975,7 +975,11 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
 	 * if the user wants it (the logic is in the subroutines).
 	 */
 	if (of_flat_tree) {
-		if (fdt_chosen(of_flat_tree, initrd_start, initrd_end, 0) < 0) {
+#ifdef CONFIG_OF_CHOSEN_UPDATE
+	if (fdt_chosen(of_flat_tree, initrd_start, initrd_end, 1) < 0) {
+#else
+	if (fdt_chosen(of_flat_tree, initrd_start, initrd_end, 0) < 0) {
+#endif
 			puts ("ERROR: /chosen node create failed - "
 				"must RESET the board to recover.\n");
 			do_reset (cmdtp, flag, argc, argv);
diff --git a/common/fdt_support.c b/common/fdt_support.c
index b5ee6e9..ba1306c 100644
--- a/common/fdt_support.c
+++ b/common/fdt_support.c
@@ -408,7 +408,7 @@ void do_fixup_by_path(void *fdt, const char *path, const char *prop,
 {
 #if defined(DEBUG)
 	int i;
-	debug("Updating property '%s/%s' = ", node, prop);
+	debug("Updating property '%s/%s' = ", path, prop);
 	for (i = 0; i < len; i++)
 		debug(" %.2x", *(u8*)(val+i));
 	debug("\n");
@@ -434,7 +434,7 @@ void do_fixup_by_prop(void *fdt,
 	int off;
 #if defined(DEBUG)
 	int i;
-	debug("Updating property '%s/%s' = ", node, prop);
+	debug("Updating property '%s/%s' = ", pname, prop);
 	for (i = 0; i < len; i++)
 		debug(" %.2x", *(u8*)(val+i));
 	debug("\n");
@@ -461,7 +461,7 @@ void do_fixup_by_compat(void *fdt, const char *compat,
 	int off = -1;
 #if defined(DEBUG)
 	int i;
-	debug("Updating property '%s/%s' = ", node, prop);
+	debug("Updating property '%s/%s' = ", compat, prop);
 	for (i = 0; i < len; i++)
 		debug(" %.2x", *(u8*)(val+i));
 	debug("\n");

[U-Boot-Users] [PATCH 2/3] fdt: Fixup compile error and add a new OF manipulation option

[Jerry Van Baren]

Bryan O'Donoghue wrote:
> Greetings.
> 
> This patch fixes up a compile error that crept with with debug switched on.
> Introduces CONFIG_OF_CHOSEN_UPDATE - which is useful if you have a /chosen
> entry in the dts - which doesn't contain a bootargs entry - in which case you'd
> want u-boot's version of this.
> 
> Signed-off-by: Bryan O'Donoghue <bodonoghue@codehermit.ie>
> ---
> 
> diff --git a/README b/README
> index 26f93c2..bc7a6a4 100644
> --- a/README
> +++ b/README
> @@ -375,6 +375,11 @@ The following options need to be configured:
>  		This define fills in the correct boot cpu in the boot
>  		param header, the default value is zero if undefined.
>  
> +		CONFIG_OF_CHOSEN_UPDATE
> +
> +		This define adds or updates a bootargs field to the /chosen
> +		entry.
> +

Hi Bryan,

I don't think CONFIG_OF_CHOSEN_UPDATE is needed.  If it *is* needed, I 
don't think it *should be* needed and we need to discuss scenarios.

Initially, we did not touch the /chosen node *at all* if it already 
existed.  That was where the "force" flag came from - the criteria 
wasn't fine grained enough.  That behavior was changed in the 1.3.1 time 
frame to be fine grained: if /chosen exists, but /chosen/<prop> doesn't, 
that property is created and set to the required value.

If /chosen/<prop> exists, it is overwritten only if the "force" flag is 
set.  What you have added with CONFIG_OF_CHOSEN_UPDATE it a compile time 
"force" flag setting.

Maybe that is desirable, but I'm going to challenge you to justify it.  :-)

The problem / limitation with adding CONFIG_OF_CHOSEN_UPDATE is that it 
is a compile time option.  If we compile "force" to be TRUE, why not 
just compile it in as a '1' bit (below)?  I guess my problem is that I 
don't see the usefulness of nearly hardcoding it to be '1' or '0' (e.g. 
via a #define) vs. absolutely hardcoding it to be '1' or '0'.

If we *do* need to support a force/noforce option (and I have not 
conceded that point yet ;-), I think it should be an env variable, 
something the user can control at run time.

[snip]

> diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c
> index 9546729..c729f52 100644
> --- a/common/cmd_bootm.c
> +++ b/common/cmd_bootm.c
> @@ -975,7 +975,11 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
>  	 * if the user wants it (the logic is in the subroutines).
>  	 */
>  	if (of_flat_tree) {
> -		if (fdt_chosen(of_flat_tree, initrd_start, initrd_end, 0) < 0) {
> +#ifdef CONFIG_OF_CHOSEN_UPDATE
> +	if (fdt_chosen(of_flat_tree, initrd_start, initrd_end, 1) < 0) {
> +#else
> +	if (fdt_chosen(of_flat_tree, initrd_start, initrd_end, 0) < 0) {
> +#endif

Compile time selection is pretty inflexible.  Note also that this is a 
pretty big hammer - *everything* in the fdt_chosen() routine 
(potentially) gets forced.

Do we really need the "force" flag any more?

[snip]

Best regards,
gvb

[U-Boot-Users] [PATCH 2/3] fdt: Fixup compile error and add a new OF manipulation option

[Bryan O'Donoghue]

On Tue, 12 Feb 2008 08:27:14 -0500
Jerry Van Baren <gerald.vanbaren@ge.com> wrote:

> Bryan O'Donoghue wrote:

> Hi Bryan,
> 
> I don't think CONFIG_OF_CHOSEN_UPDATE is needed.  If it *is* needed, I 
> don't think it *should be* needed and we need to discuss scenarios.

Hey Jerry.

I'd have to say I fundamentally agree with the principal you're putting
forward here - i.e. that using a compile time define is a bit inflexible and
doubly wrong in that it's manipulating a parameter which ideally shouldn't
exist anyway.

The behavior I'm looking for is simply where /chosen/bootargs - doesn't exist
in a dts - that U-boot will fill in it's environment bootargs entry in the
tree - i.e. it creates /chosen/<prop> if <prop> doesn't exist.

> Initially, we did not touch the /chosen node *at all* if it already 
> existed.  That was where the "force" flag came from - the criteria 
> wasn't fine grained enough.  That behavior was changed in the 1.3.1 time 
> frame to be fine grained: if /chosen exists, but /chosen/<prop> doesn't, 
> that property is created and set to the required value.

That creation won't happen though due to the code fragment below.
 
> If /chosen/<prop> exists, it is overwritten only if the "force" flag is 
> set.  What you have added with CONFIG_OF_CHOSEN_UPDATE it a compile time 
> "force" flag setting.
> 
> Maybe that is desirable, but I'm going to challenge you to justify it.  :-)

Nope it's admitedly a blunt instrument to accomplish stuffing /chosen/<prop> if
<prop> doesn't exist.

What the define works around is this in fdt_support.c

/*
 * If we have a "chosen" node already the "force the writing"
 * is not set, our job is done.
 */
if ((nodeoffset >= 0) && !force)
	return 0;

The default sans CONFIG_OF_CHOSEN_UPDATE in do_bootm_linux is to pass force = 0
- which means if /chosen is present in the dts - then no manipulation of
the /chosen entry will be performed - as I read it anyway.

I was thinking that some sort of define would be the least invasive way of
changing that.... with a view to the least amount of change being the most
likely to be acceptable to other developers - not necessarily the best way to
do the thing....

[U-Boot-Users] [PATCH 2/3] fdt: Fixup compile error and add a new OF manipulation option

[Jerry Van Baren]

Bryan O'Donoghue wrote:
> On Tue, 12 Feb 2008 08:27:14 -0500
> Jerry Van Baren <gerald.vanbaren@ge.com> wrote:
> 
>> Bryan O'Donoghue wrote:
> 
>> Hi Bryan,
>>
>> I don't think CONFIG_OF_CHOSEN_UPDATE is needed.  If it *is* needed, I 
>> don't think it *should be* needed and we need to discuss scenarios.
> 
> Hey Jerry.
> 
> I'd have to say I fundamentally agree with the principal you're putting
> forward here - i.e. that using a compile time define is a bit inflexible and
> doubly wrong in that it's manipulating a parameter which ideally shouldn't
> exist anyway.
> 
> The behavior I'm looking for is simply where /chosen/bootargs - doesn't exist
> in a dts - that U-boot will fill in it's environment bootargs entry in the
> tree - i.e. it creates /chosen/<prop> if <prop> doesn't exist.
> 
>> Initially, we did not touch the /chosen node *at all* if it already 
>> existed.  That was where the "force" flag came from - the criteria 
>> wasn't fine grained enough.  That behavior was changed in the 1.3.1 time 
>> frame to be fine grained: if /chosen exists, but /chosen/<prop> doesn't, 
>> that property is created and set to the required value.
> 
> That creation won't happen though due to the code fragment below.
>  
>> If /chosen/<prop> exists, it is overwritten only if the "force" flag is 
>> set.  What you have added with CONFIG_OF_CHOSEN_UPDATE it a compile time 
>> "force" flag setting.
>>
>> Maybe that is desirable, but I'm going to challenge you to justify it.  :-)
> 
> Nope it's admitedly a blunt instrument to accomplish stuffing /chosen/<prop> if
> <prop> doesn't exist.
> 
> What the define works around is this in fdt_support.c
> 
> /*
>  * If we have a "chosen" node already the "force the writing"
>  * is not set, our job is done.
>  */
> if ((nodeoffset >= 0) && !force)
> 	return 0;
> 
> The default sans CONFIG_OF_CHOSEN_UPDATE in do_bootm_linux is to pass force = 0
> - which means if /chosen is present in the dts - then no manipulation of
> the /chosen entry will be performed - as I read it anyway.
> 
> I was thinking that some sort of define would be the least invasive way of
> changing that.... with a view to the least amount of change being the most
> likely to be acceptable to other developers - not necessarily the best way to
> do the thing....

I think you have an older version of fdt_support.c.  Check the latest 
version - the above check is no longer in there.  That was a change that 
went in towards the end of the last patch window.

<http://www.denx.de/cgi-bin/gitweb.cgi?p=u-boot.git;a=blob;f=common/fdt_support.c;h=92f1c7f54fd5e62942c8496bde52639e420067c1;hb=HEAD>

The patch that changed this:
<http://www.denx.de/cgi-bin/gitweb.cgi?p=u-boot.git;a=commitdiff;h=b60af3d4c1680487ee37e11aa1b3db6dec04d8f0>

It is possible we have some other wires crossed as well.  There was a 
fair amount of LIBFDT support refactoring going on in the last patch 
window, so we may have missed some deletions related to refactoring that 
should have been made.

Best regards,
gvb

[U-Boot-Users] [PATCH 2/3] fdt: Fixup compile error and add a new OF manipulation option

[Stefan Roese]

Hi Jerry,

On Tuesday 12 February 2008, Jerry Van Baren wrote:
> I think you have an older version of fdt_support.c.  Check the latest
> version - the above check is no longer in there.  That was a change that
> went in towards the end of the last patch window.
>
> <http://www.denx.de/cgi-bin/gitweb.cgi?p=u-boot.git;a=blob;f=common/fdt_sup
>port.c;h=92f1c7f54fd5e62942c8496bde52639e420067c1;hb=HEAD>
>
> The patch that changed this:
> <http://www.denx.de/cgi-bin/gitweb.cgi?p=u-boot.git;a=commitdiff;h=b60af3d4
>c1680487ee37e11aa1b3db6dec04d8f0>
>
> It is possible we have some other wires crossed as well.  There was a
> fair amount of LIBFDT support refactoring going on in the last patch
> window, so we may have missed some deletions related to refactoring that
> should have been made.

I have to admit, that without looking too deep into the fdt code, I don't see 
a way to set the force flag upon Linux booting. Here the code from 
common/cmd_bootm.c:

        /*
         * Add the chosen node if it doesn't exist, add the env and bd_t
         * if the user wants it (the logic is in the subroutines).
         */
        if (of_flat_tree) {
                if (fdt_chosen(of_flat_tree, initrd_start, initrd_end, 0) < 0)

As you can see, the force flag is always set to 0. So how should "force" the 
chosen node manipulation? Perhaps I'm missing something here.

Best regards,
Stefan

=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de
=====================================================================

[U-Boot-Users] [PATCH 2/3] fdt: Fixup compile error and add a new OF manipulation option

[Bryan O'Donoghue]

> > I was thinking that some sort of define would be the least invasive way of
> > changing that.... with a view to the least amount of change being the most
> > likely to be acceptable to other developers - not necessarily the best way
> > to do the thing....
> 
> I think you have an older version of fdt_support.c.

doh !

> The patch that changed this:
> <http://www.denx.de/cgi-bin/gitweb.cgi?p=u-boot.git;a=commitdiff;h=b60af3d4c1680487ee37e11aa1b3db6dec04d8f0>

Yep - that'd do it alright. Right-o I'll resync to the latest git tree - though
I can't be more then 6 weeks out here...  and re-submit sans
CONFIG_OF_FLAT_TREE and of course CONFIG_OF_CHOSEN_UPDATE.

Cheers,
Bryan