All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] CM-x2xx NAND flash support
@ 2006-07-06 12:48 Mike Rapoport
  2006-07-09  9:26 ` Thomas Gleixner
  0 siblings, 1 reply; 8+ messages in thread
From: Mike Rapoport @ 2006-07-06 12:48 UTC (permalink / raw)
  To: linux-mtd

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

This patch provides MTD support for NAND flash devices on CM-x2xx modules.

Signed-off-by: Mike Rapoport <mike@compulab.co.il>

-- 
Sincerely yours,
Mike Rapoport


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: cm_x2xx_mtd.patch --]
[-- Type: text/x-diff; name="cm_x2xx_mtd.patch", Size: 14138 bytes --]

 drivers/mtd/nand/Kconfig       |    8 +
 drivers/mtd/nand/Makefile      |    2 
 drivers/mtd/nand/cmx255-nand.c |  247 ++++++++++++++++++++++++++++++++++++
 drivers/mtd/nand/cmx270-nand.c |  273 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 530 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index 3db77ee..f0f264e 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -232,6 +232,14 @@ config MTD_NAND_CS553X
 
 	  If you say "m", the module will be called "cs553x_nand.ko".
 
+config MTD_NAND_CM_X255
+	tristate "Support for NAND Flash on CM-X255 modules"
+	depends on MTD_NAND && CM_X255
+
+config MTD_NAND_CM_X270
+	tristate "Support for NAND Flash on CM-X270 modules"
+	depends on MTD_NAND && CM_X270
+
 config MTD_NAND_NANDSIM
 	tristate "Support for NAND Flash Simulator"
 	depends on MTD_NAND && MTD_PARTITIONS
diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
index f747593..c88e9ca 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -22,5 +22,7 @@ obj-$(CONFIG_MTD_NAND_TS7250)		+= ts7250
 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_CM_X255)		+= cmx255-nand.o
+obj-$(CONFIG_MTD_NAND_CM_X270)		+= cmx270-nand.o
 
 nand-objs = nand_base.o nand_bbt.o
diff --git a/drivers/mtd/nand/cmx255-nand.c b/drivers/mtd/nand/cmx255-nand.c
new file mode 100644
index 0000000..e556531
--- /dev/null
+++ b/drivers/mtd/nand/cmx255-nand.c
@@ -0,0 +1,247 @@
+/*
+ *  linux/drivers/mtd/nand/cmx255-nand.c
+ *
+ *  Copyright (C) 2006 Compulab, Ltd. (mike@compulab.co.il)
+ *
+ *  Derived from drivers/mtd/nand/h1910.c
+ *       Copyright (C) 2002 Marius Gröger (mag@sysgo.de)
+ *       Copyright (c) 2001 Thomas Gleixner (gleixner@autronix.de)
+ *
+ *
+ * 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.
+ *
+ *  Overview:
+ *   This is a device driver for the NAND flash device found on the
+ *   CM-X255 board.
+ */
+
+#include <linux/slab.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/nand.h>
+#include <linux/mtd/partitions.h>
+
+#include <asm/io.h>
+#include <asm/irq.h>
+#include <asm/arch/pxa-regs.h>
+
+/* NAND control pins */
+#define GPIO_NAND_CLE	3
+#define GPIO_NAND_ALE	4
+#define GPIO_NAND_CS	5
+#define GPIO_NAND_RB	10
+/*
+ * MTD structure for CM-X255 board
+ */
+static struct mtd_info *cmx255_nand_mtd = NULL;
+
+/*
+ * Module stuff
+ */
+
+#ifdef CONFIG_MTD_PARTITIONS
+/*
+ * Define static partitions for flash device
+ */
+static struct mtd_partition partition_info[] = {
+	{
+		.name	= "cmx255-0",
+		.offset	= 0,
+		.size	= MTDPART_SIZ_FULL
+	}
+};
+#define NUM_PARTITIONS (ARRAY_SIZE(partition_info))
+#endif
+
+#define DRAIN_WB() \
+	do { \
+		unsigned char dummy; \
+		asm volatile ("mcr p15, 0, r0, c7, c10, 4":::"r0"); \
+		dummy=*((unsigned char*)UNCACHED_ADDR); \
+	} while(0);
+
+/*  NAND device signal level interface  */
+static inline void nand_cs_on(void)
+{
+	GPCR(GPIO_NAND_CS) = GPIO_bit(GPIO_NAND_CS);
+}
+
+static void nand_cs_off(void)
+{
+	DRAIN_WB();
+
+	GPSR(GPIO_NAND_CS) = GPIO_bit(GPIO_NAND_CS);
+}
+
+static inline void nand_ale_on(void)
+{
+	GPSR(GPIO_NAND_ALE) = GPIO_bit(GPIO_NAND_ALE);
+}
+
+static inline void nand_ale_off(void)
+{
+	GPCR(GPIO_NAND_ALE) = GPIO_bit(GPIO_NAND_ALE);
+}
+
+static inline void nand_cle_on(void)
+{
+	GPSR(GPIO_NAND_CLE) = GPIO_bit(GPIO_NAND_CLE);
+}
+
+static inline void nand_cle_off(void)
+{
+	GPCR(GPIO_NAND_CLE) = GPIO_bit(GPIO_NAND_CLE);
+}
+
+/*
+ *	hardware specific access to control-lines
+ */
+static void cmx255_hwcontrol(struct mtd_info *mtd, int dat,
+			     unsigned int ctrl)
+{
+	struct nand_chip* this = (struct nand_chip *) (mtd->priv);
+	unsigned int nandaddr = (unsigned int)this->IO_ADDR_W;
+
+	DRAIN_WB();
+
+	if ( ctrl & NAND_CTRL_CHANGE ) {
+		if ( ctrl & NAND_NCE ) /* CE should be changed */
+			nand_cs_on();
+		else
+			nand_cs_off();
+		if ( ctrl & NAND_ALE ) /* ALE should be changed */
+			nand_ale_on();
+		else
+			nand_ale_off();
+		if ( ctrl & NAND_CLE ) /* CLE should be changed */
+			nand_cle_on();
+		else 
+			nand_cle_off();
+	}
+	this->IO_ADDR_W = (void*)nandaddr;
+
+	DRAIN_WB();
+
+	if ( dat != NAND_CMD_NONE )
+		writel(dat, this->IO_ADDR_W);
+}
+
+/*
+ *	read device ready pin
+ */
+static int cmx255_device_ready(struct mtd_info *mtd)
+{
+	DRAIN_WB();
+	return GPLR(GPIO_NAND_RB) & GPIO_bit(GPIO_NAND_RB);
+}
+
+/*
+ * Main initialization routine
+ */
+static int __init cmx255_init (void)
+{
+	struct nand_chip *this;
+	const char *part_type = 0;
+	int mtd_parts_nb = 0;
+	struct mtd_partition *mtd_parts = 0;
+	static unsigned int nandaddr = 0;
+	int retval = 0;
+
+	/* Allocate memory for MTD device structure and private data */
+	cmx255_nand_mtd = kmalloc(sizeof(struct mtd_info) +
+				    sizeof(struct nand_chip),
+				    GFP_KERNEL);
+	if (!cmx255_nand_mtd) {
+		printk("Unable to allocate CM-X255 MTD device structure.\n");
+		retval = -ENOMEM;
+		goto out;
+	}
+
+	nandaddr = (unsigned int)ioremap(PXA_CS1_PHYS, 0x100);
+	if ( !nandaddr ) {
+		printk("Unable to ioremap NAND device");
+		retval = -EINVAL;
+		goto err1;
+	}
+
+	/* Get pointer to private data */
+	this = (struct nand_chip *) (&cmx255_nand_mtd[1]);
+
+	/* Initialize structures */
+	memset((char *) cmx255_nand_mtd, 0, sizeof(struct mtd_info));
+	memset((char *) this, 0, sizeof(struct nand_chip));
+
+	/* Link the private data with the MTD structure */
+	cmx255_nand_mtd->owner = THIS_MODULE;
+	cmx255_nand_mtd->priv = this;
+
+	/* insert callbacks */
+	this->IO_ADDR_R = (void __iomem *)nandaddr;
+	this->IO_ADDR_W = (void __iomem *)nandaddr;
+	this->cmd_ctrl = cmx255_hwcontrol;
+	this->dev_ready = cmx255_device_ready;
+
+	/* 20 us command delay time */
+	this->chip_delay = 20;
+	this->ecc.mode = NAND_ECC_SOFT;
+
+	/* Scan to find existence of the device */
+	if (nand_scan (cmx255_nand_mtd, 1)) {
+		printk(KERN_NOTICE "No NAND device\n");
+		retval = -ENXIO;
+		goto err2;
+	}
+
+#ifdef CONFIG_MTD_CMDLINE_PARTS
+	mtd_parts_nb = parse_cmdline_partitions(cmx255_nand_mtd, &mtd_parts,
+						"cmx255");
+	if (mtd_parts_nb > 0)
+		part_type = "command line";
+	else
+		mtd_parts_nb = 0;
+#endif
+	if (mtd_parts_nb == 0)
+	{
+		mtd_parts = partition_info;
+		mtd_parts_nb = NUM_PARTITIONS;
+		part_type = "static";
+	}
+
+	/* Register the partitions */
+	printk(KERN_NOTICE "Using %s partition definition\n", part_type);
+	add_mtd_partitions(cmx255_nand_mtd, mtd_parts, mtd_parts_nb);
+
+  err2:
+	iounmap((void*)nandaddr);
+  err1:
+	kfree(cmx255_nand_mtd);
+  out:
+	/* Return happy */
+	return retval;
+}
+module_init(cmx255_init);
+
+/*
+ * Clean up routine
+ */
+static void __exit cmx255_cleanup (void)
+{
+	struct nand_chip *this;
+
+	this = (struct nand_chip *) (&cmx255_nand_mtd[1]);
+	iounmap(this->IO_ADDR_R);
+
+	/* Release resources, unregister device */
+	nand_release (cmx255_nand_mtd);
+
+	/* Free the MTD device structure */
+	kfree (cmx255_nand_mtd);
+}
+module_exit(cmx255_cleanup);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Mike Rapoport <mike at compulab dot co dot il>");
+MODULE_DESCRIPTION("NAND flash driver for Compulab CM-X255 Module");
diff --git a/drivers/mtd/nand/cmx270-nand.c b/drivers/mtd/nand/cmx270-nand.c
new file mode 100644
index 0000000..ffedaec
--- /dev/null
+++ b/drivers/mtd/nand/cmx270-nand.c
@@ -0,0 +1,273 @@
+/*
+ *  linux/drivers/mtd/nand/cmx270-nand.c
+ *
+ *  Copyright (C) 2006 Compulab, Ltd.
+ *  Mike Rapoport <mike@compulab.co.il>
+ *
+ *  Derived from drivers/mtd/nand/h1910.c
+ *       Copyright (C) 2002 Marius Gröger (mag@sysgo.de)
+ *       Copyright (c) 2001 Thomas Gleixner (gleixner@autronix.de)
+ *
+ *
+ * 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.
+ *
+ *  Overview:
+ *   This is a device driver for the NAND flash device found on the
+ *   CM-X270 board.
+ */
+
+#include <linux/slab.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/nand.h>
+#include <linux/mtd/partitions.h>
+
+#include <asm/io.h>
+#include <asm/irq.h>
+
+#include <asm/arch/hardware.h>
+#include <asm/arch/pxa-regs.h>
+
+#define GPIO_NAND_CS	(11)
+#define GPIO_NAND_RB	(89)
+
+#define DRAIN_WB() \
+	do { \
+		unsigned char dummy; \
+		asm volatile ("mcr p15, 0, r0, c7, c10, 4":::"r0"); \
+		dummy=*((unsigned char*)UNCACHED_ADDR); \
+	} while(0);
+
+/*
+ * MTD structure for CM-X270 board
+ */
+static struct mtd_info *cmx270_nand_mtd = NULL;
+
+/*
+ * Module stuff
+ */
+
+#ifdef CONFIG_MTD_PARTITIONS
+/*
+ * Define static partitions for flash device
+ */
+static struct mtd_partition partition_info[] = {
+	{
+		.name	= "cmx270-0",
+		.offset	= 0,
+		.size	= MTDPART_SIZ_FULL
+	}
+};
+#define NUM_PARTITIONS (ARRAY_SIZE(partition_info))
+
+#endif
+
+
+static 	u_char cmx270_read_byte(struct mtd_info *mtd)
+{
+	struct nand_chip *this = mtd->priv;
+	return (readl(this->IO_ADDR_R) >> 16);
+}
+
+static void cmx270_write_byte(struct mtd_info *mtd, u_char byte)
+{
+	struct nand_chip *this = mtd->priv;
+	writel((byte << 16), this->IO_ADDR_W);
+}
+
+static void cmx270_write_buf(struct mtd_info *mtd, const u_char *buf, int len)
+{
+	int i;
+	struct nand_chip *this = mtd->priv;
+
+	for (i=0; i<len; i++) {
+		writel((*buf++ << 16), this->IO_ADDR_W);
+	}
+}
+
+static void cmx270_read_buf(struct mtd_info *mtd, u_char *buf, int len)
+{
+	int i;
+	struct nand_chip *this = mtd->priv;
+
+	for (i=0; i<len; i++) {
+		*buf++ = readl(this->IO_ADDR_R) >> 16;
+	}
+}
+
+static int cmx270_verify_buf(struct mtd_info *mtd, const u_char *buf, int len)
+{
+	int i;
+	struct nand_chip *this = mtd->priv;
+
+	for (i=0; i<len; i++) {
+		if ( buf[i] != (u_char)(readl(this->IO_ADDR_R) >> 16) )
+			return -EFAULT;
+	}
+
+	return 0;
+}
+
+static inline void nand_cs_on(void)
+{
+	GPCR(GPIO_NAND_CS) = GPIO_bit(GPIO_NAND_CS);
+}
+
+static void nand_cs_off(void)
+{
+	DRAIN_WB();
+
+	GPSR(GPIO_NAND_CS) = GPIO_bit(GPIO_NAND_CS);
+}
+
+/*
+ *	hardware specific access to control-lines
+ */
+static void cmx270_hwcontrol(struct mtd_info *mtd, int dat,
+			     unsigned int ctrl)
+{
+	struct nand_chip* this = (struct nand_chip *) (mtd->priv);
+	unsigned int nandaddr = (unsigned int)this->IO_ADDR_W;
+
+	DRAIN_WB();
+
+	if ( ctrl & NAND_CTRL_CHANGE ) {
+		if ( ctrl & NAND_ALE ) /* ALE should be changed */
+			nandaddr |=  (1 << 3);
+		else
+			nandaddr &= ~(1 << 3);
+		if ( ctrl & NAND_CLE ) /* CLE should be changed */
+			nandaddr |=  (1 << 2);
+		else 
+			nandaddr &= ~(1 << 2);
+		if ( ctrl & NAND_NCE ) /* CE should be changed */
+			nand_cs_on();
+		else
+			nand_cs_off();
+	}
+	this->IO_ADDR_W = (void*)nandaddr;
+	DRAIN_WB();
+	if ( dat != NAND_CMD_NONE )
+		cmx270_write_byte(mtd, dat);
+	DRAIN_WB();
+}
+
+/*
+ *	read device ready pin
+ */
+static int cmx270_device_ready(struct mtd_info *mtd)
+{
+	DRAIN_WB();
+	return ( GPLR(GPIO_NAND_RB) & GPIO_bit(GPIO_NAND_RB) );
+}
+
+/*
+ * Main initialization routine
+ */
+static int __init cmx270_init (void)
+{
+	struct nand_chip *this;
+	const char *part_type = 0;
+	int mtd_parts_nb = 0;
+	struct mtd_partition *mtd_parts = 0;
+	unsigned int nandaddr = 0;
+
+	/* Allocate memory for MTD device structure and private data */
+	cmx270_nand_mtd = kmalloc(sizeof(struct mtd_info) +
+				  sizeof(struct nand_chip),
+				  GFP_KERNEL);
+	mb();
+	if (!cmx270_nand_mtd) {
+		printk("Unable to allocate CM-X270 NAND MTD device structure.\n");
+		return -ENOMEM;
+	}
+
+	nandaddr = (unsigned int)ioremap(PXA_CS1_PHYS, 12);
+	if ( !nandaddr ) {
+		printk("Unable to ioremap NAND device");
+		return -EINVAL;
+	}
+
+	/* Get pointer to private data */
+	this = (struct nand_chip *) (&cmx270_nand_mtd[1]);
+
+	/* Initialize structures */
+	memset((char *) cmx270_nand_mtd, 0, sizeof(struct mtd_info));
+	memset((char *) this, 0, sizeof(struct nand_chip));
+
+	/* Link the private data with the MTD structure */
+	cmx270_nand_mtd->owner = THIS_MODULE;
+	cmx270_nand_mtd->priv = this;
+
+	/* insert callbacks */
+	this->IO_ADDR_R = (void __iomem *)nandaddr;
+	this->IO_ADDR_W = (void __iomem *)nandaddr;
+	this->cmd_ctrl = cmx270_hwcontrol;
+	this->dev_ready = cmx270_device_ready;	/* unknown whether that was correct or not so we will just do it like this */
+
+	/* 15 us command delay time */
+	this->chip_delay = 20;
+	this->ecc.mode = NAND_ECC_SOFT;
+
+	/* read/write functions */
+	this->read_byte = cmx270_read_byte;
+	this->read_buf = cmx270_read_buf;
+	this->write_buf = cmx270_write_buf;
+	this->verify_buf = cmx270_verify_buf;
+
+	/* Scan to find existence of the device */
+	if (nand_scan (cmx270_nand_mtd, 1)) {
+		printk(KERN_NOTICE "No NAND device\n");
+		iounmap((void*)nandaddr);
+		kfree (cmx270_nand_mtd);
+		return -ENXIO;
+	}
+
+#ifdef CONFIG_MTD_CMDLINE_PARTS
+	mtd_parts_nb = parse_cmdline_partitions(cmx270_nand_mtd, &mtd_parts,
+						"cmx270");
+	if (mtd_parts_nb > 0)
+		part_type = "command line";
+	else
+		mtd_parts_nb = 0;
+#endif
+	if (mtd_parts_nb == 0)
+	{
+		mtd_parts = partition_info;
+		mtd_parts_nb = NUM_PARTITIONS;
+		part_type = "static";
+	}
+
+	/* Register the partitions */
+	printk(KERN_NOTICE "Using %s partition definition\n", part_type);
+	add_mtd_partitions(cmx270_nand_mtd, mtd_parts, mtd_parts_nb);
+
+	/* Return happy */
+	return 0;
+}
+module_init(cmx270_init);
+
+/*
+ * Clean up routine
+ */
+static void __exit cmx270_cleanup (void)
+{
+	struct nand_chip *this;
+
+	this = (struct nand_chip *) (&cmx270_nand_mtd[1]);
+	iounmap(this->IO_ADDR_R);
+
+	/* Release resources, unregister device */
+	nand_release (cmx270_nand_mtd);
+
+	/* Free the MTD device structure */
+	kfree (cmx270_nand_mtd);
+}
+module_exit(cmx270_cleanup);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Mike Rapoport <mike at compulab dot co dot il>");
+MODULE_DESCRIPTION("NAND flash driver for Compulab CM-X270 Module");

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] CM-x2xx NAND flash support
  2006-07-06 12:48 [PATCH] CM-x2xx NAND flash support Mike Rapoport
@ 2006-07-09  9:26 ` Thomas Gleixner
  2006-07-11  8:46   ` Mike Rapoport
  2006-07-12 13:21   ` Mike Rapoport
  0 siblings, 2 replies; 8+ messages in thread
