[PATCH] mtd: gpio-nand: add device tree bindings

[PATCH] mtd: gpio-nand: add device tree bindings

[Jamie Iles]

Add device tree bindings so that the gpio-nand driver may be
instantiated from the device tree.  This also allows the partitions
to be specified in the device tree.

Cc: David Woodhouse <dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
Cc: Artem Bityutskiy <Artem.Bityutskiy-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Jamie Iles <jamie-wmLquQDDieKakBO8gow8eQ@public.gmane.org>
---

I have this working on our platform using the following DT section:

	ebi {
		compatible = "simple-bus";
		#address-cells = <2>;
		#size-cells = <1>;
		ranges = <0 0 0x40000000 0x08000000
			  1 0 0x48000000 0x08000000
			  2 0 0x50000000 0x08000000
			  3 0 0x58000000 0x08000000>;

		nand: gpio-nand@2,0 {
			compatible = "gpio-nand";
			#address-cells = <0>;
			#size-cells = <0>;
			reg = <2 0x0000 0x1000
			       0 0x2000 0x0004>;

			gpios = <&banka 1 0	/* rdy */
				 &banka 2 0 	/* nce */
				 &banka 3 0 	/* ale */
				 &banka 4 0 	/* cle */
				 0		/* nwp */>;

			boot@100000 {
				label = "Boot";
				reg = <0x100000 0x80000>;
			};

			...
		};
	};
};

but to provide synchronisation with regards to the bus reordering, we actually
need to perform a read from the GPIO controller rather than the EBI, but I'm
not sure how to express this in the DT when using ranges like this, so any
suggestions would be welcome!

Jamie

 .../devicetree/bindings/mtd/gpio-nand.txt          |   43 +++++++++
 drivers/mtd/nand/gpio.c                            |   91 ++++++++++++++++++--
 2 files changed, 126 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mtd/gpio-nand.txt

