[U-Boot] [PATCH 0/3] arm: atmel: sama5d3: add spi and nand spl boot support

[U-Boot] [PATCH 0/3] arm: atmel: sama5d3: add spi and nand spl boot support

[Bo Shen]

This patch enable spi and nand spl boot support on sama5d3xek

Bo Shen (3):
  arm: atmel: sama5d3: add spi spl boot support
  mtd: nand: atmel: prepare for nand spl boot support
  arm: atmel: sama5d3: add nand spl boot support

 arch/arm/cpu/at91-common/spl.c       |    4 +
 arch/arm/include/asm/arch-at91/spl.h |    4 +
 board/atmel/sama5d3xek/sama5d3xek.c  |    4 +
 drivers/mtd/nand/atmel_nand.c        |  206 ++++++++++++++++++++++++++++++++++
 include/configs/sama5d3xek.h         |   23 ++++
 include/nand.h                       |    6 +
 6 files changed, 247 insertions(+)

-- 
1.7.9.5

[U-Boot] [PATCH 1/3] arm: atmel: sama5d3: add spi spl boot support

[Bo Shen]

Add SPI SPL boot support for sama5d3xek board.

Signed-off-by: Bo Shen <voice.shen@atmel.com>
---
 arch/arm/cpu/at91-common/spl.c       |    2 ++
 arch/arm/include/asm/arch-at91/spl.h |    2 ++
 board/atmel/sama5d3xek/sama5d3xek.c  |    2 ++
 include/configs/sama5d3xek.h         |   11 +++++++++++
 4 files changed, 17 insertions(+)

diff --git a/arch/arm/cpu/at91-common/spl.c b/arch/arm/cpu/at91-common/spl.c
index 37c0cc4..2598e09 100644
--- a/arch/arm/cpu/at91-common/spl.c
+++ b/arch/arm/cpu/at91-common/spl.c
@@ -52,6 +52,8 @@ u32 spl_boot_device(void)
 {
 #ifdef CONFIG_SYS_USE_MMC
 	return BOOT_DEVICE_MMC1;
+#elif CONFIG_SYS_USE_SERIALFLASH
+	return BOOT_DEVICE_SPI;
 #endif
 	return BOOT_DEVICE_NONE;
 }
diff --git a/arch/arm/include/asm/arch-at91/spl.h b/arch/arm/include/asm/arch-at91/spl.h
index 68c5349..f4ced49 100644
--- a/arch/arm/include/asm/arch-at91/spl.h
+++ b/arch/arm/include/asm/arch-at91/spl.h
@@ -14,6 +14,8 @@ enum {
 	BOOT_DEVICE_MMC1,
 	BOOT_DEVICE_MMC2,
 	BOOT_DEVICE_MMC2_2,
+#elif CONFIG_SYS_USE_SERIALFLASH
+	BOOT_DEVICE_SPI,
 #endif
 };
 
diff --git a/board/atmel/sama5d3xek/sama5d3xek.c b/board/atmel/sama5d3xek/sama5d3xek.c
index 0ab8020..e764c62 100644
--- a/board/atmel/sama5d3xek/sama5d3xek.c
+++ b/board/atmel/sama5d3xek/sama5d3xek.c
@@ -306,6 +306,8 @@ void spl_board_init(void)
 {
 #ifdef CONFIG_SYS_USE_MMC
 	sama5d3xek_mci_hw_init();
+#elif CONFIG_SYS_USE_SERIALFLASH
+	at91_spi0_hw_init(1 << 0);
 #endif
 }
 
diff --git a/include/configs/sama5d3xek.h b/include/configs/sama5d3xek.h
index c34feb5..510f22c 100644
--- a/include/configs/sama5d3xek.h
+++ b/include/configs/sama5d3xek.h
@@ -258,6 +258,8 @@
 #define CONFIG_SPL_SERIAL_SUPPORT
 
 #define CONFIG_SPL_BOARD_INIT
+#define CONFIG_SYS_MONITOR_LEN		(512 << 10)
+
 #ifdef CONFIG_SYS_USE_MMC
 #define CONFIG_SPL_LDSCRIPT		arch/arm/cpu/at91-common/u-boot-spl.lds
 #define CONFIG_SPL_MMC_SUPPORT
