[PATCH] [MTD] [NAND] GPIO NAND flash driver

[PATCH] [MTD] [NAND] GPIO NAND flash driver

[Mike Rapoport]

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(-)

diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index 41f361c..73e7a6e 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -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 ARM
+	help
+	  This enables a GPIO based NAND flash driver.
+
 config MTD_NAND_SPIA
 	tristate "NAND Flash device on SPIA board"
 	depends on ARCH_P720T
diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
index b786c5d..39eefc2 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -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
diff --git a/drivers/mtd/nand/gpio.c b/drivers/mtd/nand/gpio.c
new file mode 100644
index 0000000..e46ae17
--- /dev/null
+++ b/drivers/mtd/nand/gpio.c
@@ -0,0 +1,367 @@
+/*
+ * drivers/mtd/nand/gpio.c
+ *
+ * Updated, and converted to generic GPIO based driver by Russell King.
+ *
+ * Written by Ben Dooks <ben@simtec.co.uk>
+ *   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)
+
+/* 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);
+		asm volatile("mov %1, %0\n" : "=r" (tmp) : "r" (tmp));
+	}
+}
+
+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;
+	u8 read, *p = (u8 *) 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;
+		u16 *ptr = (u16 *)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;
+		u16 *ptr = (u16 *)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;
+	u16 read, *p = (u16 *) 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 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 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 -ENOMEM;
+}
+
+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 <ben@simtec.co.uk>");
+MODULE_DESCRIPTION("GPIO NAND Driver");
diff --git a/include/linux/mtd/nand-gpio.h b/include/linux/mtd/nand-gpio.h
new file mode 100644
index 0000000..51534e5
--- /dev/null
+++ b/include/linux/mtd/nand-gpio.h
@@ -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


-- 
Sincerely yours,
Mike.

Re: [PATCH] [MTD] [NAND] GPIO NAND flash driver

[Mike Frysinger]

On Wed, Oct 8, 2008 at 02:01, Mike Rapoport wrote:
> +/* 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);
> +               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.
-mike

Re: [PATCH] [MTD] [NAND] GPIO NAND flash driver

[Russell King - ARM Linux]

On Wed, Oct 08, 2008 at 02:20:13AM -0400, Mike Frysinger wrote:
> On Wed, Oct 8, 2008 at 02:01, Mike Rapoport wrote:
> > +/* 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);
> > +               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.

Re: [PATCH] [MTD] [NAND] GPIO NAND flash driver

[Mike Frysinger]

On Wed, Oct 8, 2008 at 03:28, Russell King - ARM Linux wrote:
> On Wed, Oct 08, 2008 at 02:20:13AM -0400, Mike Frysinger wrote:
>> On Wed, Oct 8, 2008 at 02:01, Mike Rapoport wrote:
>> > +/* 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);
>> > +               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.

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

Re: [PATCH] [MTD] [NAND] GPIO NAND flash driver

[Mike Rapoport]

Mike Frysinger wrote:
> On Wed, Oct 8, 2008 at 03:28, Russell King - ARM Linux wrote:
>> On Wed, Oct 08, 2008 at 02:20:13AM -0400, Mike Frysinger wrote:
>>> On Wed, Oct 8, 2008 at 02:01, Mike Rapoport wrote:
>>>> +/* 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);
>>>> +               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.
> 
> 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.

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

-- 
Sincerely yours,
Mike.

Re: [PATCH] [MTD] [NAND] GPIO NAND flash driver

[David Woodhouse]

On Wed, 2008-10-08 at 08:28 +0100, Russell King - ARM Linux wrote:
> On Wed, Oct 08, 2008 at 02:20:13AM -0400, Mike Frysinger wrote:
> > On Wed, Oct 8, 2008 at 02:01, Mike Rapoport wrote:
> > > +/* 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);
> > > +               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.

Perhaps it should have? It can do nothing on other architectures...

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [PATCH] [MTD] [NAND] GPIO NAND flash driver

[Paulius Zaleckas]

Mike Rapoport wrote:
> 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(-)
> 
> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> index 41f361c..73e7a6e 100644
> --- a/drivers/mtd/nand/Kconfig
> +++ b/drivers/mtd/nand/Kconfig
> @@ -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 ARM

&& 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

Re: [PATCH] [MTD] [NAND] GPIO NAND flash driver

[Mike Frysinger]

On Wed, Oct 8, 2008 at 04:28, Mike Rapoport wrote:
> Mike Frysinger wrote:
>> On Wed, Oct 8, 2008 at 03:28, Russell King - ARM Linux wrote:
>>> On Wed, Oct 08, 2008 at 02:20:13AM -0400, Mike Frysinger wrote:
>>>> On Wed, Oct 8, 2008 at 02:01, Mike Rapoport wrote:
>>>>> +/* 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);
>>>>> +               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.
>>
>> 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

Re: [PATCH] [MTD] [NAND] GPIO NAND flash driver

[Mike Rapoport]

Mike Frysinger wrote:
> On Wed, Oct 8, 2008 at 04:28, Mike Rapoport wrote:
>> Mike Frysinger wrote:
>>> On Wed, Oct 8, 2008 at 03:28, Russell King - ARM Linux wrote:
>>>> On Wed, Oct 08, 2008 at 02:20:13AM -0400, Mike Frysinger wrote:
>>>>> On Wed, Oct 8, 2008 at 02:01, Mike Rapoport wrote:
>>>>>> +/* 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);
>>>>>> +               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.
>>> 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

Would that be any better:

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

diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index 41f361c..e98991b 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -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
diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
index b786c5d..39eefc2 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -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
diff --git a/drivers/mtd/nand/gpio.c b/drivers/mtd/nand/gpio.c
new file mode 100644
index 0000000..0c90779
--- /dev/null
+++ b/drivers/mtd/nand/gpio.c
@@ -0,0 +1,373 @@
+/*
+ * drivers/mtd/nand/gpio.c
+ *
+ * Updated, and converted to generic GPIO based driver by Russell King.
+ *
+ * Written by Ben Dooks <ben@simtec.co.uk>
+ *   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()
+ *
+ * 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) {
+		/*
+		 * 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;
+	u8 read, *p = (u8 *) 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;
+		u16 *ptr = (u16 *)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;
+		u16 *ptr = (u16 *)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;
+	u16 read, *p = (u16 *) 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 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 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 -ENOMEM;
+}
+
+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 <ben@simtec.co.uk>");
+MODULE_DESCRIPTION("GPIO NAND Driver");
diff --git a/include/linux/mtd/nand-gpio.h b/include/linux/mtd/nand-gpio.h
new file mode 100644
index 0000000..51534e5
--- /dev/null
+++ b/include/linux/mtd/nand-gpio.h
@@ -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


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

-- 
Sincerely yours,
Mike.

Re: [PATCH] [MTD] [NAND] GPIO NAND flash driver

[Jamie Lokier]

Mike Rapoport wrote:
> +#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"?

> +		/*
> +		 * 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".

> +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 can't tell from the comments.

Thanks,
-- Jamie

Re: [PATCH] [MTD] [NAND] GPIO NAND flash driver

[Russell King - ARM Linux]

On Fri, Oct 10, 2008 at 03:19:16PM +0100, Jamie Lokier wrote:
> Mike Rapoport wrote:
> > +#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"?

It's good enough.

> > +		/*
> > +		 * 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".

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.

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

Re: [PATCH] [MTD] [NAND] GPIO NAND flash driver

[Mike Frysinger]

On Fri, Oct 10, 2008 at 17:48, Russell King - ARM Linux wrote:
> On Fri, Oct 10, 2008 at 03:19:16PM +0100, Jamie Lokier wrote:
>> Mike Rapoport wrote:
>> > +           /*
>> > +            * 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".
>
> 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"

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

the Blackfin code should do: SSYNC();
-mike

Re: [PATCH] [MTD] [NAND] GPIO NAND flash driver

[Mike Rapoport]

Mike Frysinger wrote:
>> 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.

 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(-)

diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index 41f361c..e98991b 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -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
diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
index b786c5d..39eefc2 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -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
diff --git a/drivers/mtd/nand/gpio.c b/drivers/mtd/nand/gpio.c
new file mode 100644
index 0000000..00969e8
--- /dev/null
+++ b/drivers/mtd/nand/gpio.c
@@ -0,0 +1,375 @@
+/*
+ * drivers/mtd/nand/gpio.c
+ *
+ * Updated, and converted to generic GPIO based driver by Russell King.
+ *
+ * Written by Ben Dooks <ben@simtec.co.uk>
+ *   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;
+	u8 read, *p = (u8 *) 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;
+		u16 *ptr = (u16 *)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;
+		u16 *ptr = (u16 *)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;
+	u16 read, *p = (u16 *) 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 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 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 -ENOMEM;
+}
+
+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 <ben@simtec.co.uk>");
+MODULE_DESCRIPTION("GPIO NAND Driver");
diff --git a/include/linux/mtd/nand-gpio.h b/include/linux/mtd/nand-gpio.h
new file mode 100644
index 0000000..51534e5
--- /dev/null
+++ b/include/linux/mtd/nand-gpio.h
@@ -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


-- 
Sincerely yours,
Mike.

Re: [PATCH] [MTD] [NAND] GPIO NAND flash driver

[Mike Frysinger]

On Sun, Oct 12, 2008 at 04:02, Mike Rapoport wrote:
> Mike Frysinger wrote:
>>> 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.

looks like it, thanks for that

> +static int gpio_nand_remove(struct platform_device *dev)

should have __devexit markings

> +       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 ?

> +static int gpio_nand_probe(struct platform_device *dev)

should have __devinit markings

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

Re: [PATCH] [MTD] [NAND] GPIO NAND flash driver

[Russell King - ARM Linux]

On Sun, Oct 12, 2008 at 04:14:43AM -0400, Mike Frysinger wrote:
> On Sun, Oct 12, 2008 at 04:02, Mike Rapoport wrote:
> > +       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 ?

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.

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

Probably a left over from the early days of the driver (it's 4 years old
and has been through multiple revisions to eventually turn it into what
you see today.)  Yes, it should be fixed.

Re: [PATCH] [MTD] [NAND] GPIO NAND flash driver

[Mike Frysinger]

On Sun, Oct 12, 2008 at 04:28, Russell King - ARM Linux wrote:
> On Sun, Oct 12, 2008 at 04:14:43AM -0400, Mike Frysinger wrote:
>> On Sun, Oct 12, 2008 at 04:02, Mike Rapoport wrote:
>> > +       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 ?
>
> 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?

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.

> 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.
-mike

Re: [PATCH] [MTD] [NAND] GPIO NAND flash driver

[Mike Rapoport]

Mike Frysinger wrote:

>> The patch with (hopefully) clearer gpio_nand_dosync() comments.
> 
> looks like it, thanks for that
> 
>> +static int gpio_nand_remove(struct platform_device *dev)
> 
> should have __devexit markings

Done.

>> +       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 ?

I don't quite understand the issue with setting gpio value before releasing.
On PXA (and probably others) it will add safety measure. On the systems that
change pin state after it was freed setting gpio value will eat several CPU
cycles on driver unload but it definitely will not do any damage.

>> +static int gpio_nand_probe(struct platform_device *dev)
> 
> should have __devinit markings

Done.

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

Fixed.

> -mike
> 

 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(-)

diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index 41f361c..e98991b 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -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
diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
index b786c5d..39eefc2 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -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
diff --git a/drivers/mtd/nand/gpio.c b/drivers/mtd/nand/gpio.c
new file mode 100644
index 0000000..2883b6c
--- /dev/null
+++ b/drivers/mtd/nand/gpio.c
@@ -0,0 +1,375 @@
+/*
+ * drivers/mtd/nand/gpio.c
+ *
+ * Updated, and converted to generic GPIO based driver by Russell King.
+ *
+ * Written by Ben Dooks <ben@simtec.co.uk>
+ *   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;
+	u8 read, *p = (u8 *) 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;
+		u16 *ptr = (u16 *)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;
+		u16 *ptr = (u16 *)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;
+	u16 read, *p = (u16 *) 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 <ben@simtec.co.uk>");
+MODULE_DESCRIPTION("GPIO NAND Driver");
diff --git a/include/linux/mtd/nand-gpio.h b/include/linux/mtd/nand-gpio.h
new file mode 100644
index 0000000..51534e5
--- /dev/null
+++ b/include/linux/mtd/nand-gpio.h
@@ -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

-- 
Sincerely yours,
Mike.

Re: [PATCH] [MTD] [NAND] GPIO NAND flash driver

[Russell King - ARM Linux]

On Sun, Oct 12, 2008 at 04:56:37AM -0400, Mike Frysinger wrote:
> On Sun, Oct 12, 2008 at 04:28, Russell King - ARM Linux wrote:
> > 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?
> 
> 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.

> 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 amplifier stays powered up afterwards?  Yes,
it'll kill your battery life.

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.

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

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

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.

Re: [PATCH] [MTD] [NAND] GPIO NAND flash driver

[David Woodhouse]

On Sun, 2008-10-12 at 11:13 +0100, Russell King - ARM Linux wrote:
> 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.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [PATCH] [MTD] [NAND] GPIO NAND flash driver

[Russell King - ARM Linux]

On Sun, Oct 12, 2008 at 11:35:30AM +0100, David Woodhouse wrote:
> On Sun, 2008-10-12 at 11:13 +0100, Russell King - ARM Linux wrote:
> > 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.

As I've pointed out, it's hardware dependent.

If your hardware has "sleep modes" for GPIO which are preserved until
you explicitly release the GPIO hardware from sleep state - giving you
a chance to restore the GPIO registers on resume without the external
hardware seeing glitches - then you set the sleep mode state so the
GPIO for the amplifier is set as an output, and driven to the "powered
down" level.

On hardware which doesn't have that facility, then yes you do want
pull-ups and pull-downs.  But just because your hardware doesn't have
sensible GPIO hardware, that's no reason to outlaw setting GPIOs to
their inactive states while unloading drivers.

Re: [PATCH] [MTD] [NAND] GPIO NAND flash driver

[Jamie Lokier]

Russell King - ARM Linux wrote:
> So, while you see "lack of pull ups" as crappy design, others will see
> it as a power and cost saving measure.

That's a good point that probably only low-power designers are familiar
with.

If you have a GPIO feeding a CMOS input, and if you're counting the
microamps, be wary of them.

CMOS inputs use very little power indeed, and a pull up/down resistor
will use relatively much more.

On the other hand, pull ups/downs are often to disable a component if
software to drive the GPIO is not loaded, and that can save power too.
Sometimes they are needed.

It's good if the CPU boot software can set up GPIO defaults, and the
CPU (or other GPIO driver chip) maintains them when sleeping, and the
reset sequence can take the CPU out of hardware reset before all other
components, to prevent floating inputs to those components.

-- Jamie

Re: [PATCH] [MTD] [NAND] GPIO NAND flash driver

[Mike Frysinger]

On Sun, Oct 12, 2008 at 06:13, Russell King - ARM Linux wrote:
> On Sun, Oct 12, 2008 at 04:56:37AM -0400, Mike Frysinger wrote:
>> On Sun, Oct 12, 2008 at 04:28, Russell King - ARM Linux wrote:
>> > 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?
>>
>> 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.
>
>> 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.

that depends on the customer and the quality and the product.  i know
some customers that would never settle for audible clicks & pops while
others who only care about putting out something cheap.

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

random failures in the field ?  i'm sure customers of your product
will simply love that.  while it largely depends on the product, some
people wont be willing to take that chance.

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

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

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

the Blackfin maintains state through power down as well

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

the GPIO layer is a lot better than dealing with people who never
think "my driver might actually be used in a system other than mine".
or plenty of broken drivers out there stomping on pins that they have
no business touching.  the GPIO layer has enforced a ton of
reliability that dealing with straight registers never could have.
plus there's the portability across boards and architectures that is
simply unattainable with banging on registers.  but maybe some of us
care more about the rest of the ecosystem than our own specific
processor.

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

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

then collect limitations so that at some point they be addressed.  we
have a Documentation/gpio.txt file which can easily be updated to have
a "Limitations" section.  being bitter and alone only makes you more
bitter rather than other people thinking about the issues that
probably never occurred to them.

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

i was being a realist here.  random NAND failures are not acceptable.
-mike

Re: [PATCH] [MTD] [NAND] GPIO NAND flash driver

[Russell King - ARM Linux]

On Sun, Oct 12, 2008 at 03:04:06PM -0400, Mike Frysinger wrote:
> On Sun, Oct 12, 2008 at 06:13, Russell King - ARM Linux wrote:
> > 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.

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

Re: [PATCH] [MTD] [NAND] GPIO NAND flash driver

[Mike Frysinger]

On Sun, Oct 12, 2008 at 15:09, Russell King - ARM Linux wrote:
> On Sun, Oct 12, 2008 at 03:04:06PM -0400, Mike Frysinger wrote:
>> On Sun, Oct 12, 2008 at 06:13, Russell King - ARM Linux wrote:
>> > 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.
>
>> 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.

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

your original comments:
> 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.

my question is simply:
> 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 ?  you cant say "your
implementation is broken" if you dont say *what* the expected behavior
is and the behavior you claim isnt documented.  what
$random-implementation does is irrelevant.  the agreed upon API is the
only thing that matters.
-mike

Re: [PATCH] [MTD] [NAND] GPIO NAND flash driver

[Russell King - ARM Linux]

On Sun, Oct 12, 2008 at 03:22:45PM -0400, Mike Frysinger wrote:
> On Sun, Oct 12, 2008 at 15:09, Russell King - ARM Linux wrote:
> > On Sun, Oct 12, 2008 at 03:04:06PM -0400, Mike Frysinger wrote:
> >> On Sun, Oct 12, 2008 at 06:13, Russell King - ARM Linux wrote:
> >> > 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.
> >
> >> 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.
> 
> i'm attempting to get things fixed for everyone.  stop being so
> abrasive over nothing.

Me being abrasive?  ROTFL.  You're the one being difficult and constantly
twisting what I'm saying.

> my question is simply:
> > 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 ?

How the hell do I know?  Ask the GPIO library authors what *their*
intention of their interface is!

Look, so you can be clear for my point:

1. GPIO hardware state may be unaffected by gpio_free() - since it's
   undefined whether gpio_free() has any side effects.
2. Real implementations today exist where gpio_free() does not affect
   hardware state.
3. NAND may be connected to GPIOs for the control signals.
4. For hardware where gpio_free() does not affect hardware state, it
   makes total sense to ensure that GPIOs are set to inactive states
   prior to free.

That is the basis of my point.  Not "don't need pull ups".  And not
anything else that you're busy trying to twist my damned words into.

I don't care whether pull ups exist - because that's completely
irrelevent in the case where hardware state is unaffected by gpio_free.
You're the one bringing the issue of pull ups into this discussion,
not me.  You're the one clouding the issue.

Let me repeat.

1. gpio_free() is undefined wrt hardware side effects.
2. Some implementations may preserve the existing state, others may not.
3. To cater for all, setting the hardware state to inactive is _clearly_
   the right thing to do.

So, stop twisting my bloody words into your own stupid arguments.

I'm done with this thread.

Re: [PATCH] [MTD] [NAND] GPIO NAND flash driver

[David Woodhouse]

On Sun, 2008-10-12 at 10:02 +0200, Mike Rapoport wrote:
> Mike Frysinger wrote:
> >> 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.

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.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [PATCH] [MTD] [NAND] GPIO NAND flash driver

[Mike Rapoport]

David Woodhouse wrote:
> 
> 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.

The patch adds support for NAND flashes connected to GPIOs.

Signed-off-by: Ben Dooks <ben-linux-arm@fluff.org>
Signed-off-by: Russell King <linux@arm.linux.org.uk>
Signed-off-by: Mike Rapoport <mike@compulab.co.il>

 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(-)

diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index 41f361c..e98991b 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -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
diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
index b786c5d..39eefc2 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -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
diff --git a/drivers/mtd/nand/gpio.c b/drivers/mtd/nand/gpio.c
new file mode 100644
index 0000000..083bab4
--- /dev/null
+++ b/drivers/mtd/nand/gpio.c
@@ -0,0 +1,375 @@
+/*
+ * drivers/mtd/nand/gpio.c
+ *
+ * Updated, and converted to generic GPIO based driver by Russell King.
+ *
+ * Written by Ben Dooks <ben@simtec.co.uk>
+ *   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 <ben@simtec.co.uk>");
+MODULE_DESCRIPTION("GPIO NAND Driver");
diff --git a/include/linux/mtd/nand-gpio.h b/include/linux/mtd/nand-gpio.h
new file mode 100644
index 0000000..51534e5
--- /dev/null
+++ b/include/linux/mtd/nand-gpio.h
@@ -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



-- 
Sincerely yours,
Mike.

Re: [PATCH] [MTD] [NAND] GPIO NAND flash driver

[Russell King - ARM Linux]

On Wed, Oct 15, 2008 at 08:38:49AM +0200, Mike Rapoport wrote:
> David Woodhouse wrote:
> > 
> > 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.
> 
> The patch adds support for NAND flashes connected to GPIOs.
> 
> Signed-off-by: Ben Dooks <ben-linux-arm@fluff.org>
> Signed-off-by: Russell King <linux@arm.linux.org.uk>

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 <rmk+kernel@arm.linux.org.uk>

> Signed-off-by: Mike Rapoport <mike@compulab.co.il>
> 
>  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(-)
> 
> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> index 41f361c..e98991b 100644
> --- a/drivers/mtd/nand/Kconfig
> +++ b/drivers/mtd/nand/Kconfig
> @@ -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
> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
> index b786c5d..39eefc2 100644
> --- a/drivers/mtd/nand/Makefile
> +++ b/drivers/mtd/nand/Makefile
> @@ -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
> diff --git a/drivers/mtd/nand/gpio.c b/drivers/mtd/nand/gpio.c
> new file mode 100644
> index 0000000..083bab4
> --- /dev/null
> +++ b/drivers/mtd/nand/gpio.c
> @@ -0,0 +1,375 @@
> +/*
> + * drivers/mtd/nand/gpio.c
> + *
> + * Updated, and converted to generic GPIO based driver by Russell King.
> + *
> + * Written by Ben Dooks <ben@simtec.co.uk>
> + *   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 <ben@simtec.co.uk>");
> +MODULE_DESCRIPTION("GPIO NAND Driver");
> diff --git a/include/linux/mtd/nand-gpio.h b/include/linux/mtd/nand-gpio.h
> new file mode 100644
> index 0000000..51534e5
> --- /dev/null
> +++ b/include/linux/mtd/nand-gpio.h
> @@ -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
> 
> 
> 
> -- 
> Sincerely yours,
> Mike.
> 

Re: [PATCH] [MTD] [NAND] GPIO NAND flash driver

[Mike Rapoport]

Russell King - ARM Linux wrote:
> On Wed, Oct 15, 2008 at 08:38:49AM +0200, Mike Rapoport wrote:
>> David Woodhouse wrote:
>>> 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.
>> The patch adds support for NAND flashes connected to GPIOs.
>>
>> Signed-off-by: Ben Dooks <ben-linux-arm@fluff.org>
>> Signed-off-by: Russell King <linux@arm.linux.org.uk>
> 
> 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.

I apologize for that, it seems that I misunderstood Documentation/SubmittingPatches.

Ben, can I add your "signed-off-by"?

> Please change mine to:
> 
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> 
>> Signed-off-by: Mike Rapoport <mike@compulab.co.il>
>>
>>  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(-)
>>
>> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
>> index 41f361c..e98991b 100644
>> --- a/drivers/mtd/nand/Kconfig
>> +++ b/drivers/mtd/nand/Kconfig
>> @@ -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
>> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
>> index b786c5d..39eefc2 100644
>> --- a/drivers/mtd/nand/Makefile
>> +++ b/drivers/mtd/nand/Makefile
>> @@ -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
>> diff --git a/drivers/mtd/nand/gpio.c b/drivers/mtd/nand/gpio.c
>> new file mode 100644
>> index 0000000..083bab4
>> --- /dev/null
>> +++ b/drivers/mtd/nand/gpio.c
>> @@ -0,0 +1,375 @@
>> +/*
>> + * drivers/mtd/nand/gpio.c
>> + *
>> + * Updated, and converted to generic GPIO based driver by Russell King.
>> + *
>> + * Written by Ben Dooks <ben@simtec.co.uk>
>> + *   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 <ben@simtec.co.uk>");
>> +MODULE_DESCRIPTION("GPIO NAND Driver");
>> diff --git a/include/linux/mtd/nand-gpio.h b/include/linux/mtd/nand-gpio.h
>> new file mode 100644
>> index 0000000..51534e5
>> --- /dev/null
>> +++ b/include/linux/mtd/nand-gpio.h
>> @@ -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
>>
>>
>>
>> -- 
>> Sincerely yours,
>> Mike.
>>
> 

-- 
Sincerely yours,
Mike.

Re: [PATCH] [MTD] [NAND] GPIO NAND flash driver

[David Brownell]

> +#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)
> + */
> +static void gpio_nand_dosync(struct gpiomtd *gpiomtd)

