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

[PATCHv3] 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.

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-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
Cc: Artem Bityutskiy <dedekind1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Acked-by: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
Signed-off-by: Jamie Iles <jamie-wmLquQDDieKakBO8gow8eQ@public.gmane.org>
---
 .../devicetree/bindings/mtd/gpio-control-nand.txt  |   40 ++++++++
 drivers/mtd/nand/gpio.c                            |   95 ++++++++++++++++++--
 2 files changed, 129 insertions(+), 6 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..2dc52de
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt
@@ -0,0 +1,40 @@
+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.  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 device.
+- chip-delay : chip dependent delay for transferring data from array to
+  read registers (tR).
+
+Examples:
+
+gpio-nand@1,0 {
+	compatible = "gpio-control-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 */>;
+
+	partition@0 {
+	...
+	};
+};
diff --git a/drivers/mtd/nand/gpio.c b/drivers/mtd/nand/gpio.c
index 2c2060b..045426b 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,79 @@ static void __iomem *request_and_remap(struct resource *res, size_t size,
 	return ptr;
 }
 
+#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_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;
+}
+#else /* CONFIG_OF */
+#define gpio_nand_id_table NULL
+static inline int gpio_nand_of_get_config(struct device *dev,
+					  struct gpio_nand_platdata *plat)
+{
+	return -ENODEV;
+}
+#endif /* CONFIG_OF */
+
 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,7 +325,13 @@ 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_nce;
 
 	ret = gpio_request(gpiomtd->plat.gpio_nce, "NAND NCE");
 	if (ret)
@@ -312,10 +386,18 @@ 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 = ret;
+	}
 	mtd_device_register(&gpiomtd->mtd_info, gpiomtd->plat.parts,
 			    gpiomtd->plat.num_parts);
 	platform_set_drvdata(dev, gpiomtd);
@@ -352,6 +434,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: [PATCHv3] mtd: gpio-nand: add device tree bindings

[Scott Wood]

On Mon, 1 Aug 2011 15:02:54 +0100
Jamie Iles <jamie-wmLquQDDieKakBO8gow8eQ@public.gmane.org> wrote:

> 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..2dc52de
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt
> @@ -0,0 +1,40 @@
> +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.  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.

Specify how the reg regions behave, such as "The first reg resource is a
byte or word that represents the NAND chip's data lines.  The io-sync
resource should be read when...".  What about endianness?

What if some other binding wants to add additional reg resources, while
still being backwards compatible with this binding?  Might be better to
move the sync into its own property -- something like "gpio-nand-io-sync =
<1>" indicating that it's in reg resource #1.  And maybe it should require
some PXA-specific compatible if io-sync is needed.  Even if another chip
requires some sort of sync hack, would it necessarily work the same?

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

No support for NAND chips >= 4GiB?

> +Optional properties:
> +- bank-width : Width (in bytes) of the device.
> +- chip-delay : chip dependent delay for transferring data from array to
> +  read registers (tR).

Why optional?  If there's a default assumption, document it.

