jedec_probe.c

jedec_probe.c

[Holger Speck]

Hello,

Below some NumEraseRegions are set to correct values:

--- jedec_probe.c.orig 2002-11-11 09:01:07.000000000 +0100
+++ jedec_probe.c 2002-10-31 11:30:31.000000000 +0100
@@ -43,6 +43,7 @@
 #define AM29F080 0x00D5
 #define AM29F040 0x00A4
 #define AM29LV040B 0x004F
+#define AM29F032B 0x0041
 
 /* Atmel */
 #define AT49BV512 0x0003
@@ -147,6 +148,15 @@
 static const struct amd_flash_info jedec_table[] = {
  {
   mfr_id: MANUFACTURER_AMD,
+  dev_id: AM29F032B,
+  name: "AMD AM29F032B",
+  DevSize: SIZE_4MiB,
+  CmdSet: P_ID_AMD_STD,
+  NumEraseRegions: 1,
+  regions: {ERASEINFO(0x10000,64)
+  }
+ }, {
+  mfr_id: MANUFACTURER_AMD,
   dev_id: AM29LV160DT,
   name: "AMD AM29LV160DT",
   DevSize: SIZE_2MiB,
@@ -199,7 +209,7 @@
   name: "Toshiba TC58FVB321",
   DevSize: SIZE_4MiB,
   CmdSet: P_ID_AMD_STD,
-  NumEraseRegions: 4,
+  NumEraseRegions: 2,
   regions: {ERASEINFO(0x02000,8),
      ERASEINFO(0x10000,63)
   }
@@ -209,7 +219,7 @@
   name: "Toshiba TC58FVT321",
   DevSize: SIZE_4MiB,
   CmdSet: P_ID_AMD_STD,
-  NumEraseRegions: 4,
+  NumEraseRegions: 2,
   regions: {ERASEINFO(0x10000,63),
      ERASEINFO(0x02000,8)
   }
@@ -219,7 +229,7 @@
   name: "Toshiba TC58FVB641",
   DevSize: SIZE_8MiB,
   CmdSet: P_ID_AMD_STD,
-  NumEraseRegions: 4,
+  NumEraseRegions: 2,
   regions: {ERASEINFO(0x02000,8),
      ERASEINFO(0x10000,127)
   }
@@ -229,7 +239,7 @@
   name: "Toshiba TC58FVT641",
   DevSize: SIZE_8MiB,
   CmdSet: P_ID_AMD_STD,
-  NumEraseRegions: 4,
+  NumEraseRegions: 2,
   regions: {ERASEINFO(0x10000,127),
      ERASEINFO(0x02000,8)
   }

Holger

jedec_probe.c

[Joshua Wise]

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

Hi all,

I've added a new flash chip to the jedec_probe.c definitions, and it seems 
that it's throwing back errors that the unlock codes don't match. I've 
tracked this down to jedec_probe_chip's cfi->addr_unlock* garbage - it does 
look like that function will need to be rewritten. Has anyone already started 
this, or should I just go ahead?

~joshua

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

iD8DBQE/BjjLPn9tWOqA4LMRAkOnAJ9pIVC8b1nSW3lXJZhcnnMa+uSCQACgjUsJ
fK4lWnznSpEloOaklDaQRdg=
=7gh4
-----END PGP SIGNATURE-----

Re: jedec_probe.c

[Joshua Wise]

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

On Friday 04 July 2003 10:32 pm, Joshua Wise wrote:
> tracked this down to jedec_probe_chip's cfi->addr_unlock* garbage - it does
> look like that function will need to be rewritten. Has anyone already
> started this, or should I just go ahead?
I stand corrected. It seems like it's also possible that jedec_probe() could
be rewritten to just probe all the chips that we know about. Methinks I'll
try that.

(Addendum a few minutes later)
Sigh.. This just exposes fscking LAYERS of cruft that MTD was built on, 
including the idea that every flash chip would be CFI, and this horrible 
probing cruft. Someone please shoot me.

> ~joshua

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

iD8DBQE/Blw8Pn9tWOqA4LMRAratAJ9flffrOczi5a5pTa/Vi1PestkHSQCeNXEM
PD9pfEOR53oD2NHJHXtU2I8=
=w4wO
-----END PGP SIGNATURE-----

Re: jedec_probe.c

[Eric W. Biederman]

Joshua Wise <joshua@joshuawise.com> writes:

> On Friday 04 July 2003 10:32 pm, Joshua Wise wrote:
> > tracked this down to jedec_probe_chip's cfi->addr_unlock* garbage - it does
> > look like that function will need to be rewritten. Has anyone already
> > started this, or should I just go ahead?

Why?  What is your problem.  All the pieces do is validate what
was detected.

> I stand corrected. It seems like it's also possible that jedec_probe() could
> be rewritten to just probe all the chips that we know about. Methinks I'll
> try that.

It already does test for all of the chips we know about.
 
> (Addendum a few minutes later)
> Sigh.. This just exposes fscking LAYERS of cruft that MTD was built on, 
> including the idea that every flash chip would be CFI, and this horrible 
> probing cruft. Someone please shoot me.
???

Flash chips are almost ISA devices so there is no 100% reliable
way of detecting which type of flash device you have plugged in.
But there are major trends that help quite a lot.

NOR flash devices have two different JEDEC specified methods of detecting
exactly which flash chip you have plugged in.   Either the older
jedec_probe, or the more recent cfi_probe.  Using the basic method
in jedec_probe we can only get the manufacturer and device id of the
flash chip, the rest of the information we need comes from a table.
Flash devices tend to be a lot more common than that so the newer
Common Flash Interface probe also reports the number of erase regions
and the like, so the flash drive does not need to have knowledge of 
a specific kind of flash device.

Most NOR flash devices implement either the Intel or the AMD command
set.  The probe method does not matter so the command set code
is reused by both methods.

There is no requirement to reuse the mtd infrastructure.  It is
there simply because it does a good job of factoring out the common 
pieces.

So what kind of flash device do you have?  And why does the
existing infrastructure not work for you?  

Eric

Re: jedec_probe.c

[Joshua Wise]

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

On Saturday 05 July 2003 9:38 am, Eric W. Biederman wrote:
> Why?  What is your problem.  All the pieces do is validate what
> was detected.
My problem is that jedec_probe_chip() incorrectly determines unlock addresses
for my chip. They seemingly work anyway, but then jedec_match() fails because
the unlock addresses that already are stored in the struct cfi_private do not
match what they should be for my flash chip.

> > I stand corrected. It seems like it's also possible that jedec_probe()
> > could be rewritten to just probe all the chips that we know about.
> > Methinks I'll try that.
> It already does test for all of the chips we know about.
But IMHO it does a bad job of doing it. It needs to go through each chip in
order, and try each chip's unlock addresses in order, as opposed to guessing
at unlock addresses based on interleave and chip type.

> NOR flash devices have two different JEDEC specified methods of detecting
> exactly which flash chip you have plugged in.   Either the older
> jedec_probe, or the more recent cfi_probe.  Using the basic method
> in jedec_probe we can only get the manufacturer and device id of the
> flash chip, the rest of the information we need comes from a table.
> Flash devices tend to be a lot more common than that so the newer
> Common Flash Interface probe also reports the number of erase regions
> and the like, so the flash drive does not need to have knowledge of
> a specific kind of flash device.
Right, right, I know all that - I know that there are multiple ways to probe
for chips. I was just mentioning that it seems that MTD was entirely built on
the assumption that every chip would be CFI, and that every chip would
conform to a struct cfi_private. This produces wonderful pieces of cruft like
jedec_probe.c, which gathers information from a JEDEC data table, flies
around randomly probing, and then finally, by a work of magic, finds out
which datatable entry our flash chip corresponds with and smashes it into
something like a struct cfi_private.

> Most NOR flash devices implement either the Intel or the AMD command
> set.  The probe method does not matter so the command set code
> is reused by both methods.
.. .. And don't get me started on how NAND doesn't seem to conform to the
_probe methods, etcetera. (Or at least, doesn't conform in any way that I can
FIND.) I think that the command set code works fine - I just wish it was
exported so that it could be used by other drivers.

> There is no requirement to reuse the mtd infrastructure.  It is
> there simply because it does a good job of factoring out the common
> pieces.
Oh, but there is. Everything on these iPAQs uses MTD - including JFFS2, and
various other things. The userland API, technically, rules. I'm just saying
that internally, MTD is based upon many layers of assumptions that,
unfortunately, no longer work - or that's how it looks to me.

> So what kind of flash device do you have?  And why does the
> existing infrastructure not work for you?
I believe I explained the problem earlier - jedec_probe is wildly probing
about, and making guesses at addr_unlock, which unfortunately turn out to be
correct on every chip but mine. (For what it's worth, this is a AM29LV400BT.
Specs:
http://www.amd.com/us-en/assets/content_type/white_papers_and_tech_docs/21523d1.pdf)

> Eric
~joshua

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

iD8DBQE/Bxb7Pn9tWOqA4LMRAjXSAJ0V8FCRI2t8Yuj777/CrYd3Q/FYegCbBF75
80+YAs18CzNNlGGW2rcd3XE=
=uDpu
-----END PGP SIGNATURE-----

Re: jedec_probe.c

[Eric W. Biederman]

Joshua Wise <joshua@joshuawise.com> writes:

> On Saturday 05 July 2003 9:38 am, Eric W. Biederman wrote:
> > Why?  What is your problem.  All the pieces do is validate what
> > was detected.

> My problem is that jedec_probe_chip() incorrectly determines unlock
> addresses for my chip. They seemingly work anyway, but then
> jedec_match() fails because the unlock addresses that already are
> stored in the struct cfi_private do not match what they should be
> for my flash chip. 

Ok.  So one of the TODO list items in jedec_probe finally hit.
The check in jedec_match for unlock addresses was added to verify
the proper unlock addresses was actually used so if the probe
succeeded the chip would work.  The fact that the proper unlock
addresses never get tried is certainly a problem.

When the code was changed to confirm the proper unlock addresses were
being used there were no reported cases of the code not attempting
the proper unlock addresses so that part of the existing code was left
untouched.  Now that we have the information on which unlock addresses
the various chips use we can walk through all of the known unlock
addresses attempting to match them.  That can be refined by a walk
through the table to see if there is a chance of a given unlock
address working.

> > > I stand corrected. It seems like it's also possible that jedec_probe()
> > > could be rewritten to just probe all the chips that we know about.
> > > Methinks I'll try that.
> > It already does test for all of the chips we know about.
> But IMHO it does a bad job of doing it. It needs to go through each chip in 
> order, and try each chip's unlock addresses in order, as opposed to guessing 
> at unlock addresses based on interleave and chip type.

I would hate to get to the point where the order of the table entries
matters.  Magic table orders are very hard to maintain properly, and
very easy for one inattentive update to break.  jedec_probe does get 
a manufacturer id and a device id which makes it good enough that with
a few refinements it should be able to handle this.  A better
algorithm for selecting the unlock addresses is certainly desirable,
and now we have a test case, where it actually matters. 

> > NOR flash devices have two different JEDEC specified methods of detecting
> > exactly which flash chip you have plugged in.   Either the older
> > jedec_probe, or the more recent cfi_probe.  Using the basic method
> > in jedec_probe we can only get the manufacturer and device id of the
> > flash chip, the rest of the information we need comes from a table.
> > Flash devices tend to be a lot more common than that so the newer
> > Common Flash Interface probe also reports the number of erase regions
> > and the like, so the flash drive does not need to have knowledge of
> > a specific kind of flash device.

> Right, right, I know all that - I know that there are multiple ways to probe 
> for chips. I was just mentioning that it seems that MTD was entirely
> built on the assumption that every chip would be CFI, and that every
> chip would  conform to a struct cfi_private. This produces wonderful
> pieces of cruft like jedec_probe.c, which gathers information from a
> JEDEC data table, flies around randomly probing, and then finally,
> by a work of magic, finds out which datatable entry our flash chip
> corresponds with and smashes it into something like a struct
> cfi_private.

cfi_private in this case is more badly named then anything.  It holds
generic parameters that describe NOR flash devices.   The history
is that the cfi code was written first with a generic infrastructure.
And then it was realized that chips that implemented the older probe
had the same set of requirements and the same set of commands, just
a less convenient probe method, that does not provide all of the
information needed to use a random flash chip.  Instead that
information needs to be looked up in a table.

> > There is no requirement to reuse the mtd infrastructure.  It is
> > there simply because it does a good job of factoring out the common
> > pieces.
>
> Oh, but there is. Everything on these iPAQs uses MTD - including JFFS2, and 
> various other things. The userland API, technically, rules. I'm just saying 
> that internally, MTD is based upon many layers of assumptions that, 
> unfortunately, no longer work - or that's how it looks to me.

To clarify I meant things like the command set/chip drivers and the
like.  In particular I meant a driver for sufficiently strange
hardware can fill in a mtd_info structure with a fully custom set
of methods and all of the upper layers will work.
 
> > So what kind of flash device do you have?  And why does the
> > existing infrastructure not work for you?
> I believe I explained the problem earlier - jedec_probe is wildly probing 
> about, and making guesses at addr_unlock, which unfortunately turn out to be 
> correct on every chip but mine. (For what it's worth, this is a AM29LV400BT. 
> Specs: 
> http://www.amd.com/us-en/assets/content_type/white_papers_and_tech_docs/21523d1.pdf)

Hmm.  I am surprised that this has problems as it looks like a pretty
standard part.  But looking at the table of commands on page 19 it
does appear that the standard guesses we make don't seem to line up.

Does it work if you just hard code the address in jedec_probe_chip?
Just to confirm which piece needs to get fixed.

Eric

Re: jedec_probe.c

[Joshua Wise]

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

On Saturday 05 July 2003 7:19 pm, Eric W. Biederman wrote:
> > But IMHO it does a bad job of doing it. It needs to go through each chip
> > in order, and try each chip's unlock addresses in order, as opposed to
> > guessing at unlock addresses based on interleave and chip type.

Bah, I was just pissed off :), I meant go through each chip in the array.

> I would hate to get to the point where the order of the table entries
> matters.  Magic table orders are very hard to maintain properly, and
> very easy for one inattentive update to break.  jedec_probe does get
> a manufacturer id and a device id which makes it good enough that with
> a few refinements it should be able to handle this.  A better
> algorithm for selecting the unlock addresses is certainly desirable,
> and now we have a test case, where it actually matters.

Right. This is what I mean - jedec_probe cannot use the generic probing 
functions that assume that we can query chips by doing something like Q/R/Y, 
it instead should loop.

> cfi_private in this case is more badly named then anything.  It holds
> generic parameters that describe NOR flash devices.   The history
> is that the cfi code was written first with a generic infrastructure.

Right, right - I was just confused to hell and back :)

> And then it was realized that chips that implemented the older probe
> had the same set of requirements and the same set of commands, just
> a less convenient probe method, that does not provide all of the
> information needed to use a random flash chip.  Instead that
> information needs to be looked up in a table.

goto part_where_joshua_whines_about_using_generic_probing; // :)

> To clarify I meant things like the command set/chip drivers and the
> like.  In particular I meant a driver for sufficiently strange
> hardware can fill in a mtd_info structure with a fully custom set
> of methods and all of the upper layers will work.

Oh, interesting.

> Hmm.  I am surprised that this has problems as it looks like a pretty
> standard part.  But looking at the table of commands on page 19 it
> does appear that the standard guesses we make don't seem to line up.

Indeed, I got it working over JTAG with relatively little trouble (the main 
issue being that to program, it must be used in word mode.)

> Does it work if you just hard code the address in jedec_probe_chip?
> Just to confirm which piece needs to get fixed.

I'd assume that it would. Is jedec_probe_chip used anywhere other than 
jedec_probe?

> Eric
~joshua

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

iD8DBQE/B4HwPn9tWOqA4LMRAvoRAJ0RQ4mXtk+FGIZIAfR1WsuBx2gYnwCfYTs3
xwZ1xpcJsyoyjO6HTFZnHIw=
=koC6
-----END PGP SIGNATURE-----

Re: jedec_probe.c

[Eric W. Biederman]

Joshua Wise <joshua@joshuawise.com> writes:

> On Saturday 05 July 2003 7:19 pm, Eric W. Biederman wrote:
> > > But IMHO it does a bad job of doing it. It needs to go through each chip
> > > in order, and try each chip's unlock addresses in order, as opposed to
> > > guessing at unlock addresses based on interleave and chip type.
> 
> Bah, I was just pissed off :), I meant go through each chip in the array.

Ok.  My point is that there is no reason to go through each chip in the
array.  We can just iterate through the unlock_addrs array, which
holds every unlock address.  

> > I would hate to get to the point where the order of the table entries
> > matters.  Magic table orders are very hard to maintain properly, and
> > very easy for one inattentive update to break.  jedec_probe does get
> > a manufacturer id and a device id which makes it good enough that with
> > a few refinements it should be able to handle this.  A better
> > algorithm for selecting the unlock addresses is certainly desirable,
> > and now we have a test case, where it actually matters.
> 
> Right. This is what I mean - jedec_probe cannot use the generic probing 
> functions that assume that we can query chips by doing something like Q/R/Y, 
> it instead should loop.

We can and do have generic probe functions, in the jedec case.
It is a different probe than the cfi one but it is just as valid.
The challenge is that the query opcode needs a prefix for some chips
so we need to loop through all of the prefixes (unlock addresses) instead
of just some magic subset of them.

> > Does it work if you just hard code the address in jedec_probe_chip?
> > Just to confirm which piece needs to get fixed.
> 
> I'd assume that it would. Is jedec_probe_chip used anywhere other than 
> jedec_probe?

It shouldn't be, and the probes are just used at probe time.  The
rest of the time their modules can even be unloaded if necessary.

Eric

Re: jedec_probe.c

[Joshua Wise]

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

On Saturday 05 July 2003 10:11 pm, Eric W. Biederman wrote:
> Ok.  My point is that there is no reason to go through each chip in the
> array.  We can just iterate through the unlock_addrs array, which
> holds every unlock address.

Right, that makes sense.

> We can and do have generic probe functions, in the jedec case.

Not even like that - I meant the way gen_probe.c goes through all interlaces, 
all device sizes, etcetera. I meant that 'different'.

> It is a different probe than the cfi one but it is just as valid.
> The challenge is that the query opcode needs a prefix for some chips
> so we need to loop through all of the prefixes (unlock addresses) instead
> of just some magic subset of them.

Right - guessing at what gen_probe really wants and interpreting it like that, 
IMHO, makes extremely little sense.

> It shouldn't be, and the probes are just used at probe time.  The
> rest of the time their modules can even be unloaded if necessary.

Sweet, so if jedec_probe was rewritten to not have to use jedec_probe_chip, or 
gen_probe at all for that matter, we would not have to deal with 
jedec_probe_chip anywhere else?

Also, out of curiousity, are you on IRC? I would appreciate it if you could 
'look over my shoulder' through a screen session as I hack at a new 
jedec_probe, and poke me with a generic sharp object if I do something 
obviously stupid.

> Eric
~joshua

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

iD8DBQE/B4ZGPn9tWOqA4LMRAhPaAJwP22JpVB6WzOvaUp/Ah13AjsvRagCgi3vb
9kZlolNsMbJGmt7gk+EtI0k=
=j9jm
-----END PGP SIGNATURE-----

Re: jedec_probe.c

[Eric W. Biederman]

Joshua Wise <joshua@joshuawise.com> writes:

> On Saturday 05 July 2003 10:11 pm, Eric W. Biederman wrote:
> > Ok.  My point is that there is no reason to go through each chip in the
> > array.  We can just iterate through the unlock_addrs array, which
> > holds every unlock address.
> 
> Right, that makes sense.
> 
> > We can and do have generic probe functions, in the jedec case.
> 
> Not even like that - I meant the way gen_probe.c goes through all
> interlaces, all device sizes, etcetera. I meant that 'different'.

gen_probe_chip is one case where the cfi and the jedec case are
common.  Warts are there but it is a common case.  And handling
the various combinations of interleaved chips and various word
sizes is tricky, and is quite easy for people who don't need that
logic to get it wrong if it is not handled in common.

> > It is a different probe than the cfi one but it is just as valid.
> > The challenge is that the query opcode needs a prefix for some chips
> > so we need to loop through all of the prefixes (unlock addresses) instead
> > of just some magic subset of them.
> 
> Right - guessing at what gen_probe really wants and interpreting it
> like that, IMHO, makes extremely little sense.
> 
> > It shouldn't be, and the probes are just used at probe time.  The
> > rest of the time their modules can even be unloaded if necessary.
> 
> Sweet, so if jedec_probe was rewritten to not have to use
> jedec_probe_chip, or gen_probe at all for that matter, we would not
> have to deal with jedec_probe_chip anywhere else?

Well yes, but I don't see the point in going in that direction.
 
> Also, out of curiousity, are you on IRC? I would appreciate it if you could 
> 'look over my shoulder' through a screen session as I hack at a new 
> jedec_probe, and poke me with a generic sharp object if I do something 
> obviously stupid.

Sorry, I'm not.  Submitting patches for review is another good
way of accomplishing the same thing, and it is more economical
of other peoples time.  This is a weekend and one of my rare moments
with free time.  Thayne Harbaugh should be able to do just as good
a job at answering your questions and he has a little more time
than me.

I just wanted to catch this and point you onto a useful direction
before things go to far.  

Seriously all you should need to do is to take the loop in
jedec_probe_chip where it loops through some unlock address and have
it explicitly loop through all of the combinations, or at least enough
of them until a match is found.  Look for the retry label.  It is a
significant code change but it should be contained to just that one spot.

Eric

Re: jedec_probe.c

[Joshua Wise]

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

On Saturday 05 July 2003 10:50 pm, Eric W. Biederman wrote:
> gen_probe_chip is one case where the cfi and the jedec case are
> common.  Warts are there but it is a common case.  And handling
> the various combinations of interleaved chips and various word
> sizes is tricky, and is quite easy for people who don't need that
> logic to get it wrong if it is not handled in common.

Really? I think it would be better to just probe all the unlock addresses in 
order instead of probing interleaves, etc

> > Sweet, so if jedec_probe was rewritten to not have to use
> > jedec_probe_chip, or gen_probe at all for that matter, we would not
> > have to deal with jedec_probe_chip anywhere else?
>
> Well yes, but I don't see the point in going in that direction.

See above..

> Sorry, I'm not.  Submitting patches for review is another good
> way of accomplishing the same thing, and it is more economical
> of other peoples time.  This is a weekend and one of my rare moments
> with free time.  Thayne Harbaugh should be able to do just as good
> a job at answering your questions and he has a little more time
> than me.

Understood, I can appreciate that :) Who is Thayne Harbaugh? *pokes Thayne 
with a sharp object to call their attention to the discussion*

