[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 at 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 at antmicro.com>
> Signed-off-by: Tomasz Gorochowik <tgorochowik at antmicro.com>
> Signed-off-by: Mateusz Holenko <mholenko at antmicro.com>
> Signed-off-by: Piotr Zierhoffer <pzierhoffer at 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