Fix to jedec_probe unlock addresses

Fix to jedec_probe unlock addresses

[Ben Dooks]

Fix unlock address calculation for non-x8 chips for the
cfi cmdset code, which assumes always X8

Patch against 2.6.9-rc2 with 19th September CVS

Signed-off-by: Ben Dooks <ben-mtd@fluff.org>

--- linux-2.6.9-rc2-bk6-mtd20040919/drivers/mtd/chips/jedec_probe.c	2004-09-20 13:02:46.000000000 +0100
+++ linux-2.6.9-rc2-bk6-mtd20040919-work/drivers/mtd/chips/jedec_probe.c	2004-09-20 23:21:40.000000000 +0100
@@ -2046,7 +2046,15 @@
 	printk(KERN_INFO "%s: Found %d x%d devices at 0x%x in %d-bit bank\n",
 	       map->name, cfi_interleave(cfi), cfi->device_type*8, base, 
 	       map->bankwidth*8);
-	
+
+	/* fixup unlock addresses for the cmdset */
+
+	cfi->addr_unlock1 *= cfi_interleave(cfi) * cfi->device_type;
+	cfi->addr_unlock2 *= cfi_interleave(cfi) * cfi->device_type;
+
+	printk(KERN_DEBUG "unlocks at %08x,%08x\n", 
+	       cfi->addr_unlock1, cfi->addr_unlock2);
+
 	return 1;
 }
 

Re: Fix to jedec_probe unlock addresses

[Eric W. Biederman]

Ben Dooks <ben-mtd@fluff.org> writes:

> Fix unlock address calculation for non-x8 chips for the
> cfi cmdset code, which assumes always X8
> 
> Patch against 2.6.9-rc2 with 19th September CVS
> 
> Signed-off-by: Ben Dooks <ben-mtd@fluff.org>
> 
> --- linux-2.6.9-rc2-bk6-mtd20040919/drivers/mtd/chips/jedec_probe.c 2004-09-20
> 13:02:46.000000000 +0100
> 
> +++ linux-2.6.9-rc2-bk6-mtd20040919-work/drivers/mtd/chips/jedec_probe.c
> 2004-09-20 23:21:40.000000000 +0100
> 
> @@ -2046,7 +2046,15 @@
>  	printk(KERN_INFO "%s: Found %d x%d devices at 0x%x in %d-bit bank\n",
>  	       map->name, cfi_interleave(cfi), cfi->device_type*8, base, 
>  	       map->bankwidth*8);
> -	
> +
> +	/* fixup unlock addresses for the cmdset */
> +
> +	cfi->addr_unlock1 *= cfi_interleave(cfi) * cfi->device_type;
> +	cfi->addr_unlock2 *= cfi_interleave(cfi) * cfi->device_type;
> +
> +	printk(KERN_DEBUG "unlocks at %08x,%08x\n", 
> +	       cfi->addr_unlock1, cfi->addr_unlock2);
> +
>  	return 1;
>  }

Hmm.  I think that points out a real issue but I think the bug actually
lies in cfi_cmdset_0002.  

That fix makes below comment from cfi_cmdset_0002 clearly wrong.
	/*
	 * The CFI_DEVICETYPE_X8 argument is needed even when
	 * cfi->device_type != CFI_DEVICETYPE_X8.  The addresses for
	 * command sequences don't scale even when the device is
	 * wider.  This is the case for many of the cfi_send_gen_cmd()
	 * below.  I'm not sure, however, why some use
	 * cfi->device_type.
	 */

So I think it is more likely we need to change this hunk of code below.

		/*
		 * These might already be setup (more correctly) by
		 * jedec_probe.c - still need it for cfi_probe.c path.
		 */
		if ( ! (cfi->addr_unlock1 && cfi->addr_unlock2) ) {
			switch (cfi->device_type) {
			case CFI_DEVICETYPE_X8:
				cfi->addr_unlock1 = 0x555; 
				cfi->addr_unlock2 = 0x2aa; 
				break;
			case CFI_DEVICETYPE_X16:
				cfi->addr_unlock1 = 0xaaa;
				if (map_bankwidth(map) == cfi_interleave(cfi)) {
					/* X16 chip(s) in X8 mode */
					cfi->addr_unlock2 = 0x555;
				} else {
					cfi->addr_unlock2 = 0x554;
				}
				break;
			case CFI_DEVICETYPE_X32:
				cfi->addr_unlock1 = 0x1554;
				if (map_bankwidth(map) == cfi_interleave(cfi)*2) {
					/* X32 chip(s) in X16 mode */
					cfi->addr_unlock1 = 0xaaa;
				} else {
					cfi->addr_unlock2 = 0xaa8; 
				}
				break;
			default:
				printk(KERN_WARNING
				       "MTD %s(): Unsupported device type %d\n",
				       __func__, cfi->device_type);
				kfree(mtd);
				kfree(extp);
				return NULL;
			}
		}

Does anyone have cfi cmdset 0002 devices in an interleaved configuration
who would care to comment?


Eric

Re: Fix to jedec_probe unlock addresses

[Ben Dooks]

