[U-Boot] [UBOOT PATCH v2 2/2] arasan: nfc: Add initial nand driver support for arasan

[Scott Wood <scottwood@freescale.com>]

On Fri, Nov 06, 2015 at 04:52:33PM +0530, Siva Durga Prasad Paladugu wrote:
> Added initial nand driver support for arasan nand flash
> controller.This supports nand erase,nand read, nand write
> This uses the hardware ECC for read and write operations
> ZynqMP uses this  driver.
> 
> Signed-off-by: Siva Durga Prasad Paladugu <sivadur@xilinx.com>
> ---
> Changes from v1:
> - Made some variables global to filescope
>   wherever applicable
> - Used real units of time for timeouts
> - Handled proper return values
> - Removed arasan_read_byte and merged  its
>   functionality to arasan_nand_read_byte
> - Fixed coding style changes
> - Removed On Die ECC support code for now
> - Removed code ReadId from init.
> ---
>  drivers/mtd/nand/Makefile     |    1 +
>  drivers/mtd/nand/arasan_nfc.c | 1173 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 1174 insertions(+)
>  create mode 100644 drivers/mtd/nand/arasan_nfc.c
> 
> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
> index b4e5376..6fb3718 100644
> --- a/drivers/mtd/nand/Makefile
> +++ b/drivers/mtd/nand/Makefile
> @@ -42,6 +42,7 @@ ifdef NORMAL_DRIVERS
>  obj-$(CONFIG_NAND_ECC_BCH) += nand_bch.o
>  
>  obj-$(CONFIG_NAND_ATMEL) += atmel_nand.o
> +obj-$(CONFIG_NAND_ARASAN) += arasan_nfc.o
>  obj-$(CONFIG_DRIVER_NAND_BFIN) += bfin_nand.o
>  obj-$(CONFIG_NAND_DAVINCI) += davinci_nand.o
>  obj-$(CONFIG_NAND_DENALI) += denali.o
> diff --git a/drivers/mtd/nand/arasan_nfc.c b/drivers/mtd/nand/arasan_nfc.c
> new file mode 100644
> index 0000000..a697cce
> --- /dev/null
> +++ b/drivers/mtd/nand/arasan_nfc.c
> @@ -0,0 +1,1173 @@
> +/*
> + * Arasan NAND Flash Controller Driver
> + *
> + * Copyright (C) 2014 - 2015 Xilinx, Inc.
> + *
> + * SPDX-License-Identifier:     GPL-2.0
> + */

Usually U-Boot contributions are requested to be "GPL-2.0+".  Is there
any particular reason why this can't be?


> +struct arasan_ecc_matrix {
> +	u32 pagesize;
> +	u8 ecc_codeword_size;
> +	u8 eccbits;
> +	u8 slcmlc;
> +	u16 eccaddr;
> +	u16 eccsize;
> +};
> +
> +static const struct arasan_ecc_matrix ecc_matrix[] = {
> +	{512, 9, 1, 0, 0x20D, 0x3},
> +	{512, 9, 4, 1, 0x209, 0x7},
> +	{512, 9, 8, 1, 0x203, 0xD},
> +	/*
> +	 * 2K byte page
> +	 */
> +	{2048, 9, 1, 0, 0x834, 0xC},
> +	{2048, 9, 4, 1, 0x826, 0x1A},
> +	{2048, 9, 8, 1, 0x80c, 0x34},
> +	{2048, 9, 12, 1, 0x822, 0x4E},
> +	{2048, 9, 16, 1, 0x808, 0x68},
> +	{2048, 10, 24, 1, 0x81c, 0x54},
> +	/*
> +	 * 4K byte page
> +	 */
> +	{4096, 9, 1, 0, 0x1068, 0x18},
> +	{4096, 9, 4, 1, 0x104c, 0x34},
> +	{4096, 9, 8, 1, 0x1018, 0x68},
> +	{4096, 9, 12, 1, 0x1044, 0x9C},
> +	{4096, 9, 16, 1, 0x1010, 0xD0},
> +	{4096, 10, 24, 1, 0x1038, 0xA8},
> +	/*
> +	 * 8K byte page
> +	 */
> +	{8192, 9, 1, 0, 0x20d0, 0x30},
> +	{8192, 9, 4, 1, 0x2098, 0x68},
> +	{8192, 9, 8, 1, 0x2030, 0xD0},
> +	{8192, 9, 12, 1, 0x2088, 0x138},
> +	{8192, 9, 16, 1, 0x2020, 0x1A0},
> +	{8192, 24, 10, 1, 0x2070, 0x150},
> +	/*
> +	 * 16K byte page
> +	 */
> +	{16384, 9, 1, 0, 0x4460, 0x60},
> +	{16384, 9, 4, 1, 0x43f0, 0xD0},
> +	{16384, 9, 8, 1, 0x4320, 0x1A0},
> +	{16384, 9, 12, 1, 0x4250, 0x270},
> +	{16384, 9, 16, 1, 0x4180, 0x340},
> +	{16384, 10, 24, 1, 0x4220, 0x2A0}
> +};