> Seriously all you should need to do is to take the loop in
> jedec_probe_chip where it loops through some unlock address and have
> it explicitly loop through all of the combinations, or at least enough
> of them until a match is found.  Look for the retry label.  It is a
> significant code change but it should be contained to just that one spot.

I'll take a look into that.

> Eric
~joshua

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

iD8DBQE/B5PJPn9tWOqA4LMRArvXAJ4tUS/NYq0e2t+6y2Oxj4IQ2I80ygCfaf3H
IUJ864JnVXEOtmf9EnaVhD0=
=qBOM
-----END PGP SIGNATURE-----

Re: jedec_probe.c

[Eric W. Biederman]

Joshua Wise <joshua@joshuawise.com> writes:

> On Saturday 05 July 2003 10:50 pm, Eric W. Biederman wrote:
> > gen_probe_chip is one case where the cfi and the jedec case are
> > common.  Warts are there but it is a common case.  And handling
> > the various combinations of interleaved chips and various word
> > sizes is tricky, and is quite easy for people who don't need that
> > logic to get it wrong if it is not handled in common.
> 
> Really? I think it would be better to just probe all the unlock addresses in 
> order instead of probing interleaves, etc

There is more work to do when chips are interleaved.  And that
extra work is non-obvious.  In the current setup almost everything
looks like you have only a single chip, but that is by careful design.