On Wed, Sep 22, 2004 at 07:48:40PM -0600, Eric W. Biederman wrote:
> Ben Dooks <ben-mtd@fluff.org> writes:
> 
> > Fix unlock address calculation for non-x8 chips for the
> > cfi cmdset code, which assumes always X8
> > 
> > Patch against 2.6.9-rc2 with 19th September CVS
> > 
> > Signed-off-by: Ben Dooks <ben-mtd@fluff.org>
> > 
> > --- linux-2.6.9-rc2-bk6-mtd20040919/drivers/mtd/chips/jedec_probe.c 2004-09-20
> > 13:02:46.000000000 +0100
> > 
> > +++ linux-2.6.9-rc2-bk6-mtd20040919-work/drivers/mtd/chips/jedec_probe.c
> > 2004-09-20 23:21:40.000000000 +0100
> > 
> > @@ -2046,7 +2046,15 @@
> >  	printk(KERN_INFO "%s: Found %d x%d devices at 0x%x in %d-bit bank\n",
> >  	       map->name, cfi_interleave(cfi), cfi->device_type*8, base, 
> >  	       map->bankwidth*8);
> > -	
> > +
> > +	/* fixup unlock addresses for the cmdset */
> > +
> > +	cfi->addr_unlock1 *= cfi_interleave(cfi) * cfi->device_type;
> > +	cfi->addr_unlock2 *= cfi_interleave(cfi) * cfi->device_type;
> > +
> > +	printk(KERN_DEBUG "unlocks at %08x,%08x\n", 
> > +	       cfi->addr_unlock1, cfi->addr_unlock2);
> > +
> >  	return 1;
> >  }
> 
> Hmm.  I think that points out a real issue but I think the bug actually
> lies in cfi_cmdset_0002.  
> 
> That fix makes below comment from cfi_cmdset_0002 clearly wrong.
> 	/*
> 	 * The CFI_DEVICETYPE_X8 argument is needed even when
> 	 * cfi->device_type != CFI_DEVICETYPE_X8.  The addresses for
> 	 * command sequences don't scale even when the device is
> 	 * wider.  This is the case for many of the cfi_send_gen_cmd()
> 	 * below.  I'm not sure, however, why some use
> 	 * cfi->device_type.
> 	 */

[snip]
 
> Does anyone have cfi cmdset 0002 devices in an interleaved configuration
> who would care to comment?

I took the jedec_probe alterations as I was unsure wether I was going
to damage anything else by changing the cfi cmdset 0002 code.

-- 
Ben (ben@fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

Re: Fix to jedec_probe unlock addresses

[Eric W. Biederman]

Ben Dooks <ben-mtd@fluff.org> writes:

> I took the jedec_probe alterations as I was unsure wether I was going
> to damage anything else by changing the cfi cmdset 0002 code.

Your patch is broken with respect to interleaving.  The interleave
gets applied twice.  Once in jedec_probe, and then again in
cfi_send_gen_cmd.


The bottom line is that we need to be consistent with how we do this.
Either we have magic addresses specified in terms as byte addresses
or we have magic addresses specified as device visible addresses.

Right now cfi_cmdset_0002 is the only place that does not use the
device width.

A 16bit device should not even be able to see a non-16bit 
aligned address, and we have to play games with the address
on the bus to handle interleaving so I don't see a reason
why we would want to disable multiplying by device size.

So it looks like the TODO item is to just look up what cfi standard
says are the unlock addresses for cfi command set 2. And make certain
the cfi hard codes are correct and nothing should be broken.

But even without that I would rather see cfi_cmdset_0002 fixed
then seeing this disagreement about how we should pass parameters
to cfi_cmdset_0002.  At least if we agree on conventions the broken
cases can be incrementally fixed until the code works properly for
everyone.

It is nice to see that this problem has nothing to do with
interleaving so that anyone with a 16bit or wider device can test
this.

Eric

[CFT] FIX CFI cmdset 0002 for x16 and x32 devices.

[Eric W. Biederman]

ebiederman@lnxi.com (Eric W. Biederman) writes:

> So it looks like the TODO item is to just look up what cfi standard
> says are the unlock addresses for cfi command set 2. And make certain
> the cfi hard codes are correct and nothing should be broken.

Looking at cfi specification and the AMD/Fujitsu specific
portion of it.  This is what the CFI address assignment code
needs to look like:

		/* 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 == 2)) ||
			/* x32 in x16 mode */
			((cfi->device_type == CFI_DEVICETYPE_X16) &&
				(cfi->cfiq->InterfaceDesc == 4))) 
		{
			cfi->addr_unlock1 = 0xaaa;
			cfi->addr_unlock2 = 0x555;
		}


The previous code to handle this was quite bogus.  Unless I completely
cannot read the code.

I have committed this change and corresponding change to use
cfi_send_gen_cmd.

Will people please test this?

If I don't hear any bug reports I will assume everything is perfect
and start pushing this code upstream.  

Eric

Re: [CFT] FIX CFI cmdset 0002 for x16 and x32 devices.

[Ben Dooks]

On Thu, Sep 23, 2004 at 10:29:03PM -0600, Eric W. Biederman wrote:
> ebiederman@lnxi.com (Eric W. Biederman) writes:
> 
> > So it looks like the TODO item is to just look up what cfi standard
> > says are the unlock addresses for cfi command set 2. And make certain
> > the cfi hard codes are correct and nothing should be broken.
> 
> Looking at cfi specification and the AMD/Fujitsu specific
> portion of it.  This is what the CFI address assignment code
> needs to look like:
> 
> 		/* 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 == 2)) ||
> 			/* x32 in x16 mode */
> 			((cfi->device_type == CFI_DEVICETYPE_X16) &&
> 				(cfi->cfiq->InterfaceDesc == 4))) 
> 		{
> 			cfi->addr_unlock1 = 0xaaa;
> 			cfi->addr_unlock2 = 0x555;
> 		}
> 
> 
> The previous code to handle this was quite bogus.  Unless I completely
> cannot read the code.
> 
> I have committed this change and corresponding change to use
> cfi_send_gen_cmd.
> 
> Will people please test this?

I've just had a chance to test this on my EB2410ITX (BAST) and it
seems to work (erased and then zeroed the spare partition). I'll
try and bootloader upgraded with it later.


-- 
Ben (ben@fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'