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

Scott Wood
2015-07-16T21:15:58Z

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