Fixup Intel flash that powers up locked

Fixup Intel flash that powers up locked

[Todd Poynor]

Another try at how to handle the longstanding problem of chips that lock
all blocks at power up.  This version handles it in a way that I think
is in accordance with previous suggestions: it checks for whether the
chip has this property (for Intel flash the data sheet for at least one
model implies that those with the Instant Individual Block Locking
feature bit set will also have the property of locking all blocks at
power up); and it rectifies the locks with the map write access flags at
probe time and at resume time.  It adds a chip driver callback when
partitions/devices are added, so that the driver can fix things up
according to the map info.

Any comments on whether this is or is not the way to go appreciated.


diff -ruN mtd/drivers/mtd/chips/cfi_cmdset_0001.c mtd-work/drivers/mtd/chips/cfi_cmdset_0001.c
--- mtd/drivers/mtd/chips/cfi_cmdset_0001.c	2005-04-12 17:53:29.000000000 -0700
+++ mtd-work/drivers/mtd/chips/cfi_cmdset_0001.c	2005-04-12 17:55:45.000000000 -0700
@@ -68,6 +68,7 @@
 static int cfi_intelext_suspend (struct mtd_info *);
 static void cfi_intelext_resume (struct mtd_info *);
 static int cfi_intelext_reboot (struct notifier_block *, unsigned long, void *);
+static void cfi_intelext_instantlock_add(struct mtd_info* mtd);
 
 static void cfi_intelext_destroy(struct mtd_info *);
 