In particular if you have 2 8bit devices on a 16bit bus, a 16bit
read will return one byte from each device.

It happens that jedec_probe_chip is also guessing unlock addresses
based upon the chip configuration, but that is an almost entirely
different issue.  That part of the code can be removed with no ill
effects.  cfi_send_gen_cmd already handles the issues with addresses
caused by interleaved devices.

> > Sorry, I'm not.  Submitting patches for review is another good
> > way of accomplishing the same thing, and it is more economical
> > of other peoples time.  This is a weekend and one of my rare moments
> > with free time.  Thayne Harbaugh should be able to do just as good
> > a job at answering your questions and he has a little more time
> > than me.
> 
> Understood, I can appreciate that :) Who is Thayne Harbaugh? *pokes Thayne 
> with a sharp object to call their attention to the discussion*

He is a coworker, and currently is active making certain jedec_probe
works in a lot of corner cases.  He is the guy who put all of the FIXME's
in the code.  You probably won't here from him until Monday though.
 
> > Seriously all you should need to do is to take the loop in
> > jedec_probe_chip where it loops through some unlock address and have
> > it explicitly loop through all of the combinations, or at least enough
> > of them until a match is found.  Look for the retry label.  It is a
> > significant code change but it should be contained to just that one spot.
> 
> I'll take a look into that.