diff --git a/Documentation/devicetree/bindings/mtd/gpio-nand.txt b/Documentation/devicetree/bindings/mtd/gpio-nand.txt
new file mode 100644
index 0000000..98cb152
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/gpio-nand.txt
@@ -0,0 +1,43 @@
+GPIO assisted NAND flash
+
+Required properties:
+- compatible : "gpio-nand"
+- reg : should specify localbus chip select and size used for the chip.  For
+  ARM platforms where a dummy read is needed to provide synchronisation with
+  regards to bus reordering, an optional second resource describes the
+  location to read from.
+- #address-cells, #size-cells : Must be present if the device has sub-nodes
+  representing partitions.  In this case, both #address-cells and #size-cells
+  must be equal to 1.
+- gpios : specifies the gpio pins to control the NAND device.  nwp is an
+  optional gpio and may be set to 0 if not present.
+
+Optional properties:
+- bank-width : Width (in bytes) of the bank.  Equal to the device width times
+  the number of interleaved chips.
+- chip-delay : chip dependent delay for transferring data from array to
+  read registers (tR).
+
+Examples:
+
+gpio-nand@1,0 {
+	compatible = "gpio-nand";
+	reg = <1 0x0000 0x1000>;
+	#address-cells = <1>;
+	#size-cells = <1>;
+	gpios = <&banka 1 0	/* rdy */
+		 &banka 2 0 	/* nce */
+		 &banka 3 0 	/* ale */
+		 &banka 4 0 	/* cle */
+		 0		/* nwp */>;
+
+	flash {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		compatible = "...";
+
+		partition@0 {
+			...
+		};
+	};
+};
diff --git a/drivers/mtd/nand/gpio.c b/drivers/mtd/nand/gpio.c
index 2c2060b..ee74593 100644
--- a/drivers/mtd/nand/gpio.c
+++ b/drivers/mtd/nand/gpio.c
@@ -27,6 +27,9 @@
 #include <linux/mtd/nand.h>
 #include <linux/mtd/partitions.h>
 #include <linux/mtd/nand-gpio.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_gpio.h>
 
 struct gpiomtd {
 	void __iomem		*io_sync;
@@ -221,14 +224,70 @@ static void __iomem *request_and_remap(struct resource *res, size_t size,
 	return ptr;
 }
 
+static const struct of_device_id gpio_nand_id_table[] = {
+	{ .compatible = "gpio-nand" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, gpio_nand_id_table);
+
+static int gpio_nand_of_get_options(struct device *dev,
+				    struct gpio_nand_platdata *plat)
+{
+	u32 width;
+
+	if (!of_property_read_u32(dev->of_node, "bank-width", &width)) {
+		if (width == 2) {
+			plat->options |= NAND_BUSWIDTH_16;
+		} else if (width != 1) {
+			dev_err(dev, "invalid bank-width %u\n", width);
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
+static void gpio_nand_of_get_gpio(struct device *dev,
+				  struct gpio_nand_platdata *plat)
+{
+	plat->gpio_rdy = of_get_gpio(dev->of_node, 0);
+	plat->gpio_nce = of_get_gpio(dev->of_node, 1);
+	plat->gpio_ale = of_get_gpio(dev->of_node, 2);
+	plat->gpio_cle = of_get_gpio(dev->of_node, 3);
+	plat->gpio_nwp = of_get_gpio(dev->of_node, 4);
+}
+
+static void gpio_nand_of_get_chip_delay(struct device *dev,
+					struct gpio_nand_platdata *plat)
+{
+	u32 chip_delay;
+
+	if (!of_property_read_u32(dev->of_node, "chip-delay", &chip_delay))
+		plat->chip_delay = (int)chip_delay;
+}
+
+static int gpio_nand_of_get_config(struct device *dev,
+				   struct gpio_nand_platdata *plat)
+{
+	int ret = gpio_nand_of_get_options(dev, plat);
+
+	if (ret < 0)
+		return ret;
+
+	gpio_nand_of_get_gpio(dev, plat);
+	gpio_nand_of_get_chip_delay(dev, plat);
+
+	return 0;
+}
+
 static int __devinit gpio_nand_probe(struct platform_device *dev)
 {
 	struct gpiomtd *gpiomtd;
 	struct nand_chip *this;
 	struct resource *res0, *res1;
-	int ret;
+	int ret = 0;
 
-	if (!dev->dev.platform_data)
+	if (!dev->dev.of_node && !dev->dev.platform_data)
 		return -EINVAL;
 
 	res0 = platform_get_resource(dev, IORESOURCE_MEM, 0);
@@ -257,11 +316,17 @@ static int __devinit gpio_nand_probe(struct platform_device *dev)
 		}
 	}
 
-	memcpy(&gpiomtd->plat, dev->dev.platform_data, sizeof(gpiomtd->plat));
+	if (dev->dev.platform_data)
+		memcpy(&gpiomtd->plat, dev->dev.platform_data,
+		       sizeof(gpiomtd->plat));
+	else
+		ret = gpio_nand_of_get_config(&dev->dev, &gpiomtd->plat);
+	if (ret)
+		goto err_config;
 
 	ret = gpio_request(gpiomtd->plat.gpio_nce, "NAND NCE");
 	if (ret)
-		goto err_nce;
+		goto err_config;
 	gpio_direction_output(gpiomtd->plat.gpio_nce, 1);
 	if (gpio_is_valid(gpiomtd->plat.gpio_nwp)) {
 		ret = gpio_request(gpiomtd->plat.gpio_nwp, "NAND NWP");
@@ -312,12 +377,21 @@ static int __devinit gpio_nand_probe(struct platform_device *dev)
 		goto err_wp;
 	}
 
-	if (gpiomtd->plat.adjust_parts)
-		gpiomtd->plat.adjust_parts(&gpiomtd->plat,
-					   gpiomtd->mtd_info.size);
+	if (dev->dev.platform_data) {
+		if (gpiomtd->plat.adjust_parts)
+			gpiomtd->plat.adjust_parts(&gpiomtd->plat,
+						   gpiomtd->mtd_info.size);
+	} else {
+		ret = of_mtd_parse_partitions(&dev->dev, dev->dev.of_node,
+					      &gpiomtd->plat.parts);
+		if (ret < 0)
+			goto err_wp;
 
+		gpiomtd->plat.num_parts = (unsigned int)ret;
+	}
 	mtd_device_register(&gpiomtd->mtd_info, gpiomtd->plat.parts,
 			    gpiomtd->plat.num_parts);
+
 	platform_set_drvdata(dev, gpiomtd);
 
 	return 0;
@@ -335,7 +409,7 @@ err_ale:
 		gpio_free(gpiomtd->plat.gpio_nwp);
 err_nwp:
 	gpio_free(gpiomtd->plat.gpio_nce);
-err_nce:
+err_config:
 	iounmap(gpiomtd->io_sync);
 	if (res1)
 		release_mem_region(res1->start, resource_size(res1));
@@ -352,6 +426,7 @@ static struct platform_driver gpio_nand_driver = {
 	.remove		= gpio_nand_remove,
 	.driver		= {
 		.name	= "gpio-nand",
+		.of_match_table = gpio_nand_id_table,
 	},
 };
 
-- 
1.7.4.1

[PATCH] mtd: gpio-nand: add device tree bindings

[Jamie Iles]

Add device tree bindings so that the gpio-nand driver may be
instantiated from the device tree.  This also allows the partitions
to be specified in the device tree.

Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
Signed-off-by: Jamie Iles <jamie@jamieiles.com>
---

I have this working on our platform using the following DT section:

	ebi {
		compatible = "simple-bus";
		#address-cells = <2>;
		#size-cells = <1>;
		ranges = <0 0 0x40000000 0x08000000
			  1 0 0x48000000 0x08000000
			  2 0 0x50000000 0x08000000
			  3 0 0x58000000 0x08000000>;

		nand: gpio-nand@2,0 {
			compatible = "gpio-nand";
			#address-cells = <0>;
			#size-cells = <0>;
			reg = <2 0x0000 0x1000
			       0 0x2000 0x0004>;

			gpios = <&banka 1 0	/* rdy */
				 &banka 2 0 	/* nce */
				 &banka 3 0 	/* ale */
				 &banka 4 0 	/* cle */
				 0		/* nwp */>;

			boot@100000 {
				label = "Boot";
				reg = <0x100000 0x80000>;
			};

			...
		};
	};
};

but to provide synchronisation with regards to the bus reordering, we actually
need to perform a read from the GPIO controller rather than the EBI, but I'm
not sure how to express this in the DT when using ranges like this, so any
suggestions would be welcome!

Jamie

 .../devicetree/bindings/mtd/gpio-nand.txt          |   43 +++++++++
 drivers/mtd/nand/gpio.c                            |   91 ++++++++++++++++++--
 2 files changed, 126 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mtd/gpio-nand.txt

diff --git a/Documentation/devicetree/bindings/mtd/gpio-nand.txt b/Documentation/devicetree/bindings/mtd/gpio-nand.txt
new file mode 100644
index 0000000..98cb152
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/gpio-nand.txt
@@ -0,0 +1,43 @@
+GPIO assisted NAND flash
+
+Required properties:
+- compatible : "gpio-nand"
+- reg : should specify localbus chip select and size used for the chip.  For
+  ARM platforms where a dummy read is needed to provide synchronisation with
+  regards to bus reordering, an optional second resource describes the
+  location to read from.
+- #address-cells, #size-cells : Must be present if the device has sub-nodes
+  representing partitions.  In this case, both #address-cells and #size-cells
+  must be equal to 1.
+- gpios : specifies the gpio pins to control the NAND device.  nwp is an
+  optional gpio and may be set to 0 if not present.
+
+Optional properties:
+- bank-width : Width (in bytes) of the bank.  Equal to the device width times
+  the number of interleaved chips.
+- chip-delay : chip dependent delay for transferring data from array to
+  read registers (tR).
+
+Examples:
+
+gpio-nand@1,0 {
+	compatible = "gpio-nand";
+	reg = <1 0x0000 0x1000>;
+	#address-cells = <1>;
+	#size-cells = <1>;
+	gpios = <&banka 1 0	/* rdy */
+		 &banka 2 0 	/* nce */
+		 &banka 3 0 	/* ale */
+		 &banka 4 0 	/* cle */
+		 0		/* nwp */>;
+
+	flash {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		compatible = "...";
+
+		partition@0 {
+			...
+		};
+	};
+};
diff --git a/drivers/mtd/nand/gpio.c b/drivers/mtd/nand/gpio.c
index 2c2060b..ee74593 100644
--- a/drivers/mtd/nand/gpio.c
+++ b/drivers/mtd/nand/gpio.c
@@ -27,6 +27,9 @@
 #include <linux/mtd/nand.h>
 #include <linux/mtd/partitions.h>
 #include <linux/mtd/nand-gpio.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_gpio.h>
 
 struct gpiomtd {
 	void __iomem		*io_sync;
@@ -221,14 +224,70 @@ static void __iomem *request_and_remap(struct resource *res, size_t size,
 	return ptr;
 }
 
+static const struct of_device_id gpio_nand_id_table[] = {
+	{ .compatible = "gpio-nand" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, gpio_nand_id_table);
+
+static int gpio_nand_of_get_options(struct device *dev,
+				    struct gpio_nand_platdata *plat)
+{
+	u32 width;
+
+	if (!of_property_read_u32(dev->of_node, "bank-width", &width)) {
+		if (width == 2) {
+			plat->options |= NAND_BUSWIDTH_16;
+		} else if (width != 1) {
+			dev_err(dev, "invalid bank-width %u\n", width);
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
+static void gpio_nand_of_get_gpio(struct device *dev,
+				  struct gpio_nand_platdata *plat)
+{
+	plat->gpio_rdy = of_get_gpio(dev->of_node, 0);
+	plat->gpio_nce = of_get_gpio(dev->of_node, 1);
+	plat->gpio_ale = of_get_gpio(dev->of_node, 2);
+	plat->gpio_cle = of_get_gpio(dev->of_node, 3);
+	plat->gpio_nwp = of_get_gpio(dev->of_node, 4);
+}
+
+static void gpio_nand_of_get_chip_delay(struct device *dev,
+					struct gpio_nand_platdata *plat)
+{
+	u32 chip_delay;
+
+	if (!of_property_read_u32(dev->of_node, "chip-delay", &chip_delay))
+		plat->chip_delay = (int)chip_delay;
+}
+
+static int gpio_nand_of_get_config(struct device *dev,
+				   struct gpio_nand_platdata *plat)
+{
+	int ret = gpio_nand_of_get_options(dev, plat);
+
+	if (ret < 0)
+		return ret;
+
+	gpio_nand_of_get_gpio(dev, plat);
+	gpio_nand_of_get_chip_delay(dev, plat);
+
+	return 0;
+}
+
 static int __devinit gpio_nand_probe(struct platform_device *dev)
 {
 	struct gpiomtd *gpiomtd;
 	struct nand_chip *this;
 	struct resource *res0, *res1;
-	int ret;
+	int ret = 0;
 
-	if (!dev->dev.platform_data)
+	if (!dev->dev.of_node && !dev->dev.platform_data)
 		return -EINVAL;
 
 	res0 = platform_get_resource(dev, IORESOURCE_MEM, 0);
@@ -257,11 +316,17 @@ static int __devinit gpio_nand_probe(struct platform_device *dev)
 		}
 	}
 
-	memcpy(&gpiomtd->plat, dev->dev.platform_data, sizeof(gpiomtd->plat));
+	if (dev->dev.platform_data)
+		memcpy(&gpiomtd->plat, dev->dev.platform_data,
+		       sizeof(gpiomtd->plat));
+	else
+		ret = gpio_nand_of_get_config(&dev->dev, &gpiomtd->plat);
+	if (ret)
+		goto err_config;
 
 	ret = gpio_request(gpiomtd->plat.gpio_nce, "NAND NCE");
 	if (ret)
-		goto err_nce;
+		goto err_config;
 	gpio_direction_output(gpiomtd->plat.gpio_nce, 1);
 	if (gpio_is_valid(gpiomtd->plat.gpio_nwp)) {
 		ret = gpio_request(gpiomtd->plat.gpio_nwp, "NAND NWP");
@@ -312,12 +377,21 @@ static int __devinit gpio_nand_probe(struct platform_device *dev)
 		goto err_wp;
 	}
 
-	if (gpiomtd->plat.adjust_parts)
-		gpiomtd->plat.adjust_parts(&gpiomtd->plat,
-					   gpiomtd->mtd_info.size);
+	if (dev->dev.platform_data) {
+		if (gpiomtd->plat.adjust_parts)
+			gpiomtd->plat.adjust_parts(&gpiomtd->plat,
+						   gpiomtd->mtd_info.size);
+	} else {
+		ret = of_mtd_parse_partitions(&dev->dev, dev->dev.of_node,
+					      &gpiomtd->plat.parts);
+		if (ret < 0)
+			goto err_wp;
 
+		gpiomtd->plat.num_parts = (unsigned int)ret;
+	}
 	mtd_device_register(&gpiomtd->mtd_info, gpiomtd->plat.parts,
 			    gpiomtd->plat.num_parts);
+
 	platform_set_drvdata(dev, gpiomtd);
 
 	return 0;
@@ -335,7 +409,7 @@ err_ale:
 		gpio_free(gpiomtd->plat.gpio_nwp);
 err_nwp:
 	gpio_free(gpiomtd->plat.gpio_nce);
-err_nce:
+err_config:
 	iounmap(gpiomtd->io_sync);
 	if (res1)
 		release_mem_region(res1->start, resource_size(res1));
@@ -352,6 +426,7 @@ static struct platform_driver gpio_nand_driver = {
 	.remove		= gpio_nand_remove,
 	.driver		= {
 		.name	= "gpio-nand",
+		.of_match_table = gpio_nand_id_table,
 	},
 };
 
-- 
1.7.4.1

Re: [PATCH] mtd: gpio-nand: add device tree bindings

[Scott Wood]

On Wed, 27 Jul 2011 15:03:30 +0100
Jamie Iles <jamie-wmLquQDDieKakBO8gow8eQ@public.gmane.org> wrote:

> diff --git a/Documentation/devicetree/bindings/mtd/gpio-nand.txt b/Documentation/devicetree/bindings/mtd/gpio-nand.txt
> new file mode 100644
> index 0000000..98cb152
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/gpio-nand.txt
> @@ -0,0 +1,43 @@
> +GPIO assisted NAND flash
> +
> +Required properties:
> +- compatible : "gpio-nand"
> +- reg : should specify localbus chip select and size used for the chip.  For
> +  ARM platforms where a dummy read is needed to provide synchronisation with
> +  regards to bus reordering, an optional second resource describes the
> +  location to read from.

I don't see how a pure "gpio nand" device would have any memory mapped
I/O.  I think you need a more specific compatible for this.

> +Optional properties:
> +- bank-width : Width (in bytes) of the bank.  Equal to the device width times
> +  the number of interleaved chips.

Interleaved NAND chips?  Is that actually done?

> +Examples:
> +
> +gpio-nand@1,0 {
> +	compatible = "gpio-nand";
> +	reg = <1 0x0000 0x1000>;
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +	gpios = <&banka 1 0	/* rdy */
> +		 &banka 2 0 	/* nce */
> +		 &banka 3 0 	/* ale */
> +		 &banka 4 0 	/* cle */
> +		 0		/* nwp */>;
> +
> +	flash {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		compatible = "...";
> +
> +		partition@0 {
> +			...
> +		};
> +	};
> +};

Here you have a separate flash node underneath the gpio-nand node, but
earlier in the patch comment you show the partitions being directly under
gpio-nand, and from a quick glance it appears the latter is what the code
supports.

-Scott

Re: [PATCH] mtd: gpio-nand: add device tree bindings

[Scott Wood]

On Wed, 27 Jul 2011 15:03:30 +0100
Jamie Iles <jamie@jamieiles.com> wrote:

> diff --git a/Documentation/devicetree/bindings/mtd/gpio-nand.txt b/Documentation/devicetree/bindings/mtd/gpio-nand.txt
> new file mode 100644
> index 0000000..98cb152
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/gpio-nand.txt
> @@ -0,0 +1,43 @@
> +GPIO assisted NAND flash
> +
> +Required properties:
> +- compatible : "gpio-nand"
> +- reg : should specify localbus chip select and size used for the chip.  For
> +  ARM platforms where a dummy read is needed to provide synchronisation with
> +  regards to bus reordering, an optional second resource describes the
> +  location to read from.

I don't see how a pure "gpio nand" device would have any memory mapped
I/O.  I think you need a more specific compatible for this.

> +Optional properties:
> +- bank-width : Width (in bytes) of the bank.  Equal to the device width times
> +  the number of interleaved chips.

Interleaved NAND chips?  Is that actually done?

> +Examples:
> +
> +gpio-nand@1,0 {
> +	compatible = "gpio-nand";
> +	reg = <1 0x0000 0x1000>;
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +	gpios = <&banka 1 0	/* rdy */
> +		 &banka 2 0 	/* nce */
> +		 &banka 3 0 	/* ale */
> +		 &banka 4 0 	/* cle */
> +		 0		/* nwp */>;
> +
> +	flash {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		compatible = "...";
> +
> +		partition@0 {
> +			...
> +		};
> +	};
> +};

Here you have a separate flash node underneath the gpio-nand node, but
earlier in the patch comment you show the partitions being directly under
gpio-nand, and from a quick glance it appears the latter is what the code
supports.

-Scott

Re: [PATCH] mtd: gpio-nand: add device tree bindings

[Jamie Iles]

Hmm, get_maintainer.pl got Artem's address wrong (old Nokia address) for 
gpio-nand.c so correct one on CC now!

On Wed, Jul 27, 2011 at 02:45:01PM -0500, Scott Wood wrote:
> On Wed, 27 Jul 2011 15:03:30 +0100
> Jamie Iles <jamie-wmLquQDDieKakBO8gow8eQ@public.gmane.org> wrote:
> 
> > diff --git a/Documentation/devicetree/bindings/mtd/gpio-nand.txt b/Documentation/devicetree/bindings/mtd/gpio-nand.txt
> > new file mode 100644
> > index 0000000..98cb152
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mtd/gpio-nand.txt
> > @@ -0,0 +1,43 @@
> > +GPIO assisted NAND flash
> > +
> > +Required properties:
> > +- compatible : "gpio-nand"
> > +- reg : should specify localbus chip select and size used for the chip.  For
> > +  ARM platforms where a dummy read is needed to provide synchronisation with
> > +  regards to bus reordering, an optional second resource describes the
> > +  location to read from.
> 
> I don't see how a pure "gpio nand" device would have any memory mapped
> I/O.  I think you need a more specific compatible for this.

OK, fair point.  I'm not sure what a better name would be though, maybe 
gpio-assisted-nand?

> > +Optional properties:
> > +- bank-width : Width (in bytes) of the bank.  Equal to the device width times
> > +  the number of interleaved chips.
> 
> Interleaved NAND chips?  Is that actually done?

Doh, that shouldn't read like that.  It's really just the bank width.

> > +Examples:
> > +
> > +gpio-nand@1,0 {
> > +	compatible = "gpio-nand";
> > +	reg = <1 0x0000 0x1000>;
> > +	#address-cells = <1>;
> > +	#size-cells = <1>;
> > +	gpios = <&banka 1 0	/* rdy */
> > +		 &banka 2 0 	/* nce */
> > +		 &banka 3 0 	/* ale */
> > +		 &banka 4 0 	/* cle */
> > +		 0		/* nwp */>;
> > +
> > +	flash {
> > +		#address-cells = <1>;
> > +		#size-cells = <1>;
> > +		compatible = "...";
> > +
> > +		partition@0 {
> > +			...
> > +		};
> > +	};
> > +};
> 
> Here you have a separate flash node underneath the gpio-nand node, but
> earlier in the patch comment you show the partitions being directly under
> gpio-nand, and from a quick glance it appears the latter is what the code
> supports.

Yes, that's definitely wrong!  The partitions should be directly under the 
nand controller node.

Thanks for the review!

Jamie

Re: [PATCH] mtd: gpio-nand: add device tree bindings

[Jamie Iles]

Hmm, get_maintainer.pl got Artem's address wrong (old Nokia address) for 
gpio-nand.c so correct one on CC now!

On Wed, Jul 27, 2011 at 02:45:01PM -0500, Scott Wood wrote:
> On Wed, 27 Jul 2011 15:03:30 +0100
> Jamie Iles <jamie@jamieiles.com> wrote:
> 
> > diff --git a/Documentation/devicetree/bindings/mtd/gpio-nand.txt b/Documentation/devicetree/bindings/mtd/gpio-nand.txt
> > new file mode 100644
> > index 0000000..98cb152
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mtd/gpio-nand.txt
> > @@ -0,0 +1,43 @@
> > +GPIO assisted NAND flash
> > +
> > +Required properties:
> > +- compatible : "gpio-nand"
> > +- reg : should specify localbus chip select and size used for the chip.  For
> > +  ARM platforms where a dummy read is needed to provide synchronisation with
> > +  regards to bus reordering, an optional second resource describes the
> > +  location to read from.
> 
> I don't see how a pure "gpio nand" device would have any memory mapped
> I/O.  I think you need a more specific compatible for this.

OK, fair point.  I'm not sure what a better name would be though, maybe 
gpio-assisted-nand?

> > +Optional properties:
> > +- bank-width : Width (in bytes) of the bank.  Equal to the device width times
> > +  the number of interleaved chips.
> 
> Interleaved NAND chips?  Is that actually done?

Doh, that shouldn't read like that.  It's really just the bank width.

> > +Examples:
> > +
> > +gpio-nand@1,0 {
> > +	compatible = "gpio-nand";
> > +	reg = <1 0x0000 0x1000>;
> > +	#address-cells = <1>;
> > +	#size-cells = <1>;
> > +	gpios = <&banka 1 0	/* rdy */
> > +		 &banka 2 0 	/* nce */
> > +		 &banka 3 0 	/* ale */
> > +		 &banka 4 0 	/* cle */
> > +		 0		/* nwp */>;
> > +
> > +	flash {
> > +		#address-cells = <1>;
> > +		#size-cells = <1>;
> > +		compatible = "...";
> > +
> > +		partition@0 {
> > +			...
> > +		};
> > +	};
> > +};
> 
> Here you have a separate flash node underneath the gpio-nand node, but
> earlier in the patch comment you show the partitions being directly under
> gpio-nand, and from a quick glance it appears the latter is what the code
> supports.

Yes, that's definitely wrong!  The partitions should be directly under the 
nand controller node.

Thanks for the review!

Jamie

Re: [PATCH] mtd: gpio-nand: add device tree bindings

[Scott Wood]

On Wed, 27 Jul 2011 20:55:12 +0100
Jamie Iles <jamie-wmLquQDDieKakBO8gow8eQ@public.gmane.org> wrote:

> Hmm, get_maintainer.pl got Artem's address wrong (old Nokia address) for 
> gpio-nand.c so correct one on CC now!
> 
> On Wed, Jul 27, 2011 at 02:45:01PM -0500, Scott Wood wrote:
> > On Wed, 27 Jul 2011 15:03:30 +0100
> > Jamie Iles <jamie-wmLquQDDieKakBO8gow8eQ@public.gmane.org> wrote:
> > 
> > > diff --git a/Documentation/devicetree/bindings/mtd/gpio-nand.txt b/Documentation/devicetree/bindings/mtd/gpio-nand.txt
> > > new file mode 100644
> > > index 0000000..98cb152
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mtd/gpio-nand.txt
> > > @@ -0,0 +1,43 @@
> > > +GPIO assisted NAND flash
> > > +
> > > +Required properties:
> > > +- compatible : "gpio-nand"
> > > +- reg : should specify localbus chip select and size used for the chip.  For
> > > +  ARM platforms where a dummy read is needed to provide synchronisation with
> > > +  regards to bus reordering, an optional second resource describes the
> > > +  location to read from.
> > 
> > I don't see how a pure "gpio nand" device would have any memory mapped
> > I/O.  I think you need a more specific compatible for this.
> 
> OK, fair point.  I'm not sure what a better name would be though, maybe 
> gpio-assisted-nand?

Not sure, but if the name is generic at all the binding should thorougly
describe what is expected of the hardware.

-Scott

Re: [PATCH] mtd: gpio-nand: add device tree bindings

[Scott Wood]

On Wed, 27 Jul 2011 20:55:12 +0100
Jamie Iles <jamie@jamieiles.com> wrote:

> Hmm, get_maintainer.pl got Artem's address wrong (old Nokia address) for 
> gpio-nand.c so correct one on CC now!
> 
> On Wed, Jul 27, 2011 at 02:45:01PM -0500, Scott Wood wrote:
> > On Wed, 27 Jul 2011 15:03:30 +0100
> > Jamie Iles <jamie@jamieiles.com> wrote:
> > 
> > > diff --git a/Documentation/devicetree/bindings/mtd/gpio-nand.txt b/Documentation/devicetree/bindings/mtd/gpio-nand.txt
> > > new file mode 100644
> > > index 0000000..98cb152
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mtd/gpio-nand.txt
> > > @@ -0,0 +1,43 @@
> > > +GPIO assisted NAND flash
> > > +
> > > +Required properties:
> > > +- compatible : "gpio-nand"
> > > +- reg : should specify localbus chip select and size used for the chip.  For
> > > +  ARM platforms where a dummy read is needed to provide synchronisation with
> > > +  regards to bus reordering, an optional second resource describes the
> > > +  location to read from.
> > 
> > I don't see how a pure "gpio nand" device would have any memory mapped
> > I/O.  I think you need a more specific compatible for this.
> 
> OK, fair point.  I'm not sure what a better name would be though, maybe 
> gpio-assisted-nand?

Not sure, but if the name is generic at all the binding should thorougly
describe what is expected of the hardware.

-Scott

Re: [PATCH] mtd: gpio-nand: add device tree bindings

[Mike Frysinger]

On Wed, Jul 27, 2011 at 12:55, Jamie Iles wrote:
> On Wed, Jul 27, 2011 at 02:45:01PM -0500, Scott Wood wrote:
>> On Wed, 27 Jul 2011 15:03:30 +0100
>> Jamie Iles <jamie@jamieiles.com> wrote:
>>
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/mtd/gpio-nand.txt
>> > @@ -0,0 +1,43 @@
>> > +GPIO assisted NAND flash
>> > +
>> > +Required properties:
>> > +- compatible : "gpio-nand"
>> > +- reg : should specify localbus chip select and size used for the chip.  For
>> > +  ARM platforms where a dummy read is needed to provide synchronisation with
>> > +  regards to bus reordering, an optional second resource describes the
>> > +  location to read from.
>>
>> I don't see how a pure "gpio nand" device would have any memory mapped
>> I/O.  I think you need a more specific compatible for this.
>
> OK, fair point.  I'm not sure what a better name would be though, maybe
> gpio-assisted-nand?

fwiw, i named the parallel flash mapping driver "gpio-addr-flash"
(where some of the address lines are done with GPIOs)
-mike
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH] mtd: gpio-nand: add device tree bindings

[Mike Frysinger]

On Wed, Jul 27, 2011 at 12:55, Jamie Iles wrote:
> On Wed, Jul 27, 2011 at 02:45:01PM -0500, Scott Wood wrote:
>> On Wed, 27 Jul 2011 15:03:30 +0100
>> Jamie Iles <jamie@jamieiles.com> wrote:
>>
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/mtd/gpio-nand.txt
>> > @@ -0,0 +1,43 @@
>> > +GPIO assisted NAND flash
>> > +
>> > +Required properties:
>> > +- compatible : "gpio-nand"
>> > +- reg : should specify localbus chip select and size used for the chip.  For
>> > +  ARM platforms where a dummy read is needed to provide synchronisation with
>> > +  regards to bus reordering, an optional second resource describes the
>> > +  location to read from.
>>
>> I don't see how a pure "gpio nand" device would have any memory mapped
>> I/O.  I think you need a more specific compatible for this.
>
> OK, fair point.  I'm not sure what a better name would be though, maybe
> gpio-assisted-nand?

fwiw, i named the parallel flash mapping driver "gpio-addr-flash"
(where some of the address lines are done with GPIOs)
-mike

Re: [PATCH] mtd: gpio-nand: add device tree bindings

[Mike Frysinger]

On Wed, Jul 27, 2011 at 07:03, Jamie Iles wrote:
> --- a/drivers/mtd/nand/gpio.c
> +++ b/drivers/mtd/nand/gpio.c
>
> +static const struct of_device_id gpio_nand_id_table[] = {
> +       { .compatible = "gpio-nand" },
> +       {}
> +};
> +MODULE_DEVICE_TABLE(of, gpio_nand_id_table);

all this new logic is missing CONFIG_OF ifdef protection for all the
ports not using device trees
-mike
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH] mtd: gpio-nand: add device tree bindings

[Mike Frysinger]

On Wed, Jul 27, 2011 at 07:03, Jamie Iles wrote:
> --- a/drivers/mtd/nand/gpio.c
> +++ b/drivers/mtd/nand/gpio.c
>
> +static const struct of_device_id gpio_nand_id_table[] = {
> +       { .compatible = "gpio-nand" },
> +       {}
> +};
> +MODULE_DEVICE_TABLE(of, gpio_nand_id_table);

all this new logic is missing CONFIG_OF ifdef protection for all the
ports not using device trees
-mike

Re: [PATCH] mtd: gpio-nand: add device tree bindings

[Artem Bityutskiy]

On Sun, 2011-12-18 at 10:00 +0000, Jamie Iles wrote:
> Add device tree bindings so that the gpio-nand driver may be
> instantiated from the device tree.  This also allows the partitions
> to be specified in the device tree.
> 
> v7:	- restore runtime device tree/non device tree detection
> v6:	- convert to mtd_device_parse_register()
> v5:	- fold dt config helpers into a single gpio_nand_of_get_config()
> v4:	- get io sync address from gpio-control-nand,io-sync-reg
> 	  property rather than a resource
> 	- clarified a few details in the binding
> v3:	- remove redundant cast and a couple of whitespace/naming
> 	  changes
> v2:	- add CONFIG_OF guards for non-dt platforms
> 	- compatible becomes gpio-control-nand
> 	- clarify some binding details
> 
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Artem Bityutskiy <dedekind1@gmail.com>
> Cc: Scott Wood <scottwood@freescale.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Signed-off-by: Jamie Iles <jamie@jamieiles.com>

Pushed to l2-mtd-2.6.git, thanks!

Re: [PATCH] mtd: gpio-nand: add device tree bindings

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 1128 bytes --]

On Sun, 2011-12-18 at 10:00 +0000, Jamie Iles wrote:
> Add device tree bindings so that the gpio-nand driver may be
> instantiated from the device tree.  This also allows the partitions
> to be specified in the device tree.
> 
> v7:	- restore runtime device tree/non device tree detection
> v6:	- convert to mtd_device_parse_register()
> v5:	- fold dt config helpers into a single gpio_nand_of_get_config()
> v4:	- get io sync address from gpio-control-nand,io-sync-reg
> 	  property rather than a resource
> 	- clarified a few details in the binding
> v3:	- remove redundant cast and a couple of whitespace/naming
> 	  changes
> v2:	- add CONFIG_OF guards for non-dt platforms
> 	- compatible becomes gpio-control-nand
> 	- clarify some binding details
> 
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Artem Bityutskiy <dedekind1@gmail.com>
> Cc: Scott Wood <scottwood@freescale.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Signed-off-by: Jamie Iles <jamie@jamieiles.com>

Looks ok to me, but would be great to have some ack from a Device tree
guy.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH] mtd: gpio-nand: add device tree bindings

[Jamie Iles]

Add device tree bindings so that the gpio-nand driver may be
instantiated from the device tree.  This also allows the partitions
to be specified in the device tree.

v7:	- restore runtime device tree/non device tree detection
v6:	- convert to mtd_device_parse_register()
v5:	- fold dt config helpers into a single gpio_nand_of_get_config()
v4:	- get io sync address from gpio-control-nand,io-sync-reg
	  property rather than a resource
	- clarified a few details in the binding
v3:	- remove redundant cast and a couple of whitespace/naming
	  changes
v2:	- add CONFIG_OF guards for non-dt platforms
	- compatible becomes gpio-control-nand
	- clarify some binding details

Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Artem Bityutskiy <dedekind1@gmail.com>
Cc: Scott Wood <scottwood@freescale.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
Signed-off-by: Jamie Iles <jamie@jamieiles.com>
---
 .../devicetree/bindings/mtd/gpio-control-nand.txt  |   44 ++++++++
 drivers/mtd/nand/gpio.c                            |  115 ++++++++++++++++++--
 2 files changed, 152 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mtd/gpio-control-nand.txt

diff --git a/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt b/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt
new file mode 100644
index 0000000..719f4dc
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt
@@ -0,0 +1,44 @@
+GPIO assisted NAND flash
+
+The GPIO assisted NAND flash uses a memory mapped interface to
+read/write the NAND commands and data and GPIO pins for the control
+signals.
+
+Required properties:
+- compatible : "gpio-control-nand"
+- reg : should specify localbus chip select and size used for the chip.  The
+  resource describes the data bus connected to the NAND flash and all accesses
+  are made in native endianness.
+- #address-cells, #size-cells : Must be present if the device has sub-nodes
+  representing partitions.
+- gpios : specifies the gpio pins to control the NAND device.  nwp is an
+  optional gpio and may be set to 0 if not present.
+
+Optional properties:
+- bank-width : Width (in bytes) of the device.  If not present, the width
+  defaults to 1 byte.
+- chip-delay : chip dependent delay for transferring data from array to
+  read registers (tR).  If not present then a default of 20us is used.
+- gpio-control-nand,io-sync-reg : A 64-bit physical address for a read
+  location used to guard against bus reordering with regards to accesses to
+  the GPIO's and the NAND flash data bus.  If present, then after changing
+  GPIO state and before and after command byte writes, this register will be
+  read to ensure that the GPIO accesses have completed.
+
+Examples:
+
+gpio-nand@1,0 {
+	compatible = "gpio-control-nand";
+	reg = <1 0x0000 0x2>;
+	#address-cells = <1>;
+	#size-cells = <1>;
+	gpios = <&banka 1 0	/* rdy */
+		 &banka 2 0 	/* nce */
+		 &banka 3 0 	/* ale */
+		 &banka 4 0 	/* cle */
+		 0		/* nwp */>;
+
+	partition@0 {
+	...
+	};
+};
diff --git a/drivers/mtd/nand/gpio.c b/drivers/mtd/nand/gpio.c
index 2c2060b..d4b5609 100644
--- a/drivers/mtd/nand/gpio.c
+++ b/drivers/mtd/nand/gpio.c
@@ -27,6 +27,9 @@
 #include <linux/mtd/nand.h>
 #include <linux/mtd/partitions.h>
 #include <linux/mtd/nand-gpio.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_gpio.h>
 
 struct gpiomtd {
 	void __iomem		*io_sync;
@@ -171,6 +174,96 @@ static int gpio_nand_devready(struct mtd_info *mtd)
 	return gpio_get_value(gpiomtd->plat.gpio_rdy);
 }
 
+#ifdef CONFIG_OF
+static const struct of_device_id gpio_nand_id_table[] = {
+	{ .compatible = "gpio-control-nand" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, gpio_nand_id_table);
+
+static int gpio_nand_get_config_of(const struct device *dev,
+				   struct gpio_nand_platdata *plat)
+{
+	u32 val;
+
+	if (!of_property_read_u32(dev->of_node, "bank-width", &val)) {
+		if (val == 2) {
+			plat->options |= NAND_BUSWIDTH_16;
+		} else if (val != 1) {
+			dev_err(dev, "invalid bank-width %u\n", val);
+			return -EINVAL;
+		}
+	}
+
+	plat->gpio_rdy = of_get_gpio(dev->of_node, 0);
+	plat->gpio_nce = of_get_gpio(dev->of_node, 1);
+	plat->gpio_ale = of_get_gpio(dev->of_node, 2);
+	plat->gpio_cle = of_get_gpio(dev->of_node, 3);
+	plat->gpio_nwp = of_get_gpio(dev->of_node, 4);
+
+	if (!of_property_read_u32(dev->of_node, "chip-delay", &val))
+		plat->chip_delay = val;
+
+	return 0;
+}
+
+static struct resource *gpio_nand_get_io_sync_of(struct platform_device *pdev)
+{
+	struct resource *r = devm_kzalloc(&pdev->dev, sizeof(*r), GFP_KERNEL);
+	u64 addr;
+
+	if (!r || of_property_read_u64(pdev->dev.of_node,
+				       "gpio-control-nand,io-sync-reg", &addr))
+		return NULL;
+
+	r->start = addr;
+	r->end = r->start + 0x3;
+	r->flags = IORESOURCE_MEM;
+
+	return r;
+}
+#else /* CONFIG_OF */
+#define gpio_nand_id_table NULL
+static inline int gpio_nand_get_config_of(const struct device *dev,
+					  struct gpio_nand_platdata *plat)
+{
+	return -ENOSYS;
+}
+
+static inline struct resource *
+gpio_nand_get_io_sync_of(struct platform_device *pdev)
+{
+	return NULL
+}
+#endif /* CONFIG_OF */
+
+static inline int gpio_nand_get_config(const struct device *dev,
+				       struct gpio_nand_platdata *plat)
+{
+	int ret = gpio_nand_get_config_of(dev, plat);
+
+	if (!ret)
+		return ret;
+
+	if (dev->platform_data) {
+		memcpy(plat, dev->platform_data, sizeof(*plat));
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static inline struct resource *
+gpio_nand_get_io_sync(struct platform_device *pdev)
+{
+	struct resource *r = gpio_nand_get_io_sync_of(pdev);
+
+	if (r)
+		return r;
+
+	return platform_get_resource(pdev, IORESOURCE_MEM, 1);
+}
+
 static int __devexit gpio_nand_remove(struct platform_device *dev)
 {
 	struct gpiomtd *gpiomtd = platform_get_drvdata(dev);
@@ -178,7 +271,7 @@ static int __devexit gpio_nand_remove(struct platform_device *dev)
 
 	nand_release(&gpiomtd->mtd_info);
 
-	res = platform_get_resource(dev, IORESOURCE_MEM, 1);
+	res = gpio_nand_get_io_sync(dev);
 	iounmap(gpiomtd->io_sync);
 	if (res)
 		release_mem_region(res->start, resource_size(res));
@@ -226,9 +319,10 @@ static int __devinit gpio_nand_probe(struct platform_device *dev)
 	struct gpiomtd *gpiomtd;
 	struct nand_chip *this;
 	struct resource *res0, *res1;
-	int ret;
+	struct mtd_part_parser_data ppdata = {};
+	int ret = 0;
 
-	if (!dev->dev.platform_data)
+	if (!dev->dev.of_node && !dev->dev.platform_data)
 		return -EINVAL;
 
 	res0 = platform_get_resource(dev, IORESOURCE_MEM, 0);
@@ -248,7 +342,7 @@ static int __devinit gpio_nand_probe(struct platform_device *dev)
 		goto err_map;
 	}
 
-	res1 = platform_get_resource(dev, IORESOURCE_MEM, 1);
+	res1 = gpio_nand_get_io_sync(dev);
 	if (res1) {
 		gpiomtd->io_sync = request_and_remap(res1, 4, "NAND sync", &ret);
 		if (!gpiomtd->io_sync) {
@@ -257,7 +351,9 @@ static int __devinit gpio_nand_probe(struct platform_device *dev)
 		}
 	}
 
-	memcpy(&gpiomtd->plat, dev->dev.platform_data, sizeof(gpiomtd->plat));
+	ret = gpio_nand_get_config(&dev->dev, &gpiomtd->plat);
+	if (ret)
+		goto err_nce;
 
 	ret = gpio_request(gpiomtd->plat.gpio_nce, "NAND NCE");
 	if (ret)
@@ -316,8 +412,12 @@ static int __devinit gpio_nand_probe(struct platform_device *dev)
 		gpiomtd->plat.adjust_parts(&gpiomtd->plat,
 					   gpiomtd->mtd_info.size);
 
-	mtd_device_register(&gpiomtd->mtd_info, gpiomtd->plat.parts,
-			    gpiomtd->plat.num_parts);
+	ppdata.of_node = dev->dev.of_node;
+	ret = mtd_device_parse_register(&gpiomtd->mtd_info, NULL, &ppdata,
+					gpiomtd->plat.parts,
+					gpiomtd->plat.num_parts);
+	if (ret)
+		goto err_wp;
 	platform_set_drvdata(dev, gpiomtd);
 
 	return 0;
@@ -352,6 +452,7 @@ static struct platform_driver gpio_nand_driver = {
 	.remove		= gpio_nand_remove,
 	.driver		= {
 		.name	= "gpio-nand",
+		.of_match_table = gpio_nand_id_table,
 	},
 };
 
-- 
1.7.5.4