Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1413284 > unrolled thread
| Started by | Boris Brezillon <boris.brezillon@free-electrons.com> |
|---|---|
| First post | 2016-06-03 17:00 +0200 |
| Last post | 2016-06-10 19:00 +0200 |
| Articles | 15 — 3 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH 3/4] mtd: nand: Add support for Evatronix NANDFLASH-CTRL Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-06-03 17:00 +0200
Re: [PATCH 3/4] mtd: nand: Add support for Evatronix NANDFLASH-CTRL Ricard Wanderlof <ricard.wanderlof@axis.com> - 2016-06-09 10:30 +0200
Re: [PATCH 3/4] mtd: nand: Add support for Evatronix NANDFLASH-CTRL Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-06-09 11:10 +0200
Re: [PATCH 3/4] mtd: nand: Add support for Evatronix NANDFLASH-CTRL Ricard Wanderlof <ricard.wanderlof@axis.com> - 2016-06-10 16:50 +0200
Re: [PATCH 3/4] mtd: nand: Add support for Evatronix NANDFLASH-CTRL Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-06-10 17:40 +0200
Re: [PATCH 3/4] mtd: nand: Add support for Evatronix NANDFLASH-CTRL Ricard Wanderlof <ricard.wanderlof@axis.com> - 2016-06-10 18:10 +0200
Re: [PATCH 3/4] mtd: nand: Add support for Evatronix NANDFLASH-CTRL Mychaela Falconia <mychaela.falconia@gmail.com> - 2016-06-09 19:30 +0200
Re: [PATCH 3/4] mtd: nand: Add support for Evatronix NANDFLASH-CTRL Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-06-09 20:10 +0200
Re: [PATCH 3/4] mtd: nand: Add support for Evatronix NANDFLASH-CTRL Mychaela Falconia <mychaela.falconia@gmail.com> - 2016-06-09 21:40 +0200
Re: [PATCH 3/4] mtd: nand: Add support for Evatronix NANDFLASH-CTRL Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-06-09 22:30 +0200
Re: [PATCH 3/4] mtd: nand: Add support for Evatronix NANDFLASH-CTRL Mychaela Falconia <mychaela.falconia@gmail.com> - 2016-06-10 07:10 +0200
Re: [PATCH 3/4] mtd: nand: Add support for Evatronix NANDFLASH-CTRL Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-06-10 14:50 +0200
Re: [PATCH 3/4] mtd: nand: Add support for Evatronix NANDFLASH-CTRL Ricard Wanderlof <ricard.wanderlof@axis.com> - 2016-06-10 16:30 +0200
Re: [PATCH 3/4] mtd: nand: Add support for Evatronix NANDFLASH-CTRL Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-06-10 18:10 +0200
Re: [PATCH 3/4] mtd: nand: Add support for Evatronix NANDFLASH-CTRL Ricard Wanderlof <ricard.wanderlof@axis.com> - 2016-06-10 19:00 +0200
| From | Boris Brezillon <boris.brezillon@free-electrons.com> |
|---|---|
| Date | 2016-06-03 17:00 +0200 |
| Subject | Re: [PATCH 3/4] mtd: nand: Add support for Evatronix NANDFLASH-CTRL |
| Message-ID | <rFYJP-4Ab-7@gated-at.bofh.it> |
On Thu, 2 Jun 2016 09:48:32 +0200
Ricard Wanderlof <ricard.wanderlof@axis.com> 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@axis.com>
> ---
> 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
[toc] | [next] | [standalone]
| From | Ricard Wanderlof <ricard.wanderlof@axis.com> |
|---|---|
| Date | 2016-06-09 10:30 +0200 |
| Subject | Re: [PATCH 3/4] mtd: nand: Add support for Evatronix NANDFLASH-CTRL |
| Message-ID | <rI3vH-3P1-13@gated-at.bofh.it> |
| In reply to | #1413284 |
Hi Boris,
Again, thanks for reviewing this.
On Fri, 3 Jun 2016, Boris Brezillon wrote:
> > 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.
I did run checkpatch.pl (at least the one in the mtd l2 tree), and there
should be no outstanding errors. Some of the warnings are related to lines
that are more than 80 characters which contain printouts which I don't
want to split as it makes them hard to grep for.
There is a warning regarding the KConfig entry though, which seems to
indicate that the description is missing - perhaps it just means that the
help text is too short (although it's not shorter than many other NAND
drivers in the same file)?
There are a couple of BUG()s though which are all of the type 'things that
shouldn't happen' (e.g.. an enum having a value outside its range), so
there's no real way to recover, however, one could always return early
from the function in question and hope for the best.
I see now that there's a comment on an overly long line in the commit
message that I've missed, as is a function call that's one character over
the 80 character limit.
I usually consider checkpatch.pl to be of guidence rather than a sentinel
when it comes to warnings, but if you want a '0 warnings' approach I can
certainly accomplish that.
> > +#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.
Ok, will do.
> > +/* 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.
NFC_DMA64BIT is untested so I'll take it out.
CLEAR_DMA_BUF_AFTER_WRITE is useful when debugging (and tested), and
POLLED_XFERS is also tested but normally only used for debugging (if for
instance there's a problem with interrupts). I don't really like to throw
out code that's useful because it's unnecessary work to have to put it in
again when the need arises. I could change the wording in the comment to
make it clearer though (especially after having removed NFC_DMA64BIT) if
that is enough?
The rationale for leaving this code in is that it can help bring up a new
unknown system with this IP, because I consider the most likely re-use of
this driver to be for someone who is developing a new SoC, as it seems
rather uncommon in commercially available chips.
> > +/* DMA buffer for page transfers. */
> > +#define DMA_BUF_SIZE (8192 + 640) /* main + spare for 8k page flash */
>
> This should clearly be dynamic.
8k pages are the largest the controller can handle, and there's only a
single DMA buffer for the controller, so it's a very small amount of
memory, and I didn't feel it worth the complexity to reduce the size just
because a smaller paged flash was encountered. The driver uses the DMA
buffer to read the ID data so it needs a buffer anyway before the page
size has been determined. But if you feel it's important I can rework it -
there would have to be two buffers, one smaller one for reading the ID and
a larger one subsequently allocated for page data.
> > +
> > +/* 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_.
Ok. I was thinking about replacing it with pr_debug straight off, but saw
that there were other drivers with custom debug macros so I left it in.
I'll replace it with pr_debug then.
> 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.
It's true that the debug traces were initially created during driver
development, however I have gone through the debug printouts and consider
the ones remaining to be relevant, especially if one is trying to debug a
new previously untested system with this IP. Was there any particular
one(s) you were thinking of?
> > + 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.
(Just to clarify: this doesn't have any bearing on this particular issue,
but the SoC Oleksij submitted a driver for used a rather different
(probably older) version of the same IP. The only documentation I saw for
that SoC was the IP register map, and there were several differences in
not only the register addresses but also the available registers and bit
fields, and it did not look like one was a subset of the other. So it
looked more like the IP vendor had done an at least partial rewrite of the
internal logic between the two versions.)
> 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.
It's not so much trying as reading the manual for the IP. :-)
The problem here is that the ->cmd_ctrl() logic assumes that you can pass
single bytes transparently via the IP and also directly control the state
of the ALE and CLE lines, as well as set up data transfers independently
of that, none of which is not possible with this IP. Instead, the IP
offers a number of fixed interface sequences, some more programmable than
others, a subset of which are used in this driver, which do all the heavy
lifting. There is for instance no sequence which just writes or reads data
to or from the flash, there's always at least a command write (CLE)
included.
(The command definitions in the macro sections are more or less direct
translations from a table and section in the IP manual describing how to
accomplish various functions.)
If there had been a transparent mode I would certainly have gone that
route, at least initially, rather than mess about with all this controller
specific stuff. I think that's why Oleksij arrived at a similar solution
(or possibly he used a non-Linux driver as a starting point).
The designers of this IP apparantly did not have Linux in mind when they
designed the controller, since it does all the low level stuff
autonomously (in the right IP configuration it can even remap flash blocks
transparently), with no way of intervening. For instance, when doing
hardware ECC, the OOB data is not available anywhere to the user, and if
one wants to actually read it a separate OOB write needs to be done. I
think the target market for the IP is really a general real time OS where
there is no NAND driver available, and you just want to fire off a single
high level command, wait for an interrupt, and have your data waiting for
you.
This latter property is actually advantageous in Linux too as the driver
doesn't have to do bit- and byte-banging against the NAND flash. I'm not
sure what the gain in overhead is in practice, but at any rate there's not
much of a choice.
> > +/* 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.
Ok. I'm a bit unsure what you mean by 'inherit' though; in the sunxi
driver the struct nand_hw_controller is contained within the struct
sunxi_nfc, in my case there's a pointer to a single instance of a
nand_hw_control. I agree that my approach is wasteful on a dynamic
allocation and I have no problems changing it, but conceptually there's
not much of a difference.
> > +
> > +/* 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);
> }
Ok. Will change.
> > +
> > +/* 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.
It's not a question of laziness, it was a conscious decision: why add
unnecessary bloat and complexity for a case that probably will never
occur? I can certainly change it if you think it's worth while of course.
> > +
> > +/* 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?
Not really, the problem is that the agreement we have with the IP vendor
is that we may not disclose any documentation, outside of what is
absolutely necessary to write working code.
A rationale is that anyone else wanting to use the driver will either be
designing their own SoC in which case they will have access to the
relevant documentation, or if they're using a SoC from someone else, the
SoC vendor will have to provide that information in order for the chip to
be useful anyway.
> > +
> > +/**** Utility routines. */
>
> Please use regular comments: /* */
Ugh. Yes, sorry.
> > +
> > +/* 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].
Great, I'll use that. (I don't think it existed when the first version of
this driver was written).
> > +
> > +/* 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?
Yes, using _relaxed, together with explicit memory barriers in the DMA
routines has shown a 5% increase in flash read performance on an ARM
system, the reason being that on an ARM system the implicit memory barrier
in the writel() call causes a fairly heavy penalty in terms of flushing
the L2 cache.
> > + res = request_irq(irq, nfc_irq, 0, "evatronix-nand", nfc_info);
>
> devm_?
Yes, good catch, thanks!
> 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?
Given that the controller does not have the transparency that the
->cmd_ctrl() approach requires, as noted above, I can't see how it could
be simplified.
I basically need to grab everything needed for a given operation and
interpret it before handing it over to the controller. I considered using
a higher level API, by replacing the default ->cmdfunc() (default
nand_command/nand_command_lp) with a specific version, which would have
avoided the need to interpret the NAND commands arriving via ->cmd_ctrl(),
but that meant duplicating some of the logic in nand_base.c which seemed
like a bad idea.
> Can you please remove everything that is not strictly required and
> clean the things I pointed before submitting a new version.
Sure, but I would very much like some feedback on the points I've raised
above before going on with that.
/Ricard
--
Ricard Wolf Wanderlöf ricardw(at)axis.com
Axis Communications AB, Lund, Sweden www.axis.com
Phone +46 46 272 2016 Fax +46 46 13 61 30
[toc] | [prev] | [next] | [standalone]
| From | Boris Brezillon <boris.brezillon@free-electrons.com> |
|---|---|
| Date | 2016-06-09 11:10 +0200 |
| Message-ID | <rI48p-4md-23@gated-at.bofh.it> |
| In reply to | #1418166 |
On Thu, 9 Jun 2016 10:19:51 +0200
Ricard Wanderlof <ricard.wanderlof@axis.com> wrote:
> Hi Boris,
>
> Again, thanks for reviewing this.
>
> On Fri, 3 Jun 2016, Boris Brezillon wrote:
>
> > > 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.
>
> I did run checkpatch.pl (at least the one in the mtd l2 tree), and there
> should be no outstanding errors. Some of the warnings are related to lines
> that are more than 80 characters which contain printouts which I don't
> want to split as it makes them hard to grep for.
>
> There is a warning regarding the KConfig entry though, which seems to
> indicate that the description is missing - perhaps it just means that the
> help text is too short (although it's not shorter than many other NAND
> drivers in the same file)?
>
> There are a couple of BUG()s though which are all of the type 'things that
> shouldn't happen' (e.g.. an enum having a value outside its range), so
> there's no real way to recover, however, one could always return early
> from the function in question and hope for the best.
>
> I see now that there's a comment on an overly long line in the commit
> message that I've missed, as is a function call that's one character over
> the 80 character limit.
>
> I usually consider checkpatch.pl to be of guidence rather than a sentinel
> when it comes to warnings, but if you want a '0 warnings' approach I can
> certainly accomplish that.
Well, I'm not asking to fix all 80 chars warnings, but I see a lot of
warnings and errors in there [1], and I'm pretty sure most of them can
be addressed in a sane way.
>
> > > +#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.
>
> Ok, will do.
>
> > > +/* 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.
>
> NFC_DMA64BIT is untested so I'll take it out.
>
> CLEAR_DMA_BUF_AFTER_WRITE is useful when debugging (and tested), and
> POLLED_XFERS is also tested but normally only used for debugging (if for
> instance there's a problem with interrupts). I don't really like to throw
> out code that's useful because it's unnecessary work to have to put it in
> again when the need arises. I could change the wording in the comment to
> make it clearer though (especially after having removed NFC_DMA64BIT) if
> that is enough?
>
> The rationale for leaving this code in is that it can help bring up a new
> unknown system with this IP, because I consider the most likely re-use of
> this driver to be for someone who is developing a new SoC, as it seems
> rather uncommon in commercially available chips.
That's not how it works. Either you provide a sane way to activate
these options (using Kconfig entries), or your drop dead-code sections.
I understand that debugging is important to you, but adding hundreds
lines of unused code is also hurting readability, so I keep thinking
that these options should either be removed (with the associated code
sections) or exposed as Kconfig options.
>
> > > +/* DMA buffer for page transfers. */
> > > +#define DMA_BUF_SIZE (8192 + 640) /* main + spare for 8k page flash */
> >
> > This should clearly be dynamic.
>
> 8k pages are the largest the controller can handle, and there's only a
> single DMA buffer for the controller, so it's a very small amount of
> memory, and I didn't feel it worth the complexity to reduce the size just
> because a smaller paged flash was encountered. The driver uses the DMA
> buffer to read the ID data so it needs a buffer anyway before the page
> size has been determined. But if you feel it's important I can rework it -
> there would have to be two buffers, one smaller one for reading the ID and
> a larger one subsequently allocated for page data.
If you reject all NAND above 8k + 640 oob bytes I'm fine with this
fixed size.
>
> > > +
> > > +/* 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_.
>
> Ok. I was thinking about replacing it with pr_debug straight off, but saw
> that there were other drivers with custom debug macros so I left it in.
> I'll replace it with pr_debug then.
>
> > 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.
>
> It's true that the debug traces were initially created during driver
> development, however I have gone through the debug printouts and consider
> the ones remaining to be relevant, especially if one is trying to debug a
> new previously untested system with this IP. Was there any particular
> one(s) you were thinking of?
I don't have any example, there just seem to be a lot of them. I'll
have a closer look.
>
> > > + 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.
>
> (Just to clarify: this doesn't have any bearing on this particular issue,
> but the SoC Oleksij submitted a driver for used a rather different
> (probably older) version of the same IP. The only documentation I saw for
> that SoC was the IP register map, and there were several differences in
> not only the register addresses but also the available registers and bit
> fields, and it did not look like one was a subset of the other. So it
> looked more like the IP vendor had done an at least partial rewrite of the
> internal logic between the two versions.)
>
> > 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.
>
> It's not so much trying as reading the manual for the IP. :-)
>
> The problem here is that the ->cmd_ctrl() logic assumes that you can pass
> single bytes transparently via the IP and also directly control the state
> of the ALE and CLE lines, as well as set up data transfers independently
> of that, none of which is not possible with this IP. Instead, the IP
> offers a number of fixed interface sequences, some more programmable than
> others, a subset of which are used in this driver, which do all the heavy
> lifting. There is for instance no sequence which just writes or reads data
> to or from the flash, there's always at least a command write (CLE)
> included.
>
> (The command definitions in the macro sections are more or less direct
> translations from a table and section in the IP manual describing how to
> accomplish various functions.)
>
> If there had been a transparent mode I would certainly have gone that
> route, at least initially, rather than mess about with all this controller
> specific stuff. I think that's why Oleksij arrived at a similar solution
> (or possibly he used a non-Linux driver as a starting point).
>
> The designers of this IP apparantly did not have Linux in mind when they
> designed the controller, since it does all the low level stuff
> autonomously (in the right IP configuration it can even remap flash blocks
> transparently), with no way of intervening. For instance, when doing
> hardware ECC, the OOB data is not available anywhere to the user, and if
> one wants to actually read it a separate OOB write needs to be done. I
> think the target market for the IP is really a general real time OS where
> there is no NAND driver available, and you just want to fire off a single
> high level command, wait for an interrupt, and have your data waiting for
> you.
>
> This latter property is actually advantageous in Linux too as the driver
> doesn't have to do bit- and byte-banging against the NAND flash. I'm not
> sure what the gain in overhead is in practice, but at any rate there's not
> much of a choice.
Just to be clear, you don't have to toggle the pins each time
->cmd_ctrl() is called, you can cache the operations and launch it once
it says it's dones (don't remember the flag).
Now, I agree that it's not perfect, and it would certainly be better to
have the whole thing packed together (including the data transfer
size). I'm working on it ;).
What made me think using ->cmd_ctrl() would be simpler it the fact that
you're extracting address and cmd cycles from the cmd type, and this is
something ->cmd_ctrl() is already providing (see what's done here [2]).
>
> > > +/* 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.
>
> Ok. I'm a bit unsure what you mean by 'inherit' though; in the sunxi
> driver the struct nand_hw_controller is contained within the struct
> sunxi_nfc, in my case there's a pointer to a single instance of a
> nand_hw_control. I agree that my approach is wasteful on a dynamic
> allocation and I have no problems changing it, but conceptually there's
> not much of a difference.
There's a huge different. By embedding the nand_hw_control struct into
your nfc_info object you allow things like:
static int get_nfc(struct nand_chip *chip)
{
return container_of(chip->controller, struct nfc_info,
controller);
}
This way you can retrieve the nfc_info object attached to the nand_chip.
>
> > > +
> > > +/* 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);
> > }
>
> Ok. Will change.
>
> > > +
> > > +/* 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.
>
> It's not a question of laziness, it was a conscious decision: why add
> unnecessary bloat and complexity for a case that probably will never
> occur? I can certainly change it if you think it's worth while of course.
It is. And you're okay bloating the code with dead-code, but not with
implementing this in order to avoid singletons when it clearly
shouldn't be?
>
> > > +
> > > +/* 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?
>
> Not really, the problem is that the agreement we have with the IP vendor
> is that we may not disclose any documentation, outside of what is
> absolutely necessary to write working code.
>
> A rationale is that anyone else wanting to use the driver will either be
> designing their own SoC in which case they will have access to the
> relevant documentation, or if they're using a SoC from someone else, the
> SoC vendor will have to provide that information in order for the chip to
> be useful anyway.
Hm, so I'll have a new table like that for each new SoC using this IP?
I must say I don't like the idea, but let's address the other aspects
first.
>
> > > +
> > > +/**** Utility routines. */
> >
> > Please use regular comments: /* */
>
> Ugh. Yes, sorry.
>
> > > +
> > > +/* 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].
>
> Great, I'll use that. (I don't think it existed when the first version of
> this driver was written).
>
> > > +
> > > +/* 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?
>
> Yes, using _relaxed, together with explicit memory barriers in the DMA
> routines has shown a 5% increase in flash read performance on an ARM
> system, the reason being that on an ARM system the implicit memory barrier
> in the writel() call causes a fairly heavy penalty in terms of flushing
> the L2 cache.
Ok, as long as you know what you're doing, I'm fine with it.
>
> > > + res = request_irq(irq, nfc_irq, 0, "evatronix-nand", nfc_info);
> >
> > devm_?
>
> Yes, good catch, thanks!
>
> > 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?
>
> Given that the controller does not have the transparency that the
> ->cmd_ctrl() approach requires, as noted above, I can't see how it could
> be simplified.
I'm not totally convinced, but I'll have to go through all the macros
into more details to be sure.
>
> I basically need to grab everything needed for a given operation and
> interpret it before handing it over to the controller. I considered using
> a higher level API, by replacing the default ->cmdfunc() (default
> nand_command/nand_command_lp) with a specific version, which would have
> avoided the need to interpret the NAND commands arriving via ->cmd_ctrl(),
> but that meant duplicating some of the logic in nand_base.c which seemed
> like a bad idea.
Yes. As said above, I'm planning to rework the NAND framework to
support things like:
struct nand_operation {
u8 cmds[2];
u8 addrs[5];
int ncmds;
int naddrs;
union {
void *out;
const void *in;
};
enum nand_op_direction dir;
}
->exec_op(struct nand_operation *op);
This way the NAND controller would have all the necessary information
to trigger the whole operation (omitted the ECC info on purpose, to
make it clearer).
But this is not there yet, and in the meantime, if possible, I'd prefer
seeing drivers implementing the ->cmd_ctrl() function instead of
overloading the default ->cmdfunc() implementation.
Regards,
Boris
[1]http://code.bulix.org/eqd4ce-100790
[2]https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/mtd/nand/sunxi_nand.c?id=refs/tags/v4.7-rc2#n517
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[toc] | [prev] | [next] | [standalone]
| From | Ricard Wanderlof <ricard.wanderlof@axis.com> |
|---|---|
| Date | 2016-06-10 16:50 +0200 |
| Subject | Re: [PATCH 3/4] mtd: nand: Add support for Evatronix NANDFLASH-CTRL |
| Message-ID | <rIvUZ-5O9-9@gated-at.bofh.it> |
| In reply to | #1418189 |
On Thu, 9 Jun 2016, Boris Brezillon wrote:
> > > > 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.
> >
> > I did run checkpatch.pl (at least the one in the mtd l2 tree), and there
> > should be no outstanding errors. Some of the warnings are related to lines
> > ...
> >
>
> Well, I'm not asking to fix all 80 chars warnings, but I see a lot of
> warnings and errors in there [1], and I'm pretty sure most of them can
> be addressed in a sane way.
Whoa, something must have gone seriously wrong when I mailed out the
patch. When I run checkpatch.pl locally on the patch file, I have 0 errors
and 10 warnings (not 128 errors and 65 warnings).
I tired mailing the patch to myself, extracting it from the incoming
email, and comparing with the original patch file and they were identical.
I must check up if something has gone amiss with our external email here,
as we haven't had problems before. I'll need to look into this. I would
certainly not submit a patch with any checkpatch.pl errors unless they
were really false positives.
> > > > +#undef POLLED_XFERS /* Use polled rather than interrupt based transfers */
> > > > +
> > > > +#undef CLEAR_DMA_BUF_AFTER_WRITE /* Useful for debugging */
> > >
> > [ ... ]
> > The rationale for leaving this code in is that it can help bring up a new
> > unknown system with this IP, because I consider the most likely re-use of
> > this driver to be for someone who is developing a new SoC, as it seems
> > rather uncommon in commercially available chips.
>
> That's not how it works. Either you provide a sane way to activate
> these options (using Kconfig entries), or your drop dead-code sections.
>
> I understand that debugging is important to you, but adding hundreds
> lines of unused code is also hurting readability, so I keep thinking
> that these options should either be removed (with the associated code
> sections) or exposed as Kconfig options.
Ok, I'll give it some consideration and fix it either way.
> > > > +/* DMA buffer for page transfers. */
> > > > +#define DMA_BUF_SIZE (8192 + 640) /* main + spare for 8k page flash */
> > >
> > > This should clearly be dynamic.
> >
> > 8k pages are the largest the controller can handle, and there's only a
> > [ ... ]
>
> If you reject all NAND above 8k + 640 oob bytes I'm fine with this
> fixed size.
Ok, I see what you're getting at. Yes, there needs to be a check for that,
somewhere.
> > > 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).
> > > [ ... ]
>
> Just to be clear, you don't have to toggle the pins each time
> ->cmd_ctrl() is called, you can cache the operations and launch it once
> it says it's dones (don't remember the flag).
>
I've omitted comments to this here as the discussion has been carried on
in the sub thread started by Mychaela.
> > > > +/* 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.
> >
> > Ok. I'm a bit unsure what you mean by 'inherit' though; in the sunxi
> > driver the struct nand_hw_controller is contained within the struct
> > sunxi_nfc, in my case there's a pointer to a single instance of a
> > nand_hw_control. I agree that my approach is wasteful on a dynamic
> > allocation and I have no problems changing it, but conceptually there's
> > not much of a difference.
>
> There's a huge different. By embedding the nand_hw_control struct into
> your nfc_info object you allow things like:
>
> static int get_nfc(struct nand_chip *chip)
> {
> return container_of(chip->controller, struct nfc_info,
> controller);
> }
>
> This way you can retrieve the nfc_info object attached to the nand_chip.
Yes, of course ... or rather,
static int get_nfc(struct nand_hw_control *controller)
I see new what you mean by inherit. It all comes down to if struct
nfc_info is a specialized type of struct nand_hw_control, or if it just
refers to it. I had assumed it was the latter that was the paradigm.
In the driver in question it makes no practical difference as there is no
need to go from a nand_hw_control to an nfc_info (in fact, the
nand_hw_control is nevery really used explicitly, it tags along, only used
by the framework).
Still, I'm not trying to make an argument here, just trying to understand
what the underlying paradigm is. I'll move it inside as it clearly is
better in several respects.
> > > > +
> > > > +/* 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.
> >
> > It's not a question of laziness, it was a conscious decision: why add
> > unnecessary bloat and complexity for a case that probably will never
> > occur? I can certainly change it if you think it's worth while of course.
>
> It is. And you're okay bloating the code with dead-code, but not with
> implementing this in order to avoid singletons when it clearly
> shouldn't be?
I'm ok with bloating the code with something which I consider may be
useful, but I have reservations bloating it with something which I don't
think will ever be used (and which could be added if the need arises
later) and furthermore is not possible to test and verify properly (as
there is in fact only one NAND controller on the platform on which I can
test this).
But I'm fine with adding it, I'm not really trying to knock it, just
explaining why it wasn't done in the first place. (I think I'm actually
reacting to the word 'lazy' here...).
> > > > +
> > > > +/* 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?
> >
> > Not really, the problem is that the agreement we have with the IP vendor
> > is that we may not disclose any documentation, outside of what is
> > absolutely necessary to write working code.
> >
> > A rationale is that anyone else wanting to use the driver will either be
> > designing their own SoC in which case they will have access to the
> > relevant documentation, or if they're using a SoC from someone else, the
> > SoC vendor will have to provide that information in order for the chip to
> > be useful anyway.
>
> Hm, so I'll have a new table like that for each new SoC using this IP?
Yes, I would say that would be the case.
> I must say I don't like the idea, but let's address the other aspects
> first.
Ok.
> As said above, I'm planning to rework the NAND framework to
> support things like:
>
> struct nand_operation {
> u8 cmds[2];
> u8 addrs[5];
> int ncmds;
> int naddrs;
> union {
> void *out;
> const void *in;
> };
> enum nand_op_direction dir;
> }
>
> ->exec_op(struct nand_operation *op);
>
> This way the NAND controller would have all the necessary information
> to trigger the whole operation (omitted the ECC info on purpose, to
> make it clearer).
>
> But this is not there yet, and in the meantime, if possible, I'd prefer
> seeing drivers implementing the ->cmd_ctrl() function instead of
> overloading the default ->cmdfunc() implementation.
I see, I suppose that's because during the course of this the ->cmdfunc()
logic will be significantly changed, requiring corresponding changes in
drivers that do overload that function? Fair enough, that's a pretty good
reason, probably more so than the alleged simplicity of the ->cmd_ctrl()
interface.
/Ricard
--
Ricard Wolf Wanderlöf ricardw(at)axis.com
Axis Communications AB, Lund, Sweden www.axis.com
Phone +46 46 272 2016 Fax +46 46 13 61 30
[toc] | [prev] | [next] | [standalone]
| From | Boris Brezillon <boris.brezillon@free-electrons.com> |
|---|---|
| Date | 2016-06-10 17:40 +0200 |
| Message-ID | <rIwHn-6kc-1@gated-at.bofh.it> |
| In reply to | #1419490 |
On Fri, 10 Jun 2016 16:40:39 +0200
Ricard Wanderlof <ricard.wanderlof@axis.com> wrote:
>
> > > > > +/* 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.
> > >
> > > Ok. I'm a bit unsure what you mean by 'inherit' though; in the sunxi
> > > driver the struct nand_hw_controller is contained within the struct
> > > sunxi_nfc, in my case there's a pointer to a single instance of a
> > > nand_hw_control. I agree that my approach is wasteful on a dynamic
> > > allocation and I have no problems changing it, but conceptually there's
> > > not much of a difference.
> >
> > There's a huge different. By embedding the nand_hw_control struct into
> > your nfc_info object you allow things like:
> >
> > static int get_nfc(struct nand_chip *chip)
> > {
> > return container_of(chip->controller, struct nfc_info,
> > controller);
> > }
> >
> > This way you can retrieve the nfc_info object attached to the nand_chip.
>
> Yes, of course ... or rather,
>
> static int get_nfc(struct nand_hw_control *controller)
Actually I meant
static inline struct nfc_info *get_nfc(struct nand_chip *chip) ...
>
> I see new what you mean by inherit. It all comes down to if struct
> nfc_info is a specialized type of struct nand_hw_control, or if it just
> refers to it. I had assumed it was the latter that was the paradigm.
>
> In the driver in question it makes no practical difference as there is no
> need to go from a nand_hw_control to an nfc_info (in fact, the
> nand_hw_control is nevery really used explicitly, it tags along, only used
> by the framework).
>
> Still, I'm not trying to make an argument here, just trying to understand
> what the underlying paradigm is. I'll move it inside as it clearly is
> better in several respects.
The goal here is to retrieve the controller attached to a given chip in
order to avoid the global nfc_info variable (and abusing
nand_get/set_controller_data() to store a pointer to the controller is
not a good idea either: it's supposed to be used to store per-chip
private data).
>
> > > > > +
> > > > > +/* 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.
> > >
> > > It's not a question of laziness, it was a conscious decision: why add
> > > unnecessary bloat and complexity for a case that probably will never
> > > occur? I can certainly change it if you think it's worth while of course.
> >
> > It is. And you're okay bloating the code with dead-code, but not with
> > implementing this in order to avoid singletons when it clearly
> > shouldn't be?
>
> I'm ok with bloating the code with something which I consider may be
> useful, but I have reservations bloating it with something which I don't
> think will ever be used (and which could be added if the need arises
> later) and furthermore is not possible to test and verify properly (as
> there is in fact only one NAND controller on the platform on which I can
> test this).
>
> But I'm fine with adding it, I'm not really trying to knock it, just
> explaining why it wasn't done in the first place. (I think I'm actually
> reacting to the word 'lazy' here...).
Then, let's say I really care about this clear separation between NAND
controllers and NAND chips (even if the controller is only supporting a
single device), because it makes things clearer, and because it brings
some consistency in the NAND controller drivers.
That's something I've asked to other contributors, and I'm asking it to
you too.
You'll see that implementing this separation is not much more
complicated than having this global variable, and I must admit global
variable make me scream (especially when they can be avoided).
>
> > > > > +
> > > > > +/* 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?
> > >
> > > Not really, the problem is that the agreement we have with the IP vendor
> > > is that we may not disclose any documentation, outside of what is
> > > absolutely necessary to write working code.
> > >
> > > A rationale is that anyone else wanting to use the driver will either be
> > > designing their own SoC in which case they will have access to the
> > > relevant documentation, or if they're using a SoC from someone else, the
> > > SoC vendor will have to provide that information in order for the chip to
> > > be useful anyway.
> >
> > Hm, so I'll have a new table like that for each new SoC using this IP?
>
> Yes, I would say that would be the case.
>
> > I must say I don't like the idea, but let's address the other aspects
> > first.
>
> Ok.
>
> > As said above, I'm planning to rework the NAND framework to
> > support things like:
> >
> > struct nand_operation {
> > u8 cmds[2];
> > u8 addrs[5];
> > int ncmds;
> > int naddrs;
> > union {
> > void *out;
> > const void *in;
> > };
> > enum nand_op_direction dir;
> > }
> >
> > ->exec_op(struct nand_operation *op);
> >
> > This way the NAND controller would have all the necessary information
> > to trigger the whole operation (omitted the ECC info on purpose, to
> > make it clearer).
> >
> > But this is not there yet, and in the meantime, if possible, I'd prefer
> > seeing drivers implementing the ->cmd_ctrl() function instead of
> > overloading the default ->cmdfunc() implementation.
>
> I see, I suppose that's because during the course of this the ->cmdfunc()
> logic will be significantly changed, requiring corresponding changes in
> drivers that do overload that function? Fair enough, that's a pretty good
> reason, probably more so than the alleged simplicity of the ->cmd_ctrl()
> interface.
There's another reason actually. We have chip specific functions (like
->setup_read_retry()) which might want to use
non-standard/vendor-specific operations, and this implies patching all
->cmdfunc() implementations, or at least making sure they will work
fine with these new commands.
The ->cmd_ctrl() + generic nand_command_lp() for ->cmdfunc() is making
that a lot easier.
So yes, I'm clearly trying to avoid specific ->cmdfunc() (especially
when they are not generic enough to support new commands).
Again, ->cmd_ctrl() does not have to be used in your internal
ecc->read/write_page() implementations (all you'll have to do is avoid
using the ->cmdfunc() method and create your own NAND controller
specific commands instead), but it should at least be used for basic
operations that do not require high performances (i.e. NAND detection,
NAND RESET, read-retry, ...).
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[toc] | [prev] | [next] | [standalone]
| From | Ricard Wanderlof <ricard.wanderlof@axis.com> |
|---|---|
| Date | 2016-06-10 18:10 +0200 |
| Subject | Re: [PATCH 3/4] mtd: nand: Add support for Evatronix NANDFLASH-CTRL |
| Message-ID | <rIxaq-6KH-27@gated-at.bofh.it> |
| In reply to | #1419527 |
On Fri, 10 Jun 2016, Boris Brezillon wrote: > The goal here is to retrieve the controller attached to a given chip in > order to avoid the global nfc_info variable (and abusing > nand_get/set_controller_data() to store a pointer to the controller is > not a good idea either: it's supposed to be used to store per-chip > private data). > Then, let's say I really care about this clear separation between NAND > controllers and NAND chips (even if the controller is only supporting a > single device), because it makes things clearer, and because it brings > some consistency in the NAND controller drivers. > That's something I've asked to other contributors, and I'm asking it to > you too. Certainly. As I said, I'm not trying to get away from doing it if there's any motivation for it which there clearly is. > You'll see that implementing this separation is not much more > complicated than having this global variable, and I must admit global > variable make me scream (especially when they can be avoided). Yes, I see now. With struct nand_hw_control in the proper place it allows a translation from a struct nand_chip * to a future non-global struct nfc_info * . Yeah, I would probably have stumbled upon this when rewriting it. > > > But this is not there yet, and in the meantime, if possible, I'd prefer > > > seeing drivers implementing the ->cmd_ctrl() function instead of > > > overloading the default ->cmdfunc() implementation. > > > > I see, I suppose that's because during the course of this the ->cmdfunc() > > logic will be significantly changed, requiring corresponding changes in > > drivers that do overload that function? Fair enough, that's a pretty good > > reason, probably more so than the alleged simplicity of the ->cmd_ctrl() > > interface. > > There's another reason actually. We have chip specific functions (like > ->setup_read_retry()) which might want to use > non-standard/vendor-specific operations, and this implies patching all > ->cmdfunc() implementations, or at least making sure they will work > fine with these new commands. I see. > So yes, I'm clearly trying to avoid specific ->cmdfunc() (especially > when they are not generic enough to support new commands). Yes, makes sense. > Again, ->cmd_ctrl() does not have to be used in your internal > ecc->read/write_page() implementations (all you'll have to do is avoid > using the ->cmdfunc() method and create your own NAND controller > specific commands instead), but it should at least be used for basic > operations that do not require high performances (i.e. NAND detection, > NAND RESET, read-retry, ...). Ok, good. /Ricard -- Ricard Wolf Wanderlöf ricardw(at)axis.com Axis Communications AB, Lund, Sweden www.axis.com Phone +46 46 272 2016 Fax +46 46 13 61 30
[toc] | [prev] | [next] | [standalone]
| From | Mychaela Falconia <mychaela.falconia@gmail.com> |
|---|---|
| Date | 2016-06-09 19:30 +0200 |
| Message-ID | <rIbWi-10d-37@gated-at.bofh.it> |
| In reply to | #1418166 |
On 6/9/16, Ricard Wanderlof <ricard.wanderlof@axis.com> wrote: > The designers of this IP apparantly did not have Linux in mind when they > designed the controller, since it does all the low level stuff > autonomously (in the right IP configuration it can even remap flash blocks > transparently), with no way of intervening. For instance, when doing > hardware ECC, the OOB data is not available anywhere to the user, and if > one wants to actually read it a separate OOB write needs to be done. I > think the target market for the IP is really a general real time OS where > there is no NAND driver available, and you just want to fire off a single > high level command, wait for an interrupt, and have your data waiting for > you. I expect to see more and more newer NAND flash controllers that are like this. The one I am working with (FTNANDC024 from Faraday) is like this too - very very high-level. > This latter property is actually advantageous in Linux too as the driver > doesn't have to do bit- and byte-banging against the NAND flash. I'm not > sure what the gain in overhead is in practice, but at any rate there's not > much of a choice. It should be advantageous to any OS that uses the abstractions provided by the hardware instead of fighting them. The problem is that the Linux MTD system's current idea of what a NAND controller should look like is now out of sync with the new hardware realities. > Given that the controller does not have the transparency that the > ->cmd_ctrl() approach requires, as noted above, I can't see how it could > be simplified. > > I basically need to grab everything needed for a given operation and > interpret it before handing it over to the controller. I considered using > a higher level API, by replacing the default ->cmdfunc() (default > nand_command/nand_command_lp) with a specific version, which would have > avoided the need to interpret the NAND commands arriving via ->cmd_ctrl(), > but that meant duplicating some of the logic in nand_base.c which seemed > like a bad idea. For my FTNANDC024 driver I went for an ever more radical approach: I decided to forego the "nand" layer in Linux entirely and attach my driver directly to the MTD layer. There is very little that nand_base.c provides that is useful to a high-level controller whose abstractions are "read these logical sectors", "write these logical sectors" and "erase these blocks", it is really only useful for the simpler NAND controllers that don't do all of the heavy lifting in hardware. And it is NOT a question of "optimization" - the problem is not that going through the paradigm imposed by nand_base.c precludes the use of some optional higher-performance features of smart controllers - instead the controllers which you and I are working with *require* the use of their highly abstracted interfaces, and *do not* provide any kind of raw or transparent pass-through mode. M~
[toc] | [prev] | [next] | [standalone]
| From | Boris Brezillon <boris.brezillon@free-electrons.com> |
|---|---|
| Date | 2016-06-09 20:10 +0200 |
| Message-ID | <rIcyZ-1vR-5@gated-at.bofh.it> |
| In reply to | #1418520 |
On Thu, 9 Jun 2016 09:24:19 -0800 Mychaela Falconia <mychaela.falconia@gmail.com> wrote: > On 6/9/16, Ricard Wanderlof <ricard.wanderlof@axis.com> wrote: > > The designers of this IP apparantly did not have Linux in mind when they > > designed the controller, since it does all the low level stuff > > autonomously (in the right IP configuration it can even remap flash blocks > > transparently), with no way of intervening. For instance, when doing > > hardware ECC, the OOB data is not available anywhere to the user, and if > > one wants to actually read it a separate OOB write needs to be done. I > > think the target market for the IP is really a general real time OS where > > there is no NAND driver available, and you just want to fire off a single > > high level command, wait for an interrupt, and have your data waiting for > > you. > > I expect to see more and more newer NAND flash controllers that are > like this. The one I am working with (FTNANDC024 from Faraday) is like > this too - very very high-level. Hm, I'm not so sure. I've seen a lot of recent drivers that still provide this low-level interface, while providing means to optimize things if the user care about implementing software support for it. And AFAICT, these 'high-level controllers' are not a new thing. > > > This latter property is actually advantageous in Linux too as the driver > > doesn't have to do bit- and byte-banging against the NAND flash. I'm not > > sure what the gain in overhead is in practice, but at any rate there's not > > much of a choice. > > It should be advantageous to any OS that uses the abstractions > provided by the hardware instead of fighting them. True. > The problem is that > the Linux MTD system's current idea of what a NAND controller should > look like is now out of sync with the new hardware realities. I also agree on this aspect. Though what you consider an evolution with these 'high-level' controllers is in my opinion a regression. By supporting only a subset of what NAND chips actually support, and preventing any raw access, you just limit the compatibility of the NAND controller with rather old NAND chips. For example, your controller cannot deal with MLC/TLC NANDs because it just can't send the private commands required to have reliable support for these NANDs. > > > Given that the controller does not have the transparency that the > > ->cmd_ctrl() approach requires, as noted above, I can't see how it could > > be simplified. > > > > I basically need to grab everything needed for a given operation and > > interpret it before handing it over to the controller. I considered using > > a higher level API, by replacing the default ->cmdfunc() (default > > nand_command/nand_command_lp) with a specific version, which would have > > avoided the need to interpret the NAND commands arriving via ->cmd_ctrl(), > > but that meant duplicating some of the logic in nand_base.c which seemed > > like a bad idea. > > For my FTNANDC024 driver I went for an ever more radical approach: I > decided to forego the "nand" layer in Linux entirely and attach my > driver directly to the MTD layer. There is very little that > nand_base.c provides that is useful to a high-level controller whose > abstractions are "read these logical sectors", "write these logical > sectors" and "erase these blocks", it is really only useful for the > simpler NAND controllers that don't do all of the heavy lifting in > hardware. Yep, except what you call simple controllers are actually not simpler than yours, they just provide a raw interface in addition to their advanced/high-level logic in order to let the software support features that were not supported when the controller IP was designed. This is IMO a much saner design than limiting the interface to higher level abstraction, which are likely to be compatible with only a subset of all the NAND devices available out there. > > And it is NOT a question of "optimization" - the problem is not that > going through the paradigm imposed by nand_base.c precludes the use of > some optional higher-performance features of smart controllers - > instead the controllers which you and I are working with *require* the > use of their highly abstracted interfaces, and *do not* provide any > kind of raw or transparent pass-through mode. I understand this constraint, and I know some controllers are really like this, but before deciding to move those controllers to some 'high-level NAND' framework, I'd like to make sure they are really not providing this raw interface. I've been told many times that NAND controllers were not supporting raw accesses (disabling the ECC engine when accessing the NAND), and most of the time it was false, but the developers just didn't care about supporting this feature, and things like that make the whole subsystem unmaintainable. I fear the same will happen with this high-level interface: once we'll add it, people will just decide to re-implement everything on their own, and support only the set of feature they need. And we'll end-up with another set of unmaintainable drivers. So my answer is yes, I'm okay providing this high level NAND controller framework, but I'd like to make sure drivers going in there are not abusing it, just because it's simpler to implement a driver for their specific use-case. And given the CMD registers exposed by the Evatronix IP I had the feeling that this low level interface was available... -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
[toc] | [prev] | [next] | [standalone]
| From | Mychaela Falconia <mychaela.falconia@gmail.com> |
|---|---|
| Date | 2016-06-09 21:40 +0200 |
| Message-ID | <rIdY5-2jk-9@gated-at.bofh.it> |
| In reply to | #1418541 |
On 6/9/16, Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > I also agree on this aspect. Though what you consider an evolution with > these 'high-level' controllers is in my opinion a regression. > > By supporting only a subset of what NAND chips actually support, and > preventing any raw access, you just limit the compatibility of the NAND > controller with rather old NAND chips. For example, your controller > cannot deal with MLC/TLC NANDs because it just can't send the private > commands required to have reliable support for these NANDs. I am not the one who designed the controller IP, so please don't shoot the messenger. I fully agree that it would have been much safer if they just bothered to add a couple of registers to their IP that provide a raw bypass directly to the NAND interface pins - and it would have been trivial to do so, adding just a few transistors to the silicon - but apparently the IP vendor chose not to. I agree that it was a poor decision on their part, but as Linux geeks we don't get the power to change the IP we got hired to write drivers for. As for private commands which the IP does not know about, there is a way to send them, but it is not a truly raw way. With FTNANDC024 if you need to send a private command to the NAND, you have to write some custom FTNANDC024 microcode for it. The datasheet explains how to write your own microcode, and the IP includes a small SRAM where you can put your own microcode in addition to the mask ROM with the standard microcode. But this provision still would not allow a low-level driver implementation: there is still no way to implement a driver function that just sends any arbitrary command opcode without caring what it is, instead you would have to write a different special microcode routine for each non-standard command you need to support, and naturally the microcode SRAM has a finite size. > I understand this constraint, and I know some controllers are really > like this, but before deciding to move those controllers to some > 'high-level NAND' framework, I'd like to make sure they are really not > providing this raw interface. I would be glad to share the datasheet for the controller I am working with, so you can verify with your own eyes that it does not provide any truly raw interface - but I don't have a place to post it. If you like, I can send it to you as an off-list email attachment (1775641 bytes), and you can then repost it somewhere for others to see. > I've been told many times that NAND controllers were not supporting raw > accesses (disabling the ECC engine when accessing the NAND), and most > of the time it was false, but the developers just didn't care about > supporting this feature, and things like that make the whole subsystem > unmaintainable. With FTNANDC024 the closest you can get to a raw read are the following two features: 1. You can disable the ECC engine and read the first "data size" bytes of the page raw, but it does not get you the OOB area. For example, on the Micron SLC chip I am working with currently, the raw page size is 4096+224 bytes, but the FTNANDC024 can only read the 4096 "data" bytes this way, and not the remaining 224. 2. There is a "byte read" command that performs a truly raw read of any range of bytes (any column address) within a page, but it can only read a maximum of 32 bytes per operation. If I wanted to do a truly raw dump of a full NAND page with my controller and NAND chip (like I did when I wanted to reverse-engineer their hardware BCH implementation), I would have to issue one read command to get the first 4096 bytes, then another 7 "byte read" commands to get the remaining 224 bytes. 8 read commands in total to get a raw dump of one full NAND page, and each of those 8 FTNANDC024 read commands internally involves sending a new Read Page command to the physical NAND - thus any read disturbs are invoked 8 times per page instead of once. It's even worse with raw writes - if I wanted to do a raw write of a full NAND page, I would have to do it in 8 separate write commands just like with the raw read. I assume that the NAND chip won't like it at all, as it would get 8 separate Page Program commands for the same page with different column addresses. M~
[toc] | [prev] | [next] | [standalone]
| From | Boris Brezillon <boris.brezillon@free-electrons.com> |
|---|---|
| Date | 2016-06-09 22:30 +0200 |
| Message-ID | <rIeKu-2SW-3@gated-at.bofh.it> |
| In reply to | #1418604 |
On Thu, 9 Jun 2016 11:35:53 -0800 Mychaela Falconia <mychaela.falconia@gmail.com> wrote: > On 6/9/16, Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > > I also agree on this aspect. Though what you consider an evolution with > > these 'high-level' controllers is in my opinion a regression. > > > > By supporting only a subset of what NAND chips actually support, and > > preventing any raw access, you just limit the compatibility of the NAND > > controller with rather old NAND chips. For example, your controller > > cannot deal with MLC/TLC NANDs because it just can't send the private > > commands required to have reliable support for these NANDs. > > I am not the one who designed the controller IP, so please don't shoot > the messenger. Yes, sorry, I was just over-reacting because I'm tired earing that the only solution to get a high performances is to hide everything in the controller which is likely to be incompatible with NANDs we'll see in a few month from there. > I fully agree that it would have been much safer if > they just bothered to add a couple of registers to their IP that > provide a raw bypass directly to the NAND interface pins - and it > would have been trivial to do so, adding just a few transistors to the > silicon - but apparently the IP vendor chose not to. I agree that it > was a poor decision on their part, but as Linux geeks we don't get the > power to change the IP we got hired to write drivers for. Hm, I think it's changing now that a lot of SoCs are advertised to be running Linux. But you're right in that existing IPs might not support this low-level mode. > > As for private commands which the IP does not know about, there is a > way to send them, but it is not a truly raw way. With FTNANDC024 if > you need to send a private command to the NAND, you have to write some > custom FTNANDC024 microcode for it. The datasheet explains how to > write your own microcode, and the IP includes a small SRAM where you > can put your own microcode in addition to the mask ROM with the > standard microcode. But this provision still would not allow a > low-level driver implementation: there is still no way to implement a > driver function that just sends any arbitrary command opcode without > caring what it is, instead you would have to write a different special > microcode routine for each non-standard command you need to support, > and naturally the microcode SRAM has a finite size. Hm, I don't understand why it's not possible to implement basic sequences, but I don't know anything about FTNANDC024, so let's assume you're right. > > > I understand this constraint, and I know some controllers are really > > like this, but before deciding to move those controllers to some > > 'high-level NAND' framework, I'd like to make sure they are really not > > providing this raw interface. > > I would be glad to share the datasheet for the controller I am working > with, so you can verify with your own eyes that it does not provide > any truly raw interface - but I don't have a place to post it. If you > like, I can send it to you as an off-list email attachment (1775641 > bytes), and you can then repost it somewhere for others to see. Sure, feel free to send it to me, I'll have a look. And maybe you can also share your code (both the new and old versions of the driver). > > > I've been told many times that NAND controllers were not supporting raw > > accesses (disabling the ECC engine when accessing the NAND), and most > > of the time it was false, but the developers just didn't care about > > supporting this feature, and things like that make the whole subsystem > > unmaintainable. > > With FTNANDC024 the closest you can get to a raw read are the > following two features: The raw vs ECC mode thing was just an example to illustrate my point: people usually lie (intentionally or not) about what's really supported :). > > 1. You can disable the ECC engine and read the first "data size" bytes > of the page raw, but it does not get you the OOB area. For example, on > the Micron SLC chip I am working with currently, the raw page size is > 4096+224 bytes, but the FTNANDC024 can only read the 4096 "data" bytes > this way, and not the remaining 224. Yes, that's a problem. > > 2. There is a "byte read" command that performs a truly raw read of > any range of bytes (any column address) within a page, but it can only > read a maximum of 32 bytes per operation. Raw access is usually not something you expect to be fast, it's here to help people debugging their implementation, or providing a fallback when ECC correction failed (which should not happen that often). So it's fine if it's providing fast. > > If I wanted to do a truly raw dump of a full NAND page with my > controller and NAND chip (like I did when I wanted to reverse-engineer > their hardware BCH implementation), I would have to issue one read > command to get the first 4096 bytes, then another 7 "byte read" > commands to get the remaining 224 bytes. 8 read commands in total to > get a raw dump of one full NAND page, and each of those 8 FTNANDC024 > read commands internally involves sending a new Read Page command to > the physical NAND - thus any read disturbs are invoked 8 times per > page instead of once. Hm, so you can't even move the column pointer within a page (NAND_CMD_RNDOUT)? Even if it's the case, as I said, raw access is mainly here for debugging purpose, so the read-disturbance caused by the multiple page load operations shouldn't be a problem. > > It's even worse with raw writes - if I wanted to do a raw write of a > full NAND page, I would have to do it in 8 separate write commands > just like with the raw read. I assume that the NAND chip won't like it > at all, as it would get 8 separate Page Program commands for the same > page with different column addresses. Nope, this one won't work. That's completely crazy to prevent one from doing such basic things, but given you previous explanation I'm not really surprised. Now back to the Evatronix IP. I had a closer look at Ricard's code, and it seems the controller is actually supporting a low-level mode (command seq 18 and 19 + MAKE_GEN_CMD()). So my suggestion is to implement ->cmd_ctrl() and use these generic commands. And of course optimized/packed accesses (using specific commands) can be used in ecc->read/write_page(). This would require a flag to ask the core to not send the READ/WRITE commands before calling ecc->read/write_page(), but that's doable. All other commands would use the generic mode, since they don't require high performances or any specific handling. This way you don't have to implement your own ->cmdfunc() function, you can get rid of all the specific ID/ONFI detection logic (the generic commands should allow you to retrieve those information) and rely on the default implementation. Ricard, would that work? -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
[toc] | [prev] | [next] | [standalone]
| From | Mychaela Falconia <mychaela.falconia@gmail.com> |
|---|---|
| Date | 2016-06-10 07:10 +0200 |
| Message-ID | <rImRH-lD-7@gated-at.bofh.it> |
| In reply to | #1418627 |
On 6/9/16, Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > Hm, I think it's changing now that a lot of SoCs are advertised to be > running Linux. But you're right in that existing IPs might not support > this low-level mode. Faraday (the IP vendor in the present case) advertise Linux support as well, but they never mainlined any of it, and instead they provide their own vendor Linux trees. The one I got is based on linux-3.3; I don't know if they have a newer one. They do "support" FTNANDC024 under Linux as well, but their driver for it is gawdawful - see below. > Hm, I don't understand why it's not possible to implement basic > sequences, but I don't know anything about FTNANDC024, so let's assume > you're right. Read the datasheet (link below) and tell me what you think. > Sure, feel free to send it to me, I'll have a look. And maybe you can > also share your code (both the new and old versions of the driver). I decided to go ahead and abuse my personal web space on another (nothing to do with Linux or with NAND flash) project's server for the purpose of sharing this stuff: https://www.freecalypso.org/members/falcon/linux-mtd/ There you will find the IP datasheet, the vendor's driver (GPL), and a current snapshot of my work-in-progress replacement. > Hm, so you can't even move the column pointer within a page > (NAND_CMD_RNDOUT)? See the FTNANDC024 microcode listings on datasheet PDF pages 108 through 117. Every FTNANDC024 operation is an execution of one of these complete microcode routines from start to finish. Just because a given microcode flow includes the issuance of a given NAND command (such as Change Read Column or Change Write Column) does not mean that you could just ask the controller to issue that command by itself, without executing a complete microcode flow which also includes the Read Page or Program Page command. The only workaround would be to write our own microcode. I think this approach would actually work: we could write shorter microcode routines which *just* issue a given NAND opcode and then stop there, and another separate microcode routine (to be invoked via a separate command) which would only do what they call "RD_SP" or "WR_SP" (raw byte transfers of 1 to 32 bytes), without issuing a Read Page command before or a Program Page command after. This approach would allow us to perform truly raw page reads and writes, but it would be very ugly and inefficient. It would also require a separate microcode routine for each different command, NOT one generic microcode routine that would correspond to ->cmd_ctrl() or ->cmdfunc(). M~
[toc] | [prev] | [next] | [standalone]
| From | Boris Brezillon <boris.brezillon@free-electrons.com> |
|---|---|
| Date | 2016-06-10 14:50 +0200 |
| Message-ID | <rIu2R-4CJ-3@gated-at.bofh.it> |
| In reply to | #1419022 |
On Thu, 9 Jun 2016 21:07:49 -0800 Mychaela Falconia <mychaela.falconia@gmail.com> wrote: > On 6/9/16, Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > > Hm, I think it's changing now that a lot of SoCs are advertised to be > > running Linux. But you're right in that existing IPs might not support > > this low-level mode. > > Faraday (the IP vendor in the present case) advertise Linux support as > well, but they never mainlined any of it, and instead they provide > their own vendor Linux trees. The one I got is based on linux-3.3; I > don't know if they have a newer one. They do "support" FTNANDC024 > under Linux as well, but their driver for it is gawdawful - see below. > > > Hm, I don't understand why it's not possible to implement basic > > sequences, but I don't know anything about FTNANDC024, so let's assume > > you're right. > > Read the datasheet (link below) and tell me what you think. > > > Sure, feel free to send it to me, I'll have a look. And maybe you can > > also share your code (both the new and old versions of the driver). > > I decided to go ahead and abuse my personal web space on another > (nothing to do with Linux or with NAND flash) project's server for the > purpose of sharing this stuff: > > https://www.freecalypso.org/members/falcon/linux-mtd/ > > There you will find the IP datasheet, the vendor's driver (GPL), and a > current snapshot of my work-in-progress replacement. Thanks for sharing that. That's actually a quite interesting beast, and it's way more evolved than I first thought. I might be wrong, but it seems that ->cmd_ctrl() can be supported using the custom NAND op approach (custom uCode in SRAM). This doesn't prevent you from optimizing things for operations where performances really matter (read/write with ECC), by using advanced sequencing (reusing the supported cmdset, or even implementing your own). I'm actually impressed by the degree of liberty this controller gives: while some sequences are provided you can create you own ones and still benefit from the controller optimizations. Didn't look at the code yet, but I'm pretty confident we'll be able to make the driver fit in the existing model, and that moving to the new model (where I plan to give more freedom to the controller), will make it even more interesting. I'll try to come with a proposal for you to test/review after reviewing the code. > > > Hm, so you can't even move the column pointer within a page > > (NAND_CMD_RNDOUT)? > > See the FTNANDC024 microcode listings on datasheet PDF pages 108 > through 117. Every FTNANDC024 operation is an execution of one of > these complete microcode routines from start to finish. Just because a > given microcode flow includes the issuance of a given NAND command > (such as Change Read Column or Change Write Column) does not mean that > you could just ask the controller to issue that command by itself, > without executing a complete microcode flow which also includes the > Read Page or Program Page command. True. > > The only workaround would be to write our own microcode. I think this > approach would actually work: we could write shorter microcode > routines which *just* issue a given NAND opcode and then stop there, > and another separate microcode routine (to be invoked via a separate > command) which would only do what they call "RD_SP" or "WR_SP" (raw > byte transfers of 1 to 32 bytes), without issuing a Read Page command > before or a Program Page command after. This approach would allow us > to perform truly raw page reads and writes, but it would be very ugly > and inefficient. It would also require a separate microcode routine > for each different command, NOT one generic microcode routine that > would correspond to ->cmd_ctrl() or ->cmdfunc(). Let's see if we can do something smarter. Note that my proposal was to bypass ->cmd_ctrl() usage for path that are requiring high-perfs (ecc->read/write_page()), and only rely on if for the other operations, where performances are not important, but re-usability of existing code is (I'm thinking of NAND detection here). -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
[toc] | [prev] | [next] | [standalone]
| From | Ricard Wanderlof <ricard.wanderlof@axis.com> |
|---|---|
| Date | 2016-06-10 16:30 +0200 |
| Subject | Re: [PATCH 3/4] mtd: nand: Add support for Evatronix NANDFLASH-CTRL |
| Message-ID | <rIvBE-5Ff-9@gated-at.bofh.it> |
| In reply to | #1418627 |
On Thu, 9 Jun 2016, Boris Brezillon wrote:
> > >
> > > By supporting only a subset of what NAND chips actually support, and
> > > preventing any raw access, you just limit the compatibility of the NAND
> > > controller with rather old NAND chips. For example, your controller
> > > cannot deal with MLC/TLC NANDs because it just can't send the private
> > > commands required to have reliable support for these NANDs.
> >
> > I am not the one who designed the controller IP, so please don't shoot
> > the messenger.
>
> Yes, sorry, I was just over-reacting because I'm tired earing that the
> only solution to get a high performances is to hide everything in the
> controller which is likely to be incompatible with NANDs we'll see in a
> few month from there.
I don't know what the situation is with other NAND drivers and controller
IP's, but in my case, the set of NAND flashes which the SoC in which the
Evatronix IP is included is intended to operate with is fairly limited,
partly because our products don't require a great veriety, and partly for
SoC verification reasons (the fewer flash types tested, the less time and
money, essentially). So the mindset from the outset was 'we need to
support these flashes, can we do it', rather than 'we want a general
driver', which in the end is reflected in the somewhat limited set of
flash types initially supported by the driver.
I fully understand that the opposite is true of the Linux kernel, which is
intended to be as general as possible, I'm just trying to offer an
explanation for the somewhat limited scope of driver developers,
especially those working on company time, the understanding of which
eventually might lead to use finding ways to solve this dilemma.
FWIW, in the Evatronix case, it wasn't any performance issue that drove
the driver development, it just seemed like the Right Thing to Do.
> > > I've been told many times that NAND controllers were not supporting raw
> > > accesses (disabling the ECC engine when accessing the NAND), and most
> > > of the time it was false, but the developers just didn't care about
> > > supporting this feature, and things like that make the whole subsystem
> > > unmaintainable.
Yeah, I've come across a driver (not in mainline though) with precisely
this problem. The hardware supported hardware BCH ECC, but without
returning the number of corrected bits, which made almost useless, as
there was no way of detecting when the number of bits in an ECC block was
nearing the limit for a read failure. The solution was to implement
software ECC, but the driver didn't really support this mode to start with
and it had to be added.
(FWIW, the Evatronix driver does support both hardware and software ECC,
the latter mostly intended for verification and debugging purposes).
> Now back to the Evatronix IP. I had a closer look at Ricard's code, and
> it seems the controller is actually supporting a low-level mode
> (command seq 18 and 19 + MAKE_GEN_CMD()).
Yes, that's true, it is labelled as a 'generic command sequence' in the
document. Well, it's not really a low level mode, as you can see you still
need to do the whole operation in one go, but in the end that is what it
accomplishes.
> So my suggestion is to implement ->cmd_ctrl() and use these generic
> commands. And of course optimized/packed accesses (using specific
> commands) can be used in ecc->read/write_page().
Actually, the use of ECC or not is governed outside the IP command set. I
already use the generic command sequence (SEQ18/19) for ECC reads and
writes towards flashes with 4 byte addresses. So it should be doable to
use the generic command sequencer for any number of address bytes, both
with and without ECC.
> This would require a flag to ask the core to not send the READ/WRITE
> commands before calling ecc->read/write_page(), but that's doable.
Ok, so a change required in the core to get this to work then. I've tended
to avoid writing driver code so that it requires changes to the framwork
unless absolutely necessary, as the changes tend to be rather specific and
clutter up the general code (which, honestly, is bad enough as it is, not
trying to blame anyone here, just an observation), and can usually be
resolved in the driver with a bit of ingenuity.
> All other commands would use the generic mode, since they don't require
> high performances or any specific handling.
>
> This way you don't have to implement your own ->cmdfunc() function, you
> can get rid of all the specific ID/ONFI detection logic (the generic
> commands should allow you to retrieve those information)
FWIW, there isn't much ID detection logic, although looking at it some of
the comments imply that there is (and that should be changed of course),
because the ID type byte is actually identical to what the controller uses
to select the relevant type.
> and rely on the default implementation.
>
> Ricard, would that work?
The main reason the I've been using the ->cmdfunc() interface is that the
API is on a fairly high level ("here's a sequence of address bytes", "read
a page", etc) which is on similar level to the API to the actual IP (i.e.
"read a page from this address").
In contrast, using the ->cmd_ctrl() interface means that I've got to
interpret a sequence of bytes coming in as multiple function calls. It
just seemed like a bad idea compared to interpreting a set of high level
commands, given that the controller also has a high level API. It seemed a
bit like a roundabout way letting someone (i.e. nand_command) encode a
high level command into several low level operations, which must then be
deciphered by the flash driver one by one in order to assemble another
high level command. It seemed much more direct to process the high level
commands directly - and easier to read, as the required operations are
directly visible as macros. The ->cmd_ctrl() interface is fine for
byte-banging an 8-bit I/O port as that was what it was designed for, but
quite simply seemed to be the wrong level for anything more advanced than
that. Sortof akin to reading a file with getc() rather than read(), which
is why I never really considered it.
But I can definitely see your point, especially as maintainer with the
goal of supporting as many devices as possible, and also considering your
plans (as you mentioned in another reply) to rework the API, which would
mean that the ->cmdfunc() API would be changing, with the associated
changes in drivers that use that API.
Looking at the documentation, it does look doable, but to a certain extent
it's in the category "can't really tell until I've tried it". In the worst
case, some operations would still need to use specific IP commands but
they should be few.
It's a fairly extensive rewrite though, as a lot of the internal logic of
the driver is based on the external API being a high level, even if the
code in the end will be simpler.
The company I work for has an explicit goal of getting as much of the
Linux port for our SoC upstream, so hopefully I can find time to rewrite
this in the near future, although I'm off on a fairly long summer vacation
shortly. I'll try to get it underway as soon as I'm back.
Something that I am mildly miffed about is that I've posted this driver
twice before on the mtd list (although, admittedly, not directly addressed
to any maintainer), first as an RFC and later as a complete patch, without
a single response. (Apparently Boris you did respond with comments on
Oleksij's driver though which I must have missed). Although an RFC might
not have initiated a detailed review, it would have been a large advantage
(and wasted less time for all) if the point of using 'wrong' driver
interface had been brought up and consequently discussed earlier. Yes I
know, everyone is busy, it's easy to miss things, each and every driver
can't be reviewed in detail, etc.
/Ricard
--
Ricard Wolf Wanderlöf ricardw(at)axis.com
Axis Communications AB, Lund, Sweden www.axis.com
Phone +46 46 272 2016 Fax +46 46 13 61 30
[toc] | [prev] | [next] | [standalone]
| From | Boris Brezillon <boris.brezillon@free-electrons.com> |
|---|---|
| Date | 2016-06-10 18:10 +0200 |
| Message-ID | <rIxap-6KH-7@gated-at.bofh.it> |
| In reply to | #1419474 |
On Fri, 10 Jun 2016 16:22:38 +0200
Ricard Wanderlof <ricard.wanderlof@axis.com> wrote:
> On Thu, 9 Jun 2016, Boris Brezillon wrote:
>
> > > >
> > > > By supporting only a subset of what NAND chips actually support, and
> > > > preventing any raw access, you just limit the compatibility of the NAND
> > > > controller with rather old NAND chips. For example, your controller
> > > > cannot deal with MLC/TLC NANDs because it just can't send the private
> > > > commands required to have reliable support for these NANDs.
> > >
> > > I am not the one who designed the controller IP, so please don't shoot
> > > the messenger.
> >
> > Yes, sorry, I was just over-reacting because I'm tired earing that the
> > only solution to get a high performances is to hide everything in the
> > controller which is likely to be incompatible with NANDs we'll see in a
> > few month from there.
>
> I don't know what the situation is with other NAND drivers and controller
> IP's, but in my case, the set of NAND flashes which the SoC in which the
> Evatronix IP is included is intended to operate with is fairly limited,
> partly because our products don't require a great veriety, and partly for
> SoC verification reasons (the fewer flash types tested, the less time and
> money, essentially). So the mindset from the outset was 'we need to
> support these flashes, can we do it', rather than 'we want a general
> driver', which in the end is reflected in the somewhat limited set of
> flash types initially supported by the driver.
>
> I fully understand that the opposite is true of the Linux kernel, which is
> intended to be as general as possible, I'm just trying to offer an
> explanation for the somewhat limited scope of driver developers,
> especially those working on company time, the understanding of which
> eventually might lead to use finding ways to solve this dilemma.
>
> FWIW, in the Evatronix case, it wasn't any performance issue that drove
> the driver development, it just seemed like the Right Thing to Do.
>
> > > > I've been told many times that NAND controllers were not supporting raw
> > > > accesses (disabling the ECC engine when accessing the NAND), and most
> > > > of the time it was false, but the developers just didn't care about
> > > > supporting this feature, and things like that make the whole subsystem
> > > > unmaintainable.
>
> Yeah, I've come across a driver (not in mainline though) with precisely
> this problem. The hardware supported hardware BCH ECC, but without
> returning the number of corrected bits, which made almost useless, as
> there was no way of detecting when the number of bits in an ECC block was
> nearing the limit for a read failure. The solution was to implement
> software ECC, but the driver didn't really support this mode to start with
> and it had to be added.
>
> (FWIW, the Evatronix driver does support both hardware and software ECC,
> the latter mostly intended for verification and debugging purposes).
>
> > Now back to the Evatronix IP. I had a closer look at Ricard's code, and
> > it seems the controller is actually supporting a low-level mode
> > (command seq 18 and 19 + MAKE_GEN_CMD()).
>
> Yes, that's true, it is labelled as a 'generic command sequence' in the
> document. Well, it's not really a low level mode, as you can see you still
> need to do the whole operation in one go, but in the end that is what it
> accomplishes.
>
> > So my suggestion is to implement ->cmd_ctrl() and use these generic
> > commands. And of course optimized/packed accesses (using specific
> > commands) can be used in ecc->read/write_page().
>
> Actually, the use of ECC or not is governed outside the IP command set. I
> already use the generic command sequence (SEQ18/19) for ECC reads and
> writes towards flashes with 4 byte addresses. So it should be doable to
> use the generic command sequencer for any number of address bytes, both
> with and without ECC.
>
> > This would require a flag to ask the core to not send the READ/WRITE
> > commands before calling ecc->read/write_page(), but that's doable.
>
> Ok, so a change required in the core to get this to work then. I've tended
> to avoid writing driver code so that it requires changes to the framwork
> unless absolutely necessary, as the changes tend to be rather specific and
> clutter up the general code (which, honestly, is bad enough as it is, not
> trying to blame anyone here, just an observation), and can usually be
> resolved in the driver with a bit of ingenuity.
No offense, that's my feeling too, hence the various reworks I've
recently initiated. Getting rid of the ->cmd_ctrl() ->cmdfunc()
approach in favor of a more generic ->exec_op() approach is one of them,
just need some time to figure a way to do it smoothly.
>
> > All other commands would use the generic mode, since they don't require
> > high performances or any specific handling.
> >
> > This way you don't have to implement your own ->cmdfunc() function, you
> > can get rid of all the specific ID/ONFI detection logic (the generic
> > commands should allow you to retrieve those information)
>
> FWIW, there isn't much ID detection logic, although looking at it some of
> the comments imply that there is (and that should be changed of course),
> because the ID type byte is actually identical to what the controller uses
> to select the relevant type.
>
> > and rely on the default implementation.
> >
> > Ricard, would that work?
>
> The main reason the I've been using the ->cmdfunc() interface is that the
> API is on a fairly high level ("here's a sequence of address bytes", "read
> a page", etc) which is on similar level to the API to the actual IP (i.e.
> "read a page from this address").
>
> In contrast, using the ->cmd_ctrl() interface means that I've got to
> interpret a sequence of bytes coming in as multiple function calls. It
> just seemed like a bad idea compared to interpreting a set of high level
> commands, given that the controller also has a high level API. It seemed a
> bit like a roundabout way letting someone (i.e. nand_command) encode a
> high level command into several low level operations, which must then be
> deciphered by the flash driver one by one in order to assemble another
> high level command. It seemed much more direct to process the high level
> commands directly - and easier to read, as the required operations are
> directly visible as macros. The ->cmd_ctrl() interface is fine for
> byte-banging an 8-bit I/O port as that was what it was designed for, but
> quite simply seemed to be the wrong level for anything more advanced than
> that. Sortof akin to reading a file with getc() rather than read(), which
> is why I never really considered it.
>
> But I can definitely see your point, especially as maintainer with the
> goal of supporting as many devices as possible, and also considering your
> plans (as you mentioned in another reply) to rework the API, which would
> mean that the ->cmdfunc() API would be changing, with the associated
> changes in drivers that use that API.
>
> Looking at the documentation, it does look doable, but to a certain extent
> it's in the category "can't really tell until I've tried it". In the worst
> case, some operations would still need to use specific IP commands but
> they should be few.
>
> It's a fairly extensive rewrite though, as a lot of the internal logic of
> the driver is based on the external API being a high level, even if the
> code in the end will be simpler.
>
> The company I work for has an explicit goal of getting as much of the
> Linux port for our SoC upstream, so hopefully I can find time to rewrite
> this in the near future, although I'm off on a fairly long summer vacation
> shortly. I'll try to get it underway as soon as I'm back.
>
> Something that I am mildly miffed about is that I've posted this driver
> twice before on the mtd list (although, admittedly, not directly addressed
> to any maintainer), first as an RFC and later as a complete patch, without
> a single response. (Apparently Boris you did respond with comments on
> Oleksij's driver though which I must have missed). Although an RFC might
> not have initiated a detailed review, it would have been a large advantage
> (and wasted less time for all) if the point of using 'wrong' driver
> interface had been brought up and consequently discussed earlier. Yes I
> know, everyone is busy, it's easy to miss things, each and every driver
> can't be reviewed in detail, etc.
Yes, I know. I was not maintainer at that time, and was only reviewing
a few patches from time to time.
Now that I am, I try to answer as fast as possible. Note that the set
of requirements might have change with me, so even if someone add
agreed on you first submission but not taken the patches, I would have
required the same set of changes ;).
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[toc] | [prev] | [next] | [standalone]
| From | Ricard Wanderlof <ricard.wanderlof@axis.com> |
|---|---|
| Date | 2016-06-10 19:00 +0200 |
| Subject | Re: [PATCH 3/4] mtd: nand: Add support for Evatronix NANDFLASH-CTRL |
| Message-ID | <rIxWO-74A-23@gated-at.bofh.it> |
| In reply to | #1419549 |
On Fri, 10 Jun 2016, Boris Brezillon wrote: > > Something that I am mildly miffed about is that I've posted this driver > > twice before on the mtd list (although, admittedly, not directly addressed > > to any maintainer), first as an RFC and later as a complete patch, without > > a single response. > > ... > > Yes, I know. I was not maintainer at that time, and was only reviewing > a few patches from time to time. > Now that I am, I try to answer as fast as possible. Note that the set > of requirements might have change with me, so even if someone add > agreed on you first submission but not taken the patches, I would have > required the same set of changes ;). :-) At any rate, thanks for taking the time to review the driver so promptly and also the for the time you've spent during the subsequent discussions. /Ricard -- Ricard Wolf Wanderlöf ricardw(at)axis.com Axis Communications AB, Lund, Sweden www.axis.com Phone +46 46 272 2016 Fax +46 46 13 61 30
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web