Have fun.

Eric

Re: jedec_probe.c

[Thayne Harbaugh]

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

On Fri, 2003-07-04 at 20:32, Joshua Wise wrote:

> I've added a new flash chip to the jedec_probe.c definitions, and it seems 
> that it's throwing back errors that the unlock codes don't match.

Smells like the unlock seeds at the start of jedec_probe_chip() are
incorrect.  I've expected this to turn up a problem eventually.

> I've 
> tracked this down to jedec_probe_chip's cfi->addr_unlock* garbage - it does 
> look like that function will need to be rewritten. Has anyone already started 
> this, or should I just go ahead?

I have yet to start.  I know it needs to be reworked (per the FIXME
comment).  I don't expect to get back to working on MTD code until
middle to the end of this week.  I have this and a pile of other fixes
that need to go in plus some patches that Carolyn at Tektronixs sent me.

If you are motivated then send some patches with some fixes.  The unique
unlock addresses are all enumerated in unlock_addrs[].  This could be
used as the list to iterate through.  The current seeds try to take
insight into the type of device (8, 16, etc.) being probed to select the
set of addresses to probe.  While this approach is interesting, I'm not
sure if it is good policy unless there is an obvious, bullet-proof
association that can be drawn between device types and modes and probe
addresses.

PS.  Yeah, I see that there's a long thread strung out here.  I'm going
to respond to each email rather than try to sum everything up in a
single reply.  Replying to each email makes it easier to reply to each
point in the topic and gives a better continuity to the archive.

