JEDEC probing redux

JEDEC probing redux

[Joshua Wise]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Ah, the never-ending search for the working flash chip on the iPAQ h1910. I 
think I know what the problem is now, but I'd like someone else to confirm my 
theory ...

But first, some background. On the iPAQ h1910, we have an LV400BT chip (id 
0001/22B9) that we probe by JEDEC. The chip expects unlock addresses of 
0x555/0x2AA in word mode.

However, HP had to throw a twist in - they shifted the address lines over one. 
A1 on the CPU is connected to A0 on the flash chip, etcetera, so we have to 
left-shift the unlock addresses. I added code to jedec_probe.c to do that in 
a retry loop not unlike what we do to find the actual unlock addresses, and 
it seemed to detect the chip. However, jedec_match() seems to be failing, as 
I'm actually left-shifting the unlock addresses in the CFI structure. (IE, 
jedec_match() expects unlock addresses 0x555 0x2AA for this chip, but it's 
seeing 0xAAA 0x554.)

I'm thinking two things here: 1) I'm assuming that the unlock address verify 
is just paranoia. Can I nuke that? (Will it break anyone's board?) and 2) Is 
there a better way to say that we should be left-shifting all the unlock 
addresses over?

In the mean time, I'll nuke the unlock address verify in jedec_match, and see 
if that solves my problem.

/joshua

(For those who want my evidence, it follows. I've forced the JEDEC code to 
only probe the uaddr_idx that I'm looking for right now, just to save debug 
output. Lines beginning with --- denote my commentary.)

<5>iPAQ flash: probing 16-bit flash bus, window=c4851000 with JEDEC.
<5>uaddr_idx ok, go go go!
- --- Alright, our uaddr_idx is ok. We start probing now.
<7>cfi_send_gen_cmd: writing 00F0@00000000
<7>cfi_send_gen_cmd: writing 00FF@00000000
<7>cfi_send_gen_cmd: writing 00AA@00000555
<7>cfi_send_gen_cmd: writing 0055@000002AA
<7>cfi_send_gen_cmd: writing 0090@00000555
- --- This is my instrumentation, fwiw. I found it helpful.
- --- These would be correct addresses if the chip was straight-through.
- --- But it isn't, so later, we left-shift ....
<6>Search for id:(75 ea00) interleave(1) type(2)
<7>cfi_send_gen_cmd: writing 00F0@00000000
<7>cfi_send_gen_cmd: writing 00FF@00000000
<7>cfi_send_gen_cmd: writing 00AA@00000AAA
<7>cfi_send_gen_cmd: writing 0055@00000554
<7>cfi_send_gen_cmd: writing 0090@00000AAA
- --- These are what I've really got to write.
<6>Search for id:(01 22b9) interleave(1) type(2)
- --- And what do you know, it worked.
<6>MTD jedec_match(): Check fit 0x00000000 + 0x00080000 = 0x00080000
<6>MTD jedec_match(): check unlock addrs 0x0aaa 0x0554
<6>MTD jedec_match(): 0x0555 0x02aa did not match
- --- ... At least until jedec_match() puked.

- -- 
Joshua Wise | www.joshuawise.com
GPG Key     | 0xEA80E0B3
Quote       | <lilo> I akilled *@* by mistake
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.2 (GNU/Linux)

iD8DBQE/tcMiPn9tWOqA4LMRAsboAJwNPmh8KBSs9429JwZdQ6KK4NjGLgCdH/qk
OaCN+d/HAgsmpv7cfIAqxLY=
=KjwV
-----END PGP SIGNATURE-----

Re: JEDEC probing redux

[Joshua Wise]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

> In the mean time, I'll nuke the unlock address verify in jedec_match, and
> see if that solves my problem.

I decided to go with option three out of two and just add a check for addr >> 
1 in jedec_match, so I don't have to worry about Stuff Going Boom. I'll clean 
it all up and check it in to hh.org tonight.

/joshua
- -- 
Joshua Wise | www.joshuawise.com
GPG Key     | 0xEA80E0B3
Quote       | <lilo> I akilled *@* by mistake
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.2 (GNU/Linux)

iD8DBQE/tce1Pn9tWOqA4LMRAihSAJ9/+m6QCxIRGLe7Sd71bfLrG0KjSQCeJyeO
fMDejdjv8ucdgHcjjGSGcsA=
=q4CV
-----END PGP SIGNATURE-----

Re: JEDEC probing redux

[Eric W. Biederman]

Joshua Wise <joshua@joshuawise.com> writes:

> I'm thinking two things here: 1) I'm assuming that the unlock address verify 
> is just paranoia. Can I nuke that? (Will it break anyone's board?)

It will break things.  Some flash chips will ID but nothing else with
the wrong unlock address.  So without the test some boards will not
work.

> and 2) Is 
> there a better way to say that we should be left-shifting all the unlock 
> addresses over?

I am wondering if your map driver could handle that.  Either or
a solution like LART endian might be appropriate.

Handling flash chips that are connected up peculiarly seems to be
a recurring theme.  If this isn't something the map driver can't do
we may want to enhance that interface so it can handle more peculiar
cases in the future.

Eric

Re: JEDEC probing redux

[David Woodhouse]

On Sat, 2003-11-15 at 01:09 -0500, Joshua Wise wrote:
> However, HP had to throw a twist in - they shifted the address lines over one. 
> A1 on the CPU is connected to A0 on the flash chip, etcetera, so we have to 
> left-shift the unlock addresses.

Surely this affects more than just the unlock addresses? Doesn't it mean
that when reading, each 16-bit word of flash would appear _twice_?

What if you just add the appropriate shift to your map driver?

-- 
dwmw2

Re: JEDEC probing redux

[Joshua Wise]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

(oops meant to send this to the list hte first time.)

> Surely this affects more than just the unlock addresses? Doesn't it mean
> that when reading, each 16-bit word of flash would appear _twice_?
No. I can't remember why in this decongestant-drugged state that I'm in
*wheezes loudly*, but I remember that it doesn't. Some quirk that the PXA
does. Check my debrick code in handhelds.org cvs:

http://cvs.handhelds.org/cgi-bin/viewcvs.cgi/debrick/fastjtag/rflash.c?rev=1.8&content-type=text/vnd.viewcvs-markup

Look at the programflash() code in there to see how I do it.

> What if you just add the appropriate shift to your map driver?
Then that'll shift over addresses that aren't unlock, which is definately not
what I want.

For what it's worth, when I added shift code to jedec_probe, that seemed to
work, however at some point my unlock addresses are getting reset (?!!?!) to
555/2aa, instead of 555<<1/2aa<<1. I should probably debug that.

/joshua

- --
Joshua Wise | www.joshuawise.com
GPG Key     | 0xEA80E0B3
Quote       | <lilo> I akilled *@* by mistake
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.2 (GNU/Linux)

iD8DBQE/trp4Pn9tWOqA4LMRAkjdAKCJW73dxpZT3UcC34yhZVtWpN0AcQCghjZy
cSI2GMNqnyQU3OiBhWeV42w=
=xvbs
-----END PGP SIGNATURE-----

Re: JEDEC probing redux

[Joshua Wise]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

(consider information in this post outdated. I'm having mailer issues, sorry.)

> Surely this affects more than just the unlock addresses? Doesn't it mean
> that when reading, each 16-bit word of flash would appear _twice_?
No. I can't remember why in this decongestant-drugged state that I'm in
*wheezes loudly*, but I remember that it doesn't. Some quirk that the PXA
does. Check my debrick code in handhelds.org cvs:

http://cvs.handhelds.org/cgi-bin/viewcvs.cgi/debrick/fastjtag/rflash.c?rev=1.8&content-type=text/vnd.viewcvs-markup

Look at the programflash() code in there to see how I do it.

> What if you just add the appropriate shift to your map driver?
Then that'll shift over addresses that aren't unlock, which is definately not
what I want.

For what it's worth, when I added shift code to jedec_probe, that seemed to
work, however at some point my unlock addresses are getting reset (?!!?!) to
555/2aa, instead of 555<<1/2aa<<1. I should probably debug that.

/joshua

- --
Joshua Wise | www.joshuawise.com
GPG Key     | 0xEA80E0B3
Quote       | <lilo> I akilled *@* by mistake
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.2 (GNU/Linux)

iD8DBQE/t8qCPn9tWOqA4LMRAlUvAJ9vgWGG3hWHxdhNf+PBIw32oLJKGwCfS1AX
0onqXs6f0h3MRE9u8Yy4534=
=EuPq
-----END PGP SIGNATURE-----

Re: JEDEC probing redux

[David Woodhouse]

On Sat, 2003-11-15 at 01:09 -0500, Joshua Wise wrote:
> But first, some background. On the iPAQ h1910, we have an LV400BT chip (id 
> 0001/22B9) that we probe by JEDEC. The chip expects unlock addresses of 
> 0x555/0x2AA in word mode.

> However, HP had to throw a twist in - they shifted the address lines over one. 
> A1 on the CPU is connected to A0 on the flash chip, etcetera, so we have to 
> left-shift the unlock addresses.

No. The chip manufacturer did that. Its 'A0' line is what normal people
would call 'A1', and that's why when you put them in byte mode they also
have an 'A-1' line, for which someone really deserves to burn in hell.

The chip in question wants to see a logical '1' on its A1, A3, A5 etc.
lines for the first unlock cycle, then on A0, A2, A4 etc. for the
second.

That corresponds to the CPU's addresses 0xAAA and 0x554, as you've
observed. In byte mode it's just the _same_, only it also wants a logic
'1' on its 'A-1' address line too, so the latter address is 0x555.

Looking at the tables in jedec_probe.c, mostly of the form...

		.name		= "Fujitsu MBM29LV400BC",
		.uaddr		= {
			[0] = MTD_UADDR_0x0AAA_0x0555,  /* x8 */
			[1] = MTD_UADDR_0x0555_0x02AA,  /* x16 */
		},
... it looks like they're supposed to be shifted by cfi->device_type
before actually being used. But, aside from a brief experiment of
Thayne's, they aren't actually being shifted. So the [1] /* x16 */ entry
is wrong, and should be identical to the [0] /* x8 */ one.

In fact, I assert that the entries for each device_type should _always_
be the same. We shouldn't need an array for uaddr; just a single int
should suffice. Although we do need to mask out the lower bits (0x555
vs. 0x554).

I'm going to assert it thusly...

Thayne? Eric?

Index: jedec_probe.c
===================================================================
RCS file: /home/cvs/mtd/drivers/mtd/chips/jedec_probe.c,v
retrieving revision 1.40
diff -u -r1.40 jedec_probe.c
--- jedec_probe.c	16 Nov 2003 21:42:44 -0000	1.40
+++ jedec_probe.c	16 Nov 2003 22:11:23 -0000
@@ -1510,6 +1510,12 @@
 
 	uaddr = finfo->uaddr[uaddr_idx];
 
+	if (uaddr != MTD_UADDR_NOT_SUPPORTED ) {
+		/* ASSERT("The unlock addresses for non-8-bit mode
+		   are bollocks. We don't really need an array."); */
+		uaddr = finfo->uaddr[0];
+	}
+
  uaddr_done:
 	return uaddr;
 }
@@ -1518,6 +1524,7 @@
 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);
