Re: [PATCH 2/2] mtd: nand: driver for Conexant Digicolor NAND Flash Controller

[Boris Brezillon <boris.brezillon@free-electrons.com>]

Hi Baruch,

On Thu, 12 Feb 2015 13:10:19 +0200
Baruch Siach <baruch@tkos.co.il> wrote:

> This commit adds driver for the NAND flash controller on the CX92755 SoC. This
> SoC is one of the Conexant Digicolor series, and this driver should support
> other SoCs in this series.

I haven't done any coding style review here, so make sure to fix
all errors/warnings reported by checkpatch if any.

> 
> Only hardware syndrome ECC mode is currently supported.
> 
> This driver was tested on the Equinox CX92755 EVK, with the Samsung K9GAG08U0E
> NAND chip (MLC, 8K pages, 436 bytes OOB). Test included attach of UBI volume,
> mount of UBIFS filesystem, and files read/write.

Could you also run mtd test (kernel module tests) and provide their
results in your next version ?

> 
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
>  drivers/mtd/nand/Kconfig          |   6 +
>  drivers/mtd/nand/Makefile         |   1 +
>  drivers/mtd/nand/digicolor_nand.c | 616 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 623 insertions(+)
>  create mode 100644 drivers/mtd/nand/digicolor_nand.c
> 
> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> index 7d0150d20432..035f0078e62e 100644
> --- a/drivers/mtd/nand/Kconfig
> +++ b/drivers/mtd/nand/Kconfig
> @@ -524,4 +524,10 @@ config MTD_NAND_SUNXI
>  	help
>  	  Enables support for NAND Flash chips on Allwinner SoCs.
>  
> +config MTD_NAND_DIGICOLOR
> +	tristate "Support for NAND on Conexant Digicolor SoCs"
> +	depends on ARCH_DIGICOLOR
> +	help
> +	  Enables support for NAND Flash chips on Conexant Digicolor SoCs.
> +
>  endif # MTD_NAND
> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
> index bd38f21d2e28..e7fd578123cd 100644
> --- a/drivers/mtd/nand/Makefile
> +++ b/drivers/mtd/nand/Makefile
> @@ -51,5 +51,6 @@ obj-$(CONFIG_MTD_NAND_GPMI_NAND)	+= gpmi-nand/
>  obj-$(CONFIG_MTD_NAND_XWAY)		+= xway_nand.o
>  obj-$(CONFIG_MTD_NAND_BCM47XXNFLASH)	+= bcm47xxnflash/
>  obj-$(CONFIG_MTD_NAND_SUNXI)		+= sunxi_nand.o
> +obj-$(CONFIG_MTD_NAND_DIGICOLOR)	+= digicolor_nand.o
>  
>  nand-objs := nand_base.o nand_bbt.o nand_timings.o
> diff --git a/drivers/mtd/nand/digicolor_nand.c b/drivers/mtd/nand/digicolor_nand.c
> new file mode 100644
> index 000000000000..5249953c7eba
> --- /dev/null
> +++ b/drivers/mtd/nand/digicolor_nand.c
> @@ -0,0 +1,616 @@
> +/*
> + *  Driver for Conexant Digicolor NAND Flash Controller
> + *
> + * Author: Baruch Siach <baruch@tkos.co.il>
> + *
> + * Copyright (C) 2014, 2015 Paradox Innovation Ltd.
> + *
> + * 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.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_mtd.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/mtd/nand.h>
> +#include <linux/mtd/partitions.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +
> +#define DRIVER_NAME "digicolor_nand"
> +
> +#define NFC_CONTROL			0x180
> +#define NFC_CONTROL_LOCAL_RESET		BIT(0)
> +#define NFC_CONTROL_ENABLE		BIT(1)
> +#define NFC_CONTROL_BCH_TCONFIG(n)	((n) << 13)
> +#define NFC_CONTROL_BCH_KCONFIG		(77 << 16)
> +#define NFC_CONTROL_BCH_DATA_FIELD_RANGE_1024	BIT(30)
> +
> +#define NFC_STATUS_1			0x18c
> +#define NFC_STATUS_1_CORRECTABLE_ERRORS(reg)	(((reg) >> 16) & 0x7ff)
> +#define NFC_STATUS_1_UNCORRECTED_ERROR	BIT(31)
> +
> +#define NFC_COMMAND			0x1a0
> +#define NFC_COMMAND_CODE_OFF		28
> +#define CMD_CHIP_ENABLE(n)		((~(1 << (n)) & 0xf) << 20)
> +#define CMD_STOP_ON_COMPLETE		BIT(15)
> +#define CMD_ECC_ENABLE			BIT(13)
> +#define CMD_TOGGLE			BIT(13) /* WAIT READY command */
> +#define CMD_ECC_STATUS			BIT(12)
> +#define CMD_RB_DATA			BIT(12) /* WAIT READY command */
> +#define CMD_NUMBER_BYTES(n)		((n) << 8) /* ALE command */