From: Thomas Gleixner @ 2006-07-09  9:26 UTC (permalink / raw)
  To: Mike Rapoport; +Cc: linux-mtd

On Thu, 2006-07-06 at 14:48 +0200, Mike Rapoport wrote:
> This patch provides MTD support for NAND flash devices on CM-x2xx modules.
> 
> Signed-off-by: Mike Rapoport <mike@compulab.co.il>

> +static struct mtd_info *cmx255_nand_mtd = NULL;

static variables are initialized to 0 by default

> +/*
> + * Module stuff
> + */

Comment without context

> +#define DRAIN_WB() \
> +       do { \
> +               unsigned char dummy; \
> +               asm volatile ("mcr p15, 0, r0, c7, c10, 4":::"r0"); \
> +               dummy=*((unsigned char*)UNCACHED_ADDR); \
> +       } while(0);

stray semicolon ----^

I bet xscale has this functionality somewhere as a macro / inline already

> +/*
> + *     hardware specific access to control-lines
> + */
> +static void cmx255_hwcontrol(struct mtd_info *mtd, int dat,
> +                            unsigned int ctrl)
> +{
> +       struct nand_chip* this = (struct nand_chip *) (mtd->priv);

	no type cast for void* necessary

> +       unsigned int nandaddr = (unsigned int)this->IO_ADDR_W;

what the hell is this type cast for ?

	void __iomem *nandaddr = 

> +       DRAIN_WB();
> +
> +       if ( ctrl & NAND_CTRL_CHANGE ) {
> +               if ( ctrl & NAND_NCE ) /* CE should be changed */

	if (ctrl & N...) {

	also the comment is wrong for all three if()s. The given value is
static and does not necessarily change from one call to the next. e.g.
nCE is kept low across a complete access cycle

> +                       nand_cs_on();
> +               else
> +                       nand_cs_off();
> +               if ( ctrl & NAND_ALE ) /* ALE should be changed */
> +                       nand_ale_on();
> +               else
> +                       nand_ale_off();
> +               if ( ctrl & NAND_CLE ) /* CLE should be changed */
> +                       nand_cle_on();
> +               else 
> +                       nand_cle_off();
> +       }
> +       this->IO_ADDR_W = (void*)nandaddr;

	nandaddr = this->IO_ADDR_W;
	this->IO_ADDR_W = nandaddr;

	?????

> +       DRAIN_WB();
> +
> +       if ( dat != NAND_CMD_NONE )
> +               writel(dat, this->IO_ADDR_W);
> +}
> +
> +/*
> + *     read device ready pin
> + */
> +static int cmx255_device_ready(struct mtd_info *mtd)
> +{
> +       DRAIN_WB();
> +       return GPLR(GPIO_NAND_RB) & GPIO_bit(GPIO_NAND_RB);
> +}
> +
> +/*
> + * Main initialization routine
> + */
> +static int __init cmx255_init (void)
> +{
> +       struct nand_chip *this;
> +       const char *part_type = 0;

	= NULL

> +       int mtd_parts_nb = 0;
> +       struct mtd_partition *mtd_parts = 0;

	dito

> +       static unsigned int nandaddr = 0;

	why static ? also:

	void __iomem *nandaddr;
	
> +       int retval = 0;

	Please do not initialize local variables, when there is no need to.

> +       /* Allocate memory for MTD device structure and private data */
> +       cmx255_nand_mtd = kmalloc(sizeof(struct mtd_info) +
> +                                   sizeof(struct nand_chip),
> +                                   GFP_KERNEL);

	kzalloc

> 
> +       if (!cmx255_nand_mtd) {
> +               printk("Unable to allocate CM-X255 MTD device structure.\n");
> +               retval = -ENOMEM;
> +               goto out;
> +       }
> +
> +       nandaddr = (unsigned int)ioremap(PXA_CS1_PHYS, 0x100);

	See above, also convert to a platform device !

> +       if ( !nandaddr ) {

	if (!nandaddr)

> +               printk("Unable to ioremap NAND device");
> +               retval = -EINVAL;
> +               goto err1;
> +       }
> +
> +       /* Get pointer to private data */
> +       this = (struct nand_chip *) (&cmx255_nand_mtd[1]);
> +
> +       /* Initialize structures */
> +       memset((char *) cmx255_nand_mtd, 0, sizeof(struct mtd_info));
> +       memset((char *) this, 0, sizeof(struct nand_chip));

See above (kzalloc)

> +       /* Link the private data with the MTD structure */
> +       cmx255_nand_mtd->owner = THIS_MODULE;
> +       cmx255_nand_mtd->priv = this;
> +
> +       /* insert callbacks */
> +       this->IO_ADDR_R = (void __iomem *)nandaddr;
> +       this->IO_ADDR_W = (void __iomem *)nandaddr;

See above

> +       this->cmd_ctrl = cmx255_hwcontrol;
> +       this->dev_ready = cmx255_device_ready;
> +
> +       /* 20 us command delay time */
> +       this->chip_delay = 20;
> +       this->ecc.mode = NAND_ECC_SOFT;
> +
> +       /* Scan to find existence of the device */
> +       if (nand_scan (cmx255_nand_mtd, 1)) {

	if (nand_scan(
> 
> +               printk(KERN_NOTICE "No NAND device\n");
> +               retval = -ENXIO;
> +               goto err2;
> +       }
> +
> +#ifdef CONFIG_MTD_CMDLINE_PARTS
> +       mtd_parts_nb = parse_cmdline_partitions(cmx255_nand_mtd, &mtd_parts,
> +                                               "cmx255");
> +       if (mtd_parts_nb > 0)
> +               part_type = "command line";
> +       else
> +               mtd_parts_nb = 0;
> +#endif
> +       if (mtd_parts_nb == 0)
> +       {

	if (!mtd_parts_nb) {

> +               mtd_parts = partition_info;
> +               mtd_parts_nb = NUM_PARTITIONS;
> +               part_type = "static";
> +       }
> +
> +       /* Register the partitions */
> +       printk(KERN_NOTICE "Using %s partition definition\n", part_type);
> +       add_mtd_partitions(cmx255_nand_mtd, mtd_parts, mtd_parts_nb);

	add_mtd_partitions can fail !

also, when you do not intend to use this driver w/o partitions, then
please remove the #ifdef CONFIG_MTD_PARTITIONS around partition_info,
else you have to do a add_mtd_device(mtd) when partitions are disabled

	return 0; 

	otherwise you iounmap and free the mtd data structure 

	Please submit tested drivers. This one never ever worked !

> +  err2:
> +       iounmap((void*)nandaddr);

See above

> +  err1:
> +       kfree(cmx255_nand_mtd);
> +  out:
> +       /* Return happy */
> +       return retval;
> +}
> +module_init(cmx255_init);
> +
> +/*
> + * Clean up routine
> + */
> +static void __exit cmx255_cleanup (void)
> +{
> +       struct nand_chip *this;
> +
> +       this = (struct nand_chip *) (&cmx255_nand_mtd[1]);
> +       iounmap(this->IO_ADDR_R);

unmap after nand_release(). As long as the device is not released it can
be accessed !!!!

> +       /* Release resources, unregister device */
> +       nand_release (cmx255_nand_mtd);

	nand_release(
> +
> +       /* Free the MTD device structure */
> +       kfree (cmx255_nand_mtd);

	kfree(
> +}
> +module_exit(cmx255_cleanup);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Mike Rapoport <mike at compulab dot co dot il>");
> +MODULE_DESCRIPTION("NAND flash driver for Compulab CM-X255 Module");
> diff --git a/drivers/mtd/nand/cmx270-nand.c b/drivers/mtd/nand/cmx270-nand.c
> new file mode 100644
> index 0000000..ffedaec
> --- /dev/null
> +++ b/drivers/mtd/nand/cmx270-nand.c
> @@ -0,0 +1,273 @@
> +/*
> + *  linux/drivers/mtd/nand/cmx270-nand.c
> + *
> + *  Copyright (C) 2006 Compulab, Ltd.
> + *  Mike Rapoport <mike@compulab.co.il>
> + *
> + *  Derived from drivers/mtd/nand/h1910.c
> + *       Copyright (C) 2002 Marius Grger (mag@sysgo.de)
> + *       Copyright (c) 2001 Thomas Gleixner (gleixner@autronix.de)
> + *
> + *
> + * 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.
> + *
> + *  Overview:
> + *   This is a device driver for the NAND flash device found on the
> + *   CM-X270 board.
> + */
> +
> +#include <linux/slab.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/mtd/nand.h>
> +#include <linux/mtd/partitions.h>
> +
> +#include <asm/io.h>
> +#include <asm/irq.h>
> +
> +#include <asm/arch/hardware.h>
> +#include <asm/arch/pxa-regs.h>
> +
> +#define GPIO_NAND_CS   (11)
> +#define GPIO_NAND_RB   (89)
> +
> +#define DRAIN_WB() \
> +       do { \
> +               unsigned char dummy; \
> +               asm volatile ("mcr p15, 0, r0, c7, c10, 4":::"r0"); \
> +               dummy=*((unsigned char*)UNCACHED_ADDR); \
> +       } while(0);

I bet xscale has this functionality somewhere as a macro / inline already

> +/*
> + * MTD structure for CM-X270 board
> + */
> +static struct mtd_info *cmx270_nand_mtd = NULL;
> +
> +/*
> + * Module stuff
> + */
> +
> +#ifdef CONFIG_MTD_PARTITIONS
> +/*
> + * Define static partitions for flash device
> + */
> +static struct mtd_partition partition_info[] = {
> +       {
> +               .name   = "cmx270-0",
> +               .offset = 0,
> +               .size   = MTDPART_SIZ_FULL
> +       }
> +};
> +#define NUM_PARTITIONS (ARRAY_SIZE(partition_info))
> +
> +#endif
> +
> +
> +static         u_char cmx270_read_byte(struct mtd_info *mtd)

stray tab

> +{
> +       struct nand_chip *this = mtd->priv;

blank line between variable declaration and instructions

> +       return (readl(this->IO_ADDR_R) >> 16);
> +}
> +
> +static void cmx270_write_byte(struct mtd_info *mtd, u_char byte)
> +{
> +       struct nand_chip *this = mtd->priv;

dito

> +       writel((byte << 16), this->IO_ADDR_W);
> +}
> +
> +static void cmx270_write_buf(struct mtd_info *mtd, const u_char *buf, int len)
> +{
> +       int i;
> +       struct nand_chip *this = mtd->priv;
> +
> +       for (i=0; i<len; i++) {
> +               writel((*buf++ << 16), this->IO_ADDR_W);
> +       }

No brackets for oneliners

> +}
> +
> +static void cmx270_read_buf(struct mtd_info *mtd, u_char *buf, int len)
> +{
> +       int i;
> +       struct nand_chip *this = mtd->priv;
> +
> +       for (i=0; i<len; i++) {
> +               *buf++ = readl(this->IO_ADDR_R) >> 16;
> +       }

dito

> +}
> +
> +static int cmx270_verify_buf(struct mtd_info *mtd, const u_char *buf, int len)
> +{
> +       int i;
> +       struct nand_chip *this = mtd->priv;
> +
> +       for (i=0; i<len; i++) {
> +               if ( buf[i] != (u_char)(readl(this->IO_ADDR_R) >> 16) )
> +                       return -EFAULT;

	no extra blanks after ( and before )
> +       }
> +
> +       return 0;
> +}
> +
> +static inline void nand_cs_on(void)
> +{
> +       GPCR(GPIO_NAND_CS) = GPIO_bit(GPIO_NAND_CS);
> +}
> +
> +static void nand_cs_off(void)
> +{
> +       DRAIN_WB();
> +
> +       GPSR(GPIO_NAND_CS) = GPIO_bit(GPIO_NAND_CS);
> +}
> +
> +/*
> + *     hardware specific access to control-lines
> + */
> +static void cmx270_hwcontrol(struct mtd_info *mtd, int dat,
> +                            unsigned int ctrl)
> +{
> +       struct nand_chip* this = (struct nand_chip *) (mtd->priv);

See above

> +       unsigned int nandaddr = (unsigned int)this->IO_ADDR_W;
> +
> +       DRAIN_WB();
> +
> +       if ( ctrl & NAND_CTRL_CHANGE ) {
> +               if ( ctrl & NAND_ALE ) /* ALE should be changed */
> +                       nandaddr |=  (1 << 3);
> +               else
> +                       nandaddr &= ~(1 << 3);
> +               if ( ctrl & NAND_CLE ) /* CLE should be changed */
> +                       nandaddr |=  (1 << 2);
> +               else 
> +                       nandaddr &= ~(1 << 2);
> +               if ( ctrl & NAND_NCE ) /* CE should be changed */
> +                       nand_cs_on();
> +               else
> +                       nand_cs_off();
> +       }
> +       this->IO_ADDR_W = (void*)nandaddr;

	dito	

> +       DRAIN_WB();
> +       if ( dat != NAND_CMD_NONE )
> +               cmx270_write_byte(mtd, dat);
> +       DRAIN_WB();
> +}
> +
> +/*
> + *     read device ready pin
> + */
> +static int cmx270_device_ready(struct mtd_info *mtd)
> +{
> +       DRAIN_WB();
> +       return ( GPLR(GPIO_NAND_RB) & GPIO_bit(GPIO_NAND_RB) );
> +}
> +
> +/*
> + * Main initialization routine
> + */
> +static int __init cmx270_init (void)
> +{
> +       struct nand_chip *this;
> +       const char *part_type = 0;
> +       int mtd_parts_nb = 0;
> +       struct mtd_partition *mtd_parts = 0;
> +       unsigned int nandaddr = 0;

	See above

> +       /* Allocate memory for MTD device structure and private data */
> +       cmx270_nand_mtd = kmalloc(sizeof(struct mtd_info) +
> +                                 sizeof(struct nand_chip),
> +                                 GFP_KERNEL);
> +       mb();

	Why do you beed a memory barrier here ?

> +       if (!cmx270_nand_mtd) {
> +               printk("Unable to allocate CM-X270 NAND MTD device structure.\n");
> +               return -ENOMEM;
> +       }
> +
> +       nandaddr = (unsigned int)ioremap(PXA_CS1_PHYS, 12);
> +       if ( !nandaddr ) {
> +               printk("Unable to ioremap NAND device");
> +               return -EINVAL;
> +       }
> +
> +       /* Get pointer to private data */
> +       this = (struct nand_chip *) (&cmx270_nand_mtd[1]);
> +
> +       /* Initialize structures */
> +       memset((char *) cmx270_nand_mtd, 0, sizeof(struct mtd_info));
> +       memset((char *) this, 0, sizeof(struct nand_chip));

	See above

> +       /* Link the private data with the MTD structure */
> +       cmx270_nand_mtd->owner = THIS_MODULE;
> +       cmx270_nand_mtd->priv = this;
> +
> +       /* insert callbacks */
> +       this->IO_ADDR_R = (void __iomem *)nandaddr;
> +       this->IO_ADDR_W = (void __iomem *)nandaddr;
> +       this->cmd_ctrl = cmx270_hwcontrol;
> +       this->dev_ready = cmx270_device_ready;  /* unknown whether that was correct or not so we will just do it like this */
> +
> +       /* 15 us command delay time */
> +       this->chip_delay = 20;
> +       this->ecc.mode = NAND_ECC_SOFT;
> +
> +       /* read/write functions */
> +       this->read_byte = cmx270_read_byte;
> +       this->read_buf = cmx270_read_buf;
> +       this->write_buf = cmx270_write_buf;
> +       this->verify_buf = cmx270_verify_buf;
> +
> +       /* Scan to find existence of the device */
> +       if (nand_scan (cmx270_nand_mtd, 1)) {
> +               printk(KERN_NOTICE "No NAND device\n");
> +               iounmap((void*)nandaddr);
> +               kfree (cmx270_nand_mtd);
> +               return -ENXIO;
> +       }
> +
> +#ifdef CONFIG_MTD_CMDLINE_PARTS
> +       mtd_parts_nb = parse_cmdline_partitions(cmx270_nand_mtd, &mtd_parts,
> +                                               "cmx270");
> +       if (mtd_parts_nb > 0)
> +               part_type = "command line";
> +       else
> +               mtd_parts_nb = 0;
> +#endif
> +       if (mtd_parts_nb == 0)
> +       {
> +               mtd_parts = partition_info;
> +               mtd_parts_nb = NUM_PARTITIONS;
> +               part_type = "static";
> +       }
> +
> +       /* Register the partitions */
> +       printk(KERN_NOTICE "Using %s partition definition\n", part_type);
> +       add_mtd_partitions(cmx270_nand_mtd, mtd_parts, mtd_parts_nb);

See above

> +       /* Return happy */
> +       return 0;
> +}
> +module_init(cmx270_init);
> +
> +/*
> + * Clean up routine
> + */
> +static void __exit cmx270_cleanup (void)

	_cleanup(

> +{
> +       struct nand_chip *this;
> +
> +       this = (struct nand_chip *) (&cmx270_nand_mtd[1]);
> +       iounmap(this->IO_ADDR_R);

See above

> +       /* Release resources, unregister device */
> +       nand_release (cmx270_nand_mtd);
> +
> +       /* Free the MTD device structure */
> +       kfree (cmx270_nand_mtd);
> +}
> +module_exit(cmx270_cleanup);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Mike Rapoport <mike at compulab dot co dot il>");
> +MODULE_DESCRIPTION("NAND flash driver for Compulab CM-X270 Module");

Can you please combine both drivers into one and make it a platform
device. There is no need to keep lots of duplicate functionality around.

	tglx

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] CM-x2xx NAND flash support
  2006-07-11  8:46   ` Mike Rapoport
@ 2006-07-11  8:10     ` Wolfgang Mües
  2006-07-11 11:32       ` Mike Rapoport
  2006-07-11  8:23     ` Thomas Gleixner
  1 sibling, 1 reply; 8+ messages in thread
From: Wolfgang Mües @ 2006-07-11  8:10 UTC (permalink / raw)
  To: linux-mtd, mike

Am Dienstag, 11. Juli 2006 09:48 schrieb Mike Rapoport:
> Thomas Gleixner wrote:
> >On Thu, 2006-07-06 at 14:48 +0200, Mike Rapoport wrote:
> >>This patch provides MTD support for NAND flash devices on CM-x2xx
> >> modules.
> >>+#define DRAIN_WB() \
> >>+       do { \
> >>+               unsigned char dummy; \
> >>+               asm volatile ("mcr p15, 0, r0, c7, c10, 4":::"r0"); \
> >>+               dummy=*((unsigned char*)UNCACHED_ADDR); \
> >>+       } while(0);
> >
> >stray semicolon ----^
> >
> >I bet xscale has this functionality somewhere as a macro / inline already
>
> I found none. Even md() on xscale will not drain write buffer.

dmac_clean_range() from asm/cacheflush.h is what you want.

regards

-- 
Wolfgang Muees                    Vor den Grashoefen 1
Auerswald GmbH & Co. KG       	  D-38162 Cremlingen
Hardware Development              Germany
Tel +49 5306 9219 0               Fax +49 5306 9219 94

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] CM-x2xx NAND flash support
  2006-07-11  8:46   ` Mike Rapoport
  2006-07-11  8:10     ` Wolfgang Mües
@ 2006-07-11  8:23     ` Thomas Gleixner
  2006-07-11 12:36       ` Mike Rapoport
  1 sibling, 1 reply; 8+ messages in thread
From: Thomas Gleixner @ 2006-07-11  8:23 UTC (permalink / raw)
  To: Mike Rapoport; +Cc: linux-mtd

On Tue, 2006-07-11 at 10:46 +0200, Mike Rapoport wrote:
> >>+       unsigned int nandaddr = (unsigned int)this->IO_ADDR_W;
> >>    
> >>
> >
> >what the hell is this type cast for ?
> >
> >	void __iomem *nandaddr = 
> >  
> >
> you can't do |= and &= with void __iomem*

Err, why not ?

> >Can you please combine both drivers into one and make it a platform
> >device. There is no need to keep lots of duplicate functionality around.
> >
> I'm dropping cm-x255 support (management decision) so there's only one 
> platform that can use this driver. What's the point then?

Sorry man, you submitted _two_ drivers in the first place and I did the
review on those. I'm not good at witchcrafting the decisions of your
management.

	tglx

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] CM-x2xx NAND flash support
  2006-07-09  9:26 ` Thomas Gleixner
@ 2006-07-11  8:46   ` Mike Rapoport
  2006-07-11  8:10     ` Wolfgang Mües
  2006-07-11  8:23     ` Thomas Gleixner
  2006-07-12 13:21   ` Mike Rapoport
  1 sibling, 2 replies; 8+ messages in thread
From: Mike Rapoport @ 2006-07-11  8:46 UTC (permalink / raw)
  To: tglx; +Cc: linux-mtd

Thomas Gleixner wrote:

>On Thu, 2006-07-06 at 14:48 +0200, Mike Rapoport wrote:
>  
>
>>This patch provides MTD support for NAND flash devices on CM-x2xx modules.
>>
>>Signed-off-by: Mike Rapoport <mike@compulab.co.il>
>>    
>>
>
>  
>
>  
>
>>+#define DRAIN_WB() \
>>+       do { \
>>+               unsigned char dummy; \
>>+               asm volatile ("mcr p15, 0, r0, c7, c10, 4":::"r0"); \
>>+               dummy=*((unsigned char*)UNCACHED_ADDR); \
>>+       } while(0);
>>    
>>
>
>stray semicolon ----^
>
>I bet xscale has this functionality somewhere as a macro / inline already
>  
>
I found none. Even md() on xscale will not drain write buffer.

>  
>
>>+       unsigned int nandaddr = (unsigned int)this->IO_ADDR_W;
>>    
>>
>
>what the hell is this type cast for ?
>
>	void __iomem *nandaddr = 
>  
>
you can't do |= and &= with void __iomem*

>Can you please combine both drivers into one and make it a platform
>device. There is no need to keep lots of duplicate functionality around.
>
>  
>
I'm dropping cm-x255 support (management decision) so there's only one 
platform that can use this driver. What's the point then?

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

-- 
Sincerely yours,
Mike Rapoport

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] CM-x2xx NAND flash support
  2006-07-11  8:10     ` Wolfgang Mües
@ 2006-07-11 11:32       ` Mike Rapoport
  0 siblings, 0 replies; 8+ messages in thread
From: Mike Rapoport @ 2006-07-11 11:32 UTC (permalink / raw)
  To: Wolfgang Mües; +Cc: linux-mtd

Wolfgang Mües wrote:

>Am Dienstag, 11. Juli 2006 09:48 schrieb Mike Rapoport:
>  
>
>>Thomas Gleixner wrote:
>>    
>>
>>>On Thu, 2006-07-06 at 14:48 +0200, Mike Rapoport wrote:
>>>      
>>>
>>>>This patch provides MTD support for NAND flash devices on CM-x2xx
>>>>modules.
>>>>+#define DRAIN_WB() \
>>>>+       do { \
>>>>+               unsigned char dummy; \
>>>>+               asm volatile ("mcr p15, 0, r0, c7, c10, 4":::"r0"); \
>>>>+               dummy=*((unsigned char*)UNCACHED_ADDR); \
>>>>+       } while(0);
>>>>        
>>>>
>>>stray semicolon ----^
>>>
>>>I bet xscale has this functionality somewhere as a macro / inline already
>>>      
>>>
>>I found none. Even md() on xscale will not drain write buffer.
>>    
>>
>
>dmac_clean_range() from asm/cacheflush.h is what you want.
>  
>
But I don't need cache cleanup. I need to drain WB to make sure GPIO 
level has changed.
Besides, I cannot use dmac_clean_range() if I want the driver as module

>regards
>
>  
>


-- 
Sincerely yours,
Mike Rapoport

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] CM-x2xx NAND flash support
  2006-07-11  8:23     ` Thomas Gleixner
@ 2006-07-11 12:36       ` Mike Rapoport
  0 siblings, 0 replies; 8+ messages in thread
From: Mike Rapoport @ 2006-07-11 12:36 UTC (permalink / raw)
  To: tglx; +Cc: linux-mtd

Thomas Gleixner wrote:

>On Tue, 2006-07-11 at 10:46 +0200, Mike Rapoport wrote:
>  
>
>>>>+       unsigned int nandaddr = (unsigned int)this->IO_ADDR_W;
>>>>   
>>>>
>>>>        
>>>>
>>>what the hell is this type cast for ?
>>>
>>>	void __iomem *nandaddr = 
>>> 
>>>
>>>      
>>>
>>you can't do |= and &= with void __iomem*
>>    
>>
>
>Err, why not ?
>  
>
I have this function:
static void cmx270_hwcontrol(struct mtd_info *mtd, int dat,
			     unsigned int ctrl)
{
	struct nand_chip* this = (struct nand_chip *) (mtd->priv);
	void __iomem *nandaddr = this->IO_ADDR_W;

	if (ctrl & NAND_CTRL_CHANGE) {
		if ( ctrl & NAND_ALE )
			nandaddr |=  (1 << 3);
		else
			nandaddr &= ~(1 << 3);
		if ( ctrl & NAND_CLE )
			nandaddr |=  (1 << 2);
		else
			nandaddr &= ~(1 << 2);
		if ( ctrl & NAND_NCE )
			nand_cs_on();
		else
			nand_cs_off();
	}
	this->IO_ADDR_W = (void*)nandaddr;
	if (dat != NAND_CMD_NONE)
		cmx270_write_byte(mtd, dat);
}

And these errors:
drivers/mtd/nand/cmx270-nand.c: In function 'cmx270_hwcontrol':
drivers/mtd/nand/cmx270-nand.c:137: error: invalid operands to binary |
drivers/mtd/nand/cmx270-nand.c:139: error: invalid operands to binary &
drivers/mtd/nand/cmx270-nand.c:141: error: invalid operands to binary |
drivers/mtd/nand/cmx270-nand.c:143: error: invalid operands to binary &

> gcc -v
arm-xscale-linux-gnu-gcc -v
Using built-in specs.
Target: arm-xscale-linux-gnu
...
Thread model: posix
gcc version 4.1.0



>  
>
>>>Can you please combine both drivers into one and make it a platform
>>>device. There is no need to keep lots of duplicate functionality around.
>>>
>>>      
>>>
>>I'm dropping cm-x255 support (management decision) so there's only one 
>>platform that can use this driver. What's the point then?
>>    
>>
>
>Sorry man, you submitted _two_ drivers in the first place and I did the
>review on those. I'm not good at witchcrafting the decisions of your
>management.
>  
>
You're right, and I agree that for two drivers it makes sence creating 
platform device and combining the drivers. But now I'm going to submit 
only _one_ driver. Do you still think platfrom device would be necessary?

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


-- 
Sincerely yours,
Mike Rapoport

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] CM-x2xx NAND flash support
  2006-07-09  9:26 ` Thomas Gleixner
  2006-07-11  8:46   ` Mike Rapoport
@ 2006-07-12 13:21   ` Mike Rapoport
  1 sibling, 0 replies; 8+ messages in thread
From: Mike Rapoport @ 2006-07-12 13:21 UTC (permalink / raw)
  To: tglx; +Cc: linux-mtd

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

Thomas Gleixner wrote:

>On Thu, 2006-07-06 at 14:48 +0200, Mike Rapoport wrote:
>  
>
>>This patch provides MTD support for NAND flash devices on CM-x2xx modules.
>>
>>Signed-off-by: Mike Rapoport <mike@compulab.co.il>
>>    
>>
I've tried to fix all the coding style errors.

>  
>
>>+#define DRAIN_WB() \
>>+       do { \
>>+               unsigned char dummy; \
>>+               asm volatile ("mcr p15, 0, r0, c7, c10, 4":::"r0"); \
>>+               dummy=*((unsigned char*)UNCACHED_ADDR); \
>>+       } while(0);
>>    
>>
>I bet xscale has this functionality somewhere as a macro / inline already
>
>  
>
There's dma_clean_range but it's not exactly what I need, so I left this 
macro (without the last semi column)

>>+       unsigned int nandaddr = (unsigned int)this->IO_ADDR_W;
>>    
>>
>
>what the hell is this type cast for ?
>
>	void __iomem *nandaddr = 
>
>  
>
>>+       unsigned int nandaddr = (unsigned int)this->IO_ADDR_W;
>>+
>>+       DRAIN_WB();
>>+
>>+       if ( ctrl & NAND_CTRL_CHANGE ) {
>>+               if ( ctrl & NAND_ALE ) /* ALE should be changed */
>>+                       nandaddr |=  (1 << 3);
>>+               else
>>+                       nandaddr &= ~(1 << 3);
>>+               if ( ctrl & NAND_CLE ) /* CLE should be changed */
>>+                       nandaddr |=  (1 << 2);
>>+               else 
>>+                       nandaddr &= ~(1 << 2);
>>+               if ( ctrl & NAND_NCE ) /* CE should be changed */
>>+                       nand_cs_on();
>>+               else
>>+                       nand_cs_off();
>>+       }
>>+       this->IO_ADDR_W = (void*)nandaddr;
>>    
>>
>	dito	
>
As I wrote earlier, void* cannot be operand of bitwise operations. I 
have ALE and CLE connected to local bus address lines, that's why I'm 
using bitwise operations for address. I'll appreciate any other way to 
achieve the same functionality, but I've found none.

>Can you please combine both drivers into one and make it a platform
>device. There is no need to keep lots of duplicate functionality around.
>  
>
Sorry for the inconvenience, but I've dropped x255 support, so I don't 
it makes sense to make the driver platform device.

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


-- 
Sincerely yours,
Mike Rapoport


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: cm_x2xx_mtd.patch --]
[-- Type: text/x-diff; name="cm_x2xx_mtd.patch", Size: 7782 bytes --]

 drivers/mtd/nand/Kconfig       |    4 +
 drivers/mtd/nand/Makefile      |    1 
 drivers/mtd/nand/cmx270-nand.c |  271 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 276 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index 3db77ee..4c212fd 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -232,6 +232,10 @@ config MTD_NAND_CS553X
 
 	  If you say "m", the module will be called "cs553x_nand.ko".
 
+config MTD_NAND_CM_X270
+	tristate "Support for NAND Flash on CM-X270 modules"
+	depends on MTD_NAND && MACH_CM_X270
+
 config MTD_NAND_NANDSIM
 	tristate "Support for NAND Flash Simulator"
 	depends on MTD_NAND && MTD_PARTITIONS
diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
index f747593..f2718aa 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -22,5 +22,6 @@ obj-$(CONFIG_MTD_NAND_TS7250)		+= ts7250
 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_CM_X270)		+= cmx270-nand.o
 
 nand-objs = nand_base.o nand_bbt.o
diff --git a/drivers/mtd/nand/cmx270-nand.c b/drivers/mtd/nand/cmx270-nand.c
new file mode 100644
index 0000000..8a1911c
--- /dev/null
+++ b/drivers/mtd/nand/cmx270-nand.c
@@ -0,0 +1,271 @@
+/*
+ *  linux/drivers/mtd/nand/cmx270-nand.c
+ *
+ *  Copyright (C) 2006 Compulab, Ltd.
+ *  Mike Rapoport <mike@compulab.co.il>
+ *
+ *  Derived from drivers/mtd/nand/h1910.c
+ *       Copyright (C) 2002 Marius Gröger (mag@sysgo.de)
+ *       Copyright (c) 2001 Thomas Gleixner (gleixner@autronix.de)
+ *
+ *
+ * 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.
+ *
+ *  Overview:
+ *   This is a device driver for the NAND flash device found on the
+ *   CM-X270 board.
+ */
+
+#include <linux/slab.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/nand.h>
+#include <linux/mtd/partitions.h>
+
+#include <asm/io.h>
+#include <asm/irq.h>
+
+#include <asm/arch/hardware.h>
+#include <asm/arch/pxa-regs.h>
+
+#define GPIO_NAND_CS	(11)
+#define GPIO_NAND_RB	(89)
+
+/* This macro needed to ensure in-order operation of GPIO and local
+ * bus. Without both asm command and dummy uncached read there're
+ * states when NAND access is broken. I've looked for such macro(s) in
+ * include/asm-arm but found nothing approptiate.
+ * dmac_clean_range is close, but is makes cache invalidation
+ * unnecessary here and it cannot be used in module
+ */
+#define DRAIN_WB() \
+	do { \
+		unsigned char dummy; \
+		asm volatile ("mcr p15, 0, r0, c7, c10, 4":::"r0"); \
+		dummy=*((unsigned char*)UNCACHED_ADDR); \
+	} while(0)
+
+/* MTD structure for CM-X270 board */
+static struct mtd_info *cmx270_nand_mtd;
+
+/* remaped IO address of the device */
+static void __iomem *cmx270_nand_io;
+
+/*
+ * Define static partitions for flash device
+ */
+static struct mtd_partition partition_info[] = {
+	{
+		.name	= "cmx270-0",
+		.offset	= 0,
+		.size	= MTDPART_SIZ_FULL
+	}
+};
+#define NUM_PARTITIONS (ARRAY_SIZE(partition_info))
+
+const char *part_probes[] = { "cmdlinepart", NULL };
+
+static 	u_char cmx270_read_byte(struct mtd_info *mtd)
+{
+	struct nand_chip *this = mtd->priv;
+
+	return (readl(this->IO_ADDR_R) >> 16);
+}
+
+static void cmx270_write_buf(struct mtd_info *mtd, const u_char *buf, int len)
+{
+	int i;
+	struct nand_chip *this = mtd->priv;
+
+	for (i=0; i<len; i++)
+		writel((*buf++ << 16), this->IO_ADDR_W);
+}
+
+static void cmx270_read_buf(struct mtd_info *mtd, u_char *buf, int len)
+{
+	int i;
+	struct nand_chip *this = mtd->priv;
+
+	for (i=0; i<len; i++)
+		*buf++ = readl(this->IO_ADDR_R) >> 16;
+}
+
+static int cmx270_verify_buf(struct mtd_info *mtd, const u_char *buf, int len)
+{
+	int i;
+	struct nand_chip *this = mtd->priv;
+
+	for (i=0; i<len; i++)
+		if (buf[i] != (u_char)(readl(this->IO_ADDR_R) >> 16))
+			return -EFAULT;
+
+	return 0;
+}
+
+static inline void nand_cs_on(void)
+{
+	GPCR(GPIO_NAND_CS) = GPIO_bit(GPIO_NAND_CS);
+}
+
+static void nand_cs_off(void)
+{
+	DRAIN_WB();
+
+	GPSR(GPIO_NAND_CS) = GPIO_bit(GPIO_NAND_CS);
+}
+
+/*
+ *	hardware specific access to control-lines
+ */
+static void cmx270_hwcontrol(struct mtd_info *mtd, int dat,
+			     unsigned int ctrl)
+{
+	struct nand_chip* this = mtd->priv;
+	unsigned int nandaddr = (unsigned int)this->IO_ADDR_W;
+
+	DRAIN_WB();
+
+	if (ctrl & NAND_CTRL_CHANGE) {
+		if ( ctrl & NAND_ALE )
+			nandaddr |=  (1 << 3);
+		else
+			nandaddr &= ~(1 << 3);
+		if ( ctrl & NAND_CLE )
+			nandaddr |=  (1 << 2);
+		else
+			nandaddr &= ~(1 << 2);
+		if ( ctrl & NAND_NCE )
+			nand_cs_on();
+		else
+			nand_cs_off();
+	}
+
+	DRAIN_WB();
+	this->IO_ADDR_W = (void __iomem*)nandaddr;
+	if (dat != NAND_CMD_NONE)
+		writel((dat << 16), this->IO_ADDR_W);
+
+	DRAIN_WB();
+}
+
+/*
+ *	read device ready pin
+ */
+static int cmx270_device_ready(struct mtd_info *mtd)
+{
+	DRAIN_WB();
+
+	return (GPLR(GPIO_NAND_RB) & GPIO_bit(GPIO_NAND_RB));
+}
+
+/*
+ * Main initialization routine
+ */
+static int __devinit cmx270_init(void)
+{
+	struct nand_chip *this;
+	const char *part_type;
+	struct mtd_partition *mtd_parts;
+	int mtd_parts_nb = 0;
+	int ret;
+
+	/* Allocate memory for MTD device structure and private data */
+	cmx270_nand_mtd = kzalloc(sizeof(struct mtd_info) +
+				  sizeof(struct nand_chip),
+				  GFP_KERNEL);
+	if (!cmx270_nand_mtd) {
+		printk("Unable to allocate CM-X270 NAND MTD device structure.\n");
+		return -ENOMEM;
+	}
+
+	cmx270_nand_io = ioremap(PXA_CS1_PHYS, 12);
+	if (!cmx270_nand_io) {
+		printk("Unable to ioremap NAND device\n");
+		ret = -EINVAL;
+		goto err1;
+	}
+
+	/* Get pointer to private data */
+	this = (struct nand_chip *)(&cmx270_nand_mtd[1]);
+
+	/* Link the private data with the MTD structure */
+	cmx270_nand_mtd->owner = THIS_MODULE;
+	cmx270_nand_mtd->priv = this;
+
+	/* insert callbacks */
+	this->IO_ADDR_R = cmx270_nand_io;
+	this->IO_ADDR_W = cmx270_nand_io;
+	this->cmd_ctrl = cmx270_hwcontrol;
+	this->dev_ready = cmx270_device_ready;
+
+	/* 15 us command delay time */
+	this->chip_delay = 20;
+	this->ecc.mode = NAND_ECC_SOFT;
+
+	/* read/write functions */
+	this->read_byte = cmx270_read_byte;
+	this->read_buf = cmx270_read_buf;
+	this->write_buf = cmx270_write_buf;
+	this->verify_buf = cmx270_verify_buf;
+
+	/* Scan to find existence of the device */
+	if (nand_scan (cmx270_nand_mtd, 1)) {
+		printk(KERN_NOTICE "No NAND device\n");
+		ret = -ENXIO;
+		goto err2;
+	}
+
+#ifdef CONFIG_MTD_CMDLINE_PARTS
+	mtd_parts_nb = parse_mtd_partitions(cmx270_nand_mtd, part_probes,
+					    &mtd_parts, 0);
+	if (mtd_parts_nb > 0)
+		part_type = "command line";
+	else
+		mtd_parts_nb = 0;
+#endif
+	if (!mtd_parts_nb) {
+		mtd_parts = partition_info;
+		mtd_parts_nb = NUM_PARTITIONS;
+		part_type = "static";
+	}
+
+	/* Register the partitions */
+	printk(KERN_NOTICE "Using %s partition definition\n", part_type);
+	ret = add_mtd_partitions(cmx270_nand_mtd, mtd_parts, mtd_parts_nb);
+	if (ret)
+		goto err2;
+
+	/* Return happy */
+	return 0;
+
+err2:
+	iounmap(cmx270_nand_io);
+err1:
+	kfree(cmx270_nand_mtd);
+
+	return ret;
+
+}
+module_init(cmx270_init);
+
+/*
+ * Clean up routine
+ */
+static void __devexit cmx270_cleanup(void)
+{
+	/* Release resources, unregister device */
+	nand_release(cmx270_nand_mtd);
+
+	iounmap(cmx270_nand_io);
+
+	/* Free the MTD device structure */
+	kfree (cmx270_nand_mtd);
+}
+module_exit(cmx270_cleanup);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Mike Rapoport <mike@compulab.co.il>");
+MODULE_DESCRIPTION("NAND flash driver for Compulab CM-X270 Module");

^ permalink raw reply related	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2006-07-12 12:22 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2006-07-06 12:48 [PATCH] CM-x2xx NAND flash support Mike Rapoport
2006-07-09  9:26 ` Thomas Gleixner
2006-07-11  8:46   ` Mike Rapoport
2006-07-11  8:10     ` Wolfgang Mües
2006-07-11 11:32       ` Mike Rapoport
2006-07-11  8:23     ` Thomas Gleixner
2006-07-11 12:36       ` Mike Rapoport
2006-07-12 13:21   ` Mike Rapoport

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.