[PATCH v3 0/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC

[PATCH v3 0/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC

[Ramuthevar,Vadivel MuruganX]

From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>

This patch adds the new IP of Nand Flash Controller(NFC) support
on Intel's Lightning Mountain(LGM) SoC.

DMA is used for burst data transfer operation, also DMA HW supports
aligned 32bit memory address and aligned data access by default.
DMA burst of 8 supported. Data register used to support the read/write
operation from/to device.

NAND controller also supports in-built HW ECC engine.

NAND controller driver implements ->exec_op() to replace legacy hooks,
these specific call-back method to execute NAND operations.

Thank you very much Boris and Hauke for the reviews and suggestions.
---
v3:
  - Add depends on MACRO in Kconfig
  - file name update in Makefile
  - file name update to intel-nand-controller
  - modification of MACRO divided like EBU, HSNAND and NAND
  - add NAND_ALE_OFFS, NAND_CLE_OFFS and NAND_CS_OFFS
  - rename lgm_ to ebu_ and _va suffix is removed in the whole file
  - rename structure and varaibles as per review comments.
  - remove lgm_read_byte(), lgm_dev_ready() and cmd_ctrl() un-used function  
  - update in exec_op() as per review comments
  - rename function lgm_dma_exit() by lgm_dma_cleanup()
  - hardcoded magic value  for base and offset replaced by MACRO defined
  - mtd_device_unregister() + nand_cleanup() instead of nand_release()
v2:
  - implement the ->exec_op() to replaces the legacy hook-up.
  - update the commit message
  - YAML compatible string update to intel, lgm-nand-controller
  - add MIPS maintainers and xway_nand driver author in CC

v1:
 - initial version
 


Ramuthevar Vadivel Murugan (2):
  dt-bindings: mtd: Add YAML for Nand Flash Controller support
  mtd: rawnand: Add NAND controller support on Intel LGM SoC

 .../devicetree/bindings/mtd/intel,lgm-nand.yaml    |  61 ++
 drivers/mtd/nand/raw/Kconfig                       |   8 +
 drivers/mtd/nand/raw/Makefile                      |   1 +
 drivers/mtd/nand/raw/intel-nand-controller.c       | 705 +++++++++++++++++++++
 4 files changed, 775 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/intel,lgm-nand.yaml
 create mode 100644 drivers/mtd/nand/raw/intel-nand-controller.c

-- 
2.11.0

[PATCH v3 1/2] dt-bindings: mtd: Add YAML for Nand Flash Controller support

[Ramuthevar,Vadivel MuruganX]

From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>

Add YAML file for dt-bindings to support NAND Flash Controller
on Intel's Lightning Mountain SoC.

Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
---
 .../devicetree/bindings/mtd/intel,lgm-nand.yaml    | 61 ++++++++++++++++++++++
 1 file changed, 61 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/intel,lgm-nand.yaml

diff --git a/Documentation/devicetree/bindings/mtd/intel,lgm-nand.yaml b/Documentation/devicetree/bindings/mtd/intel,lgm-nand.yaml
new file mode 100644
index 000000000000..6dd899d367b4
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/intel,lgm-nand.yaml
@@ -0,0 +1,61 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mtd/intel,lgm-nand.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Intel LGM SoC NAND Controller Device Tree Bindings
+
+allOf:
+  - $ref: "nand-controller.yaml"
+
+maintainers:
+  - Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
+
+properties:
+  compatible:
+    const: intel,lgm-nand-controller
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  dmas:
+    maxItems: 2
+
+  dma-names:
+    enum:
+      - rx
+      - tx
+
+  pinctrl-names: true
+
+patternProperties:
+  "^pinctrl-[0-9]+$": true
+
+  "^nand@[a-f0-9]+$":
+    type: object
+    properties:
+      reg:
+        minimum: 0
+        maximum: 7
+
+      nand-ecc-mode: true
+
+      nand-ecc-algo:
+        const: hw
+
+    additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - dmas
+
+additionalProperties: false
+
+...
-- 
2.11.0

Re: [PATCH v3 1/2] dt-bindings: mtd: Add YAML for Nand Flash Controller support

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2351 bytes --]

Hi,

On Fri, Apr 24, 2020 at 12:21:12AM +0800, Ramuthevar,Vadivel MuruganX wrote:
> From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
> 
> Add YAML file for dt-bindings to support NAND Flash Controller
> on Intel's Lightning Mountain SoC.
> 
> Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
> ---
>  .../devicetree/bindings/mtd/intel,lgm-nand.yaml    | 61 ++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mtd/intel,lgm-nand.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mtd/intel,lgm-nand.yaml b/Documentation/devicetree/bindings/mtd/intel,lgm-nand.yaml
> new file mode 100644
> index 000000000000..6dd899d367b4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/intel,lgm-nand.yaml
> @@ -0,0 +1,61 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mtd/intel,lgm-nand.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Intel LGM SoC NAND Controller Device Tree Bindings
> +
> +allOf:
> +  - $ref: "nand-controller.yaml"
> +
> +maintainers:
> +  - Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
> +
> +properties:
> +  compatible:
> +    const: intel,lgm-nand-controller
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  dmas:
> +    maxItems: 2
> +
> +  dma-names:
> +    enum:
> +      - rx
> +      - tx

This looks wrong?

If you have two dmas channels, I assume you'll want to have both rx and tx,
right? If so, then you need an items, not an enum.

> +  pinctrl-names: true
> +
> +patternProperties:
> +  "^pinctrl-[0-9]+$": true

both pinctrl-names and that pattern are added automatically by the tooling, you
should leave them out.

> +  "^nand@[a-f0-9]+$":
> +    type: object
> +    properties:
> +      reg:
> +        minimum: 0
> +        maximum: 7
> +
> +      nand-ecc-mode: true
> +
> +      nand-ecc-algo:
> +        const: hw
> +
> +    additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - dmas
> +
> +additionalProperties: false
> +
> +...

Can you provide an example too?

Thanks!
Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH v3 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC

[Ramuthevar,Vadivel MuruganX]

From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>

This patch adds the new IP of Nand Flash Controller(NFC) support
on Intel's Lightning Mountain(LGM) SoC.

DMA is used for burst data transfer operation, also DMA HW supports
aligned 32bit memory address and aligned data access by default.
DMA burst of 8 supported. Data register used to support the read/write
operation from/to device.

NAND controller driver implements ->exec_op() to replace legacy hooks,
these specific call-back method to execute NAND operations.

Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
---
 drivers/mtd/nand/raw/Kconfig                 |   8 +
 drivers/mtd/nand/raw/Makefile                |   1 +
 drivers/mtd/nand/raw/intel-nand-controller.c | 705 +++++++++++++++++++++++++++
 3 files changed, 714 insertions(+)
 create mode 100644 drivers/mtd/nand/raw/intel-nand-controller.c

diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
index a80a46bb5b8b..a026bec28f39 100644
--- a/drivers/mtd/nand/raw/Kconfig
+++ b/drivers/mtd/nand/raw/Kconfig
@@ -457,6 +457,14 @@ config MTD_NAND_CADENCE
 	  Enable the driver for NAND flash on platforms using a Cadence NAND
 	  controller.
 
+config MTD_NAND_INTEL_LGM
+	tristate "Support for NAND controller on Intel LGM SoC"
+	depends on OF || COMPILE_TEST
+	depends on HAS_IOMEM
+	help
+	  Enables support for NAND Flash chips on Intel's LGM SoC.
+          NAND flash interfaced through the External Bus Unit.
+
 comment "Misc"
 
 config MTD_SM_COMMON
diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile
index 2d136b158fb7..bfc8fe4d2cb0 100644
--- a/drivers/mtd/nand/raw/Makefile
+++ b/drivers/mtd/nand/raw/Makefile
@@ -58,6 +58,7 @@ 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_CADENCE)		+= cadence-nand-controller.o
+obj-$(CONFIG_MTD_NAND_INTEL_LGM)	+= intel-nand-controller.o
 
 nand-objs := nand_base.o nand_legacy.o nand_bbt.o nand_timings.o nand_ids.o
 nand-objs += nand_onfi.o
