[U-Boot] [PATCH 0/4] sunxi: nand: Basic NAND driver with SPL support

[U-Boot] [PATCH 0/4] sunxi: nand: Basic NAND driver with SPL support

[Piotr Zierhoffer]

This is a basic driver for the sunxi NAND controller for Allwinner A20.
It supports both SPL and U-Boot.

The driver uses DMA for data transfers. It does not support writing.

To enable user reading from NADN, the a20_nandread command was added.


Piotr Zierhoffer (4):
  sunxi: nand: Add basic sunxi NAND driver with DMA support
  sunxi: nand: Add board configuration options
  sunxi: nand: Add a20_nandread command to load image from NAND in SPL
  sunxi: nand: Add information to sunxi that it was run from NAND in SPL

 arch/arm/cpu/armv7/sunxi/board.c |   4 +
 board/sunxi/Kconfig              |  10 ++
 board/sunxi/Makefile             |   1 +
 board/sunxi/nand.c               | 315 +++++++++++++++++++++++++++++++++++++++
 board/sunxi/nand.h               | 146 ++++++++++++++++++
 common/Makefile                  |   1 +
 common/cmd_a20_nandread.c        |  25 ++++
 include/configs/sunxi-common.h   |  13 ++
 8 files changed, 515 insertions(+)
 create mode 100644 board/sunxi/nand.c
 create mode 100644 board/sunxi/nand.h
 create mode 100644 common/cmd_a20_nandread.c

-- 
2.3.6

[U-Boot] [PATCH 1/4] sunxi: nand: Add basic sunxi NAND driver with DMA support

[Piotr Zierhoffer]

From: Piotr Zierhoffer <piotr.zierhoffer@cs.put.poznan.pl>

This driver adds NAND support to both SPL and full U-Boot.
It was tested on AllWinner A20.

Signed-off-by: Peter Gielda <pgielda@antmicro.com>
Signed-off-by: Tomasz Gorochowik <tgorochowik@antmicro.com>
Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>
Signed-off-by: Piotr Zierhoffer <pzierhoffer@antmicro.com>
---

 board/sunxi/Makefile |   1 +
 board/sunxi/nand.c   | 315 +++++++++++++++++++++++++++++++++++++++++++++++++++
 board/sunxi/nand.h   | 146 ++++++++++++++++++++++++
 3 files changed, 462 insertions(+)
 create mode 100644 board/sunxi/nand.c
 create mode 100644 board/sunxi/nand.h

diff --git a/board/sunxi/Makefile b/board/sunxi/Makefile
index 43766e0..6f1d571 100644
--- a/board/sunxi/Makefile
+++ b/board/sunxi/Makefile
@@ -9,6 +9,7 @@
 # SPDX-License-Identifier:	GPL-2.0+
 #
 obj-y	+= board.o
+obj-$(CONFIG_SUNXI_NAND)	+= nand.o
 obj-$(CONFIG_SUNXI_GMAC)	+= gmac.o
 obj-$(CONFIG_SUNXI_AHCI)	+= ahci.o
 obj-$(CONFIG_MACH_SUN4I)	+= dram_sun4i_auto.o
diff --git a/board/sunxi/nand.c b/board/sunxi/nand.c
new file mode 100644
index 0000000..16e8e4d
--- /dev/null
+++ b/board/sunxi/nand.c
@@ -0,0 +1,315 @@
+/*
+ * Copyright (c) 2014-2015, Antmicro Ltd <www.antmicro.com>
+ * Copyright (c) 2015, AW-SOM Technologies <www.aw-som.com>
+ *
+ * SPDX-License-Identifier:     GPL-2.0+
+ */
+
+#include <common.h>
+#include <config.h>
+#include <asm/io.h>
+#include <nand.h>
+#include "nand.h" /* sunxi specific header */
+
+/* minimal "boot0" style NAND support for Allwinner A20 */
+
+/* temporary buffer in internal ram */
+#ifdef CONFIG_SPL_BUILD
+/* in SPL temporary buffer cannot be @ 0x0 */
+unsigned char temp_buf[SPL_WRITE_SIZE] __aligned(0x10) __section(".text#");
+#else
+/* put temporary buffer @ 0x0 */
+unsigned char *temp_buf = (unsigned char *)0x0;
+#endif
+
+#define MAX_RETRIES 10
+
+static int check_value_inner(int offset, int expected_bits,
+				int max_number_of_retries, int negation)
+{
+	int retries = 0;
+	do {
+		int val = readl(offset) & expected_bits;
+		if (negation ? !val : val)
+			return 1;
+		mdelay(1);
+		retries++;
+	} while (retries < max_number_of_retries);
+
+	return 0;
+}
+
+static inline int check_value(int offset, int expected_bits,
+				int max_number_of_retries)
+{
+	return check_value_inner(offset, expected_bits,
+					max_number_of_retries, 0);
+}
+
+static inline int check_value_negated(int offset, int unexpected_bits,
+					int max_number_of_retries)
+{
+	return check_value_inner(offset, unexpected_bits,
+					max_number_of_retries, 1);
+}
+
+void nand_set_clocks(void)
+{
+	uint32_t val;
+
+	writel(PORTC_PC_CFG0_NRB1 |
+		PORTC_PC_CFG0_NRB0 |
+		PORTC_PC_CFG0_NRE  |
+		PORTC_PC_CFG0_NCE0 |
+		PORTC_PC_CFG0_NCE1 |
+		PORTC_PC_CFG0_NCLE |
+		PORTC_PC_CFG0_NALE |
+		PORTC_PC_CFG0_NWE, PORTC_BASE + PORTC_PC_CFG0);
+
+	writel(PORTC_PC_CFG1_NDQ7 |
+		PORTC_PC_CFG1_NDQ6 |
+		PORTC_PC_CFG1_NDQ5 |
+		PORTC_PC_CFG1_NDQ4 |
+		PORTC_PC_CFG1_NDQ3 |
+		PORTC_PC_CFG1_NDQ2 |
+		PORTC_PC_CFG1_NDQ1 |
+		PORTC_PC_CFG1_NDQ0, PORTC_BASE + PORTC_PC_CFG1);
+
+	writel(PORTC_PC_CFG2_NCE7 |
+		PORTC_PC_CFG2_NCE6 |
+		PORTC_PC_CFG2_NCE5 |
+		PORTC_PC_CFG2_NCE4 |
+		PORTC_PC_CFG2_NCE3 |
+		PORTC_PC_CFG2_NCE2 |
+		PORTC_PC_CFG2_NWP, PORTC_BASE + PORTC_PC_CFG2);
+
+	writel(PORTC_PC_CFG3_NDQS, PORTC_BASE + PORTC_PC_CFG3);
+
+	val = readl(CCU_BASE + CCU_AHB_GATING_REG0);
+	writel(CCU_AHB_GATING_REG0_NAND | val, CCU_BASE + CCU_AHB_GATING_REG0);
+
+	val = readl(CCU_BASE + CCU_NAND_SCLK_CFG_REG);
+	writel(val | CCU_NAND_SCLK_CFG_REG_SCLK_GATING
+		| CCU_NAND_SCLK_CFG_REG_CLK_DIV_RATIO,
+		CCU_BASE + CCU_NAND_SCLK_CFG_REG);
+}
+
+int nand_initialized;
+
+void nand_init(void)
+{
+	board_nand_init(NULL);
+}
+
+uint32_t ecc_errors;
+
+/* read 0x400 bytes from real_addr to temp_buf */
+void nand_read_block(unsigned int real_addr, int syndrome)
+{
+	uint32_t val;
+	int ECC_OFF = 0;
+	uint16_t ecc_mode = 0;
+	uint16_t rand_seed;
+	uint32_t page;
+	uint16_t column;
+	uint32_t oob_offset;
+
+	if (!nand_initialized)
+		board_nand_init(NULL);
+
+	switch (CONFIG_SUNXI_ECC_STRENGTH) {
+	case 16:
+		ecc_mode = 0;
+		ECC_OFF = 0x20;
+		break;
+	case 24:
+		ecc_mode = 1;
+		ECC_OFF = 0x2e;
+		break;
+	case 28:
+		ecc_mode = 2;
+		ECC_OFF = 0x32;
+		break;
+	case 32:
+		ecc_mode = 3;
+		ECC_OFF = 0x3c;
+		break;
+	case 40:
+		ecc_mode = 4;
+		ECC_OFF = 0x4a;
+		break;
+	case 48:
+		ecc_mode = 4;
+		ECC_OFF = 0x52;
+		break;
+	case 56:
+		ecc_mode = 4;
+		ECC_OFF = 0x60;
+		break;
+	case 60:
+		ecc_mode = 4;
+		ECC_OFF = 0x0;
+		break;
+	case 64:
+		ecc_mode = 4;
+		ECC_OFF = 0x0;
+		break;
+	default:
+		ecc_mode = 0;
+		ECC_OFF = 0;
+	}
+
+	if (ECC_OFF == 0) {
+		printf("Unsupported ECC strength (%d)!\n",
+		       CONFIG_SUNXI_ECC_STRENGTH);
+		return;
+	}
+
+	/* clear temp_buf */
+	memset((void *)temp_buf, 0, SPL_WRITE_SIZE);
+
+	/* set CMD  */
+	writel(NFC_SEND_CMD1 | NFC_WAIT_FLAG | 0xff,
+	       NANDFLASHC_BASE + NANDFLASHC_CMD);
+
+	if (!check_value(NANDFLASHC_BASE + NANDFLASHC_ST, (1 << 1),
+			 MAX_RETRIES)) {
+		printf("Error while initilizing command interrupt\n");
+		return;
+	}
+
+	page = (real_addr / BLOCK_SIZE);
+	column = real_addr % BLOCK_SIZE;
+
+	if (syndrome) {
+		/* shift every 1kB in syndrome */
+		column += (column / SPL_WRITE_SIZE) * ECC_OFF;
+	}
+
+	/* clear ecc status */
+	writel(0, NANDFLASHC_BASE + NANDFLASHC_ECC_ST);
+
+	/* Choose correct seed */
+	if (syndrome)
+		rand_seed = random_seed_syndrome;
+	else
+		rand_seed = random_seed[page % 128];
+
+	writel((rand_seed << 16) | NFC_ECC_RANDOM_EN | NFC_ECC_EN
+		| NFC_ECC_PIPELINE | (ecc_mode << 12),
+		NANDFLASHC_BASE + NANDFLASHC_ECC_CTL);
+
+	val = readl(NANDFLASHC_BASE + NANDFLASHC_CTL);
+	writel(val | NFC_CTL_RAM_METHOD, NANDFLASHC_BASE + NANDFLASHC_CTL);
+
+	if (syndrome) {
+		writel(SPL_WRITE_SIZE, NANDFLASHC_BASE + NANDFLASHC_SPARE_AREA);
+	} else {
+		oob_offset = BLOCK_SIZE + (column / SPL_WRITE_SIZE) * ECC_OFF;
+		writel(oob_offset, NANDFLASHC_BASE + NANDFLASHC_SPARE_AREA);
+	}
+
+	/* DMAC */
+	writel(0x0, DMAC_BASE + DMAC_CFG_REG0); /* clr dma cmd */
+	/* read from REG_IO_DATA */
+	writel(NANDFLASHC_BASE + NANDFLASHC_IO_DATA,
+	       DMAC_BASE + DMAC_SRC_START_ADDR_REG0);
+	writel((uint32_t)temp_buf,
+	       DMAC_BASE + DMAC_DEST_START_ADDRR_REG0); /* read to RAM */
+	writel(DMAC_DDMA_PARA_REG_SRC_WAIT_CYC
+			| DMAC_DDMA_PARA_REG_SRC_BLK_SIZE,
+			DMAC_BASE + DMAC_DDMA_PARA_REG0);
+	writel(SPL_WRITE_SIZE, DMAC_BASE + DMAC_DDMA_BC_REG0); /* 1kB */
+	writel(DMAC_DDMA_CFG_REG_LOADING
+		| DMAC_DDMA_CFG_REG_DMA_DEST_DATA_WIDTH_32
+		| DMAC_DDMA_CFG_REG_DMA_SRC_DATA_WIDTH_32
+		| DMAC_DDMA_CFG_REG_DMA_SRC_ADDR_MODE_IO
+		| DMAC_DDMA_CFG_REG_DDMA_SRC_DRQ_TYPE_NFC,
+		DMAC_BASE + DMAC_CFG_REG0);
+
+	writel(0x00E00530, NANDFLASHC_BASE + NANDFLASHC_RCMD_SET); /* READ */
+	writel(1, NANDFLASHC_BASE + NANDFLASHC_SECTOR_NUM);
+	writel(((page & 0xFFFF) << 16) | column,
+	       NANDFLASHC_BASE + NANDFLASHC_ADDR_LOW);
+	writel((page >> 16) & 0xFF, NANDFLASHC_BASE + NANDFLASHC_ADDR_HIGH);
+	writel(NFC_SEND_CMD1 | NFC_SEND_CMD2 | NFC_DATA_TRANS |
+		NFC_PAGE_CMD | NFC_WAIT_FLAG | /* ADDR_CYCLE */ (4 << 16) |
+		NFC_SEND_ADR | NFC_DATA_SWAP_METHOD | (syndrome ? NFC_SEQ : 0),
+		NANDFLASHC_BASE + NANDFLASHC_CMD);
+
+	if (!check_value(NANDFLASHC_BASE + NANDFLASHC_ST, (1 << 2),
+			 MAX_RETRIES)) {
+		printf("Error while initializing dma interrupt\n");
+		return;
+	}
+
+	if (!check_value_negated(DMAC_BASE + DMAC_CFG_REG0,
+				 DMAC_DDMA_CFG_REG_LOADING, MAX_RETRIES)) {
+		printf("Error while waiting for dma transfer to finish\n");
+		return;
+	}
+
+	if (readl(NANDFLASHC_BASE + NANDFLASHC_ECC_ST))
+		ecc_errors++;
+}
+
+int helper_load(uint32_t offs, unsigned int size, void *dest)
+{
+	uint32_t dst;
+	uint32_t count;
+	uint32_t count_c;
+	uint32_t adr = offs;
+
+	memset((void *)dest, 0x0, size); /* clean destination memory */
+	ecc_errors = 0;
+	for (dst = (uint32_t)dest;
+			dst < ((uint32_t)dest + size);
+			dst += SPL_WRITE_SIZE) {
+		/* if < 0x400000 then syndrome read */
+		nand_read_block(adr, adr < 0x400000);
+		count = dst - (uint32_t)dest;
+
+		if (size - count > SPL_WRITE_SIZE)
+			count_c = SPL_WRITE_SIZE;
+		else
+			count_c = size - count;
+
+		memcpy((void *)dst,
+		       (void *)temp_buf,
+		       count_c);
+		adr += SPL_WRITE_SIZE;
+	}
+	return ecc_errors;
+}
+
+int board_nand_init(struct nand_chip *nand)
+{
+	uint32_t val;
+
+	if (nand_initialized) {
+		printf("NAND already initialized, should not be here...\n");
+		return 0;
+	}
+	nand_initialized = 1;
+	nand_set_clocks();
+	val = readl(NANDFLASHC_BASE + NANDFLASHC_CTL);
+	/* enable and reset CTL */
+	writel(val | NFC_CTL_EN | NFC_CTL_RESET,
+	       NANDFLASHC_BASE + NANDFLASHC_CTL);
+
+	if (!check_value_negated(NANDFLASHC_BASE + NANDFLASHC_CTL,
+				 NFC_CTL_RESET, MAX_RETRIES)) {
+		printf("Couldn't initialize nand\n");
+		return 1;
+	}
+
+	return 0;
+}
+
+int nand_spl_load_image(uint32_t offs, unsigned int size, void *dest)
+{
+	helper_load(offs, size, dest);
+	return 0;
+}
+
+void nand_deselect(void) {}
diff --git a/board/sunxi/nand.h b/board/sunxi/nand.h
new file mode 100644
index 0000000..b3c1e93
--- /dev/null
+++ b/board/sunxi/nand.h
@@ -0,0 +1,146 @@
+#ifndef NAND_H
+
+#define NAND_H
+
+#define PORTC_BASE                 0x01c20800
+#define CCU_BASE                   0x01c20000
+#define NANDFLASHC_BASE            0x01c03000
+#define DMAC_BASE                  0x01c02000
+
+#define CCU_AHB_GATING_REG0        0x60
+#define CCU_NAND_SCLK_CFG_REG      0x80
+#define CCU_AHB_GATING_REG0_NAND   (1 << 13)
+
+#define CCU_NAND_SCLK_CFG_REG_SCLK_GATING (1 << 31)
+#define CCU_NAND_SCLK_CFG_REG_CLK_DIV_RATIO (1 << 0)
+
+#define PORTC_PC_CFG0              0x48
+#define PORTC_PC_CFG1              0x4C
+#define PORTC_PC_CFG2              0x50
+#define PORTC_PC_CFG3              0x54
+
+#define PORTC_PC_CFG0_NRB1         (2 << 28)
+#define PORTC_PC_CFG0_NRB0         (2 << 24)
+#define PORTC_PC_CFG0_NRE          (2 << 20)
+#define PORTC_PC_CFG0_NCE0         (2 << 16)
+#define PORTC_PC_CFG0_NCE1         (2 << 12)
+#define PORTC_PC_CFG0_NCLE         (2 << 8)
+#define PORTC_PC_CFG0_NALE         (2 << 4)
+#define PORTC_PC_CFG0_NWE          (2 << 0)
+
+#define PORTC_PC_CFG1_NDQ7         (2 << 28)
+#define PORTC_PC_CFG1_NDQ6         (2 << 24)
+#define PORTC_PC_CFG1_NDQ5         (2 << 20)
+#define PORTC_PC_CFG1_NDQ4         (2 << 16)
+#define PORTC_PC_CFG1_NDQ3         (2 << 12)
+#define PORTC_PC_CFG1_NDQ2         (2 << 8)
+#define PORTC_PC_CFG1_NDQ1         (2 << 4)
+#define PORTC_PC_CFG1_NDQ0         (2 << 0)
+
+#define PORTC_PC_CFG2_NCE7         (2 << 24)
+#define PORTC_PC_CFG2_NCE6         (2 << 20)
+#define PORTC_PC_CFG2_NCE5         (2 << 16)
+#define PORTC_PC_CFG2_NCE4         (2 << 12)
+#define PORTC_PC_CFG2_NCE3         (2 << 8)
+#define PORTC_PC_CFG2_NCE2         (2 << 4)
+#define PORTC_PC_CFG2_NWP          (2 << 0)
+
+#define PORTC_PC_CFG3_NDQS         (2 << 0)
+
+#define BLOCK_SIZE                 0x2000
+
+#define DMAC_CFG_REG0              0x300
+#define DMAC_SRC_START_ADDR_REG0   0x304
+#define DMAC_DEST_START_ADDRR_REG0 0x308
+#define DMAC_DDMA_BC_REG0          0x30C
+#define DMAC_DDMA_PARA_REG0        0x318
+
+#define DMAC_DDMA_CFG_REG_LOADING  (1 << 31)
+#define DMAC_DDMA_CFG_REG_DMA_DEST_DATA_WIDTH_32 (2 << 25)
+#define DMAC_DDMA_CFG_REG_DMA_SRC_DATA_WIDTH_32 (2 << 9)
+#define DMAC_DDMA_CFG_REG_DMA_SRC_ADDR_MODE_IO (1 << 5)
+#define DMAC_DDMA_CFG_REG_DDMA_SRC_DRQ_TYPE_NFC (3 << 0)
+
+#define DMAC_DDMA_PARA_REG_SRC_WAIT_CYC (0x0F << 0)
+#define DMAC_DDMA_PARA_REG_SRC_BLK_SIZE (0x7F << 8)
+
+#define NANDFLASHC_CTL             0x00000000
+
+#define NFC_CTL_EN                 (1 << 0)
+#define NFC_CTL_RESET              (1 << 1)
+#define NFC_CTL_RAM_METHOD         (1 << 14)
+
+#define NANDFLASHC_ST              0x00000004
+#define NANDFLASHC_INT             0x00000008
+#define NANDFLASHC_TIMING_CTL      0x0000000C
+#define NANDFLASHC_TIMING_CFG      0x00000010
+#define NANDFLASHC_ADDR_LOW        0x00000014
+#define NANDFLASHC_ADDR_HIGH       0x00000018
+#define NANDFLASHC_SECTOR_NUM      0x0000001C
+#define NANDFLASHC_CNT             0x00000020
+#define NANDFLASHC_CMD             0x00000024
+#define NANDFLASHC_RCMD_SET        0x00000028
+#define NANDFLASHC_WCMD_SET        0x0000002C
+#define NANDFLASHC_IO_DATA         0x00000030
+#define NANDFLASHC_ECC_CTL         0x00000034
+
+#define NFC_ECC_EN                 (1 << 0)
+#define NFC_ECC_PIPELINE           (1 << 3)
+#define NFC_ECC_EXCEPTION          (1 << 4)
+#define NFC_ECC_BLOCK_SIZE         (1 << 5)
+#define NFC_ECC_RANDOM_EN          (1 << 9)
+#define NFC_ECC_RANDOM_DIRECTION   (1 << 10)
+
+#define NANDFLASHC_ECC_ST          0x00000038
+#define NANDFLASHC_DEBUG           0x0000003C
+#define NANDFLASHC_ECC_CNT0        0x00000040
+#define NANDFLASHC_ECC_CNT1        0x00000044
+#define NANDFLASHC_ECC_CNT2        0x00000048
+#define NANDFLASHC_ECC_CNT3        0x0000004C
+#define NANDFLASHC_USER_DATA_BASE  0x00000050
+#define NANDFLASHC_EFNAND_STATUS   0x00000090
+#define NANDFLASHC_SPARE_AREA      0x000000A0
+#define NANDFLASHC_PATTERN_ID      0x000000A4
+#define NANDFLASHC_RAM0_BASE       0x00000400
+#define NANDFLASHC_RAM1_BASE       0x00000800
+
+#define NFC_SEND_ADR               (1 << 19)
+#define NFC_ACCESS_DIR             (1 << 20)
+#define NFC_DATA_TRANS             (1 << 21)
+#define NFC_SEND_CMD1              (1 << 22)
+#define NFC_WAIT_FLAG              (1 << 23)
+#define NFC_SEND_CMD2              (1 << 24)
+#define NFC_SEQ                    (1 << 25)
+#define NFC_DATA_SWAP_METHOD       (1 << 26)
+#define NFC_ROW_AUTO_INC           (1 << 27)
+#define NFC_SEND_CMD3              (1 << 28)
+#define NFC_SEND_CMD4              (1 << 29)
+
+#define NFC_PAGE_CMD               (2 << 30)
+
+#define SPL_WRITE_SIZE             CONFIG_CMD_SPL_WRITE_SIZE
+
+/* random seed used by linux */
+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,
+};
+
+/* random seed used for syndrome calls */
+const uint16_t random_seed_syndrome = 0x4a80;
+
+#endif /* end of include guard: NAND_H */
-- 
2.3.6

