[RFC/PATCH] ARM: OMAP: OneNAND support for 2430SDP

[RFC/PATCH] ARM: OMAP: OneNAND support for 2430SDP

[Kevin Hilman]

Adds platform device for use of existing OMAP2 driver.  Also, the
following changes are made to the existing driver:

- add option for not using DMA (by passing dma_channel = -1)
- removes obsoleted pt_regs arg from interrupt handler

RFC: Notice the change to onenand_base.c which comments out the last
     OOB chunk.  Without this, it would not work.  Previous drivers
     for this board such as the 2.6.10-based MV kernel have this
     same change.  Anyone know what is going on here?

Signed-off-by: Kevin Hilman <khilman@mvista.com>

---
 arch/arm/mach-omap2/Makefile              |    3 
 arch/arm/mach-omap2/board-2430sdp-flash.c |  108 ++++++++++++++++++++++++++++++
 arch/arm/mach-omap2/board-2430sdp.c       |    4 +
 arch/arm/mach-omap2/gpmc.c                |    1 
 drivers/mtd/onenand/omap2.c               |  103 +++++++++++++++++-----------
 drivers/mtd/onenand/onenand_base.c        |    2 
 include/asm-arm/arch-omap/gpmc.h          |    7 +
 include/asm-arm/arch-omap/onenand.h       |    1 
 8 files changed, 187 insertions(+), 42 deletions(-)

Index: dev/arch/arm/mach-omap2/Makefile
===================================================================
--- dev.orig/arch/arm/mach-omap2/Makefile
+++ dev/arch/arm/mach-omap2/Makefile
@@ -20,7 +20,8 @@ mmu_mach-objs			:= mmu.o
 # Specific board support
 obj-$(CONFIG_MACH_OMAP_GENERIC)		+= board-generic.o
 obj-$(CONFIG_MACH_OMAP_H4)		+= board-h4.o
-obj-$(CONFIG_MACH_OMAP_2430SDP)		+= board-2430sdp.o
+obj-$(CONFIG_MACH_OMAP_2430SDP)		+= board-2430sdp.o \
+					   board-2430sdp-flash.o
 obj-$(CONFIG_MACH_OMAP_APOLLON)		+= board-apollon.o
 obj-$(CONFIG_MACH_NOKIA_N800)		+= board-n800.o board-n800-flash.o \
 					   board-n800-mmc.o board-n800-bt.o \
Index: dev/arch/arm/mach-omap2/board-2430sdp-flash.c
===================================================================
--- /dev/null
+++ dev/arch/arm/mach-omap2/board-2430sdp-flash.c
@@ -0,0 +1,108 @@
+/*
+ * linux/arch/arm/mach-omap2/board-2430sdp-flash.c
+ *
+ * Copyright (C) 2007 MontaVista Software, Inc. <source@mvista.com>
+ * Author: Kevin Hilman
+ *
+ * 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/platform_device.h>
+#include <asm/mach/flash.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/partitions.h>
+#include <linux/mtd/onenand_regs.h>
+
+#include <asm/io.h>
+#include <asm/arch/onenand.h>
+#include <asm/arch/board.h>
+#include <asm/arch/gpmc.h>
+
+#define ONENAND_MAP 0x20000000
+
+static struct mtd_partition onenand_partitions[] = {
+	{
+		.name		= "(OneNAND)X-Loader",
+		.offset		= 0,
+		.size		= 4*(64*2048),  /* 0-3 blks reserved.
+						   Mandated by ROM code */
+		.mask_flags	= MTD_WRITEABLE	/* force read-only */
+	},
+	{
+		.name		= "(OneNAND)U-Boot",
+		.offset		= MTDPART_OFS_APPEND,
+		.size		=  2*(64*2048),
+		.mask_flags	= MTD_WRITEABLE	/* force read-only */
+	},
+	{
+		.name		= "(OneNAND)U-Boot Environment",
+		.offset		= MTDPART_OFS_APPEND,
+		.size		= 1*(64*2048),
+	},
+	{
+		.name		= "(OneNAND)Kernel",
+		.offset		= MTDPART_OFS_APPEND,
+		.size		= 4*(64*2048),
+	},
+	{
+		.name		= "(OneNAND)File System",
+		.offset		= MTDPART_OFS_APPEND,
+		.size		= MTDPART_SIZ_FULL,
+	},
+};
+
+static struct omap_onenand_platform_data sdp_onenand_data = {
+	.parts	       = onenand_partitions,
+	.nr_parts      = ARRAY_SIZE(onenand_partitions),
+	.dma_channel   = -1,  /* disable DMA in OMAP OneNAND driver */
+};
+
+static struct platform_device sdp_onenand_device = {
+	.name		= "omap2-onenand",
+	.id		= -1,
+	.dev = {
+		.platform_data = &sdp_onenand_data,
+	},
+};
+
+void __init sdp2430_flash_init(void)
+{
+	unsigned long gpmc_base_add, gpmc_cs_base_add;
+	unsigned char cs=0;
+
+	gpmc_base_add = OMAP243X_GPMC_VIRT;
+	while (cs < GPMC_CS_NUM) {
+		int ret = 0;
+
+		/* Each GPMC set for a single CS is at offset 0x30 */
+		gpmc_cs_base_add = (gpmc_base_add + GPMC_CONFIG1_0 + (cs*0x30));
+
+		/* xloader/Uboot would have programmed the oneNAND
+		 * base address for us This is a ugly hack. The proper
+		 * way of doing this is to pass the setup of u-boot up
+		 * to kernel using kernel params - something on the
+		 * lines of machineID. Check if oneNAND is
+		 * configured */
+		ret = __raw_readl(gpmc_cs_base_add + GPMC_CS_CONFIG7);
+		if ((ret & 0x3F) == (ONENAND_MAP >> 24)) {
+			/* Found it!! */
+			break;
+		}
+		cs++;
+	}
+	if (cs >= GPMC_CS_NUM) {
+		printk("OneNAND: Unable to find oneNAND configuration in GPMC "
+		       " - not registering.\n");
+		return;
+	}
+
+	sdp_onenand_data.cs = cs;
+
+	if (platform_device_register(&sdp_onenand_device) < 0) {
+		printk(KERN_ERR "Unable to register OneNAND device\n");
+		return;
+	}
+}
Index: dev/arch/arm/mach-omap2/board-2430sdp.c
===================================================================
--- dev.orig/arch/arm/mach-omap2/board-2430sdp.c
+++ dev/arch/arm/mach-omap2/board-2430sdp.c
@@ -192,12 +192,16 @@ static struct omap_board_config_kernel s
 	{OMAP_TAG_UART, &sdp2430_uart_config},
 };
 
