All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH V4] mtd: nand: add Loongson1 NAND driver
@ 2016-11-02  1:52 Keguang Zhang
  2016-11-02  7:36 ` Boris Brezillon
  2016-11-02 11:33   ` kbuild test robot
  0 siblings, 2 replies; 6+ messages in thread
From: Keguang Zhang @ 2016-11-02  1:52 UTC (permalink / raw)
  To: linux-mtd, linux-mips, linux-kernel
  Cc: Boris Brezillon, Richard Weinberger, David Woodhouse,
	Brian Norris, Kelvin Cheung

From: Kelvin Cheung <keguang.zhang@gmail.com>

This patch adds NAND driver for Loongson1B.

Signed-off-by: Kelvin Cheung <keguang.zhang@gmail.com>

---
v4:
   Retrieve the controller from nand_hw_control.
v3:
   Replace __raw_readl/__raw_writel with readl/writel.
   Split ls1x_nand into two structures: ls1x_nand_chip and
   ls1x_nand_controller.
V2:
   Modify the dependency in Kconfig due to the changes of DMA
   module.
---
 drivers/mtd/nand/Kconfig          |   8 +
 drivers/mtd/nand/Makefile         |   1 +
 drivers/mtd/nand/loongson1_nand.c | 571 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 580 insertions(+)
 create mode 100644 drivers/mtd/nand/loongson1_nand.c

diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index 7b7a887..2a815c7 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -569,4 +569,12 @@ config MTD_NAND_MTK
 	  Enables support for NAND controller on MTK SoCs.
 	  This controller is found on mt27xx, mt81xx, mt65xx SoCs.
 
+config MTD_NAND_LOONGSON1
+	tristate "Support for Loongson1 SoC NAND controller"
+	depends on MACH_LOONGSON32
+	select DMADEVICES
+	select LOONGSON1_DMA
+	help
+		Enables support for NAND Flash on Loongson1 SoC based boards.
+
 endif # MTD_NAND
diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
index cafde6f..04f65a6 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -58,5 +58,6 @@ obj-$(CONFIG_MTD_NAND_HISI504)	        += hisi504_nand.o
 obj-$(CONFIG_MTD_NAND_BRCMNAND)		+= brcmnand/
 obj-$(CONFIG_MTD_NAND_QCOM)		+= qcom_nandc.o
 obj-$(CONFIG_MTD_NAND_MTK)		+= mtk_nand.o mtk_ecc.o
+obj-$(CONFIG_MTD_NAND_LOONGSON1)	+= loongson1_nand.o
 
 nand-objs := nand_base.o nand_bbt.o nand_timings.o
