Skip to contentSkip to conversation

jedec_probe problems with 16bit devices

PATCH Ben Dooks 4 participants 6 messages Sep 4, 2004 – Sep 14, 2004 Memory technology devices (MTD)

lore
Share & tools
lore

Ben Dooks

author
#0 lore raw

I have been trying to get an SST 39LF160 on an Simtec Electronics EB2410ITX (aka Bast). I have found some problems with jedec_probe with non-8bit devices.

Basically, the two problems are as follows:

1) A number of functions are masking out bits from the command addresses, but the cfi_send_gen_cmd() moves the addresses up depending on the chip type, so the masking is not needed.

2) the cfi_send_gen_cmd() is called with CFI_DEVICETYPE_X8 instead of cfi->device_type, which causes the wrong accesses to be generated to the chip.

This is the patch generated from the 03 Sep 2004 CVS drop:

drivers/mtd/chips/jedec_probe.c +44 -24
@@ -8,6 +8,15 @@
    Occasionally maintained by Thayne Harbaugh tharbaugh at lnxi dot com
 */
 
+/* Note from Ben Dooks:
+ *
+ * There was an attempt to mask off unused address bits depending on the
+ * device width (ie, remove A0 for 16bit). This is a proble since the 
+ * command functions shift the addresses up for the chip-width, thus
+ * negating the need to mask, as all bits of the address are presented
+ * to the chip.
+ */
+
 #include <linux/config.h>
 #include <linux/module.h>
 #include <linux/init.h>

