Introduce the Tertiary Program loader

board/freescale/p1021mds/law.c new file +22 -0

new file mode 100644
index 0000000..d0be19e
@@ -0,0 +1,38 @@
+/*
+ * Copyright (C) 2010 - 2011 Freescale Semiconductor, Inc.
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+

board/freescale/p1021mds/law.c line 32

Quoted 7 lines

board/freescale/p1021mds/law.c new file +7 -0

+
+struct law_entry law_table[] = {
+#ifndef CONFIG_IN_TPL
+	SET_LAW(CONFIG_SYS_PCIE1_MEM_PHYS, LAW_SIZE_512M, LAW_TRGT_IF_PCIE_1),
+	SET_LAW(CONFIG_SYS_PCIE1_IO_PHYS, LAW_SIZE_64K, LAW_TRGT_IF_PCIE_1),
+	SET_LAW(CONFIG_SYS_PCIE2_MEM_PHYS, LAW_SIZE_512M, LAW_TRGT_IF_PCIE_2),
+	SET_LAW(CONFIG_SYS_PCIE2_IO_PHYS, LAW_SIZE_64K, LAW_TRGT_IF_PCIE_2),

PCIe LAWs are now set by common code, you can remove these.

(quoted diff) +16 -0

+/* These are used when DDR doesn't use SPD. ?*/
+#define CONFIG_SYS_SDRAM_SIZE ? ? ? ? ? 512 ? ? ? ? ? ?/* DDR is 512MB */
+#define CONFIG_SYS_DDR_CS0_BNDS ? ? ? ? 0x0000001F
+#define CONFIG_SYS_DDR_CS0_CONFIG ? ? ? 0x80014202
+#define CONFIG_SYS_DDR_CS0_CONFIG_2 ? ?0x00000000
+#define CONFIG_SYS_DDR_SDRAM_CFG ? ? ? 0x47000000
+#define CONFIG_SYS_DDR_SDRAM_CFG_2 ? ? 0x04401040
+#define CONFIG_SYS_DDR_ZQ_CNTL ? ? ? ? 0x89080600
+#define CONFIG_SYS_DDR_WRLVL_CNTL ? ? ?0x86559608
+#define CONFIG_SYS_DDR_CDR_1 ? ? ? ? ? 0x000eaa00
+#define CONFIG_SYS_DDR_CDR_2 ? ? ? ? ? 0x00000000
+#define CONFIG_SYS_DDR_OCD_CTRL ? ? ? ? 0x00000000
+#define CONFIG_SYS_DDR_OCD_STATUS ? ? ? 0x00000000
+#define CONFIG_SYS_DDR_CONTROL ? ? ? ? ?0x470c0000 ? ? ?/* Type = DDR3 */
+#define CONFIG_SYS_DDR_CONTROL_2 ? ? ? 0x04401050
+#define CONFIG_SYS_DDR_DATA_INIT ? ? ? ?0x1021babe

+#define CONFIG_SYS_I2C_EEPROM_ADDR ? ? ?0x52

Quoted 7 lines

nand_spl/board/freescale/p1021mds/nand_boot.c new file +7 -0

+void putc(char c)
+{
+ ? ? ? if (c == '\n')
+ ? ? ? ? ? ? ? NS16550_putc((NS16550_t)CONFIG_SYS_NS16550_COM1, '\r');
+
+ ? ? ? NS16550_putc((NS16550_t)CONFIG_SYS_NS16550_COM1, c);
+}

nand_spl/board/freescale/p1021mds/nand_boot.c line 69

Quoted 6 lines

nand_spl/board/freescale/p1021mds/nand_boot.c new file +6 -0

+
+void puts(const char *str)
+{
+ ? ? ? while (*str)
+ ? ? ? ? ? ? ? putc(*str++);
+}

These look like functions that shouldn't be in board-specific code.

Quoted 21 lines · On Thu, Jan 27, 2011 at 10:58 PM, <[email protected]> wrote:

include/configs/P1021MDS.h new file +21 -0

