memory: omap-gpmc: mtd: nand: Support GPMC NAND on non-OMAP platforms

Deprecate nand register passing via platform data and use gpmc_omap_get_nand_ops() instead.

Signed-off-by Roger Quadros <[email protected]>

---
 arch/arm/mach-omap2/gpmc-nand.c              | 2 --
 drivers/mtd/nand/omap2.c                     | 9 ++++++++-
 include/linux/platform_data/mtd-nand-omap2.h | 4 +++-
 3 files changed, 11 insertions(+), 4 deletions(-)

arch/arm/mach-omap2/gpmc-nand.c +0 -2

index 72918c4..04e6998 100644
@@ -121,8 +121,6 @@ int gpmc_nand_init(struct omap_nand_platform_data *gpmc_nand_data,
 	if (err < 0)
 		goto out_free_cs;
 
-	gpmc_update_nand_reg(&gpmc_nand_data->reg, gpmc_nand_data->cs);
-
 	if (!gpmc_hwecc_bch_capable(gpmc_nand_data->ecc_opt)) {
 		pr_err("omap2-nand: Unsupported NAND ECC scheme selected\n");
 		err = -EINVAL;

drivers/mtd/nand/omap2.c +8 -1

index 60fa899..f214fe2 100644
@@ -28,6 +28,7 @@
 #include <linux/mtd/nand_bch.h>
 #include <linux/platform_data/elm.h>
 
+#include <linux/omap-gpmc.h>
 #include <linux/platform_data/mtd-nand-omap2.h>
 
 #define	DRIVER_NAME	"omap2-nand"

@@ -169,7 +170,9 @@ struct omap_nand_info {
 	} iomode;
 	u_char				*buf;
 	int					buf_len;
+	/* Interface to GPMC */
 	struct gpmc_nand_regs		reg;
+	struct gpmc_nand_ops		*ops;
 	/* generated at runtime depending on ECC algorithm and layout selected */
 	struct nand_ecclayout		oobinfo;
 	/* fields specific for BCHx_HW ECC scheme */

@@ -1677,9 +1680,13 @@ static int omap_nand_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, info);
 
+	info->ops = gpmc_omap_get_nand_ops(&info->reg, info->gpmc_cs);
+	if (!info->ops) {
+		dev_err(&pdev->dev, "Failed to get GPMC->NAND interface\n");
+		return -ENODEV;
+	}
 	info->pdev		= pdev;
 	info->gpmc_cs		= pdata->cs;
-	info->reg		= pdata->reg;
 	info->of_node		= pdata->of_node;
 	info->ecc_opt		= pdata->ecc_opt;
 	mtd			= &info->mtd;

include/linux/platform_data/mtd-nand-omap2.h +3 -1

index 090bbab..a067f58 100644
@@ -75,10 +75,12 @@ struct omap_nand_platform_data {
 	enum nand_io		xfer_type;
 	int			devsize;
 	enum omap_ecc           ecc_opt;
-	struct gpmc_nand_regs	reg;
 
 	/* for passing the partitions */
 	struct device_node	*of_node;
 	struct device_node	*elm_of_node;
+
+	/* deprecated */
+	struct gpmc_nand_regs	reg;
 };
 #endif

Signature · 1 line

2.1.4

Move NAND specific device tree parsing to NAND driver.

The NAND controller node must have a compatible id, register space resource and interrupt resource.

Signed-off-by Roger Quadros <[email protected]>

---
v4: Warn if using older incompatible DT i.e. compatible property not present in nand node.

 arch/arm/mach-omap2/gpmc-nand.c              |   5 +-
 drivers/memory/omap-gpmc.c                   | 143 +++++++--------------------
 drivers/mtd/nand/omap2.c                     | 136 +++++++++++++++++++++----
 include/linux/platform_data/mtd-nand-omap2.h |   3 +-
 4 files changed, 155 insertions(+), 132 deletions(-)

arch/arm/mach-omap2/gpmc-nand.c +1 -4

index ffe646a..e07ca27 100644
@@ -95,10 +95,7 @@ int gpmc_nand_init(struct omap_nand_platform_data *gpmc_nand_data,
 	gpmc_nand_res[1].start = gpmc_get_irq();
 
 	memset(&s, 0, sizeof(struct gpmc_settings));
-	if (gpmc_nand_data->of_node)
-		gpmc_read_settings_dt(gpmc_nand_data->of_node, &s);
-	else
-		gpmc_set_legacy(gpmc_nand_data, &s);
+	gpmc_set_legacy(gpmc_nand_data, &s);
 
 	s.device_nand = true;
 

drivers/memory/omap-gpmc.c +37 -106

index e75226d..318c187 100644
@@ -29,7 +29,6 @@
 #include <linux/of_device.h>
 #include <linux/of_platform.h>
 #include <linux/omap-gpmc.h>
-#include <linux/mtd/nand.h>
 #include <linux/pm_runtime.h>
 
 #include <linux/platform_data/mtd-nand-omap2.h>

@@ -1716,105 +1715,6 @@ static void __maybe_unused gpmc_read_timings_dt(struct device_node *np,
 		of_property_read_bool(np, "gpmc,time-para-granularity");
 }
 
-#if IS_ENABLED(CONFIG_MTD_NAND)
-
-static const char * const nand_xfer_types[] = {
-	[NAND_OMAP_PREFETCH_POLLED]		= "prefetch-polled",
-	[NAND_OMAP_POLLED]			= "polled",
-	[NAND_OMAP_PREFETCH_DMA]		= "prefetch-dma",
-	[NAND_OMAP_PREFETCH_IRQ]		= "prefetch-irq",
-};
-
-static int gpmc_probe_nand_child(struct platform_device *pdev,
-				 struct device_node *child)
-{
-	u32 val;
-	const char *s;
-	struct gpmc_timings gpmc_t;
-	struct omap_nand_platform_data *gpmc_nand_data;
-
-	if (of_property_read_u32(child, "reg", &val) < 0) {
-		dev_err(&pdev->dev, "%s has no 'reg' property\n",
-			child->full_name);
-		return -ENODEV;
-	}
-
-	gpmc_nand_data = devm_kzalloc(&pdev->dev, sizeof(*gpmc_nand_data),
-				      GFP_KERNEL);
-	if (!gpmc_nand_data)
-		return -ENOMEM;
-
-	gpmc_nand_data->cs = val;
-	gpmc_nand_data->of_node = child;
-
-	/* Detect availability of ELM module */
-	gpmc_nand_data->elm_of_node = of_parse_phandle(child, "ti,elm-id", 0);
-	if (gpmc_nand_data->elm_of_node == NULL)
-		gpmc_nand_data->elm_of_node =
-					of_parse_phandle(child, "elm_id", 0);
-
-	/* select ecc-scheme for NAND */
-	if (of_property_read_string(child, "ti,nand-ecc-opt", &s)) {
-		pr_err("%s: ti,nand-ecc-opt not found\n", __func__);
-		return -ENODEV;
-	}
-
-	if (!strcmp(s, "sw"))
-		gpmc_nand_data->ecc_opt = OMAP_ECC_HAM1_CODE_SW;
-	else if (!strcmp(s, "ham1") ||
-		 !strcmp(s, "hw") || !strcmp(s, "hw-romcode"))
-		gpmc_nand_data->ecc_opt =
-				OMAP_ECC_HAM1_CODE_HW;
-	else if (!strcmp(s, "bch4"))
-		if (gpmc_nand_data->elm_of_node)
-			gpmc_nand_data->ecc_opt =
-				OMAP_ECC_BCH4_CODE_HW;
-		else
-			gpmc_nand_data->ecc_opt =
-				OMAP_ECC_BCH4_CODE_HW_DETECTION_SW;
-	else if (!strcmp(s, "bch8"))
-		if (gpmc_nand_data->elm_of_node)
-			gpmc_nand_data->ecc_opt =
-				OMAP_ECC_BCH8_CODE_HW;
-		else
-			gpmc_nand_data->ecc_opt =
-				OMAP_ECC_BCH8_CODE_HW_DETECTION_SW;
-	else if (!strcmp(s, "bch16"))
-		if (gpmc_nand_data->elm_of_node)
-			gpmc_nand_data->ecc_opt =
-				OMAP_ECC_BCH16_CODE_HW;
-		else
-			pr_err("%s: BCH16 requires ELM support\n", __func__);
-	else
-		pr_err("%s: ti,nand-ecc-opt invalid value\n", __func__);
-
-	/* select data transfer mode for NAND controller */
-	if (!of_property_read_string(child, "ti,nand-xfer-type", &s))
-		for (val = 0; val < ARRAY_SIZE(nand_xfer_types); val++)
-			if (!strcasecmp(s, nand_xfer_types[val])) {
-				gpmc_nand_data->xfer_type = val;
-				break;
-			}
-
-	gpmc_nand_data->flash_bbt = of_get_nand_on_flash_bbt(child);
-
-	val = of_get_nand_bus_width(child);
-	if (val == 16)
-		gpmc_nand_data->devsize = NAND_BUSWIDTH_16;
-
-	gpmc_read_timings_dt(child, &gpmc_t);
-	gpmc_nand_init(gpmc_nand_data, &gpmc_t);
-
-	return 0;
-}
-#else
-static int gpmc_probe_nand_child(struct platform_device *pdev,
-				 struct device_node *child)
-{
-	return 0;
-}
-#endif
-
 #if IS_ENABLED(CONFIG_MTD_ONENAND)
 static int gpmc_probe_onenand_child(struct platform_device *pdev,
 				 struct device_node *child)

@@ -1933,9 +1833,42 @@ static int gpmc_probe_generic_child(struct platform_device *pdev,
 		goto err;
 	}
 
-	ret = of_property_read_u32(child, "bank-width", &gpmc_s.device_width);
-	if (ret < 0)
-		goto err;
+	if (of_node_cmp(child->name, "nand") == 0) {
+		/* NAND specific setup */
+		u32 val;
+
+		/* Warn about older DT blobs with no compatible property */
+		if (!of_property_read_bool(child, "compatible")) {
+			dev_warn(&pdev->dev,
+				 "Incompatible NAND node: missing compatible");
+			ret = -EINVAL;
+			goto err;
+		}
+
+		val = of_get_nand_bus_width(child);
+		switch (val) {
+		case 8:
+			gpmc_s.device_width = GPMC_DEVWIDTH_8BIT;
+			break;
+		case 16:
+			gpmc_s.device_width = GPMC_DEVWIDTH_16BIT;
+			break;
+		default:
+			dev_err(&pdev->dev, "%s: invalid 'nand-bus-width'\n",
+				child->name);
+			ret = -EINVAL;
+			goto err;
+		}
+
+		/* disable write protect */
+		gpmc_configure(GPMC_CONFIG_WP, 0);
+		gpmc_s.device_nand = true;
+	} else {
+		ret = of_property_read_u32(child, "bank-width",
+					   &gpmc_s.device_width);
+		if (ret < 0)
+			goto err;
+	}
 
 	ret = gpmc_cs_program_settings(cs, &gpmc_s);
 	if (ret < 0)

@@ -2018,9 +1951,7 @@ static int gpmc_probe_dt(struct platform_device *pdev)
 		if (!child->name)
 			continue;
 
-		if (of_node_cmp(child->name, "nand") == 0)
-			ret = gpmc_probe_nand_child(pdev, child);
-		else if (of_node_cmp(child->name, "onenand") == 0)
+		if (of_node_cmp(child->name, "onenand") == 0)
 			ret = gpmc_probe_onenand_child(pdev, child);
 		else
 			ret = gpmc_probe_generic_child(pdev, child);

drivers/mtd/nand/omap2.c +103 -17

index c35405c..228f498 100644
@@ -24,6 +24,7 @@
 #include <linux/slab.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/of_mtd.h>
 
 #include <linux/mtd/nand_bch.h>
 #include <linux/platform_data/elm.h>

@@ -177,6 +178,8 @@ struct omap_nand_info {
 	struct gpmc_nand_regs		reg;
 	struct gpmc_nand_ops		*ops;
 	/* generated at runtime depending on ECC algorithm and layout selected */
+	bool				flash_bbt;
+	/* generated at runtime depending on ECC algorithm and layout */
 	struct nand_ecclayout		oobinfo;
 	/* fields specific for BCHx_HW ECC scheme */
 	struct device			*elm_dev;

@@ -1668,10 +1671,84 @@ static bool omap2_nand_ecc_check(struct omap_nand_info *info,
 	return true;
 }
 
+static const char * const nand_xfer_types[] = {
+	[NAND_OMAP_PREFETCH_POLLED] = "prefetch-polled",
+	[NAND_OMAP_POLLED] = "polled",
+	[NAND_OMAP_PREFETCH_DMA] = "prefetch-dma",
+	[NAND_OMAP_PREFETCH_IRQ] = "prefetch-irq",
+};
+
+static int omap_get_dt_info(struct device *dev, struct omap_nand_info *info)
+{
+	struct device_node *child = dev->of_node;
+	int i;
+	const char *s;
+
+	/* In old bindings, CS num is embedded in reg property */
+	if (of_property_read_u32(child, "reg", &info->gpmc_cs) < 0) {
+		dev_err(dev, "reg not found in DT\n");
+		return -EINVAL;
+	}
+
+	/* detect availability of ELM module. Won't be present pre-OMAP4 */
+	info->elm_of_node = of_parse_phandle(child, "ti,elm-id", 0);
+	if (!info->elm_of_node)
+		dev_dbg(dev, "ti,elm-id not in DT\n");
+
+	/* select ecc-scheme for NAND */
+	if (of_property_read_string(child, "ti,nand-ecc-opt", &s)) {
+		dev_err(dev, "ti,nand-ecc-opt not found\n");
+		return -EINVAL;
+	}
+
+	if (!strcmp(s, "sw")) {
+		info->ecc_opt = OMAP_ECC_HAM1_CODE_SW;
+	} else if (!strcmp(s, "ham1") ||
+		   !strcmp(s, "hw") || !strcmp(s, "hw-romcode")) {
+		info->ecc_opt =	OMAP_ECC_HAM1_CODE_HW;
+	} else if (!strcmp(s, "bch4")) {
+		if (info->elm_of_node)
+			info->ecc_opt = OMAP_ECC_BCH4_CODE_HW;
+		else
+			info->ecc_opt = OMAP_ECC_BCH4_CODE_HW_DETECTION_SW;
+	} else if (!strcmp(s, "bch8")) {
+		if (info->elm_of_node)
+			info->ecc_opt = OMAP_ECC_BCH8_CODE_HW;
+		else
+			info->ecc_opt = OMAP_ECC_BCH8_CODE_HW_DETECTION_SW;
+	} else if (!strcmp(s, "bch16")) {
+		info->ecc_opt =	OMAP_ECC_BCH16_CODE_HW;
+	} else {
+		dev_err(dev, "unrecognized value for ti,nand-ecc-opt\n");
+		return -EINVAL;
+	}
+
+	/* select data transfer mode */
+	if (!of_property_read_string(child, "ti,nand-xfer-type", &s)) {
+		for (i = 0; i < ARRAY_SIZE(nand_xfer_types); i++) {
+			if (!strcasecmp(s, nand_xfer_types[i])) {
+				info->xfer_type = i;
+				goto next;
+			}
+		}
+
+		dev_err(dev, "unrecognized value for ti,nand-xfer-type\n");
+		return -EINVAL;
+	}
+
+next:
+	of_get_nand_on_flash_bbt(child);
+
+	if (of_get_nand_bus_width(child) == 16)
+		info->devsize = NAND_BUSWIDTH_16;
+
+	return 0;
+}
+
 static int omap_nand_probe(struct platform_device *pdev)
 {
 	struct omap_nand_info		*info;
-	struct omap_nand_platform_data	*pdata;
+	struct omap_nand_platform_data	*pdata = NULL;
 	struct mtd_info			*mtd;
 	struct nand_chip		*nand_chip;
 	struct nand_ecclayout		*ecclayout;

@@ -1682,33 +1759,42 @@ static int omap_nand_probe(struct platform_device *pdev)
 	unsigned			oob_index;
 	struct resource			*res;
 	struct mtd_part_parser_data	ppdata = {};
-
-	pdata = dev_get_platdata(&pdev->dev);
-	if (pdata == NULL) {
-		dev_err(&pdev->dev, "platform data missing\n");
-		return -ENODEV;
-	}
+	struct device			*dev = &pdev->dev;
 
 	info = devm_kzalloc(&pdev->dev, sizeof(struct omap_nand_info),
 				GFP_KERNEL);
 	if (!info)
 		return -ENOMEM;
 
-	platform_set_drvdata(pdev, info);
+	info->pdev = pdev;
 
+	if (dev->of_node) {
+		if (omap_get_dt_info(dev, info))
+			return -EINVAL;
+	} else {
+		pdata = dev_get_platdata(&pdev->dev);
+		if (!pdata) {
+			dev_err(&pdev->dev, "platform data missing\n");
+			return -EINVAL;
+		}
+
+		info->gpmc_cs = pdata->cs;
+		info->reg = pdata->reg;
+		info->of_node = pdata->of_node;
+		info->ecc_opt = pdata->ecc_opt;
+		info->dev_ready	= pdata->dev_ready;
+		info->xfer_type = pdata->xfer_type;
+		info->devsize = pdata->devsize;
+		info->elm_of_node = pdata->elm_of_node;
+		info->flash_bbt = pdata->flash_bbt;
+	}
+
+	platform_set_drvdata(pdev, info);
 	info->ops = gpmc_omap_get_nand_ops(&info->reg, info->gpmc_cs);
 	if (!info->ops) {
 		dev_err(&pdev->dev, "Failed to get GPMC->NAND interface\n");
 		return -ENODEV;
 	}
-	info->pdev		= pdev;
-	info->gpmc_cs		= pdata->cs;
-	info->of_node		= pdata->of_node;
-	info->ecc_opt		= pdata->ecc_opt;
-	info->dev_ready	= pdata->dev_ready;
-	info->xfer_type = pdata->xfer_type;
-	info->devsize = pdata->devsize;
-	info->elm_of_node = pdata->elm_of_node;
 
 	mtd			= &info->mtd;
 	mtd->priv		= &info->nand;

@@ -1744,7 +1830,7 @@ static int omap_nand_probe(struct platform_device *pdev)
 		nand_chip->chip_delay = 50;
 	}
 
-	if (pdata->flash_bbt)
+	if (info->flash_bbt)
 		nand_chip->bbt_options |= NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB;
 	else
 		nand_chip->options |= NAND_SKIP_BBTSCAN;

drivers/mtd/nand/omap2.c +11 -0

+		mtd_device_parse_register(mtd, NULL, &ppdata, NULL, 0);
+
+	} else {
+		mtd_device_register(mtd, pdata->parts, pdata->nr_parts);
+	}
 
 	platform_set_drvdata(pdev, mtd);
 

@@ -2083,11 +2173,17 @@ static int omap_nand_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct of_device_id omap_nand_ids[] = {
+	{ .compatible = "ti,omap2-nand", },
+	{},
+};
+
 static struct platform_driver omap_nand_driver = {
 	.probe		= omap_nand_probe,
 	.remove		= omap_nand_remove,
 	.driver		= {
 		.name	= DRIVER_NAME,
+		.of_match_table = of_match_ptr(omap_nand_ids),
 	},
 };
 

include/linux/platform_data/mtd-nand-omap2.h +0 -0

index a067f58..ff27e5a 100644

Hi Roger,

Quoted 2 lines · Roger Quadros

Move NAND specific device tree parsing to NAND driver.

Quoted 4 lines

The NAND controller node must have a compatible id, register space resource and interrupt resource.

Signed-off-by Roger Quadros <[email protected]>

This one's going to need rebased, as there are some conflicting of_node changes in l2-mtd.git.

Quoted 10 lines

---
v4: Warn if using older incompatible DT i.e. compatible property not present in nand node.

 arch/arm/mach-omap2/gpmc-nand.c              |   5 +-
 drivers/memory/omap-gpmc.c                   | 143 +++++++--------------------
 drivers/mtd/nand/omap2.c                     | 136 +++++++++++++++++++++----
 include/linux/platform_data/mtd-nand-omap2.h |   3 +-
 4 files changed, 155 insertions(+), 132 deletions(-)

Also, this is going to be hard to manage across trees, as you touch three drivers all at once. Is it not possible to split any of this apart better?

Quoted 29 lines

arch/arm/mach-omap2/gpmc-nand.c +1 -4

index ffe646a..e07ca27 100644
@@ -95,10 +95,7 @@ int gpmc_nand_init(struct omap_nand_platform_data *gpmc_nand_data,
 	gpmc_nand_res[1].start = gpmc_get_irq();
 
 	memset(&s, 0, sizeof(struct gpmc_settings));
-	if (gpmc_nand_data->of_node)
-		gpmc_read_settings_dt(gpmc_nand_data->of_node, &s);
-	else
-		gpmc_set_legacy(gpmc_nand_data, &s);
+	gpmc_set_legacy(gpmc_nand_data, &s);
 
 	s.device_nand = true;
 

drivers/memory/omap-gpmc.c +0 -1

index e75226d..318c187 100644
@@ -29,7 +29,6 @@
 #include <linux/of_device.h>
 #include <linux/of_platform.h>
 #include <linux/omap-gpmc.h>
-#include <linux/mtd/nand.h>
 #include <linux/pm_runtime.h>
 

drivers/memory/omap-gpmc.c

Quoted 6 lines

drivers/memory/omap-gpmc.c +2 -3

-	ppdata.of_node = pdata->of_node;
-	mtd_device_parse_register(mtd, NULL, &ppdata, pdata->parts,
-				  pdata->nr_parts);
+	if (dev->of_node) {
+		ppdata.of_node = dev->of_node;

The latest l2-mtd.git changed how the partitions' of_node is passed. Now this is handled by nand_set_flash_node().

Quoted 39 lines

drivers/memory/omap-gpmc.c +11 -0

+		mtd_device_parse_register(mtd, NULL, &ppdata, NULL, 0);
+
+	} else {
+		mtd_device_register(mtd, pdata->parts, pdata->nr_parts);
+	}
 
 	platform_set_drvdata(pdev, mtd);

@@ -2083,11 +2173,17 @@ static int omap_nand_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct of_device_id omap_nand_ids[] = {
+	{ .compatible = "ti,omap2-nand", },
+	{},
+};
+
 static struct platform_driver omap_nand_driver = {
 	.probe		= omap_nand_probe,
 	.remove		= omap_nand_remove,
 	.driver		= {
 		.name	= DRIVER_NAME,
+		.of_match_table = of_match_ptr(omap_nand_ids),
 	},
 };
 

include/linux/platform_data/mtd-nand-omap2.h +0 -2

index a067f58..ff27e5a 100644
@@ -76,11 +76,10 @@ struct omap_nand_platform_data {
 	int			devsize;
 	enum omap_ecc           ecc_opt;
 
-	/* for passing the partitions */
-	struct device_node	*of_node;
 	struct device_node	*elm_of_node;
 

include/linux/platform_data/mtd-nand-omap2.h struct omap_nand_platform_data { · line 83

Quoted 3 lines

include/linux/platform_data/mtd-nand-omap2.h +1 -0

 	/* deprecated */
 	struct gpmc_nand_regs	reg;
+	struct device_node	*of_node;

I'm a little confused here. Do you have a mixed platform data / device tree setup here? That's odd. (It also seems if that was really necessary, you could have the board file set pdev->dev.of_node before registering it, then you don't need this field.) But really, if you're partly using device tree, can't you just convert completely? Or is this a two-phase process, and you're planning to convert omap2 to full device tree?

Quoted 7 lines · Brian Norris

Quoted 5 lines · Roger Quadros

 arch/arm/mach-omap2/gpmc-nand.c              |   5 +-
 drivers/memory/omap-gpmc.c                   | 143 +++++++--------------------
 drivers/mtd/nand/omap2.c                     | 136 +++++++++++++++++++++----
 include/linux/platform_data/mtd-nand-omap2.h |   3 +-
 4 files changed, 155 insertions(+), 132 deletions(-)

Quoted 3 lines

Also, this is going to be hard to manage across trees, as you touch three drivers all at once. Is it not possible to split any of this apart better?

Will need some more effort and I can do it. Butm if we're going to start an with immutable branch with everything in, is it worth the effort?

Quoted 12 lines

Quoted 12 lines

include/linux/platform_data/mtd-nand-omap2.h +0 -2

index a067f58..ff27e5a 100644
@@ -76,11 +76,10 @@ struct omap_nand_platform_data {
 	int			devsize;
 	enum omap_ecc           ecc_opt;
 
-	/* for passing the partitions */
-	struct device_node	*of_node;
 	struct device_node	*elm_of_node;
 

Quoted 11 lines

include/linux/platform_data/mtd-nand-omap2.h struct omap_nand_platform_data { · line 83

Quoted 3 lines

include/linux/platform_data/mtd-nand-omap2.h +1 -0

 	/* deprecated */
 	struct gpmc_nand_regs	reg;
+	struct device_node	*of_node;

I'm a little confused here. Do you have a mixed platform data / device tree setup here? That's odd. (It also seems if that was really necessary, you could have the board file set pdev->dev.of_node before registering it, then you don't need this field.) But really, if you're partly using device tree, can't you just convert completely? Or is this a two-phase process, and you're planning to convert omap2 to full device tree?

The existing device tree implementation for omap2-nand was like this:
omap-gpmc.c driver was creating a platform device for the nand device and passing the device node via platform data.

After this series we no longer do this and that's why of_node is marked deprecated in platform data. I might as well could just get rid of it but wasn't sure of how we're going to integrate the changes into the subsystem trees so just marked it deprecated.

The GPMC WAIT pin status are now available over gpiolib. Update the omap_dev_ready() function to use gpio instead of directly accessing GPMC register space.

Signed-off-by Roger Quadros <[email protected]>

---
 drivers/mtd/nand/omap2.c                     | 29 +++++++++++++++++-----------
 include/linux/platform_data/mtd-nand-omap2.h |  2 +-
 2 files changed, 19 insertions(+), 12 deletions(-)

drivers/mtd/nand/omap2.c +10 -10

index 228f498..d0f2620 100644
@@ -12,6 +12,7 @@
 #include <linux/dmaengine.h>
 #include <linux/dma-mapping.h>
 #include <linux/delay.h>
+#include <linux/gpio/consumer.h>
 #include <linux/module.h>
 #include <linux/interrupt.h>
 #include <linux/jiffies.h>

@@ -184,6 +185,8 @@ struct omap_nand_info {
 	/* fields specific for BCHx_HW ECC scheme */
 	struct device			*elm_dev;
 	struct device_node		*of_node;
+	/* NAND ready gpio */
+	struct gpio_desc		*ready_gpiod;
 };
 
 /**

@@ -1047,22 +1050,17 @@ static int omap_wait(struct mtd_info *mtd, struct nand_chip *chip)
 }
 
 /**
- * omap_dev_ready - calls the platform specific dev_ready function
+ * omap_dev_ready - checks the NAND Ready GPIO line
  * @mtd: MTD device structure
+ *
+ * Returns true if ready and false if busy.
  */
 static int omap_dev_ready(struct mtd_info *mtd)
 {
-	unsigned int val = 0;
 	struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
 							mtd);
 
-	val = readl(info->reg.gpmc_status);
-
-	if ((val & 0x100) == 0x100) {
-		return 1;
-	} else {
-		return 0;
-	}
+	return gpiod_get_value(info->ready_gpiod);
 }
 
 /**

@@ -1782,7 +1780,9 @@ static int omap_nand_probe(struct platform_device *pdev)
 		info->reg = pdata->reg;
 		info->of_node = pdata->of_node;
 		info->ecc_opt = pdata->ecc_opt;
-		info->dev_ready	= pdata->dev_ready;
+		if (pdata->dev_ready)
+			dev_info(&pdev->dev, "pdata->dev_ready is deprecated\n");
+
 		info->xfer_type = pdata->xfer_type;
 		info->devsize = pdata->devsize;
 		info->elm_of_node = pdata->elm_of_node;

drivers/mtd/nand/omap2.c +6 -1

+	if (IS_ERR(info->ready_gpiod)) {
+		dev_err(dev, "failed to get ready gpio\n");
+		return PTR_ERR(info->ready_gpiod);
+	}
+
 	/*
 	 * If RDY/BSY line is connected to OMAP then use the omap ready
 	 * function and the generic nand_wait function which reads the status

@@ -1822,7 +1829,7 @@ static int omap_nand_probe(struct platform_device *pdev)
 	 * chip delay which is slightly more than tR (AC Timing) of the NAND
 	 * device and read status register until you get a failure or success
 	 */
-	if (info->dev_ready) {
+	if (info->ready_gpiod) {
 		nand_chip->dev_ready = omap_dev_ready;
 		nand_chip->chip_delay = 0;
 	} else {

include/linux/platform_data/mtd-nand-omap2.h +1 -1

index ff27e5a..19e509d 100644
@@ -70,7 +70,6 @@ struct omap_nand_platform_data {
 	int			cs;
 	struct mtd_partition	*parts;
 	int			nr_parts;
-	bool			dev_ready;
 	bool			flash_bbt;
 	enum nand_io		xfer_type;
 	int			devsize;

@@ -81,5 +80,6 @@ struct omap_nand_platform_data {
 	/* deprecated */
 	struct gpmc_nand_regs	reg;
 	struct device_node	*of_node;
+	bool			dev_ready;
 };
 #endif

Signature · 1 line

2.1.4

+ others

A few comments below.

Quoted 70 lines · Roger Quadros

The GPMC WAIT pin status are now available over gpiolib. Update the omap_dev_ready() function to use gpio instead of directly accessing GPMC register space.

Signed-off-by Roger Quadros <[email protected]>

---
 drivers/mtd/nand/omap2.c                     | 29 +++++++++++++++++-----------
 include/linux/platform_data/mtd-nand-omap2.h |  2 +-
 2 files changed, 19 insertions(+), 12 deletions(-)

drivers/mtd/nand/omap2.c +10 -10

index 228f498..d0f2620 100644
@@ -12,6 +12,7 @@
 #include <linux/dmaengine.h>
 #include <linux/dma-mapping.h>
 #include <linux/delay.h>
+#include <linux/gpio/consumer.h>
 #include <linux/module.h>
 #include <linux/interrupt.h>
 #include <linux/jiffies.h>

@@ -184,6 +185,8 @@ struct omap_nand_info {
 	/* fields specific for BCHx_HW ECC scheme */
 	struct device			*elm_dev;
 	struct device_node		*of_node;
+	/* NAND ready gpio */
+	struct gpio_desc		*ready_gpiod;
 };
 
 /**

@@ -1047,22 +1050,17 @@ static int omap_wait(struct mtd_info *mtd, struct nand_chip *chip)
 }
 
 /**
- * omap_dev_ready - calls the platform specific dev_ready function
+ * omap_dev_ready - checks the NAND Ready GPIO line
  * @mtd: MTD device structure
+ *
+ * Returns true if ready and false if busy.
  */
 static int omap_dev_ready(struct mtd_info *mtd)
 {
-	unsigned int val = 0;
 	struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
 							mtd);
 
-	val = readl(info->reg.gpmc_status);
-
-	if ((val & 0x100) == 0x100) {
-		return 1;
-	} else {
-		return 0;
-	}
+	return gpiod_get_value(info->ready_gpiod);
 }
 
 /**

@@ -1782,7 +1780,9 @@ static int omap_nand_probe(struct platform_device *pdev)
 		info->reg = pdata->reg;
 		info->of_node = pdata->of_node;
 		info->ecc_opt = pdata->ecc_opt;
-		info->dev_ready	= pdata->dev_ready;
+		if (pdata->dev_ready)
+			dev_info(&pdev->dev, "pdata->dev_ready is deprecated\n");
+
 		info->xfer_type = pdata->xfer_type;
 		info->devsize = pdata->devsize;
 		info->elm_of_node = pdata->elm_of_node;

drivers/mtd/nand/omap2.c static int omap_nand_probe(struct platform_device *pdev) · line 1819

Quoted 5 lines

drivers/mtd/nand/omap2.c +2 -0

@@ -1815,6 +1815,13 @@ static int omap_nand_probe(struct platform_device *pdev)
 	nand_chip->IO_ADDR_W = nand_chip->IO_ADDR_R;
 	nand_chip->cmd_ctrl  = omap_hwcontrol;
 
+	info->ready_gpiod = devm_gpiod_get_optional(&pdev->dev, "ready",
+						    GPIOD_IN);

Others have been looking at using GPIOs for the ready/busy pin too. At a minimum, we need an updated DT binding doc for this, since I see you're adding this via device tree in a later patch (I don't see any DT binding patch for this; but I could just be overlooking it). It'd also be great if this support was moved to nand_dt_init() so other platforms can benefit, but I won't require that.

Also, previous [0] proposers had suggested 'rb-gpios', not 'ready-gpio' (the hardware docs typically call it 'rb' for ready/busy, FWIW). I don't really care, but the name should be going into a doc, so we can choose the same one everywhere.

EDIT: looks like the discussion was partly here [1] and it seems we're landing on "rb-gpios" in the latest version [2]. Can we stick with that?

Regards,
Brian

[0] "Previous" may be subject to debate, as both series have been going for several revisions.
[1] http://patchwork.ozlabs.org/patch/515327/
[2] http://patchwork.ozlabs.org/patch/526819/

Quoted 31 lines

drivers/mtd/nand/omap2.c +6 -1

+	if (IS_ERR(info->ready_gpiod)) {
+		dev_err(dev, "failed to get ready gpio\n");
+		return PTR_ERR(info->ready_gpiod);
+	}
+
 	/*
 	 * If RDY/BSY line is connected to OMAP then use the omap ready
 	 * function and the generic nand_wait function which reads the status

@@ -1822,7 +1829,7 @@ static int omap_nand_probe(struct platform_device *pdev)
 	 * chip delay which is slightly more than tR (AC Timing) of the NAND
 	 * device and read status register until you get a failure or success
 	 */
-	if (info->dev_ready) {
+	if (info->ready_gpiod) {
 		nand_chip->dev_ready = omap_dev_ready;
 		nand_chip->chip_delay = 0;
 	} else {

include/linux/platform_data/mtd-nand-omap2.h +0 -1

index ff27e5a..19e509d 100644
@@ -70,7 +70,6 @@ struct omap_nand_platform_data {
 	int			cs;
 	struct mtd_partition	*parts;
 	int			nr_parts;
-	bool			dev_ready;
 	bool			flash_bbt;
 	enum nand_io		xfer_type;
 	int			devsize;

@@ -81,5 +80,6 @@ struct omap_nand_platform_data {
 	/* deprecated */

Quoted 8 lines

include/linux/platform_data/mtd-nand-omap2.h +1 -0

 	struct gpmc_nand_regs	reg;
 	struct device_node	*of_node;
+	bool			dev_ready;
 };
 #endif

Signature · 1 line

2.1.4

Quoted 13 lines · Brian Norris

Others have been looking at using GPIOs for the ready/busy pin too. At a minimum, we need an updated DT binding doc for this, since I see you're adding this via device tree in a later patch (I don't see any DT binding patch for this; but I could just be overlooking it). It'd also be great if this support was moved to nand_dt_init() so other platforms can benefit, but I won't require that.

Also, previous [0] proposers had suggested 'rb-gpios', not 'ready-gpio' (the hardware docs typically call it 'rb' for ready/busy, FWIW). I don't really care, but the name should be going into a doc, so we can choose the same one everywhere.

Quoted 2 lines

EDIT: looks like the discussion was partly here [1] and it seems we're landing on "rb-gpios" in the latest version [2]. Can we stick with that?

Why should it be "rb-gpios" and not "rb-gpio"?
I don't think there are multiple gpios for r/b# function.

Hi Roger,

Quoted 17 lines · On Tue, 27 Oct 2015 10:03:02 +0200 Roger Quadros…

Quoted 15 lines · Brian Norris

Others have been looking at using GPIOs for the ready/busy pin too. At a minimum, we need an updated DT binding doc for this, since I see you're adding this via device tree in a later patch (I don't see any DT binding patch for this; but I could just be overlooking it). It'd also be great if this support was moved to nand_dt_init() so other platforms can benefit, but I won't require that.

Also, previous [0] proposers had suggested 'rb-gpios', not 'ready-gpio' (the hardware docs typically call it 'rb' for ready/busy, FWIW). I don't really care, but the name should be going into a doc, so we can choose the same one everywhere.

EDIT: looks like the discussion was partly here [1] and it seems we're landing on "rb-gpios" in the latest version [2]. Can we stick with that?

Quoted 2 lines

Why should it be "rb-gpios" and not "rb-gpio"?
I don't think there are multiple gpios for r/b# function.

Because it's supposed to be a generic binding, and some NAND chips embed several dies, thus exposing several CS and RB pins, hence the rb-gpios name.
Also, as described here [1], the convention is to name your property <name>-gpios even if you only need one gpio.

+ others

A few comments below.

Quoted 73 lines · Roger Quadros

The GPMC WAIT pin status are now available over gpiolib. Update the omap_dev_ready() function to use gpio instead of directly accessing GPMC register space.

Signed-off-by Roger Quadros <[email protected]>

---
 drivers/mtd/nand/omap2.c                     | 29 +++++++++++++++++-----------
 include/linux/platform_data/mtd-nand-omap2.h |  2 +-
 2 files changed, 19 insertions(+), 12 deletions(-)

drivers/mtd/nand/omap2.c +10 -10

index 228f498..d0f2620 100644
@@ -12,6 +12,7 @@
 #include <linux/dmaengine.h>
 #include <linux/dma-mapping.h>
 #include <linux/delay.h>
+#include <linux/gpio/consumer.h>
 #include <linux/module.h>
 #include <linux/interrupt.h>
 #include <linux/jiffies.h>

@@ -184,6 +185,8 @@ struct omap_nand_info {
 	/* fields specific for BCHx_HW ECC scheme */
 	struct device			*elm_dev;
 	struct device_node		*of_node;
+	/* NAND ready gpio */
+	struct gpio_desc		*ready_gpiod;
 };
 
 /**

@@ -1047,22 +1050,17 @@ static int omap_wait(struct mtd_info *mtd, struct nand_chip *chip)
 }
 
 /**
- * omap_dev_ready - calls the platform specific dev_ready function
+ * omap_dev_ready - checks the NAND Ready GPIO line
  * @mtd: MTD device structure
+ *
+ * Returns true if ready and false if busy.
  */
 static int omap_dev_ready(struct mtd_info *mtd)
 {
-	unsigned int val = 0;
 	struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
 							mtd);
 
-	val = readl(info->reg.gpmc_status);
-
-	if ((val & 0x100) == 0x100) {
-		return 1;
-	} else {
-		return 0;
-	}
+	return gpiod_get_value(info->ready_gpiod);
 }
 
 /**

@@ -1782,7 +1780,9 @@ static int omap_nand_probe(struct platform_device *pdev)
 		info->reg = pdata->reg;
 		info->of_node = pdata->of_node;
 		info->ecc_opt = pdata->ecc_opt;
-		info->dev_ready	= pdata->dev_ready;
+		if (pdata->dev_ready)
+			dev_info(&pdev->dev, "pdata->dev_ready is deprecated\n");
+
 		info->xfer_type = pdata->xfer_type;
 		info->devsize = pdata->devsize;
 		info->elm_of_node = pdata->elm_of_node;

@@ -1815,6 +1815,13 @@ static int omap_nand_probe(struct platform_device *pdev)
 	nand_chip->IO_ADDR_W = nand_chip->IO_ADDR_R;
 	nand_chip->cmd_ctrl  = omap_hwcontrol;
 

drivers/mtd/nand/omap2.c static int omap_nand_probe(struct platform_device *pdev) · line 1819

Quoted 2 lines

drivers/mtd/nand/omap2.c +2 -0

+	info->ready_gpiod = devm_gpiod_get_optional(&pdev->dev, "ready",
+						    GPIOD_IN);

Others have been looking at using GPIOs for the ready/busy pin too. At a minimum, we need an updated DT binding doc for this, since I see you're adding this via device tree in a later patch (I don't see any DT binding patch for this; but I could just be overlooking it). It'd also be great if this support was moved to nand_dt_init() so other platforms can benefit, but I won't require that.

Quoted 16 lines · On Mon, 26 Oct 2015 13:49:00 -0700 Brian Norris <[email protected]>…

Quoted 15 lines · Roger Quadros

drivers/mtd/nand/omap2.c +3 -1

@@ -1782,7 +1780,9 @@ static int omap_nand_probe(struct platform_device *pdev)
 		info->reg = pdata->reg;
 		info->of_node = pdata->of_node;
 		info->ecc_opt = pdata->ecc_opt;
-		info->dev_ready	= pdata->dev_ready;
+		if (pdata->dev_ready)
+			dev_info(&pdev->dev, "pdata->dev_ready is deprecated\n");
+
 		info->xfer_type = pdata->xfer_type;
 		info->devsize = pdata->devsize;
 		info->elm_of_node = pdata->elm_of_node;

@@ -1815,6 +1815,13 @@ static int omap_nand_probe(struct platform_device *pdev)
 	nand_chip->IO_ADDR_W = nand_chip->IO_ADDR_R;
 	nand_chip->cmd_ctrl  = omap_hwcontrol;
 

Quoted 9 lines

drivers/mtd/nand/omap2.c static int omap_nand_probe(struct platform_device *pdev) · line 1819

Quoted 2 lines

drivers/mtd/nand/omap2.c +2 -0

+	info->ready_gpiod = devm_gpiod_get_optional(&pdev->dev, "ready",
+						    GPIOD_IN);

Others have been looking at using GPIOs for the ready/busy pin too. At a minimum, we need an updated DT binding doc for this, since I see you're adding this via device tree in a later patch (I don't see any DT binding patch for this; but I could just be overlooking it). It'd also be great if this support was moved to nand_dt_init() so other platforms can benefit, but I won't require that.

Actually I started to work on a generic solution parsing the DT and creating CS, WP and RB gpios when they are provided, but I think it's a bit more complicated than just moving the rb-gpios parsing into nand_dt_init().
First you'll need something to store your gpio_desc pointers, which means you'll have to allocate a table of gpio_desc pointers (or a table of struct embedding a gpio_desc pointer).

The other blocking point is that when nand_scan_ident() is called, the caller is supposed to have filled the ->dev_ready() or ->waitfunc() fields, and to choose how to implement it he may need to know which kind of RB handler should be used (this is the case in the sunxi driver, where the user can either use a GPIO or native R/B pin directly connected to the controller).

All this makes me think that maybe nand_dt_init() should be called separately or in a different helper (nand_init() ?) taking care of the basic nand_chip initializations/allocations without interacting with the NAND itself.

Another solution would be to add an ->init() function to nand_chip and call it after the generic initialization has been done (but before NAND chip detection). This way the NAND controller driver could adapt some fields and parse controller specific properties.

What do you think?

(to be clear, this branch of discussion isn't directly regarding the TI changes; we can handle any generic handling afterward, as long as we get the DT binding right now)

Quoted 28 lines · Boris Brezillon

Quoted 16 lines · On Mon, 26 Oct 2015 13:49:00 -0700 Brian Norris <[email protected]>…

Quoted 15 lines · Roger Quadros

drivers/mtd/nand/omap2.c +3 -1

@@ -1782,7 +1780,9 @@ static int omap_nand_probe(struct platform_device *pdev)
 		info->reg = pdata->reg;
 		info->of_node = pdata->of_node;
 		info->ecc_opt = pdata->ecc_opt;
-		info->dev_ready	= pdata->dev_ready;
+		if (pdata->dev_ready)
+			dev_info(&pdev->dev, "pdata->dev_ready is deprecated\n");
+
 		info->xfer_type = pdata->xfer_type;
 		info->devsize = pdata->devsize;
 		info->elm_of_node = pdata->elm_of_node;

@@ -1815,6 +1815,13 @@ static int omap_nand_probe(struct platform_device *pdev)
 	nand_chip->IO_ADDR_W = nand_chip->IO_ADDR_R;
 	nand_chip->cmd_ctrl  = omap_hwcontrol;
 

Quoted 9 lines

drivers/mtd/nand/omap2.c static int omap_nand_probe(struct platform_device *pdev) · line 1819

Quoted 2 lines

drivers/mtd/nand/omap2.c +2 -0

+	info->ready_gpiod = devm_gpiod_get_optional(&pdev->dev, "ready",
+						    GPIOD_IN);

Others have been looking at using GPIOs for the ready/busy pin too. At a minimum, we need an updated DT binding doc for this, since I see you're adding this via device tree in a later patch (I don't see any DT binding patch for this; but I could just be overlooking it). It'd also be great if this support was moved to nand_dt_init() so other platforms can benefit, but I won't require that.

Quoted 7 lines

Actually I started to work on a generic solution parsing the DT and creating CS, WP and RB gpios when they are provided, but I think it's a bit more complicated than just moving the rb-gpios parsing into nand_dt_init().
First you'll need something to store your gpio_desc pointers, which means you'll have to allocate a table of gpio_desc pointers (or a table of struct embedding a gpio_desc pointer).

I'm not sure what you mean by table. It seems like we would just have a few gpio_desc pointers in struct nand_chip, no?

Quoted 7 lines

The other blocking point is that when nand_scan_ident() is called, the caller is supposed to have filled the ->dev_ready() or ->waitfunc() fields, and to choose how to implement it he may need to know which kind of RB handler should be used (this is the case in the sunxi driver, where the user can either use a GPIO or native R/B pin directly connected to the controller).

Right, I was thinking about this one.

Quoted 4 lines

All this makes me think that maybe nand_dt_init() should be called separately or in a different helper (nand_init() ?) taking care of the basic nand_chip initializations/allocations without interacting with the NAND itself.

Yeah, I feel like there have been other good reasons for something like this before, and we just have worked around them so far. Maybe something more like the alloc/add split in many other subsystems? e.g., platform_device_{alloc,add}, spi_{alloc,add}_device. Now we might want nand_alloc()?

On a slight tangent, it seems silly that nand_scan_tail() doesn't register the MTD, but nand_release() unregisters it. That shouldn't be.

Quoted 5 lines

Another solution would be to add an ->init() function to nand_chip and call it after the generic initialization has been done (but before NAND chip detection). This way the NAND controller driver could adapt some fields and parse controller specific properties.

That could work too, I guess.

Add compatible id, GPMC register resource and interrupt resource to NAND controller nodes.

The GPMC driver now implements gpiochip and irqchip so enable gpio-controller and interrupt-controller properties.

With this the interrupt parent of NAND node changes so fix it accordingly.

Signed-off-by Roger Quadros <[email protected]>

---
 arch/arm/boot/dts/dra7-evm.dts  | 5 ++++-
 arch/arm/boot/dts/dra7.dtsi     | 4 ++++
 arch/arm/boot/dts/dra72-evm.dts | 5 ++++-
 3 files changed, 12 insertions(+), 2 deletions(-)

arch/arm/boot/dts/dra7-evm.dts +4 -1

index a6c82e5..8a31161 100644
@@ -585,9 +585,12 @@
 	status = "okay";
 	pinctrl-names = "default";
 	pinctrl-0 = <&nand_flash_x16>;
-	ranges = <0 0 0 0x01000000>;	/* minimum GPMC partition = 16MB */
+	ranges = <0 0 0x08000000 0x01000000>;	/* minimum GPMC partition = 16MB */
 	nand@0,0 {
+		compatible = "ti,omap2-nand";
 		reg = <0 0 4>;		/* device IO registers */
+		interrupt-parent = <&crossbar_mpu>;
+		interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
 		ti,nand-ecc-opt = "bch8";
 		ti,elm-id = <&elm>;
 		nand-bus-width = <16>;

arch/arm/boot/dts/dra7.dtsi +0 -0

index 5d65db9..f0a3616 100644

arch/arm/boot/dts/dra72-evm.dts +1 -1

index 6f6bd98..245f5f9 100644
@@ -395,13 +395,16 @@
 	status = "okay";
 	pinctrl-names = "default";
 	pinctrl-0 = <&nand_default>;
-	ranges = <0 0 0 0x01000000>;	/* minimum GPMC partition = 16MB */
+	ranges = <0 0 0x08000000 0x01000000>;	/* minimum GPMC partition = 16MB */
 	nand@0,0 {
 		/* To use NAND, DIP switch SW5 must be set like so:

Quoted 44 lines · Roger Quadros

Add compatible id, GPMC register resource and interrupt resource to NAND controller nodes.

The GPMC driver now implements gpiochip and irqchip so enable gpio-controller and interrupt-controller properties.

With this the interrupt parent of NAND node changes so fix it accordingly.

Signed-off-by Roger Quadros <[email protected]>

---
 arch/arm/boot/dts/dra7-evm.dts  | 5 ++++-
 arch/arm/boot/dts/dra7.dtsi     | 4 ++++
 arch/arm/boot/dts/dra72-evm.dts | 5 ++++-
 3 files changed, 12 insertions(+), 2 deletions(-)

arch/arm/boot/dts/dra7-evm.dts +4 -1

index a6c82e5..8a31161 100644
@@ -585,9 +585,12 @@
 	status = "okay";
 	pinctrl-names = "default";
 	pinctrl-0 = <&nand_flash_x16>;
-	ranges = <0 0 0 0x01000000>;	/* minimum GPMC partition = 16MB */
+	ranges = <0 0 0x08000000 0x01000000>;	/* minimum GPMC partition = 16MB */
 	nand@0,0 {
+		compatible = "ti,omap2-nand";
 		reg = <0 0 4>;		/* device IO registers */
+		interrupt-parent = <&crossbar_mpu>;
+		interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
 		ti,nand-ecc-opt = "bch8";
 		ti,elm-id = <&elm>;
 		nand-bus-width = <16>;

arch/arm/boot/dts/dra7.dtsi +2 -0

index 5d65db9..f0a3616 100644
@@ -1389,6 +1389,10 @@
 			gpmc,num-waitpins = <2>;
 			#address-cells = <2>;
 			#size-cells = <1>;
+			gpio-controller;
+			#gpio-cells = <2>;

arch/arm/boot/dts/dra7.dtsi line 1397

Quoted 4 lines

arch/arm/boot/dts/dra7.dtsi +2 -0

+			interrupt-controller;
+			#interrupt-cells = <2>;
 			status = "disabled";
 		};

Based on the discussion on my patchset I noticed that the nand node defines the interrupt but it is also defined in the parent node. Similar to the dma channel we should conclude where the best place for it to be defined. But to me it seems at least it should only be defined once.

This is true for your other patches making similar changes to the dt.

Quoted 46 lines · Franklin S Cooper Jr.

Quoted 44 lines · Roger Quadros

Add compatible id, GPMC register resource and interrupt resource to NAND controller nodes.

The GPMC driver now implements gpiochip and irqchip so enable gpio-controller and interrupt-controller properties.

With this the interrupt parent of NAND node changes so fix it accordingly.

Signed-off-by Roger Quadros <[email protected]>

---
 arch/arm/boot/dts/dra7-evm.dts  | 5 ++++-
 arch/arm/boot/dts/dra7.dtsi     | 4 ++++
 arch/arm/boot/dts/dra72-evm.dts | 5 ++++-
 3 files changed, 12 insertions(+), 2 deletions(-)

arch/arm/boot/dts/dra7-evm.dts +4 -1

index a6c82e5..8a31161 100644
@@ -585,9 +585,12 @@
 	status = "okay";
 	pinctrl-names = "default";
 	pinctrl-0 = <&nand_flash_x16>;
-	ranges = <0 0 0 0x01000000>;	/* minimum GPMC partition = 16MB */
+	ranges = <0 0 0x08000000 0x01000000>;	/* minimum GPMC partition = 16MB */
 	nand@0,0 {
+		compatible = "ti,omap2-nand";
 		reg = <0 0 4>;		/* device IO registers */
+		interrupt-parent = <&crossbar_mpu>;
+		interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
 		ti,nand-ecc-opt = "bch8";
 		ti,elm-id = <&elm>;
 		nand-bus-width = <16>;

arch/arm/boot/dts/dra7.dtsi +2 -0

index 5d65db9..f0a3616 100644
@@ -1389,6 +1389,10 @@
 			gpmc,num-waitpins = <2>;
 			#address-cells = <2>;
 			#size-cells = <1>;
+			gpio-controller;
+			#gpio-cells = <2>;

Quoted 8 lines

arch/arm/boot/dts/dra7.dtsi line 1397

Quoted 4 lines

arch/arm/boot/dts/dra7.dtsi +2 -0

+			interrupt-controller;
+			#interrupt-cells = <2>;
 			status = "disabled";
 		};

Based on the discussion on my patchset I noticed that the nand node defines the interrupt but it is also defined in the parent node. Similar to the dma channel we should conclude where the best place for it to be defined. But to me it seems at least it should only be defined once.

The interrupt is defined at both places because it is used at both places. It is used as a shared interrupt. Wait_pin interrupts are managed by the gpmc driver and NAND specific interrupts are managed by the NAND driver.

If GPMC dma channel is going to be used only by the NAND driver then we should define the channel in the NAND node.

Quoted 1 line

This is true for your other patches making similar changes to the dt.

Yes, GPMC IRQ is defined in both GPMC and NAND nodes.

--
cheers,
-roger

Add compatible id, GPMC register resource and interrupt resource to NAND controller nodes.

The GPMC driver now implements gpiochip and irqchip so enable gpio-controller and interrupt-controller properties.

With this the interrupt parent of NAND node changes so fix it accordingly.

arch/arm/boot/dts/logicpd-torpedo-som.dtsi +1 -1

@@ -35,11 +35,14 @@
 };
 
 &gpmc {
-	ranges = <0 0 0x00000000 0x1000000>;	/* CS0: 16MB for NAND */
+	ranges = <0 0 0x08000000 0x1000000>;	/* CS0: 16MB for NAND */
 

Quoted 8 lines · Roger Quadros

Add compatible id, GPMC register resource and interrupt resource to NAND controller nodes.

The GPMC driver now implements gpiochip and irqchip so enable gpio-controller and interrupt-controller properties.

With this the interrupt parent of NAND node changes so fix it accordingly.

...

Quoted 9 lines

arch/arm/boot/dts/logicpd-torpedo-som.dtsi +1 -1

@@ -35,11 +35,14 @@
 };
 
 &gpmc {
-	ranges = <0 0 0x00000000 0x1000000>;	/* CS0: 16MB for NAND */
+	ranges = <0 0 0x08000000 0x1000000>;	/* CS0: 16MB for NAND */
 

arch/arm/boot/dts/logicpd-torpedo-som.dtsi line 48

Quoted 10 lines

arch/arm/boot/dts/logicpd-torpedo-som.dtsi +4 -1

 	nand@0,0 {
-		linux,mtd-name = "micron,mt29f4g16abbda3w";
+		compatible = "ti,omap2-nand";
 		reg = <0 0 4>; /* CS0, offset 0, IO size 4 */
+		interrupt-parent = <&intc>;
+		interrupts = <20>;
+		linux,mtd-name = "micron,mt29f4g16abbda3w";
 		nand-bus-width = <16>;
 		ti,nand-ecc-opt = "bch8";
 		gpmc,sync-clk-ps = <0>;

At least torpedo breaks for NFSroot as NAND now overlaps with Ethernet.. What's the policy you have for moving the addresses around?