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

[Jerry Van Baren <gerald.vanbaren@ge.com>]

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