On 28.05.2018 00:19, Miquel Raynal wrote:

Hi Stefan,

A few more comments here.

On Sun, 27 May 2018 23:54:39 +0200, Stefan Agner [off-list ref]
wrote:

quoted

Add support for the NAND flash controller found on NVIDIA
Tegra 2 SoCs. This implementation does not make use of the
command queue feature. Regular operations/data transfers are
done in PIO mode. Page read/writes with hardware ECC make
use of the DMA for data transfer.

Signed-off-by: Lucas Stach <dev@lynxeye.de>
Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 MAINTAINERS                       |   7 +
 drivers/mtd/nand/raw/Kconfig      |   6 +
 drivers/mtd/nand/raw/Makefile     |   1 +
 drivers/mtd/nand/raw/tegra_nand.c | 999 ++++++++++++++++++++++++++++++
 4 files changed, 1013 insertions(+)
 create mode 100644 drivers/mtd/nand/raw/tegra_nand.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 58b9861ccf99..8cbbb7111742 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS

@@ -13844,6 +13844,13 @@ M:	Laxman Dewangan <ldewangan@nvidia.com>
 S:	Supported
 F:	drivers/input/keyboard/tegra-kbc.c

+TEGRA NAND DRIVER
+M:	Stefan Agner <stefan@agner.ch>
+M:	Lucas Stach <dev@lynxeye.de>
+S:	Maintained
+F:	Documentation/devicetree/bindings/mtd/nvidia,tegra20-nand.txt

I think most MTD bindings use '-' instead of ','. I don't have a
preference, it's just for coherence.

Sure, will use a dash.

quoted

+F:	drivers/mtd/nand/raw/tegra_nand.c
+
 TEGRA PWM DRIVER
 M:	Thierry Reding [off-list ref]
 S:	Supported

diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
index 19a2b283fbbe..012c63c6ab47 100644
--- a/drivers/mtd/nand/raw/Kconfig
+++ b/drivers/mtd/nand/raw/Kconfig

@@ -534,4 +534,10 @@ config MTD_NAND_MTK
 	  Enables support for NAND controller on MTK SoCs.
 	  This controller is found on mt27xx, mt81xx, mt65xx SoCs.

+config MTD_NAND_TEGRA
+	tristate "Support for NAND on NVIDIA Tegra"
+	depends on ARCH_TEGRA || COMPILE_TEST
+	help
+	  Enables support for NAND flash on NVIDIA Tegra SoC based boards.

Please make the term "controller" appear because it's mostly a
controller driver that you're adding.

Ok.

quoted

+
 endif # MTD_NAND

diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile
index 165b7ef9e9a1..d5a5f9832b88 100644
--- a/drivers/mtd/nand/raw/Makefile
+++ b/drivers/mtd/nand/raw/Makefile

@@ -56,6 +56,7 @@ obj-$(CONFIG_MTD_NAND_HISI504)	        += hisi504_nand.o
 obj-$(CONFIG_MTD_NAND_BRCMNAND)		+= brcmnand/
 obj-$(CONFIG_MTD_NAND_QCOM)		+= qcom_nandc.o
 obj-$(CONFIG_MTD_NAND_MTK)		+= mtk_ecc.o mtk_nand.o
+obj-$(CONFIG_MTD_NAND_TEGRA)		+= tegra_nand.o

 nand-objs := nand_base.o nand_bbt.o nand_timings.o nand_ids.o
 nand-objs += nand_amd.o

[...]

quoted

+static int tegra_nand_setup_data_interface(struct mtd_info *mtd, int csline,
+					   const struct nand_data_interface *conf)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	struct tegra_nand_controller *ctrl = to_tegra_ctrl(chip->controller);
+	const struct nand_sdr_timings *timings;
+
+	timings = nand_get_sdr_timings(conf);
+	if (IS_ERR(timings))
+		return PTR_ERR(timings);
+
+	if (csline == NAND_DATA_IFACE_CHECK_ONLY)
+		return 0;
+
+	tegra_nand_setup_timing(ctrl, timings);

Is this indirection really needed?

This evolved due to historic reasons, will get rid of it.

quoted

