MX6: SPL NAND support

We only need to read in the size of struct image_header and thus don't need to know the page size of the nand device.

Signed-off-by Tim Harvey <[email protected]>

We only need to read in the size of struct image_header and thus don't need to know the page size of the nand device.

Signed-off-by Tim Harvey <[email protected]>

---
  common/spl/spl_nand.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

common/spl/spl_nand.c +0 -0

index 9da0218..062461b 100644

We only need to read in the size of struct image_header and thus don't need to know the page size of the nand device.

Signed-off-by Tim Harvey <[email protected]>

---
 common/spl/spl_nand.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

common/spl/spl_nand.c +0 -0

index 9da0218..062461b 100644

Quoted 14 lines · Tim Harvey

We only need to read in the size of struct image_header and thus don't need to know the page size of the nand device.

Signed-off-by Tim Harvey <[email protected]>

---
 common/spl/spl_nand.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

common/spl/spl_nand.c +0 -0

index 9da0218..062461b 100644

common/spl/spl_nand.c void spl_nand_load_image(void) · line 82

Quoted 8 lines

common/spl/spl_nand.c +1 -1

@@ -76,7 +76,7 @@ void spl_nand_load_image(void)
 #endif
      /* Load u-boot */
      nand_spl_load_image(CONFIG_SYS_NAND_U_BOOT_OFFS,
-             CONFIG_SYS_NAND_PAGE_SIZE, (void *)header);
+             sizeof(*header), (void *)header);
      spl_parse_image_header(header);
      nand_spl_load_image(CONFIG_SYS_NAND_U_BOOT_OFFS,
              spl_image.size, (void *)spl_image.load_addr);

Acked-by Stefano Babic <[email protected]>

Best regards,
Stefano Babic

Quoted 26 lines · Stefano Babic

Quoted 14 lines · Tim Harvey

We only need to read in the size of struct image_header and thus don't need to know the page size of the nand device.

Signed-off-by Tim Harvey <[email protected]>

---
 common/spl/spl_nand.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

common/spl/spl_nand.c +0 -0

index 9da0218..062461b 100644

common/spl/spl_nand.c void spl_nand_load_image(void) · line 82

Quoted 8 lines

common/spl/spl_nand.c +1 -1

@@ -76,7 +76,7 @@ void spl_nand_load_image(void)
 #endif
      /* Load u-boot */
      nand_spl_load_image(CONFIG_SYS_NAND_U_BOOT_OFFS,
-             CONFIG_SYS_NAND_PAGE_SIZE, (void *)header);
+             sizeof(*header), (void *)header);
      spl_parse_image_header(header);
      nand_spl_load_image(CONFIG_SYS_NAND_U_BOOT_OFFS,
              spl_image.size, (void *)spl_image.load_addr);

Acked-by Stefano Babic <[email protected]>

Quoted 3 lines

Best regards,
Stefano Babic

Scott,

This should have been addressed to you in the first place as its NAND and not IMX specific. It's been +1'd and ack'd, so if you agree with it and commit it I'll remove it from my next patch series regarding IMX6 SPL NAND support.

This utilizes existing nand support, including the mtd layer to provide a a method to load an image off nand for SPL. It is somewhat bulky but avoids duplicating code.

This will need to be split out and re-based on top of Tom's recent patch to add MTD NAND support which is needed here.

Signed-off-by Tim Harvey <[email protected]>

---
 drivers/mtd/Makefile            | 11 ++++++++++
 drivers/mtd/nand/Makefile       |  9 ++++++++
 drivers/mtd/nand/mxs_nand_spl.c | 46 +++++++++++++++++++++++++++++++++++++++++
 spl/Makefile                    |  1 +
 4 files changed, 67 insertions(+)
 create mode 100644 drivers/mtd/nand/mxs_nand_spl.c

drivers/mtd/Makefile +0 -0

index 5467a95..10ec470 100644

drivers/mtd/Makefile +5 -0

+
+else # not spl
+
 ifneq (,$(findstring y,$(CONFIG_MTD_DEVICE)$(CONFIG_CMD_NAND)$(CONFIG_CMD_ONENAND)))
 obj-y += mtdcore.o
 endif

@@ -18,3 +27,5 @@ obj-$(CONFIG_FTSMC020) += ftsmc020.o
 obj-$(CONFIG_FLASH_CFI_LEGACY) += jedec_flash.o
 obj-$(CONFIG_MW_EEPROM) += mw_eeprom.o
 obj-$(CONFIG_ST_SMI) += st_smi.o
+
+endif

drivers/mtd/nand/Makefile +0 -0

index 02b149c..8b1a37a 100644
@@ -19,6 +19,14 @@ obj-$(CONFIG_SPL_NAND_ECC) += nand_ecc.o
 obj-$(CONFIG_SPL_NAND_BASE) += nand_base.o

drivers/mtd/nand/Makefile +1 -0

 else # not spl
 
 NORMAL_DRIVERS=y

@@ -65,5 +73,6 @@ else  # minimal SPL drivers
 obj-$(CONFIG_NAND_FSL_ELBC) += fsl_elbc_spl.o
 obj-$(CONFIG_NAND_FSL_IFC) += fsl_ifc_spl.o
 obj-$(CONFIG_NAND_MXC) += mxc_nand_spl.o
+obj-$(CONFIG_NAND_MXS) += mxs_nand_spl.o mxs_nand.o
 
 endif # drivers

drivers/mtd/nand/mxs_nand_spl.c new file +46 -0