+extern void __init sdp2430_flash_init(void);
+
 static void __init omap_2430sdp_init(void)
 {
 	platform_add_devices(sdp2430_devices, ARRAY_SIZE(sdp2430_devices));
 	omap_board_config = sdp2430_config;
 	omap_board_config_size = ARRAY_SIZE(sdp2430_config);
 	omap_serial_init();
+
+	sdp2430_flash_init();
 }
 
 static void __init omap_2430sdp_map_io(void)
Index: dev/arch/arm/mach-omap2/gpmc.c
===================================================================
--- dev.orig/arch/arm/mach-omap2/gpmc.c
+++ dev/arch/arm/mach-omap2/gpmc.c
@@ -51,7 +51,6 @@
 #define GPMC_CS0		0x60
 #define GPMC_CS_SIZE		0x30
 
-#define GPMC_CS_NUM		8
 #define GPMC_MEM_START		0x00000000
 #define GPMC_MEM_END		0x3FFFFFFF
 #define BOOT_ROM_SPACE		0x100000	/* 1MB */
Index: dev/drivers/mtd/onenand/omap2.c
===================================================================
--- dev.orig/drivers/mtd/onenand/omap2.c
+++ dev/drivers/mtd/onenand/omap2.c
@@ -80,8 +80,7 @@ static void omap2_onenand_dma_cb(int lch
 	complete(&info->dma_done);
 }
 