[U-Boot] [PATCH 2/4] sunxi: nand: Add board configuration options

[Piotr Zierhoffer]

From: Piotr Zierhoffer <piotr.zierhoffer@cs.put.poznan.pl>

When SUNXI_NAND option is selected in config, set some configuration
options for sunxi NAND.

This commit also introduces the configurable options in Kconfig.

Signed-off-by: Peter Gielda <pgielda@antmicro.com>
Signed-off-by: Tomasz Gorochowik <tgorochowik@antmicro.com>
Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>
Signed-off-by: Piotr Zierhoffer <pzierhoffer@antmicro.com>
---

 board/sunxi/Kconfig            | 10 ++++++++++
 include/configs/sunxi-common.h | 11 +++++++++++
 2 files changed, 21 insertions(+)

diff --git a/board/sunxi/Kconfig b/board/sunxi/Kconfig
index 2a1cd3c..3b76d64 100644
--- a/board/sunxi/Kconfig
+++ b/board/sunxi/Kconfig
@@ -227,6 +227,16 @@ config OLD_SUNXI_KERNEL_COMPAT
 	Set this to enable various workarounds for old kernels, this results in
 	sub-optimal settings for newer kernels, only enable if needed.
 
+config SUNXI_NAND
+	bool "Support for NAND on Allwinner A20"
+	depends on MACH_SUN7I
+	---help---
+	Enable support for internal NAND. This option allows U-Boot to read from
+	sunxi NAND using DMA transfers. It also adds the a20_nandread command
+	that allows user to transfer a specified amount of data from NAND to
+	memory. Both SPL and full U-Boot driver are enabled. Writing is not
+	supported.
+
 config MMC0_CD_PIN
 	string "Card detect pin for mmc0"
 	default ""
diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h
index 9576bc1..83922ab 100644
--- a/include/configs/sunxi-common.h
+++ b/include/configs/sunxi-common.h
@@ -139,6 +139,17 @@
 #define CONFIG_INITRD_TAG
 #define CONFIG_SERIAL_TAG
 
+#if defined(CONFIG_SUNXI_NAND)
+#define CONFIG_SPL_NAND_DRIVERS
+#define CONFIG_SPL_NAND_SUPPORT
+#define CONFIG_CMD_SPL_WRITE_SIZE 0x000400 /* 1024 byte */
+
+#define CONFIG_SYS_NAND_SPL_KERNEL_OFFS 0x280000
+#define CONFIG_SYS_NAND_U_BOOT_OFFS 0x008000
+#define CONFIG_SYS_NAND_PAGE_SIZE 0x002000 /* 8kb*/
+#define CONFIG_SUNXI_ECC_STRENGTH 40
+#endif
+
 /* mmc config */
 #if !defined(CONFIG_UART0_PORT_F)
 #define CONFIG_MMC
-- 
2.3.6

[U-Boot] [PATCH 3/4] sunxi: nand: Add a20_nandread command to load image from NAND in SPL

[Piotr Zierhoffer]

From: Piotr Zierhoffer <piotr.zierhoffer@cs.put.poznan.pl>

The usage of the command is:

a20_nandread <address> <offset> <bytes>

It allows user to copy data from NAND to memory. It employs
nand_spl_load_image from the sunxi NAND driver.

It is added only when the NAND support is enabled.

Signed-off-by: Peter Gielda <pgielda@antmicro.com>
Signed-off-by: Tomasz Gorochowik <tgorochowik@antmicro.com>
Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>
Signed-off-by: Piotr Zierhoffer <pzierhoffer@antmicro.com>
---

 common/Makefile                |  1 +
 common/cmd_a20_nandread.c      | 25 +++++++++++++++++++++++++
 include/configs/sunxi-common.h |  2 ++
 3 files changed, 28 insertions(+)
 create mode 100644 common/cmd_a20_nandread.c

diff --git a/common/Makefile b/common/Makefile
index d6c1d48..ef31646 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -126,6 +126,7 @@ obj-$(CONFIG_ID_EEPROM) += cmd_mac.o
 obj-$(CONFIG_CMD_MD5SUM) += cmd_md5sum.o
 obj-$(CONFIG_CMD_MEMORY) += cmd_mem.o
 obj-$(CONFIG_CMD_IO) += cmd_io.o
+obj-$(CONFIG_CMD_A20_NANDREAD) += cmd_a20_nandread.o
 obj-$(CONFIG_CMD_MFSL) += cmd_mfsl.o
 obj-$(CONFIG_MII) += miiphyutil.o
 obj-$(CONFIG_CMD_MII) += miiphyutil.o
diff --git a/common/cmd_a20_nandread.c b/common/cmd_a20_nandread.c
new file mode 100644
index 0000000..6469535
--- /dev/null
+++ b/common/cmd_a20_nandread.c
@@ -0,0 +1,25 @@
+#include <common.h>
+#include <command.h>
+#include <nand.h>
+
+static int do_a20_nandread(cmd_tbl_t *cmdtp, int flag, int argc,
+		char * const argv[])
+{
+	if (argc != 4) {
+		printf("usage: a20_nandread <destination> <source> <size>\n");
+		return 1;
+	}
+
+	uint32_t dst = simple_strtoul(argv[1], NULL, 16);
+	uint32_t src = simple_strtoul(argv[2], NULL, 16);
+	uint32_t cnt = simple_strtoul(argv[3], NULL, 16);
+	printf("Reading 0x%08X bytes from NAND @ 0x%08X to MEM @ 0x%08X...\n",
+	       cnt, src, dst);
+	nand_spl_load_image(src, cnt, (void *)dst);
+	printf("\n");
+	return 0;
+}
+
+U_BOOT_CMD(a20_nandread, CONFIG_SYS_MAXARGS, 3,
+	   do_a20_nandread, "a20_nandread", "[destination source size]\n" "   "
+);
diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h
index 83922ab..e9cfa9a 100644
--- a/include/configs/sunxi-common.h
+++ b/include/configs/sunxi-common.h
@@ -148,6 +148,8 @@
 #define CONFIG_SYS_NAND_U_BOOT_OFFS 0x008000
 #define CONFIG_SYS_NAND_PAGE_SIZE 0x002000 /* 8kb*/
 #define CONFIG_SUNXI_ECC_STRENGTH 40
+
+#define CONFIG_CMD_A20_NANDREAD
 #endif
 
 /* mmc config */
-- 
2.3.6

[U-Boot] [PATCH 4/4] sunxi: nand: Add information to sunxi that it was run from NAND in SPL

[Piotr Zierhoffer]

As SPL does not know which source to choose when booting U-Boot, choose
NAND if it is capable of doing so.

Signed-off-by: Peter Gielda <pgielda@antmicro.com>
Signed-off-by: Tomasz Gorochowik <tgorochowik@antmicro.com>
Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>
Signed-off-by: Piotr Zierhoffer <pzierhoffer@antmicro.com>

---

 arch/arm/cpu/armv7/sunxi/board.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/cpu/armv7/sunxi/board.c b/arch/arm/cpu/armv7/sunxi/board.c