new file mode 100644
index 0000000..ec07eb1
@@ -0,0 +1,46 @@
+/*
+ * (C) Copyright 2014
+ * Tim Harvey, Gateworks Corporation, tharvey at gateworks.com
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#include <common.h>
+#include <nand.h>
+
+static nand_info_t mtd;
+static struct nand_chip nand_chip;
+
+int nand_spl_load_image(uint32_t offs, unsigned int size, void *dst)
+{
+	size_t sz, actual;
+
+	debug("%s offset:0x%08x len:%d\n", __func__, offs, size);
+	sz = size;
+	nand_read_skip_bad(&mtd, (loff_t) offs, &sz, &actual, size, dst);
+	debug("read %d/%d bytes\n", sz, actual);
+
+	return 0;
+}
+
+void nand_init(void)
+{
+	int ret;
+
+	board_nand_init(&nand_chip);
+	mtd.priv = &nand_chip;
+	if (nand_chip.select_chip)
+		nand_chip.select_chip(&mtd, 0);
+
+	ret = nand_scan(&mtd, 1);
+	if (!ret)
+		ret = nand_scan_tail(&mtd);
+	debug("oobsize=%d pagesize=%d\n", mtd.oobsize, mtd.writesize);
+	printf("NAND: %llu MiB\n", (mtd.size / (1024 * 1024)));
+}
+
+void nand_deselect(void)
+{
+	if (nand_chip.select_chip)
+		nand_chip.select_chip(&mtd, -1);
+}

spl/Makefile +0 -0

index 346d0aa..6a2c80f 100644

Signed-off-by Tim Harvey <[email protected]>

---
  arch/arm/cpu/armv7/mx6/u-boot-spl.lds | 52 +++++++++++++++++++++++++++++++++++
  1 file changed, 52 insertions(+)

create mode 100644 arch/arm/cpu/armv7/mx6/u-boot-spl.lds

Signed-off-by Tim Harvey <[email protected]>

---
 arch/arm/cpu/armv7/mx6/u-boot-spl.lds | 52 +++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)
 create mode 100644 arch/arm/cpu/armv7/mx6/u-boot-spl.lds

arch/arm/cpu/armv7/mx6/u-boot-spl.lds new file +42 -0

new file mode 100644
index 0000000..02aa129
@@ -0,0 +1,52 @@
+/*
+ * (C) Copyright 2002
+ * Gary Jennejohn, DENX Software Engineering, <[email protected]>
+ *
+ * (C) Copyright 2010
+ * Texas Instruments, <www.ti.com>
+ *	Aneesh V <[email protected]>
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+MEMORY { .sram : ORIGIN = CONFIG_SPL_TEXT_BASE,\
+		LENGTH = CONFIG_SPL_MAX_SIZE }
+MEMORY { .sdram : ORIGIN = CONFIG_SPL_BSS_START_ADDR, \
+		LENGTH = CONFIG_SPL_BSS_MAX_SIZE }
+
+OUTPUT_FORMAT("elf32-littlearm", "elf32-littlearm", "elf32-littlearm")
+OUTPUT_ARCH(arm)
+ENTRY(_start)
+SECTIONS
+{
+	.text      :
+	{
+		__start = .;
+		arch/arm/cpu/armv7/start.o	(.text*)
+		*(.text*)
+	} >.sram
+
+	. = ALIGN(4);
+	.rodata : { *(SORT_BY_ALIGNMENT(.rodata*)) } >.sram
+
+	. = ALIGN(4);
+	.data : { *(SORT_BY_ALIGNMENT(.data*)) } >.sram
+
+	. = ALIGN(4);
+	.u_boot_list : {
+		KEEP(*(SORT(.u_boot_list*_i2c_*)));
+	} >.sram
+
+	. = ALIGN(4);
+	__image_copy_end = .;
+	_end = .;

Hi Tim,

This file is pretty identical to ./arch/arm/cpu/armv7/omap-common/u-boot-spl.lds. I understand that each SOC/arch could be a different set up, but I think we should introduce this diversity when it is needed, not as default.

Maybe can we avoid to copy the file ?

Quoted 2 lines · Stefano Babic

Hi Tim,

<snip>

Quoted 5 lines

This file is pretty identical to ./arch/arm/cpu/armv7/omap-common/u-boot-spl.lds. I understand that each SOC/arch could be a different set up, but I think we should introduce this diversity when it is needed, not as default.

Quoted 2 lines

Maybe can we avoid to copy the file ?

Stefano,

Yes, I agree - the omap-common file will work for imx6 as well. I will use it and remove this patch.

Signed-off-by Tim Harvey <[email protected]>

---
  include/configs/imx6_spl.h | 64

  1 file changed, 64 insertions(+)

create mode 100644 include/configs/imx6_spl.h

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

new file mode 100644
index 0000000..f9bdf55
@@ -0,0 +1,64 @@
+/*
+ * Author: Tim Harvey <[email protected]>
+ *
+ * SPDX-License-Identifier:     GPL-2.0+

include/configs/imx6_spl.h new file +12 -0

+#define CONFIG_SPL_LIBCOMMON_SUPPORT
+#define CONFIG_SPL_LIBGENERIC_SUPPORT
+#define CONFIG_SPL_SERIAL_SUPPORT
+#define CONFIG_SPL_I2C_SUPPORT
+#define CONFIG_SPL_GPIO_SUPPORT
+
+/* NAND support */
+#if defined(CONFIG_SPL_NAND_SUPPORT)
+#define CONFIG_SPL_NAND_MXS
+#define CONFIG_SPL_NAND_BASE
+#define CONFIG_SPL_DMA_SUPPORT
+#endif

Hi Tim,

On 04/03/2014 09:01 AM, Tim Harvey wrote:> Add a common header which can hopefully be shared among imx6 SPL users

Quoted 4 lines

Signed-off-by Tim Harvey <[email protected]>

---
  include/configs/imx6_spl.h | 64

++++++++++++++++++++++++++++++++++++++++++++++

Quoted 14 lines

  1 file changed, 64 insertions(+)

create mode 100644 include/configs/imx6_spl.h

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

new file mode 100644
index 0000000..f9bdf55
@@ -0,0 +1,64 @@
+/*
+ * Author: Tim Harvey <[email protected]>
+ *
+ * SPDX-License-Identifier:     GPL-2.0+

include/configs/imx6_spl.h line 14

Quoted 10 lines

include/configs/imx6_spl.h new file +10 -0

+ */
+#ifndef __IMX6_SPL_CONFIG_H
+#define __IMX6_SPL_CONFIG_H
+
+#ifdef CONFIG_SPL
+
+#define CONFIG_SPL_FRAMEWORK
+
+/*
+ * IMX6 OCRAM (IRAM) is from 0x00907000 to 0x0093FFFF

That's not true for all IMX6 SoCs. On i.MX6 Solo and DualLite it's 0x00907000 to 0x0091FFFF.

include/configs/imx6_spl.h line 21

Quoted 8 lines

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

+ *  - we start at 0x00908000 so as to leave some room for IVT/DCD
+ *  - recommended stack (from IMX6DQRM Figure 8-3) is at 0x0093FFB8
+ *  - this leaves about 224K for SPL image and stack
+ */
+#define CONFIG_SPL_LDSCRIPT

"arch/arm/cpu/armv7/mx6/u-boot-spl.lds"

include/configs/imx6_spl.h new file +2 -0

+#define CONFIG_SPL_TEXT_BASE           0x00908000
+#define CONFIG_SPL_MAX_SIZE            (128 * 1024)

This should be a smaller value if we want this config to apply for i.MX6 Solo and DualLite, which have a 68KB OCRAM free area.

include/configs/imx6_spl.h line 23

Quoted 4 lines

include/configs/imx6_spl.h new file +2 -0

+#define CONFIG_SPL_START_S_PATH                "arch/arm/cpu/armv7"
+#define CONFIG_SPL_STACK               0x0093FFB8

For i.MX6 Solo and DualLite this address should be lower (recommended address is 0x0091FFB8).

Quoted 12 lines

include/configs/imx6_spl.h new file +10 -0

+#define CONFIG_SPL_LIBCOMMON_SUPPORT
+#define CONFIG_SPL_LIBGENERIC_SUPPORT
+#define CONFIG_SPL_SERIAL_SUPPORT
+#define CONFIG_SPL_I2C_SUPPORT
+#define CONFIG_SPL_GPIO_SUPPORT
+
+/* NAND support */
+#if defined(CONFIG_SPL_NAND_SUPPORT)
+#define CONFIG_SPL_NAND_MXS
+#define CONFIG_SPL_NAND_BASE

include/configs/imx6_spl.h

Quoted 2 lines

include/configs/imx6_spl.h new file +2 -0

+#define CONFIG_SPL_DMA_SUPPORT
+#endif

--
Regards,
Nikita.

Quoted 41 lines · Nikita Kiryanov

Hi Tim,

On 04/03/2014 09:01 AM, Tim Harvey wrote:> Add a common header which can hopefully be shared among imx6 SPL users

Quoted 4 lines

Signed-off-by Tim Harvey <[email protected]>

---
   include/configs/imx6_spl.h | 64

++++++++++++++++++++++++++++++++++++++++++++++

Quoted 14 lines

   1 file changed, 64 insertions(+)

create mode 100644 include/configs/imx6_spl.h

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

new file mode 100644
index 0000000..f9bdf55
@@ -0,0 +1,64 @@
+/*
+ * Author: Tim Harvey <[email protected]>
+ *
+ * SPDX-License-Identifier:     GPL-2.0+

include/configs/imx6_spl.h line 14

Quoted 10 lines

include/configs/imx6_spl.h new file +10 -0

+ */
+#ifndef __IMX6_SPL_CONFIG_H
+#define __IMX6_SPL_CONFIG_H
+
+#ifdef CONFIG_SPL
+
+#define CONFIG_SPL_FRAMEWORK
+
+/*
+ * IMX6 OCRAM (IRAM) is from 0x00907000 to 0x0093FFFF

That's not true for all IMX6 SoCs. On i.MX6 Solo and DualLite it's 0x00907000 to 0x0091FFFF.

Quoted 12 lines

include/configs/imx6_spl.h line 21

Quoted 8 lines

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

+ *  - we start at 0x00908000 so as to leave some room for IVT/DCD
+ *  - recommended stack (from IMX6DQRM Figure 8-3) is at 0x0093FFB8
+ *  - this leaves about 224K for SPL image and stack
+ */
+#define CONFIG_SPL_LDSCRIPT

