Hi Miquel,
Thanks for reviewing the patch.
Please see my comments inline.
-----Original Message-----
From: Miquel Raynal [mailto:miquel.ray...@bootlin.com]
Sent: Tuesday, March 20, 2018 4:08 AM
To: nagasureshkumarre...@gmail.com
Cc: boris.brezil...@bootlin.com; rich...@nod.at; dw...@infradead.org;
computersforpe...@gmail.com; marek.va...@gmail.com;
cyrille.pitc...@wedev4u.fr; linux-...@lists.infradead.org; linux-
ker...@vger.kernel.org; Michal Simek <mich...@xilinx.com>; Punnaiah
Choudary Kalluri <punn...@xilinx.com>; Naga Sureshkumar Relli
<nagas...@xilinx.com>
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,
<nagasureshkumarre...@gmail.com> wrote:
From: Naga Sureshkumar Relli <nagas...@xilinx.com>
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 <nagas...@xilinx.com>
---
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 <punna...@xilinx.com>
+ * Author: nagasuresh <nagas...@xilinx.com>
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://urldefense.proofpoint.com/v2/url?u=https-3A__www.spinics.net_lists_kernel_msg2748832.html&d=DwIGaQ&c=I_0YwoKy7z5LMTVdyO6YCiE2uzI1jjZZuIPelcSjixA&r=8Bziuw3IaCGjyrSAphuGwHmVdHcVwza-srUYwL9U_Ms&m=Qud5yLYSsZO8CluBVnAoEJUNWNJcqCra13xxjmbxyxs&s=8kxJPM5pXwV9aHPGTi534Vr_fwSjFLWJjopTzkQsSfk&e=
https://urldefense.proofpoint.com/v2/url?u=https-3A__www.spinics.net_lists_kernel_msg2748834.html&d=DwIGaQ&c=I_0YwoKy7z5LMTVdyO6YCiE2uzI1jjZZuIPelcSjixA&r=8Bziuw3IaCGjyrSAphuGwHmVdHcVwza-srUYwL9U_Ms&m=Qud5yLYSsZO8CluBVnAoEJUNWNJcqCra13xxjmbxyxs&s=6RXTO4n9L2Frt2oHJ6H1QWvgq057zZWH2W3fsNrW6gc&e=
https://urldefense.proofpoint.com/v2/url?u=https-3A__www.spinics.net_lists_kernel_msg2748840.html&d=DwIGaQ&c=I_0YwoKy7z5LMTVdyO6YCiE2uzI1jjZZuIPelcSjixA&r=8Bziuw3IaCGjyrSAphuGwHmVdHcVwza-srUYwL9U_Ms&m=Qud5yLYSsZO8CluBVnAoEJUNWNJcqCra13xxjmbxyxs&s=hCLzAMnGjHym1NwTglIEhxbypkoDyoHVvLHInmguBVo&e=
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