Re: [PATCH 3/4] mtd: nand: Add support for Evatronix NANDFLASH-CTRL

From: Boris Brezillon
Date: Fri Jun 03 2016 - 10:59:28 EST

On Thu, 2 Jun 2016 09:48:32 +0200
Ricard Wanderlof <ricard.wanderlof@xxxxxxxx> wrote:

> Driver for the Evatronix NANDFLASH-CTRL NAND flash controller IP. This
> controller is used in the Axis ARTPEC-6 SoC.
>
> The driver supports BCH ECC using the controller's hardware, but there is
> also an option to use software BCH ECC. However, the ECC layouts are not
> compatible so it's not possible to mix them. The main advantage to using
> software ECC is that there are more OOB bytes free, as the hardware is
> slightly wasteful on OOB space.
>
> BCH ECC from 4 to 32 bits over 256, 512 or 1024 byte ECC blocks is supported.
>
> Only large-page flash chips are supported, using 4 or 5 address cycles.
>
> The driver has been extensively tested using hardware ECC on 2 Mbit flash chips,
> with 8 bit ECC over 512 bytes ECC blocks.
>
> Signed-off-by: Ricard Wanderlof <ricardw@xxxxxxxx>
> ---
> drivers/mtd/nand/Kconfig | 6 +
> drivers/mtd/nand/Makefile | 1 +
> drivers/mtd/nand/evatronix_nand.c | 1909 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 1916 insertions(+)
> create mode 100644 drivers/mtd/nand/evatronix_nand.c

Please run checkpatch.pl and fix all the ERRORS and WARNINGS.

>
> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> index f05e0e9..30fba73 100644
> --- a/drivers/mtd/nand/Kconfig
> +++ b/drivers/mtd/nand/Kconfig
> @@ -295,6 +295,12 @@ config MTD_NAND_DOCG4
> by the block containing the saftl partition table. This is probably
> typical.
>
> +config MTD_NAND_EVATRONIX
> + tristate "Enable Evatronix NANDFLASH-CTRL driver"
> + help
> + NAND hardware driver for Evatronix NANDFLASH-CTRL
> + NAND flash controller.
> +
> config MTD_NAND_SHARPSL
> tristate "Support for NAND Flash on Sharp SL Series (C7xx + others)"
> depends on ARCH_PXA
> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
> index f553353..ac89b12 100644
> --- a/drivers/mtd/nand/Makefile
> +++ b/drivers/mtd/nand/Makefile
> @@ -19,6 +19,7 @@ obj-$(CONFIG_MTD_NAND_S3C2410) += s3c2410.o
> obj-$(CONFIG_MTD_NAND_DAVINCI) += davinci_nand.o
> obj-$(CONFIG_MTD_NAND_DISKONCHIP) += diskonchip.o
> obj-$(CONFIG_MTD_NAND_DOCG4) += docg4.o
> +obj-$(CONFIG_MTD_NAND_EVATRONIX) += evatronix_nand.o
> obj-$(CONFIG_MTD_NAND_FSMC) += fsmc_nand.o
> obj-$(CONFIG_MTD_NAND_SHARPSL) += sharpsl.o
> obj-$(CONFIG_MTD_NAND_NANDSIM) += nandsim.o
> diff --git a/drivers/mtd/nand/evatronix_nand.c b/drivers/mtd/nand/evatronix_nand.c
> new file mode 100644
> index 0000000..94eb582
> --- /dev/null
> +++ b/drivers/mtd/nand/evatronix_nand.c
> @@ -0,0 +1,1909 @@
> +/*
> + * evatronix_nand.c - NAND Flash Driver for Evatronix NANDFLASH-CTRL
> + * NAND Flash Controller IP.
> + *
> + * Intended to handle one NFC, with up to two connected NAND flash chips,
> + * one per bank.
> + *
> + * This implementation has been designed against Rev 1.15 and Rev 1.16 of the
> + * NANDFLASH-CTRL Design Specification.
> + * Note that Rev 1.15 specifies up to 8 chip selects, whereas Rev 1.16
> + * only specifies one. We keep the definitions for the multiple chip
> + * selects though for future reference.
> + *
> + * The corresponding IP version is NANDFLASH-CTRL-DES-6V09H02RE08 .
> + *
> + * Copyright (c) 2016 Axis Communication AB, Lund, Sweden.
> + * Portions Copyright (c) 2010 ST Microelectronics
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <asm/dma.h>
> +#include <linux/bitops.h> /* for ffs() */
> +#include <linux/io.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +#include <linux/slab.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/mtd/nand.h>
> +#include <linux/mtd/concat.h>
> +#include <linux/mtd/partitions.h>
> +#include <linux/version.h>

You seem to include a lot of things, and even asm headers. Please make
sure you really need them.

> +
> +/* Driver configuration */
> +
> +/* Some of this could potentially be moved to DT, but it represents stuff
> + * that is either untested, only used for debugging, or things we really
> + * don't want anyone to change, so we keep it here until a clear use case
> + * emerges.
> + */
> +
> +#undef NFC_DMA64BIT /* NFC hardware support for 64-bit DMA transfers */
> +
> +#undef POLLED_XFERS /* Use polled rather than interrupt based transfers */
> +
> +#undef CLEAR_DMA_BUF_AFTER_WRITE /* Useful for debugging */