-- 
Thayne Harbaugh
Linux Networx

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

Re: jedec_probe.c

[Thayne Harbaugh]

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

On Sat, 2003-07-05 at 20:15, Joshua Wise wrote:

> On Saturday 05 July 2003 10:11 pm, Eric W. Biederman wrote:

> > It shouldn't be, and the probes are just used at probe time.  The
> > rest of the time their modules can even be unloaded if necessary.
> 
> Sweet, so if jedec_probe was rewritten to not have to use jedec_probe_chip, or 
> gen_probe at all for that matter, we would not have to deal with 
> jedec_probe_chip anywhere else?

jedec_probe only does chip setup.  Once things are setup then
jedec_probe and gen_probe are no longer needed - they can be unloaded if
they are modules.

In the case of an embedded platform I can't see gen_probe and
jedec_probe being too useful.  I would imagine that someone would write
a very simple setup that initializes what is already expected to be
there.  jedec_probe and gen_probe are very useful, however, on platforms
that are generic.  Do you propose to write a replacement jedec_probe
that is still generic?

I do realize that jedec_probe is not guaranteed to work and is poor when
the configuration is known.  I have proposed writing a module that can
take configuration information as arguments or from userspace and simply
initialize the parts without stumbling around in the dark trying to find
MTD's.  I have even written some code - I just never committed it
because I wasn't happy with the solution and didn't have time to
complete it (that might change in the future).