"arch/arm/cpu/armv7/mx6/u-boot-spl.lds"

include/configs/imx6_spl.h new file +2 -0

+#define CONFIG_SPL_TEXT_BASE           0x00908000
+#define CONFIG_SPL_MAX_SIZE            (128 * 1024)

This should be a smaller value if we want this config to apply for i.MX6 Solo and DualLite, which have a 68KB OCRAM free area.

Hi Nikita,

Agreed - I just discovered this yesterday. I had tested on an IMX6DL/SOLO via boot from OTG, but I had not tested those combinations when booting from flash and indeed they failed.

The 68KB OCRAM free area also assumes that you start at 0x00907000 and as we start at 0x00908000 to leave room for the IVT+DCD this becomes 65KB

Quoted 8 lines

include/configs/imx6_spl.h line 23

Quoted 4 lines

include/configs/imx6_spl.h new file +2 -0

+#define CONFIG_SPL_START_S_PATH                "arch/arm/cpu/armv7"
+#define CONFIG_SPL_STACK               0x0093FFB8

For i.MX6 Solo and DualLite this address should be lower (recommended address is 0x0091FFB8).

Agreed

Quoted 12 lines

Quoted 12 lines

include/configs/imx6_spl.h new file +10 -0

+#define CONFIG_SPL_LIBCOMMON_SUPPORT
+#define CONFIG_SPL_LIBGENERIC_SUPPORT
+#define CONFIG_SPL_SERIAL_SUPPORT
+#define CONFIG_SPL_I2C_SUPPORT
+#define CONFIG_SPL_GPIO_SUPPORT
+
+/* NAND support */
+#if defined(CONFIG_SPL_NAND_SUPPORT)
+#define CONFIG_SPL_NAND_MXS
+#define CONFIG_SPL_NAND_BASE

Quoted 8 lines

include/configs/imx6_spl.h

Quoted 2 lines

include/configs/imx6_spl.h new file +2 -0

+#define CONFIG_SPL_DMA_SUPPORT
+#endif

--
Regards,
Nikita.

This presents another challenge for SPL NAND as currently my SPL is ~70K. There is a lot of unnecessary code in the mtd nand layer that I'm including because that layer includes support for both read and write (I don't need write for SPL) as well as various NAND types (and I only need BCH).

I'm not sure yet what the best approach is to resolve that. I can either:
a) add a lot of ifdef's around functions in
drivers/mtd/nand/{nand_base.c,nand_bbt.c} to remove NAND write and non BCH support for CONFIG_SPL_BUILD
b) re-write the necessary functionality (code duplication) into
drivers/mtd/nand/mxs_nand_spl.c

Thanks for the review!

Tim

Quoted 13 lines · Tim Harvey

This presents another challenge for SPL NAND as currently my SPL is ~70K. There is a lot of unnecessary code in the mtd nand layer that I'm including because that layer includes support for both read and write (I don't need write for SPL) as well as various NAND types (and I only need BCH).

I'm not sure yet what the best approach is to resolve that. I can either:
a) add a lot of ifdef's around functions in
drivers/mtd/nand/{nand_base.c,nand_bbt.c} to remove NAND write and non BCH support for CONFIG_SPL_BUILD
b) re-write the necessary functionality (code duplication) into
drivers/mtd/nand/mxs_nand_spl.c

Quoted 1 line

Thanks for the review!

You're welcome. Let's see if the NAND maintainer can offer some suggestions.

Cc-ing Scott Wood

Add enums, #defines, and helper functions needed for SPL images to describe and detect IMX6 boot device.

Signed-off-by Tim Harvey <[email protected]>

---
 arch/arm/cpu/armv7/mx6/soc.c                | 56 +++++++++++++++++++++++++++++
 arch/arm/include/asm/arch-mx6/spl.h         | 26 ++++++++++++++
 arch/arm/include/asm/imx-common/boot_mode.h | 17 +++++++++
 3 files changed, 99 insertions(+)
 create mode 100644 arch/arm/include/asm/arch-mx6/spl.h

arch/arm/cpu/armv7/mx6/soc.c +0 -0

index 1725279..4ee62c7 100644

arch/arm/cpu/armv7/mx6/soc.c +23 -0

+		boot_dev = MX6_SATA_BOOT;
+		break;
+	/* Serial ROM: See 8.5.5.1, Table 8-22 */
+	case 0x3:
+		/* BOOT_CFG4[2:0] */
+		switch ((soc_sbmr & 0x07000000) >> 24) {
+		case 0x0 ... 0x4:
+			boot_dev = MX6_SPI_NOR_BOOT;
+			break;
+		case 0x5 ... 0x7:
+			boot_dev = MX6_I2C_BOOT;
+			break;
+		}
+		break;
+	/* SD/eSD: 8.5.3, Table 8-15  */
+	case 0x4:
+	case 0x5:
+		boot_dev = MX6_SD_BOOT;
+		break;
+	/* MMC/eMMC: 8.5.3 */
+	case 0x6:
+	case 0x7:
+		boot_dev = MX6_MMC_BOOT;

arch/arm/cpu/armv7/mx6/soc.c +4 -0