diff --git a/drivers/mtd/nand/loongson1_nand.c b/drivers/mtd/nand/loongson1_nand.c
new file mode 100644
index 0000000..e3999ea
--- /dev/null
+++ b/drivers/mtd/nand/loongson1_nand.c
@@ -0,0 +1,571 @@
+/*
+ * NAND Flash Driver for Loongson 1 SoC
+ *
+ * Copyright (C) 2015-2016 Zhang, Keguang <keguang.zhang@gmail.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+#include <linux/dmaengine.h>
+#include <linux/dma-mapping.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/nand.h>
+#include <linux/sizes.h>
+
+#include <nand.h>
+
+/* Loongson 1 NAND Register Definitions */
+#define NAND_CMD		0x0
+#define NAND_ADDRL		0x4
+#define NAND_ADDRH		0x8
+#define NAND_TIMING		0xc
+#define NAND_IDL		0x10
+#define NAND_IDH		0x14
+#define NAND_STATUS		0x14
+#define NAND_PARAM		0x18
+#define NAND_OP_NUM		0x1c
+#define NAND_CS_RDY		0x20
+
+#define NAND_DMA_ADDR		0x40
+
+/* NAND Command Register Bits */
+#define NAND_RDY		BIT(10)
+#define OP_SPARE		BIT(9)
+#define OP_MAIN			BIT(8)
+#define CMD_STATUS		BIT(7)
+#define CMD_RESET		BIT(6)
+#define CMD_READID		BIT(5)
+#define BLOCKS_ERASE		BIT(4)
+#define CMD_ERASE		BIT(3)
+#define CMD_WRITE		BIT(2)
+#define CMD_READ		BIT(1)
+#define CMD_VALID		BIT(0)
+
+#define	LS1X_NAND_TIMEOUT	20
+
+/* macros for registers read/write */
+#define nand_readl(nandc, off)		\
+	readl((nandc)->reg_base + (off))
+
+#define nand_writel(nandc, off, val)	\
+	writel((val), (nandc)->reg_base + (off))
+
+#define set_cmd(nandc, ctrl)		\
+	nand_writel(nandc, NAND_CMD, ctrl)
+
+#define start_nand(nandc)		\
+	nand_writel(nandc, NAND_CMD, nand_readl(nandc, NAND_CMD) | CMD_VALID)
+
+struct ls1x_nand_chip {
+	struct nand_chip chip;
+	struct plat_ls1x_nand *pdata;
+};
+
+struct ls1x_nand_controller {
+	struct nand_hw_control controller;
+	struct clk *clk;
+	void __iomem *reg_base;
+
+	int cmd_ctrl;
+	char datareg[8];
+	char *data_ptr;
+
+	/* DMA stuff */
+	unsigned char *dma_buf;
+	unsigned int buf_off;
+	unsigned int buf_len;
+
+	/* DMA Engine stuff */
+	unsigned int dma_chan_id;
+	struct dma_chan *dma_chan;
+	dma_cookie_t dma_cookie;
+	struct completion dma_complete;
+	void __iomem *dma_desc;
+};
+
+static inline struct ls1x_nand_chip *to_ls1x_nand_chip(struct mtd_info *mtd)
+{
+	return container_of(mtd_to_nand(mtd), struct ls1x_nand_chip, chip);
+}
+
+static struct ls1x_nand_controller *
+to_ls1x_nand_controller(struct nand_hw_control *ctrl)
+{
+	return container_of(ctrl, struct ls1x_nand_controller, controller);
+}
+
+static void dma_callback(void *data)
+{
+	struct mtd_info *mtd = (struct mtd_info *)data;
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	struct ls1x_nand_controller *nandc =
+	    to_ls1x_nand_controller(chip->controller);
+	struct dma_tx_state state;
+	enum dma_status status;
+
+	status =
+	    dmaengine_tx_status(nandc->dma_chan, nandc->dma_cookie, &state);
+	if (likely(status == DMA_COMPLETE))
+		dev_dbg(mtd->dev.parent, "DMA complete with cookie=%d\n",
+			nandc->dma_cookie);
+	else
+		dev_err(mtd->dev.parent, "DMA error with cookie=%d\n",
+			nandc->dma_cookie);
+
+	complete(&nandc->dma_complete);
+}
+
+static int setup_dma(struct mtd_info *mtd)
+{
+	struct ls1x_nand_chip *nand = to_ls1x_nand_chip(mtd);
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	struct ls1x_nand_controller *nandc =
+	    to_ls1x_nand_controller(chip->controller);
+	struct dma_slave_config cfg;
+	dma_cap_mask_t mask;
+	int ret;
+
+	if (!nand->pdata->dma_filter) {
+		dev_err(mtd->dev.parent, "no DMA filter\n");
+		return -ENOENT;
+	}
+
+	/* allocate DMA buffer */
+	nandc->dma_buf = devm_kzalloc(mtd->dev.parent,
+				      mtd->writesize + mtd->oobsize,
+				      GFP_KERNEL);
+	if (!nandc->dma_buf)
+		return -ENOMEM;
+
+	dma_cap_zero(mask);
+	dma_cap_set(DMA_SLAVE, mask);
+	nandc->dma_chan = dma_request_channel(mask, nand->pdata->dma_filter,
+					      &nandc->dma_chan_id);
+	if (!nandc->dma_chan) {
+		dev_err(mtd->dev.parent, "failed to request DMA channel\n");
+		return -EBUSY;
+	}
+	dev_info(mtd->dev.parent, "got %s for %s access\n",
+		 dma_chan_name(nandc->dma_chan), dev_name(mtd->dev.parent));
+
+	cfg.src_addr = CPHYSADDR(nandc->reg_base + NAND_DMA_ADDR);
+	cfg.dst_addr = CPHYSADDR(nandc->reg_base + NAND_DMA_ADDR);
+	cfg.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
+	cfg.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
+	ret = dmaengine_slave_config(nandc->dma_chan, &cfg);
+	if (ret) {
+		dev_err(mtd->dev.parent, "failed to config DMA channel\n");
+		dma_release_channel(nandc->dma_chan);
+		return ret;
+	}
+
+	init_completion(&nandc->dma_complete);
+
+	return 0;
+}
+
+static int start_dma(struct mtd_info *mtd, unsigned int len, bool is_write)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	struct ls1x_nand_controller *nandc =
+	    to_ls1x_nand_controller(chip->controller);
+	struct dma_chan *chan = nandc->dma_chan;
+	struct dma_async_tx_descriptor *desc;
+	enum dma_data_direction data_dir =
+	    is_write ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
+	enum dma_transfer_direction xfer_dir =
+	    is_write ? DMA_MEM_TO_DEV : DMA_DEV_TO_MEM;
+	dma_addr_t dma_addr;
+	int ret;
+
+	dma_addr =
+	    dma_map_single(chan->device->dev, nandc->dma_buf, len, data_dir);
+	if (dma_mapping_error(chan->device->dev, dma_addr)) {
+		dev_err(mtd->dev.parent, "failed to map DMA buffer\n");
+		return -ENXIO;
+	}
+
+	desc = dmaengine_prep_slave_single(chan, dma_addr, len, xfer_dir,
+					   DMA_PREP_INTERRUPT);
+	if (!desc) {
+		dev_err(mtd->dev.parent, "failed to prepare DMA descriptor\n");
+		ret = PTR_ERR(desc);
+		goto err;
+	}
+	desc->callback = dma_callback;
+	desc->callback_param = mtd;
+
+	nandc->dma_cookie = dmaengine_submit(desc);
+	ret = dma_submit_error(nandc->dma_cookie);
+	if (ret) {
+		dev_err(mtd->dev.parent, "failed to submit DMA descriptor\n");
+		goto err;
+	}
+
+	dev_dbg(mtd->dev.parent, "issue DMA with cookie=%d\n",
+		nandc->dma_cookie);
+	dma_async_issue_pending(chan);
+
+	ret = wait_for_completion_timeout(&nandc->dma_complete,
+					  msecs_to_jiffies(LS1X_NAND_TIMEOUT));
+	if (ret <= 0) {
+		dev_err(mtd->dev.parent, "DMA timeout\n");
+		dmaengine_terminate_all(chan);
+		ret = -EIO;
+	}
+	ret = 0;
+err:
+	dma_unmap_single(chan->device->dev, dma_addr, len, data_dir);
+
+	return ret;
+}
+
+static void ls1x_nand_select_chip(struct mtd_info *mtd, int chip)
+{
+}
+
+static int ls1x_nand_dev_ready(struct mtd_info *mtd)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	struct ls1x_nand_controller *nandc =
+	    to_ls1x_nand_controller(chip->controller);
+
+	if (nand_readl(nandc, NAND_CMD) & NAND_RDY)
+		return 1;
+
+	return 0;
+}
+
+static uint8_t ls1x_nand_read_byte(struct mtd_info *mtd)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	struct ls1x_nand_controller *nandc =
+	    to_ls1x_nand_controller(chip->controller);
+
+	return *(nandc->data_ptr++);
+}
+
+static void ls1x_nand_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	struct ls1x_nand_controller *nandc =
+	    to_ls1x_nand_controller(chip->controller);
+
+	memcpy(buf, nandc->dma_buf + nandc->buf_off, len);
+	nandc->buf_off += len;
+}
+
+static void ls1x_nand_write_buf(struct mtd_info *mtd, const uint8_t *buf,
+				int len)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	struct ls1x_nand_controller *nandc =
+	    to_ls1x_nand_controller(chip->controller);
+
+	memcpy(nandc->dma_buf + nandc->buf_off, buf, len);
+	nandc->buf_off += len;
+}
+
+static inline void set_addr_len(struct mtd_info *mtd, unsigned int command,
+				int column, int page_addr)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	struct ls1x_nand_controller *nandc =
+	    to_ls1x_nand_controller(chip->controller);
+	int page_shift, addr_low, addr_high;
+
+	if (command == NAND_CMD_ERASE1)
+		page_shift = chip->page_shift;
+	else
+		page_shift = chip->page_shift + 1;
+
+	addr_low = page_addr << page_shift;
+
+	if (column != -1) {
+		if (command == NAND_CMD_READOOB)
+			column += mtd->writesize;
+		addr_low += column;
+		nandc->buf_off = 0;
+	}
+
+	addr_high =
+	    page_addr >> (sizeof(page_addr) * BITS_PER_BYTE - page_shift);
+
+	if (command == NAND_CMD_ERASE1)
+		nandc->buf_len = 1;
+	else
+		nandc->buf_len = mtd->writesize + mtd->oobsize - column;
+
+	nand_writel(nandc, NAND_ADDRL, addr_low);
+	nand_writel(nandc, NAND_ADDRH, addr_high);
+	nand_writel(nandc, NAND_OP_NUM, nandc->buf_len);
+}
+
+static void ls1x_nand_cmdfunc(struct mtd_info *mtd, unsigned int command,
+			      int column, int page_addr)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	struct ls1x_nand_controller *nandc =
+	    to_ls1x_nand_controller(chip->controller);
+
+	dev_dbg(mtd->dev.parent, "cmd = 0x%02x, col = 0x%08x, page = 0x%08x\n",
+		command, column, page_addr);
+
+	if (command == NAND_CMD_RNDOUT) {
+		nandc->buf_off = column;
+		return;
+	}
+
+	/*set address, buffer length and buffer offset */
+	if (column != -1 || page_addr != -1)
+		set_addr_len(mtd, command, column, page_addr);
+
+	/*prepare NAND command */
+	switch (command) {
+	case NAND_CMD_RESET:
+		nandc->cmd_ctrl = CMD_RESET;
+		break;
+	case NAND_CMD_STATUS:
+		nandc->cmd_ctrl = CMD_STATUS;
+		break;
+	case NAND_CMD_READID:
+		nandc->cmd_ctrl = CMD_READID;
+		break;
+	case NAND_CMD_READ0:
+		nandc->cmd_ctrl = OP_SPARE | OP_MAIN | CMD_READ;
+		break;
+	case NAND_CMD_READOOB:
+		nandc->cmd_ctrl = OP_SPARE | CMD_READ;
+		break;
+	case NAND_CMD_ERASE1:
+		nandc->cmd_ctrl = CMD_ERASE;
+		break;
+	case NAND_CMD_PAGEPROG:
+		break;
+	case NAND_CMD_SEQIN:
+		if (column < mtd->writesize)
+			nandc->cmd_ctrl = OP_SPARE | OP_MAIN | CMD_WRITE;
+		else
+			nandc->cmd_ctrl = OP_SPARE | CMD_WRITE;
+	default:
+		return;
+	}
+
+	/*set NAND command */
+	set_cmd(nandc, nandc->cmd_ctrl);
+	/*trigger NAND operation */
+	start_nand(nandc);
+	/*trigger DMA for R/W operation */
+	if (command == NAND_CMD_READ0 || command == NAND_CMD_READOOB)
+		start_dma(mtd, nandc->buf_len, false);
+	else if (command == NAND_CMD_PAGEPROG)
+		start_dma(mtd, nandc->buf_len, true);
+	nand_wait_ready(mtd);
+
+	if (command == NAND_CMD_STATUS) {
+		nandc->datareg[0] = (char)(nand_readl(nandc, NAND_STATUS) >> 8);
+		/*work around hardware bug for invalid STATUS */
+		nandc->datareg[0] |= 0xc0;
+		nandc->data_ptr = nandc->datareg;
+	} else if (command == NAND_CMD_READID) {
+		nandc->datareg[0] = (char)(nand_readl(nandc, NAND_IDH));
+		nandc->datareg[1] = (char)(nand_readl(nandc, NAND_IDL) >> 24);
+		nandc->datareg[2] = (char)(nand_readl(nandc, NAND_IDL) >> 16);
+		nandc->datareg[3] = (char)(nand_readl(nandc, NAND_IDL) >> 8);
+		nandc->datareg[4] = (char)(nand_readl(nandc, NAND_IDL));
+		nandc->data_ptr = nandc->datareg;
+	}
+
+	nandc->cmd_ctrl = 0;
+}
+
+static void ls1x_nand_hw_init(struct mtd_info *mtd, int hold_cycle,
+			      int wait_cycle)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	struct ls1x_nand_controller *nandc =
+	    to_ls1x_nand_controller(chip->controller);
+	int chipsize = (int)(chip->chipsize >> 20);
+	int cell_size = 0x0;
+
+	switch (chipsize) {
+	case SZ_128:		/*128M */
+		cell_size = 0x0;
+		break;
+	case SZ_256:		/*256M */
+		cell_size = 0x1;
+		break;
+	case SZ_512:		/*512M */
+		cell_size = 0x2;
+		break;
+	case SZ_1K:		/*1G */
+		cell_size = 0x3;
+		break;
+	case SZ_2K:		/*2G */
+		cell_size = 0x4;
+		break;
+	case SZ_4K:		/*4G */
+		cell_size = 0x5;
+		break;
+	case SZ_8K:		/*8G */
+		cell_size = 0x6;
+		break;
+	case SZ_16K:		/*16G */
+		cell_size = 0x7;
+		break;
+	default:
+		dev_warn(mtd->dev.parent, "unsupported chip size: %d MB\n",
+			 chipsize);
+	}
+
+	nand_writel(nandc, NAND_TIMING, (hold_cycle << 8) | wait_cycle);
+	nand_writel(nandc, NAND_PARAM,
+		    (nand_readl(nandc, NAND_PARAM) & 0xfffff0ff) | (cell_size <<
+								    8));
+}
+
+static int ls1x_nand_init(struct platform_device *pdev,
+			  struct ls1x_nand_controller *nandc)
+{
+	struct device *dev = &pdev->dev;
+	struct ls1x_nand_chip *nand;
+	struct plat_ls1x_nand *pdata;
+	struct nand_chip *chip;
+	struct mtd_info *mtd;
+	int ret = 0;
+
+	nand = devm_kzalloc(dev, sizeof(*nand), GFP_KERNEL);
+	if (!nand)
+		return -ENOMEM;
+
+	pdata = dev_get_platdata(dev);
+	if (!pdata) {
+		dev_err(dev, "platform data missing\n");
+		return -EINVAL;
+	}
+	nand->pdata = pdata;
+
+	chip = &nand->chip;
+	chip->read_byte		= ls1x_nand_read_byte;
+	chip->read_buf		= ls1x_nand_read_buf;
+	chip->write_buf		= ls1x_nand_write_buf;
+	chip->select_chip	= ls1x_nand_select_chip;
+	chip->dev_ready		= ls1x_nand_dev_ready;
+	chip->cmdfunc		= ls1x_nand_cmdfunc;
+	chip->options		= NAND_NO_SUBPAGE_WRITE;
+	chip->controller	= &nandc->controller;
+	chip->ecc.mode		= NAND_ECC_SOFT;
+	chip->ecc.algo		= NAND_ECC_HAMMING;
+
+	mtd = nand_to_mtd(chip);
+	mtd->name = "ls1x-nand";
+	mtd->owner = THIS_MODULE;
+	mtd->dev.parent = dev;
+
+	ret = nand_scan_ident(mtd, 1, NULL);
+	if (ret)
+		return ret;
+
+	ls1x_nand_hw_init(mtd, pdata->hold_cycle, pdata->wait_cycle);
+
+	ret = setup_dma(mtd);
+	if (ret)
+		return ret;
+
+	ret = nand_scan_tail(mtd);
+	if (ret) {
+		dma_release_channel(nandc->dma_chan);
+		return ret;
+	}
+
+	ret = mtd_device_register(mtd, pdata->parts, pdata->nr_parts);
+	if (ret) {
+		dev_err(dev, "failed to register MTD device: %d\n", ret);
+		dma_release_channel(nandc->dma_chan);
+	}
+
+	platform_set_drvdata(pdev, mtd);
+	return ret;
+}
+
+static int ls1x_nand_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct ls1x_nand_controller *nandc;
+	struct resource *res;
+	int ret;
+
+	nandc = devm_kzalloc(dev, sizeof(*nandc), GFP_KERNEL);
+	if (!nandc)
+		return -ENOMEM;
+	nand_hw_control_init(&nandc->controller);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(dev, "failed to get I/O memory\n");
+		return -ENXIO;
+	}
+
+	nandc->reg_base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(nandc->reg_base))
+		return PTR_ERR(nandc->reg_base);
+
+	res = platform_get_resource(pdev, IORESOURCE_DMA, 0);
+	if (!res) {
+		dev_err(dev, "failed to get DMA information\n");
+		return -ENXIO;
+	}
+	nandc->dma_chan_id = res->start;
+
+	nandc->clk = devm_clk_get(dev, pdev->name);
+	if (IS_ERR(nandc->clk)) {
+		dev_err(dev, "failed to get %s clock\n", pdev->name);
+		return PTR_ERR(nandc->clk);
+	}
+	clk_prepare_enable(nandc->clk);
+
+	ret = ls1x_nand_init(pdev, nandc);
+	if (ret) {
+		clk_disable_unprepare(nandc->clk);
+		return ret;
+	}
+
+	dev_info(dev, "Loongson1 NAND driver registered\n");
+	return 0;
+}
+
+static int ls1x_nand_remove(struct platform_device *pdev)
+{
+	struct mtd_info *mtd = platform_get_drvdata(pdev);
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	struct ls1x_nand_controller *nandc =
+	    to_ls1x_nand_controller(chip->controller);
+
+	if (nandc->dma_chan)
+		dma_release_channel(nandc->dma_chan);
+	nand_release(mtd);
+	clk_disable_unprepare(nandc->clk);
+
+	return 0;
+}
+
+static struct platform_driver ls1x_nand_driver = {
+	.probe	= ls1x_nand_probe,
+	.remove	= ls1x_nand_remove,
+	.driver	= {
+		.name	= "ls1x-nand",
+		.owner	= THIS_MODULE,
+	},
+};
+
+module_platform_driver(ls1x_nand_driver);
+
+MODULE_AUTHOR("Kelvin Cheung <keguang.zhang@gmail.com>");
+MODULE_DESCRIPTION("Loongson1 NAND Flash driver");
+MODULE_LICENSE("GPL");
-- 
2.7.3

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH V4] mtd: nand: add Loongson1 NAND driver
  2016-11-02  1:52 [PATCH V4] mtd: nand: add Loongson1 NAND driver Keguang Zhang