Having a generic, forced configuration would be very useful for those
that already know what they have in the system.  It would also fix cases
where hardware is impossible to reliably probe.  This type of work would
be very much appreciated.

-- 
Thayne Harbaugh
Linux Networx

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

Re: jedec_probe.c

[Thayne Harbaugh]

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

On Sat, 2003-07-05 at 17:19, Eric W. Biederman wrote:
> Joshua Wise <joshua@joshuawise.com> writes:
> 
> > On Saturday 05 July 2003 9:38 am, Eric W. Biederman wrote:
> > > Why?  What is your problem.  All the pieces do is validate what
> > > was detected.
> 
> > My problem is that jedec_probe_chip() incorrectly determines unlock
> > addresses for my chip. They seemingly work anyway, but then
> > jedec_match() fails because the unlock addresses that already are
> > stored in the struct cfi_private do not match what they should be
> > for my flash chip. 
> 
> Ok.  So one of the TODO list items in jedec_probe finally hit.
> The check in jedec_match for unlock addresses was added to verify
> the proper unlock addresses was actually used so if the probe
> succeeded the chip would work.

More exactly: Some chips will ID with incorrect unlock addresses.  When
it comes time to use those incorrect unlock addresses to erase or write
then there is a failure.  The check was put in to prevent chips that ID
with incorrect unlock addresses from having a successful probe.  This
sounds like Joshua's MTD's are ID'ing with the wrong unlock addresses.