+/* These are used when DDR doesn't use SPD.  */
+#define CONFIG_SYS_SDRAM_SIZE           512            /* DDR is 512MB */
+#define CONFIG_SYS_DDR_CS0_BNDS         0x0000001F
+#define CONFIG_SYS_DDR_CS0_CONFIG       0x80014202
+#define CONFIG_SYS_DDR_CS0_CONFIG_2    0x00000000
+#define CONFIG_SYS_DDR_SDRAM_CFG       0x47000000
+#define CONFIG_SYS_DDR_SDRAM_CFG_2     0x04401040
+#define CONFIG_SYS_DDR_ZQ_CNTL         0x89080600
+#define CONFIG_SYS_DDR_WRLVL_CNTL      0x86559608
+#define CONFIG_SYS_DDR_CDR_1           0x000eaa00
+#define CONFIG_SYS_DDR_CDR_2           0x00000000
+#define CONFIG_SYS_DDR_OCD_CTRL         0x00000000
+#define CONFIG_SYS_DDR_OCD_STATUS       0x00000000
+#define CONFIG_SYS_DDR_CONTROL          0x470c0000      /* Type = DDR3 */
+#define CONFIG_SYS_DDR_CONTROL_2       0x04401050
+#define CONFIG_SYS_DDR_DATA_INIT        0x1021babe
+#define CONFIG_SYS_DDR_TIMING_3                0x00010000
+#define CONFIG_SYS_DDR_TIMING_0                0x00330004
+#define CONFIG_SYS_DDR_TIMING_1                0x5d5bd746
+#define CONFIG_SYS_DDR_TIMING_2                0x0fa8c8cd
+#define CONFIG_SYS_DDR_SDRAM_MODE      0x40461320

include/configs/P1021MDS.h line 146

Quoted 5 lines

include/configs/P1021MDS.h new file +5 -0

+#define CONFIG_SYS_DDR_SDRAM_MODE_2    0x8000C000
+#define CONFIG_SYS_DDR_SDRAM_INTERVAL  0x0a280000
+#define CONFIG_SYS_DDR_SDRAM_CLK_CNTL  0x03000000
+#define CONFIG_SYS_DDR_TIMING_4                0x00220001
+#define CONFIG_SYS_DDR_TIMING_5                0x03402400

Aren't static DDR configurations now handled in a board-specific source file? Look at board/freescale/corenet_ds/p4080ds_ddr.c

include/configs/P1021MDS.h line 426

Quoted 5 lines

include/configs/P1021MDS.h new file +4 -0

+#define CONFIG_ID_EEPROM
+#ifdef CONFIG_ID_EEPROM
+#define CONFIG_SYS_I2C_EEPROM_NXID
+#endif

No need for the #ifdef here. CONFIG_SYS_I2C_EEPROM_NXID is not used in any Makefile.

Quoted 2 lines

+#define CONFIG_SYS_I2C_EEPROM_ADDR 0x52

Not 0x57? That's where the NXID EEPROM almost always is.

Quoted 7 lines

nand_spl/board/freescale/p1021mds/nand_boot.c new file +7 -0

+void putc(char c)
+{
+       if (c == '\n')
+               NS16550_putc((NS16550_t)CONFIG_SYS_NS16550_COM1, '\r');
+
+       NS16550_putc((NS16550_t)CONFIG_SYS_NS16550_COM1, c);
+}

nand_spl/board/freescale/p1021mds/nand_boot.c line 69

Quoted 6 lines

nand_spl/board/freescale/p1021mds/nand_boot.c new file +6 -0

+
+void puts(const char *str)
+{
+       while (*str)
+               putc(*str++);
+}

These look like functions that shouldn't be in board-specific code.

Quoted 3 lines

Quoted 3 lines

+#define CONFIG_SYS_I2C_EEPROM_ADDR 0x52

Not 0x57? That's where the NXID EEPROM almost always is.

It is board specific value, isn't it? P1021MDS does use 0x52 for board eeprom.

Quoted 4 lines · Haiying Wang

Quoted 3 lines

Quoted 3 lines

+#define CONFIG_SYS_I2C_EEPROM_ADDR 0x52

Not 0x57? That's where the NXID EEPROM almost always is.

It is board specific value, isn't it? P1021MDS does use 0x52 for board eeprom.

I just wanted you to be sure it wasn't a typo. Did you actual test reading and writing to the EEPROM with the 'mac' command?

From: Haiying Wang <[email protected]>

Signed-off-by Haiying Wang <[email protected]>

---
 arch/powerpc/config.mk |    4 ++++
 config.mk              |    7 ++++++-
 2 files changed, 10 insertions(+), 1 deletions(-)

config.mk +0 -0

