GPIO NAND flash driver

drivers/mtd/nand/gpio.c new file +8 -0

+/* gpio_nand_dosync()
+ *
+ * needed due to bus-reordering within the PXA itself (see section on
+ * I/O ordering in PXA manual (section 2.3, p35)
+ */
+static void gpio_nand_dosync(struct gpiomtd *gpiomtd)
+{
+       unsigned long tmp;

Quoted 15 lines · Mike Rapoport

drivers/mtd/nand/gpio.c new file +15 -0

+/* gpio_nand_dosync()
+ *
+ * needed due to bus-reordering within the PXA itself (see section on
+ * I/O ordering in PXA manual (section 2.3, p35)
+ */
+static void gpio_nand_dosync(struct gpiomtd *gpiomtd)
+{
+       unsigned long tmp;
+
+       if (gpiomtd->io_sync) {
+               /*
+                * this should generate the read, followed by
+                * something that depends on the read
+                */
+               tmp = readl(gpiomtd->io_sync);

drivers/mtd/nand/gpio.c line 57

Quoted 3 lines

drivers/mtd/nand/gpio.c new file +3 -0

+               asm volatile("mov %1, %0\n" : "=r" (tmp) : "r" (tmp));
+       }
+}

there isnt much point in attempting to write a generic solution if you're just going to turn around and stick straight assembly in the middle of it. why not use a real arch-generic method like a memory barrier.

Quoted 16 lines · Mike Frysinger

Quoted 15 lines · Mike Rapoport

drivers/mtd/nand/gpio.c new file +15 -0

+/* gpio_nand_dosync()
+ *
+ * needed due to bus-reordering within the PXA itself (see section on
+ * I/O ordering in PXA manual (section 2.3, p35)
+ */
+static void gpio_nand_dosync(struct gpiomtd *gpiomtd)
+{
+       unsigned long tmp;
+
+       if (gpiomtd->io_sync) {
+               /*
+                * this should generate the read, followed by
+                * something that depends on the read
+                */
+               tmp = readl(gpiomtd->io_sync);

Quoted 8 lines

drivers/mtd/nand/gpio.c line 57

Quoted 3 lines

drivers/mtd/nand/gpio.c new file +3 -0

+               asm volatile("mov %1, %0\n" : "=r" (tmp) : "r" (tmp));
+       }
+}

there isnt much point in attempting to write a generic solution if you're just going to turn around and stick straight assembly in the middle of it. why not use a real arch-generic method like a memory barrier.

Linux memory barriers don't cater for what's required here. What's required is what's there - a read from a separate region with a dependency on that read. Linux has no such barrier.

Quoted 24 lines · Russell King - ARM Linux

Quoted 23 lines · Mike Frysinger

Quoted 8 lines · Mike Rapoport

drivers/mtd/nand/gpio.c new file +8 -0

+/* gpio_nand_dosync()
+ *
+ * needed due to bus-reordering within the PXA itself (see section on
+ * I/O ordering in PXA manual (section 2.3, p35)
+ */
+static void gpio_nand_dosync(struct gpiomtd *gpiomtd)
+{
+       unsigned long tmp;

drivers/mtd/nand/gpio.c line 57

Quoted 10 lines

drivers/mtd/nand/gpio.c new file +10 -0

+
+       if (gpiomtd->io_sync) {
+               /*
+                * this should generate the read, followed by
+                * something that depends on the read
+                */
+               tmp = readl(gpiomtd->io_sync);
+               asm volatile("mov %1, %0\n" : "=r" (tmp) : "r" (tmp));
+       }
+}

there isnt much point in attempting to write a generic solution if you're just going to turn around and stick straight assembly in the middle of it. why not use a real arch-generic method like a memory barrier.

Quoted 3 lines

Linux memory barriers don't cater for what's required here. What's required is what's there - a read from a separate region with a dependency on that read. Linux has no such barrier.

thanks, this kind of comment in the source would be useful ... but what is also needed is '#ifdef __arm__'

-mike

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

Quoted 25 lines · Mike Frysinger

Quoted 24 lines · Russell King - ARM Linux

Quoted 23 lines · Mike Frysinger

Quoted 8 lines · Mike Rapoport

drivers/mtd/nand/gpio.c new file +8 -0

+/* gpio_nand_dosync()
+ *
+ * needed due to bus-reordering within the PXA itself (see section on
+ * I/O ordering in PXA manual (section 2.3, p35)
+ */
+static void gpio_nand_dosync(struct gpiomtd *gpiomtd)
+{
+       unsigned long tmp;

drivers/mtd/nand/gpio.c line 57

Quoted 10 lines

drivers/mtd/nand/gpio.c new file +10 -0

+
+       if (gpiomtd->io_sync) {
+               /*
+                * this should generate the read, followed by
+                * something that depends on the read
+                */
+               tmp = readl(gpiomtd->io_sync);
+               asm volatile("mov %1, %0\n" : "=r" (tmp) : "r" (tmp));
+       }
+}

there isnt much point in attempting to write a generic solution if you're just going to turn around and stick straight assembly in the middle of it. why not use a real arch-generic method like a memory barrier.

Quoted 6 lines

Quoted 3 lines

Linux memory barriers don't cater for what's required here. What's required is what's there - a read from a separate region with a dependency on that read. Linux has no such barrier.

thanks, this kind of comment in the source would be useful ... but what is also needed is '#ifdef __arm__'

If '#ifdef' is preferable than 'depend on ARM' I can send an updated patch.

Quoted 29 lines · Mike Rapoport

Quoted 28 lines · Mike Frysinger

Quoted 27 lines · Russell King - ARM Linux

Quoted 15 lines · Mike Frysinger

Quoted 14 lines · Mike Rapoport

(quoted diff) +14 -0

+/* gpio_nand_dosync()
+ *
+ * needed due to bus-reordering within the PXA itself (see section on
+ * I/O ordering in PXA manual (section 2.3, p35)
+ */
+static void gpio_nand_dosync(struct gpiomtd *gpiomtd)
+{
+       unsigned long tmp;
+
+       if (gpiomtd->io_sync) {
+               /*
+                * this should generate the read, followed by
+                * something that depends on the read
+                */

Quoted 8 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+               tmp = readl(gpiomtd->io_sync);
+               asm volatile("mov %1, %0\n" : "=r" (tmp) : "r" (tmp));
+       }
+}

there isnt much point in attempting to write a generic solution if you're just going to turn around and stick straight assembly in the middle of it. why not use a real arch-generic method like a memory barrier.

Linux memory barriers don't cater for what's required here. What's required is what's there - a read from a separate region with a dependency on that read. Linux has no such barrier.

Quoted 3 lines

Quoted 2 lines

thanks, this kind of comment in the source would be useful ... but what is also needed is '#ifdef __arm__'

If '#ifdef' is preferable than 'depend on ARM' I can send an updated patch.

i'd remove the "depend on ARM" and add "#ifdef __arm__" into the .c file. unless i missed something, there's nothing else arm specific in there.
-mike

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

+static inline void gpio_nand_dosync(struct gpiomtd *gpiomtd) {}

drivers/mtd/nand/gpio.c line 46

Quoted 6 lines · Mike Rapoport

drivers/mtd/nand/gpio.c new file +6 -0

+#ifdef CONFIG_ARM
+/* gpio_nand_dosync()
+ *
+ * needed due to bus-reordering within the PXA itself (see section on
+ * I/O ordering in PXA manual (section 2.3, p35)
+ */

Is CONFIG_ARM the same as "is a PXA"?

drivers/mtd/nand/gpio.c line 56

Quoted 5 lines

drivers/mtd/nand/gpio.c new file +5 -0

+		/*
+		 * Linux memory barriers don't cater for what's required here.
+		 * What's required is what's here - a read from a separate
+		 * region with a dependency on that read.
+		 */

It would be nice if this comment explained _why_ it's needed here, not just what it's doing but why this MTD device in particular needs it - for the benefit of someone using this driver on another architecture.

If the problem is that "readl" alone doesn't force a read cycle on PXA, it sounds like "readl" on PXA contains a bug which may affect other drivers on that architecture too, and that the right place to fix it is in "readl".

Quoted 1 line

+static inline void gpio_nand_dosync(struct gpiomtd *gpiomtd) {}

Is this dummy implementation likely to be correct on non-ARM architectures, or is this just faking it so the code will compile?

Quoted 7 lines · Jamie Lokier

drivers/mtd/nand/gpio.c line 56

Quoted 5 lines · Mike Rapoport

drivers/mtd/nand/gpio.c new file +5 -0

+           /*
+            * Linux memory barriers don't cater for what's required here.
+            * What's required is what's here - a read from a separate
+            * region with a dependency on that read.
+            */

Quoted 8 lines

It would be nice if this comment explained _why_ it's needed here, not just what it's doing but why this MTD device in particular needs it - for the benefit of someone using this driver on another architecture.

If the problem is that "readl" alone doesn't force a read cycle on PXA, it sounds like "readl" on PXA contains a bug which may affect other drivers on that architecture too, and that the right place to fix it is in "readl".

The problem is that a write to GPIO may pass a write to the static memory regions, or vice versa. So, what we do is we insert a read with a dependency in the execution to ensure that we stall the pipeline until that read - and therefore the preceding write has completed.

Quoted 4 lines

Quoted 1 line

+static inline void gpio_nand_dosync(struct gpiomtd *gpiomtd) {}

Is this dummy implementation likely to be correct on non-ARM architectures, or is this just faking it so the code will compile?

I suspect what's required for various architectures has yet to be ascertained. To ask ARM folk to work out what's required for other architectures is just like asking you what's required for ARM - I'd guess that you've no idea what ARM would require. In the same way, we have no idea what other architectures require to ensure that the NAND chip sees GPIO changes before actually accessing the NAND.

Quoted 5 lines

The problem is that a write to GPIO may pass a write to the static memory regions, or vice versa. So, what we do is we insert a read with a dependency in the execution to ensure that we stall the pipeline until that read - and therefore the preceding write has completed.

so the function comment should read something like "make sure the gpio state has actually changed before returning to the higher nand layers"

Quoted 8 lines · Mike Frysinger

Quoted 5 lines

The problem is that a write to GPIO may pass a write to the static memory regions, or vice versa. So, what we do is we insert a read with a dependency in the execution to ensure that we stall the pipeline until that read - and therefore the preceding write has completed.

so the function comment should read something like "make sure the gpio state has actually changed before returning to the higher nand layers"

The patch with (hopefully) clearer gpio_nand_dosync() comments.

+static int gpio_nand_remove(struct platform_device *dev)

+static int gpio_nand_probe(struct platform_device *dev)

drivers/mtd/nand/gpio.c new file +13 -0

+err_wp:
+       if (gpio_is_valid(gpiomtd->plat.gpio_nwp))
+               gpio_set_value(gpiomtd->plat.gpio_nwp, 0);
+       gpio_free(gpiomtd->plat.gpio_rdy);
+err_rdy:
+       gpio_free(gpiomtd->plat.gpio_cle);
+err_cle:
+       gpio_free(gpiomtd->plat.gpio_ale);
+err_ale:
+       if (gpio_is_valid(gpiomtd->plat.gpio_nwp))
+               gpio_free(gpiomtd->plat.gpio_nwp);
+err_nwp:
+       gpio_free(gpiomtd->plat.gpio_nce);

drivers/mtd/nand/gpio.c line 198

Quoted 9 lines · Mike Rapoport

drivers/mtd/nand/gpio.c new file +9 -0

+       if (gpio_is_valid(gpiomtd->plat.gpio_nwp))
+               gpio_set_value(gpiomtd->plat.gpio_nwp, 0);
+       gpio_set_value(gpiomtd->plat.gpio_nce, 1);
+
+       gpio_free(gpiomtd->plat.gpio_cle);
+       gpio_free(gpiomtd->plat.gpio_ale);
+       gpio_free(gpiomtd->plat.gpio_nce);
+       if (gpio_is_valid(gpiomtd->plat.gpio_nwp))
+               gpio_free(gpiomtd->plat.gpio_nwp);

why do you bother setting the value before releasing ? when you release, the pin tends to go to the hardware default and on the Blackfin, that tends to be "tristate". are you just banking on the idea that the pin will stay the way it was last set before it gets freed ?

you really should be returning "ret" rather than "-ENOMEM" as many (most?) of the ways you'd get here will not be due to out of memory conditions. some error paths above will probably require you to manually set "ret" to something relevant ...

Quoted 11 lines · Mike Frysinger

drivers/mtd/nand/gpio.c line 198

Quoted 9 lines · Mike Rapoport

drivers/mtd/nand/gpio.c new file +9 -0

+       if (gpio_is_valid(gpiomtd->plat.gpio_nwp))
+               gpio_set_value(gpiomtd->plat.gpio_nwp, 0);
+       gpio_set_value(gpiomtd->plat.gpio_nce, 1);
+
+       gpio_free(gpiomtd->plat.gpio_cle);
+       gpio_free(gpiomtd->plat.gpio_ale);
+       gpio_free(gpiomtd->plat.gpio_nce);
+       if (gpio_is_valid(gpiomtd->plat.gpio_nwp))
+               gpio_free(gpiomtd->plat.gpio_nwp);

Quoted 5 lines

why do you bother setting the value before releasing ? when you release, the pin tends to go to the hardware default and on the Blackfin, that tends to be "tristate". are you just banking on the idea that the pin will stay the way it was last set before it gets freed ?

Maybe you should reconsider that behaviour - this is a prime example why such behaviour is wrong. If you leave the NAND signals floating, you're going to eat power - most CMOS based devices want inputs to be either logic high or low, and not floating otherwise they eat power.

Moreover, you don't want to leave memory control lines floating - you don't want them to pick up noise which may be transmitted inside the NAND chip and may cause malfunction.

Lastly, you don't want spurious writes to the NAND memory region (think DMA controller scribbling over memory because of some buggy driver) to write into the NAND, or maybe cause the NAND to erase itself.

There's another reason for doing this. Think GPIO line connected to a loudspeaker amplifier shutdown input. Do you really want that line floating when the sound driver isn't loaded?

On ARM, certainly PXA, the last GPIO state is preserved when free'd.

Quoted 13 lines · Russell King - ARM Linux

Maybe you should reconsider that behaviour - this is a prime example why such behaviour is wrong. If you leave the NAND signals floating, you're going to eat power - most CMOS based devices want inputs to be either logic high or low, and not floating otherwise they eat power.

Moreover, you don't want to leave memory control lines floating - you don't want them to pick up noise which may be transmitted inside the NAND chip and may cause malfunction.

Lastly, you don't want spurious writes to the NAND memory region (think DMA controller scribbling over memory because of some buggy driver) to write into the NAND, or maybe cause the NAND to erase itself.

Quoted 3 lines

There's another reason for doing this. Think GPIO line connected to a loudspeaker amplifier shutdown input. Do you really want that line floating when the sound driver isn't loaded?

this is what pull downs/pull ups are for.

your arguments can just as readily be applied to the powering up state and initialization stages. software cant (and shouldnt) rectify crappy hardware designs.

Quoted 1 line

On ARM, certainly PXA, the last GPIO state is preserved when free'd.

if certain behavior is expected, then it should be codified. i see no mention (unless i just missed it) of expected behavior upon freeing in Documentation/gpio.txt.

Consider the loudspeaker amplifier example. Does it matter if the amplifier is powered up for a few milliseconds on boot? No.

Quoted 3 lines · Russell King - ARM Linux

Consider the loudspeaker amplifier example. Does it matter if the amplifier is powered up for a few milliseconds on boot? No.

Sometimes it does. Partly because that usually means it's also powered up during wake from suspend -- so you get horrible clicks when resume. Which if you suspend as often as OLPC does, for example, is a pain.

So, while you see "lack of pull ups" as crappy design, others will see it as a power and cost saving measure.

Quoted 14 lines · Mike Frysinger

Quoted 13 lines · Russell King - ARM Linux

Maybe you should reconsider that behaviour - this is a prime example why such behaviour is wrong. If you leave the NAND signals floating, you're going to eat power - most CMOS based devices want inputs to be either logic high or low, and not floating otherwise they eat power.

Moreover, you don't want to leave memory control lines floating - you don't want them to pick up noise which may be transmitted inside the NAND chip and may cause malfunction.

Lastly, you don't want spurious writes to the NAND memory region (think DMA controller scribbling over memory because of some buggy driver) to write into the NAND, or maybe cause the NAND to erase itself.

Quoted 5 lines

Quoted 3 lines

There's another reason for doing this. Think GPIO line connected to a loudspeaker amplifier shutdown input. Do you really want that line floating when the sound driver isn't loaded?

this is what pull downs/pull ups are for.

So says you. Not everyone will agree with that statement, especially on designs which don't behave as you've described in your previous mail.

Quoted 3 lines

your arguments can just as readily be applied to the powering up state and initialization stages. software cant (and shouldnt) rectify crappy hardware designs.

No they don't. Consider the loudspeaker amplifier example. Does it matter if the amplifier is powered up for a few milliseconds on boot? No.

Does it matter if the NAND control lines are at an indeterminent state at boot? Probably not, you'd hope that the boot software is small and don't randomly scribble into your flash storage. The boot software may even setup the NAND control lines so it can read the kernel from flash. Does it matter while the kernel is running, which will have setup the entire system so there's more probability of things going wrong?

Yes, it could make your platform unbootable if the NAND gets corrupted.

Quoted 5 lines

Quoted 1 line

On ARM, certainly PXA, the last GPIO state is preserved when free'd.

if certain behavior is expected, then it should be codified. i see no mention (unless i just missed it) of expected behavior upon freeing in Documentation/gpio.txt.

It doesn't. The fact that the GPIO state is preserved when free'd on PXA is just that it takes _more_ code to do anything else.

Moreover, all ARM processors I've seen have the ability to set GPIO state on low power modes, which makes the addition of lots of pull up and pull down resistors not only a needlessly expense for production runs, but also wastes power when the signal is held at the opposite level to which it's being pulled to.

I'll say that I've never liked this GPIO layer, because it breads ideas like you're putting forward, which have traditionally not been needed to be thought about when writing direct to the registers - where you have system wide control of the GPIO state without any of this "on gpio_free() such and such might happen" ideas.

Another example - think about a GPIO which is shared between two drivers. Yes, there are platforms which do this. No single driver owns the GPIO signal. What happens if it's gpio_free'd while the other driver is still using it? Yes, the GPIO layer sucks for that, but like it or not, it's reality and the GPIO API doesn't cater for it.

And don't say, that's crappy hardware then. Stop being a purist and become a realist. It's reality and there is no option but to make it work.

Quoted 2 lines · Russell King - ARM Linux

It doesn't. The fact that the GPIO state is preserved when free'd on PXA is just that it takes _more_ code to do anything else.

so which is it ? GPIO state *should* be preserved, or PXA does it simply due to code frugality ?

if the API behavior is strictly documented, your complaint here is pretty moot.

Quoted 6 lines · Mike Frysinger

Quoted 2 lines · Russell King - ARM Linux

It doesn't. The fact that the GPIO state is preserved when free'd on PXA is just that it takes _more_ code to do anything else.

so which is it ? GPIO state *should* be preserved, or PXA does it simply due to code frugality ?

May I remind you that _you_ are the one with the system which doesn't preserve GPIO state.

Quoted 2 lines

if the API behavior is strictly documented, your complaint here is pretty moot.

My complaint? I don't have a complaint. You are the one with the complaint with the driver that's being discussed. You're the one who's moaning about it setting state before calling gpio_free.

I see no point in continuing this discussion; your arguments are just plain silly. I've explained _why_ we're doing it.

Our GPIO hardware behaves differently from yours. Our gpio_free() is side-effect free. Get over it.

Maybe you should reconsider that behaviour - this is a prime example why such behaviour is wrong.
...
I'll say that I've never liked this GPIO layer, because it breads ideas like you're putting forward, which have traditionally not been needed to be thought about when writing direct to the registers - where you have system wide control of the GPIO state without any of this "on gpio_free() such and such might happen" ideas.

if certain behavior is expected, then it should be codified. i see no mention (unless i just missed it) of expected behavior upon freeing in Documentation/gpio.txt.

Quoted 18 lines · Russell King - ARM Linux

Quoted 6 lines · Mike Frysinger

Quoted 2 lines · Russell King - ARM Linux

It doesn't. The fact that the GPIO state is preserved when free'd on PXA is just that it takes _more_ code to do anything else.

so which is it ? GPIO state *should* be preserved, or PXA does it simply due to code frugality ?

May I remind you that _you_ are the one with the system which doesn't preserve GPIO state.

Quoted 2 lines

if the API behavior is strictly documented, your complaint here is pretty moot.

My complaint? I don't have a complaint. You are the one with the complaint with the driver that's being discussed. You're the one who's moaning about it setting state before calling gpio_free.

Quoted 5 lines

I see no point in continuing this discussion; your arguments are just plain silly. I've explained _why_ we're doing it.

Our GPIO hardware behaves differently from yours. Our gpio_free() is side-effect free. Get over it.

i'm attempting to get things fixed for everyone. stop being so abrasive over nothing.

my question is simply:

Quoted 3 lines

if certain behavior is expected, then it should be codified. i see no mention (unless i just missed it) of expected behavior upon freeing in Documentation/gpio.txt.

and i havent gotten a straight answer out of you about this. should state be retained when a pin gets freed or not ?

Quoted 1 line

The patch with (hopefully) clearer gpio_nand_dosync() comments.

looks like it, thanks for that

Quoted 1 line

+static int gpio_nand_remove(struct platform_device *dev)

should have __devexit markings

Review

Quoted 9 lines

(quoted diff) +9 -0

+       if (gpio_is_valid(gpiomtd->plat.gpio_nwp))
+               gpio_set_value(gpiomtd->plat.gpio_nwp, 0);
+       gpio_set_value(gpiomtd->plat.gpio_nce, 1);
+
+       gpio_free(gpiomtd->plat.gpio_cle);
+       gpio_free(gpiomtd->plat.gpio_ale);
+       gpio_free(gpiomtd->plat.gpio_nce);
+       if (gpio_is_valid(gpiomtd->plat.gpio_nwp))
+               gpio_free(gpiomtd->plat.gpio_nwp);

why do you bother setting the value before releasing ? when you release, the pin tends to go to the hardware default and on the Blackfin, that tends to be "tristate". are you just banking on the idea that the pin will stay the way it was last set before it gets freed ?

Quoted 1 line

+static int gpio_nand_probe(struct platform_device *dev)

should have __devinit markings

Review

Quoted 3 lines

(quoted diff) +3 -0

+err_map:
+       kfree(gpiomtd);
+       return -ENOMEM;

you really should be returning "ret" rather than "-ENOMEM" as many (most?) of the ways you'd get here will not be due to out of memory conditions. some error paths above will probably require you to manually set "ret" to something relevant ...

-mike

Quoted 6 lines · Mike Frysinger

Quoted 5 lines

The problem is that a write to GPIO may pass a write to the static memory regions, or vice versa. So, what we do is we insert a read with a dependency in the execution to ensure that we stall the pipeline until that read - and therefore the preceding write has completed.

Quoted 3 lines

so the function comment should read something like "make sure the gpio state has actually changed before returning to the higher nand layers"

The patch with (hopefully) clearer gpio_nand_dosync() comments.

Please can I have it in a form I can apply, with changelog and signed-off-by, and without those silly 'u16' types in it. The C language has perfectly good types for specifying unsigned 16-bit integers; use them.

Thanks.

Quoted 6 lines · David Woodhouse

Please can I have it in a form I can apply, with changelog and signed-off-by, and without those silly 'u16' types in it. The C language has perfectly good types for specifying unsigned 16-bit integers; use them.

Quoted 1 line

Thanks.

The patch adds support for NAND flashes connected to GPIOs.

Signed-off-by Ben Dooks <[email protected]>

Signed-off-by Russell King <[email protected]>

Signed-off-by Mike Rapoport <[email protected]>

 drivers/mtd/nand/Kconfig      |    6 +
 drivers/mtd/nand/Makefile     |    1 +
 drivers/mtd/nand/gpio.c       |  375 +++++++++++++++++++++++++++++++++++++++++
 include/linux/mtd/nand-gpio.h |   19 ++
 4 files changed, 401 insertions(+), 0 deletions(-)

drivers/mtd/nand/Kconfig +6 -0

index 41f361c..e98991b 100644
@@ -56,6 +56,12 @@ config MTD_NAND_H1900
 	help
 	  This enables the driver for the iPAQ h1900 flash.
 
+config MTD_NAND_GPIO
+	tristate "GPIO NAND Flash driver"
+	depends on GENERIC_GPIO
+	help
+	  This enables a GPIO based NAND flash driver.
+
 config MTD_NAND_SPIA
 	tristate "NAND Flash device on SPIA board"
 	depends on ARCH_P720T

drivers/mtd/nand/Makefile +1 -0

index b786c5d..39eefc2 100644
@@ -24,6 +24,7 @@ obj-$(CONFIG_MTD_NAND_NANDSIM)		+= nandsim.o
 obj-$(CONFIG_MTD_NAND_CS553X)		+= cs553x_nand.o
 obj-$(CONFIG_MTD_NAND_NDFC)		+= ndfc.o
 obj-$(CONFIG_MTD_NAND_ATMEL)		+= atmel_nand.o
+obj-$(CONFIG_MTD_NAND_GPIO)		+= gpio.o
 obj-$(CONFIG_MTD_NAND_CM_X270)		+= cmx270_nand.o
 obj-$(CONFIG_MTD_NAND_BASLER_EXCITE)	+= excite_nandflash.o
 obj-$(CONFIG_MTD_NAND_PXA3xx)		+= pxa3xx_nand.o

drivers/mtd/nand/gpio.c new file +375 -0

new file mode 100644
index 0000000..083bab4
@@ -0,0 +1,375 @@
+/*
+ * drivers/mtd/nand/gpio.c
+ *
+ * Updated, and converted to generic GPIO based driver by Russell King.
+ *
+ * Written by Ben Dooks <[email protected]>
+ *   Based on 2.4 version by Mark Whittaker
+ *
+ * (c) 2004 Simtec Electronics
+ *
+ * Device driver for NAND connected via GPIO
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/gpio.h>
+#include <linux/io.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/nand.h>
+#include <linux/mtd/partitions.h>
+#include <linux/mtd/nand-gpio.h>
+
+struct gpiomtd {
+	void __iomem		*io_sync;
+	struct mtd_info		mtd_info;
+	struct nand_chip	nand_chip;
+	struct gpio_nand_platdata plat;
+};
+
+#define gpio_nand_getpriv(x) container_of(x, struct gpiomtd, mtd_info)
+
+
+#ifdef CONFIG_ARM
+/* gpio_nand_dosync()
+ *
+ * Make sure the GPIO state changes occur in-order with writes to NAND
+ * memory region.
+ * Needed on PXA due to bus-reordering within the SoC itself (see section on
+ * I/O ordering in PXA manual (section 2.3, p35)
+ */
+static void gpio_nand_dosync(struct gpiomtd *gpiomtd)
+{
+	unsigned long tmp;
+
+	if (gpiomtd->io_sync) {
+		/*
+		 * Linux memory barriers don't cater for what's required here.
+		 * What's required is what's here - a read from a separate
+		 * region with a dependency on that read.
+		 */
+		tmp = readl(gpiomtd->io_sync);
+		asm volatile("mov %1, %0\n" : "=r" (tmp) : "r" (tmp));
+	}
+}
+#else
+static inline void gpio_nand_dosync(struct gpiomtd *gpiomtd) {}
+#endif
+
+static void gpio_nand_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int ctrl)
+{
+	struct gpiomtd *gpiomtd = gpio_nand_getpriv(mtd);
+
+	gpio_nand_dosync(gpiomtd);
+
+	if (ctrl & NAND_CTRL_CHANGE) {
+		gpio_set_value(gpiomtd->plat.gpio_nce, !(ctrl & NAND_NCE));
+		gpio_set_value(gpiomtd->plat.gpio_cle, !!(ctrl & NAND_CLE));
+		gpio_set_value(gpiomtd->plat.gpio_ale, !!(ctrl & NAND_ALE));
+		gpio_nand_dosync(gpiomtd);
+	}
+	if (cmd == NAND_CMD_NONE)
+		return;
+
+	writeb(cmd, gpiomtd->nand_chip.IO_ADDR_W);
+	gpio_nand_dosync(gpiomtd);
+}
+
+static void gpio_nand_writebuf(struct mtd_info *mtd, const u_char *buf, int len)
+{
+	struct nand_chip *this = mtd->priv;
+
+	writesb(this->IO_ADDR_W, buf, len);
+}
+
+static void gpio_nand_readbuf(struct mtd_info *mtd, u_char *buf, int len)
+{
+	struct nand_chip *this = mtd->priv;
+
+	readsb(this->IO_ADDR_R, buf, len);
+}
+
+static int gpio_nand_verifybuf(struct mtd_info *mtd, const u_char *buf, int len)
+{
+	struct nand_chip *this = mtd->priv;
+	unsigned char read, *p = (unsigned char *) buf;
+	int i, err = 0;
+
+	for (i = 0; i < len; i++) {
+		read = readb(this->IO_ADDR_R);
+		if (read != p[i]) {
+			pr_debug("%s: err at %d (read %04x vs %04x)\n",
+			       __func__, i, read, p[i]);
+			err = -EFAULT;
+		}
+	}
+	return err;
+}
+
+static void gpio_nand_writebuf16(struct mtd_info *mtd, const u_char *buf,
+				 int len)
+{
+	struct nand_chip *this = mtd->priv;
+
+	if (IS_ALIGNED((unsigned long)buf, 2)) {
+		writesw(this->IO_ADDR_W, buf, len>>1);
+	} else {
+		int i;
+		unsigned short *ptr = (unsigned short *)buf;
+
+		for (i = 0; i < len; i += 2, ptr++)
+			writew(*ptr, this->IO_ADDR_W);
+	}
+}
+
+static void gpio_nand_readbuf16(struct mtd_info *mtd, u_char *buf, int len)
+{
+	struct nand_chip *this = mtd->priv;
+
+	if (IS_ALIGNED((unsigned long)buf, 2)) {
+		readsw(this->IO_ADDR_R, buf, len>>1);
+	} else {
+		int i;
+		unsigned short *ptr = (unsigned short *)buf;
+
+		for (i = 0; i < len; i += 2, ptr++)
+			*ptr = readw(this->IO_ADDR_R);
+	}
+}
+
+static int gpio_nand_verifybuf16(struct mtd_info *mtd, const u_char *buf,
+				 int len)
+{
+	struct nand_chip *this = mtd->priv;
+	unsigned short read, *p = (unsigned short *) buf;
+	int i, err = 0;
+	len >>= 1;
+
+	for (i = 0; i < len; i++) {
+		read = readw(this->IO_ADDR_R);
+		if (read != p[i]) {
+			pr_debug("%s: err at %d (read %04x vs %04x)\n",
+			       __func__, i, read, p[i]);
+			err = -EFAULT;
+		}
+	}
+	return err;
+}
+
+
+static int gpio_nand_devready(struct mtd_info *mtd)
+{
+	struct gpiomtd *gpiomtd = gpio_nand_getpriv(mtd);
+	return gpio_get_value(gpiomtd->plat.gpio_rdy);
+}
+
+static int __devexit gpio_nand_remove(struct platform_device *dev)
+{
+	struct gpiomtd *gpiomtd = platform_get_drvdata(dev);
+	struct resource *res;
+
+	nand_release(&gpiomtd->mtd_info);
+
+	res = platform_get_resource(dev, IORESOURCE_MEM, 1);
+	iounmap(gpiomtd->io_sync);
+	if (res)
+		release_mem_region(res->start, res->end - res->start + 1);
+
+	res = platform_get_resource(dev, IORESOURCE_MEM, 0);
+	iounmap(gpiomtd->nand_chip.IO_ADDR_R);
+	release_mem_region(res->start, res->end - res->start + 1);
+
+	if (gpio_is_valid(gpiomtd->plat.gpio_nwp))
+		gpio_set_value(gpiomtd->plat.gpio_nwp, 0);
+	gpio_set_value(gpiomtd->plat.gpio_nce, 1);
+
+	gpio_free(gpiomtd->plat.gpio_cle);
+	gpio_free(gpiomtd->plat.gpio_ale);
+	gpio_free(gpiomtd->plat.gpio_nce);
+	if (gpio_is_valid(gpiomtd->plat.gpio_nwp))
+		gpio_free(gpiomtd->plat.gpio_nwp);
+	gpio_free(gpiomtd->plat.gpio_rdy);
+
+	kfree(gpiomtd);
+
+	return 0;
+}
+
+static void __iomem *request_and_remap(struct resource *res, size_t size,
+					const char *name, int *err)
+{
+	void __iomem *ptr;
+
+	if (!request_mem_region(res->start, res->end - res->start + 1, name)) {
+		*err = -EBUSY;
+		return NULL;
+	}
+
+	ptr = ioremap(res->start, size);
+	if (!ptr) {
+		release_mem_region(res->start, res->end - res->start + 1);
+		*err = -ENOMEM;
+	}
+	return ptr;
+}
+
+static int __devinit gpio_nand_probe(struct platform_device *dev)
+{
+	struct gpiomtd *gpiomtd;
+	struct nand_chip *this;
+	struct resource *res0, *res1;
+	int ret;
+
+	if (!dev->dev.platform_data)
+		return -EINVAL;
+
+	res0 = platform_get_resource(dev, IORESOURCE_MEM, 0);
+	if (!res0)
+		return -EINVAL;
+
+	gpiomtd = kzalloc(sizeof(*gpiomtd), GFP_KERNEL);
+	if (gpiomtd == NULL) {
+		dev_err(&dev->dev, "failed to create NAND MTD\n");
+		return -ENOMEM;
+	}
+
+	this = &gpiomtd->nand_chip;
+	this->IO_ADDR_R = request_and_remap(res0, 2, "NAND", &ret);
+	if (!this->IO_ADDR_R) {
+		dev_err(&dev->dev, "unable to map NAND\n");
+		goto err_map;
+	}
+
+	res1 = platform_get_resource(dev, IORESOURCE_MEM, 1);
+	if (res1) {
+		gpiomtd->io_sync = request_and_remap(res1, 4, "NAND sync", &ret);
+		if (!gpiomtd->io_sync) {
+			dev_err(&dev->dev, "unable to map sync NAND\n");
+			goto err_sync;
+		}
+	}
+
+	memcpy(&gpiomtd->plat, dev->dev.platform_data, sizeof(gpiomtd->plat));
+
+	ret = gpio_request(gpiomtd->plat.gpio_nce, "NAND NCE");
+	if (ret)
+		goto err_nce;
+	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");
+		if (ret)
+			goto err_nwp;
+		gpio_direction_output(gpiomtd->plat.gpio_nwp, 1);
+	}
+	ret = gpio_request(gpiomtd->plat.gpio_ale, "NAND ALE");
+	if (ret)
+		goto err_ale;
+	gpio_direction_output(gpiomtd->plat.gpio_ale, 0);
+	ret = gpio_request(gpiomtd->plat.gpio_cle, "NAND CLE");
+	if (ret)
+		goto err_cle;
+	gpio_direction_output(gpiomtd->plat.gpio_cle, 0);
+	ret = gpio_request(gpiomtd->plat.gpio_rdy, "NAND RDY");
+	if (ret)
+		goto err_rdy;
+	gpio_direction_input(gpiomtd->plat.gpio_rdy);
+
+
+	this->IO_ADDR_W  = this->IO_ADDR_R;
+	this->ecc.mode   = NAND_ECC_SOFT;
+	this->options    = gpiomtd->plat.options;
+	this->chip_delay = gpiomtd->plat.chip_delay;
+
+	/* install our routines */
+	this->cmd_ctrl   = gpio_nand_cmd_ctrl;
+	this->dev_ready  = gpio_nand_devready;
+
+	if (this->options & NAND_BUSWIDTH_16) {
+		this->read_buf   = gpio_nand_readbuf16;
+		this->write_buf  = gpio_nand_writebuf16;
+		this->verify_buf = gpio_nand_verifybuf16;
+	} else {
+		this->read_buf   = gpio_nand_readbuf;
+		this->write_buf  = gpio_nand_writebuf;
+		this->verify_buf = gpio_nand_verifybuf;
+	}
+
+	/* set the mtd private data for the nand driver */
+	gpiomtd->mtd_info.priv = this;
+	gpiomtd->mtd_info.owner = THIS_MODULE;
+
+	if (nand_scan(&gpiomtd->mtd_info, 1)) {
+		dev_err(&dev->dev, "no nand chips found?\n");
+		ret = -ENXIO;
+		goto err_wp;
+	}
+
+	if (gpiomtd->plat.adjust_parts)
+		gpiomtd->plat.adjust_parts(&gpiomtd->plat,
+					   gpiomtd->mtd_info.size);
+
+	add_mtd_partitions(&gpiomtd->mtd_info, gpiomtd->plat.parts,
+			   gpiomtd->plat.num_parts);
+	platform_set_drvdata(dev, gpiomtd);
+
+	return 0;
+
+err_wp:
+	if (gpio_is_valid(gpiomtd->plat.gpio_nwp))
+		gpio_set_value(gpiomtd->plat.gpio_nwp, 0);
+	gpio_free(gpiomtd->plat.gpio_rdy);
+err_rdy:
+	gpio_free(gpiomtd->plat.gpio_cle);
+err_cle:
+	gpio_free(gpiomtd->plat.gpio_ale);
+err_ale:
+	if (gpio_is_valid(gpiomtd->plat.gpio_nwp))
+		gpio_free(gpiomtd->plat.gpio_nwp);
+err_nwp:
+	gpio_free(gpiomtd->plat.gpio_nce);
+err_nce:
+	iounmap(gpiomtd->io_sync);
+	if (res1)
+		release_mem_region(res1->start, res1->end - res1->start + 1);
+err_sync:
+	iounmap(gpiomtd->nand_chip.IO_ADDR_R);
+	release_mem_region(res0->start, res0->end - res0->start + 1);
+err_map:
+	kfree(gpiomtd);
+	return ret;
+}
+
+static struct platform_driver gpio_nand_driver = {
+	.probe		= gpio_nand_probe,
+	.remove		= gpio_nand_remove,
+	.driver		= {
+		.name	= "gpio-nand",
+	},
+};
+
+static int __init gpio_nand_init(void)
+{
+	printk(KERN_INFO "GPIO NAND driver, (c) 2004 Simtec Electronics\n");
+
+	return platform_driver_register(&gpio_nand_driver);
+}
+
+static void __exit gpio_nand_exit(void)
+{
+	platform_driver_unregister(&gpio_nand_driver);
+}
+
+module_init(gpio_nand_init);
+module_exit(gpio_nand_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Ben Dooks <[email protected]>");
+MODULE_DESCRIPTION("GPIO NAND Driver");

include/linux/mtd/nand-gpio.h new file +19 -0

new file mode 100644
index 0000000..51534e5
@@ -0,0 +1,19 @@
+#ifndef __LINUX_MTD_NAND_GPIO_H
+#define __LINUX_MTD_NAND_GPIO_H
+
+#include <linux/mtd/nand.h>
+
+struct gpio_nand_platdata {
+	int	gpio_nce;
+	int	gpio_nwp;
+	int	gpio_cle;
+	int	gpio_ale;
+	int	gpio_rdy;
+	void	(*adjust_parts)(struct gpio_nand_platdata *, size_t);
+	struct mtd_partition *parts;
+	unsigned int num_parts;
+	unsigned int options;
+	int	chip_delay;
+};
+
+#endif

Signature · 3 lines

-- 
Sincerely yours,
Mike.

Quoted 7 lines · Mike Rapoport

Quoted 6 lines · David Woodhouse

Please can I have it in a form I can apply, with changelog and signed-off-by, and without those silly 'u16' types in it. The C language has perfectly good types for specifying unsigned 16-bit integers; use them.

Thanks.

Quoted 4 lines

The patch adds support for NAND flashes connected to GPIOs.

Signed-off-by Ben Dooks <[email protected]>

Signed-off-by Russell King <[email protected]>

So you're adding signed-off-by's without asking the original people to provide them... That's very bad - you're making a statement about others which they've not made.

Please change mine to:

Signed-off-by Russell King <[email protected]>

Quoted 450 lines

Signed-off-by Mike Rapoport <[email protected]>

 drivers/mtd/nand/Kconfig      |    6 +
 drivers/mtd/nand/Makefile     |    1 +
 drivers/mtd/nand/gpio.c       |  375 +++++++++++++++++++++++++++++++++++++++++
 include/linux/mtd/nand-gpio.h |   19 ++
 4 files changed, 401 insertions(+), 0 deletions(-)

drivers/mtd/nand/Kconfig +6 -0

index 41f361c..e98991b 100644
@@ -56,6 +56,12 @@ config MTD_NAND_H1900
 	help
 	  This enables the driver for the iPAQ h1900 flash.
 
+config MTD_NAND_GPIO
+	tristate "GPIO NAND Flash driver"
+	depends on GENERIC_GPIO
+	help
+	  This enables a GPIO based NAND flash driver.
+
 config MTD_NAND_SPIA
 	tristate "NAND Flash device on SPIA board"
 	depends on ARCH_P720T

drivers/mtd/nand/Makefile +1 -0

index b786c5d..39eefc2 100644
@@ -24,6 +24,7 @@ obj-$(CONFIG_MTD_NAND_NANDSIM)		+= nandsim.o
 obj-$(CONFIG_MTD_NAND_CS553X)		+= cs553x_nand.o
 obj-$(CONFIG_MTD_NAND_NDFC)		+= ndfc.o
 obj-$(CONFIG_MTD_NAND_ATMEL)		+= atmel_nand.o
+obj-$(CONFIG_MTD_NAND_GPIO)		+= gpio.o
 obj-$(CONFIG_MTD_NAND_CM_X270)		+= cmx270_nand.o
 obj-$(CONFIG_MTD_NAND_BASLER_EXCITE)	+= excite_nandflash.o
 obj-$(CONFIG_MTD_NAND_PXA3xx)		+= pxa3xx_nand.o

drivers/mtd/nand/gpio.c new file +375 -0

new file mode 100644
index 0000000..083bab4
@@ -0,0 +1,375 @@
+/*
+ * drivers/mtd/nand/gpio.c
+ *
+ * Updated, and converted to generic GPIO based driver by Russell King.
+ *
+ * Written by Ben Dooks <[email protected]>
+ *   Based on 2.4 version by Mark Whittaker
+ *
+ * (c) 2004 Simtec Electronics
+ *
+ * Device driver for NAND connected via GPIO
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/gpio.h>
+#include <linux/io.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/nand.h>
+#include <linux/mtd/partitions.h>
+#include <linux/mtd/nand-gpio.h>
+
+struct gpiomtd {
+	void __iomem		*io_sync;
+	struct mtd_info		mtd_info;
+	struct nand_chip	nand_chip;
+	struct gpio_nand_platdata plat;
+};
+
+#define gpio_nand_getpriv(x) container_of(x, struct gpiomtd, mtd_info)
+
+
+#ifdef CONFIG_ARM
+/* gpio_nand_dosync()
+ *
+ * Make sure the GPIO state changes occur in-order with writes to NAND
+ * memory region.
+ * Needed on PXA due to bus-reordering within the SoC itself (see section on
+ * I/O ordering in PXA manual (section 2.3, p35)
+ */
+static void gpio_nand_dosync(struct gpiomtd *gpiomtd)
+{
+	unsigned long tmp;
+
+	if (gpiomtd->io_sync) {
+		/*
+		 * Linux memory barriers don't cater for what's required here.
+		 * What's required is what's here - a read from a separate
+		 * region with a dependency on that read.
+		 */
+		tmp = readl(gpiomtd->io_sync);
+		asm volatile("mov %1, %0\n" : "=r" (tmp) : "r" (tmp));
+	}
+}
+#else
+static inline void gpio_nand_dosync(struct gpiomtd *gpiomtd) {}
+#endif
+
+static void gpio_nand_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int ctrl)
+{
+	struct gpiomtd *gpiomtd = gpio_nand_getpriv(mtd);
+
+	gpio_nand_dosync(gpiomtd);
+
+	if (ctrl & NAND_CTRL_CHANGE) {
+		gpio_set_value(gpiomtd->plat.gpio_nce, !(ctrl & NAND_NCE));
+		gpio_set_value(gpiomtd->plat.gpio_cle, !!(ctrl & NAND_CLE));
+		gpio_set_value(gpiomtd->plat.gpio_ale, !!(ctrl & NAND_ALE));
+		gpio_nand_dosync(gpiomtd);
+	}
+	if (cmd == NAND_CMD_NONE)
+		return;
+
+	writeb(cmd, gpiomtd->nand_chip.IO_ADDR_W);
+	gpio_nand_dosync(gpiomtd);
+}
+
+static void gpio_nand_writebuf(struct mtd_info *mtd, const u_char *buf, int len)
+{
+	struct nand_chip *this = mtd->priv;
+
+	writesb(this->IO_ADDR_W, buf, len);
+}
+
+static void gpio_nand_readbuf(struct mtd_info *mtd, u_char *buf, int len)
+{
+	struct nand_chip *this = mtd->priv;
+
+	readsb(this->IO_ADDR_R, buf, len);
+}
+
+static int gpio_nand_verifybuf(struct mtd_info *mtd, const u_char *buf, int len)
+{
+	struct nand_chip *this = mtd->priv;
+	unsigned char read, *p = (unsigned char *) buf;
+	int i, err = 0;
+
+	for (i = 0; i < len; i++) {
+		read = readb(this->IO_ADDR_R);
+		if (read != p[i]) {
+			pr_debug("%s: err at %d (read %04x vs %04x)\n",
+			       __func__, i, read, p[i]);
+			err = -EFAULT;
+		}
+	}
+	return err;
+}
+
+static void gpio_nand_writebuf16(struct mtd_info *mtd, const u_char *buf,
+				 int len)
+{
+	struct nand_chip *this = mtd->priv;
+
+	if (IS_ALIGNED((unsigned long)buf, 2)) {
+		writesw(this->IO_ADDR_W, buf, len>>1);
+	} else {
+		int i;
+		unsigned short *ptr = (unsigned short *)buf;
+
+		for (i = 0; i < len; i += 2, ptr++)
+			writew(*ptr, this->IO_ADDR_W);
+	}
+}
+
+static void gpio_nand_readbuf16(struct mtd_info *mtd, u_char *buf, int len)
+{
+	struct nand_chip *this = mtd->priv;
+
+	if (IS_ALIGNED((unsigned long)buf, 2)) {
+		readsw(this->IO_ADDR_R, buf, len>>1);
+	} else {
+		int i;
+		unsigned short *ptr = (unsigned short *)buf;
+
+		for (i = 0; i < len; i += 2, ptr++)
+			*ptr = readw(this->IO_ADDR_R);
+	}
+}
+
+static int gpio_nand_verifybuf16(struct mtd_info *mtd, const u_char *buf,
+				 int len)
+{
+	struct nand_chip *this = mtd->priv;
+	unsigned short read, *p = (unsigned short *) buf;
+	int i, err = 0;
+	len >>= 1;
+
+	for (i = 0; i < len; i++) {
+		read = readw(this->IO_ADDR_R);
+		if (read != p[i]) {
+			pr_debug("%s: err at %d (read %04x vs %04x)\n",
+			       __func__, i, read, p[i]);
+			err = -EFAULT;
+		}
+	}
+	return err;
+}
+
+
+static int gpio_nand_devready(struct mtd_info *mtd)
+{
+	struct gpiomtd *gpiomtd = gpio_nand_getpriv(mtd);
+	return gpio_get_value(gpiomtd->plat.gpio_rdy);
+}
+
+static int __devexit gpio_nand_remove(struct platform_device *dev)
+{
+	struct gpiomtd *gpiomtd = platform_get_drvdata(dev);
+	struct resource *res;
+
+	nand_release(&gpiomtd->mtd_info);
+
+	res = platform_get_resource(dev, IORESOURCE_MEM, 1);
+	iounmap(gpiomtd->io_sync);
+	if (res)
+		release_mem_region(res->start, res->end - res->start + 1);
+
+	res = platform_get_resource(dev, IORESOURCE_MEM, 0);
+	iounmap(gpiomtd->nand_chip.IO_ADDR_R);
+	release_mem_region(res->start, res->end - res->start + 1);
+
+	if (gpio_is_valid(gpiomtd->plat.gpio_nwp))
+		gpio_set_value(gpiomtd->plat.gpio_nwp, 0);
+	gpio_set_value(gpiomtd->plat.gpio_nce, 1);
+
+	gpio_free(gpiomtd->plat.gpio_cle);
+	gpio_free(gpiomtd->plat.gpio_ale);
+	gpio_free(gpiomtd->plat.gpio_nce);
+	if (gpio_is_valid(gpiomtd->plat.gpio_nwp))
+		gpio_free(gpiomtd->plat.gpio_nwp);
+	gpio_free(gpiomtd->plat.gpio_rdy);
+
+	kfree(gpiomtd);
+
+	return 0;
+}
+
+static void __iomem *request_and_remap(struct resource *res, size_t size,
+					const char *name, int *err)
+{
+	void __iomem *ptr;
+
+	if (!request_mem_region(res->start, res->end - res->start + 1, name)) {
+		*err = -EBUSY;
+		return NULL;
+	}
+
+	ptr = ioremap(res->start, size);
+	if (!ptr) {
+		release_mem_region(res->start, res->end - res->start + 1);
+		*err = -ENOMEM;
+	}
+	return ptr;
+}
+
+static int __devinit gpio_nand_probe(struct platform_device *dev)
+{
+	struct gpiomtd *gpiomtd;
+	struct nand_chip *this;
+	struct resource *res0, *res1;
+	int ret;
+
+	if (!dev->dev.platform_data)
+		return -EINVAL;
+
+	res0 = platform_get_resource(dev, IORESOURCE_MEM, 0);
+	if (!res0)
+		return -EINVAL;
+
+	gpiomtd = kzalloc(sizeof(*gpiomtd), GFP_KERNEL);
+	if (gpiomtd == NULL) {
+		dev_err(&dev->dev, "failed to create NAND MTD\n");
+		return -ENOMEM;
+	}
+
+	this = &gpiomtd->nand_chip;
+	this->IO_ADDR_R = request_and_remap(res0, 2, "NAND", &ret);
+	if (!this->IO_ADDR_R) {
+		dev_err(&dev->dev, "unable to map NAND\n");
+		goto err_map;
+	}
+
+	res1 = platform_get_resource(dev, IORESOURCE_MEM, 1);
+	if (res1) {
+		gpiomtd->io_sync = request_and_remap(res1, 4, "NAND sync", &ret);
+		if (!gpiomtd->io_sync) {
+			dev_err(&dev->dev, "unable to map sync NAND\n");
+			goto err_sync;
+		}
+	}
+
+	memcpy(&gpiomtd->plat, dev->dev.platform_data, sizeof(gpiomtd->plat));
+
+	ret = gpio_request(gpiomtd->plat.gpio_nce, "NAND NCE");
+	if (ret)
+		goto err_nce;
+	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");
+		if (ret)
+			goto err_nwp;
+		gpio_direction_output(gpiomtd->plat.gpio_nwp, 1);
+	}
+	ret = gpio_request(gpiomtd->plat.gpio_ale, "NAND ALE");
+	if (ret)
+		goto err_ale;
+	gpio_direction_output(gpiomtd->plat.gpio_ale, 0);
+	ret = gpio_request(gpiomtd->plat.gpio_cle, "NAND CLE");
+	if (ret)
+		goto err_cle;
+	gpio_direction_output(gpiomtd->plat.gpio_cle, 0);
+	ret = gpio_request(gpiomtd->plat.gpio_rdy, "NAND RDY");
+	if (ret)
+		goto err_rdy;
+	gpio_direction_input(gpiomtd->plat.gpio_rdy);
+
+
+	this->IO_ADDR_W  = this->IO_ADDR_R;
+	this->ecc.mode   = NAND_ECC_SOFT;
+	this->options    = gpiomtd->plat.options;
+	this->chip_delay = gpiomtd->plat.chip_delay;
+
+	/* install our routines */
+	this->cmd_ctrl   = gpio_nand_cmd_ctrl;
+	this->dev_ready  = gpio_nand_devready;
+
+	if (this->options & NAND_BUSWIDTH_16) {
+		this->read_buf   = gpio_nand_readbuf16;
+		this->write_buf  = gpio_nand_writebuf16;
+		this->verify_buf = gpio_nand_verifybuf16;
+	} else {
+		this->read_buf   = gpio_nand_readbuf;
+		this->write_buf  = gpio_nand_writebuf;
+		this->verify_buf = gpio_nand_verifybuf;
+	}
+
+	/* set the mtd private data for the nand driver */
+	gpiomtd->mtd_info.priv = this;
+	gpiomtd->mtd_info.owner = THIS_MODULE;
+
+	if (nand_scan(&gpiomtd->mtd_info, 1)) {
+		dev_err(&dev->dev, "no nand chips found?\n");
+		ret = -ENXIO;
+		goto err_wp;
+	}
+
+	if (gpiomtd->plat.adjust_parts)
+		gpiomtd->plat.adjust_parts(&gpiomtd->plat,
+					   gpiomtd->mtd_info.size);
+
+	add_mtd_partitions(&gpiomtd->mtd_info, gpiomtd->plat.parts,
+			   gpiomtd->plat.num_parts);
+	platform_set_drvdata(dev, gpiomtd);
+
+	return 0;
+
+err_wp:
+	if (gpio_is_valid(gpiomtd->plat.gpio_nwp))
+		gpio_set_value(gpiomtd->plat.gpio_nwp, 0);
+	gpio_free(gpiomtd->plat.gpio_rdy);
+err_rdy:
+	gpio_free(gpiomtd->plat.gpio_cle);
+err_cle:
+	gpio_free(gpiomtd->plat.gpio_ale);
+err_ale:
+	if (gpio_is_valid(gpiomtd->plat.gpio_nwp))
+		gpio_free(gpiomtd->plat.gpio_nwp);
+err_nwp:
+	gpio_free(gpiomtd->plat.gpio_nce);
+err_nce:
+	iounmap(gpiomtd->io_sync);
+	if (res1)
+		release_mem_region(res1->start, res1->end - res1->start + 1);
+err_sync:
+	iounmap(gpiomtd->nand_chip.IO_ADDR_R);
+	release_mem_region(res0->start, res0->end - res0->start + 1);
+err_map:
+	kfree(gpiomtd);
+	return ret;
+}
+
+static struct platform_driver gpio_nand_driver = {
+	.probe		= gpio_nand_probe,
+	.remove		= gpio_nand_remove,
+	.driver		= {
+		.name	= "gpio-nand",
+	},
+};
+
+static int __init gpio_nand_init(void)
+{
+	printk(KERN_INFO "GPIO NAND driver, (c) 2004 Simtec Electronics\n");
+
+	return platform_driver_register(&gpio_nand_driver);
+}
+
+static void __exit gpio_nand_exit(void)
+{
+	platform_driver_unregister(&gpio_nand_driver);
+}
+
+module_init(gpio_nand_init);
+module_exit(gpio_nand_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Ben Dooks <[email protected]>");
+MODULE_DESCRIPTION("GPIO NAND Driver");

include/linux/mtd/nand-gpio.h new file +19 -0

new file mode 100644
index 0000000..51534e5
@@ -0,0 +1,19 @@
+#ifndef __LINUX_MTD_NAND_GPIO_H
+#define __LINUX_MTD_NAND_GPIO_H
+
+#include <linux/mtd/nand.h>
+
+struct gpio_nand_platdata {
+	int	gpio_nce;
+	int	gpio_nwp;
+	int	gpio_cle;
+	int	gpio_ale;
+	int	gpio_rdy;
+	void	(*adjust_parts)(struct gpio_nand_platdata *, size_t);
+	struct mtd_partition *parts;
+	unsigned int num_parts;
+	unsigned int options;
+	int	chip_delay;
+};
+
+#endif

Signature · 3 lines

-- 
Sincerely yours,
Mike.

Quoted 16 lines · Mike Frysinger

Quoted 15 lines · Mike Rapoport

drivers/mtd/nand/gpio.c new file +15 -0

+/* gpio_nand_dosync()
+ *
+ * needed due to bus-reordering within the PXA itself (see section on
+ * I/O ordering in PXA manual (section 2.3, p35)
+ */
+static void gpio_nand_dosync(struct gpiomtd *gpiomtd)
+{
+       unsigned long tmp;
+
+       if (gpiomtd->io_sync) {
+               /*
+                * this should generate the read, followed by
+                * something that depends on the read
+                */
+               tmp = readl(gpiomtd->io_sync);

Quoted 8 lines

drivers/mtd/nand/gpio.c line 57

Quoted 3 lines

drivers/mtd/nand/gpio.c new file +3 -0

+               asm volatile("mov %1, %0\n" : "=r" (tmp) : "r" (tmp));
+       }
+}

there isnt much point in attempting to write a generic solution if you're just going to turn around and stick straight assembly in the middle of it. why not use a real arch-generic method like a memory barrier.

Linux memory barriers don't cater for what's required here. What's required is what's there - a read from a separate region with a dependency on that read. Linux has no such barrier.

David,
Can you please review and ACK the below patch?

The patch adds support for NAND flashes connected to GPIOs.

 drivers/mtd/nand/Kconfig      |    6 +
 drivers/mtd/nand/Makefile     |    1 +
 drivers/mtd/nand/gpio.c       |  367 +++++++++++++++++++++++++++++++++++++++++
 include/linux/mtd/nand-gpio.h |   19 ++
 4 files changed, 393 insertions(+), 0 deletions(-)

drivers/mtd/nand/Kconfig +0 -0

index 41f361c..73e7a6e 100644

drivers/mtd/nand/Kconfig +3 -0

+	help
+	  This enables a GPIO based NAND flash driver.
+
 config MTD_NAND_SPIA
 	tristate "NAND Flash device on SPIA board"
 	depends on ARCH_P720T