[U-Boot] [UBOOT PATCH] arasan: nfc: Add initial nand driver support for arasan
Scott Wood
scottwood at freescale.com
Thu May 14 02:39:35 CEST 2015
On Tue, 2015-04-28 at 18:15 +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
>
> Signed-off-by: Siva Durga Prasad Paladugu <sivadur at xilinx.com>
> ---
> drivers/mtd/nand/Makefile | 1 +
> drivers/mtd/nand/arasan_nfc.c | 1187 +++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 1188 insertions(+)
> create mode 100644 drivers/mtd/nand/arasan_nfc.c
>
> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
> index 347ea62..9080835 100644
> --- a/drivers/mtd/nand/Makefile
> +++ b/drivers/mtd/nand/Makefile
> @@ -66,6 +66,7 @@ obj-$(CONFIG_NAND_OMAP_GPMC) += omap_gpmc.o
> obj-$(CONFIG_NAND_OMAP_ELM) += omap_elm.o
> obj-$(CONFIG_NAND_PLAT) += nand_plat.o
> obj-$(CONFIG_NAND_DOCG4) += docg4.o
> +obj-$(CONFIG_ARASAN_NFC) += arasan_nfc.o
When will there be a config that enables this, and shouldn't new config
options be done in kconfig?
> diff --git a/drivers/mtd/nand/arasan_nfc.c b/drivers/mtd/nand/arasan_nfc.c
> new file mode 100644
> index 0000000..3eaab8f
> --- /dev/null
> +++ b/drivers/mtd/nand/arasan_nfc.c
> @@ -0,0 +1,1187 @@
> +/*
> + * Arasan NAND Flash Controller Driver
> + *
> + * Copyright (C) 2014 - 2015 Xilinx, Inc.
> + *
> + * SPDX-License-Identifier: GPL-2.0
> + */
I believe GPL2+ is preferred for new U-Boot code.
> +#include <common.h>
> +#include <malloc.h>
> +#include <asm/io.h>
> +#include <asm/errno.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/mtd/nand.h>
> +#include <linux/mtd/partitions.h>
> +#include <linux/mtd/nand_ecc.h>
> +#include <asm/arch/hardware.h>
> +#include <asm/arch/sys_proto.h>
> +#include <nand.h>
> +
> +struct arasan_nand_info {
> +#ifdef CONFIG_MTD_PARTITIONS
> + struct mtd_partition *parts;
> +#endif
No other U-Boot NAND driver references partitions -- why is this one
different?
> + void __iomem *nand_base;
> + u32 page;
> +};
> +
> +struct nand_regs {
> + u32 pkt_reg;
> + u32 memadr_reg1;
> + u32 memadr_reg2;
> + u32 cmd_reg;
> + u32 pgm_reg;
> + u32 intsts_enr;
> + u32 intsig_enr;
> + u32 intsts_reg;
> + u32 rdy_busy;
> + u32 cms_sysadr_reg;
> + u32 flash_sts_reg;
> + u32 tmg_reg;
> + u32 buf_dataport;
> + u32 ecc_reg;
> + u32 ecc_errcnt_reg;
> + u32 ecc_sprcmd_reg;
> + u32 errcnt_1bitreg;
> + u32 errcnt_2bitreg;
> + u32 errcnt_3bitreg;
> + u32 errcnt_4bitreg;
> + u32 dma_sysadr0_reg;
> + u32 dma_bufbdry_reg;
> + u32 cpu_rls_reg;
> + u32 errcnt_5bitreg;
> + u32 errcnt_6bitreg;
> + u32 errcnt_7bitreg;
> + u32 errcnt_8bitreg;
> + u32 data_if_reg;
> +};
> +#define arasan_nand_base ((struct nand_regs *)ARASAN_NAND_BASEADDR)
__iomem
> +struct arasan_nand_command_format *curr_cmd;
Why is this global?
> +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},
2K pages with more than 64 bytes ECC?
> + /*
> + * 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}
> +};
Can this be reasonably computed at runtime?
> +u32 buf_data[16384];
> +u32 buf_index = 0;
Why are these global?
> +static u8 arasan_nand_get_addrcycle(struct mtd_info *mtd)
> +{
> + u8 addrcycles;
> + struct nand_chip *chip = mtd->priv;
> +
> + switch (curr_cmd->addr_cycles) {
> + case NAND_ADDR_CYCL_NONE:
> + addrcycles = 0;
> + break;
> + case NAND_ADDR_CYCL_ONE:
> + addrcycles = 1;
> + break;
> + case NAND_ADDR_CYCL_ROW:
> + addrcycles = chip->onfi_params.addr_cycles &
> + ARASAN_NAND_ROW_ADDR_CYCL_MASK;
> + break;
> + case NAND_ADDR_CYCL_COL:
> + addrcycles = (chip->onfi_params.addr_cycles &
> + ARASAN_NAND_COL_ADDR_CYCL_MASK) >>
> + ARASAN_NAND_COL_ADDR_CYCL_SHIFT;
> + break;
> + case NAND_ADDR_CYCL_BOTH:
> + addrcycles = chip->onfi_params.addr_cycles &
> + ARASAN_NAND_ROW_ADDR_CYCL_MASK;
> + addrcycles += (chip->onfi_params.addr_cycles &
> + ARASAN_NAND_COL_ADDR_CYCL_MASK) >>
> + ARASAN_NAND_COL_ADDR_CYCL_SHIFT;
> + break;
> + default:
> + addrcycles = ARASAN_NAND_INVALID_ADDR_CYCL;
> + break;
> + }
Nothing checks for this invalid value...
> +static int arasan_nand_read_page(struct mtd_info *mtd, u8 *buf, u32 size)
> +{
> + struct nand_chip *chip = mtd->priv;
> + u32 reg_val, i, pktsize, pktnum;
> + u32 *bufptr = (u32 *)buf;
> + u32 timeout = ARASAN_NAND_POLL_TIMEOUT;
> + u32 rdcount = 0;
> + u8 addr_cycles;
> +
> + if (chip->ecc_step_ds >= ARASAN_NAND_PKTSIZE_1K)
> + pktsize = ARASAN_NAND_PKTSIZE_1K;
> + else
> + pktsize = ARASAN_NAND_PKTSIZE_512;
> +
> + if (size%pktsize)
Spaces around binary operators, here and elsewhere.
> + while (rdcount < pktnum) {
> + timeout = ARASAN_NAND_POLL_TIMEOUT;
> + while (!(readl(&arasan_nand_base->intsts_reg) &
> + ARASAN_NAND_INT_STS_BUF_RD_RDY_MASK) && timeout) {
> + timeout--;
> + }
> + if (!timeout) {
> + puts("arasan_read_page: timedout:Buff RDY\n");
> + return -1;
> + }
Return a proper error code.
> +static void arasan_nand_fill_tx(const u8 *buf, int len)
> +{
> + u32 *nand = &arasan_nand_base->buf_dataport;
__iomem
> +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;
> + u32 *bufptr = (u32 *)buf;
const
> + reg_val = readl(&arasan_nand_base->intsts_reg);
> + writel(reg_val | ARASAN_NAND_INT_STS_XFR_CMPLT_MASK ,
> + &arasan_nand_base->intsts_reg);
No space before comma
> +static int arasan_nand_write_oob(struct mtd_info *mtd, struct nand_chip *chip,
> + int page)
> +{
> + int status = 0;
> + const u8 *buf = chip->oob_poi;
> +
> + chip->cmdfunc(mtd, NAND_CMD_SEQIN, mtd->writesize, page);
> + chip->write_buf(mtd, buf, (mtd->oobsize));
Unnecessary parens.
> + while (!(readl(&arasan_nand_base->intsts_reg) &
> + ARASAN_NAND_INT_STS_XFR_CMPLT_MASK) && timeout) {
> + timeout--;
> + }
Timeouts should be based on real units of time rather than loop iterations.
> +static u8 arasan_nand_page(struct mtd_info *mtd)
> +{
> + u8 page_val = 0;
> +
> + switch (mtd->writesize) {
> + case 512:
> + page_val = 0;
> + break;
> + case 2048:
> + page_val = 1;
> + break;
> + case 4096:
> + page_val = 2;
> + break;
> + case 8192:
> + page_val = 3;
> + break;
> + case 16384:
> + page_val = 4;
> + break;
> + case 1024:
> + page_val = 5;
> + break;
> + default:
> + break;
Either print/return an error or get rid of the default:
> +static u8 arasan_read_byte(void)
> +{
> + u8 *bufptr = (u8 *)&buf_data[0];
> + u8 val;
> +
> + val = *(bufptr + buf_index);
> + buf_index++;
> +
> + return val;
> +}
Why was buf_data declared as a u32 array if the only places that use it want a u8 array?
> +static u8 arasan_nand_read_byte(struct mtd_info *mtd)
> +{
> + struct nand_chip *chip = mtd->priv;
> + u32 size;
> + struct nand_onfi_params *p;
> +
> + if (buf_index == 0) {
> + p = &chip->onfi_params;
> + if (curr_cmd->cmd1 == NAND_CMD_READID)
> + size = 4;
> + else if (curr_cmd->cmd1 == NAND_CMD_PARAM)
> + size = sizeof(struct nand_onfi_params);
> + else if (curr_cmd->cmd1 == NAND_CMD_RNDOUT)
> + size = le16_to_cpu(p->ext_param_page_length) * 16;
> + else if (curr_cmd->cmd1 == NAND_CMD_GET_FEATURES)
> + size = 4;
> + else if (curr_cmd->cmd1 == NAND_CMD_STATUS)
> + return readb(&arasan_nand_base->flash_sts_reg);
> + else
> + size = 8;
> + chip->read_buf(mtd, (u8 *)&buf_data[0], size);
> + }
> +
> + return arasan_read_byte();
> +}
The distinction between arasan_nand_read_byte() and arasan_read_byte()
is not clear from the name. Isn't this whole file dealing with nand?
Why do you read_buf with varying fixed sizes rather than just reading a
byte from the dataport? If the answer is performance, that doesn't
make sense because read_byte is not used for high-performance paths.
> +static void arasan_nand_cmd_function(struct mtd_info *mtd, unsigned int command,
> + int column, int page_addr)
> +{
> + u32 i;
> + struct nand_chip *chip = mtd->priv;
> + struct arasan_nand_info *xnand = chip->priv;
> +
> + curr_cmd = NULL;
> + writel(0x4, &arasan_nand_base->intsts_enr);
Please define a symbolic constant for 0x4.
> +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 < sizeof(ecc_matrix)/sizeof(struct arasan_ecc_matrix);
i < ARRAY_SIZE(ecc_matrix)
> + i++) {
> + 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;
> + } else {
> + found = i;
> + }
You have "found = i" on both sides of the if/else...
> + }
> + }
> +
> + if (found) {
> + regval = ecc_matrix[i].eccaddr | (ecc_matrix[i].eccsize << 16) |
> + (ecc_matrix[i].slcmlc << 27);
Symbolic constants for 16 and 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:
> + bchmodeval = 0x0;
> + }
Again, it's pointless to have a default: that merely turns one bad
value into another (and bchmodeval has already been initialized to zero
anyway).
> +static int arasan_nand_init(struct nand_chip *nand_chip, int devnum)
> +{
> + struct arasan_nand_info *xnand;
"x"?
> + struct mtd_info *mtd;
> + u8 maf_id, dev_id;
> + int err = -1;
> + u8 get_feature[4];
> + u8 set_feature[4] = {0x08, 0x00, 0x00, 0x00};
> + int ondie_ecc_enabled = 0;
> + u32 timeout = ARASAN_NAND_POLL_TIMEOUT;
> + u32 i;
> +
> + xnand = calloc(1, sizeof(struct arasan_nand_info));
> + if (!xnand) {
> + printf("%s: failed to allocate\n", __func__);
> + return -1;
> + }
> +
> + xnand->nand_base = (void *)ARASAN_NAND_BASEADDR;
__iomem
> + mtd = &nand_info[0];
> + nand_chip->priv = xnand;
> + 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;
__iomem
Where do you even use IO_ADDR_R? And why can't the one place you use
IO_ADDR_W use buf_dataport instead?
> + /* 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;
> +
> + /* Send the command for reading device ID */
> + nand_chip->cmdfunc(mtd, NAND_CMD_RESET, -1, -1);
> + nand_chip->cmdfunc(mtd, NAND_CMD_READID, 0x00, -1);
Didn't nand_scan_ident() already read the ID?
> +
> + /* Read manufacturer and device IDs */
> + maf_id = nand_chip->read_byte(mtd);
> + dev_id = nand_chip->read_byte(mtd);
> +
> + if ((maf_id == 0x2c) && ((dev_id == 0xf1) ||
> + (dev_id == 0xa1) || (dev_id == 0xb1) ||
> + (dev_id == 0xaa) || (dev_id == 0xba) ||
> + (dev_id == 0xda) || (dev_id == 0xca) ||
> + (dev_id == 0xac) || (dev_id == 0xbc) ||
> + (dev_id == 0xdc) || (dev_id == 0xcc) ||
> + (dev_id == 0xa3) || (dev_id == 0xb3) ||
> + (dev_id == 0xd3) || (dev_id == 0xc3))) {
Where did this list come from? It feels like any logic that is based
on specific NAND chips, rather than the controller, should be in the
common NAND code.
Are these chips ONFI-compliant? If not, can you figure out the
property you're looking for by decoding the Micron id bytes rather than
using a list of devices?
> + nand_chip->cmdfunc(mtd, NAND_CMD_SET_FEATURES,
> + ONDIE_ECC_FEATURE_ADDR, -1);
> +
> + for (i = 0; i < 4; i++)
> + writeb(set_feature[i], nand_chip->IO_ADDR_W);
> +
> + while (!(readl(&arasan_nand_base->intsts_reg) &
> + ARASAN_NAND_INT_STS_XFR_CMPLT_MASK) && timeout) {
> + timeout--;
> + }
> +
> + if (!timeout) {
> + puts("ERROR:arasan_nand_init timedout:Xfer CMPLT\n");
> + goto fail;
> + }
> +
> + writel(readl(&arasan_nand_base->intsts_enr) |
> + ARASAN_NAND_INT_STS_XFR_CMPLT_MASK,
> + &arasan_nand_base->intsts_enr);
> + writel(readl(&arasan_nand_base->intsts_reg) |
> + ARASAN_NAND_INT_STS_XFR_CMPLT_MASK,
> + &arasan_nand_base->intsts_reg);
> +
> + nand_chip->cmdfunc(mtd, NAND_CMD_GET_FEATURES,
> + ONDIE_ECC_FEATURE_ADDR, -1);
> +
> + for (i = 0; i < 4; i++)
> + get_feature[i] = nand_chip->read_byte(mtd);
> +
> + if (get_feature[0] & 0x08) {
> + debug("%s: OnDie ECC flash\n", __func__);
> + ondie_ecc_enabled = 1;
> + } else {
> + printf("%s: Unable to detect OnDie ECC\n", __func__);
> + }
Please refactor this and elsewhere to reduce long functions.
> + }
> +
> + if (!ondie_ecc_enabled) {
> + 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;
> + }
> +
> + arasan_nand_ecc_init(mtd);
> +
> + if (nand_scan_tail(mtd)) {
> + printf("%s: nand_scan_tailfailed\n", __func__);
> + goto fail;
> + }
s/tailfailed/tail failed/
> + if (nand_register(devnum)) {
> + printf("Nand Register Fail\n");
> + goto fail;
> + }
> +
> + return 0;
> +fail:
> + kfree(xnand);
> + return err;
> +}
kfree() but calloc()? Is this code from Linux or not?
-Scott
More information about the U-Boot
mailing list