[U-Boot] [RFC] armv8: layerscape: Add support of u-boot device tree fix-up

[Scott Wood]

On Tue, 2016-02-23 at 04:09 +0000, Prabhakar Kushwaha wrote:
> > -----Original Message-----
> > From: Scott Wood [mailto:oss at buserror.net]
> > Sent: Tuesday, February 23, 2016 6:52 AM
> > To: Prabhakar Kushwaha <prabhakar.kushwaha at nxp.com>; u-
> > boot at lists.denx.de
> > Cc: york sun <york.sun at nxp.com>; Priyanka Jain <priyanka.jain at nxp.com>
> > Subject: Re: [RFC] armv8: layerscape: Add support of u-boot device tree
> > fix-
> > up
> > 
> > On Mon, 2016-02-22 at 16:05 +0530, Prabhakar Kushwaha wrote:
> > > There is a requirement of u-boot dts fix-up before it is being
> > > consumed by u-boot.
> > 
> > You might want to explain the reason *why* we have this requirement --
> > that the board takes a socketed SoC, and we don't want to have to reflash
> > the board if one SoC is unplugged and another plugged in.
> > 
> > > 
> > > NXP's SoC LS2085A, LS2080A and LS2088A are almost same except
> > > variation in ARM core where LS2085A/LS2080A has A57 and LS2088A has
> > A72.
> > > These SoCs will be placed on common LS2085ARDB platform.
> > > 
> > > So instead of maintaining defferent device tree per SoC, fix-up dts
> > > before being used by u-boot.
> > > 
> > > Signed-off-by: Prabhakar Kushwaha <prabhakar.kushwaha at nxp.com>
> > 
> > And what happens when the next socketed board can support chips with
> > device trees that are significantly more different?  
> 
> Usually next revision of board should not have major change in terms of
> interface. 
> If it is **different** new device tree, defconfig, <board>_config.h needs to
> be used.

This has nothing to do with different revisions of a board.  It has to do with
the set of chips that can be plugged into the *same* board.

> > There should be a  mechanism for having multiple device trees present, and
> > choosing one based
> > on platform code.
> >  That would also avoid any problems of trying to modify a device tree
> > before
> > relocation.
> > 
> 
> I agree. But it may have following problems
>  - Increasing bootloader size (u-boot + dtb(s)).

Not by much.  If some boot sources are so close to the limit that this
presents a problem, then we can deal with it, but this should not be a problem
with NOR flash at least.

>  - Need to maintain different dts per combination like LS2080 + LS2085ARDB ,
>    LS2088 + LS2085ARDB, LS2085A + LS2085ARDB

So?  Includes make it easy to maintain variants.

>  Here board is common and SoC getting change  
> 
> Assumption: SoC does not have major change. 

I don't think that's a good assumption -- and what constitutes a major change?
 A different core could be considered a major change.  In any case, you need
specialized fixup code for every type of difference between the SoCs.

Choosing from multiple device trees is a cleaner solution that does not place
limits on the type of change from SoC to SoC, and does not consume OCRAM
(among other benefits, this makes the mechanism transferable to other types of
devices that may not have enough pre-relocation RAM to hold a device tree).

> > > diff --git a/include/configs/ls2080a_common.h
> > > b/include/configs/ls2080a_common.h
> > > index def0a6f..aa5ace9 100644
> > > --- a/include/configs/ls2080a_common.h
> > > +++ b/include/configs/ls2080a_common.h
> > > @@ -24,6 +24,7 @@
> > > 
> > >  /* Link Definitions */
> > >  #define CONFIG_SYS_INIT_SP_ADDR
> > 	(CONFIG_SYS_FSL_OCRAM_BASE +
> > > 0xfff0)
> > > +#define CONFIG_SYS_DTS_ADDR
> > 	(CONFIG_SYS_FSL_OCRAM_BASE +
> > > 0xfff0)
> > 
> > Why 0xfff0, and not, say, 0x10000 (or rather, why is INIT_SP_ADDR 0xfff0
> > and
> > not 0x10000 if there is no need to reserve some space above the initial SP
> > addr (e.g. to indicate the end of a traceback))? 
> 
> I used the 0xfff0 thinking STACK moving from 0xfff0 to 0x0000. 
> So better put dts starting from 0xfff0 to high address.

My point is that there is likely a reason that the stack ends at an odd
address like 0xfff0 rather than 0x10000, and most likely that reason is to
reserve a location at the end of the stack that contains zeroes, to terminate
a stack traceback.  So please don't put something else there.

> > Where do you check that the device tree fits in OCRAM?  What about when
> > SPL is occupying OCRAM?  Does the device tree get used with SPL (I don't
> > think we were using FDT control at all the last time I worked with SPL on
> > these chips)?
> > 
> 
> I checked ls2085ardb dtb size in Linux. It is ~30K. So from 0xfff0 to 20000
> there is enough space.

What about when we have PPA or similar?  How much space will that take up?


> > > diff --git a/lib/fdtdec.c b/lib/fdtdec.c index 1b1ca02..fc200cf 100644
> > > --- a/lib/fdtdec.c
> > > +++ b/lib/fdtdec.c
> > > @@ -76,6 +76,14 @@ static const char * const
> > compat_names[COMPAT_COUNT] = {
> > >  	COMPAT(COMPAT_INTEL_IVYBRIDGE_FSP, "intel,ivybridge-fsp"),  };
> > > 
> > > +void __ft_early_cpu_setup(void **blob) {
> > > +	return;
> > > +}
> > > +void ft_early_cpu_setup(void **blob)
> > > +	__attribute__((weak, alias("__ft_early_cpu_setup")));
> > > +
> > > +
> > 
> > Why is common code getting infrastructure that assumes CPU is some special
> > consideration?  If we allow fixups for this, why not for other things?
> > 
> 
> fl_early_cpu_setup has been creating similar to ft_cpu_setup which calls all
> necessary fix-up including cpu. 

I see ft_cpu_setup() doing a bunch of things that aren't related to a cpu, so
that's not exactly a good pattern to follow.

-Scott