MTD device with multiple regions & JFFS2

MTD device with multiple regions & JFFS2

[Angelos Manousarides]

I have a system with intel P30 flash chips, which have 2 erase regions. 
The system can have either 2 flash chips in one bank or 4 flash chips in 
2 banks. I am using mtdconcat to join the two devices and have unified 
access. I have a problem with the multiple erase sizes. The chips are of 
"top" type which means that the last sectors have a smaller block size 
than the others.

The code in mtdconcat does transfer the region info to the resulting 
device, I see a mtd device with 2 (or 4 regions). The partitioning code 
however does not. "numeraseregions" is 0 and "erasesize" is only used.

While I was trying to fix the code in mtdpart.c, I discovered that this 
is futile. The JFFS2 code does not seem to support this as well. Only 
the "erasesize" is used there also. Does that mean that I cannot have a 
JFFS2 filesystem that spans across multiple block size flash space?

--
Angelos Manousaridis

Re: MTD device with multiple regions & JFFS2

[Josh Boyer]

On 6/30/06, Angelos Manousarides <amanous@inaccessnetworks.com> wrote:
>
> The code in mtdconcat does transfer the region info to the resulting
> device, I see a mtd device with 2 (or 4 regions). The partitioning code
> however does not. "numeraseregions" is 0 and "erasesize" is only used.
>
> While I was trying to fix the code in mtdpart.c, I discovered that this
> is futile. The JFFS2 code does not seem to support this as well. Only
> the "erasesize" is used there also. Does that mean that I cannot have a
> JFFS2 filesystem that spans across multiple block size flash space?

Were you trying to fix it for "correctness", or is there a real
problem here?  JFFS2 is known to work on Intel P30 chips without
requiring it to understand the multiple erasesizes.  The chip command
set driver should handle this.

Btw, you forgot to mention what kernel you're running.  I'm assuming a
fairly recent one since you didn't say you had to backport P30 support
or anything like that.

josh

Re: MTD device with multiple regions & JFFS2

[Angelos Manousarides]

On Fri, Jun 30, 2006 at 08:13:21AM -0500, Josh Boyer wrote:
> On 6/30/06, Angelos Manousarides <amanous@inaccessnetworks.com> wrote:
> >
> > The code in mtdconcat does transfer the region info to the resulting
> > device, I see a mtd device with 2 (or 4 regions). The partitioning code
> > however does not. "numeraseregions" is 0 and "erasesize" is only used.
> >
> > While I was trying to fix the code in mtdpart.c, I discovered that this
> > is futile. The JFFS2 code does not seem to support this as well. Only
> > the "erasesize" is used there also. Does that mean that I cannot have a
> > JFFS2 filesystem that spans across multiple block size flash space?
> 
> Were you trying to fix it for "correctness", or is there a real
> problem here?  JFFS2 is known to work on Intel P30 chips without
> requiring it to understand the multiple erasesizes.  The chip command
> set driver should handle this.

The code in mtdpart.c does not propagate erase region information to the
slave device. I have ckeched with the git repository online and it is
still the same. Here is the code I am talking about, it is the only reference
in regions in mtdpard.c:

    if (master->numeraseregions>1) {
      /* Deal with variable erase size stuff */
      int i;
      struct mtd_erase_region_info *regions = master->eraseregions;

      /* Find the first erase regions which is part of this partition. */
      for (i=0; i < master->numeraseregions && slave->offset >= regions[i].offset; i++)
        ;

      for (i--; i < master->numeraseregions && slave->offset + slave->mtd.size > regions[i].offset; i++) {
        if (slave->mtd.erasesize < regions[i].erasesize) {
          slave->mtd.erasesize = regions[i].erasesize;
        }
      }
    } else {
      /* Single erase size */
      slave->mtd.erasesize = master->erasesize;
    }

I have changed this to allocate slave->mtd.eraseregions and set
slave->mtd.numeraseregions properly.

If this is not needed, then definately I am missing something here.

> Btw, you forgot to mention what kernel you're running.  I'm assuming a
> fairly recent one since you didn't say you had to backport P30 support
> or anything like that.

I had to backport the P30 code, but the mtdpart and mtdconcat files do
not have any changes since them.

Now that I think of it, my first comment about the JFFS2 might be wrong.
The jffs2 layer calls mtd->erase(). Because normally all "small" sectors
are enclosed in "large" sector binaries, the erase() function will
probably handle the erase properly. The JFFS layer might be thinking it
is erasing a single block, but the mtd->erase will actually erase a few
small blocks. This is probably the reason that the erasesize in a
multi-region flash is set to the maximum block size. But this is just a
guess.

--
Angelos Manousaridis

Re: MTD device with multiple regions & JFFS2

[Josh Boyer]

