[PATCH v1] mtd: nand: tango: import driver for tango controller

[PATCH v1] mtd: nand: tango: import driver for tango controller

[Marc Gonzalez]

This driver supports the NAND Flash controller embedded in recent
Tango chips, such as SMP8758 and SMP8759.

Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
 drivers/mtd/nand/Kconfig      |   6 +
 drivers/mtd/nand/Makefile     |   1 +
 drivers/mtd/nand/tango_nand.c | 376 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 383 insertions(+)
 create mode 100644 drivers/mtd/nand/tango_nand.c

diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index f05e0e9eb2f7..22eb5457c9f7 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -205,6 +205,12 @@ config MTD_NAND_S3C2410_CLKSTOP
 	  when the is NAND chip selected or released, but will save
 	  approximately 5mA of power when there is nothing happening.
 
+config MTD_NAND_TANGO
+	tristate "NAND Flash support for Tango chips"
+	depends on ARCH_TANGO
+	help
+	  Enables the NAND Flash controller on Tango chips.
+
 config MTD_NAND_DISKONCHIP
 	tristate "DiskOnChip 2000, Millennium and Millennium Plus (NAND reimplementation)"
 	depends on HAS_IOMEM
diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
index f55335373f7c..647f727223ef 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -16,6 +16,7 @@ obj-$(CONFIG_MTD_NAND_DENALI_DT)	+= denali_dt.o
 obj-$(CONFIG_MTD_NAND_AU1550)		+= au1550nd.o
 obj-$(CONFIG_MTD_NAND_BF5XX)		+= bf5xx_nand.o
 obj-$(CONFIG_MTD_NAND_S3C2410)		+= s3c2410.o
+obj-$(CONFIG_MTD_NAND_TANGO)		+= tango_nand.o
 obj-$(CONFIG_MTD_NAND_DAVINCI)		+= davinci_nand.o
 obj-$(CONFIG_MTD_NAND_DISKONCHIP)	+= diskonchip.o
 obj-$(CONFIG_MTD_NAND_DOCG4)		+= docg4.o
diff --git a/drivers/mtd/nand/tango_nand.c b/drivers/mtd/nand/tango_nand.c
new file mode 100644
index 000000000000..dfd27081d5ec
--- /dev/null
+++ b/drivers/mtd/nand/tango_nand.c
@@ -0,0 +1,376 @@
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/clk.h>
+#include <linux/module.h>
+#include <linux/mtd/nand.h>
+#include <linux/dmaengine.h>
+#include <linux/dma-mapping.h>
+#include <linux/platform_device.h>
+
+/* Offsets relative to chip->base */
+#define PBUS_CMD	0
+#define PBUS_ADDR	4
+#define PBUS_DATA	8
+
+/* Offsets relative to reg_base */
+#define NFC_STATUS_REG	0x00
+#define NFC_FLASH_CMD	0x04
+#define NFC_DEVICE_CFG	0x08
+#define NFC_TIMING1	0x0c
+#define NFC_TIMING2	0x10
+#define NFC_XFER_CFG	0x14
+#define NFC_PKT_0_CFG	0x18
+#define NFC_PKT_N_CFG	0x1c
+#define NFC_BB_CFG	0x20
+#define NFC_ADDR_PAGE	0x24
+#define NFC_ADDR_OFFSET	0x28
+#define NFC_XFER_STATUS	0x2c
+
+/* NFC_FLASH_CMD values */
+#define NFC_READ	1
+#define NFC_WRITE	2
+
+/* NFC_XFER_STATUS values */
+#define PAGE_IS_EMPTY	BIT(16)
+
+/* Offsets relative to mem_base */
+#define ERROR_REPORT	0x1c0
+
+/* ERROR_REPORT values */
+#define DECODE_ERR_ON_PKT_0(v)	(~v & BIT(7))
+#define DECODE_ERR_ON_PKT_N(v)	(~v & BIT(15))
+#define ERR_COUNT_PKT_0(v)	((v >> 0) & 0x3f)
+#define ERR_COUNT_PKT_N(v)	((v >> 8) & 0x3f)
+
+/* Offsets relative to pbus_base */
+#define PBUS_CS_CTRL	0x83c
+#define PBUS_PAD_MODE	0x8f0
+
+/* PBUS_CS_CTRL values */
+#define PBUS_IORDY	BIT(31)
+
+/* PBUS_PAD_MODE values */
+#define MODE_RAW	0
+#define MODE_MLC	BIT(31)
+
+#define METADATA_SIZE	4
+#define BBM_SIZE	6
+#define FIELD_ORDER	15
+
+#define MAX_CS		4
+
+struct tango_nfc {
+	struct nand_hw_control hw;
+	void __iomem *reg_base, *mem_base, *pbus_base;
+	struct tango_chip *chips[MAX_CS];
+	struct dma_chan *chan;
+};
+
+#define to_tango_nfc(ptr) container_of(ptr, struct tango_nfc, hw)
+
+struct tango_chip {
+	struct nand_chip chip;
+	void __iomem *base;
+	u32 timing1, timing2, xfer_cfg, pkt_0_cfg, pkt_n_cfg, bb_cfg;
+};
+
+#define to_tango_chip(ptr) container_of(ptr, struct tango_chip, chip)
+
+#define XFER_CFG(cs, page_count, steps, metadata_size)	\
+	((cs) << 24 | (page_count) << 16 | (steps) << 8 | (metadata_size) << 0)
+
+#define PKT_CFG(size, strength) ((size) << 16 | (strength) << 0)
+
+#define BB_CFG(bb_offset, bb_size) ((bb_offset) << 16 | (bb_size) << 0)
+
+#define TIMING(t0, t1, t2, t3) (t0 << 24 | t1 << 16 | t2 << 8 | t3)
+
+#define NFC_BUSY(base) (readl_relaxed(base + NFC_STATUS_REG) & 0xf)
+
+static void tango_cmd_ctrl(struct mtd_info *mtd, int dat, unsigned int ctrl)
+{
+	struct tango_chip *chip = to_tango_chip(mtd_to_nand(mtd));
+
+	if (ctrl & NAND_CLE)
+		writeb_relaxed(dat, chip->base + PBUS_CMD);
+
+	if (ctrl & NAND_ALE)
+		writeb_relaxed(dat, chip->base + PBUS_ADDR);
+}
+
+static int tango_dev_ready(struct mtd_info *mtd)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	struct tango_nfc *nfc = to_tango_nfc(chip->controller);
+
+	return readl_relaxed(nfc->pbus_base + PBUS_CS_CTRL) & PBUS_IORDY;
+}
+
+static uint8_t tango_read_byte(struct mtd_info *mtd)
+{
+	struct tango_chip *chip = to_tango_chip(mtd_to_nand(mtd));
+
+	return readb_relaxed(chip->base + PBUS_DATA);
+}
+
+static void tango_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
+{
+	struct tango_chip *chip = to_tango_chip(mtd_to_nand(mtd));
+
+	ioread8_rep(chip->base + PBUS_DATA, buf, len);
+}
+
+static int decode_error_report(struct tango_nfc *nfc)
+{
+	u32 status, res;
+
+	status = readl_relaxed(nfc->reg_base + NFC_XFER_STATUS);
+	if (status & PAGE_IS_EMPTY)
+		return 0;
+
+	res = readl_relaxed(nfc->mem_base + ERROR_REPORT);
+
+	if (DECODE_ERR_ON_PKT_0(res) || DECODE_ERR_ON_PKT_N(res))
+		return -EBADMSG;
+
+	return max(ERR_COUNT_PKT_0(res), ERR_COUNT_PKT_N(res));
+}
+
+static void tango_dma_callback(void *arg)
+{
+	complete(arg);
+}
+
+static int do_dma(struct nand_chip *nand, int dir, int cmd, void *buf, int len, int page)
+{
+	struct tango_nfc *nfc = to_tango_nfc(nand->controller);
+	struct tango_chip *chip = to_tango_chip(nand);
+	struct dma_async_tx_descriptor *desc;
+	struct scatterlist sg;
+	struct completion tx_done;
+	int err = -EIO;
+
+	sg_init_one(&sg, buf, len);
+	if (dma_map_sg(nfc->chan->device->dev, &sg, 1, dir) != 1)
+		goto leave;
+
+	desc = dmaengine_prep_slave_sg(nfc->chan, &sg, 1, dir, DMA_PREP_INTERRUPT);
+	if (!desc)
+		goto dma_unmap;
+
+	desc->callback = tango_dma_callback;
+	desc->callback_param = &tx_done;
+	init_completion(&tx_done);
+
+	writel_relaxed(MODE_MLC, nfc->pbus_base + PBUS_PAD_MODE);
+
+	err = 0;
+	dmaengine_submit(desc);
+	dma_async_issue_pending(nfc->chan);
+
+	writel_relaxed(chip->timing1,	nfc->reg_base + NFC_TIMING1);
+	writel_relaxed(chip->timing2,	nfc->reg_base + NFC_TIMING2);
+	writel_relaxed(chip->xfer_cfg,	nfc->reg_base + NFC_XFER_CFG);
+	writel_relaxed(chip->pkt_0_cfg,	nfc->reg_base + NFC_PKT_0_CFG);
+	writel_relaxed(chip->pkt_n_cfg,	nfc->reg_base + NFC_PKT_N_CFG);
+	writel_relaxed(chip->bb_cfg,	nfc->reg_base + NFC_BB_CFG);
+	writel_relaxed(page, nfc->reg_base + NFC_ADDR_PAGE);
+	writel_relaxed(0, nfc->reg_base + NFC_ADDR_OFFSET);
+	writel(cmd, nfc->reg_base + NFC_FLASH_CMD);
+
+	wait_for_completion(&tx_done);
+
+	while (NFC_BUSY(nfc->reg_base))
+		cpu_relax();
+
+	writel_relaxed(MODE_RAW, nfc->pbus_base + PBUS_PAD_MODE);
+dma_unmap:
+	dma_unmap_sg(nfc->chan->device->dev, &sg, 1, dir);
+leave:
+	return err;
+}
+
+static int tango_read_page(struct mtd_info *mtd, struct nand_chip *chip,
+			uint8_t *buf, int oob_required, int page)
+{
+	int err, len = mtd->writesize;
+
+	err = do_dma(chip, DMA_FROM_DEVICE, NFC_READ, buf, len, page);
+	if (err)
+		return err;
+
+	return decode_error_report(to_tango_nfc(chip->controller));
+}
+
+static int tango_write_page(struct mtd_info *mtd, struct nand_chip *chip,
+			const uint8_t *buf, int oob_required, int page)
+{
+	int len = mtd->writesize;
+
+	return do_dma(chip, DMA_TO_DEVICE, NFC_WRITE, (void *)buf, len, page);
+}
+
+static u32 to_ticks(int kHz, int ps)
+{
+	return DIV_ROUND_UP_ULL((u64)kHz * ps, NSEC_PER_SEC);
+}
+
+static int set_timings(struct tango_chip *p, int kHz)
+{
+	u32 Trdy, Textw, Twc, Twpw, Tacc, Thold, Trpw, Textr;
+	const struct nand_sdr_timings *sdr;
+	int mode = onfi_get_async_timing_mode(&p->chip);
+
+	if (mode & ONFI_TIMING_MODE_UNKNOWN)
+		return -ENODEV;
+
+	sdr = onfi_async_timing_mode_to_sdr_timings(fls(mode) - 1);
+
+	Trdy	= to_ticks(kHz, sdr->tCEA_max - sdr->tREA_max);
+	Textw	= to_ticks(kHz, sdr->tWB_max);
+	Twc	= to_ticks(kHz, sdr->tWC_min);
+	Twpw	= to_ticks(kHz, sdr->tWC_min - sdr->tWP_min);
+
+	Tacc	= to_ticks(kHz, sdr->tREA_max);
+	Thold	= to_ticks(kHz, sdr->tREH_min);
+	Trpw	= to_ticks(kHz, sdr->tRC_min - sdr->tREH_min);
+	Textr	= to_ticks(kHz, sdr->tRHZ_max);
+
+	p->timing1 = TIMING(Trdy, Textw, Twc, Twpw);
+	p->timing2 = TIMING(Tacc, Thold, Trpw, Textr);
+
+	return 0;
+}
+
+static int chip_init(struct device *dev, struct device_node *np, int kHz)
+{
+	int err;
+	u32 cs, ecc_bits;
+	struct nand_ecc_ctrl *ecc;
+	struct tango_nfc *nfc = dev_get_drvdata(dev);
+	struct tango_chip *p = devm_kzalloc(dev, sizeof(*p), GFP_KERNEL);
+	if (!p)
+		return -ENOMEM;
+
+	err = of_property_read_u32_index(np, "reg", 0, &cs);
+	if (err)
+		return err;
+
+	if (cs >= MAX_CS)
+		return -ERANGE;
+
+	p->chip.read_byte	= tango_read_byte;
+	p->chip.read_buf	= tango_read_buf;
+	p->chip.cmd_ctrl	= tango_cmd_ctrl;
+	p->chip.dev_ready	= tango_dev_ready;
+	/* p->chip.options	= NAND_MAGIC_FOO */
+	p->chip.controller	= &nfc->hw;
+	p->base			= nfc->pbus_base + (cs * 256);
+
+	ecc		= &p->chip.ecc;
+	ecc->mode	= NAND_ECC_HW;
+	ecc->algo	= NAND_ECC_BCH;
+	ecc->read_page	= tango_read_page;
+	ecc->write_page	= tango_write_page;
+
+	nand_set_flash_node(&p->chip, np);
+	err = nand_scan(&p->chip.mtd, 1);
+	if (err)
+		return err;
+
+	ecc_bits = p->chip.ecc.strength * FIELD_ORDER;
+	p->chip.ecc.bytes = DIV_ROUND_UP(ecc_bits, 8);
+	set_timings(p, kHz);
+
+	err = mtd_device_register(&p->chip.mtd, NULL, 0);
+	if (err)
+		return err;
+
+	nfc->chips[cs] = p;
+
+	p->xfer_cfg	= XFER_CFG(cs, 1, ecc->steps, METADATA_SIZE);
+	p->pkt_0_cfg	= PKT_CFG(ecc->size + METADATA_SIZE, ecc->strength);
+	p->pkt_n_cfg	= PKT_CFG(ecc->size, ecc->strength);
+	p->bb_cfg	= BB_CFG(p->chip.mtd.writesize, BBM_SIZE);
+
+	return 0;
+}
+
+static int tango_nand_probe(struct platform_device *pdev)
+{
+	int i, kHz;
+	struct resource *res;
+	void __iomem *addr[3];
+	struct tango_nfc *nfc;
+	struct device_node *np;
+
+	struct clk *clk = clk_get(&pdev->dev, NULL);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	nfc = devm_kzalloc(&pdev->dev, sizeof(*nfc), GFP_KERNEL);
+	if (!nfc)
+		return -ENOMEM;
+
+	for (i = 0; i < 3; ++i) {
+		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
+		addr[i] = devm_ioremap_resource(&pdev->dev, res);
+		if (IS_ERR(addr[i]))
+			return PTR_ERR(addr[i]);
+	}
+
+	platform_set_drvdata(pdev, nfc);
+	nand_hw_control_init(&nfc->hw);
+	kHz = clk_get_rate(clk) / 1000;
+
+	nfc->reg_base	= addr[0];
+	nfc->mem_base	= addr[1];
+	nfc->pbus_base	= addr[2];
+
+	for_each_child_of_node(pdev->dev.of_node, np) {
+		int err = chip_init(&pdev->dev, np, kHz);
+		if (err)
+			return err;
+	}
+
+	nfc->chan = dma_request_chan(&pdev->dev, "mlc_flash_0");
+	if (IS_ERR(nfc->chan))
+		return PTR_ERR(nfc->chan);
+
+	/* TODO: tweak (?) Peripheral Bus setup (timings and CS config) */
+
+	return 0;
+}
+
+static int tango_nand_remove(struct platform_device *pdev)
+{
+	int cs;
+	struct tango_nfc *nfc = platform_get_drvdata(pdev);
+
+	dma_release_channel(nfc->chan);
+	for (cs = 0; cs < MAX_CS; ++cs)
+		if (nfc->chips[cs] != NULL)
+			nand_release(&nfc->chips[cs]->chip.mtd);
+
+	return 0;
+}
+
+static const struct of_device_id tango_nand_ids[] = {
+	{ .compatible = "sigma,smp8758-nand" },
+	{ /* sentinel */ }
+};
+
+static struct platform_driver tango_nand_driver = {
+	.probe	= tango_nand_probe,
+	.remove	= tango_nand_remove,
+	.driver	= {
+		.name		= "tango-nand",
+		.of_match_table	= tango_nand_ids,
+	},
+};
+
+module_platform_driver(tango_nand_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Sigma Designs");
+MODULE_DESCRIPTION("Tango4 NAND Flash controller driver");
-- 
2.9.0

Re: [PATCH v1] mtd: nand: tango: import driver for tango controller

[Boris Brezillon]

On Fri, 2 Sep 2016 12:02:27 +0200
Marc Gonzalez <marc_gonzalez@sigmadesigns.com> wrote:

> This driver supports the NAND Flash controller embedded in recent
> Tango chips, such as SMP8758 and SMP8759.
> 
> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> ---
>  drivers/mtd/nand/Kconfig      |   6 +
>  drivers/mtd/nand/Makefile     |   1 +
>  drivers/mtd/nand/tango_nand.c | 376 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 383 insertions(+)
>  create mode 100644 drivers/mtd/nand/tango_nand.c
> 
> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> index f05e0e9eb2f7..22eb5457c9f7 100644
> --- a/drivers/mtd/nand/Kconfig
> +++ b/drivers/mtd/nand/Kconfig
> @@ -205,6 +205,12 @@ config MTD_NAND_S3C2410_CLKSTOP
>  	  when the is NAND chip selected or released, but will save
>  	  approximately 5mA of power when there is nothing happening.
>  
> +config MTD_NAND_TANGO
> +	tristate "NAND Flash support for Tango chips"
> +	depends on ARCH_TANGO
> +	help
> +	  Enables the NAND Flash controller on Tango chips.
> +
>  config MTD_NAND_DISKONCHIP
>  	tristate "DiskOnChip 2000, Millennium and Millennium Plus (NAND reimplementation)"
>  	depends on HAS_IOMEM
> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
> index f55335373f7c..647f727223ef 100644
> --- a/drivers/mtd/nand/Makefile
> +++ b/drivers/mtd/nand/Makefile
> @@ -16,6 +16,7 @@ obj-$(CONFIG_MTD_NAND_DENALI_DT)	+= denali_dt.o
>  obj-$(CONFIG_MTD_NAND_AU1550)		+= au1550nd.o
>  obj-$(CONFIG_MTD_NAND_BF5XX)		+= bf5xx_nand.o
>  obj-$(CONFIG_MTD_NAND_S3C2410)		+= s3c2410.o
> +obj-$(CONFIG_MTD_NAND_TANGO)		+= tango_nand.o
>  obj-$(CONFIG_MTD_NAND_DAVINCI)		+= davinci_nand.o
>  obj-$(CONFIG_MTD_NAND_DISKONCHIP)	+= diskonchip.o
>  obj-$(CONFIG_MTD_NAND_DOCG4)		+= docg4.o
> diff --git a/drivers/mtd/nand/tango_nand.c b/drivers/mtd/nand/tango_nand.c
> new file mode 100644
> index 000000000000..dfd27081d5ec
> --- /dev/null
> +++ b/drivers/mtd/nand/tango_nand.c
> @@ -0,0 +1,376 @@
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/clk.h>
> +#include <linux/module.h>
> +#include <linux/mtd/nand.h>
> +#include <linux/dmaengine.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/platform_device.h>
> +
> +/* Offsets relative to chip->base */
> +#define PBUS_CMD	0
> +#define PBUS_ADDR	4
> +#define PBUS_DATA	8
> +
> +/* Offsets relative to reg_base */
> +#define NFC_STATUS_REG	0x00
> +#define NFC_FLASH_CMD	0x04
> +#define NFC_DEVICE_CFG	0x08
> +#define NFC_TIMING1	0x0c
> +#define NFC_TIMING2	0x10
> +#define NFC_XFER_CFG	0x14
> +#define NFC_PKT_0_CFG	0x18
> +#define NFC_PKT_N_CFG	0x1c
> +#define NFC_BB_CFG	0x20
> +#define NFC_ADDR_PAGE	0x24
> +#define NFC_ADDR_OFFSET	0x28
> +#define NFC_XFER_STATUS	0x2c
> +
> +/* NFC_FLASH_CMD values */
> +#define NFC_READ	1
> +#define NFC_WRITE	2
> +
> +/* NFC_XFER_STATUS values */
> +#define PAGE_IS_EMPTY	BIT(16)
> +
> +/* Offsets relative to mem_base */
> +#define ERROR_REPORT	0x1c0
> +
> +/* ERROR_REPORT values */
> +#define DECODE_ERR_ON_PKT_0(v)	(~v & BIT(7))
> +#define DECODE_ERR_ON_PKT_N(v)	(~v & BIT(15))

Is this really packed N? I'd say it's packet 1.

How about:

#define DECODE_ERR_ON_PKT(pkt, v)	(~(v) & BIT(((pkt) * 8) + 7))

> +#define ERR_COUNT_PKT_0(v)	((v >> 0) & 0x3f)
> +#define ERR_COUNT_PKT_N(v)	((v >> 8) & 0x3f)

Ditto.

> +
> +/* Offsets relative to pbus_base */
> +#define PBUS_CS_CTRL	0x83c
> +#define PBUS_PAD_MODE	0x8f0
> +
> +/* PBUS_CS_CTRL values */
> +#define PBUS_IORDY	BIT(31)
> +
> +/* PBUS_PAD_MODE values */
> +#define MODE_RAW	0
> +#define MODE_MLC	BIT(31)
> +
> +#define METADATA_SIZE	4
> +#define BBM_SIZE	6
> +#define FIELD_ORDER	15
> +
> +#define MAX_CS		4
> +
> +struct tango_nfc {
> +	struct nand_hw_control hw;
> +	void __iomem *reg_base, *mem_base, *pbus_base;
> +	struct tango_chip *chips[MAX_CS];
> +	struct dma_chan *chan;
> +};
> +
> +#define to_tango_nfc(ptr) container_of(ptr, struct tango_nfc, hw)
> +
> +struct tango_chip {
> +	struct nand_chip chip;
> +	void __iomem *base;

I think it better to encode the CS id, and calculate the __iomem offset
based on that at run-time. Especially if you want to support multi-CS
(multi dies) chips, which you don't seem to support here.

> +	u32 timing1, timing2, xfer_cfg, pkt_0_cfg, pkt_n_cfg, bb_cfg;

Please, one field per line in struct definitions.

> +};
> +
> +#define to_tango_chip(ptr) container_of(ptr, struct tango_chip, chip)
> +
> +#define XFER_CFG(cs, page_count, steps, metadata_size)	\
> +	((cs) << 24 | (page_count) << 16 | (steps) << 8 | (metadata_size) << 0)
> +
> +#define PKT_CFG(size, strength) ((size) << 16 | (strength) << 0)
> +
> +#define BB_CFG(bb_offset, bb_size) ((bb_offset) << 16 | (bb_size) << 0)
> +
> +#define TIMING(t0, t1, t2, t3) (t0 << 24 | t1 << 16 | t2 << 8 | t3)
> +
> +#define NFC_BUSY(base) (readl_relaxed(base + NFC_STATUS_REG) & 0xf)

I'd turn that one into an inline function taking a nand_chip pointer in
parameter. Or drop it completely, since you only have one user.

And please document why you use this 0xf mask? I guess there's one bit
per CS, so masking with 0xf is not necessarily a good idea...

> +
> +static void tango_cmd_ctrl(struct mtd_info *mtd, int dat, unsigned int ctrl)
> +{
> +	struct tango_chip *chip = to_tango_chip(mtd_to_nand(mtd));
> +
> +	if (ctrl & NAND_CLE)
> +		writeb_relaxed(dat, chip->base + PBUS_CMD);
> +
> +	if (ctrl & NAND_ALE)
> +		writeb_relaxed(dat, chip->base + PBUS_ADDR);
> +}
> +
> +static int tango_dev_ready(struct mtd_info *mtd)
> +{
> +	struct nand_chip *chip = mtd_to_nand(mtd);
> +	struct tango_nfc *nfc = to_tango_nfc(chip->controller);
> +
> +	return readl_relaxed(nfc->pbus_base + PBUS_CS_CTRL) & PBUS_IORDY;
> +}
> +
> +static uint8_t tango_read_byte(struct mtd_info *mtd)
> +{
> +	struct tango_chip *chip = to_tango_chip(mtd_to_nand(mtd));
> +
> +	return readb_relaxed(chip->base + PBUS_DATA);
> +}
> +
> +static void tango_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
> +{
> +	struct tango_chip *chip = to_tango_chip(mtd_to_nand(mtd));
> +
> +	ioread8_rep(chip->base + PBUS_DATA, buf, len);
> +}
> +
> +static int decode_error_report(struct tango_nfc *nfc)
> +{
> +	u32 status, res;
> +
> +	status = readl_relaxed(nfc->reg_base + NFC_XFER_STATUS);
> +	if (status & PAGE_IS_EMPTY)
> +		return 0;
> +
> +	res = readl_relaxed(nfc->mem_base + ERROR_REPORT);
> +
> +	if (DECODE_ERR_ON_PKT_0(res) || DECODE_ERR_ON_PKT_N(res))
> +		return -EBADMSG;

Hm, you're assuming you'll always have 2 packets in your layout, is
this true? Don't you have layouts where you only have one packet (2k
pages with 2k packets) ?

> +
> +	return max(ERR_COUNT_PKT_0(res), ERR_COUNT_PKT_N(res));
> +}
> +
> +static void tango_dma_callback(void *arg)
> +{
> +	complete(arg);
> +}
> +
> +static int do_dma(struct nand_chip *nand, int dir, int cmd, void *buf, int len, int page)
> +{
> +	struct tango_nfc *nfc = to_tango_nfc(nand->controller);
> +	struct tango_chip *chip = to_tango_chip(nand);
> +	struct dma_async_tx_descriptor *desc;
> +	struct scatterlist sg;
> +	struct completion tx_done;
> +	int err = -EIO;
> +
> +	sg_init_one(&sg, buf, len);
> +	if (dma_map_sg(nfc->chan->device->dev, &sg, 1, dir) != 1)
> +		goto leave;
> +
> +	desc = dmaengine_prep_slave_sg(nfc->chan, &sg, 1, dir, DMA_PREP_INTERRUPT);
> +	if (!desc)
> +		goto dma_unmap;
> +
> +	desc->callback = tango_dma_callback;
> +	desc->callback_param = &tx_done;
> +	init_completion(&tx_done);
> +
> +	writel_relaxed(MODE_MLC, nfc->pbus_base + PBUS_PAD_MODE);

Not sure I understand what MLC means here? Can you give more detail?

> +
> +	err = 0;
> +	dmaengine_submit(desc);
> +	dma_async_issue_pending(nfc->chan);
> +
> +	writel_relaxed(chip->timing1,	nfc->reg_base + NFC_TIMING1);
> +	writel_relaxed(chip->timing2,	nfc->reg_base + NFC_TIMING2);
> +	writel_relaxed(chip->xfer_cfg,	nfc->reg_base + NFC_XFER_CFG);
> +	writel_relaxed(chip->pkt_0_cfg,	nfc->reg_base + NFC_PKT_0_CFG);
> +	writel_relaxed(chip->pkt_n_cfg,	nfc->reg_base + NFC_PKT_N_CFG);
> +	writel_relaxed(chip->bb_cfg,	nfc->reg_base + NFC_BB_CFG);
> +	writel_relaxed(page, nfc->reg_base + NFC_ADDR_PAGE);
> +	writel_relaxed(0, nfc->reg_base + NFC_ADDR_OFFSET);
> +	writel(cmd, nfc->reg_base + NFC_FLASH_CMD);
> +
> +	wait_for_completion(&tx_done);
> +
> +	while (NFC_BUSY(nfc->reg_base))
> +		cpu_relax();
> +
> +	writel_relaxed(MODE_RAW, nfc->pbus_base + PBUS_PAD_MODE);
> +dma_unmap:
> +	dma_unmap_sg(nfc->chan->device->dev, &sg, 1, dir);
> +leave:
> +	return err;
> +}
> +
> +static int tango_read_page(struct mtd_info *mtd, struct nand_chip *chip,
> +			uint8_t *buf, int oob_required, int page)
> +{
> +	int err, len = mtd->writesize;
> +
> +	err = do_dma(chip, DMA_FROM_DEVICE, NFC_READ, buf, len, page);
> +	if (err)
> +		return err;
> +
> +	return decode_error_report(to_tango_nfc(chip->controller));
> +}
> +
> +static int tango_write_page(struct mtd_info *mtd, struct nand_chip *chip,
> +			const uint8_t *buf, int oob_required, int page)
> +{
> +	int len = mtd->writesize;
> +
> +	return do_dma(chip, DMA_TO_DEVICE, NFC_WRITE, (void *)buf, len, page);
> +}
> +
> +static u32 to_ticks(int kHz, int ps)
> +{
> +	return DIV_ROUND_UP_ULL((u64)kHz * ps, NSEC_PER_SEC);
> +}
> +
> +static int set_timings(struct tango_chip *p, int kHz)
> +{
> +	u32 Trdy, Textw, Twc, Twpw, Tacc, Thold, Trpw, Textr;
> +	const struct nand_sdr_timings *sdr;
> +	int mode = onfi_get_async_timing_mode(&p->chip);
> +
> +	if (mode & ONFI_TIMING_MODE_UNKNOWN)
> +		return -ENODEV;
> +
> +	sdr = onfi_async_timing_mode_to_sdr_timings(fls(mode) - 1);
> +
> +	Trdy	= to_ticks(kHz, sdr->tCEA_max - sdr->tREA_max);
> +	Textw	= to_ticks(kHz, sdr->tWB_max);
> +	Twc	= to_ticks(kHz, sdr->tWC_min);
> +	Twpw	= to_ticks(kHz, sdr->tWC_min - sdr->tWP_min);
> +
> +	Tacc	= to_ticks(kHz, sdr->tREA_max);
> +	Thold	= to_ticks(kHz, sdr->tREH_min);
> +	Trpw	= to_ticks(kHz, sdr->tRC_min - sdr->tREH_min);
> +	Textr	= to_ticks(kHz, sdr->tRHZ_max);
> +
> +	p->timing1 = TIMING(Trdy, Textw, Twc, Twpw);
> +	p->timing2 = TIMING(Tacc, Thold, Trpw, Textr);
> +
> +	return 0;
> +}
> +
> +static int chip_init(struct device *dev, struct device_node *np, int kHz)
> +{
> +	int err;
> +	u32 cs, ecc_bits;
> +	struct nand_ecc_ctrl *ecc;
> +	struct tango_nfc *nfc = dev_get_drvdata(dev);
> +	struct tango_chip *p = devm_kzalloc(dev, sizeof(*p), GFP_KERNEL);
> +	if (!p)
> +		return -ENOMEM;
> +
> +	err = of_property_read_u32_index(np, "reg", 0, &cs);
> +	if (err)
> +		return err;
> +
> +	if (cs >= MAX_CS)
> +		return -ERANGE;
> +
> +	p->chip.read_byte	= tango_read_byte;
> +	p->chip.read_buf	= tango_read_buf;
> +	p->chip.cmd_ctrl	= tango_cmd_ctrl;
> +	p->chip.dev_ready	= tango_dev_ready;
> +	/* p->chip.options	= NAND_MAGIC_FOO */
> +	p->chip.controller	= &nfc->hw;
> +	p->base			= nfc->pbus_base + (cs * 256);
> +
> +	ecc		= &p->chip.ecc;
> +	ecc->mode	= NAND_ECC_HW;
> +	ecc->algo	= NAND_ECC_BCH;
> +	ecc->read_page	= tango_read_page;
> +	ecc->write_page	= tango_write_page;
> +
> +	nand_set_flash_node(&p->chip, np);
> +	err = nand_scan(&p->chip.mtd, 1);
> +	if (err)
> +		return err;
> +
> +	ecc_bits = p->chip.ecc.strength * FIELD_ORDER;

Are you sure the field order is always 15? I thought the ECC controller
was adapting it depending on the packet size (512 bytes packets => 13,
1024 bytes => 14, 2k => 15), but maybe I'm wrong.

> +	p->chip.ecc.bytes = DIV_ROUND_UP(ecc_bits, 8);
> +	set_timings(p, kHz);
> +
> +	err = mtd_device_register(&p->chip.mtd, NULL, 0);
> +	if (err)
> +		return err;
> +
> +	nfc->chips[cs] = p;
> +
> +	p->xfer_cfg	= XFER_CFG(cs, 1, ecc->steps, METADATA_SIZE);
> +	p->pkt_0_cfg	= PKT_CFG(ecc->size + METADATA_SIZE, ecc->strength);
> +	p->pkt_n_cfg	= PKT_CFG(ecc->size, ecc->strength);
> +	p->bb_cfg	= BB_CFG(p->chip.mtd.writesize, BBM_SIZE);
> +
> +	return 0;
> +}
> +
> +static int tango_nand_probe(struct platform_device *pdev)
> +{
> +	int i, kHz;
> +	struct resource *res;
> +	void __iomem *addr[3];
> +	struct tango_nfc *nfc;
> +	struct device_node *np;
> +
> +	struct clk *clk = clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);
> +
> +	nfc = devm_kzalloc(&pdev->dev, sizeof(*nfc), GFP_KERNEL);
> +	if (!nfc)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < 3; ++i) {
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> +		addr[i] = devm_ioremap_resource(&pdev->dev, res);
> +		if (IS_ERR(addr[i]))
> +			return PTR_ERR(addr[i]);
> +	}
> +
> +	platform_set_drvdata(pdev, nfc);
> +	nand_hw_control_init(&nfc->hw);
> +	kHz = clk_get_rate(clk) / 1000;
> +
> +	nfc->reg_base	= addr[0];
> +	nfc->mem_base	= addr[1];
> +	nfc->pbus_base	= addr[2];