@@ -390,6 +391,7 @@
 {
 	struct map_info *map = mtd->priv;
 	struct cfi_private *cfi = map->fldrv_priv;
+	struct cfi_pri_intelext *extp = cfi->cmdset_priv;
 	unsigned long offset = 0;
 	int i,j;
 	unsigned long devsize = (1<<cfi->cfiq->DevSize) * cfi->interleave;
@@ -444,6 +446,9 @@
 	mtd->get_user_prot_info = cfi_intelext_get_user_prot_info;
 #endif
 
+	if (extp && (extp->FeatureSupport & (1 << 5)))
+		mtd->notify_add = cfi_intelext_instantlock_add;
+
 	/* This function has the potential to distort the reality
 	   a bit and therefore should be called last. */
 	if (cfi_intelext_partition_fixup(mtd, &cfi) != 0)
@@ -2227,6 +2232,28 @@
 
 #endif
 
+static void cfi_intelext_unlockall(struct mtd_info *mtd)
+{
+	int i;
+
+	for (i = 0; i < mtd->numeraseregions; i++) {
+		int j;
+
+		for (j = 0; j < mtd->eraseregions[i].numblocks; j++){
+			mtd->unlock(mtd, mtd->eraseregions[i].offset +
+				    j * mtd->eraseregions[i].erasesize,
+				    mtd->eraseregions[i].erasesize);
+		}
+	}
+	
+}
+
+static void cfi_intelext_instantlock_add(struct mtd_info *mtd)
+{
+	if (mtd->flags & MTD_WRITEABLE)
+		cfi_intelext_unlockall(mtd);
+}
+
 static int cfi_intelext_suspend(struct mtd_info *mtd)
 {
 	struct map_info *map = mtd->priv;
@@ -2300,6 +2327,7 @@
 	struct cfi_private *cfi = map->fldrv_priv;
 	int i;
 	struct flchip *chip;
+	struct cfi_pri_intelext *extp = cfi->cmdset_priv;
 
 	for (i=0; i<cfi->numchips; i++) {
 	
@@ -2316,6 +2344,11 @@
 
 		spin_unlock(chip->mutex);
 	}
+
+
+	if (extp && (extp->FeatureSupport & (1 << 5)) &&
+	    (mtd->flags & MTD_WRITEABLE))
+		cfi_intelext_unlockall(mtd);	
 }
 
 static int cfi_intelext_reset(struct mtd_info *mtd)
diff -ruN mtd/drivers/mtd/mtdcore.c mtd-work/drivers/mtd/mtdcore.c
--- mtd/drivers/mtd/mtdcore.c	2005-03-28 17:36:04.000000000 -0800
+++ mtd-work/drivers/mtd/mtdcore.c	2005-04-12 17:55:45.000000000 -0700
@@ -60,6 +60,10 @@
 			mtd->usecount = 0;
 
 			DEBUG(0, "mtd: Giving out device %d to %s\n",i, mtd->name);
+
+			if (mtd->notify_add)				
+				mtd->notify_add(mtd);
+
 			/* No need to get a refcount on the module containing
 			   the notifier, since we hold the mtd_table_mutex */
 			list_for_each(this, &mtd_notifiers) {
diff -ruN mtd/drivers/mtd/mtdpart.c mtd-work/drivers/mtd/mtdpart.c
--- mtd/drivers/mtd/mtdpart.c	2005-03-28 17:36:04.000000000 -0800
+++ mtd-work/drivers/mtd/mtdpart.c	2005-04-12 17:55:45.000000000 -0700
@@ -435,6 +435,7 @@
 			slave->mtd.get_user_prot_info = part_get_user_prot_info;
 		if(master->get_fact_prot_info)
 			slave->mtd.get_fact_prot_info = part_get_fact_prot_info;
+		slave->mtd.notify_add = master->notify_add;
 		if (master->sync)
 			slave->mtd.sync = part_sync;
 		if (!i && master->suspend && master->resume) {
diff -ruN mtd/include/linux/mtd/mtd.h mtd-work/include/linux/mtd/mtd.h
--- mtd/include/linux/mtd/mtd.h	2005-04-12 17:53:37.000000000 -0700
+++ mtd-work/include/linux/mtd/mtd.h	2005-04-12 17:55:45.000000000 -0700
@@ -148,6 +148,10 @@
 	int (*block_isbad) (struct mtd_info *mtd, loff_t ofs);
 	int (*block_markbad) (struct mtd_info *mtd, loff_t ofs);
 
+	/* Device/partition add notifier */
+
+	void (*notify_add) (struct mtd_info *mtd);
+
 	struct notifier_block reboot_notifier;  /* default mode before reboot */
 
 	void *priv;

Re: Fixup Intel flash that powers up locked

[Nicolas Pitre]

On Tue, 12 Apr 2005, Todd Poynor wrote:

> Another try at how to handle the longstanding problem of chips that lock
> all blocks at power up.  This version handles it in a way that I think
> is in accordance with previous suggestions: it checks for whether the
> chip has this property (for Intel flash the data sheet for at least one
> model implies that those with the Instant Individual Block Locking
> feature bit set will also have the property of locking all blocks at
> power up); and it rectifies the locks with the map write access flags at
> probe time and at resume time.  It adds a chip driver callback when
> partitions/devices are added, so that the driver can fix things up
> according to the map info.
> 
> Any comments on whether this is or is not the way to go appreciated.

Properly dealing with this "feature" appears to be so messy that I'm 
starting to wonder if we should not just unconditionally unlock the 
flash at boot time and be done with it.

It's only the flash used for the root filesystem which is problematic 
anyway, right?  If so then I think there should be a mount option for 
JFFS* such that it directly unlocks the whole of the MTD device it's 
about to be mounted on when it's mounted read-write.  That's it.

As for the suspend case then you should probably just preserve the flash 
lock state before suspending and restore that state upon resume.

> +static void cfi_intelext_unlockall(struct mtd_info *mtd)
> +{
> +	int i;
> +
> +	for (i = 0; i < mtd->numeraseregions; i++) {
> +		int j;
> +
> +		for (j = 0; j < mtd->eraseregions[i].numblocks; j++){
> +			mtd->unlock(mtd, mtd->eraseregions[i].offset +
> +				    j * mtd->eraseregions[i].erasesize,
> +				    mtd->eraseregions[i].erasesize);
> +		}
> +	}
> +	
> +}

Why are you doing this block by block? You could as well just do:

	cfi_intelext_unlock(mtd, 0, mtd->size);

since the block walking is already performed by cfi_varsize_frob().


Nicolas

RE: Fixup Intel flash that powers up locked

[Christopher Hoover]

>> Any comments on whether this is or is not the way to go appreciated.

>Properly dealing with this "feature" appears to be so messy that I'm
starting
>to wonder if we should not just unconditionally unlock the flash at boot
time
>and be done with it.

I put forward such code over two years ago.  I can dig it up again.

-ch

Re: Fixup Intel flash that powers up locked

[Todd Poynor]

Nicolas Pitre wrote:

> Properly dealing with this "feature" appears to be so messy that I'm 
> starting to wonder if we should not just unconditionally unlock the 
> flash at boot time and be done with it.

Or leave all blocks locked to err on the side of safety and require 
userspace init scripts to unlock writeable partitions.

> It's only the flash used for the root filesystem which is problematic 
> anyway, right?  If so then I think there should be a mount option for 
> JFFS* such that it directly unlocks the whole of the MTD device it's 
> about to be mounted on when it's mounted read-write.  That's it.

Read-only root with writeable partitions mounted afterwards is what I 
suggest, I imagine there's reasons not to do it that way, though.

> As for the suspend case then you should probably just preserve the flash 
> lock state before suspending and restore that state upon resume.

I'm guessing the code for that would trigger the messy alarms as well...

Am finding the frob DEBUG_LOCK_BITS logic doesn't read lock status 
properly on this board's chip (blocks that appear to be writeable still 
read status non-zero).  This makes it difficult to setup the bitmap at 
suspend time.  Now, I could maintain the bitmap at each block 
lock/unlock time, but the frob code doesn't pass the eraseregion info 
(useful to understand region size and maintain per-region bitmap) to the 
chip driver (which knows whether the chip has this powers-up-locked 
property).  Alas.  I will play with this some more.

> Why are you doing this block by block? You could as well just do:
> 
> 	cfi_intelext_unlock(mtd, 0, mtd->size);
> 
> since the block walking is already performed by cfi_varsize_frob().

Aargh, that code predated the frob on multi-partition flash fix, sorry. 
  Thanks,


-- 
Todd

Re: Fixup Intel flash that powers up locked

[Todd Poynor]

On Tue, Apr 12, 2005 at 09:45:36PM -0400, Nicolas Pitre wrote:

> As for the suspend case then you should probably just preserve the flash 
> lock state before suspending and restore that state upon resume.

Here's a patch which does just that.  The lock bitmap is in the generic
eraseregion data structure to facilitate compact bitmap size based on
erase region size, but could be moved to per mtd cfi private.  Comments
appreciated, thanks. -- Todd

Index: linux-mtd/drivers/mtd/chips/cfi_cmdset_0001.c
===================================================================
--- linux-mtd.orig/drivers/mtd/chips/cfi_cmdset_0001.c	2005-04-21 23:26:08.000000000 +0000
+++ linux-mtd/drivers/mtd/chips/cfi_cmdset_0001.c	2005-04-22 22:48:45.000000000 +0000
@@ -30,6 +30,7 @@
 #include <linux/delay.h>
 #include <linux/interrupt.h>
 #include <linux/reboot.h>
+#include <linux/bitmap.h>
 #include <linux/mtd/xip.h>
 #include <linux/mtd/map.h>
 #include <linux/mtd/mtd.h>
@@ -431,6 +432,7 @@
 		       i,mtd->eraseregions[i].offset,
 		       mtd->eraseregions[i].erasesize,
 		       mtd->eraseregions[i].numblocks);
+		mtd->eraseregions[i].lockmap = NULL;
 	}
 
 #ifdef CONFIG_MTD_OTP