I'm trying to understand why a PXA-specific issue is getting
applied to all ARMs.  #ifdef CONFIG_ARCH_PXA would be better
for *any* PXA-specific behavior like this.


Reading the list archives, I also see some confusion about
GPIO state after gpio_free() is called.  As soon as a GPIO
is no longer managed (as a GPIO) by a driver, it's fair for
the rest of the system to reconfigure it ... but platform
specific issues like "Will *this* system reconfigure it?
If so, how? and When?" are out-of-scope.  (Just like all
aspects of configuring pins for GPIO v. non-GPIO usage...)

The principle-of-least-surprise applies here, as it should
apply everywhere, though ... so until something takes over
the pin(s) used for that GPIO and reconfigures it (them) for
another purpose, it's reasonable to expect no state changes.
And it's likewise reasonable to expect that any kernel code
taking it over isn't fundamentally brain-dead ...

One general rule being that if you care what the hardware
is doing, you should have its driver managing that:  don't
unload the darn driver, if unloading it may break things!


That's not quite the same as the confusion about state at
low power levels.  In that case, drivers are within their
rights to claim that if platform code changes GPIO state
except according to driver requests, the platform is buggy.

(That includes the special configuration requests that can
be made on some platforms, covering low power modes.  I've
seen that on a couple systems, but it's currently rare and
thus very appropriate to treat as platform-specific.)

- Dave

p.s. In current kernel GIT there are two new hooks in
     the gpiolib framework, called on gpio_request()
     and gpio_free().  The intent is to support power
     management ... e.g. save 50uA by keeping unused
     GPIO banks unclocked or even powered off when not
     in use.  Such hooks are unfortunately open to some
     abuse ... I suspect platform-specific stupidities
     will start to appear.  Not much I can do about it.