Why not doing

	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
	nfc->reg_base = devm_ioremap_resource(&pdev->dev, res);

	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
	nfc->mem_base = devm_ioremap_resource(&pdev->dev, res);
...

> +
> +	for_each_child_of_node(pdev->dev.of_node, np) {
> +		int err = chip_init(&pdev->dev, np, kHz);
> +		if (err)
> +			return err;
> +	}
> +
> +	nfc->chan = dma_request_chan(&pdev->dev, "mlc_flash_0");
> +	if (IS_ERR(nfc->chan))
> +		return PTR_ERR(nfc->chan);
> +
> +	/* TODO: tweak (?) Peripheral Bus setup (timings and CS config) */
> +
> +	return 0;
> +}
> +
> +static int tango_nand_remove(struct platform_device *pdev)
> +{
> +	int cs;
> +	struct tango_nfc *nfc = platform_get_drvdata(pdev);
> +
> +	dma_release_channel(nfc->chan);
> +	for (cs = 0; cs < MAX_CS; ++cs)
> +		if (nfc->chips[cs] != NULL)
> +			nand_release(&nfc->chips[cs]->chip.mtd);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id tango_nand_ids[] = {
> +	{ .compatible = "sigma,smp8758-nand" },
> +	{ /* sentinel */ }
> +};
> +
> +static struct platform_driver tango_nand_driver = {
> +	.probe	= tango_nand_probe,
> +	.remove	= tango_nand_remove,
> +	.driver	= {
> +		.name		= "tango-nand",
> +		.of_match_table	= tango_nand_ids,
> +	},
> +};
> +
> +module_platform_driver(tango_nand_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Sigma Designs");
> +MODULE_DESCRIPTION("Tango4 NAND Flash controller driver");

Re: [PATCH v1] mtd: nand: tango: import driver for tango controller

[Mason]

On 05/09/2016 09:14, Boris Brezillon wrote:

> Marc Gonzalez wrote:
> 
>> +/* ERROR_REPORT values */
>> +#define DECODE_ERR_ON_PKT_0(v)	(~v & BIT(7))
>> +#define DECODE_ERR_ON_PKT_N(v)	(~v & BIT(15))
> 
> Is this really packet N? I'd say it's packet 1.

NB: "packet_0" and "packet_n" are terms used in the controller's
documentation (which is not publicly available AFAIU).

I didn't want to change the names, for fear of confusing a
future (internal) maintainer of the code. But in my mind,
ERR_ON_FIRST_PKT and ERR_ON_OTHER_PKT are clearer. Perhaps
I can use these identifiers, with a comment mentioning the
"internal" names.

DECODE_ERR_ON_PKT_N(v) actually means:
"decode error on packet N, for N > 0"
(The HW supports splitting a page in 1 to 16 packets.)

> #define DECODE_ERR_ON_PKT(pkt, v)	(~(v) & BIT(((pkt) * 8) + 7))

Byte 0 is the report for packet 0.
Byte 1 is the report for other packets.

>> +#define ERR_COUNT_PKT_0(v)	((v >> 0) & 0x3f)
>> +#define ERR_COUNT_PKT_N(v)	((v >> 8) & 0x3f)

There are only two bytes in the error report.
ERR_COUNT_PKT_N(v) returns the max error count for N > 1

>> +struct tango_chip {
>> +	struct nand_chip chip;
>> +	void __iomem *base;
> 
> I think it better to encode the CS id, and calculate the __iomem offset
> based on that at run-time. Especially if you want to support multi-CS
> (multi dies) chips, which you don't seem to support here.

To support multi-CS chips, I would have to define a
select_chip callback, where I save the requested CS
inside the struct tango_chip and use that to compute
the offset later?


>> +	u32 timing1, timing2, xfer_cfg, pkt_0_cfg, pkt_n_cfg, bb_cfg;
> 
> Please, one field per line in struct definitions.

OK.


>> +#define NFC_BUSY(base) (readl_relaxed(base + NFC_STATUS_REG) & 0xf)
> 
> I'd turn that one into an inline function taking a nand_chip pointer in
> parameter. Or drop it completely, since you only have one user.

OK.


> And please document why you use this 0xf mask? I guess there's one bit
> per CS, so masking with 0xf is not necessarily a good idea...

There's a 4-bit status code, 0 means idle, non-0 means busy.
I'll document the mask.


>> +	res = readl_relaxed(nfc->mem_base + ERROR_REPORT);
>> +
>> +	if (DECODE_ERR_ON_PKT_0(res) || DECODE_ERR_ON_PKT_N(res))
>> +		return -EBADMSG;
> 
> Hm, you're assuming you'll always have 2 packets in your layout, is
> this true? Don't you have layouts where you only have one packet (2k
> pages with 2k packets) ?

The legacy driver hard-codes packet size to 1024.
Thus 2 packets for 2k pages, 4 packets for 4k pages.
Are there recent NAND chips with 1k pages?


>> +	writel_relaxed(MODE_MLC, nfc->pbus_base + PBUS_PAD_MODE);
> 
> Not sure I understand what MLC means here? Can you give more detail?

I'll rename it to MODE_NFC (for "NAND Flash Controller").

Basically, either we access NAND chips directly in "raw" mode,
or we go through the NAND Flash Controller.

My driver uses "raw" mode for everything except read_page and
write_page (because they require DMA and HW ECC).


>> +	ecc_bits = p->chip.ecc.strength * FIELD_ORDER;
> 
> Are you sure the field order is always 15? I thought the ECC controller
> was adapting it depending on the packet size (512 bytes packets => 13,
> 1024 bytes => 14, 2k => 15), but maybe I'm wrong.

If I'm reading the doc right, it's always 15.


>> +static int tango_nand_probe(struct platform_device *pdev)
>> +{
>> +	int i, kHz;
>> +	struct resource *res;
>> +	void __iomem *addr[3];
>> +	struct tango_nfc *nfc;
>> +	struct device_node *np;
>> +
>> +	struct clk *clk = clk_get(&pdev->dev, NULL);
>> +	if (IS_ERR(clk))
>> +		return PTR_ERR(clk);
>> +
>> +	nfc = devm_kzalloc(&pdev->dev, sizeof(*nfc), GFP_KERNEL);
>> +	if (!nfc)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < 3; ++i) {
>> +		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
>> +		addr[i] = devm_ioremap_resource(&pdev->dev, res);
>> +		if (IS_ERR(addr[i]))
>> +			return PTR_ERR(addr[i]);
>> +	}
>> +
>> +	platform_set_drvdata(pdev, nfc);
>> +	nand_hw_control_init(&nfc->hw);
>> +	kHz = clk_get_rate(clk) / 1000;
>> +
>> +	nfc->reg_base	= addr[0];
>> +	nfc->mem_base	= addr[1];
>> +	nfc->pbus_base	= addr[2];
> 
> Why not doing
> 
> 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> 	nfc->reg_base = devm_ioremap_resource(&pdev->dev, res);
> 
> 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> 	nfc->mem_base = devm_ioremap_resource(&pdev->dev, res);

Do you mean why do I have a loop for the 3 ioremaps?
IIUC, you'd prefer that I unroll the loop?

Regards.

Re: [PATCH v1] mtd: nand: tango: import driver for tango controller

[Boris Brezillon]

On Mon, 5 Sep 2016 12:18:26 +0200
Mason <slash.tmp@free.fr> wrote:

> On 05/09/2016 09:14, Boris Brezillon wrote:
> 
> > Marc Gonzalez wrote:
> >   
> >> +/* ERROR_REPORT values */
> >> +#define DECODE_ERR_ON_PKT_0(v)	(~v & BIT(7))
> >> +#define DECODE_ERR_ON_PKT_N(v)	(~v & BIT(15))  
> > 
> > Is this really packet N? I'd say it's packet 1.  
> 
> NB: "packet_0" and "packet_n" are terms used in the controller's
> documentation (which is not publicly available AFAIU).
> 
> I didn't want to change the names, for fear of confusing a
> future (internal) maintainer of the code. But in my mind,
> ERR_ON_FIRST_PKT and ERR_ON_OTHER_PKT are clearer. Perhaps
> I can use these identifiers, with a comment mentioning the
> "internal" names.

Or do it the other way around: keep the datasheet wording and add a
comment explaining what it means in your code.

> 
> DECODE_ERR_ON_PKT_N(v) actually means:
> "decode error on packet N, for N > 0"
> (The HW supports splitting a page in 1 to 16 packets.)
> 
> > #define DECODE_ERR_ON_PKT(pkt, v)	(~(v) & BIT(((pkt) * 8) + 7))  
> 
> Byte 0 is the report for packet 0.
> Byte 1 is the report for other packets.
> 
> >> +#define ERR_COUNT_PKT_0(v)	((v >> 0) & 0x3f)
> >> +#define ERR_COUNT_PKT_N(v)	((v >> 8) & 0x3f)  
> 
> There are only two bytes in the error report.
> ERR_COUNT_PKT_N(v) returns the max error count for N > 1

Okay, then again, explain that in a comment.

> 
> >> +struct tango_chip {
> >> +	struct nand_chip chip;
> >> +	void __iomem *base;  
> > 
> > I think it better to encode the CS id, and calculate the __iomem offset
> > based on that at run-time. Especially if you want to support multi-CS
> > (multi dies) chips, which you don't seem to support here.  
> 
> To support multi-CS chips, I would have to define a
> select_chip callback, where I save the requested CS
> inside the struct tango_chip and use that to compute
> the offset later?

Yes, that's a solution.

> 
> 
> >> +	u32 timing1, timing2, xfer_cfg, pkt_0_cfg, pkt_n_cfg, bb_cfg;  
> > 
> > Please, one field per line in struct definitions.  
> 
> OK.
> 
> 
> >> +#define NFC_BUSY(base) (readl_relaxed(base + NFC_STATUS_REG) & 0xf)  
> > 
> > I'd turn that one into an inline function taking a nand_chip pointer in
> > parameter. Or drop it completely, since you only have one user.  
> 
> OK.
> 
> 
> > And please document why you use this 0xf mask? I guess there's one bit
> > per CS, so masking with 0xf is not necessarily a good idea...  
> 
> There's a 4-bit status code, 0 means idle, non-0 means busy.
> I'll document the mask.
> 
> 
> >> +	res = readl_relaxed(nfc->mem_base + ERROR_REPORT);
> >> +
> >> +	if (DECODE_ERR_ON_PKT_0(res) || DECODE_ERR_ON_PKT_N(res))
> >> +		return -EBADMSG;  
> > 
> > Hm, you're assuming you'll always have 2 packets in your layout, is
> > this true? Don't you have layouts where you only have one packet (2k
> > pages with 2k packets) ?  
> 
> The legacy driver hard-codes packet size to 1024.
> Thus 2 packets for 2k pages, 4 packets for 4k pages.
> Are there recent NAND chips with 1k pages?

Nope. Actually I didn't understand the meaning of PKT_N(). Now that you
clarified the situation (it returns the max value for packet > 0), I
fine with your implementation.

> 
> 
> >> +	writel_relaxed(MODE_MLC, nfc->pbus_base + PBUS_PAD_MODE);  
> > 
> > Not sure I understand what MLC means here? Can you give more detail?  
> 
> I'll rename it to MODE_NFC (for "NAND Flash Controller").
> 
> Basically, either we access NAND chips directly in "raw" mode,
> or we go through the NAND Flash Controller.
> 
> My driver uses "raw" mode for everything except read_page and
> write_page (because they require DMA and HW ECC).

Okay, then yes, please choose a better name.

> 
> 
> >> +	ecc_bits = p->chip.ecc.strength * FIELD_ORDER;  
> > 
> > Are you sure the field order is always 15? I thought the ECC controller
> > was adapting it depending on the packet size (512 bytes packets => 13,
> > 1024 bytes => 14, 2k => 15), but maybe I'm wrong.  
> 
> If I'm reading the doc right, it's always 15.

Okay, so, no matter what packet size you choose, the number of ECC
bytes for a given strength will always be the same, right?

From a storage PoV, that means you'd better choose the biggest packet
size, but I understand that you want to support existing layouts, and
this may also have an impact if you want to support subpage I/Os.

> 
> 
> >> +static int tango_nand_probe(struct platform_device *pdev)
> >> +{
> >> +	int i, kHz;
> >> +	struct resource *res;
> >> +	void __iomem *addr[3];
> >> +	struct tango_nfc *nfc;
> >> +	struct device_node *np;
> >> +
> >> +	struct clk *clk = clk_get(&pdev->dev, NULL);
> >> +	if (IS_ERR(clk))
> >> +		return PTR_ERR(clk);
> >> +
> >> +	nfc = devm_kzalloc(&pdev->dev, sizeof(*nfc), GFP_KERNEL);
> >> +	if (!nfc)
> >> +		return -ENOMEM;
> >> +
> >> +	for (i = 0; i < 3; ++i) {
> >> +		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> >> +		addr[i] = devm_ioremap_resource(&pdev->dev, res);
> >> +		if (IS_ERR(addr[i]))
> >> +			return PTR_ERR(addr[i]);
> >> +	}
> >> +
> >> +	platform_set_drvdata(pdev, nfc);
> >> +	nand_hw_control_init(&nfc->hw);
> >> +	kHz = clk_get_rate(clk) / 1000;
> >> +
> >> +	nfc->reg_base	= addr[0];
> >> +	nfc->mem_base	= addr[1];
> >> +	nfc->pbus_base	= addr[2];  
> > 
> > Why not doing
> > 
> > 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > 	nfc->reg_base = devm_ioremap_resource(&pdev->dev, res);
> > 
> > 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

I meant 

	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);

> > 	nfc->mem_base = devm_ioremap_resource(&pdev->dev, res);  
> 
> Do you mean why do I have a loop for the 3 ioremaps?
> IIUC, you'd prefer that I unroll the loop?

Having a loop would be appropriate if you were filling an nfc->iomems[]
array, but here you're just putting the values in a temporary table to
then assign each entry to a different field in your NFC struct.
It's not really useful in my opinion.

[PATCH v2] mtd: nand: tango: import driver for tango chips

[Marc Gonzalez]

This driver supports the NAND Flash controller embedded in recent
Tango chips, such as SMP8758 and SMP8759.

Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
 drivers/mtd/nand/Kconfig      |   6 +
 drivers/mtd/nand/Makefile     |   1 +
 drivers/mtd/nand/tango_nand.c | 417 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 424 insertions(+)
 create mode 100644 drivers/mtd/nand/tango_nand.c

diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index f05e0e9eb2f7..22eb5457c9f7 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -205,6 +205,12 @@ config MTD_NAND_S3C2410_CLKSTOP
 	  when the is NAND chip selected or released, but will save
 	  approximately 5mA of power when there is nothing happening.
 
+config MTD_NAND_TANGO
+	tristate "NAND Flash support for Tango chips"
+	depends on ARCH_TANGO
+	help
+	  Enables the NAND Flash controller on Tango chips.
+
 config MTD_NAND_DISKONCHIP
 	tristate "DiskOnChip 2000, Millennium and Millennium Plus (NAND reimplementation)"
 	depends on HAS_IOMEM
diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
index f55335373f7c..647f727223ef 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -16,6 +16,7 @@ obj-$(CONFIG_MTD_NAND_DENALI_DT)	+= denali_dt.o
 obj-$(CONFIG_MTD_NAND_AU1550)		+= au1550nd.o
 obj-$(CONFIG_MTD_NAND_BF5XX)		+= bf5xx_nand.o
 obj-$(CONFIG_MTD_NAND_S3C2410)		+= s3c2410.o
+obj-$(CONFIG_MTD_NAND_TANGO)		+= tango_nand.o
 obj-$(CONFIG_MTD_NAND_DAVINCI)		+= davinci_nand.o
 obj-$(CONFIG_MTD_NAND_DISKONCHIP)	+= diskonchip.o
 obj-$(CONFIG_MTD_NAND_DOCG4)		+= docg4.o
diff --git a/drivers/mtd/nand/tango_nand.c b/drivers/mtd/nand/tango_nand.c
new file mode 100644
index 000000000000..eecb98b5f1c3
--- /dev/null
+++ b/drivers/mtd/nand/tango_nand.c
@@ -0,0 +1,417 @@
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/clk.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/mtd/nand.h>
+#include <linux/dmaengine.h>
+#include <linux/dma-mapping.h>
+#include <linux/platform_device.h>
+
+/* Offsets relative to chip->base */
+#define PBUS_CMD	0
+#define PBUS_ADDR	4
+#define PBUS_DATA	8
+
+/* Offsets relative to reg_base */
+#define NFC_STATUS	0x00
+#define NFC_FLASH_CMD	0x04
+#define NFC_DEVICE_CFG	0x08
+#define NFC_TIMING1	0x0c
+#define NFC_TIMING2	0x10
+#define NFC_XFER_CFG	0x14
+#define NFC_PKT_0_CFG	0x18
+#define NFC_PKT_N_CFG	0x1c
+#define NFC_BB_CFG	0x20
+#define NFC_ADDR_PAGE	0x24
+#define NFC_ADDR_OFFSET	0x28
+#define NFC_XFER_STATUS	0x2c
+
+/* NFC_STATUS values */
+#define CMD_READY	BIT(31)
+
+/* NFC_FLASH_CMD values */
+#define NFC_READ	1
+#define NFC_WRITE	2
+
+/* NFC_XFER_STATUS values */
+#define PAGE_IS_EMPTY	BIT(16)
+
+/* Offsets relative to mem_base */
+#define ERROR_REPORT	0x1c0
+
+/*
+ * Error reports are split in two bytes:
+ * byte 0 for the first packet in a page (PACKET_0)
+ * byte 1 for other packets in a page (PACKET_N, for N > 0)
+ * ERR_COUNT_PKT_N is the max error count over all but the first packet.
+ */
+#define DECODE_ERR_ON_PKT_0(v)	(~v & BIT(7))
+#define DECODE_ERR_ON_PKT_N(v)	(~v & BIT(15))
+#define ERR_COUNT_PKT_0(v)	((v >> 0) & 0x3f)
+#define ERR_COUNT_PKT_N(v)	((v >> 8) & 0x3f)
+
+/* Offsets relative to pbus_base */
+#define PBUS_CS_CTRL	0x83c
+#define PBUS_PAD_MODE	0x8f0
+
+/* PBUS_CS_CTRL values */
+#define PBUS_IORDY	BIT(31)
+
+/*
+ * PBUS_PAD_MODE values
+ * In raw mode, the driver communicates directly with the NAND chips.
+ * In NFC mode, the NAND Flash controller manages the communication.
+ * We use NFC mode for read and write; raw mode for everything else.
+ */
+#define MODE_RAW	0
+#define MODE_NFC	BIT(31)
+
+#define METADATA_SIZE	4
+#define BBM_SIZE	6
+#define FIELD_ORDER	15
+
+#define MAX_CS		4
+
+struct tango_nfc {
+	struct nand_hw_control hw;
+	void __iomem *reg_base;
+	void __iomem *mem_base;
+	void __iomem *pbus_base;
+	struct tango_chip *chips[MAX_CS];
+	struct dma_chan *chan;
+};
+
+#define to_tango_nfc(ptr) container_of(ptr, struct tango_nfc, hw)
+
+struct tango_chip {
+	struct nand_chip chip;
+	void __iomem *base;
+	u32 timing1;
+	u32 timing2;
+	u32 xfer_cfg;
+	u32 pkt_0_cfg;
+	u32 pkt_n_cfg;
+	u32 bb_cfg;
+	int cs;
+};
+
+#define to_tango_chip(ptr) container_of(ptr, struct tango_chip, chip)
+
+#define XFER_CFG(cs, page_count, steps, metadata_size)	\
+	((cs) << 24 | (page_count) << 16 | (steps) << 8 | (metadata_size) << 0)
+
+#define PKT_CFG(size, strength) ((size) << 16 | (strength) << 0)
+
+#define BB_CFG(bb_offset, bb_size) ((bb_offset) << 16 | (bb_size) << 0)
+
+#define TIMING(t0, t1, t2, t3) (t0 << 24 | t1 << 16 | t2 << 8 | t3 << 0)
+
+static void tango_cmd_ctrl(struct mtd_info *mtd, int dat, unsigned int ctrl)
+{
+	struct tango_chip *chip = to_tango_chip(mtd_to_nand(mtd));
+
+	if (ctrl & NAND_CLE)
+		writeb_relaxed(dat, chip->base + PBUS_CMD);
+
+	if (ctrl & NAND_ALE)
+		writeb_relaxed(dat, chip->base + PBUS_ADDR);
+}
+
+static int tango_dev_ready(struct mtd_info *mtd)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	struct tango_nfc *nfc = to_tango_nfc(chip->controller);
+
+	return readl_relaxed(nfc->pbus_base + PBUS_CS_CTRL) & PBUS_IORDY;
+}
+
+static uint8_t tango_read_byte(struct mtd_info *mtd)
+{
+	struct tango_chip *chip = to_tango_chip(mtd_to_nand(mtd));
+
+	return readb_relaxed(chip->base + PBUS_DATA);
+}
+
+static void tango_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
+{
+	struct tango_chip *chip = to_tango_chip(mtd_to_nand(mtd));
+
+	ioread8_rep(chip->base + PBUS_DATA, buf, len);
+}
+
+static void tango_select_chip(struct mtd_info *mtd, int idx)
+{
+	struct nand_chip *nand = mtd_to_nand(mtd);
+	struct tango_nfc *nfc = to_tango_nfc(nand->controller);
+	struct tango_chip *chip = to_tango_chip(nand);
+
+	if (idx < 0) {
+		chip->base = NULL;
+		return;
+	}
+
+	chip->base = nfc->pbus_base + (chip->cs * 256);
+
+	writel_relaxed(chip->timing1,	nfc->reg_base + NFC_TIMING1);
+	writel_relaxed(chip->timing2,	nfc->reg_base + NFC_TIMING2);
+	writel_relaxed(chip->xfer_cfg,	nfc->reg_base + NFC_XFER_CFG);
+	writel_relaxed(chip->pkt_0_cfg,	nfc->reg_base + NFC_PKT_0_CFG);
+	writel_relaxed(chip->pkt_n_cfg,	nfc->reg_base + NFC_PKT_N_CFG);
+	writel_relaxed(chip->bb_cfg,	nfc->reg_base + NFC_BB_CFG);
+}
+
+static int decode_error_report(struct tango_nfc *nfc)
+{
+	u32 status, res;
+
+	status = readl_relaxed(nfc->reg_base + NFC_XFER_STATUS);
+	if (status & PAGE_IS_EMPTY)
+		return 0;
+
+	res = readl_relaxed(nfc->mem_base + ERROR_REPORT);
+
+	if (DECODE_ERR_ON_PKT_0(res) || DECODE_ERR_ON_PKT_N(res))
+		return -EBADMSG;
+
+	return max(ERR_COUNT_PKT_0(res), ERR_COUNT_PKT_N(res));
+}
+
+static void tango_dma_callback(void *arg)
+{
+	complete(arg);
+}
+
+static int do_dma(struct tango_nfc *nfc, int dir, int cmd, void *buf, int len, int page)
+{
+	struct dma_async_tx_descriptor *desc;
+	struct scatterlist sg;
+	struct completion tx_done;
+	int err = -EIO;
+	u32 res, val;
+
+	sg_init_one(&sg, buf, len);
+	if (dma_map_sg(nfc->chan->device->dev, &sg, 1, dir) != 1)
+		goto leave;
+
+	desc = dmaengine_prep_slave_sg(nfc->chan, &sg, 1, dir, DMA_PREP_INTERRUPT);
+	if (!desc)
+		goto dma_unmap;
+
+	desc->callback = tango_dma_callback;
+	desc->callback_param = &tx_done;
+	init_completion(&tx_done);
+
+	writel_relaxed(MODE_NFC, nfc->pbus_base + PBUS_PAD_MODE);
+
+	writel_relaxed(page, nfc->reg_base + NFC_ADDR_PAGE);
+	writel_relaxed(   0, nfc->reg_base + NFC_ADDR_OFFSET);
+	writel_relaxed( cmd, nfc->reg_base + NFC_FLASH_CMD);
+
+	dmaengine_submit(desc);
+	dma_async_issue_pending(nfc->chan);
+
+	res = wait_for_completion_timeout(&tx_done, HZ);
+	if (res > 0) {
+		void __iomem *addr = nfc->reg_base + NFC_STATUS;
+		err = readl_poll_timeout(addr, val, val & CMD_READY, 0, 1000);
+	}
+
+	writel_relaxed(MODE_RAW, nfc->pbus_base + PBUS_PAD_MODE);
+dma_unmap:
+	dma_unmap_sg(nfc->chan->device->dev, &sg, 1, dir);
+leave:
+	return err;
+}
+
+static int tango_read_page(struct mtd_info *mtd, struct nand_chip *chip,
+			uint8_t *buf, int oob_required, int page)
+{
+	struct tango_nfc *nfc = to_tango_nfc(chip->controller);
+	int err, len = mtd->writesize;
+
+	err = do_dma(nfc, DMA_FROM_DEVICE, NFC_READ, buf, len, page);
+	if (err)
+		return err;
+
+	return decode_error_report(nfc);
+}
+
+static int tango_write_page(struct mtd_info *mtd, struct nand_chip *chip,
+			const uint8_t *buf, int oob_required, int page)
+{
+	struct tango_nfc *nfc = to_tango_nfc(chip->controller);
+	int len = mtd->writesize;
+
+	return do_dma(nfc, DMA_TO_DEVICE, NFC_WRITE, (void *)buf, len, page);
+}
+
+static u32 to_ticks(int kHz, int ps)
+{
+	return DIV_ROUND_UP_ULL((u64)kHz * ps, NSEC_PER_SEC);
+}
+
+static int set_timings(struct tango_chip *p, int kHz)
+{
+	u32 Trdy, Textw, Twc, Twpw, Tacc, Thold, Trpw, Textr;
+	const struct nand_sdr_timings *sdr;
+	int mode = onfi_get_async_timing_mode(&p->chip);
+
+	if (mode & ONFI_TIMING_MODE_UNKNOWN)
+		return -ENODEV;
+
+	sdr = onfi_async_timing_mode_to_sdr_timings(fls(mode) - 1);
+
+	Trdy	= to_ticks(kHz, sdr->tCEA_max - sdr->tREA_max);
+	Textw	= to_ticks(kHz, sdr->tWB_max);
+	Twc	= to_ticks(kHz, sdr->tWC_min);
+	Twpw	= to_ticks(kHz, sdr->tWC_min - sdr->tWP_min);
+
+	Tacc	= to_ticks(kHz, sdr->tREA_max);
+	Thold	= to_ticks(kHz, sdr->tREH_min);
+	Trpw	= to_ticks(kHz, sdr->tRC_min - sdr->tREH_min);
+	Textr	= to_ticks(kHz, sdr->tRHZ_max);
+
+	p->timing1 = TIMING(Trdy, Textw, Twc, Twpw);
+	p->timing2 = TIMING(Tacc, Thold, Trpw, Textr);
+
+	return 0;
+}
+
+static int chip_init(struct device *dev, struct device_node *np, int kHz)
+{
+	int err;
+	u32 cs, ecc_bits;
+	struct nand_ecc_ctrl *ecc;
+	struct tango_nfc *nfc = dev_get_drvdata(dev);
+	struct tango_chip *p = devm_kzalloc(dev, sizeof(*p), GFP_KERNEL);
+	if (!p)
+		return -ENOMEM;
+
+	err = of_property_read_u32_index(np, "reg", 0, &cs);
+	if (err)
+		return err;
+
+	if (cs >= MAX_CS)
+		return -ERANGE;
+
+	p->chip.read_byte	= tango_read_byte;
+	p->chip.read_buf	= tango_read_buf;
+	p->chip.select_chip	= tango_select_chip;
+	p->chip.cmd_ctrl	= tango_cmd_ctrl;
+	p->chip.dev_ready	= tango_dev_ready;
+	p->chip.options		= NAND_USE_BOUNCE_BUFFER;
+	p->chip.controller	= &nfc->hw;
+
+	ecc		= &p->chip.ecc;
+	ecc->mode	= NAND_ECC_HW;
+	ecc->algo	= NAND_ECC_BCH;
+	ecc->read_page	= tango_read_page;
+	ecc->write_page	= tango_write_page;
+
+	nand_set_flash_node(&p->chip, np);
+	err = nand_scan(&p->chip.mtd, 1);
+	if (err)
+		return err;
+
+	ecc_bits = p->chip.ecc.strength * FIELD_ORDER;
+	p->chip.ecc.bytes = DIV_ROUND_UP(ecc_bits, BITS_PER_BYTE);
+	set_timings(p, kHz);
+
+	err = mtd_device_register(&p->chip.mtd, NULL, 0);
+	if (err)
+		return err;
+
+	nfc->chips[cs] = p;
+
+	p->xfer_cfg	= XFER_CFG(cs, 1, ecc->steps, METADATA_SIZE);
+	p->pkt_0_cfg	= PKT_CFG(ecc->size + METADATA_SIZE, ecc->strength);
+	p->pkt_n_cfg	= PKT_CFG(ecc->size, ecc->strength);
+	p->bb_cfg	= BB_CFG(p->chip.mtd.writesize, BBM_SIZE);
+	p->cs		= cs;
+
+	return 0;
+}
+
+static int tango_nand_probe(struct platform_device *pdev)
+{
+	int kHz;
+	struct clk *clk;
+	struct resource *res;
+	struct tango_nfc *nfc;
+	struct device_node *np;
+
+	nfc = devm_kzalloc(&pdev->dev, sizeof(*nfc), GFP_KERNEL);
+	if (!nfc)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	nfc->reg_base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(nfc->reg_base))
+		return PTR_ERR(nfc->reg_base);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	nfc->mem_base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(nfc->mem_base))
+		return PTR_ERR(nfc->mem_base);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
+	nfc->pbus_base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(nfc->pbus_base))
+		return PTR_ERR(nfc->pbus_base);
+
+	nfc->chan = dma_request_chan(&pdev->dev, "nfc_sbox");
+	if (IS_ERR(nfc->chan))
+		return PTR_ERR(nfc->chan);
+
+	clk = clk_get(&pdev->dev, NULL);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	platform_set_drvdata(pdev, nfc);
+	nand_hw_control_init(&nfc->hw);
+	kHz = clk_get_rate(clk) / 1000;
+
+	for_each_child_of_node(pdev->dev.of_node, np) {
+		int err = chip_init(&pdev->dev, np, kHz);
+		if (err)
+			return err;
+	}
+
+	/* TODO: tweak (?) Peripheral Bus setup (timings and CS config) */
+
+	return 0;
+}
+
+static int tango_nand_remove(struct platform_device *pdev)
+{
+	int cs;
+	struct tango_nfc *nfc = platform_get_drvdata(pdev);
+
+	dma_release_channel(nfc->chan);
+	for (cs = 0; cs < MAX_CS; ++cs)
+		if (nfc->chips[cs] != NULL)
+			nand_release(&nfc->chips[cs]->chip.mtd);
+
+	return 0;
+}
+
+static const struct of_device_id tango_nand_ids[] = {
+	{ .compatible = "sigma,smp8758-nand" },
+	{ /* sentinel */ }
+};
+
+static struct platform_driver tango_nand_driver = {
+	.probe	= tango_nand_probe,
+	.remove	= tango_nand_remove,
+	.driver	= {
+		.name		= "tango-nand",
+		.of_match_table	= tango_nand_ids,
+	},
+};
+
+module_platform_driver(tango_nand_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Sigma Designs");
+MODULE_DESCRIPTION("Tango4 NAND Flash controller driver");
-- 
2.9.0

Re: [PATCH v2] mtd: nand: tango: import driver for tango chips

[Marc Gonzalez]

On 08/09/2016 17:55, Marc Gonzalez wrote:

> +static int decode_error_report(struct tango_nfc *nfc)
> +{
> +	u32 status, res;
> +
> +	status = readl_relaxed(nfc->reg_base + NFC_XFER_STATUS);
> +	if (status & PAGE_IS_EMPTY)
> +		return 0;
> +
> +	res = readl_relaxed(nfc->mem_base + ERROR_REPORT);
> +
> +	if (DECODE_ERR_ON_PKT_0(res) || DECODE_ERR_ON_PKT_N(res))
> +		return -EBADMSG;
> +
> +	return max(ERR_COUNT_PKT_0(res), ERR_COUNT_PKT_N(res));
> +}

This is probably an incorrect implementation. Boris mentioned that
we shouldn't return -EBADMSG?

# time insmod mtd/mtd_nandbiterrs.ko dev=1
[   19.498729] 
[   19.501435] ==================================================
[   19.508199] mtd_nandbiterrs: MTD device: 1
[   19.512372] mtd_nandbiterrs: MTD device size 536870912, eraseblock=131072, page=2048, oob=64
[   19.520893] mtd_nandbiterrs: Device uses 2 subpages of 1024 bytes
[   19.527034] mtd_nandbiterrs: Using page=0, offset=0, eraseblock=0
[   19.533986] mtd_nandbiterrs: incremental biterrors test
[   19.539297] mtd_nandbiterrs: write_page
[   19.543413] mtd_nandbiterrs: rewrite page
[   19.547766] mtd_nandbiterrs: read_page
[   19.551746] mtd_nandbiterrs: error: read failed at 0x0
[   19.556924] mtd_nandbiterrs: After 0 biterrors per subpage, read reported error -74
[   19.565432] mtd_nandbiterrs: finished successfully.
[   19.570343] ==================================================

It says "finished successfully", should I believe that?

Regards.

[PATCH v3] mtd: nand: tango: import driver for tango chips

[Marc Gonzalez]

This driver supports the NAND Flash controller embedded in recent
Tango chips, such as SMP8758 and SMP8759.

Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
 drivers/mtd/nand/Kconfig      |   6 +
 drivers/mtd/nand/Makefile     |   1 +
 drivers/mtd/nand/tango_nand.c | 482 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 489 insertions(+)
 create mode 100644 drivers/mtd/nand/tango_nand.c

diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index f05e0e9eb2f7..22eb5457c9f7 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -205,6 +205,12 @@ config MTD_NAND_S3C2410_CLKSTOP
 	  when the is NAND chip selected or released, but will save
 	  approximately 5mA of power when there is nothing happening.
 
+config MTD_NAND_TANGO
+	tristate "NAND Flash support for Tango chips"
+	depends on ARCH_TANGO
+	help
+	  Enables the NAND Flash controller on Tango chips.
+
 config MTD_NAND_DISKONCHIP
 	tristate "DiskOnChip 2000, Millennium and Millennium Plus (NAND reimplementation)"
 	depends on HAS_IOMEM
diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
index f55335373f7c..647f727223ef 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -16,6 +16,7 @@ obj-$(CONFIG_MTD_NAND_DENALI_DT)	+= denali_dt.o
 obj-$(CONFIG_MTD_NAND_AU1550)		+= au1550nd.o
 obj-$(CONFIG_MTD_NAND_BF5XX)		+= bf5xx_nand.o
 obj-$(CONFIG_MTD_NAND_S3C2410)		+= s3c2410.o
+obj-$(CONFIG_MTD_NAND_TANGO)		+= tango_nand.o
 obj-$(CONFIG_MTD_NAND_DAVINCI)		+= davinci_nand.o
 obj-$(CONFIG_MTD_NAND_DISKONCHIP)	+= diskonchip.o
 obj-$(CONFIG_MTD_NAND_DOCG4)		+= docg4.o
diff --git a/drivers/mtd/nand/tango_nand.c b/drivers/mtd/nand/tango_nand.c
new file mode 100644
index 000000000000..1fce4ec25cc1
--- /dev/null
+++ b/drivers/mtd/nand/tango_nand.c
@@ -0,0 +1,482 @@
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/clk.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/mtd/nand.h>
+#include <linux/dmaengine.h>
+#include <linux/dma-mapping.h>
+#include <linux/platform_device.h>
+
+/* Offsets relative to chip->base */
+#define PBUS_CMD	0
+#define PBUS_ADDR	4
+#define PBUS_DATA	8
+
+/* Offsets relative to reg_base */
+#define NFC_STATUS	0x00
+#define NFC_FLASH_CMD	0x04
+#define NFC_DEVICE_CFG	0x08
+#define NFC_TIMING1	0x0c
+#define NFC_TIMING2	0x10
+#define NFC_XFER_CFG	0x14
+#define NFC_PKT_0_CFG	0x18
+#define NFC_PKT_N_CFG	0x1c
+#define NFC_BB_CFG	0x20
+#define NFC_ADDR_PAGE	0x24
+#define NFC_ADDR_OFFSET	0x28
+#define NFC_XFER_STATUS	0x2c
+
+/* NFC_STATUS values */
+#define CMD_READY	BIT(31)
+
+/* NFC_FLASH_CMD values */
+#define NFC_READ	1
+#define NFC_WRITE	2
+
+/* NFC_XFER_STATUS values */
+#define PAGE_IS_EMPTY	BIT(16)
+
+/* Offsets relative to mem_base */
+#define ERROR_REPORT	0x1c0
+
+/*
+ * Error reports are split in two bytes:
+ * byte 0 for the first packet in a page (PACKET_0)
+ * byte 1 for other packets in a page (PACKET_N, for N > 0)
+ * ERR_COUNT_PKT_N is the max error count over all but the first packet.
+ */
+#define DECODE_ERR_ON_PKT_0(v)	(~v & BIT(7))
+#define DECODE_ERR_ON_PKT_N(v)	(~v & BIT(15))
+#define ERR_COUNT_PKT_0(v)	((v >> 0) & 0x3f)
+#define ERR_COUNT_PKT_N(v)	((v >> 8) & 0x3f)
+
+/* Offsets relative to pbus_base */
+#define PBUS_CS_CTRL	0x83c
+#define PBUS_PAD_MODE	0x8f0
+
+/* PBUS_CS_CTRL values */
+#define PBUS_IORDY	BIT(31)
+
+/*
+ * PBUS_PAD_MODE values
+ * In raw mode, the driver communicates directly with the NAND chips.
+ * In NFC mode, the NAND Flash controller manages the communication.
+ * We use NFC mode for read and write; raw mode for everything else.
+ */
+#define MODE_RAW	0
+#define MODE_NFC	BIT(31)
+
+#define METADATA_SIZE	4
+#define BBM_SIZE	6
+#define FIELD_ORDER	15
+
+#define MAX_CS		4
+
+struct tango_nfc {
+	struct nand_hw_control hw;
+	void __iomem *reg_base;
+	void __iomem *mem_base;
+	void __iomem *pbus_base;
+	struct tango_chip *chips[MAX_CS];
+	struct dma_chan *chan;
+};
+
+#define to_tango_nfc(ptr) container_of(ptr, struct tango_nfc, hw)
+
+struct tango_chip {
+	struct nand_chip chip;
+	void __iomem *base;
+	u32 timing1;
+	u32 timing2;
+	u32 xfer_cfg;
+	u32 pkt_0_cfg;
+	u32 pkt_n_cfg;
+	u32 bb_cfg;
+	int cs;
+};
+
+#define to_tango_chip(ptr) container_of(ptr, struct tango_chip, chip)
+
+#define XFER_CFG(cs, page_count, steps, metadata_size)	\
+	((cs) << 24 | (page_count) << 16 | (steps) << 8 | (metadata_size) << 0)
+
+#define PKT_CFG(size, strength) ((size) << 16 | (strength) << 0)
+
+#define BB_CFG(bb_offset, bb_size) ((bb_offset) << 16 | (bb_size) << 0)
+
+#define TIMING(t0, t1, t2, t3) (t0 << 24 | t1 << 16 | t2 << 8 | t3 << 0)
+
+static void tango_cmd_ctrl(struct mtd_info *mtd, int dat, unsigned int ctrl)
+{
+	struct tango_chip *chip = to_tango_chip(mtd_to_nand(mtd));
+
+	if (ctrl & NAND_CLE)
+		writeb_relaxed(dat, chip->base + PBUS_CMD);
+
+	if (ctrl & NAND_ALE)
+		writeb_relaxed(dat, chip->base + PBUS_ADDR);
+}
+
+static int tango_dev_ready(struct mtd_info *mtd)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	struct tango_nfc *nfc = to_tango_nfc(chip->controller);
+
+	return readl_relaxed(nfc->pbus_base + PBUS_CS_CTRL) & PBUS_IORDY;
+}
+
+static uint8_t tango_read_byte(struct mtd_info *mtd)
+{
+	struct tango_chip *chip = to_tango_chip(mtd_to_nand(mtd));
+
+	return readb_relaxed(chip->base + PBUS_DATA);
+}
+
+static void tango_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
+{
+	struct tango_chip *chip = to_tango_chip(mtd_to_nand(mtd));
+
+	ioread8_rep(chip->base + PBUS_DATA, buf, len);
+}
+
+static void tango_write_buf(struct mtd_info *mtd, const uint8_t *buf, int len)
+{
+	struct tango_chip *chip = to_tango_chip(mtd_to_nand(mtd));
+
+	iowrite8_rep(chip->base + PBUS_DATA, buf, len);
+}
+
+static void tango_select_chip(struct mtd_info *mtd, int idx)
+{
+	struct nand_chip *nand = mtd_to_nand(mtd);
+	struct tango_nfc *nfc = to_tango_nfc(nand->controller);
+	struct tango_chip *chip = to_tango_chip(nand);
+
+	if (idx < 0) {
+		chip->base = NULL;
+		return;
+	}
+
+	chip->base = nfc->pbus_base + (chip->cs * 256);
+
+	writel_relaxed(chip->timing1,	nfc->reg_base + NFC_TIMING1);
+	writel_relaxed(chip->timing2,	nfc->reg_base + NFC_TIMING2);
+	writel_relaxed(chip->xfer_cfg,	nfc->reg_base + NFC_XFER_CFG);
+	writel_relaxed(chip->pkt_0_cfg,	nfc->reg_base + NFC_PKT_0_CFG);
+	writel_relaxed(chip->pkt_n_cfg,	nfc->reg_base + NFC_PKT_N_CFG);
+	writel_relaxed(chip->bb_cfg,	nfc->reg_base + NFC_BB_CFG);
+}
+
+static int decode_error_report(struct tango_nfc *nfc)
+{
+	u32 status, res;
+
+	status = readl_relaxed(nfc->reg_base + NFC_XFER_STATUS);
+	if (status & PAGE_IS_EMPTY)
+		return 0;
+
+	res = readl_relaxed(nfc->mem_base + ERROR_REPORT);
+
+	if (DECODE_ERR_ON_PKT_0(res) || DECODE_ERR_ON_PKT_N(res))
+		return -EBADMSG;
+
+	return max(ERR_COUNT_PKT_0(res), ERR_COUNT_PKT_N(res));
+}
+
+static void tango_dma_callback(void *arg)
+{
+	complete(arg);
+}
+
+static int do_dma(struct tango_nfc *nfc, int dir, int cmd, void *buf, int len, int page)
+{
+	struct dma_async_tx_descriptor *desc;
+	struct scatterlist sg;
+	struct completion tx_done;
+	int err = -EIO;
+	u32 res, val;
+
+	sg_init_one(&sg, buf, len);
+	if (dma_map_sg(nfc->chan->device->dev, &sg, 1, dir) != 1)
+		goto leave;
+
+	desc = dmaengine_prep_slave_sg(nfc->chan, &sg, 1, dir, DMA_PREP_INTERRUPT);
+	if (!desc)
+		goto dma_unmap;
+
+	desc->callback = tango_dma_callback;
+	desc->callback_param = &tx_done;
+	init_completion(&tx_done);
+
+	writel_relaxed(MODE_NFC, nfc->pbus_base + PBUS_PAD_MODE);
+
+	writel_relaxed(page, nfc->reg_base + NFC_ADDR_PAGE);
+	writel_relaxed(   0, nfc->reg_base + NFC_ADDR_OFFSET);
+	writel_relaxed( cmd, nfc->reg_base + NFC_FLASH_CMD);
+
+	dmaengine_submit(desc);
+	dma_async_issue_pending(nfc->chan);
+
+	res = wait_for_completion_timeout(&tx_done, HZ);
+	if (res > 0) {
+		void __iomem *addr = nfc->reg_base + NFC_STATUS;
+		err = readl_poll_timeout(addr, val, val & CMD_READY, 0, 1000);
+	}
+
+	writel_relaxed(MODE_RAW, nfc->pbus_base + PBUS_PAD_MODE);
+dma_unmap:
+	dma_unmap_sg(nfc->chan->device->dev, &sg, 1, dir);
+leave:
+	return err;
+}
+
+static int tango_read_page(struct mtd_info *mtd, struct nand_chip *chip,
+			uint8_t *buf, int oob_required, int page)
+{
+	struct tango_nfc *nfc = to_tango_nfc(chip->controller);
+	int err, len = mtd->writesize;
+
+	err = do_dma(nfc, DMA_FROM_DEVICE, NFC_READ, buf, len, page);
+	if (err)
+		return err;
+
+	return decode_error_report(nfc);
+}
+
+static int tango_write_page(struct mtd_info *mtd, struct nand_chip *chip,
+			const uint8_t *buf, int oob_required, int page)
+{
+	struct tango_nfc *nfc = to_tango_nfc(chip->controller);
+	int len = mtd->writesize;
+
+	return do_dma(nfc, DMA_TO_DEVICE, NFC_WRITE, (void *)buf, len, page);
+}
+
+static int tango_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
+		uint8_t *buf, int oob_required, int page)
+{
+	int pkt_size = chip->ecc.size;
+	int ecc_size = chip->ecc.bytes;
+	int lim = mtd->writesize - METADATA_SIZE;
+	uint8_t *oob = chip->oob_poi + BBM_SIZE + METADATA_SIZE;
+
+	tango_read_buf(mtd, chip->oob_poi + BBM_SIZE, METADATA_SIZE);
+
+	while (lim > pkt_size)
+	{
+		tango_read_buf(mtd, buf, pkt_size);
+		tango_read_buf(mtd, oob, ecc_size);
+
+		buf += pkt_size;
+		oob += ecc_size;
+		lim -= pkt_size + ecc_size;
+	}
+
+	tango_read_buf(mtd, buf, lim);
+	tango_read_buf(mtd, chip->oob_poi, BBM_SIZE);
+	tango_read_buf(mtd, buf + lim, pkt_size - lim);
+	tango_read_buf(mtd, oob, ecc_size);
+
+	return 0;
+}
+
+static int tango_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
+		const uint8_t *buf, int oob_required, int page)
+{
+	int pkt_size = chip->ecc.size;
+	int ecc_size = chip->ecc.bytes;
+	int lim = mtd->writesize - METADATA_SIZE;
+	uint8_t *oob = chip->oob_poi + BBM_SIZE + METADATA_SIZE;
+
+	tango_write_buf(mtd, chip->oob_poi + BBM_SIZE, METADATA_SIZE);
+
+	while (lim > pkt_size)
+	{
+		tango_write_buf(mtd, buf, pkt_size);
+		tango_write_buf(mtd, oob, ecc_size);
+
+		buf += pkt_size;
+		oob += ecc_size;
+		lim -= pkt_size + ecc_size;
+	}
+
+	tango_write_buf(mtd, buf, lim);
+	tango_write_buf(mtd, chip->oob_poi, BBM_SIZE);
+	tango_write_buf(mtd, buf + lim, pkt_size - lim);
+	tango_write_buf(mtd, oob, ecc_size);
+
+	return 0;
+}
+
+static u32 to_ticks(int kHz, int ps)
+{
+	return DIV_ROUND_UP_ULL((u64)kHz * ps, NSEC_PER_SEC);
+}
+
+static int set_timings(struct tango_chip *p, int kHz)
+{
+	u32 Trdy, Textw, Twc, Twpw, Tacc, Thold, Trpw, Textr;
+	const struct nand_sdr_timings *sdr;
+	int mode = onfi_get_async_timing_mode(&p->chip);
+
+	if (mode & ONFI_TIMING_MODE_UNKNOWN)
+		return -ENODEV;
+
+	sdr = onfi_async_timing_mode_to_sdr_timings(fls(mode) - 1);
+
+	Trdy	= to_ticks(kHz, sdr->tCEA_max - sdr->tREA_max);
+	Textw	= to_ticks(kHz, sdr->tWB_max);
+	Twc	= to_ticks(kHz, sdr->tWC_min);
+	Twpw	= to_ticks(kHz, sdr->tWC_min - sdr->tWP_min);
+
+	Tacc	= to_ticks(kHz, sdr->tREA_max);
+	Thold	= to_ticks(kHz, sdr->tREH_min);
+	Trpw	= to_ticks(kHz, sdr->tRC_min - sdr->tREH_min);
+	Textr	= to_ticks(kHz, sdr->tRHZ_max);
+
+	p->timing1 = TIMING(Trdy, Textw, Twc, Twpw);
+	p->timing2 = TIMING(Tacc, Thold, Trpw, Textr);
+
+	return 0;
+}
+
+static int chip_init(struct device *dev, struct device_node *np, int kHz)
+{
+	int err;
+	u32 cs, ecc_bits;
+	struct nand_ecc_ctrl *ecc;
+	struct tango_nfc *nfc = dev_get_drvdata(dev);
+	struct tango_chip *p = devm_kzalloc(dev, sizeof(*p), GFP_KERNEL);
+	if (!p)
+		return -ENOMEM;
+
+	err = of_property_read_u32_index(np, "reg", 0, &cs);
+	if (err)
+		return err;
+
+	if (cs >= MAX_CS)
+		return -ERANGE;
+
+	p->chip.read_byte	= tango_read_byte;
+	p->chip.read_buf	= tango_read_buf;
+	p->chip.select_chip	= tango_select_chip;
+	p->chip.cmd_ctrl	= tango_cmd_ctrl;
+	p->chip.dev_ready	= tango_dev_ready;
+	p->chip.options		= NAND_USE_BOUNCE_BUFFER;
+	p->chip.controller	= &nfc->hw;
+
+	ecc			= &p->chip.ecc;
+	ecc->mode		= NAND_ECC_HW;
+	ecc->algo		= NAND_ECC_BCH;
+	ecc->read_page_raw	= tango_read_page_raw;
+	ecc->write_page_raw	= tango_write_page_raw;
+	ecc->read_page		= tango_read_page;
+	ecc->write_page		= tango_write_page;
+
+	nand_set_flash_node(&p->chip, np);
+	err = nand_scan(&p->chip.mtd, 1);
+	if (err)
+		return err;
+
+	ecc_bits = p->chip.ecc.strength * FIELD_ORDER;
+	p->chip.ecc.bytes = DIV_ROUND_UP(ecc_bits, BITS_PER_BYTE);
+	set_timings(p, kHz);
+
+	err = mtd_device_register(&p->chip.mtd, NULL, 0);
+	if (err)
+		return err;
+
+	nfc->chips[cs] = p;
+
+	p->xfer_cfg	= XFER_CFG(cs, 1, ecc->steps, METADATA_SIZE);
+	p->pkt_0_cfg	= PKT_CFG(ecc->size + METADATA_SIZE, ecc->strength);
+	p->pkt_n_cfg	= PKT_CFG(ecc->size, ecc->strength);
+	p->bb_cfg	= BB_CFG(p->chip.mtd.writesize, BBM_SIZE);
+	p->cs		= cs;
+
+	return 0;
+}
+
+static int tango_nand_probe(struct platform_device *pdev)
+{
+	int kHz;
+	struct clk *clk;
+	struct resource *res;
+	struct tango_nfc *nfc;
+	struct device_node *np;
+
+	nfc = devm_kzalloc(&pdev->dev, sizeof(*nfc), GFP_KERNEL);
+	if (!nfc)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	nfc->reg_base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(nfc->reg_base))
+		return PTR_ERR(nfc->reg_base);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	nfc->mem_base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(nfc->mem_base))
+		return PTR_ERR(nfc->mem_base);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
+	nfc->pbus_base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(nfc->pbus_base))
+		return PTR_ERR(nfc->pbus_base);
+
+	nfc->chan = dma_request_chan(&pdev->dev, "nfc_sbox");
+	if (IS_ERR(nfc->chan))
+		return PTR_ERR(nfc->chan);
+
+	clk = clk_get(&pdev->dev, NULL);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	platform_set_drvdata(pdev, nfc);
+	nand_hw_control_init(&nfc->hw);
+	kHz = clk_get_rate(clk) / 1000;
+
+	for_each_child_of_node(pdev->dev.of_node, np) {
+		int err = chip_init(&pdev->dev, np, kHz);
+		if (err)
+			return err;
+	}
+
+	/* TODO: tweak (?) Peripheral Bus setup (timings and CS config) */
+
+	return 0;
+}
+
+static int tango_nand_remove(struct platform_device *pdev)
+{
+	int cs;
+	struct tango_nfc *nfc = platform_get_drvdata(pdev);
+
+	dma_release_channel(nfc->chan);
+	for (cs = 0; cs < MAX_CS; ++cs)
+		if (nfc->chips[cs] != NULL)
+			nand_release(&nfc->chips[cs]->chip.mtd);
+
+	return 0;
+}
+
+static const struct of_device_id tango_nand_ids[] = {
+	{ .compatible = "sigma,smp8758-nand" },
+	{ /* sentinel */ }
+};
+
+static struct platform_driver tango_nand_driver = {
+	.probe	= tango_nand_probe,
+	.remove	= tango_nand_remove,
+	.driver	= {
+		.name		= "tango-nand",
+		.of_match_table	= tango_nand_ids,
+	},
+};
+
+module_platform_driver(tango_nand_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Sigma Designs");
+MODULE_DESCRIPTION("Tango4 NAND Flash controller driver");
-- 
2.9.0

Re: [PATCH v3] mtd: nand: tango: import driver for tango chips

[Mason]

On 09/09/2016 18:13, Marc Gonzalez wrote:

> This driver supports the NAND Flash controller embedded in recent
> Tango chips, such as SMP8758 and SMP8759.
> 
> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> ---
>  drivers/mtd/nand/Kconfig      |   6 +
>  drivers/mtd/nand/Makefile     |   1 +
>  drivers/mtd/nand/tango_nand.c | 482 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 489 insertions(+)
>  create mode 100644 drivers/mtd/nand/tango_nand.c

Still on the TODO/MAYBE list:

1) Remove the cast in tango_write_page
2) Update stats.failed
3) Implement read/write_oob
4) Report OOB layout to framework (?)
5) Tweak (?) Peripheral Bus setup (timings and CS config)
6) Write device_cfg register (or report weirdness as a bug)