On 6/30/06, Angelos Manousarides <amanous@inaccessnetworks.com> wrote::
>
> The code in mtdpart.c does not propagate erase region information to the
> slave device. I have ckeched with the git repository online and it is
> still the same. Here is the code I am talking about, it is the only reference
> in regions in mtdpard.c:
>
>     if (master->numeraseregions>1) {
>       /* Deal with variable erase size stuff */
>       int i;
>       struct mtd_erase_region_info *regions = master->eraseregions;
>
>       /* Find the first erase regions which is part of this partition. */
>       for (i=0; i < master->numeraseregions && slave->offset >= regions[i].offset; i++)
>         ;
>
>       for (i--; i < master->numeraseregions && slave->offset + slave->mtd.size > regions[i].offset; i++) {
>         if (slave->mtd.erasesize < regions[i].erasesize) {
>           slave->mtd.erasesize = regions[i].erasesize;
>         }
>       }
>     } else {
>       /* Single erase size */
>       slave->mtd.erasesize = master->erasesize;
>     }
>
> I have changed this to allocate slave->mtd.eraseregions and set
> slave->mtd.numeraseregions properly.
>
> If this is not needed, then definately I am missing something here.

It should really only make a difference if a partition actually spans
two different erase regions.  And even then I'm not sure it really
matters much.  But I suppose there is nothing bad about doing it
correctly.

> Now that I think of it, my first comment about the JFFS2 might be wrong.
> The jffs2 layer calls mtd->erase(). Because normally all "small" sectors
> are enclosed in "large" sector binaries, the erase() function will
> probably handle the erase properly. The JFFS layer might be thinking it
> is erasing a single block, but the mtd->erase will actually erase a few
> small blocks. This is probably the reason that the erasesize in a
> multi-region flash is set to the maximum block size. But this is just a
> guess.

Good guess.  That is what should be happening, so JFFS2 should be
fine.  Unless I am the one that's missing something :).

josh

Re: MTD device with multiple regions & JFFS2

[Angelos Manousarides]

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

Josh Boyer wrote:
> On 6/30/06, Angelos Manousarides <amanous@inaccessnetworks.com> wrote::
> 
>>The code in mtdpart.c does not propagate erase region information to the
>>slave device. I have ckeched with the git repository online and it is
>>still the same. Here is the code I am talking about, it is the only reference
>>in regions in mtdpard.c:
>>
>>    if (master->numeraseregions>1) {
>>      /* Deal with variable erase size stuff */
>>      int i;
>>      struct mtd_erase_region_info *regions = master->eraseregions;
>>
>>      /* Find the first erase regions which is part of this partition. */
>>      for (i=0; i < master->numeraseregions && slave->offset >= regions[i].offset; i++)
>>        ;
>>
>>      for (i--; i < master->numeraseregions && slave->offset + slave->mtd.size > regions[i].offset; i++) {
>>        if (slave->mtd.erasesize < regions[i].erasesize) {
>>          slave->mtd.erasesize = regions[i].erasesize;
>>        }
>>      }
>>    } else {
>>      /* Single erase size */
>>      slave->mtd.erasesize = master->erasesize;
>>    }
>>
>>I have changed this to allocate slave->mtd.eraseregions and set
>>slave->mtd.numeraseregions properly.
>>
>>If this is not needed, then definately I am missing something here.
> 
> 
> It should really only make a difference if a partition actually spans
> two different erase regions.  And even then I'm not sure it really
> matters much.  But I suppose there is nothing bad about doing it
> correctly.

Here is the patch. Identation sucks, but it sucked in the original file 
as well.

-- 
Angelos Manousaridis

