Sorry for late reply since the email system crash.

-----Original Message----- From: Scott Wood [mailto:scottwood@freescale.com] Sent: Saturday, December 07, 2013 9:22 AM To: Liu Po-B43644 Cc: u-boot@lists.denx.de; Sun York-R58495; Kushwaha Prabhakar-B32579 Subject: Re: [PATCH v2 2/2] powerpc/c29xpcie: 8k page size NAND boot support base on TPL/SPL

On Thu, 2013-12-05 at 14:19 +0800, Po Liu wrote:

...

diff --git a/board/freescale/c29xpcie/spl.c b/board/freescale/c29xpcie/spl.c new file mode 100644 index 0000000..7bc8ce1 --- /dev/null +++ b/board/freescale/c29xpcie/spl.c @@ -0,0 +1,73 @@ +/* Copyright 2013 Freescale Semiconductor, Inc.

• • SPDX-License-Identifier: GPL-2.0+
• */

+#include <common.h> +#include <ns16550.h> +#include <malloc.h> +#include <mmc.h> +#include <nand.h> +#include <i2c.h>

+DECLARE_GLOBAL_DATA_PTR;

+ulong get_effective_memsize(void) +{

• return CONFIG_SYS_L2_SIZE;

+}

+void board_init_f(ulong bootflag) +{

• u32 plat_ratio;
• ccsr_gur_t *gur = (void *)CONFIG_SYS_MPC85xx_GUTS_ADDR;
• console_init_f();
• /* initialize selected port with appropriate baud rate */
• plat_ratio = in_be32(&gur->porpllsr) & MPC85xx_PORPLLSR_PLAT_RATIO;
• plat_ratio >>= 1;
• gd->bus_clk = CONFIG_SYS_CLK_FREQ * plat_ratio;
• NS16550_init((NS16550_t)CONFIG_SYS_NS16550_COM1,

•           gd->bus_clk / 16 / CONFIG_BAUDRATE);
  

• /* copy code to RAM and jump to it - this should not return */
• /* NOTE - code has to be copied out of NAND buffer before
• • other blocks can be read.
• */
• relocate_code(CONFIG_SPL_RELOC_STACK, 0,

+CONFIG_SPL_RELOC_TEXT_BASE); }

+void board_init_r(gd_t *gd, ulong dest_addr) {

• /* Pointer is writable since we allocated a register for it */
• gd = (gd_t *)CONFIG_SPL_GD_ADDR;
• bd_t *bd;
• memset(gd, 0, sizeof(gd_t));
• bd = (bd_t *)(CONFIG_SPL_GD_ADDR + sizeof(gd_t));
• memset(bd, 0, sizeof(bd_t));
• gd->bd = bd;
• bd->bi_memstart = CONFIG_SYS_INIT_L2_ADDR;
• bd->bi_memsize = CONFIG_SYS_L2_SIZE;
• probecpu();
• get_clocks();
• mem_malloc_init(CONFIG_SPL_RELOC_MALLOC_ADDR,

•              CONFIG_SPL_RELOC_MALLOC_SIZE);
  

• /* relocate environment function pointers etc. */
• nand_spl_load_image(CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE,

•                (uchar *)CONFIG_ENV_ADDR);
  

•                gd->env_addr  = (ulong)(CONFIG_ENV_ADDR);
  

• gd->env_valid = 1;
• i2c_init_all();
• gd->ram_size = initdram(0);
• puts("\nTertiary program loader running in sram...");

Why do you assume tertiary? Couldn't this be SPL for SD/SPI? Or was it a copy/paste error that you added things to the board config file for SD/SPI (after all, the subject line says it's a NAND patch)?

Yes, I assume this patch only for NAND boot for C29XPCIE.

...

+void board_init_r(gd_t *gd, ulong dest_addr) {

• puts("\nSecond program loader running in sram...");

I see that this isn't new to this patch, but we ought to be consistent and either change this to "secondary", or change "tertiary" to "third".

It's also probably too verbose... Simply saying "SPL\n" or "TPL\n" would suffice to indicate progress and verify that console output is working (if nothing is printed, then that path doesn't get tested in the absence of a load error).

...

diff --git a/board/freescale/c29xpcie/tlb.c b/board/freescale/c29xpcie/tlb.c index 84844ee..11f8a37 100644 --- a/board/freescale/c29xpcie/tlb.c +++ b/board/freescale/c29xpcie/tlb.c @@ -26,10 +26,20 @@ struct fsl_e_tlb_entry tlb_table[] = { 0, 0, BOOKE_PAGESZ_4K, 0),

/* TLB 1 */

+#ifdef CONFIG_SPL_NAND_MINIMAL

• SET_TLB_ENTRY(1, 0xfffff000, 0xfffff000,

•              MAS3_SX|MAS3_SW|MAS3_SR, MAS2_I|MAS2_G,
  

•              0, 10, BOOKE_PAGESZ_4K, 1),
  

• SET_TLB_ENTRY(1, 0xffffe000, 0xffffe000,

•              MAS3_SX|MAS3_SW|MAS3_SR, MAS2_I|MAS2_G,
  

•              0, 11, BOOKE_PAGESZ_4K, 1),
  

+#endif

CONFIG_SPL_NAND_MINIMAL should not exist. It was introduced by accident after a different approach was chosen in patch review (and even then, this wasn't what it meant).

Prabhakar, why did you extend that to other uses? Why are both entries ifdeffed here, but only the 0xffffe000 entry on existing boards?

If this needs to be ifdeffed (and it probably does, if only to avoid possible speculative instruction fetches), use (and document) CONFIG_SPL_NAND_BOOT.

I'll replace the CONFIG_SPL_NAND_MINIMAL with CONFIG_SPL_NAND_BOOT

...

SET_TLB_ENTRY(1, CONFIG_SYS_NAND_BASE, CONFIG_SYS_NAND_BASE_PHYS,

•              MAS3_SW|MAS3_SR, MAS2_I|MAS2_G,
  

•              MAS3_SX|MAS3_SW|MAS3_SR, MAS2_I|MAS2_G,
                0, 5, BOOKE_PAGESZ_64K, 1),
  

No.

...

SET_TLB_ENTRY(1, CONFIG_SYS_PLATFORM_SRAM_BASE, @@ -61,7 +72,8 @@

struct fsl_e_tlb_entry tlb_table[] = { MAS3_SX|MAS3_SW|MAS3_SR, 0, 0, 7, BOOKE_PAGESZ_256K, 1),

-#ifdef CONFIG_SYS_RAMBOOT +#if defined(CONFIG_SYS_RAMBOOT) || (defined(CONFIG_SPL) \

•      && !defined(CONFIG_SPL_COMMON_INIT_DDR))
  

  SET_TLB_ENTRY(1, CONFIG_SYS_DDR_SDRAM_BASE, CONFIG_SYS_DDR_SDRAM_BASE, MAS3_SX|MAS3_SW|MAS3_SR, 0,

This will have the result of mapping DDR in the SPL where it's not used, but not in the TPL where it is.

Just define: #if defined(CONFIG_SYS_RAMBOOT) || defined(CONFIG_SPL) ? I think code load image to SRAM from NAND for SPL and DDR do not need to be allocate.

-Scott