Regards.

Re: [PATCH v3] mtd: nand: tango: import driver for tango chips

[Boris Brezillon]

On Sun, 11 Sep 2016 14:50:19 +0200
Mason <slash.tmp@free.fr> wrote:

> On 09/09/2016 18:13, Marc Gonzalez wrote:
> 
> > This driver supports the NAND Flash controller embedded in recent
> > Tango chips, such as SMP8758 and SMP8759.
> > 
> > Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> > ---
> >  drivers/mtd/nand/Kconfig      |   6 +
> >  drivers/mtd/nand/Makefile     |   1 +
> >  drivers/mtd/nand/tango_nand.c | 482 ++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 489 insertions(+)
> >  create mode 100644 drivers/mtd/nand/tango_nand.c  
> 
> Still on the TODO/MAYBE list:
> 
> 1) Remove the cast in tango_write_page

Nice to have (especially since it's not required at all: do_dma() can
take a const void *)

> 2) Update stats.failed

Mandatory.

> 3) Implement read/write_oob

Mandatory. I've seen your questions on the #mtd chan regarding the
usefulness of ->read/write_oob(). I agree that most of the time it's
not needed, but some MTD users depends on it.

> 4) Report OOB layout to framework (?)

Mandatory. For the exact same reason.

> 5) Tweak (?) Peripheral Bus setup (timings and CS config)

