Skip to contentSkip to conversation

Nand driver for Nomadik 8815 SoC (on NHK8815 board)

PATCH In review Alessandro Rubini 3 participants 5 messages Jul 28, 2009 – Jul 29, 2009

Subsystems (5)
lore
Share & tools
lore
#0 lore raw

PATCH v6 1/1 Nand driver for Nomadik 8815 SoC (on NHK8815 board)

[email protected]
From: Alessandro Rubini <[email protected]>
Sender: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected],[email protected]

From: Alessandro Rubini <[email protected]>

---

changes in V6:
- blindly applied checkpatch suggestions (even if I disagree)
- switched to pm_ops
- declare soft ecc instead of hard

The last point makes the driver non-working unless the patch by Vimal Singh at http://patchwork.ozlabs.org/patch/13697/ is applied.

The patch applies to rmk's tree.

 arch/arm/configs/nhk8815_defconfig        |    2 +-
 arch/arm/mach-nomadik/board-nhk8815.c     |   92 ++++++++++
 arch/arm/mach-nomadik/include/mach/fsmc.h |   29 ++++
 arch/arm/mach-nomadik/include/mach/nand.h |   16 ++
 drivers/mtd/nand/Kconfig                  |    6 +
 drivers/mtd/nand/Makefile                 |    1 +
 drivers/mtd/nand/nomadik_nand.c           |  258 +++++++++++++++++++++++++++++
 7 files changed, 403 insertions(+), 1 deletions(-)
 create mode 100644 arch/arm/mach-nomadik/include/mach/fsmc.h
 create mode 100644 arch/arm/mach-nomadik/include/mach/nand.h
 create mode 100644 drivers/mtd/nand/nomadik_nand.c
arch/arm/configs/nhk8815_defconfig +1 -1
index 9bb45b9..600cb27 100644
@@ -498,7 +498,7 @@ CONFIG_MTD_CFI_I2=y
 # CONFIG_MTD_DOC2001PLUS is not set
 CONFIG_MTD_NAND=y
 CONFIG_MTD_NAND_VERIFY_WRITE=y
-# CONFIG_MTD_NAND_ECC_SMC is not set
+CONFIG_MTD_NAND_ECC_SMC=y
 # CONFIG_MTD_NAND_MUSEUM_IDS is not set
 # CONFIG_MTD_NAND_GPIO is not set
 CONFIG_MTD_NAND_IDS=y
arch/arm/mach-nomadik/board-nhk8815.c +92 -0

110 lines not on this page · raw message

arch/arm/mach-nomadik/include/mach/fsmc.h new file +29 -0
new file mode 100644
index 0000000..8c2c051
@@ -0,0 +1,29 @@
+
+/* Definitions for the Nomadik FSMC "Flexible Static Memory controller" */
+
+#ifndef __ASM_ARCH_FSMC_H
+#define __ASM_ARCH_FSMC_H
+
+#include <mach/hardware.h>
+/*
+ * Register list
+ */
+
+/* bus control reg. and bus timing reg. for CS0..CS3 */
+#define FSMC_BCR(x)     (NOMADIK_FSMC_VA + (x << 3))
+#define FSMC_BTR(x)     (NOMADIK_FSMC_VA + (x << 3) + 0x04)
+
+/* PC-card and NAND:
+ * PCR = control register
+ * PMEM = memory timing
+ * PATT = attribute timing
+ * PIO = I/O timing
+ * PECCR = ECC result
+ */
+#define FSMC_PCR(x)     (NOMADIK_FSMC_VA + ((2 + x) << 5) + 0x00)
+#define FSMC_PMEM(x)    (NOMADIK_FSMC_VA + ((2 + x) << 5) + 0x08)
+#define FSMC_PATT(x)    (NOMADIK_FSMC_VA + ((2 + x) << 5) + 0x0c)
+#define FSMC_PIO(x)     (NOMADIK_FSMC_VA + ((2 + x) << 5) + 0x10)
+#define FSMC_PECCR(x)   (NOMADIK_FSMC_VA + ((2 + x) << 5) + 0x14)
+
+#endif /* __ASM_ARCH_FSMC_H */
arch/arm/mach-nomadik/include/mach/nand.h new file +16 -0
new file mode 100644
index 0000000..c3c8254
@@ -0,0 +1,16 @@
+#ifndef __ASM_ARCH_NAND_H
+#define __ASM_ARCH_NAND_H
+
+struct nomadik_nand_platform_data {
+	struct mtd_partition *parts;
+	int nparts;
+	int options;
+	int (*init) (void);
+	int (*exit) (void);
+};
+
+#define NAND_IO_DATA	0x40000000
+#define NAND_IO_CMD	0x40800000
+#define NAND_IO_ADDR	0x41000000
+
+#endif				/* __ASM_ARCH_NAND_H */
drivers/mtd/nand/Kconfig +6 -0
index ce96c09..b9fa465 100644
@@ -426,6 +426,12 @@ config MTD_NAND_MXC
 	  This enables the driver for the NAND flash controller on the
 	  MXC processors.
 