Then simply drop the code in those sections and add it back when it's
been tested.

> +
> +/* DMA buffer for page transfers. */
> +#define DMA_BUF_SIZE (8192 + 640) /* main + spare for 8k page flash */

This should clearly be dynamic.

> +
> +/* # bytes into the OOB we put our ECC */
> +#define ECC_OFFSET 2
> +
> +/* Number of bytes that we read using READID command.
> + * When reading IDs the IP requires us set up the number of bytes to read
> + * prior to executing the operation, whereas the NAND subsystem would rather
> + * like us to be able to read one byte at a time from the chip. So we fake
> + * this by reading a set number of ID bytes, and then let the NAND subsystem
> + * read from our DMA buffer.
> + */
> +#define READID_LENGTH 8
> +
> +/* Debugging */
> +
> +#define MTD_TRACE(FORMAT, ...) pr_debug("%s: " FORMAT, __func__, ## __VA_ARGS__)

Hm, I'm not a big fan of those custom pr_debug() macros, but if you
really wan to keep it you shouldn't prefix it with MTD_.

Reading at the code I see a lot of MTD_TRACE() calls, while I'm not
against debug traces, it seems to me that you've kept traces you used
while developing/debugging your implementation. Can you clean it up and
keep only the relevant ones.

> +
> +/* Register offsets for Evatronix NANDFLASH-CTRL IP */
> +
> +/* Register field shift values and masks are interespersed as it makes
> + * them easier to locate.
> + *
> + * We use shift values rather than direct masks (e.g. 0x0000d000), as the
> + * hardware manual lists the bit number, making the definitions below
> + * easier to verify against the manual.
> + *
> + * All (known) registers are here, but we only put in the bit fields
> + * for the fields we need.
> + *
> + * We try to be consistent regarding _SIZE/_MASK/_value macros so as to
> + * get a consistent layout here, except for trivial cases where there is
> + * only a single bit or field in a register at bit offset 0.
> + */
> +
> +#define COMMAND_REG 0x00
> +/* The masks reflect the input data to the MAKE_COMMAND macro, rather than
> + * the bits in the register itself. These macros are not intended to be
> + * used by the user, who should use the MAKE_COMMAND et al macros.
> + */
> +#define _CMD_SEQ_SHIFT 0
> +#define _INPUT_SEL_SHIFT 6
> +#define _DATA_SEL_SHIFT 7
> +#define _CMD_0_SHIFT 8
> +#define _CMD_1_3_SHIFT 16
> +#define _CMD_2_SHIFT 24
> +
> +#define _CMD_SEQ_MASK 0x3f
> +#define _INPUT_SEL_MASK 1
> +#define _DATA_SEL_MASK 1
> +#define _CMD_MASK 0xff /* for all CMD_foo */
> +
> +#define MAKE_COMMAND(CMD_SEQ, INPUT_SEL, DATA_SEL, CMD_0, CMD_1_3, CMD_2) \
> + ((((CMD_SEQ) & _CMD_SEQ_MASK) << _CMD_SEQ_SHIFT) | \
> + (((INPUT_SEL) & _INPUT_SEL_MASK) << _INPUT_SEL_SHIFT) | \
> + (((DATA_SEL) & _DATA_SEL_MASK) << _DATA_SEL_SHIFT) | \
> + (((CMD_0) & _CMD_MASK) << _CMD_0_SHIFT) | \
> + (((CMD_1_3) & _CMD_MASK) << _CMD_1_3_SHIFT) | \
> + (((CMD_2) & _CMD_MASK) << _CMD_2_SHIFT))
> +
> +#define INPUT_SEL_SIU 0
> +#define INPUT_SEL_DMA 1
> +#define DATA_SEL_FIFO 0
> +#define DATA_SEL_DATA_REG 1
> +
> +#define CONTROL_REG 0x04
> +#define CONTROL_BLOCK_SIZE_32 (0 << 6)
> +#define CONTROL_BLOCK_SIZE_64 (1 << 6)
> +#define CONTROL_BLOCK_SIZE_128 (2 << 6)
> +#define CONTROL_BLOCK_SIZE_256 (3 << 6)
> +#define CONTROL_BLOCK_SIZE(SIZE) ((ffs(SIZE) - 6) << 6)
> +#define CONTROL_ECC_EN (1 << 5)
> +#define CONTROL_INT_EN (1 << 4)
> +#define CONTROL_ECC_BLOCK_SIZE_256 (0 << 1)
> +#define CONTROL_ECC_BLOCK_SIZE_512 (1 << 1)
> +#define CONTROL_ECC_BLOCK_SIZE_1024 (2 << 1)
> +#define CONTROL_ECC_BLOCK_SIZE(SIZE) ((ffs(SIZE) - 9) << 1)
> +#define STATUS_REG 0x08
> +#define STATUS_MEM_ST(CS) (1 << (CS))
> +#define STATUS_CTRL_STAT (1 << 8)
> +#define STATUS_MASK_REG 0x0C
> +#define STATE_MASK_SHIFT 0
> +#define STATUS_MASK_STATE_MASK(MASK) (((MASK) & 0xff) << STATE_MASK_SHIFT)
> +#define ERROR_MASK_SHIFT 8
> +#define STATUS_MASK_ERROR_MASK(MASK) (((MASK) & 0xff) << ERROR_MASK_SHIFT)
> +#define INT_MASK_REG 0x10
> +#define INT_MASK_ECC_INT_EN(CS) (1 << (24 + (CS)))
> +#define INT_MASK_STAT_ERR_INT_EN(CS) (1 << (16 + (CS)))
> +#define INT_MASK_MEM_RDY_INT_EN(CS) (1 << (8 + (CS)))
> +#define INT_MASK_DMA_INT_EN (1 << 3)
> +#define INT_MASK_DATA_REG_EN (1 << 2)
> +#define INT_MASK_CMD_END_INT_EN (1 << 1)
> +#define INT_STATUS_REG 0x14
> +#define INT_STATUS_ECC_INT_FL(CS) (1 << (24 + (CS)))
> +#define INT_STATUS_STAT_ERR_INT_FL(CS) (1 << (16 + (CS)))
> +#define INT_STATUS_MEM_RDY_INT_FL(CS) (1 << (8 + (CS)))
> +#define INT_STATUS_DMA_INT_FL (1 << 3)
> +#define INT_STATUS_DATA_REG_FL (1 << 2)
> +#define INT_STATUS_CMD_END_INT_FL (1 << 1)
> +#define ECC_CTRL_REG 0x18
> +#define ECC_CTRL_ECC_CAP_2 (0 << 0)
> +#define ECC_CTRL_ECC_CAP_4 (1 << 0)
> +#define ECC_CTRL_ECC_CAP_8 (2 << 0)
> +#define ECC_CTRL_ECC_CAP_16 (3 << 0)
> +#define ECC_CTRL_ECC_CAP_24 (4 << 0)
> +#define ECC_CTRL_ECC_CAP_32 (5 << 0)
> +#define ECC_CTRL_ECC_CAP(B) ((B) < 24 ? ffs(B) - 2 : (B) / 6)
> +/* # ECC corrections that are acceptable during read before setting OVER flag */
> +#define ECC_CTRL_ECC_THRESHOLD(VAL) (((VAL) & 0x3f) << 8)
> +#define ECC_OFFSET_REG 0x1C
> +#define ECC_STAT_REG 0x20
> +/* Correctable error flag(s) */
> +#define ECC_STAT_ERROR(CS) (1 << (0 + (CS)))
> +/* Uncorrectable error flag(s) */
> +#define ECC_STAT_UNC(CS) (1 << (8 + (CS)))
> +/* Acceptable errors level overflow flag(s) */
> +#define ECC_STAT_OVER(CS) (1 << (16 + (CS)))
> +#define ADDR0_COL_REG 0x24
> +#define ADDR0_ROW_REG 0x28
> +#define ADDR1_COL_REG 0x2C
> +#define ADDR1_ROW_REG 0x30
> +#define PROTECT_REG 0x34
> +#define FIFO_DATA_REG 0x38
> +#define DATA_REG_REG 0x3C
> +#define DATA_REG_SIZE_REG 0x40
> +#define DATA_REG_SIZE_DATA_REG_SIZE(SIZE) (((SIZE) - 1) & 3)
> +#define DEV0_PTR_REG 0x44
> +#define DEV1_PTR_REG 0x48
> +#define DEV2_PTR_REG 0x4C
> +#define DEV3_PTR_REG 0x50
> +#define DEV4_PTR_REG 0x54
> +#define DEV5_PTR_REG 0x58
> +#define DEV6_PTR_REG 0x5C
> +#define DEV7_PTR_REG 0x60
> +#define DMA_ADDR_L_REG 0x64
> +#define DMA_ADDR_H_REG 0x68
> +#define DMA_CNT_REG 0x6C
> +#define DMA_CTRL_REG 0x70
> +#define DMA_CTRL_DMA_START (1 << 7) /* start on command */
> +#define DMA_CTRL_DMA_MODE_SG (1 << 5) /* scatter/gather mode */
> +#define DMA_CTRL_DMA_BURST_I_P_4 (0 << 2) /* incr. precise burst */
> +#define DMA_CTRL_DMA_BURST_S_P_16 (1 << 2) /* stream precise burst */
> +#define DMA_CTRL_DMA_BURST_SINGLE (2 << 2) /* single transfer */
> +#define DMA_CTRL_DMA_BURST_UNSPEC (3 << 2) /* burst of unspec. length */
> +#define DMA_CTRL_DMA_BURST_I_P_8 (4 << 2) /* incr. precise burst */
> +#define DMA_CTRL_DMA_BURST_I_P_16 (5 << 2) /* incr. precise burst */
> +#define DMA_CTRL_ERR_FLAG (1 << 1) /* read only */
> +#define DMA_CTRL_DMA_READY (1 << 0) /* read only */
> +#define BBM_CTRL_REG 0x74
> +#define MEM_CTRL_REG 0x80
> +#define MEM_CTRL_MEM_CE(CE) (((CE) & 7) << 0)
> +#define MEM_CTRL_BANK_SEL(BANK) (((BANK) & 7) << 16)
> +#define MEM_CTRL_MEM0_WR BIT(8)
> +#define DATA_SIZE_REG 0x84
> +#define TIMINGS_ASYN_REG 0x88
> +#define TIMINGS_SYN_REG 0x8C
> +#define TIME_SEQ_0_REG 0x90
> +#define TIME_SEQ_1_REG 0x94
> +#define TIME_GEN_SEQ_0_REG 0x98
> +#define TIME_GEN_SEQ_1_REG 0x9C
> +#define TIME_GEN_SEQ_2_REG 0xA0
> +#define FIFO_INIT_REG 0xB0
> +#define FIFO_INIT_FIFO_INIT 1 /* Flush FIFO */
> +#define FIFO_STATE_REG 0xB4
> +#define FIFO_STATE_DF_W_EMPTY (1 << 7)
> +#define FIFO_STATE_DF_R_FULL (1 << 6)
> +#define FIFO_STATE_CF_ACCPT_W (1 << 5)
> +#define FIFO_STATE_CF_ACCPT_R (1 << 4)
> +#define FIFO_STATE_CF_FULL (1 << 3)
> +#define FIFO_STATE_CF_EMPTY (1 << 2)
> +#define FIFO_STATE_DF_W_FULL (1 << 1)
> +#define FIFO_STATE_DF_R_EMPTY (1 << 0)
> +#define GEN_SEQ_CTRL_REG 0xB8 /* aka GENERIC_SEQ_CTRL */
> +#define _CMD0_EN_SHIFT 0
> +#define _CMD1_EN_SHIFT 1
> +#define _CMD2_EN_SHIFT 2
> +#define _CMD3_EN_SHIFT 3
> +#define _COL_A0_SHIFT 4
> +#define _COL_A1_SHIFT 6
> +#define _ROW_A0_SHIFT 8
> +#define _ROW_A1_SHIFT 10
> +#define _DATA_EN_SHIFT 12
> +#define _DELAY_EN_SHIFT 13
> +#define _IMD_SEQ_SHIFT 15
> +#define _CMD3_SHIFT 16
> +#define ECC_CNT_REG 0x14C
> +#define ECC_CNT_ERR_LVL_MASK 0x3F
> +
> +#define _CMD0_EN_MASK 1
> +#define _CMD1_EN_MASK 1
> +#define _CMD2_EN_MASK 1
> +#define _CMD3_EN_MASK 1
> +#define _COL_A0_MASK 3
> +#define _COL_A1_MASK 3
> +#define _ROW_A0_MASK 3
> +#define _ROW_A1_MASK 3
> +#define _DATA_EN_MASK 1
> +#define _DELAY_EN_MASK 3
> +#define _IMD_SEQ_MASK 1
> +#define _CMD3_MASK 0xff
> +
> +/* DELAY_EN field values, non-shifted */
> +#define _BUSY_NONE 0
> +#define _BUSY_0 1
> +#define _BUSY_1 2
> +
> +/* Slightly confusingly, the DELAYx_EN fields enable BUSY phases. */
> +#define MAKE_GEN_CMD(CMD0_EN, CMD1_EN, CMD2_EN, CMD3_EN, \
> + COL_A0, ROW_A0, COL_A1, ROW_A1, \
> + DATA_EN, BUSY_EN, IMMEDIATE_SEQ, CMD3) \
> + ((((CMD0_EN) & _CMD0_EN_MASK) << _CMD0_EN_SHIFT) | \
> + (((CMD1_EN) & _CMD1_EN_MASK) << _CMD1_EN_SHIFT) | \
> + (((CMD2_EN) & _CMD2_EN_MASK) << _CMD2_EN_SHIFT) | \
> + (((CMD3_EN) & _CMD3_EN_MASK) << _CMD3_EN_SHIFT) | \
> + (((COL_A0) & _COL_A0_MASK) << _COL_A0_SHIFT) | \
> + (((COL_A1) & _COL_A1_MASK) << _COL_A1_SHIFT) | \
> + (((ROW_A0) & _ROW_A0_MASK) << _ROW_A0_SHIFT) | \
> + (((ROW_A1) & _ROW_A1_MASK) << _ROW_A1_SHIFT) | \
> + (((DATA_EN) & _DATA_EN_MASK) << _DATA_EN_SHIFT) | \
> + (((BUSY_EN) & _DELAY_EN_MASK) << _DELAY_EN_SHIFT) | \
> + (((IMMEDIATE_SEQ) & _IMD_SEQ_MASK) << _IMD_SEQ_SHIFT) | \
> + (((CMD3) & _CMD3_MASK) << _CMD3_SHIFT))
> +
> +/* The sequence encodings are not trivial. The ones we use are listed here. */
> +#define _SEQ_0 0x00 /* send one cmd, then wait for ready */
> +#define _SEQ_1 0x21 /* send one cmd, one addr, fetch data */
> +#define _SEQ_2 0x22 /* send one cmd, one addr, fetch data */
> +#define _SEQ_4 0x24 /* single cycle write then read */
> +#define _SEQ_10 0x2A /* read page */
> +#define _SEQ_12 0x0C /* write page, don't wait for R/B */
> +#define _SEQ_18 0x32 /* read page using general cycle */
> +#define _SEQ_19 0x13 /* write page using general cycle */
> +#define _SEQ_14 0x0E /* 3 address cycles, for block erase */
> +
> +#define MLUN_REG 0xBC
> +#define DEV0_SIZE_REG 0xC0
> +#define DEV1_SIZE_REG 0xC4
> +#define DEV2_SIZE_REG 0xC8
> +#define DEV3_SIZE_REG 0xCC
> +#define DEV4_SIZE_REG 0xD0
> +#define DEV5_SIZE_REG 0xD4
> +#define DEV6_SIZE_REG 0xD8
> +#define DEV7_SIZE_REG 0xDC
> +#define SS_CCNT0_REG 0xE0
> +#define SS_CCNT1_REG 0xE4
> +#define SS_SCNT_REG 0xE8
> +#define SS_ADDR_DEV_CTRL_REG 0xEC
> +#define SS_CMD0_REG 0xF0
> +#define SS_CMD1_REG 0xF4
> +#define SS_CMD2_REG 0xF8
> +#define SS_CMD3_REG 0xFC
> +#define SS_ADDR_REG 0x100
> +#define SS_MSEL_REG 0x104
> +#define SS_REQ_REG 0x108
> +#define SS_BRK_REG 0x10C
> +#define DMA_TLVL_REG 0x114
> +#define DMA_TLVL_MAX 0xFF
> +#define AES_CTRL_REG 0x118
> +#define AES_DATAW_REG 0x11C
> +#define AES_SVECT_REG 0x120
> +#define CMD_MARK_REG 0x124
> +#define LUN_STATUS_0_REG 0x128
> +#define LUN_STATUS_1_REG 0x12C
> +#define TIMINGS_TOGGLE_REG 0x130
> +#define TIME_GEN_SEQ_3_REG 0x134
> +#define SQS_DELAY_REG 0x138
> +#define CNE_MASK_REG 0x13C
> +#define CNE_VAL_REG 0x140
> +#define CNA_CTRL_REG 0x144
> +#define INTERNAL_STATUS_REG 0x148
> +#define ECC_CNT_REG 0x14C
> +#define PARAM_REG_REG 0x150
> +
> +/* NAND flash command generation */
> +
> +/* NAND flash command codes */
> +#define NAND_RESET 0xff
> +#define NAND_READ_STATUS 0x70
> +#define NAND_READ_ID 0x90
> +#define NAND_READ_ID_ADDR_STD 0x00 /* address written to ADDR0_COL */
> +#define NAND_READ_ID_ADDR_ONFI 0x20 /* address written to ADDR0_COL */
> +#define NAND_READ_ID_ADDR_JEDEC 0x40 /* address written to ADDR0_COL */
> +#define NAND_PARAM 0xEC
> +#define NAND_PARAM_SIZE_MAX 768 /* bytes */
> +#define NAND_PAGE_READ 0x00
> +#define NAND_PAGE_READ_END 0x30
> +#define NAND_BLOCK_ERASE 0x60
> +#define NAND_BLOCK_ERASE_END 0xd0
> +#define NAND_PAGE_WRITE 0x80
> +#define NAND_PAGE_WRITE_END 0x10
> +
> +#define _DONT_CARE 0x00 /* When we don't have anything better to say */
> +
> +
> +/* Assembled values for putting into COMMAND register */
> +
> +/* Reset NAND flash */
> +
> +/* Uses SEQ_0: non-directional sequence, single command, wait for ready */
> +#define COMMAND_RESET \
> + MAKE_COMMAND(_SEQ_0, INPUT_SEL_SIU, DATA_SEL_FIFO, \
> + NAND_RESET, _DONT_CARE, _DONT_CARE)
> +
> +/* Read status */
> +
> +/* Uses SEQ_4: single command, then read data via DATA_REG */
> +#define COMMAND_READ_STATUS \
> + MAKE_COMMAND(_SEQ_4, INPUT_SEL_SIU, DATA_SEL_DATA_REG, \
> + NAND_READ_STATUS, _DONT_CARE, _DONT_CARE)
> +
> +/* Read ID */
> +
> +/* Uses SEQ_1: single command, ADDR0_COL, then read data via FIFO */
> +/* ADDR0_COL is set to NAND_READ_ID_ADDR_STD for non-ONFi, and
> + * NAND_READ_ID_ADDR_ONFI for ONFi.
> + * The controller reads 5 bytes in the non-ONFi case, and 4 bytes in the
> + * ONFi case, so the data reception (DMA or FIFO_REG) needs to be set up
> + * accordingly.
> + */
> +#define COMMAND_READ_ID \
> + MAKE_COMMAND(_SEQ_1, INPUT_SEL_DMA, DATA_SEL_FIFO, \
> + NAND_READ_ID, _DONT_CARE, _DONT_CARE)
> +
> +#define COMMAND_PARAM \
> + MAKE_COMMAND(_SEQ_2, INPUT_SEL_DMA, DATA_SEL_FIFO, \
> + NAND_PARAM, _DONT_CARE, _DONT_CARE)
> +
> +/* Page read via slave interface (FIFO_DATA register) */
> +
> +/* Standard 5-cycle read command, with 0x30 end-of-cycle marker */
> +/* Uses SEQ_10: CMD0 + 5 address cycles + CMD2, read data */
> +#define COMMAND_READ_PAGE_STD \
> + MAKE_COMMAND(_SEQ_10, INPUT_SEL_SIU, DATA_SEL_FIFO, \
> + NAND_PAGE_READ, _DONT_CARE, NAND_PAGE_READ_END)
> +
> +/* 4-cycle read command, together with GEN_SEQ_CTRL_READ_PAGE_4CYCLE */
> +/* Uses SEQ_18 (generic command sequence, see GEN_SEQ_ECTRL_READ_PAGE_4CYCLE)):
> + * CMD0 + 2+2 address cycles + CMD2, read data
> + */
> +#define COMMAND_READ_PAGE_GEN \
> + MAKE_COMMAND(_SEQ_18, INPUT_SEL_SIU, DATA_SEL_FIFO, \
> + NAND_PAGE_READ, _DONT_CARE, NAND_PAGE_READ_END)
> +
> +/* Page read via master interface (DMA) */
> +
> +/* Standard 5-cycle read command, with 0x30 end-of-cycle marker */
> +/* Uses SEQ_10: CMD0 + 5 address cycles + CMD2, read data */
> +#define COMMAND_READ_PAGE_DMA_STD \
> + MAKE_COMMAND(_SEQ_10, INPUT_SEL_DMA, DATA_SEL_FIFO, \
> + NAND_PAGE_READ, _DONT_CARE, NAND_PAGE_READ_END)
> +
> +/* 4-cycle read command, together with GEN_SEQ_CTRL_READ_PAGE_4CYCLE */
> +/* Uses SEQ_18 (generic command sequence, see GEN_SEQ_ECTRL_READ_PAGE_4CYCLE)):
> + * CMD0 + 2+2 address cycles + CMD2, read data
> + */
> +#define COMMAND_READ_PAGE_DMA_GEN \
> + MAKE_COMMAND(_SEQ_18, INPUT_SEL_DMA, DATA_SEL_FIFO, \
> + NAND_PAGE_READ, _DONT_CARE, NAND_PAGE_READ_END)
> +
> +/* Page write via master interface (DMA) */
> +
> +/* Uses SEQ_12: CMD0 + 5 address cycles + write data + CMD1 */
> +#define COMMAND_WRITE_PAGE_DMA_STD \
> + MAKE_COMMAND(_SEQ_12, INPUT_SEL_DMA, DATA_SEL_FIFO, \
> + NAND_PAGE_WRITE, NAND_PAGE_WRITE_END, _DONT_CARE)
> +
> +/* Uses SEQ_19: CMD0 + 4 address cycles + write data + CMD1 */
> +#define COMMAND_WRITE_PAGE_DMA_GEN \
> + MAKE_COMMAND(_SEQ_19, INPUT_SEL_DMA, DATA_SEL_FIFO, \
> + NAND_PAGE_WRITE, NAND_PAGE_WRITE_END, _DONT_CARE)
> +
> +/* Block erase */
> +
> +/* Uses SEQ_14: CMD0 + 3 address cycles + CMD1 */
> +#define COMMAND_BLOCK_ERASE \
> + MAKE_COMMAND(_SEQ_14, INPUT_SEL_SIU, DATA_SEL_FIFO, \
> + NAND_BLOCK_ERASE, NAND_BLOCK_ERASE_END, _DONT_CARE)
> +
> +/* Assembled values for putting into GEN_SEQ_CTRL register */
> +
> +/* General command sequence specification for 4 cycle PAGE_READ command */
> +#define GEN_SEQ_CTRL_READ_PAGE_4CYCLE \
> + MAKE_GEN_CMD(1, 0, 1, 0, /* enable command 0 and 2 phases */ \
> + 2, 2, /* col A0 2 cycles, row A0 2 cycles */ \
> + 0, 0, /* col A1, row A1 not used */ \
> + 1, /* data phase enabled */ \
> + _BUSY_0, /* busy0 phase enabled */ \
> + 0, /* immediate cmd execution disabled */ \
> + _DONT_CARE) /* command 3 code not needed */
> +
> +/* General command sequence specification for 4 cycle PAGE_PROGRAM command */
> +#define GEN_SEQ_CTRL_WRITE_PAGE_4CYCLE \
> + MAKE_GEN_CMD(1, 1, 0, 0, /* enable command 0 and 1 phases */ \
> + 2, 2, /* col A0 2 cycles, row A0 2 cycles */ \
> + 0, 0, /* col A1, row A1 not used */ \
> + 1, /* data phase enabled */ \
> + _BUSY_1, /* busy1 phase enabled */ \
> + 0, /* immediate cmd execution disabled */ \
> + _DONT_CARE) /* command 3 code not needed */