Unless you have good reason to keep this name I would name it
CMD_ADDR_CYCLES.

> +#define CMD_SKIP_LENGTH(n)		(((n) & 0xf) << 8) /* READ/WRITE */
> +#define CMD_SKIP_OFFSET(n)		((n) << 16)
> +#define CMD_RB_MASK(n)			((1 << (n)) & 0xf)
> +#define CMD_IMMEDIATE_DATA		0xff /* bad block mark */
> +
> +#define CMD_CLE				(0 << NFC_COMMAND_CODE_OFF)
> +#define CMD_ALE				(1 << NFC_COMMAND_CODE_OFF)
> +#define CMD_PAGEREAD			(2 << NFC_COMMAND_CODE_OFF)
> +#define CMD_PAGEWRITE			(3 << NFC_COMMAND_CODE_OFF)
> +#define CMD_WAITREADY			(4 << NFC_COMMAND_CODE_OFF)
> +#define CMD_WAIT			(5 << NFC_COMMAND_CODE_OFF)
> +
> +#define NFC_DATA			0x1c0
> +#define ALE_DATA_OFF(n)			(((n)-1)*8)
> +
> +#define NFC_TIMING_CONFIG		0x1c4
> +#define TIMING_ASSERT(clk)		((clk) & 0xff)
> +#define TIMING_DEASSERT(clk)		(((clk) & 0xff) << 8)
> +#define TIMING_SAMPLE(clk)		(((clk) & 0xf) << 24)
> +
> +#define NFC_INTFLAG_CLEAR		0x1c8
> +#define NFC_INT_CMDREG_READY		BIT(8)
> +#define NFC_INT_READ_DATAREG_READY	BIT(9)
> +#define NFC_INT_WRITE_DATAREG_READY	BIT(10)
> +#define NFC_INT_COMMAND_COMPLETE	BIT(11)
> +#define NFC_INT_ECC_STATUS_READY	BIT(12)
> +
> +/* IO registers operations */
> +enum { CMD, DATA_READ, DATA_WRITE, ECC_STATUS };
> +
> +#define INITIAL_TCCS	500 /* ns; from the ONFI spec version 4.0, §4.17.1 */
> +#define TIMEOUT_MS	100
> +
> +struct digicolor_nfc {
> +	void __iomem		*regs;
> +	struct mtd_info		mtd;
> +	struct nand_chip	nand;
> +	struct device		*dev;
> +
> +	unsigned long		clk_rate;
> +
> +	u32 ale_cmd;
> +	u32 ale_data;
> +	int ale_data_bytes;
> +
> +	u32 nand_cs;
> +	int t_ccs;
> +};

Sorry, you're the first one I'll bother with this.
Even if most drivers are already mixing the NAND chips and NAND
controller concepts, I really think those 2 elements should be properly
separated.

Correct me if I'm wrong, but I'm pretty sure _nfc stands for NAND Flash
Controller, and as such, NFC related fields should be part of your NAND
controller implementation (inherited from the struct nand_hw_control)
and not your NAND chip implementation (inherited from nand_chip).