diff --git a/drivers/mtd/nand/raw/intel-nand-controller.c b/drivers/mtd/nand/raw/intel-nand-controller.c
new file mode 100644
index 000000000000..7f4efdb9e976
--- /dev/null
+++ b/drivers/mtd/nand/raw/intel-nand-controller.c
@@ -0,0 +1,705 @@
+// SPDX-License-Identifier: GPL-2.0+
+/* Copyright (c) 2020 Intel Corporation. */
+
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/dmaengine.h>
+#include <linux/dma-direction.h>
+#include <linux/dma-mapping.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/rawnand.h>
+#include <linux/mtd/nand_ecc.h>
+#include <linux/mtd/nand.h>
+#include <linux/resource.h>
+#include <linux/sched.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <linux/platform_device.h>
+
+#define EBU_CLC			0x000
+#define EBU_CLC_RST		0x00000000u
+
+#define	EBU_ADDR_SEL(n)		(0x20 + (n) * 4)
+#define	EBU_ADDR_MASK		(5 << 4)
+#define	EBU_ADDR_SEL_REGEN	0x1
+
+#define EBU_BUSCON(n)		(0x60 + (n) * 4)
+#define EBU_BUSCON_CMULT_V4	0x1
+#define EBU_BUSCON_RECOVC(n)	((n) << 2)
+#define EBU_BUSCON_HOLDC(n)	((n) << 4)
+#define EBU_BUSCON_WAITRDC(n)	((n) << 6)
+#define EBU_BUSCON_WAITWRC(n)	((n) << 8)
+#define EBU_BUSCON_BCGEN_CS	0x0
+#define EBU_BUSCON_SETUP_EN	BIT(22)
+#define EBU_BUSCON_ALEC		0xC000
+
+#define EBU_CON			0x0B0
+#define EBU_CON_NANDM_EN	BIT(0)
+#define EBU_CON_NANDM_DIS	0x0
+#define EBU_CON_CSMUX_E_EN	BIT(1)
+#define EBU_CON_ALE_P_LOW	BIT(2)
+#define EBU_CON_CLE_P_LOW	BIT(3)
+#define EBU_CON_CS_P_LOW	BIT(4)
+#define EBU_CON_SE_P_LOW	BIT(5)
+#define EBU_CON_WP_P_LOW	BIT(6)
+#define EBU_CON_PRE_P_LOW	BIT(7)
+#define EBU_CON_IN_CS_S(n)	((n) << 8)
+#define EBU_CON_OUT_CS_S(n)	((n) << 10)
+#define EBU_CON_LAT_EN_CS_P	((0x3D) << 18)
+
+#define EBU_WAIT		0x0B4
+#define EBU_WAIT_RDBY		BIT(0)
+#define EBU_WAIT_WR_C		BIT(3)
+
+#define	EBU_MEM_BASE_CS_0	0x17400
+#define	EBU_MEM_BASE_CS_1	0x17C00
+#define	EBU_MEM_OFFSET		0x051
+
+#define HSNAND_CTL1		0x110
+#define HSNAND_CTL1_ADDR_SHIFT	24
+
+#define HSNAND_CTL2		0x114
+#define HSNAND_CTL2_ADDR_SHIFT	8
+#define HSNAND_CTL2_CYC_N_V5	(0x2 << 16)
+
+#define HSNAND_INT_MSK_CTL	0x124
+#define HSNAND_INT_MSK_CTL_WR_C	BIT(4)
+
+#define HSNAND_INT_STA		0x128
+#define HSNAND_INT_STA_WR_C	BIT(4)
+
+#define HSNAND_CTL		0x130
+#define HSNAND_CTL_MODE_ECC	0x1
+#define HSNAND_CTL_GO		BIT(2)
+#define HSNAND_CTL_CE_SEL_CS(n)	BIT(3 + (n))
+#define HSNAND_CTL_RW_READ	0x0
+#define HSNAND_CTL_RW_WRITE	BIT(10)
+#define HSNAND_CTL_ECC_OFF_V8TH	BIT(11)
+#define HSNAND_CTL_CKFF_EN	0x0
+#define HSNAND_CTL_MSG_EN	BIT(17)
+
+#define NAND_PARA0		0x13c
+#define NAND_PARA0_PAGE_V8192	0x3
+#define NAND_PARA0_PIB_V256	(0x3 << 4)
+#define NAND_PARA0_BYP_EN_NP	0x0
+#define NAND_PARA0_BYP_DEC_NP	0x0
+#define NAND_PARA0_TYPE_ONFI	BIT(18)
+#define NAND_PARA0_ADEP_EN	BIT(21)
+
+#define NAND_CMSG_0		0x150
+#define NAND_CMSG_1		0x154
+
+#define NAND_ALE_OFFS		BIT(2)
+#define NAND_CLE_OFFS		BIT(3)
+#define NAND_CS_OFFS		BIT(4)
+
+#define NAND_WRITE_CMD		(NAND_CS_OFFS | NAND_CLE_OFFS)
+#define NAND_WRITE_ADDR		(NAND_CS_OFFS | NAND_ALE_OFFS)
+#define NAND_WRITE_DATA		EBU_CON_CS_P_LOW
+#define NAND_READ_DATA		EBU_CON_CS_P_LOW
+
+#define NAND_ECC_OFFSET		0x008
+
+struct ebu_nand_controller {
+	struct nand_controller	controller;
+	struct nand_chip	chip;
+	void __iomem		*ebu_addr;
+	void __iomem		*nand_addr;
+	void __iomem		*chip_addr;
+	struct clk		*clk;
+	unsigned long		clk_rate;
+	u32			cs;
+	u32			nd_para0;
+	struct device		*dev;
+	struct dma_chan		*dma_tx;
+	struct dma_chan		*dma_rx;
+	struct completion	dma_access_complete;
+	const char *cs_name;
+};
+
+static inline struct ebu_nand_controller *nand_to_ebu(struct nand_chip *chip)
+{
+	return container_of(chip, struct ebu_nand_controller, chip);
+}
+
+static u8 ebu_nand_readb(struct nand_chip *chip, unsigned int op)
+{
+	struct ebu_nand_controller *ebu_host = nand_get_controller_data(chip);
+	void __iomem *nand_wait = ebu_host->ebu_addr + EBU_WAIT;
+	u32 stat;
+	int ret;
+	u8 val;
+
+	val = readb(ebu_host->chip_addr + op);
+
+	ret = readl_poll_timeout(nand_wait, stat, stat & EBU_WAIT_WR_C,
+				 20, 1000);
+	if (ret)
+		dev_warn(ebu_host->dev,
+			 "ebu nand write timeout. nand_wait(0x%p)=0x%x\n",
+			 nand_wait, readl(nand_wait));
+
+	return val;
+}
+
+static void ebu_nand_writeb(struct nand_chip *chip, int op, u8 value)
+{
+	struct ebu_nand_controller *ebu_host = nand_get_controller_data(chip);
+	void __iomem *nand_wait = ebu_host->ebu_addr + EBU_WAIT;
+	u32 stat;
+	int ret;
+
+	writeb(value, ebu_host->chip_addr + op);
+
+	ret = readl_poll_timeout(nand_wait, stat, stat & EBU_WAIT_WR_C,
+				 20, 1000);
+	if (ret)
+		dev_warn(ebu_host->dev,
+			 "ebu nand write timeout. nand_wait(0x%p)=0x%x\n",
+			 nand_wait, readl(nand_wait));
+}
+
+static void ebu_read_buf(struct nand_chip *chip, u_char *buf, int len)
+{
+	int i;
+
+	for (i = 0; i < len; i++)
+		buf[i] = ebu_nand_readb(chip, NAND_READ_DATA);
+}
+
+static void ebu_write_buf(struct nand_chip *chip, const u_char *buf, int len)
+{
+	int i;
+
+	for (i = 0; i < len; i++)
+		ebu_nand_writeb(chip, NAND_WRITE_DATA, buf[i]);
+}
+
+static void ebu_unselect_chip(struct nand_chip *chip)
+{
+	struct ebu_nand_controller *ebu_host = nand_get_controller_data(chip);
+	void __iomem *nand_con = ebu_host->ebu_addr + EBU_CON;
+	int val;
+
+	val = readl(nand_con);
+	writel(val & ~EBU_CON_NANDM_EN, nand_con);
+}
+
+static void ebu_select_chip(struct nand_chip *chip)
+{
+	struct ebu_nand_controller *ebu_host = nand_get_controller_data(chip);
+	void __iomem *nand_con = ebu_host->ebu_addr + EBU_CON;
+	u32 cs = ebu_host->cs;
+
+	writel(EBU_CON_NANDM_EN | EBU_CON_CSMUX_E_EN | EBU_CON_CS_P_LOW |
+	       EBU_CON_SE_P_LOW | EBU_CON_WP_P_LOW | EBU_CON_PRE_P_LOW |
+	       EBU_CON_IN_CS_S(cs) | EBU_CON_OUT_CS_S(cs) |
+	       EBU_CON_LAT_EN_CS_P, nand_con);
+}
+
+static int ebu_nand_ooblayout_ecc(struct mtd_info *mtd, int section,
+				  struct mtd_oob_region *oobregion)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+
+	if (section)
+		return -ERANGE;
+
+	oobregion->offset = NAND_ECC_OFFSET;
+	oobregion->length = chip->ecc.total;
+
+	return 0;
+}
+
+static int ebu_nand_ooblayout_free(struct mtd_info *mtd, int section,
+				   struct mtd_oob_region *oobregion)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+
+	if (section)
+		return -ERANGE;
+
+	oobregion->offset = chip->ecc.total + NAND_ECC_OFFSET;
+	oobregion->length = mtd->oobsize - oobregion->offset;
+
+	return 0;
+}
+
+static const struct mtd_ooblayout_ops ebu_nand_ooblayout_ops = {
+	.ecc = ebu_nand_ooblayout_ecc,
+	.free = ebu_nand_ooblayout_free,
+};
+
+static void ebu_dma_rx_callback(void *cookie)
+{
+	struct ebu_nand_controller *ebu_host = cookie;
+
+	dmaengine_terminate_async(ebu_host->dma_rx);
+
+	complete(&ebu_host->dma_access_complete);
+}
+
+static void ebu_dma_tx_callback(void *cookie)
+{
+	struct ebu_nand_controller *ebu_host = cookie;
+
+	dmaengine_terminate_async(ebu_host->dma_tx);
+
+	complete(&ebu_host->dma_access_complete);
+}
+
+static int ebu_dma_start(struct ebu_nand_controller *ebu_host, u32 dir,
+			 const u8 *buf, u32 len)
+{
+	struct dma_async_tx_descriptor *tx;
+	struct completion *dma_completion;
+	dma_async_tx_callback callback;
+	struct dma_chan *chan;
+	dma_cookie_t cookie;
+	unsigned long flags = DMA_CTRL_ACK | DMA_PREP_INTERRUPT;
+	dma_addr_t buf_dma;
+	int ret;
+	u32 timeout;
+
+	if (dir == DMA_DEV_TO_MEM) {
+		chan = ebu_host->dma_rx;
+		dma_completion = &ebu_host->dma_access_complete;
+		callback = ebu_dma_rx_callback;
+	} else {
+		chan = ebu_host->dma_tx;
+		dma_completion = &ebu_host->dma_access_complete;
+		callback = ebu_dma_tx_callback;
+	}
+
+	buf_dma = dma_map_single(chan->device->dev, (void *)buf, len, dir);
+	if (dma_mapping_error(chan->device->dev, buf_dma)) {
+		dev_err(ebu_host->dev, "Failed to map DMA buffer\n");
+		ret = -EIO;
+		goto err_unmap;
+	}
+
+	tx = dmaengine_prep_slave_single(chan, buf_dma, len, dir, flags);
+	if (!tx)
+		return -ENXIO;
+
+	tx->callback = callback;
+	tx->callback_param = ebu_host;
+	cookie = tx->tx_submit(tx);
+
+	ret = dma_submit_error(cookie);
+	if (ret) {
+		dev_err(ebu_host->dev, "dma_submit_error %d\n", cookie);
+		ret = -EIO;
+		goto err_unmap;
+	}
+
+	init_completion(dma_completion);
+	dma_async_issue_pending(chan);
+
+	/* Wait DMA to finish the data transfer.*/
+	timeout =
+	wait_for_completion_timeout(dma_completion, msecs_to_jiffies(1000));
+	if (!timeout) {
+		dev_err(ebu_host->dev, "I/O Error in DMA RX (status %d)\n",
+			dmaengine_tx_status(chan, cookie, NULL));
+		dmaengine_terminate_sync(chan);
+		ret = -ETIMEDOUT;
+		goto err_unmap;
+	}
+
+	return 0;
+
+err_unmap:
+	dma_unmap_single(ebu_host->dev, buf_dma, len, dir);
+
+	return ret;
+}
+
+static void ebu_nand_trigger(struct ebu_nand_controller *ebu_host,
+			     int page, u32 cmd)
+{
+	unsigned int val;
+
+	val = cmd | (page & 0xFF) << HSNAND_CTL1_ADDR_SHIFT;
+	writel(val, ebu_host->nand_addr + HSNAND_CTL1);
+	val = (page & 0xFFFF00) >> 8 | HSNAND_CTL2_CYC_N_V5;
+	writel(val, ebu_host->nand_addr + HSNAND_CTL2);
+
+	writel(ebu_host->nd_para0, ebu_host->nand_addr + NAND_PARA0);
+
+	/* clear first, will update later */
+	writel(0xFFFFFFFF, ebu_host->nand_addr + NAND_CMSG_0);
+	writel(0xFFFFFFFF, ebu_host->nand_addr + NAND_CMSG_1);
+
+	writel(HSNAND_INT_MSK_CTL_WR_C,
+	       ebu_host->nand_addr + HSNAND_INT_MSK_CTL);
+
+	val = cmd == NAND_CMD_READ0 ? HSNAND_CTL_RW_READ : HSNAND_CTL_RW_WRITE;
+
+	writel(HSNAND_CTL_MSG_EN | HSNAND_CTL_CKFF_EN |
+	       HSNAND_CTL_ECC_OFF_V8TH | HSNAND_CTL_CE_SEL_CS(ebu_host->cs) |
+	       HSNAND_CTL_MODE_ECC | HSNAND_CTL_GO | val,
+	       ebu_host->nand_addr + HSNAND_CTL);
+}
+
+static int ebu_nand_read_page_hwecc(struct nand_chip *chip, u8 *buf,
+				    int oob_required, int page)
+{
+	struct mtd_info *mtd = nand_to_mtd(chip);
+	struct ebu_nand_controller *ebu_host = nand_get_controller_data(chip);
+	int ret, x;
+
+	ebu_nand_trigger(ebu_host, page, NAND_CMD_READ0);
+
+	ret = ebu_dma_start(ebu_host, DMA_DEV_TO_MEM, buf, mtd->writesize);
+	if (ret)
+		return ret;
+
+	if (oob_required)
+		chip->ecc.read_oob(chip, page);
+
+	x = readl(ebu_host->nand_addr + HSNAND_CTL);
+	x &= ~HSNAND_CTL_GO;
+	writel(x, ebu_host->nand_addr + HSNAND_CTL);
+
+	return 0;
+}
+
+static int ebu_nand_write_page_hwecc(struct nand_chip *chip, const u8 *buf,
+				     int oob_required, int page)
+{
+	struct mtd_info *mtd = nand_to_mtd(chip);
+	struct ebu_nand_controller *ebu_host = nand_get_controller_data(chip);
+	void __iomem *int_sta = ebu_host->nand_addr + HSNAND_INT_STA;
+	int ret, val, x;
+	__be32 reg;
+
+	ebu_nand_trigger(ebu_host, page, NAND_CMD_SEQIN);
+
+	ret = ebu_dma_start(ebu_host, DMA_MEM_TO_DEV, buf, mtd->writesize);
+	if (ret)
+		return ret;
+
+	if (oob_required) {
+		const u8 *pdata;
+
+		pdata = chip->oob_poi;
+		reg = cpu_to_be32(*pdata++);
+		writel(reg, ebu_host->nand_addr + NAND_CMSG_0);
+
+		reg = cpu_to_be32(*pdata);
+		writel(reg, ebu_host->nand_addr + NAND_CMSG_1);
+	}
+
+	ret = readl_poll_timeout_atomic(int_sta, val,
+					!(val & HSNAND_INT_STA_WR_C), 10, 1000);
+	if (ret)
+		return -EIO;
+
+	x = readl(ebu_host->nand_addr + HSNAND_CTL);
+	x &= ~HSNAND_CTL_GO;
+	writel(x, ebu_host->nand_addr + HSNAND_CTL);
+
+	return 0;
+}
+
+static const u8 ecc_strength[] = { 1, 1, 4, 8, 24, 32, 40, 60, };
+
+static int ebu_nand_attach_chip(struct nand_chip *chip)
+{
+	struct mtd_info *mtd = nand_to_mtd(chip);
+	struct ebu_nand_controller *ebu_host = nand_get_controller_data(chip);
+	u32 eccsize, eccsteps, eccbytes, ecctotal, pagesize, pg_per_blk;
+	u32 eccstrength = chip->ecc.strength;
+	u32 writesize = mtd->writesize;
+	u32 blocksize = mtd->erasesize;
+	int start, val, i;
+
+	if (chip->ecc.mode != NAND_ECC_HW)
+		return 0;
+
+	/* Check whether eccsize is 0x0 or wrong. assign eccsize = 512 if YES */
+	if (!chip->ecc.size)
+		chip->ecc.size = 512;
+	eccsize = chip->ecc.size;
+
+	switch (eccsize) {
+	case 512:
+		start = 1;
+		if (!eccstrength)
+			eccstrength = 4;
+		break;
+	case 1024:
+		start = 4;
+		if (!eccstrength)
+			eccstrength = 32;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	i = round_up(start + 1, 4);
+	for (val = start; val < i; val++) {
+		if (eccstrength == ecc_strength[val])
+			break;
+	}
+	if (val == i)
+		return -EINVAL;
+
+	if (eccstrength == 8)
+		eccbytes = 14;
+	else
+		eccbytes = DIV_ROUND_UP(eccstrength * fls(8 * eccsize), 8);
+
+	eccsteps = writesize / eccsize;
+	ecctotal = eccsteps * eccbytes;
+	if ((ecctotal + 8) > mtd->oobsize)
+		return -ERANGE;
+
+	chip->ecc.total = ecctotal;
+	pagesize = fls(writesize >> 11);
+	if (pagesize > NAND_PARA0_PAGE_V8192)
+		return -ERANGE;
+
+	pg_per_blk = fls((blocksize / writesize) >> 6) << 4;
+	if (pg_per_blk > NAND_PARA0_PIB_V256)
+		return -ERANGE;
+
+	ebu_host->nd_para0 = pagesize | pg_per_blk | NAND_PARA0_BYP_EN_NP |
+			     NAND_PARA0_BYP_DEC_NP | NAND_PARA0_ADEP_EN |
+			     NAND_PARA0_TYPE_ONFI | (val << 29);
+
+	mtd_set_ooblayout(mtd, &ebu_nand_ooblayout_ops);
+	chip->ecc.read_page = ebu_nand_read_page_hwecc;
+	chip->ecc.write_page = ebu_nand_write_page_hwecc;
+
+	return 0;
+}
+
+static int ebu_nand_exec_op(struct nand_chip *chip,
+			    const struct nand_operation *op, bool check_only)
+{
+	struct ebu_nand_controller *ctrl = nand_to_ebu(chip);
+	const struct nand_op_instr *instr = NULL;
+	unsigned int op_id;
+	int i, time_out, ret = 0;
+	u32 stat;
+
+	ebu_select_chip(chip);
+
+	for (op_id = 0; op_id < op->ninstrs; op_id++) {
+		instr = &op->instrs[op_id];
+
+		switch (instr->type) {
+		case NAND_OP_CMD_INSTR:
+			ebu_nand_writeb(chip, NAND_CLE_OFFS,
+					instr->ctx.cmd.opcode);
+			break;
+
+		case NAND_OP_ADDR_INSTR:
+			for (i = 0; i < instr->ctx.addr.naddrs; i++)
+				ebu_nand_writeb(chip, NAND_ALE_OFFS,
+						instr->ctx.addr.addrs[i]);
+			break;
+
+		case NAND_OP_DATA_IN_INSTR:
+			ebu_read_buf(chip, instr->ctx.data.buf.in,
+				     instr->ctx.data.len);
+			break;
+
+		case NAND_OP_DATA_OUT_INSTR:
+			ebu_write_buf(chip, instr->ctx.data.buf.out,
+				      instr->ctx.data.len);
+			break;
+
+		case NAND_OP_WAITRDY_INSTR:
+			time_out = instr->ctx.waitrdy.timeout_ms * 1000;
+			ret = readl_poll_timeout(ctrl->ebu_addr + EBU_WAIT,
+						 stat, stat & EBU_WAIT_RDBY,
+						 20, time_out);
+			break;
+		}
+	}
+
+	ebu_unselect_chip(chip);
+
+	return ret;
+}
+
+static const struct nand_controller_ops ebu_nand_controller_ops = {
+	.attach_chip = ebu_nand_attach_chip,
+	.exec_op = ebu_nand_exec_op,
+};
+
+static void ebu_dma_cleanup(struct ebu_nand_controller *ebu_host)
+{
+	if (ebu_host->dma_rx) {
+		dma_release_channel(ebu_host->dma_rx);
+		ebu_host->dma_rx = NULL;
+	}
+
+	if (ebu_host->dma_tx) {
+		dma_release_channel(ebu_host->dma_tx);
+		ebu_host->dma_tx = NULL;
+	}
+}
+
+static int ebu_nand_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct ebu_nand_controller *ebu_host;
+	struct nand_chip *nand;
+	phys_addr_t nandaddr_pa;
+	struct mtd_info *mtd;
+	struct resource *res;
+	int ret;
+	u32 cs;
+
+	ebu_host = devm_kzalloc(dev, sizeof(*ebu_host), GFP_KERNEL);
+	if (!ebu_host)
+		return -ENOMEM;
+
+	ebu_host->dev = dev;
+	nand_controller_init(&ebu_host->controller);
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ebunand");
+	ebu_host->ebu_addr = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(ebu_host->ebu_addr))
+		return PTR_ERR(ebu_host->ebu_addr);
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "hsnand");
+	ebu_host->nand_addr = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(ebu_host->nand_addr))
+		return PTR_ERR(ebu_host->nand_addr);
+
+	ret = device_property_read_u32(dev, "nand,cs", &cs);
+	if (ret) {
+		dev_err(dev, "failed to get chip select: %d\n", ret);
+		return ret;
+	}
+
+	ebu_host->cs = cs;
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "nand_cs0");
+	ebu_host->chip_addr = devm_ioremap_resource(&pdev->dev, res);
+	nandaddr_pa = res->start;
+	if (IS_ERR(ebu_host->chip_addr))
+		return PTR_ERR(ebu_host->chip_addr);
+
+	ebu_host->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(ebu_host->clk)) {
+		ret = PTR_ERR(ebu_host->clk);
+		dev_err(dev, "failed to get clock: %d\n", ret);
+		return ret;
+	}
+
+	ret = clk_prepare_enable(ebu_host->clk);
+	if (ret) {
+		dev_err(dev, "failed to enable clock: %d\n", ret);
+		return ret;
+	}
+	ebu_host->clk_rate = clk_get_rate(ebu_host->clk);
+
+	ebu_host->dma_tx = dma_request_chan(dev, "tx");
+	if (IS_ERR(ebu_host->dma_tx)) {
+		ret = PTR_ERR(ebu_host->dma_tx);
+		dev_err(dev, "DMA tx channel request fail!.\n");
+		goto exit_dma;
+	}
+
+	ebu_host->dma_rx = dma_request_chan(dev, "rx");
+	if (IS_ERR(ebu_host->dma_rx)) {
+		ret = PTR_ERR(ebu_host->dma_rx);
+		dev_err(dev, "DMA tx channel request fail!.\n");
+		goto exit_dma;
+	}
+
+	writel(lower_32_bits(nandaddr_pa) | EBU_ADDR_SEL_REGEN | EBU_ADDR_MASK,
+	       ebu_host->ebu_addr + EBU_ADDR_SEL(cs));
+
+	writel(EBU_BUSCON_CMULT_V4 | EBU_BUSCON_RECOVC(1) |
+	       EBU_BUSCON_HOLDC(1) | EBU_BUSCON_WAITRDC(2) |
+	       EBU_BUSCON_WAITWRC(2) | EBU_BUSCON_BCGEN_CS | EBU_BUSCON_ALEC |
+	       EBU_BUSCON_SETUP_EN, ebu_host->ebu_addr + EBU_BUSCON(cs));
+
+	/*
+	 * NAND physical address selection is based on the chip select
+	 * and written to ADDR_SEL register to get Memory Region Base address.
+	 * FPI Bus addresses are compared to this base address in conjunction
+	 * with the mask control. Driver need to program this field!
+	 * Address: 0x17400 if chip select is CS_0
+	 * Address: 0x17C00 if chip select is CS_1
+	 */
+	writel(EBU_MEM_BASE_CS_0 + EBU_MEM_OFFSET,
+	       ebu_host->ebu_addr + EBU_ADDR_SEL(0));
+	writel(EBU_MEM_BASE_CS_1 + EBU_MEM_OFFSET,
+	       ebu_host->ebu_addr + EBU_ADDR_SEL(cs));
+	nand_set_flash_node(&ebu_host->chip, dev->of_node);
+	mtd = nand_to_mtd(&ebu_host->chip);
+	mtd->dev.parent = dev;
+	ebu_host->dev = dev;
+
+	platform_set_drvdata(pdev, ebu_host);
+	nand_set_controller_data(&ebu_host->chip, ebu_host);
+
+	nand = &ebu_host->chip;
+	nand->controller = &ebu_host->controller;
+	nand->controller->ops = &ebu_nand_controller_ops;
+
+	/* Scan to find existence of the device */
+	ret = nand_scan(&ebu_host->chip, 1);
+	if (ret)
+		goto exit_dma;
+
+	ret = mtd_device_register(mtd, NULL, 0);
+	if (ret)
+		goto clean_nand;
+
+	return 0;
+
+clean_nand:
+	nand_cleanup(&ebu_host->chip);
+exit_dma:
+	ebu_dma_cleanup(ebu_host);
+	clk_disable_unprepare(ebu_host->clk);
+
+	return ret;
+}
+
+static int ebu_nand_remove(struct platform_device *pdev)
+{
+	struct ebu_nand_controller *ebu_host = platform_get_drvdata(pdev);
+
+	mtd_device_unregister(nand_to_mtd(&ebu_host->chip));
+	nand_cleanup(&ebu_host->chip);
+	clk_disable_unprepare(ebu_host->clk);
+	ebu_dma_cleanup(ebu_host);
+
+	return 0;
+}
+
+static const struct of_device_id ebu_nand_match[] = {
+	{ .compatible = "intel,nand-controller", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, ebu_nand_match);
+
+static struct platform_driver ebu_nand_driver = {
+	.probe = ebu_nand_probe,
+	.remove = ebu_nand_remove,
+	.driver = {
+		.name = "intel-nand-controller",
+		.of_match_table = ebu_nand_match,
+	},
+
+};
+module_platform_driver(ebu_nand_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Vadivel Murugan R <vadivel.muruganx.ramuthevar@intel.com>");
+MODULE_DESCRIPTION("Intel's LGM External Bus NAND Controller driver");
-- 
2.11.0

Re: [PATCH v3 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC

[Boris Brezillon]

On Fri, 24 Apr 2020 00:21:13 +0800
"Ramuthevar, Vadivel MuruganX"
<vadivel.muruganx.ramuthevar@linux.intel.com> wrote:

> +
> +#define EBU_CLC			0x000
> +#define EBU_CLC_RST		0x00000000u
> +
> +#define	EBU_ADDR_SEL(n)		(0x20 + (n) * 4)

	   ^ Please replace those tabs by a single space.

> +#define	EBU_ADDR_MASK		(5 << 4)
> +#define	EBU_ADDR_SEL_REGEN	0x1
> +
> +#define EBU_BUSCON(n)		(0x60 + (n) * 4)
> +#define EBU_BUSCON_CMULT_V4	0x1
> +#define EBU_BUSCON_RECOVC(n)	((n) << 2)
> +#define EBU_BUSCON_HOLDC(n)	((n) << 4)
> +#define EBU_BUSCON_WAITRDC(n)	((n) << 6)
> +#define EBU_BUSCON_WAITWRC(n)	((n) << 8)
> +#define EBU_BUSCON_BCGEN_CS	0x0
> +#define EBU_BUSCON_SETUP_EN	BIT(22)
> +#define EBU_BUSCON_ALEC		0xC000
> +
> +#define EBU_CON			0x0B0
> +#define EBU_CON_NANDM_EN	BIT(0)
> +#define EBU_CON_NANDM_DIS	0x0
> +#define EBU_CON_CSMUX_E_EN	BIT(1)
> +#define EBU_CON_ALE_P_LOW	BIT(2)
> +#define EBU_CON_CLE_P_LOW	BIT(3)
> +#define EBU_CON_CS_P_LOW	BIT(4)
> +#define EBU_CON_SE_P_LOW	BIT(5)
> +#define EBU_CON_WP_P_LOW	BIT(6)
> +#define EBU_CON_PRE_P_LOW	BIT(7)
> +#define EBU_CON_IN_CS_S(n)	((n) << 8)
> +#define EBU_CON_OUT_CS_S(n)	((n) << 10)
> +#define EBU_CON_LAT_EN_CS_P	((0x3D) << 18)
> +
> +#define EBU_WAIT		0x0B4
> +#define EBU_WAIT_RDBY		BIT(0)
> +#define EBU_WAIT_WR_C		BIT(3)
> +
> +#define	EBU_MEM_BASE_CS_0	0x17400
> +#define	EBU_MEM_BASE_CS_1	0x17C00

If I refer to v2, that's actually 0x17400000 and 0x17C00000.
And I suspect those addresses come from the physical addresses of the
EBU range attached to those CS lines, which can be extracted
from the resource passed to the driver. But maybe I'm wrong.

> +#define	EBU_MEM_OFFSET		0x051

Sorry but that's too vague. What is this mask/offset encoding?

> +
> +#define HSNAND_CTL1		0x110
> +#define HSNAND_CTL1_ADDR_SHIFT	24
> +
> +#define HSNAND_CTL2		0x114
> +#define HSNAND_CTL2_ADDR_SHIFT	8
> +#define HSNAND_CTL2_CYC_N_V5	(0x2 << 16)
> +
> +#define HSNAND_INT_MSK_CTL	0x124
> +#define HSNAND_INT_MSK_CTL_WR_C	BIT(4)
> +
> +#define HSNAND_INT_STA		0x128
> +#define HSNAND_INT_STA_WR_C	BIT(4)
> +
> +#define HSNAND_CTL		0x130
> +#define HSNAND_CTL_MODE_ECC	0x1

#define HSNAND_CTL_ENABLE_ECC	BIT(0)

> +#define HSNAND_CTL_GO		BIT(2)
> +#define HSNAND_CTL_CE_SEL_CS(n)	BIT(3 + (n))
> +#define HSNAND_CTL_RW_READ	0x0
> +#define HSNAND_CTL_RW_WRITE	BIT(10)
> +#define HSNAND_CTL_ECC_OFF_V8TH	BIT(11)
> +#define HSNAND_CTL_CKFF_EN	0x0
> +#define HSNAND_CTL_MSG_EN	BIT(17)
> +
> +#define NAND_PARA0		0x13c

Why are some of those registers prefixed NAND and others HSNAND? I
guess HS stands for High-Speed.

> +#define NAND_PARA0_PAGE_V8192	0x3
> +#define NAND_PARA0_PIB_V256	(0x3 << 4)
> +#define NAND_PARA0_BYP_EN_NP	0x0
> +#define NAND_PARA0_BYP_DEC_NP	0x0
> +#define NAND_PARA0_TYPE_ONFI	BIT(18)
> +#define NAND_PARA0_ADEP_EN	BIT(21)
> +
> +#define NAND_CMSG_0		0x150
> +#define NAND_CMSG_1		0x154
> +
> +#define NAND_ALE_OFFS		BIT(2)
> +#define NAND_CLE_OFFS		BIT(3)
> +#define NAND_CS_OFFS		BIT(4)
> +
> +#define NAND_WRITE_CMD		(NAND_CS_OFFS | NAND_CLE_OFFS)
> +#define NAND_WRITE_ADDR		(NAND_CS_OFFS | NAND_ALE_OFFS)

Those macros are no longer used, you can drop them.

> +#define NAND_WRITE_DATA		EBU_CON_CS_P_LOW
> +#define NAND_READ_DATA		EBU_CON_CS_P_LOW

And those should not be used, please use NAND_CS_OFFS instead.

> +
> +#define NAND_ECC_OFFSET		0x008
> +
> +struct ebu_nand_controller {
> +	struct nand_controller	controller;

Can you drop the tabs and use one space between the type and field name?

> +	struct nand_chip	chip;

Can you add a comment explaining that this should be reworked if
someone needs to support more than one chip.

> +	void __iomem		*ebu_addr;
> +	void __iomem		*nand_addr;

Can we get rid of those _addr and rename those fields so they describe
more precisely what the MMIO range is about:

	void __iomem *ebu;
	void __iomem *hsnand;


> +	void __iomem		*chip_addr;

This field should be part of a specialized nand_chip object (see what's
done here [1]). More generally, you should move anything that's
chip-specific to this driver-specific nand_chip object. 

> +	struct clk		*clk;
> +	unsigned long		clk_rate;
> +	u32			cs;
> +	u32			nd_para0;

The cs and nd_para0 fields should be attached to the ebu_nand_chip
object too (see [1]). While you move that, please add a comment saying
that you currently only support single-CE chips.

> +	struct device		*dev;
> +	struct dma_chan		*dma_tx;
> +	struct dma_chan		*dma_rx;
> +	struct completion	dma_access_complete;

Maybe

	struct {
		struct dma_chan *tx;
		struct dma_chan *rx;
		struct completion complete;
	} dma;

> +	const char *cs_name;

You no longer use this field.

> +};
> +
> +static inline struct ebu_nand_controller *nand_to_ebu(struct nand_chip *chip)
> +{
> +	return container_of(chip, struct ebu_nand_controller, chip);
> +}
> +
> +static u8 ebu_nand_readb(struct nand_chip *chip, unsigned int op)
> +{
> +	struct ebu_nand_controller *ebu_host = nand_get_controller_data(chip);
> +	void __iomem *nand_wait = ebu_host->ebu_addr + EBU_WAIT;
> +	u32 stat;
> +	int ret;
> +	u8 val;
> +
> +	val = readb(ebu_host->chip_addr + op);

No need for op here, just pass NAND_CS_OFFS directly, and you can drop
the op argument.

> +
> +	ret = readl_poll_timeout(nand_wait, stat, stat & EBU_WAIT_WR_C,
> +				 20, 1000);
> +	if (ret)
> +		dev_warn(ebu_host->dev,
> +			 "ebu nand write timeout. nand_wait(0x%p)=0x%x\n",
> +			 nand_wait, readl(nand_wait));

Maybe dev_warn_ratelimited().

> +
> +	return val;
> +}
> +
> +static void ebu_nand_writeb(struct nand_chip *chip, int op, u8 value)

							^u32 offset,

> +{
> +	struct ebu_nand_controller *ebu_host = nand_get_controller_data(chip);
> +	void __iomem *nand_wait = ebu_host->ebu_addr + EBU_WAIT;
> +	u32 stat;
> +	int ret;
> +
> +	writeb(value, ebu_host->chip_addr + op);
> +
> +	ret = readl_poll_timeout(nand_wait, stat, stat & EBU_WAIT_WR_C,
> +				 20, 1000);
> +	if (ret)
> +		dev_warn(ebu_host->dev,
> +			 "ebu nand write timeout. nand_wait(0x%p)=0x%x\n",
> +			 nand_wait, readl(nand_wait));
> +}
> +
> +static void ebu_read_buf(struct nand_chip *chip, u_char *buf, int len)

						      ^void *	   ^unsigned int

Please update all of those.

> +{
> +	int i;
> +
> +	for (i = 0; i < len; i++)
> +		buf[i] = ebu_nand_readb(chip, NAND_READ_DATA);
> +}
> +
> +static void ebu_write_buf(struct nand_chip *chip, const u_char *buf, int len)
> +{
> +	int i;
> +
> +	for (i = 0; i < len; i++)
> +		ebu_nand_writeb(chip, NAND_WRITE_DATA, buf[i]);

				      ^NAND_CS_OFFS

> +}
> +
> +static void ebu_unselect_chip(struct nand_chip *chip)
> +{
> +	struct ebu_nand_controller *ebu_host = nand_get_controller_data(chip);
> +	void __iomem *nand_con = ebu_host->ebu_addr + EBU_CON;
> +	int val;

	^u32

> +
> +	val = readl(nand_con);
> +	writel(val & ~EBU_CON_NANDM_EN, nand_con);

And I suspect you could just do:

	writel(0, ebu_host->ebu + EBU_CON);

here.

BTW, maybe we don't have to unselect the chip and can instead do this
'disable NAND module' in a runtime PM hook when the NAND has been
unused for some time, but let's keep that for later.

> +}

...

> +static int ebu_nand_exec_op(struct nand_chip *chip,
> +			    const struct nand_operation *op, bool check_only)
> +{
> +	struct ebu_nand_controller *ctrl = nand_to_ebu(chip);
> +	const struct nand_op_instr *instr = NULL;
> +	unsigned int op_id;
> +	int i, time_out, ret = 0;
> +	u32 stat;
> +
> +	ebu_select_chip(chip);
> +
> +	for (op_id = 0; op_id < op->ninstrs; op_id++) {
> +		instr = &op->instrs[op_id];
> +
> +		switch (instr->type) {
> +		case NAND_OP_CMD_INSTR:
> +			ebu_nand_writeb(chip, NAND_CLE_OFFS,

Hm, are you sure you shouldn't add NAND_CS_OFFS here? I'd expect
the CS to be asserted when the CLE/ALE pin is. Or maybe NAND_CS_OFFS
is not about CS pin assertion/de-assertion, in which case this should
be documented (and the name should be changed accordingly).

> +					instr->ctx.cmd.opcode);
> +			break;
> +
> +		case NAND_OP_ADDR_INSTR:
> +			for (i = 0; i < instr->ctx.addr.naddrs; i++)
> +				ebu_nand_writeb(chip, NAND_ALE_OFFS,
> +						instr->ctx.addr.addrs[i]);
> +			break;
> +
> +		case NAND_OP_DATA_IN_INSTR:
> +			ebu_read_buf(chip, instr->ctx.data.buf.in,
> +				     instr->ctx.data.len);
> +			break;
> +
> +		case NAND_OP_DATA_OUT_INSTR:
> +			ebu_write_buf(chip, instr->ctx.data.buf.out,
> +				      instr->ctx.data.len);
> +			break;
> +
> +		case NAND_OP_WAITRDY_INSTR:
> +			time_out = instr->ctx.waitrdy.timeout_ms * 1000;
> +			ret = readl_poll_timeout(ctrl->ebu_addr + EBU_WAIT,
> +						 stat, stat & EBU_WAIT_RDBY,
> +						 20, time_out);
> +			break;
> +		}
> +	}

Yay! I really like this new version of exec_op().

> +
> +	ebu_unselect_chip(chip);
> +
> +	return ret;
> +}
> +
> +static const struct nand_controller_ops ebu_nand_controller_ops = {
> +	.attach_chip = ebu_nand_attach_chip,
> +	.exec_op = ebu_nand_exec_op,
> +};
> +
> +static void ebu_dma_cleanup(struct ebu_nand_controller *ebu_host)
> +{
> +	if (ebu_host->dma_rx) {
> +		dma_release_channel(ebu_host->dma_rx);
> +		ebu_host->dma_rx = NULL;

You don't need to reset the value here.

> +	}
> +
> +	if (ebu_host->dma_tx) {
> +		dma_release_channel(ebu_host->dma_tx);
> +		ebu_host->dma_tx = NULL;
> +	}
> +}
> +
> +static int ebu_nand_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct ebu_nand_controller *ebu_host;
> +	struct nand_chip *nand;
> +	phys_addr_t nandaddr_pa;
> +	struct mtd_info *mtd;
> +	struct resource *res;
> +	int ret;
> +	u32 cs;
> +
> +	ebu_host = devm_kzalloc(dev, sizeof(*ebu_host), GFP_KERNEL);
> +	if (!ebu_host)
> +		return -ENOMEM;
> +
> +	ebu_host->dev = dev;
> +	nand_controller_init(&ebu_host->controller);
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ebunand");
> +	ebu_host->ebu_addr = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(ebu_host->ebu_addr))
> +		return PTR_ERR(ebu_host->ebu_addr);
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "hsnand");
> +	ebu_host->nand_addr = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(ebu_host->nand_addr))
> +		return PTR_ERR(ebu_host->nand_addr);
> +
> +	ret = device_property_read_u32(dev, "nand,cs", &cs);

