ST M29W320D incorrectly configured

ST M29W320D incorrectly configured

[Corinna Schultz]

I'm having a problem getting the unlock addresses correctly configured  
for the ST M29W320D chip. CFI query data is shown below (from  
debugging statements I enabled and/or added to the driver). The chip  
is wired so that the #BYTE pin is low, putting the chip into x8 mode.  
The data bus is physically 8 bits.

I don't understand everything that's going on in the code, but it  
seems to me that the following code (taken from cfi_cmdset_0002.c,  
which sets the unlock addresses needed for writing and erasing) has a  
logic error. Maybe someone can explain it to me?

  if (    /* x16 in x8 mode */
     ((cfi->device_type == CFI_DEVICETYPE_X8) &&
      (cfi->cfiq->InterfaceDesc == 2))


The reason I think this is in error is that both the device type and  
the interfaceDesc are defined by the hardware, and not indicative of  
the mode. Besides, isn't this conditional actually testing if the chip  
is an X8 chip and supports x8 and x16 async modes?

As far as I know the only way to determine what mode the chip is in is  
to check the QRY string at the beginning of the query data on the chip.

According to the spec, page 12 of:   
http://www.amd.com/us-en/assets/content_type/DownloadableAssets/cfi_r20.pdf

An x16 device in x16 mode has QRY at addresses 10, 11, and 12 (hex)  
and Q null R at addresses 20, 21, and 22. An x16 device in x8 mode has  
QQR at addresses 20, 21, 22.

Since this chip is an x16 in x8 mode, the unlock addresses should be  
0xaaa and 0x555.

As can be seen below, the device_type is 2, which is  
CFI_DEVICE_TYPE_X16, and the InterfaceDesc is 2. But as I understand  
the spec, these values don't change whether the chip is in x16 mode or  
in x8 mode.

I'm not sure how to tell the version number of the driver, but  
gen_probe.c is at v 1.24, and cfi_cmdset_0002.c is at v.1.122, if that  
helps.


Here's the log:

physmap flash device: 400000 at ffc00000
Number of erase regions: 4
Primary Vendor Command Set: 0002 (AMD/Fujitsu Standard)
Primary Algorithm Table at 0040
Alternative Vendor Command Set: 0000 (None)
No Alternate Algorithm Table
Vcc Minimum:  2.7 V
Vcc Maximum:  3.6 V
Vpp Minimum: 11.5 V
Vpp Maximum: 12.5 V
Typical byte/word write timeout: 16 ?s
Maximum byte/word write timeout: 512 ?s
Full buffer write not supported
Typical block erase timeout: 1024 ms
Maximum block erase timeout: 16384 ms
Chip erase not supported
Device size: 0x400000 bytes (4 MiB)
Flash Device Interface description: 0x0002
   - supports x8 and x16 via BYTE# with asynchronous interface
Max. bytes in buffer write: 0x1
Number of Erase Block Regions: 4
   Erase Region #0: BlockSize 0x4000 bytes, 1 blocks
   Erase Region #1: BlockSize 0x2000 bytes, 2 blocks
   Erase Region #2: BlockSize 0x8000 bytes, 1 blocks
   Erase Region #3: BlockSize 0x10000 bytes, 63 blocks
phys_mapped_flash: Found 1 x16 devices at 0x0 in 8-bit bank
  Amd/Fujitsu Extended Query Table at 0x0040
   Silicon revision: 0
   Address sensitive unlock: Required
   Erase Suspend: Read/write
   Block protection: 1 sectors per group
   Temporary block unprotect: Supported
   Block protect/unprotect scheme: 4
   Number of simultaneous operations: 0
   Burst mode: Not supported
   Page mode: Not supported
   Vpp Supply Minimum Program/Erase Voltage: 11.5 V
   Vpp Supply Maximum Program/Erase Voltage: 12.5 V
   Top/Bottom Boot Block: Bottom boot
addr_unlock1: 555, addr_unlock2: 2aa
number of CFI chips: 1
cfi_cmdset_0002: Disabling erase-suspend-program due to code brokenness.


-Corinna Schultz
IBM LTC

cfi_cmdset_0002.c possible BUG (was Re: ST M29W320D incorrectly configured)

[Corinna Schultz]

Quoting Corinna Schultz <cschultz@linux.vnet.ibm.com>:

> I'm having a problem getting the unlock addresses correctly configured
> for the ST M29W320D chip. CFI query data is shown below (from
> debugging statements I enabled and/or added to the driver). The chip
> is wired so that the #BYTE pin is low, putting the chip into x8 mode.
> The data bus is physically 8 bits.

OK, I understand things a bit better now. device_type is detected at  
runtime, and the constants are misleadingly named. They are scaling  
values, and in my case, (x16 chip in x8 mode) device_type should be 2,  
or CFI_DEVICETYPE_X16, which is what it is being detected as. My  
interfaceDesc is also 2 (x8/x16 async modes), as it should be. So far  
so good.

Given this, the conditional used to assign the unlock addresses seems  
to have logic errors. Here's the original patch:  
http://lists.infradead.org/pipermail/linux-mtd-cvs/2004-September/004096.html

Does anyone know the reasoning behind this change? Was this tested?  
Has anyone else reported problems with this code (it's pretty old)?


-Corinna Schultz
IBM LTC

Re: ST M29W320D incorrectly configured

[David Woodhouse]

On Wed, 2008-10-29 at 19:53 -0400, Corinna Schultz wrote:
> I'm having a problem getting the unlock addresses correctly configured  
> for the ST M29W320D chip. CFI query data is shown below (from  
> debugging statements I enabled and/or added to the driver). The chip  
> is wired so that the #BYTE pin is low, putting the chip into x8 mode.  
> The data bus is physically 8 bits.
> 
> I don't understand everything that's going on in the code, but it  
> seems to me that the following code (taken from cfi_cmdset_0002.c,  
> which sets the unlock addresses needed for writing and erasing) has a  
> logic error. Maybe someone can explain it to me?
> 
>   if (    /* x16 in x8 mode */
>      ((cfi->device_type == CFI_DEVICETYPE_X8) &&
>       (cfi->cfiq->InterfaceDesc == 2))
> 
> 
> The reason I think this is in error is that both the device type and  
> the interfaceDesc are defined by the hardware, and not indicative of  
> the mode. Besides, isn't this conditional actually testing if the chip
> is an X8 chip and supports x8 and x16 async modes?

That condition is doubly wrong, I think. Not only does it never trigger,
but it'd do the wrong thing if it _did_ trigger. I think the answer is
to revert this:
http://lists.infradead.org/pipermail/linux-mtd-cvs/2004-September/004096.html

It would be nice if we could do it that way, but these ST chips don't
seem to work. When they're in 16-bit mode, they really do need a byte
address of 0x555 for the second unlock address, not 0x554 (0x2aa*2) as
every other chip seems to accept. Although it takes _extra_ logic to
check the input on the 'A-1' pin, they seem to have it.

So I think the answer is to go back to cfi->addr_unlock[12] being _byte_
addresses within the chip...

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index 3e6f5d8..045e3c3 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -404,21 +404,18 @@ struct mtd_info *cfi_cmdset_0002(struct map_info *map, int primary)
 			}
 		}
 		/* Set the default CFI lock/unlock addresses */
-		cfi->addr_unlock1 = 0x555;
-		cfi->addr_unlock2 = 0x2aa;
-		/* Modify the unlock address if we are in compatibility mode */
-		if (	/* x16 in x8 mode */
-			((cfi->device_type == CFI_DEVICETYPE_X8) &&
-				(cfi->cfiq->InterfaceDesc ==
-					CFI_INTERFACE_X8_BY_X16_ASYNC)) ||
-			/* x32 in x16 mode */
-			((cfi->device_type == CFI_DEVICETYPE_X16) &&
-				(cfi->cfiq->InterfaceDesc ==
-					CFI_INTERFACE_X16_BY_X32_ASYNC)))
-		{
-			cfi->addr_unlock1 = 0xaaa;
-			cfi->addr_unlock2 = 0x555;
-		}
+		cfi->addr_unlock1 = 0x555 << cfi->device_type;
+		cfi->addr_unlock2 = 0x2aa << cfi->device_type;
+
+		/* Handle the case of x16 chips in x8 mode which are _really_
+		   picky about the unlock addresses, and require that A-1 is
+		   set too. This is only some ST chips so far...  */
+		if (cfi->device_type == 2 && map_bankwidth(map) == cfi_interleave(cfi))
+			cfi->addr_unlock2 |= 1; /* i.e. 0x555 instead of 0x554 */
+
+		/* Theoretically there can be x32 chips with a x16 mode, which
+		   might need similar treatment. We'll deal with that if it
+		   happens. */
 
 	} /* CFI mode */
 	else if (cfi->cfi_mode == CFI_MODE_JEDEC) {
@@ -994,16 +991,16 @@ static inline int do_read_secsi_onechip(struct map_info *map, struct flchip *chi
 
 	chip->state = FL_READY;
 
-	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0x88, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
+	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, CFI_DEVICETYPE_X8, NULL);
+	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, CFI_DEVICETYPE_X8, NULL);
+	cfi_send_gen_cmd(0x88, cfi->addr_unlock1, chip->start, map, cfi, CFI_DEVICETYPE_X8, NULL);
 
 	map_copy_from(map, buf, adr, len);
 
-	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0x90, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0x00, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
+	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, CFI_DEVICETYPE_X8, NULL);
+	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, CFI_DEVICETYPE_X8, NULL);
+	cfi_send_gen_cmd(0x90, cfi->addr_unlock1, chip->start, map, cfi, CFI_DEVICETYPE_X8, NULL);
+	cfi_send_gen_cmd(0x00, cfi->addr_unlock1, chip->start, map, cfi, CFI_DEVICETYPE_X8, NULL);
 
 	wake_up(&chip->wq);
 	spin_unlock(chip->mutex);
@@ -1102,9 +1099,9 @@ static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
 	ENABLE_VPP(map);
 	xip_disable(map, chip, adr);
  retry:
-	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0xA0, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
+	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, CFI_DEVICETYPE_X8, NULL);
+	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, CFI_DEVICETYPE_X8, NULL);
+	cfi_send_gen_cmd(0xA0, cfi->addr_unlock1, chip->start, map, cfi, CFI_DEVICETYPE_X8, NULL);
 	map_write(map, datum, adr);
 	chip->state = FL_WRITING;
 
