[U-Boot] [PATCH 0/4] lpc32xx: devkit3250 board update

[U-Boot] [PATCH 0/4] lpc32xx: devkit3250 board update

[Vladimir Zapolskiy]

This changeset improves support of Timll DevKit3250 board:
* added LPC32xx MAC and SMSC RMII phy support, this dependends on
  - http://patchwork.ozlabs.org/patch/489100/
  - http://patchwork.ozlabs.org/patch/489190/
  - http://patchwork.ozlabs.org/patch/491419/
  - http://patchwork.ozlabs.org/patch/491420/
* added GPIO, SPI, I2C support, works good, many thanks to Albert,
* added LPC32xx SLC NAND driver, testing of 50 MiB data raw reading
  shows 1 MiB/s speed, the same change has been sent to the mailing
  list separately, here it is duplicated as a build dependency:
  - http://patchwork.ozlabs.org/patch/495247/
  - http://patchwork.ozlabs.org/patch/495250/
* added an option to pass DTB to an operating system,
* changed serial console to commonly used as default UART5,
* boot delay is set to 1 for convenience,
* extended predefined environment variables and reserved space on NAND,
* added an option to build SPL image for the board, by default SPL
  downloads U-boot image from NAND (offset 0x40000, size 0x60000).

Albert, from commit logs I noticed that WORK92105 has SLC NAND chip,
but it is managed by MLC controller, if you have any plans to test
LPC32xx SLC NAND driver from the series, please *be aware* of
different OOB layouts, I found it is quite easy to screw up the chip,
if SLC and MLC are interchanged, so that only "nand scrub" helps.
FWIW this SLC NAND driver uses a default OOB layout, the same one
is found in Linux LPC32xx SLC NAND driver.

Vladimir Zapolskiy (4):
  spl: nand: simple: replace readb() with chip specific read_buf()
  nand: lpc32xx: add SLC NAND controller support
  lpc32xx: devkit3250: update of board configuration
  lpc32xx: devkit3250: add spl build support

 arch/arm/Kconfig                              |   1 +
 arch/arm/cpu/arm926ejs/lpc32xx/devices.c      |   6 +
 arch/arm/include/asm/arch-lpc32xx/clk.h       |   2 +
 arch/arm/include/asm/arch-lpc32xx/sys_proto.h |   1 +
 board/timll/devkit3250/Makefile               |   1 +
 board/timll/devkit3250/devkit3250.c           |  31 ++++-
 board/timll/devkit3250/devkit3250_spl.c       |  68 ++++++++++
 configs/devkit3250_defconfig                  |   4 +
 drivers/mtd/nand/Makefile                     |   1 +
 drivers/mtd/nand/lpc32xx_nand_slc.c           | 183 ++++++++++++++++++++++++++
 drivers/mtd/nand/nand_spl_simple.c            |   7 +-
 include/configs/devkit3250.h                  | 145 +++++++++++++++++++-
 12 files changed, 442 insertions(+), 8 deletions(-)
 create mode 100644 board/timll/devkit3250/devkit3250_spl.c
 create mode 100644 drivers/mtd/nand/lpc32xx_nand_slc.c

-- 
2.1.4

[U-Boot] [PATCH 1/4] spl: nand: simple: replace readb() with chip specific read_buf()

[Vladimir Zapolskiy]

Some NAND controllers define custom functions to read data out,
respect this in order to correctly support bad block handling in
simple SPL NAND framework.

NAND controller specific read_buf() is used even to read 1 byte in
case of connected 8-bit NAND device, it turns out that read_byte()
may become outdated.

Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
---
 drivers/mtd/nand/nand_spl_simple.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/nand_spl_simple.c b/drivers/mtd/nand/nand_spl_simple.c