index 5f39aa0..e4b7d63 100644
--- a/arch/arm/cpu/armv7/sunxi/board.c
+++ b/arch/arm/cpu/armv7/sunxi/board.c
@@ -128,6 +128,9 @@ void s_init(void)
  */
 u32 spl_boot_device(void)
 {
+#ifdef CONFIG_SPL_NAND_SUPPORT
+	return BOOT_DEVICE_NAND;
+#else
 	/*
 	 * When booting from the SD card, the "eGON.BT0" signature is expected
 	 * to be found in memory at the address 0x0004 (see the "mksunxiboot"
@@ -148,6 +151,7 @@ u32 spl_boot_device(void)
 		return BOOT_DEVICE_MMC1;
 	else
 		return BOOT_DEVICE_BOARD;
+#endif
 }
 
 /* No confirmation data available in SPL yet. Hardcode bootmode */
-- 
2.3.6

[U-Boot] [PATCH 1/4] sunxi: nand: Add basic sunxi NAND driver with DMA support

[Scott Wood]

On Thu, 2015-07-16 at 13:25 +0200, Piotr Zierhoffer wrote:
> From: Piotr Zierhoffer <piotr.zierhoffer@cs.put.poznan.pl>
> 
> This driver adds NAND support to both SPL and full U-Boot.
> It was tested on AllWinner A20.

It looks a lot like an SPL-only driver to me...

> 
> Signed-off-by: Peter Gielda <pgielda@antmicro.com>
> Signed-off-by: Tomasz Gorochowik <tgorochowik@antmicro.com>
> Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>
> Signed-off-by: Piotr Zierhoffer <pzierhoffer@antmicro.com>
> ---
> 
>  board/sunxi/Makefile |   1 +
>  board/sunxi/nand.c   | 315 
> +++++++++++++++++++++++++++++++++++++++++++++++++++
>  board/sunxi/nand.h   | 146 ++++++++++++++++++++++++
>  3 files changed, 462 insertions(+)
>  create mode 100644 board/sunxi/nand.c
>  create mode 100644 board/sunxi/nand.h

Please keep NAND drivers in drivers/mtd/nand/.

> 
> diff --git a/board/sunxi/Makefile b/board/sunxi/Makefile
> index 43766e0..6f1d571 100644
> --- a/board/sunxi/Makefile
> +++ b/board/sunxi/Makefile
> @@ -9,6 +9,7 @@
>  # SPDX-License-Identifier:   GPL-2.0+
>  #
>  obj-y        += board.o
> +obj-$(CONFIG_SUNXI_NAND)     += nand.o
>  obj-$(CONFIG_SUNXI_GMAC)     += gmac.o
>  obj-$(CONFIG_SUNXI_AHCI)     += ahci.o
>  obj-$(CONFIG_MACH_SUN4I)     += dram_sun4i_auto.o
> diff --git a/board/sunxi/nand.c b/board/sunxi/nand.c
> new file mode 100644
> index 0000000..16e8e4d
> --- /dev/null
> +++ b/board/sunxi/nand.c
> @@ -0,0 +1,315 @@
> +/*
> + * Copyright (c) 2014-2015, Antmicro Ltd <www.antmicro.com>
> + * Copyright (c) 2015, AW-SOM Technologies <www.aw-som.com>
> + *
> + * SPDX-License-Identifier:     GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <config.h>
> +#include <asm/io.h>
> +#include <nand.h>
> +#include "nand.h" /* sunxi specific header */
> +
> +/* minimal "boot0" style NAND support for Allwinner A20 */
> +
> +/* temporary buffer in internal ram */
> +#ifdef CONFIG_SPL_BUILD
> +/* in SPL temporary buffer cannot be @ 0x0 */
> +unsigned char temp_buf[SPL_WRITE_SIZE] __aligned(0x10) __section(".text#");
> +#else
> +/* put temporary buffer @ 0x0 */
> +unsigned char *temp_buf = (unsigned char *)0x0;
> +#endif

If 0x0 is the address of an SRAM, its address should be symbolically defined. 
 Also consider mapping it to a different virtual address, to avoid potential 
compiler mischief.

> +#define MAX_RETRIES 10
> +
> +static int check_value_inner(int offset, int expected_bits,
> +                             int max_number_of_retries, int negation)
> +{
> +     int retries = 0;
> +     do {
> +             int val = readl(offset) & expected_bits;
> +             if (negation ? !val : val)
> +                     return 1;
> +             mdelay(1);
> +             retries++;
> +     } while (retries < max_number_of_retries);
> +
> +     return 0;
> +}
> +
> +static inline int check_value(int offset, int expected_bits,
> +                             int max_number_of_retries)
> +{
> +     return check_value_inner(offset, expected_bits,
> +                                     max_number_of_retries, 0);
> +}
> +
> +static inline int check_value_negated(int offset, int unexpected_bits,
> +                                     int max_number_of_retries)
> +{
> +     return check_value_inner(offset, unexpected_bits,
> +                                     max_number_of_retries, 1);
> +}
> +
> +void nand_set_clocks(void)
> +{

Here and elsewhere, please either use static or less generic name.

+int nand_initialized;

static bool nand_initialized;

Or better yet, get rid of this and stop using init-on-first-use.

> +
> +void nand_init(void)
> +{
> +     board_nand_init(NULL);
> +}

nand_init() is defined in drivers/mtd/nand/nand.c, which is only optional for 
SPL -- and I don't see an SPL ifdef here.

> +uint32_t ecc_errors;

static

> +
> +/* read 0x400 bytes from real_addr to temp_buf */
> +void nand_read_block(unsigned int real_addr, int syndrome)

I don't know of any NAND chips whose erase blocks are as small as 0x400 
bytes.  The read/program unit is called a page, not a block.

Is there a reason to hardcode the page size here?  Especially since the 
comment doesn't appear to match the code, which uses SPL_WRITE_SIZE.

> +{
> +     uint32_t val;
> +     int ECC_OFF = 0;

Why is this capitalized?

> +     uint16_t ecc_mode = 0;
> +     uint16_t rand_seed;
> +     uint32_t page;
> +     uint16_t column;
> +     uint32_t oob_offset;
> +
> +     if (!nand_initialized)
> +             board_nand_init(NULL);

board_nand_init() is called from nand_init().  Why are you calling it here?

> +     switch (CONFIG_SUNXI_ECC_STRENGTH) {

No documentation for this symbol.

If it's an attribute of the hardware rather than something the user selects, 
it should be CONFIG_SYS_SUNXI_ECC_STRENGTH, or just SUNXI_ECC_STRENGTH if it 
doesn't need to integrate with kconfig or makefiles.


> +     /* clear temp_buf */
> +     memset((void *)temp_buf, 0, SPL_WRITE_SIZE);

Unnecessary cast.

> +
> +     /* set CMD  */
> +     writel(NFC_SEND_CMD1 | NFC_WAIT_FLAG | 0xff,
> +            NANDFLASHC_BASE + NANDFLASHC_CMD);
> +
> +     if (!check_value(NANDFLASHC_BASE + NANDFLASHC_ST, (1 << 1),
> +                      MAX_RETRIES)) {
> +             printf("Error while initilizing command interrupt\n");
> +             return;
> +     }
> +
> +     page = (real_addr / BLOCK_SIZE);

Unnecessary parens (and inconsistent with the following line).

> +     column = real_addr % BLOCK_SIZE;
> +
> +     if (syndrome) {
> +             /* shift every 1kB in syndrome */
> +             column += (column / SPL_WRITE_SIZE) * ECC_OFF;
> +     }
> +
> +     /* clear ecc status */
> +     writel(0, NANDFLASHC_BASE + NANDFLASHC_ECC_ST);
> +
> +     /* Choose correct seed */
> +     if (syndrome)
> +             rand_seed = random_seed_syndrome;
> +     else
> +             rand_seed = random_seed[page % 128];
> +
> +     writel((rand_seed << 16) | NFC_ECC_RANDOM_EN | NFC_ECC_EN
> +             | NFC_ECC_PIPELINE | (ecc_mode << 12),
> +             NANDFLASHC_BASE + NANDFLASHC_ECC_CTL);
> +
> +     val = readl(NANDFLASHC_BASE + NANDFLASHC_CTL);
> +     writel(val | NFC_CTL_RAM_METHOD, NANDFLASHC_BASE + NANDFLASHC_CTL);
> +
> +     if (syndrome) {
> +             writel(SPL_WRITE_SIZE, NANDFLASHC_BASE + NANDFLASHC_SPARE_AREA);
> +     } else {
> +             oob_offset = BLOCK_SIZE + (column / SPL_WRITE_SIZE) * ECC_OFF;
> +             writel(oob_offset, NANDFLASHC_BASE + NANDFLASHC_SPARE_AREA);
> +     }
> +
> +     /* DMAC */
> +     writel(0x0, DMAC_BASE + DMAC_CFG_REG0); /* clr dma cmd */
> +     /* read from REG_IO_DATA */
> +     writel(NANDFLASHC_BASE + NANDFLASHC_IO_DATA,
> +            DMAC_BASE + DMAC_SRC_START_ADDR_REG0);
> +     writel((uint32_t)temp_buf,
> +            DMAC_BASE + DMAC_DEST_START_ADDRR_REG0); /* read to RAM */
> +     writel(DMAC_DDMA_PARA_REG_SRC_WAIT_CYC
> +                     | DMAC_DDMA_PARA_REG_SRC_BLK_SIZE,
> +                     DMAC_BASE + DMAC_DDMA_PARA_REG0);
> +     writel(SPL_WRITE_SIZE, DMAC_BASE + DMAC_DDMA_BC_REG0); /* 1kB */
> +     writel(DMAC_DDMA_CFG_REG_LOADING
> +             | DMAC_DDMA_CFG_REG_DMA_DEST_DATA_WIDTH_32
> +             | DMAC_DDMA_CFG_REG_DMA_SRC_DATA_WIDTH_32
> +             | DMAC_DDMA_CFG_REG_DMA_SRC_ADDR_MODE_IO
> +             | DMAC_DDMA_CFG_REG_DDMA_SRC_DRQ_TYPE_NFC,
> +             DMAC_BASE + DMAC_CFG_REG0);
> +
> +     writel(0x00E00530, NANDFLASHC_BASE + NANDFLASHC_RCMD_SET); /* READ */

What is 0x00e00530?  Define it symbolically.


> +     writel(1, NANDFLASHC_BASE + NANDFLASHC_SECTOR_NUM);
> +     writel(((page & 0xFFFF) << 16) | column,
> +            NANDFLASHC_BASE + NANDFLASHC_ADDR_LOW);
> +     writel((page >> 16) & 0xFF, NANDFLASHC_BASE + NANDFLASHC_ADDR_HIGH);
> +     writel(NFC_SEND_CMD1 | NFC_SEND_CMD2 | NFC_DATA_TRANS |
> +             NFC_PAGE_CMD | NFC_WAIT_FLAG | /* ADDR_CYCLE */ (4 << 16) |

Why is ADDR_CYCLE commented out, and a magic number used in its place?

> +int helper_load(uint32_t offs, unsigned int size, void *dest)

This name is not clear (in addition to being way too vague for a global 
symbol).

> +{
> +     uint32_t dst;

Don't have both "dest" and "dst".  It's confusing.

> +     uint32_t count;
> +     uint32_t count_c;

What does the "_c" mean?

> +     uint32_t adr = offs;

So address is actually a NAND offset, not a memory address?  Confusing.

Don't put addresses in uint32_t (even if it doesn't make a difference on this 
platform, it's a bad habit and a bad example to others).  Use a pointer for 
virtual addresses, and phys_addr_t for physical addresses.

> +
> +     memset((void *)dest, 0x0, size); /* clean destination memory */

Unnecessary cast.

> +     ecc_errors = 0;
> +     for (dst = (uint32_t)dest;
> +                     dst < ((uint32_t)dest + size);
> +                     dst += SPL_WRITE_SIZE) {
> +             /* if < 0x400000 then syndrome read */
> +             nand_read_block(adr, adr < 0x400000);

What does 0x400000 represent?

> +int nand_spl_load_image(uint32_t offs, unsigned int size, void *dest)
> +{
> +     helper_load(offs, size, dest);
> +     return 0;
> +}

Why didn't you just call helper_load() nand_spl_load_image()?


> +
> +void nand_deselect(void) {}
> diff --git a/board/sunxi/nand.h b/board/sunxi/nand.h
> new file mode 100644
> index 0000000..b3c1e93
> --- /dev/null
> +++ b/board/sunxi/nand.h
> @@ -0,0 +1,146 @@
> +#ifndef NAND_H
> +
> +#define NAND_H

Don't use such a generic ifdef protector in a non-generic file.  It's just 
chance that include/nand.h is using _NAND_H_ instead of NAND_H...

-Scott

[U-Boot] [PATCH 3/4] sunxi: nand: Add a20_nandread command to load image from NAND in SPL

[Scott Wood]

On Thu, 2015-07-16 at 13:25 +0200, Piotr Zierhoffer wrote:
> From: Piotr Zierhoffer <piotr.zierhoffer@cs.put.poznan.pl>
> 
> The usage of the command is:
> 
> a20_nandread <address> <offset> <bytes>
> 
> It allows user to copy data from NAND to memory. It employs
> nand_spl_load_image from the sunxi NAND driver.
> 
> It is added only when the NAND support is enabled.

Please respond to the comments received when it was posted at 
http://patchwork.ozlabs.org/patch/466149/

> Signed-off-by: Peter Gielda <pgielda@antmicro.com>
> Signed-off-by: Tomasz Gorochowik <tgorochowik@antmicro.com>
> Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>
> Signed-off-by: Piotr Zierhoffer <pzierhoffer@antmicro.com>

Why does the same code now have a completely different author and sign-off 
chain?

-Scott

[U-Boot] [PATCH 1/4] sunxi: nand: Add basic sunxi NAND driver with DMA support

[Marek Vasut]

On Thursday, July 16, 2015 at 11:15:58 PM, Scott Wood wrote:

[...]

> > +/* temporary buffer in internal ram */
> > +#ifdef CONFIG_SPL_BUILD
> > +/* in SPL temporary buffer cannot be @ 0x0 */
> > +unsigned char temp_buf[SPL_WRITE_SIZE] __aligned(0x10)
> > __section(".text#"); +#else
> > +/* put temporary buffer @ 0x0 */
> > +unsigned char *temp_buf = (unsigned char *)0x0;
> > +#endif
> 
> If 0x0 is the address of an SRAM, its address should be symbolically
> defined. Also consider mapping it to a different virtual address, to avoid
> potential compiler mischief.

The DMA I believe accesses it via PA anyway, so mapping it elsewhere would
only confuse everyone who's hacking on the driver. Just my 5 cents ;-)

[...]

Best regards,
Marek Vasut

[U-Boot] [PATCH 1/4] sunxi: nand: Add basic sunxi NAND driver with DMA support

[Scott Wood]

On Thu, 2015-07-16 at 23:26 +0200, Marek Vasut wrote:
> On Thursday, July 16, 2015 at 11:15:58 PM, Scott Wood wrote:
> 
> [...]
> 
> > > +/* temporary buffer in internal ram */
> > > +#ifdef CONFIG_SPL_BUILD
> > > +/* in SPL temporary buffer cannot be @ 0x0 */
> > > +unsigned char temp_buf[SPL_WRITE_SIZE] __aligned(0x10)
> > > __section(".text#"); +#else
> > > +/* put temporary buffer @ 0x0 */
> > > +unsigned char *temp_buf = (unsigned char *)0x0;
> > > +#endif
> > 
> > If 0x0 is the address of an SRAM, its address should be symbolically
> > defined. Also consider mapping it to a different virtual address, to avoid
> > potential compiler mischief.
> 
> The DMA I believe accesses it via PA anyway, so mapping it elsewhere would
> only confuse everyone who's hacking on the driver. Just my 5 cents ;-)

Hey, if it wakes people up to the fact that they ought to be using 
virt_to_phys() (or better yet, something that distinguishes DMA addresses 
from physical), great! :-)

And yes, people go on to do the same "everything is u32" crap in non-platform-
specific files.  Just look at drivers/block/ahci.c.

-Scott

[U-Boot] [PATCH 1/4] sunxi: nand: Add basic sunxi NAND driver with DMA support

[Marek Vasut]

On Thursday, July 16, 2015 at 11:36:08 PM, Scott Wood wrote:
> On Thu, 2015-07-16 at 23:26 +0200, Marek Vasut wrote:
> > On Thursday, July 16, 2015 at 11:15:58 PM, Scott Wood wrote:
> > 
> > [...]
> > 
> > > > +/* temporary buffer in internal ram */
> > > > +#ifdef CONFIG_SPL_BUILD
> > > > +/* in SPL temporary buffer cannot be @ 0x0 */
> > > > +unsigned char temp_buf[SPL_WRITE_SIZE] __aligned(0x10)
> > > > __section(".text#"); +#else
> > > > +/* put temporary buffer @ 0x0 */
> > > > +unsigned char *temp_buf = (unsigned char *)0x0;
> > > > +#endif
> > > 
> > > If 0x0 is the address of an SRAM, its address should be symbolically
> > > defined. Also consider mapping it to a different virtual address, to
> > > avoid potential compiler mischief.
> > 
> > The DMA I believe accesses it via PA anyway, so mapping it elsewhere
> > would only confuse everyone who's hacking on the driver. Just my 5 cents
> > ;-)
> 
> Hey, if it wakes people up to the fact that they ought to be using
> virt_to_phys() (or better yet, something that distinguishes DMA addresses
> from physical), great! :-)

Not really useful here, but whatever.

> And yes, people go on to do the same "everything is u32" crap in
> non-platform- specific files.  Just look at drivers/block/ahci.c.

Errr, this file is platform specific.

Best regards,
Marek Vasut

[U-Boot] [PATCH 1/4] sunxi: nand: Add basic sunxi NAND driver with DMA support

[Scott Wood]

On Fri, 2015-07-17 at 00:06 +0200, Marek Vasut wrote:
> On Thursday, July 16, 2015 at 11:36:08 PM, Scott Wood wrote:
> > On Thu, 2015-07-16 at 23:26 +0200, Marek Vasut wrote:
> > > On Thursday, July 16, 2015 at 11:15:58 PM, Scott Wood wrote:
> > > 
> > > [...]
> > > 
> > > > > +/* temporary buffer in internal ram */
> > > > > +#ifdef CONFIG_SPL_BUILD
> > > > > +/* in SPL temporary buffer cannot be @ 0x0 */
> > > > > +unsigned char temp_buf[SPL_WRITE_SIZE] __aligned(0x10)
> > > > > __section(".text#"); +#else
> > > > > +/* put temporary buffer @ 0x0 */
> > > > > +unsigned char *temp_buf = (unsigned char *)0x0;
> > > > > +#endif
> > > > 
> > > > If 0x0 is the address of an SRAM, its address should be symbolically
> > > > defined. Also consider mapping it to a different virtual address, to
> > > > avoid potential compiler mischief.
> > > 
> > > The DMA I believe accesses it via PA anyway, so mapping it elsewhere
> > > would only confuse everyone who's hacking on the driver. Just my 5 cents
> > > ;-)
> > 
> > Hey, if it wakes people up to the fact that they ought to be using
> > virt_to_phys() (or better yet, something that distinguishes DMA addresses
> > from physical), great! :-)
> 
> Not really useful here, but whatever.
> 
> > And yes, people go on to do the same "everything is u32" crap in
> > non-platform- specific files.  Just look at drivers/block/ahci.c.
> 
> Errr, this file is platform specific.

(I'm assuming that by "this file" you mean the sunxi file, not the ahci file.)

The point is that establishing good habits everywhere reduces the likelihood 
of the bad habits migrating to places where things break.

And no, I don't seriously expect my suggestion of remapping the virtual 
address to be implemented, but GCC has been known to do all sorts of weird 
stuff when it thinks it knows a NULL pointer is being dereferenced.

-Scott

[U-Boot] [PATCH 1/4] sunxi: nand: Add basic sunxi NAND driver with DMA support

[Marek Vasut]

On Friday, July 17, 2015 at 12:12:15 AM, Scott Wood wrote:
> On Fri, 2015-07-17 at 00:06 +0200, Marek Vasut wrote:
> > On Thursday, July 16, 2015 at 11:36:08 PM, Scott Wood wrote:
> > > On Thu, 2015-07-16 at 23:26 +0200, Marek Vasut wrote:
> > > > On Thursday, July 16, 2015 at 11:15:58 PM, Scott Wood wrote:
> > > > 
> > > > [...]
> > > > 
> > > > > > +/* temporary buffer in internal ram */
> > > > > > +#ifdef CONFIG_SPL_BUILD
> > > > > > +/* in SPL temporary buffer cannot be @ 0x0 */
> > > > > > +unsigned char temp_buf[SPL_WRITE_SIZE] __aligned(0x10)
> > > > > > __section(".text#"); +#else
> > > > > > +/* put temporary buffer @ 0x0 */
> > > > > > +unsigned char *temp_buf = (unsigned char *)0x0;
> > > > > > +#endif
> > > > > 
> > > > > If 0x0 is the address of an SRAM, its address should be
> > > > > symbolically defined. Also consider mapping it to a different
> > > > > virtual address, to avoid potential compiler mischief.
> > > > 
> > > > The DMA I believe accesses it via PA anyway, so mapping it elsewhere
> > > > would only confuse everyone who's hacking on the driver. Just my 5
> > > > cents ;-)
> > > 
> > > Hey, if it wakes people up to the fact that they ought to be using
> > > virt_to_phys() (or better yet, something that distinguishes DMA
> > > addresses from physical), great! :-)
> > 
> > Not really useful here, but whatever.
> > 
> > > And yes, people go on to do the same "everything is u32" crap in
> > > non-platform- specific files.  Just look at drivers/block/ahci.c.
> > 
> > Errr, this file is platform specific.
> 
> (I'm assuming that by "this file" you mean the sunxi file, not the ahci
> file.)
> 
> The point is that establishing good habits everywhere reduces the
> likelihood of the bad habits migrating to places where things break.
> 
> And no, I don't seriously expect my suggestion of remapping the virtual
> address to be implemented, but GCC has been known to do all sorts of weird
> stuff when it thinks it knows a NULL pointer is being dereferenced.

OK, you have a point there.

Best regards,
Marek Vasut

[U-Boot] [PATCH 3/4] sunxi: nand: Add a20_nandread command to load image from NAND in SPL

[Piotr Zierhoffer]

Hello

2015-07-16 23:20 GMT+02:00 Scott Wood <scottwood@freescale.com>:
> On Thu, 2015-07-16 at 13:25 +0200, Piotr Zierhoffer wrote:
>> Signed-off-by: Peter Gielda <pgielda@antmicro.com>
>> Signed-off-by: Tomasz Gorochowik <tgorochowik@antmicro.com>
>> Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>
>> Signed-off-by: Piotr Zierhoffer <pzierhoffer@antmicro.com>
>
> Why does the same code now have a completely different author and sign-off
> chain?
>
> -Scott

This code had a wrong copyright/Signed-off-by chain, and was reverted
in da9971d1b3bdb554d4a4ac948119f8b2616bbcce.

I am going to resubmit a v2 patch set without this command.

Best regards

Piotr Zierhoffer
Antmicro Ltd | www.antmicro.com

[U-Boot] [PATCH 1/4] sunxi: nand: Add basic sunxi NAND driver with DMA support

[Piotr Zierhoffer]

2015-07-16 23:15 GMT+02:00 Scott Wood <scottwood@freescale.com>:
> On Thu, 2015-07-16 at 13:25 +0200, Piotr Zierhoffer wrote:
>> From: Piotr Zierhoffer <piotr.zierhoffer@cs.put.poznan.pl>
>>
>> This driver adds NAND support to both SPL and full U-Boot.
>> It was tested on AllWinner A20.
>
> It looks a lot like an SPL-only driver to me...
>

I am preparing a v2 patch set, that will have SPL-only code.

>> +     writel(0x00E00530, NANDFLASHC_BASE + NANDFLASHC_RCMD_SET); /* READ */
>
> What is 0x00e00530?  Define it symbolically.
>

It's difficult because we do not have much explanation for this
constant. This is how BROM operates.
We have tuned it according to http://linux-sunxi.org/NFC_Register_Guide

We have fixed the code according to your comments and will resubmit it shortly.

Best regards

Piotr Zierhoffer
Antmicro Ltd | www.antmicro.com

[U-Boot] [PATCH v2 0/3] sunxi: nand: Basic NAND driver for SPL

[Piotr Zierhoffer]

This is a basic driver for the sunxi NAND controller for Allwinner A20.
It supports only SPL.

The driver uses DMA for data transfers. It does not support writing.

Changes in v2:
- removed traces of non-SPL-specific code
- moved the driver from boards/sunxi to drivers/mtd/nand
- moved magic values to defines (whenever possible)
- removed unnecesary late initialisation code
- code style changes as suggested for the first patch set:
  - changed visibility of some symbols
  - renamed unclear variables
  - renamed header protector
  - changed types of pointer variables
  - other minor changes
- renamed defines to be more relevant
- moved Kconfig entry for the driver to drivers/mtd/nand
- reworded Kconfig entry help

Piotr Zierhoffer (3):
  sunxi: nand: Add basic sunxi NAND driver for SPL with DMA support
  sunxi: nand: Add board configuration options
  sunxi: nand: Add information to sunxi that it was run from NAND in SPL

 arch/arm/cpu/armv7/sunxi/board.c |   4 +
 drivers/mtd/nand/Kconfig         |   7 +
 drivers/mtd/nand/Makefile        |   1 +
 drivers/mtd/nand/sunxi_nand.c    | 289 +++++++++++++++++++++++++++++++++++++++
 drivers/mtd/nand/sunxi_nand.h    | 151 ++++++++++++++++++++
 include/configs/sunxi-common.h   |  11 ++
 6 files changed, 463 insertions(+)
 create mode 100644 drivers/mtd/nand/sunxi_nand.c
 create mode 100644 drivers/mtd/nand/sunxi_nand.h

-- 
2.3.6

[U-Boot] [PATCH v2 1/3] sunxi: nand: Add basic sunxi NAND driver for SPL with DMA support

[Piotr Zierhoffer]

From: Piotr Zierhoffer <piotr.zierhoffer@cs.put.poznan.pl>

This driver adds NAND support to SPL.
It was tested on AllWinner A20.

Signed-off-by: Peter Gielda <pgielda@antmicro.com>
Signed-off-by: Tomasz Gorochowik <tgorochowik@antmicro.com>
Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>
Signed-off-by: Piotr Zierhoffer <pzierhoffer@antmicro.com>
Signed-off-by: Karol Gugala <kgugala@antmicro.com>
---

Changes in v2:
- removed traces of non-SPL-specific code
- moved the driver from boards/sunxi to drivers/mtd/nand
- moved magic values to defines (whenever possible)
- removed unnecesary late initialisation code
- code style changes as suggested for the first patch set:
  - changed visibility of some symbols
  - renamed unclear variables
  - renamed header protector
  - changed types of pointer variables
  - other minor changes

 drivers/mtd/nand/Makefile     |   1 +
 drivers/mtd/nand/sunxi_nand.c | 289 ++++++++++++++++++++++++++++++++++++++++++
 drivers/mtd/nand/sunxi_nand.h | 151 ++++++++++++++++++++++
 3 files changed, 441 insertions(+)
 create mode 100644 drivers/mtd/nand/sunxi_nand.c
 create mode 100644 drivers/mtd/nand/sunxi_nand.h

diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
index 347ea62..4cf9cee 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -12,6 +12,7 @@ NORMAL_DRIVERS=y
 endif
 
 obj-$(CONFIG_SPL_NAND_AM33XX_BCH) += am335x_spl_bch.o
+obj-$(CONFIG_SPL_NAND_SUNXI) += sunxi_nand.o
 obj-$(CONFIG_SPL_NAND_DENALI) += denali_spl.o
 obj-$(CONFIG_SPL_NAND_DOCG4) += docg4_spl.o
 obj-$(CONFIG_SPL_NAND_SIMPLE) += nand_spl_simple.o
diff --git a/drivers/mtd/nand/sunxi_nand.c b/drivers/mtd/nand/sunxi_nand.c
new file mode 100644
index 0000000..8d66d2b
--- /dev/null
+++ b/drivers/mtd/nand/sunxi_nand.c
@@ -0,0 +1,289 @@
+/*
+ * Copyright (c) 2014-2015, Antmicro Ltd <www.antmicro.com>
+ * Copyright (c) 2015, AW-SOM Technologies <www.aw-som.com>
+ *
+ * SPDX-License-Identifier:     GPL-2.0+
+ */
+
+#include <common.h>
+#include <config.h>
+#include <asm/io.h>
+#include <nand.h>
+#include "sunxi_nand.h"
+
+/* minimal "boot0" style NAND support for Allwinner A20 */
+
+/* temporary buffer in internal ram */
+unsigned char temp_buf[CONFIG_SYS_NAND_PAGE_SIZE]
+	__aligned(0x10) __section(".text#");
+
+#define MAX_RETRIES 10
+
+static int check_value_inner(int offset, int expected_bits,
+				int max_number_of_retries, int negation)
+{
+	int retries = 0;
+	do {
+		int val = readl(offset) & expected_bits;
+		if (negation ? !val : val)
+			return 1;
+		mdelay(1);
+		retries++;
+	} while (retries < max_number_of_retries);
+
+	return 0;
+}
+
+static inline int check_value(int offset, int expected_bits,
+				int max_number_of_retries)
+{
+	return check_value_inner(offset, expected_bits,
+					max_number_of_retries, 0);
+}
+
+static inline int check_value_negated(int offset, int unexpected_bits,
+					int max_number_of_retries)
+{
+	return check_value_inner(offset, unexpected_bits,
+					max_number_of_retries, 1);
+}
+
+static void nand_set_clocks(void)
+{
+	uint32_t val;
+
+	writel(PORTC_PC_CFG0_NRB1 |
+		PORTC_PC_CFG0_NRB0 |
+		PORTC_PC_CFG0_NRE  |
+		PORTC_PC_CFG0_NCE0 |
+		PORTC_PC_CFG0_NCE1 |
+		PORTC_PC_CFG0_NCLE |
+		PORTC_PC_CFG0_NALE |
+		PORTC_PC_CFG0_NWE, PORTC_BASE + PORTC_PC_CFG0);
+
+	writel(PORTC_PC_CFG1_NDQ7 |
+		PORTC_PC_CFG1_NDQ6 |
+		PORTC_PC_CFG1_NDQ5 |
+		PORTC_PC_CFG1_NDQ4 |
+		PORTC_PC_CFG1_NDQ3 |
+		PORTC_PC_CFG1_NDQ2 |
+		PORTC_PC_CFG1_NDQ1 |
+		PORTC_PC_CFG1_NDQ0, PORTC_BASE + PORTC_PC_CFG1);
+
+	writel(PORTC_PC_CFG2_NCE7 |
+		PORTC_PC_CFG2_NCE6 |
+		PORTC_PC_CFG2_NCE5 |
+		PORTC_PC_CFG2_NCE4 |
+		PORTC_PC_CFG2_NCE3 |
+		PORTC_PC_CFG2_NCE2 |
+		PORTC_PC_CFG2_NWP, PORTC_BASE + PORTC_PC_CFG2);
+
+	writel(PORTC_PC_CFG3_NDQS, PORTC_BASE + PORTC_PC_CFG3);
+
+	val = readl(CCU_BASE + CCU_AHB_GATING_REG0);
+	writel(CCU_AHB_GATING_REG0_NAND | val, CCU_BASE + CCU_AHB_GATING_REG0);
+
+	val = readl(CCU_BASE + CCU_NAND_SCLK_CFG_REG);
+	writel(val | CCU_NAND_SCLK_CFG_REG_SCLK_GATING
+		| CCU_NAND_SCLK_CFG_REG_CLK_DIV_RATIO,
+		CCU_BASE + CCU_NAND_SCLK_CFG_REG);
+}
+
+void nand_init(void)
+{
+	uint32_t val;
+
+	nand_set_clocks();
+	val = readl(NANDFLASHC_BASE + NANDFLASHC_CTL);
+	/* enable and reset CTL */
+	writel(val | NFC_CTL_EN | NFC_CTL_RESET,
+	       NANDFLASHC_BASE + NANDFLASHC_CTL);
+
+	if (!check_value_negated(NANDFLASHC_BASE + NANDFLASHC_CTL,
+				 NFC_CTL_RESET, MAX_RETRIES)) {
+		printf("Couldn't initialize nand\n");
+	}
+}
+
+static uint32_t ecc_errors;
+
+void nand_read_page(unsigned int real_addr, int syndrome)
+{
+	uint32_t val;
+	int ecc_off = 0;
+	uint16_t ecc_mode = 0;
+	uint16_t rand_seed;
+	uint32_t page;
+	uint16_t column;
+	uint32_t oob_offset;
+
+	switch (SUNXI_ECC_STRENGTH) {
+	case 16:
+		ecc_mode = 0;
+		ecc_off = 0x20;
+		break;
+	case 24:
+		ecc_mode = 1;
+		ecc_off = 0x2e;
+		break;
+	case 28:
+		ecc_mode = 2;
+		ecc_off = 0x32;
+		break;
+	case 32:
+		ecc_mode = 3;
+		ecc_off = 0x3c;
+		break;
+	case 40:
+		ecc_mode = 4;
+		ecc_off = 0x4a;
+		break;
+	case 48:
+		ecc_mode = 4;
+		ecc_off = 0x52;
+		break;
+	case 56:
+		ecc_mode = 4;
+		ecc_off = 0x60;
+		break;
+	case 60:
+		ecc_mode = 4;
+		ecc_off = 0x0;
+		break;
+	case 64:
+		ecc_mode = 4;
+		ecc_off = 0x0;
+		break;
+	default:
+		ecc_mode = 0;
+		ecc_off = 0;
+	}
+
+	if (ecc_off == 0) {
+		printf("Unsupported ECC strength (%d)!\n",
+		       SUNXI_ECC_STRENGTH);
+		return;
+	}
+
+	/* clear temp_buf */
+	memset(temp_buf, 0, CONFIG_SYS_NAND_PAGE_SIZE);
+
+	/* set CMD  */
+	writel(NFC_SEND_CMD1 | NFC_WAIT_FLAG | 0xff,
+	       NANDFLASHC_BASE + NANDFLASHC_CMD);
+
+	if (!check_value(NANDFLASHC_BASE + NANDFLASHC_ST, (1 << 1),
+			 MAX_RETRIES)) {
+		printf("Error while initilizing command interrupt\n");
+		return;
+	}
+
+	page = real_addr / CONFIG_SYS_NAND_BLOCK_SIZE;
+	column = real_addr % CONFIG_SYS_NAND_BLOCK_SIZE;
+
+	if (syndrome) {
+		/* shift every 1kB in syndrome */
+		column += (column / CONFIG_SYS_NAND_PAGE_SIZE) * ecc_off;
+	}
+
+	/* clear ecc status */
+	writel(0, NANDFLASHC_BASE + NANDFLASHC_ECC_ST);
+
+	/* Choose correct seed */
+	if (syndrome)
+		rand_seed = random_seed_syndrome;
+	else
+		rand_seed = random_seed[page % 128];
+
+	writel((rand_seed << 16) | NFC_ECC_RANDOM_EN | NFC_ECC_EN
+		| NFC_ECC_PIPELINE | (ecc_mode << 12),
+		NANDFLASHC_BASE + NANDFLASHC_ECC_CTL);
+
+	val = readl(NANDFLASHC_BASE + NANDFLASHC_CTL);
+	writel(val | NFC_CTL_RAM_METHOD, NANDFLASHC_BASE + NANDFLASHC_CTL);
+
+	if (syndrome) {
+		writel(CONFIG_SYS_NAND_PAGE_SIZE,
+		       NANDFLASHC_BASE + NANDFLASHC_SPARE_AREA);
+	} else {
+		oob_offset = CONFIG_SYS_NAND_BLOCK_SIZE
+			+ (column / CONFIG_SYS_NAND_PAGE_SIZE) * ecc_off;
+		writel(oob_offset, NANDFLASHC_BASE + NANDFLASHC_SPARE_AREA);
+	}
+
+	/* DMAC */
+	writel(0x0, DMAC_BASE + DMAC_CFG_REG0); /* clr dma cmd */
+	/* read from REG_IO_DATA */
+	writel(NANDFLASHC_BASE + NANDFLASHC_IO_DATA,
+	       DMAC_BASE + DMAC_SRC_START_ADDR_REG0);
+	writel((uint32_t)temp_buf,
+	       DMAC_BASE + DMAC_DEST_START_ADDRR_REG0); /* read to RAM */
+	writel(DMAC_DDMA_PARA_REG_SRC_WAIT_CYC
+			| DMAC_DDMA_PARA_REG_SRC_BLK_SIZE,
+			DMAC_BASE + DMAC_DDMA_PARA_REG0);
+	writel(CONFIG_SYS_NAND_PAGE_SIZE,
+	       DMAC_BASE + DMAC_DDMA_BC_REG0); /* 1kB */
+	writel(DMAC_DDMA_CFG_REG_LOADING
+		| DMAC_DDMA_CFG_REG_DMA_DEST_DATA_WIDTH_32
+		| DMAC_DDMA_CFG_REG_DMA_SRC_DATA_WIDTH_32
+		| DMAC_DDMA_CFG_REG_DMA_SRC_ADDR_MODE_IO
+		| DMAC_DDMA_CFG_REG_DDMA_SRC_DRQ_TYPE_NFC,
+		DMAC_BASE + DMAC_CFG_REG0);
+
+	writel((0xE0 << NFC_RANDOM_READ_CMD1_OFFSET)
+		| (0x05 << NFC_RANDOM_READ_CMD0_OFFSET)
+		| (0x30 | NFC_READ_CMD_OFFSET), NANDFLASHC_BASE
+			+ NANDFLASHC_RCMD_SET);
+	writel(1, NANDFLASHC_BASE + NANDFLASHC_SECTOR_NUM);
+	writel(((page & 0xFFFF) << 16) | column,
+	       NANDFLASHC_BASE + NANDFLASHC_ADDR_LOW);
+	writel((page >> 16) & 0xFF, NANDFLASHC_BASE + NANDFLASHC_ADDR_HIGH);
+	writel(NFC_SEND_CMD1 | NFC_SEND_CMD2 | NFC_DATA_TRANS |
+		NFC_PAGE_CMD | NFC_WAIT_FLAG | (4 << NFC_ADDR_NUM_OFFSET) |
+		NFC_SEND_ADR | NFC_DATA_SWAP_METHOD | (syndrome ? NFC_SEQ : 0),
+		NANDFLASHC_BASE + NANDFLASHC_CMD);
+
+	if (!check_value(NANDFLASHC_BASE + NANDFLASHC_ST, (1 << 2),
+			 MAX_RETRIES)) {
+		printf("Error while initializing dma interrupt\n");
+		return;
+	}
+
+	if (!check_value_negated(DMAC_BASE + DMAC_CFG_REG0,
+				 DMAC_DDMA_CFG_REG_LOADING, MAX_RETRIES)) {
+		printf("Error while waiting for dma transfer to finish\n");
+		return;
+	}
+
+	if (readl(NANDFLASHC_BASE + NANDFLASHC_ECC_ST))
+		ecc_errors++;
+}
+
+int nand_spl_load_image(uint32_t offs, unsigned int size, void *dest)
+{
+	void *current_dest;
+	uint32_t count;
+	uint32_t current_count;
+
+	memset(dest, 0x0, size); /* clean destination memory */
+	ecc_errors = 0;
+	for (current_dest = dest;
+			current_dest < (dest + size);
+			current_dest += CONFIG_SYS_NAND_PAGE_SIZE) {
+		nand_read_page(offs, offs < SYNDROME_PARTITIONS_END);
+		count = current_dest - dest;
+
+		if (size - count > CONFIG_SYS_NAND_PAGE_SIZE)
+			current_count = CONFIG_SYS_NAND_PAGE_SIZE;
+		else
+			current_count = size - count;
+
+		memcpy(current_dest,
+		       temp_buf,
+		       current_count);
+		offs += CONFIG_SYS_NAND_PAGE_SIZE;
+	}
+	return ecc_errors;
+}
+
+void nand_deselect(void) {}
diff --git a/drivers/mtd/nand/sunxi_nand.h b/drivers/mtd/nand/sunxi_nand.h
new file mode 100644
index 0000000..d24f0ef
--- /dev/null
+++ b/drivers/mtd/nand/sunxi_nand.h
@@ -0,0 +1,151 @@
+#ifndef SUNXI_NAND_H
+
+#define SUNXI_NAND_H
+
+#define PORTC_BASE                 0x01c20800
+#define CCU_BASE                   0x01c20000
+#define NANDFLASHC_BASE            0x01c03000
+#define DMAC_BASE                  0x01c02000
+
+#define SYNDROME_PARTITIONS_END    0x00400000
+#define SUNXI_ECC_STRENGTH         40
+
+#define CCU_AHB_GATING_REG0        0x60
+#define CCU_NAND_SCLK_CFG_REG      0x80
+#define CCU_AHB_GATING_REG0_NAND   (1 << 13)
+
+#define CCU_NAND_SCLK_CFG_REG_SCLK_GATING (1 << 31)
+#define CCU_NAND_SCLK_CFG_REG_CLK_DIV_RATIO (1 << 0)
+
+#define PORTC_PC_CFG0              0x48
+#define PORTC_PC_CFG1              0x4C
+#define PORTC_PC_CFG2              0x50
+#define PORTC_PC_CFG3              0x54
+
+#define PORTC_PC_CFG0_NRB1         (2 << 28)
+#define PORTC_PC_CFG0_NRB0         (2 << 24)
+#define PORTC_PC_CFG0_NRE          (2 << 20)
+#define PORTC_PC_CFG0_NCE0         (2 << 16)
+#define PORTC_PC_CFG0_NCE1         (2 << 12)
+#define PORTC_PC_CFG0_NCLE         (2 << 8)
+#define PORTC_PC_CFG0_NALE         (2 << 4)
+#define PORTC_PC_CFG0_NWE          (2 << 0)
+
+#define PORTC_PC_CFG1_NDQ7         (2 << 28)
+#define PORTC_PC_CFG1_NDQ6         (2 << 24)
+#define PORTC_PC_CFG1_NDQ5         (2 << 20)
+#define PORTC_PC_CFG1_NDQ4         (2 << 16)
+#define PORTC_PC_CFG1_NDQ3         (2 << 12)
+#define PORTC_PC_CFG1_NDQ2         (2 << 8)
+#define PORTC_PC_CFG1_NDQ1         (2 << 4)
+#define PORTC_PC_CFG1_NDQ0         (2 << 0)
+
+#define PORTC_PC_CFG2_NCE7         (2 << 24)
+#define PORTC_PC_CFG2_NCE6         (2 << 20)
+#define PORTC_PC_CFG2_NCE5         (2 << 16)
+#define PORTC_PC_CFG2_NCE4         (2 << 12)
+#define PORTC_PC_CFG2_NCE3         (2 << 8)
+#define PORTC_PC_CFG2_NCE2         (2 << 4)
+#define PORTC_PC_CFG2_NWP          (2 << 0)
+
+#define PORTC_PC_CFG3_NDQS         (2 << 0)
+
+#define DMAC_CFG_REG0              0x300
+#define DMAC_SRC_START_ADDR_REG0   0x304
+#define DMAC_DEST_START_ADDRR_REG0 0x308
+#define DMAC_DDMA_BC_REG0          0x30C
+#define DMAC_DDMA_PARA_REG0        0x318
+
+#define DMAC_DDMA_CFG_REG_LOADING  (1 << 31)
+#define DMAC_DDMA_CFG_REG_DMA_DEST_DATA_WIDTH_32 (2 << 25)
+#define DMAC_DDMA_CFG_REG_DMA_SRC_DATA_WIDTH_32 (2 << 9)
+#define DMAC_DDMA_CFG_REG_DMA_SRC_ADDR_MODE_IO (1 << 5)
+#define DMAC_DDMA_CFG_REG_DDMA_SRC_DRQ_TYPE_NFC (3 << 0)
+
+#define DMAC_DDMA_PARA_REG_SRC_WAIT_CYC (0x0F << 0)
+#define DMAC_DDMA_PARA_REG_SRC_BLK_SIZE (0x7F << 8)
+
+#define NANDFLASHC_CTL             0x00000000
+
+#define NFC_CTL_EN                 (1 << 0)
+#define NFC_CTL_RESET              (1 << 1)
+#define NFC_CTL_RAM_METHOD         (1 << 14)
+
+#define NANDFLASHC_ST              0x00000004
+#define NANDFLASHC_INT             0x00000008
+#define NANDFLASHC_TIMING_CTL      0x0000000C
+#define NANDFLASHC_TIMING_CFG      0x00000010
+#define NANDFLASHC_ADDR_LOW        0x00000014
+#define NANDFLASHC_ADDR_HIGH       0x00000018
+#define NANDFLASHC_SECTOR_NUM      0x0000001C
+#define NANDFLASHC_CNT             0x00000020
+#define NANDFLASHC_CMD             0x00000024
+#define NANDFLASHC_RCMD_SET        0x00000028
+#define NANDFLASHC_WCMD_SET        0x0000002C
+#define NANDFLASHC_IO_DATA         0x00000030
+#define NANDFLASHC_ECC_CTL         0x00000034
+
+#define NFC_ECC_EN                 (1 << 0)
+#define NFC_ECC_PIPELINE           (1 << 3)
+#define NFC_ECC_EXCEPTION          (1 << 4)
+#define NFC_ECC_BLOCK_SIZE         (1 << 5)
+#define NFC_ECC_RANDOM_EN          (1 << 9)
+#define NFC_ECC_RANDOM_DIRECTION   (1 << 10)
+
+#define NANDFLASHC_ECC_ST          0x00000038
+#define NANDFLASHC_DEBUG           0x0000003C
+#define NANDFLASHC_ECC_CNT0        0x00000040
+#define NANDFLASHC_ECC_CNT1        0x00000044
+#define NANDFLASHC_ECC_CNT2        0x00000048
+#define NANDFLASHC_ECC_CNT3        0x0000004C
+#define NANDFLASHC_USER_DATA_BASE  0x00000050
+#define NANDFLASHC_EFNAND_STATUS   0x00000090
+#define NANDFLASHC_SPARE_AREA      0x000000A0
+#define NANDFLASHC_PATTERN_ID      0x000000A4
+#define NANDFLASHC_RAM0_BASE       0x00000400
+#define NANDFLASHC_RAM1_BASE       0x00000800
+
+#define NFC_ADDR_NUM_OFFSET        16
+#define NFC_SEND_ADR               (1 << 19)
+#define NFC_ACCESS_DIR             (1 << 20)
+#define NFC_DATA_TRANS             (1 << 21)
+#define NFC_SEND_CMD1              (1 << 22)
+#define NFC_WAIT_FLAG              (1 << 23)
+#define NFC_SEND_CMD2              (1 << 24)
+#define NFC_SEQ                    (1 << 25)
+#define NFC_DATA_SWAP_METHOD       (1 << 26)
+#define NFC_ROW_AUTO_INC           (1 << 27)
+#define NFC_SEND_CMD3              (1 << 28)
+#define NFC_SEND_CMD4              (1 << 29)
+
+#define NFC_READ_CMD_OFFSET         0
+#define NFC_RANDOM_READ_CMD0_OFFSET 8
+#define NFC_RANDOM_READ_CMD1_OFFSET 16
+
+
+#define NFC_PAGE_CMD               (2 << 30)
+
+/* random seed used by linux */
+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,
+};
+
+/* random seed used for syndrome calls */
+const uint16_t random_seed_syndrome = 0x4a80;
+
+#endif /* end of include guard: SUNXI_NAND_H */
-- 
2.3.6