@@ -1551,8 +1558,11 @@
 		kfree( p_cfi->cfiq );
 		return 0;
 	}
-	p_cfi->addr_unlock1 = unlock_addrs[uaddr].addr1;
-	p_cfi->addr_unlock2 = unlock_addrs[uaddr].addr2;
+
+	/* Mask out address bits which are smaller than the device type */
+	mask = ~((1<<p_cfi->device_type)-1);
+	p_cfi->addr_unlock1 = unlock_addrs[uaddr].addr1 & mask;
+	p_cfi->addr_unlock2 = unlock_addrs[uaddr].addr2 & mask;
 
 	return 1; 	/* ok */
 }

-- 
dwmw2

Re: JEDEC probing redux

[Eric W. Biederman]

David Woodhouse <dwmw2@infradead.org> writes:

> On Sat, 2003-11-15 at 01:09 -0500, Joshua Wise wrote:
> No. The chip manufacturer did that. Its 'A0' line is what normal people
> would call 'A1', and that's why when you put them in byte mode they also
> have an 'A-1' line, for which someone really deserves to burn in hell.
> 
> The chip in question wants to see a logical '1' on its A1, A3, A5 etc.
> lines for the first unlock cycle, then on A0, A2, A4 etc. for the
> second.
> 
> That corresponds to the CPU's addresses 0xAAA and 0x554, as you've
> observed. In byte mode it's just the _same_, only it also wants a logic
> '1' on its 'A-1' address line too, so the latter address is 0x555.
> 
> Looking at the tables in jedec_probe.c, mostly of the form...
> 
> 		.name		= "Fujitsu MBM29LV400BC",
> 		.uaddr		= {
> 			[0] = MTD_UADDR_0x0AAA_0x0555,  /* x8 */
> 			[1] = MTD_UADDR_0x0555_0x02AA,  /* x16 */
> 		},
> ... it looks like they're supposed to be shifted by cfi->device_type
> before actually being used. 