@@ -267,6 +269,15 @@
 #define CONFIG_SPL_FAT_LOAD_PAYLOAD_NAME	"u-boot.img"
 #define CONFIG_SPL_FAT_SUPPORT
 #define CONFIG_SPL_LIBDISK_SUPPORT
+
+#elif CONFIG_SYS_USE_SERIALFLASH
+#define CONFIG_SPL_SPI_SUPPORT
+#define CONFIG_SPL_SPI_FLASH_SUPPORT
+#define CONFIG_SPL_SPI_LOAD
+#define CONFIG_SPL_SPI_BUS		0
+#define CONFIG_SPL_SPI_CS		0
+#define CONFIG_SYS_SPI_U_BOOT_OFFS	0x8400
+
 #endif
 
 #endif
-- 
1.7.9.5

[U-Boot] [PATCH 2/3] mtd: nand: atmel: prepare for nand spl boot support

[Bo Shen]

Prepare for nand spl boot support. It supports nand software ECC and
hardware PMECC.
This patch is take <drivers/mtd/nand/nand_spl_simple.c> as reference.

Signed-off-by: Bo Shen <voice.shen@atmel.com>
---
 drivers/mtd/nand/atmel_nand.c |  206 +++++++++++++++++++++++++++++++++++++++++
 include/nand.h                |    6 ++
 2 files changed, 212 insertions(+)

diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
index da83f06..64e11e1 100644
--- a/drivers/mtd/nand/atmel_nand.c
+++ b/drivers/mtd/nand/atmel_nand.c
@@ -32,6 +32,12 @@
 
 #ifdef CONFIG_ATMEL_NAND_HW_PMECC
 
+#ifdef CONFIG_SPL_BUILD
+#ifdef CONFIG_SYS_NAND_ONFI_DETECTION
+#undef CONFIG_SYS_NAND_ONFI_DETECTION
+#endif
+#endif
+
 struct atmel_nand_host {
 	struct pmecc_regs __iomem *pmecc;
 	struct pmecc_errloc_regs __iomem *pmerrloc;
@@ -1163,6 +1169,8 @@ static int at91_nand_ready(struct mtd_info *mtd)
 }
 #endif
 
+#ifndef CONFIG_SPL_BUILD
+
 #ifndef CONFIG_SYS_NAND_BASE_LIST
 #define CONFIG_SYS_NAND_BASE_LIST { CONFIG_SYS_NAND_BASE }
 #endif
@@ -1217,3 +1225,201 @@ void board_nand_init(void)
 			printk(KERN_ERR "atmel_nand: Fail to initialize #%d chip",
 				i);
 }
