From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-11.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_2 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D9638C43461 for ; Tue, 15 Sep 2020 08:09:39 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 86DB721D7A for ; Tue, 15 Sep 2020 08:09:39 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726133AbgIOIJN convert rfc822-to-8bit (ORCPT ); Tue, 15 Sep 2020 04:09:13 -0400 Received: from relay5-d.mail.gandi.net ([217.70.183.197]:45259 "EHLO relay5-d.mail.gandi.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726169AbgIOIHp (ORCPT ); Tue, 15 Sep 2020 04:07:45 -0400 X-Originating-IP: 91.224.148.103 Received: from xps13 (unknown [91.224.148.103]) (Authenticated sender: miquel.raynal@bootlin.com) by relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 538551C000E; Tue, 15 Sep 2020 08:07:29 +0000 (UTC) Date: Tue, 15 Sep 2020 10:07:28 +0200 From: Miquel Raynal To: Yifeng Zhao Cc: richard@nod.at, vigneshr@ti.com, robh+dt@kernel.org, devicetree@vger.kernel.org, linux-mtd@lists.infradead.org, heiko@sntech.de, linux-rockchip@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v8 2/8] mtd: rawnand: rockchip: NFC drivers for RK3308, RK2928 and others Message-ID: <20200915100728.5c866f89@xps13> In-Reply-To: <20200717092420.19798-3-yifeng.zhao@rock-chips.com> References: <20200717092420.19798-1-yifeng.zhao@rock-chips.com> <20200717092420.19798-3-yifeng.zhao@rock-chips.com> Organization: Bootlin X-Mailer: Claws Mail 3.17.4 (GTK+ 2.24.32; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: devicetree-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org 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 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 > --- > > 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 > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +/* > + * 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 "); > +MODULE_DESCRIPTION("Rockchip Nand Flash Controller Driver"); > +MODULE_ALIAS("platform:rockchip-nand-controller"); Thanks, Miquèl