+config MTD_NAND_NOMADIK
+	tristate "ST Nomadik 8815 NAND support"
+	depends on ARCH_NOMADIK
+	help
+	  Driver for the NAND flash controller on the Nomadik, with ECC.
+
 config MTD_NAND_SH_FLCTL
 	tristate "Support for NAND on Renesas SuperH FLCTL"
 	depends on MTD_NAND && SUPERH && CPU_SUBTYPE_SH7723
drivers/mtd/nand/Makefile +1 -0
index f3a786b..0fe9a0c 100644
@@ -40,5 +40,6 @@ obj-$(CONFIG_MTD_NAND_SH_FLCTL)		+= sh_flctl.o
 obj-$(CONFIG_MTD_NAND_MXC)		+= mxc_nand.o
 obj-$(CONFIG_MTD_NAND_SOCRATES)		+= socrates_nand.o
 obj-$(CONFIG_MTD_NAND_TXX9NDFMC)	+= txx9ndfmc.o
+obj-$(CONFIG_MTD_NAND_NOMADIK)		+= nomadik_nand.o
 
 nand-objs := nand_base.o nand_bbt.o
drivers/mtd/nand/nomadik_nand.c new file +258 -0

258 lines not on this page · raw message

Signature · 1 line
1.6.0.2
#1 lore raw

Sorry to say, but you missed one comment I gave last time:

On Tue, Jul 28, 2009 at 3:29 PM, Alessandro Rubini<[email protected]> wrote:

---snip---

drivers/mtd/nand/nomadik_nand.c line 53

Quoted 7 lines
drivers/mtd/nand/nomadik_nand.c new file +7 -0
+static inline int parity(int b) /* uses low 8 bits: returns 0 or all-1 */
+{
+       b = b ^ (b >> 4);
+       b = b ^ (b >> 2);
+       return (b ^ (b >> 1)) & 1
+               ? ~0 : 0;
+}

This function is not in use... please remove it.

---snip---

Quoted 10 lines

+       /*
+        * This stanza declares ECC_HW but uses soft routines. It's because
+        * HW claims to make the calculation but not the correction. However,
+        * I haven't managed to get the desired data out of it until now.
+        */
+       nand->ecc.mode = NAND_ECC_SOFT;
+       nand->ecc.layout = &nomadik_ecc_layout;
+       nand->ecc.hwctl = nomadik_ecc_control;
+       nand->ecc.size = 512;
+       nand->ecc.bytes = 3;

As I wrote earlier too, are you aware of that currently 512 byte sector ECC will not work without below patch: (since in nand_base.c, SW ECC configurations over write the 'ecc.size' by 256)

http://patchwork.ozlabs.org/patch/13697/