+
+#else
+/* The following code is for SPL */
+static nand_info_t mtd;
+static struct nand_chip nand_chip;
+
+#ifdef CONFIG_SPL_NAND_SOFTECC
+static int nand_ecc_pos[] = CONFIG_SYS_NAND_ECCPOS;
+#define ECCSTEPS (CONFIG_SYS_NAND_PAGE_SIZE / \
+		  CONFIG_SYS_NAND_ECCSIZE)
+#define ECCTOTAL (ECCSTEPS * CONFIG_SYS_NAND_ECCBYTES)
+#endif
+
+static int nand_command(int block, int page, uint32_t offs, u8 cmd)
+{
+	struct nand_chip *this = mtd.priv;
+	int page_addr = page + block * CONFIG_SYS_NAND_PAGE_COUNT;
+	void (*hwctrl)(struct mtd_info *mtd, int cmd,
+			unsigned int ctrl) = this->cmd_ctrl;
+
+	while (this->dev_ready(&mtd))
+		;
+
+	if (cmd == NAND_CMD_READOOB) {
+		offs += CONFIG_SYS_NAND_PAGE_SIZE;
+		cmd = NAND_CMD_READ0;
+	}
+
+	hwctrl(&mtd, cmd, NAND_CTRL_CLE | NAND_CTRL_CHANGE);
+
+	if (this->options & NAND_BUSWIDTH_16)
+		offs >>= 1;
+
+	hwctrl(&mtd, offs & 0xff, NAND_CTRL_ALE | NAND_CTRL_CHANGE);
+	hwctrl(&mtd, (offs >> 8) & 0xff, NAND_CTRL_ALE);
+	hwctrl(&mtd, (page_addr & 0xff), NAND_CTRL_ALE);
+	hwctrl(&mtd, ((page_addr >> 8) & 0xff), NAND_CTRL_ALE);
+#ifdef CONFIG_SYS_NAND_5_ADDR_CYCLE
+	hwctrl(&mtd, (page_addr >> 16) & 0x0f, NAND_CTRL_ALE);
+#endif
+	hwctrl(&mtd, NAND_CMD_NONE, NAND_NCE | NAND_CTRL_CHANGE);
+
+	hwctrl(&mtd, NAND_CMD_READSTART, NAND_CTRL_CLE | NAND_CTRL_CHANGE);
+	hwctrl(&mtd, NAND_CMD_NONE, NAND_NCE | NAND_CTRL_CHANGE);
+
+	while (this->dev_ready(&mtd))
+		;
+
+	return 0;
+}
+
+static int nand_is_bad_block(int block)
+{
+	struct nand_chip *this = mtd.priv;
+
+	nand_command(block, 0, CONFIG_SYS_NAND_BAD_BLOCK_POS, NAND_CMD_READOOB);
+
+	if (this->options & NAND_BUSWIDTH_16) {
+		if (readw(this->IO_ADDR_R) != 0xffff)
+			return 1;
+	} else {
+		if (readb(this->IO_ADDR_R) != 0xff)
+			return 1;
+	}
+
+	return 0;
+}
+
+static int nand_read_page(int block, int page, void *dst)
+{
+	struct nand_chip *this = mtd.priv;
+#ifdef CONFIG_SPL_NAND_SOFTECC
+	u_char ecc_calc[ECCTOTAL];
+	u_char ecc_code[ECCTOTAL];
+	u_char oob_data[CONFIG_SYS_NAND_OOBSIZE];
+	int eccsize = CONFIG_SYS_NAND_ECCSIZE;
+	int eccbytes = CONFIG_SYS_NAND_ECCBYTES;
+	int eccsteps = ECCSTEPS;
+	int i;
+	uint8_t *p = dst;
+#endif
+	nand_command(block, page, 0, NAND_CMD_READ0);
+
+#ifdef CONFIG_SPL_NAND_SOFTECC
+	for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) {
+		if (this->ecc.mode != NAND_ECC_SOFT)
+			this->ecc.hwctl(&mtd, NAND_ECC_READ);
+		this->read_buf(&mtd, p, eccsize);
+		this->ecc.calculate(&mtd, p, &ecc_calc[i]);
+	}
+	this->read_buf(&mtd, oob_data, CONFIG_SYS_NAND_OOBSIZE);
+
+	for (i = 0; i < ECCTOTAL; i++)
+		ecc_code[i] = oob_data[nand_ecc_pos[i]];
+
+	eccsteps = ECCSTEPS;
+	p = dst;
+
+	for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize)
+		this->ecc.correct(&mtd, p, &ecc_code[i], &ecc_calc[i]);
+#else
+	atmel_nand_pmecc_read_page(&mtd, this, dst, 0, page);
+#endif
+
+	return 0;
+}
+
+int nand_spl_load_image(uint32_t offs, unsigned int size, void *dst)
+{
+	unsigned int block, lastblock;
+	unsigned int page;
+
+	block = offs / CONFIG_SYS_NAND_BLOCK_SIZE;
+	lastblock = (offs + size - 1) / CONFIG_SYS_NAND_BLOCK_SIZE;
+	page = (offs % CONFIG_SYS_NAND_BLOCK_SIZE) / CONFIG_SYS_NAND_PAGE_SIZE;
+
+	while (block <= lastblock) {
+		if (!nand_is_bad_block(block)) {
+			while (page < CONFIG_SYS_NAND_PAGE_COUNT) {
+				nand_read_page(block, page, dst);
+				dst += CONFIG_SYS_NAND_PAGE_SIZE;
+				page++;
+			}
+
+			page = 0;
+		} else {
+			lastblock++;
+		}
+
+		block++;
+	}
+
+	return 0;
+}
+
+int at91_nand_wait_ready(struct mtd_info *mtd)
+{
+	struct nand_chip *this = mtd->priv;
+
+	udelay(this->chip_delay);
+
+	return 0;
+}
+
+int board_nand_init(struct nand_chip *nand)
+{
+	int ret = 0;
+
+	nand->ecc.mode = NAND_ECC_SOFT;
+#ifdef CONFIG_SYS_NAND_DBW_16
+	nand->options = NAND_BUSWIDTH_16;
+	nand->read_buf = nand_read_buf16;
+#else
+	nand->read_buf = nand_read_buf;
+#endif
+	nand->cmd_ctrl = at91_nand_hwcontrol;
+#ifdef CONFIG_SYS_NAND_READY_PIN
+	nand->dev_ready = at91_nand_ready;
+#else
+	nand->dev_ready = at91_nand_wait_ready;
+#endif
+	nand->chip_delay = 20;
+
+#ifdef CONFIG_ATMEL_NAND_HWECC
+#ifdef CONFIG_ATMEL_NAND_HW_PMECC
+	ret = atmel_pmecc_nand_init_params(nand, &mtd);
+#endif
+#endif
+
+	return ret;
+}
+
+void nand_init(void)
+{
+	mtd.writesize = CONFIG_SYS_NAND_PAGE_SIZE;
+	mtd.oobsize = CONFIG_SYS_NAND_OOBSIZE;
+	mtd.priv = &nand_chip;
+	nand_chip.IO_ADDR_R = (void __iomem *)CONFIG_SYS_NAND_BASE;
+	nand_chip.IO_ADDR_W = (void __iomem *)CONFIG_SYS_NAND_BASE;
+	board_nand_init(&nand_chip);
+
+#ifdef CONFIG_SPL_NAND_SOFTECC
+	if (nand_chip.ecc.mode == NAND_ECC_SOFT) {
+		nand_chip.ecc.calculate = nand_calculate_ecc;
+		nand_chip.ecc.correct = nand_correct_data;
+	}
+#endif
+
+	if (nand_chip.select_chip)
+		nand_chip.select_chip(&mtd, 0);
+}
+
+void nand_deselect(void)
+{
+	if (nand_chip.select_chip)
+		nand_chip.select_chip(&mtd, -1);
+}
+#endif
diff --git a/include/nand.h b/include/nand.h
index 84116f4..fc735d1 100644
--- a/include/nand.h
+++ b/include/nand.h
@@ -21,10 +21,16 @@
  * at the same time, so do it here.  When all drivers are
  * converted, this will go away.
  */