> +Examples:
> +
> +gpio-nand@1,0 {
> +	compatible = "gpio-control-nand";
> +	reg = <1 0x0000 0x1000>;

4K seems a bit large for what I'm assuming this region is for.

-Scott

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

[Jamie Iles]

Hi Scott,

On Mon, Aug 01, 2011 at 01:38:25PM -0500, Scott Wood wrote:
> On Mon, 1 Aug 2011 15:02:54 +0100
> Jamie Iles <jamie-wmLquQDDieKakBO8gow8eQ@public.gmane.org> wrote:
> 
> > 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..2dc52de
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt
> > @@ -0,0 +1,40 @@
> > +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.  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.
> 
> Specify how the reg regions behave, such as "The first reg resource is a
> byte or word that represents the NAND chip's data lines.  The io-sync
> resource should be read when...".  What about endianness?

OK, fair points.  I'm not sure what to say about endianness though.  
Host byte order accesses are used in the driver so can I just specify 
this?  We could add a property later to support endianess swapping, but 
I don't want to add too much that I can't test.

> What if some other binding wants to add additional reg resources, while
> still being backwards compatible with this binding?  Might be better to
> move the sync into its own property -- something like "gpio-nand-io-sync =
> <1>" indicating that it's in reg resource #1.  And maybe it should require
> some PXA-specific compatible if io-sync is needed.  Even if another chip
> requires some sort of sync hack, would it necessarily work the same?

Hmm, I'm not convinced there - the sync is to protect against bus 
ordering, and a read from the right region does that.  I'm working on 
another ARM platform (not PXA) that needs this sync so sure it's not PXA 
specific.

The alternative is to not have this specified in the binding and have 
the platform attach the resource.  On my platform for example I need to 
read from the GPIO controller registers and I can't find a way to 
express this when using ranges...

> > +- #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.
> 
> No support for NAND chips >= 4GiB?

OK, I guess this can be relaxed to any value.

> > +Optional properties:
> > +- bank-width : Width (in bytes) of the device.
> > +- chip-delay : chip dependent delay for transferring data from array to
> > +  read registers (tR).
> 
> Why optional?  If there's a default assumption, document it.

OK.

> > +Examples:
> > +
> > +gpio-nand@1,0 {
> > +	compatible = "gpio-control-nand";
> > +	reg = <1 0x0000 0x1000>;
> 
> 4K seems a bit large for what I'm assuming this region is for.

OK, not the best example, it just needs to be at least 2 bytes.  I'll 
update it with a better one.

Jamie

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

[Scott Wood]

On Mon, 1 Aug 2011 20:33:16 +0100
Jamie Iles <jamie@jamieiles.com> wrote:

> OK, fair points.  I'm not sure what to say about endianness though.  
> Host byte order accesses are used in the driver so can I just specify 
> this?  We could add a property later to support endianess swapping, but 
> I don't want to add too much that I can't test.

If the assumption is host endian, that's fine, just document it.

It looks like the code uses a little-endian accessor (readw) in a couple
places.  The instance in gpio_nand_readbuf16() should never be reached
since the NAND layer should never do an unaligned buffer read, but the one
in gpio_nand_verifybuf16() could cause problems.

The implementation in nand_base.c uses readw(), but at least it uses it
consistently between read_buf16(), write_buf16(), and verify_buf16().
readsw()/writesw() do not appear to do byte swapping, at least on powerpc,
while readw() does.

Even so, the generic implementation could read data that is byte-reversed
from what another implementation wrote, or vice versa.  I wonder if there
are any big-endian platforms with 16-bit NAND that use the generic buffer
functions -- doesn't look like it from a quick glance.

> > What if some other binding wants to add additional reg resources, while
> > still being backwards compatible with this binding?  Might be better to
> > move the sync into its own property -- something like "gpio-nand-io-sync =
> > <1>" indicating that it's in reg resource #1.  And maybe it should require
> > some PXA-specific compatible if io-sync is needed.  Even if another chip
> > requires some sort of sync hack, would it necessarily work the same?
> 
> Hmm, I'm not convinced there - the sync is to protect against bus 
> ordering, and a read from the right region does that.  I'm working on 
> another ARM platform (not PXA) that needs this sync so sure it's not PXA 
> specific.

OK, though if you think this will be common enough to include in the
generic binding, is it only going to appear on ARM chips?

What about using a "gpio-nand-io-sync" property instead of assuming that if
there's a second reg resource, it must be this?

> The alternative is to not have this specified in the binding and have 
> the platform attach the resource.

That doesn't sound ideal.

> On my platform for example I need to 
> read from the GPIO controller registers and I can't find a way to 
> express this when using ranges...

I think on that platform you should not specify gpio-control-nand in the
compatible.  Have the driver or platform code match on a specific
compatible, and then do whatever is appropriate internally to Linux to make
it work.

Or perhaps the io sync address should just be a physical address, not a reg
that gets translated.

-Scott


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Jamie Iles]

On Mon, Aug 01, 2011 at 03:12:09PM -0500, Scott Wood wrote:
> On Mon, 1 Aug 2011 20:33:16 +0100
> Jamie Iles <jamie-wmLquQDDieKakBO8gow8eQ@public.gmane.org> wrote:
> 
> > OK, fair points.  I'm not sure what to say about endianness though.  
> > Host byte order accesses are used in the driver so can I just specify 
> > this?  We could add a property later to support endianess swapping, but 
> > I don't want to add too much that I can't test.
> 
> If the assumption is host endian, that's fine, just document it.
> 
> It looks like the code uses a little-endian accessor (readw) in a couple
> places.  The instance in gpio_nand_readbuf16() should never be reached
> since the NAND layer should never do an unaligned buffer read, but the one
> in gpio_nand_verifybuf16() could cause problems.
> 
> The implementation in nand_base.c uses readw(), but at least it uses it
> consistently between read_buf16(), write_buf16(), and verify_buf16().
> readsw()/writesw() do not appear to do byte swapping, at least on powerpc,
> while readw() does.
> 
> Even so, the generic implementation could read data that is byte-reversed
> from what another implementation wrote, or vice versa.  I wonder if there
> are any big-endian platforms with 16-bit NAND that use the generic buffer
> functions -- doesn't look like it from a quick glance.

OK, so for this should I just document that all accesses are 
little-endian?  We can then add properties later if we need something 
different.

> > > What if some other binding wants to add additional reg resources, while
> > > still being backwards compatible with this binding?  Might be better to
> > > move the sync into its own property -- something like "gpio-nand-io-sync =
> > > <1>" indicating that it's in reg resource #1.  And maybe it should require
> > > some PXA-specific compatible if io-sync is needed.  Even if another chip
> > > requires some sort of sync hack, would it necessarily work the same?
> > 
> > Hmm, I'm not convinced there - the sync is to protect against bus 
> > ordering, and a read from the right region does that.  I'm working on 
> > another ARM platform (not PXA) that needs this sync so sure it's not PXA 
> > specific.
> 
> OK, though if you think this will be common enough to include in the
> generic binding, is it only going to appear on ARM chips?

Well there's only one in-tree user then the platform I'm working on 
which are both ARM platforms but it's conceivable that this could appear 
in an MFD or any other platform.  I don't think it makes sense to 
restrict it to ARM as it's a generic problem and _could_ happen on any 
other platform with bus reordering.

> What about using a "gpio-nand-io-sync" property instead of assuming that if
> there's a second reg resource, it must be this?
> 
> > The alternative is to not have this specified in the binding and have 
> > the platform attach the resource.
> 
> That doesn't sound ideal.
> 
> > On my platform for example I need to 
> > read from the GPIO controller registers and I can't find a way to 
> > express this when using ranges...
> 
> I think on that platform you should not specify gpio-control-nand in the
> compatible.  Have the driver or platform code match on a specific
> compatible, and then do whatever is appropriate internally to Linux to make
> it work.
> 
> Or perhaps the io sync address should just be a physical address, not a reg
> that gets translated.

OK, I like the sound of that.  I'm a bit new to the world of device tree 
so I'm not sure of the best way to do this.  Would reading the 
#address-cells property then use of_read_number() be the right way?

Thanks for your help with this Scott, it's much appreciated!

Jamie

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

[Scott Wood]

On Mon, 1 Aug 2011 21:25:36 +0100
Jamie Iles <jamie-wmLquQDDieKakBO8gow8eQ@public.gmane.org> wrote:

> On Mon, Aug 01, 2011 at 03:12:09PM -0500, Scott Wood wrote:
> > It looks like the code uses a little-endian accessor (readw) in a couple
> > places.  The instance in gpio_nand_readbuf16() should never be reached
> > since the NAND layer should never do an unaligned buffer read, but the one
> > in gpio_nand_verifybuf16() could cause problems.
[snip]
> 
> OK, so for this should I just document that all accesses are 
> little-endian?  We can then add properties later if we need something 
> different.

Right now, the driver is using a mix of native and little endian accesses.
That's not something the binding can fix. :-)

Native endian is what it should be.

> > Or perhaps the io sync address should just be a physical address, not a reg
> > that gets translated.
> 
> OK, I like the sound of that.  I'm a bit new to the world of device tree 
> so I'm not sure of the best way to do this.  Would reading the 
> #address-cells property then use of_read_number() be the right way?

I'd just unconditionally define it as a 64-bit physical address.

-Scott

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

[Scott Wood]

On Mon, 1 Aug 2011 15:39:50 -0500
Scott Wood <scottwood@freescale.com> wrote:

> On Mon, 1 Aug 2011 21:25:36 +0100
> Jamie Iles <jamie@jamieiles.com> wrote:
> 
> > On Mon, Aug 01, 2011 at 03:12:09PM -0500, Scott Wood wrote:
> > > Or perhaps the io sync address should just be a physical address, not a reg
> > > that gets translated.
> > 
> > OK, I like the sound of that.  I'm a bit new to the world of device tree 
> > so I'm not sure of the best way to do this.  Would reading the 
> > #address-cells property then use of_read_number() be the right way?
> 
> I'd just unconditionally define it as a 64-bit physical address.

To be clear, it should go in its own property, not be associated with "reg"
at all.

-Scott


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/