[U-Boot,2/2] nand: sunxi: Add support for booting from internal NAND memory

From: Daniel Kochmański <dkochmanski@turtle-solutions.eu>

V2:
- Rename config option
- Move to separate driver
- fix DMA directly into RAM
- Many readability upgrades
- Drop R32 and W32 macros in favour of readl/writel respectively
- Use standard port controller methods for pinctl
- Make many NAND options semi-configurable

Signed-off-by: Roy Spliet <r.spliet@ultimaker.com>
---
 arch/arm/cpu/armv7/sunxi/board.c       |  12 +-
 arch/arm/include/asm/arch-sunxi/gpio.h |   2 +
 board/sunxi/Kconfig                    |  12 ++
 board/sunxi/board.c                    |  27 +++
 drivers/mtd/nand/Makefile              |   1 +
 drivers/mtd/nand/sunxi_nand_spl.c      | 290 +++++++++++++++++++++++++++++++++
 include/configs/sun4i.h                |   1 +
 include/configs/sun5i.h                |   3 +
 include/configs/sun7i.h                |   2 +
 include/configs/sun8i.h                |   6 +
 include/configs/sunxi-common.h         |  20 +++
 11 files changed, 374 insertions(+), 2 deletions(-)
 create mode 100644 drivers/mtd/nand/sunxi_nand_spl.c

Hi,

On 21-05-15 15:59, Roy Spliet wrote:
> From: Daniel Kochmański <dkochmanski@turtle-solutions.eu>
>
> V2:
> - Rename config option
> - Move to separate driver
> - fix DMA directly into RAM
> - Many readability upgrades
> - Drop R32 and W32 macros in favour of readl/writel respectively
> - Use standard port controller methods for pinctl
> - Make many NAND options semi-configurable
>
> Signed-off-by: Roy Spliet <r.spliet@ultimaker.com>
> ---
>   arch/arm/cpu/armv7/sunxi/board.c       |  12 +-
>   arch/arm/include/asm/arch-sunxi/gpio.h |   2 +
>   board/sunxi/Kconfig                    |  12 ++
>   board/sunxi/board.c                    |  27 +++
>   drivers/mtd/nand/Makefile              |   1 +
>   drivers/mtd/nand/sunxi_nand_spl.c      | 290 +++++++++++++++++++++++++++++++++
>   include/configs/sun4i.h                |   1 +
>   include/configs/sun5i.h                |   3 +
>   include/configs/sun7i.h                |   2 +
>   include/configs/sun8i.h                |   6 +
>   include/configs/sunxi-common.h         |  20 +++
>   11 files changed, 374 insertions(+), 2 deletions(-)
>   create mode 100644 drivers/mtd/nand/sunxi_nand_spl.c
>
> diff --git a/arch/arm/cpu/armv7/sunxi/board.c b/arch/arm/cpu/armv7/sunxi/board.c
> index 6718ae2..70f413f 100644
> --- a/arch/arm/cpu/armv7/sunxi/board.c
> +++ b/arch/arm/cpu/armv7/sunxi/board.c
> @@ -111,8 +111,10 @@ void s_init(void)
>   #ifdef CONFIG_SPL_BUILD
>   /* The sunxi internal brom will try to loader external bootloader
>    * from mmc0, nand flash, mmc2.
> - * Unfortunately we can't check how SPL was loaded so assume
> - * it's always the first SD/MMC controller
> + *
> + * Unfortunately we can't check how SPL was loaded so assume it's
> + * always the first SD/MMC controller, unless it was explicitly
> + * stated that SPL is on nand flash.
>    */
>   u32 spl_boot_device(void)
>   {
> @@ -122,6 +124,12 @@ u32 spl_boot_device(void)
>   	 * enabled build. It has many restrictions and can only boot over USB.
>   	 */
>   	return BOOT_DEVICE_BOARD;
> +#elif defined(CONFIG_SPL_NAND_SUPPORT)
> +	/*
> +	 * This is compile time configuration informing SPL, that it
> +	 * was loaded from nand flash.
> +	 */
> +	return BOOT_DEVICE_NAND;
>   #else
>   	/*
>   	 * When booting from the SD card, the "eGON.BT0" signature is expected
> diff --git a/arch/arm/include/asm/arch-sunxi/gpio.h b/arch/arm/include/asm/arch-sunxi/gpio.h
> index 59d8210..2b49616 100644
> --- a/arch/arm/include/asm/arch-sunxi/gpio.h
> +++ b/arch/arm/include/asm/arch-sunxi/gpio.h
> @@ -156,6 +156,8 @@ enum sunxi_gpio_number {
>   #define SUN4I_GPB_UART0		2
>   #define SUN5I_GPB_UART0		2
>
> +#define SUNXI_GPC_NAND		2
> +
>   #define SUNXI_GPC_SDC2		3
>   #define SUN6I_GPC_SDC3		4
>
> diff --git a/board/sunxi/Kconfig b/board/sunxi/Kconfig
> index a60d028..cf58d73 100644
> --- a/board/sunxi/Kconfig
> +++ b/board/sunxi/Kconfig
> @@ -269,6 +269,18 @@ config MMC_SUNXI_SLOT_EXTRA
>   	slot or emmc on mmc1 - mmc3. Setting this to 1, 2 or 3 will enable
>   	support for this.
>
> +config SPL_NAND_SUPPORT
> +	bool "SPL/NAND mode support"
> +	depends on SPL
> +	default n
> +	---help---
> +	  This enables support for booting from NAND internal
> +	  memory. U-Boot SPL doesn't detect where is it load from,
> +	  therefore this option is needed to properly load image from
> +	  flash. Option also disables MMC functionality on U-Boot due to
> +	  initialization errors encountered, when both controllers are
> +	  enabled.
> +
>   config USB0_VBUS_PIN
>   	string "Vbus enable pin for usb0 (otg)"
>   	default ""

There is a way to figure out if you're booting from sdcard or nand
actually, simply check if an sdcard is there and if it has the boot0
signature, if that is true, then we should be booting from sd, as those
are the checks the brom does itself to determine what to boot (*).

Since we support booting from internal emmc on boars which have it.
and those are connected to mmc2 rather then mmc0 we already have code
to check for this, see board/sunxi/board.c: board_mmc_init(), adapting this
for use to determine whether to look u-boot.bin from nand or mmc on
systems which have nand rather an emmc should be trivial.

So we should be able to build uniform SPL (and u-boot) binaries which work
for both nand and sdcard. Note that this is jyst FYI, I'm fine with merging
the patch as is and fixing this in a follow up patch.

*) Note this is not true on the A31 which prefers nand over sdcard unless
the fel pin is pulled down (or was it up), but is true on all other SoCs


> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
> index d9f7691..121e655 100644
> --- a/board/sunxi/board.c
> +++ b/board/sunxi/board.c
> @@ -22,6 +22,9 @@
>   #ifdef CONFIG_AXP221_POWER
>   #include <axp221.h>
>   #endif
> +#ifdef CONFIG_NAND_SUNXI
> +#include <nand.h>
> +#endif
>   #include <asm/arch/clock.h>
>   #include <asm/arch/cpu.h>
>   #include <asm/arch/display.h>
> @@ -34,6 +37,8 @@
>   #include <linux/usb/musb.h>
>   #include <net.h>
>
> +#define CCMU_BASE			0x01c20000
> +

Ugh no please, see below.

>   #if defined CONFIG_VIDEO_LCD_PANEL_I2C && !(defined CONFIG_SPL_BUILD)
>   /* So that we can use pin names in Kconfig and sunxi_name_to_gpio() */
>   int soft_i2c_gpio_sda;
> @@ -315,6 +320,28 @@ int board_mmc_init(bd_t *bis)
>   }
>   #endif
>
> +void board_nand_init(void)
> +{
> +	uint32_t val;
> +	unsigned int pin;
> +	static u8 ports[] = CONFIG_NAND_SUNXI_GPC_PORTS;
> +
> +	/* Configure AHB muxes to connect output pins with NAND controller */
> +	for (pin = 0; pin < 16; pin++)
> +		sunxi_gpio_set_cfgpin(SUNXI_GPC(pin), SUNXI_GPC_NAND);
> +
> +	for (pin = 0; pin < ARRAY_SIZE(ports); pin++)
> +		sunxi_gpio_set_cfgpin(SUNXI_GPC(ports[pin]), SUNXI_GPC_NAND);
> +
> +	/* "un-gate" NAND clock and clock source
> +	 * This assumes that the clock was already correctly configured by
> +	 * BootROM */
> +	val = readl(CCMU_BASE + 0x60);
> +	writel((val | 0x00002000), CCMU_BASE + 0x60);
> +	val = readl(CCMU_BASE + 0x80);
> +	writel((val | 0x80000000), CCMU_BASE + 0x80);
> +}
> +