index 5147c35..d7bb07f 100644
@@ -260,8 +260,13 @@ $(obj)%.s:	%.c
 #########################################################################
 
 # If the list of objects to link is empty, just create an empty built-in.o

Quoted 2 lines · On Thu, 27 Jan 2011 23:58:10 -0500 <[email protected]> wrote:

From: Haiying Wang <[email protected]>

Quoted 5 lines

Signed-off-by Haiying Wang <[email protected]>

---
 arch/powerpc/config.mk |    4 ++++
 config.mk              |    7 ++++++-
 2 files changed, 10 insertions(+), 1 deletions(-)

I see patch 3/8, 4/8, 5/8, and 7/7. Where are the rest?

Quoted 7 lines

config.mk +0 -0

index 5147c35..d7bb07f 100644
@@ -260,8 +260,13 @@ $(obj)%.s:	%.c
 #########################################################################
 

config.mk $(obj)%.s: %.c · line 271

Quoted 11 lines

config.mk +6 -1

 # If the list of objects to link is empty, just create an empty built-in.o
+ifdef CONFIG_HAS_TPL
+cmd_link_o_target = $(if $(strip $1),\
+		      $(LD) -r -o $@ $1,\
+		      rm -f $@; $(AR) rcs $@ )
+else
 cmd_link_o_target = $(if $(strip $1),\
 		      $(LD) $(LDFLAGS) -r -o $@ $1,\
 		      rm -f $@; $(AR) rcs $@ )
-
+endif

What's going on here?

Quoted 1 line

I see patch 3/8, 4/8, 5/8, and 7/7. Where are the rest?

Sorry, patch 7/7 is a wrong number here. I kept the patch # as 3/8/, 4/8, 5/8, 8/8 to be consistent with the order in the patchset(v2) I submitted in last December. I thought it would be clearer to compare them with v2 version and review. Patch 1/8,2/8 have been applied by Kumar, patch 6/8, 7/8 remain the same as v2 version. This patch, is a new patch because that TPL still needs --gc-sections in linker option to do partial link.

If it is preferable to have new whole set of patch, I can reorder them from 3/8-8/8 plus this one to submit.

Quoted 5 lines · Scott Wood

Quoted 2 lines · On Thu, 27 Jan 2011 23:58:10 -0500 <[email protected]> wrote:

From: Haiying Wang <[email protected]>

Quoted 7 lines

Quoted 5 lines

Signed-off-by Haiying Wang <[email protected]>

---
 arch/powerpc/config.mk |    4 ++++
 config.mk              |    7 ++++++-
 2 files changed, 10 insertions(+), 1 deletions(-)

I see patch 3/8, 4/8, 5/8, and 7/7. Where are the rest?

Sorry, patch 7/7 is a wrong number here. I kept the patch # as 3/8/, 4/8, 5/8, 8/8 to be consistent with the order in the patchset(v2) I submitted in last December. I thought it would be clearer to compare them with v2 version and review. Patch 1/8,2/8 have been applied by Kumar, patch 6/8, 7/8 remain the same as v2 version. This patch, is a new patch because that TPL still needs --gc-sections in linker option to do partial link.

If it is preferable to have new whole set of patch, I can reorder them from 3/8-8/8 plus this one to submit.

Quoted 7 lines

Quoted 7 lines

config.mk +0 -0

index 5147c35..d7bb07f 100644
@@ -260,8 +260,13 @@ $(obj)%.s:	%.c
 #########################################################################
 

Quoted 14 lines

config.mk $(obj)%.s: %.c · line 271

Quoted 11 lines

config.mk +6 -1

 # If the list of objects to link is empty, just create an empty built-in.o
+ifdef CONFIG_HAS_TPL
+cmd_link_o_target = $(if $(strip $1),\
+		      $(LD) -r -o $@ $1,\
+		      rm -f $@; $(AR) rcs $@ )
+else
 cmd_link_o_target = $(if $(strip $1),\
 		      $(LD) $(LDFLAGS) -r -o $@ $1,\
 		      rm -f $@; $(AR) rcs $@ )
-
+endif

What's going on here?

For CONFIG_HAS_TPL, LDFLAGS has --gc-sections now, passing it to cmd_link_o_target here will fail in linking stage:
"
powerpc-none-linux-gnuspe-ld: gc-sections requires either an entry or an undefined symbol
"

Quoted 21 lines

Quoted 21 lines

Quoted 8 lines