As an alternative, if you use 'NAND_SKIP_BBTSCAN' option, moving assignment:

      nand->ecc.size = 512;

after 'nand_scan' is done, will work.

Quoted 11 lines
drivers/mtd/nand/nomadik_nand.c new file +11 -0
+
+       nand->options = pdata->options;
+
+       /*
+        * Scan to find existance of the device
+        */
+       if (nand_scan(&host->mtd, 1)) {
+               ret = -ENXIO;
+               goto err_unmap;
+       }
+
#2 lore raw
Quoted 33 lines · vimal singh

Sorry to say, but you missed one comment I gave last time:

On Tue, Jul 28, 2009 at 3:29 PM, Alessandro Rubini<[email protected]> wrote:

---snip---

drivers/mtd/nand/nomadik_nand.c line 53

Quoted 7 lines
drivers/mtd/nand/nomadik_nand.c new file +7 -0
+static inline int parity(int b) /* uses low 8 bits: returns 0 or all-1 */
+{
+       b = b ^ (b >> 4);
+       b = b ^ (b >> 2);
+       return (b ^ (b >> 1)) & 1
+               ? ~0 : 0;
+}

This function is not in use... please remove it.

---snip---

Quoted 10 lines

+       /*
+        * This stanza declares ECC_HW but uses soft routines. It's because
+        * HW claims to make the calculation but not the correction. However,
+        * I haven't managed to get the desired data out of it until now.
+        */
+       nand->ecc.mode = NAND_ECC_SOFT;
+       nand->ecc.layout = &nomadik_ecc_layout;
+       nand->ecc.hwctl = nomadik_ecc_control;
+       nand->ecc.size = 512;
+       nand->ecc.bytes = 3;

As I wrote earlier too, are you aware of that currently 512 byte sector ECC will not work without below patch: (since in nand_base.c, SW ECC configurations over write the 'ecc.size' by 256)

Quoted 6 lines

http://patchwork.ozlabs.org/patch/13697/

As an alternative, if you use 'NAND_SKIP_BBTSCAN' option, moving assignment:
     nand->ecc.size = 512;
after 'nand_scan' is done, will work.

Sorry, you already mentioned it... I missed those lines in your mail.

-vimal

Quoted 12 lines
Quoted 11 lines
drivers/mtd/nand/nomadik_nand.c new file +11 -0
+
+       nand->options = pdata->options;
+
+       /*
+        * Scan to find existance of the device
+        */
+       if (nand_scan(&host->mtd, 1)) {
+               ret = -ENXIO;
+               goto err_unmap;
+       }
+
Acked-by Russell King on v6 1/1
#3 lore raw
In reply to Alessandro Rubini

Just some trivial comments, which I leave you to decide what to do with.

Either way...

Quoted 6 lines · Alessandro Rubini

[email protected]
From: Alessandro Rubini <[email protected]>
Sender: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected],[email protected]

Not sure what the above is, but it doesn't look like it should be part of this patch.

arch/arm/mach-nomadik/board-nhk8815.c line 23

Quoted 13 lines
arch/arm/mach-nomadik/board-nhk8815.c +5 -0
index 79bdea9..3fde972 100644
@@ -16,12 +16,103 @@
 #include <linux/amba/bus.h>
 #include <linux/interrupt.h>
 #include <linux/gpio.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/nand.h>
+#include <linux/mtd/partitions.h>
+#include <asm/sizes.h>
+#include <asm/io.h>

My mailer's setup to show asm/io.h includes in red, so it's something I'm hot on. This should be linux/io.h

arch/arm/mach-nomadik/board-nhk8815.c line 109