+
+	return 0;
+}
+
+static int tegra_nand_chips_init(struct device *dev,
+				 struct tegra_nand_controller *ctrl)
+{
+	struct device_node *np = dev->of_node;
+	struct device_node *np_nand;
+	int nchips = of_get_child_count(np);
+	struct tegra_nand_chip *nand;
+	struct mtd_info *mtd;
+	struct nand_chip *chip;
+	unsigned long config, bch_config = 0;
+	int bits_per_step;
+	int err;
+
+	if (nchips != 1) {
+		dev_err(dev, "currently only one NAND chip supported\n");
+		return -EINVAL;
+	}
+
+	np_nand = of_get_next_child(np, NULL);
+
+	nand = devm_kzalloc(dev, sizeof(*nand), GFP_KERNEL);
+	if (!nand) {
+		dev_err(dev, "could not allocate chip structure\n");
+		return -ENOMEM;
+	}
+
+	nand->wp_gpio = devm_gpiod_get_optional(dev, "wp", GPIOD_OUT_LOW);
+
+	if (IS_ERR(nand->wp_gpio)) {
+		err = PTR_ERR(nand->wp_gpio);
+		dev_err(dev, "failed to request WP GPIO: %d\n", err);
+		return err;
+	}
+
+	chip = &nand->chip;
+	chip->controller = &ctrl->controller;
+	ctrl->chip = chip;
+
+	mtd = nand_to_mtd(chip);
+
+	mtd->dev.parent = dev;
+	mtd->name = "tegra_nand";
+	mtd->owner = THIS_MODULE;
+
+	nand_set_flash_node(chip, np_nand);
+
+	chip->options = NAND_NO_SUBPAGE_WRITE | NAND_USE_BOUNCE_BUFFER;
+	chip->exec_op = tegra_nand_exec_op;
+	chip->select_chip = tegra_nand_select_chip;
+	chip->setup_data_interface = tegra_nand_setup_data_interface;
+
+	err = nand_scan_ident(mtd, 1, NULL);
+	if (err)
+		return err;
+
+	if (chip->bbt_options & NAND_BBT_USE_FLASH)
+		chip->bbt_options |= NAND_BBT_NO_OOB;
+
+	chip->ecc.mode = NAND_ECC_HW;
+	if (!chip->ecc.size)
+		chip->ecc.size = 512;
+	if (chip->ecc.size != 512)
+		return -EINVAL;

This is useless. You can force ecc.size to 512 and forget about the DT
property (please update the DT bindings).

Ok.

quoted

+
+	chip->ecc.read_page = tegra_nand_read_page_hwecc;
+	chip->ecc.write_page = tegra_nand_write_page_hwecc;
+	/* Not functional for unknown reason...
+	chip->ecc.read_page_raw = tegra_nand_read_page;
+	chip->ecc.write_page_raw = tegra_nand_write_page;

Please add the _raw suffix to these helpers.

They are also used from tegra_nand_read_page_hwecc, that is why I was
hesitant to add _raw...

quoted

+	*/
+	config = readl(ctrl->regs + CFG);
+	config |= CFG_PIPE_EN | CFG_SKIP_SPARE | CFG_SKIP_SPARE_SIZE_4;
+
+	if (chip->options & NAND_BUSWIDTH_16)
+		config |= CFG_BUS_WIDTH_16;
+
+	switch (chip->ecc.algo) {
+	case NAND_ECC_RS:
+		bits_per_step = BITS_PER_STEP_RS * chip->ecc.strength;
+		mtd_set_ooblayout(mtd, &tegra_nand_oob_rs_ops);
+		switch (chip->ecc.strength) {
+		case 4:
+			config |= CFG_ECC_SEL | CFG_TVAL_4;
+			break;
+		case 6:
+			config |= CFG_ECC_SEL | CFG_TVAL_6;
+			break;
+		case 8:
+			config |= CFG_ECC_SEL | CFG_TVAL_8;
+			break;
+		default:
+			dev_err(dev, "ECC strength %d not supported\n",
+				chip->ecc.strength);
+			return -EINVAL;
+		}
+		break;
+	case NAND_ECC_BCH:
+		bits_per_step = BITS_PER_STEP_BCH * chip->ecc.strength;
+		mtd_set_ooblayout(mtd, &tegra_nand_oob_bch_ops);
+		switch (chip->ecc.strength) {
+		case 4:
+			bch_config = BCH_TVAL_4;
+			break;
+		case 8:
+			bch_config = BCH_TVAL_8;
+			break;
+		case 14:
+			bch_config = BCH_TVAL_14;
+			break;
+		case 16:
+			bch_config = BCH_TVAL_16;
+			break;
+		default:
+			dev_err(dev, "ECC strength %d not supported\n",
+				chip->ecc.strength);
+			return -EINVAL;
+		}
+		break;
+	default:
+		dev_err(dev, "ECC algorithm not supported\n");
+		return -EINVAL;
+	}
+
+	chip->ecc.bytes = DIV_ROUND_UP(bits_per_step, 8);
+
+	switch (mtd->writesize) {
+	case 256:
+		config |= CFG_PS_256;
+		break;
+	case 512:
+		config |= CFG_PS_512;
+		break;
+	case 1024:
+		config |= CFG_PS_1024;
+		break;
+	case 2048:
+		config |= CFG_PS_2048;
+		break;
+	case 4096:
+		config |= CFG_PS_4096;
+		break;
+	default:
+		dev_err(dev, "unhandled writesize %d\n", mtd->writesize);
+		return -ENODEV;
+	}
+
+	writel(config, ctrl->regs + CFG);
+	writel(bch_config, ctrl->regs + BCH_CONFIG);
+
+	err = nand_scan_tail(mtd);
+	if (err)
+		return err;
+
+	config |= CFG_TAG_BYTE_SIZE(mtd_ooblayout_count_freebytes(mtd) - 1);
+	writel(config, ctrl->regs + CFG);
+
+	err = mtd_device_register(mtd, NULL, 0);
+	if (err)

Missing nand_cleanup() in the error path.

Ok.

--
Stefan

quoted

+		return err;
+
+	return 0;
+}
+

[...]