I think I already commented on that last time a driver for the same IP
was submitted by Oleksij.

I'm pretty sure you can implement ->cmd_ctrl() and rely on the default
cmdfunc() logic instead of manually converting those high-level NAND
commands into your own model (which seems to match pretty much the
->cmd_ctrl() model, where you can get the number of address and command
cycles).

Maybe I'm wrong, but I think it's worth a try, and if it works, it
would greatly simplify the driver.

> +
> +/* BCH ECC size calculations. */
> +/* From "Mr. NAND's Wild Ride: Warning: Suprises Ahead", by Robert Pierce,
> + * Denali Software Inc. 2009, table on page 5
> + */
> +/* Use 8 bit correction as base. */
> +#define ECC8_BYTES(BLKSIZE) (ffs(BLKSIZE) + 3)
> +/* The following would be valid for 4..24 bits of correction. */
> +#define ECC_BYTES_PACKED(CAP, BLKSIZE) ((ECC8_BYTES(BLKSIZE) * (CAP) + 7) / 8)
> +/* Our hardware however requires more bytes than strictly necessary due to
> + * the internal design.
> + */
> +#define ECC_BYTES(CAP, BLKSIZE) ((ECC8_BYTES(1024) * (CAP) + 7) / 8)
> +
> +/* Read modes */
> +enum nfc_read_mode {
> + NFC_READ_STD, /* Standard page read with ECC */
> + NFC_READ_RAW, /* Raw mode read of main area without ECC */
> + NFC_READ_OOB, /* Read oob only (no ECC) */
> + NFC_READ_ALL /* Read main+oob in raw mode (no ECC) */
> +};
> +
> +/* Timing parameters, from DT */
> +struct nfc_timings {
> + uint32_t time_seq_0;
> + uint32_t time_seq_1;
> + uint32_t timings_asyn;
> + uint32_t time_gen_seq_0;
> + uint32_t time_gen_seq_1;
> + uint32_t time_gen_seq_2;
> + uint32_t time_gen_seq_3;
> +};
> +
> +/* Configuration, from DT */
> +struct nfc_setup {
> + struct nfc_timings timings;
> + bool use_bank_select; /* CE selects 'bank' rather than 'chip' */
> + bool rb_wired_and; /* Ready/busy wired AND rather than per-chip */
> +};
> +
> +/* DMA buffer, from both software (buf) and hardware (phys) perspective. */
> +struct nfc_dma {
> + void *buf; /* mapped address */
> + dma_addr_t phys; /* physical address */
> + int bytes_left; /* how much data left to read from buffer? */
> + int buf_bytes; /* how much allocated data in the buffer? */
> + uint8_t *ptr; /* work pointer */
> +};
> +
> +#ifndef POLLED_XFERS
> +/* Interrupt management */
> +struct nfc_irq {
> + int done; /* interrupt triggered, consequently we're done. */
> + uint32_t int_status; /* INT_STATUS at time of interrupt */
> + wait_queue_head_t wq; /* For waiting on controller interrupt */
> +};
> +#endif
> +
> +/* Information common to all chips, including the NANDFLASH-CTRL IP */
> +struct nfc_info {

Should inherit from nand_hw_ctrl (see the sunxi_nand driver)...

> + void __iomem *regbase;
> + struct device *dev;
> + struct nand_hw_control *controller;

... and not point to it.

> + spinlock_t lock;
> + struct nfc_setup *setup;
> + struct nfc_dma dma;
> +#ifndef POLLED_XFERS
> + struct nfc_irq irq;
> +#endif
> +};
> +
> +/* Per-chip controller configuration */
> +struct nfc_config {
> + uint32_t mem_ctrl;
> + uint32_t control;
> + uint32_t ecc_ctrl;
> + uint32_t mem_status_mask;
> + uint32_t cs;
> +};
> +
> +/* Cache for info that we need to save across calls to nfc_command */
> +struct nfc_cmd_cache {
> + unsigned int command;
> + int page;
> + int column;
> + int write_size;
> + int oob_required;
> + int write_raw;
> +};
> +
> +/* Information for each physical NAND chip. */
> +struct chip_info {
> + struct nand_chip chip;
> + struct nfc_cmd_cache cmd_cache;
> + struct nfc_config nfc_config;
> +};
> +
> +/* What we tell mtd is an mtd_info actually is a complete chip_info */
> +#define TO_CHIP_INFO(mtd) ((struct chip_info *)(mtd_to_nand(mtd)))

