I put a Spansion chip on a m57sli. This patch lets flashrom read the chip successfully. Writing is inconsistent.
I've done two writes so far. Writing takes several minutes and I don't get any progress messages from flashrom.
The first run was 4*coreboot.rom and that booted right away with VGA and all. Yay! The read back file matched the original file.
Then I wrote rand.bin which is 2MB of /dev/random. This consistently reads back as something quite different from the original. See 8MB diff at http://stuge.se/m57sli/s25fl016a.diff (rand.bin and back.bin are there too)
//Peter
* Peter Stuge peter@stuge.se [080124 13:38]:
Index: util/flashrom/flash.h
--- util/flashrom/flash.h (revision 3073) +++ util/flashrom/flash.h (working copy) @@ -166,6 +166,14 @@ #define SHARP_LHF00L04 0xCF
/*
- Spansion was previously a joint venture of AMD and Fujitsu.
- S25 chips are SPI. The first device ID byte is memory type and
- the second device ID byte is memory capacity.
- */
+#define SPANSION_ID 0x01 /* Spansion */ +#define SPANSION_S25FL016A 0x0214
This is unrelated to your patch, but I don't like that we list all the IDs as defines in flash.h just to put that define in the same place in flashchips.c.
Can't we just put the ID directly into the array in flashchips.c and rather add another string for the vendor name?
On Thu, Jan 24, 2008 at 01:53:00PM +0100, Stefan Reinauer wrote:
This is unrelated to your patch, but I don't like that we list all the IDs as defines in flash.h just to put that define in the same place in flashchips.c.
Can't we just put the ID directly into the array in flashchips.c and rather add another string for the vendor name?
#define ID is well tested but I'm not adamant in any way. You're basically asking for less information in source and more at run time. I like more info at run time but I still think it would be nice to have IDs in the code.
I was also thinking that it would/should be simple to tell flashrom about these flash chip parameters at run time so as to not need a rebuild with as-yet unsupported flash.
Also, I'll patch away the vendor check for -m so that it would be sufficient to say -m m57sli as long as no two vendors have boards with identical names. (but -m vend:prod will still always work)
//Peter
* Peter Stuge peter@stuge.se [080125 02:17]:
On Thu, Jan 24, 2008 at 01:53:00PM +0100, Stefan Reinauer wrote:
This is unrelated to your patch, but I don't like that we list all the IDs as defines in flash.h just to put that define in the same place in flashchips.c.
Can't we just put the ID directly into the array in flashchips.c and rather add another string for the vendor name?
#define ID is well tested but I'm not adamant in any way. You're basically asking for less information in source and more at run time. I like more info at run time but I still think it would be nice to have IDs in the code.
Not really less information in the source. Just a single place for that information. flash.h is the only place where the VENDOR_ID defines are defined and the array in flashchips.c is the only place where they are used.
In fact i think it would add information if we replace the VENDOR_ID defines with strings and drop the DEVICE_ID defines completely.
{"W29C011", WINBOND_ID, W_29C011, 128, 128, ...}
should rather read
{"Winbond", "W29C011", 0xda, 0xc1, 128, 128, ...}
and we suddenly know the vendor of that chip. gcc will make sure we only store "Winbond" once in the binary.
I was also thinking that it would/should be simple to tell flashrom about these flash chip parameters at run time so as to not need a rebuild with as-yet unsupported flash.
yes, this would be really neat.
Also, I'll patch away the vendor check for -m so that it would be sufficient to say -m m57sli as long as no two vendors have boards with identical names. (but -m vend:prod will still always work)
sounds good.
On Sun, Jan 27, 2008 at 03:15:58AM +0100, Stefan Reinauer wrote:
#define ID is well tested but I'm not adamant in any way. You're basically asking for less information in source and more at run time. I like more info at run time but I still think it would be nice to have IDs in the code.
Not really less information in the source. Just a single place for that information. flash.h is the only place where the VENDOR_ID defines are defined and the array in flashchips.c is the only place where they are used.
In fact i think it would add information if we replace the VENDOR_ID defines with strings and drop the DEVICE_ID defines completely.
{"W29C011", WINBOND_ID, W_29C011, 128, 128, ...}
should rather read
{"Winbond", "W29C011", 0xda, 0xc1, 128, 128, ...}
I like it.
I was also thinking that it would/should be simple to tell flashrom about these flash chip parameters at run time so as to not need a rebuild with as-yet unsupported flash.
yes, this would be really neat.
I'll think a bit more about this. If someone wants to hack on it go for it.
//Peter
On 27.01.2008 08:08, Peter Stuge wrote:
On Sun, Jan 27, 2008 at 03:15:58AM +0100, Stefan Reinauer wrote:
#define ID is well tested but I'm not adamant in any way. You're basically asking for less information in source and more at run time. I like more info at run time but I still think it would be nice to have IDs in the code.
Not really less information in the source. Just a single place for that information. flash.h is the only place where the VENDOR_ID defines are defined and the array in flashchips.c is the only place where they are used.
In fact i think it would add information if we replace the VENDOR_ID defines with strings and drop the DEVICE_ID defines completely.
{"W29C011", WINBOND_ID, W_29C011, 128, 128, ...}
should rather read
{"Winbond", "W29C011", 0xda, 0xc1, 128, 128, ...}
I like it.
Please don't change this unless you are willing to add support for all chips mentioned in a #define to flashchips.c. That of course includes testing. Right now flash.h is an ID database which helps greatly if you need to look up an unsupported ID which is already known. Unfortunately no search engine is able to find data sheets based on the ID, so the flash.h database is absolutely essential.
Regards, Carl-Daniel
Carl-Daniel Hailfinger wrote:
{"W29C011", WINBOND_ID, W_29C011, 128, 128, ...}
should rather read
{"Winbond", "W29C011", 0xda, 0xc1, 128, 128, ...}
I like it.
Please don't change this unless you are willing to add support for all chips mentioned in a #define to flashchips.c. That of course includes testing. Right now flash.h is an ID database which helps greatly if you need to look up an unsupported ID which is already known. Unfortunately no search engine is able to find data sheets based on the ID, so the flash.h database is absolutely essential.
I don't understand. Essential for what exactly?
Heaping up a whole lot of unsupported IDs actually makes the code even harder to understand, which is why i suggested to drop it in the first place.
I think maintaining this list, eh, "database", in form of preprocessor defines is a waste of time.
If we are concerned about keeping databases, let's lend some of the design I put into /dev/bios in 1994 and put a "supported?" flag into the array in flashchips.c. This way we can identify all kinds of chips and give the user a hint that we did not have the opportunity to test the chip (properly). Anticipating from flashrom -V to searching flash.h to getting a datasheet to actually support the chip is a long way and there are obviously many better attempts to provide such help, if we feel it is necessary.
Stefan
On 24.01.2008 13:38, Peter Stuge wrote:
I put a Spansion chip on a m57sli. This patch lets flashrom read the chip successfully. Writing is inconsistent.
I've done two writes so far. Writing takes several minutes and I don't get any progress messages from flashrom.
The first run was 4*coreboot.rom and that booted right away with VGA and all. Yay! The read back file matched the original file.
As expected.
Then I wrote rand.bin which is 2MB of /dev/random. This consistently reads back as something quite different from the original. See 8MB diff at http://stuge.se/m57sli/s25fl016a.diff (rand.bin and back.bin are there too)
If the readback doesn't change between reads, the explanation may be simple. Look at the bit pattern in rand.xxd and back.xxd. Note that probably all bits which are set to 0 in rand.xxd are also 0 in back.xxd. This looks like the chip was not erased between writing 4*coreboot.rom and the random image. Can you verify the chip is indeed blank after erase, and then flash the random image again?
- {"S25FL016A", SPANSION_ID, SPANSION_S25FL016A, 2048, 256,
probe_spi, generic_spi_chip_erase_c7, over512k_spi_chip_write, over512k_spi_chip_read},
You can use generic_spi_* instead of over512k_spi_*. The code is intelligent enough to switch behaviour based on ROM chip size.
Regards, Carl-Daniel
On Thu, Jan 24, 2008 at 04:57:35PM +0100, Carl-Daniel Hailfinger wrote:
Then I wrote rand.bin which is 2MB of /dev/random. This consistently reads back as something quite different
This looks like the chip was not erased between writing 4*coreboot.rom and the random image.
You mean I have to erase? I forgot.
I'm thinking maybe -w should erase by default and that another option (-W?) writes without erase. Thoughts? I can save this idea for the ultimate larflash tool though.
Can you verify the chip is indeed blank after erase, and then flash the random image again?
Yep, that works.
probe_spi, generic_spi_chip_erase_c7, over512k_spi_chip_write, over512k_spi_chip_read},You can use generic_spi_* instead of over512k_spi_*.
Thanks, I was running with generic_, then I changed the code but never rebuilt, read up on the thread and then forgot I had changed the code before the svn diff.
Revised patch attached. Not the best performance, but it works.
m57sli tmp # time flashrom -m gigabyte:m57sli -w 2m.bin 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. S25FL016A found at physical address 0xffe00000. Flash part is S25FL016A (2048 KB).
real 10m10.446s user 10m10.260s sys 0m0.180s m57sli tmp #
//Peter
On 25.01.2008 02:11, Peter Stuge wrote:
On Thu, Jan 24, 2008 at 04:57:35PM +0100, Carl-Daniel Hailfinger wrote:
Then I wrote rand.bin which is 2MB of /dev/random. This consistently reads back as something quite different
This looks like the chip was not erased between writing 4*coreboot.rom and the random image.
You mean I have to erase? I forgot.
Yes-
I'm thinking maybe -w should erase by default and that another option (-W?) writes without erase. Thoughts? I can save this idea for the ultimate larflash tool though.
Flashrom could read in the chip, then issue a warning if it isn't empty. This increases time needed for flashing, though.
Can you verify the chip is indeed blank after erase, and then flash the random image again?
Yep, that works.
Good.
probe_spi, generic_spi_chip_erase_c7, over512k_spi_chip_write, over512k_spi_chip_read},You can use generic_spi_* instead of over512k_spi_*.
Thanks, I was running with generic_, then I changed the code but never rebuilt, read up on the thread and then forgot I had changed the code before the svn diff.
Revised patch attached. Not the best performance, but it works.
m57sli tmp # time flashrom -m gigabyte:m57sli -w 2m.bin 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. S25FL016A found at physical address 0xffe00000. Flash part is S25FL016A (2048 KB).
real 10m10.446s user 10m10.260s sys 0m0.180s m57sli tmp #
//Peter
Add ids and chip entry for Spansion S25FL016A to flashrom, tested, working.
Signed-off-by: Peter Stuge peter@stuge.se
Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Thanks, committed in r3074.
Regards, Carl-Daniel
-
Carl-Daniel Hailfinger -
Peter Stuge -
Stefan Reinauer