That certainly is not the interpretation the current code gives them.

It looks more like the unlock addresses for x16 mode was incorrectly
specified for this new device.

> But, aside from a brief experiment of
> Thayne's, they aren't actually being shifted. So the [1] /* x16 */ entry
> is wrong, and should be identical to the [0] /* x8 */ one.

For this case that sounds right.  For the general case I don't know.
This sounds like an area where people tend to get confused.

> In fact, I assert that the entries for each device_type should _always_
> be the same. We shouldn't need an array for uaddr; just a single int
> should suffice. Although we do need to mask out the lower bits (0x555
> vs. 0x554).

The array of device type gives us information about which widths
the device actually supports, in addition which unlock addresses
are preset.  So there is value there.

As for the general assertion that unlock address are the same
irregardless of width you may have a point.  But until someone does a
thorough documentation search and shows this to be so for the
currently supported devices I am not ready to accept this.

> I'm going to assert it thusly...
> 
> Thayne? Eric?

You are masking the unlock addresses way to late to be of value.
Things need to be masked out during the probe not when we finally
see what works and decide to set it up.  Not that masking should
make any difference when the address line is not hooked up.

So while the analysis sounds fine I for this particular case,
I think the patch is just wrong and should be backed out.

Eric

Re: JEDEC probing redux

