MX6: SPL NAND support

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

+static void mxs_nand_command(struct mtd_info *mtd, unsigned int command,
+			     int column, int page_addr)
+{
+	register struct nand_chip *chip = mtd->priv;
+	int ctrl = NAND_NCE | NAND_CTRL_CLE | NAND_CTRL_CHANGE;

Quoted 5 lines

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

+static void mxs_nand_command(struct mtd_info *mtd, unsigned int command,
+                          int column, int page_addr)
+{
+     register struct nand_chip *chip = mtd->priv;
+     int ctrl = NAND_NCE | NAND_CTRL_CLE | NAND_CTRL_CHANGE;

drivers/mtd/nand/mxs_nand_spl.c line 28

Quoted 10 lines

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

+     u32 timeo, time_start;
+
+     /* write out the command to the device */
+     chip->cmd_ctrl(mtd, command, ctrl);
+
+     /* Address cycle, when necessary */
+     ctrl = NAND_NCE | NAND_CTRL_ALE | NAND_CTRL_CHANGE;
+     /* Serially input address */
+     if (column != -1) {
+             ctrl = NAND_CTRL_ALE | NAND_CTRL_CHANGE;

What happened to NAND_NCE?

It looks like the cmd_ctrl in nand_mxs.c doesn't care about NAND_MCE at all, so there's no functional impact, but either use it consistently or don't use it at all.

drivers/mtd/nand/mxs_nand_spl.c line 32

Quoted 4 lines

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

+             chip->cmd_ctrl(mtd, column, ctrl);
+             ctrl &= ~NAND_CTRL_CHANGE;
+             chip->cmd_ctrl(mtd, column >> 8, ctrl);
+     }

Why not pass the ctrl flags directly to cmd_ctrl as you do in most of the rest of the function? nand_mxs.c doesn't even look at NAND_CTRL_CHANGE.

drivers/mtd/nand/mxs_nand_spl.c line 184

Quoted 4 lines

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

+int nand_spl_load_image(uint32_t offs, unsigned int size, void *buf)
+{
+     register struct nand_chip *chip;

Why "register"?

drivers/mtd/nand/mxs_nand_spl.c line 219

Quoted 11 lines

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

+             if (!(page % nand_page_per_block)) {
+                     /*
+                      * Yes, new block. See if this block is good. If not,
+                      * loop until we find a good block.
+                      */
+                     while (is_badblock(&mtd, offs, 1)) {
+                             page = page + nand_page_per_block;
+                             /* Check i we've reached the end of flash. */
+                             if (page >= mtd.size >> chip->page_shift)
+                                     return -1;

Why -1 rather than a real error value?

-Scott

Add a common header which can hopefully be shared amon imx6 SPL users

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

---
v2:
- adjust CONFIG_SPL_TEXT_BASE, CONFIG_SPL_STACK and CONFIG_SPL_MAX_SIZE to accomodate the IMX6SOLO/DUALLITE which have half the iRAM of the IMX6DUAL/IMX6QUAD

---
  include/configs/imx6_spl.h | 71 ++++++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 71 insertions(+)

create mode 100644 include/configs/imx6_spl.h

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

new file mode 100644
index 0000000..20078cc
@@ -0,0 +1,71 @@
+/*
+ * Copyright (C) 2014 Gateworks Corporation
+ * Author: Tim Harvey <[email protected]>
+ *
+ * SPDX-License-Identifier:     GPL-2.0+

Hi Tim,

Quoted 24 lines · Tim Harvey

Add a common header which can hopefully be shared amon imx6 SPL users

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

---
v2:
- adjust CONFIG_SPL_TEXT_BASE, CONFIG_SPL_STACK and CONFIG_SPL_MAX_SIZE to accomodate the IMX6SOLO/DUALLITE which have half the iRAM of the IMX6DUAL/IMX6QUAD

---
  include/configs/imx6_spl.h | 71 ++++++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 71 insertions(+)

create mode 100644 include/configs/imx6_spl.h

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

new file mode 100644
index 0000000..20078cc
@@ -0,0 +1,71 @@
+/*
+ * Copyright (C) 2014 Gateworks Corporation
+ * Author: Tim Harvey <[email protected]>
+ *
+ * SPDX-License-Identifier:     GPL-2.0+

include/configs/imx6_spl.h line 15

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
+
+/*
+ * see Figure 8-3 in IMX6DQ/IMX6SDL Reference manuals:

s/IMX6SLD/IMX6SDL/g

include/configs/imx6_spl.h line 17

Quoted 2 lines

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

+ *  - IMX6 OCRAM (IRAM) is from 0x00907000 to 0x0091FFFF (IMX6SLD)
+ *  - IMX6DQ has 2x IRAM of IMX6SLD but we intend to support IMX6SLD as well

... I was about to complain about the use of IMX6SLD (the nomenclature i.MX6DL/S seems more common), but the RM __is__ named SDL...

Hi,

Quoted 40 lines · Eric Nelson

Hi Tim,

Quoted 24 lines · Tim Harvey

Add a common header which can hopefully be shared amon imx6 SPL users

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

---
v2:
- adjust CONFIG_SPL_TEXT_BASE, CONFIG_SPL_STACK and CONFIG_SPL_MAX_SIZE to accomodate the IMX6SOLO/DUALLITE which have half the iRAM of the IMX6DUAL/IMX6QUAD

---
  include/configs/imx6_spl.h | 71 ++++++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 71 insertions(+)

create mode 100644 include/configs/imx6_spl.h

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

new file mode 100644
index 0000000..20078cc
@@ -0,0 +1,71 @@
+/*
+ * Copyright (C) 2014 Gateworks Corporation
+ * Author: Tim Harvey <[email protected]>
+ *
+ * SPDX-License-Identifier:     GPL-2.0+

include/configs/imx6_spl.h line 15

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
+
+/*
+ * see Figure 8-3 in IMX6DQ/IMX6SDL Reference manuals:

s/IMX6SLD/IMX6SDL/g

Quoted 5 lines

include/configs/imx6_spl.h line 17

Quoted 2 lines

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

+ *  - IMX6 OCRAM (IRAM) is from 0x00907000 to 0x0091FFFF (IMX6SLD)
+ *  - IMX6DQ has 2x IRAM of IMX6SLD but we intend to support IMX6SLD as well

... I was about to complain about the use of IMX6SLD (the nomenclature i.MX6DL/S seems more common), but the RM __is__ named SDL...

SDL stands for Solo/Dual Lite, so SLD is plain wrong... (Solo Lite and Dual are so different...)

--
Regards,
Igor.

Add a common header which can hopefully be shared amon imx6 SPL users

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

---
v2:
- adjust CONFIG_SPL_TEXT_BASE, CONFIG_SPL_STACK and CONFIG_SPL_MAX_SIZE to accomodate the IMX6SOLO/DUALLITE which have half the iRAM of the IMX6DUAL/IMX6QUAD

---
 include/configs/imx6_spl.h | 71 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)
 create mode 100644 include/configs/imx6_spl.h

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

new file mode 100644
index 0000000..20078cc
@@ -0,0 +1,71 @@
+/*
+ * Copyright (C) 2014 Gateworks Corporation
+ * Author: Tim Harvey <[email protected]>
+ *
+ * SPDX-License-Identifier:     GPL-2.0+
+ */
+#ifndef __IMX6_SPL_CONFIG_H
+#define __IMX6_SPL_CONFIG_H
+
+#ifdef CONFIG_SPL
+
+#define CONFIG_SPL_FRAMEWORK
+
+/*
+ * see Figure 8-3 in IMX6DQ/IMX6SDL Reference manuals:
+ *  - IMX6 OCRAM (IRAM) is from 0x00907000 to 0x0091FFFF (IMX6SLD)
+ *  - IMX6DQ has 2x IRAM of IMX6SLD but we intend to support IMX6SLD as well
+ *  - BOOT ROM stack is at 0x0091FFB8
+ *  - if icache/dcache is enabled (eFuse/strapping controlled) then the
+ *    IMX BOOT ROM will setup MMU table at 0x00918000, therefore we need to
+ *    fit between 0x00907000 and 0x00918000.
+ *  - Additionally the BOOT ROM loads what they consider the firmware image
+ *    which consists of a 4K header in front of us that contains the IVT, DCD
+ *    and some padding thus 'our' max size is really 0x00908000 - 0x00918000
+ *    or 64KB
+ */
+#define CONFIG_SPL_LDSCRIPT	"arch/arm/cpu/armv7/omap-common/u-boot-spl.lds"
+#define CONFIG_SPL_TEXT_BASE		0x00908000
+#define CONFIG_SPL_MAX_SIZE		(64 * 1024)
+#define CONFIG_SPL_START_S_PATH		"arch/arm/cpu/armv7"
+#define CONFIG_SPL_STACK		0x0091FFB8
+#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_DMA_SUPPORT
+#endif
+
+/* MMC support */
+#if defined(CONFIG_SPL_MMC_SUPPORT)
+#define CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR	138 /* offset 69KB */
+#define CONFIG_SYS_U_BOOT_MAX_SIZE_SECTORS	800 /* 400 KB */
+#define CONFIG_SYS_MMC_SD_FAT_BOOT_PARTITION	1
+#define CONFIG_SYS_MONITOR_LEN  (CONFIG_SYS_U_BOOT_MAX_SIZE_SECTORS/2*1024)
+#endif
+
+/* SATA support */
+#if defined(CONFIG_SPL_SATA_SUPPORT)
+#define CONFIG_SPL_SATA_BOOT_DEVICE		0
+#define CONFIG_SYS_SATA_FAT_BOOT_PARTITION	1
+#endif
+
+/* Define the payload for FAT/EXT support */
+#if defined(CONFIG_SPL_FAT_SUPPORT) || defined(CONFIG_SPL_EXT_SUPPORT)
+#define CONFIG_SPL_FAT_LOAD_PAYLOAD_NAME  "u-boot.img"
+#define CONFIG_SPL_LIBDISK_SUPPORT

Add a common header which can hopefully be shared amon imx6 SPL users

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

---
v2:
- adjust CONFIG_SPL_TEXT_BASE, CONFIG_SPL_STACK and CONFIG_SPL_MAX_SIZE to accomodate the IMX6SOLO/DUALLITE which have half the iRAM of the IMX6DUAL/IMX6QUAD

From: Masahiro Yamada <[email protected]>

arch/arm/include/asm/spl.h requires all SoCs to have arch/arm/include/asm/arch-*/spl.h.

But many of them just define BOOT_DEVICE_* macros.

Those macros are used in the "switch (boot_device) { ... }" statement in common/spl/spl.c.

So they should not be archtecture specific, but be described as a simpile enumeration.

This commit merges most of arch/arm/include/asm/arch-*/spl.h into arch/arm/include/asm/spl.h.

With a little more effort, arch-zynq/spl.h and arch-socfpga/spl.h will be merged, while I am not sure about OMAP and Exynos.

Signed-off-by Masahiro Yamada <[email protected]>

Cc Tom Rini <[email protected]>

Cc Michal Simek <[email protected]>

Cc Andreas Biesmann <[email protected]>

Cc Stephen Warren <[email protected]>

Cc Tom Warren <[email protected]>

CC Stefano Babic <[email protected]>

CC Minkyu Kang <[email protected]>

Cc Dinh Nguyen <[email protected]>

Acked-by Andreas Biesmann <[email protected]>

Acked-by Michal Simek <[email protected]>

Acked-by Stefano Babic <[email protected]>

Acked-by Stephen Warren <[email protected]>

Acked-by Tim Harvey <[email protected]>

Tested-by Bo Shen <[email protected]> [on sama5d3xek board for at91 part]

Acked-by Stephen Warren <[email protected]>

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

Hi Tim,
Cc : Stefano, Tom

Quoted 28 lines · On Mon, 28 Apr 2014 13:17:30 -0700 Tim Harvey <[email protected]> wrote:

From: Masahiro Yamada <[email protected]>

arch/arm/include/asm/spl.h requires all SoCs to have arch/arm/include/asm/arch-*/spl.h.

But many of them just define BOOT_DEVICE_* macros.

Those macros are used in the "switch (boot_device) { ... }" statement in common/spl/spl.c.

So they should not be archtecture specific, but be described as a simpile enumeration.

This commit merges most of arch/arm/include/asm/arch-*/spl.h into arch/arm/include/asm/spl.h.

With a little more effort, arch-zynq/spl.h and arch-socfpga/spl.h will be merged, while I am not sure about OMAP and Exynos.

Signed-off-by Masahiro Yamada <[email protected]>

Cc Tom Rini <[email protected]>

Cc Michal Simek <[email protected]>

Cc Andreas Biesmann <[email protected]>

Cc Stephen Warren <[email protected]>

Cc Tom Warren <[email protected]>

CC Stefano Babic <[email protected]>

CC Minkyu Kang <[email protected]>

Cc Dinh Nguyen <[email protected]>

Quoted 8 lines

Acked-by Andreas Biesmann <[email protected]>

Acked-by Michal Simek <[email protected]>

Acked-by Stefano Babic <[email protected]>

Acked-by Stephen Warren <[email protected]>

Acked-by Tim Harvey <[email protected]>

Tested-by Bo Shen <[email protected]> [on sama5d3xek board for at91 part]

Acked-by Stephen Warren <[email protected]>

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

I understand my patch is here because your series uses it as a prerequisite.

But, please make sure this patch should be reviewed and applied by Tom Rini because this patch is relevant to various SoCs.

Add a common spl.c file to support boot device functions needed for SPL such as detecting the boot device.

<snip>

arch/arm/imx-common/spl.c new file +3 -0

new file mode 100644
index 0000000..5c1667c
@@ -0,0 +1,79 @@
 
<snip>
 
+u32 spl_boot_device(void)
+{
+	struct src *psrc = (struct src *)SRC_BASE_ADDR;

Add a common spl.c file to support boot device functions needed for SPL such as detecting the boot device.

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

---
v2:
- re-base on top of Masahiro Yamada's consolidation patch:
http://patchwork.ozlabs.org/patch/341817/
- moved spl_boot_device() and get_boot_mode() into own common imx spl.c file
- use mem-mapped structure for smbr reg access

---
 arch/arm/imx-common/Makefile |  1 +
 arch/arm/imx-common/spl.c    | 79 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 80 insertions(+)
 create mode 100644 arch/arm/imx-common/spl.c

arch/arm/imx-common/Makefile +1 -0

index b04dfbb..c97ea48 100644
@@ -16,6 +16,7 @@ obj-$(CONFIG_SYS_I2C_MXC) += i2c-mxv7.o
 endif
 ifeq ($(SOC),$(filter $(SOC),mx6 mxs))
 obj-y	+= misc.o
+obj-$(CONFIG_SPL_BUILD)	+= spl.o
 endif
 ifeq ($(SOC),$(filter $(SOC),mx6))
 obj-$(CONFIG_CMD_SATA) += sata.o

arch/arm/imx-common/spl.c new file +0 -0

new file mode 100644
index 0000000..5c1667c

arch/arm/imx-common/spl.c new file +62 -0

+#include <common.h>
+#include <asm/io.h>
+#include <asm/arch/imx-regs.h>
+#include <asm/spl.h>
+#include <spl.h>
+
+#if defined(CONFIG_MX6)
+/* determine boot device from SRC_SBMR1 register (BOOT_CFG[4:1]) */
+u32 spl_boot_device(void)
+{
+	struct src *psrc = (struct src *)SRC_BASE_ADDR;
+	unsigned reg = readl(&psrc->sbmr1);
+
+	/* BOOT_CFG1[7:4] - see IMX6DQRM Table 8-8 */
+	switch ((reg & 0x000000FF) >> 4) {
+	 /* EIM: See 8.5.1, Table 8-9 */
+	case 0x0:
+		/* BOOT_CFG1[3]: NOR/OneNAND Selection */
+		if ((reg & 0x00000008) >> 3)
+			return BOOT_DEVICE_ONENAND;
+		else
+			return BOOT_DEVICE_NOR;
+		break;
+	/* SATA: See 8.5.4, Table 8-20 */
+	case 0x2:
+		return BOOT_DEVICE_SATA;
+	/* Serial ROM: See 8.5.5.1, Table 8-22 */
+	case 0x3:
+		/* BOOT_CFG4[2:0] */
+		switch ((reg & 0x07000000) >> 24) {
+		case 0x0 ... 0x4:
+			return BOOT_DEVICE_SPI;
+		case 0x5 ... 0x7:
+			return BOOT_DEVICE_I2C;
+		}
+		break;
+	/* SD/eSD: 8.5.3, Table 8-15  */
+	case 0x4:
+	case 0x5:
+		return BOOT_DEVICE_MMC1;
+	/* MMC/eMMC: 8.5.3 */
+	case 0x6:
+	case 0x7:
+		return BOOT_DEVICE_MMC1;
+	/* NAND Flash: 8.5.2 */
+	case 0x8 ... 0xf:
+		return BOOT_DEVICE_NAND;
+	}
+	return BOOT_DEVICE_NONE;
+}
+#endif
+
+#if defined(CONFIG_SPL_MMC_SUPPORT)
+/* called from spl_mmc to see type of boot mode for storage (RAW or FAT) */
+u32 spl_boot_mode(void)
+{
+	switch (spl_boot_device()) {
+	/* for MMC return either RAW or FAT mode */
+	case BOOT_DEVICE_MMC1:
+	case BOOT_DEVICE_MMC2:
+#ifdef CONFIG_SPL_FAT_SUPPORT
+		return MMCSD_MODE_FAT;

Hi Tim,

Quoted 34 lines · Tim Harvey

Add a common spl.c file to support boot device functions needed for SPL such as detecting the boot device.

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

---
v2:
- re-base on top of Masahiro Yamada's consolidation patch:
http://patchwork.ozlabs.org/patch/341817/
- moved spl_boot_device() and get_boot_mode() into own common imx spl.c file
- use mem-mapped structure for smbr reg access

---
 arch/arm/imx-common/Makefile |  1 +
 arch/arm/imx-common/spl.c    | 79 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 80 insertions(+)
 create mode 100644 arch/arm/imx-common/spl.c

arch/arm/imx-common/Makefile +1 -0

index b04dfbb..c97ea48 100644
@@ -16,6 +16,7 @@ obj-$(CONFIG_SYS_I2C_MXC) += i2c-mxv7.o
 endif
 ifeq ($(SOC),$(filter $(SOC),mx6 mxs))
 obj-y        += misc.o
+obj-$(CONFIG_SPL_BUILD)      += spl.o
 endif
 ifeq ($(SOC),$(filter $(SOC),mx6))
 obj-$(CONFIG_CMD_SATA) += sata.o

arch/arm/imx-common/spl.c new file +0 -0

new file mode 100644
index 0000000..5c1667c

arch/arm/imx-common/spl.c line 7

Quoted 7 lines

arch/arm/imx-common/spl.c new file +7 -0

@@ -0,0 +1,79 @@
+/*
+ * Copyright (C) 2014 Gateworks Corporation
+ * Author: Tim Harvey <[email protected]>
+ *
+ * SPDX-License-Identifier:     GPL-2.0+
+ */
+

Just a little remark. Do you have written the function on your own or have you ported it from Freescale's 2009.08 ? Apart names, it looks similar.

If this is the case, you should also add Freescale's Copyright to the header.

Quoted 64 lines

arch/arm/imx-common/spl.c new file +62 -0

+#include <common.h>
+#include <asm/io.h>
+#include <asm/arch/imx-regs.h>
+#include <asm/spl.h>
+#include <spl.h>
+
+#if defined(CONFIG_MX6)
+/* determine boot device from SRC_SBMR1 register (BOOT_CFG[4:1]) */
+u32 spl_boot_device(void)
+{
+     struct src *psrc = (struct src *)SRC_BASE_ADDR;
+     unsigned reg = readl(&psrc->sbmr1);
+
+     /* BOOT_CFG1[7:4] - see IMX6DQRM Table 8-8 */
+     switch ((reg & 0x000000FF) >> 4) {
+      /* EIM: See 8.5.1, Table 8-9 */
+     case 0x0:
+             /* BOOT_CFG1[3]: NOR/OneNAND Selection */
+             if ((reg & 0x00000008) >> 3)
+                     return BOOT_DEVICE_ONENAND;
+             else
+                     return BOOT_DEVICE_NOR;
+             break;
+     /* SATA: See 8.5.4, Table 8-20 */
+     case 0x2:
+             return BOOT_DEVICE_SATA;
+     /* Serial ROM: See 8.5.5.1, Table 8-22 */
+     case 0x3:
+             /* BOOT_CFG4[2:0] */
+             switch ((reg & 0x07000000) >> 24) {
+             case 0x0 ... 0x4:
+                     return BOOT_DEVICE_SPI;
+             case 0x5 ... 0x7:
+                     return BOOT_DEVICE_I2C;
+             }
+             break;
+     /* SD/eSD: 8.5.3, Table 8-15  */
+     case 0x4:
+     case 0x5:
+             return BOOT_DEVICE_MMC1;
+     /* MMC/eMMC: 8.5.3 */
+     case 0x6:
+     case 0x7:
+             return BOOT_DEVICE_MMC1;
+     /* NAND Flash: 8.5.2 */
+     case 0x8 ... 0xf:
+             return BOOT_DEVICE_NAND;
+     }
+     return BOOT_DEVICE_NONE;
+}
+#endif
+
+#if defined(CONFIG_SPL_MMC_SUPPORT)
+/* called from spl_mmc to see type of boot mode for storage (RAW or FAT) */
+u32 spl_boot_mode(void)
+{
+     switch (spl_boot_device()) {
+     /* for MMC return either RAW or FAT mode */
+     case BOOT_DEVICE_MMC1:
+     case BOOT_DEVICE_MMC2:
+#ifdef CONFIG_SPL_FAT_SUPPORT
+             return MMCSD_MODE_FAT;

arch/arm/imx-common/spl.c line 79

Quoted 10 lines

arch/arm/imx-common/spl.c new file +10 -0

+#else
+             return MMCSD_MODE_RAW;
+#endif
+             break;
+     default:
+             puts("spl: ERROR:  unsupported device\n");
+             hang();
+     }
+}
+#endif

Best regards,
Stefano Babic

--
=====================================================================
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

Quoted 5 lines

Just a little remark. Do you have written the function on your own or have you ported it from Freescale's 2009.08 ? Apart names, it looks similar.

If this is the case, you should also add Freescale's Copyright to the header.

Hi Stefano,

I've never looked at Freescales 2009.08 code myself (I have an aversion to vendor specific forks of u-boot almost 5 years old heh), but its likely bits of the function came from there originally but my inspiration likely came from Edward Lin's patch a while back. A switch/case seems the logical approach given the bitfield from the IMX6 reference manual with several don't-cares spread around and I felt that was much easier to read than trying to convert it all the shifts and masks.

I can add additional copyright's if anyone feels its necessary - I've added Edward to the cc to see if he feels its necessary.

+cc Edward Lin

Quoted 2 lines · Stefano Babic

Hi Tim,

Quoted 14 lines

arch/arm/imx-common/spl.c line 7

Quoted 10 lines

arch/arm/imx-common/spl.c new file +7 -0

@@ -0,0 +1,79 @@
+/*
+ * Copyright (C) 2014 Gateworks Corporation
+ * Author: Tim Harvey <[email protected]>
+ *
+ * SPDX-License-Identifier:     GPL-2.0+
+ */
+

Just a little remark. Do you have written the function on your own or have you ported it from Freescale's 2009.08 ? Apart names, it looks similar.

Quoted 2 lines

If this is the case, you should also add Freescale's Copyright to the header.

Hi Stefano,

I've never looked at Freescales 2009.08 code myself (I have an aversion to vendor specific forks of u-boot almost 5 years old heh), but its likely bits of the function came from there originally but my inspiration likely came from Edward Lin's patch a while back. A switch/case seems the logical approach given the bitfield from the IMX6 reference manual with several don't-cares spread around and I felt that was much easier to read than trying to convert it all the shifts and masks.

I can add additional copyright's if anyone feels its necessary - I've added Edward to the cc to see if he feels its necessary.

Tim

Quoted 66 lines

Quoted 66 lines

arch/arm/imx-common/spl.c new file +65 -0

+#include <common.h>
+#include <asm/io.h>
+#include <asm/arch/imx-regs.h>
+#include <asm/spl.h>
+#include <spl.h>
+
+#if defined(CONFIG_MX6)
+/* determine boot device from SRC_SBMR1 register (BOOT_CFG[4:1]) */
+u32 spl_boot_device(void)
+{
+     struct src *psrc = (struct src *)SRC_BASE_ADDR;
+     unsigned reg = readl(&psrc->sbmr1);
+
+     /* BOOT_CFG1[7:4] - see IMX6DQRM Table 8-8 */
+     switch ((reg & 0x000000FF) >> 4) {
+      /* EIM: See 8.5.1, Table 8-9 */
+     case 0x0:
+             /* BOOT_CFG1[3]: NOR/OneNAND Selection */
+             if ((reg & 0x00000008) >> 3)
+                     return BOOT_DEVICE_ONENAND;
+             else
+                     return BOOT_DEVICE_NOR;
+             break;
+     /* SATA: See 8.5.4, Table 8-20 */
+     case 0x2:
+             return BOOT_DEVICE_SATA;
+     /* Serial ROM: See 8.5.5.1, Table 8-22 */
+     case 0x3:
+             /* BOOT_CFG4[2:0] */
+             switch ((reg & 0x07000000) >> 24) {
+             case 0x0 ... 0x4:
+                     return BOOT_DEVICE_SPI;
+             case 0x5 ... 0x7:
+                     return BOOT_DEVICE_I2C;
+             }
+             break;
+     /* SD/eSD: 8.5.3, Table 8-15  */
+     case 0x4:
+     case 0x5:
+             return BOOT_DEVICE_MMC1;
+     /* MMC/eMMC: 8.5.3 */
+     case 0x6:
+     case 0x7:
+             return BOOT_DEVICE_MMC1;
+     /* NAND Flash: 8.5.2 */
+     case 0x8 ... 0xf:
+             return BOOT_DEVICE_NAND;
+     }
+     return BOOT_DEVICE_NONE;
+}
+#endif
+
+#if defined(CONFIG_SPL_MMC_SUPPORT)
+/* called from spl_mmc to see type of boot mode for storage (RAW or FAT) */
+u32 spl_boot_mode(void)
+{
+     switch (spl_boot_device()) {
+     /* for MMC return either RAW or FAT mode */
+     case BOOT_DEVICE_MMC1:
+     case BOOT_DEVICE_MMC2:
+#ifdef CONFIG_SPL_FAT_SUPPORT
+             return MMCSD_MODE_FAT;
+#else
+             return MMCSD_MODE_RAW;
+#endif

Quoted 8 lines

arch/arm/imx-common/spl.c line 79

Quoted 8 lines

arch/arm/imx-common/spl.c new file +7 -0

+             break;
+     default:
+             puts("spl: ERROR:  unsupported device\n");
+             hang();
+     }
+}
+#endif

Tim, Stefano,

that piece of code indeed originates from Freescale's 2009.08 u-boot. (Edward has changed jobs, so the CC:d address is of no use.)

Hi Tim,

Quoted 3 lines · Tapani Utriainen

Tim, Stefano,

Quoted 2 lines

that piece of code indeed originates from Freescale's 2009.08 u-boot. (Edward has changed jobs, so the CC:d address is of no use.)

I supposed this. Tim, please add Freescale's Copyright, too.

Thanks,
Stefano Babic

Add comment block for the imx_ddr_size function and remove the extra unused fields from struct esd_mmdc_regs which are also not common between IMX53 and IMX6. There are no functional changes here.

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

---
 arch/arm/imx-common/cpu.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

arch/arm/imx-common/cpu.c +1 -9

index a77c4de..392b8d4 100644
@@ -58,6 +58,7 @@ char *get_reset_cause(void)
 static const unsigned char col_lookup[] = {9, 10, 11, 8, 12, 9, 9, 9};
 static const unsigned char bank_lookup[] = {3, 2};
 
+/* these MMDC registers are common to the IMX53 and IMX6 */
 struct esd_mmdc_regs {
 	uint32_t	ctl;
 	uint32_t	pdc;

@@ -66,15 +67,6 @@ struct esd_mmdc_regs {
 	uint32_t	cfg1;
 	uint32_t	cfg2;
 	uint32_t	misc;
-	uint32_t	scr;
-	uint32_t	ref;
-	uint32_t	rsvd1;
-	uint32_t	rsvd2;
-	uint32_t	rwd;
-	uint32_t	or;
-	uint32_t	mrr;
-	uint32_t	cfg3lp;
-	uint32_t	mr4;
 };
 
 #define ESD_MMDC_CTL_GET_ROW(mdctl)	((ctl >> 24) & 7)

Add memory-mapped structures for MMDC iomux and configuration. Note that while the MMDC configuration registers are common between the IMX6DQ (IMX6DUAL/IMX6QUAD) and IMX6SDL (IMX6SOLO/IMX6DUALLITE) types the iomux registers differ. This requires two sets of structures.

Add structures to describe DDR3 device information, system information (memory layout, etc), and MMDC calibration regitsers that can be used to configure the MMDC dynamically.

We define these structures for SPL builds instead of including mx6q-ddr.h and mx6dl-ddr.h which use the same namespace and are only useful for imximage cfg files.

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

---
v2:
- split out from original mmdc configuration patch
- only define for SPL build
- do not include mx6q-ddr.h and mx6dl-ddr.h for SPL build - these use the same namespace and are only useful for imximage cfg files

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

arch/arm/include/asm/arch-mx6/mx6-ddr.h +1 -0

index 43d377a..d544d2e 100644
@@ -6,6 +6,7 @@
  #ifndef __ASM_ARCH_MX6_DDR_H__
  #define __ASM_ARCH_MX6_DDR_H__
 
+#ifndef CONFIG_SPL_BUILD
  #ifdef CONFIG_MX6Q
  #include "mx6q-ddr.h"
  #else

arch/arm/include/asm/arch-mx6/mx6-ddr.h +48 -0

+
+/* Device Information: Varies per DDR3 part number and speed grade */
+struct mx6_ddr3_cfg {
+	u16 mem_speed;	/* ie 1600 for DDR3-1600 (800,1066,1333,1600) */
+	u8 density;	/* chip density (Gb) (1,2,4,8) */
+	u8 width;	/* bus width (bits) (4,8,16) */
+	u8 banks;	/* number of banks */
+	u8 rowaddr;	/* row address bits (11-16)*/
+	u8 coladdr;	/* col address bits (9-12) */
+	u8 pagesz;	/* page size (K) (1-2) */
+	u16 trcd;	/* tRCD=tRP=CL (ns*100) */
+	u16 trcmin;	/* tRC min (ns*100) */
+	u16 trasmin;	/* tRAS min (ns*100) */
+	u8 SRT;		/* self-refresh temperature: 0=normal, 1=extended */
+};
+
+/* System Information: Varies per board design, layout, and term choices */
+struct mx6_ddr_sysinfo {
+	u8 dsize;	/* size of bus (in dwords: 0=16bit,1=32bit,2=64bit) */
+	u8 cs_density;	/* density per chip select (Gb) */
+	u8 ncs;		/* number chip selects used (1|2) */
+	char cs1_mirror;/* enable address mirror (0|1) */
+	char bi_on;	/* Bank interleaving enable */
+	u8 rtt_nom;	/* Rtt_Nom (DDR3_RTT_*) */
+	u8 rtt_wr;	/* Rtt_Wr (DDR3_RTT_*) */
+	u8 ralat;	/* Read Additional Latency (0-7) */
+	u8 walat;	/* Write Additional Latency (0-3) */
+	u8 mif3_mode;	/* Command prediction working mode */
+	u8 rst_to_cke;	/* Time from SDE enable to CKE rise */
+	u8 sde_to_rst;	/* Time from SDE enable until DDR reset# is high */
+};
+
+/*
+ * Board specific calibration:
+ *   This includes write leveling calibration values as well as DQS gating
+ *   and read/write delays. These values are board/layout/device specific.
+ *   Freescale recommends using the i.MX6 DDR Stress Test Tool V1.0.2
+ *   (DOC-96412) to determine these values over a range of boards and
+ *   temperatures.
+ */
+struct mx6_mmdc_calibration {
+	/* write leveling calibration */
+	u32 p0_mpwldectrl0;
+	u32 p0_mpwldectrl1;
+	u32 p1_mpwldectrl0;
+	u32 p1_mpwldectrl1;
+	/* read DQS gating */
+	u32 p0_mpdgctrl0;

arch/arm/include/asm/arch-mx6/mx6-ddr.h +8 -0

+
+/* configure iomux (pinctl/padctl) */
+void mx6dq_dram_iocfg(unsigned width,
+		      const struct mx6dq_iomux_ddr_regs *,
+		      const struct mx6dq_iomux_grp_regs *);
+void mx6sdl_dram_iocfg(unsigned width,
+		       const struct mx6sdl_iomux_ddr_regs *,
+		       const struct mx6sdl_iomux_grp_regs *);

Add memory-mapped structures for MMDC iomux and configuration. Note that while the MMDC configuration registers are common between the IMX6DQ (IMX6DUAL/IMX6QUAD) and IMX6SDL (IMX6SOLO/IMX6DUALLITE) types the iomux registers differ. This requires two sets of structures.

Add structures to describe DDR3 device information, system information (memory layout, etc), and MMDC calibration regitsers that can be used to configure the MMDC dynamically.

We define these structures for SPL builds instead of including mx6q-ddr.h and mx6dl-ddr.h which use the same namespace and are only useful for imximage cfg files.

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

---
v2:
- split out from original mmdc configuration patch
- only define for SPL build
- do not include mx6q-ddr.h and mx6dl-ddr.h for SPL build - these use the same namespace and are only useful for imximage cfg files

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

arch/arm/include/asm/arch-mx6/mx6-ddr.h +37 -0

index 43d377a..d544d2e 100644
@@ -6,6 +6,7 @@
 #ifndef __ASM_ARCH_MX6_DDR_H__
 #define __ASM_ARCH_MX6_DDR_H__
 
+#ifndef CONFIG_SPL_BUILD
 #ifdef CONFIG_MX6Q
 #include "mx6q-ddr.h"
 #else

@@ -15,6 +16,236 @@
 #error "Please select cpu"
 #endif	/* CONFIG_MX6DL or CONFIG_MX6S */
 #endif	/* CONFIG_MX6Q */
+#else
+
+/* MMDC P0/P1 Registers */
+struct mmdc_p_regs {
+	u32 mdctl;
+	u32 mdpdc;
+	u32 mdotc;
+	u32 mdcfg0;
+	u32 mdcfg1;
+	u32 mdcfg2;
+	u32 mdmisc;
+	u32 mdscr;
+	u32 mdref;
+	u32 res1[2];
+	u32 mdrwd;
+	u32 mdor;
+	u32 res2[3];
+	u32 mdasp;
+	u32 res3[240];
+	u32 mapsr;
+	u32 res4[254];
+	u32 mpzqhwctrl;
+	u32 res5[2];
+	u32 mpwldectrl0;
+	u32 mpwldectrl1;
+	u32 res6;
+	u32 mpodtctrl;
+	u32 mprddqby0dl;
+	u32 mprddqby1dl;
+	u32 mprddqby2dl;
+	u32 mprddqby3dl;
+	u32 res7[4];
+	u32 mpdgctrl0;
+	u32 mpdgctrl1;
+	u32 res8;
+	u32 mprddlctl;

arch/arm/include/asm/arch-mx6/mx6-ddr.h +160 -0

+struct mx6dq_iomux_ddr_regs {
+	u32 res1[3];
+	u32 dram_sdqs5;
+	u32 dram_dqm5;
+	u32 dram_dqm4;
+	u32 dram_sdqs4;
+	u32 dram_sdqs3;
+	u32 dram_dqm3;
+	u32 dram_sdqs2;
+	u32 dram_dqm2;
+	u32 res2[16];
+	u32 dram_cas;
+	u32 res3[2];
+	u32 dram_ras;
+	u32 dram_reset;
+	u32 res4[2];
+	u32 dram_sdclk_0;
+	u32 dram_sdba2;
+	u32 dram_sdcke0;
+	u32 dram_sdclk_1;
+	u32 dram_sdcke1;
+	u32 dram_sdodt0;
+	u32 dram_sdodt1;
+	u32 res5;
+	u32 dram_sdqs0;
+	u32 dram_dqm0;
+	u32 dram_sdqs1;
+	u32 dram_dqm1;
+	u32 dram_sdqs6;
+	u32 dram_dqm6;
+	u32 dram_sdqs7;
+	u32 dram_dqm7;
+};
+
+#define MX6DQ_IOM_GRP_BASE      0x020e0700
+struct mx6dq_iomux_grp_regs {
+	u32 res1[18];
+	u32 grp_b7ds;
+	u32 grp_addds;
+	u32 grp_ddrmode_ctl;
+	u32 res2;
+	u32 grp_ddrpke;
+	u32 res3[6];
+	u32 grp_ddrmode;
+	u32 res4[3];
+	u32 grp_b0ds;
+	u32 grp_b1ds;
+	u32 grp_ctlds;
+	u32 res5;
+	u32 grp_b2ds;
+	u32 grp_ddr_type;
+	u32 grp_b3ds;
+	u32 grp_b4ds;
+	u32 grp_b5ds;
+	u32 grp_b6ds;
+};
+
+#define MX6SDL_IOM_DDR_BASE     0x020e0400
+struct mx6sdl_iomux_ddr_regs {
+	u32 res1[25];
+	u32 dram_cas;
+	u32 res2[2];
+	u32 dram_dqm0;
+	u32 dram_dqm1;
+	u32 dram_dqm2;
+	u32 dram_dqm3;
+	u32 dram_dqm4;
+	u32 dram_dqm5;
+	u32 dram_dqm6;
+	u32 dram_dqm7;
+	u32 dram_ras;
+	u32 dram_reset;
+	u32 res3[2];
+	u32 dram_sdba2;
+	u32 dram_sdcke0;
+	u32 dram_sdcke1;
+	u32 dram_sdclk_0;
+	u32 dram_sdclk_1;
+	u32 dram_sdodt0;
+	u32 dram_sdodt1;
+	u32 dram_sdqs0;
+	u32 dram_sdqs1;
+	u32 dram_sdqs2;
+	u32 dram_sdqs3;
+	u32 dram_sdqs4;
+	u32 dram_sdqs5;
+	u32 dram_sdqs6;
+	u32 dram_sdqs7;
+};
+
+#define MX6SDL_IOM_GRP_BASE     0x020e0700
+struct mx6sdl_iomux_grp_regs {
+	u32 res1[18];
+	u32 grp_b7ds;
+	u32 grp_addds;
+	u32 grp_ddrmode_ctl;
+	u32 grp_ddrpke;
+	u32 res2[2];
+	u32 grp_ddrmode;
+	u32 grp_b0ds;
+	u32 res3;
+	u32 grp_ctlds;
+	u32 grp_b1ds;
+	u32 grp_ddr_type;
+	u32 grp_b2ds;
+	u32 grp_b3ds;
+	u32 grp_b4ds;
+	u32 grp_b5ds;
+	u32 res4;
+	u32 grp_b6ds;
+};
+
+/* Device Information: Varies per DDR3 part number and speed grade */
+struct mx6_ddr3_cfg {
+	u16 mem_speed;	/* ie 1600 for DDR3-1600 (800,1066,1333,1600) */
+	u8 density;	/* chip density (Gb) (1,2,4,8) */
+	u8 width;	/* bus width (bits) (4,8,16) */
+	u8 banks;	/* number of banks */
+	u8 rowaddr;	/* row address bits (11-16)*/
+	u8 coladdr;	/* col address bits (9-12) */
+	u8 pagesz;	/* page size (K) (1-2) */
+	u16 trcd;	/* tRCD=tRP=CL (ns*100) */
+	u16 trcmin;	/* tRC min (ns*100) */
+	u16 trasmin;	/* tRAS min (ns*100) */
+	u8 SRT;		/* self-refresh temperature: 0=normal, 1=extended */
+};
+
+/* System Information: Varies per board design, layout, and term choices */
+struct mx6_ddr_sysinfo {
+	u8 dsize;	/* size of bus (in dwords: 0=16bit,1=32bit,2=64bit) */
+	u8 cs_density;	/* density per chip select (Gb) */
+	u8 ncs;		/* number chip selects used (1|2) */
+	char cs1_mirror;/* enable address mirror (0|1) */
+	char bi_on;	/* Bank interleaving enable */
+	u8 rtt_nom;	/* Rtt_Nom (DDR3_RTT_*) */
+	u8 rtt_wr;	/* Rtt_Wr (DDR3_RTT_*) */
+	u8 ralat;	/* Read Additional Latency (0-7) */
+	u8 walat;	/* Write Additional Latency (0-3) */
+	u8 mif3_mode;	/* Command prediction working mode */
+	u8 rst_to_cke;	/* Time from SDE enable to CKE rise */
+	u8 sde_to_rst;	/* Time from SDE enable until DDR reset# is high */
+};
+
+/*
+ * Board specific calibration:
+ *   This includes write leveling calibration values as well as DQS gating
+ *   and read/write delays. These values are board/layout/device specific.
+ *   Freescale recommends using the i.MX6 DDR Stress Test Tool V1.0.2
+ *   (DOC-96412) to determine these values over a range of boards and
+ *   temperatures.
+ */
+struct mx6_mmdc_calibration {
+	/* write leveling calibration */
+	u32 p0_mpwldectrl0;
+	u32 p0_mpwldectrl1;
+	u32 p1_mpwldectrl0;
+	u32 p1_mpwldectrl1;
+	/* read DQS gating */
+	u32 p0_mpdgctrl0;
+	u32 p0_mpdgctrl1;

arch/arm/include/asm/arch-mx6/mx6-ddr.h +7 -0

+/* configure iomux (pinctl/padctl) */
+void mx6dq_dram_iocfg(unsigned width,
+		      const struct mx6dq_iomux_ddr_regs *,
+		      const struct mx6dq_iomux_grp_regs *);
+void mx6sdl_dram_iocfg(unsigned width,
+		       const struct mx6sdl_iomux_ddr_regs *,
+		       const struct mx6sdl_iomux_grp_regs *);

Hi Tim,

Quoted 82 lines · Tim Harvey

Add memory-mapped structures for MMDC iomux and configuration. Note that while the MMDC configuration registers are common between the IMX6DQ (IMX6DUAL/IMX6QUAD) and IMX6SDL (IMX6SOLO/IMX6DUALLITE) types the iomux registers differ. This requires two sets of structures.

Add structures to describe DDR3 device information, system information (memory layout, etc), and MMDC calibration regitsers that can be used to configure the MMDC dynamically.

We define these structures for SPL builds instead of including mx6q-ddr.h and mx6dl-ddr.h which use the same namespace and are only useful for imximage cfg files.

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

---
v2:
- split out from original mmdc configuration patch
- only define for SPL build
- do not include mx6q-ddr.h and mx6dl-ddr.h for SPL build - these use the same namespace and are only useful for imximage cfg files

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

arch/arm/include/asm/arch-mx6/mx6-ddr.h +43 -0

index 43d377a..d544d2e 100644
@@ -6,6 +6,7 @@
 #ifndef __ASM_ARCH_MX6_DDR_H__
 #define __ASM_ARCH_MX6_DDR_H__
 
+#ifndef CONFIG_SPL_BUILD
 #ifdef CONFIG_MX6Q
 #include "mx6q-ddr.h"
 #else

@@ -15,6 +16,236 @@
 #error "Please select cpu"
 #endif       /* CONFIG_MX6DL or CONFIG_MX6S */
 #endif       /* CONFIG_MX6Q */
+#else
+
+/* MMDC P0/P1 Registers */
+struct mmdc_p_regs {
+     u32 mdctl;
+     u32 mdpdc;
+     u32 mdotc;
+     u32 mdcfg0;
+     u32 mdcfg1;
+     u32 mdcfg2;
+     u32 mdmisc;
+     u32 mdscr;
+     u32 mdref;
+     u32 res1[2];
+     u32 mdrwd;
+     u32 mdor;
+     u32 res2[3];
+     u32 mdasp;
+     u32 res3[240];
+     u32 mapsr;
+     u32 res4[254];
+     u32 mpzqhwctrl;
+     u32 res5[2];
+     u32 mpwldectrl0;
+     u32 mpwldectrl1;
+     u32 res6;
+     u32 mpodtctrl;
+     u32 mprddqby0dl;
+     u32 mprddqby1dl;
+     u32 mprddqby2dl;
+     u32 mprddqby3dl;
+     u32 res7[4];
+     u32 mpdgctrl0;
+     u32 mpdgctrl1;
+     u32 res8;
+     u32 mprddlctl;
+     u32 res9;
+     u32 mpwrdlctl;
+     u32 res10[25];
+     u32 mpmur0;
+};
+

arch/arm/include/asm/arch-mx6/mx6-ddr.h line 64

Quoted 4 lines

arch/arm/include/asm/arch-mx6/mx6-ddr.h +4 -0

+/*
+ * MMDC iomux registers (pinctl/padctl) - (different for IMX6DQ vs IMX6SDL)
+ */
+#define MX6DQ_IOM_DDR_BASE      0x020e0500

It is only a question - having the value here let us know where the registers are located. Anyway, registers' offsets are stored in the imx-regs.h file of the SOC. Which is the reason to set them here ?

Add functions for configuring MMDC iomux and configuration based on board-specific configurations.

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

---
v2:
- split out mmdc and iomux structs into separate patch

---
  arch/arm/cpu/armv7/mx6/Makefile |   1 +
  arch/arm/cpu/armv7/mx6/ddr.c    | 469 ++++++++++++++++++++++++++++++++++++++++
  2 files changed, 470 insertions(+)

create mode 100644 arch/arm/cpu/armv7/mx6/ddr.c

arch/arm/cpu/armv7/mx6/Makefile +1 -0

index d7285fc..6dc9f8e 100644
@@ -8,4 +8,5 @@
  #
 
  obj-y	:= soc.o clock.o
+obj-$(CONFIG_SPL_BUILD)	     += ddr.o
  obj-$(CONFIG_SECURE_BOOT)    += hab.o

arch/arm/cpu/armv7/mx6/ddr.c new file +4 -0

new file mode 100644
index 0000000..1536418
@@ -0,0 +1,469 @@
+/*
+ * Copyright (C) 2014 Gateworks Corporation
+ * Author: Tim Harvey <[email protected]>
+ *

Hi Tim,

Quoted 36 lines · Tim Harvey

Add functions for configuring MMDC iomux and configuration based on board-specific configurations.

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

---
v2:
- split out mmdc and iomux structs into separate patch

---
  arch/arm/cpu/armv7/mx6/Makefile |   1 +
  arch/arm/cpu/armv7/mx6/ddr.c    | 469
++++++++++++++++++++++++++++++++++++++++
  2 files changed, 470 insertions(+)

create mode 100644 arch/arm/cpu/armv7/mx6/ddr.c

arch/arm/cpu/armv7/mx6/Makefile +1 -0

index d7285fc..6dc9f8e 100644
@@ -8,4 +8,5 @@
  #
 
  obj-y := soc.o clock.o
+obj-$(CONFIG_SPL_BUILD)             += ddr.o
  obj-$(CONFIG_SECURE_BOOT)    += hab.o

arch/arm/cpu/armv7/mx6/ddr.c new file +4 -0

new file mode 100644
index 0000000..1536418
@@ -0,0 +1,469 @@
+/*
+ * Copyright (C) 2014 Gateworks Corporation
+ * Author: Tim Harvey <[email protected]>
+ *

arch/arm/cpu/armv7/mx6/ddr.c line 14

Quoted 10 lines

arch/arm/cpu/armv7/mx6/ddr.c new file +10 -0

+ * SPDX-License-Identifier:     GPL-2.0+
+ */
+
+#include <common.h>
+#include <linux/types.h>
+#include <asm/arch/mx6-ddr.h>
+#include <asm/arch/sys_proto.h>
+#include <asm/io.h>
+#include <asm/types.h>
+

Do you think that someone will want to use SPL on i.MX6 with support for only one of the variants?

It seems that this should be conditionally included if i.MX6DQ is supported by the board.

arch/arm/cpu/armv7/mx6/ddr.c line 20

Quoted 9 lines

arch/arm/cpu/armv7/mx6/ddr.c new file +6 -0

+/* Configure MX6DQ mmdc iomux */
+void mx6dq_dram_iocfg(unsigned width,
+                     const struct mx6dq_iomux_ddr_regs *ddr,
+                     const struct mx6dq_iomux_grp_regs *grp)
+{
+       volatile struct mx6dq_iomux_ddr_regs *mx6_ddr_iomux;

...

Ditto for i.MX6SDL:

arch/arm/cpu/armv7/mx6/ddr.c line 95

Quoted 6 lines

arch/arm/cpu/armv7/mx6/ddr.c new file +5 -0

+/* Configure MX6SDL mmdc iomux */
+void mx6sdl_dram_iocfg(unsigned width,
+                    const struct mx6sdl_iomux_ddr_regs *ddr,
+                    const struct mx6sdl_iomux_grp_regs *grp)
+{

Regards,

Eric

Quoted 11 lines · Eric Nelson

Hi Tim,

<snip>

Do you think that someone will want to use SPL on i.MX6 with support for only one of the variants?

It seems that this should be conditionally included if i.MX6DQ is supported by the board.

Eric,

Yes - that makes sense. Someone could have a single CPU but have several memory variants. I'm not sure how much codespace it will save but things are tight at 64KB max.

I can wrap the functions and their calls with an:

#if defined(MX6QDL) || defined(MX6Q) || defined(MX6D)

Quoted 9 lines

Quoted 9 lines

arch/arm/cpu/armv7/mx6/ddr.c new file +6 -0

+/* Configure MX6DQ mmdc iomux */
+void mx6dq_dram_iocfg(unsigned width,
+                     const struct mx6dq_iomux_ddr_regs *ddr,
+                     const struct mx6dq_iomux_grp_regs *grp)
+{
+       volatile struct mx6dq_iomux_ddr_regs *mx6_ddr_iomux;

Quoted 4 lines

...

Ditto for i.MX6SDL:

and:

#if defined(MX6QDL) || defined(MX6DL) || defined(MX6S)

Quoted 42 lines · Tim Harvey

Add functions for configuring MMDC iomux and configuration based on board-specific configurations.

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

---
v2:
- split out mmdc and iomux structs into separate patch

---
  arch/arm/cpu/armv7/mx6/Makefile |   1 +
  arch/arm/cpu/armv7/mx6/ddr.c    | 469
++++++++++++++++++++++++++++++++++++++++
  2 files changed, 470 insertions(+)

create mode 100644 arch/arm/cpu/armv7/mx6/ddr.c

arch/arm/cpu/armv7/mx6/Makefile +1 -0

index d7285fc..6dc9f8e 100644
@@ -8,4 +8,5 @@
  #
 
  obj-y := soc.o clock.o
+obj-$(CONFIG_SPL_BUILD)             += ddr.o
  obj-$(CONFIG_SECURE_BOOT)    += hab.o

arch/arm/cpu/armv7/mx6/ddr.c new file +10 -0

new file mode 100644
index 0000000..1536418
@@ -0,0 +1,469 @@
+/*
+ * Copyright (C) 2014 Gateworks Corporation
+ * Author: Tim Harvey <[email protected]>
+ *
+ * SPDX-License-Identifier:     GPL-2.0+
+ */
+
+#include <common.h>
+#include <linux/types.h>
+#include <asm/arch/mx6-ddr.h>

arch/arm/cpu/armv7/mx6/ddr.c line 14

Quoted 4 lines

arch/arm/cpu/armv7/mx6/ddr.c new file +4 -0

+#include <asm/arch/sys_proto.h>
+#include <asm/io.h>
+#include <asm/types.h>
+

Do you think that someone will want to use SPL on i.MX6 with support for only one of the variants?

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

---
v2:
- moved macros for declaring and using structs for array variant
- removed non-related whitespace cleanup

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

arch/arm/imx-common/iomux-v3.c +0 -0

index b59b802..d3e1e30 100644

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

+/* 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..2f7a1cb 100644

Hi Tim,

Quoted 6 lines

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 8 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 don't think a blind reference to the previously proposed solution helps in understanding this patch, and an example would be more helpful.

arch/arm/imx-common/iomux-v3.c void imx_iomux_v3_setup_pad(iomux_v3_cfg_t pad) · line 50

Quoted 3 lines

arch/arm/imx-common/iomux-v3.c +0 -2

-void imx_iomux_v3_setup_multiple_pads(iomux_v3_cfg_t const *pad_list,
-                                     unsigned count)

I think 'start_offs' would be clearer than 'list'.

The function name ..._array() also doesn't really capture what's going on here. Naming is hard though, and I'm not coming up with something else.

Perhaps 'sparse', 'skip', or alternate?

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

Quoted 3 lines

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

+/* macros for declaring and using pinmux array */
+#define IOMUX_PADS(x) (MX6Q_##x), (MX6DL_##x)

In a similar vein to my comment about Patch 8, I do wonder if a minor extension of this will allow use with a single-variant board though.

We have some custom designs that really only function with one variant (usually i.MX6DQ) and it seems wrong to have the other variant included.

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

Quoted 12 lines

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

+#define SETUP_IOMUX_PAD(def)                                   \
+if (is_cpu_type(MXC_CPU_MX6Q)) {                               \
+       imx_iomux_v3_setup_pad(MX6Q_##def);                     \
+} else {                                                       \
+       imx_iomux_v3_setup_pad(MX6DL_##def);                    \
+}
+#define SETUP_IOMUX_PADS(x)                                    \
+       imx_iomux_v3_setup_multiple_pads_array(x,               \
+       ARRAY_SIZE(x)/2, is_cpu_type(MXC_CPU_MX6Q) ? 0 : 1, 2)
+
  #endif        /* __MACH_IOMUX_V3_H__*/

Please don't mis-interpret my comments as any form of Nack.

This patch moves the ball forward, and the approach of building two lists into one prevents duplication of tables quite nicely.

Regards,

Eric

On Tue, Apr 29, 2014 at 8:22 AM, Eric Nelson
<[email protected]> wrote:

Quoted 5 lines

The function name ..._array() also doesn't really capture what's going on here. Naming is hard though, and I'm not coming up with something else.

Perhaps 'sparse', 'skip', or alternate?

ya, I'm not sure anything else is more explanatory when we are doing something like this. Its bad enough that its likely difficult for someone to understand their first time through that we are doing this to eliminate multiple structs.

<snip>

Quoted 6 lines

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

Quoted 2 lines

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

+/* macros for declaring and using pinmux array */
+#define IOMUX_PADS(x) (MX6Q_##x), (MX6DL_##x)

In a similar vein to my comment about Patch 8, I do wonder if a minor extension of this will allow use with a single-variant board though.

for a single-variant one would just use the original IOMUX_PAD/imx_iomux_v3_setup_pad/imx_iomux_v3_setup_pad right?

Quoted 20 lines

We have some custom designs that really only function with one variant (usually i.MX6DQ) and it seems wrong to have the other variant included.

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

Quoted 12 lines

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

+#define SETUP_IOMUX_PAD(def)                                   \
+if (is_cpu_type(MXC_CPU_MX6Q)) {                               \
+       imx_iomux_v3_setup_pad(MX6Q_##def);                     \
+} else {                                                       \
+       imx_iomux_v3_setup_pad(MX6DL_##def);                    \
+}
+#define SETUP_IOMUX_PADS(x)                                    \
+       imx_iomux_v3_setup_multiple_pads_array(x,               \
+       ARRAY_SIZE(x)/2, is_cpu_type(MXC_CPU_MX6Q) ? 0 : 1, 2)
+
   #endif        /* __MACH_IOMUX_V3_H__*/

Please don't mis-interpret my comments as any form of Nack.

Quoted 8 lines

This patch moves the ball forward, and the approach of building two lists into one prevents duplication of tables quite nicely.

Regards,

Eric

Thanks for the review!

Tim
_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Quoted 3 lines · Tim Harvey

On Tue, Apr 29, 2014 at 8:22 AM, Eric Nelson
<[email protected]> wrote:

<snip>

Quoted 10 lines

Quoted 5 lines

The function name ..._array() also doesn't really capture what's going on here. Naming is hard though, and I'm not coming up with something else.

Perhaps 'sparse', 'skip', or alternate?

ya, I'm not sure anything else is more explanatory when we are doing something like this. Its bad enough that its likely difficult for someone to understand their first time through that we are doing this to eliminate multiple structs.

Come to think of it, I don't think we need an _array() function at all. The list selection and
stride size are IOMUX_PADS implementation details. It's not something we should expose to
the function user. is_cpu_type() and ifdef(CONFIG_MX6QDL) can be used to decide the
list and stride values inside imx_iomux_v3_setup_multiple_pads(), and then this function
could be used for both single and multi cpu type situations.

Quoted 11 lines

<snip>

Quoted 8 lines

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

Quoted 3 lines

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

+/* macros for declaring and using pinmux array */
+#define IOMUX_PADS(x) (MX6Q_##x), (MX6DL_##x)

In a similar vein to my comment about Patch 8, I do wonder if a minor extension of this will allow use with a single-variant board though.

Quoted 2 lines

for a single-variant one would just use the original IOMUX_PAD/imx_iomux_v3_setup_pad/imx_iomux_v3_setup_pad right?

They can, but then we don't get to use the same code for both situations.
If we define two versions of IOMUX_PADS: one for multi cpu type, and one for single cpu type, then the pinmux arrays for both situations will be syntactically similar.
When combined with my other suggestion, it will be very easy to take a U-Boot configured for one CPU type, and reconfigure it to support both CPU types.