LinuxLists.cc - [PATCH v2 0/2] mtd: rawnand: Add NAND controller support on Intel LGM SoC

[permalink] [raw]

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

From: Ramuthevar Vadivel Murugan <[email protected]>

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 <[email protected]>
---
drivers/mtd/nand/raw/Kconfig | 7 +
drivers/mtd/nand/raw/Makefile | 1 +
drivers/mtd/nand/raw/intel_lgm_nand.c | 740 ++++++++++++++++++++++++++++++++++
3 files changed, 748 insertions(+)
create mode 100644 drivers/mtd/nand/raw/intel_lgm_nand.c

diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
index a80a46bb5b8b..9efc4bbaf4a3 100644
--- a/drivers/mtd/nand/raw/Kconfig
+++ b/drivers/mtd/nand/raw/Kconfig
@@ -457,6 +457,13 @@ 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 X86
+ 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..49a301ae0c9d 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_lgm_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/intel_lgm_nand.c b/drivers/mtd/nand/raw/intel_lgm_nand.c
new file mode 100644
index 000000000000..96cd1831f070
--- /dev/null
+++ b/drivers/mtd/nand/raw/intel_lgm_nand.c
@@ -0,0 +1,740 @@
+// SPDX-License-Identifier: GPL-2.0+
+/* Copyright (c) 2019 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/iopoll.h>
+#include <linux/module.h>
+#include <linux/resource.h>
+#include <linux/sched.h>
+#include <linux/types.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/rawnand.h>
+#include <linux/mtd/nand_ecc.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/mtd/partitions.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <mtd/mtd-abi.h>
+#include <linux/mod_devicetable.h>
+#include <linux/mtd/nand.h>
+
+#define LGM_CLC 0x000
+#define LGM_CLC_RST 0x00000000u
+
+#define LGM_NAND_ECC_OFFSET 0x008
+
+#define LGM_ADDR_SEL(n) (0x20 + (n) * 4)
+#define LGM_ADDR_MASK (5 << 4)
+#define LGM_ADDR_SEL_REGEN 0x1
+
+#define LGM_BUSCON(n) (0x60 + (n) * 4)
+#define LGM_BUSCON_CMULT_V4 0x1
+#define LGM_BUSCON_RECOVC(n) ((n) << 2)
+#define LGM_BUSCON_HOLDC(n) ((n) << 4)
+#define LGM_BUSCON_WAITRDC(n) ((n) << 6)
+#define LGM_BUSCON_WAITWRC(n) ((n) << 8)
+#define LGM_BUSCON_BCGEN_CS 0x0
+#define LGM_BUSCON_SETUP_EN BIT(22)
+#define LGM_BUSCON_ALEC 0xC000
+
+#define NAND_CON 0x0B0
+#define NAND_CON_NANDM_EN BIT(0)
+#define NAND_CON_NANDM_DIS 0x0
+#define NAND_CON_CSMUX_E_EN BIT(1)
+#define NAND_CON_ALE_P_LOW BIT(2)
+#define NAND_CON_CLE_P_LOW BIT(3)
+#define NAND_CON_CS_P_LOW BIT(4)
+#define NAND_CON_SE_P_LOW BIT(5)
+#define NAND_CON_WP_P_LOW BIT(6)
+#define NAND_CON_PRE_P_LOW BIT(7)
+#define NAND_CON_IN_CS_S(n) ((n) << 8)
+#define NAND_CON_OUT_CS_S(n) ((n) << 10)
+#define NAND_CON_LAT_EN_CS_P ((0x3D) << 18)
+
+#define NAND_WAIT 0x0B4
+#define NAND_WAIT_RDBY BIT(0)
+#define NAND_WAIT_WR_C BIT(3)
+
+#define NAND_CTL1 0x110
+#define NAND_CTL1_ADDR_3_SHIFT 24
+
+#define NAND_CTL2 0x114
+#define NAND_CTL2_ADDR_5_SHIFT 8
+#define NAND_CTL2_CYC_N_V5 (0x2 << 16)
+
+#define NAND_INT_MSK_CTL 0x124
+#define NAND_INT_MSK_CTL_WR_C BIT(4)
+
+#define NAND_INT_STA 0x128
+#define NAND_INT_STA_WR_C BIT(4)
+
+#define NAND_CTL 0x130
+#define NAND_CTL_MODE_ECC 0x1
+#define NAND_CTL_GO BIT(2)
+#define NAND_CTL_CE_SEL_CS(n) BIT(3 + (n))
+#define NAND_CTL_RW_READ 0x0
+#define NAND_CTL_RW_WRITE BIT(10)
+#define NAND_CTL_ECC_OFF_V8TH BIT(11)
+#define NAND_CTL_CKFF_EN 0x0
+#define NAND_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_WRITE_CMD (NAND_CON_CS_P_LOW | NAND_CON_CLE_P_LOW)
+#define NAND_WRITE_ADDR (NAND_CON_CS_P_LOW | NAND_CON_ALE_P_LOW)
+#define NAND_WRITE_DATA NAND_CON_CS_P_LOW
+#define NAND_READ_DATA NAND_CON_CS_P_LOW
+
+#define NAND_CHIP_NO_SELECTION -1
+#define NAND_CHIP_SELECTION 0x0
+
+struct lgm_nand_host {
+ struct nand_controller controller;
+ struct nand_chip chip;
+ void __iomem *lgm_va;
+ void __iomem *hsnand_va;
+ void __iomem *nandaddr_va;
+ 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 u8 lgm_nand_readb(struct nand_chip *chip, int op)
+{
+ struct lgm_nand_host *lgm_host = nand_get_controller_data(chip);
+ void __iomem *nand_wait = lgm_host->lgm_va + NAND_WAIT;
+ u32 stat;
+ int ret;
+ u8 val;
+
+ val = readb(lgm_host->nandaddr_va + op);
+
+ ret = readl_poll_timeout(nand_wait, stat, stat & NAND_WAIT_WR_C,
+ 20, 1000);
+ if (ret)
+ dev_warn(lgm_host->dev,
+ "lgm nand write timeout. nand_wait(0x%p)=0x%x\n",
+ nand_wait, readl(nand_wait));
+
+ return val;
+}
+
+static void lgm_nand_writeb(struct nand_chip *chip, int op, u8 value)
+{
+ struct lgm_nand_host *lgm_host = nand_get_controller_data(chip);
+ void __iomem *nand_wait = lgm_host->lgm_va + NAND_WAIT;
+ u32 stat;
+ int ret;
+
+ writeb(value, lgm_host->nandaddr_va + op);
+
+ ret = readl_poll_timeout(nand_wait, stat, stat & NAND_WAIT_WR_C,
+ 20, 1000);
+ if (ret)
+ dev_warn(lgm_host->dev,
+ "lgm nand write timeout. nand_wait(0x%p)=0x%x\n",
+ nand_wait, readl(nand_wait));
+}
+
+static unsigned char lgm_read_byte(struct nand_chip *chip)
+{
+ return lgm_nand_readb(chip, NAND_READ_DATA);
+}
+
+static void lgm_read_buf(struct nand_chip *chip, u_char *buf, int len)
+{
+ int i;
+
+ for (i = 0; i < len; i++)
+ buf[i] = lgm_nand_readb(chip, NAND_WRITE_DATA);
+}
+
+static void lgm_write_buf(struct nand_chip *chip, const u_char *buf, int len)
+{
+ int i;
+
+ for (i = 0; i < len; i++)
+ lgm_nand_writeb(chip, NAND_WRITE_DATA, buf[i]);
+}
+
+static void lgm_select_chip(struct nand_chip *chip, int select)
+{
+ struct lgm_nand_host *lgm_host = nand_get_controller_data(chip);
+ void __iomem *nand_con = lgm_host->lgm_va + NAND_CON;
+ u32 cs = lgm_host->cs;
+ int val;
+
+ switch (select) {
+ case NAND_CHIP_NO_SELECTION:
+ val = readl(nand_con);
+ writel(val & ~NAND_CON_NANDM_EN, nand_con);
+ break;
+ case NAND_CHIP_SELECTION:
+ writel(NAND_CON_NANDM_EN | NAND_CON_CSMUX_E_EN |
+ NAND_CON_CS_P_LOW | NAND_CON_SE_P_LOW |
+ NAND_CON_WP_P_LOW | NAND_CON_PRE_P_LOW |
+ NAND_CON_IN_CS_S(cs) | NAND_CON_OUT_CS_S(cs) |
+ NAND_CON_LAT_EN_CS_P, nand_con);
+ break;
+ default:
+ break;
+ }
+}
+
+static int lgm_dev_ready(struct nand_chip *chip)
+{
+ struct lgm_nand_host *lgm_host = nand_get_controller_data(chip);
+
+ return readl(lgm_host->lgm_va + NAND_WAIT) & NAND_WAIT_RDBY;
+}
+
+static void lgm_cmd_ctrl(struct nand_chip *chip, int cmd, unsigned int ctrl)
+{
+ if (cmd == NAND_CMD_NONE)
+ return;
+
+ if (ctrl & NAND_CLE)
+ lgm_nand_writeb(chip, NAND_WRITE_CMD, cmd);
+ else if (ctrl & NAND_ALE)
+ lgm_nand_writeb(chip, NAND_WRITE_ADDR, cmd);
+}
+
+static int lgm_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 = LGM_NAND_ECC_OFFSET;
+ oobregion->length = chip->ecc.total;
+
+ return 0;
+}
+
+static int lgm_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 + LGM_NAND_ECC_OFFSET;
+ oobregion->length = mtd->oobsize - oobregion->offset;
+
+ return 0;
+}
+
+static const struct mtd_ooblayout_ops lgm_nand_ooblayout_ops = {
+ .ecc = lgm_nand_ooblayout_ecc,
+ .free = lgm_nand_ooblayout_free,
+};
+
+static inline struct lgm_nand_host *nand_to_lgm(struct nand_chip *chip)
+{
+ return container_of(chip, struct lgm_nand_host, chip);
+}
+
+static void lgm_dma_rx_callback(void *cookie)
+{
+ struct lgm_nand_host *lgm_host = cookie;
+
+ dmaengine_terminate_async(lgm_host->dma_rx);
+
+ complete(&lgm_host->dma_access_complete);
+}
+
+static void lgm_dma_tx_callback(void *cookie)
+{
+ struct lgm_nand_host *lgm_host = cookie;
+
+ dmaengine_terminate_async(lgm_host->dma_tx);
+
+ complete(&lgm_host->dma_access_complete);
+}
+
+static int lgm_dma_start(struct lgm_nand_host *lgm_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 = lgm_host->dma_rx;
+ dma_completion = &lgm_host->dma_access_complete;
+ callback = lgm_dma_rx_callback;
+ } else {
+ chan = lgm_host->dma_tx;
+ dma_completion = &lgm_host->dma_access_complete;
+ callback = lgm_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(lgm_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 = lgm_host;
+ cookie = tx->tx_submit(tx);
+
+ ret = dma_submit_error(cookie);
+ if (ret) {
+ dev_err(lgm_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(lgm_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(lgm_host->dev, buf_dma, len, dir);
+
+ return ret;
+}
+
+static void lgm_nand_trigger(struct lgm_nand_host *lgm_host, int page, u32 cmd)
+{
+ unsigned int val;
+
+ val = cmd | (page & 0xFF) << NAND_CTL1_ADDR_3_SHIFT;
+ writel(val, lgm_host->hsnand_va + NAND_CTL1);
+ val = (page & 0xFFFF00) >> 8 | NAND_CTL2_CYC_N_V5;
+ writel(val, lgm_host->hsnand_va + NAND_CTL2);
+
+ writel(lgm_host->nd_para0, lgm_host->hsnand_va + NAND_PARA0);
+
+ /* clear first, will update later */
+ writel(0xFFFFFFFF, lgm_host->hsnand_va + NAND_CMSG_0);
+ writel(0xFFFFFFFF, lgm_host->hsnand_va + NAND_CMSG_1);
+
+ writel(NAND_INT_MSK_CTL_WR_C, lgm_host->hsnand_va + NAND_INT_MSK_CTL);
+
+ val = cmd == NAND_CMD_READ0 ? NAND_CTL_RW_READ : NAND_CTL_RW_WRITE;
+
+ writel(NAND_CTL_MSG_EN | NAND_CTL_CKFF_EN | NAND_CTL_ECC_OFF_V8TH |
+ NAND_CTL_CE_SEL_CS(lgm_host->cs) | NAND_CTL_MODE_ECC |
+ NAND_CTL_GO | val, lgm_host->hsnand_va + NAND_CTL);
+}
+
+static int lgm_nand_read_page_hwecc(struct nand_chip *chip, u8 *buf,
+ int oob_required, int page)
+{
+ struct mtd_info *mtd = nand_to_mtd(chip);
+ struct lgm_nand_host *lgm_host = nand_get_controller_data(chip);
+ int ret, x;
+
+ lgm_nand_trigger(lgm_host, page, NAND_CMD_READ0);
+
+ ret = lgm_dma_start(lgm_host, DMA_DEV_TO_MEM, buf, mtd->writesize);
+ if (ret)
+ return ret;
+
+ if (oob_required)
+ chip->ecc.read_oob(chip, page);
+
+ x = readl(lgm_host->hsnand_va + NAND_CTL);
+ x &= ~NAND_CTL_GO;
+ writel(x, lgm_host->hsnand_va + NAND_CTL);
+
+ return 0;
+}
+
+static int lgm_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 lgm_nand_host *lgm_host = nand_get_controller_data(chip);
+ void __iomem *int_sta = lgm_host->hsnand_va + NAND_INT_STA;
+ int ret, val, x;
+ __be32 reg;
+
+ lgm_nand_trigger(lgm_host, page, NAND_CMD_SEQIN);
+
+ ret = lgm_dma_start(lgm_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, lgm_host->hsnand_va + NAND_CMSG_0);
+
+ reg = cpu_to_be32(*pdata);
+ writel(reg, lgm_host->hsnand_va + NAND_CMSG_1);
+ }
+
+ ret = readl_poll_timeout_atomic(int_sta, val,
+ !(val & NAND_INT_STA_WR_C), 10, 1000);
+ if (ret)
+ return -EIO;
+
+ x = readl(lgm_host->hsnand_va + NAND_CTL);
+ x &= ~NAND_CTL_GO;
+ writel(x, lgm_host->hsnand_va + NAND_CTL);
+
+ return 0;
+}
+
+static const u8 ecc_strength[] = { 1, 1, 4, 8, 24, 32, 40, 60, };
+
+static int lgm_nand_attach_chip(struct nand_chip *chip)
+{
+ struct mtd_info *mtd = nand_to_mtd(chip);
+ struct lgm_nand_host *lgm_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;
+
+ lgm_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, &lgm_nand_ooblayout_ops);
+ chip->ecc.read_page = lgm_nand_read_page_hwecc;
+ chip->ecc.write_page = lgm_nand_write_page_hwecc;
+
+ return 0;
+}
+
+static int lgm_nand_exec_op(struct nand_chip *chip,
+ const struct nand_operation *op, bool check_only)
+{
+ struct lgm_nand_host *host = nand_to_lgm(chip);
+ const struct nand_op_instr *instr = NULL;
+ unsigned int op_id;
+ int i, ret = 0;
+
+ for (op_id = 0; op_id < op->ninstrs; op_id++) {
+ instr = &op->instrs[op_id];
+
+ lgm_select_chip(chip, host->cs);
+
+ switch (instr->type) {
+ case NAND_OP_CMD_INSTR:
+ lgm_cmd_ctrl(chip, instr->ctx.cmd.opcode, NAND_CLE);
+ break;
+
+ case NAND_OP_ADDR_INSTR:
+ for (i = 0; i < instr->ctx.addr.naddrs; i++)
+ lgm_cmd_ctrl(chip, instr->ctx.addr.addrs[i],
+ NAND_ALE);
+ break;
+
+ case NAND_OP_DATA_IN_INSTR:
+ lgm_read_buf(chip, instr->ctx.data.buf.in,
+ instr->ctx.data.len);
+ break;
+
+ case NAND_OP_DATA_OUT_INSTR:
+ lgm_write_buf(chip, instr->ctx.data.buf.out,
+ instr->ctx.data.len);
+ break;
+
+ case NAND_OP_WAITRDY_INSTR:
+ ret = lgm_dev_ready(chip);
+ break;
+ }
+ }
+
+ return ret;
+}
+
+static const struct nand_controller_ops lgm_nand_controller_ops = {
+ .attach_chip = lgm_nand_attach_chip,
+ .exec_op = lgm_nand_exec_op,
+};
+
+static void lgm_dma_exit(struct lgm_nand_host *lgm_host)
+{
+ if (lgm_host->dma_rx) {
+ dma_release_channel(lgm_host->dma_rx);
+ lgm_host->dma_rx = NULL;
+ }
+
+ if (lgm_host->dma_tx) {
+ dma_release_channel(lgm_host->dma_tx);
+ lgm_host->dma_tx = NULL;
+ }
+}
+
+static int lgm_dma_init(struct device *dev, struct lgm_nand_host *lgm_host)
+{
+ int ret;
+
+ /* Prepare for TX DMA: */
+ lgm_host->dma_tx = dma_request_chan(dev, "tx");
+ if (IS_ERR(lgm_host->dma_tx)) {
+ ret = PTR_ERR(lgm_host->dma_tx);
+ dev_err(dev, "can't get the TX DMA channel, error %d!\n", ret);
+ goto err;
+ }
+
+ /* Prepare for RX: */
+ lgm_host->dma_rx = dma_request_chan(dev, "rx");
+ if (IS_ERR(lgm_host->dma_rx)) {
+ ret = PTR_ERR(lgm_host->dma_rx);
+ dev_err(dev, "can't get the RX DMA channel, error %d\n", ret);
+ goto err;
+ }
+
+ return 0;
+err:
+ return ret;
+}
+
+static int lgm_nand_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct lgm_nand_host *lgm_host;
+ struct nand_chip *nand;
+ phys_addr_t nandaddr_pa;
+ struct mtd_info *mtd;
+ struct resource *res;
+ int ret;
+ u32 cs;
+
+ lgm_host = devm_kzalloc(dev, sizeof(*lgm_host), GFP_KERNEL);
+ if (!lgm_host)
+ return -ENOMEM;
+
+ lgm_host->dev = dev;
+ nand_controller_init(&lgm_host->controller);
+
+ lgm_host->lgm_va =
+ devm_platform_ioremap_resource_byname(pdev, "lgmnand");
+ if (IS_ERR(lgm_host->lgm_va))
+ return PTR_ERR(lgm_host->lgm_va);
+
+ lgm_host->hsnand_va =
+ devm_platform_ioremap_resource_byname(pdev, "hsnand");
+ if (IS_ERR(lgm_host->hsnand_va))
+ return PTR_ERR(lgm_host->hsnand_va);
+
+ ret = device_property_read_u32(dev, "nand,cs", &cs);
+ if (ret) {
+ dev_err(dev, "failed to get chip select: %d\n", ret);
+ return ret;
+ }
+
+ lgm_host->cs = cs;
+
+ lgm_host->cs_name = devm_kasprintf(dev, GFP_KERNEL, "nand_cs%d", cs);
+ if (IS_ERR(lgm_host->cs_name)) {
+ ret = PTR_ERR(lgm_host->cs_name);
+ dev_err(dev, "failed to get chip select name: %d\n", ret);
+ return ret;
+ }
+
+ res = devm_platform_ioremap_resource_byname(pdev, lgm_host->cs_name);
+ lgm_host->nandaddr_va = res;
+ nandaddr_pa = res->start;
+ if (IS_ERR(lgm_host->nandaddr_va))
+ return PTR_ERR(lgm_host->nandaddr_va);
+
+ lgm_host->clk = devm_clk_get(dev, NULL);
+ if (IS_ERR(lgm_host->clk)) {
+ ret = PTR_ERR(lgm_host->clk);
+ dev_err(dev, "failed to get clock: %d\n", ret);
+ return ret;
+ }
+
+ ret = clk_prepare_enable(lgm_host->clk);
+ if (ret) {
+ dev_err(dev, "failed to enable clock: %d\n", ret);
+ return ret;
+ }
+ lgm_host->clk_rate = clk_get_rate(lgm_host->clk);
+
+ ret = lgm_dma_init(dev, lgm_host);
+ if (ret)
+ goto disable_clk;
+
+ writel(lower_32_bits(nandaddr_pa) | LGM_ADDR_SEL_REGEN | LGM_ADDR_MASK,
+ lgm_host->lgm_va + LGM_ADDR_SEL(cs));
+
+ writel(LGM_BUSCON_CMULT_V4 | LGM_BUSCON_RECOVC(1) |
+ LGM_BUSCON_HOLDC(1) | LGM_BUSCON_WAITRDC(2) |
+ LGM_BUSCON_WAITWRC(2) | LGM_BUSCON_BCGEN_CS | LGM_BUSCON_ALEC |
+ LGM_BUSCON_SETUP_EN, lgm_host->lgm_va + LGM_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
+ * Refer the Intel LGM SoC datasheet.
+ */
+ writel(0x17400051, lgm_host->lgm_va + LGM_ADDR_SEL(0));
+ writel(0x17C00051, lgm_host->lgm_va + LGM_ADDR_SEL(cs));
+ nand_set_flash_node(&lgm_host->chip, dev->of_node);
+ mtd = nand_to_mtd(&lgm_host->chip);
+ mtd->dev.parent = dev;
+ lgm_host->dev = dev;
+
+ platform_set_drvdata(pdev, lgm_host);
+ nand_set_controller_data(&lgm_host->chip, lgm_host);
+
+ nand = &lgm_host->chip;
+ nand->controller = &lgm_host->controller;
+ nand->controller->ops = &lgm_nand_controller_ops;
+
+ /* Scan to find existence of the device */
+ ret = nand_scan(&lgm_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(&lgm_host->chip);
+exit_dma:
+ lgm_dma_exit(lgm_host);
+disable_clk:
+ clk_disable_unprepare(lgm_host->clk);
+
+ return ret;
+}
+
+static int lgm_nand_remove(struct platform_device *pdev)
+{
+ struct lgm_nand_host *lgm_host = platform_get_drvdata(pdev);
+
+ nand_release(&lgm_host->chip);
+ clk_disable_unprepare(lgm_host->clk);
+ lgm_dma_exit(lgm_host);
+
+ return 0;
+}
+
+static const struct of_device_id lgm_nand_match[] = {
+ { .compatible = "intel,lgm-nand", },
+ {}
+};
+MODULE_DEVICE_TABLE(of, lgm_nand_match);
+
+static struct platform_driver lgm_nand_driver = {
+ .probe = lgm_nand_probe,
+ .remove = lgm_nand_remove,
+ .driver = {
+ .name = "intel-lgm-nand",
+ .of_match_table = lgm_nand_match,
+ },
+
+};
+module_platform_driver(lgm_nand_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Vadivel Murugan R <[email protected]>");
+MODULE_DESCRIPTION("Intel's LGM External Bus NAND Controller driver");
--
2.11.0

2020-04-17 17:07:56

by Hauke Mehrtens

[permalink] [raw]

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

On 4/17/20 10:21 AM, Ramuthevar, Vadivel MuruganX wrote:
> From: Ramuthevar Vadivel Murugan <[email protected]>

Thanks for adding me in CC, I am using my private mail it took me too
long to configure Outlook.

I compared the register description of the LGM with the VRX200, the EBU
and NAND block are looking very similar. I think the VRX200 also
supports HW ECC and DMA, nobody implimented it in the upstream driver.

I think replacing the old XWAY dirver with this one is a good approach.
Then we can add feature flags to activate the extra features only on the
SoCs which support them.

On older SoCs the EBU (NAND) and the PCI (not express) IP core are
sharing some parts like an endianess switch, this is causeing some more
problems.

> 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 <[email protected]>

.....

> +config MTD_NAND_INTEL_LGM
> + tristate "Support for NAND controller on Intel LGM SoC"
> + depends on X86

Compile test should also work for other archs

> + help
> + Enables support for NAND Flash chips on Intel's LGM SoC.
> + NAND flash interfaced through the External Bus Unit.
> +
> comment "Misc"

.....

> +
> +#define LGM_CLC 0x000
> +#define LGM_CLC_RST 0x00000000u
> +
> +#define LGM_NAND_ECC_OFFSET 0x008
It is confusing that this is not a register but a different definition.
Please move it somewheer else.

> +#define LGM_ADDR_SEL(n) (0x20 + (n) * 4)
> +#define LGM_ADDR_MASK (5 << 4)
> +#define LGM_ADDR_SEL_REGEN 0x1
> +
> +#define LGM_BUSCON(n) (0x60 + (n) * 4)
> +#define LGM_BUSCON_CMULT_V4 0x1
> +#define LGM_BUSCON_RECOVC(n) ((n) << 2)
> +#define LGM_BUSCON_HOLDC(n) ((n) << 4)
> +#define LGM_BUSCON_WAITRDC(n) ((n) << 6)
> +#define LGM_BUSCON_WAITWRC(n) ((n) << 8)
> +#define LGM_BUSCON_BCGEN_CS 0x0
> +#define LGM_BUSCON_SETUP_EN BIT(22)
> +#define LGM_BUSCON_ALEC 0xC000
> +
> +#define NAND_CON 0x0B0
> +#define NAND_CON_NANDM_EN BIT(0)
> +#define NAND_CON_NANDM_DIS 0x0
> +#define NAND_CON_CSMUX_E_EN BIT(1)
> +#define NAND_CON_ALE_P_LOW BIT(2)
> +#define NAND_CON_CLE_P_LOW BIT(3)
> +#define NAND_CON_CS_P_LOW BIT(4)
> +#define NAND_CON_SE_P_LOW BIT(5)
> +#define NAND_CON_WP_P_LOW BIT(6)
> +#define NAND_CON_PRE_P_LOW BIT(7)
> +#define NAND_CON_IN_CS_S(n) ((n) << 8)
> +#define NAND_CON_OUT_CS_S(n) ((n) << 10)
> +#define NAND_CON_LAT_EN_CS_P ((0x3D) << 18)
> +
> +#define NAND_WAIT 0x0B4
> +#define NAND_WAIT_RDBY BIT(0)
> +#define NAND_WAIT_WR_C BIT(3)

The registers with the LGM_ prefix, NAND_CON and NAND_WAIT are from the
EBU (EBU_N) register block. I would prefer if the have the same prefix.
You should use a different prefix for the register definitions below
this comment, which are from the NAND Controller (HSNAND).

> +#define NAND_CTL1 0x110
> +#define NAND_CTL1_ADDR_3_SHIFT 24
> +
> +#define NAND_CTL2 0x114
> +#define NAND_CTL2_ADDR_5_SHIFT 8
> +#define NAND_CTL2_CYC_N_V5 (0x2 << 16)
> +
> +#define NAND_INT_MSK_CTL 0x124
> +#define NAND_INT_MSK_CTL_WR_C BIT(4)
> +
> +#define NAND_INT_STA 0x128
> +#define NAND_INT_STA_WR_C BIT(4)
> +
> +#define NAND_CTL 0x130
> +#define NAND_CTL_MODE_ECC 0x1
> +#define NAND_CTL_GO BIT(2)
> +#define NAND_CTL_CE_SEL_CS(n) BIT(3 + (n))
> +#define NAND_CTL_RW_READ 0x0
> +#define NAND_CTL_RW_WRITE BIT(10)
> +#define NAND_CTL_ECC_OFF_V8TH BIT(11)
> +#define NAND_CTL_CKFF_EN 0x0
> +#define NAND_CTL_MSG_EN BIT(17)

Till here I find the same registers you use also in the VRX200
description. I haven't checked all the bits. Danube only has the
registers from the EBU, I haven't checked the SoCs in between.

> +#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_WRITE_CMD (NAND_CON_CS_P_LOW | NAND_CON_CLE_P_LOW)
> +#define NAND_WRITE_ADDR (NAND_CON_CS_P_LOW | NAND_CON_ALE_P_LOW)
> +#define NAND_WRITE_DATA NAND_CON_CS_P_LOW
> +#define NAND_READ_DATA NAND_CON_CS_P_LOW
> +
> +#define NAND_CHIP_NO_SELECTION -1
> +#define NAND_CHIP_SELECTION 0x0
> +

....
> +static int lgm_nand_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct lgm_nand_host *lgm_host;
> + struct nand_chip *nand;
> + phys_addr_t nandaddr_pa;
> + struct mtd_info *mtd;
> + struct resource *res;
> + int ret;
> + u32 cs;
> +
> + lgm_host = devm_kzalloc(dev, sizeof(*lgm_host), GFP_KERNEL);
> + if (!lgm_host)
> + return -ENOMEM;
> +
> + lgm_host->dev = dev;
> + nand_controller_init(&lgm_host->controller);
> +
> + lgm_host->lgm_va =
> + devm_platform_ioremap_resource_byname(pdev, "lgmnand");
> + if (IS_ERR(lgm_host->lgm_va))
> + return PTR_ERR(lgm_host->lgm_va);
lgm_va is named ebu_n in the register description. Could you rename this
variable to ebu. I think the _va postfix is not needed as it should be
clear that this is a virtual addresss.

> + lgm_host->hsnand_va =
> + devm_platform_ioremap_resource_byname(pdev, "hsnand");
> + if (IS_ERR(lgm_host->hsnand_va))
> + return PTR_ERR(lgm_host->hsnand_va);
> +
> + ret = device_property_read_u32(dev, "nand,cs", &cs);
> + if (ret) {
> + dev_err(dev, "failed to get chip select: %d\n", ret);
> + return ret;
> + }

The cs should be validated, ifsome uses cs == 4 there will be a problem.
LGM supports CS 0 to 3 like the VRX200, danube for example only 0 and 1.

> + lgm_host->cs = cs;
> +
> + lgm_host->cs_name = devm_kasprintf(dev, GFP_KERNEL, "nand_cs%d", cs);

cs_name is only used locally in this function you can also use some
memory on the stack to generate the name.

> + if (IS_ERR(lgm_host->cs_name)) {
> + ret = PTR_ERR(lgm_host->cs_name);
> + dev_err(dev, "failed to get chip select name: %d\n", ret);
> + return ret;
> + }
> +
> + res = devm_platform_ioremap_resource_byname(pdev, lgm_host->cs_name);

This will only support one chip select at a time on the SoC. It is not
possible to configure the DTS in a way to talk to two NAND chips on
different chip selects. I think this is an uncommon scenario and I do
not know if other NAND drivers in Linux support this feature.

> + lgm_host->nandaddr_va = res;
> + nandaddr_pa = res->start;
> + if (IS_ERR(lgm_host->nandaddr_va))
> + return PTR_ERR(lgm_host->nandaddr_va);
> +
> + lgm_host->clk = devm_clk_get(dev, NULL);
> + if (IS_ERR(lgm_host->clk)) {
> + ret = PTR_ERR(lgm_host->clk);
> + dev_err(dev, "failed to get clock: %d\n", ret);
> + return ret;
> + }
> +
> + ret = clk_prepare_enable(lgm_host->clk);
> + if (ret) {
> + dev_err(dev, "failed to enable clock: %d\n", ret);
> + return ret;
> + }
> + lgm_host->clk_rate = clk_get_rate(lgm_host->clk);
> +
> + ret = lgm_dma_init(dev, lgm_host);
> + if (ret)
> + goto disable_clk;
> +
> + writel(lower_32_bits(nandaddr_pa) | LGM_ADDR_SEL_REGEN | LGM_ADDR_MASK,
> + lgm_host->lgm_va + LGM_ADDR_SEL(cs));
> +
> + writel(LGM_BUSCON_CMULT_V4 | LGM_BUSCON_RECOVC(1) |
> + LGM_BUSCON_HOLDC(1) | LGM_BUSCON_WAITRDC(2) |
> + LGM_BUSCON_WAITWRC(2) | LGM_BUSCON_BCGEN_CS | LGM_BUSCON_ALEC |
> + LGM_BUSCON_SETUP_EN, lgm_host->lgm_va + LGM_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
> + * Refer the Intel LGM SoC datasheet.

Could you please name the section name in the document, to make it
easier to find it, the number will probably change over time.

> + */
> + writel(0x17400051, lgm_host->lgm_va + LGM_ADDR_SEL(0));
> + writel(0x17C00051, lgm_host->lgm_va + LGM_ADDR_SEL(cs));

Please do not use magic values here. For example you set here the
LGM_ADDR_SEL_REGEN bit a mask and a base.

> + nand_set_flash_node(&lgm_host->chip, dev->of_node);
> + mtd = nand_to_mtd(&lgm_host->chip);
> + mtd->dev.parent = dev;
> + lgm_host->dev = dev;
> +
> + platform_set_drvdata(pdev, lgm_host);
> + nand_set_controller_data(&lgm_host->chip, lgm_host);
> +
> + nand = &lgm_host->chip;
> + nand->controller = &lgm_host->controller;
> + nand->controller->ops = &lgm_nand_controller_ops;
> +
> + /* Scan to find existence of the device */
> + ret = nand_scan(&lgm_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(&lgm_host->chip);
> +exit_dma:
> + lgm_dma_exit(lgm_host);
> +disable_clk:
> + clk_disable_unprepare(lgm_host->clk);
> +
> + return ret;
> +}

Hauke

Attachments:

signature.asc (499.00 B)
OpenPGP digital signature

2020-04-18 08:57:08

[permalink] [raw]

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

On Fri, 17 Apr 2020 16:21:47 +0800
"Ramuthevar,Vadivel MuruganX"
<[email protected]> wrote:

> From: Ramuthevar Vadivel Murugan <[email protected]>
>
> 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 <[email protected]>
> ---
> drivers/mtd/nand/raw/Kconfig | 7 +
> drivers/mtd/nand/raw/Makefile | 1 +
> drivers/mtd/nand/raw/intel_lgm_nand.c | 740 ++++++++++++++++++++++++++++++++++

I wonder if we shouldn't name the driver infineon-nand-controller.c
since the original design comes from Infineon IIUC. intel_lgm_nand.c is
definitely misleading, as we also have a nand_intel.c file which is for
Intel NAND chips (not NAND controllers). If we keep intel in the name,
let's at least add a "-controller" suffix to make it clear.

Side note for Miquel: I guess we would also benefit from having a clear
core vs controller-drivers split as recently done for spi-nor (a
controller subdir has been created).

> 3 files changed, 748 insertions(+)
> create mode 100644 drivers/mtd/nand/raw/intel_lgm_nand.c
>
> diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
> index a80a46bb5b8b..9efc4bbaf4a3 100644
> --- a/drivers/mtd/nand/raw/Kconfig
> +++ b/drivers/mtd/nand/raw/Kconfig
> @@ -457,6 +457,13 @@ 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 X86

Do we have a hard dependency on x86 here? Maybe 'depends on HAS_MMIO'
would be enough.

> + 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..49a301ae0c9d 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_lgm_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/intel_lgm_nand.c b/drivers/mtd/nand/raw/intel_lgm_nand.c
> new file mode 100644
> index 000000000000..96cd1831f070
> --- /dev/null
> +++ b/drivers/mtd/nand/raw/intel_lgm_nand.c
> @@ -0,0 +1,740 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/* Copyright (c) 2019 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/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/resource.h>
> +#include <linux/sched.h>
> +#include <linux/types.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/mtd/rawnand.h>
> +#include <linux/mtd/nand_ecc.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +#include <linux/mtd/partitions.h>
> +#include <linux/io.h>
> +#include <linux/slab.h>
> +#include <mtd/mtd-abi.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/mtd/nand.h>
> +
> +#define LGM_CLC 0x000
> +#define LGM_CLC_RST 0x00000000u
> +
> +#define LGM_NAND_ECC_OFFSET 0x008
> +
> +#define LGM_ADDR_SEL(n) (0x20 + (n) * 4)
> +#define LGM_ADDR_MASK (5 << 4)
> +#define LGM_ADDR_SEL_REGEN 0x1
> +
> +#define LGM_BUSCON(n) (0x60 + (n) * 4)
> +#define LGM_BUSCON_CMULT_V4 0x1
> +#define LGM_BUSCON_RECOVC(n) ((n) << 2)
> +#define LGM_BUSCON_HOLDC(n) ((n) << 4)
> +#define LGM_BUSCON_WAITRDC(n) ((n) << 6)
> +#define LGM_BUSCON_WAITWRC(n) ((n) << 8)
> +#define LGM_BUSCON_BCGEN_CS 0x0
> +#define LGM_BUSCON_SETUP_EN BIT(22)
> +#define LGM_BUSCON_ALEC 0xC000
> +

Hm, I'm pretty sure we don't need the LGM_ prefix here.

> +#define NAND_CON 0x0B0
> +#define NAND_CON_NANDM_EN BIT(0)
> +#define NAND_CON_NANDM_DIS 0x0
> +#define NAND_CON_CSMUX_E_EN BIT(1)
> +#define NAND_CON_ALE_P_LOW BIT(2)
> +#define NAND_CON_CLE_P_LOW BIT(3)
> +#define NAND_CON_CS_P_LOW BIT(4)
> +#define NAND_CON_SE_P_LOW BIT(5)
> +#define NAND_CON_WP_P_LOW BIT(6)
> +#define NAND_CON_PRE_P_LOW BIT(7)
> +#define NAND_CON_IN_CS_S(n) ((n) << 8)
> +#define NAND_CON_OUT_CS_S(n) ((n) << 10)
> +#define NAND_CON_LAT_EN_CS_P ((0x3D) << 18)
> +
> +#define NAND_WAIT 0x0B4
> +#define NAND_WAIT_RDBY BIT(0)
> +#define NAND_WAIT_WR_C BIT(3)
> +
> +#define NAND_CTL1 0x110
> +#define NAND_CTL1_ADDR_3_SHIFT 24
> +
> +#define NAND_CTL2 0x114
> +#define NAND_CTL2_ADDR_5_SHIFT 8
> +#define NAND_CTL2_CYC_N_V5 (0x2 << 16)
> +
> +#define NAND_INT_MSK_CTL 0x124
> +#define NAND_INT_MSK_CTL_WR_C BIT(4)
> +
> +#define NAND_INT_STA 0x128
> +#define NAND_INT_STA_WR_C BIT(4)
> +
> +#define NAND_CTL 0x130
> +#define NAND_CTL_MODE_ECC 0x1
> +#define NAND_CTL_GO BIT(2)
> +#define NAND_CTL_CE_SEL_CS(n) BIT(3 + (n))
> +#define NAND_CTL_RW_READ 0x0
> +#define NAND_CTL_RW_WRITE BIT(10)
> +#define NAND_CTL_ECC_OFF_V8TH BIT(11)
> +#define NAND_CTL_CKFF_EN 0x0
> +#define NAND_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_WRITE_CMD (NAND_CON_CS_P_LOW | NAND_CON_CLE_P_LOW)
> +#define NAND_WRITE_ADDR (NAND_CON_CS_P_LOW | NAND_CON_ALE_P_LOW)

I would redefine ALE, CLE and CS here instead of re-using the NAND_CON
definitions. Even if they have the same value they represent different
things I think. One is encoding the signal polarity when configuring
the NAND controller, and the other one is an offset in the memory bus
MMIO range that's used to control the CLE/ALE/CS signals.

#define NAND_ALE_OFFS BIT(2)
#define NAND_CLE_OFFS BIT(3)
#define NAND_CS_OFFS BIT(4)

> +#define NAND_WRITE_DATA NAND_CON_CS_P_LOW
> +#define NAND_READ_DATA NAND_CON_CS_P_LOW

Can we not hide that behind macros. And there's no point having 2
different definitions for read/write, since all they do is keeping the
CS line asserted, the direction is selection by the operation done on
the bus (read or write). BTW, you even mix those without realizing the
mistake in your implementation :P.

> +
> +#define NAND_CHIP_NO_SELECTION -1
> +#define NAND_CHIP_SELECTION 0x0
> +
> +struct lgm_nand_host {

infineon_nand_controller?

> + struct nand_controller controller;
> + struct nand_chip chip;
> + void __iomem *lgm_va;
> + void __iomem *hsnand_va;
> + void __iomem *nandaddr_va;

You can drop the _va suffixes and pick names describing what's exposed
by the MMIO range (lgm doesn't sounds like a good name to me).

> + 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 u8 lgm_nand_readb(struct nand_chip *chip, int op)

Make op an unsigned int.

> +{
> + struct lgm_nand_host *lgm_host = nand_get_controller_data(chip);
> + void __iomem *nand_wait = lgm_host->lgm_va + NAND_WAIT;
> + u32 stat;
> + int ret;
> + u8 val;
> +
> + val = readb(lgm_host->nandaddr_va + op);
> +
> + ret = readl_poll_timeout(nand_wait, stat, stat & NAND_WAIT_WR_C,
> + 20, 1000);
> + if (ret)
> + dev_warn(lgm_host->dev,
> + "lgm nand write timeout. nand_wait(0x%p)=0x%x\n",
> + nand_wait, readl(nand_wait));
> +
> + return val;
> +}
> +
> +static void lgm_nand_writeb(struct nand_chip *chip, int op, u8 value)
> +{
> + struct lgm_nand_host *lgm_host = nand_get_controller_data(chip);
> + void __iomem *nand_wait = lgm_host->lgm_va + NAND_WAIT;
> + u32 stat;
> + int ret;
> +
> + writeb(value, lgm_host->nandaddr_va + op);

Looks like NAND_CON_CS_P_LOW is always set, so no need to force the
caller to pass it in op.

> +
> + ret = readl_poll_timeout(nand_wait, stat, stat & NAND_WAIT_WR_C,
> + 20, 1000);
> + if (ret)
> + dev_warn(lgm_host->dev,
> + "lgm nand write timeout. nand_wait(0x%p)=0x%x\n",
> + nand_wait, readl(nand_wait));
> +}
> +
> +static unsigned char lgm_read_byte(struct nand_chip *chip)
> +{
> + return lgm_nand_readb(chip, NAND_READ_DATA);
> +}

This one should not be needed.

> +
> +static void lgm_read_buf(struct nand_chip *chip, u_char *buf, int len)
> +{
> + int i;
> +
> + for (i = 0; i < len; i++)
> + buf[i] = lgm_nand_readb(chip, NAND_WRITE_DATA);
> +}
> +
> +static void lgm_write_buf(struct nand_chip *chip, const u_char *buf, int len)
> +{
> + int i;
> +
> + for (i = 0; i < len; i++)
> + lgm_nand_writeb(chip, NAND_WRITE_DATA, buf[i]);
> +}
> +
> +static void lgm_select_chip(struct nand_chip *chip, int select)
> +{
> + struct lgm_nand_host *lgm_host = nand_get_controller_data(chip);
> + void __iomem *nand_con = lgm_host->lgm_va + NAND_CON;
> + u32 cs = lgm_host->cs;
> + int val;
> +
> + switch (select) {
> + case NAND_CHIP_NO_SELECTION:
> + val = readl(nand_con);
> + writel(val & ~NAND_CON_NANDM_EN, nand_con);
> + break;

Please move that to an unselect_chip() function. I also see that this
function is never called with NAND_CHIP_NO_SELECTION. Oh, and you don't
need the select argument since you only support one CS per chip.

> + case NAND_CHIP_SELECTION:
> + writel(NAND_CON_NANDM_EN | NAND_CON_CSMUX_E_EN |
> + NAND_CON_CS_P_LOW | NAND_CON_SE_P_LOW |
> + NAND_CON_WP_P_LOW | NAND_CON_PRE_P_LOW |
> + NAND_CON_IN_CS_S(cs) | NAND_CON_OUT_CS_S(cs) |
> + NAND_CON_LAT_EN_CS_P, nand_con);
> + break;
> + default:
> + break;
> + }
> +}
> +
> +static int lgm_dev_ready(struct nand_chip *chip)
> +{
> + struct lgm_nand_host *lgm_host = nand_get_controller_data(chip);
> +
> + return readl(lgm_host->lgm_va + NAND_WAIT) & NAND_WAIT_RDBY;
> +}
> +
> +static void lgm_cmd_ctrl(struct nand_chip *chip, int cmd, unsigned int ctrl)
> +{
> + if (cmd == NAND_CMD_NONE)
> + return;
> +
> + if (ctrl & NAND_CLE)
> + lgm_nand_writeb(chip, NAND_WRITE_CMD, cmd);
> + else if (ctrl & NAND_ALE)
> + lgm_nand_writeb(chip, NAND_WRITE_ADDR, cmd);
> +}

Looks like you're still sticking to the old cmd_ctrl() interface.
Please inline what can be inlined in exec_op() (everything that's
related to CMD/ADDR cycle emission) and add helpers for the read/write
data logic.

> +
> +static int lgm_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 = LGM_NAND_ECC_OFFSET;
> + oobregion->length = chip->ecc.total;
> +
> + return 0;
> +}
> +
> +static int lgm_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 + LGM_NAND_ECC_OFFSET;
> + oobregion->length = mtd->oobsize - oobregion->offset;
> +
> + return 0;
> +}
> +
> +static const struct mtd_ooblayout_ops lgm_nand_ooblayout_ops = {
> + .ecc = lgm_nand_ooblayout_ecc,
> + .free = lgm_nand_ooblayout_free,
> +};
> +
> +static inline struct lgm_nand_host *nand_to_lgm(struct nand_chip *chip)
> +{
> + return container_of(chip, struct lgm_nand_host, chip);
> +}

Please move that function next to the struct definition, and you can
drop the inline specifier, the compiler should be smart enough to
inline it anyway.

> +static int lgm_nand_exec_op(struct nand_chip *chip,
> + const struct nand_operation *op, bool check_only)
> +{
> + struct lgm_nand_host *host = nand_to_lgm(chip);
> + const struct nand_op_instr *instr = NULL;
> + unsigned int op_id;
> + int i, ret = 0;
> +
> + for (op_id = 0; op_id < op->ninstrs; op_id++) {
> + instr = &op->instrs[op_id];
> +
> + lgm_select_chip(chip, host->cs);

Should be moved before the for() loop (no need to select the chip
every time you excute an instruction).

> +
> + switch (instr->type) {
> + case NAND_OP_CMD_INSTR:
> + lgm_cmd_ctrl(chip, instr->ctx.cmd.opcode, NAND_CLE);

lgm_nand_writeb(chip, NAND_CLE_OFFS,
instr->ctx.cmd.opcode);

> + break;

Missing tab here^.

> +
> + case NAND_OP_ADDR_INSTR:
> + for (i = 0; i < instr->ctx.addr.naddrs; i++)
> + lgm_cmd_ctrl(chip, instr->ctx.addr.addrs[i],
> + NAND_ALE);

lgm_nand_writeb(chip, NAND_ALE_OFFS,
instr->ctx.addr.addrs[i]);

> + break;
> +
> + case NAND_OP_DATA_IN_INSTR:
> + lgm_read_buf(chip, instr->ctx.data.buf.in,
> + instr->ctx.data.len);
> + break;
> +
> + case NAND_OP_DATA_OUT_INSTR:
> + lgm_write_buf(chip, instr->ctx.data.buf.out,
> + instr->ctx.data.len);
> + break;
> +
> + case NAND_OP_WAITRDY_INSTR:
> + ret = lgm_dev_ready(chip);

That's wrong, WAITRDY should wait for the NAND_WAIT_RDBY flag to be
set but you only check its value once.

ret = readl_poll_timeout(lgm_host->lgm_va +
NAND_WAIT, status,
status & NAND_WAIT_RDBY, 20,
instr->ctx.waitrdy.timeout_ms * 1000);
> + break;
> + }
> + }
> +
> + return ret;
> +}
> +
> +static const struct nand_controller_ops lgm_nand_controller_ops {
> + .attach_chip = lgm_nand_attach_chip,
> + .exec_op = lgm_nand_exec_op,
> +};
> +
> +static void lgm_dma_exit(struct lgm_nand_host *lgm_host)

lgm_dma_cleanup()?

> +{
> + if (lgm_host->dma_rx) {
> + dma_release_channel(lgm_host->dma_rx);
> + lgm_host->dma_rx = NULL;
> + }
> +
> + if (lgm_host->dma_tx) {
> + dma_release_channel(lgm_host->dma_tx);
> + lgm_host->dma_tx = NULL;
> + }
> +}
> +
> +static int lgm_dma_init(struct device *dev, struct lgm_nand_host *lgm_host)
> +{
> + int ret;
> +
> + /* Prepare for TX DMA: */
> + lgm_host->dma_tx = dma_request_chan(dev, "tx");
> + if (IS_ERR(lgm_host->dma_tx)) {
> + ret = PTR_ERR(lgm_host->dma_tx);
> + dev_err(dev, "can't get the TX DMA channel, error %d!\n", ret);
> + goto err;
> + }
> +
> + /* Prepare for RX: */
> + lgm_host->dma_rx = dma_request_chan(dev, "rx");

Hm, too bad there's not devm_ version for that one.

> + if (IS_ERR(lgm_host->dma_rx)) {
> + ret = PTR_ERR(lgm_host->dma_rx);
> + dev_err(dev, "can't get the RX DMA channel, error %d\n", ret);
> + goto err;
> + }
> +
> + return 0;
> +err:

No need for an error path if you just return the error code. BTW, I
don't like those functions that don't cleanup behind them when an error
happens. I know it's all handled in the dma_exit() function, but still.

> + return ret;
> +}
> +
> +static int lgm_nand_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct lgm_nand_host *lgm_host;
> + struct nand_chip *nand;
> + phys_addr_t nandaddr_pa;
> + struct mtd_info *mtd;
> + struct resource *res;
> + int ret;
> + u32 cs;
> +
> + lgm_host = devm_kzalloc(dev, sizeof(*lgm_host), GFP_KERNEL);
> + if (!lgm_host)
> + return -ENOMEM;
> +
> + lgm_host->dev = dev;
> + nand_controller_init(&lgm_host->controller);
> +
> + lgm_host->lgm_va =
> + devm_platform_ioremap_resource_byname(pdev, "lgmnand");

lgm_host->lgm_va = devm_platform_ioremap_resource_byname(pdev,
"lgmnand");

> + if (IS_ERR(lgm_host->lgm_va))
> + return PTR_ERR(lgm_host->lgm_va);
> +
> + lgm_host->hsnand_va =
> + devm_platform_ioremap_resource_byname(pdev, "hsnand");
> + if (IS_ERR(lgm_host->hsnand_va))
> + return PTR_ERR(lgm_host->hsnand_va);
> +
> + ret = device_property_read_u32(dev, "nand,cs", &cs);
> + if (ret) {
> + dev_err(dev, "failed to get chip select: %d\n", ret);
> + return ret;
> + }
> +
> + lgm_host->cs = cs;
> +
> + lgm_host->cs_name = devm_kasprintf(dev, GFP_KERNEL, "nand_cs%d", cs);

You don't need to keep the cs_name attached to lgm_host as it's
automatically release. You can just get rid of the lgm_host->cs_name
field.

> + if (IS_ERR(lgm_host->cs_name)) {
> + ret = PTR_ERR(lgm_host->cs_name);
> + dev_err(dev, "failed to get chip select name: %d\n", ret);
> + return ret;
> + }
> +
> + res = devm_platform_ioremap_resource_byname(pdev, lgm_host->cs_name);
> + lgm_host->nandaddr_va = res;
> + nandaddr_pa = res->start;
> + if (IS_ERR(lgm_host->nandaddr_va))
> + return PTR_ERR(lgm_host->nandaddr_va);
> +
> + lgm_host->clk = devm_clk_get(dev, NULL);
> + if (IS_ERR(lgm_host->clk)) {
> + ret = PTR_ERR(lgm_host->clk);
> + dev_err(dev, "failed to get clock: %d\n", ret);
> + return ret;
> + }
> +
> + ret = clk_prepare_enable(lgm_host->clk);
> + if (ret) {
> + dev_err(dev, "failed to enable clock: %d\n", ret);
> + return ret;
> + }
> + lgm_host->clk_rate = clk_get_rate(lgm_host->clk);
> +
> + ret = lgm_dma_init(dev, lgm_host);
> + if (ret)
> + goto disable_clk;
> +
> + writel(lower_32_bits(nandaddr_pa) | LGM_ADDR_SEL_REGEN | LGM_ADDR_MASK,
> + lgm_host->lgm_va + LGM_ADDR_SEL(cs));
> +
> + writel(LGM_BUSCON_CMULT_V4 | LGM_BUSCON_RECOVC(1) |
> + LGM_BUSCON_HOLDC(1) | LGM_BUSCON_WAITRDC(2) |
> + LGM_BUSCON_WAITWRC(2) | LGM_BUSCON_BCGEN_CS | LGM_BUSCON_ALEC |
> + LGM_BUSCON_SETUP_EN, lgm_host->lgm_va + LGM_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
> + * Refer the Intel LGM SoC datasheet.
> + */
> + writel(0x17400051, lgm_host->lgm_va + LGM_ADDR_SEL(0));
> + writel(0x17C00051, lgm_host->lgm_va + LGM_ADDR_SEL(cs));

No magic value please. I guess the 0x51 at the end encode some flags, so
please describe those fields and come with a macro to generate the base
range value (or a mapping table).

> + nand_set_flash_node(&lgm_host->chip, dev->of_node);
> + mtd = nand_to_mtd(&lgm_host->chip);
> + mtd->dev.parent = dev;
> + lgm_host->dev = dev;
> +
> + platform_set_drvdata(pdev, lgm_host);
> + nand_set_controller_data(&lgm_host->chip, lgm_host);
> +
> + nand = &lgm_host->chip;
> + nand->controller = &lgm_host->controller;
> + nand->controller->ops = &lgm_nand_controller_ops;
> +
> + /* Scan to find existence of the device */
> + ret = nand_scan(&lgm_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(&lgm_host->chip);
> +exit_dma:
> + lgm_dma_exit(lgm_host);
> +disable_clk:
> + clk_disable_unprepare(lgm_host->clk);
> +
> + return ret;
> +}
> +
> +static int lgm_nand_remove(struct platform_device *pdev)
> +{
> + struct lgm_nand_host *lgm_host = platform_get_drvdata(pdev);
> +
> + nand_release(&lgm_host->chip);

Can you use mtd_device_unregister() + nand_cleanup() instead, and check
their return value?

> + clk_disable_unprepare(lgm_host->clk);
> + lgm_dma_exit(lgm_host);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id lgm_nand_match[] = {
> + { .compatible = "intel,lgm-nand", },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, lgm_nand_match);

You probably have a missing "depends on OF" in your Kconfig.

> +
> +static struct platform_driver lgm_nand_driver = {
> + .probe = lgm_nand_probe,
> + .remove = lgm_nand_remove,
> + .driver = {
> + .name = "intel-lgm-nand",
> + .of_match_table = lgm_nand_match,
> + },
> +
> +};
> +module_platform_driver(lgm_nand_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Vadivel Murugan R <[email protected]>");
> +MODULE_DESCRIPTION("Intel's LGM External Bus NAND Controller driver");

I didn't review the DMA and ECC aspects yet, but I think you have enough
things to address for a v3.

2020-04-19 22:21:58

[permalink] [raw]

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

On Sat, Apr 18, 2020 at 10:55:33AM +0200, Boris Brezillon wrote:
> On Fri, 17 Apr 2020 16:21:47 +0800
> "Ramuthevar,Vadivel MuruganX"
> <[email protected]> wrote:
>
> > From: Ramuthevar Vadivel Murugan <[email protected]>

> > +static const struct of_device_id lgm_nand_match[] = {
> > + { .compatible = "intel,lgm-nand", },
> > + {}
> > +};
> > +MODULE_DEVICE_TABLE(of, lgm_nand_match);
>
> You probably have a missing "depends on OF" in your Kconfig.

Since it's using device property API, dependency is not needed.

--
With Best Regards,
Andy Shevchenko

2020-04-19 22:30:22

[permalink] [raw]

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

On Fri, Apr 17, 2020 at 04:21:47PM +0800, Ramuthevar,Vadivel MuruganX wrote:
> From: Ramuthevar Vadivel Murugan <[email protected]>
>
> 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.

I guess untested version slipped into mailing list...
See below why.

...

> +#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/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/resource.h>
> +#include <linux/sched.h>
> +#include <linux/types.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/mtd/rawnand.h>
> +#include <linux/mtd/nand_ecc.h>
> +#include <linux/platform_device.h>

> +#include <linux/of.h>

Do you need this?

> +#include <linux/mtd/partitions.h>
> +#include <linux/io.h>
> +#include <linux/slab.h>
> +#include <mtd/mtd-abi.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/mtd/nand.h>

Basically, do you need all of them?

And maybe keep them in order?

...

> +static int lgm_dma_init(struct device *dev, struct lgm_nand_host *lgm_host)
> +{
> + int ret;
> +
> + /* Prepare for TX DMA: */
> + lgm_host->dma_tx = dma_request_chan(dev, "tx");
> + if (IS_ERR(lgm_host->dma_tx)) {
> + ret = PTR_ERR(lgm_host->dma_tx);
> + dev_err(dev, "can't get the TX DMA channel, error %d!\n", ret);
> + goto err;
> + }
> +
> + /* Prepare for RX: */
> + lgm_host->dma_rx = dma_request_chan(dev, "rx");
> + if (IS_ERR(lgm_host->dma_rx)) {
> + ret = PTR_ERR(lgm_host->dma_rx);
> + dev_err(dev, "can't get the RX DMA channel, error %d\n", ret);

I suspect this error path hasn't been tested. I don't see where tx channel
freeing is happening.

> + goto err;
> + }
> +
> + return 0;

> +err:
> + return ret;

Redundant label.

> +}

...

> + res = devm_platform_ioremap_resource_byname(pdev, lgm_host->cs_name);
> + lgm_host->nandaddr_va = res;
> + nandaddr_pa = res->start;
> + if (IS_ERR(lgm_host->nandaddr_va))
> + return PTR_ERR(lgm_host->nandaddr_va);

I'm wondering what is this. How is it even compile?

--
With Best Regards,
Andy Shevchenko

2020-04-20 03:29:44

[permalink] [raw]

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

Hi Andy,

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

On 20/4/2020 6:28 am, Andy Shevchenko wrote:
> On Fri, Apr 17, 2020 at 04:21:47PM +0800, Ramuthevar,Vadivel MuruganX wrote:
>> From: Ramuthevar Vadivel Murugan <[email protected]>
>>
>> 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.
> I guess untested version slipped into mailing list...
> See below why.

Sorry, This is original patch only , header files are mis-aligned so
looks like un-tested patch.

> ...
>
>> +#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/iopoll.h>
>> +#include <linux/module.h>
>> +#include <linux/resource.h>
>> +#include <linux/sched.h>
>> +#include <linux/types.h>
>> +#include <linux/mtd/mtd.h>
>> +#include <linux/mtd/rawnand.h>
>> +#include <linux/mtd/nand_ecc.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/of.h>
> Do you need this?
Noted, will check and drop if it is notnecessary.
>> +#include <linux/mtd/partitions.h>
>> +#include <linux/io.h>
>> +#include <linux/slab.h>
>> +#include <mtd/mtd-abi.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/mtd/nand.h>
> Basically, do you need all of them?
>
> And maybe keep them in order?
Sure, will update.
> ...
>
>> +static int lgm_dma_init(struct device *dev, struct lgm_nand_host *lgm_host)
>> +{
>> + int ret;
>> +
>> + /* Prepare for TX DMA: */
>> + lgm_host->dma_tx = dma_request_chan(dev, "tx");
>> + if (IS_ERR(lgm_host->dma_tx)) {
>> + ret = PTR_ERR(lgm_host->dma_tx);
>> + dev_err(dev, "can't get the TX DMA channel, error %d!\n", ret);
>> + goto err;
>> + }
>> +
>> + /* Prepare for RX: */
>> + lgm_host->dma_rx = dma_request_chan(dev, "rx");
>> + if (IS_ERR(lgm_host->dma_rx)) {
>> + ret = PTR_ERR(lgm_host->dma_rx);
>> + dev_err(dev, "can't get the RX DMA channel, error %d\n", ret);
> I suspect this error path hasn't been tested. I don't see where tx channel
> freeing is happening.
Good catch, Thanks!, will update
>> + goto err;
>> + }
>> +
>> + return 0;
>> +err:
>> + return ret;
> Redundant label.
Noted.
>> +}
> ...
>
>> + res = devm_platform_ioremap_resource_byname(pdev, lgm_host->cs_name);
>> + lgm_host->nandaddr_va = res;
>> + nandaddr_pa = res->start;
>> + if (IS_ERR(lgm_host->nandaddr_va))
>> + return PTR_ERR(lgm_host->nandaddr_va);
> I'm wonderig what is this. How is it even compile?

Agreed!, need a correction, but it's compiled.

Regards
Vadivel
>

2020-04-20 03:50:37

[permalink] [raw]

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

Hi Hauke,

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

On 18/4/2020 1:05 am, Hauke Mehrtens wrote:
> On 4/17/20 10:21 AM, Ramuthevar, Vadivel MuruganX wrote:
>> From: Ramuthevar Vadivel Murugan <[email protected]>
> Thanks for adding me in CC, I am using my private mail it took me too
> long to configure Outlook.
>
> I compared the register description of the LGM with the VRX200, the EBU
> and NAND block are looking very similar. I think the VRX200 also
> supports HW ECC and DMA, nobody implimented it in the upstream driver.
>
> I think replacing the old XWAY dirver with this one is a good approach.
Thank you for confirmation to replace the xway_nand.c since you are the
owner for that driver.
> Then we can add feature flags to activate the extra features only on the
> SoCs which support them.
>
> On older SoCs the EBU (NAND) and the PCI (not express) IP core are
> sharing some parts like an endianess switch, this is causeing some more
> problems.
>
>> 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 <[email protected]>
> .....
>
>> +config MTD_NAND_INTEL_LGM
>> + tristate "Support for NAND controller on Intel LGM SoC"
>> + depends on X86
> Compile test should also work for other archs
Noted, will add it.
>> + help
>> + Enables support for NAND Flash chips on Intel's LGM SoC.
>> + NAND flash interfaced through the External Bus Unit.
>> +
>> comment "Misc"
> .....
>
>> +
>> +#define LGM_CLC 0x000
>> +#define LGM_CLC_RST 0x00000000u
>> +
>> +#define LGM_NAND_ECC_OFFSET 0x008
> It is confusing that this is not a register but a different definition.
> Please move it somewheer else.
Noted, will move it.
>> +#define LGM_ADDR_SEL(n) (0x20 + (n) * 4)
>> +#define LGM_ADDR_MASK (5 << 4)
>> +#define LGM_ADDR_SEL_REGEN 0x1
>> +
>> +#define LGM_BUSCON(n) (0x60 + (n) * 4)
>> +#define LGM_BUSCON_CMULT_V4 0x1
>> +#define LGM_BUSCON_RECOVC(n) ((n) << 2)
>> +#define LGM_BUSCON_HOLDC(n) ((n) << 4)
>> +#define LGM_BUSCON_WAITRDC(n) ((n) << 6)
>> +#define LGM_BUSCON_WAITWRC(n) ((n) << 8)
>> +#define LGM_BUSCON_BCGEN_CS 0x0
>> +#define LGM_BUSCON_SETUP_EN BIT(22)
>> +#define LGM_BUSCON_ALEC 0xC000
>> +
>> +#define NAND_CON 0x0B0
>> +#define NAND_CON_NANDM_EN BIT(0)
>> +#define NAND_CON_NANDM_DIS 0x0
>> +#define NAND_CON_CSMUX_E_EN BIT(1)
>> +#define NAND_CON_ALE_P_LOW BIT(2)
>> +#define NAND_CON_CLE_P_LOW BIT(3)
>> +#define NAND_CON_CS_P_LOW BIT(4)
>> +#define NAND_CON_SE_P_LOW BIT(5)
>> +#define NAND_CON_WP_P_LOW BIT(6)
>> +#define NAND_CON_PRE_P_LOW BIT(7)
>> +#define NAND_CON_IN_CS_S(n) ((n) << 8)
>> +#define NAND_CON_OUT_CS_S(n) ((n) << 10)
>> +#define NAND_CON_LAT_EN_CS_P ((0x3D) << 18)
>> +
>> +#define NAND_WAIT 0x0B4
>> +#define NAND_WAIT_RDBY BIT(0)
>> +#define NAND_WAIT_WR_C BIT(3)
> The registers with the LGM_ prefix, NAND_CON and NAND_WAIT are from the
> EBU (EBU_N) register block. I would prefer if the have the same prefix.
> You should use a different prefix for the register definitions below
> this comment, which are from the NAND Controller (HSNAND).
Sure, will use as you have suggested
>> +#define NAND_CTL1 0x110
>> +#define NAND_CTL1_ADDR_3_SHIFT 24
>> +
>> +#define NAND_CTL2 0x114
>> +#define NAND_CTL2_ADDR_5_SHIFT 8
>> +#define NAND_CTL2_CYC_N_V5 (0x2 << 16)
>> +
>> +#define NAND_INT_MSK_CTL 0x124
>> +#define NAND_INT_MSK_CTL_WR_C BIT(4)
>> +
>> +#define NAND_INT_STA 0x128
>> +#define NAND_INT_STA_WR_C BIT(4)
>> +
>> +#define NAND_CTL 0x130
>> +#define NAND_CTL_MODE_ECC 0x1
>> +#define NAND_CTL_GO BIT(2)
>> +#define NAND_CTL_CE_SEL_CS(n) BIT(3 + (n))
>> +#define NAND_CTL_RW_READ 0x0
>> +#define NAND_CTL_RW_WRITE BIT(10)
>> +#define NAND_CTL_ECC_OFF_V8TH BIT(11)
>> +#define NAND_CTL_CKFF_EN 0x0
>> +#define NAND_CTL_MSG_EN BIT(17)
> Till here I find the same registers you use also in the VRX200
> description. I haven't checked all the bits. Danube only has the
> registers from the EBU, I haven't checked the SoCs in between.
Okay, I don't have other SoC's datasheet to check, otherwise would have
checked it.
>> +#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_WRITE_CMD (NAND_CON_CS_P_LOW | NAND_CON_CLE_P_LOW)
>> +#define NAND_WRITE_ADDR (NAND_CON_CS_P_LOW | NAND_CON_ALE_P_LOW)
>> +#define NAND_WRITE_DATA NAND_CON_CS_P_LOW
>> +#define NAND_READ_DATA NAND_CON_CS_P_LOW
>> +
>> +#define NAND_CHIP_NO_SELECTION -1
>> +#define NAND_CHIP_SELECTION 0x0
>> +
> ....
>> +static int lgm_nand_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct lgm_nand_host *lgm_host;
>> + struct nand_chip *nand;
>> + phys_addr_t nandaddr_pa;
>> + struct mtd_info *mtd;
>> + struct resource *res;
>> + int ret;
>> + u32 cs;
>> +
>> + lgm_host = devm_kzalloc(dev, sizeof(*lgm_host), GFP_KERNEL);
>> + if (!lgm_host)
>> + return -ENOMEM;
>> +
>> + lgm_host->dev = dev;
>> + nand_controller_init(&lgm_host->controller);
>> +
>> + lgm_host->lgm_va =
>> + devm_platform_ioremap_resource_byname(pdev, "lgmnand");
>> + if (IS_ERR(lgm_host->lgm_va))
>> + return PTR_ERR(lgm_host->lgm_va);
> lgm_va is named ebu_n in the register description. Could you rename this
> variable to ebu. I think the _va postfix is not needed as it should be
> clear that this is a virtual addresss.
Noted, will rename it.
>
>> + lgm_host->hsnand_va =
>> + devm_platform_ioremap_resource_byname(pdev, "hsnand");
>> + if (IS_ERR(lgm_host->hsnand_va))
>> + return PTR_ERR(lgm_host->hsnand_va);
>> +
>> + ret = device_property_read_u32(dev, "nand,cs", &cs);
>> + if (ret) {
>> + dev_err(dev, "failed to get chip select: %d\n", ret);
>> + return ret;
>> + }
> The cs should be validated, ifsome uses cs == 4 there will be a problem.
> LGM supports CS 0 to 3 like the VRX200, danube for example only 0 and 1.
Even LGM also supports 0 or 1
>
>> + lgm_host->cs = cs;
>> +
>> + lgm_host->cs_name = devm_kasprintf(dev, GFP_KERNEL, "nand_cs%d", cs);
> cs_name is only used locally in this function you can also use some
> memory on the stack to generate the name.
>
>> + if (IS_ERR(lgm_host->cs_name)) {
>> + ret = PTR_ERR(lgm_host->cs_name);
>> + dev_err(dev, "failed to get chip select name: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + res = devm_platform_ioremap_resource_byname(pdev, lgm_host->cs_name);
> This will only support one chip select at a time on the SoC. It is not
> possible to configure the DTS in a way to talk to two NAND chips on
> different chip selects. I think this is an uncommon scenario and I do
> not know if other NAND drivers in Linux support this feature.

Yes, you are Right, LGM supports only one nand chip at a time.

>> + lgm_host->nandaddr_va = res;
>> + nandaddr_pa = res->start;
>> + if (IS_ERR(lgm_host->nandaddr_va))
>> + return PTR_ERR(lgm_host->nandaddr_va);
>> +
>> + lgm_host->clk = devm_clk_get(dev, NULL);
>> + if (IS_ERR(lgm_host->clk)) {
>> + ret = PTR_ERR(lgm_host->clk);
>> + dev_err(dev, "failed to get clock: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + ret = clk_prepare_enable(lgm_host->clk);
>> + if (ret) {
>> + dev_err(dev, "failed to enable clock: %d\n", ret);
>> + return ret;
>> + }
>> + lgm_host->clk_rate = clk_get_rate(lgm_host->clk);
>> +
>> + ret = lgm_dma_init(dev, lgm_host);
>> + if (ret)
>> + goto disable_clk;
>> +
>> + writel(lower_32_bits(nandaddr_pa) | LGM_ADDR_SEL_REGEN | LGM_ADDR_MASK,
>> + lgm_host->lgm_va + LGM_ADDR_SEL(cs));
>> +
>> + writel(LGM_BUSCON_CMULT_V4 | LGM_BUSCON_RECOVC(1) |
>> + LGM_BUSCON_HOLDC(1) | LGM_BUSCON_WAITRDC(2) |
>> + LGM_BUSCON_WAITWRC(2) | LGM_BUSCON_BCGEN_CS | LGM_BUSCON_ALEC |
>> + LGM_BUSCON_SETUP_EN, lgm_host->lgm_va + LGM_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
>> + * Refer the Intel LGM SoC datasheet.
> Could you please name the section name in the document, to make it
> easier to find it, the number will probably change over time.

okay, Noted

Regards
Vadivel
>> + */
>> + writel(0x17400051, lgm_host->lgm_va + LGM_ADDR_SEL(0));
>> + writel(0x17C00051, lgm_host->lgm_va + LGM_ADDR_SEL(cs));
> Please do not use magic values here. For example you set here the
> LGM_ADDR_SEL_REGEN bit a mask and a base.
Noted, will update with MACRO
>> + nand_set_flash_node(&lgm_host->chip, dev->of_node);
>> + mtd = nand_to_mtd(&lgm_host->chip);
>> + mtd->dev.parent = dev;
>> + lgm_host->dev = dev;
>> +
>> + platform_set_drvdata(pdev, lgm_host);
>> + nand_set_controller_data(&lgm_host->chip, lgm_host);
>> +
>> + nand = &lgm_host->chip;
>> + nand->controller = &lgm_host->controller;
>> + nand->controller->ops = &lgm_nand_controller_ops;
>> +
>> + /* Scan to find existence of the device */
>> + ret = nand_scan(&lgm_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(&lgm_host->chip);
>> +exit_dma:
>> + lgm_dma_exit(lgm_host);
>> +disable_clk:
>> + clk_disable_unprepare(lgm_host->clk);
>> +
>> + return ret;
>> +}
> Hauke
>

2020-04-20 04:20:44

[permalink] [raw]

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

Hi Boris,

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

On 18/4/2020 4:55 pm, Boris Brezillon wrote:
> On Fri, 17 Apr 2020 16:21:47 +0800
> "Ramuthevar,Vadivel MuruganX"
> <[email protected]> wrote:
>
>> From: Ramuthevar Vadivel Murugan <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> drivers/mtd/nand/raw/Kconfig | 7 +
>> drivers/mtd/nand/raw/Makefile | 1 +
>> drivers/mtd/nand/raw/intel_lgm_nand.c | 740 ++++++++++++++++++++++++++++++++++
> I wonder if we shouldn't name the driver infineon-nand-controller.c
> since the original design comes from Infineon IIUC. intel_lgm_nand.c is
> definitely misleading, as we also have a nand_intel.c file which is for
> Intel NAND chips (not NAND controllers). If we keep intel in the name,
> let's at least add a "-controller" suffix to make it clear.
sure , will add -controller suffix.
>
> Side note for Miquel: I guess we would also benefit from having a clear
> core vs controller-drivers split as recently done for spi-nor (a
> controller subdir has been created).
>
>> 3 files changed, 748 insertions(+)
>> create mode 100644 drivers/mtd/nand/raw/intel_lgm_nand.c
>>
>> diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
>> index a80a46bb5b8b..9efc4bbaf4a3 100644
>> --- a/drivers/mtd/nand/raw/Kconfig
>> +++ b/drivers/mtd/nand/raw/Kconfig
>> @@ -457,6 +457,13 @@ 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 X86
> Do we have a hard dependency on x86 here? Maybe 'depends on HAS_MMIO'
> would be enough.
yes Boris, we have hard dependency on x86.
>> + 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..49a301ae0c9d 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_lgm_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/intel_lgm_nand.c b/drivers/mtd/nand/raw/intel_lgm_nand.c
>> new file mode 100644
>> index 000000000000..96cd1831f070
>> --- /dev/null
>> +++ b/drivers/mtd/nand/raw/intel_lgm_nand.c
>> @@ -0,0 +1,740 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/* Copyright (c) 2019 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/iopoll.h>
>> +#include <linux/module.h>
>> +#include <linux/resource.h>
>> +#include <linux/sched.h>
>> +#include <linux/types.h>
>> +#include <linux/mtd/mtd.h>
>> +#include <linux/mtd/rawnand.h>
>> +#include <linux/mtd/nand_ecc.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/of.h>
>> +#include <linux/mtd/partitions.h>
>> +#include <linux/io.h>
>> +#include <linux/slab.h>
>> +#include <mtd/mtd-abi.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/mtd/nand.h>
>> +
>> +#define LGM_CLC 0x000
>> +#define LGM_CLC_RST 0x00000000u
>> +
>> +#define LGM_NAND_ECC_OFFSET 0x008
>> +
>> +#define LGM_ADDR_SEL(n) (0x20 + (n) * 4)
>> +#define LGM_ADDR_MASK (5 << 4)
>> +#define LGM_ADDR_SEL_REGEN 0x1
>> +
>> +#define LGM_BUSCON(n) (0x60 + (n) * 4)
>> +#define LGM_BUSCON_CMULT_V4 0x1
>> +#define LGM_BUSCON_RECOVC(n) ((n) << 2)
>> +#define LGM_BUSCON_HOLDC(n) ((n) << 4)
>> +#define LGM_BUSCON_WAITRDC(n) ((n) << 6)
>> +#define LGM_BUSCON_WAITWRC(n) ((n) << 8)
>> +#define LGM_BUSCON_BCGEN_CS 0x0
>> +#define LGM_BUSCON_SETUP_EN BIT(22)
>> +#define LGM_BUSCON_ALEC 0xC000
>> +
> Hm, I'm pretty sure we don't need the LGM_ prefix here.
Agreed!, will replace EBU_ prefix to LGM_
>> +#define NAND_CON 0x0B0
>> +#define NAND_CON_NANDM_EN BIT(0)
>> +#define NAND_CON_NANDM_DIS 0x0
>> +#define NAND_CON_CSMUX_E_EN BIT(1)
>> +#define NAND_CON_ALE_P_LOW BIT(2)
>> +#define NAND_CON_CLE_P_LOW BIT(3)
>> +#define NAND_CON_CS_P_LOW BIT(4)
>> +#define NAND_CON_SE_P_LOW BIT(5)
>> +#define NAND_CON_WP_P_LOW BIT(6)
>> +#define NAND_CON_PRE_P_LOW BIT(7)
>> +#define NAND_CON_IN_CS_S(n) ((n) << 8)
>> +#define NAND_CON_OUT_CS_S(n) ((n) << 10)
>> +#define NAND_CON_LAT_EN_CS_P ((0x3D) << 18)
>> +
>> +#define NAND_WAIT 0x0B4
>> +#define NAND_WAIT_RDBY BIT(0)
>> +#define NAND_WAIT_WR_C BIT(3)
>> +
>> +#define NAND_CTL1 0x110
>> +#define NAND_CTL1_ADDR_3_SHIFT 24
>> +
>> +#define NAND_CTL2 0x114
>> +#define NAND_CTL2_ADDR_5_SHIFT 8
>> +#define NAND_CTL2_CYC_N_V5 (0x2 << 16)
>> +
>> +#define NAND_INT_MSK_CTL 0x124
>> +#define NAND_INT_MSK_CTL_WR_C BIT(4)
>> +
>> +#define NAND_INT_STA 0x128
>> +#define NAND_INT_STA_WR_C BIT(4)
>> +
>> +#define NAND_CTL 0x130
>> +#define NAND_CTL_MODE_ECC 0x1
>> +#define NAND_CTL_GO BIT(2)
>> +#define NAND_CTL_CE_SEL_CS(n) BIT(3 + (n))
>> +#define NAND_CTL_RW_READ 0x0
>> +#define NAND_CTL_RW_WRITE BIT(10)
>> +#define NAND_CTL_ECC_OFF_V8TH BIT(11)
>> +#define NAND_CTL_CKFF_EN 0x0
>> +#define NAND_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_WRITE_CMD (NAND_CON_CS_P_LOW | NAND_CON_CLE_P_LOW)
>> +#define NAND_WRITE_ADDR (NAND_CON_CS_P_LOW | NAND_CON_ALE_P_LOW)
> I would redefine ALE, CLE and CS here instead of re-using the NAND_CON
> definitions. Even if they have the same value they represent different
> things I think. One is encoding the signal polarity when configuring
> the NAND controller, and the other one is an offset in the memory bus
> MMIO range that's used to control the CLE/ALE/CS signals.
Noted, will update.
>
> #define NAND_ALE_OFFS BIT(2)
> #define NAND_CLE_OFFS BIT(3)
> #define NAND_CS_OFFS BIT(4)
>
>> +#define NAND_WRITE_DATA NAND_CON_CS_P_LOW
>> +#define NAND_READ_DATA NAND_CON_CS_P_LOW
> Can we not hide that behind macros. And there's no point having 2
> different definitions for read/write, since all they do is keeping the
> CS line asserted, the direction is selection by the operation done on
> the bus (read or write). BTW, you even mix those without realizing the
> mistake in your implementation :P.
Sure, do optimization with single macro
>
>> +
>> +#define NAND_CHIP_NO_SELECTION -1
>> +#define NAND_CHIP_SELECTION 0x0
>> +
>> +struct lgm_nand_host {
> infineon_nand_controller?
better we keep intel_nand_controller , is it okay?
>> + struct nand_controller controller;
>> + struct nand_chip chip;
>> + void __iomem *lgm_va;
>> + void __iomem *hsnand_va;
>> + void __iomem *nandaddr_va;
> You can drop the _va suffixes and pick names describing what's exposed
> by the MMIO range (lgm doesn't sounds like a good name to me).
Noted, will pick the proper name
>> + 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 u8 lgm_nand_readb(struct nand_chip *chip, int op)
> Make op an unsigned int.
okay, noted.
>> +{
>> + struct lgm_nand_host *lgm_host = nand_get_controller_data(chip);
>> + void __iomem *nand_wait = lgm_host->lgm_va + NAND_WAIT;
>> + u32 stat;
>> + int ret;
>> + u8 val;
>> +
>> + val = readb(lgm_host->nandaddr_va + op);
>> +
>> + ret = readl_poll_timeout(nand_wait, stat, stat & NAND_WAIT_WR_C,
>> + 20, 1000);
>> + if (ret)
>> + dev_warn(lgm_host->dev,
>> + "lgm nand write timeout. nand_wait(0x%p)=0x%x\n",
>> + nand_wait, readl(nand_wait));
>> +
>> + return val;
>> +}
>> +
>> +static void lgm_nand_writeb(struct nand_chip *chip, int op, u8 value)
>> +{
>> + struct lgm_nand_host *lgm_host = nand_get_controller_data(chip);
>> + void __iomem *nand_wait = lgm_host->lgm_va + NAND_WAIT;
>> + u32 stat;
>> + int ret;
>> +
>> + writeb(value, lgm_host->nandaddr_va + op);
> Looks like NAND_CON_CS_P_LOW is always set, so no need to force the
> caller to pass it in op.
Agreed!
>> +
>> + ret = readl_poll_timeout(nand_wait, stat, stat & NAND_WAIT_WR_C,
>> + 20, 1000);
>> + if (ret)
>> + dev_warn(lgm_host->dev,
>> + "lgm nand write timeout. nand_wait(0x%p)=0x%x\n",
>> + nand_wait, readl(nand_wait));
>> +}
>> +
>> +static unsigned char lgm_read_byte(struct nand_chip *chip)
>> +{
>> + return lgm_nand_readb(chip, NAND_READ_DATA);
>> +}
> This one should not be needed.
Good catch , will drop it.
>> +
>> +static void lgm_read_buf(struct nand_chip *chip, u_char *buf, int len)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < len; i++)
>> + buf[i] = lgm_nand_readb(chip, NAND_WRITE_DATA);
>> +}
>> +
>> +static void lgm_write_buf(struct nand_chip *chip, const u_char *buf, int len)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < len; i++)
>> + lgm_nand_writeb(chip, NAND_WRITE_DATA, buf[i]);
>> +}
>> +
>> +static void lgm_select_chip(struct nand_chip *chip, int select)
>> +{
>> + struct lgm_nand_host *lgm_host = nand_get_controller_data(chip);
>> + void __iomem *nand_con = lgm_host->lgm_va + NAND_CON;
>> + u32 cs = lgm_host->cs;
>> + int val;
>> +
>> + switch (select) {
>> + case NAND_CHIP_NO_SELECTION:
>> + val = readl(nand_con);
>> + writel(val & ~NAND_CON_NANDM_EN, nand_con);
>> + break;
> Please move that to an unselect_chip() function. I also see that this
> function is never called with NAND_CHIP_NO_SELECTION. Oh, and you don't
> need the select argument since you only support one CS per chip.
Sure , will update as per single chip selection support.
>
>> + case NAND_CHIP_SELECTION:
>> + writel(NAND_CON_NANDM_EN | NAND_CON_CSMUX_E_EN |
>> + NAND_CON_CS_P_LOW | NAND_CON_SE_P_LOW |
>> + NAND_CON_WP_P_LOW | NAND_CON_PRE_P_LOW |
>> + NAND_CON_IN_CS_S(cs) | NAND_CON_OUT_CS_S(cs) |
>> + NAND_CON_LAT_EN_CS_P, nand_con);
>> + break;
>> + default:
>> + break;
>> + }
>> +}
>> +
>> +static int lgm_dev_ready(struct nand_chip *chip)
>> +{
>> + struct lgm_nand_host *lgm_host = nand_get_controller_data(chip);
>> +
>> + return readl(lgm_host->lgm_va + NAND_WAIT) & NAND_WAIT_RDBY;
>> +}
>> +
>> +static void lgm_cmd_ctrl(struct nand_chip *chip, int cmd, unsigned int ctrl)
>> +{
>> + if (cmd == NAND_CMD_NONE)
>> + return;
>> +
>> + if (ctrl & NAND_CLE)
>> + lgm_nand_writeb(chip, NAND_WRITE_CMD, cmd);
>> + else if (ctrl & NAND_ALE)
>> + lgm_nand_writeb(chip, NAND_WRITE_ADDR, cmd);
>> +}
> Looks like you're still sticking to the old cmd_ctrl() interface.
> Please inline what can be inlined in exec_op() (everything that's
> related to CMD/ADDR cycle emission) and add helpers for the read/write
> data logic.
Yes, Your are right, function definition need to be updated and aligned
with exec_op() based definitions
>> +
>> +static int lgm_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 = LGM_NAND_ECC_OFFSET;
>> + oobregion->length = chip->ecc.total;
>> +
>> + return 0;
>> +}
>> +
>> +static int lgm_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 + LGM_NAND_ECC_OFFSET;
>> + oobregion->length = mtd->oobsize - oobregion->offset;
>> +
>> + return 0;
>> +}
>> +
>> +static const struct mtd_ooblayout_ops lgm_nand_ooblayout_ops = {
>> + .ecc = lgm_nand_ooblayout_ecc,
>> + .free = lgm_nand_ooblayout_free,
>> +};
>> +
>> +static inline struct lgm_nand_host *nand_to_lgm(struct nand_chip *chip)
>> +{
>> + return container_of(chip, struct lgm_nand_host, chip);
>> +}
> Please move that function next to the struct definition, and you can
> drop the inline specifier, the compiler should be smart enough to
> inline it anyway.
Noted, will move next to structure and also drop inline.
>> +static int lgm_nand_exec_op(struct nand_chip *chip,
>> + const struct nand_operation *op, bool check_only)
>> +{
>> + struct lgm_nand_host *host = nand_to_lgm(chip);
>> + const struct nand_op_instr *instr = NULL;
>> + unsigned int op_id;
>> + int i, ret = 0;
>> +
>> + for (op_id = 0; op_id < op->ninstrs; op_id++) {
>> + instr = &op->instrs[op_id];
>> +
>> + lgm_select_chip(chip, host->cs);
> Should be moved before the for() loop (no need to select the chip
> every time you excute an instruction).
Good catch, Thanks!
>> +
>> + switch (instr->type) {
>> + case NAND_OP_CMD_INSTR:
>> + lgm_cmd_ctrl(chip, instr->ctx.cmd.opcode, NAND_CLE);
> lgm_nand_writeb(chip, NAND_CLE_OFFS,
> instr->ctx.cmd.opcode);
>
>> + break;
> Missing tab here^.
Noted.
>> +
>> + case NAND_OP_ADDR_INSTR:
>> + for (i = 0; i < instr->ctx.addr.naddrs; i++)
>> + lgm_cmd_ctrl(chip, instr->ctx.addr.addrs[i],
>> + NAND_ALE);
> lgm_nand_writeb(chip, NAND_ALE_OFFS,
> instr->ctx.addr.addrs[i]);
>
>> + break;
>> +
>> + case NAND_OP_DATA_IN_INSTR:
>> + lgm_read_buf(chip, instr->ctx.data.buf.in,
>> + instr->ctx.data.len);
>> + break;
>> +
>> + case NAND_OP_DATA_OUT_INSTR:
>> + lgm_write_buf(chip, instr->ctx.data.buf.out,
>> + instr->ctx.data.len);
>> + break;
>> +
>> + case NAND_OP_WAITRDY_INSTR:
>> + ret = lgm_dev_ready(chip);
> That's wrong, WAITRDY should wait for the NAND_WAIT_RDBY flag to be
> set but you only check its value once.
okay, let me check and update.
>
> ret = readl_poll_timeout(lgm_host->lgm_va +
> NAND_WAIT, status,
> status & NAND_WAIT_RDBY, 20,
> instr->ctx.waitrdy.timeout_ms * 1000);
>> + break;
>> + }
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static const struct nand_controller_ops lgm_nand_controller_ops {
>> + .attach_chip = lgm_nand_attach_chip,
>> + .exec_op = lgm_nand_exec_op,
>> +};
>> +
>> +static void lgm_dma_exit(struct lgm_nand_host *lgm_host)
> lgm_dma_cleanup()?
Noted, will update the function name
>> +{
>> + if (lgm_host->dma_rx) {
>> + dma_release_channel(lgm_host->dma_rx);
>> + lgm_host->dma_rx = NULL;
>> + }
>> +
>> + if (lgm_host->dma_tx) {
>> + dma_release_channel(lgm_host->dma_tx);
>> + lgm_host->dma_tx = NULL;
>> + }
>> +}
>> +
>> +static int lgm_dma_init(struct device *dev, struct lgm_nand_host *lgm_host)
>> +{
>> + int ret;
>> +
>> + /* Prepare for TX DMA: */
>> + lgm_host->dma_tx = dma_request_chan(dev, "tx");
>> + if (IS_ERR(lgm_host->dma_tx)) {
>> + ret = PTR_ERR(lgm_host->dma_tx);
>> + dev_err(dev, "can't get the TX DMA channel, error %d!\n", ret);
>> + goto err;
>> + }
>> +
>> + /* Prepare for RX: */
>> + lgm_host->dma_rx = dma_request_chan(dev, "rx");
> Hm, too bad there's not devm_ version for that one.
>
>> + if (IS_ERR(lgm_host->dma_rx)) {
>> + ret = PTR_ERR(lgm_host->dma_rx);
>> + dev_err(dev, "can't get the RX DMA channel, error %d\n", ret);
>> + goto err;
>> + }
>> +
>> + return 0;
>> +err:
> No need for an error path if you just return the error code. BTW, I
> don't like those functions that don't cleanup behind them when an error
> happens. I know it's all handled in the dma_exit() function, but still.
okay, will return error code and also cleanup.
>> + return ret;
>> +}
>> +
>> +static int lgm_nand_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct lgm_nand_host *lgm_host;
>> + struct nand_chip *nand;
>> + phys_addr_t nandaddr_pa;
>> + struct mtd_info *mtd;
>> + struct resource *res;
>> + int ret;
>> + u32 cs;
>> +
>> + lgm_host = devm_kzalloc(dev, sizeof(*lgm_host), GFP_KERNEL);
>> + if (!lgm_host)
>> + return -ENOMEM;
>> +
>> + lgm_host->dev = dev;
>> + nand_controller_init(&lgm_host->controller);
>> +
>> + lgm_host->lgm_va =
>> + devm_platform_ioremap_resource_byname(pdev, "lgmnand");
> lgm_host->lgm_va = devm_platform_ioremap_resource_byname(pdev,
> "lgmnand");
>
>> + if (IS_ERR(lgm_host->lgm_va))
>> + return PTR_ERR(lgm_host->lgm_va);
>> +
>> + lgm_host->hsnand_va =
>> + devm_platform_ioremap_resource_byname(pdev, "hsnand");
>> + if (IS_ERR(lgm_host->hsnand_va))
>> + return PTR_ERR(lgm_host->hsnand_va);
>> +
>> + ret = device_property_read_u32(dev, "nand,cs", &cs);
>> + if (ret) {
>> + dev_err(dev, "failed to get chip select: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + lgm_host->cs = cs;
>> +
>> + lgm_host->cs_name = devm_kasprintf(dev, GFP_KERNEL, "nand_cs%d", cs);
> You don't need to keep the cs_name attached to lgm_host as it's
> automatically release. You can just get rid of the lgm_host->cs_name
> field.
Good point for me to update, Thanks!
>> + if (IS_ERR(lgm_host->cs_name)) {
>> + ret = PTR_ERR(lgm_host->cs_name);
>> + dev_err(dev, "failed to get chip select name: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + res = devm_platform_ioremap_resource_byname(pdev, lgm_host->cs_name);
>> + lgm_host->nandaddr_va = res;
>> + nandaddr_pa = res->start;
>> + if (IS_ERR(lgm_host->nandaddr_va))
>> + return PTR_ERR(lgm_host->nandaddr_va);
>> +
>> + lgm_host->clk = devm_clk_get(dev, NULL);
>> + if (IS_ERR(lgm_host->clk)) {
>> + ret = PTR_ERR(lgm_host->clk);
>> + dev_err(dev, "failed to get clock: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + ret = clk_prepare_enable(lgm_host->clk);
>> + if (ret) {
>> + dev_err(dev, "failed to enable clock: %d\n", ret);
>> + return ret;
>> + }
>> + lgm_host->clk_rate = clk_get_rate(lgm_host->clk);
>> +
>> + ret = lgm_dma_init(dev, lgm_host);
>> + if (ret)
>> + goto disable_clk;
>> +
>> + writel(lower_32_bits(nandaddr_pa) | LGM_ADDR_SEL_REGEN | LGM_ADDR_MASK,
>> + lgm_host->lgm_va + LGM_ADDR_SEL(cs));
>> +
>> + writel(LGM_BUSCON_CMULT_V4 | LGM_BUSCON_RECOVC(1) |
>> + LGM_BUSCON_HOLDC(1) | LGM_BUSCON_WAITRDC(2) |
>> + LGM_BUSCON_WAITWRC(2) | LGM_BUSCON_BCGEN_CS | LGM_BUSCON_ALEC |
>> + LGM_BUSCON_SETUP_EN, lgm_host->lgm_va + LGM_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
>> + * Refer the Intel LGM SoC datasheet.
>> + */
>> + writel(0x17400051, lgm_host->lgm_va + LGM_ADDR_SEL(0));
>> + writel(0x17C00051, lgm_host->lgm_va + LGM_ADDR_SEL(cs));
> No magic value please. I guess the 0x51 at the end encode some flags, so
> please describe those fields and come with a macro to generate the base
> range value (or a mapping table).
sure, will add MACRO instead of magic values.
>> + nand_set_flash_node(&lgm_host->chip, dev->of_node);
>> + mtd = nand_to_mtd(&lgm_host->chip);
>> + mtd->dev.parent = dev;
>> + lgm_host->dev = dev;
>> +
>> + platform_set_drvdata(pdev, lgm_host);
>> + nand_set_controller_data(&lgm_host->chip, lgm_host);
>> +
>> + nand = &lgm_host->chip;
>> + nand->controller = &lgm_host->controller;
>> + nand->controller->ops = &lgm_nand_controller_ops;
>> +
>> + /* Scan to find existence of the device */
>> + ret = nand_scan(&lgm_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(&lgm_host->chip);
>> +exit_dma:
>> + lgm_dma_exit(lgm_host);
>> +disable_clk:
>> + clk_disable_unprepare(lgm_host->clk);
>> +
>> + return ret;
>> +}
>> +
>> +static int lgm_nand_remove(struct platform_device *pdev)
>> +{
>> + struct lgm_nand_host *lgm_host = platform_get_drvdata(pdev);
>> +
>> + nand_release(&lgm_host->chip);
> Can you use mtd_device_unregister() + nand_cleanup() instead, and check
> their return value?
Sure, will use .
>> + clk_disable_unprepare(lgm_host->clk);
>> + lgm_dma_exit(lgm_host);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id lgm_nand_match[] = {
>> + { .compatible = "intel,lgm-nand", },
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(of, lgm_nand_match);
> You probably have a missing "depends on OF" in your Kconfig.
okay , Noted.
>> +
>> +static struct platform_driver lgm_nand_driver = {
>> + .probe = lgm_nand_probe,
>> + .remove = lgm_nand_remove,
>> + .driver = {
>> + .name = "intel-lgm-nand",
>> + .of_match_table = lgm_nand_match,
>> + },
>> +
>> +};
>> +module_platform_driver(lgm_nand_driver);
>> +
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_AUTHOR("Vadivel Murugan R <[email protected]>");
>> +MODULE_DESCRIPTION("Intel's LGM External Bus NAND Controller driver");
> I didn't review the DMA and ECC aspects yet, but I think you have enough
> things to address for a v3.

Thanks a lot for the review comments and valuable inputs to me for
further driver patch preparation.

Regards
Vadviel

2020-04-20 07:41:49

[permalink] [raw]

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

On Mon, 20 Apr 2020 12:18:34 +0800
"Ramuthevar, Vadivel MuruganX"
<[email protected]> wrote:

> >> diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
> >> index a80a46bb5b8b..9efc4bbaf4a3 100644
> >> --- a/drivers/mtd/nand/raw/Kconfig
> >> +++ b/drivers/mtd/nand/raw/Kconfig
> >> @@ -457,6 +457,13 @@ 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 X86
> > Do we have a hard dependency on x86 here? Maybe 'depends on HAS_MMIO'
> > would be enough.
> yes Boris, we have hard dependency on x86.

Given that the driver will also be used on a MIPS platform I would say
no :P. Just to be clear, I was suggesting to replace the soon to emerge

depends on X86 || MIPS || COMPILE_TEST
depends HAS_IOMEM

rule by

depends on OF || COMPILE_TEST
depends HAS_IOMEM

2020-04-20 08:32:41

[permalink] [raw]

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

On Fri, 17 Apr 2020 16:21:47 +0800
"Ramuthevar,Vadivel MuruganX"
<[email protected]> wrote:

> +
> + res = devm_platform_ioremap_resource_byname(pdev, lgm_host->cs_name);
> + lgm_host->nandaddr_va = res;
> + nandaddr_pa = res->start;
> + if (IS_ERR(lgm_host->nandaddr_va))
> + return PTR_ERR(lgm_host->nandaddr_va);

Hm, I didn't realize you needed the physical address for DMA transfers.
Just use platform_get_resource_by_name()+devm_ioremap_resource() in
that case.

> +
> + writel(LGM_BUSCON_CMULT_V4 | LGM_BUSCON_RECOVC(1) |
> + LGM_BUSCON_HOLDC(1) | LGM_BUSCON_WAITRDC(2) |
> + LGM_BUSCON_WAITWRC(2) | LGM_BUSCON_BCGEN_CS | LGM_BUSCON_ALEC |
> + LGM_BUSCON_SETUP_EN, lgm_host->lgm_va + LGM_BUSCON(cs));

I'm sure some the timings you hardcode here can be extracted from the
NAND timings. Can you see if you can implement ->setup_data_interface()
instead.

2020-04-20 08:53:42

[permalink] [raw]

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

On 20/4/2020 3:40 pm, Boris Brezillon wrote:

> On Mon, 20 Apr 2020 12:18:34 +0800
> "Ramuthevar, Vadivel MuruganX"
> <[email protected]> wrote:
>
>
>>>> diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
>>>> index a80a46bb5b8b..9efc4bbaf4a3 100644
>>>> --- a/drivers/mtd/nand/raw/Kconfig
>>>> +++ b/drivers/mtd/nand/raw/Kconfig
>>>> @@ -457,6 +457,13 @@ 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 X86
>>> Do we have a hard dependency on x86 here? Maybe 'depends on HAS_MMIO'
>>> would be enough.
>> yes Boris, we have hard dependency on x86.
> Given that the driver will also be used on a MIPS platform I would say
> no :P. Just to be clear, I was suggesting to replace the soon to emerge
>
> depends on X86 || MIPS || COMPILE_TEST
> depends HAS_IOMEM
>
> rule by
>
> depends on OF || COMPILE_TEST
> depends HAS_IOMEM

Thank you for the suggestion !

Yes, you are right since MIPS based SoCs also using the same driver, so
we can add the above rule.

Regards
Vadivel

2020-04-20 09:17:15

[permalink] [raw]

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

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

On 20/4/2020 4:29 pm, Boris Brezillon wrote:
> On Fri, 17 Apr 2020 16:21:47 +0800
> "Ramuthevar,Vadivel MuruganX"
> <[email protected]> wrote:
>
>> +
>> + res = devm_platform_ioremap_resource_byname(pdev, lgm_host->cs_name);
>> + lgm_host->nandaddr_va = res;
>> + nandaddr_pa = res->start;
>> + if (IS_ERR(lgm_host->nandaddr_va))
>> + return PTR_ERR(lgm_host->nandaddr_va);
> Hm, I didn't realize you needed the physical address for DMA transfers.
> Just use platform_get_resource_by_name()+devm_ioremap_resource() in
> that case.
>
Yes, you are right, needed the physical address for DMA transfers.
>> +
>> + writel(LGM_BUSCON_CMULT_V4 | LGM_BUSCON_RECOVC(1) |
>> + LGM_BUSCON_HOLDC(1) | LGM_BUSCON_WAITRDC(2) |
>> + LGM_BUSCON_WAITWRC(2) | LGM_BUSCON_BCGEN_CS | LGM_BUSCON_ALEC |
>> + LGM_BUSCON_SETUP_EN, lgm_host->lgm_va + LGM_BUSCON(cs));
> I'm sure some the timings you hardcode here can be extracted from the
> NAND timings. Can you see if you can implement ->setup_data_interface()
> instead.

Yes, I have seen few of the drivers implemented
->setup_data_interface(), Noted.

Regards
Vadivel

2020-04-20 09:21:57

[permalink] [raw]

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

On Mon, 20 Apr 2020 01:20:40 +0300
Andy Shevchenko <[email protected]> wrote:

> On Sat, Apr 18, 2020 at 10:55:33AM +0200, Boris Brezillon wrote:
> > On Fri, 17 Apr 2020 16:21:47 +0800
> > "Ramuthevar,Vadivel MuruganX"
> > <[email protected]> wrote:
> >
> > > From: Ramuthevar Vadivel Murugan <[email protected]>
>
> > > +static const struct of_device_id lgm_nand_match[] = {
> > > + { .compatible = "intel,lgm-nand", },
> > > + {}
> > > +};
> > > +MODULE_DEVICE_TABLE(of, lgm_nand_match);
> >
> > You probably have a missing "depends on OF" in your Kconfig.
>
> Since it's using device property API, dependency is not needed.
>

There's no compile-time dependency, but this driver will be pretty
useless if all its users have the NAND controller node defined in their
DT and CONFIG_OF is not enabled. I guess the OF option is selected by
arches, so explicitly depending on OF is only relevant if you change
the dependency rules as suggested in my other reply.

2020-04-20 09:48:28

[permalink] [raw]

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

On Mon, Apr 20, 2020 at 12:21 PM Boris Brezillon
<[email protected]> wrote:
> On Mon, 20 Apr 2020 01:20:40 +0300
> Andy Shevchenko <[email protected]> wrote:
> > On Sat, Apr 18, 2020 at 10:55:33AM +0200, Boris Brezillon wrote:
> > > On Fri, 17 Apr 2020 16:21:47 +0800
> > > "Ramuthevar,Vadivel MuruganX"
> > > <[email protected]> wrote:
> > >
> > > > From: Ramuthevar Vadivel Murugan <[email protected]>
> >
> > > > +static const struct of_device_id lgm_nand_match[] = {
> > > > + { .compatible = "intel,lgm-nand", },
> > > > + {}
> > > > +};
> > > > +MODULE_DEVICE_TABLE(of, lgm_nand_match);
> > >
> > > You probably have a missing "depends on OF" in your Kconfig.
> >
> > Since it's using device property API, dependency is not needed.
> >
>
> There's no compile-time dependency, but this driver will be pretty
> useless if all its users have the NAND controller node defined in their
> DT and CONFIG_OF is not enabled.

No, it's not.
See [1] for the details how ACPI may utilize this table.

[1]: https://www.kernel.org/doc/html/latest/firmware-guide/acpi/enumeration.html#device-tree-namespace-link-device-id

> I guess the OF option is selected by
> arches, so explicitly depending on OF is only relevant if you change
> the dependency rules as suggested in my other reply.

--
With Best Regards,
Andy Shevchenko

2020-04-20 09:56:01

[permalink] [raw]

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

On Mon, 20 Apr 2020 12:44:51 +0300
Andy Shevchenko <[email protected]> wrote:

> On Mon, Apr 20, 2020 at 12:21 PM Boris Brezillon
> <[email protected]> wrote:
> > On Mon, 20 Apr 2020 01:20:40 +0300
> > Andy Shevchenko <[email protected]> wrote:
> > > On Sat, Apr 18, 2020 at 10:55:33AM +0200, Boris Brezillon wrote:
> > > > On Fri, 17 Apr 2020 16:21:47 +0800
> > > > "Ramuthevar,Vadivel MuruganX"
> > > > <[email protected]> wrote:
> > > >
> > > > > From: Ramuthevar Vadivel Murugan <[email protected]>
> > >
> > > > > +static const struct of_device_id lgm_nand_match[] = {
> > > > > + { .compatible = "intel,lgm-nand", },
> > > > > + {}
> > > > > +};
> > > > > +MODULE_DEVICE_TABLE(of, lgm_nand_match);
> > > >
> > > > You probably have a missing "depends on OF" in your Kconfig.
> > >
> > > Since it's using device property API, dependency is not needed.
> > >
> >
> > There's no compile-time dependency, but this driver will be pretty
> > useless if all its users have the NAND controller node defined in their
> > DT and CONFIG_OF is not enabled.
>
> No, it's not.
> See [1] for the details how ACPI may utilize this table.
>
> [1]: https://www.kernel.org/doc/html/latest/firmware-guide/acpi/enumeration.html#device-tree-namespace-link-device-id

Except the NAND framework does use the OF lib when parsing common DT
properties (like nand-ecc-mode, etc), so it does depend on OF if you
want those props to be parsed, which, according to the DT binding patch,
is the case.

2020-04-20 10:16:26

[permalink] [raw]

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

On Mon, Apr 20, 2020 at 12:53 PM Boris Brezillon
<[email protected]> wrote:
> On Mon, 20 Apr 2020 12:44:51 +0300
> Andy Shevchenko <[email protected]> wrote:
>
> > On Mon, Apr 20, 2020 at 12:21 PM Boris Brezillon
> > <[email protected]> wrote:
> > > On Mon, 20 Apr 2020 01:20:40 +0300
> > > Andy Shevchenko <[email protected]> wrote:
> > > > On Sat, Apr 18, 2020 at 10:55:33AM +0200, Boris Brezillon wrote:
> > > > > On Fri, 17 Apr 2020 16:21:47 +0800
> > > > > "Ramuthevar,Vadivel MuruganX"
> > > > > <[email protected]> wrote:
> > > > >
> > > > > > From: Ramuthevar Vadivel Murugan <[email protected]>
> > > >
> > > > > > +static const struct of_device_id lgm_nand_match[] = {
> > > > > > + { .compatible = "intel,lgm-nand", },
> > > > > > + {}
> > > > > > +};
> > > > > > +MODULE_DEVICE_TABLE(of, lgm_nand_match);
> > > > >
> > > > > You probably have a missing "depends on OF" in your Kconfig.
> > > >
> > > > Since it's using device property API, dependency is not needed.
> > > >
> > >
> > > There's no compile-time dependency, but this driver will be pretty
> > > useless if all its users have the NAND controller node defined in their
> > > DT and CONFIG_OF is not enabled.
> >
> > No, it's not.
> > See [1] for the details how ACPI may utilize this table.
> >
> > [1]: https://www.kernel.org/doc/html/latest/firmware-guide/acpi/enumeration.html#device-tree-namespace-link-device-id
>
> Except the NAND framework does use the OF lib when parsing common DT
> properties (like nand-ecc-mode, etc), so it does depend on OF if you
> want those props to be parsed, which, according to the DT binding patch,
> is the case.

I see, so, NAND framework can be transformed at some point. In any
case, from driver perspective it's OF independent.

--
With Best Regards,
Andy Shevchenko

2020-04-20 10:31:49

[permalink] [raw]

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

On Mon, 20 Apr 2020 13:14:42 +0300
Andy Shevchenko <[email protected]> wrote:

> On Mon, Apr 20, 2020 at 12:53 PM Boris Brezillon
> <[email protected]> wrote:
> > On Mon, 20 Apr 2020 12:44:51 +0300
> > Andy Shevchenko <[email protected]> wrote:
> >
> > > On Mon, Apr 20, 2020 at 12:21 PM Boris Brezillon
> > > <[email protected]> wrote:
> > > > On Mon, 20 Apr 2020 01:20:40 +0300
> > > > Andy Shevchenko <[email protected]> wrote:
> > > > > On Sat, Apr 18, 2020 at 10:55:33AM +0200, Boris Brezillon wrote:
> > > > > > On Fri, 17 Apr 2020 16:21:47 +0800
> > > > > > "Ramuthevar,Vadivel MuruganX"
> > > > > > <[email protected]> wrote:
> > > > > >
> > > > > > > From: Ramuthevar Vadivel Murugan <[email protected]>
> > > > >
> > > > > > > +static const struct of_device_id lgm_nand_match[] = {
> > > > > > > + { .compatible = "intel,lgm-nand", },
> > > > > > > + {}
> > > > > > > +};
> > > > > > > +MODULE_DEVICE_TABLE(of, lgm_nand_match);
> > > > > >
> > > > > > You probably have a missing "depends on OF" in your Kconfig.
> > > > >
> > > > > Since it's using device property API, dependency is not needed.
> > > > >
> > > >
> > > > There's no compile-time dependency, but this driver will be pretty
> > > > useless if all its users have the NAND controller node defined in their
> > > > DT and CONFIG_OF is not enabled.
> > >
> > > No, it's not.
> > > See [1] for the details how ACPI may utilize this table.
> > >
> > > [1]: https://www.kernel.org/doc/html/latest/firmware-guide/acpi/enumeration.html#device-tree-namespace-link-device-id
> >
> > Except the NAND framework does use the OF lib when parsing common DT
> > properties (like nand-ecc-mode, etc), so it does depend on OF if you
> > want those props to be parsed, which, according to the DT binding patch,
> > is the case.
>
> I see, so, NAND framework can be transformed at some point. In any
> case, from driver perspective it's OF independent.
>

Well, it uses it only if the driver passes an OF node which this driver
does (see the nand_set_flash_node() call), so no, it's really a driver
dependency.

2020-04-20 10:45:27

[permalink] [raw]

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

On Mon, Apr 20, 2020 at 12:28:59PM +0200, Boris Brezillon wrote:
> On Mon, 20 Apr 2020 13:14:42 +0300
> Andy Shevchenko <[email protected]> wrote:
> > On Mon, Apr 20, 2020 at 12:53 PM Boris Brezillon
> > <[email protected]> wrote:
> > > On Mon, 20 Apr 2020 12:44:51 +0300
> > > Andy Shevchenko <[email protected]> wrote:
> > > > On Mon, Apr 20, 2020 at 12:21 PM Boris Brezillon
> > > > <[email protected]> wrote:
> > > > > On Mon, 20 Apr 2020 01:20:40 +0300
> > > > > Andy Shevchenko <[email protected]> wrote:
> > > > > > On Sat, Apr 18, 2020 at 10:55:33AM +0200, Boris Brezillon wrote:
> > > > > > > On Fri, 17 Apr 2020 16:21:47 +0800
> > > > > > > "Ramuthevar,Vadivel MuruganX"
> > > > > > > <[email protected]> wrote:

...

> > > > > > > > +static const struct of_device_id lgm_nand_match[] = {
> > > > > > > > + { .compatible = "intel,lgm-nand", },
> > > > > > > > + {}
> > > > > > > > +};
> > > > > > > > +MODULE_DEVICE_TABLE(of, lgm_nand_match);
> > > > > > >
> > > > > > > You probably have a missing "depends on OF" in your Kconfig.
> > > > > >
> > > > > > Since it's using device property API, dependency is not needed.
> > > > > >
> > > > >
> > > > > There's no compile-time dependency, but this driver will be pretty
> > > > > useless if all its users have the NAND controller node defined in their
> > > > > DT and CONFIG_OF is not enabled.
> > > >
> > > > No, it's not.
> > > > See [1] for the details how ACPI may utilize this table.
> > > >
> > > > [1]: https://www.kernel.org/doc/html/latest/firmware-guide/acpi/enumeration.html#device-tree-namespace-link-device-id
> > >
> > > Except the NAND framework does use the OF lib when parsing common DT
> > > properties (like nand-ecc-mode, etc), so it does depend on OF if you
> > > want those props to be parsed, which, according to the DT binding patch,
> > > is the case.
> >
> > I see, so, NAND framework can be transformed at some point. In any
> > case, from driver perspective it's OF independent.
> >
>
> Well, it uses it only if the driver passes an OF node which this driver
> does (see the nand_set_flash_node() call), so no, it's really a driver
> dependency.

Look like still it's framework dependency which driver has to rely on.
Means more work would be needed in case NAND to convert to fwnode API.

--
With Best Regards,
Andy Shevchenko

2020-04-20 11:10:45

[permalink] [raw]

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

On Mon, 20 Apr 2020 13:41:28 +0300
Andy Shevchenko <[email protected]> wrote:

> On Mon, Apr 20, 2020 at 12:28:59PM +0200, Boris Brezillon wrote:
> > On Mon, 20 Apr 2020 13:14:42 +0300
> > Andy Shevchenko <[email protected]> wrote:
> > > On Mon, Apr 20, 2020 at 12:53 PM Boris Brezillon
> > > <[email protected]> wrote:
> > > > On Mon, 20 Apr 2020 12:44:51 +0300
> > > > Andy Shevchenko <[email protected]> wrote:
> > > > > On Mon, Apr 20, 2020 at 12:21 PM Boris Brezillon
> > > > > <[email protected]> wrote:
> > > > > > On Mon, 20 Apr 2020 01:20:40 +0300
> > > > > > Andy Shevchenko <[email protected]> wrote:
> > > > > > > On Sat, Apr 18, 2020 at 10:55:33AM +0200, Boris Brezillon wrote:
> > > > > > > > On Fri, 17 Apr 2020 16:21:47 +0800
> > > > > > > > "Ramuthevar,Vadivel MuruganX"
> > > > > > > > <[email protected]> wrote:
>
> ...
>
> > > > > > > > > +static const struct of_device_id lgm_nand_match[] = {
> > > > > > > > > + { .compatible = "intel,lgm-nand", },
> > > > > > > > > + {}
> > > > > > > > > +};
> > > > > > > > > +MODULE_DEVICE_TABLE(of, lgm_nand_match);
> > > > > > > >
> > > > > > > > You probably have a missing "depends on OF" in your Kconfig.
> > > > > > >
> > > > > > > Since it's using device property API, dependency is not needed.
> > > > > > >
> > > > > >
> > > > > > There's no compile-time dependency, but this driver will be pretty
> > > > > > useless if all its users have the NAND controller node defined in their
> > > > > > DT and CONFIG_OF is not enabled.
> > > > >
> > > > > No, it's not.
> > > > > See [1] for the details how ACPI may utilize this table.
> > > > >
> > > > > [1]: https://www.kernel.org/doc/html/latest/firmware-guide/acpi/enumeration.html#device-tree-namespace-link-device-id
> > > >
> > > > Except the NAND framework does use the OF lib when parsing common DT
> > > > properties (like nand-ecc-mode, etc), so it does depend on OF if you
> > > > want those props to be parsed, which, according to the DT binding patch,
> > > > is the case.
> > >
> > > I see, so, NAND framework can be transformed at some point. In any
> > > case, from driver perspective it's OF independent.
> > >
> >
> > Well, it uses it only if the driver passes an OF node which this driver
> > does (see the nand_set_flash_node() call), so no, it's really a driver
> > dependency.
>
> Look like still it's framework dependency which driver has to rely on.
> Means more work would be needed in case NAND to convert to fwnode API.
>

Sorry, but I'm lost here. The patch series contains a DT bindings doc,
meaning that it's using a DT representation no matter where it comes
from (the fact that it might be embedded in an ACPI table doesn't
matter, right?). The framework just provides convenient DT parsing
helpers, but they are not mandatory since they are only called if the
NAND is attached a DT node (some drivers extract those info from
driver-specific pdata structs).

To me, the lack of support of a generic fwnode parsing logic in the
NAND framework is orthogonal to this "depend on OF" problem, since, no
matter what abstraction you use to parse the DT node (fwnode would just
be a wrapper around DT parsing in this specific case), the fact
remains, for this driver, in its current state, you need OF support to
make it useful.

2020-04-27 15:41:39

by Miquel Raynal

[permalink] [raw]

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

Hi Boris,

Boris Brezillon <[email protected]> wrote on Sat, 18 Apr
2020 10:55:33 +0200:

> On Fri, 17 Apr 2020 16:21:47 +0800
> "Ramuthevar,Vadivel MuruganX"
> <[email protected]> wrote:
>
> > From: Ramuthevar Vadivel Murugan <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > drivers/mtd/nand/raw/Kconfig | 7 +
> > drivers/mtd/nand/raw/Makefile | 1 +
> > drivers/mtd/nand/raw/intel_lgm_nand.c | 740 ++++++++++++++++++++++++++++++++++
>
> I wonder if we shouldn't name the driver infineon-nand-controller.c
> since the original design comes from Infineon IIUC. intel_lgm_nand.c is
> definitely misleading, as we also have a nand_intel.c file which is for
> Intel NAND chips (not NAND controllers). If we keep intel in the name,
> let's at least add a "-controller" suffix to make it clear.
>
> Side note for Miquel: I guess we would also benefit from having a clear
> core vs controller-drivers split as recently done for spi-nor (a
> controller subdir has been created).

I would even like a core vs controller drivers vs nand chips drivers.

Macronix for instance has a NAND controller driver and a NAND chip
driver, that's why, even if it is a bit long, I enforce the -nfc or
-nand-controller (my favorite) suffix now.

Thanks,
Miquèl

2020-04-27 18:32:16