CS ids should be encoded in the reg property (see [1]).

> +	if (ret) {
> +		dev_err(dev, "failed to get chip select: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ebu_host->cs = cs;
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "nand_cs0");

	resname = devm_kasprintf("nand-cs%d", cs);
	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, resname);

> +	ebu_host->chip_addr = devm_ioremap_resource(&pdev->dev, res);
> +	nandaddr_pa = res->start;
> +	if (IS_ERR(ebu_host->chip_addr))
> +		return PTR_ERR(ebu_host->chip_addr);
> +
> +	ebu_host->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(ebu_host->clk)) {
> +		ret = PTR_ERR(ebu_host->clk);
> +		dev_err(dev, "failed to get clock: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(ebu_host->clk);
> +	if (ret) {
> +		dev_err(dev, "failed to enable clock: %d\n", ret);
> +		return ret;
> +	}
> +	ebu_host->clk_rate = clk_get_rate(ebu_host->clk);
> +
> +	ebu_host->dma_tx = dma_request_chan(dev, "tx");
> +	if (IS_ERR(ebu_host->dma_tx)) {
> +		ret = PTR_ERR(ebu_host->dma_tx);
> +		dev_err(dev, "DMA tx channel request fail!.\n");
> +		goto exit_dma;
> +	}
> +
> +	ebu_host->dma_rx = dma_request_chan(dev, "rx");
> +	if (IS_ERR(ebu_host->dma_rx)) {
> +		ret = PTR_ERR(ebu_host->dma_rx);
> +		dev_err(dev, "DMA tx channel request fail!.\n");
> +		goto exit_dma;
> +	}
> +
> +	writel(lower_32_bits(nandaddr_pa) | EBU_ADDR_SEL_REGEN | EBU_ADDR_MASK,
> +	       ebu_host->ebu_addr + EBU_ADDR_SEL(cs));
> +
> +	writel(EBU_BUSCON_CMULT_V4 | EBU_BUSCON_RECOVC(1) |
> +	       EBU_BUSCON_HOLDC(1) | EBU_BUSCON_WAITRDC(2) |
> +	       EBU_BUSCON_WAITWRC(2) | EBU_BUSCON_BCGEN_CS | EBU_BUSCON_ALEC |
> +	       EBU_BUSCON_SETUP_EN, ebu_host->ebu_addr + EBU_BUSCON(cs));

Again, can you try to implement ->setup_data_interface()?

> +
> +	/*
> +	 * NAND physical address selection is based on the chip select
> +	 * and written to ADDR_SEL register to get Memory Region Base address.
> +	 * FPI Bus addresses are compared to this base address in conjunction
> +	 * with the mask control. Driver need to program this field!
> +	 * Address: 0x17400 if chip select is CS_0
> +	 * Address: 0x17C00 if chip select is CS_1
> +	 */
> +	writel(EBU_MEM_BASE_CS_0 + EBU_MEM_OFFSET,
> +	       ebu_host->ebu_addr + EBU_ADDR_SEL(0));
> +	writel(EBU_MEM_BASE_CS_1 + EBU_MEM_OFFSET,
> +	       ebu_host->ebu_addr + EBU_ADDR_SEL(cs));

See my comments on those values. I feel like the mapping should be
created based on information we gather from somewhere else.

> +	nand_set_flash_node(&ebu_host->chip, dev->of_node);
> +	mtd = nand_to_mtd(&ebu_host->chip);
> +	mtd->dev.parent = dev;
> +	ebu_host->dev = dev;
> +
> +	platform_set_drvdata(pdev, ebu_host);
> +	nand_set_controller_data(&ebu_host->chip, ebu_host);
> +
> +	nand = &ebu_host->chip;
> +	nand->controller = &ebu_host->controller;
> +	nand->controller->ops = &ebu_nand_controller_ops;
> +
> +	/* Scan to find existence of the device */
> +	ret = nand_scan(&ebu_host->chip, 1);
> +	if (ret)
> +		goto exit_dma;
> +
> +	ret = mtd_device_register(mtd, NULL, 0);
> +	if (ret)
> +		goto clean_nand;
> +
> +	return 0;
> +
> +clean_nand:

err_cleanup_nand:

> +	nand_cleanup(&ebu_host->chip);
> +exit_dma:

err_cleanup_dma:

> +	ebu_dma_cleanup(ebu_host);
> +	clk_disable_unprepare(ebu_host->clk);
> +
> +	return ret;
> +}
> +
> +static int ebu_nand_remove(struct platform_device *pdev)
> +{
> +	struct ebu_nand_controller *ebu_host = platform_get_drvdata(pdev);
> +
> +	mtd_device_unregister(nand_to_mtd(&ebu_host->chip));

Check the return value of this function please.


[1]https://elixir.bootlin.com/linux/v5.6/source/drivers/mtd/nand/raw/atmel/nand-controller.c#L148

Re: [PATCH v3 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC

[Ramuthevar, Vadivel MuruganX]

Hi Boris,

     Thank you  very much for review comments and your time...
On 25/4/2020 12:36 am, Boris Brezillon wrote:
 > On Fri, 24 Apr 2020 00:21:13 +0800
 > "Ramuthevar, Vadivel MuruganX"
 > <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
 >
 >> +
 >> +#define EBU_CLC			0x000
 >> +#define EBU_CLC_RST		0x00000000u
 >> +
 >> +#define	EBU_ADDR_SEL(n)		(0x20 + (n) * 4)
 > 	   ^ Please replace those tabs by a single space.
Noted.
 >> +#define	EBU_ADDR_MASK		(5 << 4)
 >> +#define	EBU_ADDR_SEL_REGEN	0x1
 >> +
 >> +#define EBU_BUSCON(n)		(0x60 + (n) * 4)
 >> +#define EBU_BUSCON_CMULT_V4	0x1
 >> +#define EBU_BUSCON_RECOVC(n)	((n) << 2)
 >> +#define EBU_BUSCON_HOLDC(n)	((n) << 4)
 >> +#define EBU_BUSCON_WAITRDC(n)	((n) << 6)
 >> +#define EBU_BUSCON_WAITWRC(n)	((n) << 8)
 >> +#define EBU_BUSCON_BCGEN_CS	0x0
 >> +#define EBU_BUSCON_SETUP_EN	BIT(22)
 >> +#define EBU_BUSCON_ALEC		0xC000
 >> +
 >> +#define EBU_CON			0x0B0
 >> +#define EBU_CON_NANDM_EN	BIT(0)
 >> +#define EBU_CON_NANDM_DIS	0x0
 >> +#define EBU_CON_CSMUX_E_EN	BIT(1)
 >> +#define EBU_CON_ALE_P_LOW	BIT(2)
 >> +#define EBU_CON_CLE_P_LOW	BIT(3)
 >> +#define EBU_CON_CS_P_LOW	BIT(4)
 >> +#define EBU_CON_SE_P_LOW	BIT(5)
 >> +#define EBU_CON_WP_P_LOW	BIT(6)
 >> +#define EBU_CON_PRE_P_LOW	BIT(7)
 >> +#define EBU_CON_IN_CS_S(n)	((n) << 8)
 >> +#define EBU_CON_OUT_CS_S(n)	((n) << 10)
 >> +#define EBU_CON_LAT_EN_CS_P	((0x3D) << 18)
 >> +
 >> +#define EBU_WAIT		0x0B4
 >> +#define EBU_WAIT_RDBY		BIT(0)
 >> +#define EBU_WAIT_WR_C		BIT(3)
 >> +
 >> +#define	EBU_MEM_BASE_CS_0	0x17400
 >> +#define	EBU_MEM_BASE_CS_1	0x17C00
 > If I refer to v2, that's actually 0x17400000 and 0x17C00000.
 > And I suspect those addresses come from the physical addresses of the
 > EBU range attached to those CS lines, which can be extracted
 > from the resource passed to the driver. But maybe I'm wrong.
Agreed!, will update.
 >> +#define	EBU_MEM_OFFSET		0x051
 > Sorry but that's too vague. What is this mask/offset encoding?
it's not offset, wrongly mentioned name as offset, bit field values.
will correct it.
 >> +
 >> +#define HSNAND_CTL1		0x110
 >> +#define HSNAND_CTL1_ADDR_SHIFT	24
 >> +
 >> +#define HSNAND_CTL2		0x114
 >> +#define HSNAND_CTL2_ADDR_SHIFT	8
 >> +#define HSNAND_CTL2_CYC_N_V5	(0x2 << 16)
 >> +
 >> +#define HSNAND_INT_MSK_CTL	0x124
 >> +#define HSNAND_INT_MSK_CTL_WR_C	BIT(4)
 >> +
 >> +#define HSNAND_INT_STA		0x128
 >> +#define HSNAND_INT_STA_WR_C	BIT(4)
 >> +
 >> +#define HSNAND_CTL		0x130
 >> +#define HSNAND_CTL_MODE_ECC	0x1
 > #define HSNAND_CTL_ENABLE_ECC	BIT(0)
Noted, will update
 >> +#define HSNAND_CTL_GO		BIT(2)
 >> +#define HSNAND_CTL_CE_SEL_CS(n)	BIT(3 + (n))
 >> +#define HSNAND_CTL_RW_READ	0x0
 >> +#define HSNAND_CTL_RW_WRITE	BIT(10)
 >> +#define HSNAND_CTL_ECC_OFF_V8TH	BIT(11)
 >> +#define HSNAND_CTL_CKFF_EN	0x0
 >> +#define HSNAND_CTL_MSG_EN	BIT(17)
 >> +
 >> +#define NAND_PARA0		0x13c
 > Why are some of those registers prefixed NAND and others HSNAND? I
 > guess HS stands for High-Speed.
To differentiate the common HSNAND registers between MIPS and X86 based 
SoC's
 >> +#define NAND_PARA0_PAGE_V8192	0x3
 >> +#define NAND_PARA0_PIB_V256	(0x3 << 4)
 >> +#define NAND_PARA0_BYP_EN_NP	0x0
 >> +#define NAND_PARA0_BYP_DEC_NP	0x0
 >> +#define NAND_PARA0_TYPE_ONFI	BIT(18)
 >> +#define NAND_PARA0_ADEP_EN	BIT(21)
 >> +
 >> +#define NAND_CMSG_0		0x150
 >> +#define NAND_CMSG_1		0x154
 >> +
 >> +#define NAND_ALE_OFFS		BIT(2)
 >> +#define NAND_CLE_OFFS		BIT(3)
 >> +#define NAND_CS_OFFS		BIT(4)
 >> +
 >> +#define NAND_WRITE_CMD		(NAND_CS_OFFS | NAND_CLE_OFFS)
 >> +#define NAND_WRITE_ADDR		(NAND_CS_OFFS | NAND_ALE_OFFS)
 > Those macros are no longer used, you can drop them.
Noted, will drop them
 >> +#define NAND_WRITE_DATA		EBU_CON_CS_P_LOW
 >> +#define NAND_READ_DATA		EBU_CON_CS_P_LOW
 > And those should not be used, please use NAND_CS_OFFS instead.
Sure, Noted
 >> +
 >> +#define NAND_ECC_OFFSET		0x008
 >> +
 >> +struct ebu_nand_controller {
 >> +	struct nand_controller	controller;
 > Can you drop the tabs and use one space between the type and field name?
Noted
 >> +	struct nand_chip	chip;
 > Can you add a comment explaining that this should be reworked if
 > someone needs to support more than one chip.
Sure, will add comments
 >> +	void __iomem		*ebu_addr;
 >> +	void __iomem		*nand_addr;
 > Can we get rid of those _addr and rename those fields so they describe
 > more precisely what the MMIO range is about:
 >
 > 	void __iomem *ebu;
 > 	void __iomem *hsnand;
 >
Noted
 >> +	void __iomem		*chip_addr;
 > This field should be part of a specialized nand_chip object (see what's
 > done here [1]). More generally, you should move anything that's
 > chip-specific to this driver-specific nand_chip object.
I supposed to add like nand_cs, will change it.
 >> +	struct clk		*clk;
 >> +	unsigned long		clk_rate;
 >> +	u32			cs;
 >> +	u32			nd_para0;
 > The cs and nd_para0 fields should be attached to the ebu_nand_chip
 > object too (see [1]). While you move that, please add a comment saying
 > that you currently only support single-CE chips.
Yes, you are correct, will do that
 >> +	struct device		*dev;
 >> +	struct dma_chan		*dma_tx;
 >> +	struct dma_chan		*dma_rx;
 >> +	struct completion	dma_access_complete;
 > Maybe
 >
 > 	struct {
 > 		struct dma_chan *tx;
 > 		struct dma_chan *rx;
 > 		struct completion complete;
 > 	} dma;
Ok, Noted
 >> +	const char *cs_name;
 > You no longer use this field.
Noted, will drop it.
 >> +};
 >> +
 >> +static inline struct ebu_nand_controller *nand_to_ebu(struct 
nand_chip *chip)
 >> +{
 >> +	return container_of(chip, struct ebu_nand_controller, chip);
 >> +}
 >> +
 >> +static u8 ebu_nand_readb(struct nand_chip *chip, unsigned int op)
 >> +{
 >> +	struct ebu_nand_controller *ebu_host = nand_get_controller_data(chip);
 >> +	void __iomem *nand_wait = ebu_host->ebu_addr + EBU_WAIT;
 >> +	u32 stat;
 >> +	int ret;
 >> +	u8 val;
 >> +
 >> +	val = readb(ebu_host->chip_addr + op);
 > No need for op here, just pass NAND_CS_OFFS directly, and you can drop
 > the op argument.
Noted, will update.
 >> +
 >> +	ret = readl_poll_timeout(nand_wait, stat, stat & EBU_WAIT_WR_C,
 >> +				 20, 1000);
 >> +	if (ret)
 >> +		dev_warn(ebu_host->dev,
 >> +			 "ebu nand write timeout. nand_wait(0x%p)=0x%x\n",
 >> +			 nand_wait, readl(nand_wait));
 > Maybe dev_warn_ratelimited().
Not required, might be used for network drivers.
 >> +
 >> +	return val;
 >> +}
 >> +
 >> +static void ebu_nand_writeb(struct nand_chip *chip, int op, u8 value)
 > 							^u32 offset,
 >
 >> +{
 >> +	struct ebu_nand_controller *ebu_host = nand_get_controller_data(chip);
 >> +	void __iomem *nand_wait = ebu_host->ebu_addr + EBU_WAIT;
 >> +	u32 stat;
 >> +	int ret;
 >> +
 >> +	writeb(value, ebu_host->chip_addr + op);
 >> +
 >> +	ret = readl_poll_timeout(nand_wait, stat, stat & EBU_WAIT_WR_C,
 >> +				 20, 1000);
 >> +	if (ret)
 >> +		dev_warn(ebu_host->dev,
 >> +			 "ebu nand write timeout. nand_wait(0x%p)=0x%x\n",
 >> +			 nand_wait, readl(nand_wait));
 >> +}
 >> +
 >> +static void ebu_read_buf(struct nand_chip *chip, u_char *buf, int len)
 > 						      ^void *	   ^unsigned int
 >
 > Please update all of those.
Sure, will update all.
 >> +{
 >> +	int i;
 >> +
 >> +	for (i = 0; i < len; i++)
 >> +		buf[i] = ebu_nand_readb(chip, NAND_READ_DATA);
 >> +}
 >> +
 >> +static void ebu_write_buf(struct nand_chip *chip, const u_char 
*buf, int len)
 >> +{
 >> +	int i;
 >> +
 >> +	for (i = 0; i < len; i++)
 >> +		ebu_nand_writeb(chip, NAND_WRITE_DATA, buf[i]);
 > 				      ^NAND_CS_OFFS
Noted.
 >> +}
 >> +
 >> +static void ebu_unselect_chip(struct nand_chip *chip)
 >> +{
 >> +	struct ebu_nand_controller *ebu_host = nand_get_controller_data(chip);
 >> +	void __iomem *nand_con = ebu_host->ebu_addr + EBU_CON;
 >> +	int val;
 > 	^u32
 >
 >> +
 >> +	val = readl(nand_con);
 >> +	writel(val & ~EBU_CON_NANDM_EN, nand_con);
 > And I suspect you could just do:
 >
 > 	writel(0, ebu_host->ebu + EBU_CON);
 >
 > here.
 >
 > BTW, maybe we don't have to unselect the chip and can instead do this
 > 'disable NAND module' in a runtime PM hook when the NAND has been
 > unused for some time, but let's keep that for later.
Sure, Noted.
 >> +}
 > ...
 >
 >> +static int ebu_nand_exec_op(struct nand_chip *chip,
 >> +			    const struct nand_operation *op, bool check_only)
 >> +{
 >> +	struct ebu_nand_controller *ctrl = nand_to_ebu(chip);
 >> +	const struct nand_op_instr *instr = NULL;
 >> +	unsigned int op_id;
 >> +	int i, time_out, ret = 0;
 >> +	u32 stat;
 >> +
 >> +	ebu_select_chip(chip);
 >> +
 >> +	for (op_id = 0; op_id < op->ninstrs; op_id++) {
 >> +		instr = &op->instrs[op_id];
 >> +
 >> +		switch (instr->type) {
 >> +		case NAND_OP_CMD_INSTR:
 >> +			ebu_nand_writeb(chip, NAND_CLE_OFFS,
 > Hm, are you sure you shouldn't add NAND_CS_OFFS here? I'd expect
 > the CS to be asserted when the CLE/ALE pin is. Or maybe NAND_CS_OFFS
 > is not about CS pin assertion/de-assertion, in which case this should
 > be documented (and the name should be changed accordingly).
before doing the read/write asserting the chip by chip_select function, 
let me double check
as you said, will update accordingly
 >> +					instr->ctx.cmd.opcode);
 >> +			break;
 >> +
 >> +		case NAND_OP_ADDR_INSTR:
 >> +			for (i = 0; i < instr->ctx.addr.naddrs; i++)
 >> +				ebu_nand_writeb(chip, NAND_ALE_OFFS,
 >> +						instr->ctx.addr.addrs[i]);
 >> +			break;
 >> +
 >> +		case NAND_OP_DATA_IN_INSTR:
 >> +			ebu_read_buf(chip, instr->ctx.data.buf.in,
 >> +				     instr->ctx.data.len);
 >> +			break;
 >> +
 >> +		case NAND_OP_DATA_OUT_INSTR:
 >> +			ebu_write_buf(chip, instr->ctx.data.buf.out,
 >> +				      instr->ctx.data.len);
 >> +			break;
 >> +
 >> +		case NAND_OP_WAITRDY_INSTR:
 >> +			time_out = instr->ctx.waitrdy.timeout_ms * 1000;
 >> +			ret = readl_poll_timeout(ctrl->ebu_addr + EBU_WAIT,
 >> +						 stat, stat & EBU_WAIT_RDBY,
 >> +						 20, time_out);
 >> +			break;
 >> +		}
 >> +	}
 > Yay! I really like this new version of exec_op().
Thank you Boris!
 >> +
 >> +	ebu_unselect_chip(chip);
 >> +
 >> +	return ret;
 >> +}
 >> +
 >> +static const struct nand_controller_ops ebu_nand_controller_ops = {
 >> +	.attach_chip = ebu_nand_attach_chip,
 >> +	.exec_op = ebu_nand_exec_op,
 >> +};
 >> +
 >> +static void ebu_dma_cleanup(struct ebu_nand_controller *ebu_host)
 >> +{
 >> +	if (ebu_host->dma_rx) {
 >> +		dma_release_channel(ebu_host->dma_rx);
 >> +		ebu_host->dma_rx = NULL;
 > You don't need to reset the value here.
Noted.
 >> +	}
 >> +
 >> +	if (ebu_host->dma_tx) {
 >> +		dma_release_channel(ebu_host->dma_tx);
 >> +		ebu_host->dma_tx = NULL;
 >> +	}
 >> +}
 >> +
 >> +static int ebu_nand_probe(struct platform_device *pdev)
 >> +{
 >> +	struct device *dev = &pdev->dev;
 >> +	struct ebu_nand_controller *ebu_host;
 >> +	struct nand_chip *nand;
 >> +	phys_addr_t nandaddr_pa;
 >> +	struct mtd_info *mtd;
 >> +	struct resource *res;
 >> +	int ret;
 >> +	u32 cs;
 >> +
 >> +	ebu_host = devm_kzalloc(dev, sizeof(*ebu_host), GFP_KERNEL);
 >> +	if (!ebu_host)
 >> +		return -ENOMEM;
 >> +
 >> +	ebu_host->dev = dev;
 >> +	nand_controller_init(&ebu_host->controller);
 >> +
 >> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ebunand");
 >> +	ebu_host->ebu_addr = devm_ioremap_resource(&pdev->dev, res);
 >> +	if (IS_ERR(ebu_host->ebu_addr))
 >> +		return PTR_ERR(ebu_host->ebu_addr);
 >> +
 >> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "hsnand");
 >> +	ebu_host->nand_addr = devm_ioremap_resource(&pdev->dev, res);
 >> +	if (IS_ERR(ebu_host->nand_addr))
 >> +		return PTR_ERR(ebu_host->nand_addr);
 >> +
 >> +	ret = device_property_read_u32(dev, "nand,cs", &cs);
 > CS ids should be encoded in the reg property (see [1]).
Ok, will check.
 >> +	if (ret) {
 >> +		dev_err(dev, "failed to get chip select: %d\n", ret);
 >> +		return ret;
 >> +	}
 >> +
 >> +	ebu_host->cs = cs;
 >> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "nand_cs0");
 > 	resname = devm_kasprintf("nand-cs%d", cs);
 > 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, resname);
I have kept same-way in my earlier patches , will update.
 >> +	ebu_host->chip_addr = devm_ioremap_resource(&pdev->dev, res);
 >> +	nandaddr_pa = res->start;
 >> +	if (IS_ERR(ebu_host->chip_addr))
 >> +		return PTR_ERR(ebu_host->chip_addr);
 >> +
 >> +	ebu_host->clk = devm_clk_get(dev, NULL);
 >> +	if (IS_ERR(ebu_host->clk)) {
 >> +		ret = PTR_ERR(ebu_host->clk);
 >> +		dev_err(dev, "failed to get clock: %d\n", ret);
 >> +		return ret;
 >> +	}
 >> +
 >> +	ret = clk_prepare_enable(ebu_host->clk);
 >> +	if (ret) {
 >> +		dev_err(dev, "failed to enable clock: %d\n", ret);
 >> +		return ret;
 >> +	}
 >> +	ebu_host->clk_rate = clk_get_rate(ebu_host->clk);
 >> +
 >> +	ebu_host->dma_tx = dma_request_chan(dev, "tx");
 >> +	if (IS_ERR(ebu_host->dma_tx)) {
 >> +		ret = PTR_ERR(ebu_host->dma_tx);
 >> +		dev_err(dev, "DMA tx channel request fail!.\n");
 >> +		goto exit_dma;
 >> +	}
 >> +
 >> +	ebu_host->dma_rx = dma_request_chan(dev, "rx");
 >> +	if (IS_ERR(ebu_host->dma_rx)) {
 >> +		ret = PTR_ERR(ebu_host->dma_rx);
 >> +		dev_err(dev, "DMA tx channel request fail!.\n");
 >> +		goto exit_dma;
 >> +	}
 >> +
 >> +	writel(lower_32_bits(nandaddr_pa) | EBU_ADDR_SEL_REGEN | 
EBU_ADDR_MASK,
 >> +	       ebu_host->ebu_addr + EBU_ADDR_SEL(cs));
 >> +
 >> +	writel(EBU_BUSCON_CMULT_V4 | EBU_BUSCON_RECOVC(1) |
 >> +	       EBU_BUSCON_HOLDC(1) | EBU_BUSCON_WAITRDC(2) |
 >> +	       EBU_BUSCON_WAITWRC(2) | EBU_BUSCON_BCGEN_CS | EBU_BUSCON_ALEC |
 >> +	       EBU_BUSCON_SETUP_EN, ebu_host->ebu_addr + EBU_BUSCON(cs));
 > Again, can you try to implement ->setup_data_interface()?
Sure, will implement .
 >> +
 >> +	/*
 >> +	 * NAND physical address selection is based on the chip select
 >> +	 * and written to ADDR_SEL register to get Memory Region Base address.
 >> +	 * FPI Bus addresses are compared to this base address in conjunction
 >> +	 * with the mask control. Driver need to program this field!
 >> +	 * Address: 0x17400 if chip select is CS_0
 >> +	 * Address: 0x17C00 if chip select is CS_1
 >> +	 */
 >> +	writel(EBU_MEM_BASE_CS_0 + EBU_MEM_OFFSET,
 >> +	       ebu_host->ebu_addr + EBU_ADDR_SEL(0));
 >> +	writel(EBU_MEM_BASE_CS_1 + EBU_MEM_OFFSET,
 >> +	       ebu_host->ebu_addr + EBU_ADDR_SEL(cs));
 > See my comments on those values. I feel like the mapping should be
 > created based on information we gather from somewhere else.
Yes, of-course will update the proper way.
 >> +	nand_set_flash_node(&ebu_host->chip, dev->of_node);
 >> +	mtd = nand_to_mtd(&ebu_host->chip);
 >> +	mtd->dev.parent = dev;
 >> +	ebu_host->dev = dev;
 >> +
 >> +	platform_set_drvdata(pdev, ebu_host);
 >> +	nand_set_controller_data(&ebu_host->chip, ebu_host);
 >> +
 >> +	nand = &ebu_host->chip;
 >> +	nand->controller = &ebu_host->controller;
 >> +	nand->controller->ops = &ebu_nand_controller_ops;
 >> +
 >> +	/* Scan to find existence of the device */
 >> +	ret = nand_scan(&ebu_host->chip, 1);
 >> +	if (ret)
 >> +		goto exit_dma;
 >> +
 >> +	ret = mtd_device_register(mtd, NULL, 0);
 >> +	if (ret)
 >> +		goto clean_nand;
 >> +
 >> +	return 0;
 >> +
 >> +clean_nand:
 > err_cleanup_nand:
Noted.
 >> +	nand_cleanup(&ebu_host->chip);
 >> +exit_dma:
 > err_cleanup_dma:
Noted.
 >> +	ebu_dma_cleanup(ebu_host);
 >> +	clk_disable_unprepare(ebu_host->clk);
 >> +
 >> +	return ret;
 >> +}
 >> +
 >> +static int ebu_nand_remove(struct platform_device *pdev)
 >> +{
 >> +	struct ebu_nand_controller *ebu_host = platform_get_drvdata(pdev);
 >> +
 >> +	mtd_device_unregister(nand_to_mtd(&ebu_host->chip));
 > Check the return value of this function please.

sure, will check and update.
Regards
Vadivel

Re: [PATCH v3 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC

[Miquel Raynal]

Hi Ramuthevar,

> > +static int ebu_nand_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct ebu_nand_controller *ebu_host;
> > +	struct nand_chip *nand;
> > +	phys_addr_t nandaddr_pa;
> > +	struct mtd_info *mtd;
> > +	struct resource *res;
> > +	int ret;
> > +	u32 cs;
> > +
> > +	ebu_host = devm_kzalloc(dev, sizeof(*ebu_host), GFP_KERNEL);
> > +	if (!ebu_host)
> > +		return -ENOMEM;
> > +
> > +	ebu_host->dev = dev;
> > +	nand_controller_init(&ebu_host->controller);
> > +
> > +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ebunand");
> > +	ebu_host->ebu_addr = devm_ioremap_resource(&pdev->dev, res);
> > +	if (IS_ERR(ebu_host->ebu_addr))
> > +		return PTR_ERR(ebu_host->ebu_addr);
> > +
> > +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "hsnand");
> > +	ebu_host->nand_addr = devm_ioremap_resource(&pdev->dev, res);
> > +	if (IS_ERR(ebu_host->nand_addr))
> > +		return PTR_ERR(ebu_host->nand_addr);
> > +
> > +	ret = device_property_read_u32(dev, "nand,cs", &cs);  
> 
> CS ids should be encoded in the reg property (see [1]).

Is it your choice to only support a single CS or is it actually a
controller limitation? If the latter, it would be much better I think
to anticipate the addition of the support for another CS. And in
this case there are many places in this driver that should be
more generic.

> > +	if (ret) {
> > +		dev_err(dev, "failed to get chip select: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	ebu_host->cs = cs;
> > +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "nand_cs0");  

Thanks,
Miquèl

Re: [PATCH v3 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC

[Ramuthevar, Vadivel MuruganX]

Hi Miquel,

    Thank you very much for the review comments and your time...

On 27/4/2020 11:51 pm, Miquel Raynal wrote:
> Hi Ramuthevar,
> 
>>> +static int ebu_nand_probe(struct platform_device *pdev)
>>> +{
>>> +	struct device *dev = &pdev->dev;
>>> +	struct ebu_nand_controller *ebu_host;
>>> +	struct nand_chip *nand;
>>> +	phys_addr_t nandaddr_pa;
>>> +	struct mtd_info *mtd;
>>> +	struct resource *res;
>>> +	int ret;
>>> +	u32 cs;
>>> +
>>> +	ebu_host = devm_kzalloc(dev, sizeof(*ebu_host), GFP_KERNEL);
>>> +	if (!ebu_host)
>>> +		return -ENOMEM;
>>> +
>>> +	ebu_host->dev = dev;
>>> +	nand_controller_init(&ebu_host->controller);
>>> +
>>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ebunand");
>>> +	ebu_host->ebu_addr = devm_ioremap_resource(&pdev->dev, res);
>>> +	if (IS_ERR(ebu_host->ebu_addr))
>>> +		return PTR_ERR(ebu_host->ebu_addr);
>>> +
>>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "hsnand");
>>> +	ebu_host->nand_addr = devm_ioremap_resource(&pdev->dev, res);
>>> +	if (IS_ERR(ebu_host->nand_addr))
>>> +		return PTR_ERR(ebu_host->nand_addr);
>>> +
>>> +	ret = device_property_read_u32(dev, "nand,cs", &cs);
>>
>> CS ids should be encoded in the reg property (see [1]).
> 
> Is it your choice to only support a single CS or is it actually a
> controller limitation?

Yes , its controller limitation to support only one CS

Regards
Vadivel
  If the latter, it would be much better I think
> to anticipate the addition of the support for another CS. And in
> this case there are many places in this driver that should be
> more generic.
> 
>>> +	if (ret) {
>>> +		dev_err(dev, "failed to get chip select: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	ebu_host->cs = cs;
>>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "nand_cs0");
> 
> Thanks,
> Miquèl
> 

Re: [PATCH v3 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC

[Boris Brezillon]

On Tue, 28 Apr 2020 14:17:30 +0800
"Ramuthevar, Vadivel MuruganX"
<vadivel.muruganx.ramuthevar@linux.intel.com> wrote:

> Hi Miquel,
> 
>     Thank you very much for the review comments and your time...
> 
> On 27/4/2020 11:51 pm, Miquel Raynal wrote:
> > Hi Ramuthevar,
> >   
> >>> +static int ebu_nand_probe(struct platform_device *pdev)
> >>> +{
> >>> +	struct device *dev = &pdev->dev;
> >>> +	struct ebu_nand_controller *ebu_host;
> >>> +	struct nand_chip *nand;
> >>> +	phys_addr_t nandaddr_pa;
> >>> +	struct mtd_info *mtd;
> >>> +	struct resource *res;
> >>> +	int ret;
> >>> +	u32 cs;
> >>> +
> >>> +	ebu_host = devm_kzalloc(dev, sizeof(*ebu_host), GFP_KERNEL);
> >>> +	if (!ebu_host)
> >>> +		return -ENOMEM;
> >>> +
> >>> +	ebu_host->dev = dev;
> >>> +	nand_controller_init(&ebu_host->controller);
> >>> +
> >>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ebunand");
> >>> +	ebu_host->ebu_addr = devm_ioremap_resource(&pdev->dev, res);
> >>> +	if (IS_ERR(ebu_host->ebu_addr))
> >>> +		return PTR_ERR(ebu_host->ebu_addr);
> >>> +
> >>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "hsnand");
> >>> +	ebu_host->nand_addr = devm_ioremap_resource(&pdev->dev, res);
> >>> +	if (IS_ERR(ebu_host->nand_addr))
> >>> +		return PTR_ERR(ebu_host->nand_addr);
> >>> +
> >>> +	ret = device_property_read_u32(dev, "nand,cs", &cs);  
> >>
> >> CS ids should be encoded in the reg property (see [1]).  
> > 
> > Is it your choice to only support a single CS or is it actually a
> > controller limitation?  
> 
> Yes , its controller limitation to support only one CS

I'm pretty sure that's not true, otherwise you wouldn't have to select
the CS you want to use :P.

Re: [PATCH v3 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC

[Ramuthevar, Vadivel MuruganX]

Hi Boris,

On 28/4/2020 2:27 pm, Boris Brezillon wrote:
> On Tue, 28 Apr 2020 14:17:30 +0800
> "Ramuthevar, Vadivel MuruganX"
> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
> 
>> Hi Miquel,
>>
>>      Thank you very much for the review comments and your time...
>>
>> On 27/4/2020 11:51 pm, Miquel Raynal wrote:
>>> Hi Ramuthevar,
>>>    
>>>>> +static int ebu_nand_probe(struct platform_device *pdev)
>>>>> +{
>>>>> +	struct device *dev = &pdev->dev;
>>>>> +	struct ebu_nand_controller *ebu_host;
>>>>> +	struct nand_chip *nand;
>>>>> +	phys_addr_t nandaddr_pa;
>>>>> +	struct mtd_info *mtd;
>>>>> +	struct resource *res;
>>>>> +	int ret;
>>>>> +	u32 cs;
>>>>> +
>>>>> +	ebu_host = devm_kzalloc(dev, sizeof(*ebu_host), GFP_KERNEL);
>>>>> +	if (!ebu_host)
>>>>> +		return -ENOMEM;
>>>>> +
>>>>> +	ebu_host->dev = dev;
>>>>> +	nand_controller_init(&ebu_host->controller);
>>>>> +
>>>>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ebunand");
>>>>> +	ebu_host->ebu_addr = devm_ioremap_resource(&pdev->dev, res);
>>>>> +	if (IS_ERR(ebu_host->ebu_addr))
>>>>> +		return PTR_ERR(ebu_host->ebu_addr);
>>>>> +
>>>>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "hsnand");
>>>>> +	ebu_host->nand_addr = devm_ioremap_resource(&pdev->dev, res);
>>>>> +	if (IS_ERR(ebu_host->nand_addr))
>>>>> +		return PTR_ERR(ebu_host->nand_addr);
>>>>> +
>>>>> +	ret = device_property_read_u32(dev, "nand,cs", &cs);
>>>>
>>>> CS ids should be encoded in the reg property (see [1]).
>>>
>>> Is it your choice to only support a single CS or is it actually a
>>> controller limitation?
>>
>> Yes , its controller limitation to support only one CS
> 
> I'm pretty sure that's not true, otherwise you wouldn't have to select
> the CS you want to use :P.

At a time it supports only one chip select.
Thanks!

Regards
Vadivel
> 

Re: [PATCH v3 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC

[Boris Brezillon]

On Tue, 28 Apr 2020 14:40:58 +0800
"Ramuthevar, Vadivel MuruganX"
<vadivel.muruganx.ramuthevar@linux.intel.com> wrote:

> Hi Boris,
> 
> On 28/4/2020 2:27 pm, Boris Brezillon wrote:
> > On Tue, 28 Apr 2020 14:17:30 +0800
> > "Ramuthevar, Vadivel MuruganX"
> > <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
> >   
> >> Hi Miquel,
> >>
> >>      Thank you very much for the review comments and your time...
> >>
> >> On 27/4/2020 11:51 pm, Miquel Raynal wrote:  
> >>> Hi Ramuthevar,
> >>>      
> >>>>> +static int ebu_nand_probe(struct platform_device *pdev)
> >>>>> +{
> >>>>> +	struct device *dev = &pdev->dev;
> >>>>> +	struct ebu_nand_controller *ebu_host;
> >>>>> +	struct nand_chip *nand;
> >>>>> +	phys_addr_t nandaddr_pa;
> >>>>> +	struct mtd_info *mtd;
> >>>>> +	struct resource *res;
> >>>>> +	int ret;
> >>>>> +	u32 cs;
> >>>>> +
> >>>>> +	ebu_host = devm_kzalloc(dev, sizeof(*ebu_host), GFP_KERNEL);
> >>>>> +	if (!ebu_host)
> >>>>> +		return -ENOMEM;
> >>>>> +
> >>>>> +	ebu_host->dev = dev;
> >>>>> +	nand_controller_init(&ebu_host->controller);
> >>>>> +
> >>>>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ebunand");
> >>>>> +	ebu_host->ebu_addr = devm_ioremap_resource(&pdev->dev, res);
> >>>>> +	if (IS_ERR(ebu_host->ebu_addr))
> >>>>> +		return PTR_ERR(ebu_host->ebu_addr);
> >>>>> +
> >>>>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "hsnand");
> >>>>> +	ebu_host->nand_addr = devm_ioremap_resource(&pdev->dev, res);
> >>>>> +	if (IS_ERR(ebu_host->nand_addr))
> >>>>> +		return PTR_ERR(ebu_host->nand_addr);
> >>>>> +
> >>>>> +	ret = device_property_read_u32(dev, "nand,cs", &cs);  
> >>>>
> >>>> CS ids should be encoded in the reg property (see [1]).  
> >>>
> >>> Is it your choice to only support a single CS or is it actually a
> >>> controller limitation?  
> >>
> >> Yes , its controller limitation to support only one CS  
> > 
> > I'm pretty sure that's not true, otherwise you wouldn't have to select
> > the CS you want to use :P.  
> 
> At a time it supports only one chip select.

Yes, like 99% of the NAND controllers, but that doesn't mean you can't
support multi-CS chips. All you have to do is attach an array of
ebu_nand_cs to your ebu_nand_chip (as done in the atmel driver I
pointed to). nand_operation.cs tells you which CS (index in your
ebu_nand_cs array) a specific operation is targeting, and you can pick
the right MMIO range/reg value based on that.

Re: [PATCH v3 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC

[Ramuthevar, Vadivel MuruganX]

Hi Boris,

On 28/4/2020 2:47 pm, Boris Brezillon wrote:
> On Tue, 28 Apr 2020 14:40:58 +0800
> "Ramuthevar, Vadivel MuruganX"
> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
> 
>> Hi Boris,
>>
>> On 28/4/2020 2:27 pm, Boris Brezillon wrote:
>>> On Tue, 28 Apr 2020 14:17:30 +0800
>>> "Ramuthevar, Vadivel MuruganX"
>>> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
>>>    
>>>> Hi Miquel,
>>>>
>>>>       Thank you very much for the review comments and your time...
>>>>
>>>> On 27/4/2020 11:51 pm, Miquel Raynal wrote:
>>>>> Hi Ramuthevar,
>>>>>       
>>>>>>> +static int ebu_nand_probe(struct platform_device *pdev)
>>>>>>> +{
>>>>>>> +	struct device *dev = &pdev->dev;
>>>>>>> +	struct ebu_nand_controller *ebu_host;
>>>>>>> +	struct nand_chip *nand;
>>>>>>> +	phys_addr_t nandaddr_pa;
>>>>>>> +	struct mtd_info *mtd;
>>>>>>> +	struct resource *res;
>>>>>>> +	int ret;
>>>>>>> +	u32 cs;
>>>>>>> +
>>>>>>> +	ebu_host = devm_kzalloc(dev, sizeof(*ebu_host), GFP_KERNEL);
>>>>>>> +	if (!ebu_host)
>>>>>>> +		return -ENOMEM;
>>>>>>> +
>>>>>>> +	ebu_host->dev = dev;
>>>>>>> +	nand_controller_init(&ebu_host->controller);
>>>>>>> +
>>>>>>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ebunand");
>>>>>>> +	ebu_host->ebu_addr = devm_ioremap_resource(&pdev->dev, res);
>>>>>>> +	if (IS_ERR(ebu_host->ebu_addr))
>>>>>>> +		return PTR_ERR(ebu_host->ebu_addr);
>>>>>>> +
>>>>>>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "hsnand");
>>>>>>> +	ebu_host->nand_addr = devm_ioremap_resource(&pdev->dev, res);
>>>>>>> +	if (IS_ERR(ebu_host->nand_addr))
>>>>>>> +		return PTR_ERR(ebu_host->nand_addr);
>>>>>>> +
>>>>>>> +	ret = device_property_read_u32(dev, "nand,cs", &cs);
>>>>>>
>>>>>> CS ids should be encoded in the reg property (see [1]).
>>>>>
>>>>> Is it your choice to only support a single CS or is it actually a
>>>>> controller limitation?
>>>>
>>>> Yes , its controller limitation to support only one CS
>>>
>>> I'm pretty sure that's not true, otherwise you wouldn't have to select
>>> the CS you want to use :P.
>>
>> At a time it supports only one chip select.
> 
> Yes, like 99% of the NAND controllers, but that doesn't mean you can't
> support multi-CS chips. All you have to do is attach an array of
> ebu_nand_cs to your ebu_nand_chip (as done in the atmel driver I
> pointed to). nand_operation.cs tells you which CS (index in your
> ebu_nand_cs array) a specific operation is targeting, and you can pick
> the right MMIO range/reg value based on that.

Agreed, sure I will add that and update next series of patches .

Thanks! a lot Boris

Regards
Vadivel
> 

Re: [PATCH v3 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC

[Miquel Raynal]

Hi Vadivel MuruganX,

"Ramuthevar, Vadivel MuruganX"
<vadivel.muruganx.ramuthevar@linux.intel.com> wrote on Tue, 28 Apr 2020
14:50:35 +0800:

> Hi Boris,
> 
> On 28/4/2020 2:47 pm, Boris Brezillon wrote:
> > On Tue, 28 Apr 2020 14:40:58 +0800
> > "Ramuthevar, Vadivel MuruganX"
> > <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
> >   
> >> Hi Boris,
> >>
> >> On 28/4/2020 2:27 pm, Boris Brezillon wrote:  
> >>> On Tue, 28 Apr 2020 14:17:30 +0800
> >>> "Ramuthevar, Vadivel MuruganX"
> >>> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:  
> >>>    >>>> Hi Miquel,  
> >>>>
> >>>>       Thank you very much for the review comments and your time...
> >>>>
> >>>> On 27/4/2020 11:51 pm, Miquel Raynal wrote:  
> >>>>> Hi Ramuthevar,  
> >>>>>       >>>>>>> +static int ebu_nand_probe(struct platform_device *pdev)  
> >>>>>>> +{
> >>>>>>> +	struct device *dev = &pdev->dev;
> >>>>>>> +	struct ebu_nand_controller *ebu_host;
> >>>>>>> +	struct nand_chip *nand;
> >>>>>>> +	phys_addr_t nandaddr_pa;
> >>>>>>> +	struct mtd_info *mtd;
> >>>>>>> +	struct resource *res;
> >>>>>>> +	int ret;
> >>>>>>> +	u32 cs;
> >>>>>>> +
> >>>>>>> +	ebu_host = devm_kzalloc(dev, sizeof(*ebu_host), GFP_KERNEL);
> >>>>>>> +	if (!ebu_host)
> >>>>>>> +		return -ENOMEM;
> >>>>>>> +
> >>>>>>> +	ebu_host->dev = dev;
> >>>>>>> +	nand_controller_init(&ebu_host->controller);
> >>>>>>> +
> >>>>>>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ebunand");
> >>>>>>> +	ebu_host->ebu_addr = devm_ioremap_resource(&pdev->dev, res);
> >>>>>>> +	if (IS_ERR(ebu_host->ebu_addr))
> >>>>>>> +		return PTR_ERR(ebu_host->ebu_addr);
> >>>>>>> +
> >>>>>>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "hsnand");
> >>>>>>> +	ebu_host->nand_addr = devm_ioremap_resource(&pdev->dev, res);
> >>>>>>> +	if (IS_ERR(ebu_host->nand_addr))
> >>>>>>> +		return PTR_ERR(ebu_host->nand_addr);
> >>>>>>> +
> >>>>>>> +	ret = device_property_read_u32(dev, "nand,cs", &cs);  
> >>>>>>
> >>>>>> CS ids should be encoded in the reg property (see [1]).  
> >>>>>
> >>>>> Is it your choice to only support a single CS or is it actually a
> >>>>> controller limitation?  
> >>>>
> >>>> Yes , its controller limitation to support only one CS  
> >>>
> >>> I'm pretty sure that's not true, otherwise you wouldn't have to select
> >>> the CS you want to use :P.  
> >>
> >> At a time it supports only one chip select.  
> > 
> > Yes, like 99% of the NAND controllers, but that doesn't mean you can't
> > support multi-CS chips. All you have to do is attach an array of
> > ebu_nand_cs to your ebu_nand_chip (as done in the atmel driver I
> > pointed to). nand_operation.cs tells you which CS (index in your
> > ebu_nand_cs array) a specific operation is targeting, and you can pick
> > the right MMIO range/reg value based on that.  
> 
> Agreed, sure I will add that and update next series of patches .

There are also many other places where you assume blindly that there
is only one CS. You can check the Atmel NAND controller driver as Boris
said and we will probably propose more little changes to be more
generic.

Thanks,
Miquèl

Re: [PATCH v3 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC

[Ramuthevar, Vadivel MuruganX]

Hi Miquel,

On 28/4/2020 3:40 pm, Miquel Raynal wrote:
> Hi Vadivel MuruganX,
> 
> "Ramuthevar, Vadivel MuruganX"
> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote on Tue, 28 Apr 2020
> 14:50:35 +0800:
> 
>> Hi Boris,
>>
>> On 28/4/2020 2:47 pm, Boris Brezillon wrote:
>>> On Tue, 28 Apr 2020 14:40:58 +0800
>>> "Ramuthevar, Vadivel MuruganX"
>>> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
>>>    
>>>> Hi Boris,
>>>>
>>>> On 28/4/2020 2:27 pm, Boris Brezillon wrote:
>>>>> On Tue, 28 Apr 2020 14:17:30 +0800
>>>>> "Ramuthevar, Vadivel MuruganX"
>>>>> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
>>>>>     >>>> Hi Miquel,
>>>>>>
>>>>>>        Thank you very much for the review comments and your time...
>>>>>>
>>>>>> On 27/4/2020 11:51 pm, Miquel Raynal wrote:
>>>>>>> Hi Ramuthevar,
>>>>>>>        >>>>>>> +static int ebu_nand_probe(struct platform_device *pdev)
>>>>>>>>> +{
>>>>>>>>> +	struct device *dev = &pdev->dev;
>>>>>>>>> +	struct ebu_nand_controller *ebu_host;
>>>>>>>>> +	struct nand_chip *nand;
>>>>>>>>> +	phys_addr_t nandaddr_pa;
>>>>>>>>> +	struct mtd_info *mtd;
>>>>>>>>> +	struct resource *res;
>>>>>>>>> +	int ret;
>>>>>>>>> +	u32 cs;
>>>>>>>>> +
>>>>>>>>> +	ebu_host = devm_kzalloc(dev, sizeof(*ebu_host), GFP_KERNEL);
>>>>>>>>> +	if (!ebu_host)
>>>>>>>>> +		return -ENOMEM;
>>>>>>>>> +
>>>>>>>>> +	ebu_host->dev = dev;
>>>>>>>>> +	nand_controller_init(&ebu_host->controller);
>>>>>>>>> +
>>>>>>>>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ebunand");
>>>>>>>>> +	ebu_host->ebu_addr = devm_ioremap_resource(&pdev->dev, res);
>>>>>>>>> +	if (IS_ERR(ebu_host->ebu_addr))
>>>>>>>>> +		return PTR_ERR(ebu_host->ebu_addr);
>>>>>>>>> +
>>>>>>>>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "hsnand");
>>>>>>>>> +	ebu_host->nand_addr = devm_ioremap_resource(&pdev->dev, res);
>>>>>>>>> +	if (IS_ERR(ebu_host->nand_addr))
>>>>>>>>> +		return PTR_ERR(ebu_host->nand_addr);
>>>>>>>>> +
>>>>>>>>> +	ret = device_property_read_u32(dev, "nand,cs", &cs);
>>>>>>>>
>>>>>>>> CS ids should be encoded in the reg property (see [1]).
>>>>>>>
>>>>>>> Is it your choice to only support a single CS or is it actually a
>>>>>>> controller limitation?
>>>>>>
>>>>>> Yes , its controller limitation to support only one CS
>>>>>
>>>>> I'm pretty sure that's not true, otherwise you wouldn't have to select
>>>>> the CS you want to use :P.
>>>>
>>>> At a time it supports only one chip select.
>>>
>>> Yes, like 99% of the NAND controllers, but that doesn't mean you can't
>>> support multi-CS chips. All you have to do is attach an array of
>>> ebu_nand_cs to your ebu_nand_chip (as done in the atmel driver I
>>> pointed to). nand_operation.cs tells you which CS (index in your
>>> ebu_nand_cs array) a specific operation is targeting, and you can pick
>>> the right MMIO range/reg value based on that.
>>
>> Agreed, sure I will add that and update next series of patches .
> 
> There are also many other places where you assume blindly that there
> is only one CS. You can check the Atmel NAND controller driver as Boris
> said and we will probably propose more little changes to be more
> generic.
since LGM EBU_NAND controller supports only one chip select at a time, 
so assumed like that, will change as generic way if consider like more 
chip select supports, Thanks!

Sure , I will do the changes as per your review comments.
> 
> Thanks,
> Miquèl
> 

Re: [PATCH v3 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC

[Miquel Raynal]

Hello,

"Ramuthevar, Vadivel MuruganX"
<vadivel.muruganx.ramuthevar@linux.intel.com> wrote on Tue, 28 Apr 2020
15:50:06 +0800:

> Hi Miquel,
> 
> On 28/4/2020 3:40 pm, Miquel Raynal wrote:
> > Hi Vadivel MuruganX,
> > 
> > "Ramuthevar, Vadivel MuruganX"
> > <vadivel.muruganx.ramuthevar@linux.intel.com> wrote on Tue, 28 Apr 2020
> > 14:50:35 +0800:
> >   
> >> Hi Boris,
> >>
> >> On 28/4/2020 2:47 pm, Boris Brezillon wrote:  
> >>> On Tue, 28 Apr 2020 14:40:58 +0800
> >>> "Ramuthevar, Vadivel MuruganX"
> >>> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:  
> >>>    >>>> Hi Boris,  
> >>>>
> >>>> On 28/4/2020 2:27 pm, Boris Brezillon wrote:  
> >>>>> On Tue, 28 Apr 2020 14:17:30 +0800
> >>>>> "Ramuthevar, Vadivel MuruganX"
> >>>>> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:  
> >>>>>     >>>> Hi Miquel,  
> >>>>>>
> >>>>>>        Thank you very much for the review comments and your time...
> >>>>>>
> >>>>>> On 27/4/2020 11:51 pm, Miquel Raynal wrote:  
> >>>>>>> Hi Ramuthevar,  
> >>>>>>>        >>>>>>> +static int ebu_nand_probe(struct platform_device *pdev)  
> >>>>>>>>> +{
> >>>>>>>>> +	struct device *dev = &pdev->dev;
> >>>>>>>>> +	struct ebu_nand_controller *ebu_host;
> >>>>>>>>> +	struct nand_chip *nand;
> >>>>>>>>> +	phys_addr_t nandaddr_pa;
> >>>>>>>>> +	struct mtd_info *mtd;
> >>>>>>>>> +	struct resource *res;
> >>>>>>>>> +	int ret;
> >>>>>>>>> +	u32 cs;
> >>>>>>>>> +
> >>>>>>>>> +	ebu_host = devm_kzalloc(dev, sizeof(*ebu_host), GFP_KERNEL);
> >>>>>>>>> +	if (!ebu_host)
> >>>>>>>>> +		return -ENOMEM;
> >>>>>>>>> +
> >>>>>>>>> +	ebu_host->dev = dev;
> >>>>>>>>> +	nand_controller_init(&ebu_host->controller);
> >>>>>>>>> +
> >>>>>>>>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ebunand");
> >>>>>>>>> +	ebu_host->ebu_addr = devm_ioremap_resource(&pdev->dev, res);
> >>>>>>>>> +	if (IS_ERR(ebu_host->ebu_addr))
> >>>>>>>>> +		return PTR_ERR(ebu_host->ebu_addr);
> >>>>>>>>> +
> >>>>>>>>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "hsnand");
> >>>>>>>>> +	ebu_host->nand_addr = devm_ioremap_resource(&pdev->dev, res);
> >>>>>>>>> +	if (IS_ERR(ebu_host->nand_addr))
> >>>>>>>>> +		return PTR_ERR(ebu_host->nand_addr);
> >>>>>>>>> +
> >>>>>>>>> +	ret = device_property_read_u32(dev, "nand,cs", &cs);  
> >>>>>>>>
> >>>>>>>> CS ids should be encoded in the reg property (see [1]).  
> >>>>>>>
> >>>>>>> Is it your choice to only support a single CS or is it actually a
> >>>>>>> controller limitation?  
> >>>>>>
> >>>>>> Yes , its controller limitation to support only one CS  
> >>>>>
> >>>>> I'm pretty sure that's not true, otherwise you wouldn't have to select
> >>>>> the CS you want to use :P.  
> >>>>
> >>>> At a time it supports only one chip select.  
> >>>
> >>> Yes, like 99% of the NAND controllers, but that doesn't mean you can't
> >>> support multi-CS chips. All you have to do is attach an array of
> >>> ebu_nand_cs to your ebu_nand_chip (as done in the atmel driver I
> >>> pointed to). nand_operation.cs tells you which CS (index in your
> >>> ebu_nand_cs array) a specific operation is targeting, and you can pick
> >>> the right MMIO range/reg value based on that.  
> >>
> >> Agreed, sure I will add that and update next series of patches .  
> > 
> > There are also many other places where you assume blindly that there
> > is only one CS. You can check the Atmel NAND controller driver as Boris
> > said and we will probably propose more little changes to be more
> > generic.  
> since LGM EBU_NAND controller supports only one chip select at a time, so assumed like that, will change as generic way if consider like more chip select supports, Thanks!

What do you mean "at a time"?

Do we have access to the spec or a register map? We could tell you very
quickly if it is worth the trouble. But I am pretty sure as well that
the controller supports more than 1 CS.

> Sure , I will do the changes as per your review comments.
> > 
> > Thanks,
> > Miquèl
> >   

Thanks,
Miquèl

Re: [PATCH v3 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC

[Ramuthevar, Vadivel MuruganX]

Hi Miquel,

On 28/4/2020 3:54 pm, Miquel Raynal wrote:
> Hello,
> 
> "Ramuthevar, Vadivel MuruganX"
> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote on Tue, 28 Apr 2020
> 15:50:06 +0800:
> 
>> Hi Miquel,
>>
>> On 28/4/2020 3:40 pm, Miquel Raynal wrote:
>>> Hi Vadivel MuruganX,
>>>
>>> "Ramuthevar, Vadivel MuruganX"
>>> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote on Tue, 28 Apr 2020
>>> 14:50:35 +0800:
>>>    
>>>> Hi Boris,
>>>>
>>>> On 28/4/2020 2:47 pm, Boris Brezillon wrote:
>>>>> On Tue, 28 Apr 2020 14:40:58 +0800
>>>>> "Ramuthevar, Vadivel MuruganX"
>>>>> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
>>>>>     >>>> Hi Boris,
>>>>>>
>>>>>> On 28/4/2020 2:27 pm, Boris Brezillon wrote:
>>>>>>> On Tue, 28 Apr 2020 14:17:30 +0800
>>>>>>> "Ramuthevar, Vadivel MuruganX"
>>>>>>> <vadivel.muruganx.ramuthevar@linux.intel.com> wrote:
>>>>>>>      >>>> Hi Miquel,
>>>>>>>>
>>>>>>>>         Thank you very much for the review comments and your time...
>>>>>>>>
>>>>>>>> On 27/4/2020 11:51 pm, Miquel Raynal wrote:
>>>>>>>>> Hi Ramuthevar,
>>>>>>>>>         >>>>>>> +static int ebu_nand_probe(struct platform_device *pdev)
>>>>>>>>>>> +{
>>>>>>>>>>> +	struct device *dev = &pdev->dev;
>>>>>>>>>>> +	struct ebu_nand_controller *ebu_host;
>>>>>>>>>>> +	struct nand_chip *nand;
>>>>>>>>>>> +	phys_addr_t nandaddr_pa;
>>>>>>>>>>> +	struct mtd_info *mtd;
>>>>>>>>>>> +	struct resource *res;
>>>>>>>>>>> +	int ret;
>>>>>>>>>>> +	u32 cs;
>>>>>>>>>>> +
>>>>>>>>>>> +	ebu_host = devm_kzalloc(dev, sizeof(*ebu_host), GFP_KERNEL);
>>>>>>>>>>> +	if (!ebu_host)
>>>>>>>>>>> +		return -ENOMEM;
>>>>>>>>>>> +
>>>>>>>>>>> +	ebu_host->dev = dev;
>>>>>>>>>>> +	nand_controller_init(&ebu_host->controller);
>>>>>>>>>>> +
>>>>>>>>>>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ebunand");
>>>>>>>>>>> +	ebu_host->ebu_addr = devm_ioremap_resource(&pdev->dev, res);
>>>>>>>>>>> +	if (IS_ERR(ebu_host->ebu_addr))
>>>>>>>>>>> +		return PTR_ERR(ebu_host->ebu_addr);
>>>>>>>>>>> +
>>>>>>>>>>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "hsnand");
>>>>>>>>>>> +	ebu_host->nand_addr = devm_ioremap_resource(&pdev->dev, res);
>>>>>>>>>>> +	if (IS_ERR(ebu_host->nand_addr))
>>>>>>>>>>> +		return PTR_ERR(ebu_host->nand_addr);
>>>>>>>>>>> +
>>>>>>>>>>> +	ret = device_property_read_u32(dev, "nand,cs", &cs);
>>>>>>>>>>
>>>>>>>>>> CS ids should be encoded in the reg property (see [1]).
>>>>>>>>>
>>>>>>>>> Is it your choice to only support a single CS or is it actually a
>>>>>>>>> controller limitation?
>>>>>>>>
>>>>>>>> Yes , its controller limitation to support only one CS
>>>>>>>
>>>>>>> I'm pretty sure that's not true, otherwise you wouldn't have to select
>>>>>>> the CS you want to use :P.
>>>>>>
>>>>>> At a time it supports only one chip select.
>>>>>
>>>>> Yes, like 99% of the NAND controllers, but that doesn't mean you can't
>>>>> support multi-CS chips. All you have to do is attach an array of
>>>>> ebu_nand_cs to your ebu_nand_chip (as done in the atmel driver I
>>>>> pointed to). nand_operation.cs tells you which CS (index in your
>>>>> ebu_nand_cs array) a specific operation is targeting, and you can pick
>>>>> the right MMIO range/reg value based on that.
>>>>
>>>> Agreed, sure I will add that and update next series of patches .
>>>
>>> There are also many other places where you assume blindly that there
>>> is only one CS. You can check the Atmel NAND controller driver as Boris
>>> said and we will probably propose more little changes to be more
>>> generic.
>> since LGM EBU_NAND controller supports only one chip select at a time, so assumed like that, will change as generic way if consider like more chip select supports, Thanks!
> 
> What do you mean "at a time"?

I mean it supports multi-CS, during the run time it selects one.
Thanks!

Regards
Vadivel
> 
> Do we have access to the spec or a register map? We could tell you very
> quickly if it is worth the trouble. But I am pretty sure as well that
> the controller supports more than 1 CS.
> 
>> Sure , I will do the changes as per your review comments.
>>>
>>> Thanks,
>>> Miquèl
>>>    
> 
> Thanks,
> Miquèl
> 

Re: [PATCH v3 2/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC

[Ramuthevar, Vadivel MuruganX]

Hi Miquel,

On 28/4/2020 3:54 pm, Miquel Raynal wrote:
> Do we have access to the spec or a register map? We could tell you very
> quickly if it is worth the trouble. But I am pretty sure as well that
> the controller supports more than 1 CS.

Got it, will update the changes to support multiple CS as generic-way.

Thank you!

Regards
Vadivel