> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Wednesday, September 16, 2015 6:33 AM
> To: Gong Qianyu-B52263
> Cc: u-boot@lists.denx.de; Xie Shaohui-B21989; Hou Zhiqiang-B48286; Hu
> Mingkai-B21284; Song Wenbin-B53747; Sun York-R58495
> Subject: Re: [U-Boot] [PATCH 09/14] armv8/ls1043ardb: Add nand boot
> support
>
> [Added York Sun -- please CC him on future patches]
>
> On Tue, 2015-09-15 at 06:47 -0500, Gong Qianyu-B52263 wrote:
> > > -----Original Message-----
> > > From: Wood Scott-B07421
> > > Sent: Tuesday, September 15, 2015 7:08 AM
> > > To: Gong Qianyu-B52263
> > > Cc: u-boot@lists.denx.de; Xie Shaohui-B21989; Hou Zhiqiang-B48286;
> > > Hu Mingkai-B21284; Song Wenbin-B53747
> > > Subject: Re: [U-Boot] [PATCH 09/14] armv8/ls1043ardb: Add nand boot
> > > support
> > >
> > > On Fri, 2015-09-11 at 19:07 +0800, Gong Qianyu wrote:
> > > > Signed-off-by: Gong Qianyu <qianyu.g...@freescale.com>
> > > > Signed-off-by: Hou Zhiqiang <b48...@freescale.com>
> > > > Signed-off-by: Shaohui Xie <shaohui....@freescale.com>
> > > > Signed-off-by: Mingkai Hu <mingkai...@freescale.com>
> > > > ---
> > > > arch/arm/Kconfig | 1 +
> > > > arch/arm/cpu/armv8/fsl-lsch2/Makefile | 1 +
> > > > arch/arm/cpu/armv8/fsl-lsch2/spl.c | 91
> > > > ++++++++++++++++++++++
> > > > arch/arm/include/asm/arch-fsl-lsch2/config.h | 2 +
> > > > board/freescale/ls1043ardb/ls1043ardb_pbi.cfg | 14 ++++
> > > > board/freescale/ls1043ardb/ls1043ardb_rcw_nand.cfg | 7 ++
> > > > configs/ls1043ardb_nand_defconfig | 4 +
> > > > include/configs/ls1043a_common.h | 31 ++++++++
> > > > include/configs/ls1043ardb.h | 40 ++++++++++
> > > > 9 files changed, 191 insertions(+)
> > > >
> > > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index
> > > > f935f19..197c72d 100644
> > > > --- a/arch/arm/Kconfig
> > > > +++ b/arch/arm/Kconfig
> > > > @@ -612,6 +612,7 @@ config TARGET_VEXPRESS64_BASE_FVP config
> > > > TARGET_VEXPRESS64_JUNO
> > > > bool "Support Versatile Express Juno Development Platform"
> > > > select ARM64
> > > > + select SUPPORT_SPL
> > >
> > > The subject line says you're adding nand boot support to ls1043ardb,
> > > not Juno.
> > >
> > > Also, the previous patch adds SUPPORT_SPL to ls1043ardb -- was it
> > > supported in that patch (for non-NAND boot) or is that an error?
> > >
> >
> > Sorry, this is really a patching mistake.:(
> >
> > > >
> > > > config TARGET_LS2085A_EMU
> > > > bool "Support ls2085a_emu"
> > > > diff --git a/arch/arm/cpu/armv8/fsl-lsch2/Makefile
> > > > b/arch/arm/cpu/armv8/fsl- lsch2/Makefile index 23c5bf9..0573659
> > > > 100644
> > > > --- a/arch/arm/cpu/armv8/fsl-lsch2/Makefile
> > > > +++ b/arch/arm/cpu/armv8/fsl-lsch2/Makefile
> > > > @@ -10,3 +10,4 @@ obj-y += lowlevel.o obj-y += speed.o
> > > > obj-$(CONFIG_SYS_HAS_SERDES) += fsl_lsch2_serdes.o
> > > > ls1043a_serdes.o
> > > > obj-$(CONFIG_OF_LIBFDT) += fdt.o
> > > > +obj-$(CONFIG_SPL) += spl.o
> > > > diff --git a/arch/arm/cpu/armv8/fsl-lsch2/spl.c
> > > > b/arch/arm/cpu/armv8/fsl- lsch2/spl.c new file mode 100644 index
> > > > 0000000..980901a
> > > > --- /dev/null
> > > > +++ b/arch/arm/cpu/armv8/fsl-lsch2/spl.c
> > > > @@ -0,0 +1,91 @@
> > > > +/*
> > > > + * Copyright 2014 Freescale Semiconductor, Inc.
> > > > + *
> > > > + * SPDX-License-Identifier: GPL-2.0+ */
> > > > +
> > > > +#include <common.h>
> > > > +#include <spl.h>
> > > > +#include <asm/io.h>
> > > > +#include <fsl_ifc.h>
> > > > +#include <i2c.h>
> > > > +#include <asm/arch-fsl-lsch2/immap_lsch2.h>
> > > > +#include "../../../../../board/freescale/common/ns_access.h"
> > >
> > > Why is this header in board/freescale/common if code outside that
> > > directory needs it?
> > >
> > > Where did you note the dependency on "ARMv7/ls1021a: move ns_access
> > > to common file" which is not in this patchset?
> > >
> > >
> >
> > The ns_access.h is common and shared by not only LS1043A but also
> > LS1021A boards.
> >
> > The "ARMv7/ls1021a” patch had been sent out much earlier while the
> > ls1043a patches were not ready.
> > Some details are still overlooked, though.
>
> That doesn't answer my question. If the header needs to be accessed from
> outside board/freescale/common it should not go in board/freescale/common.
> How about include/fsl-ns-access.h? What does "ns" stand for here?
>
OK. Our team discussed and now decide to create a include/fsl_csu.h to include
the csu common parts.
> >
> > >
> > > > +
> > > > + get_clocks();
> > > > +
> > > > + preloader_console_init();
> > > > +
> > > > +#ifdef CONFIG_SPL_I2C_SUPPORT
> > > > + i2c_init_all();
> > > > +#endif
> > > > + dram_init();
> > > > +
> > > > + /* Clear the BSS */
> > > > + memset(__bss_start, 0, __bss_end - __bss_start);
> > > > +
> > > > +#ifdef CONFIG_LAYERSCAPE_NS_ACCESS
> > > > + enable_layerscape_ns_access(); #endif
> > > > + board_init_r(NULL, 0);
> > > > +}
> > >
> > > Can you explain the differences between this and the fsl-lsch3
> version?
> > >
> >
> > The basic board_init_f() for spl contains necessary initializations
> > such as clocks, serial, ddr and BSS.
> > So they are generally the same.
>
> Right. It would be nice if things that "are generally the same" between
> similar chips actually be the same, except where differences are
> justified.
>
> > I’m not very clear about how spl works on LS2085A. Why does its spl
> > need
> > arch_cpu_init() and env_init()?
>
> arch_cpu_init() is low-level CPU initialization -- primarily setting up
> caches. In theory you may not need it in SPL, but it speeds things up.
> It was especially important when we were running in a hardware emulator,
> but it's still nice to have.
OK. It's nice of it to "speed things up".
>
> env_init() initializes the environment, though we need to enable
> CONFIG_NAND_ENV_DST for it to actually get loaded from NAND.
>
But why do you need to get env variables in early spl? Is it indispensable for
ls2085?
U-Boot will just do the same initialization later.
> > > > diff --git a/include/configs/ls1043a_common.h
> > > > b/include/configs/ls1043a_common.h
> > > > index 139005c..4bda296 100644
> > > > --- a/include/configs/ls1043a_common.h
> > > > +++ b/include/configs/ls1043a_common.h
> > > > @@ -60,6 +60,37 @@
> > > > #define CONFIG_BAUDRATE 115200
> > > > #define CONFIG_SYS_BAUDRATE_TABLE { 9600, 19200, 38400, 57600,
> > > 115200 }
> > > >
> > > > +/* NAND SPL */
> > > > +#ifdef CONFIG_NAND_BOOT
> > > > +#define CONFIG_SPL_PBL_PAD
> > > > +#define CONFIG_SPL_FRAMEWORK
> > > > +#define CONFIG_SPL_LDSCRIPT "arch/arm/cpu/armv8/u-boot-
> > > spl.lds"
> > > > +#define CONFIG_SPL_TARGET "u-boot-with-spl.bin"
> > > > +#define CONFIG_SPL_LIBCOMMON_SUPPORT #define
> > > > +CONFIG_SPL_LIBGENERIC_SUPPORT #define CONFIG_SPL_ENV_SUPPORT
> > > > +#define CONFIG_SPL_WATCHDOG_SUPPORT #define
> > > > +CONFIG_SPL_I2C_SUPPORT #define CONFIG_SPL_SERIAL_SUPPORT #define
> > > > +CONFIG_SPL_MPC8XXX_INIT_DDR_SUPPORT
> > > > +#define CONFIG_SPL_NAND_SUPPORT
> > > > +#define CONFIG_SPL_DRIVERS_MISC_SUPPORT
> > > > +#define CONFIG_SPL_TEXT_BASE 0x10000000
> > > > +#define CONFIG_SPL_MAX_SIZE 0x1a000
> > > > +#define CONFIG_SPL_STACK 0x1001d000
> > > > +#define CONFIG_SPL_PAD_TO 0x1c000
> > > > +#define CONFIG_SYS_NAND_U_BOOT_SIZE (600 << 10) #define
> > > > +CONFIG_SYS_NAND_U_BOOT_OFFS CONFIG_SPL_PAD_TO
> > > > +#define CONFIG_SYS_NAND_PAGE_SIZE 2048
> > > > +#define CONFIG_SYS_NAND_U_BOOT_DST CONFIG_SYS_TEXT_BASE
> > > > +#define CONFIG_SYS_NAND_U_BOOT_START CONFIG_SYS_TEXT_BASE #define
> > > > +CONFIG_SYS_SPL_MALLOC_START 0x80200000
> > > > +#define CONFIG_SPL_BSS_START_ADDR 0x80100000
> > > > +#define CONFIG_SYS_SPL_MALLOC_SIZE 0x100000
> > > > +#define CONFIG_SPL_BSS_MAX_SIZE 0x80000
> > > > +#define CONFIG_SYS_MONITOR_LEN 0xa0000
> > > > +#endif
> > >
> > > Can you explain the differences relative to ls2085a, especially
> > > addresses, offsets, sizes, and padding?
> > >
> >
> > I’m not clear about how ls102085a specifies the values.
> > As I know on ls1043a boards,
> > CONFIG_SPL_TEXT_BASE is where SPL code is put.
>
> Which parts of OCRAM does the bootrom use while processing PBI? On
> ls208x we avoided the first 0xa000 bytes because of this.
>
At the end of ls1043a bootrom, it just jumps to OCRAM offset 0x0 to execute spl
code.
So the CONFIG_SPL_TEXT_BASE is 0x1000_0000 which is the OCRAM base address on
ls1043a.
"Avoided the first 0xa000 bytes" means the ls208x bootrom reserves the first
0xa000 bytes for itself?
> > The max size also depends on SPL image.
>
> What do you mean by "The max size also depends on SPL image"? The max
> size depends on the size of OCRAM and the offset within OCRAM of the SPL
> image (minus any areas at the end of OCRAM that need to be reserved).
>
Thanks. You make it more detailed. Of course the SPL max size mustn't be larger
than OCRAM size.
> > CONFIG_SPL_PAD_TO is actually U-Boot image offset to NAND 0x0 base
> > because the U-Boot image is put right after padded SPL image.
> > CONFIG_SPL_PAD_TO must be larger than CONFIG_SPL_MAX_SIZE.
> > Do you think the spl code could be common for ls1043a and ls2085a ?
> >
> > > Why is board-specific information such as NAND page size being
> > > hardcoded in an SoC common header file?
> > >
> > Now both LS1043ARDB and LS1043AQDS boards share the same NAND page size.
>
> So? It's still board-specific. Don't make it hard to add support for a
> custom board with a different NAND chip. Likewise for any other
> parameters that depend on page/block size (e.g. to ensure alignment).
Oh, we didn't consider any custom board. Maybe it's better to keep their
respective macros for long-term consideration.
>
> -Scott
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