[U-Boot] [PATCH v2 2/3] sunxi: nand: Add board configuration options

[Piotr Zierhoffer]

From: Piotr Zierhoffer <piotr.zierhoffer@cs.put.poznan.pl>

When SPL_NAND_SUNXI option is selected in config, set some configuration
options for sunxi NAND.

This commit also introduces the configurable options in Kconfig.

Signed-off-by: Peter Gielda <pgielda@antmicro.com>
Signed-off-by: Tomasz Gorochowik <tgorochowik@antmicro.com>
Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>
Signed-off-by: Piotr Zierhoffer <pzierhoffer@antmicro.com>
Signed-off-by: Karol Gugala <kgugala@antmicro.com>
---

Changes in v2:
- removed traces of non-SPL specific code
- renamed defines to be more relevant
- moved Kconfig entry for the driver to drivers/mtd/nand
- reworded Kconfig entry help

 drivers/mtd/nand/Kconfig       |  7 +++++++
 include/configs/sunxi-common.h | 11 +++++++++++
 2 files changed, 18 insertions(+)

diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index 3024357..2f8dbaf 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -85,6 +85,13 @@ config SPL_NAND_DENALI
 	  This is a small implementation of the Denali NAND controller
 	  for use on SPL.
 
+config SPL_NAND_SUNXI
+	bool "Support for NAND on Allwinner A20 in SPL"
+	depends on MACH_SUN7I
+	---help---
+	Enable support for internal NAND. This option allows SPL to read from
+	sunxi NAND using DMA transfers. Writing is not supported.
+
 endif
 
 endmenu
diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h
index 9576bc1..c230cff 100644
--- a/include/configs/sunxi-common.h
+++ b/include/configs/sunxi-common.h
@@ -139,6 +139,17 @@
 #define CONFIG_INITRD_TAG
 #define CONFIG_SERIAL_TAG
 
+#if defined(CONFIG_SPL_NAND_SUNXI)
+#define CONFIG_SPL_NAND_DRIVERS
+#define CONFIG_SPL_NAND_SUPPORT
+
+#define CONFIG_SYS_NAND_SPL_KERNEL_OFFS 0x280000
+#define CONFIG_SYS_NAND_U_BOOT_OFFS 0x008000
+
+#define CONFIG_SYS_NAND_PAGE_SIZE 0x000400 /* 1kb */
+#define CONFIG_SYS_NAND_BLOCK_SIZE 0x002000 /* 8kb*/
+#endif
+
 /* mmc config */
 #if !defined(CONFIG_UART0_PORT_F)
 #define CONFIG_MMC
-- 
2.3.6

[U-Boot] [PATCH v2 3/3] sunxi: nand: Add information to sunxi that it was run from NAND in SPL

[Piotr Zierhoffer]

As SPL does not know which source to choose when booting U-Boot, choose
NAND if it is capable of doing so.

Signed-off-by: Peter Gielda <pgielda@antmicro.com>
Signed-off-by: Tomasz Gorochowik <tgorochowik@antmicro.com>
Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>
Signed-off-by: Piotr Zierhoffer <pzierhoffer@antmicro.com>

