RE: [LINUX PATCH v8 2/2] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface

From: Naga Sureshkumar Relli
Date: Fri Mar 23 2018 - 10:58:47 EST

Next message: Maurice R Volaski: "Re: Did kernel 3.11 never work?"
Previous message: Greg KH: "Re: [PATCH 3/7] staging: ks7010: Factor out common members in request structs."
In reply to: Miquel Raynal: "Re: [LINUX PATCH v8 2/2] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Miquel,

Thanks for reviewing the patch.
Please see my comments inline.

> -----Original Message-----
> From: Miquel Raynal [mailto:miquel.raynal@xxxxxxxxxxx]
> Sent: Tuesday, March 20, 2018 4:08 AM
> To: nagasureshkumarrelli@xxxxxxxxx
> Cc: boris.brezillon@xxxxxxxxxxx; richard@xxxxxx; dwmw2@xxxxxxxxxxxxx;
> computersforpeace@xxxxxxxxx; marek.vasut@xxxxxxxxx;
> cyrille.pitchen@xxxxxxxxxx; linux-mtd@xxxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; Michal Simek <michals@xxxxxxxxxx>; Punnaiah
> Choudary Kalluri <punnaia@xxxxxxxxxx>; Naga Sureshkumar Relli
> <nagasure@xxxxxxxxxx>
> Subject: Re: [LINUX PATCH v8 2/2] mtd: rawnand: pl353: Add basic driver for
> arm pl353 smc nand interface
>
> Hi Naga,
>
> Thanks for sending a new version supporting ->exec_op(). A few comments
> below.
>
> On Wed, 14 Mar 2018 16:18:25 +0530,
> <nagasureshkumarrelli@xxxxxxxxx> wrote:
>
> > From: Naga Sureshkumar Relli <nagasure@xxxxxxxxxx>
> >
> > 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.
> >
> > Signed-off-by: Naga Sureshkumar Relli <nagasure@xxxxxxxxxx>
> > ---
> > 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
> >
> > 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.
> >
> > +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."?
Are you asking to change the description in help?
>
> > +
> > 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) +=
> hisi504_nand.o
> > obj-$(CONFIG_MTD_NAND_BRCMNAND) += brcmnand/
> > obj-$(CONFIG_MTD_NAND_QCOM) += qcom_nandc.o
> > obj-$(CONFIG_MTD_NAND_MTK) += mtk_ecc.o mtk_nand.o
> > +obj-$(CONFIG_MTD_NAND_PL353) += pl353_nand.o
> >
> > nand-objs := nand_base.o nand_bbt.o nand_timings.o nand_ids.o
> > nand-objs += nand_amd.o diff --git a/drivers/mtd/nand/raw/pl353_nand.c
> > b/drivers/mtd/nand/raw/pl353_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 <punnaiah@xxxxxxxxxx>
> > + * Author: nagasuresh <nagasure@xxxxxxxxxx>
>
> Please use your full names here.
Ok, I will update.
>
> > + *
> > + */
> > +
> > +#include <linux/err.h>
> > +#include <linux/delay.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/ioport.h>
> > +#include <linux/irq.h>
> > +#include <linux/module.h>
> > +#include <linux/moduleparam.h>
> > +#include <linux/mtd/mtd.h>
> > +#include <linux/mtd/rawnand.h>
> > +#include <linux/mtd/nand_ecc.h>
> > +#include <linux/mtd/partitions.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_device.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +#include <linux/platform_data/pl353-smc.h>
> > +
> > +#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?
Let me check and get back to you.
>
> > +
> > +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 = mtd_to_nand(mtd);
> > +
> > + if (section >= chip->ecc.steps)
> > + return -ERANGE;
> > +
> > + oobregion->offset = (section * 16) + 0;
> > + oobregion->length = 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 = mtd_to_nand(mtd);
> > +
> > + if (section >= chip->ecc.steps)
> > + return -ERANGE;
> > +
> > + oobregion->offset = (section * 16) + 8;
> > +
>
> Extra space
I will fix it.
>
> > + oobregion->length = 8;
> > +
> > + return 0;
> > +}
> > +
> > +static const struct mtd_ooblayout_ops pl353_ecc_ooblayout16_ops = {
> > + .ecc = pl353_ecc_ooblayout16_ecc,
> > + .free = 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 = mtd_to_nand(mtd);
> > +
> > + if (section >= chip->ecc.steps)
> > + return -ERANGE;
> > +
> > + oobregion->offset = (section * chip->ecc.bytes) + 52;
> > + oobregion->length = 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 = mtd_to_nand(mtd);
> > +
> > + if (section)
> > + return -ERANGE;
> > + if (section >= 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".
Yes, we can use just 2. I will update
>
> > +
> > + oobregion->offset = (section * chip->ecc.bytes) + 2;
> > +
> > + oobregion->length = 50;
> > +
> > + return 0;
> > +}
> > +
> > +static const struct mtd_ooblayout_ops pl353_ecc_ooblayout64_ops = {
> > + .ecc = pl353_ecc_ooblayout64_ecc,
> > + .free = pl353_ecc_ooblayout64_free,
> > +};
> > +
> > +/* Generic flash bbt decriptors */
> > +static uint8_t bbt_pattern[] = { 'B', 'b', 't', '0' }; static uint8_t
> > +mirror_pattern[] = { '1', 't', 'b', 'B' };
> > +
> > +static struct nand_bbt_descr bbt_main_descr = {
> > + .options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE |
> NAND_BBT_WRITE
> > + | NAND_BBT_2BIT | NAND_BBT_VERSION |
> NAND_BBT_PERCHIP,
> > + .offs = 4,
> > + .len = 4,
> > + .veroffs = 20,
> > + .maxblocks = 4,
> > + .pattern = bbt_pattern
> > +};
> > +
> > +static struct nand_bbt_descr bbt_mirror_descr = {
> > + .options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE |
> NAND_BBT_WRITE
> > + | NAND_BBT_2BIT | NAND_BBT_VERSION |
> NAND_BBT_PERCHIP,
> > + .offs = 4,
> > + .len = 4,
> > + .veroffs = 20,
> > + .maxblocks = 4,
> > + .pattern = 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 = (unsigned long *)in;
> > +
> > + len >>= 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.
At a time, we are reading 4bytes. Hence >> 2.
I didn't get your point.
Are you saying instead of shifting, just use divide by 4?

>
> > + for (i = 0; i < len; i++)
> > + ptr[i] = readl(chip->IO_ADDR_R);
>
> Space
Ok, I will update it
>
> > + return 0;
> > +}
> > +
> > +static void pl353_nand_write_buf_l(struct nand_chip *chip, const uint8_t
> *buf,
> > + int len)
> > +{
> > + int i;
> > + unsigned long *ptr = (unsigned long *)buf;
> > +
> > + for (i = 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().
May be the function name I have written wrongly.
When using writel, it should be nand_write_buf_l.
But the thing is, when using exec_op, core is not calling chip->read_byte(), hence I added
Byte reading.
>
> Same for the next functions. Plus, if you don't use them inside
> ->exec_op() implementation, they have to be removed anyway.
The name of the function should change to buf_l, to do 4byte writes.
The name is creating confusion.
>
> > +}
> > +
> > +/**
> > + * 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 *buf,
> > + int len)
> > +{
> > + int i;
> > + struct nand_chip *chip = mtd_to_nand(mtd);
> > + unsigned long *ptr = (unsigned long *)buf;
> > +
> > + len >>= 2;
> > +
> > + for (i = 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 = 0; i < len; i++)
> > + in[i] = 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?
Miquel, I am not using any variable called "code"
>
> > + *
> > + * 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 = 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?
The pl353 SMC has memory controller driver and this NAND driver is using those APIs.
I sent patches to add the memory controller driver for pl353.
https://www.spinics.net/lists/kernel/msg2748832.html
https://www.spinics.net/lists/kernel/msg2748834.html
https://www.spinics.net/lists/kernel/msg2748840.html

>
> I would rather prefer a readl_relaxed_poll_timeout() or similar, if possible.
Let me check this function to use.
>
> > + 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 = 0; ecc_reg < 4; ecc_reg++) {
> > + /* Read ECC value for each block */
> > + ecc_value = pl353_smc_get_ecc_val(ecc_reg);
>
> I don't have this function neither?
Same as said above.
>
> > + ecc_status = (ecc_value >> 24) & 0xFF;
> > + /* ECC value valid */
> > + if (ecc_status & 0x40) {
> > + for (ecc_byte = 0; ecc_byte < 3; ecc_byte++) {
> > + /* Copy ECC bytes to MTD buffer */
> > + *ecc_code = ~ecc_value & 0xFF;
> > + ecc_value = 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.
Ok, I will check the existing function and will modify.
>
> > + *
> > + * Return: 1 if it is onehot else 0
> > + */
> > +static int onehot(unsigned short value) {
> > + return (value & (value - 1)) == 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 errors.
> > + *
> > + * 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 = (read_ecc[0] | (read_ecc[1] << 8)) & 0xfff;
> > + read_ecc_upper = ((read_ecc[1] >> 4) | (read_ecc[2] << 4)) & 0xfff;
> > +
> > + calc_ecc_lower = (calc_ecc[0] | (calc_ecc[1] << 8)) & 0xfff;
> > + calc_ecc_upper = ((calc_ecc[1] >> 4) | (calc_ecc[2] << 4)) & 0xfff;
> > +
> > + ecc_odd = read_ecc_lower ^ calc_ecc_lower;
> > + ecc_even = read_ecc_upper ^ calc_ecc_upper;
> > +
> > + if ((ecc_odd == 0) && (ecc_even == 0))
> > + return 0; /* no error */
>
> What about:
I think, the below logic is doing that. if (ecc_odd == (~ecc_even & 0xfff))
>
> if (!ecc_odd && !ecc_even)
> return 0;
>
> > +
> > + if (ecc_odd == (~ecc_even & 0xfff)) {
> > + /* bits [11:3] of error code is byte offset */
> > + byte_addr = (ecc_odd >> 3) & 0x1ff;
> > + /* bits [2:0] of error code is bit offset */
> > + bit_addr = ecc_odd & 0x7;
> > + /* Toggling error bit */
> > + buf[byte_addr] ^= (1 << bit_addr);
>
> Use BIT(bit_addr) macro instead?
Ok, I will modify it.
>
> > + return 1;
> > + }
> > +
> > + if (onehot(ecc_odd | ecc_even) == 1)
> > + return 1; /* one error in parity */
>
> Comment should be before the if statement here.
Ok, I will update it.
>
> > +
> > + return -1; /* Uncorrectable error */
>
> And here, before the return statement.
Ok, I will update it.
>
> > +}
> > +
> > +static int pl353_dev_timeout(struct mtd_info *mtd, struct nand_chip
> > +*chip) {
> > + unsigned long timeout = 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 *chip,
> > + int page, int column, int start_cmd, int end_cmd, bool read) {
> > + unsigned long data_phase_addr;
> > + u32 end_cmd_valid = 0;
> > + void __iomem *cmd_addr;
> > + unsigned long cmd_phase_addr = 0, cmd_data = 0;
> > +
> > + struct pl353_nand_info *xnand =
> > + container_of(chip, struct pl353_nand_info, chip);
> > +
> > + if (read)
> > + end_cmd_valid = 1;
> > + else
> > + end_cmd_valid = 0;
>
> This is a good spot to use the ternary operator :)
Yes, we can use. I will add this.
>
> > +
> > + cmd_phase_addr = (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 '|'
You mean, tabbing?
>
> > + (start_cmd << START_CMD_SHIFT));
> > + cmd_addr = (void __iomem * __force)cmd_phase_addr;
> > +
> > + /* Get the data phase address */
> > + data_phase_addr = (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 = (void __iomem * __force)data_phase_addr;
> > + chip->IO_ADDR_W = chip->IO_ADDR_R;
> > + if (chip->options & NAND_BUSWIDTH_16)
> > + column >>= 1;
>
> / 2
>
> > + cmd_data = column;
> > + if (mtd->writesize > PL353_NAND_ECC_SIZE) {
> > + cmd_data |= 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.
I will check and update.
>
> > + pl353_nand_write32(cmd_addr, cmd_data);
> > + cmd_data = (page >> 16);
> > + }
> > + } else {
> > + cmd_data |= page << 8;
> > + }
>
> Space
Ok, I will update.
>
> > + pl353_nand_write32(cmd_addr, cmd_data); }
> > +
> > +/**
> > + * pl353_nand_read_oob - [REPLACEABLE] the most common OOB data read
> function
> > + * @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
> *chip,
> > + int page)
> > +{
> > +
> > + unsigned long data_phase_addr;
> > + uint8_t *p;
> > +
> > + chip->pagebuf = -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
Are you running any script apart from checkpatch?
Any way I will correct it.
>
> > +
> > + ndelay(100);
> > + pl353_dev_timeout(mtd, chip);
> > +
> > + p = chip->oob_poi;
> > + pl353_nand_read_buf_l(chip, p,
> > + (mtd->oobsize - PL353_NAND_LAST_TRANSFER_LENGTH));
>
> Alignment
Ok, I will correct it.
>
> > + p += (mtd->oobsize - PL353_NAND_LAST_TRANSFER_LENGTH);
> > +
> > + data_phase_addr = (unsigned long __force)chip->IO_ADDR_R;
> > + data_phase_addr |= PL353_NAND_CLEAR_CS;
> > + chip->IO_ADDR_R = (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 function
> > + * @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!
Ok, I will add.
>
> > +static int pl353_nand_write_oob(struct mtd_info *mtd, struct nand_chip
> *chip,
> > + int page)
> > +{
> > +
>
> Extra space
Any way I will correct it.
>
> > + const uint8_t *buf = chip->oob_poi;
> > + unsigned long data_phase_addr;
> > + struct pl353_nand_info *xnand =
> > + container_of(chip, struct pl353_nand_info, chip);
> > + u32 addrcycles = 0, ret;
> > + unsigned long timeout = jiffies + PL353_NAND_DEV_BUSY_TIMEOUT;
> > + u8 status;
> > +
> > + chip->pagebuf = -1;
> > + addrcycles = 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 += (mtd->oobsize - PL353_NAND_LAST_TRANSFER_LENGTH);
> > +
> > + data_phase_addr = (unsigned long __force)chip->IO_ADDR_W;
> > + data_phase_addr |= PL353_NAND_CLEAR_CS;
> > + data_phase_addr |= (1 << END_CMD_VALID_SHIFT);
> > + chip->IO_ADDR_W = (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 = nand_status_op(chip, &status);
>
> Doing this without exiting the status state is broken.
I didn't get.
>
> > + timeout = 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.
Hmm, you are correct, I will remove this code.
>
> > + if (status & NAND_STATUS_READY)
>
> You don't update "status", while you wait for it to change.
Basically I am waiting for device ready, but not updating the status.
I will look this implementation and will get back to you.
>
> > + 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.
Let me use this, thanks for pointing.
>
> > +
> > + 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?
Yes, when using on-die ecc, this function is getting called.
i.e. nand_micron.c is calling nand_set_features_op, with DATA_OUT_INSTR, and there
we are using this.
>
> Not sure this is needed.
>
> > +{
> > + unsigned long data_phase_addr;
> > + uint8_t *p;
> > +
> > + pl353_nand_read_buf_l(chip, buf, mtd->writesize);
> > + p = chip->oob_poi;
> > + pl353_nand_read_buf_l(chip, p,
> > + (mtd->oobsize - PL353_NAND_LAST_TRANSFER_LENGTH));
> > + p += (mtd->oobsize - PL353_NAND_LAST_TRANSFER_LENGTH);
> > +
> > + data_phase_addr = (unsigned long __force)chip->IO_ADDR_R;
> > + data_phase_addr |= PL353_NAND_CLEAR_CS;
> > + chip->IO_ADDR_R = (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 = chip->oob_poi;
> > + pl353_nand_write_buf(mtd, p,
> > + (mtd->oobsize -
> PL353_NAND_LAST_TRANSFER_LENGTH));
> > + p += (mtd->oobsize - PL353_NAND_LAST_TRANSFER_LENGTH);
> > +
> > + data_phase_addr = (unsigned long __force)chip->IO_ADDR_W;
> > + data_phase_addr |= PL353_NAND_CLEAR_CS;
> > + data_phase_addr |= (1 << END_CMD_VALID_SHIFT);
> > + chip->IO_ADDR_W = (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 the
> 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 = chip->ecc.size;
> > + int eccsteps = chip->ecc.steps;
> > + uint8_t *ecc_calc = chip->ecc.calc_buf;
> > + const uint8_t *p = 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 += eccsize;
> > + }
> > + pl353_nand_write_buf(mtd, p,
> > + (eccsize - PL353_NAND_LAST_TRANSFER_LENGTH));
> > + p += (eccsize - PL353_NAND_LAST_TRANSFER_LENGTH);
> > +
> > + /* Set ECC Last bit to 1 */
> > + data_phase_addr = (unsigned long __force)chip->IO_ADDR_W;
> > + data_phase_addr |= PL353_NAND_ECC_LAST;
> > + chip->IO_ADDR_W = (void __iomem * __force)data_phase_addr;
> > + pl353_nand_write_buf(mtd, p,
> PL353_NAND_LAST_TRANSFER_LENGTH);
> > +
> > + p = buf;
> > + chip->ecc.calculate(mtd, p, &ecc_calc[0]);
> > +
> > + /* Wait for ECC to be calculated and read the error values */
> > + ret = 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 = (unsigned long __force)chip->IO_ADDR_W;
> > + data_phase_addr &= ~PL353_NAND_ECC_LAST;
> > + chip->IO_ADDR_W = (void __iomem * __force)data_phase_addr;
> > +
> > + /* Write the spare area with ECC bytes */
> > + oob_ptr = chip->oob_poi;
> > + pl353_nand_write_buf(mtd, oob_ptr,
> > + (mtd->oobsize -
> PL353_NAND_LAST_TRANSFER_LENGTH));
> > +
> > + data_phase_addr = (unsigned long __force)chip->IO_ADDR_W;
> > + data_phase_addr |= PL353_NAND_CLEAR_CS;
> > + data_phase_addr |= (1 << END_CMD_VALID_SHIFT);
> > + chip->IO_ADDR_W = (void __iomem * __force)data_phase_addr;
> > + oob_ptr += (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().
Yes, I will update this.
> > +
> > + /*
> > + * Apply this short delay always to ensure that we do wait tWB in any
> > + * case on any machine.
> > + */
> > + ndelay(100);
> > + ret = nand_status_op(chip, &status);
> > + timeo = 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().
Ok
>
> > + return (status & NAND_STATUS_FAIL) ? -EIO : 0; }
> > +
> > +/**
> > + * pl353_nand_write_page_swecc - [REPLACEABLE] software 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
> > + *
> > + * 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.
Ok, I will remove it in next version.
>
> > + int i, eccsize = chip->ecc.size;
> > + int eccbytes = chip->ecc.bytes;
> > + int eccsteps = chip->ecc.steps;
> > + uint8_t *ecc_calc = chip->ecc.calc_buf;
> > + const uint8_t *p = buf;
> > + u32 ret;
> > +
> > + for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize)
> > + chip->ecc.calculate(mtd, p, &ecc_calc[0]);
> > +
> > + ret = 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 = chip->ecc.size;
> > + int eccbytes = chip->ecc.bytes;
> > + int eccsteps = chip->ecc.steps;
> > + uint8_t *p = buf;
> > + uint8_t *ecc_calc = chip->ecc.calc_buf;
> > + uint8_t *ecc_code = chip->ecc.code_buf;
> > +
> > + uint8_t *oob_ptr;
> > + u32 ret;
> > + unsigned long data_phase_addr;
> > + unsigned long timeout = 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 += eccsize;
> > + }
> > + pl353_nand_read_buf_l(chip, p,
> > + (eccsize - PL353_NAND_LAST_TRANSFER_LENGTH));
> > + p += (eccsize - PL353_NAND_LAST_TRANSFER_LENGTH);
> > +
> > + /* Set ECC Last bit to 1 */
> > + data_phase_addr = (unsigned long __force)chip->IO_ADDR_R;
> > + data_phase_addr |= PL353_NAND_ECC_LAST;
> > + chip->IO_ADDR_R = (void __iomem * __force)data_phase_addr;
> > + pl353_nand_read_buf_l(chip, p,
> PL353_NAND_LAST_TRANSFER_LENGTH);
> > +
> > + /* Read the calculated ECC value */
> > + p = buf;
> > + chip->ecc.calculate(mtd, p, &ecc_calc[0]);
> > +
> > + /* Clear ECC last bit */
> > + data_phase_addr = (unsigned long __force)chip->IO_ADDR_R;
> > + data_phase_addr &= ~PL353_NAND_ECC_LAST;
> > + chip->IO_ADDR_R = (void __iomem * __force)data_phase_addr;
> > +
> > + /* Read the stored ECC value */
> > + oob_ptr = 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 = (unsigned long __force)chip->IO_ADDR_R;
> > + data_phase_addr |= PL353_NAND_CLEAR_CS;
> > + chip->IO_ADDR_R = (void __iomem * __force)data_phase_addr;
> > +
> > + oob_ptr += (mtd->oobsize - PL353_NAND_LAST_TRANSFER_LENGTH);
> > + pl353_nand_read_buf_l(chip, oob_ptr,
> > +PL353_NAND_LAST_TRANSFER_LENGTH);
> > +
> > + ret = mtd_ooblayout_get_eccbytes(mtd, ecc_code, chip->oob_poi, 0,
> > + chip->ecc.total);
> > + if (ret)
> > + return ret;
> > +
> > + eccsteps = chip->ecc.steps;
> > + p = buf;
> > +
> > + /* Check ECC error for all blocks and correct if it is correctable */
> > + for (i = 0 ; eccsteps; eccsteps--, i += eccbytes, p += eccsize) {
>
> In this loop you should monitore a max_bitflips value and return it instead of
> zero at the end of the function.
Ok, I will add it.
>
> > + stat = chip->ecc.correct(mtd, p, &ecc_code[i], &ecc_calc[i]);
> > + if (stat < 0)
> > + mtd->ecc_stats.failed++;
> > + else
> > + mtd->ecc_stats.corrected += stat;
> > + }
> > + return 0;
> > +}
> > +
> > +/**
> > + * pl353_nand_read_page_swecc - [REPLACEABLE] software 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
> > + *
> > + * 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.
Yes, I will remove in next version.
>
> > + int i, eccsize = chip->ecc.size;
> > + int eccbytes = chip->ecc.bytes;
> > + int eccsteps = chip->ecc.steps;
> > + uint8_t *p = buf;
> > + uint8_t *ecc_calc = chip->ecc.calc_buf;
> > + uint8_t *ecc_code = chip->ecc.code_buf;
> > + u32 ret;
> > +
> > + chip->ecc.read_page_raw(mtd, chip, buf, page, 1);
> > +
> > + for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize)
> > + chip->ecc.calculate(mtd, p, &ecc_calc[i]);
> > +
> > + ret = mtd_ooblayout_get_eccbytes(mtd, ecc_calc, chip->oob_poi,
> > + 0, chip->ecc.total);
> > +
> > + eccsteps = chip->ecc.steps;
> > + p = buf;
> > +
> > + for (i = 0 ; eccsteps; eccsteps--, i += eccbytes, p += eccsize) {
> > + int stat;
> > +
> > + stat = chip->ecc.correct(mtd, p, &ecc_code[i], &ecc_calc[i]);
> > + if (stat < 0)
> > + mtd->ecc_stats.failed++;
> > + else
> > + mtd->ecc_stats.corrected += 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 = NULL;
> > + unsigned int op_id, offset, naddrs;
> > + int i;
> > + const u8 *addrs;
> > +
> > + memset(nfc_op, 0, sizeof(struct pl353_nfc_op));
> > + for (op_id = 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?
I think, breaking switch case only not for loop.
>
> > + nfc_op->len = nand_subop_get_data_len(subop, op_id);
> > +
> > + instr = &subop->instrs[op_id];
> > + if (subop->ninstrs == 1)
> > + nfc_op->cmnds[0] = -1;
> > + switch (instr->type) {
> > + case NAND_OP_CMD_INSTR:
> > + nfc_op->type = NAND_OP_CMD_INSTR;
> > + nfc_op->end_cmd = op_id - 1;
> > + if (op_id)
>
> You should put { } on the if also if the else statement needs braces.
Ok, but I didn't see any warning from checkpatch.
>
> > + nfc_op->cmnds[1] = instr->ctx.cmd.opcode;
> > + else {
> > + nfc_op->cmnds[0] = instr->ctx.cmd.opcode;
> > + nfc_op->cmnds[1] = -1;
> > + }
> > + break;
> > +
> > + case NAND_OP_ADDR_INSTR:
> > + offset = nand_subop_get_addr_start_off(subop, op_id);
> > + naddrs = nand_subop_get_num_addr_cyc(subop,
> op_id);
> > + addrs = &instr->ctx.addr.addrs[offset];
> > + nfc_op->addrs = instr->ctx.addr.addrs[offset];
> > + for (i = 0; i < min_t(unsigned int, 4, naddrs); i++)
> > + nfc_op->addrs |= instr->ctx.addr.addrs[i] <<
> > + (8 * i);
> > +
> > + if (naddrs >= 5) {
> > + nfc_op->addrs >>= 16;
> > + nfc_op->addrs |= (addrs[4] << 16);
> > + nfc_op->thirdrow = 1;
> > + }
> > + nfc_op->naddrs = nand_subop_get_num_addr_cyc
> > + (subop, op_id);
>
> Don't put the parameters of a function on the next line like that?
Ok, I will update it.
>
> > + break;
> > +
> > + case NAND_OP_DATA_IN_INSTR:
> > + nfc_op->data_instr = instr;
> > + nfc_op->type = NAND_OP_DATA_IN_INSTR;
> > + nfc_op->data_instr_idx = op_id;
> > + break;
> > +
> > + case NAND_OP_DATA_OUT_INSTR:
> > + nfc_op->data_instr = instr;
> > + nfc_op->type = NAND_OP_DATA_IN_INSTR;
> DATA_OUT_INSTR?
Yes, I will correct it.
But I am using instr->type in cmd_function.
I will update it.
>
> Is this really tested?
Yes, During on-die ECC test.
When nand_micron is asking to set some features, this will execute.
>
> > + nfc_op->data_instr_idx = op_id;
> > + break;
> > +
> > + case NAND_OP_WAITRDY_INSTR:
> > + nfc_op->rdy_timeout_ms = instr-
> >ctx.waitrdy.timeout_ms;
> > + nfc_op->rdy_delay_ns = instr->delay_ns;
> > + nfc_op->wait = 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 = nand_to_mtd(chip);
> > + const struct nand_op_instr *instr;
> > + struct pl353_nfc_op nfc_op;
>
> nfc_op = {}; to initialize to 0.
Ok, I will update it.
>
> > + struct pl353_nand_info *xnand =
> > + container_of(chip, struct pl353_nand_info, chip);
> > + void __iomem *cmd_addr;
> > + unsigned long cmd_data = 0, end_cmd_valid = 0;
> > + unsigned long cmd_phase_addr, data_phase_addr, end_cmd;
> > + unsigned long timeout = jiffies + PL353_NAND_DEV_BUSY_TIMEOUT;
> > + u32 addrcycles = 0;
> > + unsigned int op_id, len, offset;
> > +
> > + pl353_nfc_parse_instructions(chip, subop, &nfc_op);
> > + instr = nfc_op.data_instr;
> > + op_id = nfc_op.data_instr_idx;
> > + len = nand_subop_get_data_len(subop, op_id);
> > + offset = nand_subop_get_data_start_off(subop, op_id);
> > +
> > + if (nfc_op.cmnds[0] != -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 == nfc_op.cmnds[0]) {
> > + xnand->end_cmd = 0;
> > + xnand->end_cmd_pending = 0;
> > + return 0;
> > + }
> > + }
> > +
> > + /* Clear interrupt */
> > + pl353_smc_clr_nand_int();
> > + end_cmd_valid = 0;
>
> Space
Ok, I will correct it.
>
> > + /* Get the command phase address */
> > + if (nfc_op.cmnds[1] != -1) {
> > + end_cmd_valid = 1;
> > + } else {
> > + if (nfc_op.cmnds[0] == NAND_CMD_READ0)
> > + return 0;
> > + }
> > + if (nfc_op.end_cmd == NAND_CMD_NONE)
> > + end_cmd = 0x0;
> > + else
> > + end_cmd = nfc_op.cmnds[1];
> > +
> > + addrcycles = nfc_op.naddrs;
> > + if (nfc_op.cmnds[0] == NAND_CMD_READ0 ||
> > + nfc_op.cmnds[0] == NAND_CMD_SEQIN)
> > + addrcycles = xnand->row_addr_cycles +
> > + xnand->col_addr_cycles;
> > + else if ((nfc_op.cmnds[0] == NAND_CMD_ERASE1) ||
> > + (nfc_op.cmnds[0] == NAND_CMD_ERASE2))
> > + addrcycles = xnand->row_addr_cycles;
> > + else
> > + addrcycles = nfc_op.naddrs;
>
> A switch block would probably be appropriate.
Yes we can use.
>
> > + cmd_phase_addr = (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 = (void __iomem * __force)cmd_phase_addr;
>
> Space
Ok, I will correc it.
>
> > + /* Get the data phase address */
> > + end_cmd_valid = 0;
> > +
> > + data_phase_addr = (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 = (void __iomem *
> __force)data_phase_addr;
>
> Do you really need this "__force" ?
Let me check and get back to you on this.
>
> > + chip->IO_ADDR_W = chip->IO_ADDR_R;
> > + /* Command phase AXI write */
> > + /* Read & Write */
> > + if (nfc_op.thirdrow) {
> > + nfc_op.thirdrow = 0;
> > + if (mtd->writesize > PL353_NAND_ECC_SIZE) {
> > + cmd_data |= nfc_op.addrs << 16;
> > + /* Another address cycle for devices > 128MiB
> */
> > + if (chip->chipsize > (128 << 20)) {
> > + pl353_nand_write32(cmd_addr,
> cmd_data);
> > + cmd_data = (nfc_op.addrs >> 16);
> > + }
> > + }
> > + } else {
> > + if (nfc_op.addrs != -1) {
> > + int column = 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] == NAND_CMD_READ0) ||
> > + (nfc_op.cmnds[0] == NAND_CMD_SEQIN) ||
> > + (nfc_op.cmnds[0] == NAND_CMD_RNDOUT) ||
> > + (nfc_op.cmnds[0] == NAND_CMD_RNDIN))) {
>
> Alignment.
Ok, I will update it.
>
> > + column >>= 1;
> > + }
> > + cmd_data = nfc_op.addrs;
> > + }
> > + }
> > + pl353_nand_write32(cmd_addr, cmd_data);
> > + if (nfc_op.type != 0) {
> > + xnand->end_cmd = nfc_op.end_cmd;
> > + xnand->end_cmd_pending = 1;
> > + }
> > + ndelay(100);
>
> Why?
I found some errors during on_die testing, hence I added this delay.
Let me check once
>
> > + if (nfc_op.cmnds[0] == 0xef)
> > + nfc_op.wait = false;
> > + if (nfc_op.wait) {
> > + nfc_op.wait = false;
>
> You can remove this line.
Yes, it is initializing to zero in cmd_function.
>
> > + 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.
Ok, I will update it.
>
> > + return 0;
> > + }
> > + }
> > +
> > + if (instr == NULL)
>
> if (!instr)
I will modify it.
>
> > + return 0;
> > + if (instr->type == NAND_OP_DATA_IN_INSTR)
> > + return pl353_nand_read_buf(chip, instr->ctx.data.buf.in, len);
> > +
> > + if (instr->type == NAND_OP_DATA_OUT_INSTR) {
> > + if ((nfc_op.cmnds[0] == NAND_CMD_PAGEPROG) ||
> > + (nfc_op.cmnds[0] == 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 =
> NAND_OP_PARSER(
> > + 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.
Can you explain little bit? I didn't get.
>
> > + );
> > +
> > +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.
Added links above for this driver.
>
> > + pl353_smc_clr_nand_int();
> > + return 1;
> > + }
> > + return 0;
> > +}
> > +
> > +/**
> > + * pl353_nand_ecc_init - Initialize the ecc information as per the ecc mode
> > + * @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
> > +per the
> > + * ecc mode
> > + */
> > +static void pl353_nand_ecc_init(struct mtd_info *mtd, struct nand_ecc_ctrl
> *ecc,
> > + int ecc_mode)
> > +{
> > + struct nand_chip *chip = mtd_to_nand(mtd);
> > +
> > + ecc->read_oob = pl353_nand_read_oob;
> > + ecc->read_page_raw = pl353_nand_read_page_raw;
> > + ecc->write_oob = pl353_nand_write_oob;
> > + ecc->write_page_raw = pl353_nand_write_page_raw;
> > +
> > + if (ecc_mode == 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 = pl353_nand_read_page_raw;
> > + ecc->write_page = 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 = &bbt_main_descr;
> > + chip->bbt_md = &bbt_mirror_descr;
> > + } else {
> > + ecc->mode = NAND_ECC_HW;
> > + /* Hardware ECC generates 3 bytes ECC code for each 512
> bytes */
> > + ecc->bytes = 3;
> > + ecc->strength = 1;
> > + ecc->calculate = pl353_nand_calculate_hwecc;
> > + ecc->correct = pl353_nand_correct_data;
> > + ecc->hwctl = NULL;
>
> You can drop this.
Ok, I will update it.
>
> > + ecc->read_page = pl353_nand_read_page_hwecc;
> > + ecc->size = PL353_NAND_ECC_SIZE;
> > + ecc->write_page = 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.
Ok, I will update it.
Thanks again.
If any of the above comments given by you, are not addressed properly,
Please let me know.

Thanks,
Naga Sureshkumar Reli.
>
> > +
> pl353_smc_set_ecc_mode(PL353_SMC_ECCMODE_APB);
> > + break;
> > + default:
> > + /*
> > + * The software ECC routines won't work with the
> > + * SMC controller
> > + */
> > + ecc->calculate = nand_calculate_ecc;
> > + ecc->correct = nand_correct_data;
> > + ecc->read_page = pl353_nand_read_page_swecc;
> > + ecc->write_page = pl353_nand_write_page_swecc;
> > + ecc->size = 256;
> > + break;
> > + }
> > +
> > + if (mtd->oobsize == 16)
> > + mtd_set_ooblayout(mtd,
> &pl353_ecc_ooblayout16_ops);
> > + else if (mtd->oobsize == 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 = devm_kzalloc(&pdev->dev, sizeof(*xnand), GFP_KERNEL);
> > + if (!xnand)
> > + return -ENOMEM;
> > +
> > + /* Map physical address of NAND flash */
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + xnand->nand_base = devm_ioremap_resource(&pdev->dev, res);
> > + if (IS_ERR(xnand->nand_base))
> > + return PTR_ERR(xnand->nand_base);
> > +
> > + nand_chip = &xnand->chip;
> > + mtd = nand_to_mtd(nand_chip);
> > + nand_chip->exec_op = pl353_nfc_exec_op;
> > + nand_set_controller_data(nand_chip, xnand);
> > + mtd->priv = nand_chip;
> > + mtd->owner = THIS_MODULE;
> > + mtd->name = 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 = xnand->nand_base;
> > + nand_chip->IO_ADDR_W = xnand->nand_base;
> > + /* Set the driver entry points for MTD */
> > + nand_chip->dev_ready = pl353_nand_device_ready;
> > + nand_chip->select_chip = pl353_nand_select_chip;
> > + /* If we don't set this delay driver sets 20us by default */
> > + nand_chip->chip_delay = 30;
> > +
> > + /* Set the device option and flash width */
> > + nand_chip->options = NAND_BUSWIDTH_AUTO;
> > + nand_chip->bbt_options = 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 = nand_chip->onfi_params.addr_cycles & 0xF;
> > + xnand->col_addr_cycles =
> > + (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
> > +frees 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 = platform_get_drvdata(pdev);
> > + struct mtd_info *mtd = 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[] = {
> > + { .compatible = "arm,pl353-nand-r2p1" },
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, pl353_nand_of_match);
> > +
> > +/*
> > + * pl353_nand_driver - This structure defines the NAND subsystem
> > +platform driver */ static struct platform_driver pl353_nand_driver =
> > +{
> > + .probe = pl353_nand_probe,
> > + .remove = pl353_nand_remove,
> > + .driver = {
> > + .name = PL353_NAND_DRIVER_NAME,
> > + .of_match_table = 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Ãl
>
> --
> Miquel Raynal, Bootlin (formerly Free Electrons) Embedded Linux and Kernel
> engineering https://bootlin.com

Next message: Maurice R Volaski: "Re: Did kernel 3.11 never work?"
Previous message: Greg KH: "Re: [PATCH 3/7] staging: ks7010: Factor out common members in request structs."
In reply to: Miquel Raynal: "Re: [LINUX PATCH v8 2/2] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]