[PATCH] flashrom: ST M25P40 bug?

[Fredrik Tolf]

I don't know if this is a bug, or if there are somehow two completely
different versions of the ST M25P40 SPI chip.

I just tried to burn such a chip with flashrom, but it wouldn't detect
it at all, so I checked the datasheet for it, and it turned out that the
probe was completely different from what one would think, reading
flashrom's source.

It seems that the M25P40 doesn't use the RDID command to send its ID,
but rather a "RES" (Read Electronic Signature) command instead, which
returns just one byte. I added support for it, and then it worked
perfectly for me.

I'm attaching my patch to fix the problem. I would imagine that the
other M25Pxx chips would be affected by the same problem, but since I
can neither verify nor test what IDs they would have, they are excluded
from my patch.

Fredrik Tolf

[Uwe Hermann]

On Sat, Mar 29, 2008 at 04:42:24AM +0100, Fredrik Tolf wrote:
> I don't know if this is a bug, or if there are somehow two completely
> different versions of the ST M25P40 SPI chip.
>
> I just tried to burn such a chip with flashrom, but it wouldn't detect
> it at all, so I checked the datasheet for it, and it turned out that the
> probe was completely different from what one would think, reading
> flashrom's source.
>
> It seems that the M25P40 doesn't use the RDID command to send its ID,
> but rather a "RES" (Read Electronic Signature) command instead, which
> returns just one byte. I added support for it, and then it worked
> perfectly for me.
>
> I'm attaching my patch to fix the problem. I would imagine that the
> other M25Pxx chips would be affected by the same problem, but since I
> can neither verify nor test what IDs they would have, they are excluded
> from my patch.

I don't know too much about SPI, I'll let Carl-Daniel review this patch ;)
But please resend with a Signed-off-by line, otherwise we cannot commit
the patch, see

http://www.coreboot.org/Development_Guidelines#Sign-off_Procedure

Thanks, Uwe.

[Fredrik Tolf]

On Sat, 2008-03-29 at 17:28 +0100, Uwe Hermann wrote:
> On Sat, Mar 29, 2008 at 04:42:24AM +0100, Fredrik Tolf wrote:
> [...]
> But please resend with a Signed-off-by line, otherwise we cannot commit
> the patch

OK, I'll do that.

Signed-off-by: Fredrik Tolf

[Fredrik Tolf]

On Sat, 2008-03-29 at 17:28 +0100, Uwe Hermann wrote:
> But please resend with a Signed-off-by line, otherwise we cannot
> commit
> the patch, see
>
> http://www.coreboot.org/Development_Guidelines#Sign-off_Procedure