[Joshua Wise]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

> You are masking the unlock addresses way to late to be of value.
> Things need to be masked out during the probe not when we finally
> see what works and decide to set it up.  Not that masking should
> make any difference when the address line is not hooked up.
The PXA does weird stuff sometimes. I found that it just Didn't Work when the 
lower line wasn't masked.

> Eric
/joshua

- -- 
Joshua Wise | www.joshuawise.com
GPG Key     | 0xEA80E0B3
Quote       | <lilo> I akilled *@* by mistake
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.2 (GNU/Linux)

iD8DBQE/uCb9Pn9tWOqA4LMRAo9tAJ9ltr1S8NFEMZUmEyYQvfFyNr/V5wCfdk1i
HUMP6lMP/EimCzOWs6zZwnI=
=4LtR
-----END PGP SIGNATURE-----

Re: JEDEC probing redux

[David Woodhouse]

On Sun, 2003-11-16 at 20:40 -0500, Joshua Wise wrote:
> The PXA does weird stuff sometimes. I found that it just Didn't Work when the 
> lower line wasn't masked.

That'll be because you take an alignment trap and don't actually make
the bus cycles you intended.

-- 
dwmw2

Re: JEDEC probing redux

[David Woodhouse]

On Sun, 2003-11-16 at 18:17 -0700, Eric W. Biederman wrote:
> It looks more like the unlock addresses for x16 mode was incorrectly
> specified for this new device.

Well, it was right at the time, because on the day I added it we _were_
shifting unlock addresses by device_type.

> For this case that sounds right.  For the general case I don't know.
> This sounds like an area where people tend to get confused.

The confusion is mostly because of the idiotic nomenclature of the
address lines. You end up with stuff like the Toshiba TC58FVT160
datasheet, which says:
	"The system should generate the following address patterns:
	"Word mode: 555H or 2AAH to addresses A10 to A0.
	"Byte mode: AAAH or 555H to addresses A10 to A-1."

Now couldn't that have been explained more sanely?

> > In fact, I assert that the entries for each device_type should _always_
> > be the same. We shouldn't need an array for uaddr; just a single int
> > should suffice. Although we do need to mask out the lower bits (0x555
> > vs. 0x554).
> 
> The array of device type gives us information about which widths
> the device actually supports, in addition which unlock addresses
> are preset.  So there is value there.
> 
> As for the general assertion that unlock address are the same
> irregardless of width you may have a point.  But until someone does a
> thorough documentation search and shows this to be so for the
> currently supported devices I am not ready to accept this.

Chips which currently have differing unlock addresses in the table:

Toshiba TC58FV[BT]{160,321,641}
Macronix MX29LV160][TB]
Fujitsu MBM29LV{160,320,400,800}xx
AMD AM29LV{160,400,800},AM29F800BT.

I checked the datasheets for the Toshiba TC58FV[BT]160, the Macronix
MX29LV160[BT], the AMD 29LV400, and the Fujitsu MBM29LV160[TB].

They all agree; it's 0x555,0x2AA on A0 upwards in word mode, or
0xAAA,0x555 on A-1 upwards in byte mode -- i.e. the same voltages on the
same pins, except for the logic 1 on A-1 in the second cycle in byte
mode.

One from each family is enough to convince me. 

> > I'm going to assert it thusly...
> > 
> > Thayne? Eric?
> 
> You are masking the unlock addresses way to late to be of value.
> Things need to be masked out during the probe not when we finally
> see what works and decide to set it up. 

True; I fixed that in CVS already.

>  Not that masking should make any difference when the address
>  line is not hooked up.

It triggers an alignment trap, which does two word reads and two word
writes with one byte changed... :)

> So while the analysis sounds fine I for this particular case,
> I think the patch is just wrong and should be backed out.

I think the analysis sounds fine for all cases, and I fixed the patch.
Happier now? I'm leaving the code as-is for a while, so anyone who
reports problems can revert. Once we know it works in practice as well
as in theory, we can take it out, if we've found a replacement for the
information about what device types are supported by each chip.

-- 
dwmw2