From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.bootlin.com ([62.4.15.54]) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1ey3Px-0006Uj-Kl for linux-mtd@lists.infradead.org; Mon, 19 Mar 2018 22:38:18 +0000 Date: Mon, 19 Mar 2018 23:37:48 +0100 From: Miquel Raynal To: Cc: , , , , , , , , , , Naga Sureshkumar Relli Subject: Re: [LINUX PATCH v8 2/2] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface Message-ID: <20180319233748.65b5a7b9@xps13> In-Reply-To: <1521024505-30677-1-git-send-email-nagasureshkumarrelli@gmail.com> References: <1521024505-30677-1-git-send-email-nagasureshkumarrelli@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Hi Naga, Thanks for sending a new version supporting ->exec_op(). A few comments below. On Wed, 14 Mar 2018 16:18:25 +0530, wrote: > From: Naga Sureshkumar Relli >=20 > Add driver for arm pl353 static memory controller nand interface with > HW ECC support. This controller is used in xilinx zynq soc for interfacing > the nand flash memory. >=20 > Signed-off-by: Naga Sureshkumar Relli > --- > Changes in v8: > - Added exec_op() implementation > - Fixed the below v7 review comments > - removed mtd_info from pl353_nand_info struct > - Corrected ecc layout offsets > - Added on-die ecc support > Changes in v7: > - Currently not implemented the memclk rate adjustments. I will > look into this later and once the basic driver is accepted. > - Fixed GPL licence ident > Changes in v6: > - Fixed the checkpatch.pl reported warnings > - Using the address cycles information from the onfi param page > earlier it is hardcoded to 5 in driver > Changes in v5: > - Configure the nand timing parameters as per the onfi spec > Changes in v4: > - Updated the driver to sync with pl353_smc driver APIs > Changes in v3: > - implemented the proper error codes > - further breakdown this patch to multiple sets > - added the controller and driver details to Documentation section > - updated the licenece to GPLv2 > - reorganized the pl353_nand_ecc_init function > Changes in v2: > - use "depends on" rather than "select" option in kconfig > - remove unused variable parts > - remove dummy helper and use writel_relaxed directly > --- > drivers/mtd/nand/raw/Kconfig | 8 + > drivers/mtd/nand/raw/Makefile | 1 + > drivers/mtd/nand/raw/pl353_nand.c | 1363 +++++++++++++++++++++++++++++++= ++++++ > 3 files changed, 1372 insertions(+) > create mode 100644 drivers/mtd/nand/raw/pl353_nand.c >=20 > diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig > index 2c6ecb7..5e20391 100644 > --- a/drivers/mtd/nand/raw/Kconfig > +++ b/drivers/mtd/nand/raw/Kconfig > @@ -566,4 +566,12 @@ config MTD_NAND_MTK > Enables support for NAND controller on MTK SoCs. > This controller is found on mt27xx, mt81xx, mt65xx SoCs. > =20 > +config MTD_NAND_PL353 > + tristate "ARM Pl353 NAND flash driver" > + depends on MTD_NAND && ARM > + depends on PL35X_SMC > + help > + This enables access to the NAND flash device on PL353 > + SMC controller. What about: "Enables support for PrimeCell Static Memory Controller PL353."? > + > endif # MTD_NAND > diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile > index f16f59a..3e943f3 100644 > --- a/drivers/mtd/nand/raw/Makefile > +++ b/drivers/mtd/nand/raw/Makefile > @@ -57,6 +57,7 @@ obj-$(CONFIG_MTD_NAND_HISI504) +=3D hisi504_nan= d.o > obj-$(CONFIG_MTD_NAND_BRCMNAND) +=3D brcmnand/ > obj-$(CONFIG_MTD_NAND_QCOM) +=3D qcom_nandc.o > obj-$(CONFIG_MTD_NAND_MTK) +=3D mtk_ecc.o mtk_nand.o > +obj-$(CONFIG_MTD_NAND_PL353) +=3D pl353_nand.o > =20 > nand-objs :=3D nand_base.o nand_bbt.o nand_timings.o nand_ids.o > nand-objs +=3D nand_amd.o > diff --git a/drivers/mtd/nand/raw/pl353_nand.c b/drivers/mtd/nand/raw/pl3= 53_nand.c > new file mode 100644 > index 0000000..55c51e2 > --- /dev/null > +++ b/drivers/mtd/nand/raw/pl353_nand.c > @@ -0,0 +1,1363 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * ARM PL353 NAND flash controller driver > + * > + * Copyright (C) 2017 Xilinx, Inc > + * Author: Punnaiah > + * Author: nagasuresh Please use your full names here. > + * > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define PL353_NAND_DRIVER_NAME "pl353-nand" > + > +/* NAND flash driver defines */ > +#define PL353_NAND_CMD_PHASE 1 /* End command valid in command phase */ > +#define PL353_NAND_DATA_PHASE 2 /* End command valid in data phase */ > +#define PL353_NAND_ECC_SIZE 512 /* Size of data for ECC operation */ > + > +/* Flash memory controller operating parameters */ > + > +#define PL353_NAND_ECC_CONFIG (BIT(4) | /* ECC read at end of page */ \ > + (0 << 5)) /* No Jumping */ > + > +/* AXI Address definitions */ > +#define START_CMD_SHIFT 3 > +#define END_CMD_SHIFT 11 > +#define END_CMD_VALID_SHIFT 20 > +#define ADDR_CYCLES_SHIFT 21 > +#define CLEAR_CS_SHIFT 21 > +#define ECC_LAST_SHIFT 10 > +#define COMMAND_PHASE (0 << 19) > +#define DATA_PHASE BIT(19) > + > +#define PL353_NAND_ECC_LAST BIT(ECC_LAST_SHIFT) /* Set ECC_Last */ > +#define PL353_NAND_CLEAR_CS BIT(CLEAR_CS_SHIFT) /* Clear chip select */ > + > +#define ONDIE_ECC_FEATURE_ADDR 0x90 > +#define PL353_NAND_ECC_BUSY_TIMEOUT (1 * HZ) > +#define PL353_NAND_DEV_BUSY_TIMEOUT (1 * HZ) > +#define PL353_NAND_LAST_TRANSFER_LENGTH 4 > + > +/* Inline function for the NAND controller register write */ > +static inline void pl353_nand_write32(void __iomem *addr, u32 val) > +{ > + writel_relaxed((val), (addr)); > +} Is there an actual need for this inline function? Why not calling writel_relaxed() directly? > + > +struct pl353_nfc_op { > + u32 cmnds[4]; > + u32 thirdrow; > + u32 type; > + u32 end_cmd; > + u32 addrs; > + bool wait; > + u32 len; > + u32 naddrs; > + unsigned int data_instr_idx; > + const struct nand_op_instr *data_instr; > + unsigned int rdy_timeout_ms; > + unsigned int rdy_delay_ns; > +}; > + > +/** > + * struct pl353_nand_info - Defines the NAND flash driver instance > + * @chip: NAND chip information structure > + * @nand_base: Virtual address of the NAND flash device > + * @end_cmd_pending: End command is pending > + * @end_cmd: End command > + * @row_addr_cycles: Row address cycles > + * @col_addr_cycles: Column address cycles > + * @address: Page address > + * @cmd_pending: More command is needed > + */ > +struct pl353_nand_info { > + struct nand_chip chip; > + void __iomem *nand_base; > + unsigned long end_cmd_pending; > + unsigned long end_cmd; > + u8 row_addr_cycles; > + u8 col_addr_cycles; > + u32 address; > + u32 cmd_pending; > +}; > + > +static int pl353_ecc_ooblayout16_ecc(struct mtd_info *mtd, int section, > + struct mtd_oob_region *oobregion) > +{ > + struct nand_chip *chip =3D mtd_to_nand(mtd); > + > + if (section >=3D chip->ecc.steps) > + return -ERANGE; > + > + oobregion->offset =3D (section * 16) + 0; > + oobregion->length =3D chip->ecc.bytes; > + > + return 0; > +} > + > +static int pl353_ecc_ooblayout16_free(struct mtd_info *mtd, int section, > + struct mtd_oob_region *oobregion) > +{ > + struct nand_chip *chip =3D mtd_to_nand(mtd); > + > + if (section >=3D chip->ecc.steps) > + return -ERANGE; > + > + oobregion->offset =3D (section * 16) + 8; > + Extra space > + oobregion->length =3D 8; > + > + return 0; > +} > + > +static const struct mtd_ooblayout_ops pl353_ecc_ooblayout16_ops =3D { > + .ecc =3D pl353_ecc_ooblayout16_ecc, > + .free =3D pl353_ecc_ooblayout16_free, > +}; > + > +static int pl353_ecc_ooblayout64_ecc(struct mtd_info *mtd, int section, > + struct mtd_oob_region *oobregion) > +{ > + struct nand_chip *chip =3D mtd_to_nand(mtd); > + > + if (section >=3D chip->ecc.steps) > + return -ERANGE; > + > + oobregion->offset =3D (section * chip->ecc.bytes) + 52; > + oobregion->length =3D chip->ecc.bytes; > + > + return 0; > +} > + > +static int pl353_ecc_ooblayout64_free(struct mtd_info *mtd, int section, > + struct mtd_oob_region *oobregion) > +{ > + struct nand_chip *chip =3D mtd_to_nand(mtd); > + > + if (section) > + return -ERANGE; > + if (section >=3D chip->ecc.steps) > + return -ERANGE; If you can't support more than one section, the second if is useless, and the offset is just "2". > + > + oobregion->offset =3D (section * chip->ecc.bytes) + 2; > + > + oobregion->length =3D 50; > + > + return 0; > +} > + > +static const struct mtd_ooblayout_ops pl353_ecc_ooblayout64_ops =3D { > + .ecc =3D pl353_ecc_ooblayout64_ecc, > + .free =3D pl353_ecc_ooblayout64_free, > +}; > + > +/* Generic flash bbt decriptors */ > +static uint8_t bbt_pattern[] =3D { 'B', 'b', 't', '0' }; > +static uint8_t mirror_pattern[] =3D { '1', 't', 'b', 'B' }; > + > +static struct nand_bbt_descr bbt_main_descr =3D { > + .options =3D NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE > + | NAND_BBT_2BIT | NAND_BBT_VERSION | NAND_BBT_PERCHIP, > + .offs =3D 4, > + .len =3D 4, > + .veroffs =3D 20, > + .maxblocks =3D 4, > + .pattern =3D bbt_pattern > +}; > + > +static struct nand_bbt_descr bbt_mirror_descr =3D { > + .options =3D NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE > + | NAND_BBT_2BIT | NAND_BBT_VERSION | NAND_BBT_PERCHIP, > + .offs =3D 4, > + .len =3D 4, > + .veroffs =3D 20, > + .maxblocks =3D 4, > + .pattern =3D mirror_pattern > +}; > + > +/** > + * pl353_nand_read_buf_l - read chip data into buffer > + * @chip: Pointer to the NAND chip info structure > + * @in: Pointer to the buffer to store read data > + * @len: Number of bytes to read > + * Return: Always return zero > + */ > +static int pl353_nand_read_buf_l(struct nand_chip *chip, > + uint8_t *in, > + unsigned int len) > +{ > + int i; > + unsigned long *ptr =3D (unsigned long *)in; > + > + len >>=3D 2; Can you please let the compiler optimize things? I don't find this very readable, I would prefer a division here. And if this division by 4 is related to the size of *ptr, please use the sizeof() macro. Otherwise please document this value. > + for (i =3D 0; i < len; i++) > + ptr[i] =3D readl(chip->IO_ADDR_R); Space > + return 0; > +} > + > +static void pl353_nand_write_buf_l(struct nand_chip *chip, const uint8_t= *buf, > + int len) > +{ > + int i; > + unsigned long *ptr =3D (unsigned long *)buf; > + > + for (i =3D 0; i < len; i++) > + writeb(ptr[i], chip->IO_ADDR_W); Here you use writeb (as opposed to readl previously). Then, I guess you can also read byte per byte. If so, you can drop both helpers and let the core use its defaults ones: nand_read/write_buf(). Same for the next functions. Plus, if you don't use them inside ->exec_op() implementation, they have to be removed anyway. > +} > + > +/** > + * pl353_nand_write_buf - write buffer to chip > + * @mtd: Pointer to the mtd info structure > + * @buf: Pointer to the buffer to store read data > + * @len: Number of bytes to write > + */ > +static void pl353_nand_write_buf(struct mtd_info *mtd, const uint8_t *bu= f, > + int len) > +{ > + int i; > + struct nand_chip *chip =3D mtd_to_nand(mtd); > + unsigned long *ptr =3D (unsigned long *)buf; > + > + len >>=3D 2; > + > + for (i =3D 0; i < len; i++) > + writel(ptr[i], chip->IO_ADDR_W); > +} > + > +/** > + * pl353_nand_read_buf - read chip data into buffer > + * @chip: Pointer to the NAND chip info structure > + * @in: Pointer to the buffer to store read data > + * @len: Number of bytes to read > + * Return: 0 on success or error value on failure > + */ > +static int pl353_nand_read_buf(struct nand_chip *chip, > + uint8_t *in, > + unsigned int len) > +{ > + int i; > + > + for (i =3D 0; i < len; i++) > + in[i] =3D readb(chip->IO_ADDR_R); > + > + return 0; > +} > + > +/** > + * pl353_nand_calculate_hwecc - Calculate Hardware ECC > + * @mtd: Pointer to the mtd_info structure > + * @data: Pointer to the page data > + * @ecc_code: Pointer to the ECC buffer where ECC data needs to be stored You store ECC in a variable called "code", can you please make it consistent? > + * > + * This function retrieves the Hardware ECC data from the controller and= returns > + * ECC data back to the MTD subsystem. > + * > + * Return: 0 on success or error value on failure > + */ > +static int pl353_nand_calculate_hwecc(struct mtd_info *mtd, > + const u8 *data, u8 *ecc_code) > +{ > + u32 ecc_value, ecc_status; > + u8 ecc_reg, ecc_byte; > + unsigned long timeout =3D jiffies + PL353_NAND_ECC_BUSY_TIMEOUT; > + /* Wait till the ECC operation is complete or timeout */ > + do { > + if (pl353_smc_ecc_is_busy()) Where does this function come from? I would rather prefer a readl_relaxed_poll_timeout() or similar, if possible. > + cpu_relax(); > + else > + break; > + } while (!time_after_eq(jiffies, timeout)); > + > + if (time_after_eq(jiffies, timeout)) { > + pr_err("%s timed out\n", __func__); > + return -ETIMEDOUT; > + } > + > + for (ecc_reg =3D 0; ecc_reg < 4; ecc_reg++) { > + /* Read ECC value for each block */ > + ecc_value =3D pl353_smc_get_ecc_val(ecc_reg); I don't have this function neither? > + ecc_status =3D (ecc_value >> 24) & 0xFF; > + /* ECC value valid */ > + if (ecc_status & 0x40) { > + for (ecc_byte =3D 0; ecc_byte < 3; ecc_byte++) { > + /* Copy ECC bytes to MTD buffer */ > + *ecc_code =3D ~ecc_value & 0xFF; > + ecc_value =3D ecc_value >> 8; > + ecc_code++; > + } > + } else { > + pr_warn("%s status failed\n", __func__); > + return -1; > + } > + } > + return 0; > +} > + > +/** > + * onehot - onehot function > + * @value: Value to check for onehot > + * > + * This function checks whether a value is onehot or not. > + * onehot is if and only if onebit is set. s/onebit/one bit/ But I am sure there is already a function for that in the kernel, please don't use your own implementation for this kind of stuff. > + * > + * Return: 1 if it is onehot else 0 > + */ > +static int onehot(unsigned short value) > +{ > + return (value & (value - 1)) =3D=3D 0; > +} > + > +/** > + * pl353_nand_correct_data - ECC correction function > + * @mtd: Pointer to the mtd_info structure > + * @buf: Pointer to the page data > + * @read_ecc: Pointer to the ECC value read from spare data area > + * @calc_ecc: Pointer to the calculated ECC value > + * > + * This function corrects the ECC single bit errors & detects 2-bit erro= rs. > + * > + * Return: 0 if no ECC errors found > + * 1 if single bit error found and corrected. > + * -1 if multiple ECC errors found. > + */ > +static int pl353_nand_correct_data(struct mtd_info *mtd, unsigned char *= buf, > + unsigned char *read_ecc, > + unsigned char *calc_ecc) > +{ > + unsigned char bit_addr; > + unsigned int byte_addr; > + unsigned short ecc_odd, ecc_even, read_ecc_lower, read_ecc_upper; > + unsigned short calc_ecc_lower, calc_ecc_upper; > + > + read_ecc_lower =3D (read_ecc[0] | (read_ecc[1] << 8)) & 0xfff; > + read_ecc_upper =3D ((read_ecc[1] >> 4) | (read_ecc[2] << 4)) & 0xfff; > + > + calc_ecc_lower =3D (calc_ecc[0] | (calc_ecc[1] << 8)) & 0xfff; > + calc_ecc_upper =3D ((calc_ecc[1] >> 4) | (calc_ecc[2] << 4)) & 0xfff; > + > + ecc_odd =3D read_ecc_lower ^ calc_ecc_lower; > + ecc_even =3D read_ecc_upper ^ calc_ecc_upper; > + > + if ((ecc_odd =3D=3D 0) && (ecc_even =3D=3D 0)) > + return 0; /* no error */ What about: if (!ecc_odd && !ecc_even) return 0; > + > + if (ecc_odd =3D=3D (~ecc_even & 0xfff)) { > + /* bits [11:3] of error code is byte offset */ > + byte_addr =3D (ecc_odd >> 3) & 0x1ff; > + /* bits [2:0] of error code is bit offset */ > + bit_addr =3D ecc_odd & 0x7; > + /* Toggling error bit */ > + buf[byte_addr] ^=3D (1 << bit_addr); Use BIT(bit_addr) macro instead? > + return 1; > + } > + > + if (onehot(ecc_odd | ecc_even) =3D=3D 1) > + return 1; /* one error in parity */ Comment should be before the if statement here. > + > + return -1; /* Uncorrectable error */ And here, before the return statement. > +} > + > +static int pl353_dev_timeout(struct mtd_info *mtd, struct nand_chip *chi= p) > +{ > + unsigned long timeout =3D jiffies + PL353_NAND_DEV_BUSY_TIMEOUT; > + > + do { > + if (chip->dev_ready(mtd)) > + break; > + cpu_relax(); > + } while (!time_after_eq(jiffies, timeout)); > + > + if (time_after_eq(jiffies, timeout)) { > + pr_err("%s timed out\n", __func__); > + return -1; > + } > + > + return 0; > +} > + > +static void pl353_prepare_cmd(struct mtd_info *mtd, struct nand_chip *ch= ip, > + int page, int column, int start_cmd, int end_cmd, bool read) > +{ > + unsigned long data_phase_addr; > + u32 end_cmd_valid =3D 0; > + void __iomem *cmd_addr; > + unsigned long cmd_phase_addr =3D 0, cmd_data =3D 0; > + > + struct pl353_nand_info *xnand =3D > + container_of(chip, struct pl353_nand_info, chip); > + > + if (read) > + end_cmd_valid =3D 1; > + else > + end_cmd_valid =3D 0; This is a good spot to use the ternary operator :) > + > + cmd_phase_addr =3D (unsigned long __force)xnand->nand_base + ( > + (((xnand->row_addr_cycles) + (xnand->col_addr_cycles)) > + << ADDR_CYCLES_SHIFT) | > + (end_cmd_valid << END_CMD_VALID_SHIFT) | > + (COMMAND_PHASE) | > + (end_cmd << END_CMD_SHIFT) | Please don't align the '|' > + (start_cmd << START_CMD_SHIFT)); > + cmd_addr =3D (void __iomem * __force)cmd_phase_addr; > + > + /* Get the data phase address */ > + data_phase_addr =3D (unsigned long __force)xnand->nand_base + ( > + (0x0 << CLEAR_CS_SHIFT) | > + (0 << END_CMD_VALID_SHIFT) | > + (DATA_PHASE) | > + (end_cmd << END_CMD_SHIFT) | > + (0x0 << ECC_LAST_SHIFT)); > + > + chip->IO_ADDR_R =3D (void __iomem * __force)data_phase_addr; > + chip->IO_ADDR_W =3D chip->IO_ADDR_R; > + if (chip->options & NAND_BUSWIDTH_16) > + column >>=3D 1; / 2 > + cmd_data =3D column; > + if (mtd->writesize > PL353_NAND_ECC_SIZE) { > + cmd_data |=3D page << 16; > + /* Another address cycle for devices > 128MiB */ > + if (chip->chipsize > (128 << 20)) { Now there is a flag for that in the core, called NAND_ROW_ADDR_3. > + pl353_nand_write32(cmd_addr, cmd_data); > + cmd_data =3D (page >> 16); > + } > + } else { > + cmd_data |=3D page << 8; > + } Space > + pl353_nand_write32(cmd_addr, cmd_data); > +} > + > +/** > + * pl353_nand_read_oob - [REPLACEABLE] the most common OOB data read fun= ction > + * @mtd: Pointer to the mtd info structure > + * @chip: Pointer to the NAND chip info structure > + * @page: Page number to read > + * > + * Return: Always return zero > + */ > +static int pl353_nand_read_oob(struct mtd_info *mtd, struct nand_chip *c= hip, > + int page) > +{ > + > + unsigned long data_phase_addr; > + uint8_t *p; > + > + chip->pagebuf =3D -1; > + if (mtd->writesize < PL353_NAND_ECC_SIZE) > + return 0; > + > + pl353_prepare_cmd(mtd, chip, page, mtd->writesize, NAND_CMD_READ0, > + NAND_CMD_READSTART, 1); Alignment > + > + ndelay(100); > + pl353_dev_timeout(mtd, chip); > + > + p =3D chip->oob_poi; > + pl353_nand_read_buf_l(chip, p, > + (mtd->oobsize - PL353_NAND_LAST_TRANSFER_LENGTH)); Alignment > + p +=3D (mtd->oobsize - PL353_NAND_LAST_TRANSFER_LENGTH); > + > + data_phase_addr =3D (unsigned long __force)chip->IO_ADDR_R; > + data_phase_addr |=3D PL353_NAND_CLEAR_CS; > + chip->IO_ADDR_R =3D (void __iomem * __force)data_phase_addr; > + pl353_nand_read_buf_l(chip, p, PL353_NAND_LAST_TRANSFER_LENGTH); > + > + return 0; > +} > + > +/** > + * pl353_nand_write_oob - [REPLACEABLE] the most common OOB data write f= unction > + * @mtd: Pointer to the mtd info structure > + * @chip: Pointer to the NAND chip info structure > + * @page: Page number to write > + * > + * Return: Zero on success and EIO on failure > + */ Comments inside this function would be welcome! > +static int pl353_nand_write_oob(struct mtd_info *mtd, struct nand_chip *= chip, > + int page) > +{ > + Extra space > + const uint8_t *buf =3D chip->oob_poi; > + unsigned long data_phase_addr; > + struct pl353_nand_info *xnand =3D > + container_of(chip, struct pl353_nand_info, chip); > + u32 addrcycles =3D 0, ret; > + unsigned long timeout =3D jiffies + PL353_NAND_DEV_BUSY_TIMEOUT; > + u8 status; > + > + chip->pagebuf =3D -1; > + addrcycles =3D xnand->row_addr_cycles + xnand->col_addr_cycles; > + pl353_prepare_cmd(mtd, chip, page, mtd->writesize, NAND_CMD_SEQIN, > + NAND_CMD_PAGEPROG, 0); > + ndelay(100); > + pl353_nand_write_buf(mtd, buf, > + (mtd->oobsize - PL353_NAND_LAST_TRANSFER_LENGTH)); > + buf +=3D (mtd->oobsize - PL353_NAND_LAST_TRANSFER_LENGTH); > + > + data_phase_addr =3D (unsigned long __force)chip->IO_ADDR_W; > + data_phase_addr |=3D PL353_NAND_CLEAR_CS; > + data_phase_addr |=3D (1 << END_CMD_VALID_SHIFT); > + chip->IO_ADDR_W =3D (void __iomem * __force)data_phase_addr; > + pl353_nand_write_buf(mtd, buf, PL353_NAND_LAST_TRANSFER_LENGTH); > + > + /* Send command to program the OOB data */ > + ret =3D nand_status_op(chip, &status); Doing this without exiting the status state is broken. > + timeout =3D jiffies + msecs_to_jiffies(timeout); > + do { > + if (chip->dev_ready) { > + if (chip->dev_ready(mtd)) > + break; > + } else { You give your own implementation of ->dev_ready(). So this is dead code. > + if (status & NAND_STATUS_READY) You don't update "status", while you wait for it to change. > + break; > + } > + cond_resched(); > + } while (time_before(jiffies, timeout)); > + > + /* This can happen if in case of timeout or buggy dev_ready */ > + WARN_ON(!(status & NAND_STATUS_READY)); I think the whole block has to be replaced by a simple nand_wait_ready() call. > + > + return (status & NAND_STATUS_FAIL) ? -EIO : 0; > +} > + > +/** > + * pl353_nand_read_page_raw - [Intern] read raw page data without ecc > + * @mtd: Pointer to the mtd info structure > + * @chip: Pointer to the NAND chip info structure > + * @buf: Pointer to the data buffer > + * @oob_required: Caller requires OOB data read to chip->oob_poi > + * @page: Page number to read > + * > + * Return: Always return zero > + */ > +static int pl353_nand_read_page_raw(struct mtd_info *mtd, > + struct nand_chip *chip, > + uint8_t *buf, int oob_required, int page) Do you really need raw accessors? Not sure this is needed. > +{ > + unsigned long data_phase_addr; > + uint8_t *p; > + > + pl353_nand_read_buf_l(chip, buf, mtd->writesize); > + p =3D chip->oob_poi; > + pl353_nand_read_buf_l(chip, p, > + (mtd->oobsize - PL353_NAND_LAST_TRANSFER_LENGTH)); > + p +=3D (mtd->oobsize - PL353_NAND_LAST_TRANSFER_LENGTH); > + > + data_phase_addr =3D (unsigned long __force)chip->IO_ADDR_R; > + data_phase_addr |=3D PL353_NAND_CLEAR_CS; > + chip->IO_ADDR_R =3D (void __iomem * __force)data_phase_addr; > + > + pl353_nand_read_buf_l(chip, p, PL353_NAND_LAST_TRANSFER_LENGTH); > + return 0; > +} > + > +/** > + * pl353_nand_write_page_raw - [Intern] raw page write function > + * @mtd: Pointer to the mtd info structure > + * @chip: Pointer to the NAND chip info structure > + * @buf: Pointer to the data buffer > + * @oob_required: Caller requires OOB data read to chip->oob_poi > + * @page: Page number to write > + * > + * Return: Always return zero > + */ > +static int pl353_nand_write_page_raw(struct mtd_info *mtd, > + struct nand_chip *chip, > + const uint8_t *buf, int oob_required, > + int page) > +{ > + unsigned long data_phase_addr; > + uint8_t *p; > + > + pl353_prepare_cmd(mtd, chip, page, 0, NAND_CMD_SEQIN, > + NAND_CMD_PAGEPROG, 0); > + pl353_nand_write_buf(mtd, buf, mtd->writesize); > + p =3D chip->oob_poi; > + pl353_nand_write_buf(mtd, p, > + (mtd->oobsize - PL353_NAND_LAST_TRANSFER_LENGTH)); > + p +=3D (mtd->oobsize - PL353_NAND_LAST_TRANSFER_LENGTH); > + > + data_phase_addr =3D (unsigned long __force)chip->IO_ADDR_W; > + data_phase_addr |=3D PL353_NAND_CLEAR_CS; > + data_phase_addr |=3D (1 << END_CMD_VALID_SHIFT); > + chip->IO_ADDR_W =3D (void __iomem * __force)data_phase_addr; > + > + pl353_nand_write_buf(mtd, p, PL353_NAND_LAST_TRANSFER_LENGTH); > + > + return 0; > +} > + > +/** > + * nand_write_page_hwecc - Hardware ECC based page write function > + * @mtd: Pointer to the mtd info structure > + * @chip: Pointer to the NAND chip info structure > + * @buf: Pointer to the data buffer > + * @oob_required: Caller requires OOB data read to chip->oob_poi > + * @page: Page number to write > + * > + * This functions writes data and hardware generated ECC values in to th= e page. > + * > + * Return: Always return zero > + */ > +static int pl353_nand_write_page_hwecc(struct mtd_info *mtd, > + struct nand_chip *chip, const uint8_t *buf, > + int oob_required, int page) > +{ > + > + int eccsize =3D chip->ecc.size; > + int eccsteps =3D chip->ecc.steps; > + uint8_t *ecc_calc =3D chip->ecc.calc_buf; > + const uint8_t *p =3D buf; > + uint8_t *oob_ptr; > + u32 ret; > + unsigned long data_phase_addr, timeo; > + u8 status; > + > + pl353_prepare_cmd(mtd, chip, page, 0, NAND_CMD_SEQIN, > + NAND_CMD_PAGEPROG, 0); > + ndelay(100); > + for ( ; (eccsteps - 1); eccsteps--) { > + pl353_nand_write_buf(mtd, p, eccsize); > + p +=3D eccsize; > + } > + pl353_nand_write_buf(mtd, p, > + (eccsize - PL353_NAND_LAST_TRANSFER_LENGTH)); > + p +=3D (eccsize - PL353_NAND_LAST_TRANSFER_LENGTH); > + > + /* Set ECC Last bit to 1 */ > + data_phase_addr =3D (unsigned long __force)chip->IO_ADDR_W; > + data_phase_addr |=3D PL353_NAND_ECC_LAST; > + chip->IO_ADDR_W =3D (void __iomem * __force)data_phase_addr; > + pl353_nand_write_buf(mtd, p, PL353_NAND_LAST_TRANSFER_LENGTH); > + > + p =3D buf; > + chip->ecc.calculate(mtd, p, &ecc_calc[0]); > + > + /* Wait for ECC to be calculated and read the error values */ > + ret =3D mtd_ooblayout_set_eccbytes(mtd, ecc_calc, chip->oob_poi, > + 0, chip->ecc.total); > + if (ret) > + return ret; > + /* Clear ECC last bit */ > + data_phase_addr =3D (unsigned long __force)chip->IO_ADDR_W; > + data_phase_addr &=3D ~PL353_NAND_ECC_LAST; > + chip->IO_ADDR_W =3D (void __iomem * __force)data_phase_addr; > + > + /* Write the spare area with ECC bytes */ > + oob_ptr =3D chip->oob_poi; > + pl353_nand_write_buf(mtd, oob_ptr, > + (mtd->oobsize - PL353_NAND_LAST_TRANSFER_LENGTH)); > + > + data_phase_addr =3D (unsigned long __force)chip->IO_ADDR_W; > + data_phase_addr |=3D PL353_NAND_CLEAR_CS; > + data_phase_addr |=3D (1 << END_CMD_VALID_SHIFT); > + chip->IO_ADDR_W =3D (void __iomem * __force)data_phase_addr; > + oob_ptr +=3D (mtd->oobsize - PL353_NAND_LAST_TRANSFER_LENGTH); > + pl353_nand_write_buf(mtd, oob_ptr, PL353_NAND_LAST_TRANSFER_LENGTH); Once using ->exec_op(), you should use something like nand_write_data_op(). > + > + /* > + * Apply this short delay always to ensure that we do wait tWB in any > + * case on any machine. > + */ > + ndelay(100); > + ret =3D nand_status_op(chip, &status); > + timeo =3D jiffies + msecs_to_jiffies(400); > + do { > + if (chip->dev_ready) { > + if (chip->dev_ready(mtd)) > + break; > + } else { > + if (status & NAND_STATUS_READY) > + break; > + } > + cond_resched(); > + } while (time_before(jiffies, timeo)); > + > + /* This can happen if in case of timeout or buggy dev_ready */ > + WARN_ON(!(status & NAND_STATUS_READY)); > + Same here about ->dev_ready(). > + return (status & NAND_STATUS_FAIL) ? -EIO : 0; > +} > + > +/** > + * pl353_nand_write_page_swecc - [REPLACEABLE] software ecc based page w= rite > + * function > + * @mtd: Pointer to the mtd info structure > + * @chip: Pointer to the NAND chip info structure > + * @buf: Pointer to the data buffer > + * @oob_required: Caller requires OOB data read to chip->oob_poi > + * @page: Page number to write > + * > + * Return: Always return zero > + */ > +static int pl353_nand_write_page_swecc(struct mtd_info *mtd, > + struct nand_chip *chip, const uint8_t *buf, > + int oob_required, int page) > +{ This is an exact copy of the core's function, you probably don't need it. > + int i, eccsize =3D chip->ecc.size; > + int eccbytes =3D chip->ecc.bytes; > + int eccsteps =3D chip->ecc.steps; > + uint8_t *ecc_calc =3D chip->ecc.calc_buf; > + const uint8_t *p =3D buf; > + u32 ret; > + > + for (i =3D 0; eccsteps; eccsteps--, i +=3D eccbytes, p +=3D eccsize) > + chip->ecc.calculate(mtd, p, &ecc_calc[0]); > + > + ret =3D mtd_ooblayout_set_eccbytes(mtd, ecc_calc, chip->oob_poi, > + 0, chip->ecc.total); > + if (ret) > + return ret; > + chip->ecc.write_page_raw(mtd, chip, buf, 1, page); > + > + return 0; > +} > + > +/** > + * pl353_nand_read_page_hwecc - Hardware ECC based page read function > + * @mtd: Pointer to the mtd info structure > + * @chip: Pointer to the NAND chip info structure > + * @buf: Pointer to the buffer to store read data > + * @oob_required: Caller requires OOB data read to chip->oob_poi > + * @page: Page number to read > + * > + * This functions reads data and checks the data integrity by comparing = hardware > + * generated ECC values and read ECC values from spare area. > + * > + * Return: 0 always and updates ECC operation status in to MTD structure > + */ > +static int pl353_nand_read_page_hwecc(struct mtd_info *mtd, > + struct nand_chip *chip, > + uint8_t *buf, int oob_required, int page) > +{ > + int i, stat, eccsize =3D chip->ecc.size; > + int eccbytes =3D chip->ecc.bytes; > + int eccsteps =3D chip->ecc.steps; > + uint8_t *p =3D buf; > + uint8_t *ecc_calc =3D chip->ecc.calc_buf; > + uint8_t *ecc_code =3D chip->ecc.code_buf; > + > + uint8_t *oob_ptr; > + u32 ret; > + unsigned long data_phase_addr; > + unsigned long timeout =3D jiffies + PL353_NAND_DEV_BUSY_TIMEOUT; > + > + pl353_prepare_cmd(mtd, chip, page, 0, NAND_CMD_READ0, > + NAND_CMD_READSTART, 1); > + ndelay(100); > + do { > + if (chip->dev_ready(mtd)) > + break; > + cpu_relax(); > + } while (!time_after_eq(jiffies, timeout)); > + > + if (time_after_eq(jiffies, timeout)) { > + pr_err("%s timed out\n", __func__); > + return -1; > + } > + for ( ; (eccsteps - 1); eccsteps--) { > + pl353_nand_read_buf_l(chip, p, eccsize); > + p +=3D eccsize; > + } > + pl353_nand_read_buf_l(chip, p, > + (eccsize - PL353_NAND_LAST_TRANSFER_LENGTH)); > + p +=3D (eccsize - PL353_NAND_LAST_TRANSFER_LENGTH); > + > + /* Set ECC Last bit to 1 */ > + data_phase_addr =3D (unsigned long __force)chip->IO_ADDR_R; > + data_phase_addr |=3D PL353_NAND_ECC_LAST; > + chip->IO_ADDR_R =3D (void __iomem * __force)data_phase_addr; > + pl353_nand_read_buf_l(chip, p, PL353_NAND_LAST_TRANSFER_LENGTH); > + > + /* Read the calculated ECC value */ > + p =3D buf; > + chip->ecc.calculate(mtd, p, &ecc_calc[0]); > + > + /* Clear ECC last bit */ > + data_phase_addr =3D (unsigned long __force)chip->IO_ADDR_R; > + data_phase_addr &=3D ~PL353_NAND_ECC_LAST; > + chip->IO_ADDR_R =3D (void __iomem * __force)data_phase_addr; > + > + /* Read the stored ECC value */ > + oob_ptr =3D chip->oob_poi; > + pl353_nand_read_buf_l(chip, oob_ptr, > + (mtd->oobsize - PL353_NAND_LAST_TRANSFER_LENGTH)); > + > + /* de-assert chip select */ > + data_phase_addr =3D (unsigned long __force)chip->IO_ADDR_R; > + data_phase_addr |=3D PL353_NAND_CLEAR_CS; > + chip->IO_ADDR_R =3D (void __iomem * __force)data_phase_addr; > + > + oob_ptr +=3D (mtd->oobsize - PL353_NAND_LAST_TRANSFER_LENGTH); > + pl353_nand_read_buf_l(chip, oob_ptr, PL353_NAND_LAST_TRANSFER_LENGTH); > + > + ret =3D mtd_ooblayout_get_eccbytes(mtd, ecc_code, chip->oob_poi, 0, > + chip->ecc.total); > + if (ret) > + return ret; > + > + eccsteps =3D chip->ecc.steps; > + p =3D buf; > + > + /* Check ECC error for all blocks and correct if it is correctable */ > + for (i =3D 0 ; eccsteps; eccsteps--, i +=3D eccbytes, p +=3D eccsize) { In this loop you should monitore a max_bitflips value and return it instead of zero at the end of the function. > + stat =3D chip->ecc.correct(mtd, p, &ecc_code[i], &ecc_calc[i]); > + if (stat < 0) > + mtd->ecc_stats.failed++; > + else > + mtd->ecc_stats.corrected +=3D stat; > + } > + return 0; > +} > + > +/** > + * pl353_nand_read_page_swecc - [REPLACEABLE] software ecc based page re= ad > + * function > + * @mtd: Pointer to the mtd info structure > + * @chip: Pointer to the NAND chip info structure > + * @buf: Pointer to the buffer to store read data > + * @oob_required: Caller requires OOB data read to chip->oob_poi > + * @page: Page number to read > + * > + * Return: Always return zero > + */ > +static int pl353_nand_read_page_swecc(struct mtd_info *mtd, > + struct nand_chip *chip, > + uint8_t *buf, int oob_required, int page) > +{ Same, you probably don't need this function. > + int i, eccsize =3D chip->ecc.size; > + int eccbytes =3D chip->ecc.bytes; > + int eccsteps =3D chip->ecc.steps; > + uint8_t *p =3D buf; > + uint8_t *ecc_calc =3D chip->ecc.calc_buf; > + uint8_t *ecc_code =3D chip->ecc.code_buf; > + u32 ret; > + > + chip->ecc.read_page_raw(mtd, chip, buf, page, 1); > + > + for (i =3D 0; eccsteps; eccsteps--, i +=3D eccbytes, p +=3D eccsize) > + chip->ecc.calculate(mtd, p, &ecc_calc[i]); > + > + ret =3D mtd_ooblayout_get_eccbytes(mtd, ecc_calc, chip->oob_poi, > + 0, chip->ecc.total); > + > + eccsteps =3D chip->ecc.steps; > + p =3D buf; > + > + for (i =3D 0 ; eccsteps; eccsteps--, i +=3D eccbytes, p +=3D eccsize) { > + int stat; > + > + stat =3D chip->ecc.correct(mtd, p, &ecc_code[i], &ecc_calc[i]); > + if (stat < 0) > + mtd->ecc_stats.failed++; > + else > + mtd->ecc_stats.corrected +=3D stat; > + } > + return 0; > +} > + > +/** > + * pl353_nand_select_chip - Select the flash device > + * @mtd: Pointer to the mtd info structure > + * @chip: Pointer to the NAND chip info structure > + * > + * This function is empty as the NAND controller handles chip select line > + * internally based on the chip address passed in command and data phase. > + */ > +static void pl353_nand_select_chip(struct mtd_info *mtd, int chip) > +{ > +} > + > +/* NAND framework ->exec_op() hooks and related helpers */ > +static void pl353_nfc_parse_instructions(struct nand_chip *chip, > + const struct nand_subop *subop, > + struct pl353_nfc_op *nfc_op) > +{ > + const struct nand_op_instr *instr =3D NULL; > + unsigned int op_id, offset, naddrs; > + int i; > + const u8 *addrs; > + > + memset(nfc_op, 0, sizeof(struct pl353_nfc_op)); > + for (op_id =3D 0; op_id < subop->ninstrs; op_id++) { > + What is this for-loop for? I don't get it as you break the switch in every case? > + nfc_op->len =3D nand_subop_get_data_len(subop, op_id); > + > + instr =3D &subop->instrs[op_id]; > + if (subop->ninstrs =3D=3D 1) > + nfc_op->cmnds[0] =3D -1; > + switch (instr->type) { > + case NAND_OP_CMD_INSTR: > + nfc_op->type =3D NAND_OP_CMD_INSTR; > + nfc_op->end_cmd =3D op_id - 1; > + if (op_id) You should put { } on the if also if the else statement needs braces. > + nfc_op->cmnds[1] =3D instr->ctx.cmd.opcode; > + else { > + nfc_op->cmnds[0] =3D instr->ctx.cmd.opcode; > + nfc_op->cmnds[1] =3D -1; > + } > + break; > + > + case NAND_OP_ADDR_INSTR: > + offset =3D nand_subop_get_addr_start_off(subop, op_id); > + naddrs =3D nand_subop_get_num_addr_cyc(subop, op_id); > + addrs =3D &instr->ctx.addr.addrs[offset]; > + nfc_op->addrs =3D instr->ctx.addr.addrs[offset]; > + for (i =3D 0; i < min_t(unsigned int, 4, naddrs); i++) > + nfc_op->addrs |=3D instr->ctx.addr.addrs[i] << > + (8 * i); > + > + if (naddrs >=3D 5) { > + nfc_op->addrs >>=3D 16; > + nfc_op->addrs |=3D (addrs[4] << 16); > + nfc_op->thirdrow =3D 1; > + } > + nfc_op->naddrs =3D nand_subop_get_num_addr_cyc > + (subop, op_id); Don't put the parameters of a function on the next line like that? > + break; > + > + case NAND_OP_DATA_IN_INSTR: > + nfc_op->data_instr =3D instr; > + nfc_op->type =3D NAND_OP_DATA_IN_INSTR; > + nfc_op->data_instr_idx =3D op_id; > + break; > + > + case NAND_OP_DATA_OUT_INSTR: > + nfc_op->data_instr =3D instr; > + nfc_op->type =3D NAND_OP_DATA_IN_INSTR; DATA_OUT_INSTR? Is this really tested? > + nfc_op->data_instr_idx =3D op_id; > + break; > + > + case NAND_OP_WAITRDY_INSTR: > + nfc_op->rdy_timeout_ms =3D instr->ctx.waitrdy.timeout_ms; > + nfc_op->rdy_delay_ns =3D instr->delay_ns; > + nfc_op->wait =3D true; > + break; > + } > + } > +} > + > +/** > + * pl353_nand_cmd_function - Send command to NAND device > + * @chip: Pointer to the NAND chip info structure > + * @subop: Pointer to array of instructions > + * Return: Always return zero > + */ > +static int pl353_nand_cmd_function(struct nand_chip *chip, > + const struct nand_subop *subop) > +{ > + struct mtd_info *mtd =3D nand_to_mtd(chip); > + const struct nand_op_instr *instr; > + struct pl353_nfc_op nfc_op; nfc_op =3D {}; to initialize to 0. > + struct pl353_nand_info *xnand =3D > + container_of(chip, struct pl353_nand_info, chip); > + void __iomem *cmd_addr; > + unsigned long cmd_data =3D 0, end_cmd_valid =3D 0; > + unsigned long cmd_phase_addr, data_phase_addr, end_cmd; > + unsigned long timeout =3D jiffies + PL353_NAND_DEV_BUSY_TIMEOUT; > + u32 addrcycles =3D 0; > + unsigned int op_id, len, offset; > + > + pl353_nfc_parse_instructions(chip, subop, &nfc_op); > + instr =3D nfc_op.data_instr; > + op_id =3D nfc_op.data_instr_idx; > + len =3D nand_subop_get_data_len(subop, op_id); > + offset =3D nand_subop_get_data_start_off(subop, op_id); > + > + if (nfc_op.cmnds[0] !=3D -1) { > + if (xnand->end_cmd_pending) { > + /* > + * Check for end command if this command request is > + * same as the pending command then return > + */ > + if (xnand->end_cmd =3D=3D nfc_op.cmnds[0]) { > + xnand->end_cmd =3D 0; > + xnand->end_cmd_pending =3D 0; > + return 0; > + } > + } > + > + /* Clear interrupt */ > + pl353_smc_clr_nand_int(); > + end_cmd_valid =3D 0; Space > + /* Get the command phase address */ > + if (nfc_op.cmnds[1] !=3D -1) { > + end_cmd_valid =3D 1; > + } else { > + if (nfc_op.cmnds[0] =3D=3D NAND_CMD_READ0) > + return 0; > + } > + if (nfc_op.end_cmd =3D=3D NAND_CMD_NONE) > + end_cmd =3D 0x0; > + else > + end_cmd =3D nfc_op.cmnds[1]; > + > + addrcycles =3D nfc_op.naddrs; > + if (nfc_op.cmnds[0] =3D=3D NAND_CMD_READ0 || > + nfc_op.cmnds[0] =3D=3D NAND_CMD_SEQIN) > + addrcycles =3D xnand->row_addr_cycles + > + xnand->col_addr_cycles; > + else if ((nfc_op.cmnds[0] =3D=3D NAND_CMD_ERASE1) || > + (nfc_op.cmnds[0] =3D=3D NAND_CMD_ERASE2)) > + addrcycles =3D xnand->row_addr_cycles; > + else > + addrcycles =3D nfc_op.naddrs; A switch block would probably be appropriate. > + cmd_phase_addr =3D (unsigned long __force)xnand->nand_base + ( > + (addrcycles << ADDR_CYCLES_SHIFT) | > + (end_cmd_valid << END_CMD_VALID_SHIFT) | > + (COMMAND_PHASE) | > + (end_cmd << END_CMD_SHIFT) | > + (nfc_op.cmnds[0] << START_CMD_SHIFT)); > + > + cmd_addr =3D (void __iomem * __force)cmd_phase_addr; Space > + /* Get the data phase address */ > + end_cmd_valid =3D 0; > + > + data_phase_addr =3D (unsigned long __force)xnand->nand_base + ( > + (0x0 << CLEAR_CS_SHIFT) | > + (end_cmd_valid << END_CMD_VALID_SHIFT)| > + (DATA_PHASE) | > + (end_cmd << END_CMD_SHIFT) | > + (0x0 << ECC_LAST_SHIFT)); > + chip->IO_ADDR_R =3D (void __iomem * __force)data_phase_addr; Do you really need this "__force" ? > + chip->IO_ADDR_W =3D chip->IO_ADDR_R; > + /* Command phase AXI write */ > + /* Read & Write */ > + if (nfc_op.thirdrow) { > + nfc_op.thirdrow =3D 0; > + if (mtd->writesize > PL353_NAND_ECC_SIZE) { > + cmd_data |=3D nfc_op.addrs << 16; > + /* Another address cycle for devices > 128MiB */ > + if (chip->chipsize > (128 << 20)) { > + pl353_nand_write32(cmd_addr, cmd_data); > + cmd_data =3D (nfc_op.addrs >> 16); > + } > + } > + } else { > + if (nfc_op.addrs !=3D -1) { > + int column =3D nfc_op.addrs; > + /* > + * Change read/write column, read id etc > + * Adjust columns for 16 bit bus width > + */ > + if ((chip->options & NAND_BUSWIDTH_16) && > + ((nfc_op.cmnds[0] =3D=3D NAND_CMD_READ0) || > + (nfc_op.cmnds[0] =3D=3D NAND_CMD_SEQIN) || > + (nfc_op.cmnds[0] =3D=3D NAND_CMD_RNDOUT) || > + (nfc_op.cmnds[0] =3D=3D NAND_CMD_RNDIN))) { Alignment. > + column >>=3D 1; > + } > + cmd_data =3D nfc_op.addrs; > + } > + } > + pl353_nand_write32(cmd_addr, cmd_data); > + if (nfc_op.type !=3D 0) { > + xnand->end_cmd =3D nfc_op.end_cmd; > + xnand->end_cmd_pending =3D 1; > + } > + ndelay(100); Why? > + if (nfc_op.cmnds[0] =3D=3D 0xef) > + nfc_op.wait =3D false; > + if (nfc_op.wait) { > + nfc_op.wait =3D false; You can remove this line. > + do { > + if (chip->dev_ready(mtd)) > + break; > + cpu_relax(); > + } while (!time_after_eq(jiffies, timeout)); > + if (time_after_eq(jiffies, timeout)) { > + pr_err("%s timed out\n", __func__); > + return -ETIMEDOUT; > + } Same comment about the wait. > + return 0; > + } > + } > + > + if (instr =3D=3D NULL) if (!instr) > + return 0; > + if (instr->type =3D=3D NAND_OP_DATA_IN_INSTR) > + return pl353_nand_read_buf(chip, instr->ctx.data.buf.in, len); > + > + if (instr->type =3D=3D NAND_OP_DATA_OUT_INSTR) { > + if ((nfc_op.cmnds[0] =3D=3D NAND_CMD_PAGEPROG) || > + (nfc_op.cmnds[0] =3D=3D NAND_CMD_SEQIN)) > + pl353_nand_write_page_raw(mtd, chip, > + instr->ctx.data.buf.out, 0, nfc_op.addrs); > + else > + pl353_nand_write_buf_l(chip, instr->ctx.data.buf.out, > + len); > + return 0; > + } > + return 0; > +} > + > +static const struct nand_op_parser pl353_nfc_op_parser =3D NAND_OP_PARSE= R( > + NAND_OP_PARSER_PATTERN( > + pl353_nand_cmd_function, > + NAND_OP_PARSER_PAT_CMD_ELEM(false), > + NAND_OP_PARSER_PAT_ADDR_ELEM(false, 7), > + NAND_OP_PARSER_PAT_DATA_IN_ELEM(false, 8)), > + NAND_OP_PARSER_PATTERN( > + pl353_nand_cmd_function, > + NAND_OP_PARSER_PAT_CMD_ELEM(false), > + NAND_OP_PARSER_PAT_ADDR_ELEM(false, 7), > + NAND_OP_PARSER_PAT_CMD_ELEM(false), > + NAND_OP_PARSER_PAT_WAITRDY_ELEM(false)), > + NAND_OP_PARSER_PATTERN( > + pl353_nand_cmd_function, > + NAND_OP_PARSER_PAT_CMD_ELEM(false), > + NAND_OP_PARSER_PAT_DATA_IN_ELEM(false, 8)), > + NAND_OP_PARSER_PATTERN( > + pl353_nand_cmd_function, > + NAND_OP_PARSER_PAT_CMD_ELEM(false), > + NAND_OP_PARSER_PAT_ADDR_ELEM(false, 7), > + NAND_OP_PARSER_PAT_WAITRDY_ELEM(false)), > + NAND_OP_PARSER_PATTERN( > + pl353_nand_cmd_function, > + NAND_OP_PARSER_PAT_CMD_ELEM(false), > + NAND_OP_PARSER_PAT_ADDR_ELEM(false, 8), > + NAND_OP_PARSER_PAT_DATA_OUT_ELEM(false, 2048), > + NAND_OP_PARSER_PAT_CMD_ELEM(true), > + NAND_OP_PARSER_PAT_WAITRDY_ELEM(true)), > + NAND_OP_PARSER_PATTERN( > + pl353_nand_cmd_function, > + NAND_OP_PARSER_PAT_CMD_ELEM(false), > + NAND_OP_PARSER_PAT_WAITRDY_ELEM(false)), > + NAND_OP_PARSER_PATTERN( > + pl353_nand_cmd_function, > + NAND_OP_PARSER_PAT_DATA_IN_ELEM(false, 2048)), > + NAND_OP_PARSER_PATTERN( > + pl353_nand_cmd_function, > + NAND_OP_PARSER_PAT_CMD_ELEM(false)), I am pretty sure you can factorize all these patterns now. Use the "optional" parameter for that. > + ); > + > +static int pl353_nfc_exec_op(struct nand_chip *chip, > + const struct nand_operation *op, > + bool check_only) > +{ > + return nand_op_parser_exec_op(chip, &pl353_nfc_op_parser, > + op, check_only); > +} > + > +/** > + * pl353_nand_device_ready - Check device ready/busy line > + * @mtd: Pointer to the mtd_info structure > + * > + * Return: 0 on busy or 1 on ready state > + */ > +static int pl353_nand_device_ready(struct mtd_info *mtd) > +{ > + if (pl353_smc_get_nand_int_status_raw()) { I don't understand where this new function is declared. > + pl353_smc_clr_nand_int(); > + return 1; > + } > + return 0; > +} > + > +/** > + * pl353_nand_ecc_init - Initialize the ecc information as per the ecc m= ode > + * @mtd: Pointer to the mtd_info structure > + * @ecc: Pointer to ECC control structure > + * @ecc_mode: ondie ecc status > + * > + * This function initializes the ecc block and functional pointers as pe= r the > + * ecc mode > + */ > +static void pl353_nand_ecc_init(struct mtd_info *mtd, struct nand_ecc_ct= rl *ecc, > + int ecc_mode) > +{ > + struct nand_chip *chip =3D mtd_to_nand(mtd); > + > + ecc->read_oob =3D pl353_nand_read_oob; > + ecc->read_page_raw =3D pl353_nand_read_page_raw; > + ecc->write_oob =3D pl353_nand_write_oob; > + ecc->write_page_raw =3D pl353_nand_write_page_raw; > + > + if (ecc_mode =3D=3D NAND_ECC_ON_DIE) { > + pl353_smc_set_ecc_mode(PL353_SMC_ECCMODE_BYPASS); > + > + /* > + * The software ECC routines won't work with the > + * SMC controller > + */ > + ecc->read_page =3D pl353_nand_read_page_raw; > + ecc->write_page =3D pl353_nand_write_page_raw; > + /* > + * On-Die ECC spare bytes offset 8 is used for ECC codes > + * Use the BBT pattern descriptors > + */ > + chip->bbt_td =3D &bbt_main_descr; > + chip->bbt_md =3D &bbt_mirror_descr; > + } else { > + ecc->mode =3D NAND_ECC_HW; > + /* Hardware ECC generates 3 bytes ECC code for each 512 bytes */ > + ecc->bytes =3D 3; > + ecc->strength =3D 1; > + ecc->calculate =3D pl353_nand_calculate_hwecc; > + ecc->correct =3D pl353_nand_correct_data; > + ecc->hwctl =3D NULL; You can drop this. > + ecc->read_page =3D pl353_nand_read_page_hwecc; > + ecc->size =3D PL353_NAND_ECC_SIZE; > + ecc->write_page =3D pl353_nand_write_page_hwecc; > + pl353_smc_set_ecc_pg_size(mtd->writesize); > + switch (mtd->writesize) { > + case 512: > + case 1024: > + case 2048: I prefer SZ_512, SZ_1K, SZ_2K. > + pl353_smc_set_ecc_mode(PL353_SMC_ECCMODE_APB); > + break; > + default: > + /* > + * The software ECC routines won't work with the > + * SMC controller > + */ > + ecc->calculate =3D nand_calculate_ecc; > + ecc->correct =3D nand_correct_data; > + ecc->read_page =3D pl353_nand_read_page_swecc; > + ecc->write_page =3D pl353_nand_write_page_swecc; > + ecc->size =3D 256; > + break; > + } > + > + if (mtd->oobsize =3D=3D 16) > + mtd_set_ooblayout(mtd, &pl353_ecc_ooblayout16_ops); > + else if (mtd->oobsize =3D=3D 64) > + mtd_set_ooblayout(mtd, &pl353_ecc_ooblayout64_ops); > + } > +} > + > +/** > + * pl353_nand_probe - Probe method for the NAND driver > + * @pdev: Pointer to the platform_device structure > + * > + * This function initializes the driver data structures and the hardware. > + * > + * Return: 0 on success or error value on failure > + */ > +static int pl353_nand_probe(struct platform_device *pdev) > +{ > + struct pl353_nand_info *xnand; > + struct mtd_info *mtd; > + struct nand_chip *nand_chip; > + struct resource *res; > + > + xnand =3D devm_kzalloc(&pdev->dev, sizeof(*xnand), GFP_KERNEL); > + if (!xnand) > + return -ENOMEM; > + > + /* Map physical address of NAND flash */ > + res =3D platform_get_resource(pdev, IORESOURCE_MEM, 0); > + xnand->nand_base =3D devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(xnand->nand_base)) > + return PTR_ERR(xnand->nand_base); > + > + nand_chip =3D &xnand->chip; > + mtd =3D nand_to_mtd(nand_chip); > + nand_chip->exec_op =3D pl353_nfc_exec_op; > + nand_set_controller_data(nand_chip, xnand); > + mtd->priv =3D nand_chip; > + mtd->owner =3D THIS_MODULE; > + mtd->name =3D PL353_NAND_DRIVER_NAME; > + nand_set_flash_node(nand_chip, pdev->dev.of_node); > + > + /* Set address of NAND IO lines */ > + nand_chip->IO_ADDR_R =3D xnand->nand_base; > + nand_chip->IO_ADDR_W =3D xnand->nand_base; > + /* Set the driver entry points for MTD */ > + nand_chip->dev_ready =3D pl353_nand_device_ready; > + nand_chip->select_chip =3D pl353_nand_select_chip; > + /* If we don't set this delay driver sets 20us by default */ > + nand_chip->chip_delay =3D 30; > + > + /* Set the device option and flash width */ > + nand_chip->options =3D NAND_BUSWIDTH_AUTO; > + nand_chip->bbt_options =3D NAND_BBT_USE_FLASH; > + platform_set_drvdata(pdev, xnand); > + > + /* first scan to find the device and get the page size */ > + if (nand_scan_ident(mtd, 1, NULL)) { > + dev_err(&pdev->dev, "nand_scan_ident for NAND failed\n"); > + return -ENXIO; > + } > + > + xnand->row_addr_cycles =3D nand_chip->onfi_params.addr_cycles & 0xF; > + xnand->col_addr_cycles =3D > + (nand_chip->onfi_params.addr_cycles >> 4) & 0xF; > + > + pl353_nand_ecc_init(mtd, &nand_chip->ecc, nand_chip->ecc.mode); > + if (nand_chip->options & NAND_BUSWIDTH_16) > + pl353_smc_set_buswidth(PL353_SMC_MEM_WIDTH_16); > + /* second phase scan */ > + if (nand_scan_tail(mtd)) { > + dev_err(&pdev->dev, "nand_scan_tail for NAND failed\n"); > + return -ENXIO; > + } > + > + mtd_device_register(mtd, NULL, 0); > + > + return 0; > +} > + > +/** > + * pl353_nand_remove - Remove method for the NAND driver > + * @pdev: Pointer to the platform_device structure > + * > + * This function is called if the driver module is being unloaded. It fr= ees all > + * resources allocated to the device. > + * > + * Return: 0 on success or error value on failure > + */ > +static int pl353_nand_remove(struct platform_device *pdev) > +{ > + struct pl353_nand_info *xnand =3D platform_get_drvdata(pdev); > + struct mtd_info *mtd =3D nand_to_mtd(&xnand->chip); > + > + /* Release resources, unregister device */ > + nand_release(mtd); > + > + return 0; > +} > + > +/* Match table for device tree binding */ > +static const struct of_device_id pl353_nand_of_match[] =3D { > + { .compatible =3D "arm,pl353-nand-r2p1" }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, pl353_nand_of_match); > + > +/* > + * pl353_nand_driver - This structure defines the NAND subsystem platfor= m driver > + */ > +static struct platform_driver pl353_nand_driver =3D { > + .probe =3D pl353_nand_probe, > + .remove =3D pl353_nand_remove, > + .driver =3D { > + .name =3D PL353_NAND_DRIVER_NAME, > + .of_match_table =3D pl353_nand_of_match, > + }, > +}; > + > +module_platform_driver(pl353_nand_driver); > + > +MODULE_AUTHOR("Xilinx, Inc."); > +MODULE_ALIAS("platform:" PL353_NAND_DRIVER_NAME); > +MODULE_DESCRIPTION("ARM PL353 NAND Flash Driver"); > +MODULE_LICENSE("GPL"); Thanks, Miqu=C3=A8l --=20 Miquel Raynal, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com