>   The fact that the proper unlock
> addresses never get tried is certainly a problem.

This is the _real_ problem.

> When the code was changed to confirm the proper unlock addresses were
> being used there were no reported cases of the code not attempting
> the proper unlock addresses so that part of the existing code was left
> untouched.

Although it was anticipated that this problem would eventually surface. 
Blame the problem on lazy programmers (that's me).

>   Now that we have the information on which unlock addresses
> the various chips use we can walk through all of the known unlock
> addresses attempting to match them.

Yes.

> That can be refined by a walk
> through the table to see if there is a chance of a given unlock
> address working.

I'm not convinced that "refining" the list is a good thing unless there
are obvious connections between chip type/mode and unlock addresses. 
Maybe there's a slick way to organize the jedec_table[] so that extra
code isn't necessary.

> > Oh, but there is. Everything on these iPAQs uses MTD

So this is a well-defined platform.  You shouldn't need generic probing
- just write a simple init module that initializes the CFI structure and
loads the proper command set.

> > I believe I explained the problem earlier - jedec_probe is wildly probing 
> > about, and making guesses at addr_unlock, which unfortunately turn out to be 
> > correct on every chip but mine.

Not necessarily correct for every chip but yours - I do know of some
specific cases where the probe is broken and needs to be fixed:
jedec_match has a problem when checking fit with interleaved devices; 16
bit chips don't probe correctly in 8 bit mode; I know of another
situation where the unlock seeds are insufficient.  Sorry to burst your
bubble when you think that we targeted the iPAQ 8^) (yes, humor
implied).