@@ -2235,10 +2237,50 @@
 
 #endif
 
+static void cfi_intelext_save_locks(struct mtd_info *mtd)
+{
+	struct map_info *map = mtd->priv;
+	struct cfi_private *cfi = map->fldrv_priv;
+	int ofs_factor = cfi->interleave * cfi->device_type;
+	int i;
+
+	for (i = 0; i < mtd->numeraseregions; i++) {
+		struct mtd_erase_region_info *region = &mtd->eraseregions[i];
+		int mapsize = region->numblocks / 8 + 1;
+
+		if (region->lockmap ||
+		    (region->lockmap = kmalloc(mapsize, GFP_KERNEL))) {
+			int block;
+
+			memset(region->lockmap, 0, mapsize);
+
+			for (block = 0; block < region->numblocks; block++){
+				struct flchip *chip;
+				int status, chipnum;
+				unsigned long adr = region->offset +
+					block * region->erasesize;
+
+				chipnum = adr >> cfi->chipshift;
+				chip = &cfi->chips[chipnum];
+				xip_disable(map, chip, adr+(2*ofs_factor));
+				map_write(map, CMD(0x90), adr+(2*ofs_factor));
+				chip->state = FL_JEDEC_QUERY;
+				status = cfi_read_query(map, 
+							adr+(2*ofs_factor));
+				xip_enable(map, chip, 0);
+
+				if (status & 0x1)
+					set_bit(block, region->lockmap);
+			}
+		}
+	}
+}
+
 static int cfi_intelext_suspend(struct mtd_info *mtd)
 {
 	struct map_info *map = mtd->priv;
 	struct cfi_private *cfi = map->fldrv_priv;
+	struct cfi_pri_intelext *extp = cfi->cmdset_priv;
 	int i;
 	struct flchip *chip;
 	int ret = 0;
@@ -2279,6 +2321,9 @@
 		spin_unlock(chip->mutex);
 	}
 