---

Changes in v2:
- none

 arch/arm/cpu/armv7/sunxi/board.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/cpu/armv7/sunxi/board.c b/arch/arm/cpu/armv7/sunxi/board.c
index 5f39aa0..e4b7d63 100644
--- a/arch/arm/cpu/armv7/sunxi/board.c
+++ b/arch/arm/cpu/armv7/sunxi/board.c
@@ -128,6 +128,9 @@ void s_init(void)
  */
 u32 spl_boot_device(void)
 {
+#ifdef CONFIG_SPL_NAND_SUPPORT
+	return BOOT_DEVICE_NAND;
+#else
 	/*
 	 * When booting from the SD card, the "eGON.BT0" signature is expected
 	 * to be found in memory at the address 0x0004 (see the "mksunxiboot"
@@ -148,6 +151,7 @@ u32 spl_boot_device(void)
 		return BOOT_DEVICE_MMC1;
 	else
 		return BOOT_DEVICE_BOARD;
+#endif
 }
 
 /* No confirmation data available in SPL yet. Hardcode bootmode */
-- 
2.3.6

[U-Boot] [PATCH v2 0/3] sunxi: nand: Basic NAND driver for SPL

[Boris Brezillon]

Hi Piotr,

I don't know what's the policy in u-boot, but in most open source
projects we usually start a new thread when sending a new version of a
specific patch series.

On Mon, 20 Jul 2015 14:37:24 +0200
Piotr Zierhoffer <pzierhoffer@antmicro.com> wrote:

> 
> This is a basic driver for the sunxi NAND controller for Allwinner A20.
> It supports only SPL.
> 
> The driver uses DMA for data transfers. It does not support writing.
> 
> Changes in v2:
> - removed traces of non-SPL-specific code
> - moved the driver from boards/sunxi to drivers/mtd/nand
> - moved magic values to defines (whenever possible)
> - removed unnecesary late initialisation code
> - code style changes as suggested for the first patch set:
>   - changed visibility of some symbols
>   - renamed unclear variables
>   - renamed header protector
>   - changed types of pointer variables
>   - other minor changes
> - renamed defines to be more relevant
> - moved Kconfig entry for the driver to drivers/mtd/nand
> - reworded Kconfig entry help

You also completely dropped the a20_nandread command (which is a good
thing IMHO), right ?

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[U-Boot] [PATCH v2 0/3] sunxi: nand: Basic NAND driver for SPL

[Piotr Zierhoffer]

Hello Boris

2015-07-20 17:05 GMT+02:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> I don't know what's the policy in u-boot, but in most open source
> projects we usually start a new thread when sending a new version of a
> specific patch series.

I have tried to follow the guidelines from
http://www.denx.de/wiki/U-Boot/Patches and understood
that I should set -r option in patman to the message-id of my previous
cover letter - like in
the documentation of git-send-email (http://git-scm.com/docs/git-send-email)

Sorry if I got it wrong, should I resubmit it?


> You also completely dropped the a20_nandread command (which is a good
> thing IMHO), right ?

This is true. The command was the only non-SPL part, and since it was
not a full driver we have
decided to remove it from this patch set.

Best regards

Piotr Zierhoffer
Antmicro Ltd | www.antmicro.com

[U-Boot] [PATCH v2 1/3] sunxi: nand: Add basic sunxi NAND driver for SPL with DMA support

[Boris Brezillon]

Hi Piotr,

Here is a quick review.

On Mon, 20 Jul 2015 14:37:25 +0200
Piotr Zierhoffer <pzierhoffer@antmicro.com> wrote:

> From: Piotr Zierhoffer <piotr.zierhoffer@cs.put.poznan.pl>
> 
> This driver adds NAND support to SPL.
> It was tested on AllWinner A20.
> 
> Signed-off-by: Peter Gielda <pgielda@antmicro.com>
> Signed-off-by: Tomasz Gorochowik <tgorochowik@antmicro.com>
> Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>
> Signed-off-by: Piotr Zierhoffer <pzierhoffer@antmicro.com>
> Signed-off-by: Karol Gugala <kgugala@antmicro.com>
> ---
> 
> Changes in v2:
> - removed traces of non-SPL-specific code
> - moved the driver from boards/sunxi to drivers/mtd/nand
> - moved magic values to defines (whenever possible)
> - removed unnecesary late initialisation code
> - code style changes as suggested for the first patch set:
>   - changed visibility of some symbols
>   - renamed unclear variables
>   - renamed header protector
>   - changed types of pointer variables
>   - other minor changes
> 
>  drivers/mtd/nand/Makefile     |   1 +
>  drivers/mtd/nand/sunxi_nand.c | 289 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/mtd/nand/sunxi_nand.h | 151 ++++++++++++++++++++++
>  3 files changed, 441 insertions(+)
>  create mode 100644 drivers/mtd/nand/sunxi_nand.c
>  create mode 100644 drivers/mtd/nand/sunxi_nand.h
> 
> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
> index 347ea62..4cf9cee 100644
> --- a/drivers/mtd/nand/Makefile
> +++ b/drivers/mtd/nand/Makefile
> @@ -12,6 +12,7 @@ NORMAL_DRIVERS=y
>  endif
>  
>  obj-$(CONFIG_SPL_NAND_AM33XX_BCH) += am335x_spl_bch.o
> +obj-$(CONFIG_SPL_NAND_SUNXI) += sunxi_nand.o
>  obj-$(CONFIG_SPL_NAND_DENALI) += denali_spl.o
>  obj-$(CONFIG_SPL_NAND_DOCG4) += docg4_spl.o
>  obj-$(CONFIG_SPL_NAND_SIMPLE) += nand_spl_simple.o
> diff --git a/drivers/mtd/nand/sunxi_nand.c b/drivers/mtd/nand/sunxi_nand.c
> new file mode 100644
> index 0000000..8d66d2b
> --- /dev/null
> +++ b/drivers/mtd/nand/sunxi_nand.c
> @@ -0,0 +1,289 @@
> +/*
> + * Copyright (c) 2014-2015, Antmicro Ltd <www.antmicro.com>
> + * Copyright (c) 2015, AW-SOM Technologies <www.aw-som.com>
> + *
> + * SPDX-License-Identifier:     GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <config.h>
> +#include <asm/io.h>
> +#include <nand.h>
> +#include "sunxi_nand.h"
> +
> +/* minimal "boot0" style NAND support for Allwinner A20 */
> +
> +/* temporary buffer in internal ram */
> +unsigned char temp_buf[CONFIG_SYS_NAND_PAGE_SIZE]
> +	__aligned(0x10) __section(".text#");
> +
> +#define MAX_RETRIES 10
> +
> +static int check_value_inner(int offset, int expected_bits,
> +				int max_number_of_retries, int negation)
> +{
> +	int retries = 0;
> +	do {
> +		int val = readl(offset) & expected_bits;
> +		if (negation ? !val : val)
> +			return 1;
> +		mdelay(1);
> +		retries++;
> +	} while (retries < max_number_of_retries);
> +
> +	return 0;
> +}
> +
> +static inline int check_value(int offset, int expected_bits,
> +				int max_number_of_retries)
> +{
> +	return check_value_inner(offset, expected_bits,
> +					max_number_of_retries, 0);
> +}
> +
> +static inline int check_value_negated(int offset, int unexpected_bits,
> +					int max_number_of_retries)
> +{
> +	return check_value_inner(offset, unexpected_bits,
> +					max_number_of_retries, 1);
> +}
> +
> +static void nand_set_clocks(void)

You're setting more than the clks in there: you're also doing the
pinmux.
Could you either rename the function or create a new one for the pinmux
setting.

BTW, I'm not sure, but I think the pinmux should be done at the board
level.

> +{
> +	uint32_t val;
> +
> +	writel(PORTC_PC_CFG0_NRB1 |
> +		PORTC_PC_CFG0_NRB0 |
> +		PORTC_PC_CFG0_NRE  |
> +		PORTC_PC_CFG0_NCE0 |
> +		PORTC_PC_CFG0_NCE1 |
> +		PORTC_PC_CFG0_NCLE |
> +		PORTC_PC_CFG0_NALE |
> +		PORTC_PC_CFG0_NWE, PORTC_BASE + PORTC_PC_CFG0);

At least the NCEX and NRBX should be board specific ?

> +
> +	writel(PORTC_PC_CFG1_NDQ7 |
> +		PORTC_PC_CFG1_NDQ6 |
> +		PORTC_PC_CFG1_NDQ5 |
> +		PORTC_PC_CFG1_NDQ4 |
> +		PORTC_PC_CFG1_NDQ3 |
> +		PORTC_PC_CFG1_NDQ2 |
> +		PORTC_PC_CFG1_NDQ1 |
> +		PORTC_PC_CFG1_NDQ0, PORTC_BASE + PORTC_PC_CFG1);
> +
> +	writel(PORTC_PC_CFG2_NCE7 |
> +		PORTC_PC_CFG2_NCE6 |
> +		PORTC_PC_CFG2_NCE5 |
> +		PORTC_PC_CFG2_NCE4 |
> +		PORTC_PC_CFG2_NCE3 |
> +		PORTC_PC_CFG2_NCE2 |
> +		PORTC_PC_CFG2_NWP, PORTC_BASE + PORTC_PC_CFG2);

Ditto

> +
> +	writel(PORTC_PC_CFG3_NDQS, PORTC_BASE + PORTC_PC_CFG3);
> +
> +	val = readl(CCU_BASE + CCU_AHB_GATING_REG0);
> +	writel(CCU_AHB_GATING_REG0_NAND | val, CCU_BASE + CCU_AHB_GATING_REG0);
> +
> +	val = readl(CCU_BASE + CCU_NAND_SCLK_CFG_REG);
> +	writel(val | CCU_NAND_SCLK_CFG_REG_SCLK_GATING
> +		| CCU_NAND_SCLK_CFG_REG_CLK_DIV_RATIO,
> +		CCU_BASE + CCU_NAND_SCLK_CFG_REG);
> +}
> +
> +void nand_init(void)
> +{
> +	uint32_t val;
> +
> +	nand_set_clocks();
> +	val = readl(NANDFLASHC_BASE + NANDFLASHC_CTL);
> +	/* enable and reset CTL */
> +	writel(val | NFC_CTL_EN | NFC_CTL_RESET,
> +	       NANDFLASHC_BASE + NANDFLASHC_CTL);
> +
> +	if (!check_value_negated(NANDFLASHC_BASE + NANDFLASHC_CTL,
> +				 NFC_CTL_RESET, MAX_RETRIES)) {
> +		printf("Couldn't initialize nand\n");
> +	}
> +}
> +
> +static uint32_t ecc_errors;

You could get rid of this global variable if you pass an ecc_errors
argument to the nand_read_page function.

> +
> +void nand_read_page(unsigned int real_addr, int syndrome)
> +{
> +	uint32_t val;
> +	int ecc_off = 0;
> +	uint16_t ecc_mode = 0;
> +	uint16_t rand_seed;
> +	uint32_t page;
> +	uint16_t column;
> +	uint32_t oob_offset;
> +
> +	switch (SUNXI_ECC_STRENGTH) {
> +	case 16:
> +		ecc_mode = 0;
> +		ecc_off = 0x20;
> +		break;
> +	case 24:
> +		ecc_mode = 1;
> +		ecc_off = 0x2e;
> +		break;
> +	case 28:
> +		ecc_mode = 2;
> +		ecc_off = 0x32;
> +		break;
> +	case 32:
> +		ecc_mode = 3;
> +		ecc_off = 0x3c;
> +		break;
> +	case 40:
> +		ecc_mode = 4;
> +		ecc_off = 0x4a;
> +		break;
> +	case 48:
> +		ecc_mode = 4;
> +		ecc_off = 0x52;
> +		break;
> +	case 56:
> +		ecc_mode = 4;
> +		ecc_off = 0x60;
> +		break;
> +	case 60:
> +		ecc_mode = 4;
> +		ecc_off = 0x0;
> +		break;
> +	case 64:
> +		ecc_mode = 4;
> +		ecc_off = 0x0;
> +		break;
> +	default:
> +		ecc_mode = 0;
> +		ecc_off = 0;
> +	}
> +
> +	if (ecc_off == 0) {
> +		printf("Unsupported ECC strength (%d)!\n",
> +		       SUNXI_ECC_STRENGTH);
> +		return;
> +	}
> +
> +	/* clear temp_buf */
> +	memset(temp_buf, 0, CONFIG_SYS_NAND_PAGE_SIZE);
> +
> +	/* set CMD  */
> +	writel(NFC_SEND_CMD1 | NFC_WAIT_FLAG | 0xff,
> +	       NANDFLASHC_BASE + NANDFLASHC_CMD);

Could you replace the 0xff value by NAND_CMD_RESET, because what you're
really trying to do is reset the NAND chip.

> +
> +	if (!check_value(NANDFLASHC_BASE + NANDFLASHC_ST, (1 << 1),

Please replace this magic value by the appropriate MACRO [1].

> +			 MAX_RETRIES)) {
> +		printf("Error while initilizing command interrupt\n");
> +		return;
> +	}
> +
> +	page = real_addr / CONFIG_SYS_NAND_BLOCK_SIZE;
> +	column = real_addr % CONFIG_SYS_NAND_BLOCK_SIZE;
> +
> +	if (syndrome) {
> +		/* shift every 1kB in syndrome */

Well, this scheme is not really related to the ECC syndrome scheme,
it comes from the BROM implementation which can only really 1K of data
per page.
Actually, this is not exactly true, depending on the BROM version it
will try different things, see this description [2].
So once more, you're making assumptions that could be wrong on some
boards.

> +		column += (column / CONFIG_SYS_NAND_PAGE_SIZE) * ecc_off;
> +	}
> +
> +	/* clear ecc status */
> +	writel(0, NANDFLASHC_BASE + NANDFLASHC_ECC_ST);
> +
> +	/* Choose correct seed */
> +	if (syndrome)
> +		rand_seed = random_seed_syndrome;
> +	else
> +		rand_seed = random_seed[page % 128];
> +
> +	writel((rand_seed << 16) | NFC_ECC_RANDOM_EN | NFC_ECC_EN
> +		| NFC_ECC_PIPELINE | (ecc_mode << 12),
> +		NANDFLASHC_BASE + NANDFLASHC_ECC_CTL);
> +
> +	val = readl(NANDFLASHC_BASE + NANDFLASHC_CTL);
> +	writel(val | NFC_CTL_RAM_METHOD, NANDFLASHC_BASE + NANDFLASHC_CTL);
> +
> +	if (syndrome) {
> +		writel(CONFIG_SYS_NAND_PAGE_SIZE,
> +		       NANDFLASHC_BASE + NANDFLASHC_SPARE_AREA);
> +	} else {
> +		oob_offset = CONFIG_SYS_NAND_BLOCK_SIZE
> +			+ (column / CONFIG_SYS_NAND_PAGE_SIZE) * ecc_off;
> +		writel(oob_offset, NANDFLASHC_BASE + NANDFLASHC_SPARE_AREA);
> +	}
> +
> +	/* DMAC */
> +	writel(0x0, DMAC_BASE + DMAC_CFG_REG0); /* clr dma cmd */
> +	/* read from REG_IO_DATA */
> +	writel(NANDFLASHC_BASE + NANDFLASHC_IO_DATA,
> +	       DMAC_BASE + DMAC_SRC_START_ADDR_REG0);
> +	writel((uint32_t)temp_buf,
> +	       DMAC_BASE + DMAC_DEST_START_ADDRR_REG0); /* read to RAM */
> +	writel(DMAC_DDMA_PARA_REG_SRC_WAIT_CYC
> +			| DMAC_DDMA_PARA_REG_SRC_BLK_SIZE,
> +			DMAC_BASE + DMAC_DDMA_PARA_REG0);
> +	writel(CONFIG_SYS_NAND_PAGE_SIZE,
> +	       DMAC_BASE + DMAC_DDMA_BC_REG0); /* 1kB */
> +	writel(DMAC_DDMA_CFG_REG_LOADING
> +		| DMAC_DDMA_CFG_REG_DMA_DEST_DATA_WIDTH_32
> +		| DMAC_DDMA_CFG_REG_DMA_SRC_DATA_WIDTH_32
> +		| DMAC_DDMA_CFG_REG_DMA_SRC_ADDR_MODE_IO
> +		| DMAC_DDMA_CFG_REG_DDMA_SRC_DRQ_TYPE_NFC,
> +		DMAC_BASE + DMAC_CFG_REG0);
> +
> +	writel((0xE0 << NFC_RANDOM_READ_CMD1_OFFSET)
> +		| (0x05 << NFC_RANDOM_READ_CMD0_OFFSET)
> +		| (0x30 | NFC_READ_CMD_OFFSET), NANDFLASHC_BASE
> +			+ NANDFLASHC_RCMD_SET);

Same comment as above, please use the appropriate macros:

0xE0 => NAND_CMD_RNDOUTSTART
0x05 => NAND_CMD_RNDOUT
0x30 => NAND_CMD_READSTART

> +	writel(1, NANDFLASHC_BASE + NANDFLASHC_SECTOR_NUM);
> +	writel(((page & 0xFFFF) << 16) | column,
> +	       NANDFLASHC_BASE + NANDFLASHC_ADDR_LOW);
> +	writel((page >> 16) & 0xFF, NANDFLASHC_BASE + NANDFLASHC_ADDR_HIGH);
> +	writel(NFC_SEND_CMD1 | NFC_SEND_CMD2 | NFC_DATA_TRANS |
> +		NFC_PAGE_CMD | NFC_WAIT_FLAG | (4 << NFC_ADDR_NUM_OFFSET) |
> +		NFC_SEND_ADR | NFC_DATA_SWAP_METHOD | (syndrome ? NFC_SEQ : 0),
> +		NANDFLASHC_BASE + NANDFLASHC_CMD);
> +
> +	if (!check_value(NANDFLASHC_BASE + NANDFLASHC_ST, (1 << 2),
> +			 MAX_RETRIES)) {
> +		printf("Error while initializing dma interrupt\n");
> +		return;
> +	}
> +
> +	if (!check_value_negated(DMAC_BASE + DMAC_CFG_REG0,
> +				 DMAC_DDMA_CFG_REG_LOADING, MAX_RETRIES)) {
> +		printf("Error while waiting for dma transfer to finish\n");
> +		return;
> +	}
> +
> +	if (readl(NANDFLASHC_BASE + NANDFLASHC_ECC_ST))
> +		ecc_errors++;
> +}
> +
> +int nand_spl_load_image(uint32_t offs, unsigned int size, void *dest)
> +{
> +	void *current_dest;
> +	uint32_t count;
> +	uint32_t current_count;
> +
> +	memset(dest, 0x0, size); /* clean destination memory */
> +	ecc_errors = 0;
> +	for (current_dest = dest;
> +			current_dest < (dest + size);
> +			current_dest += CONFIG_SYS_NAND_PAGE_SIZE) {
> +		nand_read_page(offs, offs < SYNDROME_PARTITIONS_END);
> +		count = current_dest - dest;
> +
> +		if (size - count > CONFIG_SYS_NAND_PAGE_SIZE)
> +			current_count = CONFIG_SYS_NAND_PAGE_SIZE;
> +		else
> +			current_count = size - count;
> +
> +		memcpy(current_dest,
> +		       temp_buf,
> +		       current_count);
> +		offs += CONFIG_SYS_NAND_PAGE_SIZE;
> +	}
> +	return ecc_errors;

I'm not sure what's the exact convention for nand_spl_load_image return
code, but I'd say that returning a negative error code if ecc_errors !=
0 is better than returning the number of ECC errors.

> +}
> +
> +void nand_deselect(void) {}
> diff --git a/drivers/mtd/nand/sunxi_nand.h b/drivers/mtd/nand/sunxi_nand.h
> new file mode 100644
> index 0000000..d24f0ef
> --- /dev/null
> +++ b/drivers/mtd/nand/sunxi_nand.h
> @@ -0,0 +1,151 @@
> +#ifndef SUNXI_NAND_H
> +
> +#define SUNXI_NAND_H
> +
> +#define PORTC_BASE                 0x01c20800
> +#define CCU_BASE                   0x01c20000
> +#define NANDFLASHC_BASE            0x01c03000
> +#define DMAC_BASE                  0x01c02000

I'm not sure how drivers are supposed to interact with a DMA
controller, a clk controller or a gpio/pinmux controller in u-boot, but
I would expect those definitions to be common to the sunxi platform and
not directly defined in the NAND controller driver.

The same goes for all the following CCU_, PORTC_ and DMAC_ definitions.

> +
> +#define SYNDROME_PARTITIONS_END    0x00400000
> +#define SUNXI_ECC_STRENGTH         40

These two variables should definitely not be defined in this headers:
they are representing NAND chip requirements and these ones are
definitely board specific (they depends on the NAND chip you have on
your board).

> +
> +#define CCU_AHB_GATING_REG0        0x60
> +#define CCU_NAND_SCLK_CFG_REG      0x80
> +#define CCU_AHB_GATING_REG0_NAND   (1 << 13)
> +
> +#define CCU_NAND_SCLK_CFG_REG_SCLK_GATING (1 << 31)
> +#define CCU_NAND_SCLK_CFG_REG_CLK_DIV_RATIO (1 << 0)
> +
> +#define PORTC_PC_CFG0              0x48
> +#define PORTC_PC_CFG1              0x4C
> +#define PORTC_PC_CFG2              0x50
> +#define PORTC_PC_CFG3              0x54
> +
> +#define PORTC_PC_CFG0_NRB1         (2 << 28)
> +#define PORTC_PC_CFG0_NRB0         (2 << 24)
> +#define PORTC_PC_CFG0_NRE          (2 << 20)
> +#define PORTC_PC_CFG0_NCE0         (2 << 16)
> +#define PORTC_PC_CFG0_NCE1         (2 << 12)
> +#define PORTC_PC_CFG0_NCLE         (2 << 8)
> +#define PORTC_PC_CFG0_NALE         (2 << 4)
> +#define PORTC_PC_CFG0_NWE          (2 << 0)
> +
> +#define PORTC_PC_CFG1_NDQ7         (2 << 28)
> +#define PORTC_PC_CFG1_NDQ6         (2 << 24)
> +#define PORTC_PC_CFG1_NDQ5         (2 << 20)
> +#define PORTC_PC_CFG1_NDQ4         (2 << 16)
> +#define PORTC_PC_CFG1_NDQ3         (2 << 12)
> +#define PORTC_PC_CFG1_NDQ2         (2 << 8)
> +#define PORTC_PC_CFG1_NDQ1         (2 << 4)
> +#define PORTC_PC_CFG1_NDQ0         (2 << 0)
> +
> +#define PORTC_PC_CFG2_NCE7         (2 << 24)
> +#define PORTC_PC_CFG2_NCE6         (2 << 20)
> +#define PORTC_PC_CFG2_NCE5         (2 << 16)
> +#define PORTC_PC_CFG2_NCE4         (2 << 12)
> +#define PORTC_PC_CFG2_NCE3         (2 << 8)
> +#define PORTC_PC_CFG2_NCE2         (2 << 4)
> +#define PORTC_PC_CFG2_NWP          (2 << 0)
> +
> +#define PORTC_PC_CFG3_NDQS         (2 << 0)
> +
> +#define DMAC_CFG_REG0              0x300
> +#define DMAC_SRC_START_ADDR_REG0   0x304
> +#define DMAC_DEST_START_ADDRR_REG0 0x308
> +#define DMAC_DDMA_BC_REG0          0x30C
> +#define DMAC_DDMA_PARA_REG0        0x318
> +
> +#define DMAC_DDMA_CFG_REG_LOADING  (1 << 31)
> +#define DMAC_DDMA_CFG_REG_DMA_DEST_DATA_WIDTH_32 (2 << 25)
> +#define DMAC_DDMA_CFG_REG_DMA_SRC_DATA_WIDTH_32 (2 << 9)
> +#define DMAC_DDMA_CFG_REG_DMA_SRC_ADDR_MODE_IO (1 << 5)
> +#define DMAC_DDMA_CFG_REG_DDMA_SRC_DRQ_TYPE_NFC (3 << 0)
> +
> +#define DMAC_DDMA_PARA_REG_SRC_WAIT_CYC (0x0F << 0)
> +#define DMAC_DDMA_PARA_REG_SRC_BLK_SIZE (0x7F << 8)
> +
> +#define NANDFLASHC_CTL             0x00000000
> +
> +#define NFC_CTL_EN                 (1 << 0)
> +#define NFC_CTL_RESET              (1 << 1)
> +#define NFC_CTL_RAM_METHOD         (1 << 14)
> +
> +#define NANDFLASHC_ST              0x00000004
> +#define NANDFLASHC_INT             0x00000008
> +#define NANDFLASHC_TIMING_CTL      0x0000000C
> +#define NANDFLASHC_TIMING_CFG      0x00000010
> +#define NANDFLASHC_ADDR_LOW        0x00000014
> +#define NANDFLASHC_ADDR_HIGH       0x00000018
> +#define NANDFLASHC_SECTOR_NUM      0x0000001C
> +#define NANDFLASHC_CNT             0x00000020
> +#define NANDFLASHC_CMD             0x00000024
> +#define NANDFLASHC_RCMD_SET        0x00000028
> +#define NANDFLASHC_WCMD_SET        0x0000002C
> +#define NANDFLASHC_IO_DATA         0x00000030
> +#define NANDFLASHC_ECC_CTL         0x00000034
> +
> +#define NFC_ECC_EN                 (1 << 0)
> +#define NFC_ECC_PIPELINE           (1 << 3)
> +#define NFC_ECC_EXCEPTION          (1 << 4)
> +#define NFC_ECC_BLOCK_SIZE         (1 << 5)
> +#define NFC_ECC_RANDOM_EN          (1 << 9)
> +#define NFC_ECC_RANDOM_DIRECTION   (1 << 10)
> +
> +#define NANDFLASHC_ECC_ST          0x00000038
> +#define NANDFLASHC_DEBUG           0x0000003C
> +#define NANDFLASHC_ECC_CNT0        0x00000040
> +#define NANDFLASHC_ECC_CNT1        0x00000044
> +#define NANDFLASHC_ECC_CNT2        0x00000048
> +#define NANDFLASHC_ECC_CNT3        0x0000004C
> +#define NANDFLASHC_USER_DATA_BASE  0x00000050
> +#define NANDFLASHC_EFNAND_STATUS   0x00000090
> +#define NANDFLASHC_SPARE_AREA      0x000000A0
> +#define NANDFLASHC_PATTERN_ID      0x000000A4
> +#define NANDFLASHC_RAM0_BASE       0x00000400
> +#define NANDFLASHC_RAM1_BASE       0x00000800
> +
> +#define NFC_ADDR_NUM_OFFSET        16
> +#define NFC_SEND_ADR               (1 << 19)
> +#define NFC_ACCESS_DIR             (1 << 20)
> +#define NFC_DATA_TRANS             (1 << 21)
> +#define NFC_SEND_CMD1              (1 << 22)
> +#define NFC_WAIT_FLAG              (1 << 23)
> +#define NFC_SEND_CMD2              (1 << 24)
> +#define NFC_SEQ                    (1 << 25)
> +#define NFC_DATA_SWAP_METHOD       (1 << 26)
> +#define NFC_ROW_AUTO_INC           (1 << 27)
> +#define NFC_SEND_CMD3              (1 << 28)
> +#define NFC_SEND_CMD4              (1 << 29)
> +
> +#define NFC_READ_CMD_OFFSET         0
> +#define NFC_RANDOM_READ_CMD0_OFFSET 8
> +#define NFC_RANDOM_READ_CMD1_OFFSET 16
> +
> +
> +#define NFC_PAGE_CMD               (2 << 30)

Are all these NFC_ macros used outside of the driver itself.
If that's not the case then I would recommend moving them direcly in
the .c file.

Moreover, you're sometime mixing the NFC_ and NANDFLASHC_ prefix, is
there a reason for doing that ?

Also, I haven't checked if the MACRO names are matching the one defined
in the linux driver, but if that's not the case then I would recommend
using the same definition since the final goal is to port the Linux
driver to u-boot (I know you're just implementing the SPL part, but
since you moved your code into drivers/mtd/nand/sunxi_nand* I'll have
to merge the Linux implementation into this file).

> +
> +/* random seed used by linux */
> +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,
> +};
> +
> +/* random seed used for syndrome calls */
> +const uint16_t random_seed_syndrome = 0x4a80;

The random_seed and random_seed_syndrome variables should not be
defined in the header, because this implies duplicating the same global
variable if the header is included multiple times (which should trigger
a linker error).

> +
> +#endif /* end of include guard: SUNXI_NAND_H */

/* SUNXI_NAND_H */ should be enough.

Best Regards,

Boris

[1]http://lxr.free-electrons.com/source/drivers/mtd/nand/sunxi_nand.c#L84
[2]http://linux-sunxi.org/NAND#More_information_on_BROM_NAND

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[U-Boot] [PATCH v2 1/3] sunxi: nand: Add basic sunxi NAND driver for SPL with DMA support

[Piotr Zierhoffer]

Hi Boris,

thanks for your review. I have applied most of your comments, but I
have few remarks and questions.

2015-07-20 18:13 GMT+02:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
>> +     page = real_addr / CONFIG_SYS_NAND_BLOCK_SIZE;
>> +     column = real_addr % CONFIG_SYS_NAND_BLOCK_SIZE;
>> +
>> +     if (syndrome) {
>> +             /* shift every 1kB in syndrome */
>
> Well, this scheme is not really related to the ECC syndrome scheme,
> it comes from the BROM implementation which can only really 1K of data
> per page.
> Actually, this is not exactly true, depending on the BROM version it
> will try different things, see this description [2].
> So once more, you're making assumptions that could be wrong on some
> boards.

Actually, I have only one board and I wouldn't like to submit code
that is supposed to be general and was never really tested.

I'd suggest removing the comment and making the following options
configurable, with the default values as provided, in Kconfig:
CONFIG_NAND_PAGE_SIZE 0x2000
CONFIG_NAND_ECC_PAGE_SIZE 0x400
CONFIG_NAND_SUNXI_ECC_STRENGTH 40
CONFIG_NAND_SYNDROME_PARTITIONS_END 0x400000

Do you think that would be sensible?

>> +int nand_spl_load_image(uint32_t offs, unsigned int size, void *dest)
>> +{
>> +     void *current_dest;
>> +     uint32_t count;
>> +     uint32_t current_count;
>> +
>> +     memset(dest, 0x0, size); /* clean destination memory */
>> +     ecc_errors = 0;
>> +     for (current_dest = dest;
>> +                     current_dest < (dest + size);
>> +                     current_dest += CONFIG_SYS_NAND_PAGE_SIZE) {
>> +             nand_read_page(offs, offs < SYNDROME_PARTITIONS_END);
>> +             count = current_dest - dest;
>> +
>> +             if (size - count > CONFIG_SYS_NAND_PAGE_SIZE)
>> +                     current_count = CONFIG_SYS_NAND_PAGE_SIZE;
>> +             else
>> +                     current_count = size - count;
>> +
>> +             memcpy(current_dest,
>> +                    temp_buf,
>> +                    current_count);
>> +             offs += CONFIG_SYS_NAND_PAGE_SIZE;
>> +     }
>> +     return ecc_errors;
>
> I'm not sure what's the exact convention for nand_spl_load_image return
> code, but I'd say that returning a negative error code if ecc_errors !=
> 0 is better than returning the number of ECC errors.
>

From my observations it seems that there is no established convention, but
returning -1 as an error indicator can be found here and there, so I
may do that.

It's not really interpreted anywhere, though.

>
> Are all these NFC_ macros used outside of the driver itself.
> If that's not the case then I would recommend moving them direcly in
> the .c file.
>

In general you may find that constants regarding NANDs are kept in .h files
around U-Boot, so I'd like stick with that convention.

> Also, I haven't checked if the MACRO names are matching the one defined
> in the linux driver, but if that's not the case then I would recommend
> using the same definition since the final goal is to port the Linux
> driver to u-boot (I know you're just implementing the SPL part, but
> since you moved your code into drivers/mtd/nand/sunxi_nand* I'll have
> to merge the Linux implementation into this file).
>

I have made the macros consistent, but unfortunately I won't be able to verify
them all with the Linux drivers, due to time constraints.
It can always be done later as a part of a separate patchset with a
Linux driver.

I will submit another version of this patchset later today.

I will follow your suggestion to create a new thread for that.

> Boris
>

Best regards


*Piotr Zierhoffer*
Antmicro Ltd | www.antmicro.com

[U-Boot] [PATCH v2 1/3] sunxi: nand: Add basic sunxi NAND driver for SPL with DMA support

[Boris Brezillon]

Hi Piotr,

On Wed, 22 Jul 2015 13:27:37 +0200
Piotr Zierhoffer <pzierhoffer@antmicro.com> wrote:

> Hi Boris,
> 
> thanks for your review. I have applied most of your comments, but I
> have few remarks and questions.
> 
> 2015-07-20 18:13 GMT+02:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> >> +     page = real_addr / CONFIG_SYS_NAND_BLOCK_SIZE;
> >> +     column = real_addr % CONFIG_SYS_NAND_BLOCK_SIZE;
> >> +
> >> +     if (syndrome) {
> >> +             /* shift every 1kB in syndrome */
> >
> > Well, this scheme is not really related to the ECC syndrome scheme,
> > it comes from the BROM implementation which can only really 1K of data
> > per page.
> > Actually, this is not exactly true, depending on the BROM version it
> > will try different things, see this description [2].
> > So once more, you're making assumptions that could be wrong on some
> > boards.
> 
> Actually, I have only one board and I wouldn't like to submit code
> that is supposed to be general and was never really tested.

I understand, but I was not talking about testing your code on all
existing boards, just making your code generic enough so that other
users can test on their platforms without having to modify the SPL code.
Of course if they detect that something is buggy or missing, they will
provide follow-up patches to fix those problems.

> 
> I'd suggest removing the comment and making the following options
> configurable, with the default values as provided, in Kconfig:
> CONFIG_NAND_PAGE_SIZE 0x2000
> CONFIG_NAND_ECC_PAGE_SIZE 0x400
> CONFIG_NAND_SUNXI_ECC_STRENGTH 40
> CONFIG_NAND_SYNDROME_PARTITIONS_END 0x400000
> 
> Do you think that would be sensible?

Yep, that pretty much what I was expecting. Just put those definition
into a board config header, or add a Kconfig entry.

> 
> >> +int nand_spl_load_image(uint32_t offs, unsigned int size, void *dest)
> >> +{
> >> +     void *current_dest;
> >> +     uint32_t count;
> >> +     uint32_t current_count;
> >> +
> >> +     memset(dest, 0x0, size); /* clean destination memory */
> >> +     ecc_errors = 0;
> >> +     for (current_dest = dest;
> >> +                     current_dest < (dest + size);
> >> +                     current_dest += CONFIG_SYS_NAND_PAGE_SIZE) {
> >> +             nand_read_page(offs, offs < SYNDROME_PARTITIONS_END);
> >> +             count = current_dest - dest;
> >> +
> >> +             if (size - count > CONFIG_SYS_NAND_PAGE_SIZE)
> >> +                     current_count = CONFIG_SYS_NAND_PAGE_SIZE;
> >> +             else
> >> +                     current_count = size - count;
> >> +
> >> +             memcpy(current_dest,
> >> +                    temp_buf,
> >> +                    current_count);
> >> +             offs += CONFIG_SYS_NAND_PAGE_SIZE;
> >> +     }
> >> +     return ecc_errors;
> >
> > I'm not sure what's the exact convention for nand_spl_load_image return
> > code, but I'd say that returning a negative error code if ecc_errors !=
> > 0 is better than returning the number of ECC errors.
> >
> 
> From my observations it seems that there is no established convention, but
> returning -1 as an error indicator can be found here and there, so I
> may do that.
> 
> It's not really interpreted anywhere, though.

Okay, then I'll let u-boot maintainers decide what should be done.

> 
> >
> > Are all these NFC_ macros used outside of the driver itself.
> > If that's not the case then I would recommend moving them direcly in
> > the .c file.
> >
> 
> In general you may find that constants regarding NANDs are kept in .h files
> around U-Boot, so I'd like stick with that convention.

This is true for all generic definitions, not for driver specific ones.

> 
> > Also, I haven't checked if the MACRO names are matching the one defined
> > in the linux driver, but if that's not the case then I would recommend
> > using the same definition since the final goal is to port the Linux
> > driver to u-boot (I know you're just implementing the SPL part, but
> > since you moved your code into drivers/mtd/nand/sunxi_nand* I'll have
> > to merge the Linux implementation into this file).
> >
> 
> I have made the macros consistent, but unfortunately I won't be able to verify
> them all with the Linux drivers, due to time constraints.

How about copying the definition from the Linux driver and fixing the
mismatch in your implementation ?

> It can always be done later as a part of a separate patchset with a
> Linux driver.

Hm, the merge will be even more complicated if you do that.
Could you at least move your code to sunxi_nand_spl.c so that we can
directly apply the linux commit adding support for the sunxi NAND
controller ?

Thanks.

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[U-Boot] [PATCH v2 1/3] sunxi: nand: Add basic sunxi NAND driver for SPL with DMA support

[Piotr Zierhoffer]

Hello

2015-07-22 13:53 GMT+02:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
>>
>> I'd suggest removing the comment and making the following options
>> configurable, with the default values as provided, in Kconfig:
>> CONFIG_NAND_PAGE_SIZE 0x2000
>> CONFIG_NAND_ECC_PAGE_SIZE 0x400
>> CONFIG_NAND_SUNXI_ECC_STRENGTH 40
>> CONFIG_NAND_SYNDROME_PARTITIONS_END 0x400000
>>
>> Do you think that would be sensible?
>
> Yep, that pretty much what I was expecting. Just put those definition
> into a board config header, or add a Kconfig entry.
>

Ok, will be done.

>>
>> In general you may find that constants regarding NANDs are kept in .h files
>> around U-Boot, so I'd like stick with that convention.
>
> This is true for all generic definitions, not for driver specific ones.
>

Well, that's not true for all the drivers, as we have tegra_nand.h and
atmel_nand_ecc.h, never used elsewhere apart from their corresponding
*.c files.

Especially if we'd have two drivers, one header will be needed.

But if that would ease your work, I'll merge sunxi_nand.h to
sunxi_nand.c and rename it as you suggest.


Best regards

Piotr Zierhoffer
Antmicro Ltd | www.antmicro.com