@@ -1340,9 +1337,9 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip,
 	ENABLE_VPP(map);
 	xip_disable(map, chip, cmd_adr);
 
-	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, cfi->device_type, NULL);
-	//cfi_send_gen_cmd(0xA0, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
+	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, CFI_DEVICETYPE_X8, NULL);
+	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, CFI_DEVICETYPE_X8, NULL);
+	//cfi_send_gen_cmd(0xA0, cfi->addr_unlock1, chip->start, map, cfi, CFI_DEVICETYPE_X8, NULL);
 
 	/* Write Buffer Load */
 	map_write(map, CMD(0x25), cmd_adr);
@@ -1528,12 +1525,12 @@ static int __xipram do_erase_chip(struct map_info *map, struct flchip *chip)
 	ENABLE_VPP(map);
 	xip_disable(map, chip, adr);
 
-	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0x80, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0x10, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
+	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, CFI_DEVICETYPE_X8, NULL);
+	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, CFI_DEVICETYPE_X8, NULL);
+	cfi_send_gen_cmd(0x80, cfi->addr_unlock1, chip->start, map, cfi, CFI_DEVICETYPE_X8, NULL);
+	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, CFI_DEVICETYPE_X8, NULL);
+	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, CFI_DEVICETYPE_X8, NULL);
+	cfi_send_gen_cmd(0x10, cfi->addr_unlock1, chip->start, map, cfi, CFI_DEVICETYPE_X8, NULL);
 
 	chip->state = FL_ERASING;
 	chip->erase_suspended = 0;
@@ -1616,11 +1613,11 @@ static int __xipram do_erase_oneblock(struct map_info *map, struct flchip *chip,
 	ENABLE_VPP(map);
 	xip_disable(map, chip, adr);
 
-	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0x80, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, cfi->device_type, NULL);
+	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, CFI_DEVICETYPE_X8, NULL);
+	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, CFI_DEVICETYPE_X8, NULL);
+	cfi_send_gen_cmd(0x80, cfi->addr_unlock1, chip->start, map, cfi, CFI_DEVICETYPE_X8, NULL);
+	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, CFI_DEVICETYPE_X8, NULL);
+	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, CFI_DEVICETYPE_X8, NULL);
 	map_write(map, CMD(0x30), adr);
 
 	chip->state = FL_ERASING;
@@ -1739,15 +1736,15 @@ static int do_atmel_lock(struct map_info *map, struct flchip *chip,
 	      __func__, adr, len);
 
 	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi,
-			 cfi->device_type, NULL);
+			 CFI_DEVICETYPE_X8, NULL);
 	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi,
-			 cfi->device_type, NULL);
+			 CFI_DEVICETYPE_X8, NULL);
 	cfi_send_gen_cmd(0x80, cfi->addr_unlock1, chip->start, map, cfi,
-			 cfi->device_type, NULL);
+			 CFI_DEVICETYPE_X8, NULL);
 	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi,
-			 cfi->device_type, NULL);
+			 CFI_DEVICETYPE_X8, NULL);
 	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi,
-			 cfi->device_type, NULL);
+			 CFI_DEVICETYPE_X8, NULL);
 	map_write(map, CMD(0x40), chip->start + adr);
 
 	chip->state = FL_READY;
@@ -1775,7 +1772,7 @@ static int do_atmel_unlock(struct map_info *map, struct flchip *chip,
 	      __func__, adr, len);
 
 	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi,
-			 cfi->device_type, NULL);
+			 CFI_DEVICETYPE_X8, NULL);
 	map_write(map, CMD(0x70), adr);
 
 	chip->state = FL_READY;
diff --git a/drivers/mtd/chips/jedec_probe.c b/drivers/mtd/chips/jedec_probe.c
index f84ab61..93f464f 100644
--- a/drivers/mtd/chips/jedec_probe.c
+++ b/drivers/mtd/chips/jedec_probe.c
@@ -1844,11 +1844,11 @@ static void jedec_reset(u32 base, struct map_info *map, struct cfi_private *cfi)
 		DEBUG( MTD_DEBUG_LEVEL3,
 		       "reset unlock called %x %x \n",
 		       cfi->addr_unlock1,cfi->addr_unlock2);
-		cfi_send_gen_cmd(0xaa, cfi->addr_unlock1, base, map, cfi, cfi->device_type, NULL);
-		cfi_send_gen_cmd(0x55, cfi->addr_unlock2, base, map, cfi, cfi->device_type, NULL);
+		cfi_send_gen_cmd(0xaa, cfi->addr_unlock1, base, map, cfi, CFI_DEVICETYPE_X8, NULL);
+		cfi_send_gen_cmd(0x55, cfi->addr_unlock2, base, map, cfi, CFI_DEVICETYPE_X8, NULL);
 	}
 
-	cfi_send_gen_cmd(0xF0, cfi->addr_unlock1, base, map, cfi, cfi->device_type, NULL);
+	cfi_send_gen_cmd(0xF0, cfi->addr_unlock1, base, map, cfi, CFI_DEVICETYPE_X8, NULL);
 	/* Some misdesigned Intel chips do not respond for 0xF0 for a reset,
 	 * so ensure we're in read mode.  Send both the Intel and the AMD command
 	 * for this.  Intel uses 0xff for this, AMD uses 0xff for NOP, so
@@ -1859,9 +1859,10 @@ static void jedec_reset(u32 base, struct map_info *map, struct cfi_private *cfi)
 }
 
 
-static int cfi_jedec_setup(struct cfi_private *p_cfi, int index)
+static int cfi_jedec_setup(struct map_info *map, struct cfi_private *p_cfi, int index)
 {
 	int i,num_erase_regions;
+	unsigned unlock_mask = 0xFFFF << (map_bankwidth(map) / cfi_interleave(p_cfi) - 1);
 	uint8_t uaddr;
 
 	if (! (jedec_table[index].devtypes & p_cfi->device_type)) {
@@ -1900,10 +1901,9 @@ static int cfi_jedec_setup(struct cfi_private *p_cfi, int index)
 
 	/* The table has unlock addresses in _bytes_, and we try not to let
 	   our brains explode when we see the datasheets talking about address
-	   lines numbered from A-1 to A18. The CFI table has unlock addresses
-	   in device-words according to the mode the device is connected in */
-	p_cfi->addr_unlock1 = unlock_addrs[uaddr].addr1 / p_cfi->device_type;
-	p_cfi->addr_unlock2 = unlock_addrs[uaddr].addr2 / p_cfi->device_type;
+	   lines numbered from A-1 to A18. */
+	p_cfi->addr_unlock1 = unlock_addrs[uaddr].addr1 & unlock_mask;
+	p_cfi->addr_unlock2 = unlock_addrs[uaddr].addr2 & unlock_mask;
 
 	return 1; 	/* ok */
 }