Ouch! Please, don't do that, container_of() is here for a good reason.
And prefer static inline functions over macros for this kind of things.

static inline struct chip_info *mtd_to_chip_info(struct mtd_info *mtd)
{
return container_of(mtd_to_nand(mtd), struct chip_info, chip);
}

> +
> +/* This is a global pointer, as we only support one single instance of the NFC.
> + * For multiple instances, we would need to add nfc_info as a parameter to
> + * several functions, as well as adding it as a member of the chip_info struct.
> + * Since most likely a system would only have one NFC instance, we don't
> + * go all the way implementing that feature now.
> + */
> +static struct nfc_info *nfc_info;

Come on! Don't be so lazy, do the right thing.

> +
> +/* The timing setup is expected to come via DT. We keep some default timings
> + * here for reference, based on a 100 MHz reference clock.
> + */
> +
> +static const struct nfc_timings default_mode0_pll_enabled = {
> + 0x0d151533, 0x000b0515, 0x00000046,
> + 0x00150000, 0x00000000, 0x00000005, 0x00000015 };

Can you explain those magic values?

> +
> +/**** Utility routines. */

Please use regular comments: /* */

> +
> +/* Count the number of 0's in buff up to a max of max_bits */
> +/* Used to determine how many bitflips there are in an allegedly erased block */
> +static int count_zero_bits(uint8_t *buff, int size, int max_bits)
> +{
> + int k, zero_bits = 0;
> +
> + for (k = 0; k < size; k++) {
> + zero_bits += hweight8(~buff[k]);
> + if (zero_bits > max_bits)
> + break;
> + }
> +
> + return zero_bits;
> +}