By the way, the link in that Wiki page (which points to
http://osdlab.org/newsroom/press_releases/2004/2004_05_24_dco.html) is
broken.

Fredrik Tolf

[Carl-Daniel Hailfinger]

On 29.03.2008 04:42, Fredrik Tolf wrote:
> I don't know if this is a bug, or if there are somehow two completely
> different versions of the ST M25P40 SPI chip.

The data sheet says:
"The Read Identification (RDID) instruction is available in products
with Process Technology code X only."
That means the previous code was correct for only a subset of devices.
Unfortunately, that limitation is not mentioned in the ST M25P
application note.
> I just tried to burn such a chip with flashrom, but it wouldn't detect
> it at all, so I checked the datasheet for it, and it turned out that the
> probe was completely different from what one would think, reading
> flashrom's source.
>
> It seems that the M25P40 doesn't use the RDID command to send its ID,
> but rather a "RES" (Read Electronic Signature) command instead, which
> returns just one byte. I added support for it, and then it worked
> perfectly for me.

See above.
> I'm attaching my patch to fix the problem. I would imagine that the
> other M25Pxx chips would be affected by the same problem, but since I
> can neither verify nor test what IDs they would have, they are excluded
> from my patch.

Thanks for investigating and coding this. However, I have to veto the
patch in its current form. I know at least 4 flash chips with different
sizes from different manufacturers which have the same RES code.
Can you post the RDID output for your device? Maybe we can figure out a
way to identify it safely with the help of RDID and RES and possibly READ.

Regards,
Carl-Daniel

[Fredrik Tolf]

On Sat, 2008-03-29 at 22:17 +0100, Carl-Daniel Hailfinger wrote:
> > I'm attaching my patch to fix the problem. I would imagine that the
> > other M25Pxx chips would be affected by the same problem, but since I
> > can neither verify nor test what IDs they would have, they are excluded
> > from my patch.
>
> Thanks for investigating and coding this. However, I have to veto the
> patch in its current form. I know at least 4 flash chips with different
> sizes from different manufacturers which have the same RES code.
> Can you post the RDID output for your device? Maybe we can figure out a
> way to identify it safely with the help of RDID and RES and possibly READ.

The RDID output was just FF FF FF, so I'm pretty sure it just doesn't
recognize the command at all. How do you suppose the chip's identity
could be deduced from a READ command?

Would it perhaps not be wise to create a command-line switch to enable
detection for chips like this, that might not be possible to
auto-identify safely? The flashchip struct could be gifted with a
pre-initialized boolean that indicates if a chip should be
auto-identified without being explicitly specified.

Fredrik Tolf

[Fredrik Tolf]

On Sat, 2008-03-29 at 22:58 +0100, Fredrik Tolf wrote:
> Would it perhaps not be wise to create a command-line switch to enable
> detection for chips like this, that might not be possible to
> auto-identify safely? The flashchip struct could be gifted with a
> pre-initialized boolean that indicates if a chip should be
> auto-identified without being explicitly specified.

Oh, just to be clear: I intended to do the actual work for that. I just
wanted to see if you think it's a good idea.

Fredrik Tolf

[Carl-Daniel Hailfinger]

On 29.03.2008 22:58, Fredrik Tolf wrote:
> On Sat, 2008-03-29 at 22:17 +0100, Carl-Daniel Hailfinger wrote:
> > > I'm attaching my patch to fix the problem. I would imagine that the
> > > other M25Pxx chips would be affected by the same problem, but since I
> > > can neither verify nor test what IDs they would have, they are excluded
> > > from my patch.
> >
> > Thanks for investigating and coding this. However, I have to veto the
> > patch in its current form. I know at least 4 flash chips with different
> > sizes from different manufacturers which have the same RES code.
> > Can you post the RDID output for your device? Maybe we can figure out a
> > way to identify it safely with the help of RDID and RES and possibly READ.
>
> The RDID output was just FF FF FF, so I'm pretty sure it just doesn't
> recognize the command at all.

Good. That means we can probably use RDID in combination with RES to
identify this special ST M25P40 variant.
> How do you suppose the chip's identity
> could be deduced from a READ command?

You can read the contents of the chip and deduce a minimum size by
analyzing repetitions. While it is possible to underestimate the size of
the chip (in case 2^n identical images are behind each other), I can't
see a way to overestimate chip size.
> Would it perhaps not be wise to create a command-line switch to enable
> detection for chips like this, that might not be possible to
> auto-identify safely? The flashchip struct could be gifted with a
> pre-initialized boolean that indicates if a chip should be
> auto-identified without being explicitly specified.

Yes, a command line switch would probably work, but I'm currently
working on a redesign of the SPI part of flashrom, so I'd like to delay
that a bit. Details will be announced after Tuesday.

Some parts of your patch (except the modification of the ST_M25P40
#define and the flashchips.c changes) are definitely worth merging
instantly, though. Could you split these parts from the patch and resend
them with a Signed-off-by statement?

Regards,
Carl-Daniel

[Carl-Daniel Hailfinger]

Hi Fredrik,

On 29.03.2008 23:17, Carl-Daniel Hailfinger wrote:
> On 29.03.2008 22:58, Fredrik Tolf wrote:
> > On Sat, 2008-03-29 at 22:17 +0100, Carl-Daniel Hailfinger wrote:
> > > > I'm attaching my patch to fix the problem. I would imagine that the
> > > > other M25Pxx chips would be affected by the same problem, but since I
> > > > can neither verify nor test what IDs they would have, they are excluded
> > > > from my patch.
> > >
> > > Thanks for investigating and coding this. However, I have to veto the
> > > patch in its current form. I know at least 4 flash chips with different
> > > sizes from different manufacturers which have the same RES code.
> > > Can you post the RDID output for your device? Maybe we can figure out a
> > > way to identify it safely with the help of RDID and RES and possibly READ.
> >
> > The RDID output was just FF FF FF, so I'm pretty sure it just doesn't
> > recognize the command at all.
>
> Good. That means we can probably use RDID in combination with RES to
> identify this special ST M25P40 variant.

Can you test the patch below? It should have a lower error probability
and work fine for you. If you are OK with my changes to your patch,
please re-add your Signed-off-by line.

It would be interesting to try the REMS (0x90) instruction as well and
match on that one if possible, but this patch is good enough for now.

Regards,
Carl-Daniel

Signed-off-by: Carl-Daniel Hailfinger

Index: flashrom-res/flash.h
===================================================================
--- flashrom-res/flash.h	(Revision 3296)
+++ flashrom-res/flash.h	(Arbeitskopie)
@@ -249,6 +249,7 @@
 #define ST_M25P10A		0x2011
 #define ST_M25P20		0x2012
 #define ST_M25P40		0x2013
+#define ST_M25P40_RES		0x12
 #define ST_M25P80		0x2014
 #define ST_M25P16		0x2015
 #define ST_M25P32		0x2016
@@ -347,7 +348,8 @@
 extern char *lb_part, *lb_vendor;

/* spi.c */
-int probe_spi(struct flashchip *flash);
+int probe_spi_rdid(struct flashchip *flash);
+int probe_spi_res(struct flashchip *flash);
 int it87xx_probe_spi_flash(const char *name);
 int spi_command(unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr);
 void spi_write_enable();
Index: flashrom-res/flashchips.c
===================================================================
--- flashrom-res/flashchips.c	(Revision 3296)
+++ flashrom-res/flashchips.c	(Arbeitskopie)
@@ -49,25 +49,25 @@
    {"Fujitsu",	"MBM29F400TC",		FUJITSU_ID,	MBM29F400TC_STRANGE,	512,	64 * 1024,	TEST_UNTESTED,	probe_m29f400bt,	erase_m29f400bt,		write_coreboot_m29f400bt},
    {"Intel",	"82802AB",		INTEL_ID,	173,			512,	64 * 1024,	TEST_UNTESTED,	probe_82802ab,		erase_82802ab,			write_82802ab},
    {"Intel",	"82802AC",		INTEL_ID,	172,			1024,	64 * 1024,	TEST_UNTESTED,	probe_82802ab,		erase_82802ab,			write_82802ab},
-	{"Macronix",	"MX25L3205",		MX_ID,		MX_25L3205,		4096,	256,		TEST_UNTESTED,	probe_spi,		spi_chip_erase_c7,	spi_chip_write,	spi_chip_read},
-	{"Macronix",	"MX25L4005",		MX_ID,		MX_25L4005,		512,	256,		TEST_UNTESTED,	probe_spi,		spi_chip_erase_c7,	spi_chip_write,	spi_chip_read},
-	{"Macronix",	"MX25L8005",		MX_ID,		MX_25L8005,		1024,	256,		TEST_UNTESTED,	probe_spi,		spi_chip_erase_c7,	spi_chip_write,	spi_chip_read},
+	{"Macronix",	"MX25L3205",		MX_ID,		MX_25L3205,		4096,	256,		TEST_UNTESTED,	probe_spi_rdid,		spi_chip_erase_c7,	spi_chip_write,	spi_chip_read},
+	{"Macronix",	"MX25L4005",		MX_ID,		MX_25L4005,		512,	256,		TEST_UNTESTED,	probe_spi_rdid,		spi_chip_erase_c7,	spi_chip_write,	spi_chip_read},
+	{"Macronix",	"MX25L8005",		MX_ID,		MX_25L8005,		1024,	256,		TEST_UNTESTED,	probe_spi_rdid,		spi_chip_erase_c7,	spi_chip_write,	spi_chip_read},
    {"Macronix",	"MX29F002",		MX_ID,		MX_29F002,		256,	64 * 1024,	TEST_UNTESTED,	probe_29f002,		erase_29f002,			write_29f002},
 #ifndef DISABLE_DOC
    {"M-Systems",	"MD-2802",		MSYSTEMS_ID,	MSYSTEMS_MD2802,	8,	8 * 1024,	TEST_UNTESTED,	probe_md2802,		erase_md2802,			write_md2802,		read_md2802},
 #endif
-	{"PMC",		"Pm25LV010",		PMC_ID,		PMC_25LV010,		128,	256,		TEST_UNTESTED,	probe_spi,		spi_chip_erase_c7,	spi_chip_write,	spi_chip_read},
-	{"PMC",		"Pm25LV016B",		PMC_ID,		PMC_25LV016B,		2048,	256,		TEST_UNTESTED,	probe_spi,		spi_chip_erase_c7,	spi_chip_write,	spi_chip_read},
-	{"PMC",		"Pm25LV020",		PMC_ID,		PMC_25LV020,		256,	256,		TEST_UNTESTED,	probe_spi,		spi_chip_erase_c7,	spi_chip_write,	spi_chip_read},
-	{"PMC",		"Pm25LV040",		PMC_ID,		PMC_25LV040,		512,	256,		TEST_UNTESTED,	probe_spi,		spi_chip_erase_c7,	spi_chip_write,	spi_chip_read},
-	{"PMC",		"Pm25LV080B",		PMC_ID,		PMC_25LV080B,		1024,	256,		TEST_UNTESTED,	probe_spi,		spi_chip_erase_c7,	spi_chip_write,	spi_chip_read},
-	{"PMC",		"Pm25LV512",		PMC_ID,		PMC_25LV512,		64,	256,		TEST_UNTESTED,	probe_spi,		spi_chip_erase_c7,	spi_chip_write,	spi_chip_read},
+	{"PMC",		"Pm25LV010",		PMC_ID,		PMC_25LV010,		128,	256,		TEST_UNTESTED,	probe_spi_rdid,		spi_chip_erase_c7,	spi_chip_write,	spi_chip_read},
+	{"PMC",		"Pm25LV016B",		PMC_ID,		PMC_25LV016B,		2048,	256,		TEST_UNTESTED,	probe_spi_rdid,		spi_chip_erase_c7,	spi_chip_write,	spi_chip_read},
+	{"PMC",		"Pm25LV020",		PMC_ID,		PMC_25LV020,		256,	256,		TEST_UNTESTED,	probe_spi_rdid,		spi_chip_erase_c7,	spi_chip_write,	spi_chip_read},
+	{"PMC",		"Pm25LV040",		PMC_ID,		PMC_25LV040,		512,	256,		TEST_UNTESTED,	probe_spi_rdid,		spi_chip_erase_c7,	spi_chip_write,	spi_chip_read},
+	{"PMC",		"Pm25LV080B",		PMC_ID,		PMC_25LV080B,		1024,	256,		TEST_UNTESTED,	probe_spi_rdid,		spi_chip_erase_c7,	spi_chip_write,	spi_chip_read},
+	{"PMC",		"Pm25LV512",		PMC_ID,		PMC_25LV512,		64,	256,		TEST_UNTESTED,	probe_spi_rdid,		spi_chip_erase_c7,	spi_chip_write,	spi_chip_read},
    {"PMC",		"Pm49FL002",		PMC_ID_NOPREFIX,PMC_49FL002,		256,	16 * 1024,	TEST_UNTESTED,	probe_jedec,		erase_chip_jedec,		write_49fl004},
    {"PMC",		"Pm49FL004",		PMC_ID_NOPREFIX,PMC_49FL004,		512,	64 * 1024,	TEST_UNTESTED,	probe_jedec,		erase_chip_jedec,		write_49fl004},
    {"Sharp",	"LHF00L04",		SHARP_ID,	SHARP_LHF00L04,		1024,	64 * 1024,	TEST_UNTESTED,	probe_lhf00l04,		erase_lhf00l04,			write_lhf00l04},
-	{"Spansion",	"S25FL016A",		SPANSION_ID,	SPANSION_S25FL016A,	2048,	256,		TEST_UNTESTED,	probe_spi,		spi_chip_erase_c7,	spi_chip_write,	spi_chip_read},
-	{"SST",		"SST25VF016B",		SST_ID,		SST_25VF016B,		2048,	256,		TEST_UNTESTED,	probe_spi,		spi_chip_erase_c7,	spi_chip_write,	spi_chip_read},
-	{"SST",		"SST25VF040B",		SST_ID,		SST_25VF040B,		512,	256,		TEST_UNTESTED,	probe_spi,		spi_chip_erase_c7,	spi_chip_write,	spi_chip_read},
+	{"Spansion",	"S25FL016A",		SPANSION_ID,	SPANSION_S25FL016A,	2048,	256,		TEST_UNTESTED,	probe_spi_rdid,		spi_chip_erase_c7,	spi_chip_write,	spi_chip_read},
+	{"SST",		"SST25VF016B",		SST_ID,		SST_25VF016B,		2048,	256,		TEST_UNTESTED,	probe_spi_rdid,		spi_chip_erase_c7,	spi_chip_write,	spi_chip_read},
+	{"SST",		"SST25VF040B",		SST_ID,		SST_25VF040B,		512,	256,		TEST_UNTESTED,	probe_spi_rdid,		spi_chip_erase_c7,	spi_chip_write,	spi_chip_read},
    {"SST",		"SST28SF040A",		SST_ID,		SST_28SF040,		512,	256,		TEST_UNTESTED,	probe_28sf040,		erase_28sf040,			write_28sf040},
    {"SST",		"SST29EE020A",		SST_ID,		SST_29EE020A,		256,	128,		TEST_UNTESTED,	probe_jedec,		erase_chip_jedec,		write_jedec},
    {"SST",		"SST39SF010A",		SST_ID,		SST_39SF010,		128,	4096,		TEST_UNTESTED,	probe_jedec,		erase_chip_jedec,		write_39sf020},
@@ -87,15 +87,16 @@
    {"SST",		"SST49LF040B",		SST_ID,		SST_49LF040B,		512,	64 * 1024,	TEST_UNTESTED,	probe_sst_fwhub,	erase_sst_fwhub,		write_sst_fwhub},
    {"SST",		"SST49LF080A",		SST_ID,		SST_49LF080A,		1024,	4096,		TEST_UNTESTED,	probe_jedec,		erase_49lf040,			write_49lf040},
    {"SST",		"SST49LF160C",		SST_ID,		SST_49LF160C,		2048,	4 * 1024,	TEST_UNTESTED,	probe_49lfxxxc,		erase_49lfxxxc,			write_49lfxxxc},
-	{"ST",		"M25P05-A",		ST_ID,		ST_M25P05A,		64,	256,		TEST_UNTESTED,	probe_spi,		spi_chip_erase_c7,	spi_chip_write, spi_chip_read},
-	{"ST",		"M25P10-A",		ST_ID,		ST_M25P10A,		128,	256,		TEST_UNTESTED,	probe_spi,		spi_chip_erase_c7,	spi_chip_write, spi_chip_read},
-	{"ST",		"M25P128",		ST_ID,		ST_M25P128,		16384,	256,		TEST_UNTESTED,	probe_spi,		spi_chip_erase_c7,	spi_chip_write, spi_chip_read},
-	{"ST",		"M25P16",		ST_ID,		ST_M25P16,		2048,	256,		TEST_UNTESTED,	probe_spi,		spi_chip_erase_c7,	spi_chip_write, spi_chip_read},
-	{"ST",		"M25P20",		ST_ID,		ST_M25P20,		256,	256,		TEST_UNTESTED,	probe_spi,		spi_chip_erase_c7,	spi_chip_write, spi_chip_read},
-	{"ST",		"M25P32",		ST_ID,		ST_M25P32,		4096,	256,		TEST_UNTESTED,	probe_spi,		spi_chip_erase_c7,	spi_chip_write, spi_chip_read},
-	{"ST",		"M25P40",		ST_ID,		ST_M25P40,		512,	256,		TEST_UNTESTED,	probe_spi,		spi_chip_erase_c7,	spi_chip_write, spi_chip_read},
-	{"ST",		"M25P64",		ST_ID,		ST_M25P64,		8192,	256,		TEST_UNTESTED,	probe_spi,		spi_chip_erase_c7,	spi_chip_write, spi_chip_read},
-	{"ST",		"M25P80",		ST_ID,		ST_M25P80,		1024,	256,		TEST_UNTESTED,	probe_spi,		spi_chip_erase_c7,	spi_chip_write, spi_chip_read},
+	{"ST",		"M25P05-A",		ST_ID,		ST_M25P05A,		64,	256,		TEST_UNTESTED,	probe_spi_rdid,		spi_chip_erase_c7,	spi_chip_write, spi_chip_read},
+	{"ST",		"M25P10-A",		ST_ID,		ST_M25P10A,		128,	256,		TEST_UNTESTED,	probe_spi_rdid,		spi_chip_erase_c7,	spi_chip_write, spi_chip_read},
+	{"ST",		"M25P128",		ST_ID,		ST_M25P128,		16384,	256,		TEST_UNTESTED,	probe_spi_rdid,		spi_chip_erase_c7,	spi_chip_write, spi_chip_read},
+	{"ST",		"M25P16",		ST_ID,		ST_M25P16,		2048,	256,		TEST_UNTESTED,	probe_spi_rdid,		spi_chip_erase_c7,	spi_chip_write, spi_chip_read},
+	{"ST",		"M25P20",		ST_ID,		ST_M25P20,		256,	256,		TEST_UNTESTED,	probe_spi_rdid,		spi_chip_erase_c7,	spi_chip_write, spi_chip_read},
+	{"ST",		"M25P32",		ST_ID,		ST_M25P32,		4096,	256,		TEST_UNTESTED,	probe_spi_rdid,		spi_chip_erase_c7,	spi_chip_write, spi_chip_read},
+	{"ST",		"M25P40",		ST_ID,		ST_M25P40,		512,	256,		TEST_UNTESTED,	probe_spi_rdid,		spi_chip_erase_c7,	spi_chip_write, spi_chip_read},
+	{"ST",		"M25P40(old)",		ST_ID,		ST_M25P40_RES,		512,	256,		TEST_UNTESTED,	probe_spi_res,		spi_chip_erase_c7,	spi_chip_write, spi_chip_read},
+	{"ST",		"M25P64",		ST_ID,		ST_M25P64,		8192,	256,		TEST_UNTESTED,	probe_spi_rdid,		spi_chip_erase_c7,	spi_chip_write, spi_chip_read},
+	{"ST",		"M25P80",		ST_ID,		ST_M25P80,		1024,	256,		TEST_UNTESTED,	probe_spi_rdid,		spi_chip_erase_c7,	spi_chip_write, spi_chip_read},
    {"ST",		"M29F002B",		ST_ID,		ST_M29F002B,		256,	64 * 1024,	TEST_UNTESTED,	probe_jedec,		erase_chip_jedec,		write_jedec},
    {"ST",		"M29F002T/NT",		ST_ID,		ST_M29F002T,		256,	64 * 1024,	TEST_UNTESTED,	probe_jedec,		erase_chip_jedec,		write_jedec},
    {"ST",		"M29F040B",		ST_ID,		ST_M29F040B,		512,	64 * 1024,	TEST_UNTESTED,	probe_29f040b,		erase_29f040b,			write_29f040b},
@@ -114,10 +115,10 @@
    {"SyncMOS",	"S29C51001T",		SYNCMOS_ID,	S29C51001T,		128,	128,		TEST_UNTESTED,	probe_jedec,		erase_chip_jedec,		write_49f002},
    {"SyncMOS",	"S29C51002T",		SYNCMOS_ID,	S29C51002T,		256,	128,		TEST_UNTESTED,	probe_jedec,		erase_chip_jedec,		write_49f002},
    {"SyncMOS",	"S29C51004T",		SYNCMOS_ID,	S29C51004T,		512,	128,		TEST_UNTESTED,	probe_jedec,		erase_chip_jedec,		write_49f002},
-	{"Winbond",	"W25x10",		WINBOND_NEX_ID,	W_25X10,		128,	256,		TEST_UNTESTED,	probe_spi,		spi_chip_erase_c7,	spi_chip_write, spi_chip_read},
-	{"Winbond",	"W25x20",		WINBOND_NEX_ID,	W_25X20,		256,	256,		TEST_UNTESTED,	probe_spi,		spi_chip_erase_c7,	spi_chip_write, spi_chip_read},
-	{"Winbond",	"W25x40",		WINBOND_NEX_ID,	W_25X40,		512,	256,		TEST_UNTESTED,	probe_spi,		spi_chip_erase_c7,	spi_chip_write, spi_chip_read},
-	{"Winbond",	"W25x80",		WINBOND_NEX_ID,	W_25X80,		1024,	256,		TEST_UNTESTED,	probe_spi,		spi_chip_erase_c7,	spi_chip_write, spi_chip_read},
+	{"Winbond",	"W25x10",		WINBOND_NEX_ID,	W_25X10,		128,	256,		TEST_UNTESTED,	probe_spi_rdid,		spi_chip_erase_c7,	spi_chip_write, spi_chip_read},
+	{"Winbond",	"W25x20",		WINBOND_NEX_ID,	W_25X20,		256,	256,		TEST_UNTESTED,	probe_spi_rdid,		spi_chip_erase_c7,	spi_chip_write, spi_chip_read},
+	{"Winbond",	"W25x40",		WINBOND_NEX_ID,	W_25X40,		512,	256,		TEST_UNTESTED,	probe_spi_rdid,		spi_chip_erase_c7,	spi_chip_write, spi_chip_read},
+	{"Winbond",	"W25x80",		WINBOND_NEX_ID,	W_25X80,		1024,	256,		TEST_UNTESTED,	probe_spi_rdid,		spi_chip_erase_c7,	spi_chip_write, spi_chip_read},
    {"Winbond",	"W29C011",		WINBOND_ID,	W_29C011,		128,	128,		TEST_UNTESTED,	probe_jedec,		erase_chip_jedec,		write_jedec},
    {"Winbond",	"W29C020C",		WINBOND_ID,	W_29C020C,		256,	128,		TEST_UNTESTED,	probe_jedec,		erase_chip_jedec,		write_jedec},
    {"Winbond",	"W29C040P",		WINBOND_ID,	W_29C040P,		512,	256,		TEST_UNTESTED,	probe_jedec,		erase_chip_jedec,		write_jedec},
@@ -132,11 +133,11 @@
    {"Winbond",	"W39V080FA",		WINBOND_ID,	W_39V080FA,		1024,	64*1024,	TEST_UNTESTED,	probe_winbond_fwhub,	erase_winbond_fwhub,		write_winbond_fwhub},
    {"Winbond",	"W39V080FA (dual mode)",WINBOND_ID,	W_39V080FA_DM,		512,	64*1024,	TEST_UNTESTED,	probe_winbond_fwhub,	erase_winbond_fwhub,		write_winbond_fwhub},

-	{"EON",		"unknown SPI chip",	EON_ID_NOPREFIX,GENERIC_DEVICE_ID,	0,	0,		TEST_UNTESTED,	probe_spi,		NULL,				NULL},
-	{"Macronix",	"unknown SPI chip",	MX_ID,		GENERIC_DEVICE_ID,	0,	0,		TEST_UNTESTED,	probe_spi,		NULL,				NULL},
-	{"PMC",		"unknown SPI chip",	PMC_ID,		GENERIC_DEVICE_ID,	0,	0,		TEST_UNTESTED,	probe_spi,		NULL,				NULL},
-	{"SST",		"unknown SPI chip",	SST_ID,		GENERIC_DEVICE_ID,	0,	0,		TEST_UNTESTED,	probe_spi,		NULL,				NULL},
-	{"ST",		"unknown SPI chip",	ST_ID,		GENERIC_DEVICE_ID,	0,	0,		TEST_UNTESTED,	probe_spi,		NULL,				NULL},
+	{"EON",		"unknown SPI chip",	EON_ID_NOPREFIX,GENERIC_DEVICE_ID,	0,	0,		TEST_UNTESTED,	probe_spi_rdid,		NULL,				NULL},
+	{"Macronix",	"unknown SPI chip",	MX_ID,		GENERIC_DEVICE_ID,	0,	0,		TEST_UNTESTED,	probe_spi_rdid,		NULL,				NULL},
+	{"PMC",		"unknown SPI chip",	PMC_ID,		GENERIC_DEVICE_ID,	0,	0,		TEST_UNTESTED,	probe_spi_rdid,		NULL,				NULL},
+	{"SST",		"unknown SPI chip",	SST_ID,		GENERIC_DEVICE_ID,	0,	0,		TEST_UNTESTED,	probe_spi_rdid,		NULL,				NULL},
+	{"ST",		"unknown SPI chip",	ST_ID,		GENERIC_DEVICE_ID,	0,	0,		TEST_UNTESTED,	probe_spi_rdid,		NULL,				NULL},

{NULL,}
 };
Index: flashrom-res/spi.c
===================================================================
--- flashrom-res/spi.c	(Revision 3296)
+++ flashrom-res/spi.c	(Arbeitskopie)
@@ -36,6 +36,11 @@
 #define JEDEC_RDID_OUTSIZE	0x01
 #define JEDEC_RDID_INSIZE	0x03

+/* Read Electronic Signature */
+#define JEDEC_RES	0xab
+#define JEDEC_RES_OUTSIZE	0x04
+#define JEDEC_RES_INSIZE	0x01
+
 /* Write Enable */
 #define JEDEC_WREN	{0x06}
 #define JEDEC_WREN_OUTSIZE	0x01
@@ -259,6 +264,16 @@
    return 0;
 }