If you need an example of such NAND chip/NAND controller separation,
you can take a look at the sunxi driver ;-). 

> +
> +/*
> + * Table of BCH configuration options. The index of this table (0 - 5) is set
> + * in the BchTconfig field of the NFC_CONTROL register.
> + */
> +struct bch_configs_t {

I haven't seen any struct defined with an _t, AFAIK _t are reserved for
typedef definitions.

> +	int bits;	/* correctable error bits number (strength) per step */
> +	int r_bytes;	/* extra bytes needed per step */
> +} bch_configs[] = {
> +	{  6, 11 },
> +	{  7, 13 },
> +	{  8, 14 },
> +	{ 24, 42 },
> +	{ 28, 49 },
> +	{ 30, 53 },
> +};

Just giving my opinion here, but I don't like when values assignment
and struct (or type) definitions are mixed.

> +
> +static int digicolor_nfc_buf_blank(const uint8_t *buf, int len)
> +{
> +	const uint32_t *p = (const uint32_t *)buf;
> +	int i;
> +
> +	for (i = 0; i < (len >> 2); i++)
> +		if (p[i] != 0xffffffff)
> +			return 0;
> +
> +	return 1;
> +}
> +
> +static bool digicolor_nfc_ready(struct digicolor_nfc *nfc, u32 mask)
> +{
> +	u32 status = readl_relaxed(nfc->regs + NFC_INTFLAG_CLEAR);
> +
> +	return !!(status & mask);
> +}
> +
> +static int digicolor_nfc_wait_ready(struct digicolor_nfc *nfc, int op)
> +{
> +	unsigned long timeout = jiffies + msecs_to_jiffies(TIMEOUT_MS);
> +	u32 mask;
> +
> +	switch (op) {
> +	case CMD:		mask = NFC_INT_CMDREG_READY; break;
> +	case DATA_READ:		mask = NFC_INT_READ_DATAREG_READY; break;
> +	case DATA_WRITE:	mask = NFC_INT_WRITE_DATAREG_READY; break;
> +	case ECC_STATUS:	mask = NFC_INT_ECC_STATUS_READY; break;
> +	}

I had a look at digicolor_nfc_wait_ready callers, and IMHO this
op -> mask conversion is pretty much useless.
Callers already know what they expect and could easily pass flags
directly.

> +
> +	do {
> +		if (digicolor_nfc_ready(nfc, mask))
> +			return 0;
> +	} while (time_before(jiffies, timeout));
> +
> +	dev_err(nfc->dev, "register ready (op: %d) timeout\n", op);
> +	return -ETIMEDOUT;
> +}
> +
> +static void digicolor_nfc_cmd_write(struct digicolor_nfc *nfc, u32 data)
> +{
> +	if (digicolor_nfc_wait_ready(nfc, CMD))
> +		return;
> +	writel_relaxed(data, nfc->regs + NFC_COMMAND);

Are you sure you shouldn't provide a return code here ?
If the wait_ready call times out, you're just assuming it succeed,
which is not really safe.

> +}
> +
> +static int digicolor_nfc_ecc_status(struct digicolor_nfc *nfc)
> +{
> +	u32 status;
> +
> +	if (digicolor_nfc_wait_ready(nfc, ECC_STATUS))
> +		return -1;

		return -ETIMEDOUT

would be more appropriate (or just returning the
digicolor_nfc_wait_ready result if it's not 0).

> +
> +	status = readl_relaxed(nfc->regs + NFC_STATUS_1);
> +	writel_relaxed(NFC_INT_ECC_STATUS_READY, nfc->regs + NFC_INTFLAG_CLEAR);
> +
> +	if (status & NFC_STATUS_1_UNCORRECTED_ERROR)
> +		return -1;

		return -EIO;

(or something else, but I don't recall).

> +
> +	return NFC_STATUS_1_CORRECTABLE_ERRORS(status);
> +}
> +
> +static void digicolor_nfc_wait_ns(struct digicolor_nfc *nfc, int wait_ns)
> +{
> +	uint64_t tmp = ((uint64_t) nfc->clk_rate * wait_ns);
> +	u8 clk;
> +
> +	do_div(tmp, NSEC_PER_SEC);
> +	clk = tmp > 0xff ? 0xff : tmp;
> +	digicolor_nfc_cmd_write(nfc, CMD_WAIT | clk);
> +}
> +
> +static int digicolor_nfc_dev_ready(struct mtd_info *mtd)
> +{
> +	struct nand_chip *chip = mtd->priv;
> +	struct digicolor_nfc *nfc = chip->priv;
> +	u32 readready;
> +	u32 cs = nfc->nand_cs;
> +
> +	readready = CMD_WAITREADY | CMD_CHIP_ENABLE(cs) | CMD_RB_MASK(cs)
> +		| CMD_TOGGLE | CMD_RB_DATA;
> +	digicolor_nfc_cmd_write(nfc, readready);
> +
> +	return 1;

Is your device always ready ? What if your digicolor_nfc_cmd_write
timed out ?

> +}
> +
> +static void digicolor_nfc_cmd_ctrl(struct mtd_info *mtd, int cmd,
> +				   unsigned int ctrl)
> +{
> +	struct nand_chip *chip = mtd->priv;
> +	struct digicolor_nfc *nfc = chip->priv;
> +	u32 cs = nfc->nand_cs;
> +
> +	if (ctrl & NAND_CLE) {
> +		digicolor_nfc_cmd_write(nfc,
> +					CMD_CLE | CMD_CHIP_ENABLE(cs) | cmd);
> +		if (cmd == NAND_CMD_RNDOUTSTART || cmd == NAND_CMD_RNDIN) {
> +			digicolor_nfc_wait_ns(nfc, nfc->t_ccs);
> +		} else if (cmd == NAND_CMD_RESET) {
> +			digicolor_nfc_wait_ns(nfc, 200);
> +			digicolor_nfc_dev_ready(mtd);
> +		}

These wait and dev_ready calls are supposed to be part of the generic
cmdfunc implementation, did you encounter any issues when relying on the
default implementation ?

> +	} else if (ctrl & NAND_ALE) {
> +		if (ctrl & NAND_CTRL_CHANGE) {
> +			/* First ALE data byte */
> +			nfc->ale_cmd = CMD_ALE | CMD_CHIP_ENABLE(cs)
> +				| (cmd & 0xff);
> +			nfc->ale_data_bytes++;
> +			return;
> +		}
> +		/* More ALE data bytes. Assume no more than 5 address cycles */
> +		nfc->ale_data |= cmd << ALE_DATA_OFF(nfc->ale_data_bytes++);
> +		return;
> +	} else if (nfc->ale_data_bytes > 0) {
> +		/* Finish ALE */
> +		nfc->ale_cmd |= CMD_NUMBER_BYTES(nfc->ale_data_bytes - 1);
> +		digicolor_nfc_cmd_write(nfc, nfc->ale_cmd);
> +		if (nfc->ale_data_bytes > 1)
> +			digicolor_nfc_cmd_write(nfc, nfc->ale_data);
> +		nfc->ale_data_bytes = nfc->ale_data = 0;
> +	}
> +}
> +
> +static uint8_t digicolor_nfc_rw_byte(struct digicolor_nfc *nfc, int byte)
> +{
> +	bool read = (byte == -1);
> +	u32 cs = nfc->nand_cs;
> +
> +	digicolor_nfc_cmd_write(nfc, read ? CMD_PAGEREAD : CMD_PAGEWRITE
> +				| CMD_CHIP_ENABLE(cs));
> +	digicolor_nfc_cmd_write(nfc, 1);
> +
> +	if (digicolor_nfc_wait_ready(nfc, read ? DATA_READ : DATA_WRITE))
> +		return 0;
> +
> +	if (read)
> +		return readl_relaxed(nfc->regs + NFC_DATA);
> +	else
> +		writel_relaxed(byte & 0xff, nfc->regs + NFC_DATA);
> +
> +	return 0;
> +}

Is there a real need to keep read and write handling in the same
function ?
You're testing twice the operation type in a ~10 lines function.
Just move the appropriate code in the following functions.

> +
> +static uint8_t digicolor_nfc_read_byte(struct mtd_info *mtd)
> +{
> +	struct nand_chip *chip = mtd->priv;
> +	struct digicolor_nfc *nfc = chip->priv;
> +
> +	return digicolor_nfc_rw_byte(nfc, -1);
> +}
> +
> +static void digicolor_nfc_write_byte(struct mtd_info *mtd, uint8_t byte)
> +{
> +	struct nand_chip *chip = mtd->priv;
> +	struct digicolor_nfc *nfc = chip->priv;
> +
> +	digicolor_nfc_rw_byte(nfc, byte);
> +}
> +
> +static void digicolor_nfc_rw_buf(struct digicolor_nfc *nfc, uint8_t *read_buf,
> +				 const uint8_t *write_buf, int len, bool ecc)
> +{
> +	uint32_t *pr = (uint32_t *)read_buf;
> +	const uint32_t *pw = (const uint32_t *)write_buf;
> +	u32 cs = nfc->nand_cs;
> +	int op = read_buf ? DATA_READ : DATA_WRITE;
> +	int i;
> +	u32 cmd, data, buf_tail;
> +
> +	cmd = read_buf ? CMD_PAGEREAD : CMD_PAGEWRITE;
> +	cmd |= CMD_CHIP_ENABLE(cs);
> +	data = len & 0xffff;
> +	if (ecc) {
> +		cmd |= CMD_ECC_ENABLE | CMD_SKIP_LENGTH(1);
> +		if (op == DATA_READ)
> +			cmd |= CMD_ECC_STATUS;
> +		if (op == DATA_WRITE)
> +			cmd |= CMD_IMMEDIATE_DATA;
> +		data |= CMD_SKIP_OFFSET(nfc->mtd.writesize);
> +	}
> +
> +	digicolor_nfc_cmd_write(nfc, cmd);
> +	digicolor_nfc_cmd_write(nfc, data);
> +
> +	while (len >= 4) {
> +		if (digicolor_nfc_wait_ready(nfc, op))
> +			return;
> +		if (op == DATA_READ)
> +			*pr++ = readl_relaxed(nfc->regs + NFC_DATA);
> +		else
> +			writel_relaxed(*pw++, nfc->regs + NFC_DATA);
> +		len -= 4;
> +	}

How about using readsl/writesl here (instead of this loop) ?

> +
> +	if (len > 0) {
> +		if (digicolor_nfc_wait_ready(nfc, op))
> +			return;
> +		if (op == DATA_READ)
> +			buf_tail = readl_relaxed(nfc->regs + NFC_DATA);
> +		for (i = 0; i < len; i++) {
> +			u8 *tr = (u8 *)pr;
> +			const u8 *tw = (const u8 *)pw;
> +
> +			if (op == DATA_READ) {
> +				tr[i] = buf_tail & 0xff;
> +				buf_tail >>= 8;
> +			} else {
> +				buf_tail <<= 8;
> +				buf_tail |= tw[i];
> +			}
> +		}

I still don't get that part, but I guess you have a good reason for
doing that.
Could add a comment explaining what you're doing and why you're doing
it ?

> +		if (op == DATA_WRITE)
> +			writel_relaxed(buf_tail, nfc->regs + NFC_DATA);
> +	}
> +}

Again, the code in this function should be dispatched in the
digicolor_nfc_read/write_buf functions.

> +
> +static void digicolor_nfc_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
> +{
> +	struct nand_chip *chip = mtd->priv;
> +	struct digicolor_nfc *nfc = chip->priv;
> +
> +	digicolor_nfc_rw_buf(nfc, buf, NULL, len, false);
> +}
> +
> +static void digicolor_nfc_write_buf(struct mtd_info *mtd, const uint8_t *buf,
> +				    int len)
> +{
> +	struct nand_chip *chip = mtd->priv;
> +	struct digicolor_nfc *nfc = chip->priv;
> +
> +	digicolor_nfc_rw_buf(nfc, NULL, buf, len, false);
> +}
> +
> +static int digicolor_nfc_read_page_syndrome(struct mtd_info *mtd,
> +					    struct nand_chip *chip,
> +					    uint8_t *buf, int oob_required,
> +					    int page)
> +{
> +	struct digicolor_nfc *nfc = chip->priv;
> +	int step, ecc_stat;
> +	struct nand_oobfree *oobfree = &chip->ecc.layout->oobfree[0];
> +	u8 *oob = chip->oob_poi + oobfree->offset;
> +	unsigned int max_bitflips = 0;
> +
> +	for (step = 0; step < chip->ecc.steps; step++) {
> +		digicolor_nfc_rw_buf(nfc, buf, NULL, chip->ecc.size, true);
> +
> +		ecc_stat = digicolor_nfc_ecc_status(nfc);

If the returned error is a timeout you might want to stop the whole
operation.

> +		if (ecc_stat < 0 &&
> +		    !digicolor_nfc_buf_blank(buf, chip->ecc.size)) {
> +			mtd->ecc_stats.failed++;
> +		} else if (ecc_stat > 0) {
> +			mtd->ecc_stats.corrected += ecc_stat;
> +			max_bitflips = max_t(unsigned int, max_bitflips,
> +					     ecc_stat);
> +		}
> +
> +		buf += chip->ecc.size;
> +	}
> +
> +	if (oob_required)
> +		digicolor_nfc_rw_buf(nfc, oob, NULL, oobfree->length, false);
> +
> +	return max_bitflips;
> +}
> +
> +static int digicolor_nfc_write_page_syndrome(struct mtd_info *mtd,
> +					     struct nand_chip *chip,
> +					     const uint8_t *buf,
> +					     int oob_required)
> +{
> +	struct digicolor_nfc *nfc = chip->priv;
> +	struct nand_oobfree *oobfree = &chip->ecc.layout->oobfree[0];
> +	u8 *oob = chip->oob_poi + oobfree->offset;
> +
> +	digicolor_nfc_rw_buf(nfc, NULL, buf, mtd->writesize, true);
> +
> +	if (oob_required)
> +		digicolor_nfc_rw_buf(nfc, NULL, oob, oobfree->length, false);
> +
> +	return 0;
> +}
> +
> +static int digicolor_nfc_read_oob_syndrome(struct mtd_info *mtd,
> +					   struct nand_chip *chip, int page)
> +{
> +	struct digicolor_nfc *nfc = chip->priv;
> +
> +	chip->cmdfunc(mtd, NAND_CMD_READ0, mtd->writesize, page);
> +	digicolor_nfc_rw_buf(nfc, chip->oob_poi, NULL, mtd->oobsize, false);
> +
> +	return 0;
> +}
> +
> +static void digicolor_nfc_hw_init(struct digicolor_nfc *nfc)
> +{
> +	unsigned int ns_per_clk = NSEC_PER_SEC / nfc->clk_rate;
> +	u32 timing = 0;
> +
> +	writel_relaxed(NFC_CONTROL_LOCAL_RESET, nfc->regs + NFC_CONTROL);
> +	udelay(10);
> +	writel_relaxed(0, nfc->regs + NFC_CONTROL);
> +	udelay(5);
> +	/*
> +	 * Maximum assert/deassert time; asynchronous SDR mode 0
> +	 * Deassert time = max(tWH,tREH) = 30ns
> +	 * Assert time = max(tRC,tRP,tWC,tWP) = 100ns
> +	 * Sample time = 0
> +	 */
> +	timing |= TIMING_DEASSERT(DIV_ROUND_UP(30, ns_per_clk));
> +	timing |= TIMING_ASSERT(DIV_ROUND_UP(100, ns_per_clk));
> +	timing |= TIMING_SAMPLE(0);
> +	writel_relaxed(timing, nfc->regs + NFC_TIMING_CONFIG);
> +	writel_relaxed(NFC_CONTROL_ENABLE, nfc->regs + NFC_CONTROL);

Helper functions are provided to extract timings from ONFI timing modes
(either those defined by the chip if it supports ONFI commands, or
those extracted from the datasheet):

http://lxr.free-electrons.com/source/include/linux/mtd/nand.h#L976

> +}
> +
> +static int digicolor_nfc_ecc_init(struct digicolor_nfc *nfc,
> +				  struct device_node *np)
> +{
> +	struct mtd_info *mtd = &nfc->mtd;
> +	struct nand_chip *chip = &nfc->nand;
> +	int bch_data_range, bch_t, steps, mode, i;
> +	u32 ctrl = NFC_CONTROL_ENABLE | NFC_CONTROL_BCH_KCONFIG;
> +	struct nand_ecclayout *layout;
> +
> +	mode = of_get_nand_ecc_mode(np);
> +	if (mode < 0)
> +		return mode;
> +	if (mode != NAND_ECC_HW_SYNDROME) {
> +		dev_err(nfc->dev, "unsupported ECC mode\n");
> +		return -EINVAL;
> +	}
> +
> +	bch_data_range = of_get_nand_ecc_step_size(np);
> +	if (bch_data_range < 0)
> +		return bch_data_range;
> +	if (bch_data_range != 512 && bch_data_range != 1024) {
> +		dev_err(nfc->dev, "unsupported nand-ecc-step-size value\n");
> +		return -EINVAL;
> +	}
> +	if (bch_data_range == 1024)
> +		ctrl |= NFC_CONTROL_BCH_DATA_FIELD_RANGE_1024;
> +	steps = mtd->writesize / bch_data_range;
> +
> +	bch_t = of_get_nand_ecc_strength(np);
> +	if (bch_t < 0)
> +		return bch_t;

You should fallback to datasheet values (ecc_strength_ds and
ecc_step_ds) if ECC strength and step are not specified in the DT.

> +	for (i = 0; i < ARRAY_SIZE(bch_configs); i++)
> +		if (bch_t == bch_configs[i].bits)
> +			break;
> +	if (i >= ARRAY_SIZE(bch_configs)) {
> +		dev_err(nfc->dev, "unsupported nand-ecc-strength value %d\n",
> +			bch_t);
> +		return -EINVAL;
> +	}
> +	if (bch_configs[i].r_bytes * steps > (mtd->oobsize-1)) {
> +		dev_err(nfc->dev, "OOB too small for selected ECC strength\n");
> +		return -EINVAL;
> +	}
> +	ctrl |= NFC_CONTROL_BCH_TCONFIG(i);
> +
> +	writel_relaxed(ctrl, nfc->regs + NFC_CONTROL);
> +
> +	chip->ecc.size = bch_data_range;
> +	chip->ecc.strength = bch_t;
> +	chip->ecc.bytes = bch_configs[i].r_bytes;
> +	chip->ecc.steps = steps;
> +	chip->ecc.mode = mode;
> +	chip->ecc.read_page = digicolor_nfc_read_page_syndrome;
> +	chip->ecc.write_page = digicolor_nfc_write_page_syndrome;
> +	chip->ecc.read_oob = digicolor_nfc_read_oob_syndrome;
> +
> +	layout = devm_kzalloc(nfc->dev, sizeof(*layout), GFP_KERNEL);
> +	if (layout == NULL)
> +		return -ENOMEM;
> +	layout->eccbytes = chip->ecc.bytes * steps;
> +	/* leave 1 byte for bad block mark at the beginning of oob */
> +	for (i = 0; i < layout->eccbytes; i++)
> +		layout->eccpos[i] = i + 1;
> +	layout->oobfree[0].length = mtd->oobsize - layout->eccbytes - 1;
> +	layout->oobfree[0].offset = layout->eccbytes + 1;
> +
> +	chip->ecc.layout = layout;
> +
> +	return 0;
> +}
> +
> +static int digicolor_nfc_probe(struct platform_device *pdev)
> +{
> +	struct mtd_info *mtd;
> +	struct nand_chip *chip;
> +	struct digicolor_nfc *nfc;
> +	struct resource *r;
> +	struct clk *clk;
> +	struct device_node *nand_np;
> +	struct mtd_part_parser_data ppdata;
> +	int irq, ret;
> +	u32 cs;
> +
> +	nfc = devm_kzalloc(&pdev->dev, sizeof(*nfc), GFP_KERNEL);
> +	if (!nfc)
> +		return -ENOMEM;
> +
> +	nfc->dev = &pdev->dev;
> +	chip = &nfc->nand;
> +	mtd = &nfc->mtd;
> +	mtd->priv = chip;
> +	mtd->dev.parent = &pdev->dev;
> +	mtd->owner = THIS_MODULE;
> +	mtd->name = DRIVER_NAME;
> +
> +	chip->priv = nfc;
> +	chip->cmd_ctrl = digicolor_nfc_cmd_ctrl;
> +	chip->read_byte = digicolor_nfc_read_byte;
> +	chip->read_buf = digicolor_nfc_read_buf;
> +	chip->write_byte = digicolor_nfc_write_byte;
> +	chip->write_buf = digicolor_nfc_write_buf;
> +	chip->dev_ready = digicolor_nfc_dev_ready;
> +
> +	clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);
> +	nfc->clk_rate = clk_get_rate(clk);
> +
> +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	nfc->regs = devm_ioremap_resource(&pdev->dev, r);
> +	if (IS_ERR(nfc->regs))
> +		return PTR_ERR(nfc->regs);
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (IS_ERR_VALUE(irq))
> +		return irq;
> +
> +	if (of_get_child_count(pdev->dev.of_node) > 1)
> +		dev_warn(&pdev->dev,
> +			 "only one NAND chip is currently supported\n");
> +	nand_np = of_get_next_available_child(pdev->dev.of_node, NULL);
> +	if (!nand_np) {
> +		dev_err(&pdev->dev, "missing NAND chip node\n");
> +		return -ENXIO;
> +	}
> +	ret = of_property_read_u32(nand_np, "reg", &cs);
> +	if (ret) {
> +		dev_err(&pdev->dev, "%s: no valid reg property\n",
> +			nand_np->full_name);
> +		return ret;
> +	}
> +	nfc->nand_cs = cs;
> +
> +	nfc->t_ccs = INITIAL_TCCS;
> +
> +	digicolor_nfc_hw_init(nfc);
> +
> +	ret = nand_scan_ident(mtd, 1, NULL);
> +	if (ret)
> +		return ret;
> +	ret = digicolor_nfc_ecc_init(nfc, nand_np);
> +	if (ret)
> +		return ret;
> +	ret = nand_scan_tail(mtd);
> +	if (ret)
> +		return ret;
> +
> +	ppdata.of_node = nand_np;
> +	ret = mtd_device_parse_register(mtd, NULL, &ppdata, NULL, 0);
> +	if (ret) {
> +		nand_release(mtd);
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, nfc);
> +
> +	return 0;
> +}
> +
> +static int digicolor_nfc_remove(struct platform_device *pdev)
> +{
> +	struct digicolor_nfc *nfc = platform_get_drvdata(pdev);
> +
> +	nand_release(&nfc->mtd);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id digicolor_nfc_ids[] = {
> +	{ .compatible = "cnxt,cx92755-nfc" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, sunxi_nfc_ids);

Hm, let me guess, you've based your work on the sunxi driver, isn't
it ? :-)

That's all I got for now.

I might have missed some details, but all in all I really like the way
this driver was designed (but I'm not sure to be objective since this
one is based on the sunxi driver ;-)):
- pretty straightforward implementation
- make use, as much as possible, of the NAND infrastructure (no specific
  cmdfunc, seems to support raw accesses, ...)

The only missing parts are:
- proper timing configuration
- replace active waits (polling) by passive waits (interrupt +
  waitqueue)

But that should be fixed quite easily.

Best Regards,

Boris

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