We have an helper for that [1].

> +
> +/**** Low level stuff. Read and write registers, interrupt routine, etc. */

Ditto.

> +
> +/* Read and write NFC SFR registers */
> +
> +static uint32_t nfc_read(uint reg_offset)
> +{
> + return readl_relaxed(nfc_info->regbase + reg_offset);
> +}
> +
> +static void nfc_write(uint32_t data, uint reg_offset)
> +{
> + /* Note: According to NANDFLASH-CTRL Design Specification, rev 1.14,
> + * p19, the NFC SFR's can only be written when STATUS.CTRL_STAT is 0.
> + * However, this doesn't seem to be an issue in practice.
> + */
> + writel_relaxed(data, nfc_info->regbase + reg_offset);
> +}

Do you really want to use the _relaxed functions?

> +
> +#ifndef POLLED_XFERS
> +static irqreturn_t nfc_irq(int irq, void *device_info)
> +{
> + /* Note that device_info = nfc_info, so if we don't want a global
> + * nfc_info we can get it via device_info.
> + */
> +
> + /* Save interrupt status in case caller wants to check what actually
> + * happened.
> + */
> + nfc_info->irq.int_status = nfc_read(INT_STATUS_REG);
> +
> + MTD_TRACE("Got interrupt %d, INT_STATUS 0x%08x\n",
> + irq, nfc_info->irq.int_status);
> +
> + /* Note: We can't clear the interrupts by clearing CONTROL.INT_EN,
> + * as that does not disable the interrupt output port from the
> + * nfc towards the gic.
> + */
> + nfc_write(0, INT_STATUS_REG);
> +
> + nfc_info->irq.done = 1;
> + wake_up(&nfc_info->irq.wq);
> +
> + return IRQ_HANDLED;
> +}
> +#endif