@@ -1924,6 +1924,7 @@ static inline int jedec_match( uint32_t base,
 	int rc = 0;           /* failure until all tests pass */
 	u32 mfr, id;
 	uint8_t uaddr;
+	unsigned unlock_mask = 0xFFFF << (map_bankwidth(map) / cfi_interleave(cfi) - 1);
 
 	/*
 	 * The IDs must match.  For X16 and X32 devices operating in
@@ -1987,8 +1988,8 @@ static inline int jedec_match( uint32_t base,
 	DEBUG( MTD_DEBUG_LEVEL3, "MTD %s(): check unlock addrs 0x%.4x 0x%.4x\n",
 	       __func__, cfi->addr_unlock1, cfi->addr_unlock2 );
 	if ( MTD_UADDR_UNNECESSARY != uaddr && MTD_UADDR_DONT_CARE != uaddr
-	     && ( unlock_addrs[uaddr].addr1 / cfi->device_type != cfi->addr_unlock1 ||
-		  unlock_addrs[uaddr].addr2 / cfi->device_type != cfi->addr_unlock2 ) ) {
+	     && ( (unlock_addrs[uaddr].addr1 & unlock_mask) != cfi->addr_unlock1 ||
+		  (unlock_addrs[uaddr].addr2 & unlock_mask) != cfi->addr_unlock2 ) ) {
 		DEBUG( MTD_DEBUG_LEVEL3,
 			"MTD %s(): 0x%.4x 0x%.4x did not match\n",
 			__func__,
@@ -2029,10 +2030,10 @@ static inline int jedec_match( uint32_t base,
 	 */
 	DEBUG( MTD_DEBUG_LEVEL3, "MTD %s(): return to ID mode\n", __func__ );
 	if (cfi->addr_unlock1) {
-		cfi_send_gen_cmd(0xaa, cfi->addr_unlock1, base, map, cfi, cfi->device_type, NULL);
-		cfi_send_gen_cmd(0x55, cfi->addr_unlock2, base, map, cfi, cfi->device_type, NULL);
+		cfi_send_gen_cmd(0xaa, cfi->addr_unlock1, base, map, cfi, CFI_DEVICETYPE_X8, NULL);
+		cfi_send_gen_cmd(0x55, cfi->addr_unlock2, base, map, cfi, CFI_DEVICETYPE_X8, NULL);
 	}
-	cfi_send_gen_cmd(0x90, cfi->addr_unlock1, base, map, cfi, cfi->device_type, NULL);
+	cfi_send_gen_cmd(0x90, cfi->addr_unlock1, base, map, cfi, CFI_DEVICETYPE_X8, NULL);
 	/* FIXME - should have a delay before continuing */
 
  match_done:
@@ -2049,13 +2050,15 @@ static int jedec_probe_chip(struct map_info *map, __u32 base,
 
  retry:
 	if (!cfi->numchips) {
+		unsigned unlock_mask = 0xFFFF << (map_bankwidth(map) / cfi_interleave(cfi) - 1);
+
 		uaddr_idx++;
 
 		if (MTD_UADDR_UNNECESSARY == uaddr_idx)
 			return 0;
 
-		cfi->addr_unlock1 = unlock_addrs[uaddr_idx].addr1 / cfi->device_type;
-		cfi->addr_unlock2 = unlock_addrs[uaddr_idx].addr2 / cfi->device_type;
+		cfi->addr_unlock1 = unlock_addrs[uaddr_idx].addr1 & unlock_mask;
+		cfi->addr_unlock2 = unlock_addrs[uaddr_idx].addr2 & unlock_mask;
 	}
 
 	/* Make certain we aren't probing past the end of map */
@@ -2067,8 +2070,8 @@ static int jedec_probe_chip(struct map_info *map, __u32 base,
 
 	}
 	/* Ensure the unlock addresses we try stay inside the map */
-	probe_offset1 = cfi_build_cmd_addr(cfi->addr_unlock1, cfi_interleave(cfi), cfi->device_type);
-	probe_offset2 = cfi_build_cmd_addr(cfi->addr_unlock2, cfi_interleave(cfi), cfi->device_type);
+	probe_offset1 = cfi_build_cmd_addr(cfi->addr_unlock1, cfi_interleave(cfi), CFI_DEVICETYPE_X8);
+	probe_offset2 = cfi_build_cmd_addr(cfi->addr_unlock2, cfi_interleave(cfi), CFI_DEVICETYPE_X8);
 	if (	((base + probe_offset1 + map_bankwidth(map)) >= map->size) ||
 		((base + probe_offset2 + map_bankwidth(map)) >= map->size))
 		goto retry;
@@ -2078,10 +2081,10 @@ static int jedec_probe_chip(struct map_info *map, __u32 base,
 
 	/* Autoselect Mode */
 	if(cfi->addr_unlock1) {
-		cfi_send_gen_cmd(0xaa, cfi->addr_unlock1, base, map, cfi, cfi->device_type, NULL);
-		cfi_send_gen_cmd(0x55, cfi->addr_unlock2, base, map, cfi, cfi->device_type, NULL);
+		cfi_send_gen_cmd(0xaa, cfi->addr_unlock1, base, map, cfi, CFI_DEVICETYPE_X8, NULL);
+		cfi_send_gen_cmd(0x55, cfi->addr_unlock2, base, map, cfi, CFI_DEVICETYPE_X8, NULL);
 	}
-	cfi_send_gen_cmd(0x90, cfi->addr_unlock1, base, map, cfi, cfi->device_type, NULL);
+	cfi_send_gen_cmd(0x90, cfi->addr_unlock1, base, map, cfi, CFI_DEVICETYPE_X8, NULL);
 	/* FIXME - should have a delay before continuing */
 
 	if (!cfi->numchips) {
@@ -2099,7 +2102,7 @@ static int jedec_probe_chip(struct map_info *map, __u32 base,
 				       "MTD %s(): matched device 0x%x,0x%x unlock_addrs: 0x%.4x 0x%.4x\n",
 				       __func__, cfi->mfr, cfi->id,
 				       cfi->addr_unlock1, cfi->addr_unlock2 );
-				if (!cfi_jedec_setup(cfi, i))
+				if (!cfi_jedec_setup(map, cfi, i))
 					return 0;
 				goto ok_out;
 			}
diff --git a/include/linux/mtd/cfi.h b/include/linux/mtd/cfi.h
index ee5124e..f32bd96 100644
--- a/include/linux/mtd/cfi.h
+++ b/include/linux/mtd/cfi.h
@@ -267,8 +267,8 @@ struct cfi_private {
 	int interleave;
 	int device_type;
 	int cfi_mode;		/* Are we a JEDEC device pretending to be CFI? */
-	int addr_unlock1;
-	int addr_unlock2;
+	int addr_unlock1;	/* These are _byte_ addresses, rather than */
+	int addr_unlock2;	/* needing to be multiplied by device_type */
 	struct mtd_info *(*cmdset_setup)(struct map_info *);
 	struct cfi_ident *cfiq; /* For now only one. We insist that all devs
 				  must be of the same type. */

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: ST M29W320D incorrectly configured

[Corinna Schultz]

Following up, for the sake of anyone reading this thread, now or in  
the future. :-)

Quoting David Woodhouse <dwmw2@infradead.org>:
> It would be nice if we could do it that way, but these ST chips don't
> seem to work. When they're in 16-bit mode, they really do need a byte
> address of 0x555 for the second unlock address, not 0x554 (0x2aa*2) as
> every other chip seems to accept. Although it takes _extra_ logic to
> check the input on the 'A-1' pin, they seem to have it.
>
> So I think the answer is to go back to cfi->addr_unlock[12] being _byte_
> addresses within the chip...
>
> +		cfi->addr_unlock1 = 0x555 << cfi->device_type;
> +		cfi->addr_unlock2 = 0x2aa << cfi->device_type;

The patch works on my hardware, except for this part here, which  
should be multiplying, not shifting.

We will be doing more extensive testing, and I'll post back in this  
thread if we find any significant problems.

Thanks, David!

-Corinna Schultz
IBM LTC

Re: ST M29W320D incorrectly configured

[Eric W. Biederman]

David Woodhouse <dwmw2@infradead.org> writes:

> On Wed, 2008-10-29 at 19:53 -0400, Corinna Schultz wrote:
>> I'm having a problem getting the unlock addresses correctly configured  
>> for the ST M29W320D chip. CFI query data is shown below (from  
>> debugging statements I enabled and/or added to the driver). The chip  
>> is wired so that the #BYTE pin is low, putting the chip into x8 mode.  
>> The data bus is physically 8 bits.
>> 
>> I don't understand everything that's going on in the code, but it  
>> seems to me that the following code (taken from cfi_cmdset_0002.c,  
>> which sets the unlock addresses needed for writing and erasing) has a  
>> logic error. Maybe someone can explain it to me?
>> 
>>   if (    /* x16 in x8 mode */
>>      ((cfi->device_type == CFI_DEVICETYPE_X8) &&
>>       (cfi->cfiq->InterfaceDesc == 2))
>> 
>> 
>> The reason I think this is in error is that both the device type and  
>> the interfaceDesc are defined by the hardware, and not indicative of  
>> the mode. Besides, isn't this conditional actually testing if the chip
>> is an X8 chip and supports x8 and x16 async modes?

Ok.  I spent some time trying to understand the confusion.  And after looking
at this the code I don't see how we could possibly have a problem.

The only thing that sets device_type is gen_probe.  gen_probe sets
device_type to: map_bankwidth(map)/nr_chips.  Which is the bus width
the chip experiences.  Not how wide the chip actually is.

So I don't have a clue how you can get a device type of x16.  In a 8 bit wide
bus.

> It would be nice if we could do it that way, but these ST chips don't
> seem to work. When they're in 16-bit mode, they really do need a byte
> address of 0x555 for the second unlock address, not 0x554 (0x2aa*2) as
> every other chip seems to accept. Although it takes _extra_ logic to
> check the input on the 'A-1' pin, they seem to have it.

???  In 16bit mode there isn't an a low address pin so you can't even
express a byte address of 0x555.

David if device_type was actually returned or generated by the hardware
your patch makes sense.  But I can't find anywhere that seems to be
happening.

Now perhaps the proper fix is to rename device_type something like device_bus_width,
so it is clearer what is going.  Beyond that I don't see the need for change.

Eric

Re: ST M29W320D incorrectly configured

[Eric W. Biederman]

David Woodhouse <dwmw2@infradead.org> writes:

> On Wed, 2008-10-29 at 19:53 -0400, Corinna Schultz wrote:
>> I'm having a problem getting the unlock addresses correctly configured  
>> for the ST M29W320D chip. CFI query data is shown below (from  
>> debugging statements I enabled and/or added to the driver). The chip  
>> is wired so that the #BYTE pin is low, putting the chip into x8 mode.  
>> The data bus is physically 8 bits.
>> 
>> I don't understand everything that's going on in the code, but it  
>> seems to me that the following code (taken from cfi_cmdset_0002.c,  
>> which sets the unlock addresses needed for writing and erasing) has a  
>> logic error. Maybe someone can explain it to me?
>> 
>>   if (    /* x16 in x8 mode */
>>      ((cfi->device_type == CFI_DEVICETYPE_X8) &&
>>       (cfi->cfiq->InterfaceDesc == 2))
>> 
>> 
>> The reason I think this is in error is that both the device type and  
>> the interfaceDesc are defined by the hardware, and not indicative of  
>> the mode. Besides, isn't this conditional actually testing if the chip
>> is an X8 chip and supports x8 and x16 async modes?
>
> That condition is doubly wrong, I think. Not only does it never trigger,
> but it'd do the wrong thing if it _did_ trigger. I think the answer is
> to revert this:
> http://lists.infradead.org/pipermail/linux-mtd-cvs/2004-September/004096.html
>
> It would be nice if we could do it that way, but these ST chips don't
> seem to work. When they're in 16-bit mode, they really do need a byte
> address of 0x555 for the second unlock address, not 0x554 (0x2aa*2) as
> every other chip seems to accept. Although it takes _extra_ logic to
> check the input on the 'A-1' pin, they seem to have it.
>
> So I think the answer is to go back to cfi->addr_unlock[12] being _byte_
> addresses within the chip...

Forget my last I see now how we can try a 16bit device on an 8 bit bus.
I had missed the type <<=1 in gen_probe_new_chip.  Ooops.

With that said I think we need to fix cfi_send_gen_cmd as this problem
applies to all uses of it.

Unless I've missed something the patch below completely fixes the problem.

From: Eric W. Biederman <ebiederm@maxwell.arastra.com>
Subject: [PATCH] mtd: Fix cfi_send_gen_cmd the handling of x16 devices in x8 mode

cfi_send_gen_cmd is only passed in the addresses:
0, 0x55, 0x2aa, 0x555 and the addresses addr_unlock1 and addr_unlock2
from jeded_probe.  The address 0, 0x55, and 0x555 will not be
effected by this patch.

For 16bit devices in 8bit compatibility mode we need to use the
byte address:  0xaaa and 0x555.  Which effectively match
the word address 0x555 and 0x2aa.  Except the last has it's low byte
set.  We need to set the low bit to maintain the alternating bit
sequence.

Likewise the addresses in jedec_probe whose word address ends
in the bits 10 also need their low bit set.

So generically modify cfi_build_cmd_addr to extend alternating bit
sequences in addresses.  And every current cfi_send_gen_cmd that
assumes cfi_cmd_set_0002 (i.e. uses addresses 0x2aa and 0x555)
needs to be updated.

Signed-off-by: Eric W. Biederman <ebiederm@maxwell.arastra.com>
---
 drivers/mtd/chips/cfi_cmdset_0002.c |   13 -------------
 include/linux/mtd/cfi.h             |    9 ++++++++-
 2 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index a972cc6..9e7a236 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -362,19 +362,6 @@ struct mtd_info *cfi_cmdset_0002(struct map_info *map, int primary)
 		/* Set the default CFI lock/unlock addresses */
 		cfi->addr_unlock1 = 0x555;
 		cfi->addr_unlock2 = 0x2aa;
-		/* Modify the unlock address if we are in compatibility mode */
-		if (	/* x16 in x8 mode */
-			((cfi->device_type == CFI_DEVICETYPE_X8) &&
-				(cfi->cfiq->InterfaceDesc ==
-					CFI_INTERFACE_X8_BY_X16_ASYNC)) ||
-			/* x32 in x16 mode */
-			((cfi->device_type == CFI_DEVICETYPE_X16) &&
-				(cfi->cfiq->InterfaceDesc ==
-					CFI_INTERFACE_X16_BY_X32_ASYNC)))
-		{
-			cfi->addr_unlock1 = 0xaaa;
-			cfi->addr_unlock2 = 0x555;
-		}
 
 	} /* CFI mode */
 	else if (cfi->cfi_mode == CFI_MODE_JEDEC) {
diff --git a/include/linux/mtd/cfi.h b/include/linux/mtd/cfi.h
index d6fb115..0b62217 100644
--- a/include/linux/mtd/cfi.h
+++ b/include/linux/mtd/cfi.h
@@ -283,7 +283,14 @@ struct cfi_private {
  */
 static inline uint32_t cfi_build_cmd_addr(uint32_t cmd_ofs, int interleave, int type)
 {
-	return (cmd_ofs * type) * interleave;
+	uint32_t addr = cmd_ofs * type;
+	/* Modify the unlock address if we are in compatiblity mode.
+	 * For 16bit devices on 8 bit busses
+	 * and 32bit devices on 16 bit busses
+	 * set the low bit of the alternating bit sequence of the address.
+	 */
+	addr |= ((cmd_ofs & 2) * type) >> 2;
+	return addr * interleave;
 }
 
 /*
-- 
1.5.6.5

Re: ST M29W320D incorrectly configured

[Eric W. Biederman]

David Woodhouse <dwmw2@infradead.org> writes:

> On Wed, 2008-10-29 at 19:53 -0400, Corinna Schultz wrote:
>> I'm having a problem getting the unlock addresses correctly configured  
>> for the ST M29W320D chip. CFI query data is shown below (from  
>> debugging statements I enabled and/or added to the driver). The chip  
>> is wired so that the #BYTE pin is low, putting the chip into x8 mode.  
>> The data bus is physically 8 bits.
>> 
>> I don't understand everything that's going on in the code, but it  
>> seems to me that the following code (taken from cfi_cmdset_0002.c,  
>> which sets the unlock addresses needed for writing and erasing) has a  
>> logic error. Maybe someone can explain it to me?
>> 
>>   if (    /* x16 in x8 mode */
>>      ((cfi->device_type == CFI_DEVICETYPE_X8) &&
>>       (cfi->cfiq->InterfaceDesc == 2))
>> 
>> 
>> The reason I think this is in error is that both the device type and  
>> the interfaceDesc are defined by the hardware, and not indicative of  
>> the mode. Besides, isn't this conditional actually testing if the chip
>> is an X8 chip and supports x8 and x16 async modes?
>
> That condition is doubly wrong, I think. Not only does it never trigger,
> but it'd do the wrong thing if it _did_ trigger. I think the answer is
> to revert this:

Yep.  The logic came from a misunderstanding of how the devices would show up.
> http://lists.infradead.org/pipermail/linux-mtd-cvs/2004-September/004096.html

> It would be nice if we could do it that way, but these ST chips don't
> seem to work. When they're in 16-bit mode, they really do need a byte
> address of 0x555 for the second unlock address, not 0x554 (0x2aa*2) as
> every other chip seems to accept. Although it takes _extra_ logic to
> check the input on the 'A-1' pin, they seem to have it.
>
> So I think the answer is to go back to cfi->addr_unlock[12] being _byte_
> addresses within the chip...

And in case you prefer to go do everything in byte address here are some
comments in line.

> +		cfi->addr_unlock1 = 0x555 << cfi->device_type;
> +		cfi->addr_unlock2 = 0x2aa << cfi->device_type;
> +
> +		/* Handle the case of x16 chips in x8 mode which are _really_
> +		   picky about the unlock addresses, and require that A-1 is
> +		   set too. This is only some ST chips so far...  */
> + if (cfi->device_type == 2 && map_bankwidth(map) == cfi_interleave(cfi))
> + cfi->addr_unlock2 |= 1; /* i.e. 0x555 instead of 0x554 */

Note the correct test here is:
		if (cfi->device_type/2 == (map_bankwidth(map)/cfi_interleave(cfi)))
			cfi_addr_unlock2 |= 1;

You need the division or else crazy cases like interleaved x16 devices in x8 mode 
won't work.  Testing device_type/2 == map_bankwidth(map)/cfi_interleave(cfi) automatically
enables x32 devices in x16 mode as well.
                        
>   retry:
>  	if (!cfi->numchips) {
> + unsigned unlock_mask = 0xFFFF << (map_bankwidth(map) / cfi_interleave(cfi) -
> 1);
> +

Looking at the specified unlock address it looks like the loss of the low
bit here is actually a bug in jedec_probe.  So we should not need unlock_mask.

It appears you have passed over the probe sequence in cfi_probe_setup to reset
the chip.  It doesn't seem to make a difference in this case but still it
won't work on x16 devices in x8 mode as current written.

Eric

Re: ST M29W320D incorrectly configured

[David Woodhouse]

On Fri, 2008-10-31 at 21:44 -0700, Eric W. Biederman wrote:
> ???  In 16bit mode there isn't an a low address pin so you can't even
> express a byte address of 0x555.

Sorry, I meant to say 'in 8-bit mode'.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: ST M29W320D incorrectly configured

[David Woodhouse]

On Fri, 2008-10-31 at 23:33 -0700, Eric W. Biederman wrote:
> Forget my last I see now how we can try a 16bit device on an 8 bit
> bus. I had missed the type <<=1 in gen_probe_new_chip.  Ooops.
> 
> With that said I think we need to fix cfi_send_gen_cmd as this problem
> applies to all uses of it.
> 
> Unless I've missed something the patch below completely fixes the
> problem.

Yeah, that looks better. I had looked at the possibility of continuing
to use 'word' addresses and fixing up cfi_build_cmd_addr(), but for some
reason I hadn't noticed that it was used _only_ for the unlock addresses
(or zero). I thought I was going to need to put a special case in for
when it was being used with unlock addresses, and it all got a bit
complex. So I switched to using byte addresses in the variables instead.

I prefer your approach, although I think the patch isn't quite correct.

You have to make sure we properly handle the case of a 16-bit device in
16-bit mode. We mustn't set the byte address to 0x555 there; it has to
remain 0x554. We need to do the 'addr |= ....' bit _only_ if the device
is in compatibility mode (i.e. interleave * type > map_bankwidth(map)).

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: ST M29W320D incorrectly configured

[Eric W. Biederman]

David Woodhouse <dwmw2@infradead.org> writes:

> On Fri, 2008-10-31 at 23:33 -0700, Eric W. Biederman wrote:
>> Forget my last I see now how we can try a 16bit device on an 8 bit
>> bus. I had missed the type <<=1 in gen_probe_new_chip.  Ooops.
>> 
>> With that said I think we need to fix cfi_send_gen_cmd as this problem
>> applies to all uses of it.
>> 
>> Unless I've missed something the patch below completely fixes the
>> problem.
>
> Yeah, that looks better. I had looked at the possibility of continuing
> to use 'word' addresses and fixing up cfi_build_cmd_addr(), but for some
> reason I hadn't noticed that it was used _only_ for the unlock addresses
> (or zero). I thought I was going to need to put a special case in for
> when it was being used with unlock addresses, and it all got a bit
> complex. So I switched to using byte addresses in the variables instead.
>
> I prefer your approach, although I think the patch isn't quite correct.

> You have to make sure we properly handle the case of a 16-bit device in
> 16-bit mode. We mustn't set the byte address to 0x555 there; it has to
> remain 0x554. We need to do the 'addr |= ....' bit _only_ if the device
> is in compatibility mode (i.e. interleave * type > map_bankwidth(map)).

Good point.

From: Eric W. Biederman <ebiederm@xmission.com>
Subject: [PATCH] mtd: Fix cfi_send_gen_cmd the handling of x16 devices in x8 mode

cfi_send_gen_cmd is only passed in the addresses:
0, 0x55, 0x2aa, 0x555 and the addresses addr_unlock1 and addr_unlock2
from jeded_probe.  The address 0, 0x55, and 0x555 will not be
effected by this patch.

For 16bit devices in 8bit compatibility mode we need to use the
byte address:  0xaaa and 0x555.  Which effectively match
the word address 0x555 and 0x2aa.  Except the last has it's low byte
set.  We need to set the low bit to maintain the alternating bit
sequence.

Likewise the addresses in jedec_probe whose word address ends
in the bits 10 also need their low bit set.

So generically modify cfi_send_gen_cmd to extend alternating bit
sequences in addresses.  And every current cfi_send_gen_cmd that
assumes cfi_cmd_set_0002 (i.e. uses addresses 0x2aa and 0x555)
needs to be updated.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 drivers/mtd/chips/cfi_cmdset_0002.c |   13 -------------
 include/linux/mtd/cfi.h             |   12 +++++++++++-
 2 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index a972cc6..9e7a236 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -362,19 +362,6 @@ struct mtd_info *cfi_cmdset_0002(struct map_info *map, int primary)
 		/* Set the default CFI lock/unlock addresses */
 		cfi->addr_unlock1 = 0x555;
 		cfi->addr_unlock2 = 0x2aa;
-		/* Modify the unlock address if we are in compatibility mode */
-		if (	/* x16 in x8 mode */
-			((cfi->device_type == CFI_DEVICETYPE_X8) &&
-				(cfi->cfiq->InterfaceDesc ==
-					CFI_INTERFACE_X8_BY_X16_ASYNC)) ||
-			/* x32 in x16 mode */
-			((cfi->device_type == CFI_DEVICETYPE_X16) &&
-				(cfi->cfiq->InterfaceDesc ==
-					CFI_INTERFACE_X16_BY_X32_ASYNC)))
-		{
-			cfi->addr_unlock1 = 0xaaa;
-			cfi->addr_unlock2 = 0x555;
-		}
 
 	} /* CFI mode */
 	else if (cfi->cfi_mode == CFI_MODE_JEDEC) {
diff --git a/include/linux/mtd/cfi.h b/include/linux/mtd/cfi.h
index d6fb115..70499e2 100644
--- a/include/linux/mtd/cfi.h
+++ b/include/linux/mtd/cfi.h
@@ -429,7 +429,17 @@ static inline uint32_t cfi_send_gen_cmd(u_char cmd, uint32_t cmd_addr, uint32_t
 				int type, map_word *prev_val)
 {
 	map_word val;
-	uint32_t addr = base + cfi_build_cmd_addr(cmd_addr, cfi_interleave(cfi), type);
+	unsigned bankwidth = map_bankwidth(map);
+	unsigned interleave = cfi_interleave(cfi);
+	uint32_t addr = cfi_build_cmd_addr(cmd_addr, interleave, type);
+
+	/* Modify the unlock address if we are in compatiblity mode.
+	 * For 16bit devices on 8 bit busses
+	 * and 32bit devices on 16 bit busses
+	 * set the low bit of the alternating bit sequence of the address.
+	 */
+	if (((type * interleave) > bankwidth) && (cmd_addr & 2))
+		addr |= (type >> 1)*interleave;
 
 	val = cfi_build_cmd(cmd, map, cfi);
 
-- 
1.5.6.5

Re: ST M29W320D incorrectly configured

[David Woodhouse]

On Sat, 2008-11-01 at 01:36 -0700, Eric W. Biederman wrote:
> Likewise the addresses in jedec_probe whose word address ends
> in the bits 10 also need their low bit set.

Some of these places in jedec_probe.c use cfi_build_cmd_addr() directly,
and this version of the patch puts the low-bit-mangling into
cfi_gen_send_cmd() so those bits miss out...

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: ST M29W320D incorrectly configured

[Eric W. Biederman]

David Woodhouse <dwmw2@infradead.org> writes:

> On Sat, 2008-11-01 at 01:36 -0700, Eric W. Biederman wrote:
>> Likewise the addresses in jedec_probe whose word address ends
>> in the bits 10 also need their low bit set.
>
> Some of these places in jedec_probe.c use cfi_build_cmd_addr() directly,
> and this version of the patch puts the low-bit-mangling into
> cfi_gen_send_cmd() so those bits miss out...

Sorry.  It's late and was tired.   One more try.
This time as a patch series.

The first patch is the bug fix.  The second patch kills
the unnecessary arguments to cfi_send_gen_cmd, making the
code that uses it more readable.

Eric

[PATCH 1/2] mtd: Fix cfi_send_gen_cmd the handling of x16 devices in x8 mode

[Eric W. Biederman]

cfi_send_gen_cmd is only passed in the addresses:
0, 0x55, 0x2aa, 0x555 and the addresses addr_unlock1 and addr_unlock2
from jeded_probe.  The address 0, 0x55, and 0x555 will not be
effected by this patch.

For 16bit devices in 8bit compatibility mode we need to use the
byte address:  0xaaa and 0x555.  Which effectively match
the word address 0x555 and 0x2aa.  Except the last has it's low byte
set.  We need to set the low bit to maintain the alternating bit
sequence.

Likewise the addresses in jedec_probe whose word address ends
in the bits 1 0 also need their low bit set.

So generically modify cfi_build_cmd_addr to extend alternating bit
sequences in addresses.  And every current cfi_send_gen_cmd that
assumes cfi_cmd_set_0002 (i.e. uses addresses 0x2aa and 0x555)
needs to be updated.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 drivers/mtd/chips/cfi_cmdset_0002.c |   13 -------------
 drivers/mtd/chips/jedec_probe.c     |   10 ++++------
 include/linux/mtd/cfi.h             |   22 +++++++++++++++++++---
 3 files changed, 23 insertions(+), 22 deletions(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index a972cc6..9e7a236 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -362,19 +362,6 @@ struct mtd_info *cfi_cmdset_0002(struct map_info *map, int primary)
 		/* Set the default CFI lock/unlock addresses */
 		cfi->addr_unlock1 = 0x555;
 		cfi->addr_unlock2 = 0x2aa;
-		/* Modify the unlock address if we are in compatibility mode */
-		if (	/* x16 in x8 mode */
-			((cfi->device_type == CFI_DEVICETYPE_X8) &&
-				(cfi->cfiq->InterfaceDesc ==
-					CFI_INTERFACE_X8_BY_X16_ASYNC)) ||
-			/* x32 in x16 mode */
-			((cfi->device_type == CFI_DEVICETYPE_X16) &&
-				(cfi->cfiq->InterfaceDesc ==
-					CFI_INTERFACE_X16_BY_X32_ASYNC)))
-		{
-			cfi->addr_unlock1 = 0xaaa;
-			cfi->addr_unlock2 = 0x555;
-		}
 
 	} /* CFI mode */
 	else if (cfi->cfi_mode == CFI_MODE_JEDEC) {
diff --git a/drivers/mtd/chips/jedec_probe.c b/drivers/mtd/chips/jedec_probe.c
index f84ab61..2f3f2f7 100644
--- a/drivers/mtd/chips/jedec_probe.c
+++ b/drivers/mtd/chips/jedec_probe.c
@@ -1808,9 +1808,7 @@ static inline u32 jedec_read_mfr(struct map_info *map, uint32_t base,
 	 * several first banks can contain 0x7f instead of actual ID
 	 */
 	do {
-		uint32_t ofs = cfi_build_cmd_addr(0 + (bank << 8),
-						  cfi_interleave(cfi),
-						  cfi->device_type);
+		uint32_t ofs = cfi_build_cmd_addr(0 + (bank << 8), map, cfi);
 		mask = (1 << (cfi->device_type * 8)) - 1;
 		result = map_read(map, base + ofs);
 		bank++;
@@ -1824,7 +1822,7 @@ static inline u32 jedec_read_id(struct map_info *map, uint32_t base,
 {
 	map_word result;
 	unsigned long mask;
-	u32 ofs = cfi_build_cmd_addr(1, cfi_interleave(cfi), cfi->device_type);
+	u32 ofs = cfi_build_cmd_addr(1, map, cfi);
 	mask = (1 << (cfi->device_type * 8)) -1;
 	result = map_read(map, base + ofs);
 	return result.x[0] & mask;
@@ -2067,8 +2065,8 @@ static int jedec_probe_chip(struct map_info *map, __u32 base,
 
 	}
 	/* Ensure the unlock addresses we try stay inside the map */
-	probe_offset1 = cfi_build_cmd_addr(cfi->addr_unlock1, cfi_interleave(cfi), cfi->device_type);
-	probe_offset2 = cfi_build_cmd_addr(cfi->addr_unlock2, cfi_interleave(cfi), cfi->device_type);
+	probe_offset1 = cfi_build_cmd_addr(cfi->addr_unlock1, map, cfi);
+	probe_offset2 = cfi_build_cmd_addr(cfi->addr_unlock2, map, cfi);
 	if (	((base + probe_offset1 + map_bankwidth(map)) >= map->size) ||
 		((base + probe_offset2 + map_bankwidth(map)) >= map->size))
 		goto retry;
diff --git a/include/linux/mtd/cfi.h b/include/linux/mtd/cfi.h
index d6fb115..0c85510 100644
--- a/include/linux/mtd/cfi.h
+++ b/include/linux/mtd/cfi.h
@@ -281,9 +281,25 @@ struct cfi_private {
 /*
  * Returns the command address according to the given geometry.
  */
-static inline uint32_t cfi_build_cmd_addr(uint32_t cmd_ofs, int interleave, int type)
+static inline uint32_t cfi_build_cmd_addr(uint32_t cmd_ofs,
+				struct map_info *map, struct cfi_private *cfi)
 {
-	return (cmd_ofs * type) * interleave;
+	unsigned bankwidth = map_bankwidth(map);
+	unsigned interleave = cfi_interleave(cfi);
+	unsigned type = cfi->device_type;
+	uint32_t addr;
+	
+	addr = (cmd_ofs * type) * interleave;
+
+	/* Modify the unlock address if we are in compatiblity mode.
+	 * For 16bit devices on 8 bit busses
+	 * and 32bit devices on 16 bit busses
+	 * set the low bit of the alternating bit sequence of the address.
+	 */
+	if (((type * interleave) > bankwidth) && (cmd_addr & 2))
+		addr |= (type >> 1)*interleave;
+
+	return  addr;
 }
 
 /*
@@ -429,7 +445,7 @@ static inline uint32_t cfi_send_gen_cmd(u_char cmd, uint32_t cmd_addr, uint32_t
 				int type, map_word *prev_val)
 {
 	map_word val;
-	uint32_t addr = base + cfi_build_cmd_addr(cmd_addr, cfi_interleave(cfi), type);
+	uint32_t addr = base + cfi_build_cmd_addr(cmd_addr, map, cfi);
 
 	val = cfi_build_cmd(cmd, map, cfi);
 
-- 
1.5.6.5

[PATCH 2/2] mtd: Simplify cfi_send_gen_cmd.

[Eric W. Biederman]

- type is current unused and is always passed as cfi->device_type
  so it is completely redudndant, so kill it.
- prev_val is not used and so we can remove it and the code that
  stores a value in it.
- The return address is never used so we don't need to have
  one or to compute the return address.

With these cleanups calling cfi_send_gen_cmd no longers scrolls
off the right edge of the screen.  Hooray!

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 drivers/mtd/chips/cfi_cmdset_0002.c |   66 ++++++++++++++++-------------------
 drivers/mtd/chips/cfi_cmdset_0020.c |   16 ++++----
 drivers/mtd/chips/cfi_probe.c       |   40 ++++++++++----------
 drivers/mtd/chips/cfi_util.c        |    6 ++--
 drivers/mtd/chips/jedec_probe.c     |   20 +++++-----
 drivers/mtd/maps/nettel.c           |    3 +-
 include/linux/mtd/cfi.h             |   14 +------
 7 files changed, 74 insertions(+), 91 deletions(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index 9e7a236..ea27ed1 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -937,16 +937,16 @@ static inline int do_read_secsi_onechip(struct map_info *map, struct flchip *chi
 
 	chip->state = FL_READY;
 
-	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0x88, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
+	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi);
+	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi);
+	cfi_send_gen_cmd(0x88, cfi->addr_unlock1, chip->start, map, cfi);
 
 	map_copy_from(map, buf, adr, len);
 
-	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0x90, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0x00, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
+	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi);
+	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi);
+	cfi_send_gen_cmd(0x90, cfi->addr_unlock1, chip->start, map, cfi);
+	cfi_send_gen_cmd(0x00, cfi->addr_unlock1, chip->start, map, cfi);
 
 	wake_up(&chip->wq);
 	spin_unlock(chip->mutex);
@@ -1045,9 +1045,9 @@ static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip,
 	ENABLE_VPP(map);
 	xip_disable(map, chip, adr);
  retry:
-	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0xA0, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
+	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi);
+	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi);
+	cfi_send_gen_cmd(0xA0, cfi->addr_unlock1, chip->start, map, cfi);
 	map_write(map, datum, adr);
 	chip->state = FL_WRITING;
 
@@ -1283,9 +1283,9 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip,
 	ENABLE_VPP(map);
 	xip_disable(map, chip, cmd_adr);
 
-	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, cfi->device_type, NULL);
-	//cfi_send_gen_cmd(0xA0, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
+	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi);
+	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi);
+	//cfi_send_gen_cmd(0xA0, cfi->addr_unlock1, chip->start, map, cfi);
 
 	/* Write Buffer Load */
 	map_write(map, CMD(0x25), cmd_adr);
@@ -1471,12 +1471,12 @@ static int __xipram do_erase_chip(struct map_info *map, struct flchip *chip)
 	ENABLE_VPP(map);
 	xip_disable(map, chip, adr);
 
-	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0x80, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0x10, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
+	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi);
+	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi);
+	cfi_send_gen_cmd(0x80, cfi->addr_unlock1, chip->start, map, cfi);
+	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi);
+	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi);
+	cfi_send_gen_cmd(0x10, cfi->addr_unlock1, chip->start, map, cfi);
 
 	chip->state = FL_ERASING;
 	chip->erase_suspended = 0;
@@ -1559,11 +1559,11 @@ static int __xipram do_erase_oneblock(struct map_info *map, struct flchip *chip,
 	ENABLE_VPP(map);
 	xip_disable(map, chip, adr);
 
-	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0x80, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, cfi->device_type, NULL);
+	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi);
+	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi);
+	cfi_send_gen_cmd(0x80, cfi->addr_unlock1, chip->start, map, cfi);
+	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi);
+	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi);
 	map_write(map, CMD(0x30), adr);
 
 	chip->state = FL_ERASING;
@@ -1681,16 +1681,11 @@ static int do_atmel_lock(struct map_info *map, struct flchip *chip,
 	DEBUG(MTD_DEBUG_LEVEL3, "MTD %s(): LOCK 0x%08lx len %d\n",
 	      __func__, adr, len);
 
-	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi,
-			 cfi->device_type, NULL);
-	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi,
-			 cfi->device_type, NULL);
-	cfi_send_gen_cmd(0x80, cfi->addr_unlock1, chip->start, map, cfi,
-			 cfi->device_type, NULL);
-	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi,
-			 cfi->device_type, NULL);
-	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi,
-			 cfi->device_type, NULL);
+	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi);
+	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi);
+	cfi_send_gen_cmd(0x80, cfi->addr_unlock1, chip->start, map, cfi);
+	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi);
+	cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi);
 	map_write(map, CMD(0x40), chip->start + adr);
 
 	chip->state = FL_READY;
