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;elserand_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