Remove the #ifndef section, since it's always activated (see my first
comment).

> +
> +/* Get resources from platform: register bank mapping, irqs, etc */
> +static int nfc_init_resources(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct resource *resource;
> +#ifndef POLLED_XFERS
> + int irq;
> + int res;
> +#endif

Ditto.

> +
> + /* Register base for controller, ultimately from device tree */
> + resource = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!resource) {
> + dev_err(dev, "No register addresses configured!\n");
> + return -ENOMEM;
> + }
> + nfc_info->regbase = devm_ioremap_resource(dev, resource);
> + if (IS_ERR(nfc_info->regbase))
> + return PTR_ERR(nfc_info->regbase);
> +
> + dev_dbg(dev, "Got SFRs at phys %p..%p, mapped to %p\n",
> + (void *)resource->start, (void *)resource->end,
> + nfc_info->regbase);
> +
> + /* A DMA buffer */
> + nfc_info->dma.buf =
> + dma_alloc_coherent(dev, DMA_BUF_SIZE,
> + &nfc_info->dma.phys, GFP_KERNEL);
> + if (nfc_info->dma.buf == NULL) {
> + dev_err(dev, "dma_alloc_coherent failed!\n");
> + return -ENOMEM;
> + }
> +
> + dev_dbg(dev, "DMA buffer %p at physical %p\n",
> + nfc_info->dma.buf, (void *)nfc_info->dma.phys);
> +
> +#ifndef POLLED_XFERS
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0) {
> + dev_err(dev, "No irq configured\n");
> + return irq;
> + }
> + res = request_irq(irq, nfc_irq, 0, "evatronix-nand", nfc_info);

devm_?

> + if (res < 0) {
> + dev_err(dev, "request_irq failed\n");
> + return res;
> + }
> + dev_dbg(dev, "Successfully registered IRQ %d\n", irq);
> +#endif
> +
> + return 0;
> +}

I'm stopping there for now.

Can you please remove everything that is not strictly required and
clean the things I pointed before submitting a new version.

To be honest, your driver seems really complicated compared to what
it's supposed to do, and I suspect it could be a lot simpler, but
again, maybe I'm wrong.

If you didn't try yet, please investigate the ->cmd_ctrl() approach,
and if you did, could you explain why it didn't work out?

Regards,

Boris

[1]http://lxr.free-electrons.com/source/drivers/mtd/nand/nand_base.c#L1183

--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com