That last line of the 8k section looks suspicious with 24, 10 being
reversed compared to the others.

> +static void arasan_nand_fill_tx(const u8 *buf, int len)
> +{
> +	u32 __iomem *nand = (u32 __iomem *)&arasan_nand_base->buf_dataport;

Unnecessary cast.

> +static int arasan_nand_write_page_hwecc(struct mtd_info *mtd,
> +		struct nand_chip *chip, const u8 *buf, int oob_required)
> +{
> +	u32 reg_val, i, pktsize, pktnum;
> +	const u32 *bufptr = (u32 *)buf;

Cast is missing "const".


> +static void arasan_nand_ecc_init(struct mtd_info *mtd)
> +{
> +	u32 found = 0;
> +	u8 bchmodeval = 0;
> +	u32 regval, eccpos_start, i;
> +	struct nand_chip *nand_chip = mtd->priv;
> +
> +	for (i = 0; i < ARRAY_SIZE(ecc_matrix);i++) {

Space after ;

> +		if ((ecc_matrix[i].pagesize == mtd->writesize) &&
> +		    ((1 << ecc_matrix[i].ecc_codeword_size) >=
> +		     nand_chip->ecc_step_ds)) {
> +			if (ecc_matrix[i].eccbits >=
> +			    nand_chip->ecc_strength_ds) {
> +				found = i;
> +				break;
> +			}
> +			found = i;
> +		}
> +	}
> +
> +	if (found) {

	if (!found)
		return;

...rather than indenting the rest of the function.

Shouldn't there be some sort of warning if you're unable to provide an
ECC strength that meets the chips requirement?  Or if ECC was not
initialized at all?

> +		regval = ecc_matrix[i].eccaddr | (ecc_matrix[i].eccsize << 16) |
> +			 (ecc_matrix[i].slcmlc << 27);
> +		writel(regval, &arasan_nand_base->ecc_reg);
> +
> +		if (ecc_matrix[i].slcmlc) {
> +			switch (ecc_matrix[i].eccbits) {
> +			case 16:
> +				bchmodeval = 0x0;
> +				break;
> +			case 12:
> +				bchmodeval = 0x1;
> +				break;
> +			case 8:
> +				bchmodeval = 0x2;
> +				break;
> +			case 4:
> +				bchmodeval = 0x3;
> +				break;
> +			case 24:
> +				bchmodeval = 0x4;
> +				break;
> +			default:
> +				break;
> +			}

This is a useless default clause.

Shouldn't there be some sort of error handling there instead of silently
using zero?

Or better yet, why isn't bchmodeval a field in struct arasan_ecc_matrix?

> +			regval = readl(&arasan_nand_base->memadr_reg2);
> +			regval &= ~ARASAN_NAND_MEM_ADDR2_BCH_MASK;
> +			regval |= (bchmodeval <<
> +				   ARASAN_NAND_MEM_ADDR2_BCH_SHIFT);
> +			writel(regval, &arasan_nand_base->memadr_reg2);
> +		}
> +
> +		nand_oob.eccbytes = ecc_matrix[i].eccsize;
> +		eccpos_start = mtd->oobsize - nand_oob.eccbytes;
> +
> +		for (i = 0; i < nand_oob.eccbytes; i++)
> +			nand_oob.eccpos[i] = eccpos_start + i;
> +
> +		nand_oob.oobfree[0].offset = 2;
> +		nand_oob.oobfree[0].length = eccpos_start - 2;
> +
> +		if (ecc_matrix[i].eccbits == 24)
> +			nand_chip->ecc.size = 1024;
> +		else
> +			nand_chip->ecc.size = 512;

Why not just use 1 << ecc_matrix[i].ecc_codeword_size?

Why is ecc_codeword_size stored as a logarithm when the only place that
uses it turns it back into a byte count?

> +		nand_chip->ecc.bytes = ecc_matrix[i].eccsize;
> +		nand_chip->ecc.layout = &nand_oob;
> +	}
> +}
> +
> +static int arasan_nand_init(struct nand_chip *nand_chip, int devnum)
> +{
> +	struct arasan_nand_info *nand;
> +	struct mtd_info *mtd;
> +	int err = -1;
> +
> +	nand = calloc(1, sizeof(struct arasan_nand_info));
> +	if (!nand) {
> +		printf("%s: failed to allocate\n", __func__);
> +		return err;
> +	}
> +
> +	nand->nand_base = (void *)ARASAN_NAND_BASEADDR;

You already have arsan_nand_base with the proper cast.  Why not use it?

> +	mtd = &nand_info[0];
> +	nand_chip->priv = nand;
> +	mtd->priv = nand_chip;
> +
> +	/* Set address of NAND IO lines */
> +	nand_chip->IO_ADDR_R = (void *)&arasan_nand_base->buf_dataport;
> +	nand_chip->IO_ADDR_W = (void *)&arasan_nand_base->buf_dataport;

Where do these get used?

> +	/* Set the driver entry points for MTD */
> +	nand_chip->cmdfunc = arasan_nand_cmd_function;
> +	nand_chip->select_chip = arasan_nand_select_chip;
> +	nand_chip->read_byte = arasan_nand_read_byte;
> +
> +	/* Buffer read/write routines */
> +	nand_chip->read_buf = arasan_nand_read_buf;
> +	nand_chip->write_buf = arasan_nand_write_buf;
> +	nand_chip->bbt_options = NAND_BBT_USE_FLASH;
> +
> +	writel(0x0, &arasan_nand_base->cmd_reg);
> +	writel(0x0, &arasan_nand_base->pgm_reg);
> +
> +	/* first scan to find the device and get the page size */
> +	if (nand_scan_ident(mtd, 1, NULL)) {
> +		printf("%s: nand_scan_ident failed\n", __func__);
> +		goto fail;
> +	}
> +
> +	mtd->size = nand_chip->chipsize;

nand_scan_ident() already takes care of setting mtd->size.

> +
> +	nand_chip->ecc.mode = NAND_ECC_HW;
> +	nand_chip->ecc.strength = 1;
> +	nand_chip->ecc.hwctl = NULL;
> +	nand_chip->ecc.read_page = arasan_nand_read_page_hwecc;
> +	nand_chip->ecc.write_page = arasan_nand_write_page_hwecc;
> +	nand_chip->ecc.read_oob = arasan_nand_read_oob;
> +	nand_chip->ecc.write_oob = arasan_nand_write_oob;

None of this needs to be after nand_scan_ident().  Why not keep it
together with the other nand_chip init?

Why is ecc.strength being set to 1 when other parts of the code make it
look like stronger ECC is supported?

-Scott