Regression of mtd/chips/jedec_probe.c in 2.6.0-test7

Regression of mtd/chips/jedec_probe.c in 2.6.0-test7

[Christopher Hoover]

Perhaps this is already known, but it may serve as useful news to some. 

The version of mtd/chips/jedec_probe.c in 2.6.0-test7 (revision 1.29
according to $Id$) has suffered a regression.  That version fails to detect
AMD AM29LV800BB flash in 16-bit x 1 configuration.  Replacing it with
revision 1.32 from CVS or downgrading to revision 1.6 from 2.5.70-rmk1,
holding everything else constant, solves the problem.

Also, all versions of mtd/chips/jedec_probe.c need:

#include <linux/init.h>

for 2.6.0-test7.  (I cannot ssh from my current network location, otherwise
I'd make this change to CVS.)

Cheers,
-ch
ch(at)murgatroid.com
ch(at)hpl.hp.com

Re: Regression of mtd/chips/jedec_probe.c in 2.6.0-test7

[Thayne Harbaugh]

On Wed, 2003-10-15 at 17:46, Christopher Hoover wrote:
> Perhaps this is already known, but it may serve as useful news to some. 
> 
> The version of mtd/chips/jedec_probe.c in 2.6.0-test7 (revision 1.29
> according to $Id$) has suffered a regression.  That version fails to detect
> AMD AM29LV800BB flash in 16-bit x 1 configuration.  Replacing it with
> revision 1.32 from CVS or downgrading to revision 1.6 from 2.5.70-rmk1,
> holding everything else constant, solves the problem.

Does that mean that 1.29 and 1.7 didn't work or did you just try those
versions because they were convenient for your?

> Also, all versions of mtd/chips/jedec_probe.c need:
> 
> #include <linux/init.h>

I fixed it moments before I read your email - thanks.

I'm concerned that the changes from 1.29 -> 1.33 are significant and
will meet resistance for inclusion in 2.6.0.  If you can narrow the
problem down to just a few lines that are obviously incorrect then the
patch should have a fighting chance.  There is a section that is an
obvious bug in jedec_match() that should be fixed:

@@ -1480,7 +1524,7 @@
 	DEBUG( MTD_DEBUG_LEVEL3,
 	       "MTD %s(): Check fit 0x%.8x + 0x%.8x = 0x%.8x\n",
 	       __func__, base, 1 << finfo->DevSize, base + (1 <<
finfo->DevSize) );
-	if ( base + ( 1 << finfo->DevSize ) > map->size ) {
+	if ( base + cfi->interleave * ( 1 << finfo->DevSize ) > map->size ) {
 		DEBUG( MTD_DEBUG_LEVEL3,
 		       "MTD %s(): 0x%.4x 0x%.4x %dKiB doesn't fit\n",
 		       __func__, finfo->mfr_id, finfo->dev_id,

The most likely problem, however is this chunk:

@@ -1538,10 +1582,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:	


Of course I can point to a few more chunks that should go into 2.6 (and
2.4 obviously), but the arguments are much weaker about why they have to
go in *now*.

If you figure out exactly what the regression is then we should lob it
at lkml.

Once again, thanks.

RE: Regression of mtd/chips/jedec_probe.c in 2.6.0-test7

[Christopher Hoover]

> If you can narrow the problem down to just a few lines that 
> are obviously incorrect then the patch should have a fighting 
> chance.

Here's what I tried:

--- linux-2.6.0-test8-rmk1/drivers/mtd/chips/jedec_probe.c	2003-10-20
22:19:24.000000000 -0700
+++ linux-2.6.0-test8-rmk1-ceiva1/drivers/mtd/chips/jedec_probe.c
2003-10-22 15:24:55.000000000 -0700
@@ -1481,7 +1481,7 @@
 	DEBUG( MTD_DEBUG_LEVEL3,
 	       "MTD %s(): Check fit 0x%.8x + 0x%.8x = 0x%.8x\n",
 	       __func__, base, 1 << finfo->DevSize, base + (1 <<
finfo->DevSize) );
-	if ( base + ( 1 << finfo->DevSize ) > map->size ) {
+	if ( base + cfi->interleave * ( 1 << finfo->DevSize ) > map->size )
{
 		DEBUG( MTD_DEBUG_LEVEL3,
 		       "MTD %s(): 0x%.4x 0x%.4x %dKiB doesn't fit\n",
 		       __func__, finfo->mfr_id, finfo->dev_id,
@@ -1539,10 +1539,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:	

But it fails to find the chip:

Uncompressing Linux................................... done, booting the
kernel.Linux version 2.6.0-test8-rmk1-ceiva1 (ch@laptop-vmware) (gcc version
3.3.1) #33CPU: ARM720T [41807202] revision 2 (ARMv4T)
Machine: CEIVA/Polaroid Photo MAX Digital Picture Frame
Memory policy: ECC disabled, Data cache write back
On node 0 totalpages: 1024
  DMA zone: 1024 pages, LIFO batch:1
  Normal zone: 0 pages, LIFO batch:1
  HighMem zone: 0 pages, LIFO batch:1
Building zonelist for node : 0
Kernel command line: root=/dev/mtdblock3 rootfstype=cramfs console=ttyCL0
debug
Relocating machine vectors to 0xffff0000
PID hash table entries: 32 (order 5: 256 bytes)
Memory: 4MB = 4MB total
Memory: 2908KB available (921K code, 116K data, 52K init)
Calibrating delay loop... 36.76 BogoMIPS
Dentry cache hash table entries: 1024 (order: 0, 4096 bytes)
Inode-cache hash table entries: 1024 (order: 0, 4096 bytes)
Mount-cache hash table entries: 512 (order: 0, 4096 bytes)
CPU: Testing write buffer coherency: ok
POSIX conformance testing by UNIFIX
NET: Registered protocol family 16
Bluetooth: Core ver 2.3
NET: Registered protocol family 31
Bluetooth: HCI device and connection manager initialized
Bluetooth: HCI socket layer initialized
epson1355fb: regs mapped at 0xc0800000, fb 2048 KiB mapped at 0xc0802000
epson1355fb: xres=640, yres=480, is_color=1, is_dual=1, is_tft=0
epson1355fb: bpp=16, lcd_bpp=16, crt_enabled=0, lcd_enabled=1
fb0: S1D13505 frame buffer device
Fast Floating Point Emulator V0.9 (c) Peter Teichmann.
JFFS2 version 2.2. (C) 2001-2003 Red Hat, Inc.
Serial: CLPS711x driver $Revision: 1.42 $
ttyCL0 at I/O 0x100 (irq = 12) is a CLPS711x
ttyCL1 at I/O 0x1100 (irq = 28) is a CLPS711x
Using noop io scheduler
Search for id:(01 225b) interleave(1) type(2)
Search for id:(01 225b) interleave(1) type(2)
Search for id:(70 e3a0) interleave(1) type(2)
Search for id:(01 5b) interleave(2) type(1)
Search for id:(01 5b) interleave(2) type(1)
Search for id:(70 a0) interleave(2) type(1)
Search for id:(70 f10) interleave(2) type(2)
Search for id:(70 f10) interleave(2) type(2)
Search for id:(70 f10) interleave(2) type(2)
JEDEC: Found no ceiva flash device at location zero
clps_setup_mtd: do_map_probe failed to find flash
Bluetooth: HCI UART driver ver 2.1
Bluetooth: HCI H4 protocol initialized
NET: Registered protocol family 1
Bluetooth: L2CAP ver 2.1
Bluetooth: L2CAP socket layer initialized
Bluetooth: RFCOMM ver 1.0
Bluetooth: RFCOMM socket layer initialized
Bluetooth: RFCOMM TTY layer initialized
VFS: Cannot open root device "mtdblock3" or unknown-block(0,0)
Please append a correct "root=" boot option
Kernel panic: VFS: Unable to mount root fs on unknown-block(0,0)



Is there something else you'd like me to try to narrow this down?

-ch

RE: Regression of mtd/chips/jedec_probe.c in 2.6.0-test7

[Thayne Harbaugh]

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

On Wed, 2003-10-22 at 16:33, Christopher Hoover wrote:
> > If you can narrow the problem down to just a few lines that 
> > are obviously incorrect then the patch should have a fighting 
> > chance.
> 
> Here's what I tried:
> 
> --- linux-2.6.0-test8-rmk1/drivers/mtd/chips/jedec_probe.c	2003-10-20
> 22:19:24.000000000 -0700
> +++ linux-2.6.0-test8-rmk1-ceiva1/drivers/mtd/chips/jedec_probe.c
> 2003-10-22 15:24:55.000000000 -0700
> @@ -1481,7 +1481,7 @@
>  	DEBUG( MTD_DEBUG_LEVEL3,
>  	       "MTD %s(): Check fit 0x%.8x + 0x%.8x = 0x%.8x\n",
>  	       __func__, base, 1 << finfo->DevSize, base + (1 <<
> finfo->DevSize) );
> -	if ( base + ( 1 << finfo->DevSize ) > map->size ) {
> +	if ( base + cfi->interleave * ( 1 << finfo->DevSize ) > map->size )
> {
>  		DEBUG( MTD_DEBUG_LEVEL3,
>  		       "MTD %s(): 0x%.4x 0x%.4x %dKiB doesn't fit\n",
>  		       __func__, finfo->mfr_id, finfo->dev_id,

This is needed.

> @@ -1539,10 +1539,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:	

There's one more fragment where cfi->device_type should revert back to
CFI_DEVICETYPE_X8

@@ -1674,10 +1674,10 @@
 
 	/* 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) {


Let me know if this fixes it.

-- 
Thayne Harbaugh
Linux Networx

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

RE: Regression of mtd/chips/jedec_probe.c in 2.6.0-test7

[Christopher Hoover]

> There's one more fragment where cfi->device_type should
> revert back to CFI_DEVICETYPE_X8

OK, now I'm confused.  Revert *back* to CFI_DEVICETYPE_X8?

I though we were going the other direction.

Appended is the last diff (n.b., original vs new) I tried.  It changes that
last fragment *from* CFI_DEVICETYPE_X8 *to* cfi->device_type -- it still
fails.  

The boot messages show different ids:

Search for id:(70 e3a0) interleave(1) type(2)
Search for id:(70 e3a0) interleave(1) type(2)
Search for id:(70 e3a0) interleave(1) type(2)
Search for id:(01 5b) interleave(2) type(1)
Search for id:(01 5b) interleave(2) type(1)
Search for id:(70 a0) interleave(2) type(1)
Search for id:(70 f10) interleave(2) type(2)
Search for id:(70 f10) interleave(2) type(2)
Search for id:(70 f10) interleave(2) type(2)
JEDEC: Found no ceiva flash device at location zero
clps_setup_mtd: do_map_probe failed to find flash

And before we found it fails with that last fragment left as is (i.e.
CFI_DEVICETYPE_X8).

-ch


--- linux-2.6.0-test8-rmk1/drivers/mtd/chips/jedec_probe.c	2003-10-20
22:19:24.000000000 -0700
+++ linux-2.6.0-test8-rmk1-ceiva1/drivers/mtd/chips/jedec_probe.c
2003-10-22 17:26:35.000000000 -0700
@@ -1481,7 +1481,7 @@
 	DEBUG( MTD_DEBUG_LEVEL3,
 	       "MTD %s(): Check fit 0x%.8x + 0x%.8x = 0x%.8x\n",
 	       __func__, base, 1 << finfo->DevSize, base + (1 <<
finfo->DevSize) );
-	if ( base + ( 1 << finfo->DevSize ) > map->size ) {
+	if ( base + cfi->interleave * ( 1 << finfo->DevSize ) > map->size )
{
 		DEBUG( MTD_DEBUG_LEVEL3,
 		       "MTD %s(): 0x%.4x 0x%.4x %dKiB doesn't fit\n",
 		       __func__, finfo->mfr_id, finfo->dev_id,
@@ -1539,10 +1539,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:	
@@ -1614,10 +1614,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) {

RE: Regression of mtd/chips/jedec_probe.c in 2.6.0-test7

[Thayne Harbaugh]

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

On Wed, 2003-10-22 at 18:43, Christopher Hoover wrote:
> > There's one more fragment where cfi->device_type should
> > revert back to CFI_DEVICETYPE_X8
> 
> OK, now I'm confused.  Revert *back* to CFI_DEVICETYPE_X8?
> 
> I though we were going the other direction.

Okay, I didn't read the patch hunks very closely.  We really want to
move *from* cfi->device_type *to* CFI_DEVICETYPE_X8.  Do me a favor,
check out jedec_probe.c v1.36 from CVS and try that - yes, I know,
you've already mentioned that v1.32 worked, but trying 1.36 will be
helpful.

-- 
Thayne Harbaugh
Linux Networx

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

RE: Regression of mtd/chips/jedec_probe.c in 2.6.0-test7

[Thayne Harbaugh]

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

Now that I've been unhelpful, if not confusing, I figure I should look
at your emails a bit closer.

On Wed, 2003-10-22 at 16:33, Christopher Hoover wrote:

> Search for id:(01 225b) interleave(1) type(2)
> Search for id:(01 225b) interleave(1) type(2)

This is the manufacturer and device id for the AM29LV800BB.

01 = AMD
225b = AM29LV800BB in x16 mode.

If that's correct, why doesn't it find it?  Have you tried turning on
debug and setting it to 3?

> Search for id:(70 e3a0) interleave(1) type(2)
> Search for id:(01 5b) interleave(2) type(1)
> Search for id:(01 5b) interleave(2) type(1)

01 = AMD
5b = AM29LV800BB in x8 mode.

Once again, why isn't the chip found?

> Search for id:(70 a0) interleave(2) type(1)
> Search for id:(70 f10) interleave(2) type(2)
> Search for id:(70 f10) interleave(2) type(2)
> Search for id:(70 f10) interleave(2) type(2)
> JEDEC: Found no ceiva flash device at location zero
                  ^^^^^
Looks like a Polaroid camera?

> clps_setup_mtd: do_map_probe failed to find flash

We nee the debug verbosity turned up for jedec_probe.  Hopefully that
will tell us why jedec_match() (I believe) is failing.

-- 
Thayne Harbaugh
Linux Networx

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]