-static irqreturn_t omap2_onenand_interrupt(int irq, void *dev_id,
-					   struct pt_regs *regs)
+static irqreturn_t omap2_onenand_interrupt(int irq, void *dev_id)
 {
 	struct omap2_onenand *info = dev_id;
 
@@ -111,11 +110,15 @@ static int omap2_onenand_wait(struct mtd
 		omap2_onenand_writew(syscfg, info->onenand.base + ONENAND_REG_SYS_CFG1);
 
 		INIT_COMPLETION(info->irq_done);
-		result = omap_get_gpio_datain(info->gpio_irq);
-		if (result == -1) {
-			ctrl = omap2_onenand_readw(info->onenand.base + ONENAND_REG_CTRL_STATUS);
-			printk(KERN_ERR "onenand_wait: gpio error, state = %d, ctrl = 0x%04x\n", state, ctrl);
-			return -EIO;
+		if (info->gpio_irq) {
+			result = omap_get_gpio_datain(info->gpio_irq);
+			if (result == -1) {
+				ctrl = omap2_onenand_readw(info->onenand.base + ONENAND_REG_CTRL_STATUS);
+				printk(KERN_ERR "onenand_wait: gpio error, state = %d, ctrl = 0x%04x\n", state, ctrl);
+				return -EIO;
+			}
+		} else {
+			result = 0;
 		}
 		if (result == 0) {
 			int retry_cnt = 0;
@@ -317,6 +320,11 @@ static int __devinit omap2_onenand_probe
 	init_completion(&info->dma_done);
 	info->gpmc_cs = pdata->cs;
 	info->gpio_irq = pdata->gpio_irq;
+	info->dma_channel = pdata->dma_channel;
+	if (info->dma_channel < 0) {
+		/* if -1, don't use DMA */
+		info->gpio_irq = 0;
+	}
 
 	r = gpmc_cs_request(info->gpmc_cs, ONENAND_IO_SIZE, &info->phys_base);
 	if (r < 0) {
@@ -345,31 +353,41 @@ static int __devinit omap2_onenand_probe
 		}
         }
 
-	if ((r = omap_request_gpio(info->gpio_irq)) < 0) {
-		dev_err(&pdev->dev,  "Failed to request GPIO%d for OneNAND\n",
-		        info->gpio_irq);
-		goto err_iounmap;
-	}
-	omap_set_gpio_direction(info->gpio_irq, 1);
-
-	if ((r = request_irq(OMAP_GPIO_IRQ(info->gpio_irq),
-			     omap2_onenand_interrupt, SA_TRIGGER_RISING,
-			     pdev->dev.driver->name, info)) < 0)
-		goto err_release_gpio;
-
-	r = omap_request_dma(0, pdev->dev.driver->name,
-			     omap2_onenand_dma_cb, (void *) info,
-			     &info->dma_channel);
-	if (r == 0) {
-		omap_set_dma_write_mode(info->dma_channel, OMAP_DMA_WRITE_NON_POSTED);
-		omap_set_dma_src_data_pack(info->dma_channel, 1);
-		omap_set_dma_src_burst_mode(info->dma_channel, OMAP_DMA_DATA_BURST_8);
-		omap_set_dma_dest_data_pack(info->dma_channel, 1);
-		omap_set_dma_dest_burst_mode(info->dma_channel, OMAP_DMA_DATA_BURST_8);
-	} else {
-		dev_info(&pdev->dev,
-			 "failed to allocate DMA for OneNAND, using PIO instead\n");
-		info->dma_channel = -1;
+	if (info->gpio_irq) {
+		if ((r = omap_request_gpio(info->gpio_irq)) < 0) {
+			dev_err(&pdev->dev,
+				"Failed to request GPIO%d for OneNAND\n",
+				info->gpio_irq);
+			goto err_iounmap;
+		}
+		omap_set_gpio_direction(info->gpio_irq, 1);
+
+		if ((r = request_irq(OMAP_GPIO_IRQ(info->gpio_irq),
+				     omap2_onenand_interrupt,
+				     SA_TRIGGER_RISING,
+				     pdev->dev.driver->name, info)) < 0)
+			goto err_release_gpio;
+	}
+
+	if (info->dma_channel >= 0) {
+		r = omap_request_dma(0, pdev->dev.driver->name,
+				     omap2_onenand_dma_cb, (void *) info,
+				     &info->dma_channel);
+		if (r == 0) {
+			omap_set_dma_write_mode(info->dma_channel,
+						OMAP_DMA_WRITE_NON_POSTED);
+			omap_set_dma_src_data_pack(info->dma_channel, 1);
+			omap_set_dma_src_burst_mode(info->dma_channel,
+						    OMAP_DMA_DATA_BURST_8);
+			omap_set_dma_dest_data_pack(info->dma_channel, 1);
+			omap_set_dma_dest_burst_mode(info->dma_channel,
+						     OMAP_DMA_DATA_BURST_8);
+		} else {
+			dev_info(&pdev->dev,
+				 "failed to allocate DMA for OneNAND, "
+				 "using PIO instead\n");
+			info->dma_channel = -1;
+		}
 	}
 
 	dev_info(&pdev->dev, "initializing on CS%d, phys base 0x%08lx, virtual base %p\n",
@@ -379,9 +397,12 @@ static int __devinit omap2_onenand_probe
 	info->mtd.name = pdev->dev.bus_id;
 	info->mtd.priv = &info->onenand;
 	info->mtd.owner = THIS_MODULE;
-	info->onenand.wait = omap2_onenand_wait;
-	info->onenand.read_bufferram = omap2_onenand_read_bufferram;
-	info->onenand.write_bufferram = omap2_onenand_write_bufferram;
+
+	if (info->dma_channel >= 0) {
+		info->onenand.wait = omap2_onenand_wait;
+		info->onenand.read_bufferram = omap2_onenand_read_bufferram;
+		info->onenand.write_bufferram = omap2_onenand_write_bufferram;
+	}
 
 	if ((r = onenand_scan(&info->mtd, 1)) < 0)
 		goto err_release_dma;
@@ -404,9 +425,11 @@ err_release_onenand:
 err_release_dma:
 	if (info->dma_channel != -1)
 		omap_free_dma(info->dma_channel);
-	free_irq(OMAP_GPIO_IRQ(info->gpio_irq), info);
+	if (info->gpio_irq)
+		free_irq(OMAP_GPIO_IRQ(info->gpio_irq), info);
 err_release_gpio:
-	omap_free_gpio(info->gpio_irq);
+	if (info->gpio_irq)
+		omap_free_gpio(info->gpio_irq);
 err_iounmap:
 	iounmap(info->onenand.base);
 err_release_mem_region:
@@ -439,8 +462,10 @@ static int __devexit omap2_onenand_remov
 		omap_free_dma(info->dma_channel);
 	omap2_onenand_shutdown(pdev);
 	platform_set_drvdata(pdev, NULL);
-	free_irq(OMAP_GPIO_IRQ(info->gpio_irq), info);
-	omap_free_gpio(info->gpio_irq);
+	if (info->gpio_irq) {
+		free_irq(OMAP_GPIO_IRQ(info->gpio_irq), info);
+		omap_free_gpio(info->gpio_irq);
+	}
 	iounmap(info->onenand.base);
 	release_mem_region(info->phys_base, ONENAND_IO_SIZE);
 	kfree(info);
Index: dev/drivers/mtd/onenand/onenand_base.c
===================================================================
--- dev.orig/drivers/mtd/onenand/onenand_base.c
+++ dev/drivers/mtd/onenand/onenand_base.c
@@ -39,7 +39,7 @@ static struct nand_ecclayout onenand_oob
 		},
 	.oobfree	= {
 		{2, 3}, {14, 2}, {18, 3}, {30, 2},
-		{34, 3}, {46, 2}, {50, 3}, {62, 2}
+		{34, 3}, {46, 2}, {50, 3}, /* {62, 2} */
 	}
 };
 
Index: dev/include/asm-arm/arch-omap/gpmc.h
===================================================================
--- dev.orig/include/asm-arm/arch-omap/gpmc.h
+++ dev/include/asm-arm/arch-omap/gpmc.h
@@ -11,6 +11,9 @@
 #ifndef __OMAP2_GPMC_H
 #define __OMAP2_GPMC_H
 
+/* Maximum Number of Chip Selects */
+#define GPMC_CS_NUM		8
+
 #define GPMC_CS_CONFIG1		0x00
 #define GPMC_CS_CONFIG2		0x04
 #define GPMC_CS_CONFIG3		0x08
@@ -22,6 +25,10 @@
 #define GPMC_CS_NAND_ADDRESS	0x20
 #define GPMC_CS_NAND_DATA	0x24
 
+#define GPMC_CONFIG		0x50
+#define GPMC_STATUS		0x54
+#define GPMC_CONFIG1_0		0x60
+
 #define GPMC_CONFIG1_WRAPBURST_SUPP     (1 << 31)
 #define GPMC_CONFIG1_READMULTIPLE_SUPP  (1 << 30)
 #define GPMC_CONFIG1_READTYPE_ASYNC     (0 << 29)
Index: dev/include/asm-arm/arch-omap/onenand.h
===================================================================
--- dev.orig/include/asm-arm/arch-omap/onenand.h
+++ dev/include/asm-arm/arch-omap/onenand.h
@@ -17,4 +17,5 @@ struct omap_onenand_platform_data {
 	struct mtd_partition	*parts;
 	int			nr_parts;
 	int                     (*onenand_setup)(void __iomem *);
+	int			dma_channel;
 };
--

RE: [RFC/PATCH] ARM: OMAP: OneNAND support for 2430SDP

[Jalori, Mohit]

> -----Original Message-----
> From: linux-omap-open-source-bounces@linux.omap.com
[mailto:linux-omap-
> open-source-bounces@linux.omap.com] On Behalf Of Kevin Hilman
> Sent: Tuesday, March 20, 2007 7:41 PM
> To: linux-omap-open-source@linux.omap.com
> Subject: [RFC/PATCH] ARM: OMAP: OneNAND support for 2430SDP
> 
> Adds platform device for use of existing OMAP2 driver.  Also, the
> following changes are made to the existing driver:
> 
> - add option for not using DMA (by passing dma_channel = -1)
> - removes obsoleted pt_regs arg from interrupt handler
> 
> RFC: Notice the change to onenand_base.c which comments out the last
>      OOB chunk.  Without this, it would not work.  Previous drivers
>      for this board such as the 2.6.10-based MV kernel have this
>      same change.  Anyone know what is going on here?
> 

We got this change from MV in the patch he sent to fix an issue we were
seeing with mounting an empty jffs2 partition. Here are the comments
that came in with it

" The oobfree list in the MTD OneNAND driver is of length 8.  This
exceeds the maximum length of 7, because the eighth entry must be used
as a null terminator.  The absence of a terminator causes a variety of
problems in various places (especially JFFS2 and YAFFS2) where the
oobfree list is scanned.  The fix is to do without the eighth entry. 

This fix is only of local interest because the handling of oobinfo has
been completely redesigned as of 2.6.17-rc6. "


>From the last line we should not need this for later kernel versions.

Regards
Mohit

RE: [RFC/PATCH] ARM: OMAP: OneNAND support for 2430SDP

[Kevin Hilman]

On Wed, 2007-03-21 at 14:15 -0500, Jalori, Mohit wrote:

> > RFC: Notice the change to onenand_base.c which comments out the last
> >      OOB chunk.  Without this, it would not work.  Previous drivers
> >      for this board such as the 2.6.10-based MV kernel have this
> >      same change.  Anyone know what is going on here?
> > 
> 
> We got this change from MV in the patch he sent to fix an issue we were
> seeing with mounting an empty jffs2 partition. Here are the comments
> that came in with it
> 
> " The oobfree list in the MTD OneNAND driver is of length 8.  This
> exceeds the maximum length of 7, because the eighth entry must be used
> as a null terminator.  The absence of a terminator causes a variety of
> problems in various places (especially JFFS2 and YAFFS2) where the
> oobfree list is scanned.  The fix is to do without the eighth entry. 
> 
> This fix is only of local interest because the handling of oobinfo has
> been completely redesigned as of 2.6.17-rc6. "

This problem still exists in the current OMAP tree (2.6.21-rc4), so the
MTD driver still needs some work.

I will work to get a fix for this submitted upstream.  In the mean-time,
my patch is required for OneNAND to work in the current git tree.

Kevin

RE: [RFC/PATCH] ARM: OMAP: OneNAND support for 2430SDP

[Kyungmin Park]

> 
> > > RFC: Notice the change to onenand_base.c which comments 
> out the last
> > >      OOB chunk.  Without this, it would not work.  
> Previous drivers
> > >      for this board such as the 2.6.10-based MV kernel have this
> > >      same change.  Anyone know what is going on here?
> > > 
> > 
> > We got this change from MV in the patch he sent to fix an 
> issue we were
> > seeing with mounting an empty jffs2 partition. Here are the comments
> > that came in with it
> > 
> > " The oobfree list in the MTD OneNAND driver is of length 8.  This
> > exceeds the maximum length of 7, because the eighth entry 
> must be used
> > as a null terminator.  The absence of a terminator causes a 
> variety of
> > problems in various places (especially JFFS2 and YAFFS2) where the
> > oobfree list is scanned.  The fix is to do without the 
> eighth entry. 

Colud you point out the source code line where used the eightn entry used as a
null terminator?

In linux-2.6.15-omap they also allocate 8 oobfree. 

struct nand_oobinfo {
        uint32_t useecc;
        uint32_t eccbytes;
        uint32_t oobfree[8][2];
        uint32_t eccpos[32];
};

> > 
> > This fix is only of local interest because the handling of 
> oobinfo has
> > been completely redesigned as of 2.6.17-rc6. "
> 
> This problem still exists in the current OMAP tree 
> (2.6.21-rc4), so the
> MTD driver still needs some work.

It's some strange. I'm always working with omap tree with OneNAND support.
Of course it's working well with latest omap tree. The apollon board can boot
and use the jffs2 on OneNAND.

There are two cases using OOB. 
1. OOB_AUTO
2. OOB_PLACE
The recent change in JFFS2 which uses the OOB_AUTO.

In case 1, it's only used the first oobfree in JFFS2. So they don't use the
last oobfree
In case 2, it uses the full oobfree. but actually it used 12 bytes in jffs2. it
means it used 5 oobfree

> 
> I will work to get a fix for this submitted upstream.  In the 
> mean-time,
> my patch is required for OneNAND to work in the current git tree.

Could you send the debug message or error mesage when it is used without
oobfree patch?

Thank you,
Kyungmin Park

Re: [RFC/PATCH] ARM: OMAP: OneNAND support for 2430SDP

[Kevin Hilman]

Kyungmin Park wrote:
>>>> RFC: Notice the change to onenand_base.c which comments 
>> out the last
>>>>      OOB chunk.  Without this, it would not work.  
>> Previous drivers
>>>>      for this board such as the 2.6.10-based MV kernel have this
>>>>      same change.  Anyone know what is going on here?
>>>>
>>> We got this change from MV in the patch he sent to fix an 
>> issue we were
>>> seeing with mounting an empty jffs2 partition. Here are the comments
>>> that came in with it
>>>
>>> " The oobfree list in the MTD OneNAND driver is of length 8.  This
>>> exceeds the maximum length of 7, because the eighth entry 
>> must be used
>>> as a null terminator.  The absence of a terminator causes a 
>> variety of
>>> problems in various places (especially JFFS2 and YAFFS2) where the
>>> oobfree list is scanned.  The fix is to do without the 
>> eighth entry. 
> 
> Colud you point out the source code line where used the eightn entry used as a
> null terminator?
> 

There are several loops in both the NAND and the OneNAND code where the
oobfree list is walked and the loop only terminates when it finds a NULL
entry (specifically, one with a zero length.)

For example, look in onenand_fill_auto_oob() where you have a couple
loops like this:

    for (free = this->ecclayout->oobfree; free->length; ++free) {
    }

In the case where there is 8 non-zero entries in the oobfree list, this
loop will walk off the end of the list before it terminates, and having
undefined results depending on what is after the oobfree list in memory.

Instead, these loops should probably loop over MTD_MAX_OOBFREE_ENTRIES,
and break out early if they find a zero-length entry.

I'm not sure why this works for you.  For me, here's a simple way to
trigger the bug using the OneNAND device on the OMAP2430SDP:

# flash_eraseall /dev/mtd8
Erasing 128 Kibyte @ 7e80000 -- 99 % complete.
# mount -t jffs2 /dev/mtdblock8 /media/onenand
Cowardly refusing to erase blocks on filesystem with no valid JFFS2 nodes
empty_blocks 0, bad_blocks 0, c->nr_blocks 1013
mount: /dev/mtdblock8: can't read superblock


Kevin

RE: [RFC/PATCH] ARM: OMAP: OneNAND support for 2430SDP

[Kyungmin Park]

 

> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@mvista.com] 
> Sent: Thursday, March 22, 2007 12:14 PM
> To: kmpark@infradead.org
> Cc: 'Jalori, Mohit'; linux-omap-open-source@linux.omap.com
> Subject: Re: [RFC/PATCH] ARM: OMAP: OneNAND support for 2430SDP
> 
> Kyungmin Park wrote:
> >>>> RFC: Notice the change to onenand_base.c which comments 
> >> out the last
> >>>>      OOB chunk.  Without this, it would not work.  
> >> Previous drivers
> >>>>      for this board such as the 2.6.10-based MV kernel have this
> >>>>      same change.  Anyone know what is going on here?
> >>>>
> >>> We got this change from MV in the patch he sent to fix an 
> >> issue we were
> >>> seeing with mounting an empty jffs2 partition. Here are 
> the comments
> >>> that came in with it
> >>>
> >>> " The oobfree list in the MTD OneNAND driver is of length 8.  This
> >>> exceeds the maximum length of 7, because the eighth entry 
> >> must be used
> >>> as a null terminator.  The absence of a terminator causes a 
> >> variety of
> >>> problems in various places (especially JFFS2 and YAFFS2) where the
> >>> oobfree list is scanned.  The fix is to do without the 
> >> eighth entry. 
> > 
> > Colud you point out the source code line where used the 
> eightn entry used as a
> > null terminator?
> > 
> 
> There are several loops in both the NAND and the OneNAND code 
> where the
> oobfree list is walked and the loop only terminates when it 
> finds a NULL
> entry (specifically, one with a zero length.)
> 
> For example, look in onenand_fill_auto_oob() where you have a couple
> loops like this:
> 
>     for (free = this->ecclayout->oobfree; free->length; ++free) {
>     }
> 
> In the case where there is 8 non-zero entries in the oobfree 
> list, this
> loop will walk off the end of the list before it terminates, 
> and having
> undefined results depending on what is after the oobfree list 
> in memory.
> 
> Instead, these loops should probably loop over 
> MTD_MAX_OOBFREE_ENTRIES,
> and break out early if they find a zero-length entry.

Yes, you're right. It met the same problem in the previous mail [1]. 
Could you apply patch [2] and test again. it will be work.

Anyway I will fix it to check array length.

But I wonder it is only affected with OOB_AUTO. however the linux 2.6.10 kernel
don't use OOB_AUTO. but you said it has same problem.
Is there another point?

> 
> I'm not sure why this works for you.  For me, here's a simple way to
> trigger the bug using the OneNAND device on the OMAP2430SDP:


> 
> # flash_eraseall /dev/mtd8
> Erasing 128 Kibyte @ 7e80000 -- 99 % complete.
> # mount -t jffs2 /dev/mtdblock8 /media/onenand
> Cowardly refusing to erase blocks on filesystem with no valid 
> JFFS2 nodes
> empty_blocks 0, bad_blocks 0, c->nr_blocks 1013
> mount: /dev/mtdblock8: can't read superblock
> 

Thank you,
Kyungmin Park

1. http://lists.infradead.org/pipermail/linux-mtd/2007-February/017418.html
2. http://git.infradead.org/?p=users/kmpark/onenand-mtd-
2.6.git;a=commitdiff;h=5bc399e9ef430efd5725b66aa2ad7ad2d81e372b;hp=81280d5879761
f90b3a341d52371d03998730d8e

RE: [RFC/PATCH] ARM: OMAP: OneNAND support for 2430SDP

[Kyungmin Park]

> > 
> > Kyungmin Park wrote:
> > >>>> RFC: Notice the change to onenand_base.c which comments 
> > >> out the last
> > >>>>      OOB chunk.  Without this, it would not work.  
> > >> Previous drivers
> > >>>>      for this board such as the 2.6.10-based MV kernel 
> have this
> > >>>>      same change.  Anyone know what is going on here?
> > >>>>
> > >>> We got this change from MV in the patch he sent to fix an 
> > >> issue we were
> > >>> seeing with mounting an empty jffs2 partition. Here are 
> > the comments
> > >>> that came in with it
> > >>>
> > >>> " The oobfree list in the MTD OneNAND driver is of 
> length 8.  This
> > >>> exceeds the maximum length of 7, because the eighth entry 
> > >> must be used
> > >>> as a null terminator.  The absence of a terminator causes a 
> > >> variety of
> > >>> problems in various places (especially JFFS2 and 
> YAFFS2) where the
> > >>> oobfree list is scanned.  The fix is to do without the 
> > >> eighth entry. 
> > > 
> > > Colud you point out the source code line where used the 
> > eightn entry used as a
> > > null terminator?
> > > 
> > 
> > There are several loops in both the NAND and the OneNAND code 
> > where the
> > oobfree list is walked and the loop only terminates when it 
> > finds a NULL
> > entry (specifically, one with a zero length.)
> > 
> > For example, look in onenand_fill_auto_oob() where you have a couple
> > loops like this:
> > 
> >     for (free = this->ecclayout->oobfree; free->length; ++free) {
> >     }
> > 
> > In the case where there is 8 non-zero entries in the oobfree 
> > list, this
> > loop will walk off the end of the list before it terminates, 
> > and having
> > undefined results depending on what is after the oobfree list 
> > in memory.
> > 
> > Instead, these loops should probably loop over 
> > MTD_MAX_OOBFREE_ENTRIES,
> > and break out early if they find a zero-length entry.
> 
> Yes, you're right. It met the same problem in the previous mail [1]. 
> Could you apply patch [2] and test again. it will be work.
> 
> Anyway I will fix it to check array length.


Here's patch. Yes I know it's maybe wordwrapped. Sorry :)

diff --git a/drivers/mtd/onenand/onenand_base.c
b/drivers/mtd/onenand/onenand_base.c
index 9e14a26..2a88a95 100644
--- a/drivers/mtd/onenand/onenand_base.c
+++ b/drivers/mtd/onenand/onenand_base.c
@@ -836,9 +836,11 @@ static int onenand_transfer_auto_oob(struct mtd_info *mtd,
uint8_t *buf, int col
        int readcol = column;
        int readend = column + thislen;
        int lastgap = 0;
+       unsigned int i;
        uint8_t *oob_buf = this->oob_buf;

-       for (free = this->ecclayout->oobfree; free->length; ++free) {
+       free = this->ecclayout->oobfree;
+       for (i = 0; i < MTD_MAX_OOBFREE_ENTRIES && free->length; i++, free++) {
                if (readcol >= lastgap)
                        readcol += free->offset - lastgap;
                if (readend >= lastgap)
@@ -846,7 +848,8 @@ static int onenand_transfer_auto_oob(struct mtd_info *mtd,
uint8_t *buf, int col
                lastgap = free->offset + free->length;
        }
        this->read_bufferram(mtd, ONENAND_SPARERAM, oob_buf, 0, mtd->oobsize);
-       for (free = this->ecclayout->oobfree; free->length; ++free) {
+       free = this->ecclayout->oobfree;
+       for (i = 0; i < MTD_MAX_OOBFREE_ENTRIES && free->length; i++, free++) {
                int free_end = free->offset + free->length;
                if (free->offset < readend && free_end > readcol) {
                        int st = max_t(int,free->offset,readcol);
@@ -1280,15 +1283,18 @@ static int onenand_fill_auto_oob(struct mtd_info *mtd,
u_char *oob_buf,
        int writecol = column;
        int writeend = column + thislen;
        int lastgap = 0;
+       unsigned int i;

-       for (free = this->ecclayout->oobfree; free->length; ++free) {
+       free = this->ecclayout->oobfree;
+       for (i = 0; i < MTD_MAX_OOBFREE_ENTRIES && free->length; i++, free++) {
                if (writecol >= lastgap)
                        writecol += free->offset - lastgap;
                if (writeend >= lastgap)
                        writeend += free->offset - lastgap;
                lastgap = free->offset + free->length;
        }
-       for (free = this->ecclayout->oobfree; free->length; ++free) {
+       free = this->ecclayout->oobfree;
+       for (i = 0; i < MTD_MAX_OOBFREE_ENTRIES && free->length; i++, free++) {
                int free_end = free->offset + free->length;
                if (free->offset < writeend && free_end > writecol) {
                        int st = max_t(int,free->offset,writecol);

RE: [RFC/PATCH] ARM: OMAP: OneNAND support for 2430SDP

[Kevin Hilman]

On Thu, 2007-03-22 at 14:02 +0900, Kyungmin Park wrote:
> > > 
> > > Kyungmin Park wrote:
> > > >>>> RFC: Notice the change to onenand_base.c which comments 
> > > >> out the last
> > > >>>>      OOB chunk.  Without this, it would not work.  
> > > >> Previous drivers
> > > >>>>      for this board such as the 2.6.10-based MV kernel 
> > have this
> > > >>>>      same change.  Anyone know what is going on here?
> > > >>>>
> > > >>> We got this change from MV in the patch he sent to fix an 
> > > >> issue we were
> > > >>> seeing with mounting an empty jffs2 partition. Here are 
> > > the comments
> > > >>> that came in with it
> > > >>>
> > > >>> " The oobfree list in the MTD OneNAND driver is of 
> > length 8.  This
> > > >>> exceeds the maximum length of 7, because the eighth entry 
> > > >> must be used
> > > >>> as a null terminator.  The absence of a terminator causes a 
> > > >> variety of
> > > >>> problems in various places (especially JFFS2 and 
> > YAFFS2) where the
> > > >>> oobfree list is scanned.  The fix is to do without the 
> > > >> eighth entry. 
> > > > 
> > > > Colud you point out the source code line where used the 
> > > eightn entry used as a
> > > > null terminator?
> > > > 
> > > 
> > > There are several loops in both the NAND and the OneNAND code 
> > > where the
> > > oobfree list is walked and the loop only terminates when it 
> > > finds a NULL
> > > entry (specifically, one with a zero length.)
> > > 
> > > For example, look in onenand_fill_auto_oob() where you have a couple
> > > loops like this:
> > > 
> > >     for (free = this->ecclayout->oobfree; free->length; ++free) {
> > >     }
> > > 
> > > In the case where there is 8 non-zero entries in the oobfree 
> > > list, this
> > > loop will walk off the end of the list before it terminates, 
> > > and having
> > > undefined results depending on what is after the oobfree list 
> > > in memory.
> > > 
> > > Instead, these loops should probably loop over 
> > > MTD_MAX_OOBFREE_ENTRIES,
> > > and break out early if they find a zero-length entry.
> > 
> > Yes, you're right. It met the same problem in the previous mail [1]. 
> > Could you apply patch [2] and test again. it will be work.
> > 
> > Anyway I will fix it to check array length.
> 
> 
> Here's patch. Yes I know it's maybe wordwrapped. Sorry :)

You missed the loop in onenand_scan() which walks the freelist.  Patch
below covers all the cases, and was tested to work on 2430.

Note that you should raise this on linux-mtd as there are similar loops
in the drivers/mtd/nand* code.  The problem has never come up because
all the drivers have less than 8 oobfree chunks defined.

Kevin

Index: dev/drivers/mtd/onenand/onenand_base.c
===================================================================
--- dev.orig/drivers/mtd/onenand/onenand_base.c
+++ dev/drivers/mtd/onenand/onenand_base.c
@@ -837,8 +837,10 @@ static int onenand_transfer_auto_oob(str
 	int readend = column + thislen;
 	int lastgap = 0;
 	uint8_t *oob_buf = this->oob_buf;
+	unsigned int i;
 
-	for (free = this->ecclayout->oobfree; free->length; ++free) {
+	free = this->ecclayout->oobfree;
+	for (i = 0; i < MTD_MAX_OOBFREE_ENTRIES && free->length; i++, free++)
{
 		if (readcol >= lastgap)
 			readcol += free->offset - lastgap;
 		if (readend >= lastgap)
@@ -846,7 +848,8 @@ static int onenand_transfer_auto_oob(str
 		lastgap = free->offset + free->length;
 	}
 	this->read_bufferram(mtd, ONENAND_SPARERAM, oob_buf, 0, mtd->oobsize);
-	for (free = this->ecclayout->oobfree; free->length; ++free) {
+	free = this->ecclayout->oobfree;
+	for (i = 0; i < MTD_MAX_OOBFREE_ENTRIES && free->length; i++, free++)
{
 		int free_end = free->offset + free->length;
 		if (free->offset < readend && free_end > readcol) {
 			int st = max_t(int,free->offset,readcol);
@@ -1280,15 +1283,18 @@ static int onenand_fill_auto_oob(struct 
 	int writecol = column;
 	int writeend = column + thislen;
 	int lastgap = 0;
+	unsigned int i;
 
-	for (free = this->ecclayout->oobfree; free->length; ++free) {
+	free = this->ecclayout->oobfree;
+	for (i = 0; i < MTD_MAX_OOBFREE_ENTRIES && free->length; i++, free++)
{
 		if (writecol >= lastgap)
 			writecol += free->offset - lastgap;
 		if (writeend >= lastgap)
 			writeend += free->offset - lastgap;
 		lastgap = free->offset + free->length;
 	}
-	for (free = this->ecclayout->oobfree; free->length; ++free) {
+	free = this->ecclayout->oobfree;
+	for (i = 0; i < MTD_MAX_OOBFREE_ENTRIES && free->length; i++, free++)
{
 		int free_end = free->offset + free->length;
 		if (free->offset < writeend && free_end > writecol) {
 			int st = max_t(int,free->offset,writecol);
@@ -2386,7 +2392,9 @@ int onenand_scan(struct mtd_info *mtd, i
 	 * the out of band area
 	 */
 	this->ecclayout->oobavail = 0;
-	for (i = 0; this->ecclayout->oobfree[i].length; i++)
+	for (i = 0;
+	     i < MTD_MAX_OOBFREE_ENTRIES &&
this->ecclayout->oobfree[i].length;
+	     i++)
 		this->ecclayout->oobavail +=
 			this->ecclayout->oobfree[i].length;
 	mtd->oobavail = this->ecclayout->oobavail;

RE: [RFC/PATCH] ARM: OMAP: OneNAND support for 2430SDP

[Kyungmin Park]

> > > > 
> > > > Kyungmin Park wrote:
> > > > >>>> RFC: Notice the change to onenand_base.c which comments 
> > > > >> out the last
> > > > >>>>      OOB chunk.  Without this, it would not work.  
> > > > >> Previous drivers
> > > > >>>>      for this board such as the 2.6.10-based MV kernel 
> > > have this
> > > > >>>>      same change.  Anyone know what is going on here?
> > > > >>>>
> > > > >>> We got this change from MV in the patch he sent to fix an 
> > > > >> issue we were
> > > > >>> seeing with mounting an empty jffs2 partition. Here are 
> > > > the comments
> > > > >>> that came in with it
> > > > >>>
> > > > >>> " The oobfree list in the MTD OneNAND driver is of 
> > > length 8.  This
> > > > >>> exceeds the maximum length of 7, because the eighth entry 
> > > > >> must be used
> > > > >>> as a null terminator.  The absence of a terminator causes a 
> > > > >> variety of
> > > > >>> problems in various places (especially JFFS2 and 
> > > YAFFS2) where the
> > > > >>> oobfree list is scanned.  The fix is to do without the 
> > > > >> eighth entry. 
> > > > > 
> > > > > Colud you point out the source code line where used the 
> > > > eightn entry used as a
> > > > > null terminator?
> > > > > 
> > > > 
> > > > There are several loops in both the NAND and the OneNAND code 
> > > > where the
> > > > oobfree list is walked and the loop only terminates when it 
> > > > finds a NULL
> > > > entry (specifically, one with a zero length.)
> > > > 
> > > > For example, look in onenand_fill_auto_oob() where you 
> have a couple
> > > > loops like this:
> > > > 
> > > >     for (free = this->ecclayout->oobfree; free->length; 
> ++free) {
> > > >     }
> > > > 
> > > > In the case where there is 8 non-zero entries in the oobfree 
> > > > list, this
> > > > loop will walk off the end of the list before it terminates, 
> > > > and having
> > > > undefined results depending on what is after the oobfree list 
> > > > in memory.
> > > > 
> > > > Instead, these loops should probably loop over 
> > > > MTD_MAX_OOBFREE_ENTRIES,
> > > > and break out early if they find a zero-length entry.
> > > 
> > > Yes, you're right. It met the same problem in the 
> previous mail [1]. 
> > > Could you apply patch [2] and test again. it will be work.
> > > 
> > > Anyway I will fix it to check array length.
> > 
> > 
> > Here's patch. Yes I know it's maybe wordwrapped. Sorry :)
> 
> You missed the loop in onenand_scan() which walks the freelist.  Patch
> below covers all the cases, and was tested to work on 2430.

Ah yes, thank you for point out.
Then is it OK to work on 2430?

> 
> Note that you should raise this on linux-mtd as there are 
> similar loops
> in the drivers/mtd/nand* code.  The problem has never come up because
> all the drivers have less than 8 oobfree chunks defined.
> 

I commit it to OneNAND MTD tree.

http://git.infradead.org/?p=users/kmpark/onenand-mtd-
2.6.git;a=commitdiff;h=f8405f395dc8a53426ba4ed629a90955b9a46ade

Frankly why do we calculate the oobavail variable? It's fixed. is it?

I prepared to patch moving from data section to rodata section by make static
const.

Thank you,
Kyungmin Park

Re: [RFC/PATCH] ARM: OMAP: OneNAND support for 2430SDP

[Tony Lindgren]

* Kevin Hilman <khilman@mvista.com> [070321 00:49]:
> Adds platform device for use of existing OMAP2 driver.  Also, the
> following changes are made to the existing driver:
> 
> - add option for not using DMA (by passing dma_channel = -1)
> - removes obsoleted pt_regs arg from interrupt handler
> 
> RFC: Notice the change to onenand_base.c which comments out the last
>      OOB chunk.  Without this, it would not work.  Previous drivers
>      for this board such as the 2.6.10-based MV kernel have this
>      same change.  Anyone know what is going on here?

Can you please refresh this one and split it into two patches:

>  arch/arm/mach-omap2/Makefile              |    3 
>  arch/arm/mach-omap2/board-2430sdp-flash.c |  108 ++++++++++++++++++++++++++++++
>  arch/arm/mach-omap2/board-2430sdp.c       |    4 +
>  arch/arm/mach-omap2/gpmc.c                |    1 
>  include/asm-arm/arch-omap/gpmc.h          |    7 +
>  include/asm-arm/arch-omap/onenand.h       |    1 

The above could be one patch ready for omap-upstream queue.

>  drivers/mtd/onenand/omap2.c               |  103 +++++++++++++++++-----------
>  drivers/mtd/onenand/onenand_base.c        |    2 

This should be separate and sent to MTD list.

Regards,

Tony

Re: [RFC/PATCH] ARM: OMAP: OneNAND support for 2430SDP

[Kevin Hilman]

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

On Thu, 2007-04-26 at 17:35 +0000, Tony Lindgren wrote:
> * Kevin Hilman <khilman@mvista.com> [070321 00:49]:
> > Adds platform device for use of existing OMAP2 driver.  Also, the
> > following changes are made to the existing driver:
> > 
> > - add option for not using DMA (by passing dma_channel = -1)
> > - removes obsoleted pt_regs arg from interrupt handler
> > 
> > RFC: Notice the change to onenand_base.c which comments out the last
> >      OOB chunk.  Without this, it would not work.  Previous drivers
> >      for this board such as the 2.6.10-based MV kernel have this
> >      same change.  Anyone know what is going on here?
> 
> Can you please refresh this one and split it into two patches:
> 
> >  arch/arm/mach-omap2/Makefile              |    3 
> >  arch/arm/mach-omap2/board-2430sdp-flash.c |  108 ++++++++++++++++++++++++++++++
> >  arch/arm/mach-omap2/board-2430sdp.c       |    4 +
> >  arch/arm/mach-omap2/gpmc.c                |    1 
> >  include/asm-arm/arch-omap/gpmc.h          |    7 +
> >  include/asm-arm/arch-omap/onenand.h       |    1 
> 
> The above could be one patch ready for omap-upstream queue.
> 
> >  drivers/mtd/onenand/omap2.c               |  103 +++++++++++++++++-----------
> >  drivers/mtd/onenand/onenand_base.c        |    2 
> 
> This should be separate and sent to MTD list.

Kyungmin,

What's the status of the patch you submitted to the MTD list for this
fix?

FWIW, attached is a patch which fixes the walking of the ooblist, as
well as an update to the omap2 driver for the non-DMA case.

Kevin


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC/PATCH] ARM: OMAP: OneNAND support for 2430SDP

[Tony Lindgren]

* Kevin Hilman <khilman@mvista.com> [070430 22:39]:
> On Thu, 2007-04-26 at 17:35 +0000, Tony Lindgren wrote:
> > * Kevin Hilman <khilman@mvista.com> [070321 00:49]:
> > > Adds platform device for use of existing OMAP2 driver.  Also, the
> > > following changes are made to the existing driver:
> > > 
> > > - add option for not using DMA (by passing dma_channel = -1)
> > > - removes obsoleted pt_regs arg from interrupt handler
> > > 
> > > RFC: Notice the change to onenand_base.c which comments out the last
> > >      OOB chunk.  Without this, it would not work.  Previous drivers
> > >      for this board such as the 2.6.10-based MV kernel have this
> > >      same change.  Anyone know what is going on here?
> > 
> > Can you please refresh this one and split it into two patches:
> > 
> > >  arch/arm/mach-omap2/Makefile              |    3 
> > >  arch/arm/mach-omap2/board-2430sdp-flash.c |  108 ++++++++++++++++++++++++++++++
> > >  arch/arm/mach-omap2/board-2430sdp.c       |    4 +
> > >  arch/arm/mach-omap2/gpmc.c                |    1 
> > >  include/asm-arm/arch-omap/gpmc.h          |    7 +
> > >  include/asm-arm/arch-omap/onenand.h       |    1 
> > 
> > The above could be one patch ready for omap-upstream queue.
> > 
> > >  drivers/mtd/onenand/omap2.c               |  103 +++++++++++++++++-----------
> > >  drivers/mtd/onenand/onenand_base.c        |    2 
> > 
> > This should be separate and sent to MTD list.
> 
> Kyungmin,
> 
> What's the status of the patch you submitted to the MTD list for this
> fix?
> 
> FWIW, attached is a patch which fixes the walking of the ooblist, as
> well as an update to the omap2 driver for the non-DMA case.

The attached patch did not seem to make it to the mailing list.

Tony