[U-Boot] [PATCH 09/14] armv8/ls1043ardb: Add nand boot support

[Scott Wood <scottwood@freescale.com>]

[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 at 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.Gong@freescale.com>
> > > Signed-off-by: Hou Zhiqiang <B48286@freescale.com>
> > > Signed-off-by: Shaohui Xie <Shaohui.Xie@freescale.com>
> > > Signed-off-by: Mingkai Hu <Mingkai.Hu@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?
 
>  
> >
> > > +
> > > +     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.

env_init() initializes the environment, though we need to enable 
CONFIG_NAND_ENV_DST for it to actually get loaded from NAND.

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

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

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

-Scott