diff mbox series

[v3,24/42] mtd: nand: add support for ts72xx

Message ID 20230605-ep93xx-v3-24-3d63a5f1103e@maquefel.me
State Changes Requested
Headers show
Series ep93xx device tree conversion | expand

Commit Message

Nikita Shubin via B4 Relay July 20, 2023, 11:29 a.m. UTC
From: Nikita Shubin <nikita.shubin@maquefel.me>

Technologic Systems has it's own nand controller implementation in CPLD.

Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
---
 drivers/mtd/nand/raw/Kconfig                       |   7 +
 drivers/mtd/nand/raw/Makefile                      |   1 +
 drivers/mtd/nand/raw/technologic-nand-controller.c | 162 +++++++++++++++++++++
 3 files changed, 170 insertions(+)

Comments

Andy Shevchenko July 21, 2023, 4:27 p.m. UTC | #1
On Thu, Jul 20, 2023 at 02:29:24PM +0300, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
> 
> Technologic Systems has it's own nand controller implementation in CPLD.

...

+ bits.h

> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>

...

> +static int ts72xx_nand_attach_chip(struct nand_chip *chip)
> +{
> +	switch (chip->ecc.engine_type) {
> +	case NAND_ECC_ENGINE_TYPE_SOFT:
> +		if (chip->ecc.algo == NAND_ECC_ALGO_UNKNOWN)
> +			chip->ecc.algo = NAND_ECC_ALGO_HAMMING;
> +		break;
> +	case NAND_ECC_ENGINE_TYPE_ON_HOST:
> +		return -EINVAL;
> +	default:

> +		break;

Here it will return 0, is it a problem?

> +	}
> +
> +	return 0;
> +}

...

> +static int ts72xx_nand_probe(struct platform_device *pdev)
> +{
> +	struct ts72xx_nand_data *data;
> +	struct device_node *child;
> +	struct mtd_info *mtd;
> +	int err;

> +	/* Allocate memory for the device structure (and zero it) */

Useless comment.

> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->controller.ops = &ts72xx_nand_ops;
> +	nand_controller_init(&data->controller);
> +	data->chip.controller = &data->controller;
> +
> +	data->io_base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(data->io_base))
> +		return PTR_ERR(data->io_base);
> +
> +	child = of_get_next_child(pdev->dev.of_node, NULL);

Why not using device property API from day 1?

	fwnode_get_next_child_node()

> +	if (!child)
> +		return dev_err_probe(&pdev->dev, -ENXIO,
> +				"ts72xx controller node should have exactly one child\n");

From now on you leak the reference count in error path.

> +	nand_set_flash_node(&data->chip, child);
> +	mtd = nand_to_mtd(&data->chip);
> +	mtd->dev.parent = &pdev->dev;
> +
> +	data->chip.legacy.IO_ADDR_R = data->io_base;
> +	data->chip.legacy.IO_ADDR_W = data->io_base;
> +	data->chip.legacy.cmd_ctrl = ts72xx_nand_hwcontrol;
> +	data->chip.legacy.dev_ready = ts72xx_nand_device_ready;
> +
> +	platform_set_drvdata(pdev, data);
> +
> +	/*
> +	 * This driver assumes that the default ECC engine should be TYPE_SOFT.
> +	 * Set ->engine_type before registering the NAND devices in order to
> +	 * provide a driver specific default value.
> +	 */
> +	data->chip.ecc.engine_type = NAND_ECC_ENGINE_TYPE_SOFT;
> +
> +	/* Scan to find existence of the device */
> +	err = nand_scan(&data->chip, 1);
> +	if (err)
> +		return err;
> +
> +	err = mtd_device_parse_register(mtd, NULL, NULL, NULL, 0);
> +	if (err) {
> +		nand_cleanup(&data->chip);

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


These 4 lines can be simply

	return err;

but see above.

> +}

...

> +static void ts72xx_nand_remove(struct platform_device *pdev)
> +{
> +	struct ts72xx_nand_data *data = platform_get_drvdata(pdev);
> +	struct nand_chip *chip = &data->chip;
> +	int ret;
> +
> +	ret = mtd_device_unregister(nand_to_mtd(chip));

> +	WARN_ON(ret);

Why?!  Is it like this in other MTD drivers?

> +	nand_cleanup(chip);
> +}
Miquel Raynal July 24, 2023, 7:09 a.m. UTC | #2
Hi Andy,

