Hi Baruch,
On Thu, 12 Feb 2015 13:10:19 +0200
Baruch Siach <baruch@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
> + *
> + * 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
