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