@ 2016-11-02  7:36 ` Boris Brezillon
       [not found]   ` <CAJhJPsWcJiPjUfGmsHn7v2ODgj-0tb5XfqfyJVyr0gsbRxYPZA@mail.gmail.com>
  2016-11-02 11:33   ` kbuild test robot
  1 sibling, 1 reply; 6+ messages in thread
From: Boris Brezillon @ 2016-11-02  7:36 UTC (permalink / raw)
  To: Keguang Zhang
  Cc: linux-mtd, linux-mips, linux-kernel, Brian Norris,
	David Woodhouse, Richard Weinberger

On Wed,  2 Nov 2016 09:52:06 +0800
Keguang Zhang <keguang.zhang@gmail.com> wrote:

> From: Kelvin Cheung <keguang.zhang@gmail.com>
> 
> This patch adds NAND driver for Loongson1B.
> 
> Signed-off-by: Kelvin Cheung <keguang.zhang@gmail.com>
> 
> ---
> v4:
>    Retrieve the controller from nand_hw_control.
> v3:
>    Replace __raw_readl/__raw_writel with readl/writel.
>    Split ls1x_nand into two structures: ls1x_nand_chip and
>    ls1x_nand_controller.
> V2:
>    Modify the dependency in Kconfig due to the changes of DMA
>    module.
> ---
>  drivers/mtd/nand/Kconfig          |   8 +
>  drivers/mtd/nand/Makefile         |   1 +
>  drivers/mtd/nand/loongson1_nand.c | 571 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 580 insertions(+)
>  create mode 100644 drivers/mtd/nand/loongson1_nand.c
> 

[...]

> +static void ls1x_nand_cmdfunc(struct mtd_info *mtd, unsigned int command,
> +			      int column, int page_addr)
> +{
> +	struct nand_chip *chip = mtd_to_nand(mtd);
> +	struct ls1x_nand_controller *nandc =
> +	    to_ls1x_nand_controller(chip->controller);
> +
> +	dev_dbg(mtd->dev.parent, "cmd = 0x%02x, col = 0x%08x, page = 0x%08x\n",
> +		command, column, page_addr);
> +
> +	if (command == NAND_CMD_RNDOUT) {
> +		nandc->buf_off = column;
> +		return;
> +	}
> +
> +	/*set address, buffer length and buffer offset */
> +	if (column != -1 || page_addr != -1)
> +		set_addr_len(mtd, command, column, page_addr);
> +
> +	/*prepare NAND command */
> +	switch (command) {
> +	case NAND_CMD_RESET:
> +		nandc->cmd_ctrl = CMD_RESET;
> +		break;
> +	case NAND_CMD_STATUS:
> +		nandc->cmd_ctrl = CMD_STATUS;
> +		break;
> +	case NAND_CMD_READID:
> +		nandc->cmd_ctrl = CMD_READID;
> +		break;
> +	case NAND_CMD_READ0:
> +		nandc->cmd_ctrl = OP_SPARE | OP_MAIN | CMD_READ;
> +		break;
> +	case NAND_CMD_READOOB:
> +		nandc->cmd_ctrl = OP_SPARE | CMD_READ;
> +		break;
> +	case NAND_CMD_ERASE1:
> +		nandc->cmd_ctrl = CMD_ERASE;
> +		break;
> +	case NAND_CMD_PAGEPROG:
> +		break;
> +	case NAND_CMD_SEQIN:
> +		if (column < mtd->writesize)
> +			nandc->cmd_ctrl = OP_SPARE | OP_MAIN | CMD_WRITE;
> +		else
> +			nandc->cmd_ctrl = OP_SPARE | CMD_WRITE;
> +	default:
> +		return;
> +	}

Sorry, but I'm still convinced this driver does not fit in the NAND
subsystem, or at least not the one we have now.

You're just supporting a limited number of the commands a raw NAND can
support (for example, no SET_FEATURE support, which is required for
ONFI NANDs), and your ->cmdfunc() implementation clearly shows that
you're converting all low-level commands into high-level ones.

Can you try to implement the mtd_info hooks directly? This only things
you'll miss are the nand_ids table and the BBT code, but I guess this is
something we can expose so that you don't have to re-use it.

Of course, you'll also have to convert absolute addresses into
page/block numbers, but that's not the hard part of the job.

You may also want to have a look at [1]. I started to abstract away the
NAND interface type to share some code between SPI NANDs and raw NANDs.
By implementing this interface, you'll at least be able to re-use the
BBT code.

I'm really sorry to ask you that now, after you've reworked most of the
driver to address my comments, but the more I look at it the more I
feel it's just a big hack to make it fit in a framework that was not
designed to support such "high-level" controllers.

Regards,

Boris

> +
> +	/*set NAND command */
> +	set_cmd(nandc, nandc->cmd_ctrl);
> +	/*trigger NAND operation */
> +	start_nand(nandc);
> +	/*trigger DMA for R/W operation */
> +	if (command == NAND_CMD_READ0 || command == NAND_CMD_READOOB)
> +		start_dma(mtd, nandc->buf_len, false);
> +	else if (command == NAND_CMD_PAGEPROG)
> +		start_dma(mtd, nandc->buf_len, true);
> +	nand_wait_ready(mtd);
> +
> +	if (command == NAND_CMD_STATUS) {
> +		nandc->datareg[0] = (char)(nand_readl(nandc, NAND_STATUS) >> 8);
> +		/*work around hardware bug for invalid STATUS */
> +		nandc->datareg[0] |= 0xc0;
> +		nandc->data_ptr = nandc->datareg;
> +	} else if (command == NAND_CMD_READID) {
> +		nandc->datareg[0] = (char)(nand_readl(nandc, NAND_IDH));
> +		nandc->datareg[1] = (char)(nand_readl(nandc, NAND_IDL) >> 24);
> +		nandc->datareg[2] = (char)(nand_readl(nandc, NAND_IDL) >> 16);
> +		nandc->datareg[3] = (char)(nand_readl(nandc, NAND_IDL) >> 8);
> +		nandc->datareg[4] = (char)(nand_readl(nandc, NAND_IDL));
> +		nandc->data_ptr = nandc->datareg;
> +	}
> +
> +	nandc->cmd_ctrl = 0;
> +}

[1]http://lkml.iu.edu/hypermail/linux/kernel/1610.2/00066.html

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH V4] mtd: nand: add Loongson1 NAND driver
@ 2016-11-02 11:33   ` kbuild test robot
  0 siblings, 0 replies; 6+ messages in thread