+static int spi_res(unsigned char *readarr)
+{
+	const unsigned char cmd[] = {JEDEC_RES, 0, 0, 0};
+
+	if (spi_command(JEDEC_RES_OUTSIZE, JEDEC_RES_INSIZE, cmd, readarr))
+		return 1;
+	printf_debug("RES returned %02x.\n", readarr[0]);
+	return 0;
+}
+
 void spi_write_enable()
 {
    const unsigned char cmd[] = JEDEC_WREN;
@@ -275,7 +290,7 @@
    spi_command(JEDEC_WRDI_OUTSIZE, JEDEC_WRDI_INSIZE, cmd, NULL);
 }

-int probe_spi(struct flashchip *flash)
+int probe_spi_rdid(struct flashchip *flash)
 {
    unsigned char readarr[3];
    uint32_t manuf_id;
@@ -308,6 +323,35 @@
    return 0;
 }

+int probe_spi_res(struct flashchip *flash)
+{
+	unsigned char readarr[3];
+	uint32_t model_id;
+	if (!spi_rdid(readarr)) {
+		/* Check if RDID returns 0xff 0xff 0xff, then we use RES. */
+		if ((readarr[0] != 0xff) || (readarr[1] != 0xff) ||
+		    (readarr[2] != 0xff))
+			return 0;
+	} else {
+		/* We couldn't issue RDID, it's pointless to try RES. */
+		return 0;
+	}
+	if (!spi_res(readarr)) {
+		model_id = readarr[0];
+		printf_debug("%s: id 0x%x\n", __FUNCTION__, model_id);
+		if (model_id == flash->model_id) {
+			/* Print the status register to tell the
+			 * user about possible write protection.
+			 */
+			spi_prettyprint_status_register(flash);
+
+			return 1;
+		}
+	}
+
+	return 0;
+}
+
 uint8_t spi_read_status_register()
 {
    const unsigned char cmd[] = JEDEC_RDSR;

[Fredrik Tolf]

On Sun, 2008-05-11 at 15:32 +0200, Carl-Daniel Hailfinger wrote:
> Can you test the patch below? It should have a lower error probability
> and work fine for you. If you are OK with my changes to your patch,
> please re-add your Signed-off-by line.

Yes, the flashrom code with that patch seems to detect my chip and read
its contents quite well. However, the same code does not detect the
original chip on the board (an MX25L4005) satisfactory, and it seems to
be a conflict with the SPI code. Here's the output:

Calibrating delay loop... OK.
No coreboot table found.
Found chipset "NVIDIA MCP55", enabling flash write... OK.
Found board "GIGABYTE GA-M57SLI-S4": enabling flash write... Serial
flash segment 0xfffe0000-0xffffffff enabled
Serial flash segment 0x000e0000-0x000fffff enabled
Serial flash segment 0xffee0000-0xffefffff disabled
Serial flash segment 0xfff80000-0xfffeffff enabled
LPC write to serial flash enabled
serial flash pin 29
OK.
MX25L4005 found at physical address 0xfff80000.
unknown SPI chip found at physical address 0x0.
Multiple flash chips were detected: MX25L4005 unknown SPI chip
Please specify which chip to use with the -c <chipname> option.

Fredrik Tolf

[Carl-Daniel Hailfinger]

Peter?
This is a side effect of r3291, the "multiple flash chip" patch. All of
our SPI chips where a generic per-vendor fallback detection exists
(that's all of them right now) will exhibit this problem.

On 11.05.2008 18:01, Fredrik Tolf wrote:
> On Sun, 2008-05-11 at 15:32 +0200, Carl-Daniel Hailfinger wrote:
> > Can you test the patch below? It should have a lower error probability
> > and work fine for you. If you are OK with my changes to your patch,
> > please re-add your Signed-off-by line.
>
> Yes, the flashrom code with that patch seems to detect my chip and read
> its contents quite well. However, the same code does not detect the
> original chip on the board (an MX25L4005) satisfactory, and it seems to
> be a conflict with the SPI code. Here's the output:
>
> Calibrating delay loop... OK.
> No coreboot table found.
> Found chipset "NVIDIA MCP55", enabling flash write... OK.
> Found board "GIGABYTE GA-M57SLI-S4": enabling flash write... Serial
> flash segment 0xfffe0000-0xffffffff enabled
> Serial flash segment 0x000e0000-0x000fffff enabled
> Serial flash segment 0xffee0000-0xffefffff disabled
> Serial flash segment 0xfff80000-0xfffeffff enabled
> LPC write to serial flash enabled
> serial flash pin 29
> OK.
> MX25L4005 found at physical address 0xfff80000.
> unknown SPI chip found at physical address 0x0.
> Multiple flash chips were detected: MX25L4005 unknown SPI chip
> Please specify which chip to use with the -c <chipname> option.

Fredrik, you should be able to work around this by calling
flashrom -c MX25L4005
The patch I sent does not have anything to do with the MX25L4005 problem
you're seeing because that problem was introduced in r3291.
Can you signoff this patch since it works for you?

Regards,
Carl-Daniel

[Peter Stuge]

On Sun, May 11, 2008 at 06:17:48PM +0200, Carl-Daniel Hailfinger wrote:
> On 11.05.2008 18:01, Fredrik Tolf wrote:
> > Yes, the flashrom code with that patch seems to detect my chip
> > and read its contents quite well. However, the same code does not
> > detect the original chip on the board??? (an MX25L4005)
> > satisfactory, and it seems to be a conflict with the SPI code.
>
> Peter?
> This is a side effect of r3291, the "multiple flash chip" patch.

Yes, 3291 exposes this.
> All of our SPI chips where a generic per-vendor fallback detection
> exists (that's all of them right now) will exhibit this problem.

I had reservations about "unknown SPI chip" and this is why.

This is also why I don't like fake flash chip entries for forcing
reads from undetectable chips.

Both simply do not accurately describe or model what is actually
going on and just end up confusing users and code.

It is ridiculous to claim that an "unknown chip" has been detected.
If a chip had been positively detected it would not be unknown.
This is behavior/language I could expect from commercial OS vendors.

I think we have to stop saying this and instead flashrom needs
infrastructure and commands that can correctly model and handle
SPI situation where the memory bus master is no longer transparent.
> Fredrik, you should be able to work around this by calling
> flashrom -c MX25L4005
> The patch I sent does not have anything to do with the MX25L4005
> problem you're seeing because that problem was introduced in r3291.
> Can you signoff this patch since it works for you?

Please do sign off the patch if it works for you Fredrik.
There are issues in flashrom, but as Carl-Daniel writes they are
not closely related and I think the proposed RES change can go in
for now.

//Peter

[Carl-Daniel Hailfinger]

On 12.05.2008 05:16, Peter Stuge wrote:
> On Sun, May 11, 2008 at 06:17:48PM +0200, Carl-Daniel Hailfinger wrote:
> > On 11.05.2008 18:01, Fredrik Tolf wrote:
> > > Yes, the flashrom code with that patch seems to detect my chip
> > > and read its contents quite well. However, the same code does not
> > > detect the original chip on the board??? (an MX25L4005)
> > > satisfactory, and it seems to be a conflict with the SPI code.
> >
> > Peter?
> > This is a side effect of r3291, the "multiple flash chip" patch.
>
> Yes, 3291 exposes this.
> > All of our SPI chips where a generic per-vendor fallback detection
> > exists (that's all of them right now) will exhibit this problem.
>
> I had reservations about "unknown SPI chip" and this is why.
>
> This is also why I don't like fake flash chip entries for forcing
> reads from undetectable chips.
>
> Both simply do not accurately describe or model what is actually
> going on and just end up confusing users and code.

Feel free to propose an alternative that works and doesn't uglify the
flashrom architecture. Be warned that this is difficult.
> It is ridiculous to claim that an "unknown chip" has been detected.
> If a chip had been positively detected it would not be unknown.
> This is behavior/language I could expect from commercial OS vendors.

That was a mismerge. The correct message would have been "unknown
Macronix SPI flash chip" and that's a big improvement over "no chip found".
> I think we have to stop saying this and instead flashrom needs
> infrastructure and commands that can correctly model and handle
> SPI situation where the memory bus master is no longer transparent.

Which infrastructure are you missing?
> > Fredrik, you should be able to work around this by calling
> > flashrom -c MX25L4005
> > The patch I sent does not have anything to do with the MX25L4005
> > problem you're seeing because that problem was introduced in r3291.
> > Can you signoff this patch since it works for you?
>
> Please do sign off the patch if it works for you Fredrik.
> There are issues in flashrom, but as Carl-Daniel writes they are
> not closely related and I think the proposed RES change can go in
> for now.

Indeed. I'm also looking for an Ack from someone who is not Fredrik or me.
The patch is bitrotting in my tree and all SPI restructuring conflicts
with it, so I have to merge manually every time.

Regards,
Carl-Daniel

[Fredrik Tolf]

> Fredrik, you should be able to work around this by calling
> flashrom -c MX25L4005

Yes, it does. I even tried that before, but forgot to mention it.
> The patch I sent does not have anything to do with the MX25L4005 problem
> you're seeing because that problem was introduced in r3291.
> Can you signoff this patch since it works for you?

I see. I thought it was related, since the patch was for SPI in some
generality. Sure thing, then. I'm not sure what to do in this particular
case, though -- am I supposed to re-quote the entire patch with your
sign as well? If not, I'm just adding it here:

Signed-off-by: Fredrik Tolf

(OT: Though, I have to question the utility of these signatures. I've
questioned it silently for myself for the Linux kernel team as well, but
at least they use Git which seals the signature along with the entire
development history with SHA-1, while Coreboot's signatures seem very
much looser than that.)

[Peter Stuge]

On Sun, May 11, 2008 at 03:32:46PM +0200, Carl-Daniel Hailfinger wrote:
> Signed-off-by: Carl-Daniel Hailfinger

Needs a commit message of course.
> {"ST",		"M25P40",		ST_ID,		ST_M25P40,		512,	256,		TEST_UNTESTED,	probe_spi,		spi_chip_erase_c7,	spi_chip_write, spi_chip_read},
> {"ST",		"M25P40",		ST_ID,		ST_M25P40,		512,	256,		TEST_UNTESTED,	probe_spi_rdid,		spi_chip_erase_c7,	spi_chip_write, spi_chip_read},
> {"ST",		"M25P40(old)",		ST_ID,		ST_M25P40_RES,		512,	256,		TEST_UNTESTED,	probe_spi_res,		spi_chip_erase_c7,	spi_chip_write, spi_chip_read},

Hmm. probe_spi_res() calls spi_rdid() first. I don't like duplicating
chips? If multiple checks are needed for one chip I think they should
be in code?

//Peter

[Carl-Daniel Hailfinger]

On 15.05.2008 04:22, Peter Stuge wrote:
> On Sun, May 11, 2008 at 03:32:46PM +0200, Carl-Daniel Hailfinger wrote:
> > Signed-off-by: Carl-Daniel Hailfinger
>
> Needs a commit message of course.

Yes. How about
Add support for the JEDEC RES (Read Electronic Signature and Resume from
Powerdown) SPI command to flashrom to identify older SPI chips which
can't handle JEDEC RDID. Since RES gives a one-byte identifier which is
shared among many different vendors and even different sizes, we want to
match RES as a last resort if RDID returns 0xff 0xff 0xff.
> > {"ST",		"M25P40",		ST_ID,		ST_M25P40,		512,	256,		TEST_UNTESTED,	probe_spi,		spi_chip_erase_c7,	spi_chip_write, spi_chip_read},
> > {"ST",		"M25P40",		ST_ID,		ST_M25P40,		512,	256,		TEST_UNTESTED,	probe_spi_rdid,		spi_chip_erase_c7,	spi_chip_write, spi_chip_read},
> > {"ST",		"M25P40(old)",		ST_ID,		ST_M25P40_RES,		512,	256,		TEST_UNTESTED,	probe_spi_res,		spi_chip_erase_c7,	spi_chip_write, spi_chip_read},
>
> Hmm. probe_spi_res() calls spi_rdid() first. I don't like duplicating
> chips? If multiple checks are needed for one chip I think they should
> be in code?

To be honest, the "M25P40(old)" entry should more look like
"STM25P40/SST25something/AT26something/dozens_of_other_chips". For more
details, see the changelog.

Regards,
Carl-Daniel

[Peter Stuge]

On Thu, May 15, 2008 at 04:37:18AM +0200, Carl-Daniel Hailfinger wrote:
> > Needs a commit message of course.
>
> Add support for the JEDEC RES (Read Electronic Signature and Resume from
> Powerdown) SPI command to flashrom to identify older SPI chips which
> can't handle JEDEC RDID. Since RES gives a one-byte identifier which is
> shared among many different vendors and even different sizes, we want to
> match RES as a last resort if RDID returns 0xff 0xff 0xff.

Great!
> > > {"ST",		"M25P40",		ST_ID,		ST_M25P40,		512,	256,		TEST_UNTESTED,	probe_spi,		spi_chip_erase_c7,	spi_chip_write, spi_chip_read},
> > > {"ST",		"M25P40",		ST_ID,		ST_M25P40,		512,	256,		TEST_UNTESTED,	probe_spi_rdid,		spi_chip_erase_c7,	spi_chip_write, spi_chip_read},
> > > {"ST",		"M25P40(old)",		ST_ID,		ST_M25P40_RES,		512,	256,		TEST_UNTESTED,	probe_spi_res,		spi_chip_erase_c7,	spi_chip_write, spi_chip_read},
> >
> > Hmm. probe_spi_res() calls spi_rdid() first. I don't like duplicating
> > chips? If multiple checks are needed for one chip I think they should
> > be in code?
>
> To be honest, the "M25P40(old)" entry should more look like
> "STM25P40/SST25something/AT26something/dozens_of_other_chips". For
> more details, see the changelog.

With name change per IRC discussion:

Acked-by: Peter Stuge

[Carl-Daniel Hailfinger]

On 15.05.2008 05:06, Peter Stuge wrote:
> On Thu, May 15, 2008 at 04:37:18AM +0200, Carl-Daniel Hailfinger wrote:
> > To be honest, the "M25P40(old)" entry should more look like
> > "STM25P40/SST25something/AT26something/dozens_of_other_chips". For
> > more details, see the changelog.
>
> With name change per IRC discussion:
>
> Acked-by: Peter Stuge

Thanks, r3320. A big merge pain is now gone on my side.

Regards,
Carl-Daniel