> > +static int ts72xx_nand_attach_chip(struct nand_chip *chip)
> > +{
> > +	switch (chip->ecc.engine_type) {
> > +	case NAND_ECC_ENGINE_TYPE_SOFT:
> > +		if (chip->ecc.algo == NAND_ECC_ALGO_UNKNOWN)
> > +			chip->ecc.algo = NAND_ECC_ALGO_HAMMING;
> > +		break;
> > +	case NAND_ECC_ENGINE_TYPE_ON_HOST:
> > +		return -EINVAL;
> > +	default:  
> 
> > +		break;  
> 
> Here it will return 0, is it a problem?

Seems ok, there are two other situations: on-die ECC engine and no ECC
engine, both do not require any specific handling on the controller
side.

> 
> > +	}
> > +
> > +	return 0;
> > +}  
> 
> ...
> 
> > +static void ts72xx_nand_remove(struct platform_device *pdev)
> > +{
> > +	struct ts72xx_nand_data *data = platform_get_drvdata(pdev);
> > +	struct nand_chip *chip = &data->chip;
> > +	int ret;
> > +
> > +	ret = mtd_device_unregister(nand_to_mtd(chip));  
> 
> > +	WARN_ON(ret);  
> 
> Why?!  Is it like this in other MTD drivers?

Yes, we did not yet change the internal machinery to return void, and
we don't want people to think getting errors there is normal.

> > +	nand_cleanup(chip);
> > +}  
> 

Thanks,
Miquèl
diff mbox series

Patch

diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
index b523354dfb00..94788da1a169 100644
--- a/drivers/mtd/nand/raw/Kconfig
+++ b/drivers/mtd/nand/raw/Kconfig
@@ -456,6 +456,13 @@  config MTD_NAND_RENESAS
 	  Enables support for the NAND controller found on Renesas R-Car
 	  Gen3 and RZ/N1 SoC families.
 
+config MTD_NAND_TS72XX
+	bool "ts72xx NAND controller"
+	depends on ARCH_EP93XX && HAS_IOMEM
+	help
+	  Enables support for NAND controller on ts72xx SBCs.
+	  This is a legacy driver based on gen_nand.
+
 comment "Misc"
 
 config MTD_SM_COMMON
diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile
index d93e861d8ba7..b85161c3296b 100644
--- a/drivers/mtd/nand/raw/Makefile
+++ b/drivers/mtd/nand/raw/Makefile
@@ -23,6 +23,7 @@  omap2_nand-objs := omap2.o
 obj-$(CONFIG_MTD_NAND_OMAP2) 		+= omap2_nand.o
 obj-$(CONFIG_MTD_NAND_OMAP_BCH_BUILD)	+= omap_elm.o
 obj-$(CONFIG_MTD_NAND_MARVELL)		+= marvell_nand.o
+obj-$(CONFIG_MTD_NAND_TS72XX)		+= technologic-nand-controller.o
 obj-$(CONFIG_MTD_NAND_PLATFORM)		+= plat_nand.o
 obj-$(CONFIG_MTD_NAND_PASEMI)		+= pasemi_nand.o
 obj-$(CONFIG_MTD_NAND_ORION)		+= orion_nand.o