From: kbuild test robot @ 2016-11-02 11:33 UTC (permalink / raw)
  To: Keguang Zhang
  Cc: kbuild-all, linux-mtd, linux-mips, linux-kernel, Boris Brezillon,
	Richard Weinberger, David Woodhouse, Brian Norris, Kelvin Cheung

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

Hi Kelvin,

[auto build test ERROR on mtd/master]
[also build test ERROR on v4.9-rc3 next-20161028]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Keguang-Zhang/mtd-nand-add-Loongson1-NAND-driver/20161102-095605
base:   git://git.infradead.org/linux-mtd.git master
config: mips-loongson1c_defconfig (attached as .config)
compiler: mipsel-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=mips 

All errors (new ones prefixed by >>):

   drivers/mtd/nand/loongson1_nand.c: In function 'setup_dma':
>> drivers/mtd/nand/loongson1_nand.c:134:18: error: 'struct plat_ls1x_nand' has no member named 'dma_filter'
     if (!nand->pdata->dma_filter) {
                     ^~
   In file included from drivers/mtd/nand/loongson1_nand.c:15:0:
   drivers/mtd/nand/loongson1_nand.c:148:57: error: 'struct plat_ls1x_nand' has no member named 'dma_filter'
     nandc->dma_chan = dma_request_channel(mask, nand->pdata->dma_filter,
                                                            ^
   include/linux/dmaengine.h:1398:72: note: in definition of macro 'dma_request_channel'
    #define dma_request_channel(mask, x, y) __dma_request_channel(&(mask), x, y)
                                                                           ^

vim +134 drivers/mtd/nand/loongson1_nand.c

   128		struct ls1x_nand_controller *nandc =
   129		    to_ls1x_nand_controller(chip->controller);
   130		struct dma_slave_config cfg;
   131		dma_cap_mask_t mask;
   132		int ret;
   133	
 > 134		if (!nand->pdata->dma_filter) {
   135			dev_err(mtd->dev.parent, "no DMA filter\n");
   136			return -ENOENT;
   137		}

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 12631 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH V4] mtd: nand: add Loongson1 NAND driver
@ 2016-11-02 11:33   ` kbuild test robot
  0 siblings, 0 replies; 6+ messages in thread
From: kbuild test robot @ 2016-11-02 11:33 UTC (permalink / raw)
  To: Keguang Zhang
  Cc: kbuild-all, linux-mtd, linux-mips, linux-kernel, Boris Brezillon,
	Richard Weinberger, David Woodhouse, Brian Norris

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

Hi Kelvin,

[auto build test ERROR on mtd/master]
[also build test ERROR on v4.9-rc3 next-20161028]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Keguang-Zhang/mtd-nand-add-Loongson1-NAND-driver/20161102-095605
base:   git://git.infradead.org/linux-mtd.git master
config: mips-loongson1c_defconfig (attached as .config)
compiler: mipsel-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=mips 

All errors (new ones prefixed by >>):

   drivers/mtd/nand/loongson1_nand.c: In function 'setup_dma':
>> drivers/mtd/nand/loongson1_nand.c:134:18: error: 'struct plat_ls1x_nand' has no member named 'dma_filter'
     if (!nand->pdata->dma_filter) {
                     ^~
   In file included from drivers/mtd/nand/loongson1_nand.c:15:0:
   drivers/mtd/nand/loongson1_nand.c:148:57: error: 'struct plat_ls1x_nand' has no member named 'dma_filter'
     nandc->dma_chan = dma_request_channel(mask, nand->pdata->dma_filter,
                                                            ^
   include/linux/dmaengine.h:1398:72: note: in definition of macro 'dma_request_channel'
    #define dma_request_channel(mask, x, y) __dma_request_channel(&(mask), x, y)
                                                                           ^

vim +134 drivers/mtd/nand/loongson1_nand.c

   128		struct ls1x_nand_controller *nandc =
   129		    to_ls1x_nand_controller(chip->controller);
   130		struct dma_slave_config cfg;
   131		dma_cap_mask_t mask;
   132		int ret;
   133	
 > 134		if (!nand->pdata->dma_filter) {
   135			dev_err(mtd->dev.parent, "no DMA filter\n");
   136			return -ENOENT;
   137		}

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 12631 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH V4] mtd: nand: add Loongson1 NAND driver
       [not found]           ` <CAJhJPsWZ77a33rhjfB0MagZuvJzAMFOtf2qi1HYy-1+5gd7v1w@mail.gmail.com>
@ 2016-11-06 18:03             ` Boris Brezillon
  2016-11-07  9:17               ` Peter Pan
  0 siblings, 1 reply; 6+ messages in thread
From: Boris Brezillon @ 2016-11-06 18:03 UTC (permalink / raw)
  To: Kelvin Cheung; +Cc: linux-mtd, Peter Pan

+Peter

Hi Kelvin,

On Thu, 3 Nov 2016 19:09:55 +0800
Kelvin Cheung <keguang.zhang@gmail.com> wrote:

> >  
> > > I doubt whether the MTD maintainer will be happy with this kind of MTD
> > > driver.
> > > After all, it is a NAND controller driver.  
> >
> > Yes, it is a raw NAND controller driver, but one with a limited set of
> > features.
> > Letting the core think that this is a regular raw NAND controller
> > driver is just wrong, and as I said, I want to stop this insanity.
> > The nand_chip interface has been abused for too long, and it's one of
> > the reason the NAND subsystem has become a pile of hardly maintainable
> > hacks preventing support for advanced features in a generic way.
> >
> > There are probably a few things that could be re-used (like the SW ECC
> > implementation), and that's the reason for the "interface-agnostic NAND
> > framework" series I pointed to in my previous answer. So yes, there are
> > better solutions than developing an MTD driver
> >
> > I got it now.  
> The Loongson1 is classified as interface-agnostic NAND, while others are
> raw NANDs.

Well, it's not really the case, since this controller is really
interfacing with raw NANDs.
The interface-agnostic framework is here to share NAND device
functionalities (like BBT handling, soft ECC handling, ...) no matter
the I/O interface.
But it appears that your driver would be more easily implemented using
this interface, and more importantly, it would not let the raw NAND
framework think this controller is able to send any kind of raw NAND
commands, which is the point I'm complaining about. 

> 
> 
> > >  
> > > >  
> > > > >
> > > > >  
> > > > > > You may also want to have a look at [1]. I started to abstract  
> > away the  
> > > > > > NAND interface type to share some code between SPI NANDs and raw  
> > NANDs.  
> > > > > > By implementing this interface, you'll at least be able to re-use  
> > the  
> > > > > > BBT code.
> > > > > >
> > > > > > I'm really sorry to ask you that now, after you've reworked most  
> > of the  
> > > > > > driver to address my comments, but the more I look at it the more I
> > > > > > feel it's just a big hack to make it fit in a framework that was  
> > not  
> > > > > > designed to support such "high-level" controllers.
> > > > > >  
> > > > >
> > > > > Since the driver of "high-level" controllers is still ongoing.
> > > > > Could you please accept this patch for now?  
> > > >
> > > > Sorry, but no. I'm okay helping you improve/develop what you need to
> > > > support this controller, but I don't want to accept more hacky drivers
> > > > that do not fit in the current NAND framework, and yours is falling in
> > > > this case.
> > > >  
> > >
> > > Is there anything left I can do with my patch?  
> >
> > There are several things you could try:
> >
> > 1/ Try to extend interface-agnostic framework to expose high level
> >    functionality like
> >    - reading/writing one of several pages
> >    - erasing a block (already done)
> >    - making SW ECC implementation generic enough to get rid of the
> >      nand_chip specific aspects so that it can be re-used for SPI NANDs
> >      or high-level NAND controllers (AFAICT, this shouldn't be to hard,
> >      the SW implementations are already providing generic functions and
> >      implementing nand_chip specific wrappers)
> >    Once you have such an interface, you can start implementing generic
> >    mtd_info hooks at the interface-agnostic NAND level.
> >
> > 2/ Try to directly implement the mtd_info hooks I am mentioning above.
> >
> > Of course I'd prefer solution 1 since it would benefit to everybody. I
> > also understand you need to have access to the nand_ids table. But this
> > should be rather simple, since it's completely independent from the
> > nand_chip interface.
> >  
> 
> OK, I will try solution 1.

Great!

> 
> >  
> > > BTW, what is status of "high-level" controllers drivers?  
> >
> > Since no one worked on it, there's not much done here. But I really
> > think it could be based on the interface-agnostic framework I started.
> >  
> 
> I'd like to know when the patch set of interface-agnostic framework will be
> merged,
> Then, I can try solution 1.

Actually, I started this work to help Peter support SPI NANDs without
duplicating the BBT code, and I was expecting him to review/test this
implementation, but I never hear back from him.

I really don't want to merge these changes until someone really starts
using/testing it. But I can provide a branch containing those patches
if you need.

Regards,

Boris

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH V4] mtd: nand: add Loongson1 NAND driver
  2016-11-06 18:03             ` Boris Brezillon
@ 2016-11-07  9:17               ` Peter Pan
  0 siblings, 0 replies; 6+ messages in thread
