[U-Boot-Users] [PATCH 2/3] fdt: Fixup compile error and add a new OF manipulation option
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");
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_UPDATEThis define adds or updates a bootargs field to the /chosenentry.
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
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....
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.
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
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.
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 =====================================================================
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
participants (3)
-
Bryan O'Donoghue -
Jerry Van Baren -
Stefan Roese