@@ -1379,6 +1388,22 @@
 			ERASEINFO(0x01000,256),
 		}
 	}, {
+		.mfr_id		= MANUFACTURER_SST,	/* should be CFI */
+		.dev_id		= SST39LF160,
+		.name		= "SST 39LF160",
+ 		.uaddr		= {
+			[0] = MTD_UADDR_0x5555_0x2AAA,  /* x8 */
+			[1] = MTD_UADDR_0x5555_0x2AAA   /* x16 */
+		},
+		.DevSize	= SIZE_2MiB,
+		.CmdSet		= P_ID_AMD_STD,
+		.NumEraseRegions= 2,
+		.regions	= {
+			ERASEINFO(0x1000,256),
+			ERASEINFO(0x1000,256)
+		}
+
+	}, {
 		.mfr_id		= MANUFACTURER_ST,	/* FIXME - CFI device? */
 		.dev_id		= M29W800DT,
 		.name		= "ST M29W800DT",

@@ -1654,8 +1679,8 @@
 	 * the F0 is written to */
 	if(cfi->addr_unlock1) {
 		/*printk("reset unlock called %x %x \n",cfi->addr_unlock1,cfi->addr_unlock2);*/
-		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(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(0xF0, cfi->addr_unlock1, base, map, cfi, cfi->device_type, NULL);

@@ -1700,7 +1725,6 @@
 static int cfi_jedec_setup(struct cfi_private *p_cfi, int index)
 {
 	int i,num_erase_regions;
-	unsigned long mask;
 	__u8 uaddr;
 
 	printk("Found: %s\n",jedec_table[index].name);

@@ -1735,10 +1759,11 @@
 		return 0;
 	}
 
-	/* Mask out address bits which are smaller than the device type */
-	mask = ~(p_cfi->device_type-1);
-	p_cfi->addr_unlock1 = unlock_addrs[uaddr].addr1 & mask;
-	p_cfi->addr_unlock2 = unlock_addrs[uaddr].addr2 & mask;
+	/* Do not mask out `unused bits` due to the command function
+	 * shifting the addresses depdning on the chip type */
+
+	p_cfi->addr_unlock1 = unlock_addrs[uaddr].addr1;
+	p_cfi->addr_unlock2 = unlock_addrs[uaddr].addr2;
 
 	return 1; 	/* ok */
 }

@@ -1759,7 +1784,6 @@
 	int rc = 0;           /* failure until all tests pass */
 	u32 mfr, id;
 	__u8 uaddr;
-	unsigned long mask;
 
 	/*
 	 * The IDs must match.  For X16 and X32 devices operating in

@@ -1810,18 +1834,17 @@
 		goto match_done;
 	}
 
-	mask = ~(cfi->device_type-1);
 
 	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 & mask) != cfi->addr_unlock1 ||
-		  (unlock_addrs[uaddr].addr2 & mask) != cfi->addr_unlock2 ) ) {
+	     && ( unlock_addrs[uaddr].addr1 != cfi->addr_unlock1 ||
+		  unlock_addrs[uaddr].addr2 != cfi->addr_unlock2 ) ) {
 		DEBUG( MTD_DEBUG_LEVEL3,
 		       "MTD %s(): 0x%.4lx 0x%.4lx did not match\n",
 		       __func__,
-		       unlock_addrs[uaddr].addr1 & mask,
-		       unlock_addrs[uaddr].addr2 & mask);
+		       unlock_addrs[uaddr].addr1,
+		       unlock_addrs[uaddr].addr2);
 		goto match_done;
 	}
 

@@ -1857,10 +1880,10 @@
 	 */
 	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_DEVICETYPE_X8, NULL);
-		cfi_send_gen_cmd(0x55, cfi->addr_unlock2, base, map, cfi, CFI_DEVICETYPE_X8, NULL);
+		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(0x90, cfi->addr_unlock1, base, map, cfi, CFI_DEVICETYPE_X8, NULL);
+	cfi_send_gen_cmd(0x90, cfi->addr_unlock1, base, map, cfi, cfi->device_type, NULL);
 	/* FIXME - should have a delay before continuing */
 
  match_done:	

@@ -1876,16 +1899,13 @@
 
  retry:
 	if (!cfi->numchips) {
-		unsigned long mask = ~(cfi->device_type-1);
-
 		uaddr_idx++;
 
 		if (MTD_UADDR_UNNECESSARY == uaddr_idx)
 			return 0;
 
-		/* Mask out address bits which are smaller than the device type */
-		cfi->addr_unlock1 = unlock_addrs[uaddr_idx].addr1 & mask;
-		cfi->addr_unlock2 = unlock_addrs[uaddr_idx].addr2 & mask;
+		cfi->addr_unlock1 = unlock_addrs[uaddr_idx].addr1;
+		cfi->addr_unlock2 = unlock_addrs[uaddr_idx].addr2;
 	}
 
 	/* Make certain we aren't probing past the end of map */

@@ -1916,10 +1936,10 @@
 
 	/* Autoselect Mode */
 	if(cfi->addr_unlock1) {
-		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(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(0x90, cfi->addr_unlock1, base, map, cfi, CFI_DEVICETYPE_X8, NULL);
+	cfi_send_gen_cmd(0x90, cfi->addr_unlock1, base, map, cfi, cfi->device_type, NULL);
 	/* FIXME - should have a delay before continuing */
 
 	if (!cfi->numchips) {
#1 lore raw
Quoted 7 lines · Ben Dooks

I have been trying to get an SST 39LF160 on an Simtec Electronics EB2410ITX (aka Bast). I have found some problems with jedec_probe with non-8bit devices.

1) A number of functions are masking out bits from the command addresses, but the cfi_send_gen_cmd() moves the addresses up depending on the chip type, so the masking is not needed.

Dave, can you comment on this? It seems like it was something that you worked with - something about not probing at unaligned addresses?

Quoted 3 lines

2) the cfi_send_gen_cmd() is called with CFI_DEVICETYPE_X8 instead of cfi->device_type, which causes the wrong accesses to be generated to the chip.

It seems like the CFI_DEVICETYPE_X8 was changed to cfi->device_type at one point and it broke some 16-bit chips.

I think all of the CFI_DEVICETYPE_X8s were removed from jedec_probe.c 1.30, but then some were added back in at 1.36. I'm trying to find the discussion as to why this happened, but what I'm finding isn't very descriptive:

http://lists.infradead.org/pipermail/linux-mtd/2003-October/008685.html

I seem to remember IRC'ing with Christopher Hoover on this. Maybe he can remember something? Christopher? My memory is somewhat murky.

#2 lore raw
Quoted 11 lines · Thayne Harbaugh
Quoted 7 lines · Ben Dooks

I have been trying to get an SST 39LF160 on an Simtec Electronics EB2410ITX (aka Bast). I have found some problems with jedec_probe with non-8bit devices.

1) A number of functions are masking out bits from the command addresses, but the cfi_send_gen_cmd() moves the addresses up depending on the chip type, so the masking is not needed.

Dave, can you comment on this? It seems like it was something that you worked with - something about not probing at unaligned addresses?

The probe code used to probe for impossible combinations at unaligned addresses. So people hacked around it by masking out the lower bits. Now we fixed the probe code. Those hacks can go.

Quoted 11 lines
Quoted 3 lines

2) the cfi_send_gen_cmd() is called with CFI_DEVICETYPE_X8 instead of cfi->device_type, which causes the wrong accesses to be generated to the chip.