From: Peter Pan @ 2016-11-07  9:17 UTC (permalink / raw)
  To: Boris Brezillon; +Cc: Kelvin Cheung, linux-mtd, peterpandong

Hi Boris,

First of all, I'm sorry for reply your mail late. I was quite busy with
other things for several month and somehow I missed your v2 patches...


On Mon, Nov 7, 2016 at 2:03 AM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> +Peter
>
> Hi Kelvin,
>
> On Thu, 3 Nov 2016 19:09:55 +0800
> Kelvin Cheung <keguang.zhang@gmail.com> wrote:
>
>> >
>> > > I doubt whether the MTD maintainer will be happy with this kind of MTD
>> > > driver.
>> > > After all, it is a NAND controller driver.
>> >
>> > Yes, it is a raw NAND controller driver, but one with a limited set of
>> > features.
>> > Letting the core think that this is a regular raw NAND controller
>> > driver is just wrong, and as I said, I want to stop this insanity.
>> > The nand_chip interface has been abused for too long, and it's one of
>> > the reason the NAND subsystem has become a pile of hardly maintainable
>> > hacks preventing support for advanced features in a generic way.
>> >
>> > There are probably a few things that could be re-used (like the SW ECC
>> > implementation), and that's the reason for the "interface-agnostic NAND
>> > framework" series I pointed to in my previous answer. So yes, there are
>> > better solutions than developing an MTD driver
>> >
>> > I got it now.
>> The Loongson1 is classified as interface-agnostic NAND, while others are
>> raw NANDs.
>
> Well, it's not really the case, since this controller is really
> interfacing with raw NANDs.
> The interface-agnostic framework is here to share NAND device
> functionalities (like BBT handling, soft ECC handling, ...) no matter
> the I/O interface.
> But it appears that your driver would be more easily implemented using
> this interface, and more importantly, it would not let the raw NAND
> framework think this controller is able to send any kind of raw NAND
> commands, which is the point I'm complaining about.
>
>>
>>
>> > >
>> > > >
>> > > > >
>> > > > >
>> > > > > > You may also want to have a look at [1]. I started to abstract
>> > away the
>> > > > > > NAND interface type to share some code between SPI NANDs and raw
>> > NANDs.
>> > > > > > By implementing this interface, you'll at least be able to re-use
>> > the
>> > > > > > BBT code.
>> > > > > >
>> > > > > > I'm really sorry to ask you that now, after you've reworked most
>> > of the
>> > > > > > driver to address my comments, but the more I look at it the more I
>> > > > > > feel it's just a big hack to make it fit in a framework that was
>> > not
>> > > > > > designed to support such "high-level" controllers.
>> > > > > >
>> > > > >
>> > > > > Since the driver of "high-level" controllers is still ongoing.
>> > > > > Could you please accept this patch for now?
>> > > >
>> > > > Sorry, but no. I'm okay helping you improve/develop what you need to
>> > > > support this controller, but I don't want to accept more hacky drivers
>> > > > that do not fit in the current NAND framework, and yours is falling in
>> > > > this case.
>> > > >
>> > >
>> > > Is there anything left I can do with my patch?
>> >
>> > There are several things you could try:
>> >
>> > 1/ Try to extend interface-agnostic framework to expose high level
>> >    functionality like
>> >    - reading/writing one of several pages
>> >    - erasing a block (already done)
>> >    - making SW ECC implementation generic enough to get rid of the
>> >      nand_chip specific aspects so that it can be re-used for SPI NANDs
>> >      or high-level NAND controllers (AFAICT, this shouldn't be to hard,
>> >      the SW implementations are already providing generic functions and
>> >      implementing nand_chip specific wrappers)
>> >    Once you have such an interface, you can start implementing generic
>> >    mtd_info hooks at the interface-agnostic NAND level.
>> >
>> > 2/ Try to directly implement the mtd_info hooks I am mentioning above.
>> >
>> > Of course I'd prefer solution 1 since it would benefit to everybody. I
>> > also understand you need to have access to the nand_ids table. But this
>> > should be rather simple, since it's completely independent from the
>> > nand_chip interface.
>> >
>>
>> OK, I will try solution 1.
>
> Great!
>
>>
>> >
>> > > BTW, what is status of "high-level" controllers drivers?
>> >
>> > Since no one worked on it, there's not much done here. But I really
>> > think it could be based on the interface-agnostic framework I started.
>> >
>>
>> I'd like to know when the patch set of interface-agnostic framework will be
>> merged,
>> Then, I can try solution 1.
>
> Actually, I started this work to help Peter support SPI NANDs without
> duplicating the BBT code, and I was expecting him to review/test this
> implementation, but I never hear back from him.