>  (For what it's worth, this is a AM29LV400BT. 
> > Specs: 
> > http://www.amd.com/us-en/assets/content_type/white_papers_and_tech_docs/21523d1.pdf)

Thanks for the link.  The more broken cases I know about and the more
data sheets I read, the easier it is to generalize the code so that it
just works (especially since right now it doesn't).

I'll be able to work on this sometime near the end of this week.

-- 
Thayne Harbaugh
Linux Networx

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

Re: jedec_probe.c

[David Woodhouse]

On Sun, 2003-07-06 at 03:15, Joshua Wise wrote:
> Also, out of curiousity, are you on IRC? I would appreciate it if you could 
> 'look over my shoulder' through a screen session as I hack at a new 
> jedec_probe, and poke me with a generic sharp object if I do something 
> obviously stupid.

irc.freenode.net #mtd

-- 
dwmw2

Re: jedec_probe.c

[David Woodhouse]

On Mon, 2003-07-07 at 15:52, Thayne Harbaugh wrote:
> Having a generic, forced configuration would be very useful for those
> that already know what they have in the system.  It would also fix cases
> where hardware is impossible to reliably probe.  This type of work would
> be very much appreciated.

If we do this, it needs to be easy to switch back to the generic probe
code. Many people don't really know how their board is wired up, or are
given incorrect information. I have customers who _insist_ their boot
flash is wired up as 32 bits, and file bugs against the working kernel
because of the comments and board setup for 16 bit flash. Not because
they claim it doesn't work, but just because they're comparing with
their docs.

-- 
dwmw2

Re: jedec_probe.c

[Thayne Harbaugh]

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

On Tue, 2003-07-08 at 02:21, David Woodhouse wrote:
> On Mon, 2003-07-07 at 15:52, Thayne Harbaugh wrote:
> > Having a generic, forced configuration would be very useful for those
> > that already know what they have in the system.  It would also fix cases
> > where hardware is impossible to reliably probe.  This type of work would
> > be very much appreciated.
> 
> If we do this, it needs to be easy to switch back to the generic probe
> code.

The forced configuration would be an override.  I've played around and
have some modules that take parameters that override usual behavior. 
I'm not sure if I like this or if I would rather have a separate module
(maybe forced_probe) that could be used instead of gen_probe and
jedec_probe (or even cfi_probe).

>  Many people don't really know how their board is wired up, or are
> given incorrect information. I have customers who _insist_ their boot
> flash is wired up as 32 bits, and file bugs against the working kernel
> because of the comments and board setup for 16 bit flash. Not because
> they claim it doesn't work, but just because they're comparing with
> their docs.

Makes perfect sense.  Most people would use the already established
probing mechanisms.

-- 
Thayne Harbaugh
Linux Networx

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