That's up to you, but I'd recommend to sort this out before submitting
the next version.

> 6) Write device_cfg register (or report weirdness as a bug)

I don't get this one.

[PATCH v5] mtd: nand: tango: import driver for tango chips

[Marc Gonzalez]

This driver supports the NAND Flash controller embedded in recent
Tango chips, such as SMP8758 and SMP8759.

Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
Tests done to validate this driver:
  mtd_stresstest dev=1 count=500000
  mtd_nandbiterrs dev=1
  ubiattach -m 1 -d 3 && runtests.sh /dev/ubi3
---
 drivers/mtd/nand/Kconfig      |   6 +
 drivers/mtd/nand/Makefile     |   1 +
 drivers/mtd/nand/tango_nand.c | 555 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 562 insertions(+)
 create mode 100644 drivers/mtd/nand/tango_nand.c

diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index f05e0e9eb2f7..22eb5457c9f7 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -205,6 +205,12 @@ config MTD_NAND_S3C2410_CLKSTOP
 	  when the is NAND chip selected or released, but will save
 	  approximately 5mA of power when there is nothing happening.
 
+config MTD_NAND_TANGO
+	tristate "NAND Flash support for Tango chips"
+	depends on ARCH_TANGO
+	help
+	  Enables the NAND Flash controller on Tango chips.
+
 config MTD_NAND_DISKONCHIP
 	tristate "DiskOnChip 2000, Millennium and Millennium Plus (NAND reimplementation)"
 	depends on HAS_IOMEM
diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
index f55335373f7c..647f727223ef 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -16,6 +16,7 @@ obj-$(CONFIG_MTD_NAND_DENALI_DT)	+= denali_dt.o
 obj-$(CONFIG_MTD_NAND_AU1550)		+= au1550nd.o
 obj-$(CONFIG_MTD_NAND_BF5XX)		+= bf5xx_nand.o
 obj-$(CONFIG_MTD_NAND_S3C2410)		+= s3c2410.o
+obj-$(CONFIG_MTD_NAND_TANGO)		+= tango_nand.o
 obj-$(CONFIG_MTD_NAND_DAVINCI)		+= davinci_nand.o
 obj-$(CONFIG_MTD_NAND_DISKONCHIP)	+= diskonchip.o
 obj-$(CONFIG_MTD_NAND_DOCG4)		+= docg4.o
