> -----Original Message-----
> From: Scott Wood [mailto:o...@buserror.net]
> Sent: Tuesday, February 23, 2016 6:52 AM
> To: Prabhakar Kushwaha <prabhakar.kushw...@nxp.com>; u-
> b...@lists.denx.de
> Cc: york sun <york....@nxp.com>; Priyanka Jain <priyanka.j...@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.kushw...@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.
> 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)).
- Need to maintain different dts per combination like LS2080 + LS2085ARDB ,
LS2088 + LS2085ARDB, LS2085A + LS2085ARDB
Here board is common and SoC getting change
Assumption: SoC does not have major change.
> > ---
> > arch/arm/cpu/armv8/fsl-layerscape/fdt.c | 53
> > ++++++++++++++++++++++++++
> > arch/arm/include/asm/arch-fsl-layerscape/soc.h | 4 ++
> > include/common.h | 2 +
> > include/configs/ls2080a_common.h | 1 +
> > lib/fdtdec.c | 10 +++++
> > 5 files changed, 70 insertions(+)
> >
> > diff --git a/arch/arm/cpu/armv8/fsl-layerscape/fdt.c
> > b/arch/arm/cpu/armv8/fsl-layerscape/fdt.c
> > index 4e4861d..cbdeef3 100644
> > --- a/arch/arm/cpu/armv8/fsl-layerscape/fdt.c
> > +++ b/arch/arm/cpu/armv8/fsl-layerscape/fdt.c
> > @@ -5,11 +5,13 @@
> > */
> >
> > #include <common.h>
> > +#include <asm/io.h>
> > #include <libfdt.h>
> > #include <fdt_support.h>
> > #include <phy.h>
> > #ifdef CONFIG_FSL_LSCH3
> > #include <asm/arch/fdt.h>
> > +#include <asm/arch/soc.h>
> > #endif
> > #ifdef CONFIG_FSL_ESDHC
> > #include <fsl_esdhc.h>
> > @@ -18,6 +20,8 @@
> > #include <asm/arch/mp.h>
> > #endif
> >
> > +DECLARE_GLOBAL_DATA_PTR;
> > +
> > int fdt_fixup_phy_connection(void *blob, int offset, phy_interface_t
> > phyc) {
> > return fdt_setprop_string(blob, offset, "phy-connection-type", @@
> > -205,3 +209,52 @@ void ft_cpu_setup(void *blob, bd_t *bd)
> > fdt_fixup_smmu(blob);
> > #endif
> > }
> > +
> > +void ft_early_fixup_cpu(void *blob)
> > +{
> > + int off;
> > + u32 svr, ver;
> > + struct ccsr_gur __iomem *gur = (void
> *)(CONFIG_SYS_FSL_GUTS_ADDR);
> > +
> > + off = fdt_path_offset(blob, "/cpus");
> > + if (off < 0) {
> > + puts("couldn't find /cpus node\n");
> > + return;
> > + }
> > +
> > + off = fdt_node_offset_by_prop_value(blob, -1, "device_type",
> "cpu",
> > 4);
> > + svr = gur_in32(&gur->svr);
> > + ver = SVR_SOC_VER(svr);
> > +
> > + while (off != -FDT_ERR_NOTFOUND) {
> > + switch(ver) {
> > + case SVR_LS2080:
> > + case SVR_LS2085:
> > + case SVR_LS2045:
> > + case SVR_LS2040:
> > + fdt_setprop_string(blob, off, "compatible",
> > + "arm,cortex-a57");
> > + break;
> > + case SVR_LS2088:
> > + case SVR_LS2048:
> > + case SVR_LS2084:
> > + case SVR_LS2028:
> > + fdt_setprop_string(blob, off, "compatible",
> > + "arm,cortex-a72");
> > + break;
> > + }
> > +
> > + off = fdt_node_offset_by_prop_value(blob, off,
> > "device_type",
> > + "cpu", 4);
> > + }
> > +}
> > +
> > +void ft_early_cpu_setup(void **blob)
> > +{
> > + fdt_move(*blob, (void *)CONFIG_SYS_DTS_ADDR,
> fdt_totalsize(blob));
> > +
> > + *blob = (void *)CONFIG_SYS_DTS_ADDR;
> > +
> > + ft_early_fixup_cpu((void *) *blob);
> > +
> > +}
>
> This is hard to follow. Eliminate unnecessary casts, and s/DTS/DTB/ -- we're
> not dealing with the source code here.
Sure, I will fix it.
> Why do you need to do *blob instead of just referencing gd directly?
>
Then the code will be like below
fdt_move(gd->blob, (void *)CONFIG_SYS_DTS_ADDR, fdt_totalsize(blob));
gd->blob = (void *)CONFIG_SYS_DTS_ADDR
ft_early_fixup_cpu(gd->blob); --> As gd is global variable. May be avoid
passing it as function parameter
> > diff --git a/include/common.h b/include/common.h index
> > 1563d64..6dc8a7f 100644
> > --- a/include/common.h
> > +++ b/include/common.h
> > @@ -603,6 +603,8 @@ void ft_pci_setup(void *blob, bd_t *bd); #endif
> > #endif
> >
> > +void ft_early_cpu_setup(void **);
> > +
>
> Arguments should have names.
>
> > 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.
> Is there anywhere that
> documents how various pieces of OCRAM are used?
>
IIUC there is no such document available. Need to create one.
Let me work on this.
> BTW, arch/arm/include/asm/arch-fsl-layerscape/config.h says OCRAM 2 MiB
> but the RM says it's 128 KiB.
Thanks for pointing out. Yes, It is wrong.
I don’t have reason for this. May be just to have generic define.
>
> 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.
For SPL. Assumption is SPL never use dts. It will only be used by u-boot.
Once control is transferred to u-boot, things work as it is like NOR boot.
> > 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.
> > const char *fdtdec_get_compatible(enum fdt_compat_id id) {
> > /* We allow reading of the 'unknown' ID for testing purposes */ @@
> > -605,6 +613,8 @@ int fdtdec_prepare_fdt(void) #endif
> > return -1;
> > }
> > +
> > + ft_early_cpu_setup((void *)&gd->fdt_blob);
>
> Unnecessary cast (or, it would be if you added the appropriate const in
> fdt_early_cpu_setup).
>
Sure.
--prabhakar
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