diff --git a/drivers/mtd/nand/raw/technologic-nand-controller.c b/drivers/mtd/nand/raw/technologic-nand-controller.c
new file mode 100644
index 000000000000..c6a656210099
--- /dev/null
+++ b/drivers/mtd/nand/raw/technologic-nand-controller.c
@@ -0,0 +1,162 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Technologic Systems TS72xx NAND controller driver
+ *
+ * Copyright (C) 2023 Nikita Shubin <nikita.shubin@maquefel.me>
+ *
+ * Derived from: plat_nand.c
+ *  Author: Vitaly Wool <vitalywool@gmail.com>
+ */
+
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/platnand.h>
+
+#define TS72XX_NAND_CONTROL_ADDR_LINE	22	/* 0xN0400000 */
+#define TS72XX_NAND_BUSY_ADDR_LINE	23	/* 0xN0800000 */
+
+struct ts72xx_nand_data {
+	struct nand_controller	controller;
+	struct nand_chip	chip;
+	void __iomem		*io_base;
+};
+
+static int ts72xx_nand_attach_chip(struct nand_chip *chip)
+{
+	switch (chip->ecc.engine_type) {
+	case NAND_ECC_ENGINE_TYPE_SOFT:
+		if (chip->ecc.algo == NAND_ECC_ALGO_UNKNOWN)
+			chip->ecc.algo = NAND_ECC_ALGO_HAMMING;
+		break;
+	case NAND_ECC_ENGINE_TYPE_ON_HOST:
+		return -EINVAL;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+static const struct nand_controller_ops ts72xx_nand_ops = {
+	.attach_chip = ts72xx_nand_attach_chip,
+};
+
+static void ts72xx_nand_hwcontrol(struct nand_chip *chip,
+				  int cmd, unsigned int ctrl)
+{
+	if (ctrl & NAND_CTRL_CHANGE) {
+		void __iomem *addr = chip->legacy.IO_ADDR_R;
+		unsigned char bits;
+
+		addr += BIT(TS72XX_NAND_CONTROL_ADDR_LINE);
+
+		bits = readb(addr) & ~GENMASK(2, 0);
+		bits |= (ctrl & NAND_NCE) << 2;	/* bit 0 -> bit 2 */
+		bits |= (ctrl & NAND_CLE);	/* bit 1 -> bit 1 */
+		bits |= (ctrl & NAND_ALE) >> 2;	/* bit 2 -> bit 0 */
+
+		writeb(bits, addr);
+	}
+
+	if (cmd != NAND_CMD_NONE)
+		writeb(cmd, chip->legacy.IO_ADDR_W);
+}
+
+static int ts72xx_nand_device_ready(struct nand_chip *chip)
+{
+	void __iomem *addr = chip->legacy.IO_ADDR_R;
+
+	addr += BIT(TS72XX_NAND_BUSY_ADDR_LINE);
+
+	return !!(readb(addr) & BIT(5));
+}
+
+static int ts72xx_nand_probe(struct platform_device *pdev)
+{
+	struct ts72xx_nand_data *data;
+	struct device_node *child;
+	struct mtd_info *mtd;
+	int err;
+
+	/* Allocate memory for the device structure (and zero it) */
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->controller.ops = &ts72xx_nand_ops;
+	nand_controller_init(&data->controller);
+	data->chip.controller = &data->controller;
+
+	data->io_base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(data->io_base))
+		return PTR_ERR(data->io_base);
+
+	child = of_get_next_child(pdev->dev.of_node, NULL);
+	if (!child)
+		return dev_err_probe(&pdev->dev, -ENXIO,
+				"ts72xx controller node should have exactly one child\n");
+
+	nand_set_flash_node(&data->chip, child);
+	mtd = nand_to_mtd(&data->chip);
+	mtd->dev.parent = &pdev->dev;
+
+	data->chip.legacy.IO_ADDR_R = data->io_base;
+	data->chip.legacy.IO_ADDR_W = data->io_base;
+	data->chip.legacy.cmd_ctrl = ts72xx_nand_hwcontrol;
+	data->chip.legacy.dev_ready = ts72xx_nand_device_ready;
+
+	platform_set_drvdata(pdev, data);
+
+	/*
+	 * This driver assumes that the default ECC engine should be TYPE_SOFT.
+	 * Set ->engine_type before registering the NAND devices in order to
+	 * provide a driver specific default value.
+	 */
+	data->chip.ecc.engine_type = NAND_ECC_ENGINE_TYPE_SOFT;
+
+	/* Scan to find existence of the device */
+	err = nand_scan(&data->chip, 1);
+	if (err)
+		return err;
+
+	err = mtd_device_parse_register(mtd, NULL, NULL, NULL, 0);
+	if (err) {
+		nand_cleanup(&data->chip);
+		return err;
+	}
+
+	return 0;
+}
+
+static void ts72xx_nand_remove(struct platform_device *pdev)
+{
+	struct ts72xx_nand_data *data = platform_get_drvdata(pdev);
+	struct nand_chip *chip = &data->chip;
+	int ret;
+
+	ret = mtd_device_unregister(nand_to_mtd(chip));
+	WARN_ON(ret);
+	nand_cleanup(chip);
+}
+
+static const struct of_device_id ts72xx_id_table[] = {
+	{ .compatible = "technologic,ts7200-nand" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, ts72xx_id_table);
+
+static struct platform_driver ts72xx_nand_driver = {
+	.driver = {
+		.name = "ts72xx-nand",
+		.of_match_table = ts72xx_id_table,
+	},
+	.probe = ts72xx_nand_probe,
+	.remove_new = ts72xx_nand_remove,
+};
+module_platform_driver(ts72xx_nand_driver);
+