diff --git a/drivers/mtd/nand/tango_nand.c b/drivers/mtd/nand/tango_nand.c
new file mode 100644
index 000000000000..ec18cad546fb
--- /dev/null
+++ b/drivers/mtd/nand/tango_nand.c
@@ -0,0 +1,555 @@
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/clk.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/mtd/nand.h>
+#include <linux/dmaengine.h>
+#include <linux/dma-mapping.h>
+#include <linux/platform_device.h>
+
+/* Offsets relative to chip->base */
+#define PBUS_CMD	0
+#define PBUS_ADDR	4
+#define PBUS_DATA	8
+
+/* Offsets relative to reg_base */
+#define NFC_STATUS	0x00
+#define NFC_FLASH_CMD	0x04
+#define NFC_DEVICE_CFG	0x08
+#define NFC_TIMING1	0x0c
+#define NFC_TIMING2	0x10
+#define NFC_XFER_CFG	0x14
+#define NFC_PKT_0_CFG	0x18
+#define NFC_PKT_N_CFG	0x1c
+#define NFC_BB_CFG	0x20
+#define NFC_ADDR_PAGE	0x24
+#define NFC_ADDR_OFFSET	0x28
+#define NFC_XFER_STATUS	0x2c
+
+/* NFC_STATUS values */
+#define CMD_READY	BIT(31)
+
+/* NFC_FLASH_CMD values */
+#define NFC_READ	1
+#define NFC_WRITE	2
+
+/* NFC_XFER_STATUS values */
+#define PAGE_IS_EMPTY	BIT(16)
+
+/* Offsets relative to mem_base */
+#define METADATA	0x000
+#define ERROR_REPORT	0x1c0
+
+/*
+ * Error reports are split in two bytes:
+ * byte 0 for the first packet in a page (PKT_0)
+ * byte 1 for other packets in a page (PKT_N, for N > 0)
+ * ERR_COUNT_PKT_N is the max error count over all but the first packet.
+ */
+#define DECODE_OK_PKT_0(v)	(v & BIT(7))
+#define DECODE_OK_PKT_N(v)	(v & BIT(15))
+#define ERR_COUNT_PKT_0(v)	((v >> 0) & 0x3f)
+#define ERR_COUNT_PKT_N(v)	((v >> 8) & 0x3f)
+
+/* Offsets relative to pbus_base */
+#define PBUS_CS_CTRL	0x83c
+#define PBUS_PAD_MODE	0x8f0
+
+/* PBUS_CS_CTRL values */
+#define PBUS_IORDY	BIT(31)
+
+/*
+ * PBUS_PAD_MODE values
+ * In raw mode, the driver communicates directly with the NAND chips.
+ * In NFC mode, the NAND Flash controller manages the communication.
+ * We use NFC mode for read and write; raw mode for everything else.
+ */
+#define MODE_RAW	0
+#define MODE_NFC	BIT(31)
+
+#define METADATA_SIZE	4
+#define BBM_SIZE	6
+#define FIELD_ORDER	15
+
+#define MAX_CS		4
+
+struct tango_nfc {
+	struct nand_hw_control hw;
+	void __iomem *reg_base;
+	void __iomem *mem_base;
+	void __iomem *pbus_base;
+	struct tango_chip *chips[MAX_CS];
+	struct dma_chan *chan;
+};
+
+#define to_tango_nfc(ptr) container_of(ptr, struct tango_nfc, hw)
+
+struct tango_chip {
+	struct nand_chip chip;
+	void __iomem *base;
+	u32 timing1;
+	u32 timing2;
+	u32 xfer_cfg;
+	u32 pkt_0_cfg;
+	u32 pkt_n_cfg;
+	u32 bb_cfg;
+	int cs;
+};
+
+#define to_tango_chip(ptr) container_of(ptr, struct tango_chip, chip)
+
+#define XFER_CFG(cs, page_count, steps, metadata_size)	\
+	((cs) << 24 | (page_count) << 16 | (steps) << 8 | (metadata_size) << 0)
+
+#define PKT_CFG(size, strength) ((size) << 16 | (strength) << 0)
+
+#define BB_CFG(bb_offset, bb_size) ((bb_offset) << 16 | (bb_size) << 0)
+
+#define TIMING(t0, t1, t2, t3) (t0 << 24 | t1 << 16 | t2 << 8 | t3 << 0)
+
+static void tango_cmd_ctrl(struct mtd_info *mtd, int dat, unsigned int ctrl)
+{
+	struct tango_chip *chip = to_tango_chip(mtd_to_nand(mtd));
+
+	if (ctrl & NAND_CLE)
+		writeb_relaxed(dat, chip->base + PBUS_CMD);
+
+	if (ctrl & NAND_ALE)
+		writeb_relaxed(dat, chip->base + PBUS_ADDR);
+}
+
+static int tango_dev_ready(struct mtd_info *mtd)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	struct tango_nfc *nfc = to_tango_nfc(chip->controller);
+
+	return readl_relaxed(nfc->pbus_base + PBUS_CS_CTRL) & PBUS_IORDY;
+}
+
+static uint8_t tango_read_byte(struct mtd_info *mtd)
+{
+	struct tango_chip *chip = to_tango_chip(mtd_to_nand(mtd));
+
+	return readb_relaxed(chip->base + PBUS_DATA);
+}
+
+static void tango_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
+{
+	struct tango_chip *chip = to_tango_chip(mtd_to_nand(mtd));
+
+	ioread8_rep(chip->base + PBUS_DATA, buf, len);
+}
+
+static void tango_write_buf(struct mtd_info *mtd, const uint8_t *buf, int len)
+{
+	struct tango_chip *chip = to_tango_chip(mtd_to_nand(mtd));
+
+	iowrite8_rep(chip->base + PBUS_DATA, buf, len);
+}
+
+static void tango_select_chip(struct mtd_info *mtd, int idx)
+{
+	struct nand_chip *nand = mtd_to_nand(mtd);
+	struct tango_nfc *nfc = to_tango_nfc(nand->controller);
+	struct tango_chip *chip = to_tango_chip(nand);
+
+	if (idx < 0) {
+		chip->base = NULL;
+		return;
+	}
+
+	chip->base = nfc->pbus_base + (chip->cs * 256);
+
+	writel_relaxed(chip->timing1,	nfc->reg_base + NFC_TIMING1);
+	writel_relaxed(chip->timing2,	nfc->reg_base + NFC_TIMING2);
+	writel_relaxed(chip->xfer_cfg,	nfc->reg_base + NFC_XFER_CFG);
+	writel_relaxed(chip->pkt_0_cfg,	nfc->reg_base + NFC_PKT_0_CFG);
+	writel_relaxed(chip->pkt_n_cfg,	nfc->reg_base + NFC_PKT_N_CFG);
+	writel_relaxed(chip->bb_cfg,	nfc->reg_base + NFC_BB_CFG);
+}
+
+static int decode_error_report(struct tango_nfc *nfc)
+{
+	u32 status, res;
+
+	status = readl_relaxed(nfc->reg_base + NFC_XFER_STATUS);
+	if (status & PAGE_IS_EMPTY)
+		return 0;
+
+	res = readl_relaxed(nfc->mem_base + ERROR_REPORT);
+
+	if (DECODE_OK_PKT_0(res) && DECODE_OK_PKT_N(res))
+		return max(ERR_COUNT_PKT_0(res), ERR_COUNT_PKT_N(res));
+
+	return -EBADMSG;
+}
+
+static void tango_dma_callback(void *arg)
+{
+	complete(arg);
+}
+
+static int do_dma(struct tango_nfc *nfc, int dir, int cmd,
+		const void *buf, int len, int page)
+{
+	struct dma_async_tx_descriptor *desc;
+	struct scatterlist sg;
+	struct completion tx_done;
+	int err = -EIO;
+	u32 res, val;
+
+	sg_init_one(&sg, buf, len);
+	if (dma_map_sg(nfc->chan->device->dev, &sg, 1, dir) != 1)
+		goto leave;
+
+	desc = dmaengine_prep_slave_sg(nfc->chan, &sg, 1, dir, DMA_PREP_INTERRUPT);
+	if (!desc)
+		goto dma_unmap;
+
+	desc->callback = tango_dma_callback;
+	desc->callback_param = &tx_done;
+	init_completion(&tx_done);
+
+	writel_relaxed(MODE_NFC, nfc->pbus_base + PBUS_PAD_MODE);
+
+	writel_relaxed(page, nfc->reg_base + NFC_ADDR_PAGE);
+	writel_relaxed(   0, nfc->reg_base + NFC_ADDR_OFFSET);
+	writel_relaxed( cmd, nfc->reg_base + NFC_FLASH_CMD);
+
+	dmaengine_submit(desc);
+	dma_async_issue_pending(nfc->chan);
+
+	res = wait_for_completion_timeout(&tx_done, HZ);
+	if (res > 0) {
+		void __iomem *addr = nfc->reg_base + NFC_STATUS;
+		err = readl_poll_timeout(addr, val, val & CMD_READY, 0, 1000);
+	}
+
+	writel_relaxed(MODE_RAW, nfc->pbus_base + PBUS_PAD_MODE);
+dma_unmap:
+	dma_unmap_sg(nfc->chan->device->dev, &sg, 1, dir);
+leave:
+	return err;
+}
+
+static int tango_read_page(struct mtd_info *mtd, struct nand_chip *chip,
+		uint8_t *buf, int oob_required, int page)
+{
+	struct tango_nfc *nfc = to_tango_nfc(chip->controller);
+	int err, res, len = mtd->writesize;
+
+	err = do_dma(nfc, DMA_FROM_DEVICE, NFC_READ, buf, len, page);
+	if (err)
+		return err;
+
+	if (oob_required)
+		chip->ecc.read_oob(mtd, chip, page);
+
+	res = decode_error_report(nfc);
+	if (res >= 0)
+		return res;
+
+	chip->ecc.read_oob(mtd, chip, page);
+	res = nand_check_erased_ecc_chunk(buf, len, chip->oob_poi, mtd->oobsize,
+			NULL, 0, chip->ecc.strength);
+	if (res < 0)
+		mtd->ecc_stats.failed++;
+
+	return res;
+}
+
+static int tango_write_page(struct mtd_info *mtd, struct nand_chip *chip,
+		const uint8_t *buf, int oob_required, int page)
+{
+	struct tango_nfc *nfc = to_tango_nfc(chip->controller);
+	int err, len = mtd->writesize;
+
+	writel_relaxed(0xffffffff, nfc->mem_base + METADATA);
+	err = do_dma(nfc, DMA_TO_DEVICE, NFC_WRITE, buf, len, page);
+	if (err)
+		return err;
+
+	if (oob_required)
+		chip->ecc.write_oob(mtd, chip, page);
+
+	return 0;
+}
+
+enum { OP_SKIP, OP_READ, OP_WRITE };
+
+static void raw_aux(struct mtd_info *mtd, u8 **buf, int len, int op, int *lim)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	int pos = mtd->writesize - *lim;
+
+	if (op == OP_SKIP)
+		chip->cmdfunc(mtd, NAND_CMD_RNDOUT, pos + len, -1);
+	else if (op == OP_READ)
+		tango_read_buf(mtd, *buf, len);
+	else if (op == OP_WRITE)
+		tango_write_buf(mtd, *buf, len);
+
+	*lim -= len;
+	*buf += len;
+}
+
+static int raw_access(struct mtd_info *mtd, uint8_t *buf, uint8_t *oob, int op)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	int pkt_size = chip->ecc.size;
+	int ecc_size = chip->ecc.bytes;
+	int buf_op = buf ? op : OP_SKIP;
+	int oob_op = oob ? op : OP_SKIP;
+	int rem, lim = mtd->writesize;
+	u8 *oob_orig = oob;
+
+	oob += BBM_SIZE;
+	raw_aux(mtd, &oob, METADATA_SIZE, oob_op, &lim);
+
+	while (lim > pkt_size)
+	{
+		raw_aux(mtd, &buf, pkt_size, buf_op, &lim);
+		raw_aux(mtd, &oob, ecc_size, oob_op, &lim);
+	}
+
+	rem = pkt_size - lim;
+	raw_aux(mtd, &buf, lim, buf_op, &lim);
+	raw_aux(mtd, &oob_orig, BBM_SIZE, oob_op, &lim);
+	raw_aux(mtd, &buf, rem, buf_op, &lim);
+	raw_aux(mtd, &oob, ecc_size, oob_op, &lim);
+
+	return 0;
+}
+
+static int tango_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
+		uint8_t *buf, int oob_required, int page)
+{
+	return raw_access(mtd, buf, chip->oob_poi, OP_READ);
+}
+
+static int tango_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
+		const uint8_t *buf, int oob_required, int page)
+{
+	return raw_access(mtd, (void *)buf, chip->oob_poi, OP_WRITE);
+}
+
+static int tango_read_oob(struct mtd_info *mtd, struct nand_chip *chip, int page)
+{
+	chip->cmdfunc(mtd, NAND_CMD_READ0, 0, page);
+	return raw_access(mtd, NULL, chip->oob_poi, OP_READ);
+}
+
+static int tango_write_oob(struct mtd_info *mtd, struct nand_chip *chip, int page)
+{
+	chip->pagebuf = -1;
+	memset(chip->buffers->databuf, 0xff, mtd->writesize);
+	chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0, page);
+	raw_access(mtd, chip->buffers->databuf, chip->oob_poi, OP_WRITE);
+	chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
+	chip->waitfunc(mtd, chip);
+	return 0;
+}
+
+static int oob_ecc(struct mtd_info *mtd, int idx, struct mtd_oob_region *res)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	struct nand_ecc_ctrl *ecc = &chip->ecc;
+
+	if (idx >= ecc->steps)
+		return -ERANGE;
+
+	res->offset = BBM_SIZE + METADATA_SIZE + ecc->bytes * idx;
+	res->length = ecc->bytes;
+
+	return 0;
+}
+
+static int oob_free(struct mtd_info *mtd, int idx, struct mtd_oob_region *res)
+{
+	return -ERANGE; /* no free space in spare area */
+}
+
+static const struct mtd_ooblayout_ops tango_nand_ooblayout_ops = {
+	.ecc	= oob_ecc,
+	.free	= oob_free,
+};
+
+static u32 to_ticks(int kHz, int ps)
+{
+	return DIV_ROUND_UP_ULL((u64)kHz * ps, NSEC_PER_SEC);
+}
+
+static int set_timings(struct tango_chip *p, int kHz)
+{
+	u32 Trdy, Textw, Twc, Twpw, Tacc, Thold, Trpw, Textr;
+	const struct nand_sdr_timings *sdr;
+	int mode = onfi_get_async_timing_mode(&p->chip);
+
+	if (mode & ONFI_TIMING_MODE_UNKNOWN)
+		return -ENODEV;
+
+	sdr = onfi_async_timing_mode_to_sdr_timings(fls(mode) - 1);
+
+	Trdy	= to_ticks(kHz, sdr->tCEA_max - sdr->tREA_max);
+	Textw	= to_ticks(kHz, sdr->tWB_max);
+	Twc	= to_ticks(kHz, sdr->tWC_min);
+	Twpw	= to_ticks(kHz, sdr->tWC_min - sdr->tWP_min);
+
+	Tacc	= to_ticks(kHz, sdr->tREA_max);
+	Thold	= to_ticks(kHz, sdr->tREH_min);
+	Trpw	= to_ticks(kHz, sdr->tRC_min - sdr->tREH_min);
+	Textr	= to_ticks(kHz, sdr->tRHZ_max);
+
+	p->timing1 = TIMING(Trdy, Textw, Twc, Twpw);
+	p->timing2 = TIMING(Tacc, Thold, Trpw, Textr);
+
+	return 0;
+}
+
+static int chip_init(struct device *dev, struct device_node *np, int kHz)
+{
+	int err;
+	u32 cs, ecc_bits;
+	struct nand_ecc_ctrl *ecc;
+	struct tango_nfc *nfc = dev_get_drvdata(dev);
+	struct tango_chip *p = devm_kzalloc(dev, sizeof(*p), GFP_KERNEL);
+	if (!p)
+		return -ENOMEM;
+
+	err = of_property_read_u32_index(np, "reg", 0, &cs);
+	if (err)
+		return err;
+
+	if (cs >= MAX_CS)
+		return -ERANGE;
+
+	p->chip.read_byte	= tango_read_byte;
+	p->chip.read_buf	= tango_read_buf;
+	p->chip.select_chip	= tango_select_chip;
+	p->chip.cmd_ctrl	= tango_cmd_ctrl;
+	p->chip.dev_ready	= tango_dev_ready;
+	p->chip.options		= NAND_USE_BOUNCE_BUFFER | NAND_NO_SUBPAGE_WRITE;
+	p->chip.controller	= &nfc->hw;
+
+	ecc			= &p->chip.ecc;
+	ecc->mode		= NAND_ECC_HW;
+	ecc->algo		= NAND_ECC_BCH;
+	ecc->read_page_raw	= tango_read_page_raw;
+	ecc->write_page_raw	= tango_write_page_raw;
+	ecc->read_page		= tango_read_page;
+	ecc->write_page		= tango_write_page;
+	ecc->read_oob		= tango_read_oob;
+	ecc->write_oob		= tango_write_oob;
+
+	nand_set_flash_node(&p->chip, np);
+	mtd_set_ooblayout(&p->chip.mtd, &tango_nand_ooblayout_ops);
+	err = nand_scan_ident(&p->chip.mtd, 1, NULL);
+	if (err)
+		return err;
+
+	ecc_bits = ecc->strength * FIELD_ORDER;
+	p->chip.ecc.bytes = DIV_ROUND_UP(ecc_bits, BITS_PER_BYTE);
+	set_timings(p, kHz);
+
+	err = nand_scan_tail(&p->chip.mtd);
+	if (err)
+		return err;
+
+	err = mtd_device_register(&p->chip.mtd, NULL, 0);
+	if (err)
+		return err;
+
+	nfc->chips[cs] = p;
+
+	p->xfer_cfg	= XFER_CFG(cs, 1, ecc->steps, METADATA_SIZE);
+	p->pkt_0_cfg	= PKT_CFG(ecc->size + METADATA_SIZE, ecc->strength);
+	p->pkt_n_cfg	= PKT_CFG(ecc->size, ecc->strength);
+	p->bb_cfg	= BB_CFG(p->chip.mtd.writesize, BBM_SIZE);
+	p->cs		= cs;
+
+	return 0;
+}
+
+static int tango_nand_probe(struct platform_device *pdev)
+{
+	int kHz;
+	struct clk *clk;
+	struct resource *res;
+	struct tango_nfc *nfc;
+	struct device_node *np;
+
+	nfc = devm_kzalloc(&pdev->dev, sizeof(*nfc), GFP_KERNEL);
+	if (!nfc)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	nfc->reg_base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(nfc->reg_base))
+		return PTR_ERR(nfc->reg_base);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	nfc->mem_base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(nfc->mem_base))
+		return PTR_ERR(nfc->mem_base);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
+	nfc->pbus_base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(nfc->pbus_base))
+		return PTR_ERR(nfc->pbus_base);
+
+	nfc->chan = dma_request_chan(&pdev->dev, "nfc_sbox");
+	if (IS_ERR(nfc->chan))
+		return PTR_ERR(nfc->chan);
+
+	clk = clk_get(&pdev->dev, NULL);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	platform_set_drvdata(pdev, nfc);
+	nand_hw_control_init(&nfc->hw);
+	kHz = clk_get_rate(clk) / 1000;
+
+	for_each_child_of_node(pdev->dev.of_node, np) {
+		int err = chip_init(&pdev->dev, np, kHz);
+		if (err)
+			return err;
+	}
+
+	/* TODO: tweak (?) Peripheral Bus setup (timings and CS config) */
+
+	return 0;
+}
+
+static int tango_nand_remove(struct platform_device *pdev)
+{
+	int cs;
+	struct tango_nfc *nfc = platform_get_drvdata(pdev);
+
+	dma_release_channel(nfc->chan);
+	for (cs = 0; cs < MAX_CS; ++cs)
+		if (nfc->chips[cs] != NULL)
+			nand_release(&nfc->chips[cs]->chip.mtd);
+
+	return 0;
+}
+
+static const struct of_device_id tango_nand_ids[] = {
+	{ .compatible = "sigma,smp8758-nand" },
+	{ /* sentinel */ }
+};
+
+static struct platform_driver tango_nand_driver = {
+	.probe	= tango_nand_probe,
+	.remove	= tango_nand_remove,
+	.driver	= {
+		.name		= "tango-nand",
+		.of_match_table	= tango_nand_ids,
+	},
+};
+
+module_platform_driver(tango_nand_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Sigma Designs");
+MODULE_DESCRIPTION("Tango4 NAND Flash controller driver");
-- 
2.9.0

Re: [PATCH v5] mtd: nand: tango: import driver for tango chips

[Marc Gonzalez]

On 19/09/2016 15:12, Marc Gonzalez wrote:

> Tests done to validate this driver:
>   mtd_stresstest dev=1 count=500000

# time modprobe mtd_stresstest dev=1 count=500000
[ 5872.077247] 
[ 5872.078768] =================================================
[ 5872.084628] mtd_stresstest: MTD device: 1
[ 5872.088707] mtd_stresstest: MTD device size 536870912, eraseblock size 131072, page size 2048, count of eraseblocks 4096, pages per eraseblock 64, OOB size 64
[ 5872.104813] mtd_test: scanning for bad eraseblocks
[ 5872.113166] mtd_test: scanned 4096 eraseblocks, 0 are bad
[ 5872.118624] mtd_stresstest: doing operations
[ 5872.122941] mtd_stresstest: 0 operations done
[ 5883.636505] mtd_stresstest: 1024 operations done
[ 5894.626418] mtd_stresstest: 2048 operations done
[ 5904.911443] mtd_stresstest: 3072 operations done
...
[10740.794754] mtd_stresstest: 499712 operations done
[10743.412941] mtd_stresstest: finished, 500000 operations done
[10743.418765] =================================================

real    81m11.354s
user    0m0.000s
sys     51m46.666s


>   mtd_nandbiterrs dev=1

# modprobe mtd_nandbiterrs dev=1
[11713.669415] 
[11713.670936] ==================================================
[11713.677186] mtd_nandbiterrs: MTD device: 1
[11713.681442] mtd_nandbiterrs: MTD device size 536870912, eraseblock=131072, page=2048, oob=64
[11713.690045] mtd_nandbiterrs: Device uses 1 subpages of 2048 bytes
[11713.696249] mtd_nandbiterrs: Using page=0, offset=0, eraseblock=0
[11713.703241] mtd_nandbiterrs: incremental biterrors test
[11713.708608] mtd_nandbiterrs: write_page
[11713.712783] mtd_nandbiterrs: rewrite page
[11713.717164] mtd_nandbiterrs: read_page
[11713.721166] mtd_nandbiterrs: verify_page
[11713.725180] mtd_nandbiterrs: Successfully corrected 0 bit errors per subpage
[11713.732360] mtd_nandbiterrs: Inserted biterror @ 0/5
[11713.737403] mtd_nandbiterrs: rewrite page
[11713.741760] mtd_nandbiterrs: read_page
[11713.745749] mtd_nandbiterrs: verify_page
[11713.749805] mtd_nandbiterrs: Successfully corrected 1 bit errors per subpage
[11713.756947] mtd_nandbiterrs: Inserted biterror @ 0/2
[11713.761990] mtd_nandbiterrs: rewrite page
[11713.766340] mtd_nandbiterrs: read_page
[11713.770331] mtd_nandbiterrs: verify_page
[11713.774382] mtd_nandbiterrs: Successfully corrected 2 bit errors per subpage
[11713.781524] mtd_nandbiterrs: Inserted biterror @ 0/0
[11713.786565] mtd_nandbiterrs: rewrite page
[11713.790917] mtd_nandbiterrs: read_page
[11713.794881] mtd_nandbiterrs: verify_page
[11713.798887] mtd_nandbiterrs: Successfully corrected 3 bit errors per subpage
[11713.805985] mtd_nandbiterrs: Inserted biterror @ 1/7
[11713.810985] mtd_nandbiterrs: rewrite page
[11713.815299] mtd_nandbiterrs: read_page
[11713.819240] mtd_nandbiterrs: verify_page
[11713.823253] mtd_nandbiterrs: Successfully corrected 4 bit errors per subpage
[11713.830351] mtd_nandbiterrs: Inserted biterror @ 1/5
[11713.835350] mtd_nandbiterrs: rewrite page
[11713.839654] mtd_nandbiterrs: read_page
[11713.843601] mtd_nandbiterrs: verify_page
[11713.847608] mtd_nandbiterrs: Successfully corrected 5 bit errors per subpage
[11713.854705] mtd_nandbiterrs: Inserted biterror @ 1/2
[11713.859700] mtd_nandbiterrs: rewrite page
[11713.864018] mtd_nandbiterrs: read_page
[11713.867961] mtd_nandbiterrs: verify_page
[11713.871972] mtd_nandbiterrs: Successfully corrected 6 bit errors per subpage
[11713.879071] mtd_nandbiterrs: Inserted biterror @ 1/0
[11713.884069] mtd_nandbiterrs: rewrite page
[11713.888375] mtd_nandbiterrs: read_page
[11713.892321] mtd_nandbiterrs: verify_page
[11713.896327] mtd_nandbiterrs: Successfully corrected 7 bit errors per subpage
[11713.903424] mtd_nandbiterrs: Inserted biterror @ 2/6
[11713.908421] mtd_nandbiterrs: rewrite page
[11713.912731] mtd_nandbiterrs: read_page
[11713.916671] mtd_nandbiterrs: verify_page
[11713.920681] mtd_nandbiterrs: Successfully corrected 8 bit errors per subpage
[11713.927788] mtd_nandbiterrs: Inserted biterror @ 2/5
[11713.932787] mtd_nandbiterrs: rewrite page
[11713.937092] mtd_nandbiterrs: read_page
[11713.941038] mtd_nandbiterrs: verify_page
[11713.945046] mtd_nandbiterrs: Successfully corrected 9 bit errors per subpage
[11713.952143] mtd_nandbiterrs: Inserted biterror @ 2/2
[11713.957138] mtd_nandbiterrs: rewrite page
[11713.961448] mtd_nandbiterrs: read_page
[11713.965388] mtd_nandbiterrs: verify_page
[11713.969402] mtd_nandbiterrs: Successfully corrected 10 bit errors per subpage
[11713.976584] mtd_nandbiterrs: Inserted biterror @ 2/0
[11713.981582] mtd_nandbiterrs: rewrite page
[11713.985886] mtd_nandbiterrs: read_page
[11713.989832] mtd_nandbiterrs: verify_page
[11713.993837] mtd_nandbiterrs: Successfully corrected 11 bit errors per subpage
[11714.001022] mtd_nandbiterrs: Inserted biterror @ 3/7
[11714.006017] mtd_nandbiterrs: rewrite page
[11714.010352] mtd_nandbiterrs: read_page
[11714.014351] mtd_nandbiterrs: verify_page
[11714.018371] mtd_nandbiterrs: Successfully corrected 12 bit errors per subpage
[11714.025554] mtd_nandbiterrs: Inserted biterror @ 3/6
[11714.030557] mtd_nandbiterrs: rewrite page
[11714.034867] mtd_nandbiterrs: read_page
[11714.038817] mtd_nandbiterrs: verify_page
[11714.042827] mtd_nandbiterrs: Successfully corrected 13 bit errors per subpage
[11714.050012] mtd_nandbiterrs: Inserted biterror @ 3/5
[11714.055009] mtd_nandbiterrs: rewrite page
[11714.059361] mtd_nandbiterrs: read_page
[11714.063301] mtd_nandbiterrs: verify_page
[11714.067312] mtd_nandbiterrs: Successfully corrected 14 bit errors per subpage
[11714.074496] mtd_nandbiterrs: Inserted biterror @ 3/2
[11714.079495] mtd_nandbiterrs: rewrite page
[11714.083801] mtd_nandbiterrs: read_page
[11714.093403] mtd_nandbiterrs: error: read failed at 0x0
[11714.098578] mtd_nandbiterrs: After 15 biterrors per subpage, read reported error -74
[11714.106779] mtd_nandbiterrs: finished successfully.
[11714.111691] ==================================================


>   ubiattach -m 1 -d 3 && runtests.sh /dev/ubi3

# time ./runtests.sh /dev/ubi3
Running mkvol_basic /dev/ubi3
Running mkvol_bad /dev/ubi3
[ 2319.279975] ubi3 error: ubi_cdev_ioctl [ubi]: bad volume creation request
[ 2319.286844] Volume creation request dump:
[ 2319.291238]  vol_id    -2
[ 2319.294023]  alignment 1
[ 2319.296649]  bytes     509427712
[ 2319.299956]  vol_type  3
[ 2319.302498]  name_len  22
[ 2319.305216]  1st 16 characters of name: mkvol_bad:test_m
[ 2319.310672] ubi3 error: ubi_cdev_ioctl [ubi]: bad volume creation request
[ 2319.317571] Volume creation request dump:
[ 2319.321662]  vol_id    128
[ 2319.324440]  alignment 1
[ 2319.327042]  bytes     509427712
[ 2319.330341]  vol_type  3
[ 2319.332882]  name_len  22
[ 2319.335594]  1st 16 characters of name: mkvol_bad:test_m
[ 2319.341023] ubi3 error: ubi_cdev_ioctl [ubi]: bad volume creation request
[ 2319.347871] Volume creation request dump:
[ 2319.351912]  vol_id    0
[ 2319.354466]  alignment 0
[ 2319.357024]  bytes     509427712
[ 2319.360277]  vol_type  3
[ 2319.362825]  name_len  22
[ 2319.365464]  1st 16 characters of name: mkvol_bad:test_m
[ 2319.370837] ubi3 error: ubi_cdev_ioctl [ubi]: bad volume creation request
[ 2319.377677] Volume creation request dump:
[ 2319.381716]  vol_id    0
[ 2319.384270]  alignment -1
[ 2319.386910]  bytes     509427712
[ 2319.390163]  vol_type  3
[ 2319.392704]  name_len  22
[ 2319.395343]  1st 16 characters of name: mkvol_bad:test_m
[ 2319.400713] ubi3 error: ubi_cdev_ioctl [ubi]: bad volume creation request
[ 2319.407554] Volume creation request dump:
[ 2319.411593]  vol_id    0
[ 2319.414148]  alignment 126977
[ 2319.417139]  bytes     509427712
[ 2319.420393]  vol_type  3
[ 2319.422934]  name_len  22
[ 2319.425573]  1st 16 characters of name: mkvol_bad:test_m
[ 2319.430944] ubi3 error: ubi_cdev_ioctl [ubi]: bad volume creation request
[ 2319.437783] Volume creation request dump:
[ 2319.441822]  vol_id    0
[ 2319.444377]  alignment 2049
[ 2319.447193]  bytes     509427712
[ 2319.450446]  vol_type  3
[ 2319.452987]  name_len  22
[ 2319.455627]  1st 16 characters of name: mkvol_bad:test_m
[ 2319.460997] ubi3 error: ubi_cdev_ioctl [ubi]: bad volume creation request
[ 2319.467843] Volume creation request dump:
[ 2319.471883]  vol_id    0
[ 2319.474438]  alignment 1
[ 2319.476992]  bytes     -1
[ 2319.479620]  vol_type  3
[ 2319.482174]  name_len  22
[ 2319.484811]  1st 16 characters of name: mkvol_bad:test_m
[ 2319.490188] ubi3 error: ubi_cdev_ioctl [ubi]: bad volume creation request
[ 2319.497026] Volume creation request dump:
[ 2319.501066]  vol_id    0
[ 2319.503624]  alignment 1
[ 2319.506164]  bytes     0
[ 2319.508718]  vol_type  3
[ 2319.511273]  name_len  22
[ 2319.513912]  1st 16 characters of name: mkvol_bad:test_m
[ 2319.519288] ubi3 error: ubi_create_volume [ubi]: not enough PEBs, only 4012 available
[ 2319.527183] ubi3 error: ubi_create_volume [ubi]: cannot create volume 0, error -28
[ 2319.534830] ubi3 error: ubi_create_volume [ubi]: not enough PEBs, only 4012 available
[ 2319.542721] ubi3 error: ubi_create_volume [ubi]: cannot create volume 0, error -28
[ 2319.550365] ubi3 error: ubi_cdev_ioctl [ubi]: bad volume creation request
[ 2319.557205] Volume creation request dump:
[ 2319.561244]  vol_id    0
[ 2319.563800]  alignment 1
[ 2319.566345]  bytes     126976
[ 2319.569337]  vol_type  7
[ 2319.571891]  name_len  22
[ 2319.574531]  1st 16 characters of name: mkvol_bad:test_m
[ 2319.688559] ubi3 error: ubi_create_volume [ubi]: volume 0 already exists
[ 2319.695328] ubi3 error: ubi_create_volume [ubi]: cannot create volume 0, error -17
[ 2319.702987] ubi3 error: ubi_create_volume [ubi]: volume "mkvol_bad:test_mkvol()" exists (ID 0)
[ 2319.711676] ubi3 error: ubi_create_volume [ubi]: cannot create volume 1, error -17
[ 2319.936067] ubi3 error: ubi_create_volume [ubi]: volume "mkvol_bad:test_mkvol()" exists (ID 0)
[ 2319.944757] ubi3 error: ubi_create_volume [ubi]: cannot create volume 1, error -17
[ 2347.540770] ubi3 error: ubi_unregister_volume_notifier [ubi]: cannot open device 3, volume 128, error -22
[ 2347.550452] ubi3 error: ubi_unregister_volume_notifier [ubi]: cannot open device 3, volume -1, error -22
[ 2347.560458] ubi3 error: ubi_unregister_volume_notifier [ubi]: cannot open device 3, volume 128, error -22
[ 2347.570244] ubi3 error: ubi_unregister_volume_notifier [ubi]: cannot open device 3, volume 0, error -19
[ 2347.798164] ubi3 error: ubi_unregister_volume_notifier [ubi]: cannot open device 3, volume 0, error -19
Running mkvol_paral /dev/ubi3
Running rsvol /dev/ubi3
Running io_basic /dev/ubi3
Running io_read /dev/ubi3
Running io_update /dev/ubi3
Running io_paral /dev/ubi3
Running volrefcnt /dev/ubi3
SUCCESS

real    40m16.730s
user    11m33.653s
sys     20m57.187s



NB: Richard also suggested running a decent filesystem load using bonnie++

Re: [PATCH v5] mtd: nand: tango: import driver for tango chips

[Boris Brezillon]

Hi Marc,

On Mon, 19 Sep 2016 15:12:36 +0200
Marc Gonzalez <marc_gonzalez@sigmadesigns.com> wrote:

> This driver supports the NAND Flash controller embedded in recent
> Tango chips, such as SMP8758 and SMP8759.
> 

Please send another patch to document the DT bindings, and Cc the DT
maintainers.

> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> ---
> Tests done to validate this driver:
>   mtd_stresstest dev=1 count=500000
>   mtd_nandbiterrs dev=1
>   ubiattach -m 1 -d 3 && runtests.sh /dev/ubi3
> ---
>  drivers/mtd/nand/Kconfig      |   6 +
>  drivers/mtd/nand/Makefile     |   1 +
>  drivers/mtd/nand/tango_nand.c | 555 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 562 insertions(+)
>  create mode 100644 drivers/mtd/nand/tango_nand.c
> 
> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> index f05e0e9eb2f7..22eb5457c9f7 100644
> --- a/drivers/mtd/nand/Kconfig
> +++ b/drivers/mtd/nand/Kconfig
> @@ -205,6 +205,12 @@ config MTD_NAND_S3C2410_CLKSTOP
>  	  when the is NAND chip selected or released, but will save
>  	  approximately 5mA of power when there is nothing happening.
>  
> +config MTD_NAND_TANGO
> +	tristate "NAND Flash support for Tango chips"
> +	depends on ARCH_TANGO
> +	help
> +	  Enables the NAND Flash controller on Tango chips.
> +
>  config MTD_NAND_DISKONCHIP
>  	tristate "DiskOnChip 2000, Millennium and Millennium Plus (NAND reimplementation)"
>  	depends on HAS_IOMEM
> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
> index f55335373f7c..647f727223ef 100644
> --- a/drivers/mtd/nand/Makefile
> +++ b/drivers/mtd/nand/Makefile
> @@ -16,6 +16,7 @@ obj-$(CONFIG_MTD_NAND_DENALI_DT)	+= denali_dt.o
>  obj-$(CONFIG_MTD_NAND_AU1550)		+= au1550nd.o
>  obj-$(CONFIG_MTD_NAND_BF5XX)		+= bf5xx_nand.o
>  obj-$(CONFIG_MTD_NAND_S3C2410)		+= s3c2410.o
> +obj-$(CONFIG_MTD_NAND_TANGO)		+= tango_nand.o
>  obj-$(CONFIG_MTD_NAND_DAVINCI)		+= davinci_nand.o
>  obj-$(CONFIG_MTD_NAND_DISKONCHIP)	+= diskonchip.o
>  obj-$(CONFIG_MTD_NAND_DOCG4)		+= docg4.o
> diff --git a/drivers/mtd/nand/tango_nand.c b/drivers/mtd/nand/tango_nand.c
> new file mode 100644
> index 000000000000..ec18cad546fb
> --- /dev/null
> +++ b/drivers/mtd/nand/tango_nand.c
> @@ -0,0 +1,555 @@
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/clk.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/mtd/nand.h>
> +#include <linux/dmaengine.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/platform_device.h>
> +
> +/* Offsets relative to chip->base */
> +#define PBUS_CMD	0
> +#define PBUS_ADDR	4
> +#define PBUS_DATA	8
> +
> +/* Offsets relative to reg_base */
> +#define NFC_STATUS	0x00
> +#define NFC_FLASH_CMD	0x04
> +#define NFC_DEVICE_CFG	0x08
> +#define NFC_TIMING1	0x0c
> +#define NFC_TIMING2	0x10
> +#define NFC_XFER_CFG	0x14
> +#define NFC_PKT_0_CFG	0x18
> +#define NFC_PKT_N_CFG	0x1c
> +#define NFC_BB_CFG	0x20
> +#define NFC_ADDR_PAGE	0x24
> +#define NFC_ADDR_OFFSET	0x28
> +#define NFC_XFER_STATUS	0x2c
> +
> +/* NFC_STATUS values */
> +#define CMD_READY	BIT(31)
> +
> +/* NFC_FLASH_CMD values */
> +#define NFC_READ	1
> +#define NFC_WRITE	2
> +
> +/* NFC_XFER_STATUS values */
> +#define PAGE_IS_EMPTY	BIT(16)
> +
> +/* Offsets relative to mem_base */
> +#define METADATA	0x000
> +#define ERROR_REPORT	0x1c0
> +
> +/*
> + * Error reports are split in two bytes:
> + * byte 0 for the first packet in a page (PKT_0)
> + * byte 1 for other packets in a page (PKT_N, for N > 0)
> + * ERR_COUNT_PKT_N is the max error count over all but the first packet.
> + */
> +#define DECODE_OK_PKT_0(v)	(v & BIT(7))
> +#define DECODE_OK_PKT_N(v)	(v & BIT(15))
> +#define ERR_COUNT_PKT_0(v)	((v >> 0) & 0x3f)
> +#define ERR_COUNT_PKT_N(v)	((v >> 8) & 0x3f)
> +
> +/* Offsets relative to pbus_base */
> +#define PBUS_CS_CTRL	0x83c
> +#define PBUS_PAD_MODE	0x8f0
> +
> +/* PBUS_CS_CTRL values */
> +#define PBUS_IORDY	BIT(31)
> +
> +/*
> + * PBUS_PAD_MODE values
> + * In raw mode, the driver communicates directly with the NAND chips.
> + * In NFC mode, the NAND Flash controller manages the communication.
> + * We use NFC mode for read and write; raw mode for everything else.
> + */
> +#define MODE_RAW	0
> +#define MODE_NFC	BIT(31)
> +
> +#define METADATA_SIZE	4
> +#define BBM_SIZE	6
> +#define FIELD_ORDER	15
> +
> +#define MAX_CS		4
> +
> +struct tango_nfc {
> +	struct nand_hw_control hw;
> +	void __iomem *reg_base;
> +	void __iomem *mem_base;
> +	void __iomem *pbus_base;
> +	struct tango_chip *chips[MAX_CS];
> +	struct dma_chan *chan;
> +};
> +
> +#define to_tango_nfc(ptr) container_of(ptr, struct tango_nfc, hw)
> +
> +struct tango_chip {
> +	struct nand_chip chip;
> +	void __iomem *base;

I'm still not convinced this is needed (->base can be calculated where
needed based on the chip->cs and nfc->reg_base values), but if you want
to keep it, let's keep it.

> +	u32 timing1;
> +	u32 timing2;
> +	u32 xfer_cfg;
> +	u32 pkt_0_cfg;
> +	u32 pkt_n_cfg;
> +	u32 bb_cfg;
> +	int cs;
> +};
> +
> +#define to_tango_chip(ptr) container_of(ptr, struct tango_chip, chip)
> +
> +#define XFER_CFG(cs, page_count, steps, metadata_size)	\
> +	((cs) << 24 | (page_count) << 16 | (steps) << 8 | (metadata_size) << 0)
> +
> +#define PKT_CFG(size, strength) ((size) << 16 | (strength) << 0)
> +
> +#define BB_CFG(bb_offset, bb_size) ((bb_offset) << 16 | (bb_size) << 0)
> +
> +#define TIMING(t0, t1, t2, t3) (t0 << 24 | t1 << 16 | t2 << 8 | t3 << 0)
> +
> +static void tango_cmd_ctrl(struct mtd_info *mtd, int dat, unsigned int ctrl)
> +{
> +	struct tango_chip *chip = to_tango_chip(mtd_to_nand(mtd));
> +
> +	if (ctrl & NAND_CLE)
> +		writeb_relaxed(dat, chip->base + PBUS_CMD);
> +
> +	if (ctrl & NAND_ALE)
> +		writeb_relaxed(dat, chip->base + PBUS_ADDR);
> +}
> +
> +static int tango_dev_ready(struct mtd_info *mtd)
> +{
> +	struct nand_chip *chip = mtd_to_nand(mtd);
> +	struct tango_nfc *nfc = to_tango_nfc(chip->controller);
> +
> +	return readl_relaxed(nfc->pbus_base + PBUS_CS_CTRL) & PBUS_IORDY;
> +}
> +
> +static uint8_t tango_read_byte(struct mtd_info *mtd)
> +{
> +	struct tango_chip *chip = to_tango_chip(mtd_to_nand(mtd));
> +
> +	return readb_relaxed(chip->base + PBUS_DATA);
> +}
> +
> +static void tango_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
> +{
> +	struct tango_chip *chip = to_tango_chip(mtd_to_nand(mtd));
> +
> +	ioread8_rep(chip->base + PBUS_DATA, buf, len);
> +}
> +
> +static void tango_write_buf(struct mtd_info *mtd, const uint8_t *buf, int len)
> +{
> +	struct tango_chip *chip = to_tango_chip(mtd_to_nand(mtd));
> +
> +	iowrite8_rep(chip->base + PBUS_DATA, buf, len);
> +}
> +
> +static void tango_select_chip(struct mtd_info *mtd, int idx)
> +{
> +	struct nand_chip *nand = mtd_to_nand(mtd);
> +	struct tango_nfc *nfc = to_tango_nfc(nand->controller);
> +	struct tango_chip *chip = to_tango_chip(nand);
> +
> +	if (idx < 0) {
> +		chip->base = NULL;
> +		return;
> +	}
> +
> +	chip->base = nfc->pbus_base + (chip->cs * 256);

You support only one CS per chip, so this is something you can
configure at init time.

When I asked you to remove the chip->base field, I had multi-CS chips
in mind, but given this implementation, there's no need to set/reset
the chip->base field each time ->select_chip() is called.

> +
> +	writel_relaxed(chip->timing1,	nfc->reg_base + NFC_TIMING1);
> +	writel_relaxed(chip->timing2,	nfc->reg_base + NFC_TIMING2);
> +	writel_relaxed(chip->xfer_cfg,	nfc->reg_base + NFC_XFER_CFG);
> +	writel_relaxed(chip->pkt_0_cfg,	nfc->reg_base + NFC_PKT_0_CFG);
> +	writel_relaxed(chip->pkt_n_cfg,	nfc->reg_base + NFC_PKT_N_CFG);
> +	writel_relaxed(chip->bb_cfg,	nfc->reg_base + NFC_BB_CFG);

Nit: can you avoid these weird alignments. Use a single space after the
comma.

> +}
> +
> +static int decode_error_report(struct tango_nfc *nfc)
> +{
> +	u32 status, res;
> +
> +	status = readl_relaxed(nfc->reg_base + NFC_XFER_STATUS);
> +	if (status & PAGE_IS_EMPTY)
> +		return 0;
> +
> +	res = readl_relaxed(nfc->mem_base + ERROR_REPORT);
> +
> +	if (DECODE_OK_PKT_0(res) && DECODE_OK_PKT_N(res))
> +		return max(ERR_COUNT_PKT_0(res), ERR_COUNT_PKT_N(res));
> +
> +	return -EBADMSG;
> +}
> +
> +static void tango_dma_callback(void *arg)
> +{
> +	complete(arg);
> +}
> +
> +static int do_dma(struct tango_nfc *nfc, int dir, int cmd,
> +		const void *buf, int len, int page)
> +{
> +	struct dma_async_tx_descriptor *desc;
> +	struct scatterlist sg;
> +	struct completion tx_done;
> +	int err = -EIO;
> +	u32 res, val;
> +
> +	sg_init_one(&sg, buf, len);
> +	if (dma_map_sg(nfc->chan->device->dev, &sg, 1, dir) != 1)
> +		goto leave;

Return -EIO directly instead of creating this 'leave' label.

> +
> +	desc = dmaengine_prep_slave_sg(nfc->chan, &sg, 1, dir, DMA_PREP_INTERRUPT);

Put DMA_PREP_INTERRUPT on a second line to avoid >80 char lines.

> +	if (!desc)
> +		goto dma_unmap;
> +
> +	desc->callback = tango_dma_callback;
> +	desc->callback_param = &tx_done;
> +	init_completion(&tx_done);
> +
> +	writel_relaxed(MODE_NFC, nfc->pbus_base + PBUS_PAD_MODE);
> +
> +	writel_relaxed(page, nfc->reg_base + NFC_ADDR_PAGE);
> +	writel_relaxed(   0, nfc->reg_base + NFC_ADDR_OFFSET);
> +	writel_relaxed( cmd, nfc->reg_base + NFC_FLASH_CMD);
> +
> +	dmaengine_submit(desc);
> +	dma_async_issue_pending(nfc->chan);
> +
> +	res = wait_for_completion_timeout(&tx_done, HZ);
> +	if (res > 0) {
> +		void __iomem *addr = nfc->reg_base + NFC_STATUS;
> +		err = readl_poll_timeout(addr, val, val & CMD_READY, 0, 1000);

Why do you need this local variable?

		err = readl_poll_timeout(nfc->reg_base + NFC_STATUS,
					 val, val & CMD_READY, 0, 1000);

> +	}
> +
> +	writel_relaxed(MODE_RAW, nfc->pbus_base + PBUS_PAD_MODE);

Add an extra blank line here.

> +dma_unmap:
> +	dma_unmap_sg(nfc->chan->device->dev, &sg, 1, dir);
> +leave:
> +	return err;
> +}
> +
> +static int tango_read_page(struct mtd_info *mtd, struct nand_chip *chip,
> +		uint8_t *buf, int oob_required, int page)
> +{
> +	struct tango_nfc *nfc = to_tango_nfc(chip->controller);
> +	int err, res, len = mtd->writesize;
> +
> +	err = do_dma(nfc, DMA_FROM_DEVICE, NFC_READ, buf, len, page);
> +	if (err)
> +		return err;
> +
> +	if (oob_required)
> +		chip->ecc.read_oob(mtd, chip, page);
> +
> +	res = decode_error_report(nfc);
> +	if (res >= 0)
> +		return res;
> +
> +	chip->ecc.read_oob(mtd, chip, page);

There's no different in your case, but it should be read in raw mode.
How about calling ecc.read_oob_raw() so that you're safe even if you
want to differentiate the read_oob() and read_oob_raw() cases at some
point (IIUC, the METADATA section is ECC protected).

> +	res = nand_check_erased_ecc_chunk(buf, len, chip->oob_poi, mtd->oobsize,
> +			NULL, 0, chip->ecc.strength);

You should check each ECC packet/chunk independently, because ECC
strength is referring to an ECC packet not a full page.

> +	if (res < 0)
> +		mtd->ecc_stats.failed++;
> +
> +	return res;
> +}
> +
> +static int tango_write_page(struct mtd_info *mtd, struct nand_chip *chip,
> +		const uint8_t *buf, int oob_required, int page)
> +{
> +	struct tango_nfc *nfc = to_tango_nfc(chip->controller);
> +	int err, len = mtd->writesize;
> +
> +	writel_relaxed(0xffffffff, nfc->mem_base + METADATA);
> +	err = do_dma(nfc, DMA_TO_DEVICE, NFC_WRITE, buf, len, page);
> +	if (err)
> +		return err;
> +
> +	if (oob_required)
> +		chip->ecc.write_oob(mtd, chip, page);
> +
> +	return 0;
> +}
> +
> +enum { OP_SKIP, OP_READ, OP_WRITE };
> +
> +static void raw_aux(struct mtd_info *mtd, u8 **buf, int len, int op, int *lim)