@@ -1717,8 +1712,7 @@ static int do_atmel_unlock(struct map_info *map, struct flchip *chip,
 	DEBUG(MTD_DEBUG_LEVEL3, "MTD %s(): LOCK 0x%08lx len %d\n",
 	      __func__, adr, len);
 
-	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi,
-			 cfi->device_type, NULL);
+	cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi);
 	map_write(map, CMD(0x70), adr);
 
 	chip->state = FL_READY;
diff --git a/drivers/mtd/chips/cfi_cmdset_0020.c b/drivers/mtd/chips/cfi_cmdset_0020.c
index d4714dd..6ea8dd5 100644
--- a/drivers/mtd/chips/cfi_cmdset_0020.c
+++ b/drivers/mtd/chips/cfi_cmdset_0020.c
@@ -1160,17 +1160,17 @@ static int cfi_staa_lock(struct mtd_info *mtd, loff_t ofs, size_t len)
 	while(len) {
 
 #ifdef DEBUG_LOCK_BITS
-		cfi_send_gen_cmd(0x90, 0x55, 0, map, cfi, cfi->device_type, NULL);
+		cfi_send_gen_cmd(0x90, 0x55, 0, map, cfi);
 		printk("before lock: block status register is %x\n",cfi_read_query(map, adr+(2*ofs_factor)));
-		cfi_send_gen_cmd(0xff, 0x55, 0, map, cfi, cfi->device_type, NULL);
+		cfi_send_gen_cmd(0xff, 0x55, 0, map, cfi);
 #endif
 
 		ret = do_lock_oneblock(map, &cfi->chips[chipnum], adr);
 
 #ifdef DEBUG_LOCK_BITS
-		cfi_send_gen_cmd(0x90, 0x55, 0, map, cfi, cfi->device_type, NULL);
+		cfi_send_gen_cmd(0x90, 0x55, 0, map, cfi);
 		printk("after lock: block status register is %x\n",cfi_read_query(map, adr+(2*ofs_factor)));
-		cfi_send_gen_cmd(0xff, 0x55, 0, map, cfi, cfi->device_type, NULL);
+		cfi_send_gen_cmd(0xff, 0x55, 0, map, cfi);
 #endif
 
 		if (ret)
@@ -1302,22 +1302,22 @@ static int cfi_staa_unlock(struct mtd_info *mtd, loff_t ofs, size_t len)
 		unsigned long temp_adr = adr;
 		unsigned long temp_len = len;
 
-		cfi_send_gen_cmd(0x90, 0x55, 0, map, cfi, cfi->device_type, NULL);
+		cfi_send_gen_cmd(0x90, 0x55, 0, map, cfi);
                 while (temp_len) {
 			printk("before unlock %x: block status register is %x\n",temp_adr,cfi_read_query(map, temp_adr+(2*ofs_factor)));
 			temp_adr += mtd->erasesize;
 			temp_len -= mtd->erasesize;
 		}
-		cfi_send_gen_cmd(0xff, 0x55, 0, map, cfi, cfi->device_type, NULL);
+		cfi_send_gen_cmd(0xff, 0x55, 0, map, cfi);
 	}
 #endif
 
 	ret = do_unlock_oneblock(map, &cfi->chips[chipnum], adr);
 
 #ifdef DEBUG_LOCK_BITS