[-- Attachment #2: l.patch --]
[-- Type: text/x-patch, Size: 5380 bytes --]

--- drivers/mtd/mtdpart.c	2006-06-18 04:49:35.000000000 +0300
+++ /home/u0/amanous/sbx/linux/drivers/mtd/mtdpart.c	2006-07-04 15:02:18.000000000 +0300
@@ -465,10 +465,9 @@
 		if (slave->offset == MTDPART_OFS_APPEND)
 			slave->offset = cur_offset;
 		if (slave->offset == MTDPART_OFS_NXTBLK) {
-			slave->offset = cur_offset;
-			if ((cur_offset % master->erasesize) != 0) {
-				/* Round up to next erasesize */
-				slave->offset = ((cur_offset / master->erasesize) + 1) * master->erasesize;
+			u_int32_t emask = master->erasesize-1;
+			slave->offset = (cur_offset + emask) & ~emask;
+			if (slave->offset != cur_offset) {
 				printk(KERN_NOTICE "Moving partition %d: "
 				       "0x%08x -> 0x%08x\n", i,
 				       cur_offset, slave->offset);
@@ -496,24 +495,97 @@
 		}
 		if (master->numeraseregions>1) {
 			/* Deal with variable erase size stuff */
-			int i;
-			struct mtd_erase_region_info *regions = master->eraseregions;
+			struct mtd_erase_region_info *mreg = master->eraseregions;
+			int firstr, lastr;
+			int r;
 
-			/* Find the first erase regions which is part of this partition. */
-			for (i=0; i < master->numeraseregions && slave->offset >= regions[i].offset; i++)
+			/* find the starting region */
+			for (r=0; r < master->numeraseregions && slave->offset >= mreg[r].offset; r++)
+				;
+			firstr = r - 1;
+			/* find the ending region */
+			for (r = firstr; r < master->numeraseregions &&
+				slave->offset + slave->mtd.size > 
+				mreg[r].offset + mreg[r].numblocks * mreg[r].erasesize; r++)
 				;
+			lastr = r;
+			if ( lastr == firstr ) {
+				/* Master device has multiple regions, but this partition is contained within one region */
+				slave->mtd.numeraseregions = 0;
+				slave->mtd.erasesize = mreg[firstr].erasesize;
+				printk(KERN_NOTICE "  single erase region with size 0x%08x and erasesize 0x%08x\n",
+					slave->mtd.size, slave->mtd.erasesize);
+			} else {
+				/* partition does span across multiple regions */
+				u_int32_t soff, max_erasesize;
+				struct mtd_erase_region_info *sreg;
+				int mr, sr;
+
+				if ( (slave->offset - mreg[firstr].offset) % mreg[firstr].erasesize ) {
+					printk("mtd: ignoring partition \"%s\", "
+						"doesn't start on an erase block boundary\n", parts[i].name);
+					continue;
+				}
+				if ( (slave->offset + slave->mtd.size - mreg[lastr].offset) % mreg[lastr].erasesize ) {
+					printk("mtd: ignoring partition \"%s\", "
+						"doesn't end on an erase block boundary\n", parts[i].name);
+					continue;
+				}
 
-			for (i--; i < master->numeraseregions && slave->offset + slave->mtd.size > regions[i].offset; i++) {
-				if (slave->mtd.erasesize < regions[i].erasesize) {
-					slave->mtd.erasesize = regions[i].erasesize;
+				slave->mtd.numeraseregions = lastr - firstr + 1;
+				slave->mtd.eraseregions = sreg = kmalloc(slave->mtd.numeraseregions *
+					sizeof(struct mtd_erase_region_info), GFP_KERNEL);
+				if  ( ! slave->mtd.eraseregions ) {
+					printk("memory allocation error while creating erase region list"
+						" for partition \"%s\"\n", slave->mtd.name);
+					return -ENOMEM;
 				}
+
+				max_erasesize = 0;
+				mr = firstr;
+				sr = 0;
+
+				while ( mr <= lastr ) {
+					sreg[sr].erasesize = mreg[mr].erasesize;
+					sreg[sr].offset = mreg[mr].offset - slave->offset;
+					sreg[sr].numblocks = mreg[mr].numblocks;
+				
+					if ( sreg[sr].erasesize > max_erasesize )
+						max_erasesize = sreg[sr].erasesize;
+					mr++; sr++;
+				}
+
+				if ( slave->offset > mreg[firstr].offset ) {
+					/* fix first region if it is not on sector boundary */
+					sreg[0].offset = 0;
+					sreg[0].numblocks = mreg[firstr].numblocks -
+						(slave->offset - mreg[firstr].offset) / mreg[firstr].erasesize;
+				}
+
+				soff = mreg[lastr].offset + mreg[lastr].erasesize * mreg[lastr].numblocks
+					- slave->offset - slave->mtd.size;
+				if ( soff ) {
+					/* fix last region if it is not on sector boundary */
+					sreg[slave->mtd.numeraseregions-1].numblocks -= soff / mreg[lastr].erasesize;
+				}
+
+				slave->mtd.erasesize = max_erasesize;
+
+				for ( sr = 0; sr < slave->mtd.numeraseregions; sr++ )
+					printk("  region %d : offset 0x%08x erasesize 0x%08x blocks %d\n", sr,
+						slave->mtd.eraseregions[sr].offset,
+						slave->mtd.eraseregions[sr].erasesize,
+						slave->mtd.eraseregions[sr].numblocks);
 			}
 		} else {
 			/* Single erase size */
+			slave->mtd.numeraseregions = 0;
 			slave->mtd.erasesize = master->erasesize;
+			printk(KERN_NOTICE "  single erase region with size 0x%08x and erasesize 0x%08x\n",
+				slave->mtd.size, slave->mtd.erasesize);
 		}
 
-		if ((slave->mtd.flags & MTD_WRITEABLE) &&
+		if (!slave->mtd.numeraseregions && (slave->mtd.flags & MTD_WRITEABLE) && 
 		    (slave->offset % slave->mtd.erasesize)) {
 			/* Doesn't start on a boundary of major erase size */
 			/* FIXME: Let it be writable if it is on a boundary of _minor_ erase size though */
@@ -521,7 +593,7 @@
 			printk ("mtd: partition \"%s\" doesn't start on an erase block boundary -- force read-only\n",
 				parts[i].name);
 		}
-		if ((slave->mtd.flags & MTD_WRITEABLE) &&
+		if (!slave->mtd.numeraseregions && (slave->mtd.flags & MTD_WRITEABLE) && 
 		    (slave->mtd.size % slave->mtd.erasesize)) {
 			slave->mtd.flags &= ~MTD_WRITEABLE;
 			printk ("mtd: partition \"%s\" doesn't end on an erase block -- force read-only\n",