mtd: nand: tango: import driver for tango controller

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

Signed-off-by Marc Gonzalez <[email protected]>

---
 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

drivers/mtd/nand/Kconfig +6 -0

index f05e0e9eb2f7..22eb5457c9f7 100644
@@ -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

drivers/mtd/nand/Makefile +1 -0

index f55335373f7c..647f727223ef 100644
@@ -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

drivers/mtd/nand/tango_nand.c new file +31 -0

new file mode 100644
index 000000000000..dfd27081d5ec
@@ -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

drivers/mtd/nand/tango_nand.c new file +20 -0

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

drivers/mtd/nand/tango_nand.c new file +4 -0

+};
+
+#define to_tango_chip(ptr) container_of(ptr, struct tango_chip, chip)
+

drivers/mtd/nand/tango_nand.c new file +36 -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 int decode_error_report(struct tango_nfc *nfc)
+{

drivers/mtd/nand/tango_nand.c new file +21 -0

+
+	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;

drivers/mtd/nand/tango_nand.c new file +106 -0

+
+	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;

drivers/mtd/nand/tango_nand.c new file +37 -0

+	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]))

drivers/mtd/nand/tango_nand.c new file +48 -0

+
+	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");

drivers/mtd/nand/tango_nand.c line 41

Quoted 3 lines · Marc Gonzalez

drivers/mtd/nand/tango_nand.c new file +3 -0

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

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

drivers/mtd/nand/tango_nand.c line 43

Quoted 2 lines

drivers/mtd/nand/tango_nand.c new file +2 -0

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

drivers/mtd/nand/tango_nand.c line 73

Quoted 3 lines

drivers/mtd/nand/tango_nand.c new file +3 -0

+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.

drivers/mtd/nand/tango_nand.c line 74

Quoted 1 line

drivers/mtd/nand/tango_nand.c new file +1 -0

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

Please, one field per line in struct definitions.

Quoted 1 line

+#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...

drivers/mtd/nand/tango_nand.c line 134

Quoted 4 lines

drivers/mtd/nand/tango_nand.c new file +4 -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) ?

drivers/mtd/nand/tango_nand.c line 165

Quoted 1 line

drivers/mtd/nand/tango_nand.c new file +1 -0

+	writel_relaxed(MODE_MLC, nfc->pbus_base + PBUS_PAD_MODE);

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

drivers/mtd/nand/tango_nand.c line 281

Quoted 1 line

drivers/mtd/nand/tango_nand.c new file +1 -0

+	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.

Quoted 20 lines

drivers/mtd/nand/tango_nand.c new file +20 -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]))

drivers/mtd/nand/tango_nand.c line 328

Quoted 10 lines

drivers/mtd/nand/tango_nand.c new file +10 -0

+			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);

Quoted 7 lines · Boris Brezillon

drivers/mtd/nand/tango_nand.c line 41

Quoted 3 lines · Marc Gonzalez

drivers/mtd/nand/tango_nand.c new file +3 -0

