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

[Scott Wood]

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?  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.

> ---
>  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.  Why do you need to do *blob instead of
just referencing gd directly?

> 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))?  Is there anywhere that
documents how various pieces of OCRAM are used?

BTW, arch/arm/include/asm/arch-fsl-layerscape/config.h says OCRAM 2 MiB but
the RM says it's 128 KiB.

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)?

> 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?

>  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).

-Scott