-	cfi_send_gen_cmd(0x90, 0x55, 0, map, cfi, cfi->device_type, NULL);
+	cfi_send_gen_cmd(0x90, 0x55, 0, map, cfi);
 	printk("after unlock: block status register is %x\n",cfi_read_query(map, adr+(2*ofs_factor)));
-	cfi_send_gen_cmd(0xff, 0x55, 0, map, cfi, cfi->device_type, NULL);
+	cfi_send_gen_cmd(0xff, 0x55, 0, map, cfi);
 #endif
 
 	return ret;
diff --git a/drivers/mtd/chips/cfi_probe.c b/drivers/mtd/chips/cfi_probe.c
index c418e92..59f1d80 100644
--- a/drivers/mtd/chips/cfi_probe.c
+++ b/drivers/mtd/chips/cfi_probe.c
@@ -44,17 +44,17 @@ do { \
 
 #define xip_enable(base, map, cfi) \
 do { \
-	cfi_send_gen_cmd(0xF0, 0, base, map, cfi, cfi->device_type, NULL); \
-	cfi_send_gen_cmd(0xFF, 0, base, map, cfi, cfi->device_type, NULL); \
+	cfi_send_gen_cmd(0xF0, 0, base, map, cfi); \
+	cfi_send_gen_cmd(0xFF, 0, base, map, cfi); \
 	xip_allowed(base, map); \
 } while (0)
 
 #define xip_disable_qry(base, map, cfi) \
 do { \
 	xip_disable(); \
-	cfi_send_gen_cmd(0xF0, 0, base, map, cfi, cfi->device_type, NULL); \
-	cfi_send_gen_cmd(0xFF, 0, base, map, cfi, cfi->device_type, NULL); \
-	cfi_send_gen_cmd(0x98, 0x55, base, map, cfi, cfi->device_type, NULL); \
+	cfi_send_gen_cmd(0xF0, 0, base, map, cfi); \
+	cfi_send_gen_cmd(0xFF, 0, base, map, cfi); \
+	cfi_send_gen_cmd(0x98, 0x55, base, map, cfi); \
 } while (0)
 
 #else
@@ -116,9 +116,9 @@ static int __xipram cfi_probe_chip(struct map_info *map, __u32 base,
 	}
 
 	xip_disable();
-	cfi_send_gen_cmd(0xF0, 0, base, map, cfi, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0xFF, 0, base, map, cfi, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0x98, 0x55, base, map, cfi, cfi->device_type, NULL);
+	cfi_send_gen_cmd(0xF0, 0, base, map, cfi);
+	cfi_send_gen_cmd(0xFF, 0, base, map, cfi);
+	cfi_send_gen_cmd(0x98, 0x55, base, map, cfi);
 
 	if (!qry_present(map,base,cfi)) {
 		xip_enable(base, map, cfi);
@@ -144,8 +144,8 @@ static int __xipram cfi_probe_chip(struct map_info *map, __u32 base,
 		if (qry_present(map, start, cfi)) {
 			/* Eep. This chip also had the QRY marker.
 			 * Is it an alias for the new one? */
-			cfi_send_gen_cmd(0xF0, 0, start, map, cfi, cfi->device_type, NULL);
-			cfi_send_gen_cmd(0xFF, 0, start, map, cfi, cfi->device_type, NULL);
+			cfi_send_gen_cmd(0xF0, 0, start, map, cfi);
+			cfi_send_gen_cmd(0xFF, 0, start, map, cfi);
 
 			/* If the QRY marker goes away, it's an alias */
 			if (!qry_present(map, start, cfi)) {
@@ -158,8 +158,8 @@ static int __xipram cfi_probe_chip(struct map_info *map, __u32 base,
 			 * unfortunate. Stick the new chip in read mode
 			 * too and if it's the same, assume it's an alias. */
 			/* FIXME: Use other modes to do a proper check */
-			cfi_send_gen_cmd(0xF0, 0, base, map, cfi, cfi->device_type, NULL);
-			cfi_send_gen_cmd(0xFF, 0, start, map, cfi, cfi->device_type, NULL);
+			cfi_send_gen_cmd(0xF0, 0, base, map, cfi);
+			cfi_send_gen_cmd(0xFF, 0, start, map, cfi);
 
 			if (qry_present(map, base, cfi)) {
 				xip_allowed(base, map);
@@ -176,8 +176,8 @@ static int __xipram cfi_probe_chip(struct map_info *map, __u32 base,
 	cfi->numchips++;
 
 	/* Put it back into Read Mode */
-	cfi_send_gen_cmd(0xF0, 0, base, map, cfi, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0xFF, 0, base, map, cfi, cfi->device_type, NULL);
+	cfi_send_gen_cmd(0xF0, 0, base, map, cfi);
+	cfi_send_gen_cmd(0xFF, 0, base, map, cfi);
 	xip_allowed(base, map);
 
 	printk(KERN_INFO "%s: Found %d x%d devices at 0x%x in %d-bit bank\n",
@@ -224,10 +224,10 @@ static int __xipram cfi_chip_setup(struct map_info *map,
 	 * so should be treated as nops or illegal (and so put the device
 	 * back into Read Mode, which is a nop in this case).
 	 */
-	cfi_send_gen_cmd(0xf0,     0, base, map, cfi, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0xaa, 0x555, base, map, cfi, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0x55, 0x2aa, base, map, cfi, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0x90, 0x555, base, map, cfi, cfi->device_type, NULL);
+	cfi_send_gen_cmd(0xf0,     0, base, map, cfi);
+	cfi_send_gen_cmd(0xaa, 0x555, base, map, cfi);
+	cfi_send_gen_cmd(0x55, 0x2aa, base, map, cfi);
+	cfi_send_gen_cmd(0x90, 0x555, base, map, cfi);
 	cfi->mfr = cfi_read_query16(map, base);
 	cfi->id = cfi_read_query16(map, base + ofs_factor);
 
@@ -237,9 +237,9 @@ static int __xipram cfi_chip_setup(struct map_info *map,
 			  cfi_read_query(map, base + 0xf * ofs_factor);
 
 	/* Put it back into Read Mode */
-	cfi_send_gen_cmd(0xF0, 0, base, map, cfi, cfi->device_type, NULL);
+	cfi_send_gen_cmd(0xF0, 0, base, map, cfi);
 	/* ... even if it's an Intel chip */
-	cfi_send_gen_cmd(0xFF, 0, base, map, cfi, cfi->device_type, NULL);
+	cfi_send_gen_cmd(0xFF, 0, base, map, cfi);
 	xip_allowed(base, map);
 
 	/* Do any necessary byteswapping */
diff --git a/drivers/mtd/chips/cfi_util.c b/drivers/mtd/chips/cfi_util.c
index 0ee4570..8379c71 100644
--- a/drivers/mtd/chips/cfi_util.c
+++ b/drivers/mtd/chips/cfi_util.c
@@ -48,7 +48,7 @@ __xipram cfi_read_pri(struct map_info *map, __u16 adr, __u16 size, const char* n
 #endif
 
 	/* Switch it into Query Mode */
-	cfi_send_gen_cmd(0x98, 0x55, base, map, cfi, cfi->device_type, NULL);
+	cfi_send_gen_cmd(0x98, 0x55, base, map, cfi);
 
 	/* Read in the Extended Query Table */
 	for (i=0; i<size; i++) {
@@ -57,8 +57,8 @@ __xipram cfi_read_pri(struct map_info *map, __u16 adr, __u16 size, const char* n
 	}
 
 	/* Make sure it returns to read mode */
-	cfi_send_gen_cmd(0xf0, 0, base, map, cfi, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0xff, 0, base, map, cfi, cfi->device_type, NULL);
+	cfi_send_gen_cmd(0xf0, 0, base, map, cfi);
+	cfi_send_gen_cmd(0xff, 0, base, map, cfi);
 
 #ifdef CONFIG_MTD_XIP
 	(void) map_read(map, base);
diff --git a/drivers/mtd/chips/jedec_probe.c b/drivers/mtd/chips/jedec_probe.c
index 2f3f2f7..98afaf9 100644
--- a/drivers/mtd/chips/jedec_probe.c
+++ b/drivers/mtd/chips/jedec_probe.c
@@ -1842,17 +1842,17 @@ static void jedec_reset(u32 base, struct map_info *map, struct cfi_private *cfi)
 		DEBUG( MTD_DEBUG_LEVEL3,
 		       "reset unlock called %x %x \n",
 		       cfi->addr_unlock1,cfi->addr_unlock2);
-		cfi_send_gen_cmd(0xaa, cfi->addr_unlock1, base, map, cfi, cfi->device_type, NULL);
-		cfi_send_gen_cmd(0x55, cfi->addr_unlock2, base, map, cfi, cfi->device_type, NULL);
+		cfi_send_gen_cmd(0xaa, cfi->addr_unlock1, base, map, cfi);
+		cfi_send_gen_cmd(0x55, cfi->addr_unlock2, base, map, cfi);
 	}
 
-	cfi_send_gen_cmd(0xF0, cfi->addr_unlock1, base, map, cfi, cfi->device_type, NULL);
+	cfi_send_gen_cmd(0xF0, cfi->addr_unlock1, base, map, cfi);
 	/* Some misdesigned Intel chips do not respond for 0xF0 for a reset,
 	 * so ensure we're in read mode.  Send both the Intel and the AMD command
 	 * for this.  Intel uses 0xff for this, AMD uses 0xff for NOP, so
 	 * this should be safe.
 	 */
-	cfi_send_gen_cmd(0xFF, 0, base, map, cfi, cfi->device_type, NULL);
+	cfi_send_gen_cmd(0xFF, 0, base, map, cfi);
 	/* FIXME - should have reset delay before continuing */
 }
 
@@ -2027,10 +2027,10 @@ static inline int jedec_match( uint32_t base,
 	 */
 	DEBUG( MTD_DEBUG_LEVEL3, "MTD %s(): return to ID mode\n", __func__ );
 	if (cfi->addr_unlock1) {
-		cfi_send_gen_cmd(0xaa, cfi->addr_unlock1, base, map, cfi, cfi->device_type, NULL);
-		cfi_send_gen_cmd(0x55, cfi->addr_unlock2, base, map, cfi, cfi->device_type, NULL);
+		cfi_send_gen_cmd(0xaa, cfi->addr_unlock1, base, map, cfi);
+		cfi_send_gen_cmd(0x55, cfi->addr_unlock2, base, map, cfi);
 	}
-	cfi_send_gen_cmd(0x90, cfi->addr_unlock1, base, map, cfi, cfi->device_type, NULL);
+	cfi_send_gen_cmd(0x90, cfi->addr_unlock1, base, map, cfi);
 	/* FIXME - should have a delay before continuing */
 
  match_done:
@@ -2076,10 +2076,10 @@ static int jedec_probe_chip(struct map_info *map, __u32 base,
 
 	/* Autoselect Mode */
 	if(cfi->addr_unlock1) {
-		cfi_send_gen_cmd(0xaa, cfi->addr_unlock1, base, map, cfi, cfi->device_type, NULL);
-		cfi_send_gen_cmd(0x55, cfi->addr_unlock2, base, map, cfi, cfi->device_type, NULL);
+		cfi_send_gen_cmd(0xaa, cfi->addr_unlock1, base, map, cfi);
+		cfi_send_gen_cmd(0x55, cfi->addr_unlock2, base, map, cfi);
 	}
-	cfi_send_gen_cmd(0x90, cfi->addr_unlock1, base, map, cfi, cfi->device_type, NULL);
+	cfi_send_gen_cmd(0x90, cfi->addr_unlock1, base, map, cfi);
 	/* FIXME - should have a delay before continuing */
 
 	if (!cfi->numchips) {
diff --git a/drivers/mtd/maps/nettel.c b/drivers/mtd/maps/nettel.c
index 965e6c6..2586d2f 100644
--- a/drivers/mtd/maps/nettel.c
+++ b/drivers/mtd/maps/nettel.c
@@ -146,8 +146,7 @@ static int nettel_reboot_notifier(struct notifier_block *nb, unsigned long val,
 
 	/* Make sure all FLASH chips are put back into read mode */
 	for (b = 0; (b < nettel_intel_partitions[3].size); b += 0x100000) {
-		cfi_send_gen_cmd(0xff, 0x55, b, &nettel_intel_map, cfi,
-			cfi->device_type, NULL);
+		cfi_send_gen_cmd(0xff, 0x55, b, &nettel_intel_map, cfi);
 	}
 	return(NOTIFY_OK);
 }
diff --git a/include/linux/mtd/cfi.h b/include/linux/mtd/cfi.h
index 0c85510..67931dd 100644
--- a/include/linux/mtd/cfi.h
+++ b/include/linux/mtd/cfi.h
@@ -435,26 +435,16 @@ static inline unsigned long cfi_merge_status(map_word val, struct map_info *map,
 
 /*
  * Sends a CFI command to a bank of flash for the given geometry.
- *
- * Returns the offset in flash where the command was written.
- * If prev_val is non-null, it will be set to the value at the command address,
- * before the command was written.
  */
-static inline uint32_t cfi_send_gen_cmd(u_char cmd, uint32_t cmd_addr, uint32_t base,
-				struct map_info *map, struct cfi_private *cfi,
-				int type, map_word *prev_val)
+static inline void cfi_send_gen_cmd(u_char cmd, uint32_t cmd_addr, uint32_t base,
+				struct map_info *map, struct cfi_private *cfi)
 {
 	map_word val;
 	uint32_t addr = base + cfi_build_cmd_addr(cmd_addr, map, cfi);
 
 	val = cfi_build_cmd(cmd, map, cfi);
 
-	if (prev_val)
-		*prev_val = map_read(map, addr);
-
 	map_write(map, val, addr);
-
-	return addr - base;
 }
 
 static inline uint8_t cfi_read_query(struct map_info *map, uint32_t addr)
-- 
1.5.6.5

Re: [PATCH 1/2] mtd: Fix cfi_send_gen_cmd the handling of x16 devices in x8 mode

[David Woodhouse]

On Sat, 2008-11-01 at 03:29 -0700, Eric W. Biederman wrote:
> @@ -1808,9 +1808,7 @@ static inline u32 jedec_read_mfr(struct map_info *map, uint32_t base,
>          * several first banks can contain 0x7f instead of actual ID
>          */
>         do {
> -               uint32_t ofs = cfi_build_cmd_addr(0 + (bank << 8),
> -                                                 cfi_interleave(cfi),
> -                                                 cfi->device_type);
> +               uint32_t ofs = cfi_build_cmd_addr(0 + (bank << 8), map, cfi);
>                 mask = (1 << (cfi->device_type * 8)) - 1;
>                 result = map_read(map, base + ofs);
>                 bank++;

I think this one is still broken by your patch -- it's the exception to
your observation that we only ever use addresses ending in 00, 55 or aa.

> 
> +       /* Modify the unlock address if we are in compatiblity mode.
> +        * For 16bit devices on 8 bit busses
> +        * and 32bit devices on 16 bit busses
> +        * set the low bit of the alternating bit sequence of the address.
> +        */
> +       if (((type * interleave) > bankwidth) && (cmd_addr & 2))
> +               addr |= (type >> 1)*interleave;

Perhaps '&& ((cmd_addr & 0xff) == 0xaa)' is the answer?

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [PATCH 1/2] mtd: Fix cfi_send_gen_cmd the handling of x16 devices in x8 mode

[David Woodhouse]

On Sat, 2008-11-01 at 10:43 +0000, David Woodhouse wrote:
> I think this one is still broken by your patch -- it's the exception to
> your observation that we only ever use addresses ending in 00, 55 or aa.

No, I misunderstood how that works -- your patch is fine. Although I
think I'll split patch 1/2 into cleanup vs. the actual fix, too.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

[PATCH] mtd: Fix cfi_send_gen_cmd the handling of x16 devices in x8 mode v4

[Eric W. Biederman]

cfi_send_gen_cmd is only passed in the addresses:
0, 0x55, 0x2aa, 0x555 and the addresses addr_unlock1 and addr_unlock2
from jeded_probe.  The address 0, 0x55, and 0x555 will not be
effected by this patch.

For 16bit devices in 8bit compatibility mode we need to use the
byte address:  0xaaa and 0x555.  Which effectively match
the word address 0x555 and 0x2aa.  Except the last has it's low byte
set.  We need to set the low bit to maintain the alternating bit
sequence.

Likewise the addresses in jedec_probe whose word address ends
in the bits 10 also need their low bit set.

So generically modify cfi_build_cmd_addr to extend alternating bit
sequences in addresses.  And every current cfi_send_gen_cmd that
assumes cfi_cmd_set_0002 (i.e. uses addresses 0x2aa and 0x555)
needs to be updated.

v4: Fix  stupid typo in cfi_build_cmd_addr that failed to compile
    I'm writing this patch way to late at night.
v3: Bring all of the work back into cfi_build_cmd_addr
    including calling of map_bankwidth(map) and cfi_interleave(cfi)
    So every caller doesn't need to.
v2: Only modified the address if we our device_type is larger than our
    bus width.

Signed-off-by: Eric W. Biederman <ebiederm@maxwell.arastra.com>
---

 drivers/mtd/chips/cfi_cmdset_0002.c |   13 -------------
 drivers/mtd/chips/jedec_probe.c     |   10 ++++------
 include/linux/mtd/cfi.h             |   22 +++++++++++++++++++---
 3 files changed, 23 insertions(+), 22 deletions(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index a972cc6..9e7a236 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -362,19 +362,6 @@ struct mtd_info *cfi_cmdset_0002(struct map_info *map, int primary)
 		/* Set the default CFI lock/unlock addresses */
 		cfi->addr_unlock1 = 0x555;
 		cfi->addr_unlock2 = 0x2aa;
-		/* Modify the unlock address if we are in compatibility mode */
-		if (	/* x16 in x8 mode */
-			((cfi->device_type == CFI_DEVICETYPE_X8) &&
-				(cfi->cfiq->InterfaceDesc ==
-					CFI_INTERFACE_X8_BY_X16_ASYNC)) ||
-			/* x32 in x16 mode */
-			((cfi->device_type == CFI_DEVICETYPE_X16) &&
-				(cfi->cfiq->InterfaceDesc ==
-					CFI_INTERFACE_X16_BY_X32_ASYNC)))
-		{
-			cfi->addr_unlock1 = 0xaaa;
-			cfi->addr_unlock2 = 0x555;
-		}
 
 	} /* CFI mode */
 	else if (cfi->cfi_mode == CFI_MODE_JEDEC) {
diff --git a/drivers/mtd/chips/jedec_probe.c b/drivers/mtd/chips/jedec_probe.c
index f84ab61..2f3f2f7 100644
--- a/drivers/mtd/chips/jedec_probe.c
+++ b/drivers/mtd/chips/jedec_probe.c
@@ -1808,9 +1808,7 @@ static inline u32 jedec_read_mfr(struct map_info *map, uint32_t base,
 	 * several first banks can contain 0x7f instead of actual ID
 	 */
 	do {
-		uint32_t ofs = cfi_build_cmd_addr(0 + (bank << 8),
-						  cfi_interleave(cfi),
-						  cfi->device_type);
+		uint32_t ofs = cfi_build_cmd_addr(0 + (bank << 8), map, cfi);
 		mask = (1 << (cfi->device_type * 8)) - 1;
 		result = map_read(map, base + ofs);
 		bank++;
@@ -1824,7 +1822,7 @@ static inline u32 jedec_read_id(struct map_info *map, uint32_t base,
 {
 	map_word result;
 	unsigned long mask;
-	u32 ofs = cfi_build_cmd_addr(1, cfi_interleave(cfi), cfi->device_type);
+	u32 ofs = cfi_build_cmd_addr(1, map, cfi);
 	mask = (1 << (cfi->device_type * 8)) -1;
 	result = map_read(map, base + ofs);
 	return result.x[0] & mask;
@@ -2067,8 +2065,8 @@ static int jedec_probe_chip(struct map_info *map, __u32 base,
 
 	}
 	/* Ensure the unlock addresses we try stay inside the map */
-	probe_offset1 = cfi_build_cmd_addr(cfi->addr_unlock1, cfi_interleave(cfi), cfi->device_type);
-	probe_offset2 = cfi_build_cmd_addr(cfi->addr_unlock2, cfi_interleave(cfi), cfi->device_type);
+	probe_offset1 = cfi_build_cmd_addr(cfi->addr_unlock1, map, cfi);
+	probe_offset2 = cfi_build_cmd_addr(cfi->addr_unlock2, map, cfi);
 	if (	((base + probe_offset1 + map_bankwidth(map)) >= map->size) ||
 		((base + probe_offset2 + map_bankwidth(map)) >= map->size))
 		goto retry;
diff --git a/include/linux/mtd/cfi.h b/include/linux/mtd/cfi.h
index d6fb115..52d2e11 100644
--- a/include/linux/mtd/cfi.h
+++ b/include/linux/mtd/cfi.h
@@ -281,9 +281,25 @@ struct cfi_private {
 /*
  * Returns the command address according to the given geometry.
  */
-static inline uint32_t cfi_build_cmd_addr(uint32_t cmd_ofs, int interleave, int type)
+static inline uint32_t cfi_build_cmd_addr(uint32_t cmd_ofs,
+				struct map_info *map, struct cfi_private *cfi)
 {
-	return (cmd_ofs * type) * interleave;
+	unsigned bankwidth = map_bankwidth(map);
+	unsigned interleave = cfi_interleave(cfi);
+	unsigned type = cfi->device_type;
+	uint32_t addr;
+	
+	addr = (cmd_ofs * type) * interleave;
+
+	/* Modify the unlock address if we are in compatiblity mode.
+	 * For 16bit devices on 8 bit busses
+	 * and 32bit devices on 16 bit busses
+	 * set the low bit of the alternating bit sequence of the address.
+	 */
+	if (((type * interleave) > bankwidth) && (cmd_ofs & 2))
+		addr |= (type >> 1)*interleave;
+
+	return  addr;
 }
 
 /*
@@ -429,7 +445,7 @@ static inline uint32_t cfi_send_gen_cmd(u_char cmd, uint32_t cmd_addr, uint32_t
 				int type, map_word *prev_val)
 {
 	map_word val;
-	uint32_t addr = base + cfi_build_cmd_addr(cmd_addr, cfi_interleave(cfi), type);
+	uint32_t addr = base + cfi_build_cmd_addr(cmd_addr, map, cfi);
 
 	val = cfi_build_cmd(cmd, map, cfi);
 
-- 
1.5.6.5

Re: [PATCH 1/2] mtd: Fix cfi_send_gen_cmd the handling of x16 devices in x8 mode

[Eric W. Biederman]

David Woodhouse <dwmw2@infradead.org> writes:

> On Sat, 2008-11-01 at 10:43 +0000, David Woodhouse wrote:
>> I think this one is still broken by your patch -- it's the exception to
>> your observation that we only ever use addresses ending in 00, 55 or aa.
>
> No, I misunderstood how that works -- your patch is fine. Although I
> think I'll split patch 1/2 into cleanup vs. the actual fix, too.

Sounds good.   The (cmd_ofs & 0xff) == 0xaa might be less prone to false
positives.  Or perhaps it just doesn't make sense for the id reads in jedec_probe.
Sounds like you have a handle on it though.

Eric