Although my loading is still high, I will try to finish your v2 patch
review and testing
in two weeks. For my SPI NAND patchset, I'll rebase it on your v2
patch, split it to
patch series and then send it out. But it will take some time.

Sorry for the late reply again.

Thanks,
Peter Pan

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-11-07  9:18 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-02  1:52 [PATCH V4] mtd: nand: add Loongson1 NAND driver Keguang Zhang
2016-11-02  7:36 ` Boris Brezillon
     [not found]   ` <CAJhJPsWcJiPjUfGmsHn7v2ODgj-0tb5XfqfyJVyr0gsbRxYPZA@mail.gmail.com>
     [not found]     ` <20161102115728.52e8b187@bbrezillon>
     [not found]       ` <CAJhJPsWttKnF2WhHmX6J9PuSgUw8F9Yqrmd6aX9Qez7xyubH7A@mail.gmail.com>
     [not found]         ` <20161102205404.0611ca4c@bbrezillon>
     [not found]           ` <CAJhJPsWZ77a33rhjfB0MagZuvJzAMFOtf2qi1HYy-1+5gd7v1w@mail.gmail.com>
2016-11-06 18:03             ` Boris Brezillon
2016-11-07  9:17               ` Peter Pan
2016-11-02 11:33 ` kbuild test robot
2016-11-02 11:33   ` kbuild test robot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.