Nit: please pass struct nand_chip *chip in first argument, and extract
the mtd using nand_to_mtd(). I know it's just a detail, but I'd like to
avoid passing mtd pointers, especially in internal functions.

> +{
> +	struct nand_chip *chip = mtd_to_nand(mtd);
> +	int pos = mtd->writesize - *lim;
> +
> +	if (op == OP_SKIP)
> +		chip->cmdfunc(mtd, NAND_CMD_RNDOUT, pos + len, -1);

Okay, we already discussed that one, and I already showed that this was
not working: NAND_CMD_RNDOUT is only valid for read accesses.

And I told you I'd like to avoid the const to non-const cast in write
accessors.

> +	else if (op == OP_READ)
> +		tango_read_buf(mtd, *buf, len);
> +	else if (op == OP_WRITE)
> +		tango_write_buf(mtd, *buf, len);
> +
> +	*lim -= len;
> +	*buf += len;
> +}

I'm not sure factorizing this code is really important, but since you
insist, how about doing the following instead:

enum tango_raw_op { OP_READ, OP_WRITE };

union tango_raw_buffer {
	void *in;
	const void *out;
};

struct tango_raw_access {
	enum tango_raw_op op;
	union tango_raw_buffer *buf;
	int pos;
};

static void raw_aux(struct nand_chip *chip,
		    struct tango_raw_access *info,
		    union tango_raw_buffer *buf, int len)
{
	struct mtd_info *mtd = nand_to_mtd(chip);

	if (!buf) {
		if (info->op == OP_READ)
			cmd = NAND_CMD_RNDOUT;
		else
			cmd = NAND_CMD_SEQIN;

		chip->cmdfunc(mtd, cmd, info->pos + len, -1);
	} else {
		if (info->op == OP_READ)
			tango_read_buf(mtd, buf->out, len);
		else
			tango_write_buf(mtd, buf->in, len);

		buf->in += len;
	}

	info->pos += len;
}

> +
> +static int raw_access(struct mtd_info *mtd, uint8_t *buf, uint8_t *oob, int op)
> +{
> +	struct nand_chip *chip = mtd_to_nand(mtd);
> +	int pkt_size = chip->ecc.size;
> +	int ecc_size = chip->ecc.bytes;
> +	int buf_op = buf ? op : OP_SKIP;
> +	int oob_op = oob ? op : OP_SKIP;
> +	int rem, lim = mtd->writesize;
> +	u8 *oob_orig = oob;
> +
> +	oob += BBM_SIZE;
> +	raw_aux(mtd, &oob, METADATA_SIZE, oob_op, &lim);
> +
> +	while (lim > pkt_size)
> +	{
> +		raw_aux(mtd, &buf, pkt_size, buf_op, &lim);
> +		raw_aux(mtd, &oob, ecc_size, oob_op, &lim);
> +	}
> +
> +	rem = pkt_size - lim;
> +	raw_aux(mtd, &buf, lim, buf_op, &lim);
> +	raw_aux(mtd, &oob_orig, BBM_SIZE, oob_op, &lim);
> +	raw_aux(mtd, &buf, rem, buf_op, &lim);
> +	raw_aux(mtd, &oob, ecc_size, oob_op, &lim);
> +
> +	return 0;
> +}

With the above changes this gives:

static int raw_access(struct nand_chip *chip,
		      union tango_raw_buffer *buf,
		      union tango_raw_buffer *oob,
		      int op)
{
	struct mtd_info *mtd = nand_to_mtd(chip);
	int pkt_size = chip->ecc.size;
	int ecc_size = chip->ecc.bytes;
	int steps = 0, rem = 0;
	struct tango_raw_access info = {
		.op = op,
		.pos = 0,
	};
	union tango_raw_buffer oob_orig;

	if (oob) {
		oob->in = chip->oob_poi + BBM_SIZE;
		oob_orig.in = chip->oob_poi;
	}

	raw_aux(chip, &info, oob, METADATA_SIZE);

	while (info.pos + pkt_size + ecc_size <= mtd->writesize) {
		raw_aux(chip, &info, buf, pkt_size);
		raw_aux(mtd, &info, oob, ecc_size);
		steps++;
	}

	if (steps < chip->ecc.steps)
		rem = pkt_size - (mtd->writesize - info.pos);
		raw_aux(mtd, &info, buf, mtd->writesize - info.pos);
	}

	raw_aux(mtd, &info, oob ? &oob_orig : NULL, BBM_SIZE);
	raw_aux(mtd, &info, buf, rem);

	rem = mtd->writesize + mtd->oobsize - info.pos;
	raw_aux(mtd, &info, oob, rem);

	return 0;
}

> +
> +static int tango_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
> +		uint8_t *buf, int oob_required, int page)
> +{
> +	return raw_access(mtd, buf, chip->oob_poi, OP_READ);
> +}

static int tango_read_page_raw(struct mtd_info *mtd,
			       struct nand_chip *chip, uint8_t *buf,
			       int oob_required, int page)
{
	union tango_raw_buffer data_buf = { .in = buf }
	union tango_raw_buffer oob_buf;

	return raw_access(mtd, data_buf, oob_required ? oob_buf : NULL,
			  OP_READ);
}