Quoted 4 lines
arch/arm/mach-nomadik/board-nhk8815.c +4 -0
+static struct platform_device nhk8815_nand_device = {
+	.name		= "nomadik_nand",
+	.dev		= {
+				.platform_data = &nhk8815_nand_data,

Not sure why this is tabbed so far over. Just two tabs seems sane.

Quoted 58 lines
arch/arm/mach-nomadik/board-nhk8815.c +7 -0
+	},
+	.resource	= nhk8815_nand_resources,
+	.num_resources	= ARRAY_SIZE(nhk8815_nand_resources),
+};
+
+
 #define __MEM_4K_RESOURCE(x) \
 	.res = {.start = (x), .end = (x) + SZ_4K - 1, .flags = IORESOURCE_MEM}
 
@@ -81,6 +172,7 @@ static int __init nhk8815_eth_init(void)
 device_initcall(nhk8815_eth_init);
 
 static struct platform_device *nhk8815_platform_devices[] __initdata = {
+	&nhk8815_nand_device,
 	&nhk8815_eth_device,
 	/* will add more devices */
 };
arch/arm/mach-nomadik/include/mach/fsmc.h new file +29 -0
new file mode 100644
index 0000000..8c2c051
@@ -0,0 +1,29 @@
+
+/* Definitions for the Nomadik FSMC "Flexible Static Memory controller" */
+
+#ifndef __ASM_ARCH_FSMC_H
+#define __ASM_ARCH_FSMC_H
+
+#include <mach/hardware.h>
+/*
+ * Register list
+ */
+
+/* bus control reg. and bus timing reg. for CS0..CS3 */
+#define FSMC_BCR(x)     (NOMADIK_FSMC_VA + (x << 3))
+#define FSMC_BTR(x)     (NOMADIK_FSMC_VA + (x << 3) + 0x04)
+
+/* PC-card and NAND:
+ * PCR = control register
+ * PMEM = memory timing
+ * PATT = attribute timing
+ * PIO = I/O timing
+ * PECCR = ECC result
+ */
+#define FSMC_PCR(x)     (NOMADIK_FSMC_VA + ((2 + x) << 5) + 0x00)
+#define FSMC_PMEM(x)    (NOMADIK_FSMC_VA + ((2 + x) << 5) + 0x08)
+#define FSMC_PATT(x)    (NOMADIK_FSMC_VA + ((2 + x) << 5) + 0x0c)
+#define FSMC_PIO(x)     (NOMADIK_FSMC_VA + ((2 + x) << 5) + 0x10)
+#define FSMC_PECCR(x)   (NOMADIK_FSMC_VA + ((2 + x) << 5) + 0x14)
+
+#endif /* __ASM_ARCH_FSMC_H */
arch/arm/mach-nomadik/include/mach/nand.h new file +0 -0
new file mode 100644
index 0000000..c3c8254

arch/arm/mach-nomadik/include/mach/nand.h line 7

Quoted 7 lines
arch/arm/mach-nomadik/include/mach/nand.h new file +7 -0
@@ -0,0 +1,16 @@
+#ifndef __ASM_ARCH_NAND_H
+#define __ASM_ARCH_NAND_H
+
+struct nomadik_nand_platform_data {
+	struct mtd_partition *parts;
+	int nparts;
+	int options;

What's 'options' - it doesn't seem to be used by the driver, other than taking a copy.

#4 lore raw
Quoted 101 lines · Russell King - ARM Linux

Just some trivial comments, which I leave you to decide what to do with.

Either way...

Quoted 6 lines · Alessandro Rubini

        [email protected]
From: Alessandro Rubini <[email protected]>
Sender: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected],[email protected]

Not sure what the above is, but it doesn't look like it should be part of this patch.

arch/arm/mach-nomadik/board-nhk8815.c line 23

Quoted 13 lines
arch/arm/mach-nomadik/board-nhk8815.c +5 -0
index 79bdea9..3fde972 100644
@@ -16,12 +16,103 @@
  #include <linux/amba/bus.h>
  #include <linux/interrupt.h>
  #include <linux/gpio.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/nand.h>
+#include <linux/mtd/partitions.h>
+#include <asm/sizes.h>
+#include <asm/io.h>

My mailer's setup to show asm/io.h includes in red, so it's something I'm hot on.  This should be linux/io.h