config.mk +0 -0

index 5147c35..d7bb07f 100644
@@ -260,8 +260,13 @@ $(obj)%.s:	%.c
 #########################################################################
 
 # If the list of objects to link is empty, just create an empty built-in.o

config.mk $(obj)%.s: %.c · line 271

Quoted 10 lines

config.mk +6 -1

+ifdef CONFIG_HAS_TPL
+cmd_link_o_target = $(if $(strip $1),\
+		      $(LD) -r -o $@ $1,\
+		      rm -f $@; $(AR) rcs $@ )
+else
 cmd_link_o_target = $(if $(strip $1),\
 		      $(LD) $(LDFLAGS) -r -o $@ $1,\
 		      rm -f $@; $(AR) rcs $@ )
-
+endif

What's going on here?

Quoted 6 lines

For CONFIG_HAS_TPL, LDFLAGS has --gc-sections now, passing it to cmd_link_o_target here will fail in linking stage:
"
powerpc-none-linux-gnuspe-ld: gc-sections requires either an entry or an undefined symbol
"

I think --gc-sections should go in LDFLAGS_u-boot instead.

In any case, I don't think we want different behavior here based on whether we have TPL. Either LDFLAGS is used in partial linking, or it's not.

Quoted 1 line · Scott Wood

I think --gc-sections should go in LDFLAGS_u-boot instead.

LDFLAGS_u-boot has --gc-sections already, I did not change it.

Quoted 3 lines

In any case, I don't think we want different behavior here based on whether we have TPL. Either LDFLAGS is used in partial linking, or it's not.

I don't understand why LDFLAGS was added here in patch
http://lists.denx.de/pipermail/u-boot/2011-January/084705.html

It says "LDFLAGS sets necessary option by partial linking (use in cmd_link_o_target)." But without this changing, the partial linking worked well before. Please correct me if I am wrong.

So if someone can confirm LDFLAGS is not necessary to be added in cmd_link_o_target, I prefer not add it here.

Quoted 3 lines

In any case, I don't think we want different behavior here based on whether we have TPL. Either LDFLAGS is used in partial linking, or it's not.

I don't understand why LDFLAGS was added here in patch
http://lists.denx.de/pipermail/u-boot/2011-January/084705.html

It says "LDFLAGS sets necessary option by partial linking (use in cmd_link_o_target)." But without this changing, the partial linking worked well before. Please correct me if I am wrong.

So if someone can confirm LDFLAGS is not necessary to be added in cmd_link_o_target, I prefer not add it here.

Quoted 10 lines · Haiying Wang

Quoted 3 lines

In any case, I don't think we want different behavior here based on whether we have TPL. Either LDFLAGS is used in partial linking, or it's not.

I don't understand why LDFLAGS was added here in patch
http://lists.denx.de/pipermail/u-boot/2011-January/084705.html

It says "LDFLAGS sets necessary option by partial linking (use in cmd_link_o_target)." But without this changing, the partial linking worked well before. Please correct me if I am wrong.

Quoted 2 lines

So if someone can confirm LDFLAGS is not necessary to be added in cmd_link_o_target, I prefer not add it here.

BTW, I doubt removing --gc-sections for PLATFORM_FLAGS by patch
http://lists.denx.de/pipermail/u-boot/2011-January/084705.html may have the risk of building failure for nand_spl, as we encountered the message "NAND bootstrap too big" before

From: Haiying Wang <[email protected]>

Support P1021MDS board to boot from NAND flash (No NOR flash on this board). And because P1021 only has 256K L2 SRAM, which can not used for final uboot image, this patch also enables the TPL BOOT on P1021MDS so that DDR can be initialized in L2 SRAM through SPD code. So there are three stage uboot images:
* nand_spl, pad from 4KB size to 16KB, load tpl_boot from offset 16KB in NAND.
* tpl_boot, 112KB size. The env variables are copied to offset 128KB in L2 SRAM, so that ddr spd code can get the interleaving mode setting in env. It loads final uboot image from offset 128KB in NAND.
* final uboot image, size is variable depends on the functions enabled.

I'm not questioning the patch, I'm just trying to understand.

Review

Quoted 3 lines

(quoted diff) +3 -0

+#define CONFIG_MP                      /* Multiprocessor support */
+
+#define CONFIG_PCI                     /* Disable PCI/PCIE */

Shouldn't that be "Enable" PCI?