> +
> +static int tango_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
> +		const uint8_t *buf, int oob_required, int page)
> +{
> +	return raw_access(mtd, (void *)buf, chip->oob_poi, OP_WRITE);
> +}
> +
> +static int tango_read_oob(struct mtd_info *mtd, struct nand_chip *chip, int page)
> +{
> +	chip->cmdfunc(mtd, NAND_CMD_READ0, 0, page);
> +	return raw_access(mtd, NULL, chip->oob_poi, OP_READ);
> +}
> +
> +static int tango_write_oob(struct mtd_info *mtd, struct nand_chip *chip, int page)
> +{
> +	chip->pagebuf = -1;
> +	memset(chip->buffers->databuf, 0xff, mtd->writesize);
> +	chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0, page);
> +	raw_access(mtd, chip->buffers->databuf, chip->oob_poi, OP_WRITE);
> +	chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
> +	chip->waitfunc(mtd, chip);
> +	return 0;
> +}
> +
> +static int oob_ecc(struct mtd_info *mtd, int idx, struct mtd_oob_region *res)
> +{
> +	struct nand_chip *chip = mtd_to_nand(mtd);
> +	struct nand_ecc_ctrl *ecc = &chip->ecc;
> +
> +	if (idx >= ecc->steps)
> +		return -ERANGE;
> +
> +	res->offset = BBM_SIZE + METADATA_SIZE + ecc->bytes * idx;
> +	res->length = ecc->bytes;
> +
> +	return 0;
> +}
> +
> +static int oob_free(struct mtd_info *mtd, int idx, struct mtd_oob_region *res)
> +{
> +	return -ERANGE; /* no free space in spare area */
> +}
> +
> +static const struct mtd_ooblayout_ops tango_nand_ooblayout_ops = {
> +	.ecc	= oob_ecc,
> +	.free	= oob_free,
> +};
> +
> +static u32 to_ticks(int kHz, int ps)
> +{
> +	return DIV_ROUND_UP_ULL((u64)kHz * ps, NSEC_PER_SEC);
> +}
> +
> +static int set_timings(struct tango_chip *p, int kHz)
> +{
> +	u32 Trdy, Textw, Twc, Twpw, Tacc, Thold, Trpw, Textr;
> +	const struct nand_sdr_timings *sdr;
> +	int mode = onfi_get_async_timing_mode(&p->chip);
> +
> +	if (mode & ONFI_TIMING_MODE_UNKNOWN)
> +		return -ENODEV;
> +
> +	sdr = onfi_async_timing_mode_to_sdr_timings(fls(mode) - 1);

We recently introduced a generic interface to automate nand timings
configuration [1].
Can you make use of it (the patches are in my nand/next branch [2], you
can rebase your patch series on top of this branch).

> +
> +	Trdy	= to_ticks(kHz, sdr->tCEA_max - sdr->tREA_max);
> +	Textw	= to_ticks(kHz, sdr->tWB_max);
> +	Twc	= to_ticks(kHz, sdr->tWC_min);
> +	Twpw	= to_ticks(kHz, sdr->tWC_min - sdr->tWP_min);
> +
> +	Tacc	= to_ticks(kHz, sdr->tREA_max);
> +	Thold	= to_ticks(kHz, sdr->tREH_min);
> +	Trpw	= to_ticks(kHz, sdr->tRC_min - sdr->tREH_min);
> +	Textr	= to_ticks(kHz, sdr->tRHZ_max);

Can you please be consistent in your indentation?
You're using spaces before '=' almost everywhere except in a few
places (like here).

> +
> +	p->timing1 = TIMING(Trdy, Textw, Twc, Twpw);
> +	p->timing2 = TIMING(Tacc, Thold, Trpw, Textr);
> +
> +	return 0;
> +}
> +
> +static int chip_init(struct device *dev, struct device_node *np, int kHz)
> +{
> +	int err;
> +	u32 cs, ecc_bits;
> +	struct nand_ecc_ctrl *ecc;
> +	struct tango_nfc *nfc = dev_get_drvdata(dev);
> +	struct tango_chip *p = devm_kzalloc(dev, sizeof(*p), GFP_KERNEL);
> +	if (!p)
> +		return -ENOMEM;
> +
> +	err = of_property_read_u32_index(np, "reg", 0, &cs);
> +	if (err)
> +		return err;
> +

Can you please check that you only have a single value in reg? Just to
make sure the user doesn't try to define a multi-CS chip.

> +	if (cs >= MAX_CS)
> +		return -ERANGE;
> +
> +	p->chip.read_byte	= tango_read_byte;
> +	p->chip.read_buf	= tango_read_buf;
> +	p->chip.select_chip	= tango_select_chip;
> +	p->chip.cmd_ctrl	= tango_cmd_ctrl;
> +	p->chip.dev_ready	= tango_dev_ready;
> +	p->chip.options		= NAND_USE_BOUNCE_BUFFER | NAND_NO_SUBPAGE_WRITE;
> +	p->chip.controller	= &nfc->hw;

Again, be consistent in you coding style: use spaces.

> +
> +	ecc			= &p->chip.ecc;
> +	ecc->mode		= NAND_ECC_HW;
> +	ecc->algo		= NAND_ECC_BCH;
> +	ecc->read_page_raw	= tango_read_page_raw;
> +	ecc->write_page_raw	= tango_write_page_raw;
> +	ecc->read_page		= tango_read_page;
> +	ecc->write_page		= tango_write_page;
> +	ecc->read_oob		= tango_read_oob;
> +	ecc->write_oob		= tango_write_oob;

Can you move this part after nand_scan_ident(). I'd also suggest to
support software ECC (it's just taking a few more lines), but that's up
to you.

> +
> +	nand_set_flash_node(&p->chip, np);
> +	mtd_set_ooblayout(&p->chip.mtd, &tango_nand_ooblayout_ops);
> +	err = nand_scan_ident(&p->chip.mtd, 1, NULL);
> +	if (err)
> +		return err;
> +
> +	ecc_bits = ecc->strength * FIELD_ORDER;
> +	p->chip.ecc.bytes = DIV_ROUND_UP(ecc_bits, BITS_PER_BYTE);
> +	set_timings(p, kHz);

You won't need that one if you implement the
chip->setup_data_interface() method.

> +
> +	err = nand_scan_tail(&p->chip.mtd);
> +	if (err)
> +		return err;
> +
> +	err = mtd_device_register(&p->chip.mtd, NULL, 0);
> +	if (err)
> +		return err;
> +
> +	nfc->chips[cs] = p;
> +
> +	p->xfer_cfg	= XFER_CFG(cs, 1, ecc->steps, METADATA_SIZE);
> +	p->pkt_0_cfg	= PKT_CFG(ecc->size + METADATA_SIZE, ecc->strength);
> +	p->pkt_n_cfg	= PKT_CFG(ecc->size, ecc->strength);
> +	p->bb_cfg	= BB_CFG(p->chip.mtd.writesize, BBM_SIZE);

This should go before nand_scan_tail(), because, as soon as you call
mtd_device_register(), someone might use your NAND device.

> +	p->cs		= cs;

And this one should go before nand_scan_ident().

> +
> +	return 0;
> +}
> +
> +static int tango_nand_probe(struct platform_device *pdev)
> +{
> +	int kHz;
> +	struct clk *clk;
> +	struct resource *res;
> +	struct tango_nfc *nfc;
> +	struct device_node *np;
> +
> +	nfc = devm_kzalloc(&pdev->dev, sizeof(*nfc), GFP_KERNEL);
> +	if (!nfc)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	nfc->reg_base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(nfc->reg_base))
> +		return PTR_ERR(nfc->reg_base);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	nfc->mem_base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(nfc->mem_base))
> +		return PTR_ERR(nfc->mem_base);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> +	nfc->pbus_base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(nfc->pbus_base))
> +		return PTR_ERR(nfc->pbus_base);
> +
> +	nfc->chan = dma_request_chan(&pdev->dev, "nfc_sbox");
> +	if (IS_ERR(nfc->chan))
> +		return PTR_ERR(nfc->chan);
> +
> +	clk = clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);
> +
> +	platform_set_drvdata(pdev, nfc);
> +	nand_hw_control_init(&nfc->hw);
> +	kHz = clk_get_rate(clk) / 1000;
> +
> +	for_each_child_of_node(pdev->dev.of_node, np) {
> +		int err = chip_init(&pdev->dev, np, kHz);
> +		if (err)
> +			return err;

You should unregister all NAND/MTD devices before returning an error.

> +	}
> +
> +	/* TODO: tweak (?) Peripheral Bus setup (timings and CS config) */
> +
> +	return 0;
> +}
> +
> +static int tango_nand_remove(struct platform_device *pdev)
> +{
> +	int cs;
> +	struct tango_nfc *nfc = platform_get_drvdata(pdev);
> +
> +	dma_release_channel(nfc->chan);
> +	for (cs = 0; cs < MAX_CS; ++cs)
> +		if (nfc->chips[cs] != NULL)
> +			nand_release(&nfc->chips[cs]->chip.mtd);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id tango_nand_ids[] = {
> +	{ .compatible = "sigma,smp8758-nand" },
> +	{ /* sentinel */ }
> +};
> +
> +static struct platform_driver tango_nand_driver = {
> +	.probe	= tango_nand_probe,
> +	.remove	= tango_nand_remove,
> +	.driver	= {
> +		.name		= "tango-nand",
> +		.of_match_table	= tango_nand_ids,
> +	},
> +};
> +
> +module_platform_driver(tango_nand_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Sigma Designs");
> +MODULE_DESCRIPTION("Tango4 NAND Flash controller driver");

[1]http://www.spinics.net/lists/arm-kernel/msg530498.html
[2]https://github.com/linux-nand/linux/tree/nand/next

Re: [PATCH v5] mtd: nand: tango: import driver for tango chips

[Mason]

On 19/09/2016 19:06, Boris Brezillon wrote:

> Marc Gonzalez <marc_gonzalez@sigmadesigns.com> wrote:
> 
>> This driver supports the NAND Flash controller embedded in recent
>> Tango chips, such as SMP8758 and SMP8759.
> 
> Please send another patch to document the DT bindings, and Cc the DT
> maintainers.

OK.


>> +struct tango_chip {
>> +	struct nand_chip chip;
>> +	void __iomem *base;
> 
> I'm still not convinced this is needed (->base can be calculated where
> needed based on the chip->cs and nfc->reg_base values), but if you want
> to keep it, let's keep it.

I thought it was bad form to duplicate the code computing
chip->base, especially if I need to update that function.


>> +static void tango_select_chip(struct mtd_info *mtd, int idx)
>> +{
>> +	struct nand_chip *nand = mtd_to_nand(mtd);
>> +	struct tango_nfc *nfc = to_tango_nfc(nand->controller);
>> +	struct tango_chip *chip = to_tango_chip(nand);
>> +
>> +	if (idx < 0) {
>> +		chip->base = NULL;
>> +		return;
>> +	}
>> +
>> +	chip->base = nfc->pbus_base + (chip->cs * 256);
> 
> You support only one CS per chip, so this is something you can
> configure at init time.
> 
> When I asked you to remove the chip->base field, I had multi-CS chips
> in mind, but given this implementation, there's no need to set/reset
> the chip->base field each time ->select_chip() is called.

OK. I'll set chip->cs and chip->base in the probe function.
Just to be sure, I'll ask internally whether we want to
support multi-CS chips right now, or if this can be added
later on.


>> +	writel_relaxed(chip->timing1,	nfc->reg_base + NFC_TIMING1);
>> +	writel_relaxed(chip->timing2,	nfc->reg_base + NFC_TIMING2);
>> +	writel_relaxed(chip->xfer_cfg,	nfc->reg_base + NFC_XFER_CFG);
>> +	writel_relaxed(chip->pkt_0_cfg,	nfc->reg_base + NFC_PKT_0_CFG);
>> +	writel_relaxed(chip->pkt_n_cfg,	nfc->reg_base + NFC_PKT_N_CFG);
>> +	writel_relaxed(chip->bb_cfg,	nfc->reg_base + NFC_BB_CFG);
> 
> Nit: can you avoid these weird alignments. Use a single space after the
> comma.

I was trying to highlight the fact that all these writes are
within the reg_base area. I'll do as you ask, since you are
the gatekeeper.


>> +static int do_dma(struct tango_nfc *nfc, int dir, int cmd,
>> +		const void *buf, int len, int page)
>> +{
>> +	struct dma_async_tx_descriptor *desc;
>> +	struct scatterlist sg;
>> +	struct completion tx_done;
>> +	int err = -EIO;
>> +	u32 res, val;
>> +
>> +	sg_init_one(&sg, buf, len);
>> +	if (dma_map_sg(nfc->chan->device->dev, &sg, 1, dir) != 1)
>> +		goto leave;
> 
> Return -EIO directly instead of creating this 'leave' label.

OK. I wasn't sure it was good form to make the first test
special, especially if someone adds a new alloc before this
one later on. But I don't care either way.


>> +
>> +	desc = dmaengine_prep_slave_sg(nfc->chan, &sg, 1, dir, DMA_PREP_INTERRUPT);
> 
> Put DMA_PREP_INTERRUPT on a second line to avoid >80 char lines.

I was told some maintainers don't enforce the >80 char line rule.
Am I to understand you're not one of them? :-)


>> +	if (!desc)
>> +		goto dma_unmap;
>> +
>> +	desc->callback = tango_dma_callback;
>> +	desc->callback_param = &tx_done;
>> +	init_completion(&tx_done);
>> +
>> +	writel_relaxed(MODE_NFC, nfc->pbus_base + PBUS_PAD_MODE);
>> +
>> +	writel_relaxed(page, nfc->reg_base + NFC_ADDR_PAGE);
>> +	writel_relaxed(   0, nfc->reg_base + NFC_ADDR_OFFSET);
>> +	writel_relaxed( cmd, nfc->reg_base + NFC_FLASH_CMD);
>> +
>> +	dmaengine_submit(desc);
>> +	dma_async_issue_pending(nfc->chan);
>> +
>> +	res = wait_for_completion_timeout(&tx_done, HZ);
>> +	if (res > 0) {
>> +		void __iomem *addr = nfc->reg_base + NFC_STATUS;
>> +		err = readl_poll_timeout(addr, val, val & CMD_READY, 0, 1000);
> 
> Why do you need this local variable?
> 
> 		err = readl_poll_timeout(nfc->reg_base + NFC_STATUS,
> 					 val, val & CMD_READY, 0, 1000);

I find it hard to read when function arguments are split
over multiple lines. So if there is a hard "80 char" limit,
I prefer using temp variables for "long" arguments.


>> +	}
>> +
>> +	writel_relaxed(MODE_RAW, nfc->pbus_base + PBUS_PAD_MODE);
> 
> Add an extra blank line here.

OK.


>> +static int tango_read_page(struct mtd_info *mtd, struct nand_chip *chip,
>> +		uint8_t *buf, int oob_required, int page)
>> +{
>> +	struct tango_nfc *nfc = to_tango_nfc(chip->controller);
>> +	int err, res, len = mtd->writesize;
>> +
>> +	err = do_dma(nfc, DMA_FROM_DEVICE, NFC_READ, buf, len, page);
>> +	if (err)
>> +		return err;
>> +
>> +	if (oob_required)
>> +		chip->ecc.read_oob(mtd, chip, page);
>> +
>> +	res = decode_error_report(nfc);
>> +	if (res >= 0)
>> +		return res;
>> +
>> +	chip->ecc.read_oob(mtd, chip, page);
> 
> There's no different in your case, but it should be read in raw mode.
> How about calling ecc.read_oob_raw() so that you're safe even if you
> want to differentiate the read_oob() and read_oob_raw() cases at some
> point (IIUC, the METADATA section is ECC protected).

OK.


>> +	res = nand_check_erased_ecc_chunk(buf, len, chip->oob_poi, mtd->oobsize,
>> +			NULL, 0, chip->ecc.strength);
> 
> You should check each ECC packet/chunk independently, because ECC
> strength is referring to an ECC packet not a full page.

Because of our weird physical layout, accessing data chunks is
really awkward. We are willing to sacrifice a few bad pages
by using a sub-optimal check (allowing only "strength" bitflips
over the entire page, instead of over individual packets).


>> +static int tango_write_page(struct mtd_info *mtd, struct nand_chip *chip,
>> +		const uint8_t *buf, int oob_required, int page)
>> +{
>> +	struct tango_nfc *nfc = to_tango_nfc(chip->controller);
>> +	int err, len = mtd->writesize;
>> +
>> +	writel_relaxed(0xffffffff, nfc->mem_base + METADATA);
>> +	err = do_dma(nfc, DMA_TO_DEVICE, NFC_WRITE, buf, len, page);
>> +	if (err)
>> +		return err;
>> +
>> +	if (oob_required)
>> +		chip->ecc.write_oob(mtd, chip, page);

I suppose we should use write_oob_raw here?


>> +enum { OP_SKIP, OP_READ, OP_WRITE };
>> +
>> +static void raw_aux(struct mtd_info *mtd, u8 **buf, int len, int op, int *lim)
> 
> Nit: please pass struct nand_chip *chip in first argument, and extract
> the mtd using nand_to_mtd(). I know it's just a detail, but I'd like to
> avoid passing mtd pointers, especially in internal functions.

OK.


>> +{
>> +	struct nand_chip *chip = mtd_to_nand(mtd);
>> +	int pos = mtd->writesize - *lim;
>> +
>> +	if (op == OP_SKIP)
>> +		chip->cmdfunc(mtd, NAND_CMD_RNDOUT, pos + len, -1);
> 
> Okay, we already discussed that one, and I already showed that this was
> not working: NAND_CMD_RNDOUT is only valid for read accesses.
> 
> And I told you I'd like to avoid the const to non-const cast in write
> accessors.

Note: I don't use SKIP for writes, therefore there's no need
to implement that code.


>> +	else if (op == OP_READ)
>> +		tango_read_buf(mtd, *buf, len);
>> +	else if (op == OP_WRITE)
>> +		tango_write_buf(mtd, *buf, len);
>> +
>> +	*lim -= len;
>> +	*buf += len;
>> +}
> 
> I'm not sure factorizing this code is really important, but since you
> insist, how about doing the following instead:

If the code is not factorized, then it must be duplicated for
read/write_raw and read/write_oob, I think (i.e. 4 times).

> enum tango_raw_op { OP_READ, OP_WRITE };
> 
> union tango_raw_buffer {
> 	void *in;
> 	const void *out;
> };
> 
> struct tango_raw_access {
> 	enum tango_raw_op op;
> 	union tango_raw_buffer *buf;
> 	int pos;
> };
> 
> static void raw_aux(struct nand_chip *chip,
> 		    struct tango_raw_access *info,
> 		    union tango_raw_buffer *buf, int len)
> {
> 	struct mtd_info *mtd = nand_to_mtd(chip);
> 
> 	if (!buf) {
> 		if (info->op == OP_READ)
> 			cmd = NAND_CMD_RNDOUT;
> 		else
> 			cmd = NAND_CMD_SEQIN;
> 
> 		chip->cmdfunc(mtd, cmd, info->pos + len, -1);
> 	} else {
> 		if (info->op == OP_READ)
> 			tango_read_buf(mtd, buf->out, len);
> 		else
> 			tango_write_buf(mtd, buf->in, len);
> 
> 		buf->in += len;
> 	}
> 
> 	info->pos += len;
> }

I'll take a closer look tomorrow.


>> +static int raw_access(struct mtd_info *mtd, uint8_t *buf, uint8_t *oob, int op)
>> +{
>> +	struct nand_chip *chip = mtd_to_nand(mtd);
>> +	int pkt_size = chip->ecc.size;
>> +	int ecc_size = chip->ecc.bytes;
>> +	int buf_op = buf ? op : OP_SKIP;
>> +	int oob_op = oob ? op : OP_SKIP;
>> +	int rem, lim = mtd->writesize;
>> +	u8 *oob_orig = oob;
>> +
>> +	oob += BBM_SIZE;
>> +	raw_aux(mtd, &oob, METADATA_SIZE, oob_op, &lim);
>> +
>> +	while (lim > pkt_size)
>> +	{
>> +		raw_aux(mtd, &buf, pkt_size, buf_op, &lim);
>> +		raw_aux(mtd, &oob, ecc_size, oob_op, &lim);
>> +	}
>> +
>> +	rem = pkt_size - lim;
>> +	raw_aux(mtd, &buf, lim, buf_op, &lim);
>> +	raw_aux(mtd, &oob_orig, BBM_SIZE, oob_op, &lim);
>> +	raw_aux(mtd, &buf, rem, buf_op, &lim);
>> +	raw_aux(mtd, &oob, ecc_size, oob_op, &lim);
>> +
>> +	return 0;
>> +}
> 
> With the above changes this gives:
> 
> static int raw_access(struct nand_chip *chip,
> 		      union tango_raw_buffer *buf,
> 		      union tango_raw_buffer *oob,
> 		      int op)
> {
> 	struct mtd_info *mtd = nand_to_mtd(chip);
> 	int pkt_size = chip->ecc.size;
> 	int ecc_size = chip->ecc.bytes;
> 	int steps = 0, rem = 0;
> 	struct tango_raw_access info = {
> 		.op = op,
> 		.pos = 0,
> 	};
> 	union tango_raw_buffer oob_orig;
> 
> 	if (oob) {
> 		oob->in = chip->oob_poi + BBM_SIZE;
> 		oob_orig.in = chip->oob_poi;
> 	}
> 
> 	raw_aux(chip, &info, oob, METADATA_SIZE);
> 
> 	while (info.pos + pkt_size + ecc_size <= mtd->writesize) {
> 		raw_aux(chip, &info, buf, pkt_size);
> 		raw_aux(mtd, &info, oob, ecc_size);
> 		steps++;
> 	}
> 
> 	if (steps < chip->ecc.steps)
> 		rem = pkt_size - (mtd->writesize - info.pos);
> 		raw_aux(mtd, &info, buf, mtd->writesize - info.pos);
> 	}

I don't think we need the 'steps' variable (I didn't need one).

> 	raw_aux(mtd, &info, oob ? &oob_orig : NULL, BBM_SIZE);
> 	raw_aux(mtd, &info, buf, rem);
> 
> 	rem = mtd->writesize + mtd->oobsize - info.pos;
> 	raw_aux(mtd, &info, oob, rem);
> 
> 	return 0;
> }

I'll take a closer look tomorrow.


>> +static u32 to_ticks(int kHz, int ps)
>> +{
>> +	return DIV_ROUND_UP_ULL((u64)kHz * ps, NSEC_PER_SEC);
>> +}
>> +
>> +static int set_timings(struct tango_chip *p, int kHz)
>> +{
>> +	u32 Trdy, Textw, Twc, Twpw, Tacc, Thold, Trpw, Textr;
>> +	const struct nand_sdr_timings *sdr;
>> +	int mode = onfi_get_async_timing_mode(&p->chip);
>> +
>> +	if (mode & ONFI_TIMING_MODE_UNKNOWN)
>> +		return -ENODEV;
>> +
>> +	sdr = onfi_async_timing_mode_to_sdr_timings(fls(mode) - 1);
> 
> We recently introduced a generic interface to automate nand timings
> configuration [1].
> Can you make use of it (the patches are in my nand/next branch [2], you
> can rebase your patch series on top of this branch).

I'll take a look. I still need to have this driver working
on 4.7 for the time being...


>> +
>> +	Trdy	= to_ticks(kHz, sdr->tCEA_max - sdr->tREA_max);
>> +	Textw	= to_ticks(kHz, sdr->tWB_max);
>> +	Twc	= to_ticks(kHz, sdr->tWC_min);
>> +	Twpw	= to_ticks(kHz, sdr->tWC_min - sdr->tWP_min);
>> +
>> +	Tacc	= to_ticks(kHz, sdr->tREA_max);
>> +	Thold	= to_ticks(kHz, sdr->tREH_min);
>> +	Trpw	= to_ticks(kHz, sdr->tRC_min - sdr->tREH_min);
>> +	Textr	= to_ticks(kHz, sdr->tRHZ_max);
> 
> Can you please be consistent in your indentation?
> You're using spaces before '=' almost everywhere except in a few
> places (like here).

When initializing fields of a struct, most code I've seen
uses tabs to align the equal signs. I thought it should be
the same when assigning the fields of a struct, or assigning
several variables of the same nature (such as above).

Are you saying you prefer

  Trdy = to_ticks(kHz, sdr->tCEA_max - sdr->tREA_max);
  Textw = to_ticks(kHz, sdr->tWB_max);
  Twc = to_ticks(kHz, sdr->tWC_min);

Thus the to_ticks calls are not aligned, and it's less obvious
that the first argument is always the same.


>> +static int chip_init(struct device *dev, struct device_node *np, int kHz)
>> +{
>> +	int err;
>> +	u32 cs, ecc_bits;
>> +	struct nand_ecc_ctrl *ecc;
>> +	struct tango_nfc *nfc = dev_get_drvdata(dev);
>> +	struct tango_chip *p = devm_kzalloc(dev, sizeof(*p), GFP_KERNEL);
>> +	if (!p)
>> +		return -ENOMEM;
>> +
>> +	err = of_property_read_u32_index(np, "reg", 0, &cs);
>> +	if (err)
>> +		return err;
>> +
> 
> Can you please check that you only have a single value in reg? Just to
> make sure the user doesn't try to define a multi-CS chip.

OK.


>> +	if (cs >= MAX_CS)
>> +		return -ERANGE;
>> +
>> +	p->chip.read_byte	= tango_read_byte;
>> +	p->chip.read_buf	= tango_read_buf;
>> +	p->chip.select_chip	= tango_select_chip;
>> +	p->chip.cmd_ctrl	= tango_cmd_ctrl;
>> +	p->chip.dev_ready	= tango_dev_ready;
>> +	p->chip.options		= NAND_USE_BOUNCE_BUFFER | NAND_NO_SUBPAGE_WRITE;
>> +	p->chip.controller	= &nfc->hw;
> 
> Again, be consistent in you coding style: use spaces.

OK. I'll take another look at the sunxi driver.

It makes no sense to me that we should tab-align when using
struct initialization with named fields, but space-align
when using assignment.


>> +
>> +	ecc			= &p->chip.ecc;
>> +	ecc->mode		= NAND_ECC_HW;
>> +	ecc->algo		= NAND_ECC_BCH;
>> +	ecc->read_page_raw	= tango_read_page_raw;
>> +	ecc->write_page_raw	= tango_write_page_raw;
>> +	ecc->read_page		= tango_read_page;
>> +	ecc->write_page		= tango_write_page;
>> +	ecc->read_oob		= tango_read_oob;
>> +	ecc->write_oob		= tango_write_oob;
> 
> Can you move this part after nand_scan_ident(). I'd also suggest to
> support software ECC (it's just taking a few more lines), but that's up
> to you.

OK. What does it mean to support SW ECC?

>> +
>> +	nand_set_flash_node(&p->chip, np);
>> +	mtd_set_ooblayout(&p->chip.mtd, &tango_nand_ooblayout_ops);
>> +	err = nand_scan_ident(&p->chip.mtd, 1, NULL);
>> +	if (err)
>> +		return err;
>> +
>> +	ecc_bits = ecc->strength * FIELD_ORDER;
>> +	p->chip.ecc.bytes = DIV_ROUND_UP(ecc_bits, BITS_PER_BYTE);
>> +	set_timings(p, kHz);
> 
> You won't need that one if you implement the
> chip->setup_data_interface() method.

I'll take a closer look.


>> +	p->xfer_cfg	= XFER_CFG(cs, 1, ecc->steps, METADATA_SIZE);
>> +	p->pkt_0_cfg	= PKT_CFG(ecc->size + METADATA_SIZE, ecc->strength);
>> +	p->pkt_n_cfg	= PKT_CFG(ecc->size, ecc->strength);
>> +	p->bb_cfg	= BB_CFG(p->chip.mtd.writesize, BBM_SIZE);
> 
> This should go before nand_scan_tail(), because, as soon as you call
> mtd_device_register(), someone might use your NAND device.
> 
>> +	p->cs		= cs;
> 
> And this one should go before nand_scan_ident().

OK to both.


>> +	for_each_child_of_node(pdev->dev.of_node, np) {
>> +		int err = chip_init(&pdev->dev, np, kHz);
>> +		if (err)
>> +			return err;
> 
> You should unregister all NAND/MTD devices before returning an error.

Good catch. I was used to devm_* wrappers for everything in
some frameworks. I'll call tango_nand_remove on error.

Thanks for the detailed review.

Regards.

Re: [PATCH v5] mtd: nand: tango: import driver for tango chips

[Boris Brezillon]

On Tue, 20 Sep 2016 00:37:06 +0200
Mason <slash.tmp@free.fr> wrote:

> On 19/09/2016 19:06, Boris Brezillon wrote:
> 
> > Marc Gonzalez <marc_gonzalez@sigmadesigns.com> wrote:
> >   
> >> This driver supports the NAND Flash controller embedded in recent
> >> Tango chips, such as SMP8758 and SMP8759.  
> > 
> > Please send another patch to document the DT bindings, and Cc the DT
> > maintainers.  
> 
> OK.
> 
> 
> >> +struct tango_chip {
> >> +	struct nand_chip chip;
> >> +	void __iomem *base;  
> > 
> > I'm still not convinced this is needed (->base can be calculated where
> > needed based on the chip->cs and nfc->reg_base values), but if you want
> > to keep it, let's keep it.  
> 
> I thought it was bad form to duplicate the code computing
> chip->base, especially if I need to update that function.

I simple macro would avoid the code duplication:

#define TANGO_CHIP_REG(nfc, cs, reg)	\
	((nfc)->pbus_base + ((cs) *256) + reg)

> 
> 
> >> +static void tango_select_chip(struct mtd_info *mtd, int idx)
> >> +{
> >> +	struct nand_chip *nand = mtd_to_nand(mtd);
> >> +	struct tango_nfc *nfc = to_tango_nfc(nand->controller);
> >> +	struct tango_chip *chip = to_tango_chip(nand);
> >> +
> >> +	if (idx < 0) {
> >> +		chip->base = NULL;
> >> +		return;
> >> +	}
> >> +
> >> +	chip->base = nfc->pbus_base + (chip->cs * 256);  
> > 
> > You support only one CS per chip, so this is something you can
> > configure at init time.
> > 
> > When I asked you to remove the chip->base field, I had multi-CS chips
> > in mind, but given this implementation, there's no need to set/reset
> > the chip->base field each time ->select_chip() is called.  
> 
> OK. I'll set chip->cs and chip->base in the probe function.
> Just to be sure, I'll ask internally whether we want to
> support multi-CS chips right now, or if this can be added
> later on.
> 
> 
> >> +	writel_relaxed(chip->timing1,	nfc->reg_base + NFC_TIMING1);
> >> +	writel_relaxed(chip->timing2,	nfc->reg_base + NFC_TIMING2);
> >> +	writel_relaxed(chip->xfer_cfg,	nfc->reg_base + NFC_XFER_CFG);
> >> +	writel_relaxed(chip->pkt_0_cfg,	nfc->reg_base + NFC_PKT_0_CFG);
> >> +	writel_relaxed(chip->pkt_n_cfg,	nfc->reg_base + NFC_PKT_N_CFG);
> >> +	writel_relaxed(chip->bb_cfg,	nfc->reg_base + NFC_BB_CFG);  
> > 
> > Nit: can you avoid these weird alignments. Use a single space after the
> > comma.  
> 
> I was trying to highlight the fact that all these writes are
> within the reg_base area. I'll do as you ask, since you are
> the gatekeeper.

Is that really less clear without the tabs?

> 
> 
> >> +static int do_dma(struct tango_nfc *nfc, int dir, int cmd,
> >> +		const void *buf, int len, int page)
> >> +{
> >> +	struct dma_async_tx_descriptor *desc;
> >> +	struct scatterlist sg;
> >> +	struct completion tx_done;
> >> +	int err = -EIO;
> >> +	u32 res, val;
> >> +
> >> +	sg_init_one(&sg, buf, len);
> >> +	if (dma_map_sg(nfc->chan->device->dev, &sg, 1, dir) != 1)
> >> +		goto leave;  
> > 
> > Return -EIO directly instead of creating this 'leave' label.  
> 
> OK. I wasn't sure it was good form to make the first test
> special, especially if someone adds a new alloc before this
> one later on. But I don't care either way.

If one adds an allocation, it's likely to be put after this call, and
if it's not, we'll patch the code and create a new exit patch.

> 
> 
> >> +
> >> +	desc = dmaengine_prep_slave_sg(nfc->chan, &sg, 1, dir, DMA_PREP_INTERRUPT);  
> > 
> > Put DMA_PREP_INTERRUPT on a second line to avoid >80 char lines.  
> 
> I was told some maintainers don't enforce the >80 char line rule.
> Am I to understand you're not one of them? :-)

Yes, I'm one of them :P.

> 
> 
> >> +	if (!desc)
> >> +		goto dma_unmap;
> >> +
> >> +	desc->callback = tango_dma_callback;
> >> +	desc->callback_param = &tx_done;
> >> +	init_completion(&tx_done);
> >> +
> >> +	writel_relaxed(MODE_NFC, nfc->pbus_base + PBUS_PAD_MODE);
> >> +
> >> +	writel_relaxed(page, nfc->reg_base + NFC_ADDR_PAGE);
> >> +	writel_relaxed(   0, nfc->reg_base + NFC_ADDR_OFFSET);
> >> +	writel_relaxed( cmd, nfc->reg_base + NFC_FLASH_CMD);
> >> +
> >> +	dmaengine_submit(desc);
> >> +	dma_async_issue_pending(nfc->chan);
> >> +
> >> +	res = wait_for_completion_timeout(&tx_done, HZ);
> >> +	if (res > 0) {
> >> +		void __iomem *addr = nfc->reg_base + NFC_STATUS;
> >> +		err = readl_poll_timeout(addr, val, val & CMD_READY, 0, 1000);  
> > 
> > Why do you need this local variable?
> > 
> > 		err = readl_poll_timeout(nfc->reg_base + NFC_STATUS,
> > 					 val, val & CMD_READY, 0, 1000);  
> 
> I find it hard to read when function arguments are split
> over multiple lines. So if there is a hard "80 char" limit,
> I prefer using temp variables for "long" arguments.

Really? Is that really hard to read?

> 
> 
> >> +	}
> >> +
> >> +	writel_relaxed(MODE_RAW, nfc->pbus_base + PBUS_PAD_MODE);  
> > 
> > Add an extra blank line here.  
> 
> OK.
> 
> 
> >> +static int tango_read_page(struct mtd_info *mtd, struct nand_chip *chip,
> >> +		uint8_t *buf, int oob_required, int page)
> >> +{
> >> +	struct tango_nfc *nfc = to_tango_nfc(chip->controller);
> >> +	int err, res, len = mtd->writesize;
> >> +
> >> +	err = do_dma(nfc, DMA_FROM_DEVICE, NFC_READ, buf, len, page);
> >> +	if (err)
> >> +		return err;
> >> +
> >> +	if (oob_required)
> >> +		chip->ecc.read_oob(mtd, chip, page);
> >> +
> >> +	res = decode_error_report(nfc);
> >> +	if (res >= 0)
> >> +		return res;
> >> +
> >> +	chip->ecc.read_oob(mtd, chip, page);  
> > 
> > There's no different in your case, but it should be read in raw mode.
> > How about calling ecc.read_oob_raw() so that you're safe even if you
> > want to differentiate the read_oob() and read_oob_raw() cases at some
> > point (IIUC, the METADATA section is ECC protected).  
> 
> OK.
> 
> 
> >> +	res = nand_check_erased_ecc_chunk(buf, len, chip->oob_poi, mtd->oobsize,
> >> +			NULL, 0, chip->ecc.strength);  
> > 
> > You should check each ECC packet/chunk independently, because ECC
> > strength is referring to an ECC packet not a full page.  
> 
> Because of our weird physical layout, accessing data chunks is
> really awkward. We are willing to sacrifice a few bad pages
> by using a sub-optimal check (allowing only "strength" bitflips
> over the entire page, instead of over individual packets).

Hm, it doesn't look so complicated to me:

	bitflips = 0;

	for (i = 0; i < chip->ecc.steps; i++) {
		void *data = buf + (i * chip->ecc.size);
		void *oob = chip->oob_poi + (i * chip->ecc.bytes);
		void *extra_oob = NULL;
		int extra_oob_len = 0;

		if (!i) {
			extra_oob = chip->oob_poi;
			extra_oob_len = METADATA_SIZE + BBM_SIZE;
		} else if (i == chip->ecc.steps) {
			extra_oob_len = mtd->oobsize - METADATA_SIZE
			-		BBM_SIZE -
					(chip->ecc.steps *
					 chip->ecc.bytes);
			extra_oob_len = chip->oob_poi + mtd->oobsize -
					extra_oob_len;
		}

		res = nand_check_erased_ecc_chunk(data, chip->ecc.size,
						  oob, chip->ecc.bytes,
						  extra_oob,
						  extra_oob_len,
						  chip->ecc.strength);
		if (res >= 0)
			bitflips = max(res, bitflips);
		else
			mtd->ecc_stats.failed++; 
	}

> 
> 
> >> +static int tango_write_page(struct mtd_info *mtd, struct nand_chip *chip,
> >> +		const uint8_t *buf, int oob_required, int page)
> >> +{
> >> +	struct tango_nfc *nfc = to_tango_nfc(chip->controller);
> >> +	int err, len = mtd->writesize;
> >> +
> >> +	writel_relaxed(0xffffffff, nfc->mem_base + METADATA);
> >> +	err = do_dma(nfc, DMA_TO_DEVICE, NFC_WRITE, buf, len, page);
> >> +	if (err)
> >> +		return err;
> >> +
> >> +	if (oob_required)
> >> +		chip->ecc.write_oob(mtd, chip, page);  
> 
> I suppose we should use write_oob_raw here?

Oops, I didn't spot that one in my review. You said your engine sends
the PAGEPROG command, which means the page has already been programmed
when do_dma() returns, right?
In that case, you can't call ->write_oob() or ->write_oob_raw(),
because you're not allowed to program a page twice.

Don't you have a way to program OOB along with data (extra registers
you can use to pass your OOB data)?

Anyway, since you don't expose free OOB sections, I think you can
ignore the oob_required parameter. Just add a comment explaining why.

> 
> 
> >> +enum { OP_SKIP, OP_READ, OP_WRITE };
> >> +
> >> +static void raw_aux(struct mtd_info *mtd, u8 **buf, int len, int op, int *lim)  
> > 
> > Nit: please pass struct nand_chip *chip in first argument, and extract
> > the mtd using nand_to_mtd(). I know it's just a detail, but I'd like to
> > avoid passing mtd pointers, especially in internal functions.  
> 
> OK.
> 
> 
> >> +{
> >> +	struct nand_chip *chip = mtd_to_nand(mtd);
> >> +	int pos = mtd->writesize - *lim;
> >> +
> >> +	if (op == OP_SKIP)
> >> +		chip->cmdfunc(mtd, NAND_CMD_RNDOUT, pos + len, -1);  
> > 
> > Okay, we already discussed that one, and I already showed that this was
> > not working: NAND_CMD_RNDOUT is only valid for read accesses.
> > 
> > And I told you I'd like to avoid the const to non-const cast in write
> > accessors.  
> 
> Note: I don't use SKIP for writes, therefore there's no need
> to implement that code.

Except you still have to cast the buf pointer in tango_write_page_raw().

> 
> 
> >> +	else if (op == OP_READ)
> >> +		tango_read_buf(mtd, *buf, len);
> >> +	else if (op == OP_WRITE)
> >> +		tango_write_buf(mtd, *buf, len);
> >> +
> >> +	*lim -= len;
> >> +	*buf += len;
> >> +}  
> > 
> > I'm not sure factorizing this code is really important, but since you
> > insist, how about doing the following instead:  
> 
> If the code is not factorized, then it must be duplicated for
> read/write_raw and read/write_oob, I think (i.e. 4 times).

Only twice: read_oob and read_page_raw can still be factorized, same
goes for write_oob and write_page_raw.

Anyway, I fear I won't convince you, so please make sure you remove
this const to non-const cast.

> 
> > enum tango_raw_op { OP_READ, OP_WRITE };
> > 
> > union tango_raw_buffer {
> > 	void *in;
> > 	const void *out;
> > };
> > 
> > struct tango_raw_access {
> > 	enum tango_raw_op op;
> > 	union tango_raw_buffer *buf;
> > 	int pos;
> > };
> > 
> > static void raw_aux(struct nand_chip *chip,
> > 		    struct tango_raw_access *info,
> > 		    union tango_raw_buffer *buf, int len)
> > {
> > 	struct mtd_info *mtd = nand_to_mtd(chip);
> > 
> > 	if (!buf) {
> > 		if (info->op == OP_READ)
> > 			cmd = NAND_CMD_RNDOUT;
> > 		else
> > 			cmd = NAND_CMD_SEQIN;
> > 
> > 		chip->cmdfunc(mtd, cmd, info->pos + len, -1);
> > 	} else {
> > 		if (info->op == OP_READ)
> > 			tango_read_buf(mtd, buf->out, len);
> > 		else
> > 			tango_write_buf(mtd, buf->in, len);
> > 
> > 		buf->in += len;
> > 	}
> > 
> > 	info->pos += len;
> > }  
> 
> I'll take a closer look tomorrow.
> 
> 
> >> +static int raw_access(struct mtd_info *mtd, uint8_t *buf, uint8_t *oob, int op)
> >> +{
> >> +	struct nand_chip *chip = mtd_to_nand(mtd);
> >> +	int pkt_size = chip->ecc.size;
> >> +	int ecc_size = chip->ecc.bytes;
> >> +	int buf_op = buf ? op : OP_SKIP;
> >> +	int oob_op = oob ? op : OP_SKIP;
> >> +	int rem, lim = mtd->writesize;
> >> +	u8 *oob_orig = oob;
> >> +
> >> +	oob += BBM_SIZE;
> >> +	raw_aux(mtd, &oob, METADATA_SIZE, oob_op, &lim);
> >> +
> >> +	while (lim > pkt_size)
> >> +	{
> >> +		raw_aux(mtd, &buf, pkt_size, buf_op, &lim);
> >> +		raw_aux(mtd, &oob, ecc_size, oob_op, &lim);
> >> +	}
> >> +
> >> +	rem = pkt_size - lim;
> >> +	raw_aux(mtd, &buf, lim, buf_op, &lim);
> >> +	raw_aux(mtd, &oob_orig, BBM_SIZE, oob_op, &lim);
> >> +	raw_aux(mtd, &buf, rem, buf_op, &lim);
> >> +	raw_aux(mtd, &oob, ecc_size, oob_op, &lim);
> >> +
> >> +	return 0;
> >> +}  
> > 
> > With the above changes this gives:
> > 
> > static int raw_access(struct nand_chip *chip,
> > 		      union tango_raw_buffer *buf,
> > 		      union tango_raw_buffer *oob,
> > 		      int op)
> > {
> > 	struct mtd_info *mtd = nand_to_mtd(chip);
> > 	int pkt_size = chip->ecc.size;
> > 	int ecc_size = chip->ecc.bytes;
> > 	int steps = 0, rem = 0;
> > 	struct tango_raw_access info = {
> > 		.op = op,
> > 		.pos = 0,
> > 	};
> > 	union tango_raw_buffer oob_orig;
> > 
> > 	if (oob) {
> > 		oob->in = chip->oob_poi + BBM_SIZE;
> > 		oob_orig.in = chip->oob_poi;
> > 	}
> > 
> > 	raw_aux(chip, &info, oob, METADATA_SIZE);
> > 
> > 	while (info.pos + pkt_size + ecc_size <= mtd->writesize) {
> > 		raw_aux(chip, &info, buf, pkt_size);
> > 		raw_aux(mtd, &info, oob, ecc_size);
> > 		steps++;
> > 	}
> > 
> > 	if (steps < chip->ecc.steps)
> > 		rem = pkt_size - (mtd->writesize - info.pos);
> > 		raw_aux(mtd, &info, buf, mtd->writesize - info.pos);
> > 	}  
> 
> I don't think we need the 'steps' variable (I didn't need one).
> 
> > 	raw_aux(mtd, &info, oob ? &oob_orig : NULL, BBM_SIZE);
> > 	raw_aux(mtd, &info, buf, rem);
> > 
> > 	rem = mtd->writesize + mtd->oobsize - info.pos;
> > 	raw_aux(mtd, &info, oob, rem);
> > 
> > 	return 0;
> > }  
> 
> I'll take a closer look tomorrow.
> 
> 
> >> +static u32 to_ticks(int kHz, int ps)
> >> +{
> >> +	return DIV_ROUND_UP_ULL((u64)kHz * ps, NSEC_PER_SEC);
> >> +}
> >> +
> >> +static int set_timings(struct tango_chip *p, int kHz)
> >> +{
> >> +	u32 Trdy, Textw, Twc, Twpw, Tacc, Thold, Trpw, Textr;
> >> +	const struct nand_sdr_timings *sdr;
> >> +	int mode = onfi_get_async_timing_mode(&p->chip);
> >> +
> >> +	if (mode & ONFI_TIMING_MODE_UNKNOWN)
> >> +		return -ENODEV;
> >> +
> >> +	sdr = onfi_async_timing_mode_to_sdr_timings(fls(mode) - 1);  
> > 
> > We recently introduced a generic interface to automate nand timings
> > configuration [1].
> > Can you make use of it (the patches are in my nand/next branch [2], you
> > can rebase your patch series on top of this branch).  
> 
> I'll take a look. I still need to have this driver working
> on 4.7 for the time being...

And? You're trying to mainline the thing. What did you expect?
I'm asking you to use an infrastructure we introduced recently (will
be part of 4.9), the fact that you need to make the driver work on 4.7
is not a good reason to not use it (you can either backport the
series, or keep 2 versions of your driver).

> 
> 
> >> +
> >> +	Trdy	= to_ticks(kHz, sdr->tCEA_max - sdr->tREA_max);
> >> +	Textw	= to_ticks(kHz, sdr->tWB_max);
> >> +	Twc	= to_ticks(kHz, sdr->tWC_min);
> >> +	Twpw	= to_ticks(kHz, sdr->tWC_min - sdr->tWP_min);
> >> +
> >> +	Tacc	= to_ticks(kHz, sdr->tREA_max);
> >> +	Thold	= to_ticks(kHz, sdr->tREH_min);
> >> +	Trpw	= to_ticks(kHz, sdr->tRC_min - sdr->tREH_min);
> >> +	Textr	= to_ticks(kHz, sdr->tRHZ_max);  
> > 
> > Can you please be consistent in your indentation?
> > You're using spaces before '=' almost everywhere except in a few
> > places (like here).  
> 
> When initializing fields of a struct, most code I've seen
> uses tabs to align the equal signs. I thought it should be
> the same when assigning the fields of a struct, or assigning
> several variables of the same nature (such as above).
> 
> Are you saying you prefer
> 
>   Trdy = to_ticks(kHz, sdr->tCEA_max - sdr->tREA_max);
>   Textw = to_ticks(kHz, sdr->tWB_max);
>   Twc = to_ticks(kHz, sdr->tWC_min);

Yes, I prefer that one.

> 
> Thus the to_ticks calls are not aligned, and it's less obvious
> that the first argument is always the same.

And? What's the point of making it obvious? Does it help understanding
the code?

I prefer when the coding is consistent, and here, it's not consistent
with the rest of the file.

> 
> 
> >> +static int chip_init(struct device *dev, struct device_node *np, int kHz)
> >> +{
> >> +	int err;
> >> +	u32 cs, ecc_bits;
> >> +	struct nand_ecc_ctrl *ecc;
> >> +	struct tango_nfc *nfc = dev_get_drvdata(dev);
> >> +	struct tango_chip *p = devm_kzalloc(dev, sizeof(*p), GFP_KERNEL);
> >> +	if (!p)
> >> +		return -ENOMEM;
> >> +
> >> +	err = of_property_read_u32_index(np, "reg", 0, &cs);
> >> +	if (err)
> >> +		return err;
> >> +  
> > 
> > Can you please check that you only have a single value in reg? Just to
> > make sure the user doesn't try to define a multi-CS chip.  
> 
> OK.
> 
> 
> >> +	if (cs >= MAX_CS)
> >> +		return -ERANGE;
> >> +
> >> +	p->chip.read_byte	= tango_read_byte;
> >> +	p->chip.read_buf	= tango_read_buf;
> >> +	p->chip.select_chip	= tango_select_chip;
> >> +	p->chip.cmd_ctrl	= tango_cmd_ctrl;
> >> +	p->chip.dev_ready	= tango_dev_ready;
> >> +	p->chip.options		= NAND_USE_BOUNCE_BUFFER | NAND_NO_SUBPAGE_WRITE;
> >> +	p->chip.controller	= &nfc->hw;  
> > 
> > Again, be consistent in you coding style: use spaces.  
> 
> OK. I'll take another look at the sunxi driver.
> 
> It makes no sense to me that we should tab-align when using
> struct initialization with named fields, but space-align
> when using assignment.

I never asked to tab-align struct field assignments. Actually, I don't
like it either.

> 
> 
> >> +
> >> +	ecc			= &p->chip.ecc;
> >> +	ecc->mode		= NAND_ECC_HW;
> >> +	ecc->algo		= NAND_ECC_BCH;
> >> +	ecc->read_page_raw	= tango_read_page_raw;
> >> +	ecc->write_page_raw	= tango_write_page_raw;
> >> +	ecc->read_page		= tango_read_page;
> >> +	ecc->write_page		= tango_write_page;
> >> +	ecc->read_oob		= tango_read_oob;
> >> +	ecc->write_oob		= tango_write_oob;  
> > 
> > Can you move this part after nand_scan_ident(). I'd also suggest to
> > support software ECC (it's just taking a few more lines), but that's up
> > to you.  
> 
> OK. What does it mean to support SW ECC?

I means that, if someone wants to use your NAND controller with
software ECC or ECC at all he can.

Regards,

Boris

Re: [PATCH v5] mtd: nand: tango: import driver for tango chips

[Artem Bityutskiy]

On Tue, 2016-09-20 at 09:24 +0200, Boris Brezillon wrote:
> > >> +  writel_relaxed(chip->timing1,   nfc->reg_base + NFC_TIMING1);
> > >> +  writel_relaxed(chip->timing2,   nfc->reg_base + NFC_TIMING2);
> > >> +  writel_relaxed(chip->xfer_cfg,  nfc->reg_base + NFC_XFER_CFG);
> > >> +  writel_relaxed(chip->pkt_0_cfg, nfc->reg_base + NFC_PKT_0_CFG);
> > >> +  writel_relaxed(chip->pkt_n_cfg, nfc->reg_base + NFC_PKT_N_CFG);
> > >> +  writel_relaxed(chip->bb_cfg,    nfc->reg_base + NFC_BB_CFG);  
> > > 
> > > Nit: can you avoid these weird alignments. Use a single space after the
> > > comma.  
> > 
> > I was trying to highlight the fact that all these writes are
> > within the reg_base area. I'll do as you ask, since you are
> > the gatekeeper.
> 
> Is that really less clear without the tabs?

Visually the aligned stuff looks nicer, and some kernel maintainers
prefer this style. Other maintainers avoid it because it causes more
churn. Indeed, imaging you need to add another field with longer name,
or one of the existing fields is renamed - then you need to re-align,
your patches become larger.

So my suggestion is to just follow maintainers' preferences.

Re: [PATCH v5] mtd: nand: tango: import driver for tango chips

[Richard Weinberger]

On 20.09.2016 09:39, Artem Bityutskiy wrote:
> On Tue, 2016-09-20 at 09:24 +0200, Boris Brezillon wrote:
>>>>> +  writel_relaxed(chip->timing1,   nfc->reg_base + NFC_TIMING1);
>>>>> +  writel_relaxed(chip->timing2,   nfc->reg_base + NFC_TIMING2);
>>>>> +  writel_relaxed(chip->xfer_cfg,  nfc->reg_base + NFC_XFER_CFG);
>>>>> +  writel_relaxed(chip->pkt_0_cfg, nfc->reg_base + NFC_PKT_0_CFG);
>>>>> +  writel_relaxed(chip->pkt_n_cfg, nfc->reg_base + NFC_PKT_N_CFG);
>>>>> +  writel_relaxed(chip->bb_cfg,    nfc->reg_base + NFC_BB_CFG);  
>>>>  
>>>> Nit: can you avoid these weird alignments. Use a single space after the
>>>> comma.  
>>>  
>>> I was trying to highlight the fact that all these writes are
>>> within the reg_base area. I'll do as you ask, since you are
>>> the gatekeeper.
>>
>> Is that really less clear without the tabs?
> 
> Visually the aligned stuff looks nicer, and some kernel maintainers
> prefer this style. Other maintainers avoid it because it causes more
> churn. Indeed, imaging you need to add another field with longer name,
> or one of the existing fields is renamed - then you need to re-align,
> your patches become larger.

FWIW, I don't like tab aligned stuff for the simple fact because
git grep "foo =" does not find them.
Of course I could use regex magic but this does not make things easier. :-)

Thanks,
//richard

Re: [PATCH v5] mtd: nand: tango: import driver for tango chips

[Marc Gonzalez]

On 19/09/2016 15:12, Marc Gonzalez wrote:

> +static int chip_init(struct device *dev, struct device_node *np, int kHz)
> +{
> +	int err;
> +	u32 cs, ecc_bits;
> +	struct nand_ecc_ctrl *ecc;
> +	struct tango_nfc *nfc = dev_get_drvdata(dev);
> +	struct tango_chip *p = devm_kzalloc(dev, sizeof(*p), GFP_KERNEL);

As pointed out by Boris on IRC, every chip needs a pointer back
to the parent device.

	p->chip.mtd.dev.parent = dev;

http://lxr.free-electrons.com/source/drivers/mtd/nand/sunxi_nand.c#L1729