+	return boot_dev;
+}
+
+
 void s_init(void)
 {
 	struct anatop_regs *anatop = (struct anatop_regs *)ANATOP_BASE_ADDR;

arch/arm/include/asm/arch-mx6/spl.h new file +15 -0

new file mode 100644
index 0000000..5611c71
@@ -0,0 +1,26 @@
+/*
+ * Copyright (C) 2013 TechNexion Ltd.
+ *
+ * Author: Richard Hu <[email protected]>
+ *
+ * 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.
+ */
+
+#ifndef __ASM_ARCH_SPL_H__

arch/arm/include/asm/arch-mx6/spl.h new file +1 -0

+#endif	/* __ASM_ARCH_SPL_H__ */

arch/arm/include/asm/imx-common/boot_mode.h +5 -0

index de0205c..3686367 100644
@@ -17,4 +17,21 @@ struct boot_mode {
 void add_board_boot_modes(const struct boot_mode *p);
 void boot_mode_apply(unsigned cfg_val);
 extern const struct boot_mode soc_boot_modes[];
+
+/* boot devices */
+enum boot_device {
+	MX6_SD_BOOT,
+	MX6_MMC_BOOT,

Hi Tim,

Quoted 8 lines

arch/arm/cpu/armv7/mx6/soc.c +0 -0

index 1725279..4ee62c7 100644
@@ -305,6 +305,62 @@ const struct boot_mode soc_boot_modes[] = {
      {NULL,          0},
 };
 

arch/arm/cpu/armv7/mx6/soc.c const struct boot_mode soc_boot_modes[] = { · line 313

Quoted 6 lines

arch/arm/cpu/armv7/mx6/soc.c +6 -0

+/* determine boot device from SRC_SBMR1 register (BOOT_CFG[4:1]) */
+enum boot_device get_boot_device(void)
+{
+     enum boot_device boot_dev;
+
+     uint soc_sbmr = readl(SRC_BASE_ADDR + 0x4); /* SRC_SBMR1 */

Use structure to access internal registers. The structure is already defined in imx-regs.h.

Quoted 4 lines

arch/arm/cpu/armv7/mx6/soc.c +3 -0

+
+     /* BOOT_CFG1[7:4] - see IMX6DQRM Table 8-8 */
+     switch ((soc_sbmr & 0x000000FF) >> 4) {

arch/arm/cpu/armv7/mx6/soc.c const struct boot_mode soc_boot_modes[] = { · line 326

Quoted 10 lines

arch/arm/cpu/armv7/mx6/soc.c +10 -0

+      /* EIM: See 8.5.1, Table 8-9 */
+     case 0x0:
+             /* BOOT_CFG1[3]: NOR/OneNAND Selection */
+             if ((soc_sbmr & 0x00000008) >> 3)
+                     boot_dev = MX6_ONE_NAND_BOOT;
+             else
+                     boot_dev = MX6_WEIM_NOR_BOOT;
+             break;
+     /* SATA: See 8.5.4, Table 8-20 */
+     case 0x2:

Can we use #defines for that ?

Quoted 24 lines

arch/arm/cpu/armv7/mx6/soc.c +23 -0

+             boot_dev = MX6_SATA_BOOT;
+             break;
+     /* Serial ROM: See 8.5.5.1, Table 8-22 */
+     case 0x3:
+             /* BOOT_CFG4[2:0] */
+             switch ((soc_sbmr & 0x07000000) >> 24) {
+             case 0x0 ... 0x4:
+                     boot_dev = MX6_SPI_NOR_BOOT;
+                     break;
+             case 0x5 ... 0x7:
+                     boot_dev = MX6_I2C_BOOT;
+                     break;
+             }
+             break;
+     /* SD/eSD: 8.5.3, Table 8-15  */
+     case 0x4:
+     case 0x5:
+             boot_dev = MX6_SD_BOOT;
+             break;
+     /* MMC/eMMC: 8.5.3 */
+     case 0x6:
+     case 0x7:
+             boot_dev = MX6_MMC_BOOT;

arch/arm/cpu/armv7/mx6/soc.c const struct boot_mode soc_boot_modes[] = { · line 359

Quoted 10 lines

arch/arm/cpu/armv7/mx6/soc.c +10 -0

+             break;
+     /* NAND Flash: 8.5.2 */
+     case 0x8 ... 0xf:
+             boot_dev = MX6_NAND_BOOT;
+             break;
+     default:
+             boot_dev = MX6_UNKNOWN_BOOT;
+             break;
+     }
+

The function can be used as well for MX5 SOCs. Move it into imx-common, and use constants without SOC names. Instead of MX6_NAND_BOOT, IMX_NAND_BOOT (or whatever you find makes sense..)

Quoted 35 lines

arch/arm/cpu/armv7/mx6/soc.c +4 -0

+     return boot_dev;
+}
+
+
 void s_init(void)
 {
      struct anatop_regs *anatop = (struct anatop_regs *)ANATOP_BASE_ADDR;

arch/arm/include/asm/arch-mx6/spl.h new file +21 -0

new file mode 100644
index 0000000..5611c71
@@ -0,0 +1,26 @@
+/*
+ * Copyright (C) 2013 TechNexion Ltd.
+ *
+ * Author: Richard Hu <[email protected]>
+ *
+ * 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.
+ */
+
+#ifndef __ASM_ARCH_SPL_H__
+#define __ASM_ARCH_SPL_H__
+
+#define BOOT_DEVICE_MMC1     0
+#define BOOT_DEVICE_MMC2     1
+#define BOOT_DEVICE_MMC2_2   2
+#define BOOT_DEVICE_NAND     3

arch/arm/include/asm/arch-mx6/spl.h line 25

Quoted 4 lines

arch/arm/include/asm/arch-mx6/spl.h new file +4 -0

+#define BOOT_DEVICE_SATA     4
+#define BOOT_DEVICE_USBETH   5
+#define BOOT_DEVICE_NONE     6
+

They are only defines and are not yet related to i.MX6 anymnore, because you have already mapped them. What about to move this defines into imx-common ?

Quoted 19 lines

arch/arm/include/asm/arch-mx6/spl.h new file +1 -0

+#endif       /* __ASM_ARCH_SPL_H__ */

arch/arm/include/asm/imx-common/boot_mode.h +9 -0

index de0205c..3686367 100644
@@ -17,4 +17,21 @@ struct boot_mode {
 void add_board_boot_modes(const struct boot_mode *p);
 void boot_mode_apply(unsigned cfg_val);
 extern const struct boot_mode soc_boot_modes[];
+
+/* boot devices */
+enum boot_device {
+     MX6_SD_BOOT,
+     MX6_MMC_BOOT,
+     MX6_NAND_BOOT,
+     MX6_SATA_BOOT,
+     MX6_WEIM_NOR_BOOT,
+     MX6_ONE_NAND_BOOT,

arch/arm/include/asm/imx-common/boot_mode.h struct boot_mode { · line 34

Quoted 6 lines

arch/arm/include/asm/imx-common/boot_mode.h +6 -0

+     MX6_PATA_BOOT,
+     MX6_I2C_BOOT,
+     MX6_SPI_NOR_BOOT,
+     MX6_UNKNOWN_BOOT,
+     MX6_BOOT_DEV_NUM = MX6_UNKNOWN_BOOT,
+};

See my remark before.

Quoted 4 lines

arch/arm/include/asm/imx-common/boot_mode.h +2 -0

+enum boot_device get_boot_device(void);
+
 #endif

Quoted 34 lines

Quoted 33 lines

arch/arm/cpu/armv7/mx6/soc.c +33 -0

+             boot_dev = MX6_SATA_BOOT;
+             break;
+     /* Serial ROM: See 8.5.5.1, Table 8-22 */
+     case 0x3:
+             /* BOOT_CFG4[2:0] */
+             switch ((soc_sbmr & 0x07000000) >> 24) {
+             case 0x0 ... 0x4:
+                     boot_dev = MX6_SPI_NOR_BOOT;
+                     break;
+             case 0x5 ... 0x7:
+                     boot_dev = MX6_I2C_BOOT;
+                     break;
+             }
+             break;
+     /* SD/eSD: 8.5.3, Table 8-15  */
+     case 0x4:
+     case 0x5:
+             boot_dev = MX6_SD_BOOT;
+             break;
+     /* MMC/eMMC: 8.5.3 */
+     case 0x6:
+     case 0x7:
+             boot_dev = MX6_MMC_BOOT;
+             break;
+     /* NAND Flash: 8.5.2 */
+     case 0x8 ... 0xf:
+             boot_dev = MX6_NAND_BOOT;
+             break;
+     default:
+             boot_dev = MX6_UNKNOWN_BOOT;
+             break;
+     }
+

Quoted 3 lines

The function can be used as well for MX5 SOCs. Move it into imx-common, and use constants without SOC names. Instead of MX6_NAND_BOOT, IMX_NAND_BOOT (or whatever you find makes sense..)

Are you saying the i.MX5's have the same register and meaning? I figured the sbmr is different.

Quoted 36 lines

Quoted 36 lines

arch/arm/cpu/armv7/mx6/soc.c +4 -0

+     return boot_dev;
+}
+
+
 void s_init(void)
 {
      struct anatop_regs *anatop = (struct anatop_regs *)ANATOP_BASE_ADDR;

arch/arm/include/asm/arch-mx6/spl.h new file +21 -0

new file mode 100644
index 0000000..5611c71
@@ -0,0 +1,26 @@
+/*
+ * Copyright (C) 2013 TechNexion Ltd.
+ *
+ * Author: Richard Hu <[email protected]>
+ *
+ * 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.
+ */
+
+#ifndef __ASM_ARCH_SPL_H__
+#define __ASM_ARCH_SPL_H__
+
+#define BOOT_DEVICE_MMC1     0
+#define BOOT_DEVICE_MMC2     1
+#define BOOT_DEVICE_MMC2_2   2
+#define BOOT_DEVICE_NAND     3

Quoted 8 lines

arch/arm/include/asm/arch-mx6/spl.h line 25

Quoted 4 lines

arch/arm/include/asm/arch-mx6/spl.h new file +4 -0

+#define BOOT_DEVICE_SATA     4
+#define BOOT_DEVICE_USBETH   5
+#define BOOT_DEVICE_NONE     6
+

They are only defines and are not yet related to i.MX6 anymnore, because you have already mapped them. What about to move this defines into imx-common ?

ok

I hope to have a v2 posted soon.

This can be used to pass info between the SPL and u-boot.

Signed-off-by Tim Harvey <[email protected]>

---
  arch/arm/include/asm/arch-mx6/sys_proto.h | 5 +++++
  1 file changed, 5 insertions(+)

arch/arm/include/asm/arch-mx6/sys_proto.h +0 -0

index 38851a1..f43e09c 100644

This can be used to pass info between the SPL and u-boot.

Signed-off-by Tim Harvey <[email protected]>

---
 arch/arm/include/asm/arch-mx6/sys_proto.h | 5 +++++
 1 file changed, 5 insertions(+)

arch/arm/include/asm/arch-mx6/sys_proto.h +0 -0

index 38851a1..f43e09c 100644

Hi Tim,

I see checking your patch that the MXS uses the same concept. And as far as I can see, boot_mode_idx is used only to print the boot devioce

However, we have not generally a concept to pass data between SPL and u-boot. My question is even if it is really needed. The size of DRAM is retrived at run time by u-boot running get_ram_size(), that is a better solution.

SPL is thought to generally load an image (of course, in most cases it is u-boot). In Falcon mode, the kernel is started without running u-boot, making this structure useless. Do we really need such a way (but then, it must be generalized as SPL API), or can we get rid of it ?

Quoted 4 lines · Stefano Babic

Hi Tim,

I see checking your patch that the MXS uses the same concept. And as far as I can see, boot_mode_idx is used only to print the boot devioce

Stefano,

yes, that is where I got the concept from.

I've been told this before, but I've found that get_ram_size() will hang on an i.MX6 if you pass it a maxsize larger than the memory in your system. Perhaps you can verify you see the same behavior?

Quoted 4 lines

SPL is thought to generally load an image (of course, in most cases it is u-boot). In Falcon mode, the kernel is started without running u-boot, making this structure useless. Do we really need such a way (but then, it must be generalized as SPL API), or can we get rid of it ?

As we have an EEPROM on the board that tells us the physical ram size, I use that to avoid the lockup.

Eventually I would like to read and
validate the entire EEPROM once in SPL and pass this to u-boot.img to avoid reading and validating it again. I think this is a good example of why sharing data between SPL and u-boot.img could be useful.

Hi Tim,

Quoted 9 lines · Tim Harvey

Quoted 4 lines · Stefano Babic

Hi Tim,

I see checking your patch that the MXS uses the same concept. And as far as I can see, boot_mode_idx is used only to print the boot devioce

Stefano,

yes, that is where I got the concept from.

Well, it slipped into mainline, but adding such a structure only to have a better print is not very useful...

Quoted 3 lines

I've been told this before, but I've found that get_ram_size() will hang on an i.MX6 if you pass it a maxsize larger than the memory in your system. Perhaps you can verify you see the same behavior?

get_ram_size() works if the memory is accessible, that means that no exception is generated when it tries to access. If the RAM controller is programmed for a larger amount of memory, because you can have board with different capacity of RAM, get_ram_size() finds dynamically the soldered value.

However, if the RAM controller is programmed for an amount of RAM smaller as what get_ram_size() tries to identify, an exception is raised. In get_ram_size(start, size) we pass the maximum amount of memory that the function is allowed to check. I can imagine we are running in this case. Checking on i.MX6, we have both boards checking the size with get_ram_size() as mx6sabresd, as well as boards fixing at compile time the value of the RAM (nitrogen6x).

Quoted 6 lines

Quoted 5 lines

SPL is thought to generally load an image (of course, in most cases it is u-boot). In Falcon mode, the kernel is started without running u-boot, making this structure useless. Do we really need such a way (but then, it must be generalized as SPL API), or can we get rid of it ?

Quoted 2 lines

As we have an EEPROM on the board that tells us the physical ram size, I use that to avoid the lockup.

It seems weird. There is several boards in mainline (I am sure about Freescale's PowerPC) discovering the mounted RAM. Of course, this is simpler if many parameters (row/columns/...) are the same, I am not sure about this.

Are we sure it is not possible here ? What does it happen with an inconsistent value in EEprom ?

Quoted 4 lines

Eventually I would like to read and
validate the entire EEPROM once in SPL and pass this to u-boot.img to avoid reading and validating it again. I think this is a good example of why sharing data between SPL and u-boot.img could be useful.

I am not sure, and I doubt it is a good idea to use persistent data to store that. It is potentially dangerous, and if some reasons the EEprom is changed, the board could not boot at all.

On the other side, I like the current concept that the SPL is simply a loader and can load images of different types, and fixing an API between SPL and U-Boot goes against this concept.

get_ram_size() works on cm-fx6 all DRAM configurations.

Quoted 10 lines

Quoted 9 lines

Quoted 6 lines

SPL is thought to generally load an image (of course, in most cases it is u-boot). In Falcon mode, the kernel is started without running u-boot, making this structure useless. Do we really need such a way (but then, it must be generalized as SPL API), or can we get rid of it ?

As we have an EEPROM on the board that tells us the physical ram size, I use that to avoid the lockup.

Quoted 4 lines

It seems weird. There is several boards in mainline (I am sure about Freescale's PowerPC) discovering the mounted RAM. Of course, this is simpler if many parameters (row/columns/...) are the same, I am not sure about this.

Even if the parameters are different, this should not result in lockup... AFAICS, lockup of that type has nothing to do with the DRAM itself, but the controller configuration.

Quoted 9 lines

Are we sure it is not possible here ? What does it happen with an inconsistent value in EEprom ?

Quoted 4 lines

Eventually I would like to read and
validate the entire EEPROM once in SPL and pass this to u-boot.img to avoid reading and validating it again. I think this is a good example of why sharing data between SPL and u-boot.img could be useful.

Quoted 3 lines

I am not sure, and I doubt it is a good idea to use persistent data to store that. It is potentially dangerous, and if some reasons the EEprom is changed, the board could not boot at all.

This is more a question of design and definition of what purpose the eeprom should serve, so I would let the vendor decide if he wants to depend on eeprom or not.

Hi Igor, hi Tim

Quoted 2 lines · Igor Grinberg

get_ram_size() works on cm-fx6 all DRAM configurations.

As on most boards in mainline ;-)

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

Quoted 6 lines

SPL is thought to generally load an image (of course, in most cases it is u-boot). In Falcon mode, the kernel is started without running u-boot, making this structure useless. Do we really need such a way (but then, it must be generalized as SPL API), or can we get rid of it ?

As we have an EEPROM on the board that tells us the physical ram size, I use that to avoid the lockup.

Quoted 8 lines

Quoted 4 lines

It seems weird. There is several boards in mainline (I am sure about Freescale's PowerPC) discovering the mounted RAM. Of course, this is simpler if many parameters (row/columns/...) are the same, I am not sure about this.

Even if the parameters are different, this should not result in lockup... AFAICS, lockup of that type has nothing to do with the DRAM itself, but the controller configuration.

Exactly, this is what I meant. Thanks for clarifying it better.

Quoted 9 lines

Quoted 9 lines

Are we sure it is not possible here ? What does it happen with an inconsistent value in EEprom ?

Quoted 4 lines

Eventually I would like to read and
validate the entire EEPROM once in SPL and pass this to u-boot.img to avoid reading and validating it again. I think this is a good example of why sharing data between SPL and u-boot.img could be useful.

Quoted 7 lines

Quoted 3 lines

I am not sure, and I doubt it is a good idea to use persistent data to store that. It is potentially dangerous, and if some reasons the EEprom is changed, the board could not boot at all.

This is more a question of design and definition of what purpose the eeprom should serve, so I would let the vendor decide if he wants to depend on eeprom or not.

Agree. Parsing and evaluating vendor specific information can be done inside board specific part, as now in read_eeprom inside gw_ventana.c.

Anyway, if there is a way to detect at runtime the hardware configuration, it remains a better way as to store the size into the Eeprom.

Quoted 7 lines · Stefano Babic

Hi Igor, hi Tim

Quoted 2 lines · Igor Grinberg

get_ram_size() works on cm-fx6 all DRAM configurations.

Quoted 2 lines

As on most boards in mainline ;-)

It looks like I mis-interpreted the failure. This issue is that get_ram_size() only works with powers of 2 and maxes out at 2GB due to 32bit addressing. I was simply passing in a bogus value. I'm not sure how you would deal with detecting systems with 4GB but I don't need to support that for my boards currently.

Quoted 11 lines · Tim Harvey

Quoted 9 lines · Stefano Babic

Hi Igor, hi Tim

Quoted 2 lines · Igor Grinberg

get_ram_size() works on cm-fx6 all DRAM configurations.

As on most boards in mainline ;-)

Quoted 6 lines

It looks like I mis-interpreted the failure. This issue is that get_ram_size() only works with powers of 2 and maxes out at 2GB due to 32bit addressing. I was simply passing in a bogus value. I'm not sure how you would deal with detecting systems with 4GB but I don't need to support that for my boards currently.

cm-fx6 supports 4GB and detects it properly.

Quoted 19 lines · Igor Grinberg

Quoted 17 lines · Tim Harvey

Quoted 9 lines · Stefano Babic

Hi Igor, hi Tim

Quoted 2 lines · Igor Grinberg

get_ram_size() works on cm-fx6 all DRAM configurations.

As on most boards in mainline ;-)

It looks like I mis-interpreted the failure. This issue is that get_ram_size() only works with powers of 2 and maxes out at 2GB due to 32bit addressing. I was simply passing in a bogus value. I'm not sure how you would deal with detecting systems with 4GB but I don't need to support that for my boards currently.

Quoted 1 line

cm-fx6 supports 4GB and detects it properly.

Igor,

That's interesting. What value are you passing get_ram_size() then?

Where is your code? I don't see any cm-fx6 in mainline or posted patchsets.

This is an attempt at using a macro to allow mx6dl-ddr.h and mx6q-ddr.h registers to be used together which is needed for an SPL bootloader that can run on either CPU's and must configure MMDC iomux dynamically.

I am trying to come up with a solution similar to Eric's approach with the similar issue regarding IMX pinmux but this approach is broken in that imximage will choke on the cfgtmp file due to the fact that the pre-processor won't use the enum's as it did the #defines. I'm looking for some positive suggestions here or perhaps someone else can come up with a solution for this particular issue which I haven't been able to resolve.

Hi Tim,

Quoted 6 lines · Tim Harvey

This is an attempt at using a macro to allow mx6dl-ddr.h and mx6q-ddr.h registers to be used together which is needed for an SPL bootloader
that can run on either CPU's and must configure MMDC iomux dynamically.

Quoted 8 lines

I am trying to come up with a solution similar to Eric's approach with the similar issue regarding IMX pinmux but this approach is broken in that imximage
will choke on the cfgtmp file due to the fact that the pre-processor won't use the enum's as it did the #defines. I'm looking for some positive suggestions here or perhaps someone else can come up with a solution for this
particular issue which I haven't been able to resolve.

Why can't you just rename the register name #defines without enclosing them in an anonymous enum? Then they could coexist and will be usable by imximage.

--
Regards,
Nikita.

Quoted 20 lines · Nikita Kiryanov

Hi Tim,

Quoted 6 lines · Tim Harvey

This is an attempt at using a macro to allow mx6dl-ddr.h and mx6q-ddr.h registers to be used together which is needed for an SPL bootloader
that can run on either CPU's and must configure MMDC iomux dynamically.

Quoted 8 lines

I am trying to come up with a solution similar to Eric's approach with the similar issue regarding IMX pinmux but this approach is broken in that imximage
will choke on the cfgtmp file due to the fact that the pre-processor won't use the enum's as it did the #defines. I'm looking for some positive suggestions here or perhaps someone else can come up with a solution for this
particular issue which I haven't been able to resolve.

Quoted 7 lines

Why can't you just rename the register name #defines without enclosing them in an anonymous enum? Then they could coexist and will be usable by imximage.

--
Regards,
Nikita.

Nikita,

The cfg files are currently all written to use the IOMUX register names as MX6_ (no Q vs DL) so that a single cfg file can be used for a build-time configuration of IMX6Q or IMX6DL.

Regards,

Tim

Quoted 33 lines · Tim Harvey

Quoted 23 lines · Nikita Kiryanov

Hi Tim,

Quoted 6 lines · Tim Harvey

This is an attempt at using a macro to allow mx6dl-ddr.h and mx6q-ddr.h registers to be used together which is needed for an SPL bootloader
that can run on either CPU's and must configure MMDC iomux dynamically.

Quoted 11 lines

I am trying to come up with a solution similar to Eric's approach with the
similar issue regarding IMX pinmux but this approach is broken in that imximage
will choke on the cfgtmp file due to the fact that the pre-processor won't
use the enum's as it did the #defines. I'm looking for some positive suggestions here or perhaps someone else can come up with a solution for
this
particular issue which I haven't been able to resolve.

Quoted 7 lines

Why can't you just rename the register name #defines without enclosing them in an anonymous enum? Then they could coexist and will be usable by imximage.

--
Regards,
Nikita.

Quoted 5 lines

Nikita,

The cfg files are currently all written to use the IOMUX register names as MX6_ (no Q vs DL) so that a single cfg file can be used for a build-time configuration of IMX6Q or IMX6DL.

OK now I understand. It seems to me that you only have this problem because you are using these address #defines as values for mx6_mmdc_ioregs structs to define a register mapping. You can also define this mapping by using a struct template that matches the register layout and a base address, both of which change between CPU types.

You can find an example of this in the Wandboard SPL implementation.

It's not in mainline ATM so you'll have to download their BSP. The template + base addr method also doesn't use up additional memory to define this layout, which is a point in its favor.

Quoted 5 lines

Regards,

Tim

On 04/10/2014 05:08 PM, Nikita Kiryanov wrote:

Here I'm referring to the file arch/arm/include/asm/arch-mx6/mx6_ddr_regs.h (which I'm guessing you may have already seen based on your comment on patch 11...)

Quoted 3 lines

The cfg files are currently all written to use the IOMUX register names as MX6_ (no Q vs DL) so that a single cfg file can be used for a build-time configuration of IMX6Q or IMX6DL.

OK now I understand. It seems to me that you only have this problem because you are using these address #defines as values for mx6_mmdc_ioregs structs to define a register mapping. You can also define this mapping by using a struct template that matches the register layout and a base address, both of which change between CPU types.

You can find an example of this in the Wandboard SPL implementation. It's not in mainline ATM so you'll have to download their BSP. The template + base addr method also doesn't use up additional memory to define this layout, which is a point in its favor.

Hi Tim, hi Nikita,

Quoted 11 lines · Nikita Kiryanov

Quoted 3 lines

The cfg files are currently all written to use the IOMUX register names as MX6_ (no Q vs DL) so that a single cfg file can be used for a build-time configuration of IMX6Q or IMX6DL.

OK now I understand. It seems to me that you only have this problem because you are using these address #defines as values for mx6_mmdc_ioregs structs to define a register mapping. You can also define this mapping by using a struct template that matches the register layout and a base address, both of which change between CPU types.

Reason to do in this way is the thought that the layout among the processsors can be completely different, thing that for other area is also true.

This was discussed also in a previous RFC by Eric:

	http://lists.denx.de/pipermail/u-boot/2013-November/166665.html

Tim, what about to repost in your patchset Eric's patch ? It completes your patchset and add documentation that is now missing (added Eric in CC if he will complain about this..)

However, if we can recognize the same layout for all three variations (I have only check some registers in 6Q and 6DL layout), using the same structure with a different base address can be even more readable. I admit I have not checked deeply, but it seems at first glance that mx6dq_ctrl and mx6sdl_ctrl in 7/11 share the same layout.

Quoted 4 lines

You can find an example of this in the Wandboard SPL implementation. It's not in mainline ATM so you'll have to download their BSP. The template + base addr method also doesn't use up additional memory to define this layout, which is a point in its favor.

If the layout is exactly the same, you could make the structure available to all imx6 flavors.

Hi Stefano,

Quoted 20 lines · Stefano Babic

Hi Tim, hi Nikita,

Quoted 11 lines · Nikita Kiryanov

Quoted 3 lines

The cfg files are currently all written to use the IOMUX register names as MX6_ (no Q vs DL) so that a single cfg file can be used for a build-time configuration of IMX6Q or IMX6DL.

OK now I understand. It seems to me that you only have this problem because you are using these address #defines as values for mx6_mmdc_ioregs structs to define a register mapping. You can also define this mapping by using a struct template that matches the register layout and a base address, both of which change between CPU types.

Reason to do in this way is the thought that the layout among the processsors can be completely different, thing that for other area is also true.

Quoted 8 lines

This was discussed also in a previous RFC by Eric:

    http://lists.denx.de/pipermail/u-boot/2013-November/166665.html

Tim, what about to repost in your patchset Eric's patch ? It completes your patchset and add documentation that is now missing (added Eric in CC if he will complain about this..)

I have no problem with this, but when I did some follow-on work to bring up SPL on Nitrogen6x boards, I recall finding some bugs in that patch set.

I didn't follow up with updates because I ran into some other snags. In particular, our use of SPI-NOR makes it crucial that we be able to download a secondary U-Boot over USB.

Quoted 6 lines

However, if we can recognize the same layout for all three variations (I have only check some registers in 6Q and 6DL layout), using the same structure with a different base address can be even more readable. I admit I have not checked deeply, but it seems at first glance that mx6dq_ctrl and mx6sdl_ctrl in 7/11 share the same layout.

There's a lot of commonality, but a quick diff of arch-mx6/mx6q-ddr.h and arch-mx6/mx6dl-ddr.h will show the problem areas.

Quoted 5 lines

Quoted 4 lines

You can find an example of this in the Wandboard SPL implementation. It's not in mainline ATM so you'll have to download their BSP. The template + base addr method also doesn't use up additional memory to define this layout, which is a point in its favor.

Quoted 3 lines

If the layout is exactly the same, you could make the structure available to all imx6 flavors.

Unfortunately, I don't believe it is.

Hi Tim, hi Nikita,

Quoted 11 lines · Nikita Kiryanov

Quoted 3 lines

The cfg files are currently all written to use the IOMUX register names as MX6_ (no Q vs DL) so that a single cfg file can be used for a build-time configuration of IMX6Q or IMX6DL.

OK now I understand. It seems to me that you only have this problem because you are using these address #defines as values for mx6_mmdc_ioregs structs to define a register mapping. You can also define this mapping by using a struct template that matches the register layout and a base address, both of which change between CPU types.

Reason to do in this way is the thought that the layout among the processsors can be completely different, thing that for other area is also true. This was discussed also in a previous RFC by Eric:

        http://lists.denx.de/pipermail/u-boot/2013-November/166665.html

Tim, what about to repost in your patchset Eric's patch ? It completes your patchset and add documentation that is now missing (added Eric in CC if he will complain about this..)

However, if we can recognize the same layout for all three variations (I have only check some registers in 6Q and 6DL layout), using the same structure with a different base address can be even more readable. I admit I have not checked deeply, but it seems at first glance that mx6dq_ctrl and mx6sdl_ctrl in 7/11 share the same layout.

Quoted 4 lines

You can find an example of this in the Wandboard SPL implementation. It's not in mainline ATM so you'll have to download their BSP. The template + base addr method also doesn't use up additional memory to define this layout, which is a point in its favor.

If the layout is exactly the same, you could make the structure available to all imx6 flavors.

Best regards,
Stefano Babic

Quoted 10 lines · Nikita Kiryanov

Hi Tim,

Quoted 6 lines · Tim Harvey

This is an attempt at using a macro to allow mx6dl-ddr.h and mx6q-ddr.h registers to be used together which is needed for an SPL bootloader
that can run on either CPU's and must configure MMDC iomux dynamically.

Quoted 9 lines

Quoted 8 lines

I am trying to come up with a solution similar to Eric's approach with the similar issue regarding IMX pinmux but this approach is broken in that imximage
will choke on the cfgtmp file due to the fact that the pre-processor won't use the enum's as it did the #defines. I'm looking for some positive suggestions here or perhaps someone else can come up with a solution for this
particular issue which I haven't been able to resolve.

Add new function that can take an array of iomux configs, an index, and a stride to allow a multi-dimentional array of pinmux values to be used to define pinmux values per cpu-type.

This takes a different approach to previously proposed solutions which used multiple arrays of pad lists. The goal is to eliminate having these multiple arrays such as 'mx6q_uart1_pads' and 'mx6dl_uart1_pads' which are almost identical copies of each other except for the MX6Q/MX6DL prefix on the PAD.

Signed-off-by Tim Harvey <[email protected]>

---
  arch/arm/imx-common/iomux-v3.c             | 19 +++++++++++++++----
  arch/arm/include/asm/imx-common/iomux-v3.h |  5 ++++-
  2 files changed, 19 insertions(+), 5 deletions(-)

arch/arm/imx-common/iomux-v3.c +15 -4

index b59b802..d3e1e30 100644
@@ -46,12 +46,23 @@ void imx_iomux_v3_setup_pad(iomux_v3_cfg_t pad)
  #endif
  }
 
-void imx_iomux_v3_setup_multiple_pads(iomux_v3_cfg_t const *pad_list,
-				      unsigned count)
+/* configures a list of pads within an array of lists */
+void imx_iomux_v3_setup_multiple_pads_array(iomux_v3_cfg_t const *pad_list,
+					    unsigned count, unsigned list,
+					    unsigned stride)
  {
  	iomux_v3_cfg_t const *p = pad_list;
  	int i;
 
-	for (i = 0; i < count; i++)
-		imx_iomux_v3_setup_pad(*p++);
+	p += list;
+	for (i = 0; i < count; i++) {
+		imx_iomux_v3_setup_pad(*p);
+		p += stride;
+	}
+}
+
+void imx_iomux_v3_setup_multiple_pads(iomux_v3_cfg_t const *pad_list,
+				      unsigned count)
+{
+	imx_iomux_v3_setup_multiple_pads_array(pad_list, count, 0, 1);
  }

arch/arm/include/asm/imx-common/iomux-v3.h +0 -0

index dec11a1..c8cc45b 100644

arch/arm/include/asm/imx-common/iomux-v3.h +0 -0

  #endif	/* __MACH_IOMUX_V3_H__*/
 

Hi Tim,

Quoted 5 lines · Tim Harvey

Add new function that can take an array of iomux configs, an index, and a stride to allow a multi-dimentional array of pinmux values to be used to define pinmux values per cpu-type.

Quoted 7 lines

This takes a different approach to previously proposed solutions which used
multiple arrays of pad lists. The goal is to eliminate having these multiple
arrays such as 'mx6q_uart1_pads' and 'mx6dl_uart1_pads' which are almost identical copies of each other except for the MX6Q/MX6DL prefix on the PAD.

I like this approach, but I think you should also define the IOMUX, SETUP_PAD, and SETUP_PADS macros from patch 10 in this file, as they (macros and function) are clearly meant to be used together.

Quoted 50 lines

Signed-off-by Tim Harvey <[email protected]>

---
  arch/arm/imx-common/iomux-v3.c             | 19 +++++++++++++++----
  arch/arm/include/asm/imx-common/iomux-v3.h |  5 ++++-
  2 files changed, 19 insertions(+), 5 deletions(-)

arch/arm/imx-common/iomux-v3.c +15 -4

index b59b802..d3e1e30 100644
@@ -46,12 +46,23 @@ void imx_iomux_v3_setup_pad(iomux_v3_cfg_t pad)
  #endif
  }
 
-void imx_iomux_v3_setup_multiple_pads(iomux_v3_cfg_t const *pad_list,
-                                     unsigned count)
+/* configures a list of pads within an array of lists */
+void imx_iomux_v3_setup_multiple_pads_array(iomux_v3_cfg_t const
 *pad_list,
+                                           unsigned count, unsigned list,
+                                           unsigned stride)
  {
        iomux_v3_cfg_t const *p = pad_list;
        int i;
 
-       for (i = 0; i < count; i++)
-               imx_iomux_v3_setup_pad(*p++);
+       p += list;
+       for (i = 0; i < count; i++) {
+               imx_iomux_v3_setup_pad(*p);
+               p += stride;
+       }
+}
+
+void imx_iomux_v3_setup_multiple_pads(iomux_v3_cfg_t const *pad_list,
+                                     unsigned count)
+{
+       imx_iomux_v3_setup_multiple_pads_array(pad_list, count, 0, 1);

  }

arch/arm/include/asm/imx-common/iomux-v3.h +0 -0

index dec11a1..c8cc45b 100644
@@ -167,7 +167,10 @@ typedef u64 iomux_v3_cfg_t;
  #define GPIO_PORTF            (5 << GPIO_PORT_SHIFT)
 

arch/arm/include/asm/imx-common/iomux-v3.h typedef u64 iomux_v3_cfg_t; · line 174

Quoted 8 lines

arch/arm/include/asm/imx-common/iomux-v3.h +4 -1

  void imx_iomux_v3_setup_pad(iomux_v3_cfg_t pad);
+void imx_iomux_v3_setup_multiple_pads_array(iomux_v3_cfg_t const
 *pad_list,
+                                           unsigned count, unsigned list,
+                                           unsigned stride);
  void imx_iomux_v3_setup_multiple_pads(iomux_v3_cfg_t const *pad_list,
-                                    unsigned count);
+                                     unsigned count);

The above change is a cleanup and should not be in this patch.

arch/arm/include/asm/imx-common/iomux-v3.h typedef u64 iomux_v3_cfg_t; · line 176

Quoted 3 lines

arch/arm/include/asm/imx-common/iomux-v3.h +0 -0

  #endif        /* __MACH_IOMUX_V3_H__*/

--
Regards,
Nikita.

Quoted 18 lines · Nikita Kiryanov

Hi Tim,

Quoted 12 lines · Tim Harvey

Add new function that can take an array of iomux configs, an index, and a stride to allow a multi-dimentional array of pinmux values to be used to define pinmux values per cpu-type.

This takes a different approach to previously proposed solutions which used
multiple arrays of pad lists. The goal is to eliminate having these multiple
arrays such as 'mx6q_uart1_pads' and 'mx6dl_uart1_pads' which are almost identical copies of each other except for the MX6Q/MX6DL prefix on the PAD.

Quoted 4 lines

I like this approach, but I think you should also define the IOMUX, SETUP_PAD, and SETUP_PADS macros from patch 10 in this file, as they (macros and function) are clearly meant to be used together.

I agree with this. Do the macro names IOMUX, SETUP_PAD, SETUP_PADS make sense?

Quoted 24 lines · Tim Harvey

Quoted 21 lines · Nikita Kiryanov

Hi Tim,

Quoted 13 lines · Tim Harvey

Add new function that can take an array of iomux configs, an index, and a stride to allow a multi-dimentional array of pinmux values to be used to define pinmux values per cpu-type.

This takes a different approach to previously proposed solutions which used
multiple arrays of pad lists. The goal is to eliminate having these multiple
arrays such as 'mx6q_uart1_pads' and 'mx6dl_uart1_pads' which are almost identical copies of each other except for the MX6Q/MX6DL prefix on the PAD.

Quoted 8 lines

Quoted 4 lines

I like this approach, but I think you should also define the IOMUX, SETUP_PAD, and SETUP_PADS macros from patch 10 in this file, as they (macros and function) are clearly meant to be used together.

I agree with this. Do the macro names IOMUX, SETUP_PAD, SETUP_PADS make sense?

My suggestion would be MX6QDL_DDR_IOMUX, MX6QDL_DDR_SETUP_PADS, and MX6QDL_SETUP_PAD (this last one is not DDR specific).

--
Regards,
Nikita.

use the new iomux function and a macro to create a multi-dimensional array of iomux values without duplicating the defintions.

Signed-off-by Tim Harvey <[email protected]>

---
 board/gateworks/gw_ventana/gw_ventana.c | 497 ++++++++++++++++++++------------
 1 file changed, 316 insertions(+), 181 deletions(-)

board/gateworks/gw_ventana/gw_ventana.c +0 -0

index 2113740..ebf7e7d 100644

board/gateworks/gw_ventana/gw_ventana.c +8 -4

+#define SETUP_PADS(x) \
+	imx_iomux_v3_setup_multiple_pads_array(x, \
+	ARRAY_SIZE(x)/2, is_cpu_type(MXC_CPU_MX6Q) ? 0 : 1, 2)
+
 /* GPIO's common to all baseboards */
 #define GP_PHY_RST	IMX_GPIO_NR(1, 30)
 #define GP_USB_OTG_PWR	IMX_GPIO_NR(3, 22)

@@ -94,109 +105,145 @@ int board_type;
 
 /* UART1: Function varies per baseboard */
 iomux_v3_cfg_t const uart1_pads[] = {
-	MX6_PAD_SD3_DAT6__UART1_RX_DATA | MUX_PAD_CTRL(UART_PAD_CTRL),
-	MX6_PAD_SD3_DAT7__UART1_TX_DATA | MUX_PAD_CTRL(UART_PAD_CTRL),
+	IOMUX(PAD_SD3_DAT6__UART1_RX_DATA | MUX_PAD_CTRL(UART_PAD_CTRL)),
+	IOMUX(PAD_SD3_DAT7__UART1_TX_DATA | MUX_PAD_CTRL(UART_PAD_CTRL)),
 };
 
 /* UART2: Serial Console */
 iomux_v3_cfg_t const uart2_pads[] = {
-	MX6_PAD_SD4_DAT7__UART2_TX_DATA | MUX_PAD_CTRL(UART_PAD_CTRL),
-	MX6_PAD_SD4_DAT4__UART2_RX_DATA | MUX_PAD_CTRL(UART_PAD_CTRL),
+	IOMUX(PAD_SD4_DAT7__UART2_TX_DATA | MUX_PAD_CTRL(UART_PAD_CTRL)),
+	IOMUX(PAD_SD4_DAT4__UART2_RX_DATA | MUX_PAD_CTRL(UART_PAD_CTRL)),
 };