2 remarks here:

1) The ccmu poking needs to be done in the same way it is done everywhere else,
see e.g. drivers/mmc/sunxi_mmc.c which does:

         struct sunxi_ccm_reg * const ccm =
                         (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;

	/* config ahb clock */
	setbits_le32(&ccm->ahb_gate0, 1 << AHB_GATE_OFFSET_MMC(sdc_no));
#ifdef CONFIG_SUNXI_GEN_SUN6I
         /* unassert reset */
         setbits_le32(&ccm->ahb_reset0_cfg, 1 << AHB_RESET_OFFSET_MMC(sdc_no));
#endif

Also why are you doing this in the board init code? All other sunxi code
only does pinmux setup in the board_init code and does the clk gating
stuff in the actual driver code.

Last: "This assumes that the clock was already correctly configured by BROM"
that will need to be fixed (eventually, can be in a follow up patch), as
in u-boot.bin we will want to support reading nand while booted from sdcard
(for the unified binaries)

>   void i2c_init_board(void)
>   {
>   #ifdef CONFIG_I2C0_ENABLE
> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
> index 347ea62..a0cf4d5 100644
> --- a/drivers/mtd/nand/Makefile
> +++ b/drivers/mtd/nand/Makefile
> @@ -73,5 +73,6 @@ obj-$(CONFIG_NAND_FSL_ELBC) += fsl_elbc_spl.o
>   obj-$(CONFIG_NAND_FSL_IFC) += fsl_ifc_spl.o
>   obj-$(CONFIG_NAND_MXC) += mxc_nand_spl.o
>   obj-$(CONFIG_NAND_MXS) += mxs_nand_spl.o mxs_nand.o
> +obj-$(CONFIG_NAND_SUNXI) += sunxi_nand_spl.o
>
>   endif # drivers
> diff --git a/drivers/mtd/nand/sunxi_nand_spl.c b/drivers/mtd/nand/sunxi_nand_spl.c
> new file mode 100644
> index 0000000..b8d7a7a
> --- /dev/null
> +++ b/drivers/mtd/nand/sunxi_nand_spl.c
> @@ -0,0 +1,290 @@
> +/*
> + * Copyright (c) 2014, Antmicro Ltd <www.antmicro.com>
> + * Copyright (c) 2015, Turtle Solutions <www.turtle-solutions.eu>
> + * Copyright (c) 2015, Roy Spliet <rspliet@ultimaker.com>
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + *
> + * \todo Detect chip parameters (page size, ECC mode, randomisation...)
> + */
> +
> +#include <common.h>
> +#include <config.h>
> +#include <asm/io.h>
> +#include <nand.h>
> +
> +/* DMAC */
> +#define DMAC_BASE			0x01c02000

Please use the base address define from arch/arm/include/asm/arch-sunxi/cpu_sun4i.h
or add one there if necessary.


> +#define DMAC_REG(a)			(DMAC_BASE + a)


> +
> +#define DMAC_INT			DMAC_REG(0x000)
> +#define DMAC_DDMA_CFG			DMAC_REG(0x300)
> +#define DMAC_DDMA_SRC			DMAC_REG(0x304)
> +#define DMAC_DDMA_DST			DMAC_REG(0x308)
> +#define DMAC_DDMA_BYTE_COUNT		DMAC_REG(0x30C)
> +#define DMAC_DDMA_PARAM			DMAC_REG(0x318)
> +
> +/* NAND controller */
> +#define NANDFLASHC_BASE			0x01c03000
> +#define NREG(a)				(0x01c03000 + a)
> +
> +#define NANDFLASHC_CTL			NREG(0x00)
> +#define NANDFLASHC_CTL_EN		0x00000001
> +#define NANDFLASHC_CTL_RST		0x00000002
> +#define NANDFLASHC_CTL_RAM_METHOD	0x00004000
> +
> +#define NANDFLASHC_ST			NREG(0x004)
> +#define NANDFLASHC_INT			NREG(0x008)
> +#define NANDFLASHC_TIMING_CTL		NREG(0x00C)
> +#define NANDFLASHC_TIMING_CFG		NREG(0x010)
> +#define NANDFLASHC_ADDR_LOW		NREG(0x014)
> +#define NANDFLASHC_ADDR_HIGH		NREG(0x018)
> +#define NANDFLASHC_SECTOR_NUM		NREG(0x01C)
> +#define NANDFLASHC_CNT			NREG(0x020)
> +
> +#define NANDFLASHC_CMD			NREG(0x024)
> +#define NANDFLASHC_SEND_CMD1		(1 << 22)
> +#define NANDFLASHC_WAIT_FLAG		(1 << 23)
> +
> +#define NANDFLASHC_RCMD_SET		NREG(0x028)
> +#define NANDFLASHC_WCMD_SET		NREG(0x02C)
> +#define NANDFLASHC_IO_DATA		NREG(0x030)
> +#define NANDFLASHC_ECC_CTL		NREG(0x034)
> +#define NANDFLASHC_ECC_ST		NREG(0x038)
> +#define NANDFLASHC_DEBUG		NREG(0x03c)
> +#define NANDFLASHC_ECC_CNT0		NREG(0x040)
> +#define NANDFLASHC_ECC_CNT1		NREG(0x044)
> +#define NANDFLASHC_ECC_CNT2		NREG(0x048)
> +#define NANDFLASHC_ECC_CNT3		NREG(0x04c)
> +#define NANDFLASHC_USER_DATA_BASE	NREG(0x050)
> +#define NANDFLASHC_EFNAND_STATUS	NREG(0x090)
> +#define NANDFLASHC_SPARE_AREA		NREG(0x0A0)
> +#define NANDFLASHC_PATTERN_ID		NREG(0x0A4)
> +#define NANDFLASHC_RAM0_BASE		NREG(0x400)
> +#define NANDFLASHC_RAM1_BASE		NREG(0x800)

Please create a struct reflecting the register layout and
then initialize a ptr to this struct from the base-address
and get register addresses this way, this is how we deal
with this in pretty much all other sunxi code, see e.g.:

arch/arm/include/asm/arch-sunxi/mmc.h



> +
> +void
> +nand_init(void)
> +{
> +	uint32_t val;
> +
> +	board_nand_init();
> +	val = readl(NANDFLASHC_CTL);
> +	val |= NANDFLASHC_CTL_RST;
> +	writel(val, NANDFLASHC_CTL);
> +
> +	/* Wait until reset pin is deasserted */
> +	do {
> +		val = readl(NANDFLASHC_CTL);
> +		if (!(val & NANDFLASHC_CTL_RST))
> +			break;
> +	} while (1);

Please put a timeout on all waits, see mctl_await_completion()
from arch/arm/cpu/armv7/sunxi/dram_helpers.c, or just use
that outright. It is intended for use during dram init, but
it should work fine for things like this too.

> +
> +	/** \todo Chip select, currently kind of static */
> +	val = readl(NANDFLASHC_CTL);
> +	val &= 0xf0fff0f2;
> +	val |= NANDFLASHC_CTL_EN;
> +	val |= (3 << 8);
> +	writel(val, NANDFLASHC_CTL);
> +
> +	writel(0x100, NANDFLASHC_TIMING_CTL);
> +	writel(0x7ff, NANDFLASHC_TIMING_CFG);
> +
> +	/* reset CMD  */
> +	val = NANDFLASHC_SEND_CMD1 | NANDFLASHC_WAIT_FLAG | NAND_CMD_RESET;
> +	writel(val, NANDFLASHC_CMD);
> +	do {
> +		val = readl(NANDFLASHC_ST);
> +		if (val & (1<<1))
> +			break;
> +		udelay(1000);
> +	} while (1);

Idem. Also the udelay buys you nothing.


> +
> +	printf("Nand initialised\n");
> +}
> +
> +/* random seed */
> +static const uint16_t random_seed[128] = {
> +	0x2b75, 0x0bd0, 0x5ca3, 0x62d1, 0x1c93, 0x07e9, 0x2162, 0x3a72,
> +	0x0d67, 0x67f9, 0x1be7, 0x077d, 0x032f, 0x0dac, 0x2716, 0x2436,
> +	0x7922, 0x1510, 0x3860, 0x5287, 0x480f, 0x4252, 0x1789, 0x5a2d,
> +	0x2a49, 0x5e10, 0x437f, 0x4b4e, 0x2f45, 0x216e, 0x5cb7, 0x7130,
> +	0x2a3f, 0x60e4, 0x4dc9, 0x0ef0, 0x0f52, 0x1bb9, 0x6211, 0x7a56,
> +	0x226d, 0x4ea7, 0x6f36, 0x3692, 0x38bf, 0x0c62, 0x05eb, 0x4c55,
> +	0x60f4, 0x728c, 0x3b6f, 0x2037, 0x7f69, 0x0936, 0x651a, 0x4ceb,
> +	0x6218, 0x79f3, 0x383f, 0x18d9, 0x4f05, 0x5c82, 0x2912, 0x6f17,
> +	0x6856, 0x5938, 0x1007, 0x61ab, 0x3e7f, 0x57c2, 0x542f, 0x4f62,
> +	0x7454, 0x2eac, 0x7739, 0x42d4, 0x2f90, 0x435a, 0x2e52, 0x2064,
> +	0x637c, 0x66ad, 0x2c90, 0x0bad, 0x759c, 0x0029, 0x0986, 0x7126,
> +	0x1ca7, 0x1605, 0x386a, 0x27f5, 0x1380, 0x6d75, 0x24c3, 0x0f8e,
> +	0x2b7a, 0x1418, 0x1fd1, 0x7dc1, 0x2d8e, 0x43af, 0x2267, 0x7da3,
> +	0x4e3d, 0x1338, 0x50db, 0x454d, 0x764d, 0x40a3, 0x42e6, 0x262b,
> +	0x2d2e, 0x1aea, 0x2e17, 0x173d, 0x3a6e, 0x71bf, 0x25f9, 0x0a5d,
> +	0x7c57, 0x0fbe, 0x46ce, 0x4939, 0x6b17, 0x37bb, 0x3e91, 0x76db,
> +};
> +
> +uint32_t ecc_errors = 0;
> +
> +int nand_waid_cmd_fifo_free(void)
> +{
> +	do {
> +		if (!(readl(NANDFLASHC_ST) & 0x8))
> +			return 0;
> +	} while (1);

Idem.

> +	return -1;
> +}
> +
> +static void
> +nand_config_ecc(uint32_t page, int syndrome)
> +{
> +	static u8 strength[] = {16, 24, 28, 32, 40, 48, 56, 60, 64};
> +	int i;
> +	uint32_t ecc_mode;
> +	u32 ecc;
> +
> +	for (i = 0; i < ARRAY_SIZE(strength); i++) {
> +		if (CONFIG_NAND_SUNXI_ECC_STRENGTH == strength[i]) {
> +			ecc_mode = i;
> +			break;
> +		}
> +	}
> +
> +	if (i == ARRAY_SIZE(strength)) {
> +		printf("ECC strength unsupported\n");
> +		return;
> +	}
> +
> +	ecc = 1 | (1<<3) | (1 << 9) | (ecc_mode << 12);
> +
> +	if (CONFIG_NAND_SUNXI_ECC_STEP == 512)
> +		ecc |= 1 << 5;
> +
> +	if (syndrome)
> +		ecc |= (0x4A80 << 16);
> +	else
> +		ecc |= (random_seed[page % ARRAY_SIZE(random_seed)] << 16);
> +
> +	writel(ecc, NANDFLASHC_ECC_CTL);
> +}
> +
> +/* read CONFIG_NAND_SUNXI_ECC_STEP bytes from real_addr to temp_buf */
> +void
> +nand_read_block(phys_addr_t src, dma_addr_t dst, int syndrome)
> +{
> +	uint32_t shift;
> +	uint32_t page;
> +	uint32_t addr;
> +	uint32_t oob_offset;
> +	uint32_t ecc_bytes;
> +	u32 val;
> +	u32 cmd;
> +
> +	page = src / CONFIG_NAND_SUNXI_PAGE_SIZE;
> +	if (page > 0xFFFF) {
> +		/* TODO: currently this is not supported */
> +		printf("Reading from address >= %08X is not allowed.\n",
> +		       0xFFFF * CONFIG_NAND_SUNXI_PAGE_SIZE);
> +		return;
> +	}
> +
> +	shift = src % CONFIG_NAND_SUNXI_PAGE_SIZE;
> +	writel(0, NANDFLASHC_ECC_ST);
> +
> +	/* ECC_CTL, randomization */
> +	ecc_bytes = CONFIG_NAND_SUNXI_ECC_STRENGTH *
> +			fls(CONFIG_NAND_SUNXI_ECC_STEP * 8);
> +	ecc_bytes = DIV_ROUND_UP(ecc_bytes, 8);
> +	ecc_bytes += (ecc_bytes & 1); /* Align to 2-bytes */
> +	ecc_bytes += 4;
> +
> +	nand_config_ecc(page, syndrome);
> +	if (syndrome) {
> +		/* shift every 1kB in syndrome */
> +		shift += (shift / CONFIG_NAND_SUNXI_ECC_STEP) * ecc_bytes;
> +		oob_offset = CONFIG_NAND_SUNXI_ECC_STEP + shift;
> +	} else {
> +		oob_offset = CONFIG_NAND_SUNXI_PAGE_SIZE  +
> +			(shift / CONFIG_NAND_SUNXI_ECC_STEP) * ecc_bytes;
> +	}
> +
> +	addr = (page << 16) | shift;
> +
> +	/* DMA */
> +	val = readl(NANDFLASHC_CTL);
> +	writel(val | NANDFLASHC_CTL_RAM_METHOD, NANDFLASHC_CTL);
> +
> +	writel(oob_offset, NANDFLASHC_SPARE_AREA);
> +
> +	/* DMAC
> +	 * \todo Separate this into a tidy driver */
> +	writel(0x0, DMAC_INT); /* clear dma interrupts */
> +	writel(NANDFLASHC_IO_DATA, DMAC_DDMA_SRC);
> +	writel(dst               , DMAC_DDMA_DST);
> +	writel(0x00007F0F        , DMAC_DDMA_PARAM);
> +	writel(CONFIG_NAND_SUNXI_ECC_STEP, DMAC_DDMA_BYTE_COUNT);
> +	/*
> +	 * [ 0: 4] Source     - NAND
> +	 * [ 5: 6] Mode       - IO
> +	 * [ 9:10] Dada width - 32-bits
> +	 * [16:20] Dest       - SDRAM
> +	 * [25:26] Data width - 32-bits
> +	 * [   31] Enable
> +	 */
> +	writel(0x84010423, DMAC_DDMA_CFG);
> +
> +	writel(0x00E00530, NANDFLASHC_RCMD_SET);
> +	nand_waid_cmd_fifo_free();
> +	writel(1, NANDFLASHC_SECTOR_NUM);
> +	writel(addr, NANDFLASHC_ADDR_LOW);
> +	writel(0, NANDFLASHC_ADDR_HIGH);
> +
> +	/* CMD (PAGE READ) */
> +	cmd = 0x85E80000;
> +	cmd |= ((CONFIG_NAND_SUNXI_ADDR_CYCLES - 1) << 16);
> +	cmd |= (syndrome ? 0x02000000 : 0x0);
> +	writel(cmd, NANDFLASHC_CMD);
> +
> +	do { /* wait for dma irq */
> +		val = readl(NANDFLASHC_ST);
> +		if (val & (1<<2))
> +			break;
> +		udelay(1000);
> +	} while (1);
> +
> +	do {/* make sure cmd is finished */
> +		val = readl(DMAC_BASE + 0x300);
> +		if (!(val & 0x80000000))
> +			break;
> +		udelay(1000);
> +	} while (1);
> +
> +	if (readl(NANDFLASHC_ECC_ST))
> +		ecc_errors++;
> +}
> +
> +int
> +nand_spl_load_image(uint32_t offs, unsigned int size, void *dest)
> +{
> +	dma_addr_t dst_block;
> +	dma_addr_t dst_end;
> +	phys_addr_t addr = offs;
> +
> +	dst_end = ((dma_addr_t) dest) + size;
> +
> +	memset((void *)dest, 0x0, size);
> +	ecc_errors = 0;
> +	for (dst_block = (dma_addr_t) dest; dst_block < dst_end;
> +			dst_block += CONFIG_NAND_SUNXI_ECC_STEP,
> +			addr += CONFIG_NAND_SUNXI_ECC_STEP) {
> +		/* syndrome read first 4MiB to match Allwinner BootROM */
> +		nand_read_block(addr, dst_block, addr < 0x400000);
> +	}
> +
> +	if (ecc_errors)
> +		printf("Error: %d ECC failures detected\n", ecc_errors);
> +	return ecc_errors == 0;
> +}
> +
> +void
> +nand_deselect(void)
> +{}
> diff --git a/include/configs/sun4i.h b/include/configs/sun4i.h
> index ea079eb..a3c9408 100644
> --- a/include/configs/sun4i.h
> +++ b/include/configs/sun4i.h
> @@ -18,6 +18,7 @@
>   #endif
>
>   #define CONFIG_SUNXI_USB_PHYS	3
> +#define CONFIG_NAND_SUNXI_GPC_PORTS	{16, 17, 18, 19, 20, 21, 22, 24}
>
>   /*
>    * Include common sunxi configuration where most the settings are
> diff --git a/include/configs/sun5i.h b/include/configs/sun5i.h
> index d257659..8e13df5 100644
> --- a/include/configs/sun5i.h
> +++ b/include/configs/sun5i.h
> @@ -19,6 +19,9 @@
>
>   #define CONFIG_SUNXI_USB_PHYS	2
>
> +/* \todo A13 only defines port 19, whereas A10s requires each of these */
> +#define CONFIG_NAND_SUNXI_GPC_PORTS	{16, 17, 18, 19}
> +
>   /*
>    * Include common sunxi configuration where most the settings are
>    */
> diff --git a/include/configs/sun7i.h b/include/configs/sun7i.h
> index 56101a9..3d26ce8 100644
> --- a/include/configs/sun7i.h
> +++ b/include/configs/sun7i.h
> @@ -24,6 +24,8 @@
>   #define CONFIG_ARMV7_SECURE_BASE	SUNXI_SRAM_B_BASE
>   #define CONFIG_TIMER_CLK_FREQ		24000000
>
> +#define CONFIG_NAND_SUNXI_GPC_PORTS	{16, 17, 18, 19, 20, 21, 22, 24}
> +
>   /*
>    * Include common sunxi configuration where most the settings are
>    */
> diff --git a/include/configs/sun8i.h b/include/configs/sun8i.h
> index 7111c63..cd33758 100644
> --- a/include/configs/sun8i.h
> +++ b/include/configs/sun8i.h
> @@ -20,6 +20,12 @@
>
>   #define CONFIG_SUNXI_USB_PHYS	2
>
> +#if defined(CONFIG_MACH_SUN8I_A23)
> +#define CONFIG_NAND_SUNXI_GPC_PORTS	{16, 17, 18}
> +#elif defined(CONFIG_MACH_SUN8I_A33)
> +#define CONFIG_NAND_SUNXI_GPC_PORTS	{16}
> +#endif
> +
>   /*
>    * Include common sunxi configuration where most the settings are
>    */
> diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h
> index c8ebb54..cce0441 100644
> --- a/include/configs/sunxi-common.h
> +++ b/include/configs/sunxi-common.h
> @@ -106,8 +106,10 @@
>   #define CONFIG_CMD_MMC
>   #define CONFIG_MMC_SUNXI
>   #define CONFIG_MMC_SUNXI_SLOT		0
> +#if !defined(CONFIG_SPL_NAND_SUPPORT)
>   #define CONFIG_ENV_IS_IN_MMC
>   #define CONFIG_SYS_MMC_ENV_DEV		0	/* first detected MMC controller */
> +#endif /* CONFIG_SPL_NAND_SUPPORT */
>   #endif
>
>   /* 4MB of malloc() pool */
> @@ -324,6 +326,24 @@ extern int soft_i2c_gpio_scl;
>   #define CONFIG_ENV_IS_NOWHERE
>   #endif
>
> +#ifdef CONFIG_SPL_NAND_SUPPORT
> +#define CONFIG_NAND
> +#define CONFIG_SYS_NAND_SELF_INIT
> +#define CONFIG_NAND_SUNXI
> +#define CONFIG_CMD_SPL_WRITE_SIZE		0x000400
> +#define CONFIG_SYS_NAND_U_BOOT_OFFS		0x008000
> +
> +/* \todo Make these parameterisable in kernel config ? */
> +#define CONFIG_NAND_SUNXI_PAGE_SIZE		8192
> +#define CONFIG_NAND_SUNXI_ECC_STEP		1024
> +#define CONFIG_NAND_SUNXI_ECC_STRENGTH		40
> +#define CONFIG_NAND_SUNXI_ADDR_CYCLES		5
> +
> +#ifndef CONFIG_NAND_SUNXI_GPC_PORTS
> +#error "No NAND GPC ports defined, NAND unsupported"
> +#endif
> +#endif /* CONFIG_SPL_NAND_SUPPORT */
> +
>   #define CONFIG_MISC_INIT_R
>   #define CONFIG_SYS_CONSOLE_IS_IN_ENV
>
>

Otherwise this looks like a good start, with the coding
style issues fixed I would not be opposed against merging
this as a first step to growing proper nand support.

Ian, is that ok with you ?

Regards,

Hans

On Thu, 2015-05-21 at 20:39 +0200, Hans de Goede wrote:
[...]
> Please create a struct reflecting the register layout and
> then initialize a ptr to this struct from the base-address
> and get register addresses this way, this is how we deal
> with this in pretty much all other sunxi code, see e.g.:

This isn't just sunxi, I believe it is u-boot's preferred way to do
things generally.
[...]
> Otherwise this looks like a good start, with the coding
> style issues fixed I would not be opposed against merging
> this as a first step to growing proper nand support.
> 
> Ian, is that ok with you ?

You mean with the coding style fixed but not (necessarily) all the other
issues you pointed out (timeouts on loops, structs for register
accesses)?

I think at least the structs for register access stuff should be fixed
first, especially given it was already raised in the previous round of
review.

Ian

Hi,

On 21-05-15 21:02, Ian Campbell wrote:
> On Thu, 2015-05-21 at 20:39 +0200, Hans de Goede wrote:
> [...]
>> Please create a struct reflecting the register layout and
>> then initialize a ptr to this struct from the base-address
>> and get register addresses this way, this is how we deal
>> with this in pretty much all other sunxi code, see e.g.:
>
> This isn't just sunxi, I believe it is u-boot's preferred way to do
> things generally.
> [...]
>> Otherwise this looks like a good start, with the coding
>> style issues fixed I would not be opposed against merging
>> this as a first step to growing proper nand support.
>>
>> Ian, is that ok with you ?
>
> You mean with the coding style fixed but not (necessarily) all the other
> issues you pointed out (timeouts on loops, structs for register
> accesses)?

Sorry, what I meant is are you ok with starting with merging just
the SPL support without having support for actually reading
the kernel, etc. as a first step. The SPL support would have to
have all the issues (*) I pointed out fixed before merging.

Regards,

Hans

*) Except for those where I explicitly said they can be fixed up
with a follow up commit.

On Fri, 2015-05-22 at 09:30 +0200, Hans de Goede wrote:
> Hi,
> 
> On 21-05-15 21:02, Ian Campbell wrote:
> > On Thu, 2015-05-21 at 20:39 +0200, Hans de Goede wrote:
> > [...]
> >> Please create a struct reflecting the register layout and
> >> then initialize a ptr to this struct from the base-address
> >> and get register addresses this way, this is how we deal
> >> with this in pretty much all other sunxi code, see e.g.:
> >
> > This isn't just sunxi, I believe it is u-boot's preferred way to do
> > things generally.
> > [...]
> >> Otherwise this looks like a good start, with the coding
> >> style issues fixed I would not be opposed against merging
> >> this as a first step to growing proper nand support.
> >>
> >> Ian, is that ok with you ?
> >
> > You mean with the coding style fixed but not (necessarily) all the other
> > issues you pointed out (timeouts on loops, structs for register
> > accesses)?
> 
> Sorry, what I meant is are you ok with starting with merging just
> the SPL support without having support for actually reading
> the kernel, etc. as a first step.

Oh yes, that seems completely reasonable.

>  The SPL support would have to
> have all the issues (*) I pointed out fixed before merging.

Agreed.

Ian.

On Thu, 2015-05-21 at 15:59 +0200, Roy Spliet wrote:
> +#ifdef CONFIG_NAND_SUNXI
> +#include <nand.h>
> +#endif

Why do you need the ifdef?

+#include <common.h>
> +#include <config.h>
> +#include <asm/io.h>
> +#include <nand.h>
> +
> +/* DMAC */
> +#define DMAC_BASE                    0x01c02000
> +#define DMAC_REG(a)                  (DMAC_BASE + a)
> +
> +#define DMAC_INT                     DMAC_REG(0x000)
> +#define DMAC_DDMA_CFG                        DMAC_REG(0x300)
> +#define DMAC_DDMA_SRC                        DMAC_REG(0x304)
> +#define DMAC_DDMA_DST                        DMAC_REG(0x308)
> +#define DMAC_DDMA_BYTE_COUNT         DMAC_REG(0x30C)
> +#define DMAC_DDMA_PARAM                      DMAC_REG(0x318)
> +
> +/* NAND controller */
> +#define NANDFLASHC_BASE                      0x01c03000
> +#define NREG(a)                              (0x01c03000 + a)
> +
> +#define NANDFLASHC_CTL                       NREG(0x00)
> +#define NANDFLASHC_CTL_EN            0x00000001
> +#define NANDFLASHC_CTL_RST           0x00000002
> +#define NANDFLASHC_CTL_RAM_METHOD    0x00004000
> +
> +#define NANDFLASHC_ST                        NREG(0x004)
> +#define NANDFLASHC_INT                       NREG(0x008)
> +#define NANDFLASHC_TIMING_CTL                NREG(0x00C)
> +#define NANDFLASHC_TIMING_CFG                NREG(0x010)
> +#define NANDFLASHC_ADDR_LOW          NREG(0x014)
> +#define NANDFLASHC_ADDR_HIGH         NREG(0x018)
> +#define NANDFLASHC_SECTOR_NUM                NREG(0x01C)
> +#define NANDFLASHC_CNT                       NREG(0x020)
> +
> +#define NANDFLASHC_CMD                       NREG(0x024)
> +#define NANDFLASHC_SEND_CMD1         (1 << 22)
> +#define NANDFLASHC_WAIT_FLAG         (1 << 23)
> +
> +#define NANDFLASHC_RCMD_SET          NREG(0x028)
> +#define NANDFLASHC_WCMD_SET          NREG(0x02C)
> +#define NANDFLASHC_IO_DATA           NREG(0x030)
> +#define NANDFLASHC_ECC_CTL           NREG(0x034)
> +#define NANDFLASHC_ECC_ST            NREG(0x038)
> +#define NANDFLASHC_DEBUG             NREG(0x03c)
> +#define NANDFLASHC_ECC_CNT0          NREG(0x040)
> +#define NANDFLASHC_ECC_CNT1          NREG(0x044)
> +#define NANDFLASHC_ECC_CNT2          NREG(0x048)
> +#define NANDFLASHC_ECC_CNT3          NREG(0x04c)
> +#define NANDFLASHC_USER_DATA_BASE    NREG(0x050)
> +#define NANDFLASHC_EFNAND_STATUS     NREG(0x090)
> +#define NANDFLASHC_SPARE_AREA                NREG(0x0A0)
> +#define NANDFLASHC_PATTERN_ID                NREG(0x0A4)
> +#define NANDFLASHC_RAM0_BASE         NREG(0x400)
> +#define NANDFLASHC_RAM1_BASE         NREG(0x800)

Shouldn't these be in a header file so they can be shared with a non-
SPL driver?

> +void
> +nand_init(void)
> +{

Don't put a newline after the return type.

> +     uint32_t val;
> +
> +     board_nand_init();
> +     val = readl(NANDFLASHC_CTL);
> +     val |= NANDFLASHC_CTL_RST;
> +     writel(val, NANDFLASHC_CTL);
> +
> +     /* Wait until reset pin is deasserted */
> +     do {
> +             val = readl(NANDFLASHC_CTL);
> +             if (!(val & NANDFLASHC_CTL_RST))
> +                     break;
> +     } while (1);

Add a timeout to delay loops.

> +
> +     /** \todo Chip select, currently kind of static */
> +     val = readl(NANDFLASHC_CTL);
> +     val &= 0xf0fff0f2;

Don't put magic numbers in the code -- use symbolic constants taht 
describe what the fields mean.

+/* random seed */
> +static const uint16_t random_seed[128] = {
> +     0x2b75, 0x0bd0, 0x5ca3, 0x62d1, 0x1c93, 0x07e9, 0x2162, 0x3a72,
> +     0x0d67, 0x67f9, 0x1be7, 0x077d, 0x032f, 0x0dac, 0x2716, 0x2436,
> +     0x7922, 0x1510, 0x3860, 0x5287, 0x480f, 0x4252, 0x1789, 0x5a2d,
> +     0x2a49, 0x5e10, 0x437f, 0x4b4e, 0x2f45, 0x216e, 0x5cb7, 0x7130,
> +     0x2a3f, 0x60e4, 0x4dc9, 0x0ef0, 0x0f52, 0x1bb9, 0x6211, 0x7a56,
> +     0x226d, 0x4ea7, 0x6f36, 0x3692, 0x38bf, 0x0c62, 0x05eb, 0x4c55,
> +     0x60f4, 0x728c, 0x3b6f, 0x2037, 0x7f69, 0x0936, 0x651a, 0x4ceb,
> +     0x6218, 0x79f3, 0x383f, 0x18d9, 0x4f05, 0x5c82, 0x2912, 0x6f17,
> +     0x6856, 0x5938, 0x1007, 0x61ab, 0x3e7f, 0x57c2, 0x542f, 0x4f62,
> +     0x7454, 0x2eac, 0x7739, 0x42d4, 0x2f90, 0x435a, 0x2e52, 0x2064,
> +     0x637c, 0x66ad, 0x2c90, 0x0bad, 0x759c, 0x0029, 0x0986, 0x7126,
> +     0x1ca7, 0x1605, 0x386a, 0x27f5, 0x1380, 0x6d75, 0x24c3, 0x0f8e,
> +     0x2b7a, 0x1418, 0x1fd1, 0x7dc1, 0x2d8e, 0x43af, 0x2267, 0x7da3,
> +     0x4e3d, 0x1338, 0x50db, 0x454d, 0x764d, 0x40a3, 0x42e6, 0x262b,
> +     0x2d2e, 0x1aea, 0x2e17, 0x173d, 0x3a6e, 0x71bf, 0x25f9, 0x0a5d,
> +     0x7c57, 0x0fbe, 0x46ce, 0x4939, 0x6b17, 0x37bb, 0x3e91, 0x76db,
> +};

Why is randomness needed?

> +uint32_t ecc_errors = 0;

Why is this global?

> +int
> +nand_spl_load_image(uint32_t offs, unsigned int size, void *dest)
> +{
> +     dma_addr_t dst_block;
> +     dma_addr_t dst_end;
> +     phys_addr_t addr = offs;
> +
> +     dst_end = ((dma_addr_t) dest) + size;
> +
> +     memset((void *)dest, 0x0, size);

Unnecessary cast.

-Scott

Dear Scott,

Thank you for taking your time to feedback. However, it seems to be 
about a week and two versions of the patchset too late. Most of your 
issues have been addressed in the meanwhile, as you can see in Hans de 
Goede's sunxi branch.
Yours,

Roy

Op 02-06-15 om 00:14 schreef Scott Wood:
> On Thu, 2015-05-21 at 15:59 +0200, Roy Spliet wrote:
>> +#ifdef CONFIG_NAND_SUNXI
>> +#include <nand.h>
>> +#endif
> Why do you need the ifdef?
>
> +#include <common.h>
>> +#include <config.h>
>> +#include <asm/io.h>
>> +#include <nand.h>
>> +
>> +/* DMAC */
>> +#define DMAC_BASE                    0x01c02000
>> +#define DMAC_REG(a)                  (DMAC_BASE + a)
>> +
>> +#define DMAC_INT                     DMAC_REG(0x000)
>> +#define DMAC_DDMA_CFG                        DMAC_REG(0x300)
>> +#define DMAC_DDMA_SRC                        DMAC_REG(0x304)
>> +#define DMAC_DDMA_DST                        DMAC_REG(0x308)
>> +#define DMAC_DDMA_BYTE_COUNT         DMAC_REG(0x30C)
>> +#define DMAC_DDMA_PARAM                      DMAC_REG(0x318)
>> +
>> +/* NAND controller */
>> +#define NANDFLASHC_BASE                      0x01c03000
>> +#define NREG(a)                              (0x01c03000 + a)
>> +
>> +#define NANDFLASHC_CTL                       NREG(0x00)
>> +#define NANDFLASHC_CTL_EN            0x00000001
>> +#define NANDFLASHC_CTL_RST           0x00000002
>> +#define NANDFLASHC_CTL_RAM_METHOD    0x00004000
>> +
>> +#define NANDFLASHC_ST                        NREG(0x004)
>> +#define NANDFLASHC_INT                       NREG(0x008)
>> +#define NANDFLASHC_TIMING_CTL                NREG(0x00C)
>> +#define NANDFLASHC_TIMING_CFG                NREG(0x010)
>> +#define NANDFLASHC_ADDR_LOW          NREG(0x014)
>> +#define NANDFLASHC_ADDR_HIGH         NREG(0x018)
>> +#define NANDFLASHC_SECTOR_NUM                NREG(0x01C)
>> +#define NANDFLASHC_CNT                       NREG(0x020)
>> +
>> +#define NANDFLASHC_CMD                       NREG(0x024)
>> +#define NANDFLASHC_SEND_CMD1         (1 << 22)
>> +#define NANDFLASHC_WAIT_FLAG         (1 << 23)
>> +
>> +#define NANDFLASHC_RCMD_SET          NREG(0x028)
>> +#define NANDFLASHC_WCMD_SET          NREG(0x02C)
>> +#define NANDFLASHC_IO_DATA           NREG(0x030)
>> +#define NANDFLASHC_ECC_CTL           NREG(0x034)
>> +#define NANDFLASHC_ECC_ST            NREG(0x038)
>> +#define NANDFLASHC_DEBUG             NREG(0x03c)
>> +#define NANDFLASHC_ECC_CNT0          NREG(0x040)
>> +#define NANDFLASHC_ECC_CNT1          NREG(0x044)
>> +#define NANDFLASHC_ECC_CNT2          NREG(0x048)
>> +#define NANDFLASHC_ECC_CNT3          NREG(0x04c)
>> +#define NANDFLASHC_USER_DATA_BASE    NREG(0x050)
>> +#define NANDFLASHC_EFNAND_STATUS     NREG(0x090)
>> +#define NANDFLASHC_SPARE_AREA                NREG(0x0A0)
>> +#define NANDFLASHC_PATTERN_ID                NREG(0x0A4)
>> +#define NANDFLASHC_RAM0_BASE         NREG(0x400)
>> +#define NANDFLASHC_RAM1_BASE         NREG(0x800)
> Shouldn't these be in a header file so they can be shared with a non-
> SPL driver?
>
>> +void
>> +nand_init(void)
>> +{
> Don't put a newline after the return type.
>
>> +     uint32_t val;
>> +
>> +     board_nand_init();
>> +     val = readl(NANDFLASHC_CTL);
>> +     val |= NANDFLASHC_CTL_RST;
>> +     writel(val, NANDFLASHC_CTL);
>> +
>> +     /* Wait until reset pin is deasserted */
>> +     do {
>> +             val = readl(NANDFLASHC_CTL);
>> +             if (!(val & NANDFLASHC_CTL_RST))
>> +                     break;
>> +     } while (1);
> Add a timeout to delay loops.
>
>> +
>> +     /** \todo Chip select, currently kind of static */
>> +     val = readl(NANDFLASHC_CTL);
>> +     val &= 0xf0fff0f2;
> Don't put magic numbers in the code -- use symbolic constants taht
> describe what the fields mean.
>
> +/* random seed */
>> +static const uint16_t random_seed[128] = {
>> +     0x2b75, 0x0bd0, 0x5ca3, 0x62d1, 0x1c93, 0x07e9, 0x2162, 0x3a72,
>> +     0x0d67, 0x67f9, 0x1be7, 0x077d, 0x032f, 0x0dac, 0x2716, 0x2436,
>> +     0x7922, 0x1510, 0x3860, 0x5287, 0x480f, 0x4252, 0x1789, 0x5a2d,
>> +     0x2a49, 0x5e10, 0x437f, 0x4b4e, 0x2f45, 0x216e, 0x5cb7, 0x7130,
>> +     0x2a3f, 0x60e4, 0x4dc9, 0x0ef0, 0x0f52, 0x1bb9, 0x6211, 0x7a56,
>> +     0x226d, 0x4ea7, 0x6f36, 0x3692, 0x38bf, 0x0c62, 0x05eb, 0x4c55,
>> +     0x60f4, 0x728c, 0x3b6f, 0x2037, 0x7f69, 0x0936, 0x651a, 0x4ceb,
>> +     0x6218, 0x79f3, 0x383f, 0x18d9, 0x4f05, 0x5c82, 0x2912, 0x6f17,
>> +     0x6856, 0x5938, 0x1007, 0x61ab, 0x3e7f, 0x57c2, 0x542f, 0x4f62,
>> +     0x7454, 0x2eac, 0x7739, 0x42d4, 0x2f90, 0x435a, 0x2e52, 0x2064,
>> +     0x637c, 0x66ad, 0x2c90, 0x0bad, 0x759c, 0x0029, 0x0986, 0x7126,
>> +     0x1ca7, 0x1605, 0x386a, 0x27f5, 0x1380, 0x6d75, 0x24c3, 0x0f8e,
>> +     0x2b7a, 0x1418, 0x1fd1, 0x7dc1, 0x2d8e, 0x43af, 0x2267, 0x7da3,
>> +     0x4e3d, 0x1338, 0x50db, 0x454d, 0x764d, 0x40a3, 0x42e6, 0x262b,
>> +     0x2d2e, 0x1aea, 0x2e17, 0x173d, 0x3a6e, 0x71bf, 0x25f9, 0x0a5d,
>> +     0x7c57, 0x0fbe, 0x46ce, 0x4939, 0x6b17, 0x37bb, 0x3e91, 0x76db,
>> +};
> Why is randomness needed?
>
>> +uint32_t ecc_errors = 0;
> Why is this global?
>
>> +int
>> +nand_spl_load_image(uint32_t offs, unsigned int size, void *dest)
>> +{
>> +     dma_addr_t dst_block;
>> +     dma_addr_t dst_end;
>> +     phys_addr_t addr = offs;
>> +
>> +     dst_end = ((dma_addr_t) dest) + size;
>> +
>> +     memset((void *)dest, 0x0, size);
> Unnecessary cast.
>
> -Scott
>