arch/arm/mach-nomadik/board-nhk8815.c line 109

Quoted 4 lines
arch/arm/mach-nomadik/board-nhk8815.c +4 -0
+static struct platform_device nhk8815_nand_device = {
+     .name           = "nomadik_nand",
+     .dev            = {
+                             .platform_data = &nhk8815_nand_data,

Not sure why this is tabbed so far over.  Just two tabs seems sane.

Quoted 60 lines
arch/arm/mach-nomadik/board-nhk8815.c +7 -0
+     },
+     .resource       = nhk8815_nand_resources,
+     .num_resources  = ARRAY_SIZE(nhk8815_nand_resources),
+};
+
+
  #define __MEM_4K_RESOURCE(x) \
       .res = {.start = (x), .end = (x) + SZ_4K - 1, .flags = IORESOURCE_MEM}
 
@@ -81,6 +172,7 @@ static int __init nhk8815_eth_init(void)
  device_initcall(nhk8815_eth_init);
 
  static struct platform_device *nhk8815_platform_devices[] __initdata = {
+     &nhk8815_nand_device,
       &nhk8815_eth_device,
       /* will add more devices */
  };
arch/arm/mach-nomadik/include/mach/fsmc.h new file +29 -0
new file mode 100644
index 0000000..8c2c051
@@ -0,0 +1,29 @@
+
+/* Definitions for the Nomadik FSMC "Flexible Static Memory controller" */
+
+#ifndef __ASM_ARCH_FSMC_H
+#define __ASM_ARCH_FSMC_H
+
+#include <mach/hardware.h>
+/*
+ * Register list
+ */
+
+/* bus control reg. and bus timing reg. for CS0..CS3 */
+#define FSMC_BCR(x)     (NOMADIK_FSMC_VA + (x << 3))
+#define FSMC_BTR(x)     (NOMADIK_FSMC_VA + (x << 3) + 0x04)
+
+/* PC-card and NAND:
+ * PCR = control register
+ * PMEM = memory timing
+ * PATT = attribute timing
+ * PIO = I/O timing
+ * PECCR = ECC result
+ */
+#define FSMC_PCR(x)     (NOMADIK_FSMC_VA + ((2 + x) << 5) + 0x00)
+#define FSMC_PMEM(x)    (NOMADIK_FSMC_VA + ((2 + x) << 5) + 0x08)
+#define FSMC_PATT(x)    (NOMADIK_FSMC_VA + ((2 + x) << 5) + 0x0c)
+#define FSMC_PIO(x)     (NOMADIK_FSMC_VA + ((2 + x) << 5) + 0x10)
+#define FSMC_PECCR(x)   (NOMADIK_FSMC_VA + ((2 + x) << 5) + 0x14)
+
+#endif /* __ASM_ARCH_FSMC_H */
arch/arm/mach-nomadik/include/mach/nand.h new file +2 -0
new file mode 100644
index 0000000..c3c8254
@@ -0,0 +1,16 @@
+#ifndef __ASM_ARCH_NAND_H
+#define __ASM_ARCH_NAND_H
Quoted 8 lines

arch/arm/mach-nomadik/include/mach/nand.h line 7

Quoted 5 lines
arch/arm/mach-nomadik/include/mach/nand.h new file +5 -0
+
+struct nomadik_nand_platform_data {
+     struct mtd_partition *parts;
+     int nparts;
+     int options;

What's 'options' - it doesn't seem to be used by the driver, other than taking a copy.

There are a few 'options' provided by board specific file to the driver. Which will be used by generic layer of NAND driver (i.e. nand_base.c, nand_bbt.c).

for example in this driver 'options' provided by board file are:
.options = NAND_COPYBACK | NAND_CACHEPRG | NAND_NO_PADDING \
| NAND_NO_READRDY | NAND_NO_AUTOINCR,

-vimal

Quoted 5 lines

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