+	if (! ret && extp && (extp->FeatureSupport & (1 << 5)))
+		cfi_intelext_save_locks(mtd);
+
 	/* Unlock the chips again */
 
 	if (ret) {
@@ -2302,10 +2347,47 @@
 	return ret;
 }
 
+static void cfi_intelext_restore_locks(struct mtd_info *mtd)
+{
+	struct map_info *map = mtd->priv;
+	struct cfi_private *cfi = map->fldrv_priv;
+	int ofs_factor = cfi->interleave * cfi->device_type;
+	int i;
+
+	for (i = 0; i < mtd->numeraseregions; i++) {
+		struct mtd_erase_region_info *region = &mtd->eraseregions[i];
+		int block;
+
+		if (! region->lockmap)
+			continue;
+
+		for (block = 0; block < region->numblocks; block++){
+			struct flchip *chip;
+			int status, chipnum;
+			unsigned long adr = region->offset +
+				block * region->erasesize;
+
+			chipnum = adr >> cfi->chipshift;
+			chip = &cfi->chips[chipnum];
+
+			if (! test_bit(block, region->lockmap)) {
+				xip_disable(map, chip, adr+(2*ofs_factor));
+				do_xxlock_oneblock(map, chip,
+						   adr - chip->start,
+						   region->erasesize,
+						   DO_XXLOCK_ONEBLOCK_UNLOCK);
+				xip_enable(map, chip, 0);
+			}
+			
+		}
+	}
+}
+
 static void cfi_intelext_resume(struct mtd_info *mtd)
 {
 	struct map_info *map = mtd->priv;
 	struct cfi_private *cfi = map->fldrv_priv;
+	struct cfi_pri_intelext *extp = cfi->cmdset_priv;
 	int i;
 	struct flchip *chip;
 
@@ -2324,6 +2406,9 @@
 
 		spin_unlock(chip->mutex);
 	}
+
+	if (extp && (extp->FeatureSupport & (1 << 5)))
+		cfi_intelext_restore_locks(mtd);
 }
 
 static int cfi_intelext_reset(struct mtd_info *mtd)
Index: linux-mtd/include/linux/mtd/mtd.h
===================================================================
--- linux-mtd.orig/include/linux/mtd/mtd.h	2005-04-21 23:26:08.000000000 +0000
+++ linux-mtd/include/linux/mtd/mtd.h	2005-04-21 23:34:52.000000000 +0000
@@ -55,6 +55,7 @@
 	u_int32_t offset;			/* At which this region starts, from the beginning of the MTD */
 	u_int32_t erasesize;		/* For this region */
 	u_int32_t numblocks;		/* Number of blocks of erasesize in this region */
+	unsigned long *lockmap;		/* If keeping bitmap of locks */
 };
 
 struct mtd_info {

Re: Fixup Intel flash that powers up locked

[rick adams]

On Wed, 2005-04-13 at 09:59 -0400, linux-mtd-
request@lists.infradead.org 
> Message: 2
> Date: Tue, 12 Apr 2005 18:03:15 -0700
> From: Todd Poynor <tpoynor@mvista.com>
> Subject: Fixup Intel flash that powers up locked
>
> Any comments on whether this is or is not the way to go appreciated.
> 

What I have done when I have encountered this Intel "feature" of their
version "K" of their stataFlash devices is to unlock all sectors in the
boot code. This is done before the kernel even loads, so the kernel
doesn't even know it's not using the "J" version of the part (the device
the driver was written for). The boot code reads the device ID register
and if it's not a "J" version it unlocks all the sectors. I had to do
this anyhow in the boot code since my boot code needs to write to FLASH.
There are several reasons I feel this is a better approach to address
the Intel "feature" the most important reason is the KISS philosophy.
Why add all the complexity to the kernel when it can be done simply in
the boot code and the kernel doesn't even know (or need to know) the
joys of these version differences of the FLASH chip on the board. It
just needs to now that it can talk to either the "J" or "K" version with
the same code. If you don't do the boot code and have a problem with
this dependency on non kernel code, you can always add it to head.s,
headxxxx.s, or some board specific code that runs before the file system
(or any write to FLASH) is used.

NOTE:: Intel is obsoleting the "J" part and selling the "K" part as a
direct replacement. So if you have a product in production that uses the
Intel StrataFLASH device you may want to address this "feature" before
you get the call that production has stopped (accompanied by more help
from management they you can imagine).

Rick Adams
rick@thePTRgroup.com