[U-Boot] [PATCH v2 1/2] nand/denali: Adding Denali NAND driver support
Scott Wood
scottwood at freescale.com
Tue Mar 4 01:03:11 CET 2014
On Fri, 2014-02-21 at 14:51 -0600, Chin Liang See wrote:
> To add the Denali NAND driver support into U-Boot. It required
> information such as register base address from configuration
> header file within include/configs folder.
>
> Signed-off-by: Chin Liang See <clsee at altera.com>
> Cc: Artem Bityutskiy <artem.bityutskiy at linux.intel.com>
> Cc: David Woodhouse <David.Woodhouse at intel.com>
> Cc: Brian Norris <computersforpeace at gmail.com>
> Cc: Scott Wood <scottwood at freescale.com>
> ---
> Changes for v2
> - Enable this driver support for SOCFPGA
> ---
> drivers/mtd/nand/Makefile | 1 +
> drivers/mtd/nand/denali_nand.c | 1166 ++++++++++++++++++++++++++++++++++++++++
> drivers/mtd/nand/denali_nand.h | 501 +++++++++++++++++
> 3 files changed, 1668 insertions(+)
> create mode 100644 drivers/mtd/nand/denali_nand.c
> create mode 100644 drivers/mtd/nand/denali_nand.h
>
> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
> index 02b149c..24e8218 100644
> --- a/drivers/mtd/nand/Makefile
> +++ b/drivers/mtd/nand/Makefile
> @@ -39,6 +39,7 @@ obj-$(CONFIG_NAND_ECC_BCH) += nand_bch.o
> obj-$(CONFIG_NAND_ATMEL) += atmel_nand.o
> obj-$(CONFIG_DRIVER_NAND_BFIN) += bfin_nand.o
> obj-$(CONFIG_NAND_DAVINCI) += davinci_nand.o
> +obj-$(CONFIG_NAND_DENALI) += denali_nand.o
> obj-$(CONFIG_NAND_FSL_ELBC) += fsl_elbc_nand.o
> obj-$(CONFIG_NAND_FSL_IFC) += fsl_ifc_nand.o
> obj-$(CONFIG_NAND_FSL_UPM) += fsl_upm.o
> diff --git a/drivers/mtd/nand/denali_nand.c b/drivers/mtd/nand/denali_nand.c
> new file mode 100644
> index 0000000..55246c9
> --- /dev/null
> +++ b/drivers/mtd/nand/denali_nand.c
It's "denali.c" in Linux -- why "denali_nand.c" here?
> @@ -0,0 +1,1166 @@
> +/*
> + * Copyright (C) 2013 Altera Corporation <www.altera.com>
> + * Copyright (C) 2009-2010, Intel Corporation and its suppliers.
> + *
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <nand.h>
> +#include <asm/errno.h>
> +#include <asm/io.h>
> +
> +#include "denali_nand.h"
> +
> +/* We define a module parameter that allows the user to override
> + * the hardware and decide what timing mode should be used.
> + */
> +#define NAND_DEFAULT_TIMINGS -1
A module parameter? In U-Boot?
Sharing code with Linux is fine, but try to edit out the stuff that's
irrelevant in U-Boot.
> +static struct denali_nand_info denali;
> +static int onfi_timing_mode = NAND_DEFAULT_TIMINGS;
> +
> +/* We define a macro here that combines all interrupts this driver uses into
> + * a single constant value, for convenience. */
> +#define DENALI_IRQ_ALL (INTR_STATUS__DMA_CMD_COMP | \
> + INTR_STATUS__ECC_TRANSACTION_DONE | \
> + INTR_STATUS__ECC_ERR | \
> + INTR_STATUS__PROGRAM_FAIL | \
> + INTR_STATUS__LOAD_COMP | \
> + INTR_STATUS__PROGRAM_COMP | \
> + INTR_STATUS__TIME_OUT | \
> + INTR_STATUS__ERASE_FAIL | \
> + INTR_STATUS__RST_COMP | \
> + INTR_STATUS__ERASE_COMP | \
> + INTR_STATUS__ECC_UNCOR_ERR | \
> + INTR_STATUS__INT_ACT | \
> + INTR_STATUS__LOCKED_BLK)
> +
> +/* indicates whether or not the internal value for the flash bank is
> + * valid or not */
> +#define CHIP_SELECT_INVALID -1
> +
> +#define SUPPORT_8BITECC 1
> +
> +/* This macro divides two integers and rounds fractional values up
> + * to the nearest integer value. */
> +#define CEIL_DIV(X, Y) (((X)%(Y)) ? ((X)/(Y)+1) : ((X)/(Y)))
> +
> +/* These constants are defined by the driver to enable common driver
> + * configuration options. */
> +#define SPARE_ACCESS 0x41
> +#define MAIN_ACCESS 0x42
> +#define MAIN_SPARE_ACCESS 0x43
> +
> +#define DENALI_UNLOCK_START 0x10
> +#define DENALI_UNLOCK_END 0x11
> +#define DENALI_LOCK 0x21
> +#define DENALI_LOCK_TIGHT 0x31
> +#define DENALI_BUFFER_LOAD 0x60
> +#define DENALI_BUFFER_WRITE 0x62
> +
> +#define DENALI_READ 0
> +#define DENALI_WRITE 0x100
> +
> +/* types of device accesses. We can issue commands and get status */
> +#define COMMAND_CYCLE 0
> +#define ADDR_CYCLE 1
> +#define STATUS_CYCLE 2
> +
> +/* this is a helper macro that allows us to
> + * format the bank into the proper bits for the controller */
> +#define BANK(x) ((x) << 24)
> +
> +/* Interrupts are cleared by writing a 1 to the appropriate status bit */
> +static inline void clear_interrupt(uint32_t irq_mask)
> +{
> + uint32_t intr_status_reg = 0;
> + intr_status_reg = INTR_STATUS(denali.flash_bank);
> + __raw_writel(irq_mask, denali.flash_reg + intr_status_reg);
> +}
Why are you using raw I/O accessors? The Linux driver doesn't do this.
> +static uint32_t read_interrupt_status(void)
> +{
> + uint32_t intr_status_reg = 0;
> + intr_status_reg = INTR_STATUS(denali.flash_bank);
> + return __raw_readl(denali.flash_reg + intr_status_reg);
> +}
> +
> +static void clear_interrupts(void)
> +{
> + uint32_t status = 0x0;
> + status = read_interrupt_status();
> + clear_interrupt(status);
> + denali.irq_status = 0x0;
> +}
> +
> +static void denali_irq_enable(uint32_t int_mask)
> +{
> + int i;
> + for (i = 0; i < denali.max_banks; ++i)
> + __raw_writel(int_mask, denali.flash_reg + INTR_EN(i));
> +}
> +
> +static uint32_t wait_for_irq(uint32_t irq_mask)
> +{
> + unsigned long comp_res = 1000;
> + uint32_t intr_status = 0;
> +
> + do {
> + intr_status = read_interrupt_status() & DENALI_IRQ_ALL;
> + if (intr_status & irq_mask) {
> + denali.irq_status &= ~irq_mask;
> + /* our interrupt was detected */
> + break;
> + }
> + udelay(1);
> + comp_res--;
> + } while (comp_res != 0);
This looks like a much shorter timeout than Linux uses (1000us versus
1000ms). Though FWIW the Linux timeout code looks buggy.
Also, comp_res is a very odd name for a timeout variable.
> +/* Certain operations for the denali NAND controller use
> + * an indexed mode to read/write data. The operation is
> + * performed by writing the address value of the command
> + * to the device memory followed by the data. This function
> + * abstracts this common operation.
> +*/
> +static void index_addr(uint32_t address, uint32_t data)
> +{
> + __raw_writel(address, denali.flash_mem);
> + __raw_writel(data, denali.flash_mem + 0x10);
> +}
What is 0x10? No magic numbers, please.
> +
> +/* Perform an indexed read of the device */
> +static void index_addr_read_data(uint32_t address, uint32_t *pdata)
> +{
> + __raw_writel(address, denali.flash_mem);
> + *pdata = __raw_readl(denali.flash_mem + 0x10);
> +}
> +
> +/* We need to buffer some data for some of the NAND core routines.
> + * The operations manage buffering that data. */
> +static void reset_buf(void)
> +{
> + denali.buf.head = denali.buf.tail = 0;
> +}
> +
> +static void write_byte_to_buf(uint8_t byte)
> +{
> + BUG_ON(denali.buf.tail >= sizeof(denali.buf.buf));
> + denali.buf.buf[denali.buf.tail++] = byte;
> +}
> +
> +/* resets a specific device connected to the core */
> +static void reset_bank(void)
> +{
> + uint32_t irq_status = 0;
> + uint32_t irq_mask = INTR_STATUS__RST_COMP |
> + INTR_STATUS__TIME_OUT;
> +
> + clear_interrupts();
> +
> + __raw_writel(1 << denali.flash_bank, denali.flash_reg + DEVICE_RESET);
> +
> + irq_status = wait_for_irq(irq_mask);
> + if (irq_status & INTR_STATUS__TIME_OUT)
> + debug(KERN_ERR "reset bank failed.\n");
> +}
> +
> +/* Reset the flash controller */
> +static uint16_t denali_nand_reset(void)
> +{
> + uint32_t i;
> +
> + for (i = 0 ; i < denali.max_banks; i++)
> + __raw_writel(INTR_STATUS__RST_COMP | INTR_STATUS__TIME_OUT,
> + denali.flash_reg + INTR_STATUS(i));
> +
> + for (i = 0 ; i < denali.max_banks; i++) {
> + __raw_writel(1 << i, denali.flash_reg + DEVICE_RESET);
> + while (!(__raw_readl(denali.flash_reg + INTR_STATUS(i)) &
> + (INTR_STATUS__RST_COMP | INTR_STATUS__TIME_OUT)))
> + if (__raw_readl(denali.flash_reg + INTR_STATUS(i)) &
> + INTR_STATUS__TIME_OUT)
> + debug(KERN_DEBUG "NAND Reset operation "
> + "timed out on bank %d\n", i);
> + }
> +
> + for (i = 0; i < denali.max_banks; i++)
> + __raw_writel(INTR_STATUS__RST_COMP | INTR_STATUS__TIME_OUT,
> + denali.flash_reg + INTR_STATUS(i));
> +
> + return PASS;
> +}
> +
> +/* this routine calculates the ONFI timing values for a given mode and
> + * programs the clocking register accordingly. The mode is determined by
> + * the get_onfi_nand_para routine.
> + */
> +static void nand_onfi_timing_set(uint16_t mode)
> +{
> + uint16_t Trea[6] = {40, 30, 25, 20, 20, 16};
> + uint16_t Trp[6] = {50, 25, 17, 15, 12, 10};
> + uint16_t Treh[6] = {30, 15, 15, 10, 10, 7};
> + uint16_t Trc[6] = {100, 50, 35, 30, 25, 20};
> + uint16_t Trhoh[6] = {0, 15, 15, 15, 15, 15};
> + uint16_t Trloh[6] = {0, 0, 0, 0, 5, 5};
> + uint16_t Tcea[6] = {100, 45, 30, 25, 25, 25};
> + uint16_t Tadl[6] = {200, 100, 100, 100, 70, 70};
> + uint16_t Trhw[6] = {200, 100, 100, 100, 100, 100};
> + uint16_t Trhz[6] = {200, 100, 100, 100, 100, 100};
> + uint16_t Twhr[6] = {120, 80, 80, 60, 60, 60};
> + uint16_t Tcs[6] = {70, 35, 25, 25, 20, 15};
> +
> + uint16_t TclsRising = 1;
> + uint16_t data_invalid_rhoh, data_invalid_rloh, data_invalid;
> + uint16_t dv_window = 0;
> + uint16_t en_lo, en_hi;
> + uint16_t acc_clks;
> + uint16_t addr_2_data, re_2_we, re_2_re, we_2_re, cs_cnt;
> +
> + en_lo = CEIL_DIV(Trp[mode], CLK_X);
> + en_hi = CEIL_DIV(Treh[mode], CLK_X);
> +#if ONFI_BLOOM_TIME
> + if ((en_hi * CLK_X) < (Treh[mode] + 2))
> + en_hi++;
> +#endif
> +
> + if ((en_lo + en_hi) * CLK_X < Trc[mode])
> + en_lo += CEIL_DIV((Trc[mode] - (en_lo + en_hi) * CLK_X), CLK_X);
> +
> + if ((en_lo + en_hi) < CLK_MULTI)
> + en_lo += CLK_MULTI - en_lo - en_hi;
> +
> + while (dv_window < 8) {
> + data_invalid_rhoh = en_lo * CLK_X + Trhoh[mode];
> +
> + data_invalid_rloh = (en_lo + en_hi) * CLK_X + Trloh[mode];
> +
> + data_invalid =
> + data_invalid_rhoh <
> + data_invalid_rloh ? data_invalid_rhoh : data_invalid_rloh;
> +
> + dv_window = data_invalid - Trea[mode];
> +
> + if (dv_window < 8)
> + en_lo++;
> + }
> +
> + acc_clks = CEIL_DIV(Trea[mode], CLK_X);
> +
> + while (((acc_clks * CLK_X) - Trea[mode]) < 3)
> + acc_clks++;
> +
> + if ((data_invalid - acc_clks * CLK_X) < 2)
> + debug(KERN_WARNING "%s, Line %d: Warning!\n",
> + __FILE__, __LINE__);
> +
> + addr_2_data = CEIL_DIV(Tadl[mode], CLK_X);
> + re_2_we = CEIL_DIV(Trhw[mode], CLK_X);
> + re_2_re = CEIL_DIV(Trhz[mode], CLK_X);
> + we_2_re = CEIL_DIV(Twhr[mode], CLK_X);
> + cs_cnt = CEIL_DIV((Tcs[mode] - Trp[mode]), CLK_X);
> + if (!TclsRising)
> + cs_cnt = CEIL_DIV(Tcs[mode], CLK_X);
> + if (cs_cnt == 0)
> + cs_cnt = 1;
> +
> + if (Tcea[mode]) {
> + while (((cs_cnt * CLK_X) + Trea[mode]) < Tcea[mode])
> + cs_cnt++;
> + }
> +
> +#if MODE5_WORKAROUND
> + if (mode == 5)
> + acc_clks = 5;
> +#endif
> +
> + /* Sighting 3462430: Temporary hack for MT29F128G08CJABAWP:B */
> + if ((__raw_readl(denali.flash_reg + MANUFACTURER_ID) == 0) &&
> + (__raw_readl(denali.flash_reg + DEVICE_ID) == 0x88))
> + acc_clks = 6;
> +
> + __raw_writel(acc_clks, denali.flash_reg + ACC_CLKS);
> + __raw_writel(re_2_we, denali.flash_reg + RE_2_WE);
> + __raw_writel(re_2_re, denali.flash_reg + RE_2_RE);
> + __raw_writel(we_2_re, denali.flash_reg + WE_2_RE);
> + __raw_writel(addr_2_data, denali.flash_reg + ADDR_2_DATA);
> + __raw_writel(en_lo, denali.flash_reg + RDWR_EN_LO_CNT);
> + __raw_writel(en_hi, denali.flash_reg + RDWR_EN_HI_CNT);
> + __raw_writel(cs_cnt, denali.flash_reg + CS_SETUP_CNT);
> +}
> +
> +/* queries the NAND device to see what ONFI modes it supports. */
> +static uint16_t get_onfi_nand_para(void)
> +{
> + int i;
> + /* we needn't to do a reset here because driver has already
> + * reset all the banks before
> + * */
> + if (!(__raw_readl(denali.flash_reg + ONFI_TIMING_MODE) &
> + ONFI_TIMING_MODE__VALUE))
> + return FAIL;
Don't align continuation lines with an if/for body -- it makes it harder
to see where one ends and the other begins.
Why PASS/FAIL rather than normal "0 on success, negative error code on
error"? Why uint16_t?
> + for (i = 5; i > 0; i--) {
> + if (__raw_readl(denali.flash_reg + ONFI_TIMING_MODE) &
> + (0x01 << i))
> + break;
> + }
>
> + nand_onfi_timing_set(i);
> +
> + /* By now, all the ONFI devices we know support the page cache */
> + /* rw feature. So here we enable the pipeline_rw_ahead feature */
> + /* __raw_writel(1, denali.flash_reg + CACHE_WRITE_ENABLE); */
> + /* __raw_writel(1, denali.flash_reg + CACHE_READ_ENABLE); */
Don't add commented-out-code.
> +/* determines how many NAND chips are connected to the controller. Note for
> + * Intel CE4100 devices we don't support more than one device.
> + */
> +static void find_valid_banks(void)
> +{
> + uint32_t id[denali.max_banks];
> + int i;
> +
> + denali.total_used_banks = 1;
> + for (i = 0; i < denali.max_banks; i++) {
> + index_addr((uint32_t)(MODE_11 | (i << 24) | 0), 0x90);
> + index_addr((uint32_t)(MODE_11 | (i << 24) | 1), 0);
> + index_addr_read_data((uint32_t)(MODE_11 | (i << 24) | 2),
> + &id[i]);
> +
> + if (i == 0) {
> + if (!(id[i] & 0x0ff))
> + break; /* WTF? */
I'm not sure that this is the most helpful comment that could go here.
> +static void detect_partition_feature(void)
> +{
> + /* For MRST platform, denali.fwblks represent the
> + * number of blocks firmware is taken,
> + * FW is in protect partition and MTD driver has no
> + * permission to access it. So let driver know how many
> + * blocks it can't touch.
> + * */
> + if (__raw_readl(denali.flash_reg + FEATURES) & FEATURES__PARTITION) {
> + if ((__raw_readl(denali.flash_reg + PERM_SRC_ID(1)) &
> + PERM_SRC_ID__SRCID) == SPECTRA_PARTITION_ID) {
> + denali.fwblks =
> + ((__raw_readl(denali.flash_reg + MIN_MAX_BANK(1)) &
> + MIN_MAX_BANK__MIN_VALUE) *
> + denali.blksperchip)
> + +
> + (__raw_readl(denali.flash_reg + MIN_BLK_ADDR(1)) &
> + MIN_BLK_ADDR__VALUE);
> + } else
> + denali.fwblks = SPECTRA_START_BLOCK;
> + } else
> + denali.fwblks = SPECTRA_START_BLOCK;
If braces are needed on one side of if/else, use on both sides.
> +static void denali_set_intr_modes(uint16_t INT_ENABLE)
> +{
> + if (INT_ENABLE)
> + __raw_writel(1, denali.flash_reg + GLOBAL_INT_ENABLE);
> + else
> + __raw_writel(0, denali.flash_reg + GLOBAL_INT_ENABLE);
> +}
CAPS are for macros, not function parameters.
Why not just require the caller to pass in 0/1 rather than 0/non-0 and
write that directly to the register?
> +/* helper function that simply writes a buffer to the flash */
> +static int write_data_to_flash_mem(const uint8_t *buf,
> + int len)
There's no point in a continuation line if you indent so far that you're
beyond where the previous line ended. If the above doesn't violate 80
columns, then you could just as well do it on one line.
> +{
> + uint32_t i = 0, *buf32;
> +
> + /* verify that the len is a multiple of 4. see comment in
> + * read_data_from_flash_mem() */
> + BUG_ON((len % 4) != 0);
> +
> + /* write the data to the flash memory */
> + buf32 = (uint32_t *)buf;
> + for (i = 0; i < len / 4; i++)
> + __raw_writel(*buf32++, denali.flash_mem + 0x10);
This violates C99 aliasing rules.
> + return i*4; /* intent is to return the number of bytes read */
> +}
> +
> +static void denali_mode_main_access(void)
> +{
> + uint32_t addr, cmd;
> + addr = BANK(denali.flash_bank) | denali.page;
> + cmd = MODE_10 | addr;
> + index_addr((uint32_t)cmd, MAIN_ACCESS);
Unnecessary cast.
> +/* raw include ECC value and all the spare area */
> +static int denali_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
> + uint8_t *buf, int oob_required, int page)
> +{
> + uint32_t irq_status, irq_mask = INTR_STATUS__DMA_CMD_COMP;
> +
> + debug("denali_read_page_raw at page %08x\n", page);
> + if (denali.page != page) {
> + debug("Missing NAND_CMD_READ0 command\n");
> + return -EIO;
> + }
> +
> + if (oob_required)
> + /* switch to main + spare access */
> + denali_mode_main_spare_access();
> + else
> + /* switch to main access only */
> + denali_mode_main_access();
> +
> + /* setting up the DMA where ecc_enable is false */
> + irq_status = denali_dma_configuration(DENALI_READ, true, irq_mask,
> + oob_required);
> +
> + /* if timeout happen, error out */
> + if (!(irq_status & INTR_STATUS__DMA_CMD_COMP)) {
> + debug("DMA timeout for denali_read_page_raw\n");
> + return -EIO;
> + }
> +
> + /* splitting the content to destination buffer holder */
> + memcpy(chip->oob_poi, (const void *)(denali.buf.dma_buf +
> + mtd->writesize), mtd->oobsize);
> + memcpy(buf, (const void *)denali.buf.dma_buf, mtd->writesize);
Unnecessary casts.
> +static void denali_erase(struct mtd_info *mtd, int page)
> +{
> + uint32_t cmd = 0x0, irq_status = 0;
> +
> + debug("denali_erase at page %08x\n", page);
> +
> + /* clear interrupts */
> + clear_interrupts();
> +
> + /* setup page read request for access type */
> + cmd = MODE_10 | BANK(denali.flash_bank) | page;
> + index_addr((uint32_t)cmd, 0x1);
> +
> + /* wait for erase to complete or failure to occur */
> + irq_status = wait_for_irq(INTR_STATUS__ERASE_COMP |
> + INTR_STATUS__ERASE_FAIL);
> +
> + if (irq_status & INTR_STATUS__ERASE_FAIL ||
> + irq_status & INTR_STATUS__LOCKED_BLK)
> + denali.status = NAND_STATUS_FAIL;
> + else
> + denali.status = PASS;
> +}
So PASS/FAIL are supposed to be in the same numberspace as
NAND_STATUS_foo? Why use a separate FAIL elsewhere, then?
> +static void denali_cmdfunc(struct mtd_info *mtd, unsigned int cmd, int col,
> + int page)
> +{
> + uint32_t addr;
> +
> + switch (cmd) {
> + case NAND_CMD_PAGEPROG:
> + break;
> + case NAND_CMD_STATUS:
> + addr = (uint32_t)MODE_11 | BANK(denali.flash_bank);
> + index_addr((uint32_t)addr | 0, cmd);
More unnecessary casts...
> +/* stubs for ECC functions not used by the NAND core */
> +static int denali_ecc_calculate(struct mtd_info *mtd, const uint8_t *data,
> + uint8_t *ecc_code)
> +{
> + debug("Should not be called as ECC handled by hardware\n");
> + BUG();
> + return -EIO;
> +}
> +
> +static int denali_ecc_correct(struct mtd_info *mtd, uint8_t *data,
> + uint8_t *read_ecc, uint8_t *calc_ecc)
> +{
> + debug("Should not be called as ECC handled by hardware\n");
> + BUG();
> + return -EIO;
> +}
> +
> +static void denali_ecc_hwctl(struct mtd_info *mtd, int mode)
> +{
> + debug("Should not be called as ECC handled by hardware\n");
> + BUG();
> +}
These debug() messages seem superfluous given the BUG() will identify
the line in the code. debug() is not for commenting code.
> +/* ffsdefs.h */
> +#define CLEAR 0 /*use this to clear a field instead of "fail"*/
> +#define SET 1 /*use this to set a field instead of "pass"*/
> +#define FAIL 1 /*failed flag*/
> +#define PASS 0 /*success flag*/
> +#define ERR -1 /*error flag*/
What's the difference between FAIL and ERR?
-Scott
More information about the U-Boot
mailing list