index 700ca32..e69f662 100644
--- a/drivers/mtd/nand/nand_spl_simple.c
+++ b/drivers/mtd/nand/nand_spl_simple.c
@@ -115,6 +115,7 @@ static int nand_command(int block, int page, uint32_t offs,
 static int nand_is_bad_block(int block)
 {
 	struct nand_chip *this = mtd.priv;
+	u_char bb_data[2];
 
 	nand_command(block, 0, CONFIG_SYS_NAND_BAD_BLOCK_POS,
 		NAND_CMD_READOOB);
@@ -123,10 +124,12 @@ static int nand_is_bad_block(int block)
 	 * Read one byte (or two if it's a 16 bit chip).
 	 */
 	if (this->options & NAND_BUSWIDTH_16) {
-		if (readw(this->IO_ADDR_R) != 0xffff)
+		this->read_buf(&mtd, bb_data, 2);
+		if (bb_data[0] != 0xff || bb_data[1] != 0xff)
 			return 1;
 	} else {
-		if (readb(this->IO_ADDR_R) != 0xff)
+		this->read_buf(&mtd, bb_data, 1);
+		if (bb_data[0] != 0xff)
 			return 1;
 	}
 
-- 
2.1.4

[U-Boot] [PATCH 2/4] nand: lpc32xx: add SLC NAND controller support

[Vladimir Zapolskiy]

The change adds support of LPC32xx SLC NAND controller.

LPC32xx SoC has two different mutually exclusive NAND controllers to
communicate with single and multiple layer chips.

This simple driver allows to specify NAND chip timings and defines
custom read_buf()/write_buf() operations, because access to 8-bit data
register must be 32-bit aligned.

Support of hardware ECC calculation is not implemented (data
correction is always done by software), since it requires a working
DMA engine.

The driver can be included to an SPL image.

Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
---
 arch/arm/cpu/arm926ejs/lpc32xx/devices.c      |   6 +
 arch/arm/include/asm/arch-lpc32xx/clk.h       |   2 +
 arch/arm/include/asm/arch-lpc32xx/sys_proto.h |   1 +
 drivers/mtd/nand/Makefile                     |   1 +
 drivers/mtd/nand/lpc32xx_nand_slc.c           | 183 ++++++++++++++++++++++++++
 5 files changed, 193 insertions(+)
 create mode 100644 drivers/mtd/nand/lpc32xx_nand_slc.c

diff --git a/arch/arm/cpu/arm926ejs/lpc32xx/devices.c b/arch/arm/cpu/arm926ejs/lpc32xx/devices.c
index 5a453e3..b0287be 100644
--- a/arch/arm/cpu/arm926ejs/lpc32xx/devices.c
+++ b/arch/arm/cpu/arm926ejs/lpc32xx/devices.c
@@ -54,6 +54,12 @@ void lpc32xx_mlc_nand_init(void)
 	writel(CLK_NAND_MLC | CLK_NAND_MLC_INT, &clk->flashclk_ctrl);
 }
 
+void lpc32xx_slc_nand_init(void)
+{
+	/* Enable SLC NAND interface */
+	writel(CLK_NAND_SLC | CLK_NAND_SLC_SELECT, &clk->flashclk_ctrl);
+}
+
 void lpc32xx_i2c_init(unsigned int devnum)
 {
 	/* Enable I2C interface */
diff --git a/arch/arm/include/asm/arch-lpc32xx/clk.h b/arch/arm/include/asm/arch-lpc32xx/clk.h
index 9449869..010211a 100644
--- a/arch/arm/include/asm/arch-lpc32xx/clk.h
+++ b/arch/arm/include/asm/arch-lpc32xx/clk.h
@@ -153,7 +153,9 @@ struct clk_pm_regs {
 #define CLK_DMA_ENABLE			(1 << 0)
 
 /* NAND Clock Control Register bits */
+#define CLK_NAND_SLC			(1 << 0)
 #define CLK_NAND_MLC			(1 << 1)
+#define CLK_NAND_SLC_SELECT		(1 << 2)
 #define CLK_NAND_MLC_INT		(1 << 5)
 
 /* SSP Clock Control Register bits */
diff --git a/arch/arm/include/asm/arch-lpc32xx/sys_proto.h b/arch/arm/include/asm/arch-lpc32xx/sys_proto.h
index c3d890d..0845f83 100644
--- a/arch/arm/include/asm/arch-lpc32xx/sys_proto.h
+++ b/arch/arm/include/asm/arch-lpc32xx/sys_proto.h
@@ -12,6 +12,7 @@
 void lpc32xx_uart_init(unsigned int uart_id);
 void lpc32xx_mac_init(void);
 void lpc32xx_mlc_nand_init(void);
+void lpc32xx_slc_nand_init(void);
 void lpc32xx_i2c_init(unsigned int devnum);
 void lpc32xx_ssp_init(void);
 #if defined(CONFIG_SPL_BUILD)
diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
index 347ea62..e2dc99a 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -53,6 +53,7 @@ obj-$(CONFIG_NAND_KB9202) += kb9202_nand.o
 obj-$(CONFIG_NAND_KIRKWOOD) += kirkwood_nand.o
 obj-$(CONFIG_NAND_KMETER1) += kmeter1_nand.o
 obj-$(CONFIG_NAND_LPC32XX_MLC) += lpc32xx_nand_mlc.o
+obj-$(CONFIG_NAND_LPC32XX_SLC) += lpc32xx_nand_slc.o
 obj-$(CONFIG_NAND_MPC5121_NFC) += mpc5121_nfc.o
 obj-$(CONFIG_NAND_VF610_NFC) += vf610_nfc.o
 obj-$(CONFIG_NAND_MXC) += mxc_nand.o
diff --git a/drivers/mtd/nand/lpc32xx_nand_slc.c b/drivers/mtd/nand/lpc32xx_nand_slc.c
new file mode 100644
index 0000000..c888dd6
--- /dev/null
+++ b/drivers/mtd/nand/lpc32xx_nand_slc.c
@@ -0,0 +1,183 @@
+/*
+ * LPC32xx SLC NAND flash controller driver
+ *
+ * (C) Copyright 2015 Vladimir Zapolskiy <vz@mleia.com>
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#include <common.h>
+#include <nand.h>
+#include <asm/errno.h>
+#include <asm/io.h>
+#include <asm/arch/clk.h>
+#include <asm/arch/sys_proto.h>
+
+struct lpc32xx_nand_slc_registers {
+	u32 data;
+	u32 addr;
+	u32 cmd;
+	u32 stop;
+	u32 ctrl;
+	u32 cfg;
+	u32 stat;
+	u32 int_stat;
+	u32 ien;
+	u32 isr;
+	u32 icr;
+	u32 tac;
+	u32 tc;
+	u32 ecc;
+	u32 dma_data;
+};
+
+/* CFG register */
+#define CFG_CE_LOW		(1 << 5)
+#define CFG_ECC_EN		(1 << 3)
+
+/* CTRL register */
+#define CTRL_SW_RESET		(1 << 2)
+#define CTRL_ECC_CLEAR		(1 << 1)
+
+/* STAT register */
+#define STAT_NAND_READY		(1 << 0)
+
+/* INT_STAT register */
+#define INT_STAT_TC		(1 << 1)
+#define INT_STAT_RDY		(1 << 0)
+
+/* TAC register bits, be aware of overflows */
+#define TAC_W_RDY(n)		(max_t(uint32_t, (n), 0xF) << 28)
+#define TAC_W_WIDTH(n)		(max_t(uint32_t, (n), 0xF) << 24)
+#define TAC_W_HOLD(n)		(max_t(uint32_t, (n), 0xF) << 20)
+#define TAC_W_SETUP(n)		(max_t(uint32_t, (n), 0xF) << 16)
+#define TAC_R_RDY(n)		(max_t(uint32_t, (n), 0xF) << 12)
+#define TAC_R_WIDTH(n)		(max_t(uint32_t, (n), 0xF) << 8)
+#define TAC_R_HOLD(n)		(max_t(uint32_t, (n), 0xF) << 4)
+#define TAC_R_SETUP(n)		(max_t(uint32_t, (n), 0xF) << 0)
+
+static struct lpc32xx_nand_slc_registers __iomem *lpc32xx_nand_slc_registers
+	= (struct lpc32xx_nand_slc_registers __iomem *)SLC_NAND_BASE;
+
+static void lpc32xx_nand_init(void)
+{
+	uint32_t hclk = get_hclk_clk_rate();
+
+	/* Reset SLC NAND controller */
+	writel(CTRL_SW_RESET, &lpc32xx_nand_slc_registers->ctrl);
+
+	/* 8-bit bus, no DMA, no ECC, ordinary CE signal */
+	writel(0, &lpc32xx_nand_slc_registers->cfg);
+
+	/* Interrupts disabled and cleared */
+	writel(0, &lpc32xx_nand_slc_registers->ien);
+	writel(INT_STAT_TC | INT_STAT_RDY,
+	       &lpc32xx_nand_slc_registers->icr);
+
+	/* Configure NAND flash timings */
+	writel(TAC_W_RDY(CONFIG_LPC32XX_NAND_SLC_WDR_CLKS) |
+	       TAC_W_WIDTH(hclk / CONFIG_LPC32XX_NAND_SLC_WWIDTH) |
+	       TAC_W_HOLD(hclk / CONFIG_LPC32XX_NAND_SLC_WHOLD) |
+	       TAC_W_SETUP(hclk / CONFIG_LPC32XX_NAND_SLC_WSETUP) |
+	       TAC_R_RDY(CONFIG_LPC32XX_NAND_SLC_RDR_CLKS) |
+	       TAC_R_WIDTH(hclk / CONFIG_LPC32XX_NAND_SLC_RWIDTH) |
+	       TAC_R_HOLD(hclk / CONFIG_LPC32XX_NAND_SLC_RHOLD) |
+	       TAC_R_SETUP(hclk / CONFIG_LPC32XX_NAND_SLC_RSETUP),
+	       &lpc32xx_nand_slc_registers->tac);
+}
+
+static void lpc32xx_nand_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int ctrl)
+{
+	debug("ctrl: 0x%08x, cmd: 0x%08x\n", ctrl, cmd);
+
+	if (ctrl & NAND_NCE)
+		setbits_le32(&lpc32xx_nand_slc_registers->cfg, CFG_CE_LOW);
+	else
+		clrbits_le32(&lpc32xx_nand_slc_registers->cfg, CFG_CE_LOW);
+
+	if (cmd == NAND_CMD_NONE)
+		return;
+
+	if (ctrl & NAND_CLE)
+		writel(cmd & 0xFF, &lpc32xx_nand_slc_registers->cmd);
+	else /* if (ctrl & NAND_ALE) */
+		writel(cmd & 0xFF, &lpc32xx_nand_slc_registers->addr);
+}
+
+static int lpc32xx_nand_dev_ready(struct mtd_info *mtd)
+{
+	return readl(&lpc32xx_nand_slc_registers->stat) & STAT_NAND_READY;
+}
+
+static void lpc32xx_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
+{
+	while (len-- > 0)
+		*buf++ = (uint8_t)readl(&lpc32xx_nand_slc_registers->data);
+}
+
+static uint8_t lpc32xx_read_byte(struct mtd_info *mtd)
+{
+	return (uint8_t)readl(&lpc32xx_nand_slc_registers->data);
+}
+
+static void lpc32xx_write_buf(struct mtd_info *mtd, const uint8_t *buf, int len)
+{
+	while (len-- > 0)
+		writel((uint32_t)*buf++, &lpc32xx_nand_slc_registers->data);
+}
+
+static void lpc32xx_write_byte(struct mtd_info *mtd, uint8_t byte)
+{
+	writel((uint32_t)byte, &lpc32xx_nand_slc_registers->data);
+}
+
+/*
+ * LPC32xx has only one SLC NAND controller, don't utilize
+ * CONFIG_SYS_NAND_SELF_INIT to be able to reuse this function
+ * both in SPL NAND and U-boot images.
+ */
+int board_nand_init(struct nand_chip *lpc32xx_chip)
+{
+	lpc32xx_chip->IO_ADDR_R	= &lpc32xx_nand_slc_registers->data;
+	lpc32xx_chip->IO_ADDR_W	= &lpc32xx_nand_slc_registers->data;
+
+	lpc32xx_chip->cmd_ctrl	= lpc32xx_nand_cmd_ctrl;
+	lpc32xx_chip->dev_ready	= lpc32xx_nand_dev_ready;
+
+	/*
+	 * Hardware ECC calculation is not supported by the driver, because it
+	 * requires DMA support, see Note after SLC_ECC register description
+	 */
+	lpc32xx_chip->ecc.mode	= NAND_ECC_SOFT;
+
+	/*
+	 * The implementation of these functions is quite common, but
+	 * they MUST be defined, because access to data register
+	 * is strictly 32-bit aligned.
+	 */
+	lpc32xx_chip->read_buf	= lpc32xx_read_buf;
+	lpc32xx_chip->read_byte	= lpc32xx_read_byte;
+	lpc32xx_chip->write_buf	= lpc32xx_write_buf;
+	lpc32xx_chip->write_byte	= lpc32xx_write_byte;
+
+	/*
+	 * Use default ECC layout, but these values are predefined
+	 * for both small and large page NAND flash devices.
+	 */
+	lpc32xx_chip->ecc.size	= 256;
+	lpc32xx_chip->ecc.bytes	= 3;
+	lpc32xx_chip->ecc.strength	= 1;
+
+#if defined(CONFIG_SPL_BUILD)
+	lpc32xx_chip->options		|= NAND_SKIP_BBTSCAN;
+#endif
+
+#if defined(CONFIG_SYS_NAND_USE_FLASH_BBT)
+	lpc32xx_chip->bbt_options	|= NAND_BBT_USE_FLASH;
+#endif
+
+	/* Initialize NAND interface */
+	lpc32xx_nand_init();
+
+	return 0;
+}
-- 
2.1.4

[U-Boot] [PATCH 3/4] lpc32xx: devkit3250: update of board configuration

[Vladimir Zapolskiy]

This change adds more peripherals to Timll DevKit3250 board, namely
MAC and SMSC phy, SLC NAND, GPIO, SPI and I2C.

Also the default serial console is changed to UART5, added an option
to pass device tree blob by means of bootm, predefined environment
variables are slightly extended and reserved space on NAND to store
user defined U-boot environment.

Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
---
 board/timll/devkit3250/devkit3250.c | 31 ++++++++++++-
 configs/devkit3250_defconfig        |  3 ++
 include/configs/devkit3250.h        | 92 +++++++++++++++++++++++++++++++++++--
 3 files changed, 120 insertions(+), 6 deletions(-)

diff --git a/board/timll/devkit3250/devkit3250.c b/board/timll/devkit3250/devkit3250.c
index 6acc416..4b3c94e 100644
--- a/board/timll/devkit3250/devkit3250.c
+++ b/board/timll/devkit3250/devkit3250.c
@@ -1,23 +1,52 @@
 /*
  * Embest/Timll DevKit3250 board support
  *
- * Copyright (C) 2011 Vladimir Zapolskiy <vz@mleia.com>
+ * Copyright (C) 2011-2015 Vladimir Zapolskiy <vz@mleia.com>
  *
  * SPDX-License-Identifier:	GPL-2.0+
  */
 
 #include <common.h>
 #include <asm/arch/sys_proto.h>
+#include <asm/arch/clk.h>
 #include <asm/arch/cpu.h>
 #include <asm/arch/emc.h>
+#include <asm/arch/wdt.h>
+#include <asm/io.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
 static struct emc_regs *emc = (struct emc_regs *)EMC_BASE;
+static struct clk_pm_regs *clk = (struct clk_pm_regs *)CLK_PM_BASE;
+static struct wdt_regs *wdt = (struct wdt_regs *)WDT_BASE;
+
+void reset_periph(void)
+{
+	/* This function resets peripherals by triggering RESOUT_N */
+	setbits_le32(&clk->timclk_ctrl, CLK_TIMCLK_WATCHDOG);
+	writel(WDTIM_MCTRL_RESFRC1, &wdt->mctrl);
+	udelay(300);
+
+	writel(0, &wdt->mctrl);
+	clrbits_le32(&clk->timclk_ctrl, CLK_TIMCLK_WATCHDOG);
+
+	/* Such a long delay is needed to initialize SMSC phy */
+	udelay(10000);
+}
 
 int board_early_init_f(void)
 {
 	lpc32xx_uart_init(CONFIG_SYS_LPC32XX_UART);
+	lpc32xx_i2c_init(1);
+	lpc32xx_i2c_init(2);
+	lpc32xx_ssp_init();
+	lpc32xx_mac_init();
+
+	/*
+	 * nWP may be controlled by GPO19, but unpopulated by default R23
+	 * makes no sense to configure this GPIO level, nWP is always high
+	 */
+	lpc32xx_slc_nand_init();
 
 	return 0;
 }
diff --git a/configs/devkit3250_defconfig b/configs/devkit3250_defconfig
index f0c4ee1..56d719f 100644
--- a/configs/devkit3250_defconfig
+++ b/configs/devkit3250_defconfig
@@ -1,3 +1,6 @@
 CONFIG_ARM=y
 CONFIG_TARGET_DEVKIT3250=y
+# CONFIG_CMD_FPGA is not set
 # CONFIG_CMD_SETEXPR is not set
+CONFIG_DM=y
+CONFIG_DM_GPIO=y
diff --git a/include/configs/devkit3250.h b/include/configs/devkit3250.h
index 4f35234..b8218b5 100644
--- a/include/configs/devkit3250.h
+++ b/include/configs/devkit3250.h
@@ -1,7 +1,7 @@
 /*
  * Embest/Timll DevKit3250 board configuration file
  *
- * Copyright (C) 2011 Vladimir Zapolskiy <vz@mleia.com>
+ * Copyright (C) 2011-2015 Vladimir Zapolskiy <vz@mleia.com>
  *
  * SPDX-License-Identifier:	GPL-2.0+
  */
@@ -43,10 +43,44 @@
 /*
  * Serial Driver
  */
-#define CONFIG_SYS_LPC32XX_UART		2   /* UART2 */
+#define CONFIG_SYS_LPC32XX_UART		5   /* UART5 */
 #define CONFIG_BAUDRATE			115200
 
 /*
+ * I2C
+ */
+#define CONFIG_SYS_I2C
+#define CONFIG_SYS_I2C_LPC32XX
+#define CONFIG_SYS_I2C_SPEED		100000
+#define CONFIG_CMD_I2C
+
+/*
+ * GPIO
+ */
+#define CONFIG_LPC32XX_GPIO
+#define CONFIG_CMD_GPIO
+
+/*
+ * SSP/SPI
+ */
+#define CONFIG_LPC32XX_SSP
+#define CONFIG_LPC32XX_SSP_TIMEOUT	100000
+#define CONFIG_CMD_SPI
+
+/*
+ * Ethernet
+ */
+#define CONFIG_RMII
+#define CONFIG_PHY_SMSC
+#define CONFIG_LPC32XX_ETH
+#define CONFIG_PHYLIB
+#define CONFIG_PHY_ADDR			0x1F
+#define CONFIG_SYS_FAULT_ECHO_LINK_DOWN
+#define CONFIG_CMD_MII
+#define CONFIG_CMD_PING
+#define CONFIG_CMD_DHCP
+
+/*
  * NOR Flash
  */
 #define CONFIG_SYS_MAX_FLASH_BANKS	1
@@ -56,6 +90,29 @@
 #define CONFIG_SYS_FLASH_CFI
 
 /*
+ * NAND controller
+ */
+#define CONFIG_NAND_LPC32XX_SLC
+#define CONFIG_SYS_NAND_BASE		SLC_NAND_BASE
+#define CONFIG_SYS_MAX_NAND_DEVICE	1
+#define CONFIG_SYS_NAND_BASE_LIST	{ CONFIG_SYS_NAND_BASE }
+
+/*
+ * NAND chip timings
+ */
+#define CONFIG_LPC32XX_NAND_SLC_WDR_CLKS	14
+#define CONFIG_LPC32XX_NAND_SLC_WWIDTH		66666666
+#define CONFIG_LPC32XX_NAND_SLC_WHOLD		200000000
+#define CONFIG_LPC32XX_NAND_SLC_WSETUP		50000000
+#define CONFIG_LPC32XX_NAND_SLC_RDR_CLKS	14
+#define CONFIG_LPC32XX_NAND_SLC_RWIDTH		66666666
+#define CONFIG_LPC32XX_NAND_SLC_RHOLD		200000000
+#define CONFIG_LPC32XX_NAND_SLC_RSETUP		50000000
+
+#define CONFIG_SYS_NAND_USE_FLASH_BBT
+#define CONFIG_CMD_NAND
+
+/*
  * U-Boot General Configurations
  */
 #define CONFIG_SYS_LONGHELP
@@ -71,8 +128,33 @@
 #define CONFIG_DISPLAY_CPUINFO
 #define CONFIG_DOS_PARTITION
 
-#define CONFIG_ENV_IS_NOWHERE
+/*
+ * Pass open firmware flat tree
+ */
+#define CONFIG_OF_LIBFDT
+
+/*
+ * Environment
+ */
+#define CONFIG_ENV_IS_IN_NAND		1
 #define CONFIG_ENV_SIZE			SZ_128K
+#define CONFIG_ENV_OFFSET		0x000A0000
+
+#define CONFIG_BOOTCOMMAND			\
+	"dhcp; "				\
+	"tftp ${loadaddr} ${serverip}:${tftpdir}/${bootfile}; "		\
+	"tftp ${dtbaddr} ${serverip}:${tftpdir}/devkit3250.dtb; "	\
+	"setenv nfsargs ip=dhcp root=/dev/nfs nfsroot=${serverip}:${nfsroot},tcp; "	\
+	"setenv bootargs ${bootargs} ${nfsargs} ${userargs}; "			\
+	"bootm ${loadaddr} - ${dtbaddr}"
+
+#define CONFIG_EXTRA_ENV_SETTINGS		\
+	"autoload=no\0"				\
+	"ethaddr=00:01:90:00:C0:81\0"		\
+	"dtbaddr=0x81000000\0"			\
+	"nfsroot=/opt/projects/images/vladimir/oe/devkit3250/rootfs\0"	\
+	"tftpdir=vladimir/oe/devkit3250\0"	\
+	"userargs=oops=panic\0"
 
 /*
  * U-Boot Commands
@@ -85,10 +167,10 @@
 #define CONFIG_CMDLINE_TAG
 #define CONFIG_SETUP_MEMORY_TAGS
 #define CONFIG_ZERO_BOOTDELAY_CHECK
-#define CONFIG_BOOTDELAY		3
+#define CONFIG_BOOTDELAY		1
 
 #define CONFIG_BOOTFILE			"uImage"
-#define CONFIG_BOOTARGS			"console=ttyS2,115200n8"
+#define CONFIG_BOOTARGS			"console=ttyS0,115200n8"
 #define CONFIG_LOADADDR			0x80008000
 
 /*
-- 
2.1.4

[U-Boot] [PATCH 4/4] lpc32xx: devkit3250: add spl build support

[Vladimir Zapolskiy]

The change adds SPL build support to Timll DevKit3250 board, the
generated SPL image can be uploaded over UART5, JTAG or stored on
NAND. SPL is designed to load U-boot image from NAND.

All new NAND chip defines in board configuration are needed by
SPL NAND "simple" framework, the framework is used to reduce
potentially duplicated code from LPC32xx SLC NAND driver.

Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
---
 arch/arm/Kconfig                        |  1 +
 board/timll/devkit3250/Makefile         |  1 +
 board/timll/devkit3250/devkit3250_spl.c | 68 +++++++++++++++++++++++++++++++++
 configs/devkit3250_defconfig            |  1 +
 include/configs/devkit3250.h            | 53 +++++++++++++++++++++++++
 5 files changed, 124 insertions(+)
 create mode 100644 board/timll/devkit3250/devkit3250_spl.c

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 506463c..95b15bc 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -121,6 +121,7 @@ config TARGET_MAXBCM
 config TARGET_DEVKIT3250
 	bool "Support devkit3250"
 	select CPU_ARM926EJS
+	select SUPPORT_SPL
 
 config TARGET_WORK_92105
 	bool "Support work_92105"
diff --git a/board/timll/devkit3250/Makefile b/board/timll/devkit3250/Makefile
index 4722986..74d5cd3 100644
--- a/board/timll/devkit3250/Makefile
+++ b/board/timll/devkit3250/Makefile
@@ -6,3 +6,4 @@
 #
 
 obj-y	:= devkit3250.o
+obj-$(CONFIG_SPL_BUILD) += devkit3250_spl.o
diff --git a/board/timll/devkit3250/devkit3250_spl.c b/board/timll/devkit3250/devkit3250_spl.c
new file mode 100644
index 0000000..bf52698
--- /dev/null
+++ b/board/timll/devkit3250/devkit3250_spl.c
@@ -0,0 +1,68 @@
+/*
+ * Timll DevKit3250 board support, SPL board configuration
+ *
+ * (C) Copyright 2015 Vladimir Zapolskiy <vz@mleia.com>
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#include <common.h>
+#include <asm/io.h>
+#include <asm/arch/sys_proto.h>
+#include <asm/arch/cpu.h>
+#include <asm/arch/emc.h>
+#include <asm/arch-lpc32xx/gpio.h>
+#include <spl.h>
+
+static struct gpio_regs *gpio = (struct gpio_regs *)GPIO_BASE;
+
+/*
+ * SDRAM K4S561632N-LC60 settings are selected in assumption that
+ * SDRAM clock may be set up to 166 MHz, however at the moment
+ * it is 104 MHz. Most delay values are converted to be a multiple of
+ * base clock, and precise pinned values are not needed here.
+ */
+struct emc_dram_settings dram_64mb = {
+	.cmddelay	= 0x0001C000,
+	.config0	= 0x00005682,
+	.rascas0	= 0x00000302,
+	.rdconfig	= 0x00000011,	/* undocumented but crucial value */
+
+	.trp	= 83333333,
+	.tras	= 23809524,
+	.tsrex	= 12500000,
+	.twr	= 83000000,		/* tWR = tRDL = 2 CLK */
+	.trc	= 15384616,
+	.trfc	= 15384616,
+	.txsr	= 12500000,
+	.trrd	= 1,
+	.tmrd	= 1,
+	.tcdlr	= 0,
+
+	.refresh	= 130000,	/* 800 clock cycles */
+
+	.mode	= 0x00018000,
+	.emode	= 0x02000000,
+};
+
+void spl_board_init(void)
+{
+	/* First of all silence buzzer controlled by GPO_20 */
+	writel((1 << 20), &gpio->p3_outp_clr);
+
+	lpc32xx_uart_init(CONFIG_SYS_LPC32XX_UART);
+	preloader_console_init();
+
+	ddr_init(&dram_64mb);
+
+	/*
+	 * NAND initialization is done by nand_init(),
+	 * here just enable NAND SLC clocks
+	 */
+	lpc32xx_slc_nand_init();
+}
+
+u32 spl_boot_device(void)
+{
+	return BOOT_DEVICE_NAND;
+}
diff --git a/configs/devkit3250_defconfig b/configs/devkit3250_defconfig
index 56d719f..7246da5 100644
--- a/configs/devkit3250_defconfig
+++ b/configs/devkit3250_defconfig
@@ -1,5 +1,6 @@
 CONFIG_ARM=y
 CONFIG_TARGET_DEVKIT3250=y
+CONFIG_SPL=y
 # CONFIG_CMD_FPGA is not set
 # CONFIG_CMD_SETEXPR is not set
 CONFIG_DM=y
diff --git a/include/configs/devkit3250.h b/include/configs/devkit3250.h
index b8218b5..cc6a53e 100644
--- a/include/configs/devkit3250.h
+++ b/include/configs/devkit3250.h
@@ -21,7 +21,9 @@
 
 #define CONFIG_SYS_ICACHE_OFF
 #define CONFIG_SYS_DCACHE_OFF
+#if !defined(CONFIG_SPL_BUILD)
 #define CONFIG_SKIP_LOWLEVEL_INIT
+#endif
 #define CONFIG_BOARD_EARLY_INIT_F
 
 /*
@@ -174,6 +176,57 @@
 #define CONFIG_LOADADDR			0x80008000
 
 /*
+ * SPL specific defines
+ */
+/* SPL will be executed at offset 0 */
+#define CONFIG_SPL_TEXT_BASE		0x00000000
+
+/* SPL will use SRAM as stack */
+#define CONFIG_SPL_STACK		0x0000FFF8
+#define CONFIG_SPL_BOARD_INIT
+
+/* Use the framework and generic lib */
+#define CONFIG_SPL_FRAMEWORK
+#define CONFIG_SPL_LIBGENERIC_SUPPORT
+#define CONFIG_SPL_LIBCOMMON_SUPPORT
+
+/* SPL will use serial */
+#define CONFIG_SPL_SERIAL_SUPPORT
+
+/* SPL loads an image from NAND */
+#define CONFIG_SPL_NAND_SIMPLE
+#define CONFIG_SPL_NAND_RAW_ONLY
+#define CONFIG_SPL_NAND_SUPPORT
+#define CONFIG_SPL_NAND_DRIVERS
+
+#define CONFIG_SYS_NAND_BLOCK_SIZE	0x20000
+#define CONFIG_SYS_NAND_PAGE_SIZE	0x800
+#define CONFIG_SYS_NAND_ECCSIZE		0x100
+#define CONFIG_SYS_NAND_OOBSIZE		64
+#define CONFIG_SYS_NAND_ECCPOS		{ 40, 41, 42, 43, 44, 45, 46, 47, \
+					  48, 49, 50, 51, 52, 53, 54, 55, \
+					  56, 57, 58, 59, 60, 61, 62, 63, }
+#define CONFIG_SYS_NAND_ECCBYTES	3
+#define CONFIG_SYS_NAND_PAGE_COUNT	64
+#define CONFIG_SYS_NAND_BAD_BLOCK_POS	NAND_LARGE_BADBLOCK_POS
+
+#define CONFIG_SPL_NAND_ECC
+#define CONFIG_SPL_NAND_SOFTECC
+
+#define CONFIG_SPL_MAX_SIZE		0x20000
+#define CONFIG_SPL_PAD_TO		CONFIG_SPL_MAX_SIZE
+
+/* U-Boot will be 0x60000 bytes, loaded and run at CONFIG_SYS_TEXT_BASE */
+#define CONFIG_SYS_NAND_U_BOOT_OFFS	0x40000
+#define CONFIG_SYS_NAND_U_BOOT_SIZE	0x60000
+
+#define CONFIG_SYS_NAND_U_BOOT_START	CONFIG_SYS_TEXT_BASE
+#define CONFIG_SYS_NAND_U_BOOT_DST	CONFIG_SYS_TEXT_BASE
+
+/* See common/spl/spl.c  spl_set_header_raw_uboot() */
+#define CONFIG_SYS_MONITOR_LEN		CONFIG_SYS_NAND_U_BOOT_SIZE
+
+/*
  * Include SoC specific configuration
  */
 #include <asm/arch/config.h>
-- 
2.1.4

[U-Boot] [PATCH 3/4] lpc32xx: devkit3250: update of board configuration

[Albert ARIBAUD]

Hello Vladimir,

On Thu, 16 Jul 2015 02:33:47 +0300, Vladimir Zapolskiy <vz@mleia.com>
wrote:
> This change adds more peripherals to Timll DevKit3250 board, namely
> MAC and SMSC phy, SLC NAND, GPIO, SPI and I2C.
> 
> Also the default serial console is changed to UART5, added an option
> to pass device tree blob by means of bootm, predefined environment
> variables are slightly extended and reserved space on NAND to store
> user defined U-boot environment.
> 
> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
> ---
>  board/timll/devkit3250/devkit3250.c | 31 ++++++++++++-
>  configs/devkit3250_defconfig        |  3 ++
>  include/configs/devkit3250.h        | 92 +++++++++++++++++++++++++++++++++++--
>  3 files changed, 120 insertions(+), 6 deletions(-)
> 
> diff --git a/board/timll/devkit3250/devkit3250.c b/board/timll/devkit3250/devkit3250.c
> index 6acc416..4b3c94e 100644
> --- a/board/timll/devkit3250/devkit3250.c
> +++ b/board/timll/devkit3250/devkit3250.c
> @@ -1,23 +1,52 @@
>  /*
>   * Embest/Timll DevKit3250 board support
>   *
> - * Copyright (C) 2011 Vladimir Zapolskiy <vz@mleia.com>
> + * Copyright (C) 2011-2015 Vladimir Zapolskiy <vz@mleia.com>
>   *
>   * SPDX-License-Identifier:	GPL-2.0+
>   */
>  
>  #include <common.h>
>  #include <asm/arch/sys_proto.h>
> +#include <asm/arch/clk.h>
>  #include <asm/arch/cpu.h>
>  #include <asm/arch/emc.h>
> +#include <asm/arch/wdt.h>
> +#include <asm/io.h>
>  
>  DECLARE_GLOBAL_DATA_PTR;
>  
>  static struct emc_regs *emc = (struct emc_regs *)EMC_BASE;
> +static struct clk_pm_regs *clk = (struct clk_pm_regs *)CLK_PM_BASE;
> +static struct wdt_regs *wdt = (struct wdt_regs *)WDT_BASE;
> +
> +void reset_periph(void)
> +{
> +	/* This function resets peripherals by triggering RESOUT_N */
> +	setbits_le32(&clk->timclk_ctrl, CLK_TIMCLK_WATCHDOG);
> +	writel(WDTIM_MCTRL_RESFRC1, &wdt->mctrl);
> +	udelay(300);
> +
> +	writel(0, &wdt->mctrl);
> +	clrbits_le32(&clk->timclk_ctrl, CLK_TIMCLK_WATCHDOG);
> +
> +	/* Such a long delay is needed to initialize SMSC phy */
> +	udelay(10000);
> +}
>  
>  int board_early_init_f(void)
>  {
>  	lpc32xx_uart_init(CONFIG_SYS_LPC32XX_UART);
> +	lpc32xx_i2c_init(1);
> +	lpc32xx_i2c_init(2);
> +	lpc32xx_ssp_init();
> +	lpc32xx_mac_init();
> +
> +	/*
> +	 * nWP may be controlled by GPO19, but unpopulated by default R23
> +	 * makes no sense to configure this GPIO level, nWP is always high
> +	 */
> +	lpc32xx_slc_nand_init();
>  
>  	return 0;
>  }
> diff --git a/configs/devkit3250_defconfig b/configs/devkit3250_defconfig
> index f0c4ee1..56d719f 100644
> --- a/configs/devkit3250_defconfig
> +++ b/configs/devkit3250_defconfig
> @@ -1,3 +1,6 @@
>  CONFIG_ARM=y
>  CONFIG_TARGET_DEVKIT3250=y
> +# CONFIG_CMD_FPGA is not set
>  # CONFIG_CMD_SETEXPR is not set
> +CONFIG_DM=y
> +CONFIG_DM_GPIO=y
> diff --git a/include/configs/devkit3250.h b/include/configs/devkit3250.h
> index 4f35234..b8218b5 100644
> --- a/include/configs/devkit3250.h
> +++ b/include/configs/devkit3250.h
> @@ -1,7 +1,7 @@
>  /*
>   * Embest/Timll DevKit3250 board configuration file
>   *
> - * Copyright (C) 2011 Vladimir Zapolskiy <vz@mleia.com>
> + * Copyright (C) 2011-2015 Vladimir Zapolskiy <vz@mleia.com>
>   *
>   * SPDX-License-Identifier:	GPL-2.0+
>   */
> @@ -43,10 +43,44 @@
>  /*
>   * Serial Driver
>   */
> -#define CONFIG_SYS_LPC32XX_UART		2   /* UART2 */
> +#define CONFIG_SYS_LPC32XX_UART		5   /* UART5 */

This will affect existing users of this board, who expect it to use
UART2 as console. Was this expectation wrong, or can both UART2 and
UART5 be used as console? IOW, is this change required or is it just
easier for you secifically?

>  #define CONFIG_BAUDRATE			115200
>  
>  /*
> + * I2C
> + */
> +#define CONFIG_SYS_I2C
> +#define CONFIG_SYS_I2C_LPC32XX
> +#define CONFIG_SYS_I2C_SPEED		100000
> +#define CONFIG_CMD_I2C
> +
> +/*
> + * GPIO
> + */
> +#define CONFIG_LPC32XX_GPIO
> +#define CONFIG_CMD_GPIO
> +
> +/*
> + * SSP/SPI
> + */
> +#define CONFIG_LPC32XX_SSP
> +#define CONFIG_LPC32XX_SSP_TIMEOUT	100000
> +#define CONFIG_CMD_SPI
> +
> +/*
> + * Ethernet
> + */
> +#define CONFIG_RMII
> +#define CONFIG_PHY_SMSC
> +#define CONFIG_LPC32XX_ETH
> +#define CONFIG_PHYLIB
> +#define CONFIG_PHY_ADDR			0x1F
> +#define CONFIG_SYS_FAULT_ECHO_LINK_DOWN
> +#define CONFIG_CMD_MII
> +#define CONFIG_CMD_PING
> +#define CONFIG_CMD_DHCP
> +
> +/*
>   * NOR Flash
>   */
>  #define CONFIG_SYS_MAX_FLASH_BANKS	1
> @@ -56,6 +90,29 @@
>  #define CONFIG_SYS_FLASH_CFI
>  
>  /*
> + * NAND controller
> + */
> +#define CONFIG_NAND_LPC32XX_SLC
> +#define CONFIG_SYS_NAND_BASE		SLC_NAND_BASE
> +#define CONFIG_SYS_MAX_NAND_DEVICE	1
> +#define CONFIG_SYS_NAND_BASE_LIST	{ CONFIG_SYS_NAND_BASE }
> +
> +/*
> + * NAND chip timings
> + */
> +#define CONFIG_LPC32XX_NAND_SLC_WDR_CLKS	14
> +#define CONFIG_LPC32XX_NAND_SLC_WWIDTH		66666666
> +#define CONFIG_LPC32XX_NAND_SLC_WHOLD		200000000
> +#define CONFIG_LPC32XX_NAND_SLC_WSETUP		50000000
> +#define CONFIG_LPC32XX_NAND_SLC_RDR_CLKS	14
> +#define CONFIG_LPC32XX_NAND_SLC_RWIDTH		66666666
> +#define CONFIG_LPC32XX_NAND_SLC_RHOLD		200000000
> +#define CONFIG_LPC32XX_NAND_SLC_RSETUP		50000000
> +
> +#define CONFIG_SYS_NAND_USE_FLASH_BBT
> +#define CONFIG_CMD_NAND
> +
> +/*
>   * U-Boot General Configurations
>   */
>  #define CONFIG_SYS_LONGHELP
> @@ -71,8 +128,33 @@
>  #define CONFIG_DISPLAY_CPUINFO
>  #define CONFIG_DOS_PARTITION
>  
> -#define CONFIG_ENV_IS_NOWHERE
> +/*
> + * Pass open firmware flat tree
> + */
> +#define CONFIG_OF_LIBFDT
> +
> +/*
> + * Environment
> + */
> +#define CONFIG_ENV_IS_IN_NAND		1
>  #define CONFIG_ENV_SIZE			SZ_128K
> +#define CONFIG_ENV_OFFSET		0x000A0000

This in itself is an unanticipated change too, but I think it does not
affect current users of the boards much, except for a warning at boot
that the (NAND) env is invalid -- but the env does not contain much
anyway.

But how does this affect potential mapping of the NAND? Do Linux images
hard-code partitions on the NAND and is the address and size chosen
compatible with that, or do the images just not care about NAND?

> +#define CONFIG_BOOTCOMMAND			\
> +	"dhcp; "				\
> +	"tftp ${loadaddr} ${serverip}:${tftpdir}/${bootfile}; "		\
> +	"tftp ${dtbaddr} ${serverip}:${tftpdir}/devkit3250.dtb; "	\
> +	"setenv nfsargs ip=dhcp root=/dev/nfs nfsroot=${serverip}:${nfsroot},tcp; "	\
> +	"setenv bootargs ${bootargs} ${nfsargs} ${userargs}; "			\
> +	"bootm ${loadaddr} - ${dtbaddr}"
> +
> +#define CONFIG_EXTRA_ENV_SETTINGS		\
> +	"autoload=no\0"				\
> +	"ethaddr=00:01:90:00:C0:81\0"		\
> +	"dtbaddr=0x81000000\0"			\
> +	"nfsroot=/opt/projects/images/vladimir/oe/devkit3250/rootfs\0"	\
> +	"tftpdir=vladimir/oe/devkit3250\0"	\
> +	"userargs=oops=panic\0"
>  
>  /*
>   * U-Boot Commands
> @@ -85,10 +167,10 @@
>  #define CONFIG_CMDLINE_TAG
>  #define CONFIG_SETUP_MEMORY_TAGS
>  #define CONFIG_ZERO_BOOTDELAY_CHECK
> -#define CONFIG_BOOTDELAY		3
> +#define CONFIG_BOOTDELAY		1
>  
>  #define CONFIG_BOOTFILE			"uImage"
> -#define CONFIG_BOOTARGS			"console=ttyS2,115200n8"
> +#define CONFIG_BOOTARGS			"console=ttyS0,115200n8"
>  #define CONFIG_LOADADDR			0x80008000
>  
>  /*
> -- 
> 2.1.4
> 



Amicalement,
-- 
Albert.

[U-Boot] [PATCH 1/4] spl: nand: simple: replace readb() with chip specific read_buf()

[Albert ARIBAUD]

Hello Vladimir,

On Thu, 16 Jul 2015 02:33:45 +0300, Vladimir Zapolskiy <vz@mleia.com>
wrote:
> Some NAND controllers define custom functions to read data out,
> respect this in order to correctly support bad block handling in
> simple SPL NAND framework.
> 
> NAND controller specific read_buf() is used even to read 1 byte in
> case of connected 8-bit NAND device, it turns out that read_byte()
> may become outdated.
> 
> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
> ---
>  drivers/mtd/nand/nand_spl_simple.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_spl_simple.c b/drivers/mtd/nand/nand_spl_simple.c
> index 700ca32..e69f662 100644
> --- a/drivers/mtd/nand/nand_spl_simple.c
> +++ b/drivers/mtd/nand/nand_spl_simple.c
> @@ -115,6 +115,7 @@ static int nand_command(int block, int page, uint32_t offs,
>  static int nand_is_bad_block(int block)
>  {
>  	struct nand_chip *this = mtd.priv;
> +	u_char bb_data[2];
>  
>  	nand_command(block, 0, CONFIG_SYS_NAND_BAD_BLOCK_POS,
>  		NAND_CMD_READOOB);
> @@ -123,10 +124,12 @@ static int nand_is_bad_block(int block)
>  	 * Read one byte (or two if it's a 16 bit chip).
>  	 */
>  	if (this->options & NAND_BUSWIDTH_16) {
> -		if (readw(this->IO_ADDR_R) != 0xffff)
> +		this->read_buf(&mtd, bb_data, 2);
> +		if (bb_data[0] != 0xff || bb_data[1] != 0xff)
>  			return 1;
>  	} else {
> -		if (readb(this->IO_ADDR_R) != 0xff)
> +		this->read_buf(&mtd, bb_data, 1);
> +		if (bb_data[0] != 0xff)
>  			return 1;
>  	}
>  
> -- 
> 2.1.4
> 

The way you describe this patch, it looks like a bug that should have
bitten way more boards than lpc32xx. Did you have a look at some other
boards to see why this did not affect them? Conversively, what is the
actual failure scenario that led you to writing this patch? 

Amicalement,
-- 
Albert.

[U-Boot] [PATCH 1/4] spl: nand: simple: replace readb() with chip specific read_buf()

[Vladimir Zapolskiy]

Hello Albert,

On 16.07.2015 11:02, Albert ARIBAUD wrote:
> Hello Vladimir,
> 
> On Thu, 16 Jul 2015 02:33:45 +0300, Vladimir Zapolskiy <vz@mleia.com>
> wrote:
>> Some NAND controllers define custom functions to read data out,
>> respect this in order to correctly support bad block handling in
>> simple SPL NAND framework.
>>
>> NAND controller specific read_buf() is used even to read 1 byte in
>> case of connected 8-bit NAND device, it turns out that read_byte()
>> may become outdated.
>>
>> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
>> ---
>>  drivers/mtd/nand/nand_spl_simple.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/nand_spl_simple.c b/drivers/mtd/nand/nand_spl_simple.c
>> index 700ca32..e69f662 100644
>> --- a/drivers/mtd/nand/nand_spl_simple.c
>> +++ b/drivers/mtd/nand/nand_spl_simple.c
>> @@ -115,6 +115,7 @@ static int nand_command(int block, int page, uint32_t offs,
>>  static int nand_is_bad_block(int block)
>>  {
>>  	struct nand_chip *this = mtd.priv;
>> +	u_char bb_data[2];
>>  
>>  	nand_command(block, 0, CONFIG_SYS_NAND_BAD_BLOCK_POS,
>>  		NAND_CMD_READOOB);
>> @@ -123,10 +124,12 @@ static int nand_is_bad_block(int block)
>>  	 * Read one byte (or two if it's a 16 bit chip).
>>  	 */
>>  	if (this->options & NAND_BUSWIDTH_16) {
>> -		if (readw(this->IO_ADDR_R) != 0xffff)
>> +		this->read_buf(&mtd, bb_data, 2);
>> +		if (bb_data[0] != 0xff || bb_data[1] != 0xff)
>>  			return 1;
>>  	} else {
>> -		if (readb(this->IO_ADDR_R) != 0xff)
>> +		this->read_buf(&mtd, bb_data, 1);
>> +		if (bb_data[0] != 0xff)
>>  			return 1;
>>  	}
>>  
>> -- 
>> 2.1.4
>>
> 
> The way you describe this patch, it looks like a bug that should have
> bitten way more boards than lpc32xx. Did you have a look at some other
> boards to see why this did not affect them?

Yes, it is a bug IMHO.

Grepping for CONFIG_SPL_NAND_SIMPLE I see that TI and Tegra boards may
be impacted (positively or negatively):

* CONFIG_NAND_OMAP_GPMC --- own .read_buf(), default .read_byte()
* CONFIG_NAND_DAVINCI   --- own .read_buf(), default .read_byte()
* CONFIG_TEGRA_NAND     --- own .read_byte(), own .read_buf()

> Conversively, what is the actual failure scenario that led you to 
> writing this patch?

The failure scenario is quite simple, the ARM core gets stuck on first
attempt to access SLC NAND data register -- traced with JTAG.

The same happens, if I remove custom .read_byte()/.read_buf() from SLC
NAND driver. The only difference between custom .read_byte() and shared
read_byte() is in readb()/readl() access to the data register, it is
essential to have only 32-bit wide access to SLC NAND registers.

--
With best wishes,
Vladimir

[U-Boot] [PATCH 3/4] lpc32xx: devkit3250: update of board configuration

[Vladimir Zapolskiy]

Hello Albert,

On 16.07.2015 10:27, Albert ARIBAUD wrote:
> Hello Vladimir,
> 
> On Thu, 16 Jul 2015 02:33:47 +0300, Vladimir Zapolskiy <vz@mleia.com>
> wrote:
>> This change adds more peripherals to Timll DevKit3250 board, namely
>> MAC and SMSC phy, SLC NAND, GPIO, SPI and I2C.
>>
>> Also the default serial console is changed to UART5, added an option
>> to pass device tree blob by means of bootm, predefined environment
>> variables are slightly extended and reserved space on NAND to store
>> user defined U-boot environment.
>>
>> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
>> ---
>>  board/timll/devkit3250/devkit3250.c | 31 ++++++++++++-
>>  configs/devkit3250_defconfig        |  3 ++
>>  include/configs/devkit3250.h        | 92 +++++++++++++++++++++++++++++++++++--
>>  3 files changed, 120 insertions(+), 6 deletions(-)
>>
>> diff --git a/board/timll/devkit3250/devkit3250.c b/board/timll/devkit3250/devkit3250.c
>> index 6acc416..4b3c94e 100644
>> --- a/board/timll/devkit3250/devkit3250.c
>> +++ b/board/timll/devkit3250/devkit3250.c
>> @@ -1,23 +1,52 @@
>>  /*
>>   * Embest/Timll DevKit3250 board support
>>   *
>> - * Copyright (C) 2011 Vladimir Zapolskiy <vz@mleia.com>
>> + * Copyright (C) 2011-2015 Vladimir Zapolskiy <vz@mleia.com>
>>   *
>>   * SPDX-License-Identifier:	GPL-2.0+
>>   */
>>  
>>  #include <common.h>
>>  #include <asm/arch/sys_proto.h>
>> +#include <asm/arch/clk.h>
>>  #include <asm/arch/cpu.h>
>>  #include <asm/arch/emc.h>
>> +#include <asm/arch/wdt.h>
>> +#include <asm/io.h>
>>  
>>  DECLARE_GLOBAL_DATA_PTR;
>>  
>>  static struct emc_regs *emc = (struct emc_regs *)EMC_BASE;
>> +static struct clk_pm_regs *clk = (struct clk_pm_regs *)CLK_PM_BASE;
>> +static struct wdt_regs *wdt = (struct wdt_regs *)WDT_BASE;
>> +
>> +void reset_periph(void)
>> +{
>> +	/* This function resets peripherals by triggering RESOUT_N */
>> +	setbits_le32(&clk->timclk_ctrl, CLK_TIMCLK_WATCHDOG);
>> +	writel(WDTIM_MCTRL_RESFRC1, &wdt->mctrl);
>> +	udelay(300);
>> +
>> +	writel(0, &wdt->mctrl);
>> +	clrbits_le32(&clk->timclk_ctrl, CLK_TIMCLK_WATCHDOG);
>> +
>> +	/* Such a long delay is needed to initialize SMSC phy */
>> +	udelay(10000);
>> +}
>>  
>>  int board_early_init_f(void)
>>  {
>>  	lpc32xx_uart_init(CONFIG_SYS_LPC32XX_UART);
>> +	lpc32xx_i2c_init(1);
>> +	lpc32xx_i2c_init(2);
>> +	lpc32xx_ssp_init();
>> +	lpc32xx_mac_init();
>> +
>> +	/*
>> +	 * nWP may be controlled by GPO19, but unpopulated by default R23
>> +	 * makes no sense to configure this GPIO level, nWP is always high
>> +	 */
>> +	lpc32xx_slc_nand_init();
>>  
>>  	return 0;
>>  }
>> diff --git a/configs/devkit3250_defconfig b/configs/devkit3250_defconfig
>> index f0c4ee1..56d719f 100644
>> --- a/configs/devkit3250_defconfig
>> +++ b/configs/devkit3250_defconfig
>> @@ -1,3 +1,6 @@
>>  CONFIG_ARM=y
>>  CONFIG_TARGET_DEVKIT3250=y
>> +# CONFIG_CMD_FPGA is not set
>>  # CONFIG_CMD_SETEXPR is not set
>> +CONFIG_DM=y
>> +CONFIG_DM_GPIO=y
>> diff --git a/include/configs/devkit3250.h b/include/configs/devkit3250.h
>> index 4f35234..b8218b5 100644
>> --- a/include/configs/devkit3250.h
>> +++ b/include/configs/devkit3250.h
>> @@ -1,7 +1,7 @@
>>  /*
>>   * Embest/Timll DevKit3250 board configuration file
>>   *
>> - * Copyright (C) 2011 Vladimir Zapolskiy <vz@mleia.com>
>> + * Copyright (C) 2011-2015 Vladimir Zapolskiy <vz@mleia.com>
>>   *
>>   * SPDX-License-Identifier:	GPL-2.0+
>>   */
>> @@ -43,10 +43,44 @@
>>  /*
>>   * Serial Driver
>>   */
>> -#define CONFIG_SYS_LPC32XX_UART		2   /* UART2 */
>> +#define CONFIG_SYS_LPC32XX_UART		5   /* UART5 */
> 
> This will affect existing users of this board, who expect it to use
> UART2 as console. Was this expectation wrong, or can both UART2 and
> UART5 be used as console? IOW, is this change required or is it just
> easier for you secifically?

The board has 8 UARTs (provided by LPC32xx and SC16IS752IPW), 4 of them
can be used as a console interchangeably, namely LPC32xx UART2, UART3,
UART4 and UART5.

Previously UART2 was selected as a default UART, because it is a faster
14-clock UART, this plays a role if e.g. an image is downloaded over
ymodem. Now with MAC support in Uboot this is not needed, and preferably
to use UART5, which is mapped as a default serial console in Linux.

>>  #define CONFIG_BAUDRATE			115200
>>  
>>  /*
>> + * I2C
>> + */
>> +#define CONFIG_SYS_I2C
>> +#define CONFIG_SYS_I2C_LPC32XX
>> +#define CONFIG_SYS_I2C_SPEED		100000
>> +#define CONFIG_CMD_I2C
>> +
>> +/*
>> + * GPIO
>> + */
>> +#define CONFIG_LPC32XX_GPIO
>> +#define CONFIG_CMD_GPIO
>> +
>> +/*
>> + * SSP/SPI
>> + */
>> +#define CONFIG_LPC32XX_SSP
>> +#define CONFIG_LPC32XX_SSP_TIMEOUT	100000
>> +#define CONFIG_CMD_SPI
>> +
>> +/*
>> + * Ethernet
>> + */
>> +#define CONFIG_RMII
>> +#define CONFIG_PHY_SMSC
>> +#define CONFIG_LPC32XX_ETH
>> +#define CONFIG_PHYLIB
>> +#define CONFIG_PHY_ADDR			0x1F
>> +#define CONFIG_SYS_FAULT_ECHO_LINK_DOWN
>> +#define CONFIG_CMD_MII
>> +#define CONFIG_CMD_PING
>> +#define CONFIG_CMD_DHCP
>> +
>> +/*
>>   * NOR Flash
>>   */
>>  #define CONFIG_SYS_MAX_FLASH_BANKS	1
>> @@ -56,6 +90,29 @@
>>  #define CONFIG_SYS_FLASH_CFI
>>  
>>  /*
>> + * NAND controller
>> + */
>> +#define CONFIG_NAND_LPC32XX_SLC
>> +#define CONFIG_SYS_NAND_BASE		SLC_NAND_BASE
>> +#define CONFIG_SYS_MAX_NAND_DEVICE	1
>> +#define CONFIG_SYS_NAND_BASE_LIST	{ CONFIG_SYS_NAND_BASE }
>> +
>> +/*
>> + * NAND chip timings
>> + */
>> +#define CONFIG_LPC32XX_NAND_SLC_WDR_CLKS	14
>> +#define CONFIG_LPC32XX_NAND_SLC_WWIDTH		66666666
>> +#define CONFIG_LPC32XX_NAND_SLC_WHOLD		200000000
>> +#define CONFIG_LPC32XX_NAND_SLC_WSETUP		50000000
>> +#define CONFIG_LPC32XX_NAND_SLC_RDR_CLKS	14
>> +#define CONFIG_LPC32XX_NAND_SLC_RWIDTH		66666666
>> +#define CONFIG_LPC32XX_NAND_SLC_RHOLD		200000000
>> +#define CONFIG_LPC32XX_NAND_SLC_RSETUP		50000000
>> +
>> +#define CONFIG_SYS_NAND_USE_FLASH_BBT
>> +#define CONFIG_CMD_NAND
>> +
>> +/*
>>   * U-Boot General Configurations
>>   */
>>  #define CONFIG_SYS_LONGHELP
>> @@ -71,8 +128,33 @@
>>  #define CONFIG_DISPLAY_CPUINFO
>>  #define CONFIG_DOS_PARTITION
>>  
>> -#define CONFIG_ENV_IS_NOWHERE
>> +/*
>> + * Pass open firmware flat tree
>> + */
>> +#define CONFIG_OF_LIBFDT
>> +
>> +/*
>> + * Environment
>> + */
>> +#define CONFIG_ENV_IS_IN_NAND		1
>>  #define CONFIG_ENV_SIZE			SZ_128K
>> +#define CONFIG_ENV_OFFSET		0x000A0000
> 
> This in itself is an unanticipated change too, but I think it does not
> affect current users of the boards much, except for a warning at boot
> that the (NAND) env is invalid -- but the env does not contain much
> anyway.

Correct, new users will get a warning until their partition layout is
updated.

> But how does this affect potential mapping of the NAND? Do Linux images
> hard-code partitions on the NAND and is the address and size chosen
> compatible with that, or do the images just not care about NAND?

The default manufacturer's NAND layout is slightly changed, but kernel
and rootfs partitions are untouched, for users who care, kernel will be
updated to reflect this change.

Previously only S1L partition occupied pages 0x00000 - 0xc0000, new
layout is:

  0x00000 - 0x40000 - S1L / SPL
  0x40000 - 0xa0000 - U-boot
  0xa0000 - 0xc0000 - U-boot environment

>> +#define CONFIG_BOOTCOMMAND			\
>> +	"dhcp; "				\
>> +	"tftp ${loadaddr} ${serverip}:${tftpdir}/${bootfile}; "		\
>> +	"tftp ${dtbaddr} ${serverip}:${tftpdir}/devkit3250.dtb; "	\
>> +	"setenv nfsargs ip=dhcp root=/dev/nfs nfsroot=${serverip}:${nfsroot},tcp; "	\
>> +	"setenv bootargs ${bootargs} ${nfsargs} ${userargs}; "			\
>> +	"bootm ${loadaddr} - ${dtbaddr}"
>> +
>> +#define CONFIG_EXTRA_ENV_SETTINGS		\
>> +	"autoload=no\0"				\
>> +	"ethaddr=00:01:90:00:C0:81\0"		\
>> +	"dtbaddr=0x81000000\0"			\
>> +	"nfsroot=/opt/projects/images/vladimir/oe/devkit3250/rootfs\0"	\
>> +	"tftpdir=vladimir/oe/devkit3250\0"	\
>> +	"userargs=oops=panic\0"
>>  
>>  /*
>>   * U-Boot Commands
>> @@ -85,10 +167,10 @@
>>  #define CONFIG_CMDLINE_TAG
>>  #define CONFIG_SETUP_MEMORY_TAGS
>>  #define CONFIG_ZERO_BOOTDELAY_CHECK
>> -#define CONFIG_BOOTDELAY		3
>> +#define CONFIG_BOOTDELAY		1
>>  
>>  #define CONFIG_BOOTFILE			"uImage"
>> -#define CONFIG_BOOTARGS			"console=ttyS2,115200n8"
>> +#define CONFIG_BOOTARGS			"console=ttyS0,115200n8"
>>  #define CONFIG_LOADADDR			0x80008000
>>  
>>  /*
>> -- 
>> 2.1.4
>>

--
With best wishes,
Vladimir

[U-Boot] [PATCH 1/4] spl: nand: simple: replace readb() with chip specific read_buf()

[Albert ARIBAUD]

Hello Vladimir,

On Thu, 16 Jul 2015 14:31:26 +0300, Vladimir Zapolskiy <vz@mleia.com>
wrote:
> Hello Albert,
> 
> On 16.07.2015 11:02, Albert ARIBAUD wrote:
> > Hello Vladimir,
> > 
> > On Thu, 16 Jul 2015 02:33:45 +0300, Vladimir Zapolskiy <vz@mleia.com>
> > wrote:
> >> Some NAND controllers define custom functions to read data out,
> >> respect this in order to correctly support bad block handling in
> >> simple SPL NAND framework.
> >>
> >> NAND controller specific read_buf() is used even to read 1 byte in
> >> case of connected 8-bit NAND device, it turns out that read_byte()
> >> may become outdated.
> >>
> >> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
> >> ---
> >>  drivers/mtd/nand/nand_spl_simple.c | 7 +++++--
> >>  1 file changed, 5 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/mtd/nand/nand_spl_simple.c b/drivers/mtd/nand/nand_spl_simple.c
> >> index 700ca32..e69f662 100644
> >> --- a/drivers/mtd/nand/nand_spl_simple.c
> >> +++ b/drivers/mtd/nand/nand_spl_simple.c
> >> @@ -115,6 +115,7 @@ static int nand_command(int block, int page, uint32_t offs,
> >>  static int nand_is_bad_block(int block)
> >>  {
> >>  	struct nand_chip *this = mtd.priv;
> >> +	u_char bb_data[2];
> >>  
> >>  	nand_command(block, 0, CONFIG_SYS_NAND_BAD_BLOCK_POS,
> >>  		NAND_CMD_READOOB);
> >> @@ -123,10 +124,12 @@ static int nand_is_bad_block(int block)
> >>  	 * Read one byte (or two if it's a 16 bit chip).
> >>  	 */
> >>  	if (this->options & NAND_BUSWIDTH_16) {
> >> -		if (readw(this->IO_ADDR_R) != 0xffff)
> >> +		this->read_buf(&mtd, bb_data, 2);
> >> +		if (bb_data[0] != 0xff || bb_data[1] != 0xff)
> >>  			return 1;
> >>  	} else {
> >> -		if (readb(this->IO_ADDR_R) != 0xff)
> >> +		this->read_buf(&mtd, bb_data, 1);
> >> +		if (bb_data[0] != 0xff)
> >>  			return 1;
> >>  	}
> >>  
> >> -- 
> >> 2.1.4
> >>
> > 
> > The way you describe this patch, it looks like a bug that should have
> > bitten way more boards than lpc32xx. Did you have a look at some other
> > boards to see why this did not affect them?
> 
> Yes, it is a bug IMHO.

If it is, then it has hit no board which defines CONFIG_SPL_NAND_SIMPLE
and we should understand why.

> Grepping for CONFIG_SPL_NAND_SIMPLE I see that TI and Tegra boards may
> be impacted (positively or negatively):
> 
> * CONFIG_NAND_OMAP_GPMC --- own .read_buf(), default .read_byte()
> * CONFIG_NAND_DAVINCI   --- own .read_buf(), default .read_byte()
> * CONFIG_TEGRA_NAND     --- own .read_byte(), own .read_buf()

They may be impacted by your change, but they are working now -- they
are not obscure models. Let's dig a bit...

> > Conversively, what is the actual failure scenario that led you to 
> > writing this patch?
> 
> The failure scenario is quite simple, the ARM core gets stuck on first
> attempt to access SLC NAND data register -- traced with JTAG.
> 
> The same happens, if I remove custom .read_byte()/.read_buf() from SLC
> NAND driver. The only difference between custom .read_byte() and shared
> read_byte() is in readb()/readl() access to the data register, it is
> essential to have only 32-bit wide access to SLC NAND registers.

README describes CONFIG_SPL_NAND_SIMPLE as "Support for NAND boot using
simple NAND drivers that expose the cmd_ctrl() interface". The cmd_ctrl
interface actually contains the cmd_ctrl() function *and* the
IO_ADDR_[RW] fields. IOW, a simple NAND driver provides byte or word
access to the NAND's I/O lines on which command, address and data are
passed. If the NAND is 8 bits, then there are 8 lines; if it is 16
bit, then there are 16 lines. 

I reckon that the OMAP/DAVINCI and TEGRA simple drivers just do that:
set IO_ADDR_[RW] to the IP register through which direct access to the
NAND's I/O lines can be performed, byte or word depending on the chip
width.

As such, there is no bug in the way nand_simple.c uses IO_ADDR_[RW].

OTOH, the SLC IP does not provide direct access to the NAND I/O lines
through a general register; what it provides is a set of three
specialized registers one for commands, one for addresses and one
for data. Plus, even though only 8 bit NANDs are supported, the data
register does not physically support 8-bit wide accesses (NXP: I am
still struggling to understand what you were trying to achieve there).

All this basically makes the SLC controller a 'not simple NAND IP', and
its driver a 'not simple NAND driver' incompatible with nand_simple.c.

Your solution to this incompatibility is to change nand_simple.c to
support other types of drivers; but by doing that, you're changing the
API between nand_simple.c and all simple drivers, possibly changing the
established behavior.

I personally don't think this is the right way; nand_simple.c should be
left unchanged and the board should simply not use it since it does not
have a simple NAND controller, and instead it should provide its own
nand_spl_load_image().

But hey, I'm not then NAND custodian. Scott: your call. :)

> --
> With best wishes,
> Vladimir

Amicalement,
-- 
Albert.

[U-Boot] [PATCH 3/4] lpc32xx: devkit3250: update of board configuration

[Albert ARIBAUD]

Hello Vladimir,

Thanks for the explanations, OK for both.

Amicalement,
-- 
Albert.

[U-Boot] [PATCH 2/4] nand: lpc32xx: add SLC NAND controller support

[Albert ARIBAUD]

Hello Vladimir,

On Thu, 16 Jul 2015 02:33:46 +0300, Vladimir Zapolskiy <vz@mleia.com>
wrote:

> diff --git a/drivers/mtd/nand/lpc32xx_nand_slc.c b/drivers/mtd/nand/lpc32xx_nand_slc.c
> new file mode 100644

> +int board_nand_init(struct nand_chip *lpc32xx_chip)
> +{
> +	lpc32xx_chip->IO_ADDR_R	= &lpc32xx_nand_slc_registers->data;
> +	lpc32xx_chip->IO_ADDR_W	= &lpc32xx_nand_slc_registers->data;

Consistent with my comment re nand_simpl.c, I think that the two
assignments above are incorrect since the data register may not provide
general access to the NAND's I/O lines, and is not 8-bit accessible
even though the NAND is 8-bit wide.

If Scott give his go, though, disregard my comment.
 
Amicalement,
-- 
Albert.

[U-Boot] [PATCH 1/4] spl: nand: simple: replace readb() with chip specific read_buf()

[Vladimir Zapolskiy]

Hello Albert,

On 16.07.2015 15:43, Albert ARIBAUD wrote:
> Hello Vladimir,
> 
> On Thu, 16 Jul 2015 14:31:26 +0300, Vladimir Zapolskiy <vz@mleia.com>
> wrote:
>> Hello Albert,
>>
>> On 16.07.2015 11:02, Albert ARIBAUD wrote:
>>> Hello Vladimir,
>>>
>>> On Thu, 16 Jul 2015 02:33:45 +0300, Vladimir Zapolskiy <vz@mleia.com>
>>> wrote:
>>>> Some NAND controllers define custom functions to read data out,
>>>> respect this in order to correctly support bad block handling in
>>>> simple SPL NAND framework.
>>>>
>>>> NAND controller specific read_buf() is used even to read 1 byte in
>>>> case of connected 8-bit NAND device, it turns out that read_byte()
>>>> may become outdated.
>>>>
>>>> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
>>>> ---
>>>>  drivers/mtd/nand/nand_spl_simple.c | 7 +++++--
>>>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/nand/nand_spl_simple.c b/drivers/mtd/nand/nand_spl_simple.c
>>>> index 700ca32..e69f662 100644
>>>> --- a/drivers/mtd/nand/nand_spl_simple.c
>>>> +++ b/drivers/mtd/nand/nand_spl_simple.c
>>>> @@ -115,6 +115,7 @@ static int nand_command(int block, int page, uint32_t offs,
>>>>  static int nand_is_bad_block(int block)
>>>>  {
>>>>  	struct nand_chip *this = mtd.priv;
>>>> +	u_char bb_data[2];
>>>>  
>>>>  	nand_command(block, 0, CONFIG_SYS_NAND_BAD_BLOCK_POS,
>>>>  		NAND_CMD_READOOB);
>>>> @@ -123,10 +124,12 @@ static int nand_is_bad_block(int block)
>>>>  	 * Read one byte (or two if it's a 16 bit chip).
>>>>  	 */
>>>>  	if (this->options & NAND_BUSWIDTH_16) {
>>>> -		if (readw(this->IO_ADDR_R) != 0xffff)
>>>> +		this->read_buf(&mtd, bb_data, 2);
>>>> +		if (bb_data[0] != 0xff || bb_data[1] != 0xff)
>>>>  			return 1;
>>>>  	} else {
>>>> -		if (readb(this->IO_ADDR_R) != 0xff)
>>>> +		this->read_buf(&mtd, bb_data, 1);
>>>> +		if (bb_data[0] != 0xff)
>>>>  			return 1;
>>>>  	}
>>>>  
>>>> -- 
>>>> 2.1.4
>>>>
>>>
>>> The way you describe this patch, it looks like a bug that should have
>>> bitten way more boards than lpc32xx. Did you have a look at some other
>>> boards to see why this did not affect them?
>>
>> Yes, it is a bug IMHO.
> 
> If it is, then it has hit no board which defines CONFIG_SPL_NAND_SIMPLE
> and we should understand why.
> 
>> Grepping for CONFIG_SPL_NAND_SIMPLE I see that TI and Tegra boards may
>> be impacted (positively or negatively):
>>
>> * CONFIG_NAND_OMAP_GPMC --- own .read_buf(), default .read_byte()
>> * CONFIG_NAND_DAVINCI   --- own .read_buf(), default .read_byte()
>> * CONFIG_TEGRA_NAND     --- own .read_byte(), own .read_buf()
> 
> They may be impacted by your change, but they are working now -- they
> are not obscure models. Let's dig a bit...

For simplicity let's neglect 16-bit NANDs at the moment, readb() can be
replaced by readw() etc. in my message.

You may notice that nand_spl_simple.c exploits NAND driver specific
.read_buf(), therefore the latter one must be defined (by driver or NAND
framework), can it happen that .read_buf(..., 1) returns a different
result from readb()?

I hope in all cases above .read_buf(..., 1) is equal to common readb(),
so okay, this is not a bug fix for currently supported drivers, but a
misfeature, which does not allow to reuse nand_spl_simple.c for a
slightly different NAND controller.

Fortunately the proposed generalization of nand_spl_simple.c is
straightforward.

>>> Conversively, what is the actual failure scenario that led you to 
>>> writing this patch?
>>
>> The failure scenario is quite simple, the ARM core gets stuck on first
>> attempt to access SLC NAND data register -- traced with JTAG.
>>
>> The same happens, if I remove custom .read_byte()/.read_buf() from SLC
>> NAND driver. The only difference between custom .read_byte() and shared
>> read_byte() is in readb()/readl() access to the data register, it is
>> essential to have only 32-bit wide access to SLC NAND registers.
> 
> README describes CONFIG_SPL_NAND_SIMPLE as "Support for NAND boot using
> simple NAND drivers that expose the cmd_ctrl() interface". The cmd_ctrl
> interface actually contains the cmd_ctrl() function *and* the
> IO_ADDR_[RW] fields. IOW, a simple NAND driver provides byte or word
> access to the NAND's I/O lines on which command, address and data are
> passed. If the NAND is 8 bits, then there are 8 lines; if it is 16
> bit, then there are 16 lines. 

Please see my note above about read_buf().

nand_spl_simple.c is designed in such a way that it uses NAND driver
specific interfaces (ECC specific interfaces are omitted here):

* cmd_ctrl()
* dev_ready()
* read_buf()

I agree that some of these interfaces may be populated by default NAND
framework, if it is a deliberate intention to have only default
functions, it might be better to hardcode default functions into
nand_spl_simple.c , but this destroys flexibility.

I would say README description of CONFIG_SPL_NAND_SIMPLE is not
precisely correct.

> I reckon that the OMAP/DAVINCI and TEGRA simple drivers just do that:
> set IO_ADDR_[RW] to the IP register through which direct access to the
> NAND's I/O lines can be performed, byte or word depending on the chip
> width.
> 
> As such, there is no bug in the way nand_simple.c uses IO_ADDR_[RW].

Okay, there is no bug in currently supported drivers, I believe.

> OTOH, the SLC IP does not provide direct access to the NAND I/O lines
> through a general register; what it provides is a set of three
> specialized registers one for commands, one for addresses and one
> for data. Plus, even though only 8 bit NANDs are supported, the data
> register does not physically support 8-bit wide accesses (NXP: I am
> still struggling to understand what you were trying to achieve there).
> 
> All this basically makes the SLC controller a 'not simple NAND IP', and
> its driver a 'not simple NAND driver' incompatible with nand_simple.c.
> 
> Your solution to this incompatibility is to change nand_simple.c to
> support other types of drivers; but by doing that, you're changing the
> API between nand_simple.c and all simple drivers, possibly changing the
> established behavior.

Could my statement be confirmed or rejected?

   One byte data read is the same if .read_buf(..., 1), .read_byte() or
readb() is used on TI and Tegra platforms, and the interfaces can be
interchanged.

I haven't tested it, but I will be surprised, if my statement is wrong.

> I personally don't think this is the right way; nand_simple.c should be
> left unchanged and the board should simply not use it since it does not
> have a simple NAND controller, and instead it should provide its own
> nand_spl_load_image().

For me an alternative change to the proposed one is to duplicate
nand_spl_simple.c functionality in LPC32xx SLC NAND driver. From
maintenance point of view this is not the best thing to do IMHO.

> But hey, I'm not then NAND custodian. Scott: your call. :)

Anyway thank you for review and comments.

--
With best wishes,
Vladimir

[U-Boot] [PATCH 1/4] spl: nand: simple: replace readb() with chip specific read_buf()

[Albert ARIBAUD]

Hello Vladimir,

On Thu, 16 Jul 2015 16:48:26 +0300, Vladimir Zapolskiy <vz@mleia.com>
wrote:

(cutting short to the essential point remaining)

> > I personally don't think this is the right way; nand_simple.c should be
> > left unchanged and the board should simply not use it since it does not
> > have a simple NAND controller, and instead it should provide its own
> > nand_spl_load_image().
> 
> For me an alternative change to the proposed one is to duplicate
> nand_spl_simple.c functionality in LPC32xx SLC NAND driver. From
> maintenance point of view this is not the best thing to do IMHO.

You're right that some of the functionality present in nand_simple.c is
duplicated elsewhere; however, that functionality is not the one specific
to nand_simple; actually, it is nand_spl_load_image(), the overall load
functionality which should be in no driver at all but of which I see ten
incarnations in ten drivers.

There should be only one nand_spl_load_image(), in its own file, with
placeholders for driver-specific actions such as page read, bad page
check, etc. One day, maybe, there will be a patch for that.

Anyway: you propose changes in the design of the NAND subsystem so that
the SLC controller driver can use the "simple" model, while I think the
SLC controller is not "simple" so it should not be managed as one. This
dilemma is about the NAND subsystem design, which is waaaay outside my
province -- hence my deferring to Scott, who will decide which way this
should go.

> > But hey, I'm not then NAND custodian. Scott: your call. :)
> 
> Anyway thank you for review and comments.

You're welcome!

> --
> With best wishes,
> Vladimir

Amicalement,
-- 
Albert.

[U-Boot] [PATCH 1/4] spl: nand: simple: replace readb() with chip specific read_buf()

[Scott Wood]

On Thu, 2015-07-16 at 16:12 +0200, Albert ARIBAUD wrote:
> Hello Vladimir,
> 
> On Thu, 16 Jul 2015 16:48:26 +0300, Vladimir Zapolskiy <vz@mleia.com>
> wrote:
> 
> (cutting short to the essential point remaining)
> 
> > > I personally don't think this is the right way; nand_simple.c should be
> > > left unchanged and the board should simply not use it since it does not
> > > have a simple NAND controller, and instead it should provide its own
> > > nand_spl_load_image().

I don't have a problem with expanding the definition of "simple" to include 
custom read_byte and such, as long as we make sure that existing targets 
don't break.  For drivers that don't supply these functions, where are the 
default functions coming from?  I don't see nand_spl_simple's nand_init() 
initializing them.

> > For me an alternative change to the proposed one is to duplicate
> > nand_spl_simple.c functionality in LPC32xx SLC NAND driver. From
> > maintenance point of view this is not the best thing to do IMHO.
> 
> You're right that some of the functionality present in nand_simple.c is
> duplicated elsewhere; however, that functionality is not the one specific
> to nand_simple; actually, it is nand_spl_load_image(), the overall load
> functionality which should be in no driver at all but of which I see ten
> incarnations in ten drivers.
> 
> There should be only one nand_spl_load_image(), in its own file, with
> placeholders for driver-specific actions such as page read, bad page
> check, etc. One day, maybe, there will be a patch for that.

You'll never get it down to just one, since there are some targets that rely 
on the tight integration within a single file to squeak in under a 4 KiB size 
limit.  Maybe some of the implementations could be replaced with an optional 
generic version, though.

-Scott

[U-Boot] [PATCH 2/4] nand: lpc32xx: add SLC NAND controller support

[Scott Wood]

On Thu, 2015-07-16 at 14:53 +0200, Albert ARIBAUD wrote:
> Hello Vladimir,
> 
> On Thu, 16 Jul 2015 02:33:46 +0300, Vladimir Zapolskiy <vz@mleia.com>
> wrote:
> 
> > diff --git a/drivers/mtd/nand/lpc32xx_nand_slc.c 
> > b/drivers/mtd/nand/lpc32xx_nand_slc.c
> > new file mode 100644
> 
> > +int board_nand_init(struct nand_chip *lpc32xx_chip)
> > +{
> > +   lpc32xx_chip->IO_ADDR_R = &lpc32xx_nand_slc_registers->data;
> > +   lpc32xx_chip->IO_ADDR_W = &lpc32xx_nand_slc_registers->data;
> 
> Consistent with my comment re nand_simpl.c, I think that the two
> assignments above are incorrect since the data register may not provide
> general access to the NAND's I/O lines, and is not 8-bit accessible
> even though the NAND is 8-bit wide.
> 
> If Scott give his go, though, disregard my comment.

I agree with Albert -- if you need custom accessors, there's no benefit to 
using IO_ADDR_R/W, and it could make it harder to debug if some user of 
IO_ADDR_R/W is missed.

-Scott

[U-Boot] [PATCH 2/4] nand: lpc32xx: add SLC NAND controller support

[Vladimir Zapolskiy]

Hello Scott,

On 16.07.2015 23:14, Scott Wood wrote:
> On Thu, 2015-07-16 at 14:53 +0200, Albert ARIBAUD wrote:
>> Hello Vladimir,
>>
>> On Thu, 16 Jul 2015 02:33:46 +0300, Vladimir Zapolskiy <vz@mleia.com>
>> wrote:
>>
>>> diff --git a/drivers/mtd/nand/lpc32xx_nand_slc.c 
>>> b/drivers/mtd/nand/lpc32xx_nand_slc.c
>>> new file mode 100644
>>
>>> +int board_nand_init(struct nand_chip *lpc32xx_chip)
>>> +{
>>> +   lpc32xx_chip->IO_ADDR_R = &lpc32xx_nand_slc_registers->data;
>>> +   lpc32xx_chip->IO_ADDR_W = &lpc32xx_nand_slc_registers->data;
>>
>> Consistent with my comment re nand_simpl.c, I think that the two
>> assignments above are incorrect since the data register may not provide
>> general access to the NAND's I/O lines, and is not 8-bit accessible
>> even though the NAND is 8-bit wide.
>>
>> If Scott give his go, though, disregard my comment.
> 
> I agree with Albert -- if you need custom accessors, there's no benefit to 
> using IO_ADDR_R/W, and it could make it harder to debug if some user of 
> IO_ADDR_R/W is missed.

No objections from my side regarding this notice, I'll remove these two
lines in v2. Please let me know, if you notice something else, what can
be improved.

--
With best wishes,
Vladimir

[U-Boot] [PATCH 2/4] nand: lpc32xx: add SLC NAND controller support

[Scott Wood]

On Thu, 2015-07-16 at 02:33 +0300, Vladimir Zapolskiy wrote:
> +static void lpc32xx_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
> +{
> +     while (len-- > 0)
> +             *buf++ = (uint8_t)readl(&lpc32xx_nand_slc_registers->data);
> +}
> +
> +static uint8_t lpc32xx_read_byte(struct mtd_info *mtd)
> +{
> +     return (uint8_t)readl(&lpc32xx_nand_slc_registers->data);
> +}
> +
> +static void lpc32xx_write_buf(struct mtd_info *mtd, const uint8_t *buf, 
> int len)
> +{
> +     while (len-- > 0)
> +             writel((uint32_t)*buf++, &lpc32xx_nand_slc_registers->data);
> +}
> +
> +static void lpc32xx_write_byte(struct mtd_info *mtd, uint8_t byte)
> +{
> +     writel((uint32_t)byte, &lpc32xx_nand_slc_registers->data);
> +}

Unnecessary casts.

> +     /*
> +      * Hardware ECC calculation is not supported by the driver, because it
> +      * requires DMA support, see Note after SLC_ECC register description
> +      */
> +     lpc32xx_chip->ecc.mode  = NAND_ECC_SOFT;

Where can I find this note?  I don't see any  "SLC_ECC register description".

> +#if defined(CONFIG_SPL_BUILD)
> +     lpc32xx_chip->options           |= NAND_SKIP_BBTSCAN;
> +#endif

Does this make any difference?  nand_spl_simple will not do a bbt scan in any 
case.

-Scott

[U-Boot] [PATCH 2/4] nand: lpc32xx: add SLC NAND controller support

[Vladimir Zapolskiy]

On 16.07.2015 23:20, Scott Wood wrote:
> On Thu, 2015-07-16 at 02:33 +0300, Vladimir Zapolskiy wrote:
>> +static void lpc32xx_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
>> +{
>> +     while (len-- > 0)
>> +             *buf++ = (uint8_t)readl(&lpc32xx_nand_slc_registers->data);
>> +}
>> +
>> +static uint8_t lpc32xx_read_byte(struct mtd_info *mtd)
>> +{
>> +     return (uint8_t)readl(&lpc32xx_nand_slc_registers->data);
>> +}
>> +
>> +static void lpc32xx_write_buf(struct mtd_info *mtd, const uint8_t *buf, 
>> int len)
>> +{
>> +     while (len-- > 0)
>> +             writel((uint32_t)*buf++, &lpc32xx_nand_slc_registers->data);
>> +}
>> +
>> +static void lpc32xx_write_byte(struct mtd_info *mtd, uint8_t byte)
>> +{
>> +     writel((uint32_t)byte, &lpc32xx_nand_slc_registers->data);
>> +}
> 
> Unnecessary casts.

Ok.

>> +     /*
>> +      * Hardware ECC calculation is not supported by the driver, because it
>> +      * requires DMA support, see Note after SLC_ECC register description
>> +      */
>> +     lpc32xx_chip->ecc.mode  = NAND_ECC_SOFT;
> 
> Where can I find this note?  I don't see any  "SLC_ECC register description".

This is a reference to SLC_ECC register description from LPC32xx User's
Manual, will state it clearly in v2.

>> +#if defined(CONFIG_SPL_BUILD)
>> +     lpc32xx_chip->options           |= NAND_SKIP_BBTSCAN;
>> +#endif
> 
> Does this make any difference?  nand_spl_simple will not do a bbt scan in any 
> case.

Agree, I will remove it.

--
With best wishes,
Vladimir