Hi Yifeng,
I am very sorry for the delay it took me to review this driver, but
there are still some small incoherences IMHO, see below.
Yifeng Zhao <yifeng.zhao@xxxxxxxxxxxxxx> wrote on Fri, 17 Jul 2020
17:24:18 +0800:
> This driver supports Rockchip NFC (NAND Flash Controller) found on RK3308,
> RK2928, RKPX30, RV1108 and other SOCs. The driver has been tested using
> 8-bit NAND interface on the ARM based RK3308 platform.
>
> Support Rockchip SoCs and NFC versions:
> - PX30 and RK3326(NFCv900).
> ECC: 16/40/60/70 bits/1KB.
> CLOCK: ahb and nfc.
> - RK3308 and RV1108(NFCv800).
> ECC: 16 bits/1KB.
> CLOCK: ahb and nfc.
> - RK3036 and RK3128(NFCv622).
> ECC: 16/24/40/60 bits/1KB.
> CLOCK: ahb and nfc.
> - RK3066, RK3188 and RK2928(NFCv600).
> ECC: 16/24/40/60 bits/1KB.
> CLOCK: ahb.
>
> Supported features:
> - Read full page data by DMA.
> - Support HW ECC(one step is 1KB).
> - Support 2 - 32K page size.
> - Support 8 CS(depend on SoCs)
>
> Limitations:
> - No support for the ecc step size is 512.
> - Untested on some SoCs.
> - No support for subpages.
> - No support for the builtin randomizer.
> - The original bad block mask is not supported. It is recommended to use
> the BBT(bad block table).
>
> Signed-off-by: Yifeng Zhao <yifeng.zhao@xxxxxxxxxxxxxx>
> ---
>
> Changes in v8: None
> Changes in v7:
> - Rebase to linux-next.
> - Fix coding style.
> - Reserved 4 bytes at the begining of the oob area.
> - Page raw read and write included ecc data.
>
> Changes in v6:
> - The mtd->name set by NAND label property.
> - Add some comments.
> - Fix compile error.
>
> Changes in v5:
> - Add boot blocks support with different ECC for bootROM.
> - Rename rockchip-nand.c to rockchip-nand-controller.c.
> - Unification of other variable names.
> - Remove some compatible define.
>
> Changes in v4:
> - Define platform data structure for the register offsets.
> - The compatible define with rkxx_nfc.
> - Use SET_SYSTEM_SLEEP_PM_OPS to define PM_OPS.
> - Use exec_op instead of legacy hooks.
>
> Changes in v3: None
> Changes in v2:
> - Fix compile error.
> - Include header files sorted by file name.
>
> drivers/mtd/nand/raw/Kconfig | 12 +
> drivers/mtd/nand/raw/Makefile | 1 +
> .../mtd/nand/raw/rockchip-nand-controller.c | 1422 +++++++++++++++++
> 3 files changed, 1435 insertions(+)
> create mode 100644 drivers/mtd/nand/raw/rockchip-nand-controller.c
>
> diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
> index 113f61052269..6492855d4a55 100644
> --- a/drivers/mtd/nand/raw/Kconfig
> +++ b/drivers/mtd/nand/raw/Kconfig
> @@ -461,6 +461,18 @@ config MTD_NAND_ARASAN
> Enables the driver for the Arasan NAND flash controller on
> Zynq Ultrascale+ MPSoC.
>
> +config MTD_NAND_ROCKCHIP
> + tristate "Rockchip NAND controller"
> + depends on ARCH_ROCKCHIP && HAS_IOMEM
> + help
> + Enables support for NAND controller on Rockchip SoCs.
> + There are four different versions of NAND FLASH Controllers,
> + including:
> + NFC v600: RK2928, RK3066, RK3188
> + NFC v622: RK3036, RK3128
> + NFC v800: RK3308, RV1108
> + NFC v900: PX30, RK3326
> +
> comment "Misc"
>
> config MTD_SM_COMMON
> diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile
> index 2930f5b9015d..960c9be25204 100644
> --- a/drivers/mtd/nand/raw/Makefile
> +++ b/drivers/mtd/nand/raw/Makefile
> @@ -58,6 +58,7 @@ obj-$(CONFIG_MTD_NAND_STM32_FMC2) += stm32_fmc2_nand.o
> obj-$(CONFIG_MTD_NAND_MESON) += meson_nand.o
> obj-$(CONFIG_MTD_NAND_CADENCE) += cadence-nand-controller.o
> obj-$(CONFIG_MTD_NAND_ARASAN) += arasan-nand-controller.o
> +obj-$(CONFIG_MTD_NAND_ROCKCHIP) += rockchip-nand-controller.o
>
> nand-objs := nand_base.o nand_legacy.o nand_bbt.o nand_timings.o nand_ids.o
> nand-objs += nand_onfi.o
> diff --git a/drivers/mtd/nand/raw/rockchip-nand-controller.c b/drivers/mtd/nand/raw/rockchip-nand-controller.c
> new file mode 100644
> index 000000000000..fec1360603e0
> --- /dev/null
> +++ b/drivers/mtd/nand/raw/rockchip-nand-controller.c
> @@ -0,0 +1,1422 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +/*
> + * Rockchip NAND Flash controller driver.
> + * Copyright (C) 2020 Rockchip Inc.
> + * Author: Yifeng Zhao <yifeng.zhao@xxxxxxxxxxxxxx>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/dmaengine.h>
> +#include <linux/interrupt.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/mtd/rawnand.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +
> +/*
> + * NFC Page Data Layout:
> + * 1024 Bytes Data + 4Bytes sys data + 28Bytes~124Bytes ecc +
> + * 1024 Bytes Data + 4Bytes sys data + 28Bytes~124Bytes ecc +
> + * ......
> + * NAND Page Data Layout:
> + * 1024 * n Data + m Bytes oob
> + * Original Bad Block Mask Location:
> + * First byte of oob(spare).
> + * nand_chip->oob_poi data layout:
> + * 4Bytes sys data + .... + 4Bytes sys data + ecc data.
> + */
> +
> +/* NAND controller register definition */
> +#define NFC_READ (0)
> +#define NFC_WRITE (1)
> +#define NFC_FMCTL (0x00)
> +#define FMCTL_CE_SEL_M 0xFF
Nitpicking: I'd prefer
#define NFC_FMCTL 0x00
#define FMCTL_CE_SEL_M 0xFF
> +#define FMCTL_CE_SEL(x) (1 << (x))
> +#define FMCTL_WP BIT(8)
> +#define FMCTL_RDY BIT(9)
> +#define NFC_FMWAIT (0x04)
> +#define FLCTL_RST BIT(0)
> +#define FLCTL_WR (1) /* 0: read, 1: write */
> +#define FLCTL_XFER_ST BIT(2)
> +#define FLCTL_XFER_EN BIT(3)
> +#define FLCTL_ACORRECT BIT(10) /* Auto correct error bits. */
> +#define FLCTL_XFER_READY BIT(20)
> +#define FLCTL_XFER_SECTOR (22)
> +#define FLCTL_TOG_FIX BIT(29)
> +#define BCHCTL_BANK_M (7 << 5)
> +#define BCHCTL_BANK (5)
> +#define DMA_ST BIT(0)
> +#define DMA_WR (1) /* 0: write, 1: read */
> +#define DMA_EN BIT(2)
> +#define DMA_AHB_SIZE (3) /* 0: 1, 1: 2, 2: 4 */
> +#define DMA_BURST_SIZE (6) /* 0: 1, 3: 4, 5: 8, 7: 16 */
> +#define DMA_INC_NUM (9) /* 1 - 16 */
> +#define ECC_ERR_CNT(x, e) ((((x) >> (e).low) & (e).low_mask) \
> + | (((x) >> (e).high) & (e).high_mask) << (e).low_bn)
> +#define INT_DMA BIT(0)
> +#define NFC_BANK (0x800)
> +#define NFC_BANK_STEP (0x100)
> +#define BANK_DATA (0x00)
> +#define BANK_ADDR (0x04)
> +#define BANK_CMD (0x08)
> +#define NFC_SRAM0 (0x1000)
> +#define NFC_SRAM1 (0x1400)
> +#define NFC_SRAM_SIZE (0x400)
> +#define NFC_TIMEOUT (500000)
> +#define NFC_MAX_OOB_PER_STEP 128
> +#define NFC_MIN_OOB_PER_STEP 64
> +#define MAX_DATA_SIZE 0xFFFC
> +#define MAX_ADDRESS_CYC 6
> +#define NFC_ECC_MAX_MODES 4
> +#define NFC_MAX_NSELS (8) /* Some Socs only have 1 or 2 CSs. */
> +#define NFC_SYS_DATA_SIZE (4) /* 4 bytes sys data in oob pre 1024 data.*/
> +#define RK_DEFAULT_CLOCK_RATE (150 * 1000 * 1000) /* 150 Mhz */
> +#define ACCTIMING(csrw, rwpw, rwcs) ((csrw) << 12 | (rwpw) << 5 | (rwcs))
> +
> +enum nfc_type {
> + NFC_V6,
> + NFC_V8,
> + NFC_V9,
> +};
> +
> +/**
> + * struct rk_ecc_cnt_status: represent a ecc status data.
> + * @err_flag_bit: error flag bit index at register.
> + * @low: ecc count low bit index at register.
> + * @low_mask: mask bit.
> + * @low_bn: ecc count low bit number.
> + * @high: ecc count high bit index at register.
> + * @high_mask: mask bit
> + */
> +struct ecc_cnt_status {
> + u8 err_flag_bit;
> + u8 low;
> + u8 low_mask;
> + u8 low_bn;
> + u8 high;
> + u8 high_mask;
> +};
> +
> +/*
> + * @type: nfc version
> + * @ecc_strengths: ecc strengths
> + * @ecc_cfgs: ecc config values
> + * @flctl_off: FLCTL register offset
> + * @bchctl_off: BCHCTL register offset
> + * @dma_data_buf_off: DMA_DATA_BUF register offset
> + * @dma_oob_buf_off: DMA_OOB_BUF register offset
> + * @dma_cfg_off: DMA_CFG register offset
> + * @dma_st_off: DMA_ST register offset
> + * @bch_st_off: BCG_ST register offset
> + * @randmz_off: RANDMZ register offset
> + * @int_en_off: interrupt enable register offset
> + * @int_clr_off: interrupt clean register offset
> + * @int_st_off: interrupt status register offset
> + * @oob0_off: oob0 register offset
> + * @oob1_off: oob1 register offset
> + * @ecc0: represent ECC0 status data
> + * @ecc1: represent ECC1 status data
> + */
> +struct nfc_cfg {
> + enum nfc_type type;
> + u8 ecc_strengths[NFC_ECC_MAX_MODES];
> + u32 ecc_cfgs[NFC_ECC_MAX_MODES];
> + u32 flctl_off;
> + u32 bchctl_off;
> + u32 dma_cfg_off;
> + u32 dma_data_buf_off;
> + u32 dma_oob_buf_off;
> + u32 dma_st_off;
> + u32 bch_st_off;
> + u32 randmz_off;
> + u32 int_en_off;
> + u32 int_clr_off;
> + u32 int_st_off;
> + u32 oob0_off;
> + u32 oob1_off;
> + struct ecc_cnt_status ecc0;
> + struct ecc_cnt_status ecc1;
> +};
> +
> +struct rk_nfc_nand_chip {
> + struct list_head node;
> + struct nand_chip chip;
> +
> + u16 spare_per_sector;
> + u16 oob_buf_per_sector;
> + u16 boot_blks;
> + u16 boot_ecc;
> + u16 metadata_size;
> +
> + u8 nsels;
> + u8 sels[0];
> + /* Nothing after this field. */
> +};
> +
> +struct rk_nfc_clk {
> + int nfc_rate;
> + struct clk *nfc_clk;
> + struct clk *ahb_clk;
Not sure this deserves its own structure?
> +};
> +
> +struct rk_nfc {
> + struct nand_controller controller;
> + struct rk_nfc_clk clk;
> +
> + struct device *dev;
> + const struct nfc_cfg *cfg;
> + void __iomem *regs;
> +
> + int selected_bank;
> + int band_offset;
> +
> + struct completion done;
> + struct list_head chips;
> +
> + u8 *buffer;
> + u8 *page_buf;
> + u32 *oob_buf;
> +
> + unsigned long assigned_cs;
> +};
> +
> +static inline struct rk_nfc_nand_chip *to_rknand(struct nand_chip *chip)
> +{
> + return container_of(chip, struct rk_nfc_nand_chip, chip);
> +}
> +
> +static inline u8 *data_ptr(struct nand_chip *chip, const u8 *p, int i)
rk_nfc_buf_to_data_ptr ?
> +{
> + return (u8 *)p + i * chip->ecc.size;
> +}
> +
> +static inline u8 *oob_ptr(struct nand_chip *chip, int i)
same here
> +{
> + u8 *poi;
> +
> + poi = chip->oob_poi + i * NFC_SYS_DATA_SIZE;
> +
> + return poi;
> +}
> +
> +static inline u8 *oob_ecc_ptr(struct nand_chip *chip, int i)
same here
> +{
> + struct rk_nfc_nand_chip *rknand = to_rknand(chip);
> + u8 *poi;
> +
> + poi = chip->oob_poi + rknand->metadata_size +
> + chip->ecc.bytes * i;
> +
> + return poi;
> +}
> +
> +static inline int rk_data_len(struct nand_chip *chip)
Please prepend all you functions with rk_nfc_
> +{
> + struct rk_nfc_nand_chip *rknand = to_rknand(chip);
> +
> + return chip->ecc.size + rknand->spare_per_sector;
> +}
> +
> +static inline u8 *rk_data_ptr(struct nand_chip *chip, int i)
> +{
> + struct rk_nfc *nfc = nand_get_controller_data(chip);
> +
> + return nfc->buffer + i * rk_data_len(chip);
> +}
> +
> +static inline u8 *rk_oob_ptr(struct nand_chip *chip, int i)
> +{
> + struct rk_nfc *nfc = nand_get_controller_data(chip);
> +
> + return nfc->buffer + i * rk_data_len(chip) + chip->ecc.size;
> +}
> +
> +static void rk_nfc_select_chip(struct nand_chip *chip, int cs)
> +{
> + struct rk_nfc *nfc = nand_get_controller_data(chip);
> + struct rk_nfc_nand_chip *rknand = to_rknand(chip);
> + u32 val;
> +
> + if (cs < 0) {
> + nfc->selected_bank = -1;
> + /* Deselect the currently selected target. */
> + val = readl_relaxed(nfc->regs + NFC_FMCTL);
> + val &= ~FMCTL_CE_SEL_M;
> + writel(val, nfc->regs + NFC_FMCTL);
> + return;
> + }
> +
> + nfc->selected_bank = rknand->sels[cs];
> + nfc->band_offset = NFC_BANK + nfc->selected_bank * NFC_BANK_STEP;
> +
> + val = readl_relaxed(nfc->regs + NFC_FMCTL);
> + val &= ~FMCTL_CE_SEL_M;
> + val |= FMCTL_CE_SEL(nfc->selected_bank);
> +
> + writel(val, nfc->regs + NFC_FMCTL);
> +}
> +
> +static inline int rk_nfc_wait_ioready(struct rk_nfc *nfc)
> +{
> + int rc;
> + u32 val;
> +
> + rc = readl_poll_timeout_atomic(nfc->regs + NFC_FMCTL, val,
> + val & FMCTL_RDY, 10, NFC_TIMEOUT);
Do you really need it to be atomic ? ->exec_op() is not called in an
atomic context.
> +
> + return rc;
> +}
> +
> +static inline u8 rk_nfc_read_byte(struct nand_chip *chip)
> +{
> + struct rk_nfc *nfc = nand_get_controller_data(chip);
> +
> + return readb_relaxed(nfc->regs + nfc->band_offset + BANK_DATA);
no need for this helper, just move the code below.
> +}
> +
> +static void rk_nfc_read_buf(struct nand_chip *chip, u8 *buf, int len)
> +{
> + int i;
> +
> + for (i = 0; i < len; i++)
> + buf[i] = rk_nfc_read_byte(chip);
Either you find more places where to call this function, or you can
move this code directly to the unique location where it is called.
> +}
> +
> +static void rk_nfc_write_byte(struct nand_chip *chip, u8 byte)
Same here
> +{
> + struct rk_nfc *nfc = nand_get_controller_data(chip);
> +
> + writeb(byte, nfc->regs + nfc->band_offset + BANK_DATA);
> +}
> +
> +static void rk_nfc_write_buf(struct nand_chip *chip, const u8 *buf, int len)
> +{
> + int i;
> +
> + for (i = 0; i < len; i++)
> + rk_nfc_write_byte(chip, buf[i]);
> +}
> +
> +static int rk_nfc_cmd(struct nand_chip *chip,
> + const struct nand_subop *subop)
> +{
> + struct rk_nfc *nfc = nand_get_controller_data(chip);
> + unsigned int i, j, remaining, start;
> + int reg_offset = nfc->band_offset;
> + void __iomem *data_reg;
> + u8 *inbuf = NULL;
> + const u8 *outbuf;
> + u32 cnt = 0;
> + int ret = 0;
> +
> + for (i = 0; i < subop->ninstrs; i++) {
> + const struct nand_op_instr *instr = &subop->instrs[i];
> +
> + switch (instr->type) {
> + case NAND_OP_CMD_INSTR:
> + writeb(instr->ctx.cmd.opcode,
> + nfc->regs + reg_offset + BANK_CMD);
> + break;
> +
> + case NAND_OP_ADDR_INSTR:
> + remaining = nand_subop_get_num_addr_cyc(subop, i);
> + start = nand_subop_get_addr_start_off(subop, i);
> +
> + for (j = 0; j < 8 && j + start < remaining; j++)
> + writeb(instr->ctx.addr.addrs[j + start],
> + nfc->regs + reg_offset + BANK_ADDR);
> + break;
> +
> + case NAND_OP_DATA_IN_INSTR:
> + case NAND_OP_DATA_OUT_INSTR:
> + start = nand_subop_get_data_start_off(subop, i);
> + cnt = nand_subop_get_data_len(subop, i);
> + data_reg = nfc->regs + nfc->band_offset + BANK_DATA;
> +
> + if (instr->type == NAND_OP_DATA_OUT_INSTR) {
> + outbuf = instr->ctx.data.buf.out + start;
> + for (j = 0; j < cnt; j++)
> + writeb(outbuf[j], data_reg);
Why not calling the read/write buf helpers here? Which would be better
named read/write_data instead due to the register names of your IP.
> + } else {
> + inbuf = instr->ctx.data.buf.in + start;
> + for (j = 0; j < cnt; j++)
> + inbuf[j] = readb_relaxed(data_reg);
> + }
> + break;
> +
> + case NAND_OP_WAITRDY_INSTR:
> + if (rk_nfc_wait_ioready(nfc) < 0) {
> + ret = -ETIMEDOUT;
> + dev_err(nfc->dev, "IO not ready\n");
> + }
> + break;
> + }
> + }
> +
> + return ret;
> +}
> +
> +static const struct nand_op_parser rk_nfc_op_parser = NAND_OP_PARSER(
> + NAND_OP_PARSER_PATTERN(
> + rk_nfc_cmd,
> + NAND_OP_PARSER_PAT_CMD_ELEM(true),
> + NAND_OP_PARSER_PAT_ADDR_ELEM(true, MAX_ADDRESS_CYC),
> + NAND_OP_PARSER_PAT_CMD_ELEM(true),
> + NAND_OP_PARSER_PAT_WAITRDY_ELEM(true),
> + NAND_OP_PARSER_PAT_DATA_IN_ELEM(true, MAX_DATA_SIZE)),
> + NAND_OP_PARSER_PATTERN(
> + rk_nfc_cmd,
> + NAND_OP_PARSER_PAT_CMD_ELEM(true),
> + NAND_OP_PARSER_PAT_ADDR_ELEM(true, MAX_ADDRESS_CYC),
> + NAND_OP_PARSER_PAT_DATA_OUT_ELEM(true, MAX_DATA_SIZE),
> + NAND_OP_PARSER_PAT_CMD_ELEM(true),
> + NAND_OP_PARSER_PAT_WAITRDY_ELEM(true)),
> +);
> +
> +static int rk_nfc_exec_op(struct nand_chip *chip,
> + const struct nand_operation *op,
> + bool check_only)
> +{
> + rk_nfc_select_chip(chip, op->cs);
Don't select the die if check_only == true
> + return nand_op_parser_exec_op(chip, &rk_nfc_op_parser, op,
> + check_only);
> +}
> +
> +static int rk_nfc_setup_data_interface(struct nand_chip *chip, int csline,
> + const struct nand_data_interface *conf)
> +{
> + struct rk_nfc *nfc = nand_get_controller_data(chip);
> + const struct nand_sdr_timings *timings;
> + u32 rate, tc2rw, trwpw, trw2c;
> + u32 temp;
> +
> + if (csline == NAND_DATA_IFACE_CHECK_ONLY)
> + return 0;
> +
> + if (!chip->parameters.onfi)
> + return 0;
You must accept all kind of timings if they fit the controller
possibilities, please drop it off. If you don't support a given
configuration, just check it and return an error when CHECK_ONLY is
used.
> +
> + timings = nand_get_sdr_timings(conf);
> + if (IS_ERR(timings))
> + return -EOPNOTSUPP;
> +
> + if (IS_ERR(nfc->clk.nfc_clk))
> + rate = clk_get_rate(nfc->clk.ahb_clk);
> + else
> + rate = clk_get_rate(nfc->clk.nfc_clk);
> +
> + /* Turn clock rate into kHz. */
> + rate /= 1000;
> +
> + tc2rw = 1;
> + trw2c = 1;
> +
> + trwpw = max(timings->tWC_min, timings->tRC_min) / 1000;
> + trwpw = DIV_ROUND_UP(trwpw * rate, 1000000);
> +
> + temp = timings->tREA_max / 1000;
> + temp = DIV_ROUND_UP(temp * rate, 1000000);
> +
> + if (trwpw < temp)
> + trwpw = temp;
> +
> + /*
> + * ACCON: access timing control register
> + * -------------------------------------
> + * 31:18: reserved
> + * 17:12: csrw, clock cycles from the falling edge of CSn to the
> + falling edge of RDn or WRn
Style check: '*' to start every comment line
> + * 11:11: reserved
> + * 10:05: rwpw, the width of RDn or WRn in processor clock cycles
> + * 04:00: rwcs, clock cycles from the rising edge of RDn or WRn to the
> + rising edge of CSn
> + */
> + temp = ACCTIMING(tc2rw, trwpw, trw2c);
> + writel(temp, nfc->regs + NFC_FMWAIT);
> +
> + return 0;
> +}
> +
> +static int rk_nfc_hw_ecc_setup(struct nand_chip *chip,
> + struct nand_ecc_ctrl *ecc,
> + uint32_t strength)
> +{
> + struct rk_nfc *nfc = nand_get_controller_data(chip);
> + u32 reg, i;
> +
> + for (i = 0; i < NFC_ECC_MAX_MODES; i++) {
> + if (ecc->strength == nfc->cfg->ecc_strengths[i]) {
> + reg = nfc->cfg->ecc_cfgs[i];
> + break;
> + }
> + }
> +
> + if (i >= NFC_ECC_MAX_MODES)
> + return -EINVAL;
> +
> + writel(reg, nfc->regs + nfc->cfg->bchctl_off);
> +
> + return 0;
> +}
> +
> +static void rk_nfc_xfer_start(struct rk_nfc *nfc, u8 rw, u8 n_KB,
> + dma_addr_t dma_data, dma_addr_t dma_oob)
> +{
> + u32 dma_reg, fl_reg, bch_reg;
> +
> + dma_reg = DMA_ST | ((!rw) << DMA_WR) | DMA_EN | (2 << DMA_AHB_SIZE) |
> + (7 << DMA_BURST_SIZE) | (16 << DMA_INC_NUM);
> +
> + fl_reg = (rw << FLCTL_WR) | FLCTL_XFER_EN | FLCTL_ACORRECT |
> + (n_KB << FLCTL_XFER_SECTOR) | FLCTL_TOG_FIX;
> +
> + if (nfc->cfg->type == NFC_V6 || nfc->cfg->type == NFC_V8) {
> + bch_reg = readl_relaxed(nfc->regs + nfc->cfg->bchctl_off);
> + bch_reg = (bch_reg & (~BCHCTL_BANK_M)) |
> + (nfc->selected_bank << BCHCTL_BANK);
> + writel(bch_reg, nfc->regs + nfc->cfg->bchctl_off);
> + }
> +
> + writel(dma_reg, nfc->regs + nfc->cfg->dma_cfg_off);
> + writel((u32)dma_data, nfc->regs + nfc->cfg->dma_data_buf_off);
> + writel((u32)dma_oob, nfc->regs + nfc->cfg->dma_oob_buf_off);
> + writel(fl_reg, nfc->regs + nfc->cfg->flctl_off);
> + fl_reg |= FLCTL_XFER_ST;
> + writel(fl_reg, nfc->regs + nfc->cfg->flctl_off);
> +}
> +
> +static int rk_nfc_wait_for_xfer_done(struct rk_nfc *nfc)
> +{
> + void __iomem *ptr;
> + int ret = 0;
> + u32 reg;
> +
> + ptr = nfc->regs + nfc->cfg->flctl_off;
> +
> + ret = readl_poll_timeout_atomic(ptr, reg,
> + reg & FLCTL_XFER_READY,
> + 10, NFC_TIMEOUT);
Same question, do you need it to be atomic?
> +
> + return ret;
> +}
> +
> +static int rk_nfc_write_page(struct mtd_info *mtd, struct nand_chip *chip,
> + const u8 *buf, int page, int raw)
> +{
> + struct rk_nfc *nfc = nand_get_controller_data(chip);
> + struct rk_nfc_nand_chip *rknand = to_rknand(chip);
> + struct nand_ecc_ctrl *ecc = &chip->ecc;
> + int oob_step = (ecc->bytes > 60) ? NFC_MAX_OOB_PER_STEP :
> + NFC_MIN_OOB_PER_STEP;
> + int pages_per_blk = mtd->erasesize / mtd->writesize;
> + int ret = 0, i, boot_rom_mode = 0;
> + dma_addr_t dma_data, dma_oob;
> + u32 reg;
> + u8 *oob;
> +
> + nand_prog_page_begin_op(chip, page, 0, NULL, 0);
> +
> + if (!raw)
I think it will be much better to have two rk_nfc_write_page and
rk_nfc_write_page_raw distinct helpers.
> + memcpy(nfc->page_buf, buf, mtd->writesize);
> + memset(nfc->oob_buf, 0xff, oob_step * ecc->steps);
You are discarding OOB data intentionally, this is strange.
> +
> + /*
> + * The first 8(some devices are 4 or 16) blocks are in use by
"The first blocks (4, 8 or 16 depending on the device) are used by...
> + * the boot ROM and the first 32 bits of oob need to link
> + * to the next page address in the same block.
> + * Config the ECC algorithm supported by the boot ROM.
s/config/configure/
This conflicts with bad block markers...
> + */
> + if (page < pages_per_blk * rknand->boot_blks &&
( )
> + chip->options & NAND_IS_BOOT_MEDIUM) {
> + boot_rom_mode = 1;
> + if (rknand->boot_ecc != ecc->strength)
> + rk_nfc_hw_ecc_setup(chip, ecc,
> + rknand->boot_ecc);
> + }
> +
> + for (i = 0; i < ecc->steps; i++) {
> + if (!i) {
> + reg = 0xFFFFFFFF;
> + } else {
> + oob = chip->oob_poi + (i - 1) * NFC_SYS_DATA_SIZE;
> + reg = oob[0] | oob[1] << 8 | oob[2] << 16 |
> + oob[3] << 24;
> + }
> + if (!i && boot_rom_mode)
> + reg = (page & (pages_per_blk - 1)) * 4;
> +
> + if (nfc->cfg->type == NFC_V9)
> + nfc->oob_buf[i] = reg;
> + else
> + nfc->oob_buf[i * oob_step / 4] = reg;
Please use brackets.
> + }
> +
> + dma_data = dma_map_single(nfc->dev, (void *)nfc->page_buf,
Do you need this cast?
> + mtd->writesize, DMA_TO_DEVICE);
> + dma_oob = dma_map_single(nfc->dev, nfc->oob_buf,
> + ecc->steps * oob_step,
> + DMA_TO_DEVICE);
> +
> + reinit_completion(&nfc->done);
> + writel(INT_DMA, nfc->regs + nfc->cfg->int_en_off);
> +
> + rk_nfc_xfer_start(nfc, NFC_WRITE, ecc->steps, dma_data,
> + dma_oob);
> + ret = wait_for_completion_timeout(&nfc->done,
> + msecs_to_jiffies(100));
> + if (!ret)
> + dev_warn(nfc->dev, "write: wait dma done timeout.\n");
> + /*
> + * Whether the DMA transfer is completed or not. The driver
> + * needs to check the NFC`s status register to see if the data
> + * transfer was completed.
> + */
> + ret = rk_nfc_wait_for_xfer_done(nfc);
> +
> + dma_unmap_single(nfc->dev, dma_data, mtd->writesize,
> + DMA_TO_DEVICE);
> + dma_unmap_single(nfc->dev, dma_oob, ecc->steps * oob_step,
> + DMA_TO_DEVICE);
> +
> + if (boot_rom_mode && rknand->boot_ecc != ecc->strength)
> + rk_nfc_hw_ecc_setup(chip, ecc, ecc->strength);
> +
> + if (ret) {
> + ret = -EIO;
> + dev_err(nfc->dev,
> + "write: wait transfer done timeout.\n");
> + }
> + } else {
> + rk_nfc_write_buf(chip, buf, mtd->writesize + mtd->oobsize);
> + }
> +
> + if (ret)
> + return ret;
> +
> + ret = nand_prog_page_end_op(chip);
> +
> + /* Deselect the currently selected target. */
> + rk_nfc_select_chip(chip, -1);
Not sure this is needed?
> +
> + return ret;
> +}
> +
> +static int rk_nfc_write_page_raw(struct nand_chip *chip, const u8 *buf,
> + int oob_on, int page)
> +{
> + struct mtd_info *mtd = nand_to_mtd(chip);
> + struct rk_nfc *nfc = nand_get_controller_data(chip);
> + u32 i;
> +
> + memset(nfc->buffer, 0xff, mtd->writesize + mtd->oobsize);
> +
> + for (i = 0; i < chip->ecc.steps; i++) {
> + if (buf)
Can this condition be unmet really?
> + memcpy(rk_data_ptr(chip, i), data_ptr(chip, buf, i),
> + chip->ecc.size);
> +
> + if (!i)
> + memcpy(rk_oob_ptr(chip, i),
> + oob_ptr(chip, chip->ecc.steps - 1),
> + NFC_SYS_DATA_SIZE);
What is this?
> + else
> + memcpy(rk_oob_ptr(chip, i), oob_ptr(chip, i - 1),
> + NFC_SYS_DATA_SIZE);
> +
> + memcpy(rk_oob_ptr(chip, i) + NFC_SYS_DATA_SIZE,
> + oob_ecc_ptr(chip, i),
> + chip->ecc.bytes);
> + }
> +
> + return rk_nfc_write_page(mtd, chip, nfc->buffer, page, 1);
> +}
> +
> +static int rk_nfc_write_oob_std(struct nand_chip *chip, int page)
> +{
> + return rk_nfc_write_page_raw(chip, NULL, 1, page);
> +}
What is the purpose of this indirection? Please get rid of it.
> +
> +static int rk_nfc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
> + u32 data_offs, u32 readlen,
> + u8 *buf, int page, int raw)
> +{
> + struct rk_nfc *nfc = nand_get_controller_data(chip);
> + struct rk_nfc_nand_chip *rknand = to_rknand(chip);
> + struct nand_ecc_ctrl *ecc = &chip->ecc;
> + int oob_step = (ecc->bytes > 60) ? NFC_MAX_OOB_PER_STEP :
> + NFC_MIN_OOB_PER_STEP;
> + int pages_per_blk = mtd->erasesize / mtd->writesize;
> + dma_addr_t dma_data, dma_oob;
> + int ret = 0, i, boot_rom_mode = 0;
> + int bitflips = 0, bch_st;
> + u8 *oob;
> + u32 tmp;
> +
> + nand_read_page_op(chip, page, 0, NULL, 0);
> + if (!raw) {
Same comment, please write two helpers.
> + dma_data = dma_map_single(nfc->dev, nfc->page_buf,
> + mtd->writesize,
> + DMA_FROM_DEVICE);
> + dma_oob = dma_map_single(nfc->dev, nfc->oob_buf,
> + ecc->steps * oob_step,
> + DMA_FROM_DEVICE);
> +
> + /*
> + * The first 8(some devices are 4 or 16) blocks are in use by
Same rewording
> + * the boot ROM.
> + * Config the ECC algorithm supported by the boot ROM.
> + */
> + if (page < pages_per_blk * rknand->boot_blks &&
> + chip->options & NAND_IS_BOOT_MEDIUM) {
> + boot_rom_mode = 1;
> + if (rknand->boot_ecc != ecc->strength)
> + rk_nfc_hw_ecc_setup(chip, ecc,
> + rknand->boot_ecc);
> + }
> +
> + reinit_completion(&nfc->done);
> + writel(INT_DMA, nfc->regs + nfc->cfg->int_en_off);
> + rk_nfc_xfer_start(nfc, NFC_READ, ecc->steps, dma_data,
> + dma_oob);
> + ret = wait_for_completion_timeout(&nfc->done,
> + msecs_to_jiffies(100));
> + if (!ret)
> + dev_warn(nfc->dev, "read: wait dma done timeout.\n");
> + /*
> + * Whether the DMA transfer is completed or not. The driver
> + * needs to check the NFC`s status register to see if the data
> + * transfer was completed.
> + */
> + ret = rk_nfc_wait_for_xfer_done(nfc);
> + dma_unmap_single(nfc->dev, dma_data, mtd->writesize,
> + DMA_FROM_DEVICE);
> + dma_unmap_single(nfc->dev, dma_oob, ecc->steps * oob_step,
> + DMA_FROM_DEVICE);
> +
> + if (ret) {
> + bitflips = -EIO;
> + dev_err(nfc->dev,
> + "read: wait transfer done timeout.\n");
> + goto out;
> + }
> +
> + for (i = 1; i < ecc->steps; i++) {
> + oob = chip->oob_poi + (i - 1) * NFC_SYS_DATA_SIZE;
> + if (nfc->cfg->type == NFC_V9)
> + tmp = nfc->oob_buf[i];
> + else
> + tmp = nfc->oob_buf[i * oob_step / 4];
> + *oob++ = (u8)tmp;
> + *oob++ = (u8)(tmp >> 8);
> + *oob++ = (u8)(tmp >> 16);
> + *oob++ = (u8)(tmp >> 24);
> + }
> +
> + for (i = 0; i < ecc->steps / 2; i++) {
Brackets here as well please
> + bch_st = readl_relaxed(nfc->regs +
> + nfc->cfg->bch_st_off + i * 4);
> + if (bch_st & BIT(nfc->cfg->ecc0.err_flag_bit) ||
> + bch_st & BIT(nfc->cfg->ecc1.err_flag_bit)) {
> + mtd->ecc_stats.failed++;
> + bitflips = -1;
I think you should return 0.
Then the upper layer will check for failures.
> + } else {
> + ret = ECC_ERR_CNT(bch_st, nfc->cfg->ecc0);
> + mtd->ecc_stats.corrected += ret;
> + bitflips = max_t(u32, bitflips, ret);
> +
> + ret = ECC_ERR_CNT(bch_st, nfc->cfg->ecc1);
> + mtd->ecc_stats.corrected += ret;
> + bitflips = max_t(u32, bitflips, ret);
> + }
> + }
> +out:
> + memcpy(buf, nfc->page_buf, mtd->writesize);
> +
> + if (boot_rom_mode && rknand->boot_ecc != ecc->strength)
> + rk_nfc_hw_ecc_setup(chip, ecc, ecc->strength);
> +
> + if (bitflips < 0)
> + dev_err(nfc->dev, "read page: %x ecc error!\n", page);
> + } else {
> + rk_nfc_read_buf(chip, buf, mtd->writesize + mtd->oobsize);
> + }
> + /* Deselect the currently selected target. */
> + rk_nfc_select_chip(chip, -1);
> +
> + return bitflips;
> +}
> +
> +static int rk_nfc_write_page_hwecc(struct nand_chip *chip, const u8 *buf,
> + int oob_on, int page)
> +{
> + return rk_nfc_write_page(nand_to_mtd(chip), chip, buf, page, 0);
> +}
> +
> +static int rk_nfc_read_page_hwecc(struct nand_chip *chip, u8 *p, int oob_on,
> + int pg)
> +{
> + struct mtd_info *mtd = nand_to_mtd(chip);
> +
> + return rk_nfc_read_page(mtd, chip, 0, mtd->writesize, p, pg, 0);
> +}
> +
> +static int rk_nfc_read_page_raw(struct nand_chip *chip, u8 *buf, int oob_on,
> + int page)
> +{
> + struct mtd_info *mtd = nand_to_mtd(chip);
> + struct rk_nfc *nfc = nand_get_controller_data(chip);
> + int i, ret;
> +
> + ret = rk_nfc_read_page(mtd, chip, 0, mtd->writesize, nfc->buffer,
> + page, 1);
> + if (ret < 0)
> + return ret;
> +
> + for (i = 0; i < chip->ecc.steps; i++) {
> + if (!i)
> + memcpy(oob_ptr(chip, chip->ecc.steps - 1),
> + rk_oob_ptr(chip, i),
> + NFC_SYS_DATA_SIZE);
> + else
> + memcpy(oob_ptr(chip, i - 1), rk_oob_ptr(chip, i),
> + NFC_SYS_DATA_SIZE);
> +
> + memcpy(oob_ecc_ptr(chip, i),
> + rk_oob_ptr(chip, i) + NFC_SYS_DATA_SIZE,
> + chip->ecc.bytes);
> +
> + if (buf)
> + memcpy(data_ptr(chip, buf, i), rk_data_ptr(chip, i),
> + chip->ecc.size);
> + }
> +
> + return ret;
> +}
> +
> +static int rk_nfc_read_oob_std(struct nand_chip *chip, int page)
> +{
> + return rk_nfc_read_page_raw(chip, NULL, 1, page);
> +}
> +
> +static inline void rk_nfc_hw_init(struct rk_nfc *nfc)
> +{
> + /* Disable flash wp. */
> + writel(FMCTL_WP, nfc->regs + NFC_FMCTL);
> + /* Config default timing 40ns at 150 Mhz nfc clock. */
> + writel(0x1081, nfc->regs + NFC_FMWAIT);
> + /* Disable randomizer and DMA. */
> + writel(0, nfc->regs + nfc->cfg->randmz_off);
> + writel(0, nfc->regs + nfc->cfg->dma_cfg_off);
> + writel(FLCTL_RST, nfc->regs + nfc->cfg->flctl_off);
> +}
> +
> +static irqreturn_t rk_nfc_irq(int irq, void *id)
> +{
> + struct rk_nfc *nfc = id;
> + u32 sta, ien;
> +
> + sta = readl_relaxed(nfc->regs + nfc->cfg->int_st_off);
> + ien = readl_relaxed(nfc->regs + nfc->cfg->int_en_off);
> +
> + if (!(sta & ien))
> + return IRQ_NONE;
> +
> + writel(sta, nfc->regs + nfc->cfg->int_clr_off);
> + writel(~sta & ien, nfc->regs + nfc->cfg->int_en_off);
> +
> + complete(&nfc->done);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int rk_nfc_enable_clk(struct device *dev, struct rk_nfc_clk *clk)
_clks with an s to inform that there are several of them?
> +{
> + int ret;
> +
> + if (!IS_ERR(clk->nfc_clk)) {
> + ret = clk_prepare_enable(clk->nfc_clk);
> + if (ret) {
> + dev_err(dev, "failed to enable nfc clk\n");
> + return ret;
> + }
> + }
> +
> + ret = clk_prepare_enable(clk->ahb_clk);
> + if (ret) {
> + dev_err(dev, "failed to enable ahb clk\n");
> + if (!IS_ERR(clk->nfc_clk))
> + clk_disable_unprepare(clk->nfc_clk);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static void rk_nfc_disable_clk(struct rk_nfc_clk *clk)
ditto
> +{
> + if (!IS_ERR(clk->nfc_clk))
> + clk_disable_unprepare(clk->nfc_clk);
> + clk_disable_unprepare(clk->ahb_clk);
> +}
> +
> +static int rk_nfc_ooblayout_free(struct mtd_info *mtd, int section,
> + struct mtd_oob_region *oob_region)
> +{
> + struct nand_chip *chip = mtd_to_nand(mtd);
> + struct rk_nfc_nand_chip *rknand = to_rknand(chip);
> +
> + if (section)
> + return -ERANGE;
> +
> + /*
> + * The beginning of the oob area stores the reserved data for the NFC,
> + * the size of the reserved data is NFC_SYS_DATA_SIZE bytes.
> + */
> + oob_region->length = rknand->metadata_size - NFC_SYS_DATA_SIZE - 2;
> + oob_region->offset = NFC_SYS_DATA_SIZE + 2;
> +
> + return 0;
> +}
> +
> +static int rk_nfc_ooblayout_ecc(struct mtd_info *mtd, int section,
> + struct mtd_oob_region *oob_region)
> +{
> + struct nand_chip *chip = mtd_to_nand(mtd);
> + struct rk_nfc_nand_chip *rknand = to_rknand(chip);
> +
> + if (section)
> + return -ERANGE;
> +
> + oob_region->offset = rknand->metadata_size;
> + oob_region->length = mtd->oobsize - oob_region->offset;
> +
> + return 0;
> +}
> +
> +static const struct mtd_ooblayout_ops rk_nfc_ooblayout_ops = {
> + .free = rk_nfc_ooblayout_free,
> + .ecc = rk_nfc_ooblayout_ecc,
> +};
> +
> +static int rk_nfc_ecc_init(struct device *dev, struct mtd_info *mtd)
> +{
> + struct nand_chip *chip = mtd_to_nand(mtd);
> + struct rk_nfc *nfc = nand_get_controller_data(chip);
> + struct nand_ecc_ctrl *ecc = &chip->ecc;
> + const u8 *strengths = nfc->cfg->ecc_strengths;
> + u8 max_strength, nfc_max_strength;
> + int i;
> +
> + nfc_max_strength = nfc->cfg->ecc_strengths[0];
> + /* If optional dt settings not present. */
> + if (!ecc->size || !ecc->strength ||
> + ecc->strength > nfc_max_strength) {
> + /* Use datasheet requirements. */
> + ecc->strength = chip->base.eccreq.strength;
> + ecc->size = chip->base.eccreq.step_size;
> +
> + /* Align ECC strength and ECC size. */
> + if (chip->ecc.size < 1024) {
> + if (mtd->writesize > 512) {
> + chip->ecc.size = 1024;
> + chip->ecc.strength <<= 1;
> + } else {
> + dev_err(dev, "Unsupported ecc.size\n");
> + return -EINVAL;
> + }
> + } else {
> + chip->ecc.size = 1024;
> + }
> +
> + ecc->steps = mtd->writesize / ecc->size;
> +
> + /*
> + * HW ECC always requests the number of ECC bytes per 1024 byte
> + * blocks. 4 Bytes is oob for sys data.
> + */
> + max_strength = ((mtd->oobsize / ecc->steps) - 4) * 8 /
> + fls(8 * 1024);
> + if (max_strength > nfc_max_strength)
> + max_strength = nfc_max_strength;
> +
> + for (i = 0; i < 4; i++) {
> + if (max_strength >= strengths[i])
> + break;
> + }
> +
> + if (i >= 4) {
> + dev_err(nfc->dev, "Unsupported ECC strength\n");
> + return -EOPNOTSUPP;
> + }
> +
> + ecc->strength = strengths[i];
> + }
> + ecc->steps = mtd->writesize / ecc->size;
> + ecc->bytes = DIV_ROUND_UP(ecc->strength * fls(8 * 1024), 8);
> + /* HW ECC always work with even numbers of ECC bytes. */
> + ecc->bytes = ALIGN(ecc->bytes, 2);
> +
> + rk_nfc_hw_ecc_setup(chip, ecc, ecc->strength);
> +
> + return 0;
> +}
> +
> +static int rk_nfc_attach_chip(struct nand_chip *chip)
> +{
> + struct mtd_info *mtd = nand_to_mtd(chip);
> + struct device *dev = mtd->dev.parent;
> + struct rk_nfc *nfc = nand_get_controller_data(chip);
> + struct rk_nfc_nand_chip *rknand = to_rknand(chip);
> + struct nand_ecc_ctrl *ecc = &chip->ecc;
> + int len;
> + int ret;
> +
> + if (chip->options & NAND_BUSWIDTH_16) {
> + dev_err(dev, "16 bits bus width not supported");
> + return -EINVAL;
> + }
> +
> + if (ecc->mode != NAND_ECC_HW)
> + return 0;
> +
> + ret = rk_nfc_ecc_init(dev, mtd);
> + if (ret)
> + return ret;
> + rknand->spare_per_sector = ecc->bytes + NFC_SYS_DATA_SIZE;
> + rknand->metadata_size = NFC_SYS_DATA_SIZE * ecc->steps;
> +
> + if (rknand->metadata_size < NFC_SYS_DATA_SIZE + 2) {
> + dev_err(dev,
> + "Driver needs at least %d bytes of meta data\n",
> + NFC_SYS_DATA_SIZE + 2);
> + return -EIO;
> + }
> + /* Check buffer first, avoid duplicate alloc buffer. */
> + if (nfc->buffer)
> + return 0;
You can't do that this way: what if the first mtd to be registered uses
a smaller page size than the second?
> +
> + len = mtd->writesize + mtd->oobsize;
> + nfc->buffer = devm_kzalloc(dev, len, GFP_KERNEL | GFP_DMA);
> + if (!nfc->buffer)
> + return -ENOMEM;
> +
> + nfc->page_buf = nfc->buffer;
> + len = ecc->steps * NFC_MAX_OOB_PER_STEP;
> + nfc->oob_buf = devm_kzalloc(dev, len, GFP_KERNEL | GFP_DMA);
> + if (!nfc->oob_buf) {
> + nfc->buffer = NULL;
> + nfc->oob_buf = NULL;
I don't think this is needed
> + return -ENOMEM;
> + }
> +
> + chip->ecc.write_page_raw = rk_nfc_write_page_raw;
> + chip->ecc.write_page = rk_nfc_write_page_hwecc;
> + chip->ecc.write_oob_raw = rk_nfc_write_oob_std;
> + chip->ecc.write_oob = rk_nfc_write_oob_std;
> +
> + chip->ecc.read_page_raw = rk_nfc_read_page_raw;
> + chip->ecc.read_page = rk_nfc_read_page_hwecc;
> + chip->ecc.read_oob_raw = rk_nfc_read_oob_std;
> + chip->ecc.read_oob = rk_nfc_read_oob_std;
I don't like the _std prefix, but it should be gone when splitting the
functions as advised.
> +
> + return 0;
> +}
> +
> +static const struct nand_controller_ops rk_nfc_controller_ops = {
> + .attach_chip = rk_nfc_attach_chip,
> + .exec_op = rk_nfc_exec_op,
> + .setup_data_interface = rk_nfc_setup_data_interface,
> +};
> +
> +static int rk_nfc_nand_chip_init(struct device *dev, struct rk_nfc *nfc,
> + struct device_node *np)
> +{
> + struct rk_nfc_nand_chip *rknand;
> + struct nand_chip *chip;
> + struct mtd_info *mtd;
> + int nsels;
> + u32 tmp;
> + int ret;
> + int i;
> +
> + if (!of_get_property(np, "reg", &nsels))
> + return -ENODEV;
> + nsels /= sizeof(u32);
> + if (!nsels || nsels > NFC_MAX_NSELS) {
> + dev_err(dev, "invalid reg property size %d\n", nsels);
> + return -EINVAL;
> + }
> +
> + rknand = devm_kzalloc(dev, sizeof(*rknand) + nsels * sizeof(u8),
> + GFP_KERNEL);
> + if (!rknand)
> + return -ENOMEM;
> +
> + rknand->nsels = nsels;
> + for (i = 0; i < nsels; i++) {
> + ret = of_property_read_u32_index(np, "reg", i, &tmp);
> + if (ret) {
> + dev_err(dev, "reg property failure : %d\n", ret);
> + return ret;
> + }
> +
> + if (tmp >= NFC_MAX_NSELS) {
> + dev_err(dev, "invalid CS: %u\n", tmp);
> + return -EINVAL;
> + }
> +
> + if (test_and_set_bit(tmp, &nfc->assigned_cs)) {
> + dev_err(dev, "CS %u already assigned\n", tmp);
> + return -EINVAL;
> + }
> +
> + rknand->sels[i] = tmp;
> + }
> +
> + chip = &rknand->chip;
> + chip->controller = &nfc->controller;
> +
> + nand_set_flash_node(chip, np);
> +
> + nand_set_controller_data(chip, nfc);
> +
> + chip->options |= NAND_USES_DMA | NAND_NO_SUBPAGE_WRITE;
> + chip->bbt_options = NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB;
> +
> + /* Set default mode in case dt entry is missing. */
> + chip->ecc.mode = NAND_ECC_HW;
> +
> + mtd = nand_to_mtd(chip);
> + mtd->owner = THIS_MODULE;
> + mtd->dev.parent = dev;
> +
> + if (!mtd->name) {
> + dev_err(nfc->dev, "NAND label property is mandatory\n");
> + return -EINVAL;
> + }
> +
> + mtd_set_ooblayout(mtd, &rk_nfc_ooblayout_ops);
> + rk_nfc_hw_init(nfc);
> + ret = nand_scan(chip, nsels);
> + if (ret)
> + return ret;
> +
> + if (chip->options & NAND_IS_BOOT_MEDIUM) {
> + ret = of_property_read_u32(np, "rockchip,boot-blks", &tmp);
> + rknand->boot_blks = ret ? 0 : tmp;
Can't you guess this entry knowing the IP version/SoC version?
> +
> + ret = of_property_read_u32(np, "rockchip,boot-ecc-strength",
> + &tmp);
> + rknand->boot_ecc = ret ? chip->ecc.strength : tmp;
> + }
> +
> + ret = mtd_device_register(mtd, NULL, 0);
> + if (ret) {
> + dev_err(dev, "mtd parse partition error\n");
> + nand_cleanup(chip);
> + return ret;
> + }
> +
> + list_add_tail(&rknand->node, &nfc->chips);
> +
> + return 0;
> +}
> +
> +static void rk_nfc_chips_cleanup(struct rk_nfc *nfc)
> +{
> + struct rk_nfc_nand_chip *rknand, *tmp;
> + struct nand_chip *chip;
> + int ret;
> +
> + list_for_each_entry_safe(rknand, tmp, &nfc->chips, node) {
> + chip = &rknand->chip;
> + ret = mtd_device_unregister(nand_to_mtd(chip));
> + WARN_ON(ret);
> + nand_cleanup(chip);
> + list_del(&rknand->node);
> + }
> +}
> +
> +static int rk_nfc_nand_chips_init(struct device *dev, struct rk_nfc *nfc)
> +{
> + struct device_node *np = dev->of_node, *nand_np;
> + int nchips = of_get_child_count(np);
> + int ret;
> +
> + if (!nchips || nchips > NFC_MAX_NSELS) {
> + dev_err(nfc->dev, "Incorrect number of NAND chips (%d)\n",
> + nchips);
> + return -EINVAL;
> + }
> +
> + for_each_child_of_node(np, nand_np) {
> + ret = rk_nfc_nand_chip_init(dev, nfc, nand_np);
> + if (ret) {
> + of_node_put(nand_np);
> + rk_nfc_chips_cleanup(nfc);
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static struct nfc_cfg nfc_v6_cfg = {
> + .type = NFC_V6,
> + .ecc_strengths = {60, 40, 24, 16},
> + .ecc_cfgs = {
> + 0x00040011, 0x00040001, 0x00000011, 0x00000001,
> + },
> + .flctl_off = 0x08,
> + .bchctl_off = 0x0C,
> + .dma_cfg_off = 0x10,
> + .dma_data_buf_off = 0x14,
> + .dma_oob_buf_off = 0x18,
> + .dma_st_off = 0x1C,
> + .bch_st_off = 0x20,
> + .randmz_off = 0x150,
> + .int_en_off = 0x16C,
> + .int_clr_off = 0x170,
> + .int_st_off = 0x174,
> + .oob0_off = 0x200,
> + .oob1_off = 0x230,
> + .ecc0 = {
> + .err_flag_bit = 2,
> + .low = 3,
> + .low_mask = 0x1F,
> + .low_bn = 5,
> + .high = 27,
> + .high_mask = 0x1,
> + },
> + .ecc1 = {
> + .err_flag_bit = 15,
> + .low = 16,
> + .low_mask = 0x1F,
> + .low_bn = 5,
> + .high = 29,
> + .high_mask = 0x1,
> + },
> +};
> +
> +static struct nfc_cfg nfc_v8_cfg = {
> + .type = NFC_V8,
> + .ecc_strengths = {16, 16, 16, 16},
> + .ecc_cfgs = {
> + 0x00000001, 0x00000001, 0x00000001, 0x00000001,
> + },
> + .flctl_off = 0x08,
> + .bchctl_off = 0x0C,
> + .dma_cfg_off = 0x10,
> + .dma_data_buf_off = 0x14,
> + .dma_oob_buf_off = 0x18,
> + .dma_st_off = 0x1C,
> + .bch_st_off = 0x20,
> + .randmz_off = 0x150,
> + .int_en_off = 0x16C,
> + .int_clr_off = 0x170,
> + .int_st_off = 0x174,
> + .oob0_off = 0x200,
> + .oob1_off = 0x230,
> + .ecc0 = {
> + .err_flag_bit = 2,
> + .low = 3,
> + .low_mask = 0x1F,
> + .low_bn = 5,
> + .high = 27,
> + .high_mask = 0x1,
> + },
> + .ecc1 = {
> + .err_flag_bit = 15,
> + .low = 16,
> + .low_mask = 0x1F,
> + .low_bn = 5,
> + .high = 29,
> + .high_mask = 0x1,
> + },
> +};
> +
> +static struct nfc_cfg nfc_v9_cfg = {
> + .type = NFC_V9,
> + .ecc_strengths = {70, 60, 40, 16},
> + .ecc_cfgs = {
> + 0x00000001, 0x06000001, 0x04000001, 0x02000001,
> + },
> + .flctl_off = 0x10,
> + .bchctl_off = 0x20,
> + .dma_cfg_off = 0x30,
> + .dma_data_buf_off = 0x34,
> + .dma_oob_buf_off = 0x38,
> + .dma_st_off = 0x3C,
> + .bch_st_off = 0x150,
> + .randmz_off = 0x208,
> + .int_en_off = 0x120,
> + .int_clr_off = 0x124,
> + .int_st_off = 0x128,
> + .oob0_off = 0x200,
> + .oob1_off = 0x204,
> + .ecc0 = {
> + .err_flag_bit = 2,
> + .low = 3,
> + .low_mask = 0x7F,
> + .low_bn = 7,
> + .high = 0,
> + .high_mask = 0x0,
> + },
> + .ecc1 = {
> + .err_flag_bit = 18,
> + .low = 19,
> + .low_mask = 0x7F,
> + .low_bn = 7,
> + .high = 0,
> + .high_mask = 0x0,
> + },
> +};
> +
> +static const struct of_device_id rk_nfc_id_table[] = {
> + {.compatible = "rockchip,px30-nfc",
> + .data = &nfc_v9_cfg },
{
.compatible =
.data =
},
or
{ .compatible = , .data = },
but not:
> + {.compatible = "rockchip,rk2928-nfc",
> + .data = &nfc_v6_cfg },
> + {.compatible = "rockchip,rv1108-nfc",
> + .data = &nfc_v8_cfg },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, rk_nfc_id_table);
> +
> +static int rk_nfc_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct rk_nfc *nfc;
> + int ret, irq;
> +
> + nfc = devm_kzalloc(dev, sizeof(*nfc), GFP_KERNEL);
> + if (!nfc)
> + return -ENOMEM;
> +
> + nand_controller_init(&nfc->controller);
> + INIT_LIST_HEAD(&nfc->chips);
> + nfc->controller.ops = &rk_nfc_controller_ops;
> +
> + nfc->cfg = of_device_get_match_data(dev);
> + nfc->dev = dev;
> +
> + init_completion(&nfc->done);
> +
> + nfc->regs = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(nfc->regs)) {
> + ret = PTR_ERR(nfc->regs);
> + goto release_nfc;
> + }
> +
> + nfc->clk.nfc_clk = devm_clk_get(dev, "nfc");
> + if (IS_ERR(nfc->clk.nfc_clk)) {
> + dev_dbg(dev, "no nfc clk\n");
> + /* Some earlier models, such as rk3066, have no nfc clk. */
> + }
> +
> + nfc->clk.ahb_clk = devm_clk_get(dev, "ahb");
> + if (IS_ERR(nfc->clk.ahb_clk)) {
> + dev_err(dev, "no ahb clk\n");
> + ret = PTR_ERR(nfc->clk.ahb_clk);
> + goto release_nfc;
> + }
> +
> + ret = rk_nfc_enable_clk(dev, &nfc->clk);
> + if (ret)
> + goto release_nfc;
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0) {
> + dev_err(dev, "no nfc irq resource\n");
> + ret = -EINVAL;
> + goto clk_disable;
> + }
> +
> + writel(0, nfc->regs + nfc->cfg->int_en_off);
> + ret = devm_request_irq(dev, irq, rk_nfc_irq, 0x0, "rk-nand", nfc);
> + if (ret) {
> + dev_err(dev, "failed to request nfc irq\n");
> + goto clk_disable;
> + }
> +
> + platform_set_drvdata(pdev, nfc);
> +
> + ret = rk_nfc_nand_chips_init(dev, nfc);
> + if (ret) {
> + dev_err(dev, "failed to init NAND chips\n");
> + goto clk_disable;
> + }
> + return 0;
> +
> +clk_disable:
> + rk_nfc_disable_clk(&nfc->clk);
> +release_nfc:
> + return ret;
> +}
> +
> +static int rk_nfc_remove(struct platform_device *pdev)
> +{
> + struct rk_nfc *nfc = platform_get_drvdata(pdev);
> +
> + rk_nfc_chips_cleanup(nfc);
> + rk_nfc_disable_clk(&nfc->clk);
Don't you have DMA resources and !devm allocated buffers to free?
> +
> + return 0;
> +}
> +
> +static int __maybe_unused rk_nfc_suspend(struct device *dev)
> +{
> + struct rk_nfc *nfc = dev_get_drvdata(dev);
> +
> + rk_nfc_disable_clk(&nfc->clk);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused rk_nfc_resume(struct device *dev)
> +{
> + struct rk_nfc *nfc = dev_get_drvdata(dev);
> + struct rk_nfc_nand_chip *rknand;
> + struct nand_chip *chip;
> + int ret;
> + u32 i;
> +
> + ret = rk_nfc_enable_clk(dev, &nfc->clk);
> + if (ret)
> + return ret;
> +
> + /* Reset NAND chip if VCC was powered off. */
> + list_for_each_entry(rknand, &nfc->chips, node) {
> + chip = &rknand->chip;
> + for (i = 0; i < rknand->nsels; i++)
> + nand_reset(chip, i);
I assume this is handled by the core already (please verify it)
> + }
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops rk_nfc_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(rk_nfc_suspend, rk_nfc_resume)
> +};
> +
> +static struct platform_driver rk_nfc_driver = {
> + .probe = rk_nfc_probe,
> + .remove = rk_nfc_remove,
> + .driver = {
> + .name = "rockchip-nfc",
> + .of_match_table = rk_nfc_id_table,
> + .pm = &rk_nfc_pm_ops,
> + },
> +};
> +
> +module_platform_driver(rk_nfc_driver);
> +
> +MODULE_LICENSE("Dual MIT/GPL");
> +MODULE_AUTHOR("Yifeng Zhao <yifeng.zhao@xxxxxxxxxxxxxx>");
> +MODULE_DESCRIPTION("Rockchip Nand Flash Controller Driver");
> +MODULE_ALIAS("platform:rockchip-nand-controller");
Thanks,
Miquèl
