On Thu, 2015-07-16 at 13:25 +0200, Piotr Zierhoffer wrote: > From: Piotr Zierhoffer <piotr.zierhof...@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 <pgie...@antmicro.com> > Signed-off-by: Tomasz Gorochowik <tgorocho...@antmicro.com> > Signed-off-by: Mateusz Holenko <mhole...@antmicro.com> > Signed-off-by: Piotr Zierhoffer <pzierhof...@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 mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot