[LINUX PATCH v14] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface

[LINUX PATCH v14] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface

[Naga Sureshkumar Relli]

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 <naga.sureshkumar.relli@xilinx.com>
---
xilinx zynq TRM link:
https://www.xilinx.com/support/documentation/user_guides/ug585-Zynq-7000-TRM.pdf

ARM pl353 smc TRM link:
http://infocenter.arm.com/help/topic/com.arm.doc.ddi0380g/DDI0380G_smc_pl350_series_r2p1_trm.pdf

Tested Micron MT29F2G08ABAEAWP (On-die capable) and AMD/Spansion S34ML01G1.

SMC memory controller driver is at drivers/memory/pl353-smc.c

Changes in v14:
 - Removed legacy hooks as per Miquel comments
Changes in v13:
 - Rebased the driver to mtd/next
Changes in v12:
 - Rebased the driver on top of v4.19 nand tree
 - Removed nand_scan_ident() and nand_scan_tail(), and added nand_controller_ops
   with ->attach_chip() and used nand_scan() instead.
 - Renamed pl353_nand_info structure to pl353_nand_controller
 - Renamed nand_base and nandaddr in pl353_nand_controller to 'regs' and 'buf_addr'
 - Added new API pl353_wait_for_ecc_done() to wait for ecc done and call it from
   pl353_nand_write_page_hwecc() and pl353_nand_read_page_hwecc()
 - Defined new macro for max ECC blocks
 - Added return value check for ecc.calculate()
 - Renamed pl353_nand_cmd_function() to pl353_nand_exec_op_cmd()
 - Added x16 bus-width support
 - The dependent driver pl353-smc is already reviewed and hence dropped the
   smc driver
Changes in v11:
 - Removed Documentation patch and added the required info in driver as
   per Boris comments.
 - Removed unwanted variables from pl353_nand_info as per Miquel comments
 - Removed IO_ADDR_R/W.
 - Replaced onhot() with hweight32()
 - Defined macros for static values in function pl353_nand_correct_data()
 - Removed all unnecessary delays
 - Used nand_wait_ready() where ever is required
 - Modifed the pl353_setup_data_interface() logic as per Miquel comments.
 - Taken array instead of 7 values in pl353_setup_data_interface() and pass
   it to smc driver.
 - Added check to collect the return value of mtd_device_register().
Changes in 10:
 - Typos correction like nand to NAND and soc to SOC etc..
 - Defined macros for the values in pl353_nand_calculate_hwecc()
 - Modifed ecc_status from int to char in pl353_nand_calculate_hwecc()
 - Changed the return type form int to bool to the function
   onehot()
 - Removed udelay(1000) in pl353_cmd_function, as it is not required
 - Dropped ecc->hwctl = NULL in pl353_ecc_init()
 - Added an error message in pl353_ecc_init(), when there is no matching
   oobsize
 - Changed the variable from xnand to xnfc
 - Added logic to get mtd->name from DT, if it is specified in DT
Changes in v9:
 - Addressed the below comments given by Miquel
 - instead of using pl353_nand_write32, use directly writel_relaxed
 - Fixed check patch warnings
 - Renamed write_buf/read_buf to write_data_op/read_data_op
 - use BIT macro instead of 1 << nr
 - Use NAND_ROW_ADDR_3 flag
 - Use nand_wait_ready()
 - Removed swecc functions
 - Use address cycles as per size, instead of reading it from Parameter page
 - Instead of writing too many patterns, use optional property
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
---
 drivers/mtd/nand/raw/Kconfig      |    7 +
 drivers/mtd/nand/raw/Makefile     |    1 +
 drivers/mtd/nand/raw/pl353_nand.c | 1399 +++++++++++++++++++++++++++++++++++++
 3 files changed, 1407 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 e604625..d0f8947 100644
--- a/drivers/mtd/nand/raw/Kconfig
+++ b/drivers/mtd/nand/raw/Kconfig
@@ -558,4 +558,11 @@ config MTD_NAND_MESON
 	  Enables support for NAND controller on Amlogic's Meson SoCs.
 	  This controller is found on Meson SoCs.
 
+config MTD_NAND_PL353
+	tristate "ARM Pl353 NAND flash driver"
+	depends on MTD_NAND && ARM
+	depends on PL353_SMC
+	help
+	  Enables support for PrimeCell Static Memory Controller PL353
+
 endif # MTD_NAND
diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile
index 5a5a72f..a26a484 100644
--- a/drivers/mtd/nand/raw/Makefile
+++ b/drivers/mtd/nand/raw/Makefile
@@ -58,6 +58,7 @@ obj-$(CONFIG_MTD_NAND_MTK)		+= mtk_ecc.o mtk_nand.o
 obj-$(CONFIG_MTD_NAND_TEGRA)		+= tegra_nand.o
 obj-$(CONFIG_MTD_NAND_STM32_FMC2)	+= stm32_fmc2_nand.o
 obj-$(CONFIG_MTD_NAND_MESON)		+= meson_nand.o
+obj-$(CONFIG_MTD_NAND_PL353)		+= pl353_nand.o
 
 nand-objs := nand_base.o nand_legacy.o nand_bbt.o nand_timings.o nand_ids.o
 nand-objs += nand_onfi.o
diff --git a/drivers/mtd/nand/raw/pl353_nand.c b/drivers/mtd/nand/raw/pl353_nand.c
new file mode 100644
index 0000000..eb63778
--- /dev/null
+++ b/drivers/mtd/nand/raw/pl353_nand.c
@@ -0,0 +1,1399 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ARM PL353 NAND flash controller driver
+ *
+ * Copyright (C) 2017 Xilinx, Inc
+ * Author: Punnaiah chowdary kalluri <punnaiah@xilinx.com>
+ * Author: Naga Sureshkumar Relli <nagasure@xilinx.com>
+ *
+ */
+
+#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/pl353-smc.h>
+#include <linux/clk.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 PL353_NAND_ECC_BUSY_TIMEOUT	(1 * HZ)
+#define PL353_NAND_DEV_BUSY_TIMEOUT	(1 * HZ)
+#define PL353_NAND_LAST_TRANSFER_LENGTH	4
+#define PL353_NAND_ECC_VALID_SHIFT	24
+#define PL353_NAND_ECC_VALID_MASK	0x40
+#define PL353_ECC_BITS_BYTEOFF_MASK	0x1FF
+#define PL353_ECC_BITS_BITOFF_MASK	0x7
+#define PL353_ECC_BIT_MASK		0xFFF
+#define PL353_TREA_MAX_VALUE		1
+#define PL353_MAX_ECC_CHUNKS		4
+#define PL353_MAX_ECC_BYTES		3
+
+struct pl353_nfc_op {
+	u32 cmnds[4];
+	u32 end_cmd;
+	u32 addrs;
+	u32 naddrs;
+	u32 addr5;
+	u32 addr6;
+	unsigned int data_instr_idx;
+	unsigned int rdy_timeout_ms;
+	unsigned int rdy_delay_ns;
+	unsigned int cle_ale_delay_ns;
+	const struct nand_op_instr *data_instr;
+};
+
+/**
+ * struct pl353_nand_controller - Defines the NAND flash controller driver
+ *				  instance
+ * @chip:		NAND chip information structure
+ * @dev:		Parent device (used to print error messages)
+ * @regs:		Virtual address of the NAND flash device
+ * @buf_addr:		Virtual address of the NAND flash device for
+ *			data read/writes
+ * @addr_cycles:	Address cycles
+ * @mclk:		Memory controller clock
+ * @buswidth:		Bus width 8 or 16
+ */
+struct pl353_nand_controller {
+	struct nand_controller controller;
+	struct nand_chip chip;
+	struct device *dev;
+	void __iomem *regs;
+	void __iomem *buf_addr;
+	u8 addr_cycles;
+	struct clk *mclk;
+	u32 buswidth;
+};
+
+static inline struct pl353_nand_controller *
+			to_pl353_nand(struct nand_chip *chip)
+{
+	return container_of(chip, struct pl353_nand_controller, chip);
+}
+
+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 * chip->ecc.bytes);
+	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 * chip->ecc.bytes) + 8;
+	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;
+
+	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 u8 bbt_pattern[] = { 'B', 'b', 't', '0' };
+static u8 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
+};
+
+static void pl353_nfc_force_byte_access(struct nand_chip *chip,
+					bool force_8bit)
+{
+	int ret;
+	struct pl353_nand_controller *xnfc =
+		container_of(chip, struct pl353_nand_controller, chip);
+
+	if (xnfc->buswidth == 8)
+		return;
+
+	if (force_8bit)
+		ret = pl353_smc_set_buswidth(PL353_SMC_MEM_WIDTH_8);
+	else
+		ret = pl353_smc_set_buswidth(PL353_SMC_MEM_WIDTH_16);
+
+	if (ret)
+		dev_err(xnfc->dev, "Error in Buswidth\n");
+}
+
+static inline int pl353_wait_for_dev_ready(struct nand_chip *chip)
+{
+	unsigned long timeout = jiffies + PL353_NAND_DEV_BUSY_TIMEOUT;
+
+	do {
+		if (pl353_smc_get_nand_int_status_raw()) {
+			pl353_smc_clr_nand_int();
+			break;
+
+		cpu_relax();
+	} while (!time_after_eq(jiffies, timeout));
+
+	if (time_after_eq(jiffies, timeout)) {
+		pr_err("%s timed out\n", __func__);
+		return -ETIMEDOUT;
+	}
+
+	return 0;
+}
+
+/**
+ * pl353_nand_read_data_op - 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
+ * @force_8bit:	Force 8-bit bus access
+ * Return:	Always return zero
+ */
+static void pl353_nand_read_data_op(struct nand_chip *chip, u8 *in,
+				    unsigned int len, bool force_8bit)
+{
+	int i;
+	struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
+
+	if (force_8bit)
+		pl353_nfc_force_byte_access(chip, true);
+
+	if ((IS_ALIGNED((uint32_t)in, sizeof(uint32_t)) &&
+	     IS_ALIGNED(len, sizeof(uint32_t))) || !force_8bit) {
+		u32 *ptr = (u32 *)in;
+
+		len /= 4;
+		for (i = 0; i < len; i++)
+			ptr[i] = readl(xnfc->buf_addr);
+	} else {
+		for (i = 0; i < len; i++)
+			in[i] = readb(xnfc->buf_addr);
+	}
+
+	if (force_8bit)
+		pl353_nfc_force_byte_access(chip, false);
+}
+
+/**
+ * pl353_nand_write_buf - write buffer to chip
+ * @mtd:	Pointer to the mtd info structure
+ * @buf:	Pointer to the buffer to store write data
+ * @len:	Number of bytes to write
+ * @force_8bit:	Force 8-bit bus access
+ */
+static void pl353_nand_write_data_op(struct nand_chip *chip, const u8 *buf,
+				     int len, bool force_8bit)
+{
+	int i;
+	struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
+
+	if (force_8bit)
+		pl353_nfc_force_byte_access(chip, true);
+
+	if ((IS_ALIGNED((uint32_t)buf, sizeof(uint32_t)) &&
+	     IS_ALIGNED(len, sizeof(uint32_t))) || !force_8bit) {
+		u32 *ptr = (u32 *)buf;
+
+		len /= 4;
+		for (i = 0; i < len; i++)
+			writel(ptr[i], xnfc->buf_addr);
+	} else {
+		for (i = 0; i < len; i++)
+			writeb(buf[i], xnfc->buf_addr);
+	}
+
+	if (force_8bit)
+		pl353_nfc_force_byte_access(chip, false);
+}
+
+static inline int pl353_wait_for_ecc_done(void)
+{
+	unsigned long timeout = jiffies + PL353_NAND_ECC_BUSY_TIMEOUT;
+
+	do {
+		if (pl353_smc_ecc_is_busy())
+			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;
+	}
+
+	return 0;
+}
+
+/**
+ * pl353_nand_calculate_hwecc - Calculate Hardware ECC
+ * @mtd:	Pointer to the mtd_info structure
+ * @data:	Pointer to the page data
+ * @ecc:	Pointer to the ECC buffer where ECC data needs to be stored
+ *
+ * This function retrieves the Hardware ECC data from the controller and returns
+ * ECC data back to the MTD subsystem.
+ * It operates on a number of 512 byte blocks of NAND memory and can be
+ * programmed to store the ECC codes after the data in memory. For writes,
+ * the ECC is written to the spare area of the page. For reads, the result of
+ * a block ECC check are made available to the device driver.
+ *
+ * ------------------------------------------------------------------------
+ * |               n * 512 blocks                  | extra  | ecc    |     |
+ * |                                               | block  | codes  |     |
+ * ------------------------------------------------------------------------
+ *
+ * The ECC calculation uses a simple Hamming code, using 1-bit correction 2-bit
+ * detection. It starts when a valid read or write command with a 512 byte
+ * aligned address is detected on the memory interface.
+ *
+ * Return:	0 on success or error value on failure
+ */
+static int pl353_nand_calculate_hwecc(struct nand_chip *chip,
+				      const u8 *data, u8 *ecc)
+{
+	u32 ecc_value;
+	u8 chunk, ecc_byte, ecc_status;
+
+	for (chunk = 0; chunk < PL353_MAX_ECC_CHUNKS; chunk++) {
+		/* Read ECC value for each block */
+		ecc_value = pl353_smc_get_ecc_val(chunk);
+		ecc_status = (ecc_value >> PL353_NAND_ECC_VALID_SHIFT);
+
+		/* ECC value valid */
+		if (ecc_status & PL353_NAND_ECC_VALID_MASK) {
+			for (ecc_byte = 0; ecc_byte < PL353_MAX_ECC_BYTES;
+			     ecc_byte++) {
+				/* Copy ECC bytes to MTD buffer */
+				*ecc = ~ecc_value & 0xFF;
+				ecc_value = ecc_value >> 8;
+				ecc++;
+			}
+		} else {
+			pr_warn("%s status failed\n", __func__);
+			return -1;
+		}
+	}
+
+	return 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 uncorrectable ECC errors found.
+ */
+static int pl353_nand_correct_data(struct nand_chip *chip, 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)) &
+			  PL353_ECC_BIT_MASK;
+	read_ecc_upper = ((read_ecc[1] >> 4) | (read_ecc[2] << 4)) &
+			  PL353_ECC_BIT_MASK;
+
+	calc_ecc_lower = (calc_ecc[0] | (calc_ecc[1] << 8)) &
+			  PL353_ECC_BIT_MASK;
+	calc_ecc_upper = ((calc_ecc[1] >> 4) | (calc_ecc[2] << 4)) &
+			  PL353_ECC_BIT_MASK;
+
+	ecc_odd = read_ecc_lower ^ calc_ecc_lower;
+	ecc_even = read_ecc_upper ^ calc_ecc_upper;
+
+	/* no error */
+	if (!ecc_odd && !ecc_even)
+		return 0;
+
+	if (ecc_odd == (~ecc_even & PL353_ECC_BIT_MASK)) {
+		/* bits [11:3] of error code is byte offset */
+		byte_addr = (ecc_odd >> 3) & PL353_ECC_BITS_BYTEOFF_MASK;
+		/* bits [2:0] of error code is bit offset */
+		bit_addr = ecc_odd & PL353_ECC_BITS_BITOFF_MASK;
+		/* Toggling error bit */
+		buf[byte_addr] ^= (BIT(bit_addr));
+		return 1;
+	}
+
+	/* one error in parity */
+	if (hweight32(ecc_odd | ecc_even) == 1)
+		return 1;
+
+	/* Uncorrectable error */
+	return -1;
+}
+
+static void pl353_prepare_cmd(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;
+	unsigned long cmd_phase_addr = 0, cmd_phase_data = 0;
+	struct mtd_info *mtd = nand_to_mtd(chip);
+
+	struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
+
+	end_cmd_valid = read ? 1 : 0;
+
+	cmd_phase_addr = (unsigned long __force)xnfc->regs +
+			 ((xnfc->addr_cycles
+			 << ADDR_CYCLES_SHIFT) |
+			 (end_cmd_valid << END_CMD_VALID_SHIFT) |
+			 (COMMAND_PHASE) |
+			 (end_cmd << END_CMD_SHIFT) |
+			 (start_cmd << START_CMD_SHIFT));
+
+	/* Get the data phase address */
+	data_phase_addr = (unsigned long __force)xnfc->regs +
+			  ((0x0 << CLEAR_CS_SHIFT) |
+			  (0 << END_CMD_VALID_SHIFT) |
+			  (DATA_PHASE) |
+			  (end_cmd << END_CMD_SHIFT) |
+			  (0x0 << ECC_LAST_SHIFT));
+
+	xnfc->buf_addr = (void __iomem * __force)data_phase_addr;
+
+	if (chip->options & NAND_BUSWIDTH_16)
+		column /= 2;
+	cmd_phase_data = column;
+	if (mtd->writesize > PL353_NAND_ECC_SIZE) {
+		cmd_phase_data |= page << 16;
+		/* Another address cycle for devices > 128MiB */
+		if (chip->options & NAND_ROW_ADDR_3) {
+			writel_relaxed(cmd_phase_data,
+				       (void __iomem * __force)cmd_phase_addr);
+			cmd_phase_data = (page >> 16);
+		}
+	} else {
+		cmd_phase_data |= page << 8;
+	}
+
+	writel_relaxed(cmd_phase_data, (void __iomem * __force)cmd_phase_addr);
+}
+
+/**
+ * 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 structure
+ * @page:	Page number to read
+ *
+ * Return:	Always return zero
+ */
+static int pl353_nand_read_oob(struct nand_chip *chip,
+			       int page)
+{
+	struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
+	struct mtd_info *mtd = nand_to_mtd(chip);
+	unsigned long data_phase_addr;
+	unsigned long nand_offset = (unsigned long __force)xnfc->regs;
+	u8 *p;
+
+	chip->pagebuf = -1;
+	if (mtd->writesize < PL353_NAND_ECC_SIZE)
+		return 0;
+
+	pl353_prepare_cmd(chip, page, mtd->writesize, NAND_CMD_READ0,
+			  NAND_CMD_READSTART, 1);
+	if (pl353_wait_for_dev_ready(chip))
+		return -ETIMEDOUT;
+
+	p = chip->oob_poi;
+	pl353_nand_read_data_op(chip, p,
+				(mtd->oobsize -
+				PL353_NAND_LAST_TRANSFER_LENGTH), false);
+	p += (mtd->oobsize - PL353_NAND_LAST_TRANSFER_LENGTH);
+	data_phase_addr = (unsigned long __force)xnfc->buf_addr;
+	data_phase_addr -= nand_offset;
+	data_phase_addr |= PL353_NAND_CLEAR_CS;
+	data_phase_addr += nand_offset;
+	xnfc->buf_addr = (void __iomem * __force)data_phase_addr;
+	pl353_nand_read_data_op(chip, p, PL353_NAND_LAST_TRANSFER_LENGTH,
+				false);
+
+	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
+ */
+static int pl353_nand_write_oob(struct nand_chip *chip,
+				int page)
+{
+	struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
+	struct mtd_info *mtd = nand_to_mtd(chip);
+	unsigned long nand_offset = (unsigned long __force)xnfc->regs;
+	unsigned long data_phase_addr;
+	const u8 *buf = chip->oob_poi;
+
+	chip->pagebuf = -1;
+	pl353_prepare_cmd(chip, page, mtd->writesize, NAND_CMD_SEQIN,
+			  NAND_CMD_PAGEPROG, 0);
+
+	pl353_nand_write_data_op(chip, buf,
+				 (mtd->oobsize -
+				 PL353_NAND_LAST_TRANSFER_LENGTH), false);
+	buf += (mtd->oobsize - PL353_NAND_LAST_TRANSFER_LENGTH);
+
+	data_phase_addr = (unsigned long __force)xnfc->buf_addr;
+	data_phase_addr -= nand_offset;
+	data_phase_addr |= PL353_NAND_CLEAR_CS;
+	data_phase_addr |= (1 << END_CMD_VALID_SHIFT);
+	data_phase_addr += nand_offset;
+	xnfc->buf_addr = (void __iomem * __force)data_phase_addr;
+	pl353_nand_write_data_op(chip, buf, PL353_NAND_LAST_TRANSFER_LENGTH,
+				 false);
+	if (pl353_wait_for_dev_ready(chip))
+		return -ETIMEDOUT;
+
+	return 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 nand_chip *chip,
+				    u8 *buf, int oob_required, int page)
+{
+	struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
+	struct mtd_info *mtd = nand_to_mtd(chip);
+	unsigned long nand_offset = (unsigned long __force)xnfc->regs;
+	unsigned long data_phase_addr;
+	u8 *p;
+
+	pl353_prepare_cmd(chip, page, 0, NAND_CMD_READ0,
+			  NAND_CMD_READSTART, 1);
+	if (pl353_wait_for_dev_ready(chip))
+		return -ETIMEDOUT;
+
+	pl353_nand_read_data_op(chip, buf, mtd->writesize, false);
+	p = chip->oob_poi;
+	pl353_nand_read_data_op(chip, p,
+				(mtd->oobsize -
+				PL353_NAND_LAST_TRANSFER_LENGTH), false);
+	p += (mtd->oobsize - PL353_NAND_LAST_TRANSFER_LENGTH);
+
+	data_phase_addr = (unsigned long __force)xnfc->buf_addr;
+	data_phase_addr -= nand_offset;
+	data_phase_addr |= PL353_NAND_CLEAR_CS;
+	data_phase_addr += nand_offset;
+	xnfc->buf_addr = (void __iomem * __force)data_phase_addr;
+
+	pl353_nand_read_data_op(chip, p, PL353_NAND_LAST_TRANSFER_LENGTH,
+				false);
+
+	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 nand_chip *chip,
+				     const u8 *buf, int oob_required,
+				     int page)
+{
+	struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
+	struct mtd_info *mtd = nand_to_mtd(chip);
+	unsigned long nand_offset = (unsigned long __force)xnfc->regs;
+	unsigned long data_phase_addr;
+	u8 *p;
+
+	pl353_prepare_cmd(chip, page, 0, NAND_CMD_SEQIN,
+			  NAND_CMD_PAGEPROG, 0);
+	pl353_nand_write_data_op(chip, buf, mtd->writesize, false);
+	p = chip->oob_poi;
+	pl353_nand_write_data_op(chip, p,
+				 (mtd->oobsize -
+				 PL353_NAND_LAST_TRANSFER_LENGTH), false);
+	p += (mtd->oobsize - PL353_NAND_LAST_TRANSFER_LENGTH);
+
+	data_phase_addr = (unsigned long __force)xnfc->buf_addr;
+	data_phase_addr -= nand_offset;
+	data_phase_addr |= PL353_NAND_CLEAR_CS;
+	data_phase_addr |= (1 << END_CMD_VALID_SHIFT);
+	data_phase_addr += nand_offset;
+	xnfc->buf_addr = (void __iomem * __force)data_phase_addr;
+	pl353_nand_write_data_op(chip, p, PL353_NAND_LAST_TRANSFER_LENGTH,
+				 false);
+
+	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 nand_chip *chip,
+				       const u8 *buf, int oob_required,
+				       int page)
+{
+	int eccsize = chip->ecc.size;
+	int eccsteps = chip->ecc.steps;
+	u8 *ecc_calc = chip->ecc.calc_buf;
+	u8 *oob_ptr;
+	const u8 *p = buf;
+	u32 ret;
+	struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
+	struct mtd_info *mtd = nand_to_mtd(chip);
+	unsigned long nand_offset = (unsigned long __force)xnfc->regs;
+	unsigned long data_phase_addr;
+
+	pl353_prepare_cmd(chip, page, 0, NAND_CMD_SEQIN,
+			  NAND_CMD_PAGEPROG, 0);
+
+	for ( ; (eccsteps - 1); eccsteps--) {
+		pl353_nand_write_data_op(chip, p, eccsize, false);
+		p += eccsize;
+	}
+	pl353_nand_write_data_op(chip, p,
+				 (eccsize - PL353_NAND_LAST_TRANSFER_LENGTH),
+				 false);
+	p += (eccsize - PL353_NAND_LAST_TRANSFER_LENGTH);
+
+	/* Set ECC Last bit to 1 */
+	data_phase_addr = (unsigned long __force)xnfc->buf_addr;
+	data_phase_addr -= nand_offset;
+	data_phase_addr |= PL353_NAND_ECC_LAST;
+	data_phase_addr += nand_offset;
+	xnfc->buf_addr = (void __iomem * __force)data_phase_addr;
+	pl353_nand_write_data_op(chip, p, PL353_NAND_LAST_TRANSFER_LENGTH,
+				 false);
+
+	/* Wait till the ECC operation is complete or timeout */
+	ret = pl353_wait_for_ecc_done();
+	if (ret)
+		dev_err(xnfc->dev, "ECC Timeout\n");
+	p = buf;
+	ret = chip->ecc.calculate(chip, p, &ecc_calc[0]);
+	if (ret)
+		return ret;
+
+	/* 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)xnfc->buf_addr;
+	data_phase_addr -= nand_offset;
+	data_phase_addr &= ~PL353_NAND_ECC_LAST;
+	data_phase_addr += nand_offset;
+	xnfc->buf_addr = (void __iomem * __force)data_phase_addr;
+
+	/* Write the spare area with ECC bytes */
+	oob_ptr = chip->oob_poi;
+	pl353_nand_write_data_op(chip, oob_ptr,
+				 (mtd->oobsize -
+				 PL353_NAND_LAST_TRANSFER_LENGTH), false);
+
+	data_phase_addr = (unsigned long __force)xnfc->buf_addr;
+	data_phase_addr -= nand_offset;
+	data_phase_addr |= PL353_NAND_CLEAR_CS;
+	data_phase_addr |= (1 << END_CMD_VALID_SHIFT);
+	data_phase_addr += nand_offset;
+	xnfc->buf_addr = (void __iomem * __force)data_phase_addr;
+	oob_ptr += (mtd->oobsize - PL353_NAND_LAST_TRANSFER_LENGTH);
+	pl353_nand_write_data_op(chip, oob_ptr, PL353_NAND_LAST_TRANSFER_LENGTH,
+				 false);
+	if (pl353_wait_for_dev_ready(chip))
+		return -ETIMEDOUT;
+
+	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.
+ * There is a limitation in SMC controller, that we must set ECC LAST on
+ * last data phase access, to tell ECC block not to expect any data further.
+ * Ex:  When number of ECC STEPS are 4, then till 3 we will write to flash
+ * using SMC with HW ECC enabled. And for the last ECC STEP, we will subtract
+ * 4bytes from page size, and will initiate a transfer. And the remaining 4 as
+ * one more transfer with ECC_LAST bit set in NAND data phase register to
+ * notify ECC block not to expect any more data. The last block should be align
+ * with end of 512 byte block. Because of this limitation, we are not using
+ * core routines.
+ *
+ * Return:	0 always and updates ECC operation status in to MTD structure
+ */
+static int pl353_nand_read_page_hwecc(struct nand_chip *chip,
+				      u8 *buf, int oob_required, int page)
+{
+	int i, stat, eccsize = chip->ecc.size;
+	int eccbytes = chip->ecc.bytes;
+	int eccsteps = chip->ecc.steps;
+	u8 *p = buf;
+	u8 *ecc_calc = chip->ecc.calc_buf;
+	u8 *ecc = chip->ecc.code_buf;
+	unsigned int max_bitflips = 0;
+	u8 *oob_ptr;
+	u32 ret;
+	unsigned long data_phase_addr;
+	unsigned long nand_offset = (unsigned long __force)xnfc->regs;
+	struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
+	struct mtd_info *mtd = nand_to_mtd(chip);
+
+	pl353_prepare_cmd(chip, page, 0, NAND_CMD_READ0,
+			  NAND_CMD_READSTART, 1);
+	if (pl353_wait_for_dev_ready(chip))
+		return -ETIMEDOUT;
+
+	for ( ; (eccsteps - 1); eccsteps--) {
+		pl353_nand_read_data_op(chip, p, eccsize, false);
+		p += eccsize;
+	}
+
+	pl353_nand_read_data_op(chip, p,
+				(eccsize - PL353_NAND_LAST_TRANSFER_LENGTH),
+				false);
+	p += (eccsize - PL353_NAND_LAST_TRANSFER_LENGTH);
+
+	/* Set ECC Last bit to 1 */
+	data_phase_addr = (unsigned long __force)xnfc->buf_addr;
+	data_phase_addr -= nand_offset;
+	data_phase_addr |= PL353_NAND_ECC_LAST;
+	data_phase_addr += nand_offset;
+	xnfc->buf_addr = (void __iomem * __force)data_phase_addr;
+	pl353_nand_read_data_op(chip, p, PL353_NAND_LAST_TRANSFER_LENGTH,
+				false);
+
+	/* Wait till the ECC operation is complete or timeout */
+	ret = pl353_wait_for_ecc_done();
+	if (ret)
+		dev_err(xnfc->dev, "ECC Timeout\n");
+
+	/* Read the calculated ECC value */
+	p = buf;
+	ret = chip->ecc.calculate(chip, p, &ecc_calc[0]);
+	if (ret)
+		return ret;
+
+	/* Clear ECC last bit */
+	data_phase_addr = (unsigned long __force)xnfc->buf_addr;
+	data_phase_addr -= nand_offset;
+	data_phase_addr &= ~PL353_NAND_ECC_LAST;
+	data_phase_addr += nand_offset;
+	xnfc->buf_addr = (void __iomem * __force)data_phase_addr;
+
+	/* Read the stored ECC value */
+	oob_ptr = chip->oob_poi;
+	pl353_nand_read_data_op(chip, oob_ptr,
+				(mtd->oobsize -
+				PL353_NAND_LAST_TRANSFER_LENGTH), false);
+
+	/* de-assert chip select */
+	data_phase_addr = (unsigned long __force)xnfc->buf_addr;
+	data_phase_addr -= nand_offset;
+	data_phase_addr |= PL353_NAND_CLEAR_CS;
+	data_phase_addr += nand_offset;
+	xnfc->buf_addr = (void __iomem * __force)data_phase_addr;
+
+	oob_ptr += (mtd->oobsize - PL353_NAND_LAST_TRANSFER_LENGTH);
+	pl353_nand_read_data_op(chip, oob_ptr, PL353_NAND_LAST_TRANSFER_LENGTH,
+				false);
+
+	ret = mtd_ooblayout_get_eccbytes(mtd, ecc, 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) {
+		stat = chip->ecc.correct(chip, p, &ecc[i], &ecc_calc[i]);
+		if (stat < 0) {
+			mtd->ecc_stats.failed++;
+		} else {
+			mtd->ecc_stats.corrected += stat;
+			max_bitflips = max_t(unsigned int, max_bitflips, stat);
+		}
+	}
+
+	return max_bitflips;
+}
+
+/* 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++) {
+		instr = &subop->instrs[op_id];
+
+		switch (instr->type) {
+		case NAND_OP_CMD_INSTR:
+			if (op_id)
+				nfc_op->cmnds[1] = instr->ctx.cmd.opcode;
+			else
+				nfc_op->cmnds[0] = instr->ctx.cmd.opcode;
+			nfc_op->cle_ale_delay_ns = instr->delay_ns;
+			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->addr5 = addrs[4];
+			if (naddrs >= 6)
+				nfc_op->addr6 = addrs[5];
+			nfc_op->naddrs = nand_subop_get_num_addr_cyc(subop,
+								     op_id);
+			nfc_op->cle_ale_delay_ns = instr->delay_ns;
+			break;
+
+		case NAND_OP_DATA_IN_INSTR:
+			nfc_op->data_instr = instr;
+			nfc_op->data_instr_idx = op_id;
+			break;
+
+		case NAND_OP_DATA_OUT_INSTR:
+			nfc_op->data_instr = instr;
+			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;
+			break;
+		}
+	}
+}
+
+static void cond_delay(unsigned int ns)
+{
+	if (!ns)
+		return;
+
+	if (ns < 10000)
+		ndelay(ns);
+	else
+		udelay(DIV_ROUND_UP(ns, 1000));
+}
+
+/**
+ * pl353_nand_exec_op_cmd - 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_exec_op_cmd(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 = {};
+	struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
+	unsigned long cmd_phase_data = 0, end_cmd_valid = 0;
+	unsigned long cmd_phase_addr, data_phase_addr, end_cmd;
+	unsigned int op_id, len;
+	bool reading;
+
+	pl353_nfc_parse_instructions(chip, subop, &nfc_op);
+	instr = nfc_op.data_instr;
+	op_id = nfc_op.data_instr_idx;
+
+	pl353_smc_clr_nand_int();
+	/* Get the command phase address */
+	if (nfc_op.cmnds[1] != 0) {
+		if (nfc_op.cmnds[0] == NAND_CMD_SEQIN)
+			end_cmd_valid = 0;
+		else
+			end_cmd_valid = 1;
+		end_cmd = nfc_op.cmnds[1];
+	}  else {
+		end_cmd = 0x0;
+	}
+
+	/*
+	 * The SMC defines two phases of commands when transferring data to or
+	 * from NAND flash.
+	 * Command phase: Commands and optional address information are written
+	 * to the NAND flash.The command and address can be associated with
+	 * either a data phase operation to write to or read from the array,
+	 * or a status/ID register transfer.
+	 * Data phase: Data is either written to or read from the NAND flash.
+	 * This data can be either data transferred to or from the array,
+	 * or status/ID register information.
+	 */
+	cmd_phase_addr = (unsigned long __force)xnfc->regs +
+			 ((nfc_op.naddrs << ADDR_CYCLES_SHIFT) |
+			 (end_cmd_valid << END_CMD_VALID_SHIFT) |
+			 (COMMAND_PHASE) |
+			 (end_cmd << END_CMD_SHIFT) |
+			 (nfc_op.cmnds[0] << START_CMD_SHIFT));
+
+	/* Get the data phase address */
+	end_cmd_valid = 0;
+
+	data_phase_addr = (unsigned long __force)xnfc->regs +
+			  ((0x0 << CLEAR_CS_SHIFT) |
+			  (end_cmd_valid << END_CMD_VALID_SHIFT) |
+			  (DATA_PHASE) |
+			  (end_cmd << END_CMD_SHIFT) |
+			  (0x0 << ECC_LAST_SHIFT));
+	xnfc->buf_addr = (void __iomem * __force)data_phase_addr;
+
+	/* Command phase AXI Read & Write */
+	if (nfc_op.naddrs >= 5) {
+		if (mtd->writesize > PL353_NAND_ECC_SIZE) {
+			cmd_phase_data = nfc_op.addrs;
+			/* Another address cycle for devices > 128MiB */
+			if (chip->options & NAND_ROW_ADDR_3) {
+				writel_relaxed(cmd_phase_data,
+					       (void __iomem * __force)
+					       cmd_phase_addr);
+				cmd_phase_data = nfc_op.addr5;
+				if (nfc_op.naddrs >= 6)
+					cmd_phase_data |= (nfc_op.addr6 << 8);
+			}
+		}
+	}  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)) {
+				column >>= 1;
+			}
+			cmd_phase_data = column;
+		}
+	}
+
+	writel_relaxed(cmd_phase_data, (void __iomem * __force)cmd_phase_addr);
+	if (!nfc_op.data_instr) {
+		if (nfc_op.rdy_timeout_ms) {
+			if (pl353_wait_for_dev_ready(chip))
+				return -ETIMEDOUT;
+		}
+
+		return 0;
+	}
+
+	reading = (nfc_op.data_instr->type == NAND_OP_DATA_IN_INSTR);
+	if (!reading) {
+		len = nand_subop_get_data_len(subop, op_id);
+		pl353_nand_write_data_op(chip, instr->ctx.data.buf.out,
+					 len, instr->ctx.data.force_8bit);
+		if (nfc_op.rdy_timeout_ms) {
+			if (pl353_wait_for_dev_ready(chip))
+				return -ETIMEDOUT;
+		}
+
+		cond_delay(nfc_op.rdy_delay_ns);
+	}
+
+	if (reading) {
+		len = nand_subop_get_data_len(subop, op_id);
+		cond_delay(nfc_op.rdy_delay_ns);
+		if (nfc_op.rdy_timeout_ms) {
+			if (pl353_wait_for_dev_ready(chip))
+				return -ETIMEDOUT;
+		}
+
+		pl353_nand_read_data_op(chip, instr->ctx.data.buf.in, len,
+					instr->ctx.data.force_8bit);
+	}
+
+	return 0;
+}
+
+static const struct nand_op_parser pl353_nfc_op_parser = NAND_OP_PARSER
+	(NAND_OP_PARSER_PATTERN
+		(pl353_nand_exec_op_cmd,
+		NAND_OP_PARSER_PAT_CMD_ELEM(true),
+		NAND_OP_PARSER_PAT_ADDR_ELEM(true, 7),
+		NAND_OP_PARSER_PAT_WAITRDY_ELEM(true),
+		NAND_OP_PARSER_PAT_DATA_IN_ELEM(false, 2048)),
+	NAND_OP_PARSER_PATTERN
+		(pl353_nand_exec_op_cmd,
+		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_PAT_DATA_IN_ELEM(false, 2048)),
+	NAND_OP_PARSER_PATTERN
+		(pl353_nand_exec_op_cmd,
+		NAND_OP_PARSER_PAT_CMD_ELEM(false),
+		NAND_OP_PARSER_PAT_ADDR_ELEM(true, 7),
+		NAND_OP_PARSER_PAT_CMD_ELEM(true),
+		NAND_OP_PARSER_PAT_WAITRDY_ELEM(false)),
+	NAND_OP_PARSER_PATTERN
+		(pl353_nand_exec_op_cmd,
+		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_exec_op_cmd,
+		NAND_OP_PARSER_PAT_CMD_ELEM(false)),
+	);
+
+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_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
+ *
+ * Return:	0 on success or negative errno.
+ */
+static int pl353_nand_ecc_init(struct mtd_info *mtd, struct nand_ecc_ctrl *ecc,
+			       int ecc_mode)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
+	int err = 0, ret;
+
+	ecc->read_oob = pl353_nand_read_oob;
+	ecc->write_oob = pl353_nand_write_oob;
+	if (ecc_mode == NAND_ECC_ON_DIE) {
+		ecc->write_page_raw = pl353_nand_write_page_raw;
+		ecc->read_page_raw = pl353_nand_read_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;
+		ret = pl353_smc_set_ecc_mode(PL353_SMC_ECCMODE_BYPASS);
+		if (ret)
+			return ret;
+
+	} 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->read_page = pl353_nand_read_page_hwecc;
+		ecc->size = PL353_NAND_ECC_SIZE;
+		ecc->read_page = pl353_nand_read_page_hwecc;
+		ecc->write_page = pl353_nand_write_page_hwecc;
+		pl353_smc_set_ecc_pg_size(mtd->writesize);
+		switch (mtd->writesize) {
+		case SZ_512:
+		case SZ_1K:
+		case SZ_2K:
+			pl353_smc_set_ecc_mode(PL353_SMC_ECCMODE_APB);
+			break;
+		default:
+			ecc->calculate = nand_calculate_ecc;
+			ecc->correct = nand_correct_data;
+			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);
+		} else {
+			err = -ENXIO;
+			dev_err(xnfc->dev, "Unsupported oob Layout\n");
+		}
+	}
+
+	return err;
+}
+
+static int pl353_nfc_setup_data_interface(struct nand_chip *chip, int csline,
+					  const struct nand_data_interface
+					  *conf)
+{
+	struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
+	const struct nand_sdr_timings *sdr;
+	u32 timings[7], mckperiodps;
+
+	if (csline == NAND_DATA_IFACE_CHECK_ONLY)
+		return 0;
+
+	sdr = nand_get_sdr_timings(conf);
+	if (IS_ERR(sdr))
+		return PTR_ERR(sdr);
+
+	/*
+	 * SDR timings are given in pico-seconds while NFC timings must be
+	 * expressed in NAND controller clock cycles.
+	 */
+	mckperiodps = NSEC_PER_SEC / clk_get_rate(xnfc->mclk);
+	mckperiodps *= 1000;
+	if (sdr->tRC_min <= 20000)
+		/*
+		 * PL353 SMC needs one extra read cycle in SDR Mode 5
+		 * This is not written anywhere in the datasheet but
+		 * the results observed during testing.
+		 */
+		timings[0] = DIV_ROUND_UP(sdr->tRC_min, mckperiodps) + 1;
+	else
+		timings[0] = DIV_ROUND_UP(sdr->tRC_min, mckperiodps);
+
+	timings[1] = DIV_ROUND_UP(sdr->tWC_min, mckperiodps);
+	/*
+	 * For all SDR modes, PL353 SMC needs tREA max value as 1,
+	 * Results observed during testing.
+	 */
+	timings[2] = PL353_TREA_MAX_VALUE;
+	timings[3] = DIV_ROUND_UP(sdr->tWP_min, mckperiodps);
+	timings[4] = DIV_ROUND_UP(sdr->tCLR_min, mckperiodps);
+	timings[5] = DIV_ROUND_UP(sdr->tAR_min, mckperiodps);
+	timings[6] = DIV_ROUND_UP(sdr->tRR_min, mckperiodps);
+	pl353_smc_set_cycles(timings);
+
+	return 0;
+}
+
+static int pl353_nand_attach_chip(struct nand_chip *chip)
+{
+	struct mtd_info *mtd = nand_to_mtd(chip);
+	struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
+	int ret;
+
+	if (chip->options & NAND_BUSWIDTH_16) {
+		ret = pl353_smc_set_buswidth(PL353_SMC_MEM_WIDTH_16);
+		if (ret) {
+			dev_err(xnfc->dev, "Set BusWidth failed\n");
+			return ret;
+		}
+	}
+
+	if (mtd->writesize <= SZ_512)
+		xnfc->addr_cycles = 1;
+	else
+		xnfc->addr_cycles = 2;
+
+	if (chip->options & NAND_ROW_ADDR_3)
+		xnfc->addr_cycles += 3;
+	else
+		xnfc->addr_cycles += 2;
+
+	ret = pl353_nand_ecc_init(mtd, &chip->ecc, chip->ecc.mode);
+	if (ret) {
+		dev_err(xnfc->dev, "ECC init failed\n");
+		return ret;
+	}
+
+	if (!mtd->name) {
+		/*
+		 * If the new bindings are used and the bootloader has not been
+		 * updated to pass a new mtdparts parameter on the cmdline, you
+		 * should define the following property in your NAND node, ie:
+		 *
+		 *	label = "pl353-nand";
+		 *
+		 * This way, mtd->name will be set by the core when
+		 * nand_set_flash_node() is called.
+		 */
+		mtd->name = devm_kasprintf(xnfc->dev, GFP_KERNEL,
+					   "%s", PL353_NAND_DRIVER_NAME);
+		if (!mtd->name) {
+			dev_err(xnfc->dev, "Failed to allocate mtd->name\n");
+			return -ENOMEM;
+		}
+	}
+
+	return 0;
+}
+
+static const struct nand_controller_ops pl353_nand_controller_ops = {
+	.attach_chip = pl353_nand_attach_chip,
+	.exec_op = pl353_nfc_exec_op,
+	.setup_data_interface = pl353_nfc_setup_data_interface,
+};
+
+/**
+ * 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.
+ * The NAND driver has dependency with the pl353_smc memory controller
+ * driver for initializing the NAND timing parameters, bus width, ECC modes,
+ * control and status information.
+ *
+ * Return:	0 on success or error value on failure
+ */
+static int pl353_nand_probe(struct platform_device *pdev)
+{
+	struct pl353_nand_controller *xnfc;
+	struct mtd_info *mtd;
+	struct nand_chip *chip;
+	struct resource *res;
+	struct device_node *np, *dn;
+	u32 ret, val;
+
+	xnfc = devm_kzalloc(&pdev->dev, sizeof(*xnfc), GFP_KERNEL);
+	if (!xnfc)
+		return -ENOMEM;
+
+	xnfc->dev = &pdev->dev;
+	nand_controller_init(&xnfc->controller);
+	xnfc->controller.ops = &pl353_nand_controller_ops;
+	/* Map physical address of NAND flash */
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	xnfc->regs = devm_ioremap_resource(xnfc->dev, res);
+	if (IS_ERR(xnfc->regs))
+		return PTR_ERR(xnfc->regs);
+
+	chip = &xnfc->chip;
+	chip->controller = &xnfc->controller;
+	mtd = nand_to_mtd(chip);
+	nand_set_controller_data(chip, xnfc);
+	mtd->priv = chip;
+	mtd->owner = THIS_MODULE;
+	nand_set_flash_node(chip, xnfc->dev->of_node);
+
+	np = of_get_next_parent(xnfc->dev->of_node);
+	xnfc->mclk = of_clk_get(np, 0);
+	if (IS_ERR(xnfc->mclk)) {
+		dev_err(xnfc->dev, "Failed to retrieve MCK clk\n");
+		return PTR_ERR(xnfc->mclk);
+	}
+
+	dn = nand_get_flash_node(chip);
+	ret = of_property_read_u32(dn, "nand-bus-width", &val);
+	if (ret)
+		val = 8;
+
+	/* Set the device option and flash width */
+	chip->options = NAND_BUSWIDTH_AUTO;
+	chip->bbt_options = NAND_BBT_USE_FLASH;
+	platform_set_drvdata(pdev, xnfc);
+	ret = nand_scan(chip, 1);
+	if (ret) {
+		dev_err(xnfc->dev, "could not scan the nand chip\n");
+		return ret;
+	}
+
+	ret = mtd_device_register(mtd, NULL, 0);
+	if (ret) {
+		dev_err(xnfc->dev, "Failed to register mtd device: %d\n", ret);
+		nand_cleanup(chip);
+		return ret;
+	}
+
+	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_controller *xnfc = platform_get_drvdata(pdev);
+	struct mtd_info *mtd = nand_to_mtd(&xnfc->chip);
+	struct nand_chip *chip = mtd_to_nand(mtd);
+
+	/* Release resources, unregister device */
+	nand_release(chip);
+
+	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");
-- 
2.7.4


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [LINUX PATCH v14] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface

[Helmut Grohne]

WARNING: This driver might brick the hardware. See below.

Hi Naga,

On Mon, Apr 15, 2019 at 04:40:13PM +0530, Naga Sureshkumar Relli wrote:
> Changes in v14:
>  - Removed legacy hooks as per Miquel comments

Thank you for the update.

> +static inline int pl353_wait_for_dev_ready(struct nand_chip *chip)
> +{
> +	unsigned long timeout = jiffies + PL353_NAND_DEV_BUSY_TIMEOUT;
> +
> +	do {
> +		if (pl353_smc_get_nand_int_status_raw()) {
> +			pl353_smc_clr_nand_int();
> +			break;

A closing brace is missing here. This causes a compilation failure.

> +
> +		cpu_relax();

You previously used cond_resched (via nand_wait_ready) here. Why did you
change it to cpu_relax()?

> +	} while (!time_after_eq(jiffies, timeout));
> +
> +	if (time_after_eq(jiffies, timeout)) {
> +		pr_err("%s timed out\n", __func__);
> +		return -ETIMEDOUT;
> +	}
> +
> +	return 0;
> +}


> +static int pl353_nand_read_page_hwecc(struct nand_chip *chip,
> +				      u8 *buf, int oob_required, int page)
> +{
> +	int i, stat, eccsize = chip->ecc.size;
> +	int eccbytes = chip->ecc.bytes;
> +	int eccsteps = chip->ecc.steps;
> +	u8 *p = buf;
> +	u8 *ecc_calc = chip->ecc.calc_buf;
> +	u8 *ecc = chip->ecc.code_buf;
> +	unsigned int max_bitflips = 0;
> +	u8 *oob_ptr;
> +	u32 ret;
> +	unsigned long data_phase_addr;
> +	unsigned long nand_offset = (unsigned long __force)xnfc->regs;

The variable xnfc is undeclared here. Consider swapping the line with
the next one.

> +	struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
> +	struct mtd_info *mtd = nand_to_mtd(chip);

After loading the driver, the device does not work. The dmesg output is:

nand: device found, Manufacturer ID: 0x2c, Chip ID: 0xda
nand: Micron MT29F2G08ABAEAWP
nand: 256 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64
Bad block table not found for chip 0
Bad block table not found for chip 0
Scanning device for bad blocks
nand_bbt: error while writing BBT block -524
nand_bbt: error while writing BBT block -524
nand_bbt: error while writing BBT block -524
nand_bbt: error while writing BBT block -524
No space left to write bad block table
nand_bbt: error while writing bad block table -28
pl353-nand e1000000.flash: could not scan the nand chip
pl353-nand: probe of e1000000.flash failed with error -28

After trying the driver, the flash chip was bricked. Neither the old
driver nor the uboot-xlnx driver nor the Xilinx fsbl are able to talk to
the chip afterwards. This behaviour persists even after a full power
cycle. I'll try reinitializing the flash chip next. I've only seen this
behaviour once, so there is a slight chance that the cause is something
else.

Helmut

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Naga Sureshkumar Relli]

Hi Helmut,

> -----Original Message-----
> From: Helmut Grohne <helmut.grohne@intenta.de>
> Sent: Tuesday, April 23, 2019 6:15 PM
> To: Naga Sureshkumar Relli <nagasure@xilinx.com>
> Cc: bbrezillon@kernel.org; miquel.raynal@bootlin.com; richard@nod.at;
> dwmw2@infradead.org; computersforpeace@gmail.com; marek.vasut@gmail.com; linux-
> mtd@lists.infradead.org; linux-kernel@vger.kernel.org; Michal Simek <michals@xilinx.com>;
> nagasureshkumarrelli@gmail.com
> Subject: Re: [LINUX PATCH v14] mtd: rawnand: pl353: Add basic driver for arm pl353 smc
> nand interface
> 
> WARNING: This driver might brick the hardware. See below.
> 
> Hi Naga,
> 
> On Mon, Apr 15, 2019 at 04:40:13PM +0530, Naga Sureshkumar Relli wrote:
> > Changes in v14:
> >  - Removed legacy hooks as per Miquel comments
> 
> Thank you for the update.
> 
> > +static inline int pl353_wait_for_dev_ready(struct nand_chip *chip) {
> > +	unsigned long timeout = jiffies + PL353_NAND_DEV_BUSY_TIMEOUT;
> > +
> > +	do {
> > +		if (pl353_smc_get_nand_int_status_raw()) {
> > +			pl353_smc_clr_nand_int();
> > +			break;
> 
> A closing brace is missing here. This causes a compilation failure.
While cleaning up the warnings reported by checkpatch,  this happened.
sorry for that. I will correct it. 
> 
> > +
> > +		cpu_relax();
> 
> You previously used cond_resched (via nand_wait_ready) here. Why did you change it to
> cpu_relax()?
I just replicated the pl353_wait_for_ecc_done() API definition.
But did you see any issue with this?
Anyway I will replace it with cond_resched(), instead of cpu_releax()
> 
> > +	} while (!time_after_eq(jiffies, timeout));
> > +
> > +	if (time_after_eq(jiffies, timeout)) {
> > +		pr_err("%s timed out\n", __func__);
> > +		return -ETIMEDOUT;
> > +	}
> > +
> > +	return 0;
> > +}
> 
> 
> > +static int pl353_nand_read_page_hwecc(struct nand_chip *chip,
> > +				      u8 *buf, int oob_required, int page) {
> > +	int i, stat, eccsize = chip->ecc.size;
> > +	int eccbytes = chip->ecc.bytes;
> > +	int eccsteps = chip->ecc.steps;
> > +	u8 *p = buf;
> > +	u8 *ecc_calc = chip->ecc.calc_buf;
> > +	u8 *ecc = chip->ecc.code_buf;
> > +	unsigned int max_bitflips = 0;
> > +	u8 *oob_ptr;
> > +	u32 ret;
> > +	unsigned long data_phase_addr;
> > +	unsigned long nand_offset = (unsigned long __force)xnfc->regs;
> 
> The variable xnfc is undeclared here. Consider swapping the line with the next one.
> 
> > +	struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
> > +	struct mtd_info *mtd = nand_to_mtd(chip);
> 
> After loading the driver, the device does not work. The dmesg output is:
> 
> nand: device found, Manufacturer ID: 0x2c, Chip ID: 0xda
> nand: Micron MT29F2G08ABAEAWP
> nand: 256 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64 Bad block table not
> found for chip 0 Bad block table not found for chip 0 Scanning device for bad blocks
> nand_bbt: error while writing BBT block -524
> nand_bbt: error while writing BBT block -524
> nand_bbt: error while writing BBT block -524
> nand_bbt: error while writing BBT block -524 No space left to write bad block table
> nand_bbt: error while writing bad block table -28 pl353-nand e1000000.flash: could not scan
> the nand chip
> pl353-nand: probe of e1000000.flash failed with error -28
Did you follow the same thing that you tried earlier?
i.e. updated "nand-bus-width" property and "nand-ecc-mode" ?
I haven't seen any issue in BBT scanning, with this patch.

> 
> After trying the driver, the flash chip was bricked. Neither the old driver nor the uboot-xlnx
> driver nor the Xilinx fsbl are able to talk to the chip afterwards. This behaviour persists even
> after a full power cycle. I'll try reinitializing the flash chip next. I've only seen this behaviour
> once, so there is a slight chance that the cause is something else.
Sometimes I also faced the same problem during driver development.
What I did is, in standalone nandps driver example,  I forcibly created BBT in the init and once
 it is done. I just reloaded the actual example. Then after wards u-boot and Linux are able to scan
the BBT.

Thanks,
Naga Sureshkumar Relli
> 
> Helmut

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [LINUX PATCH v14] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface

[Helmut Grohne]

On Wed, Apr 24, 2019 at 05:04:38AM +0000, Naga Sureshkumar Relli wrote:
> > You previously used cond_resched (via nand_wait_ready) here. Why did you change it to
> > cpu_relax()?
> I just replicated the pl353_wait_for_ecc_done() API definition.
> But did you see any issue with this?
> Anyway I will replace it with cond_resched(), instead of cpu_releax()

This was an observation and it made me ask for reasons. I did not have
any practical issues here.

> Did you follow the same thing that you tried earlier?
> i.e. updated "nand-bus-width" property and "nand-ecc-mode" ?

Yes, I used the same device tree that made v13 partially work here.

> > After trying the driver, the flash chip was bricked. Neither the old driver nor the uboot-xlnx
> > driver nor the Xilinx fsbl are able to talk to the chip afterwards. This behaviour persists even
> > after a full power cycle. I'll try reinitializing the flash chip next. I've only seen this behaviour
> > once, so there is a slight chance that the cause is something else.
> Sometimes I also faced the same problem during driver development.
> What I did is, in standalone nandps driver example,  I forcibly created BBT in the init and once
>  it is done. I just reloaded the actual example. Then after wards u-boot and Linux are able to scan
> the BBT.

I confirm. It was just the BBT being bad. It can also be recreated using
u-boot with "nand scrib.chip".

I also spent some time reviewing the code and will send another mail
about that.

Helmut

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [LINUX PATCH v14] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface

[Helmut Grohne]

Without much knowledge of the nand framework, I attempted reviewing the
code. Hope this helps.

Helmut

On Mon, Apr 15, 2019 at 04:40:13PM +0530, Naga Sureshkumar Relli wrote:
> diff --git a/drivers/mtd/nand/raw/pl353_nand.c b/drivers/mtd/nand/raw/pl353_nand.c
> new file mode 100644
> index 0000000..eb63778
> --- /dev/null
> +++ b/drivers/mtd/nand/raw/pl353_nand.c
> @@ -0,0 +1,1399 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ARM PL353 NAND flash controller driver
> + *
> + * Copyright (C) 2017 Xilinx, Inc
> + * Author: Punnaiah chowdary kalluri <punnaiah@xilinx.com>
> + * Author: Naga Sureshkumar Relli <nagasure@xilinx.com>
> + *
> + */
> +
> +#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/pl353-smc.h>
> +#include <linux/clk.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 */

The two macros above are entirely unused. They're a relict from an
earlier driver version of the driver and were used in struct
pl35x_nand_command_format member end_cmd_valid. I think they can safely
be removed now.

> +#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 */

This macro is also unused even in older versions of the driver.

> +/* 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 PL353_NAND_ECC_BUSY_TIMEOUT	(1 * HZ)
> +#define PL353_NAND_DEV_BUSY_TIMEOUT	(1 * HZ)

These timeouts are a second each. I've remarked earlier that you are
waiting with cpu_relax() on these. Having the CPU spin for a full second
is bad. Please try using less intensive waiting methods for such long
delays or reduce the timeouts.

> +#define PL353_NAND_LAST_TRANSFER_LENGTH	4
> +#define PL353_NAND_ECC_VALID_SHIFT	24
> +#define PL353_NAND_ECC_VALID_MASK	0x40
> +#define PL353_ECC_BITS_BYTEOFF_MASK	0x1FF
> +#define PL353_ECC_BITS_BITOFF_MASK	0x7
> +#define PL353_ECC_BIT_MASK		0xFFF
> +#define PL353_TREA_MAX_VALUE		1
> +#define PL353_MAX_ECC_CHUNKS		4
> +#define PL353_MAX_ECC_BYTES		3
> +
> +struct pl353_nfc_op {
> +	u32 cmnds[4];

Why does this hold 4 elements? In the code, this array is only indexed
with 0 and 1.

> +	u32 end_cmd;

What is the purpose of this field. It is never accessed.

> +	u32 addrs;
> +	u32 naddrs;
> +	u32 addr5;
> +	u32 addr6;

Why are addr5 and addr6 u32? You only ever store u8 values in here. How
about merging them into an u16 addr56? Doing so would make the access in
pl353_nand_exec_op_cmd simpler and move a little complexity into
pl353_nfc_parse_instructions.

> +	unsigned int data_instr_idx;
> +	unsigned int rdy_timeout_ms;
> +	unsigned int rdy_delay_ns;
> +	unsigned int cle_ale_delay_ns;

What is the purpose of this field. It is set in two places, but never
read. No driver logic depends on its value.

> +	const struct nand_op_instr *data_instr;
> +};
> +
> +/**
> + * struct pl353_nand_controller - Defines the NAND flash controller driver
> + *				  instance
> + * @chip:		NAND chip information structure
> + * @dev:		Parent device (used to print error messages)
> + * @regs:		Virtual address of the NAND flash device
> + * @buf_addr:		Virtual address of the NAND flash device for
> + *			data read/writes
> + * @addr_cycles:	Address cycles
> + * @mclk:		Memory controller clock
> + * @buswidth:		Bus width 8 or 16
> + */
> +struct pl353_nand_controller {
> +	struct nand_controller controller;
> +	struct nand_chip chip;
> +	struct device *dev;
> +	void __iomem *regs;
> +	void __iomem *buf_addr;

I find the use of buf_addr unfortunate. It is consumed by two functions
pl353_nand_read_data_op and pl353_nand_write_data_op. All other
functions update its value. Semantically, its value is regs + some
flags. For the updaters that means a complex logic where they first have
to subtract reg, then change flags and add reg again. To make matters
worse, this computation involves __force casts between long and __iomem
(which yielded complaints in earlier reviews).  I think it would
simplify the code if you replaced buf_addr with something like
addr_flags and then compute buf_addr as regs + addr_flags in those two
consumers. What do you think?

> +	u8 addr_cycles;
> +	struct clk *mclk;

All you need here is the memory clock frequency. Wouldn't it be easier
to extract that frequency once during probe and store it here? That
assumes a constant frequency, but if the frequency isn't constant, you
have a race condition.

> +	u32 buswidth;
> +};
> +
> +static inline struct pl353_nand_controller *
> +			to_pl353_nand(struct nand_chip *chip)
> +{
> +	return container_of(chip, struct pl353_nand_controller, chip);
> +}
> +
> +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 * chip->ecc.bytes);
> +	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 * chip->ecc.bytes) + 8;
> +	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;

We already know that section == 0 here. This second condition can only
be met if chip->ecc.steps < 0. Is that really what you want to test
here?

> +
> +	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 u8 bbt_pattern[] = { 'B', 'b', 't', '0' };
> +static u8 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

I have a general question concerning the nand framework here. The
pattern member in struct nand_bbt_descr is not declared const.
Therefore, bbt_pattern cannot be const here. As far as I looked, all
accesses of pattern use it with memcmp or as source for memcpy. Also the
diskonchip.c driver assigns a string constant here. This suggests, that
pattern should be declared const or that diskonchip.c is doing it wrong.

> +};
> +
> +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
> +};
> +
> +static void pl353_nfc_force_byte_access(struct nand_chip *chip,
> +					bool force_8bit)
> +{
> +	int ret;
> +	struct pl353_nand_controller *xnfc =
> +		container_of(chip, struct pl353_nand_controller, chip);
> +
> +	if (xnfc->buswidth == 8)

This buswidth member is never assigned anywhere. Thus the value is
always 0 and this comparison always fails.

> +		return;
> +
> +	if (force_8bit)
> +		ret = pl353_smc_set_buswidth(PL353_SMC_MEM_WIDTH_8);
> +	else
> +		ret = pl353_smc_set_buswidth(PL353_SMC_MEM_WIDTH_16);
> +
> +	if (ret)
> +		dev_err(xnfc->dev, "Error in Buswidth\n");
> +}
> +
> +static inline int pl353_wait_for_dev_ready(struct nand_chip *chip)
> +{
> +	unsigned long timeout = jiffies + PL353_NAND_DEV_BUSY_TIMEOUT;
> +
> +	do {
> +		if (pl353_smc_get_nand_int_status_raw()) {
> +			pl353_smc_clr_nand_int();
> +			break;
> +
> +		cpu_relax();
> +	} while (!time_after_eq(jiffies, timeout));
> +
> +	if (time_after_eq(jiffies, timeout)) {
> +		pr_err("%s timed out\n", __func__);
> +		return -ETIMEDOUT;
> +	}

This could be simplified and avoid repeating the timeout condition:

	while (!pl353_smc_get_nand_int_status_raw()) {
		if (time_after_eq(jiffies, timeout)) {
			pr_err("%s timed out\n", __func__);
			return -ETIMEDOUT;
		}
		cpu_relax();
	}
	pl353_smc_clr_nand_int();

> +
> +	return 0;
> +}
> +
> +/**
> + * pl353_nand_read_data_op - 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
> + * @force_8bit:	Force 8-bit bus access
> + * Return:	Always return zero
> + */
> +static void pl353_nand_read_data_op(struct nand_chip *chip, u8 *in,
> +				    unsigned int len, bool force_8bit)
> +{
> +	int i;
> +	struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
> +
> +	if (force_8bit)
> +		pl353_nfc_force_byte_access(chip, true);
> +
> +	if ((IS_ALIGNED((uint32_t)in, sizeof(uint32_t)) &&
> +	     IS_ALIGNED(len, sizeof(uint32_t))) || !force_8bit) {
> +		u32 *ptr = (u32 *)in;
> +
> +		len /= 4;
> +		for (i = 0; i < len; i++)
> +			ptr[i] = readl(xnfc->buf_addr);
> +	} else {
> +		for (i = 0; i < len; i++)
> +			in[i] = readb(xnfc->buf_addr);
> +	}
> +
> +	if (force_8bit)
> +		pl353_nfc_force_byte_access(chip, false);
> +}
> +
> +/**
> + * pl353_nand_write_buf - write buffer to chip
> + * @mtd:	Pointer to the mtd info structure
> + * @buf:	Pointer to the buffer to store write data
> + * @len:	Number of bytes to write
> + * @force_8bit:	Force 8-bit bus access
> + */
> +static void pl353_nand_write_data_op(struct nand_chip *chip, const u8 *buf,
> +				     int len, bool force_8bit)
> +{
> +	int i;
> +	struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
> +
> +	if (force_8bit)
> +		pl353_nfc_force_byte_access(chip, true);
> +
> +	if ((IS_ALIGNED((uint32_t)buf, sizeof(uint32_t)) &&
> +	     IS_ALIGNED(len, sizeof(uint32_t))) || !force_8bit) {
> +		u32 *ptr = (u32 *)buf;
> +
> +		len /= 4;
> +		for (i = 0; i < len; i++)
> +			writel(ptr[i], xnfc->buf_addr);
> +	} else {
> +		for (i = 0; i < len; i++)
> +			writeb(buf[i], xnfc->buf_addr);
> +	}
> +
> +	if (force_8bit)
> +		pl353_nfc_force_byte_access(chip, false);
> +}
> +
> +static inline int pl353_wait_for_ecc_done(void)
> +{
> +	unsigned long timeout = jiffies + PL353_NAND_ECC_BUSY_TIMEOUT;
> +
> +	do {
> +		if (pl353_smc_ecc_is_busy())
> +			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;
> +	}

This could be simplified and avoid repeating the timeout condition:

	while (pl353_smc_ecc_is_busy()) {
		if (time_after_eq(jiffies, timeout)) {
			pr_err("%s timed out\n", __func__);
			return -ETIMEDOUT;
		}
		cpu_relax();
	}

> +
> +	return 0;
> +}
> +
> +/**
> + * pl353_nand_calculate_hwecc - Calculate Hardware ECC
> + * @mtd:	Pointer to the mtd_info structure
> + * @data:	Pointer to the page data
> + * @ecc:	Pointer to the ECC buffer where ECC data needs to be stored
> + *
> + * This function retrieves the Hardware ECC data from the controller and returns
> + * ECC data back to the MTD subsystem.
> + * It operates on a number of 512 byte blocks of NAND memory and can be
> + * programmed to store the ECC codes after the data in memory. For writes,
> + * the ECC is written to the spare area of the page. For reads, the result of
> + * a block ECC check are made available to the device driver.
> + *
> + * ------------------------------------------------------------------------
> + * |               n * 512 blocks                  | extra  | ecc    |     |
> + * |                                               | block  | codes  |     |
> + * ------------------------------------------------------------------------
> + *
> + * The ECC calculation uses a simple Hamming code, using 1-bit correction 2-bit
> + * detection. It starts when a valid read or write command with a 512 byte
> + * aligned address is detected on the memory interface.
> + *
> + * Return:	0 on success or error value on failure
> + */
> +static int pl353_nand_calculate_hwecc(struct nand_chip *chip,
> +				      const u8 *data, u8 *ecc)
> +{
> +	u32 ecc_value;
> +	u8 chunk, ecc_byte, ecc_status;
> +
> +	for (chunk = 0; chunk < PL353_MAX_ECC_CHUNKS; chunk++) {
> +		/* Read ECC value for each block */
> +		ecc_value = pl353_smc_get_ecc_val(chunk);
> +		ecc_status = (ecc_value >> PL353_NAND_ECC_VALID_SHIFT);
> +
> +		/* ECC value valid */
> +		if (ecc_status & PL353_NAND_ECC_VALID_MASK) {
> +			for (ecc_byte = 0; ecc_byte < PL353_MAX_ECC_BYTES;
> +			     ecc_byte++) {
> +				/* Copy ECC bytes to MTD buffer */
> +				*ecc = ~ecc_value & 0xFF;
> +				ecc_value = ecc_value >> 8;
> +				ecc++;
> +			}
> +		} else {
> +			pr_warn("%s status failed\n", __func__);
> +			return -1;
> +		}
> +	}
> +
> +	return 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 uncorrectable ECC errors found.
> + */
> +static int pl353_nand_correct_data(struct nand_chip *chip, 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)) &
> +			  PL353_ECC_BIT_MASK;
> +	read_ecc_upper = ((read_ecc[1] >> 4) | (read_ecc[2] << 4)) &
> +			  PL353_ECC_BIT_MASK;
> +
> +	calc_ecc_lower = (calc_ecc[0] | (calc_ecc[1] << 8)) &
> +			  PL353_ECC_BIT_MASK;
> +	calc_ecc_upper = ((calc_ecc[1] >> 4) | (calc_ecc[2] << 4)) &
> +			  PL353_ECC_BIT_MASK;
> +
> +	ecc_odd = read_ecc_lower ^ calc_ecc_lower;
> +	ecc_even = read_ecc_upper ^ calc_ecc_upper;
> +
> +	/* no error */
> +	if (!ecc_odd && !ecc_even)
> +		return 0;
> +
> +	if (ecc_odd == (~ecc_even & PL353_ECC_BIT_MASK)) {
> +		/* bits [11:3] of error code is byte offset */
> +		byte_addr = (ecc_odd >> 3) & PL353_ECC_BITS_BYTEOFF_MASK;
> +		/* bits [2:0] of error code is bit offset */
> +		bit_addr = ecc_odd & PL353_ECC_BITS_BITOFF_MASK;
> +		/* Toggling error bit */
> +		buf[byte_addr] ^= (BIT(bit_addr));
> +		return 1;
> +	}
> +
> +	/* one error in parity */
> +	if (hweight32(ecc_odd | ecc_even) == 1)
> +		return 1;
> +
> +	/* Uncorrectable error */
> +	return -1;
> +}
> +
> +static void pl353_prepare_cmd(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;
> +	unsigned long cmd_phase_addr = 0, cmd_phase_data = 0;
> +	struct mtd_info *mtd = nand_to_mtd(chip);
> +
> +	struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
> +
> +	end_cmd_valid = read ? 1 : 0;
> +
> +	cmd_phase_addr = (unsigned long __force)xnfc->regs +
> +			 ((xnfc->addr_cycles
> +			 << ADDR_CYCLES_SHIFT) |
> +			 (end_cmd_valid << END_CMD_VALID_SHIFT) |
> +			 (COMMAND_PHASE) |
> +			 (end_cmd << END_CMD_SHIFT) |
> +			 (start_cmd << START_CMD_SHIFT));
> +
> +	/* Get the data phase address */
> +	data_phase_addr = (unsigned long __force)xnfc->regs +
> +			  ((0x0 << CLEAR_CS_SHIFT) |
> +			  (0 << END_CMD_VALID_SHIFT) |
> +			  (DATA_PHASE) |
> +			  (end_cmd << END_CMD_SHIFT) |
> +			  (0x0 << ECC_LAST_SHIFT));
> +
> +	xnfc->buf_addr = (void __iomem * __force)data_phase_addr;
> +
> +	if (chip->options & NAND_BUSWIDTH_16)
> +		column /= 2;
> +	cmd_phase_data = column;
> +	if (mtd->writesize > PL353_NAND_ECC_SIZE) {
> +		cmd_phase_data |= page << 16;
> +		/* Another address cycle for devices > 128MiB */
> +		if (chip->options & NAND_ROW_ADDR_3) {
> +			writel_relaxed(cmd_phase_data,
> +				       (void __iomem * __force)cmd_phase_addr);
> +			cmd_phase_data = (page >> 16);
> +		}
> +	} else {
> +		cmd_phase_data |= page << 8;
> +	}
> +
> +	writel_relaxed(cmd_phase_data, (void __iomem * __force)cmd_phase_addr);
> +}
> +
> +/**
> + * 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 structure
> + * @page:	Page number to read
> + *
> + * Return:	Always return zero
> + */
> +static int pl353_nand_read_oob(struct nand_chip *chip,
> +			       int page)
> +{
> +	struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
> +	struct mtd_info *mtd = nand_to_mtd(chip);
> +	unsigned long data_phase_addr;
> +	unsigned long nand_offset = (unsigned long __force)xnfc->regs;
> +	u8 *p;
> +
> +	chip->pagebuf = -1;
> +	if (mtd->writesize < PL353_NAND_ECC_SIZE)
> +		return 0;
> +
> +	pl353_prepare_cmd(chip, page, mtd->writesize, NAND_CMD_READ0,
> +			  NAND_CMD_READSTART, 1);
> +	if (pl353_wait_for_dev_ready(chip))
> +		return -ETIMEDOUT;
> +
> +	p = chip->oob_poi;
> +	pl353_nand_read_data_op(chip, p,
> +				(mtd->oobsize -
> +				PL353_NAND_LAST_TRANSFER_LENGTH), false);
> +	p += (mtd->oobsize - PL353_NAND_LAST_TRANSFER_LENGTH);
> +	data_phase_addr = (unsigned long __force)xnfc->buf_addr;
> +	data_phase_addr -= nand_offset;
> +	data_phase_addr |= PL353_NAND_CLEAR_CS;
> +	data_phase_addr += nand_offset;
> +	xnfc->buf_addr = (void __iomem * __force)data_phase_addr;
> +	pl353_nand_read_data_op(chip, p, PL353_NAND_LAST_TRANSFER_LENGTH,
> +				false);
> +
> +	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
> + */
> +static int pl353_nand_write_oob(struct nand_chip *chip,
> +				int page)
> +{
> +	struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
> +	struct mtd_info *mtd = nand_to_mtd(chip);
> +	unsigned long nand_offset = (unsigned long __force)xnfc->regs;
> +	unsigned long data_phase_addr;
> +	const u8 *buf = chip->oob_poi;
> +
> +	chip->pagebuf = -1;
> +	pl353_prepare_cmd(chip, page, mtd->writesize, NAND_CMD_SEQIN,
> +			  NAND_CMD_PAGEPROG, 0);
> +
> +	pl353_nand_write_data_op(chip, buf,
> +				 (mtd->oobsize -
> +				 PL353_NAND_LAST_TRANSFER_LENGTH), false);
> +	buf += (mtd->oobsize - PL353_NAND_LAST_TRANSFER_LENGTH);
> +
> +	data_phase_addr = (unsigned long __force)xnfc->buf_addr;
> +	data_phase_addr -= nand_offset;
> +	data_phase_addr |= PL353_NAND_CLEAR_CS;
> +	data_phase_addr |= (1 << END_CMD_VALID_SHIFT);
> +	data_phase_addr += nand_offset;
> +	xnfc->buf_addr = (void __iomem * __force)data_phase_addr;
> +	pl353_nand_write_data_op(chip, buf, PL353_NAND_LAST_TRANSFER_LENGTH,
> +				 false);
> +	if (pl353_wait_for_dev_ready(chip))
> +		return -ETIMEDOUT;
> +
> +	return 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 nand_chip *chip,
> +				    u8 *buf, int oob_required, int page)
> +{
> +	struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
> +	struct mtd_info *mtd = nand_to_mtd(chip);
> +	unsigned long nand_offset = (unsigned long __force)xnfc->regs;
> +	unsigned long data_phase_addr;
> +	u8 *p;
> +
> +	pl353_prepare_cmd(chip, page, 0, NAND_CMD_READ0,
> +			  NAND_CMD_READSTART, 1);
> +	if (pl353_wait_for_dev_ready(chip))
> +		return -ETIMEDOUT;
> +
> +	pl353_nand_read_data_op(chip, buf, mtd->writesize, false);
> +	p = chip->oob_poi;
> +	pl353_nand_read_data_op(chip, p,
> +				(mtd->oobsize -
> +				PL353_NAND_LAST_TRANSFER_LENGTH), false);
> +	p += (mtd->oobsize - PL353_NAND_LAST_TRANSFER_LENGTH);
> +
> +	data_phase_addr = (unsigned long __force)xnfc->buf_addr;
> +	data_phase_addr -= nand_offset;
> +	data_phase_addr |= PL353_NAND_CLEAR_CS;
> +	data_phase_addr += nand_offset;
> +	xnfc->buf_addr = (void __iomem * __force)data_phase_addr;
> +
> +	pl353_nand_read_data_op(chip, p, PL353_NAND_LAST_TRANSFER_LENGTH,
> +				false);
> +
> +	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 nand_chip *chip,
> +				     const u8 *buf, int oob_required,
> +				     int page)
> +{
> +	struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
> +	struct mtd_info *mtd = nand_to_mtd(chip);
> +	unsigned long nand_offset = (unsigned long __force)xnfc->regs;
> +	unsigned long data_phase_addr;
> +	u8 *p;
> +
> +	pl353_prepare_cmd(chip, page, 0, NAND_CMD_SEQIN,
> +			  NAND_CMD_PAGEPROG, 0);
> +	pl353_nand_write_data_op(chip, buf, mtd->writesize, false);
> +	p = chip->oob_poi;
> +	pl353_nand_write_data_op(chip, p,
> +				 (mtd->oobsize -
> +				 PL353_NAND_LAST_TRANSFER_LENGTH), false);
> +	p += (mtd->oobsize - PL353_NAND_LAST_TRANSFER_LENGTH);
> +
> +	data_phase_addr = (unsigned long __force)xnfc->buf_addr;
> +	data_phase_addr -= nand_offset;
> +	data_phase_addr |= PL353_NAND_CLEAR_CS;
> +	data_phase_addr |= (1 << END_CMD_VALID_SHIFT);
> +	data_phase_addr += nand_offset;
> +	xnfc->buf_addr = (void __iomem * __force)data_phase_addr;
> +	pl353_nand_write_data_op(chip, p, PL353_NAND_LAST_TRANSFER_LENGTH,
> +				 false);
> +
> +	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 nand_chip *chip,
> +				       const u8 *buf, int oob_required,
> +				       int page)
> +{
> +	int eccsize = chip->ecc.size;
> +	int eccsteps = chip->ecc.steps;
> +	u8 *ecc_calc = chip->ecc.calc_buf;
> +	u8 *oob_ptr;
> +	const u8 *p = buf;
> +	u32 ret;
> +	struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
> +	struct mtd_info *mtd = nand_to_mtd(chip);
> +	unsigned long nand_offset = (unsigned long __force)xnfc->regs;
> +	unsigned long data_phase_addr;
> +
> +	pl353_prepare_cmd(chip, page, 0, NAND_CMD_SEQIN,
> +			  NAND_CMD_PAGEPROG, 0);
> +
> +	for ( ; (eccsteps - 1); eccsteps--) {
> +		pl353_nand_write_data_op(chip, p, eccsize, false);
> +		p += eccsize;
> +	}
> +	pl353_nand_write_data_op(chip, p,
> +				 (eccsize - PL353_NAND_LAST_TRANSFER_LENGTH),
> +				 false);
> +	p += (eccsize - PL353_NAND_LAST_TRANSFER_LENGTH);
> +
> +	/* Set ECC Last bit to 1 */
> +	data_phase_addr = (unsigned long __force)xnfc->buf_addr;
> +	data_phase_addr -= nand_offset;
> +	data_phase_addr |= PL353_NAND_ECC_LAST;
> +	data_phase_addr += nand_offset;
> +	xnfc->buf_addr = (void __iomem * __force)data_phase_addr;
> +	pl353_nand_write_data_op(chip, p, PL353_NAND_LAST_TRANSFER_LENGTH,
> +				 false);
> +
> +	/* Wait till the ECC operation is complete or timeout */
> +	ret = pl353_wait_for_ecc_done();
> +	if (ret)
> +		dev_err(xnfc->dev, "ECC Timeout\n");
> +	p = buf;
> +	ret = chip->ecc.calculate(chip, p, &ecc_calc[0]);
> +	if (ret)
> +		return ret;
> +
> +	/* 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)xnfc->buf_addr;
> +	data_phase_addr -= nand_offset;
> +	data_phase_addr &= ~PL353_NAND_ECC_LAST;
> +	data_phase_addr += nand_offset;
> +	xnfc->buf_addr = (void __iomem * __force)data_phase_addr;
> +
> +	/* Write the spare area with ECC bytes */
> +	oob_ptr = chip->oob_poi;
> +	pl353_nand_write_data_op(chip, oob_ptr,
> +				 (mtd->oobsize -
> +				 PL353_NAND_LAST_TRANSFER_LENGTH), false);
> +
> +	data_phase_addr = (unsigned long __force)xnfc->buf_addr;
> +	data_phase_addr -= nand_offset;
> +	data_phase_addr |= PL353_NAND_CLEAR_CS;
> +	data_phase_addr |= (1 << END_CMD_VALID_SHIFT);
> +	data_phase_addr += nand_offset;
> +	xnfc->buf_addr = (void __iomem * __force)data_phase_addr;
> +	oob_ptr += (mtd->oobsize - PL353_NAND_LAST_TRANSFER_LENGTH);
> +	pl353_nand_write_data_op(chip, oob_ptr, PL353_NAND_LAST_TRANSFER_LENGTH,
> +				 false);
> +	if (pl353_wait_for_dev_ready(chip))
> +		return -ETIMEDOUT;
> +
> +	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.
> + * There is a limitation in SMC controller, that we must set ECC LAST on
> + * last data phase access, to tell ECC block not to expect any data further.
> + * Ex:  When number of ECC STEPS are 4, then till 3 we will write to flash
> + * using SMC with HW ECC enabled. And for the last ECC STEP, we will subtract
> + * 4bytes from page size, and will initiate a transfer. And the remaining 4 as
> + * one more transfer with ECC_LAST bit set in NAND data phase register to
> + * notify ECC block not to expect any more data. The last block should be align
> + * with end of 512 byte block. Because of this limitation, we are not using
> + * core routines.
> + *
> + * Return:	0 always and updates ECC operation status in to MTD structure
> + */
> +static int pl353_nand_read_page_hwecc(struct nand_chip *chip,
> +				      u8 *buf, int oob_required, int page)
> +{
> +	int i, stat, eccsize = chip->ecc.size;
> +	int eccbytes = chip->ecc.bytes;
> +	int eccsteps = chip->ecc.steps;
> +	u8 *p = buf;
> +	u8 *ecc_calc = chip->ecc.calc_buf;
> +	u8 *ecc = chip->ecc.code_buf;
> +	unsigned int max_bitflips = 0;
> +	u8 *oob_ptr;
> +	u32 ret;
> +	unsigned long data_phase_addr;
> +	unsigned long nand_offset = (unsigned long __force)xnfc->regs;
> +	struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
> +	struct mtd_info *mtd = nand_to_mtd(chip);
> +
> +	pl353_prepare_cmd(chip, page, 0, NAND_CMD_READ0,
> +			  NAND_CMD_READSTART, 1);
> +	if (pl353_wait_for_dev_ready(chip))
> +		return -ETIMEDOUT;
> +
> +	for ( ; (eccsteps - 1); eccsteps--) {
> +		pl353_nand_read_data_op(chip, p, eccsize, false);
> +		p += eccsize;
> +	}
> +
> +	pl353_nand_read_data_op(chip, p,
> +				(eccsize - PL353_NAND_LAST_TRANSFER_LENGTH),
> +				false);
> +	p += (eccsize - PL353_NAND_LAST_TRANSFER_LENGTH);
> +
> +	/* Set ECC Last bit to 1 */
> +	data_phase_addr = (unsigned long __force)xnfc->buf_addr;
> +	data_phase_addr -= nand_offset;
> +	data_phase_addr |= PL353_NAND_ECC_LAST;
> +	data_phase_addr += nand_offset;
> +	xnfc->buf_addr = (void __iomem * __force)data_phase_addr;
> +	pl353_nand_read_data_op(chip, p, PL353_NAND_LAST_TRANSFER_LENGTH,
> +				false);
> +
> +	/* Wait till the ECC operation is complete or timeout */
> +	ret = pl353_wait_for_ecc_done();
> +	if (ret)
> +		dev_err(xnfc->dev, "ECC Timeout\n");
> +
> +	/* Read the calculated ECC value */
> +	p = buf;
> +	ret = chip->ecc.calculate(chip, p, &ecc_calc[0]);
> +	if (ret)
> +		return ret;
> +
> +	/* Clear ECC last bit */
> +	data_phase_addr = (unsigned long __force)xnfc->buf_addr;
> +	data_phase_addr -= nand_offset;
> +	data_phase_addr &= ~PL353_NAND_ECC_LAST;
> +	data_phase_addr += nand_offset;
> +	xnfc->buf_addr = (void __iomem * __force)data_phase_addr;
> +
> +	/* Read the stored ECC value */
> +	oob_ptr = chip->oob_poi;
> +	pl353_nand_read_data_op(chip, oob_ptr,
> +				(mtd->oobsize -
> +				PL353_NAND_LAST_TRANSFER_LENGTH), false);
> +
> +	/* de-assert chip select */
> +	data_phase_addr = (unsigned long __force)xnfc->buf_addr;
> +	data_phase_addr -= nand_offset;
> +	data_phase_addr |= PL353_NAND_CLEAR_CS;
> +	data_phase_addr += nand_offset;
> +	xnfc->buf_addr = (void __iomem * __force)data_phase_addr;
> +
> +	oob_ptr += (mtd->oobsize - PL353_NAND_LAST_TRANSFER_LENGTH);
> +	pl353_nand_read_data_op(chip, oob_ptr, PL353_NAND_LAST_TRANSFER_LENGTH,
> +				false);
> +
> +	ret = mtd_ooblayout_get_eccbytes(mtd, ecc, 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) {
> +		stat = chip->ecc.correct(chip, p, &ecc[i], &ecc_calc[i]);
> +		if (stat < 0) {
> +			mtd->ecc_stats.failed++;
> +		} else {
> +			mtd->ecc_stats.corrected += stat;
> +			max_bitflips = max_t(unsigned int, max_bitflips, stat);
> +		}
> +	}
> +
> +	return max_bitflips;
> +}
> +
> +/* 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++) {
> +		instr = &subop->instrs[op_id];
> +
> +		switch (instr->type) {
> +		case NAND_OP_CMD_INSTR:
> +			if (op_id)
> +				nfc_op->cmnds[1] = instr->ctx.cmd.opcode;
> +			else
> +				nfc_op->cmnds[0] = instr->ctx.cmd.opcode;
> +			nfc_op->cle_ale_delay_ns = instr->delay_ns;
> +			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] <<

I don't quite understand what this code does, but it looks strange to
me. I compared it to other drivers. The code here is quite similar to
marvell_nand.c. It seems like we are copying a varying number (0 to 6)
of addresses from the buffer instr->ctx.addr.addrs. However their
indices are special: 0, 1, 2, 3, offset + 4, offset + 5. This is
non-consecutive and different from marvell_nand.c in this regard. Could
it be that you really meant index offset+i here?

> +						 (8 * i);
> +			}
> +
> +			if (naddrs >= 5)
> +				nfc_op->addr5 = addrs[4];
> +			if (naddrs >= 6)
> +				nfc_op->addr6 = addrs[5];
> +			nfc_op->naddrs = nand_subop_get_num_addr_cyc(subop,
> +								     op_id);
> +			nfc_op->cle_ale_delay_ns = instr->delay_ns;
> +			break;
> +
> +		case NAND_OP_DATA_IN_INSTR:
> +			nfc_op->data_instr = instr;
> +			nfc_op->data_instr_idx = op_id;
> +			break;
> +
> +		case NAND_OP_DATA_OUT_INSTR:
> +			nfc_op->data_instr = instr;
> +			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;
> +			break;
> +		}
> +	}
> +}
> +
> +static void cond_delay(unsigned int ns)
> +{
> +	if (!ns)
> +		return;
> +
> +	if (ns < 10000)
> +		ndelay(ns);
> +	else
> +		udelay(DIV_ROUND_UP(ns, 1000));
> +}

This function has an exact copy in marvell_nand.c. Would it make sense
to move it to a more central place? There are only two copies yet.

Note that on arm (the primary target of this driver), ndelay is
implemented using udelay.

> +/**
> + * pl353_nand_exec_op_cmd - 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_exec_op_cmd(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 = {};
> +	struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
> +	unsigned long cmd_phase_data = 0, end_cmd_valid = 0;
> +	unsigned long cmd_phase_addr, data_phase_addr, end_cmd;
> +	unsigned int op_id, len;
> +	bool reading;
> +
> +	pl353_nfc_parse_instructions(chip, subop, &nfc_op);
> +	instr = nfc_op.data_instr;
> +	op_id = nfc_op.data_instr_idx;
> +
> +	pl353_smc_clr_nand_int();
> +	/* Get the command phase address */
> +	if (nfc_op.cmnds[1] != 0) {
> +		if (nfc_op.cmnds[0] == NAND_CMD_SEQIN)
> +			end_cmd_valid = 0;
> +		else
> +			end_cmd_valid = 1;
> +		end_cmd = nfc_op.cmnds[1];
> +	}  else {
> +		end_cmd = 0x0;

In this branch, nfc_op.cmnds[1] == 0, so end_cmd is always
nfc_op.cmnds[1]. Would it make sense to pull the assignment out of the
branch?

> +	}
> +
> +	/*
> +	 * The SMC defines two phases of commands when transferring data to or
> +	 * from NAND flash.
> +	 * Command phase: Commands and optional address information are written
> +	 * to the NAND flash.The command and address can be associated with
> +	 * either a data phase operation to write to or read from the array,
> +	 * or a status/ID register transfer.
> +	 * Data phase: Data is either written to or read from the NAND flash.
> +	 * This data can be either data transferred to or from the array,
> +	 * or status/ID register information.
> +	 */
> +	cmd_phase_addr = (unsigned long __force)xnfc->regs +
> +			 ((nfc_op.naddrs << ADDR_CYCLES_SHIFT) |
> +			 (end_cmd_valid << END_CMD_VALID_SHIFT) |
> +			 (COMMAND_PHASE) |
> +			 (end_cmd << END_CMD_SHIFT) |
> +			 (nfc_op.cmnds[0] << START_CMD_SHIFT));
> +
> +	/* Get the data phase address */
> +	end_cmd_valid = 0;
> +
> +	data_phase_addr = (unsigned long __force)xnfc->regs +
> +			  ((0x0 << CLEAR_CS_SHIFT) |
> +			  (end_cmd_valid << END_CMD_VALID_SHIFT) |
> +			  (DATA_PHASE) |
> +			  (end_cmd << END_CMD_SHIFT) |
> +			  (0x0 << ECC_LAST_SHIFT));
> +	xnfc->buf_addr = (void __iomem * __force)data_phase_addr;
> +
> +	/* Command phase AXI Read & Write */
> +	if (nfc_op.naddrs >= 5) {
> +		if (mtd->writesize > PL353_NAND_ECC_SIZE) {
> +			cmd_phase_data = nfc_op.addrs;
> +			/* Another address cycle for devices > 128MiB */
> +			if (chip->options & NAND_ROW_ADDR_3) {
> +				writel_relaxed(cmd_phase_data,
> +					       (void __iomem * __force)
> +					       cmd_phase_addr);
> +				cmd_phase_data = nfc_op.addr5;
> +				if (nfc_op.naddrs >= 6)
> +					cmd_phase_data |= (nfc_op.addr6 << 8);
> +			}
> +		}
> +	}  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)) {
> +				column >>= 1;
> +			}
> +			cmd_phase_data = column;
> +		}
> +	}
> +
> +	writel_relaxed(cmd_phase_data, (void __iomem * __force)cmd_phase_addr);
> +	if (!nfc_op.data_instr) {
> +		if (nfc_op.rdy_timeout_ms) {
> +			if (pl353_wait_for_dev_ready(chip))
> +				return -ETIMEDOUT;
> +		}
> +
> +		return 0;
> +	}
> +
> +	reading = (nfc_op.data_instr->type == NAND_OP_DATA_IN_INSTR);
> +	if (!reading) {
> +		len = nand_subop_get_data_len(subop, op_id);
> +		pl353_nand_write_data_op(chip, instr->ctx.data.buf.out,
> +					 len, instr->ctx.data.force_8bit);
> +		if (nfc_op.rdy_timeout_ms) {
> +			if (pl353_wait_for_dev_ready(chip))
> +				return -ETIMEDOUT;
> +		}
> +
> +		cond_delay(nfc_op.rdy_delay_ns);
> +	}
> +
> +	if (reading) {

You could use an else branch instead of inverting the condition here.
When Miquel complained about this in v13, you said you'd change it,
but you didn't.

> +		len = nand_subop_get_data_len(subop, op_id);
> +		cond_delay(nfc_op.rdy_delay_ns);
> +		if (nfc_op.rdy_timeout_ms) {
> +			if (pl353_wait_for_dev_ready(chip))
> +				return -ETIMEDOUT;
> +		}
> +
> +		pl353_nand_read_data_op(chip, instr->ctx.data.buf.in, len,
> +					instr->ctx.data.force_8bit);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct nand_op_parser pl353_nfc_op_parser = NAND_OP_PARSER
> +	(NAND_OP_PARSER_PATTERN
> +		(pl353_nand_exec_op_cmd,
> +		NAND_OP_PARSER_PAT_CMD_ELEM(true),
> +		NAND_OP_PARSER_PAT_ADDR_ELEM(true, 7),
> +		NAND_OP_PARSER_PAT_WAITRDY_ELEM(true),
> +		NAND_OP_PARSER_PAT_DATA_IN_ELEM(false, 2048)),
> +	NAND_OP_PARSER_PATTERN
> +		(pl353_nand_exec_op_cmd,
> +		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_PAT_DATA_IN_ELEM(false, 2048)),
> +	NAND_OP_PARSER_PATTERN
> +		(pl353_nand_exec_op_cmd,
> +		NAND_OP_PARSER_PAT_CMD_ELEM(false),
> +		NAND_OP_PARSER_PAT_ADDR_ELEM(true, 7),
> +		NAND_OP_PARSER_PAT_CMD_ELEM(true),
> +		NAND_OP_PARSER_PAT_WAITRDY_ELEM(false)),
> +	NAND_OP_PARSER_PATTERN
> +		(pl353_nand_exec_op_cmd,
> +		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_exec_op_cmd,
> +		NAND_OP_PARSER_PAT_CMD_ELEM(false)),
> +	);
> +
> +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_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
> + *
> + * Return:	0 on success or negative errno.
> + */
> +static int pl353_nand_ecc_init(struct mtd_info *mtd, struct nand_ecc_ctrl *ecc,
> +			       int ecc_mode)
> +{
> +	struct nand_chip *chip = mtd_to_nand(mtd);
> +	struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
> +	int err = 0, ret;

These variables serve the same purpose. Both err and ret determine the
return value of this function. Can you merge them into one variable?

> +
> +	ecc->read_oob = pl353_nand_read_oob;
> +	ecc->write_oob = pl353_nand_write_oob;
> +	if (ecc_mode == NAND_ECC_ON_DIE) {
> +		ecc->write_page_raw = pl353_nand_write_page_raw;
> +		ecc->read_page_raw = pl353_nand_read_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;
> +		ret = pl353_smc_set_ecc_mode(PL353_SMC_ECCMODE_BYPASS);
> +		if (ret)
> +			return ret;
> +
> +	} 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->read_page = pl353_nand_read_page_hwecc;
> +		ecc->size = PL353_NAND_ECC_SIZE;
> +		ecc->read_page = pl353_nand_read_page_hwecc;
> +		ecc->write_page = pl353_nand_write_page_hwecc;
> +		pl353_smc_set_ecc_pg_size(mtd->writesize);
> +		switch (mtd->writesize) {
> +		case SZ_512:
> +		case SZ_1K:
> +		case SZ_2K:
> +			pl353_smc_set_ecc_mode(PL353_SMC_ECCMODE_APB);
> +			break;
> +		default:
> +			ecc->calculate = nand_calculate_ecc;
> +			ecc->correct = nand_correct_data;
> +			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);
> +		} else {
> +			err = -ENXIO;
> +			dev_err(xnfc->dev, "Unsupported oob Layout\n");
> +		}
> +	}
> +
> +	return err;
> +}
> +
> +static int pl353_nfc_setup_data_interface(struct nand_chip *chip, int csline,
> +					  const struct nand_data_interface
> +					  *conf)
> +{
> +	struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
> +	const struct nand_sdr_timings *sdr;
> +	u32 timings[7], mckperiodps;
> +
> +	if (csline == NAND_DATA_IFACE_CHECK_ONLY)
> +		return 0;
> +
> +	sdr = nand_get_sdr_timings(conf);
> +	if (IS_ERR(sdr))
> +		return PTR_ERR(sdr);
> +
> +	/*
> +	 * SDR timings are given in pico-seconds while NFC timings must be
> +	 * expressed in NAND controller clock cycles.
> +	 */
> +	mckperiodps = NSEC_PER_SEC / clk_get_rate(xnfc->mclk);
> +	mckperiodps *= 1000;
> +	if (sdr->tRC_min <= 20000)
> +		/*
> +		 * PL353 SMC needs one extra read cycle in SDR Mode 5
> +		 * This is not written anywhere in the datasheet but
> +		 * the results observed during testing.
> +		 */
> +		timings[0] = DIV_ROUND_UP(sdr->tRC_min, mckperiodps) + 1;
> +	else
> +		timings[0] = DIV_ROUND_UP(sdr->tRC_min, mckperiodps);
> +
> +	timings[1] = DIV_ROUND_UP(sdr->tWC_min, mckperiodps);
> +	/*
> +	 * For all SDR modes, PL353 SMC needs tREA max value as 1,
> +	 * Results observed during testing.
> +	 */
> +	timings[2] = PL353_TREA_MAX_VALUE;
> +	timings[3] = DIV_ROUND_UP(sdr->tWP_min, mckperiodps);
> +	timings[4] = DIV_ROUND_UP(sdr->tCLR_min, mckperiodps);
> +	timings[5] = DIV_ROUND_UP(sdr->tAR_min, mckperiodps);
> +	timings[6] = DIV_ROUND_UP(sdr->tRR_min, mckperiodps);
> +	pl353_smc_set_cycles(timings);
> +
> +	return 0;
> +}
> +
> +static int pl353_nand_attach_chip(struct nand_chip *chip)
> +{
> +	struct mtd_info *mtd = nand_to_mtd(chip);
> +	struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
> +	int ret;
> +
> +	if (chip->options & NAND_BUSWIDTH_16) {
> +		ret = pl353_smc_set_buswidth(PL353_SMC_MEM_WIDTH_16);
> +		if (ret) {
> +			dev_err(xnfc->dev, "Set BusWidth failed\n");
> +			return ret;
> +		}
> +	}
> +
> +	if (mtd->writesize <= SZ_512)
> +		xnfc->addr_cycles = 1;
> +	else
> +		xnfc->addr_cycles = 2;
> +
> +	if (chip->options & NAND_ROW_ADDR_3)
> +		xnfc->addr_cycles += 3;
> +	else
> +		xnfc->addr_cycles += 2;
> +
> +	ret = pl353_nand_ecc_init(mtd, &chip->ecc, chip->ecc.mode);
> +	if (ret) {
> +		dev_err(xnfc->dev, "ECC init failed\n");
> +		return ret;
> +	}
> +
> +	if (!mtd->name) {
> +		/*
> +		 * If the new bindings are used and the bootloader has not been
> +		 * updated to pass a new mtdparts parameter on the cmdline, you
> +		 * should define the following property in your NAND node, ie:
> +		 *
> +		 *	label = "pl353-nand";
> +		 *
> +		 * This way, mtd->name will be set by the core when
> +		 * nand_set_flash_node() is called.
> +		 */
> +		mtd->name = devm_kasprintf(xnfc->dev, GFP_KERNEL,
> +					   "%s", PL353_NAND_DRIVER_NAME);
> +		if (!mtd->name) {
> +			dev_err(xnfc->dev, "Failed to allocate mtd->name\n");
> +			return -ENOMEM;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct nand_controller_ops pl353_nand_controller_ops = {
> +	.attach_chip = pl353_nand_attach_chip,
> +	.exec_op = pl353_nfc_exec_op,
> +	.setup_data_interface = pl353_nfc_setup_data_interface,
> +};
> +
> +/**
> + * 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.
> + * The NAND driver has dependency with the pl353_smc memory controller
> + * driver for initializing the NAND timing parameters, bus width, ECC modes,
> + * control and status information.
> + *
> + * Return:	0 on success or error value on failure
> + */
> +static int pl353_nand_probe(struct platform_device *pdev)
> +{
> +	struct pl353_nand_controller *xnfc;
> +	struct mtd_info *mtd;
> +	struct nand_chip *chip;
> +	struct resource *res;
> +	struct device_node *np, *dn;
> +	u32 ret, val;
> +
> +	xnfc = devm_kzalloc(&pdev->dev, sizeof(*xnfc), GFP_KERNEL);
> +	if (!xnfc)
> +		return -ENOMEM;
> +
> +	xnfc->dev = &pdev->dev;
> +	nand_controller_init(&xnfc->controller);
> +	xnfc->controller.ops = &pl353_nand_controller_ops;
> +	/* Map physical address of NAND flash */
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	xnfc->regs = devm_ioremap_resource(xnfc->dev, res);
> +	if (IS_ERR(xnfc->regs))
> +		return PTR_ERR(xnfc->regs);
> +
> +	chip = &xnfc->chip;
> +	chip->controller = &xnfc->controller;
> +	mtd = nand_to_mtd(chip);
> +	nand_set_controller_data(chip, xnfc);
> +	mtd->priv = chip;
> +	mtd->owner = THIS_MODULE;
> +	nand_set_flash_node(chip, xnfc->dev->of_node);
> +
> +	np = of_get_next_parent(xnfc->dev->of_node);
> +	xnfc->mclk = of_clk_get(np, 0);

I think it would be more robust to look up the clock by name rather than
index to mirror what pl353-smc does:

	xnfc->mclk = of_clk_get_by_name(np, "memclk");

> +	if (IS_ERR(xnfc->mclk)) {
> +		dev_err(xnfc->dev, "Failed to retrieve MCK clk\n");
> +		return PTR_ERR(xnfc->mclk);
> +	}
> +
> +	dn = nand_get_flash_node(chip);
> +	ret = of_property_read_u32(dn, "nand-bus-width", &val);
> +	if (ret)
> +		val = 8;

This val seems to be entirely unused.

> +
> +	/* Set the device option and flash width */
> +	chip->options = NAND_BUSWIDTH_AUTO;
> +	chip->bbt_options = NAND_BBT_USE_FLASH;
> +	platform_set_drvdata(pdev, xnfc);
> +	ret = nand_scan(chip, 1);
> +	if (ret) {
> +		dev_err(xnfc->dev, "could not scan the nand chip\n");
> +		return ret;
> +	}
> +
> +	ret = mtd_device_register(mtd, NULL, 0);
> +	if (ret) {
> +		dev_err(xnfc->dev, "Failed to register mtd device: %d\n", ret);
> +		nand_cleanup(chip);
> +		return ret;
> +	}
> +
> +	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_controller *xnfc = platform_get_drvdata(pdev);
> +	struct mtd_info *mtd = nand_to_mtd(&xnfc->chip);
> +	struct nand_chip *chip = mtd_to_nand(mtd);
> +
> +	/* Release resources, unregister device */
> +	nand_release(chip);
> +
> +	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");
> -- 
> 2.7.4
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [LINUX PATCH v14] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface

[Miquel Raynal]

Hi Helmut,

Helmut Grohne <helmut.grohne@intenta.de> wrote on Thu, 25 Apr 2019
13:23:39 +0200:

> Without much knowledge of the nand framework, I attempted reviewing the
> code. Hope this helps.

It does help a lot, thanks for your time!

Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Naga Sureshkumar Relli]

Hi Helmut,

> -----Original Message-----
> From: Helmut Grohne <helmut.grohne@intenta.de>
> Sent: Thursday, April 25, 2019 4:54 PM
> To: Naga Sureshkumar Relli <nagasure@xilinx.com>
> Cc: bbrezillon@kernel.org; miquel.raynal@bootlin.com; richard@nod.at;
> dwmw2@infradead.org; computersforpeace@gmail.com; marek.vasut@gmail.com; linux-
> mtd@lists.infradead.org; linux-kernel@vger.kernel.org; Michal Simek <michals@xilinx.com>;
> nagasureshkumarrelli@gmail.com
> Subject: Re: [LINUX PATCH v14] mtd: rawnand: pl353: Add basic driver for arm pl353 smc
> nand interface
> 
> Without much knowledge of the nand framework, I attempted reviewing the code. Hope this
> helps.
Thanks for your time. It helps.
> 
> Helmut
> 
> On Mon, Apr 15, 2019 at 04:40:13PM +0530, Naga Sureshkumar Relli wrote:
> > diff --git a/drivers/mtd/nand/raw/pl353_nand.c
> > b/drivers/mtd/nand/raw/pl353_nand.c
> > new file mode 100644
> > index 0000000..eb63778
> > --- /dev/null
> > +++ b/drivers/mtd/nand/raw/pl353_nand.c
> > @@ -0,0 +1,1399 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * ARM PL353 NAND flash controller driver
> > + *
> > + * Copyright (C) 2017 Xilinx, Inc
> > + * Author: Punnaiah chowdary kalluri <punnaiah@xilinx.com>
> > + * Author: Naga Sureshkumar Relli <nagasure@xilinx.com>
> > + *
> > + */
> > +
> > +#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/pl353-smc.h>
> > +#include <linux/clk.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
> */
> 
> The two macros above are entirely unused. They're a relict from an earlier driver version of the
> driver and were used in struct pl35x_nand_command_format member end_cmd_valid. I think
> they can safely be removed now.
Ok. will remove it.
> 
> > +#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 */
> 
> This macro is also unused even in older versions of the driver.
Ok. will remove it.
> 
> > +/* 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 PL353_NAND_ECC_BUSY_TIMEOUT	(1 * HZ)
> > +#define PL353_NAND_DEV_BUSY_TIMEOUT	(1 * HZ)
> 
> These timeouts are a second each. I've remarked earlier that you are waiting with cpu_relax()
> on these. Having the CPU spin for a full second is bad. Please try using less intensive waiting
> methods for such long delays or reduce the timeouts.
Ok, as I said previously, will use cond_resched() as like nand_wait_ready().
> 
> > +#define PL353_NAND_LAST_TRANSFER_LENGTH	4
> > +#define PL353_NAND_ECC_VALID_SHIFT	24
> > +#define PL353_NAND_ECC_VALID_MASK	0x40
> > +#define PL353_ECC_BITS_BYTEOFF_MASK	0x1FF
> > +#define PL353_ECC_BITS_BITOFF_MASK	0x7
> > +#define PL353_ECC_BIT_MASK		0xFFF
> > +#define PL353_TREA_MAX_VALUE		1
> > +#define PL353_MAX_ECC_CHUNKS		4
> > +#define PL353_MAX_ECC_BYTES		3
> > +
> > +struct pl353_nfc_op {
> > +	u32 cmnds[4];
> 
> Why does this hold 4 elements? In the code, this array is only indexed with 0 and 1.
Yes, it should be cmnds[2]. I will update it.
> 
> > +	u32 end_cmd;
> 
> What is the purpose of this field. It is never accessed.
Ok, will remove it.
> 
> > +	u32 addrs;
> > +	u32 naddrs;
> > +	u32 addr5;
> > +	u32 addr6;
> 
> Why are addr5 and addr6 u32? You only ever store u8 values in here. How about merging
> them into an u16 addr56? Doing so would make the access in pl353_nand_exec_op_cmd
> simpler and move a little complexity into pl353_nfc_parse_instructions.
Will try this. But I don't see any complex logic involved using addr5 and addr6.
> 
> > +	unsigned int data_instr_idx;
> > +	unsigned int rdy_timeout_ms;
> > +	unsigned int rdy_delay_ns;
> > +	unsigned int cle_ale_delay_ns;
> 
> What is the purpose of this field. It is set in two places, but never read. No driver logic depends
> on its value.
Yes, will remove it.
> 
> > +	const struct nand_op_instr *data_instr; };
> > +
> > +/**
> > + * struct pl353_nand_controller - Defines the NAND flash controller driver
> > + *				  instance
> > + * @chip:		NAND chip information structure
> > + * @dev:		Parent device (used to print error messages)
> > + * @regs:		Virtual address of the NAND flash device
> > + * @buf_addr:		Virtual address of the NAND flash device for
> > + *			data read/writes
> > + * @addr_cycles:	Address cycles
> > + * @mclk:		Memory controller clock
> > + * @buswidth:		Bus width 8 or 16
> > + */
> > +struct pl353_nand_controller {
> > +	struct nand_controller controller;
> > +	struct nand_chip chip;
> > +	struct device *dev;
> > +	void __iomem *regs;
> > +	void __iomem *buf_addr;
> 
> I find the use of buf_addr unfortunate. It is consumed by two functions
> pl353_nand_read_data_op and pl353_nand_write_data_op. All other functions update its
> value. Semantically, its value is regs + some flags. For the updaters that means a complex logic
> where they first have to subtract reg, then change flags and add reg again. To make matters
> worse, this computation involves __force casts between long and __iomem (which yielded
> complaints in earlier reviews).  I think it would simplify the code if you replaced buf_addr with
> something like addr_flags and then compute buf_addr as regs + addr_flags in those two
> consumers. What do you think?
This definitely simplifies the driver logic, we have to do that.
I tried it previously, regarding __force and __iomem casting changes, but the driver functionality was broken
With this update.
Let me update it and check it again.
But just wanted to know, do you see issues with these __force and __iomem castings?
> 
> > +	u8 addr_cycles;
> > +	struct clk *mclk;
> 
> All you need here is the memory clock frequency. Wouldn't it be easier to extract that
> frequency once during probe and store it here? That assumes a constant frequency, but if the
> frequency isn't constant, you have a race condition.
That is what we are doing in the probe.
In the probe, we are getting mclk using of_clk_get() and then we are getting the actual frequency
Using clk_get_rate().
And this is constant frequency only(getting from dts)
> 
> > +	u32 buswidth;
> > +};
> > +
> > +static inline struct pl353_nand_controller *
> > +			to_pl353_nand(struct nand_chip *chip) {
> > +	return container_of(chip, struct pl353_nand_controller, chip); }
> > +
> > +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 * chip->ecc.bytes);
> > +	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 * chip->ecc.bytes) + 8;
> > +	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;
> 
> We already know that section == 0 here. This second condition can only be met if chip-
> >ecc.steps < 0. Is that really what you want to test here?
Yes, ecc.steps checking is not needed. I will remove it.
> 
> > +
> > +	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 u8 bbt_pattern[] = { 'B', 'b', 't', '0' }; static u8
> > +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
> 
> I have a general question concerning the nand framework here. The pattern member in struct
> nand_bbt_descr is not declared const.
> Therefore, bbt_pattern cannot be const here. As far as I looked, all accesses of pattern use it
> with memcmp or as source for memcpy. Also the diskonchip.c driver assigns a string constant
> here. This suggests, that pattern should be declared const or that diskonchip.c is doing it
> wrong.
May be Miquel can comment on this.
> 
> > +};
> > +
> > +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
> > +};
> > +
> > +static void pl353_nfc_force_byte_access(struct nand_chip *chip,
> > +					bool force_8bit)
> > +{
> > +	int ret;
> > +	struct pl353_nand_controller *xnfc =
> > +		container_of(chip, struct pl353_nand_controller, chip);
> > +
> > +	if (xnfc->buswidth == 8)
> 
> This buswidth member is never assigned anywhere. Thus the value is always 0 and this
> comparison always fails.
No, in the probe we should update this, by reading it from dts.
Unfortunately, the assignment was removed, during checkpatch clean up(its my editor issue).
I will update it. 
> 
> > +		return;
> > +
> > +	if (force_8bit)
> > +		ret = pl353_smc_set_buswidth(PL353_SMC_MEM_WIDTH_8);
> > +	else
> > +		ret = pl353_smc_set_buswidth(PL353_SMC_MEM_WIDTH_16);
> > +
> > +	if (ret)
> > +		dev_err(xnfc->dev, "Error in Buswidth\n"); }
> > +
> > +static inline int pl353_wait_for_dev_ready(struct nand_chip *chip) {
> > +	unsigned long timeout = jiffies + PL353_NAND_DEV_BUSY_TIMEOUT;
> > +
> > +	do {
> > +		if (pl353_smc_get_nand_int_status_raw()) {
> > +			pl353_smc_clr_nand_int();
> > +			break;
> > +
> > +		cpu_relax();
> > +	} while (!time_after_eq(jiffies, timeout));
> > +
> > +	if (time_after_eq(jiffies, timeout)) {
> > +		pr_err("%s timed out\n", __func__);
> > +		return -ETIMEDOUT;
> > +	}
> 
> This could be simplified and avoid repeating the timeout condition:
> 
> 	while (!pl353_smc_get_nand_int_status_raw()) {
> 		if (time_after_eq(jiffies, timeout)) {
> 			pr_err("%s timed out\n", __func__);
> 			return -ETIMEDOUT;
> 		}
> 		cpu_relax();
> 	}
> 	pl353_smc_clr_nand_int();
Ok, I will update like this. With this we can avoid repeating timeout condition.
> 
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * pl353_nand_read_data_op - 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
> > + * @force_8bit:	Force 8-bit bus access
> > + * Return:	Always return zero
> > + */
> > +static void pl353_nand_read_data_op(struct nand_chip *chip, u8 *in,
> > +				    unsigned int len, bool force_8bit) {
> > +	int i;
> > +	struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
> > +
> > +	if (force_8bit)
> > +		pl353_nfc_force_byte_access(chip, true);
> > +
> > +	if ((IS_ALIGNED((uint32_t)in, sizeof(uint32_t)) &&
> > +	     IS_ALIGNED(len, sizeof(uint32_t))) || !force_8bit) {
> > +		u32 *ptr = (u32 *)in;
> > +
> > +		len /= 4;
> > +		for (i = 0; i < len; i++)
> > +			ptr[i] = readl(xnfc->buf_addr);
> > +	} else {
> > +		for (i = 0; i < len; i++)
> > +			in[i] = readb(xnfc->buf_addr);
> > +	}
> > +
> > +	if (force_8bit)
> > +		pl353_nfc_force_byte_access(chip, false); }
> > +
> > +/**
> > + * pl353_nand_write_buf - write buffer to chip
> > + * @mtd:	Pointer to the mtd info structure
> > + * @buf:	Pointer to the buffer to store write data
> > + * @len:	Number of bytes to write
> > + * @force_8bit:	Force 8-bit bus access
> > + */
> > +static void pl353_nand_write_data_op(struct nand_chip *chip, const u8 *buf,
> > +				     int len, bool force_8bit)
> > +{
> > +	int i;
> > +	struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
> > +
> > +	if (force_8bit)
> > +		pl353_nfc_force_byte_access(chip, true);
> > +
> > +	if ((IS_ALIGNED((uint32_t)buf, sizeof(uint32_t)) &&
> > +	     IS_ALIGNED(len, sizeof(uint32_t))) || !force_8bit) {
> > +		u32 *ptr = (u32 *)buf;
> > +
> > +		len /= 4;
> > +		for (i = 0; i < len; i++)
> > +			writel(ptr[i], xnfc->buf_addr);
> > +	} else {
> > +		for (i = 0; i < len; i++)
> > +			writeb(buf[i], xnfc->buf_addr);
> > +	}
> > +
> > +	if (force_8bit)
> > +		pl353_nfc_force_byte_access(chip, false); }
> > +
> > +static inline int pl353_wait_for_ecc_done(void) {
> > +	unsigned long timeout = jiffies + PL353_NAND_ECC_BUSY_TIMEOUT;
> > +
> > +	do {
> > +		if (pl353_smc_ecc_is_busy())
> > +			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;
> > +	}
> 
> This could be simplified and avoid repeating the timeout condition:
> 
> 	while (pl353_smc_ecc_is_busy()) {
> 		if (time_after_eq(jiffies, timeout)) {
> 			pr_err("%s timed out\n", __func__);
> 			return -ETIMEDOUT;
> 		}
> 		cpu_relax();
> 	}
Sure. I will update it.
> 
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * pl353_nand_calculate_hwecc - Calculate Hardware ECC
> > + * @mtd:	Pointer to the mtd_info structure
> > + * @data:	Pointer to the page data
> > + * @ecc:	Pointer to the ECC buffer where ECC data needs to be stored
> > + *
> > + * This function retrieves the Hardware ECC data from the controller
> > +and returns
> > + * ECC data back to the MTD subsystem.
> > + * It operates on a number of 512 byte blocks of NAND memory and can
> > +be
> > + * programmed to store the ECC codes after the data in memory. For
> > +writes,
> > + * the ECC is written to the spare area of the page. For reads, the
> > +result of
> > + * a block ECC check are made available to the device driver.
> > + *
> > + * ------------------------------------------------------------------------
> > + * |               n * 512 blocks                  | extra  | ecc    |     |
> > + * |                                               | block  | codes  |     |
> > + *
> > +---------------------------------------------------------------------
> > +---
> > + *
> > + * The ECC calculation uses a simple Hamming code, using 1-bit
> > +correction 2-bit
> > + * detection. It starts when a valid read or write command with a 512
> > +byte
> > + * aligned address is detected on the memory interface.
> > + *
> > + * Return:	0 on success or error value on failure
> > + */
> > +static int pl353_nand_calculate_hwecc(struct nand_chip *chip,
> > +				      const u8 *data, u8 *ecc)
> > +{
> > +	u32 ecc_value;
> > +	u8 chunk, ecc_byte, ecc_status;
> > +
> > +	for (chunk = 0; chunk < PL353_MAX_ECC_CHUNKS; chunk++) {
> > +		/* Read ECC value for each block */
> > +		ecc_value = pl353_smc_get_ecc_val(chunk);
> > +		ecc_status = (ecc_value >> PL353_NAND_ECC_VALID_SHIFT);
> > +
> > +		/* ECC value valid */
> > +		if (ecc_status & PL353_NAND_ECC_VALID_MASK) {
> > +			for (ecc_byte = 0; ecc_byte < PL353_MAX_ECC_BYTES;
> > +			     ecc_byte++) {
> > +				/* Copy ECC bytes to MTD buffer */
> > +				*ecc = ~ecc_value & 0xFF;
> > +				ecc_value = ecc_value >> 8;
> > +				ecc++;
> > +			}
> > +		} else {
> > +			pr_warn("%s status failed\n", __func__);
> > +			return -1;
> > +		}
> > +	}
> > +
> > +	return 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 uncorrectable ECC errors found.
> > + */
> > +static int pl353_nand_correct_data(struct nand_chip *chip, 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)) &
> > +			  PL353_ECC_BIT_MASK;
> > +	read_ecc_upper = ((read_ecc[1] >> 4) | (read_ecc[2] << 4)) &
> > +			  PL353_ECC_BIT_MASK;
> > +
> > +	calc_ecc_lower = (calc_ecc[0] | (calc_ecc[1] << 8)) &
> > +			  PL353_ECC_BIT_MASK;
> > +	calc_ecc_upper = ((calc_ecc[1] >> 4) | (calc_ecc[2] << 4)) &
> > +			  PL353_ECC_BIT_MASK;
> > +
> > +	ecc_odd = read_ecc_lower ^ calc_ecc_lower;
> > +	ecc_even = read_ecc_upper ^ calc_ecc_upper;
> > +
> > +	/* no error */
> > +	if (!ecc_odd && !ecc_even)
> > +		return 0;
> > +
> > +	if (ecc_odd == (~ecc_even & PL353_ECC_BIT_MASK)) {
> > +		/* bits [11:3] of error code is byte offset */
> > +		byte_addr = (ecc_odd >> 3) & PL353_ECC_BITS_BYTEOFF_MASK;
> > +		/* bits [2:0] of error code is bit offset */
> > +		bit_addr = ecc_odd & PL353_ECC_BITS_BITOFF_MASK;
> > +		/* Toggling error bit */
> > +		buf[byte_addr] ^= (BIT(bit_addr));
> > +		return 1;
> > +	}
> > +
> > +	/* one error in parity */
> > +	if (hweight32(ecc_odd | ecc_even) == 1)
> > +		return 1;
> > +
> > +	/* Uncorrectable error */
> > +	return -1;
> > +}
> > +
> > +static void pl353_prepare_cmd(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;
> > +	unsigned long cmd_phase_addr = 0, cmd_phase_data = 0;
> > +	struct mtd_info *mtd = nand_to_mtd(chip);
> > +
> > +	struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
> > +
> > +	end_cmd_valid = read ? 1 : 0;
> > +
> > +	cmd_phase_addr = (unsigned long __force)xnfc->regs +
> > +			 ((xnfc->addr_cycles
> > +			 << ADDR_CYCLES_SHIFT) |
> > +			 (end_cmd_valid << END_CMD_VALID_SHIFT) |
> > +			 (COMMAND_PHASE) |
> > +			 (end_cmd << END_CMD_SHIFT) |
> > +			 (start_cmd << START_CMD_SHIFT));
> > +
> > +	/* Get the data phase address */
> > +	data_phase_addr = (unsigned long __force)xnfc->regs +
> > +			  ((0x0 << CLEAR_CS_SHIFT) |
> > +			  (0 << END_CMD_VALID_SHIFT) |
> > +			  (DATA_PHASE) |
> > +			  (end_cmd << END_CMD_SHIFT) |
> > +			  (0x0 << ECC_LAST_SHIFT));
> > +
> > +	xnfc->buf_addr = (void __iomem * __force)data_phase_addr;
> > +
> > +	if (chip->options & NAND_BUSWIDTH_16)
> > +		column /= 2;
> > +	cmd_phase_data = column;
> > +	if (mtd->writesize > PL353_NAND_ECC_SIZE) {
> > +		cmd_phase_data |= page << 16;
> > +		/* Another address cycle for devices > 128MiB */
> > +		if (chip->options & NAND_ROW_ADDR_3) {
> > +			writel_relaxed(cmd_phase_data,
> > +				       (void __iomem * __force)cmd_phase_addr);
> > +			cmd_phase_data = (page >> 16);
> > +		}
> > +	} else {
> > +		cmd_phase_data |= page << 8;
> > +	}
> > +
> > +	writel_relaxed(cmd_phase_data, (void __iomem *
> > +__force)cmd_phase_addr); }
> > +
> > +/**
> > + * 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 structure
> > + * @page:	Page number to read
> > + *
> > + * Return:	Always return zero
> > + */
> > +static int pl353_nand_read_oob(struct nand_chip *chip,
> > +			       int page)
> > +{
> > +	struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
> > +	struct mtd_info *mtd = nand_to_mtd(chip);
> > +	unsigned long data_phase_addr;
> > +	unsigned long nand_offset = (unsigned long __force)xnfc->regs;
> > +	u8 *p;
> > +
> > +	chip->pagebuf = -1;
> > +	if (mtd->writesize < PL353_NAND_ECC_SIZE)
> > +		return 0;
> > +
> > +	pl353_prepare_cmd(chip, page, mtd->writesize, NAND_CMD_READ0,
> > +			  NAND_CMD_READSTART, 1);
> > +	if (pl353_wait_for_dev_ready(chip))
> > +		return -ETIMEDOUT;
> > +
> > +	p = chip->oob_poi;
> > +	pl353_nand_read_data_op(chip, p,
> > +				(mtd->oobsize -
> > +				PL353_NAND_LAST_TRANSFER_LENGTH), false);
> > +	p += (mtd->oobsize - PL353_NAND_LAST_TRANSFER_LENGTH);
> > +	data_phase_addr = (unsigned long __force)xnfc->buf_addr;
> > +	data_phase_addr -= nand_offset;
> > +	data_phase_addr |= PL353_NAND_CLEAR_CS;
> > +	data_phase_addr += nand_offset;
> > +	xnfc->buf_addr = (void __iomem * __force)data_phase_addr;
> > +	pl353_nand_read_data_op(chip, p, PL353_NAND_LAST_TRANSFER_LENGTH,
> > +				false);
> > +
> > +	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
> > + */
> > +static int pl353_nand_write_oob(struct nand_chip *chip,
> > +				int page)
> > +{
> > +	struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
> > +	struct mtd_info *mtd = nand_to_mtd(chip);
> > +	unsigned long nand_offset = (unsigned long __force)xnfc->regs;
> > +	unsigned long data_phase_addr;
> > +	const u8 *buf = chip->oob_poi;
> > +
> > +	chip->pagebuf = -1;
> > +	pl353_prepare_cmd(chip, page, mtd->writesize, NAND_CMD_SEQIN,
> > +			  NAND_CMD_PAGEPROG, 0);
> > +
> > +	pl353_nand_write_data_op(chip, buf,
> > +				 (mtd->oobsize -
> > +				 PL353_NAND_LAST_TRANSFER_LENGTH), false);
> > +	buf += (mtd->oobsize - PL353_NAND_LAST_TRANSFER_LENGTH);
> > +
> > +	data_phase_addr = (unsigned long __force)xnfc->buf_addr;
> > +	data_phase_addr -= nand_offset;
> > +	data_phase_addr |= PL353_NAND_CLEAR_CS;
> > +	data_phase_addr |= (1 << END_CMD_VALID_SHIFT);
> > +	data_phase_addr += nand_offset;
> > +	xnfc->buf_addr = (void __iomem * __force)data_phase_addr;
> > +	pl353_nand_write_data_op(chip, buf, PL353_NAND_LAST_TRANSFER_LENGTH,
> > +				 false);
> > +	if (pl353_wait_for_dev_ready(chip))
> > +		return -ETIMEDOUT;
> > +
> > +	return 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 nand_chip *chip,
> > +				    u8 *buf, int oob_required, int page) {
> > +	struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
> > +	struct mtd_info *mtd = nand_to_mtd(chip);
> > +	unsigned long nand_offset = (unsigned long __force)xnfc->regs;
> > +	unsigned long data_phase_addr;
> > +	u8 *p;
> > +
> > +	pl353_prepare_cmd(chip, page, 0, NAND_CMD_READ0,
> > +			  NAND_CMD_READSTART, 1);
> > +	if (pl353_wait_for_dev_ready(chip))
> > +		return -ETIMEDOUT;
> > +
> > +	pl353_nand_read_data_op(chip, buf, mtd->writesize, false);
> > +	p = chip->oob_poi;
> > +	pl353_nand_read_data_op(chip, p,
> > +				(mtd->oobsize -
> > +				PL353_NAND_LAST_TRANSFER_LENGTH), false);
> > +	p += (mtd->oobsize - PL353_NAND_LAST_TRANSFER_LENGTH);
> > +
> > +	data_phase_addr = (unsigned long __force)xnfc->buf_addr;
> > +	data_phase_addr -= nand_offset;
> > +	data_phase_addr |= PL353_NAND_CLEAR_CS;
> > +	data_phase_addr += nand_offset;
> > +	xnfc->buf_addr = (void __iomem * __force)data_phase_addr;
> > +
> > +	pl353_nand_read_data_op(chip, p, PL353_NAND_LAST_TRANSFER_LENGTH,
> > +				false);
> > +
> > +	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 nand_chip *chip,
> > +				     const u8 *buf, int oob_required,
> > +				     int page)
> > +{
> > +	struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
> > +	struct mtd_info *mtd = nand_to_mtd(chip);
> > +	unsigned long nand_offset = (unsigned long __force)xnfc->regs;
> > +	unsigned long data_phase_addr;
> > +	u8 *p;
> > +
> > +	pl353_prepare_cmd(chip, page, 0, NAND_CMD_SEQIN,
> > +			  NAND_CMD_PAGEPROG, 0);
> > +	pl353_nand_write_data_op(chip, buf, mtd->writesize, false);
> > +	p = chip->oob_poi;
> > +	pl353_nand_write_data_op(chip, p,
> > +				 (mtd->oobsize -
> > +				 PL353_NAND_LAST_TRANSFER_LENGTH), false);
> > +	p += (mtd->oobsize - PL353_NAND_LAST_TRANSFER_LENGTH);
> > +
> > +	data_phase_addr = (unsigned long __force)xnfc->buf_addr;
> > +	data_phase_addr -= nand_offset;
> > +	data_phase_addr |= PL353_NAND_CLEAR_CS;
> > +	data_phase_addr |= (1 << END_CMD_VALID_SHIFT);
> > +	data_phase_addr += nand_offset;
> > +	xnfc->buf_addr = (void __iomem * __force)data_phase_addr;
> > +	pl353_nand_write_data_op(chip, p, PL353_NAND_LAST_TRANSFER_LENGTH,
> > +				 false);
> > +
> > +	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 nand_chip *chip,
> > +				       const u8 *buf, int oob_required,
> > +				       int page)
> > +{
> > +	int eccsize = chip->ecc.size;
> > +	int eccsteps = chip->ecc.steps;
> > +	u8 *ecc_calc = chip->ecc.calc_buf;
> > +	u8 *oob_ptr;
> > +	const u8 *p = buf;
> > +	u32 ret;
> > +	struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
> > +	struct mtd_info *mtd = nand_to_mtd(chip);
> > +	unsigned long nand_offset = (unsigned long __force)xnfc->regs;
> > +	unsigned long data_phase_addr;
> > +
> > +	pl353_prepare_cmd(chip, page, 0, NAND_CMD_SEQIN,
> > +			  NAND_CMD_PAGEPROG, 0);
> > +
> > +	for ( ; (eccsteps - 1); eccsteps--) {
> > +		pl353_nand_write_data_op(chip, p, eccsize, false);
> > +		p += eccsize;
> > +	}
> > +	pl353_nand_write_data_op(chip, p,
> > +				 (eccsize - PL353_NAND_LAST_TRANSFER_LENGTH),
> > +				 false);
> > +	p += (eccsize - PL353_NAND_LAST_TRANSFER_LENGTH);
> > +
> > +	/* Set ECC Last bit to 1 */
> > +	data_phase_addr = (unsigned long __force)xnfc->buf_addr;
> > +	data_phase_addr -= nand_offset;
> > +	data_phase_addr |= PL353_NAND_ECC_LAST;
> > +	data_phase_addr += nand_offset;
> > +	xnfc->buf_addr = (void __iomem * __force)data_phase_addr;
> > +	pl353_nand_write_data_op(chip, p, PL353_NAND_LAST_TRANSFER_LENGTH,
> > +				 false);
> > +
> > +	/* Wait till the ECC operation is complete or timeout */
> > +	ret = pl353_wait_for_ecc_done();
> > +	if (ret)
> > +		dev_err(xnfc->dev, "ECC Timeout\n");
> > +	p = buf;
> > +	ret = chip->ecc.calculate(chip, p, &ecc_calc[0]);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* 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)xnfc->buf_addr;
> > +	data_phase_addr -= nand_offset;
> > +	data_phase_addr &= ~PL353_NAND_ECC_LAST;
> > +	data_phase_addr += nand_offset;
> > +	xnfc->buf_addr = (void __iomem * __force)data_phase_addr;
> > +
> > +	/* Write the spare area with ECC bytes */
> > +	oob_ptr = chip->oob_poi;
> > +	pl353_nand_write_data_op(chip, oob_ptr,
> > +				 (mtd->oobsize -
> > +				 PL353_NAND_LAST_TRANSFER_LENGTH), false);
> > +
> > +	data_phase_addr = (unsigned long __force)xnfc->buf_addr;
> > +	data_phase_addr -= nand_offset;
> > +	data_phase_addr |= PL353_NAND_CLEAR_CS;
> > +	data_phase_addr |= (1 << END_CMD_VALID_SHIFT);
> > +	data_phase_addr += nand_offset;
> > +	xnfc->buf_addr = (void __iomem * __force)data_phase_addr;
> > +	oob_ptr += (mtd->oobsize - PL353_NAND_LAST_TRANSFER_LENGTH);
> > +	pl353_nand_write_data_op(chip, oob_ptr,
> PL353_NAND_LAST_TRANSFER_LENGTH,
> > +				 false);
> > +	if (pl353_wait_for_dev_ready(chip))
> > +		return -ETIMEDOUT;
> > +
> > +	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.
> > + * There is a limitation in SMC controller, that we must set ECC LAST
> > +on
> > + * last data phase access, to tell ECC block not to expect any data further.
> > + * Ex:  When number of ECC STEPS are 4, then till 3 we will write to
> > +flash
> > + * using SMC with HW ECC enabled. And for the last ECC STEP, we will
> > +subtract
> > + * 4bytes from page size, and will initiate a transfer. And the
> > +remaining 4 as
> > + * one more transfer with ECC_LAST bit set in NAND data phase
> > +register to
> > + * notify ECC block not to expect any more data. The last block
> > +should be align
> > + * with end of 512 byte block. Because of this limitation, we are not
> > +using
> > + * core routines.
> > + *
> > + * Return:	0 always and updates ECC operation status in to MTD structure
> > + */
> > +static int pl353_nand_read_page_hwecc(struct nand_chip *chip,
> > +				      u8 *buf, int oob_required, int page) {
> > +	int i, stat, eccsize = chip->ecc.size;
> > +	int eccbytes = chip->ecc.bytes;
> > +	int eccsteps = chip->ecc.steps;
> > +	u8 *p = buf;
> > +	u8 *ecc_calc = chip->ecc.calc_buf;
> > +	u8 *ecc = chip->ecc.code_buf;
> > +	unsigned int max_bitflips = 0;
> > +	u8 *oob_ptr;
> > +	u32 ret;
> > +	unsigned long data_phase_addr;
> > +	unsigned long nand_offset = (unsigned long __force)xnfc->regs;
> > +	struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
> > +	struct mtd_info *mtd = nand_to_mtd(chip);
> > +
> > +	pl353_prepare_cmd(chip, page, 0, NAND_CMD_READ0,
> > +			  NAND_CMD_READSTART, 1);
> > +	if (pl353_wait_for_dev_ready(chip))
> > +		return -ETIMEDOUT;
> > +
> > +	for ( ; (eccsteps - 1); eccsteps--) {
> > +		pl353_nand_read_data_op(chip, p, eccsize, false);
> > +		p += eccsize;
> > +	}
> > +
> > +	pl353_nand_read_data_op(chip, p,
> > +				(eccsize - PL353_NAND_LAST_TRANSFER_LENGTH),
> > +				false);
> > +	p += (eccsize - PL353_NAND_LAST_TRANSFER_LENGTH);
> > +
> > +	/* Set ECC Last bit to 1 */
> > +	data_phase_addr = (unsigned long __force)xnfc->buf_addr;
> > +	data_phase_addr -= nand_offset;
> > +	data_phase_addr |= PL353_NAND_ECC_LAST;
> > +	data_phase_addr += nand_offset;
> > +	xnfc->buf_addr = (void __iomem * __force)data_phase_addr;
> > +	pl353_nand_read_data_op(chip, p, PL353_NAND_LAST_TRANSFER_LENGTH,
> > +				false);
> > +
> > +	/* Wait till the ECC operation is complete or timeout */
> > +	ret = pl353_wait_for_ecc_done();
> > +	if (ret)
> > +		dev_err(xnfc->dev, "ECC Timeout\n");
> > +
> > +	/* Read the calculated ECC value */
> > +	p = buf;
> > +	ret = chip->ecc.calculate(chip, p, &ecc_calc[0]);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Clear ECC last bit */
> > +	data_phase_addr = (unsigned long __force)xnfc->buf_addr;
> > +	data_phase_addr -= nand_offset;
> > +	data_phase_addr &= ~PL353_NAND_ECC_LAST;
> > +	data_phase_addr += nand_offset;
> > +	xnfc->buf_addr = (void __iomem * __force)data_phase_addr;
> > +
> > +	/* Read the stored ECC value */
> > +	oob_ptr = chip->oob_poi;
> > +	pl353_nand_read_data_op(chip, oob_ptr,
> > +				(mtd->oobsize -
> > +				PL353_NAND_LAST_TRANSFER_LENGTH), false);
> > +
> > +	/* de-assert chip select */
> > +	data_phase_addr = (unsigned long __force)xnfc->buf_addr;
> > +	data_phase_addr -= nand_offset;
> > +	data_phase_addr |= PL353_NAND_CLEAR_CS;
> > +	data_phase_addr += nand_offset;
> > +	xnfc->buf_addr = (void __iomem * __force)data_phase_addr;
> > +
> > +	oob_ptr += (mtd->oobsize - PL353_NAND_LAST_TRANSFER_LENGTH);
> > +	pl353_nand_read_data_op(chip, oob_ptr,
> PL353_NAND_LAST_TRANSFER_LENGTH,
> > +				false);
> > +
> > +	ret = mtd_ooblayout_get_eccbytes(mtd, ecc, 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) {
> > +		stat = chip->ecc.correct(chip, p, &ecc[i], &ecc_calc[i]);
> > +		if (stat < 0) {
> > +			mtd->ecc_stats.failed++;
> > +		} else {
> > +			mtd->ecc_stats.corrected += stat;
> > +			max_bitflips = max_t(unsigned int, max_bitflips, stat);
> > +		}
> > +	}
> > +
> > +	return max_bitflips;
> > +}
> > +
> > +/* 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++) {
> > +		instr = &subop->instrs[op_id];
> > +
> > +		switch (instr->type) {
> > +		case NAND_OP_CMD_INSTR:
> > +			if (op_id)
> > +				nfc_op->cmnds[1] = instr->ctx.cmd.opcode;
> > +			else
> > +				nfc_op->cmnds[0] = instr->ctx.cmd.opcode;
> > +			nfc_op->cle_ale_delay_ns = instr->delay_ns;
> > +			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] <<
> 
> I don't quite understand what this code does, but it looks strange to me. I compared it to other
> drivers. The code here is quite similar to marvell_nand.c. It seems like we are copying a
> varying number (0 to 6) of addresses from the buffer instr->ctx.addr.addrs. However their
> indices are special: 0, 1, 2, 3, offset + 4, offset + 5. This is non-consecutive and different from
> marvell_nand.c in this regard. Could it be that you really meant index offset+i here?
I didn't get, what you are saying here.
It is about updating page and column addresses.
Are you asking me to remove nfc_op->addrs = instr->ctx.addr.addrs[offset]; before for loop?

> 
> > +						 (8 * i);
> > +			}
> > +
> > +			if (naddrs >= 5)
> > +				nfc_op->addr5 = addrs[4];
> > +			if (naddrs >= 6)
> > +				nfc_op->addr6 = addrs[5];
> > +			nfc_op->naddrs = nand_subop_get_num_addr_cyc(subop,
> > +								     op_id);
> > +			nfc_op->cle_ale_delay_ns = instr->delay_ns;
> > +			break;
> > +
> > +		case NAND_OP_DATA_IN_INSTR:
> > +			nfc_op->data_instr = instr;
> > +			nfc_op->data_instr_idx = op_id;
> > +			break;
> > +
> > +		case NAND_OP_DATA_OUT_INSTR:
> > +			nfc_op->data_instr = instr;
> > +			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;
> > +			break;
> > +		}
> > +	}
> > +}
> > +
> > +static void cond_delay(unsigned int ns) {
> > +	if (!ns)
> > +		return;
> > +
> > +	if (ns < 10000)
> > +		ndelay(ns);
> > +	else
> > +		udelay(DIV_ROUND_UP(ns, 1000));
> > +}
> 
> This function has an exact copy in marvell_nand.c. Would it make sense to move it to a more
> central place? There are only two copies yet.
I will check and update.
> 
> Note that on arm (the primary target of this driver), ndelay is implemented using udelay.
Ok
> 
> > +/**
> > + * pl353_nand_exec_op_cmd - 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_exec_op_cmd(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 = {};
> > +	struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
> > +	unsigned long cmd_phase_data = 0, end_cmd_valid = 0;
> > +	unsigned long cmd_phase_addr, data_phase_addr, end_cmd;
> > +	unsigned int op_id, len;
> > +	bool reading;
> > +
> > +	pl353_nfc_parse_instructions(chip, subop, &nfc_op);
> > +	instr = nfc_op.data_instr;
> > +	op_id = nfc_op.data_instr_idx;
> > +
> > +	pl353_smc_clr_nand_int();
> > +	/* Get the command phase address */
> > +	if (nfc_op.cmnds[1] != 0) {
> > +		if (nfc_op.cmnds[0] == NAND_CMD_SEQIN)
> > +			end_cmd_valid = 0;
> > +		else
> > +			end_cmd_valid = 1;
> > +		end_cmd = nfc_op.cmnds[1];
> > +	}  else {
> > +		end_cmd = 0x0;
> 
> In this branch, nfc_op.cmnds[1] == 0, so end_cmd is always nfc_op.cmnds[1]. Would it make
> sense to pull the assignment out of the branch?
Yes, it make sense. I will update it.
> 
> > +	}
> > +
> > +	/*
> > +	 * The SMC defines two phases of commands when transferring data to or
> > +	 * from NAND flash.
> > +	 * Command phase: Commands and optional address information are written
> > +	 * to the NAND flash.The command and address can be associated with
> > +	 * either a data phase operation to write to or read from the array,
> > +	 * or a status/ID register transfer.
> > +	 * Data phase: Data is either written to or read from the NAND flash.
> > +	 * This data can be either data transferred to or from the array,
> > +	 * or status/ID register information.
> > +	 */
> > +	cmd_phase_addr = (unsigned long __force)xnfc->regs +
> > +			 ((nfc_op.naddrs << ADDR_CYCLES_SHIFT) |
> > +			 (end_cmd_valid << END_CMD_VALID_SHIFT) |
> > +			 (COMMAND_PHASE) |
> > +			 (end_cmd << END_CMD_SHIFT) |
> > +			 (nfc_op.cmnds[0] << START_CMD_SHIFT));
> > +
> > +	/* Get the data phase address */
> > +	end_cmd_valid = 0;
> > +
> > +	data_phase_addr = (unsigned long __force)xnfc->regs +
> > +			  ((0x0 << CLEAR_CS_SHIFT) |
> > +			  (end_cmd_valid << END_CMD_VALID_SHIFT) |
> > +			  (DATA_PHASE) |
> > +			  (end_cmd << END_CMD_SHIFT) |
> > +			  (0x0 << ECC_LAST_SHIFT));
> > +	xnfc->buf_addr = (void __iomem * __force)data_phase_addr;
> > +
> > +	/* Command phase AXI Read & Write */
> > +	if (nfc_op.naddrs >= 5) {
> > +		if (mtd->writesize > PL353_NAND_ECC_SIZE) {
> > +			cmd_phase_data = nfc_op.addrs;
> > +			/* Another address cycle for devices > 128MiB */
> > +			if (chip->options & NAND_ROW_ADDR_3) {
> > +				writel_relaxed(cmd_phase_data,
> > +					       (void __iomem * __force)
> > +					       cmd_phase_addr);
> > +				cmd_phase_data = nfc_op.addr5;
> > +				if (nfc_op.naddrs >= 6)
> > +					cmd_phase_data |= (nfc_op.addr6 << 8);
> > +			}
> > +		}
> > +	}  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)) {
> > +				column >>= 1;
> > +			}
> > +			cmd_phase_data = column;
> > +		}
> > +	}
> > +
> > +	writel_relaxed(cmd_phase_data, (void __iomem * __force)cmd_phase_addr);
> > +	if (!nfc_op.data_instr) {
> > +		if (nfc_op.rdy_timeout_ms) {
> > +			if (pl353_wait_for_dev_ready(chip))
> > +				return -ETIMEDOUT;
> > +		}
> > +
> > +		return 0;
> > +	}
> > +
> > +	reading = (nfc_op.data_instr->type == NAND_OP_DATA_IN_INSTR);
> > +	if (!reading) {
> > +		len = nand_subop_get_data_len(subop, op_id);
> > +		pl353_nand_write_data_op(chip, instr->ctx.data.buf.out,
> > +					 len, instr->ctx.data.force_8bit);
> > +		if (nfc_op.rdy_timeout_ms) {
> > +			if (pl353_wait_for_dev_ready(chip))
> > +				return -ETIMEDOUT;
> > +		}
> > +
> > +		cond_delay(nfc_op.rdy_delay_ns);
> > +	}
> > +
> > +	if (reading) {
> 
> You could use an else branch instead of inverting the condition here.
> When Miquel complained about this in v13, you said you'd change it, but you didn't.
Sorry for that. I will change it.
> 
> > +		len = nand_subop_get_data_len(subop, op_id);
> > +		cond_delay(nfc_op.rdy_delay_ns);
> > +		if (nfc_op.rdy_timeout_ms) {
> > +			if (pl353_wait_for_dev_ready(chip))
> > +				return -ETIMEDOUT;
> > +		}
> > +
> > +		pl353_nand_read_data_op(chip, instr->ctx.data.buf.in, len,
> > +					instr->ctx.data.force_8bit);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct nand_op_parser pl353_nfc_op_parser = NAND_OP_PARSER
> > +	(NAND_OP_PARSER_PATTERN
> > +		(pl353_nand_exec_op_cmd,
> > +		NAND_OP_PARSER_PAT_CMD_ELEM(true),
> > +		NAND_OP_PARSER_PAT_ADDR_ELEM(true, 7),
> > +		NAND_OP_PARSER_PAT_WAITRDY_ELEM(true),
> > +		NAND_OP_PARSER_PAT_DATA_IN_ELEM(false, 2048)),
> > +	NAND_OP_PARSER_PATTERN
> > +		(pl353_nand_exec_op_cmd,
> > +		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_PAT_DATA_IN_ELEM(false, 2048)),
> > +	NAND_OP_PARSER_PATTERN
> > +		(pl353_nand_exec_op_cmd,
> > +		NAND_OP_PARSER_PAT_CMD_ELEM(false),
> > +		NAND_OP_PARSER_PAT_ADDR_ELEM(true, 7),
> > +		NAND_OP_PARSER_PAT_CMD_ELEM(true),
> > +		NAND_OP_PARSER_PAT_WAITRDY_ELEM(false)),
> > +	NAND_OP_PARSER_PATTERN
> > +		(pl353_nand_exec_op_cmd,
> > +		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_exec_op_cmd,
> > +		NAND_OP_PARSER_PAT_CMD_ELEM(false)),
> > +	);
> > +
> > +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_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
> > + *
> > + * Return:	0 on success or negative errno.
> > + */
> > +static int pl353_nand_ecc_init(struct mtd_info *mtd, struct nand_ecc_ctrl *ecc,
> > +			       int ecc_mode)
> > +{
> > +	struct nand_chip *chip = mtd_to_nand(mtd);
> > +	struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
> > +	int err = 0, ret;
> 
> These variables serve the same purpose. Both err and ret determine the return value of this
> function. Can you merge them into one variable?
Ok, I will merge them.
> 
> > +
> > +	ecc->read_oob = pl353_nand_read_oob;
> > +	ecc->write_oob = pl353_nand_write_oob;
> > +	if (ecc_mode == NAND_ECC_ON_DIE) {
> > +		ecc->write_page_raw = pl353_nand_write_page_raw;
> > +		ecc->read_page_raw = pl353_nand_read_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;
> > +		ret = pl353_smc_set_ecc_mode(PL353_SMC_ECCMODE_BYPASS);
> > +		if (ret)
> > +			return ret;
> > +
> > +	} 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->read_page = pl353_nand_read_page_hwecc;
> > +		ecc->size = PL353_NAND_ECC_SIZE;
> > +		ecc->read_page = pl353_nand_read_page_hwecc;
> > +		ecc->write_page = pl353_nand_write_page_hwecc;
> > +		pl353_smc_set_ecc_pg_size(mtd->writesize);
> > +		switch (mtd->writesize) {
> > +		case SZ_512:
> > +		case SZ_1K:
> > +		case SZ_2K:
> > +			pl353_smc_set_ecc_mode(PL353_SMC_ECCMODE_APB);
> > +			break;
> > +		default:
> > +			ecc->calculate = nand_calculate_ecc;
> > +			ecc->correct = nand_correct_data;
> > +			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);
> > +		} else {
> > +			err = -ENXIO;
> > +			dev_err(xnfc->dev, "Unsupported oob Layout\n");
> > +		}
> > +	}
> > +
> > +	return err;
> > +}
> > +
> > +static int pl353_nfc_setup_data_interface(struct nand_chip *chip, int csline,
> > +					  const struct nand_data_interface
> > +					  *conf)
> > +{
> > +	struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
> > +	const struct nand_sdr_timings *sdr;
> > +	u32 timings[7], mckperiodps;
> > +
> > +	if (csline == NAND_DATA_IFACE_CHECK_ONLY)
> > +		return 0;
> > +
> > +	sdr = nand_get_sdr_timings(conf);
> > +	if (IS_ERR(sdr))
> > +		return PTR_ERR(sdr);
> > +
> > +	/*
> > +	 * SDR timings are given in pico-seconds while NFC timings must be
> > +	 * expressed in NAND controller clock cycles.
> > +	 */
> > +	mckperiodps = NSEC_PER_SEC / clk_get_rate(xnfc->mclk);
> > +	mckperiodps *= 1000;
> > +	if (sdr->tRC_min <= 20000)
> > +		/*
> > +		 * PL353 SMC needs one extra read cycle in SDR Mode 5
> > +		 * This is not written anywhere in the datasheet but
> > +		 * the results observed during testing.
> > +		 */
> > +		timings[0] = DIV_ROUND_UP(sdr->tRC_min, mckperiodps) + 1;
> > +	else
> > +		timings[0] = DIV_ROUND_UP(sdr->tRC_min, mckperiodps);
> > +
> > +	timings[1] = DIV_ROUND_UP(sdr->tWC_min, mckperiodps);
> > +	/*
> > +	 * For all SDR modes, PL353 SMC needs tREA max value as 1,
> > +	 * Results observed during testing.
> > +	 */
> > +	timings[2] = PL353_TREA_MAX_VALUE;
> > +	timings[3] = DIV_ROUND_UP(sdr->tWP_min, mckperiodps);
> > +	timings[4] = DIV_ROUND_UP(sdr->tCLR_min, mckperiodps);
> > +	timings[5] = DIV_ROUND_UP(sdr->tAR_min, mckperiodps);
> > +	timings[6] = DIV_ROUND_UP(sdr->tRR_min, mckperiodps);
> > +	pl353_smc_set_cycles(timings);
> > +
> > +	return 0;
> > +}
> > +
> > +static int pl353_nand_attach_chip(struct nand_chip *chip) {
> > +	struct mtd_info *mtd = nand_to_mtd(chip);
> > +	struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
> > +	int ret;
> > +
> > +	if (chip->options & NAND_BUSWIDTH_16) {
> > +		ret = pl353_smc_set_buswidth(PL353_SMC_MEM_WIDTH_16);
> > +		if (ret) {
> > +			dev_err(xnfc->dev, "Set BusWidth failed\n");
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	if (mtd->writesize <= SZ_512)
> > +		xnfc->addr_cycles = 1;
> > +	else
> > +		xnfc->addr_cycles = 2;
> > +
> > +	if (chip->options & NAND_ROW_ADDR_3)
> > +		xnfc->addr_cycles += 3;
> > +	else
> > +		xnfc->addr_cycles += 2;
> > +
> > +	ret = pl353_nand_ecc_init(mtd, &chip->ecc, chip->ecc.mode);
> > +	if (ret) {
> > +		dev_err(xnfc->dev, "ECC init failed\n");
> > +		return ret;
> > +	}
> > +
> > +	if (!mtd->name) {
> > +		/*
> > +		 * If the new bindings are used and the bootloader has not been
> > +		 * updated to pass a new mtdparts parameter on the cmdline, you
> > +		 * should define the following property in your NAND node, ie:
> > +		 *
> > +		 *	label = "pl353-nand";
> > +		 *
> > +		 * This way, mtd->name will be set by the core when
> > +		 * nand_set_flash_node() is called.
> > +		 */
> > +		mtd->name = devm_kasprintf(xnfc->dev, GFP_KERNEL,
> > +					   "%s", PL353_NAND_DRIVER_NAME);
> > +		if (!mtd->name) {
> > +			dev_err(xnfc->dev, "Failed to allocate mtd->name\n");
> > +			return -ENOMEM;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct nand_controller_ops pl353_nand_controller_ops = {
> > +	.attach_chip = pl353_nand_attach_chip,
> > +	.exec_op = pl353_nfc_exec_op,
> > +	.setup_data_interface = pl353_nfc_setup_data_interface, };
> > +
> > +/**
> > + * 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.
> > + * The NAND driver has dependency with the pl353_smc memory
> > +controller
> > + * driver for initializing the NAND timing parameters, bus width, ECC
> > +modes,
> > + * control and status information.
> > + *
> > + * Return:	0 on success or error value on failure
> > + */
> > +static int pl353_nand_probe(struct platform_device *pdev) {
> > +	struct pl353_nand_controller *xnfc;
> > +	struct mtd_info *mtd;
> > +	struct nand_chip *chip;
> > +	struct resource *res;
> > +	struct device_node *np, *dn;
> > +	u32 ret, val;
> > +
> > +	xnfc = devm_kzalloc(&pdev->dev, sizeof(*xnfc), GFP_KERNEL);
> > +	if (!xnfc)
> > +		return -ENOMEM;
> > +
> > +	xnfc->dev = &pdev->dev;
> > +	nand_controller_init(&xnfc->controller);
> > +	xnfc->controller.ops = &pl353_nand_controller_ops;
> > +	/* Map physical address of NAND flash */
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	xnfc->regs = devm_ioremap_resource(xnfc->dev, res);
> > +	if (IS_ERR(xnfc->regs))
> > +		return PTR_ERR(xnfc->regs);
> > +
> > +	chip = &xnfc->chip;
> > +	chip->controller = &xnfc->controller;
> > +	mtd = nand_to_mtd(chip);
> > +	nand_set_controller_data(chip, xnfc);
> > +	mtd->priv = chip;
> > +	mtd->owner = THIS_MODULE;
> > +	nand_set_flash_node(chip, xnfc->dev->of_node);
> > +
> > +	np = of_get_next_parent(xnfc->dev->of_node);
> > +	xnfc->mclk = of_clk_get(np, 0);
> 
> I think it would be more robust to look up the clock by name rather than index to mirror what
> pl353-smc does:
Ok. will update it.
> 
> 	xnfc->mclk = of_clk_get_by_name(np, "memclk");
> 
> > +	if (IS_ERR(xnfc->mclk)) {
> > +		dev_err(xnfc->dev, "Failed to retrieve MCK clk\n");
> > +		return PTR_ERR(xnfc->mclk);
> > +	}
> > +
> > +	dn = nand_get_flash_node(chip);
> > +	ret = of_property_read_u32(dn, "nand-bus-width", &val);
> > +	if (ret)
> > +		val = 8;
> 
> This val seems to be entirely unused.
As I said above, the below line got deleted because of my editor issue during checkpatch clean up.
It is like below
Xnfc->buswidth = val;
> 
> > +
> > +	/* Set the device option and flash width */
> > +	chip->options = NAND_BUSWIDTH_AUTO;
> > +	chip->bbt_options = NAND_BBT_USE_FLASH;
> > +	platform_set_drvdata(pdev, xnfc);
> > +	ret = nand_scan(chip, 1);
> > +	if (ret) {
> > +		dev_err(xnfc->dev, "could not scan the nand chip\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = mtd_device_register(mtd, NULL, 0);
> > +	if (ret) {
> > +		dev_err(xnfc->dev, "Failed to register mtd device: %d\n", ret);
> > +		nand_cleanup(chip);
> > +		return ret;
> > +	}
> > +
> > +	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_controller *xnfc = platform_get_drvdata(pdev);
> > +	struct mtd_info *mtd = nand_to_mtd(&xnfc->chip);
> > +	struct nand_chip *chip = mtd_to_nand(mtd);
> > +
> > +	/* Release resources, unregister device */
> > +	nand_release(chip);
> > +
> > +	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");
> > --
> > 2.7.4
> >
Thanks for your time. I will address all the comments in the next version and I will mention the reason,
if some of the comments mentioned above are not addressed.

Thanks,
Naga Sureshkumar Relli

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [LINUX PATCH v14] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface

[Helmut Grohne]

On Mon, Apr 29, 2019 at 11:31:14AM +0000, Naga Sureshkumar Relli wrote:
> But just wanted to know, do you see issues with these __force and __iomem castings?

I only see a minor issue: They're (deliberately) lengthy. Using many of
them diverts attention of the reader. Therefore, my proposal attempted
to reduce their frequency. The only issue I see here is readability.

> > 
> > > +	u8 addr_cycles;
> > > +	struct clk *mclk;
> > 
> > All you need here is the memory clock frequency. Wouldn't it be easier to extract that
> > frequency once during probe and store it here? That assumes a constant frequency, but if the
> > frequency isn't constant, you have a race condition.
> That is what we are doing in the probe.
> In the probe, we are getting mclk using of_clk_get() and then we are getting the actual frequency
> Using clk_get_rate().
> And this is constant frequency only(getting from dts)

Not quite. You're getting a clock reference in probe and then repeatedly
access the frequency elswhere. I am suggesting that you get the clock
frequency during probe and never save the clock reference to a struct.

> > > +		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] <<
> > 
> > I don't quite understand what this code does, but it looks strange to me. I compared it to other
> > drivers. The code here is quite similar to marvell_nand.c. It seems like we are copying a
> > varying number (0 to 6) of addresses from the buffer instr->ctx.addr.addrs. However their
> > indices are special: 0, 1, 2, 3, offset + 4, offset + 5. This is non-consecutive and different from
> > marvell_nand.c in this regard. Could it be that you really meant index offset+i here?
> I didn't get, what you are saying here.
> It is about updating page and column addresses.
> Are you asking me to remove nfc_op->addrs = instr->ctx.addr.addrs[offset]; before for loop?

I compared this code to marvell_nand.c and noticed a subtle difference.
Both snippets read 6 address bytes and consume them in a driver-specific
way. Now which address bytes are consumed differs.

marvell_nand.c consumes instr->ctx.addr.addrs at indices offset,
offset+1, offset+2, offset+3, offset+4, offset+5. pl353_nand.c consumes
instr->ctx.addr.addrs at indices 0, 1, 2, 3, offset, offset+4, offset+5.
(In my previous mail, I didn't notice that it was also consuming the
offset index.)

I would have expected this behaviour to be consistent between different
drivers. If I assume marvell_nand.c to do the right thing and
pl353_nand.c to be wrong (which is not necessarily a correct
assumption), then the code woule likely becom:

	addrs = &instr->ctx.addr.addrs[offset];
	for (i = 0; i < min_t(unsigned int, 4, naddrs); i++) {
		nfc_op->addrs |= addrs[i] << (8 * i);
		              // ^^^^^
	}

Hope this helps.

Helmut

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Naga Sureshkumar Relli]

Hi Helmut,

> -----Original Message-----
> From: Helmut Grohne <helmut.grohne@intenta.de>
> Sent: Monday, April 29, 2019 5:48 PM
> To: Naga Sureshkumar Relli <nagasure@xilinx.com>
> Cc: bbrezillon@kernel.org; miquel.raynal@bootlin.com; richard@nod.at;
> dwmw2@infradead.org; computersforpeace@gmail.com; marek.vasut@gmail.com; linux-
> mtd@lists.infradead.org; linux-kernel@vger.kernel.org; Michal Simek <michals@xilinx.com>;
> nagasureshkumarrelli@gmail.com
> Subject: Re: [LINUX PATCH v14] mtd: rawnand: pl353: Add basic driver for arm pl353 smc
> nand interface
> 
> On Mon, Apr 29, 2019 at 11:31:14AM +0000, Naga Sureshkumar Relli wrote:
> > But just wanted to know, do you see issues with these __force and __iomem castings?
> 
> I only see a minor issue: They're (deliberately) lengthy. Using many of them diverts attention
> of the reader. Therefore, my proposal attempted to reduce their frequency. The only issue I see
> here is readability.
Ok then, I will update it.
> 
> > >
> > > > +	u8 addr_cycles;
> > > > +	struct clk *mclk;
> > >
> > > All you need here is the memory clock frequency. Wouldn't it be
> > > easier to extract that frequency once during probe and store it
> > > here? That assumes a constant frequency, but if the frequency isn't constant, you have a
> race condition.
> > That is what we are doing in the probe.
> > In the probe, we are getting mclk using of_clk_get() and then we are
> > getting the actual frequency Using clk_get_rate().
> > And this is constant frequency only(getting from dts)
> 
> Not quite. You're getting a clock reference in probe and then repeatedly access the frequency
> elswhere. I am suggesting that you get the clock frequency during probe and never save the
> clock reference to a struct.
Ok. got it. Will update.
> 
> > > > +		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] <<
> > >
> > > I don't quite understand what this code does, but it looks strange
> > > to me. I compared it to other drivers. The code here is quite
> > > similar to marvell_nand.c. It seems like we are copying a varying
> > > number (0 to 6) of addresses from the buffer instr->ctx.addr.addrs.
> > > However their indices are special: 0, 1, 2, 3, offset + 4, offset + 5. This is non-consecutive
> and different from marvell_nand.c in this regard. Could it be that you really meant index
> offset+i here?
> > I didn't get, what you are saying here.
> > It is about updating page and column addresses.
> > Are you asking me to remove nfc_op->addrs = instr->ctx.addr.addrs[offset]; before for
> loop?
> 
> I compared this code to marvell_nand.c and noticed a subtle difference.
> Both snippets read 6 address bytes and consume them in a driver-specific way. Now which
> address bytes are consumed differs.
> 
> marvell_nand.c consumes instr->ctx.addr.addrs at indices offset,
> offset+1, offset+2, offset+3, offset+4, offset+5. pl353_nand.c consumes
> instr->ctx.addr.addrs at indices 0, 1, 2, 3, offset, offset+4, offset+5.
> (In my previous mail, I didn't notice that it was also consuming the offset index.)
> 
> I would have expected this behaviour to be consistent between different drivers. If I assume
> marvell_nand.c to do the right thing and pl353_nand.c to be wrong (which is not necessarily a
> correct assumption), then the code woule likely becom:
> 
> 	addrs = &instr->ctx.addr.addrs[offset];
> 	for (i = 0; i < min_t(unsigned int, 4, naddrs); i++) {
> 		nfc_op->addrs |= addrs[i] << (8 * i);
> 		              // ^^^^^
> 	}
> 
> Hope this helps.
Ok. let me re check this and I will update this accordingly.

Thanks,
Naga Sureshkumar Relli
> 
> Helmut

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Naga Sureshkumar Relli]

Hi Helmut,

> -----Original Message-----
> From: Helmut Grohne <helmut.grohne@intenta.de>
> Sent: Monday, April 29, 2019 5:48 PM
> To: Naga Sureshkumar Relli <nagasure@xilinx.com>
> Cc: bbrezillon@kernel.org; miquel.raynal@bootlin.com; richard@nod.at;
> dwmw2@infradead.org; computersforpeace@gmail.com; marek.vasut@gmail.com; linux-
> mtd@lists.infradead.org; linux-kernel@vger.kernel.org; Michal Simek <michals@xilinx.com>;
> nagasureshkumarrelli@gmail.com
> Subject: Re: [LINUX PATCH v14] mtd: rawnand: pl353: Add basic driver for arm pl353 smc
> nand interface
> 
> On Mon, Apr 29, 2019 at 11:31:14AM +0000, Naga Sureshkumar Relli wrote:
> > But just wanted to know, do you see issues with these __force and __iomem castings?
> 
> I only see a minor issue: They're (deliberately) lengthy. Using many of them diverts attention
> of the reader. Therefore, my proposal attempted to reduce their frequency. The only issue I see
> here is readability.
> 
> > >
> > > > +	u8 addr_cycles;
> > > > +	struct clk *mclk;
> > >
> > > All you need here is the memory clock frequency. Wouldn't it be
> > > easier to extract that frequency once during probe and store it
> > > here? That assumes a constant frequency, but if the frequency isn't constant, you have a
> race condition.
> > That is what we are doing in the probe.
> > In the probe, we are getting mclk using of_clk_get() and then we are
> > getting the actual frequency Using clk_get_rate().
> > And this is constant frequency only(getting from dts)
> 
> Not quite. You're getting a clock reference in probe and then repeatedly access the frequency
> elswhere. I am suggesting that you get the clock frequency during probe and never save the
> clock reference to a struct.
> 
> > > > +		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] <<
> > >
> > > I don't quite understand what this code does, but it looks strange
> > > to me. I compared it to other drivers. The code here is quite
> > > similar to marvell_nand.c. It seems like we are copying a varying
> > > number (0 to 6) of addresses from the buffer instr->ctx.addr.addrs.
> > > However their indices are special: 0, 1, 2, 3, offset + 4, offset + 5. This is non-consecutive
> and different from marvell_nand.c in this regard. Could it be that you really meant index
> offset+i here?
> > I didn't get, what you are saying here.
> > It is about updating page and column addresses.
> > Are you asking me to remove nfc_op->addrs = instr->ctx.addr.addrs[offset]; before for
> loop?
> 
> I compared this code to marvell_nand.c and noticed a subtle difference.
> Both snippets read 6 address bytes and consume them in a driver-specific way. Now which
> address bytes are consumed differs.
> 
> marvell_nand.c consumes instr->ctx.addr.addrs at indices offset,
> offset+1, offset+2, offset+3, offset+4, offset+5. pl353_nand.c consumes
> instr->ctx.addr.addrs at indices 0, 1, 2, 3, offset, offset+4, offset+5.
> (In my previous mail, I didn't notice that it was also consuming the offset index.)
> 
> I would have expected this behaviour to be consistent between different drivers. If I assume
> marvell_nand.c to do the right thing and pl353_nand.c to be wrong (which is not necessarily a
> correct assumption), then the code woule likely becom:
> 
> 	addrs = &instr->ctx.addr.addrs[offset];
> 	for (i = 0; i < min_t(unsigned int, 4, naddrs); i++) {
> 		nfc_op->addrs |= addrs[i] << (8 * i);
> 		              // ^^^^^
> 	}
> 
> Hope this helps.
I spent much of time to address all your comments.
All are addressed and tested. except the above one(address offset calculation)
I didn't see any issue with the address calculation.
for (i = 0; i < min_t(unsigned int, 4, naddrs); i++) {
	nfc_op->addrs |= instr->ctx.addr.addrs[i] <<
			 (8 * i);
}
If you go through the nand_base.c, there nand_fill_column_cycles() API, fills the first two or one address cycle
Based on bus width and page size.
That means, addrs[0]/[1] will be updated here.
And the page is updated to the next offsets.
In the similar way we have to extract the offsets in driver.
So the first four address bytes are stored using the above for() loop and if the
Address cycles are more than 4, then store the remaining offsets as well.

I just compared the offsets that are updated in driver with the offsets(page and column) that the frame work(nand_base.c) is sending, and the offsets are same.
I have also checked these offsets with older driver(not exec_op() implemented) and both are matching.

So I didn't see any issue with this addrs calculation.
As per the statement mentioned by you, this driver consumes addr[0], addr[1], addr[2], addr[3] and
If more address cycles needed, then addr[4] and addr[5]. This is correct.

Please let me know, what exactly I am missing.

Thank you for your inputs and sorry for the delay, as I am on leave for some days.

Regards,
Naga Sureshkumar Relli
> 
> Helmut

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [LINUX PATCH v14] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface

[Helmut Grohne]

Hi Naga,

On Thu, Jun 13, 2019 at 10:18:00AM +0000, Naga Sureshkumar Relli wrote:
> I spent much of time to address all your comments.
> All are addressed and tested. except the above one(address offset calculation)
> I didn't see any issue with the address calculation.

Let me first point out that this comment was not trying to imply a bug.
I was trying to understand the code by comparing it to similar code and
that turned up an inconsistency, which can be intentional or a bug in
either of the sides being compared.

> for (i = 0; i < min_t(unsigned int, 4, naddrs); i++) {
> 	nfc_op->addrs |= instr->ctx.addr.addrs[i] <<
> 			 (8 * i);
> }
> If you go through the nand_base.c, there nand_fill_column_cycles() API, fills the first two or one address cycle
> Based on bus width and page size.
> That means, addrs[0]/[1] will be updated here.

The problem at hand is that `addrs` is imprecise. In this code, there
are `instr->ctx.addr.addrs`, `addrs`, and `nfc_op->addrs`. All of them
are different. My original remark was targeting the possible confusion
of these different `addrs`.

> And the page is updated to the next offsets.
> In the similar way we have to extract the offsets in driver.
> So the first four address bytes are stored using the above for() loop and if the
> Address cycles are more than 4, then store the remaining offsets as well.
> 
> I just compared the offsets that are updated in driver with the offsets(page and column) that the frame work(nand_base.c) is sending, and the offsets are same.
> I have also checked these offsets with older driver(not exec_op() implemented) and both are matching.
> 
> So I didn't see any issue with this addrs calculation.
> As per the statement mentioned by you, this driver consumes addr[0], addr[1], addr[2], addr[3] and
> If more address cycles needed, then addr[4] and addr[5]. This is correct.

Again, the lack of precision makes it difficult to discuss the matter.
You refer to `addr`, but there is no `addr`. I assume that you meant
`addrs` here. Based on that assumption, your second last statement is
wrong. The driver consumes `addrs[0]|addrs[-offset]` rather than
`addrs[0]` as the first byte.  Then it proceeds consuming
`addrs[1-offset]` instead of `addrs[1]`, `addrs[2-offset]` instead of
`addrs[2]`, and `addrs[3-offset]` instead of `addrs[3]`. Finally it
consumes `addrs[4]` and `addrs[5]` if more cycles are needed.

I would not have commented the code if it were actually using `addrs[0]`
through `addrs[5]`. Your description looks reasonable to me, but it
doesn't match the code.

I'm looking forward to the next version of the patch.

Helmut

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [LINUX PATCH v14] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface

[Miquel Raynal]

Hi Naga,

> > > +	u32 addrs;
> > > +	u32 naddrs;
> > > +	u32 addr5;
> > > +	u32 addr6;  
> > 
> > Why are addr5 and addr6 u32? You only ever store u8 values in here. How about merging
> > them into an u16 addr56? Doing so would make the access in pl353_nand_exec_op_cmd
> > simpler and move a little complexity into pl353_nfc_parse_instructions.  
> Will try this. But I don't see any complex logic involved using addr5 and addr6.

This is a relic of a too quick copy from marvell_nand.c. Please match
the structure with your controller memory layout instead.

> > > +	const struct nand_op_instr *data_instr; };
> > > +
> > > +/**
> > > + * struct pl353_nand_controller - Defines the NAND flash controller driver
> > > + *				  instance
> > > + * @chip:		NAND chip information structure
> > > + * @dev:		Parent device (used to print error messages)
> > > + * @regs:		Virtual address of the NAND flash device
> > > + * @buf_addr:		Virtual address of the NAND flash device for
> > > + *			data read/writes
> > > + * @addr_cycles:	Address cycles
> > > + * @mclk:		Memory controller clock
> > > + * @buswidth:		Bus width 8 or 16
> > > + */
> > > +struct pl353_nand_controller {
> > > +	struct nand_controller controller;
> > > +	struct nand_chip chip;
> > > +	struct device *dev;
> > > +	void __iomem *regs;
> > > +	void __iomem *buf_addr;  
> > 
> > I find the use of buf_addr unfortunate. It is consumed by two functions
> > pl353_nand_read_data_op and pl353_nand_write_data_op. All other functions update its
> > value. Semantically, its value is regs + some flags. For the updaters that means a complex logic
> > where they first have to subtract reg, then change flags and add reg again. To make matters
> > worse, this computation involves __force casts between long and __iomem (which yielded
> > complaints in earlier reviews).  I think it would simplify the code if you replaced buf_addr with
> > something like addr_flags and then compute buf_addr as regs + addr_flags in those two
> > consumers. What do you think?  
> This definitely simplifies the driver logic, we have to do that.
> I tried it previously, regarding __force and __iomem casting changes, but the driver functionality was broken
> With this update.
> Let me update it and check it again.
> But just wanted to know, do you see issues with these __force and __iomem castings?
> >   
> > > +	u8 addr_cycles;
> > > +	struct clk *mclk;  
> > 
> > All you need here is the memory clock frequency. Wouldn't it be easier to extract that
> > frequency once during probe and store it here? That assumes a constant frequency, but if the
> > frequency isn't constant, you have a race condition.  
> That is what we are doing in the probe.
> In the probe, we are getting mclk using of_clk_get() and then we are getting the actual frequency
> Using clk_get_rate().
> And this is constant frequency only(getting from dts)

What Helmut proposes is, I think, saving the clock frequency to avoid
requesting it everytime you want to use it (if this is done many times).

[...]

> > > +
> > > +	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 u8 bbt_pattern[] = { 'B', 'b', 't', '0' }; static u8
> > > +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  
> > 
> > I have a general question concerning the nand framework here. The pattern member in struct
> > nand_bbt_descr is not declared const.
> > Therefore, bbt_pattern cannot be const here. As far as I looked, all accesses of pattern use it
> > with memcmp or as source for memcpy. Also the diskonchip.c driver assigns a string constant
> > here. This suggests, that pattern should be declared const or that diskonchip.c is doing it
> > wrong.  
> May be Miquel can comment on this.

I did not check diskonchip.c's implementation but turning it into const
seems reasonable to me. However in the scope of this driver, Naga, you
can keep it as it is right now.

> >   
> > > +};
> > > +
> > > +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
> > > +};
> > > +
> > > +static void pl353_nfc_force_byte_access(struct nand_chip *chip,
> > > +					bool force_8bit)
> > > +{
> > > +	int ret;
> > > +	struct pl353_nand_controller *xnfc =
> > > +		container_of(chip, struct pl353_nand_controller, chip);
> > > +
> > > +	if (xnfc->buswidth == 8)  
> > 
> > This buswidth member is never assigned anywhere. Thus the value is always 0 and this
> > comparison always fails.  
> No, in the probe we should update this, by reading it from dts.
> Unfortunately, the assignment was removed, during checkpatch clean up(its my editor issue).
> I will update it. 

This answer is scary, there are probably many other places where you
deleted useful code then?



[...]

> > > +/* 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++) {
> > > +		instr = &subop->instrs[op_id];
> > > +
> > > +		switch (instr->type) {
> > > +		case NAND_OP_CMD_INSTR:
> > > +			if (op_id)
> > > +				nfc_op->cmnds[1] = instr->ctx.cmd.opcode;
> > > +			else
> > > +				nfc_op->cmnds[0] = instr->ctx.cmd.opcode;
> > > +			nfc_op->cle_ale_delay_ns = instr->delay_ns;
> > > +			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] <<  
> > 
> > I don't quite understand what this code does, but it looks strange to me. I compared it to other
> > drivers. The code here is quite similar to marvell_nand.c. It seems like we are copying a
> > varying number (0 to 6) of addresses from the buffer instr->ctx.addr.addrs. However their
> > indices are special: 0, 1, 2, 3, offset + 4, offset + 5. This is non-consecutive and different from
> > marvell_nand.c in this regard. Could it be that you really meant index offset+i here?  
> I didn't get, what you are saying here.
> It is about updating page and column addresses.
> Are you asking me to remove nfc_op->addrs = instr->ctx.addr.addrs[offset]; before for loop?
> 
> >   
> > > +						 (8 * i);
> > > +			}
> > > +
> > > +			if (naddrs >= 5)
> > > +				nfc_op->addr5 = addrs[4];
> > > +			if (naddrs >= 6)
> > > +				nfc_op->addr6 = addrs[5];

Also, you probably don't need addr5 and addr6 here, they were needed in
marvell_nand.c because addresses are stored in three different
registers and are limited to 6 cycles, but this is probably not the
case in your driver.

> > > +			nfc_op->naddrs = nand_subop_get_num_addr_cyc(subop,
> > > +								     op_id);
> > > +			nfc_op->cle_ale_delay_ns = instr->delay_ns;
> > > +			break;
> > > +
> > > +		case NAND_OP_DATA_IN_INSTR:
> > > +			nfc_op->data_instr = instr;
> > > +			nfc_op->data_instr_idx = op_id;
> > > +			break;
> > > +
> > > +		case NAND_OP_DATA_OUT_INSTR:
> > > +			nfc_op->data_instr = instr;
> > > +			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;
> > > +			break;
> > > +		}
> > > +	}
> > > +}
> > > +
> > > +static void cond_delay(unsigned int ns) {
> > > +	if (!ns)
> > > +		return;
> > > +
> > > +	if (ns < 10000)
> > > +		ndelay(ns);
> > > +	else
> > > +		udelay(DIV_ROUND_UP(ns, 1000));
> > > +}  
> > 

[...]

> > > +
> > > +	writel_relaxed(cmd_phase_data, (void __iomem * __force)cmd_phase_addr);
> > > +	if (!nfc_op.data_instr) {
> > > +		if (nfc_op.rdy_timeout_ms) {
> > > +			if (pl353_wait_for_dev_ready(chip))
> > > +				return -ETIMEDOUT;
> > > +		}
> > > +
> > > +		return 0;
> > > +	}
> > > +
> > > +	reading = (nfc_op.data_instr->type == NAND_OP_DATA_IN_INSTR);
> > > +	if (!reading) {
> > > +		len = nand_subop_get_data_len(subop, op_id);
> > > +		pl353_nand_write_data_op(chip, instr->ctx.data.buf.out,
> > > +					 len, instr->ctx.data.force_8bit);
> > > +		if (nfc_op.rdy_timeout_ms) {
> > > +			if (pl353_wait_for_dev_ready(chip))
> > > +				return -ETIMEDOUT;
> > > +		}
> > > +
> > > +		cond_delay(nfc_op.rdy_delay_ns);
> > > +	}
> > > +
> > > +	if (reading) {  
> > 
> > You could use an else branch instead of inverting the condition here.
> > When Miquel complained about this in v13, you said you'd change it, but you didn't.  
> Sorry for that. I will change it.

Yes please, be careful and not in a hurry when re-posting, it already
takes quite some time to review the entire driver.

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Naga Sureshkumar Relli]

Hi Miquel,

> -----Original Message-----
> From: Miquel Raynal <miquel.raynal@bootlin.com>
> Sent: Monday, April 29, 2019 5:54 PM
> To: Naga Sureshkumar Relli <nagasure@xilinx.com>
> Cc: Helmut Grohne <helmut.grohne@intenta.de>; bbrezillon@kernel.org; richard@nod.at;
> dwmw2@infradead.org; computersforpeace@gmail.com; marek.vasut@gmail.com; linux-
> mtd@lists.infradead.org; linux-kernel@vger.kernel.org; Michal Simek <michals@xilinx.com>;
> nagasureshkumarrelli@gmail.com
> Subject: Re: [LINUX PATCH v14] mtd: rawnand: pl353: Add basic driver for arm pl353 smc
> nand interface
> 
> Hi Naga,
> 
> > > > +	u32 addrs;
> > > > +	u32 naddrs;
> > > > +	u32 addr5;
> > > > +	u32 addr6;
> > >
> > > Why are addr5 and addr6 u32? You only ever store u8 values in here.
> > > How about merging them into an u16 addr56? Doing so would make the
> > > access in pl353_nand_exec_op_cmd simpler and move a little complexity into
> pl353_nfc_parse_instructions.
> > Will try this. But I don't see any complex logic involved using addr5 and addr6.
> 
> This is a relic of a too quick copy from marvell_nand.c. Please match the structure with your
> controller memory layout instead.
Ok, I will re check this.
> 
> > > > +	const struct nand_op_instr *data_instr; };
> > > > +
> > > > +/**
> > > > + * struct pl353_nand_controller - Defines the NAND flash controller driver
> > > > + *				  instance
> > > > + * @chip:		NAND chip information structure
> > > > + * @dev:		Parent device (used to print error messages)
> > > > + * @regs:		Virtual address of the NAND flash device
> > > > + * @buf_addr:		Virtual address of the NAND flash device for
> > > > + *			data read/writes
> > > > + * @addr_cycles:	Address cycles
> > > > + * @mclk:		Memory controller clock
> > > > + * @buswidth:		Bus width 8 or 16
> > > > + */
> > > > +struct pl353_nand_controller {
> > > > +	struct nand_controller controller;
> > > > +	struct nand_chip chip;
> > > > +	struct device *dev;
> > > > +	void __iomem *regs;
> > > > +	void __iomem *buf_addr;
> > >
> > > I find the use of buf_addr unfortunate. It is consumed by two
> > > functions pl353_nand_read_data_op and pl353_nand_write_data_op. All
> > > other functions update its value. Semantically, its value is regs +
> > > some flags. For the updaters that means a complex logic where they
> > > first have to subtract reg, then change flags and add reg again. To
> > > make matters worse, this computation involves __force casts between
> > > long and __iomem (which yielded complaints in earlier reviews).  I
> > > think it would simplify the code if you replaced buf_addr with something like addr_flags
> and then compute buf_addr as regs + addr_flags in those two consumers. What do you think?
> > This definitely simplifies the driver logic, we have to do that.
> > I tried it previously, regarding __force and __iomem casting changes,
> > but the driver functionality was broken With this update.
> > Let me update it and check it again.
> > But just wanted to know, do you see issues with these __force and __iomem castings?
> > >
> > > > +	u8 addr_cycles;
> > > > +	struct clk *mclk;
> > >
> > > All you need here is the memory clock frequency. Wouldn't it be
> > > easier to extract that frequency once during probe and store it
> > > here? That assumes a constant frequency, but if the frequency isn't constant, you have a
> race condition.
> > That is what we are doing in the probe.
> > In the probe, we are getting mclk using of_clk_get() and then we are
> > getting the actual frequency Using clk_get_rate().
> > And this is constant frequency only(getting from dts)
> 
> What Helmut proposes is, I think, saving the clock frequency to avoid requesting it everytime
> you want to use it (if this is done many times).
Ok. got it.
> 
> [...]
> 
> > > > +
> > > > +	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 u8 bbt_pattern[] = {
> > > > +'B', 'b', 't', '0' }; static u8 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
> > >
> > > I have a general question concerning the nand framework here. The
> > > pattern member in struct nand_bbt_descr is not declared const.
> > > Therefore, bbt_pattern cannot be const here. As far as I looked, all
> > > accesses of pattern use it with memcmp or as source for memcpy. Also
> > > the diskonchip.c driver assigns a string constant here. This
> > > suggests, that pattern should be declared const or that diskonchip.c is doing it wrong.
> > May be Miquel can comment on this.
> 
> I did not check diskonchip.c's implementation but turning it into const seems reasonable to me.
> However in the scope of this driver, Naga, you can keep it as it is right now.
Ok.
> 
> > >
> > > > +};
> > > > +
> > > > +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
> > > > +};
> > > > +
> > > > +static void pl353_nfc_force_byte_access(struct nand_chip *chip,
> > > > +					bool force_8bit)
> > > > +{
> > > > +	int ret;
> > > > +	struct pl353_nand_controller *xnfc =
> > > > +		container_of(chip, struct pl353_nand_controller, chip);
> > > > +
> > > > +	if (xnfc->buswidth == 8)
> > >
> > > This buswidth member is never assigned anywhere. Thus the value is
> > > always 0 and this comparison always fails.
> > No, in the probe we should update this, by reading it from dts.
> > Unfortunately, the assignment was removed, during checkpatch clean up(its my editor issue).
> > I will update it.
> 
> This answer is scary, there are probably many other places where you deleted useful code then?
Sorry for that. But I won't repeat this.
> 
> 
> 
> [...]
> 
> > > > +/* 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++) {
> > > > +		instr = &subop->instrs[op_id];
> > > > +
> > > > +		switch (instr->type) {
> > > > +		case NAND_OP_CMD_INSTR:
> > > > +			if (op_id)
> > > > +				nfc_op->cmnds[1] = instr->ctx.cmd.opcode;
> > > > +			else
> > > > +				nfc_op->cmnds[0] = instr->ctx.cmd.opcode;
> > > > +			nfc_op->cle_ale_delay_ns = instr->delay_ns;
> > > > +			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] <<
> > >
> > > I don't quite understand what this code does, but it looks strange
> > > to me. I compared it to other drivers. The code here is quite
> > > similar to marvell_nand.c. It seems like we are copying a varying
> > > number (0 to 6) of addresses from the buffer instr->ctx.addr.addrs.
> > > However their indices are special: 0, 1, 2, 3, offset + 4, offset + 5. This is non-consecutive
> and different from marvell_nand.c in this regard. Could it be that you really meant index
> offset+i here?
> > I didn't get, what you are saying here.
> > It is about updating page and column addresses.
> > Are you asking me to remove nfc_op->addrs = instr->ctx.addr.addrs[offset]; before for
> loop?
> >
> > >
> > > > +						 (8 * i);
> > > > +			}
> > > > +
> > > > +			if (naddrs >= 5)
> > > > +				nfc_op->addr5 = addrs[4];
> > > > +			if (naddrs >= 6)
> > > > +				nfc_op->addr6 = addrs[5];
> 
> Also, you probably don't need addr5 and addr6 here, they were needed in marvell_nand.c
> because addresses are stored in three different registers and are limited to 6 cycles, but this is
> probably not the case in your driver.
Ok. I will cross check it once and update it next version.
> 
> > > > +			nfc_op->naddrs = nand_subop_get_num_addr_cyc(subop,
> > > > +								     op_id);
> > > > +			nfc_op->cle_ale_delay_ns = instr->delay_ns;
> > > > +			break;
> > > > +
> > > > +		case NAND_OP_DATA_IN_INSTR:
> > > > +			nfc_op->data_instr = instr;
> > > > +			nfc_op->data_instr_idx = op_id;
> > > > +			break;
> > > > +
> > > > +		case NAND_OP_DATA_OUT_INSTR:
> > > > +			nfc_op->data_instr = instr;
> > > > +			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;
> > > > +			break;
> > > > +		}
> > > > +	}
> > > > +}
> > > > +
> > > > +static void cond_delay(unsigned int ns) {
> > > > +	if (!ns)
> > > > +		return;
> > > > +
> > > > +	if (ns < 10000)
> > > > +		ndelay(ns);
> > > > +	else
> > > > +		udelay(DIV_ROUND_UP(ns, 1000)); }
> > >
> 
> [...]
> 
> > > > +
> > > > +	writel_relaxed(cmd_phase_data, (void __iomem * __force)cmd_phase_addr);
> > > > +	if (!nfc_op.data_instr) {
> > > > +		if (nfc_op.rdy_timeout_ms) {
> > > > +			if (pl353_wait_for_dev_ready(chip))
> > > > +				return -ETIMEDOUT;
> > > > +		}
> > > > +
> > > > +		return 0;
> > > > +	}
> > > > +
> > > > +	reading = (nfc_op.data_instr->type == NAND_OP_DATA_IN_INSTR);
> > > > +	if (!reading) {
> > > > +		len = nand_subop_get_data_len(subop, op_id);
> > > > +		pl353_nand_write_data_op(chip, instr->ctx.data.buf.out,
> > > > +					 len, instr->ctx.data.force_8bit);
> > > > +		if (nfc_op.rdy_timeout_ms) {
> > > > +			if (pl353_wait_for_dev_ready(chip))
> > > > +				return -ETIMEDOUT;
> > > > +		}
> > > > +
> > > > +		cond_delay(nfc_op.rdy_delay_ns);
> > > > +	}
> > > > +
> > > > +	if (reading) {
> > >
> > > You could use an else branch instead of inverting the condition here.
> > > When Miquel complained about this in v13, you said you'd change it, but you didn't.
> > Sorry for that. I will change it.
> 
> Yes please, be careful and not in a hurry when re-posting, it already takes quite some time to
> review the entire driver.
Sure Miquel and thanks for the review.

Thanks,
Naga Sureshkumar Relli
> 
> Thanks,
> Miquèl
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/