+#ifdef CONFIG_SPL_BUILD
+#if defined(CONFIG_NAND_FSL_ELBC) || defined(CONFIG_NAND_FSL_IFC)
+#define CONFIG_SYS_NAND_SELF_INIT
+#endif
+#else
 #if defined(CONFIG_NAND_FSL_ELBC) || defined(CONFIG_NAND_ATMEL)\
 	|| defined(CONFIG_NAND_FSL_IFC)
 #define CONFIG_SYS_NAND_SELF_INIT
 #endif
+#endif
 
 extern void nand_init(void);
 
-- 
1.7.9.5

[U-Boot] [PATCH 3/3] arm: atmel: sama5d3: add nand spl boot support

[Bo Shen]

Add NAND SPL boot support with hardware PMECC.

Signed-off-by: Bo Shen <voice.shen@atmel.com>

---
 arch/arm/cpu/at91-common/spl.c       |    2 ++
 arch/arm/include/asm/arch-at91/spl.h |    2 ++
 board/atmel/sama5d3xek/sama5d3xek.c  |    2 ++
 include/configs/sama5d3xek.h         |   12 ++++++++++++
 4 files changed, 18 insertions(+)

diff --git a/arch/arm/cpu/at91-common/spl.c b/arch/arm/cpu/at91-common/spl.c
index 2598e09..7f4debb 100644
--- a/arch/arm/cpu/at91-common/spl.c
+++ b/arch/arm/cpu/at91-common/spl.c
@@ -52,6 +52,8 @@ u32 spl_boot_device(void)
 {
 #ifdef CONFIG_SYS_USE_MMC
 	return BOOT_DEVICE_MMC1;
+#elif CONFIG_SYS_USE_NANDFLASH
+	return BOOT_DEVICE_NAND;
 #elif CONFIG_SYS_USE_SERIALFLASH
 	return BOOT_DEVICE_SPI;
 #endif
diff --git a/arch/arm/include/asm/arch-at91/spl.h b/arch/arm/include/asm/arch-at91/spl.h
index f4ced49..d8a87da 100644
--- a/arch/arm/include/asm/arch-at91/spl.h
+++ b/arch/arm/include/asm/arch-at91/spl.h
@@ -14,6 +14,8 @@ enum {
 	BOOT_DEVICE_MMC1,
 	BOOT_DEVICE_MMC2,
 	BOOT_DEVICE_MMC2_2,
+#elif CONFIG_SYS_USE_NANDFLASH
+	BOOT_DEVICE_NAND,
 #elif CONFIG_SYS_USE_SERIALFLASH
 	BOOT_DEVICE_SPI,
 #endif
diff --git a/board/atmel/sama5d3xek/sama5d3xek.c b/board/atmel/sama5d3xek/sama5d3xek.c
index e764c62..c522ca1 100644
--- a/board/atmel/sama5d3xek/sama5d3xek.c
+++ b/board/atmel/sama5d3xek/sama5d3xek.c
@@ -306,6 +306,8 @@ void spl_board_init(void)
 {
 #ifdef CONFIG_SYS_USE_MMC
 	sama5d3xek_mci_hw_init();
+#elif CONFIG_SYS_USE_NANDFLASH
+	sama5d3xek_nand_hw_init();
 #elif CONFIG_SYS_USE_SERIALFLASH
 	at91_spi0_hw_init(1 << 0);
 #endif
diff --git a/include/configs/sama5d3xek.h b/include/configs/sama5d3xek.h
index 510f22c..516be85 100644
--- a/include/configs/sama5d3xek.h
+++ b/include/configs/sama5d3xek.h
@@ -270,6 +270,18 @@
 #define CONFIG_SPL_FAT_SUPPORT
 #define CONFIG_SPL_LIBDISK_SUPPORT
 
+#elif CONFIG_SYS_USE_NANDFLASH
+#define CONFIG_SPL_NAND_SUPPORT
+#define CONFIG_SPL_NAND_DRIVERS
+#define CONFIG_SPL_NAND_BASE
+#define CONFIG_SYS_NAND_U_BOOT_OFFS	0x40000
+#define CONFIG_SYS_NAND_5_ADDR_CYCLE
+#define CONFIG_SYS_NAND_PAGE_SIZE	0x800
+#define CONFIG_SYS_NAND_PAGE_COUNT	64
+#define CONFIG_SYS_NAND_OOBSIZE		64
+#define CONFIG_SYS_NAND_BLOCK_SIZE	0x20000
+#define CONFIG_SYS_NAND_BAD_BLOCK_POS	0x0
+
 #elif CONFIG_SYS_USE_SERIALFLASH
 #define CONFIG_SPL_SPI_SUPPORT
 #define CONFIG_SPL_SPI_FLASH_SUPPORT
-- 
1.7.9.5

[U-Boot] [PATCH 1/3] arm: atmel: sama5d3: add spi spl boot support

[Andreas Bießmann]

Hi Bo,

On 12/02/2013 04:24 AM, Bo Shen wrote:
> Add SPI SPL boot support for sama5d3xek board.
> 
> Signed-off-by: Bo Shen <voice.shen@atmel.com>
> ---
>  arch/arm/cpu/at91-common/spl.c       |    2 ++
>  arch/arm/include/asm/arch-at91/spl.h |    2 ++
>  board/atmel/sama5d3xek/sama5d3xek.c  |    2 ++
>  include/configs/sama5d3xek.h         |   11 +++++++++++
>  4 files changed, 17 insertions(+)
> 
> diff --git a/arch/arm/cpu/at91-common/spl.c b/arch/arm/cpu/at91-common/spl.c
> index 37c0cc4..2598e09 100644
> --- a/arch/arm/cpu/at91-common/spl.c
> +++ b/arch/arm/cpu/at91-common/spl.c
> @@ -52,6 +52,8 @@ u32 spl_boot_device(void)
>  {
>  #ifdef CONFIG_SYS_USE_MMC
>  	return BOOT_DEVICE_MMC1;
> +#elif CONFIG_SYS_USE_SERIALFLASH
> +	return BOOT_DEVICE_SPI;
>  #endif
>  	return BOOT_DEVICE_NONE;
>  }

I'm not really happy with this solution. We will get a ifdef-nightmare
here for each specific SPL implementation.

Could you please check with your ROM developers if there is some way to
get the boot source? I know Ti guys have implemented some function
parameter to tell that to the next running code in ROM. I can't believe
Atmel has nothing comparable.
The last resort could be to add some proprietary parameter to the SPL
that we could read then and return the correct value in
spl_boot_device() depending on that that stored value ... quite ugly but
I think it is better than clutter this place with a lot of ifdef's.

Another solution could be to move this function to board code.

Any other suggestions?

Best regards

Andreas Bie?mann

[U-Boot] [PATCH 1/3] arm: atmel: sama5d3: add spi spl boot support

[Bo Shen]

Hi Andreas,

On 12/09/2013 06:28 PM, Andreas Bie?mann wrote:
> Hi Bo,
>
> On 12/02/2013 04:24 AM, Bo Shen wrote:
>> Add SPI SPL boot support for sama5d3xek board.
>>
>> Signed-off-by: Bo Shen <voice.shen@atmel.com>
>> ---
>>   arch/arm/cpu/at91-common/spl.c       |    2 ++
>>   arch/arm/include/asm/arch-at91/spl.h |    2 ++
>>   board/atmel/sama5d3xek/sama5d3xek.c  |    2 ++
>>   include/configs/sama5d3xek.h         |   11 +++++++++++
>>   4 files changed, 17 insertions(+)
>>
>> diff --git a/arch/arm/cpu/at91-common/spl.c b/arch/arm/cpu/at91-common/spl.c
>> index 37c0cc4..2598e09 100644
>> --- a/arch/arm/cpu/at91-common/spl.c
>> +++ b/arch/arm/cpu/at91-common/spl.c
>> @@ -52,6 +52,8 @@ u32 spl_boot_device(void)
>>   {
>>   #ifdef CONFIG_SYS_USE_MMC
>>   	return BOOT_DEVICE_MMC1;
>> +#elif CONFIG_SYS_USE_SERIALFLASH
>> +	return BOOT_DEVICE_SPI;
>>   #endif
>>   	return BOOT_DEVICE_NONE;
>>   }
>
> I'm not really happy with this solution. We will get a ifdef-nightmare
> here for each specific SPL implementation.
>
> Could you please check with your ROM developers if there is some way to
> get the boot source? I know Ti guys have implemented some function
> parameter to tell that to the next running code in ROM. I can't believe
> Atmel has nothing comparable.

Unfortunately, we don't have this mechanism.
The BootRom has a boot sequence, detect one by one. More information 
please reference sama5d3 datasheet on page 65, figure 12-2. NVM 
Bootloader Sequence Diagram.

http://www.atmel.com/images/atmel_11121_32-bit-cortex-a5-microcontroller_sama5d3_datasheet.pdf

> The last resort could be to add some proprietary parameter to the SPL
> that we could read then and return the correct value in
> spl_boot_device() depending on that that stored value ... quite ugly but
> I think it is better than clutter this place with a lot of ifdef's.
>
> Another solution could be to move this function to board code.

As the most Atmel SoC based boards support some of these boot mode, so 
it can keep minimal code for these boards. So, if we move this to board 
code, I think every should copy this code.

> Any other suggestions?
>
> Best regards
>
> Andreas Bie?mann
>

Best Regards,
Bo Shen

[U-Boot] [PATCH 2/3] mtd: nand: atmel: prepare for nand spl boot support

[Scott Wood]

On Mon, 2013-12-02 at 11:24 +0800, Bo Shen wrote:
> Prepare for nand spl boot support. It supports nand software ECC and
> hardware PMECC.
> This patch is take <drivers/mtd/nand/nand_spl_simple.c> as reference.
> 
> Signed-off-by: Bo Shen <voice.shen@atmel.com>
> ---
>  drivers/mtd/nand/atmel_nand.c |  206 +++++++++++++++++++++++++++++++++++++++++
>  include/nand.h                |    6 ++
>  2 files changed, 212 insertions(+)

Looks OK but some style nits:
 
> diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
> index da83f06..64e11e1 100644
> --- a/drivers/mtd/nand/atmel_nand.c
> +++ b/drivers/mtd/nand/atmel_nand.c
> @@ -32,6 +32,12 @@
>  
>  #ifdef CONFIG_ATMEL_NAND_HW_PMECC
>  
> +#ifdef CONFIG_SPL_BUILD
> +#ifdef CONFIG_SYS_NAND_ONFI_DETECTION
> +#undef CONFIG_SYS_NAND_ONFI_DETECTION
> +#endif
> +#endif

There's no need to ifdef before undeffing.

>  struct atmel_nand_host {
>  	struct pmecc_regs __iomem *pmecc;
>  	struct pmecc_errloc_regs __iomem *pmerrloc;
> @@ -1163,6 +1169,8 @@ static int at91_nand_ready(struct mtd_info *mtd)
>  }
>  #endif
>  
> +#ifndef CONFIG_SPL_BUILD

Use ifdef rather than ifndef for if/else.

> +#ifdef CONFIG_SPL_NAND_SOFTECC

This symbol needs to be documented (I realize it isn't new).

> +static int nand_ecc_pos[] = CONFIG_SYS_NAND_ECCPOS;
> +#define ECCSTEPS (CONFIG_SYS_NAND_PAGE_SIZE / \
> +		  CONFIG_SYS_NAND_ECCSIZE)
> +#define ECCTOTAL (ECCSTEPS * CONFIG_SYS_NAND_ECCBYTES)
> +#endif

Is this stuff used anywhere but nand_read_page()?  If not, move it
there (as local variables, not #defines).

> +static int nand_read_page(int block, int page, void *dst)
> +{
> +	struct nand_chip *this = mtd.priv;
> +#ifdef CONFIG_SPL_NAND_SOFTECC
> +	u_char ecc_calc[ECCTOTAL];
> +	u_char ecc_code[ECCTOTAL];
> +	u_char oob_data[CONFIG_SYS_NAND_OOBSIZE];
> +	int eccsize = CONFIG_SYS_NAND_ECCSIZE;
> +	int eccbytes = CONFIG_SYS_NAND_ECCBYTES;
> +	int eccsteps = ECCSTEPS;
> +	int i;
> +	uint8_t *p = dst;
> +#endif
> +	nand_command(block, page, 0, NAND_CMD_READ0);
> +
> +#ifdef CONFIG_SPL_NAND_SOFTECC
> +	for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) {
> +		if (this->ecc.mode != NAND_ECC_SOFT)
> +			this->ecc.hwctl(&mtd, NAND_ECC_READ);
> +		this->read_buf(&mtd, p, eccsize);
> +		this->ecc.calculate(&mtd, p, &ecc_calc[i]);
> +	}
> +	this->read_buf(&mtd, oob_data, CONFIG_SYS_NAND_OOBSIZE);
> +
> +	for (i = 0; i < ECCTOTAL; i++)
> +		ecc_code[i] = oob_data[nand_ecc_pos[i]];
> +
> +	eccsteps = ECCSTEPS;
> +	p = dst;
> +
> +	for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize)
> +		this->ecc.correct(&mtd, p, &ecc_code[i], &ecc_calc[i]);
> +#else
> +	atmel_nand_pmecc_read_page(&mtd, this, dst, 0, page);
> +#endif
> +
> +	return 0;
> +}

These don't share enough code to warrant interleaving like this -- just
have one big ifdef/else.  It will be more readable.

-Scott

[U-Boot] [PATCH 2/3] mtd: nand: atmel: prepare for nand spl boot support

[Bo Shen]

Hi Scott,

On 02/28/2014 08:35 AM, Scott Wood wrote:
> On Mon, 2013-12-02 at 11:24 +0800, Bo Shen wrote:
>> Prepare for nand spl boot support. It supports nand software ECC and
>> hardware PMECC.
>> This patch is take <drivers/mtd/nand/nand_spl_simple.c> as reference.
>>
>> Signed-off-by: Bo Shen <voice.shen@atmel.com>
>> ---
>>   drivers/mtd/nand/atmel_nand.c |  206 +++++++++++++++++++++++++++++++++++++++++
>>   include/nand.h                |    6 ++
>>   2 files changed, 212 insertions(+)
>
> Looks OK but some style nits:
>
>> diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
>> index da83f06..64e11e1 100644
>> --- a/drivers/mtd/nand/atmel_nand.c
>> +++ b/drivers/mtd/nand/atmel_nand.c
>> @@ -32,6 +32,12 @@
>>
>>   #ifdef CONFIG_ATMEL_NAND_HW_PMECC
>>
>> +#ifdef CONFIG_SPL_BUILD
>> +#ifdef CONFIG_SYS_NAND_ONFI_DETECTION
>> +#undef CONFIG_SYS_NAND_ONFI_DETECTION
>> +#endif
>> +#endif
>
> There's no need to ifdef before undeffing.

I will remove the ifdef.

>>   struct atmel_nand_host {
>>   	struct pmecc_regs __iomem *pmecc;
>>   	struct pmecc_errloc_regs __iomem *pmerrloc;
>> @@ -1163,6 +1169,8 @@ static int at91_nand_ready(struct mtd_info *mtd)
>>   }
>>   #endif
>>
>> +#ifndef CONFIG_SPL_BUILD
>
> Use ifdef rather than ifndef for if/else.

OK, I will use ifdef instead of ifndef.

>> +#ifdef CONFIG_SPL_NAND_SOFTECC
>
> This symbol needs to be documented (I realize it isn't new).

OK, I will document it.

>> +static int nand_ecc_pos[] = CONFIG_SYS_NAND_ECCPOS;
>> +#define ECCSTEPS (CONFIG_SYS_NAND_PAGE_SIZE / \
>> +		  CONFIG_SYS_NAND_ECCSIZE)
>> +#define ECCTOTAL (ECCSTEPS * CONFIG_SYS_NAND_ECCBYTES)
>> +#endif
>
> Is this stuff used anywhere but nand_read_page()?  If not, move it
> there (as local variables, not #defines).

OK, I will move where  use it.

>> +static int nand_read_page(int block, int page, void *dst)
>> +{
>> +	struct nand_chip *this = mtd.priv;
>> +#ifdef CONFIG_SPL_NAND_SOFTECC
>> +	u_char ecc_calc[ECCTOTAL];
>> +	u_char ecc_code[ECCTOTAL];
>> +	u_char oob_data[CONFIG_SYS_NAND_OOBSIZE];
>> +	int eccsize = CONFIG_SYS_NAND_ECCSIZE;
>> +	int eccbytes = CONFIG_SYS_NAND_ECCBYTES;
>> +	int eccsteps = ECCSTEPS;
>> +	int i;
>> +	uint8_t *p = dst;
>> +#endif
>> +	nand_command(block, page, 0, NAND_CMD_READ0);
>> +
>> +#ifdef CONFIG_SPL_NAND_SOFTECC
>> +	for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) {
>> +		if (this->ecc.mode != NAND_ECC_SOFT)
>> +			this->ecc.hwctl(&mtd, NAND_ECC_READ);
>> +		this->read_buf(&mtd, p, eccsize);
>> +		this->ecc.calculate(&mtd, p, &ecc_calc[i]);
>> +	}
>> +	this->read_buf(&mtd, oob_data, CONFIG_SYS_NAND_OOBSIZE);
>> +
>> +	for (i = 0; i < ECCTOTAL; i++)
>> +		ecc_code[i] = oob_data[nand_ecc_pos[i]];
>> +
>> +	eccsteps = ECCSTEPS;
>> +	p = dst;
>> +
>> +	for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize)
>> +		this->ecc.correct(&mtd, p, &ecc_code[i], &ecc_calc[i]);
>> +#else
>> +	atmel_nand_pmecc_read_page(&mtd, this, dst, 0, page);
>> +#endif
>> +
>> +	return 0;
>> +}
>
> These don't share enough code to warrant interleaving like this -- just
> have one big ifdef/else.  It will be more readable.

I will change in next version.

> -Scott
>
>

Best Regards,
Bo Shen

[U-Boot] [PATCH 2/3] mtd: nand: atmel: prepare for nand spl boot support

[Bo Shen]

Hi Scott,

On 03/03/2014 10:41 AM, Bo Shen wrote:
>
>>> +#ifdef CONFIG_SPL_NAND_SOFTECC
>>
>> This symbol needs to be documented (I realize it isn't new).
>
> OK, I will document it.

I see in README file, it uses CONFIG_SPL_NAND_ECC for software ecc 
selection. So, I will use this one while not add new 
CONFIG_SPL_NAND_SOFTECC in next version.

Best Regards,
Bo Shen