+/* 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.)

Quoted 1 line

#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.

Quoted 2 lines

drivers/mtd/nand/tango_nand.c line 43

Quoted 2 lines

drivers/mtd/nand/tango_nand.c new file +2 -0

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

Quoted 8 lines

drivers/mtd/nand/tango_nand.c line 73

Quoted 4 lines

drivers/mtd/nand/tango_nand.c new file +3 -0

+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?

Quoted 5 lines

drivers/mtd/nand/tango_nand.c line 74

Quoted 3 lines

drivers/mtd/nand/tango_nand.c new file +1 -0

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

Please, one field per line in struct definitions.

OK.

Quoted 4 lines

Quoted 1 line

+#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.

Quoted 2 lines

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.

Quoted 8 lines

drivers/mtd/nand/tango_nand.c line 134

Quoted 4 lines

drivers/mtd/nand/tango_nand.c new file +4 -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) ?

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?

Quoted 5 lines

drivers/mtd/nand/tango_nand.c line 165

Quoted 3 lines

drivers/mtd/nand/tango_nand.c new file +1 -0

+	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).

Quoted 7 lines

drivers/mtd/nand/tango_nand.c line 281

Quoted 3 lines

drivers/mtd/nand/tango_nand.c new file +1 -0

+	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.

Quoted 35 lines

Quoted 22 lines

drivers/mtd/nand/tango_nand.c new file +20 -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]))

drivers/mtd/nand/tango_nand.c line 328

Quoted 10 lines

drivers/mtd/nand/tango_nand.c new file +10 -0

+			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

Quoted 4 lines

	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);

Quoted 1 line

	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?

drivers/mtd/nand/tango_nand.c new file +5 -0

+static int decode_error_report(struct tango_nfc *nfc)
+{
+	u32 status, res;
+
+	status = readl_relaxed(nfc->reg_base + NFC_XFER_STATUS);

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

Signed-off-by Marc Gonzalez <[email protected]>

---
 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

Quoted 10 lines · Marc Gonzalez

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

Signed-off-by Marc Gonzalez <[email protected]>

---
 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)

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

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

Signed-off-by Marc Gonzalez <[email protected]>

---
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

drivers/mtd/nand/Kconfig +6 -0

index f05e0e9eb2f7..22eb5457c9f7 100644
@@ -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

drivers/mtd/nand/Makefile +1 -0

index f55335373f7c..647f727223ef 100644
@@ -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

drivers/mtd/nand/tango_nand.c new file +80 -0

new file mode 100644
index 000000000000..ec18cad546fb
@@ -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;

drivers/mtd/nand/tango_nand.c new file +62 -0

+	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)
+{

drivers/mtd/nand/tango_nand.c new file +25 -0

+}
+
+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)

drivers/mtd/nand/tango_nand.c new file +10 -0

+	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);

drivers/mtd/nand/tango_nand.c new file +14 -0

+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)

drivers/mtd/nand/tango_nand.c new file +16 -0

+	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;

drivers/mtd/nand/tango_nand.c new file +18 -0

+
+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);

drivers/mtd/nand/tango_nand.c new file +53 -0

+
+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);
+}
+

drivers/mtd/nand/tango_nand.c new file +11 -0

+
+	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;

drivers/mtd/nand/tango_nand.c new file +5 -0

+
+	err = nand_scan_tail(&p->chip.mtd);
+	if (err)
+		return err;
+

drivers/mtd/nand/tango_nand.c new file +37 -0

+
+	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))

drivers/mtd/nand/tango_nand.c new file +29 -0

+	}
+
+	/* 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	= {

Quoted 2 lines · Marc Gonzalez

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.

drivers/mtd/nand/tango_nand.c line 90

Quoted 3 lines

drivers/mtd/nand/tango_nand.c new file +3 -0

+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.

drivers/mtd/nand/tango_nand.c line 162

Quoted 12 lines

drivers/mtd/nand/tango_nand.c new file +12 -0

+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.

drivers/mtd/nand/tango_nand.c line 169

Quoted 6 lines

drivers/mtd/nand/tango_nand.c new file +6 -0

+	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.

Quoted 6 lines

drivers/mtd/nand/tango_nand.c new file +6 -0

+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;

drivers/mtd/nand/tango_nand.c line 204

Quoted 6 lines

drivers/mtd/nand/tango_nand.c new file +6 -0

+	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.

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+	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.

Quoted 10 lines

drivers/mtd/nand/tango_nand.c new file +10 -0

+	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);

drivers/mtd/nand/tango_nand.c line 226

Quoted 10 lines

drivers/mtd/nand/tango_nand.c new file +10 -0

+	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);

drivers/mtd/nand/tango_nand.c line 229

Quoted 3 lines

drivers/mtd/nand/tango_nand.c new file +3 -0

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

Add an extra blank line here.

Quoted 15 lines

drivers/mtd/nand/tango_nand.c new file +15 -0

+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)

drivers/mtd/nand/tango_nand.c line 253

Quoted 3 lines

drivers/mtd/nand/tango_nand.c new file +3 -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).

drivers/mtd/nand/tango_nand.c line 255

Quoted 2 lines

drivers/mtd/nand/tango_nand.c new file +2 -0

+	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.

Quoted 5 lines

drivers/mtd/nand/tango_nand.c new file +5 -0

+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;

drivers/mtd/nand/tango_nand.c line 274

Quoted 8 lines

drivers/mtd/nand/tango_nand.c new file +8 -0

+
+	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);

drivers/mtd/nand/tango_nand.c line 281

Quoted 3 lines

drivers/mtd/nand/tango_nand.c new file +3 -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.

drivers/mtd/nand/tango_nand.c line 287

Quoted 6 lines

drivers/mtd/nand/tango_nand.c new file +6 -0

+{
+	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.

drivers/mtd/nand/tango_nand.c line 295

Quoted 8 lines

drivers/mtd/nand/tango_nand.c new file +8 -0

+	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;
}

Quoted 17 lines

drivers/mtd/nand/tango_nand.c new file +17 -0

+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);

drivers/mtd/nand/tango_nand.c line 323

Quoted 10 lines

drivers/mtd/nand/tango_nand.c new file +10 -0

+	}
+
+	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;
}

Quoted 12 lines

drivers/mtd/nand/tango_nand.c new file +12 -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)

drivers/mtd/nand/tango_nand.c line 392

Quoted 3 lines

drivers/mtd/nand/tango_nand.c new file +3 -0

+		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).

Quoted 6 lines

drivers/mtd/nand/tango_nand.c new file +6 -0

+
+	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);
+

drivers/mtd/nand/tango_nand.c line 402

Quoted 4 lines

drivers/mtd/nand/tango_nand.c new file +4 -0

+	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).

Quoted 9 lines

drivers/mtd/nand/tango_nand.c new file +9 -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;

drivers/mtd/nand/tango_nand.c line 423

Quoted 5 lines

drivers/mtd/nand/tango_nand.c new file +5 -0

+
+	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.

Quoted 4 lines

drivers/mtd/nand/tango_nand.c new file +4 -0

+	if (cs >= MAX_CS)
+		return -ERANGE;
+
+	p->chip.read_byte	= tango_read_byte;

drivers/mtd/nand/tango_nand.c line 433

Quoted 6 lines

drivers/mtd/nand/tango_nand.c new file +6 -0

+	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.

Quoted 6 lines

drivers/mtd/nand/tango_nand.c new file +6 -0

+
+	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;

drivers/mtd/nand/tango_nand.c line 443

Quoted 4 lines

drivers/mtd/nand/tango_nand.c new file +4 -0

+	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.

Quoted 5 lines

drivers/mtd/nand/tango_nand.c new file +5 -0

+
+	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)

drivers/mtd/nand/tango_nand.c line 453

Quoted 5 lines

drivers/mtd/nand/tango_nand.c new file +5 -0

+		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.

drivers/mtd/nand/tango_nand.c line 468

Quoted 4 lines

drivers/mtd/nand/tango_nand.c new file +4 -0

+	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.

drivers/mtd/nand/tango_nand.c line 469

Quoted 1 line

drivers/mtd/nand/tango_nand.c new file +1 -0

+	p->cs		= cs;

And this one should go before nand_scan_ident().

drivers/mtd/nand/tango_nand.c line 516

Quoted 4 lines

drivers/mtd/nand/tango_nand.c new file +4 -0

+	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.

Quoted 7 lines · Boris Brezillon

Quoted 2 lines · Marc Gonzalez

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.

Quoted 7 lines

drivers/mtd/nand/tango_nand.c line 90

Quoted 3 lines

drivers/mtd/nand/tango_nand.c new file +3 -0

+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.

Quoted 18 lines

Quoted 4 lines

drivers/mtd/nand/tango_nand.c new file +2 -0

+static void tango_select_chip(struct mtd_info *mtd, int idx)
+{

drivers/mtd/nand/tango_nand.c line 162

Quoted 10 lines

drivers/mtd/nand/tango_nand.c new file +10 -0

+	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.

Quoted 3 lines

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.

Quoted 9 lines

drivers/mtd/nand/tango_nand.c line 169

Quoted 6 lines

drivers/mtd/nand/tango_nand.c new file +6 -0

+	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.

Quoted 16 lines

Quoted 4 lines

drivers/mtd/nand/tango_nand.c new file +2 -0

+static int do_dma(struct tango_nfc *nfc, int dir, int cmd,
+		const void *buf, int len, int page)

drivers/mtd/nand/tango_nand.c line 204

Quoted 10 lines

drivers/mtd/nand/tango_nand.c new file +10 -0

+{
+	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.

Quoted 6 lines

Review

Quoted 4 lines

(quoted diff) +2 -0

+
+	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? :-)

Quoted 25 lines

Quoted 12 lines

drivers/mtd/nand/tango_nand.c new file +10 -0

+	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);

drivers/mtd/nand/tango_nand.c line 226

Quoted 10 lines

drivers/mtd/nand/tango_nand.c new file +10 -0

+	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?

Quoted 2 lines

		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.

Quoted 7 lines

drivers/mtd/nand/tango_nand.c line 229

Quoted 5 lines

drivers/mtd/nand/tango_nand.c new file +3 -0

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

Add an extra blank line here.

OK.

Quoted 15 lines

Quoted 15 lines

drivers/mtd/nand/tango_nand.c new file +15 -0

+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)

Quoted 8 lines

drivers/mtd/nand/tango_nand.c line 253

Quoted 3 lines

drivers/mtd/nand/tango_nand.c new file +3 -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.

Quoted 5 lines

drivers/mtd/nand/tango_nand.c line 255

Quoted 2 lines

drivers/mtd/nand/tango_nand.c new file +2 -0

+	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).

Quoted 15 lines

Quoted 5 lines

drivers/mtd/nand/tango_nand.c new file +3 -0

+static int tango_write_page(struct mtd_info *mtd, struct nand_chip *chip,
+		const uint8_t *buf, int oob_required, int page)
+{

drivers/mtd/nand/tango_nand.c line 274

Quoted 10 lines

drivers/mtd/nand/tango_nand.c new file +10 -0

+	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?

Quoted 9 lines

drivers/mtd/nand/tango_nand.c line 281

Quoted 5 lines

drivers/mtd/nand/tango_nand.c new file +3 -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.

OK.

Quoted 10 lines

drivers/mtd/nand/tango_nand.c line 287

Quoted 6 lines

drivers/mtd/nand/tango_nand.c new file +6 -0

+{
+	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.

Quoted 2 lines

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.

Quoted 13 lines

drivers/mtd/nand/tango_nand.c line 295

Quoted 10 lines

drivers/mtd/nand/tango_nand.c new file +8 -0

+	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).

Quoted 30 lines

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);

Quoted 8 lines

		else
			tango_write_buf(mtd, buf->in, len);

		buf->in += len;
	}

	info->pos += len;
}  

I'll take a closer look tomorrow.

Quoted 54 lines

Quoted 17 lines

drivers/mtd/nand/tango_nand.c new file +17 -0

+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);

drivers/mtd/nand/tango_nand.c line 323

Quoted 10 lines

drivers/mtd/nand/tango_nand.c new file +10 -0

+	}
+
+	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);

Quoted 8 lines

		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).

Quoted 8 lines

	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.

Quoted 12 lines

Quoted 12 lines

drivers/mtd/nand/tango_nand.c new file +12 -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)

Quoted 8 lines

drivers/mtd/nand/tango_nand.c line 392

Quoted 3 lines

drivers/mtd/nand/tango_nand.c new file +3 -0

+		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...

Quoted 16 lines

drivers/mtd/nand/tango_nand.c line 402

Quoted 12 lines

drivers/mtd/nand/tango_nand.c new file +10 -0

+
+	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.

Quoted 11 lines

Quoted 11 lines

drivers/mtd/nand/tango_nand.c new file +9 -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;

Quoted 8 lines

drivers/mtd/nand/tango_nand.c line 423

Quoted 5 lines

drivers/mtd/nand/tango_nand.c new file +5 -0

+
+	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.

Quoted 4 lines

Quoted 4 lines

drivers/mtd/nand/tango_nand.c new file +4 -0

+	if (cs >= MAX_CS)
+		return -ERANGE;
+
+	p->chip.read_byte	= tango_read_byte;

Quoted 8 lines

drivers/mtd/nand/tango_nand.c line 433

Quoted 6 lines

drivers/mtd/nand/tango_nand.c new file +6 -0

+	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.

Quoted 16 lines

drivers/mtd/nand/tango_nand.c line 443

Quoted 12 lines

drivers/mtd/nand/tango_nand.c new file +10 -0

+
+	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?

Quoted 9 lines

Quoted 9 lines

Quoted 6 lines

+  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.

Quoted 4 lines

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?

Quoted 14 lines · Boris Brezillon

Quoted 13 lines

Quoted 9 lines

drivers/mtd/nand/tango_nand.c line 169

Quoted 6 lines

drivers/mtd/nand/tango_nand.c new file +6 -0

+  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.

Quoted 1 line

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.