MTD device with multiple regions & JFFS2

DISCUSSION Angelos Manousarides 2 participants 5 messages Jun 30, 2006 – Jul 4, 2006 Memory technology devices (MTD)

Share & tools

Angelos Manousarides

author

2006-06-30 12:31 UTC #0 lore raw

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

Josh Boyer

2006-06-30 13:13 UTC #1 lore raw

Quoted 9 lines · Angelos Manousarides

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

Angelos Manousarides

author

2006-06-30 13:57 UTC #2 lore raw

Quoted 11 lines · Josh Boyer

Quoted 9 lines · Angelos Manousarides
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?

Quoted 4 lines

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.

Quoted 3 lines

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

Josh Boyer

2006-06-30 14:36 UTC #3 lore raw

On 6/30/06, Angelos Manousarides <[email protected]> wrote::

Quoted 25 lines

    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;
    }

Quoted 4 lines

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.

Quoted 8 lines

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

Angelos Manousarides

author

2006-07-04 12:23 UTC #4 lore raw

Quoted 29 lines · Josh Boyer

On 6/30/06, Angelos Manousarides <[email protected]> wrote::
Quoted 27 lines
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.

Quoted 7 lines

Quoted 1 line
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.

Signature · 1 line

Angelos Manousaridis

home/u0/amanous/sbx/linux/drivers/mtd/mtdpart.c renamed from mtd/mtdpart.c +85 ~~-13~~

@@ -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",