It seems like the CFI_DEVICETYPE_X8 was changed to cfi->device_type at one point and it broke some 16-bit chips.

I think all of the CFI_DEVICETYPE_X8s were removed from jedec_probe.c 1.30, but then some were added back in at 1.36. I'm trying to find the discussion as to why this happened, but what I'm finding isn't very descriptive:

We need to agree --- either we can use cfi->device_type for all, or we use CFI_DEVICETYPE_X8 for all, and we set the uaddr in the device table accordingly.

I think we had CFI_DEVICETYPE_X8 so that we could hard-code different addresses for different modes of the same chip, on the basis that the addresses couldn't be calculated from a single entry in the chip table and the cfi->device_type.

I think that's unnecessary though -- I think we should have only a _single_ uaddr field in the device table, and use cfi->device_type with it.

Signature · 1 line
dwmw2
#3 lore raw
In reply to Thayne Harbaugh
Quoted 10 lines

It seems like the CFI_DEVICETYPE_X8 was changed to cfi->device_type at one point and it broke some 16-bit chips.

I think all of the CFI_DEVICETYPE_X8s were removed from jedec_probe.c 1.30, but then some were added back in at 1.36. I'm trying to find the
discussion as to why this happened, but what I'm finding isn't very descriptive:

Quoted 5 lines

http://lists.infradead.org/pipermail/linux-mtd/2003-October/00
8685.html

I seem to remember IRC'ing with Christopher Hoover on this. Maybe he can remember something? Christopher? My memory is somewhat murky.

I don't have any new information to report -- only what is noted above, that some regression in the mtd tree about the time the CFI_DEVICETYPE_X8's disappeared. It was never fixed while I worked with the board. It may be fine now, but unfortunately I am unable to check easily.

Can we locate someone with a suitable platform and coax them into testing the latest code?

-ch

#4 lore raw
In reply to Ben Dooks
Quoted 6 lines · Ben Dooks

I have been trying to get an SST 39LF160 on an Simtec Electronics EB2410ITX (aka Bast). I have found some problems with jedec_probe with non-8bit devices.

Basically, the two problems are as follows:

Quoted 7 lines

1) A number of functions are masking out bits from the command addresses, but the cfi_send_gen_cmd() moves the addresses up depending on the chip type, so the masking is not needed.

2) the cfi_send_gen_cmd() is called with CFI_DEVICETYPE_X8 instead of cfi->device_type, which causes the wrong accesses to be generated to the chip.

committed patch - please try it.

author
#5 lore raw
Quoted 11 lines · Thayne Harbaugh
Quoted 10 lines · Ben Dooks

I have been trying to get an SST 39LF160 on an Simtec Electronics EB2410ITX (aka Bast). I have found some problems with jedec_probe with non-8bit devices.

Basically, the two problems are as follows:

1) A number of functions are masking out bits from the command addresses, but the cfi_send_gen_cmd() moves the addresses up depending on the chip type, so the masking is not needed.

Quoted 5 lines
Quoted 3 lines

2) the cfi_send_gen_cmd() is called with CFI_DEVICETYPE_X8 instead of cfi->device_type, which causes the wrong accesses to be generated to the chip.

committed patch - please try it.

I'll try this on my EB2410ITX as soon as I can, would like to polish the NOR flash driver and get it uploaded.

Signature · 3 lines
Ben ([email protected], http://www.fluff.org/)

  'a smiley only costs 4 bytes'