see patch
On Fri, Jun 27, 2008 at 05:44:01PM +0200, Stefan Reinauer wrote:
- add support for Numonyx (formerly ST) M25PE80
See below.
- drop erroneous "unknown flash chip" entries. They badly clash with the "multiple flash chip support" and are no longer required since it's possible to force a flash chip with Peter's last patch.
Thumbs up on this. I thought on doing this in my patch, but I wanted a flashrom mode that was completely equivalent - ie. it would scan for known vendors. But I am fine with removing this right now.
{"Macronix", "MX25L3205", MX_ID, MX_25L3205, 4096, 256, TEST_OK_PREW, 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},
{"Numonyx", "M25PE80", ST_ID, 0x8014, 1024, 256, TEST_UNTESTED, probe_spi_rdid, spi_chip_erase_c7, spi_chip_write, spi_chip_read},
Does this indent just break in the patch but look good in your file?
And, did you intentionally not make a #define for the product id?
//Peter
* Peter Stuge peter@stuge.se [080627 17:48]:
- drop erroneous "unknown flash chip" entries. They badly clash with the "multiple flash chip support" and are no longer required since it's possible to force a flash chip with Peter's last patch.
Thumbs up on this. I thought on doing this in my patch, but I wanted a flashrom mode that was completely equivalent - ie. it would scan for known vendors. But I am fine with removing this right now.
{"Macronix", "MX25L3205", MX_ID, MX_25L3205, 4096, 256, TEST_OK_PREW, 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},
{"Numonyx", "M25PE80", ST_ID, 0x8014, 1024, 256, TEST_UNTESTED, probe_spi_rdid, spi_chip_erase_c7, spi_chip_write, spi_chip_read},Does this indent just break in the patch but look good in your file?
Thanks for noticing. It's indeed broken. I will fix it in the commit if I get an Acked.
And, did you intentionally not make a #define for the product id?
Yes, that is on purpose. I think it is a bad bad idea to define every flash chip in two different places in two different files in the tree. I think to remember this was discussed quite a while ago, and to some extent agreed upon, even.
The array is the only place in the code really using all those defines, and the each line with the #defined ID also contains the human readable name of the chip, so it really is a false friend of readability maing the code uglier and harder to maintain (much like the PCI IDs #define list)
Stefan
On Fri, Jun 27, 2008 at 06:25:35PM +0200, Stefan Reinauer wrote:
{"Macronix", "MX29F002", MX_ID, MX_29F002, 256, 64 * 1024, TEST_UNTESTED, probe_29f002, erase_29f002, write_29f002},
{"Numonyx", "M25PE80", ST_ID, 0x8014, 1024, 256, TEST_UNTESTED, probe_spi_rdid, spi_chip_erase_c7, spi_chip_write, spi_chip_read},Does this indent just break in the patch but look good in your file?
Thanks for noticing. It's indeed broken. I will fix it in the commit if I get an Acked.
And, did you intentionally not make a #define for the product id?
Yes, that is on purpose. I think it is a bad bad idea to define every flash chip in two different places in two different files in the tree.
With the indent fix:
Acked-by: Peter Stuge peter@stuge.se
* Peter Stuge peter@stuge.se [080627 17:48]:
- drop erroneous "unknown flash chip" entries. They badly clash with the "multiple flash chip support" and are no longer required since it's possible to force a flash chip with Peter's last patch.
Thumbs up on this. I thought on doing this in my patch, but I wanted a flashrom mode that was completely equivalent - ie. it would scan for known vendors. But I am fine with removing this right now.
Great. This has been giving me the weirdest errors and outputs over the time, nearly rendering the tool unusable for production use.
In /dev/bios I had an extra array with { VENDOR_ID, "vendorname" } that I could use to match an unknown flash chip against a vendor.
One idea I had a long time ago was also to always print an vendor/device ID in case the probe command changed the chip's state (ie the read ID is different from the memory read before in non-spi mode)
Then we could identify the "you have a flashchip in your system, but we don't fully know it" case much better.
Stefan
On 27.06.2008 18:34, Stefan Reinauer wrote:
- Peter Stuge peter@stuge.se [080627 17:48]:
- drop erroneous "unknown flash chip" entries. They badly clash with the "multiple flash chip support" and are no longer required since it's possible to force a flash chip with Peter's last patch.
Thumbs up on this. I thought on doing this in my patch, but I wanted a flashrom mode that was completely equivalent - ie. it would scan for known vendors. But I am fine with removing this right now.
Great. This has been giving me the weirdest errors and outputs over the time, nearly rendering the tool unusable for production use.
In /dev/bios I had an extra array with { VENDOR_ID, "vendorname" } that I could use to match an unknown flash chip against a vendor.
Good idea. Do that per bus and we have a nearly perfect solution.
One idea I had a long time ago was also to always print an vendor/device ID in case the probe command changed the chip's state (ie the read ID is different from the memory read before in non-spi mode)
There is a 1/256th chance that real the vendor ID is identical to the content of the flash chip at that location. Besides that, it is possible that some chips don't leave ID mode until the next reboot, so only the initial run may yield correct results. Still, I think that approach is worthwhile.
Then we could identify the "you have a flashchip in your system, but we don't fully know it" case much better.
Indeed.
Regards, Carl-Daniel
On 27.06.2008 17:44, Stefan Reinauer wrote:
see patch
- drop erroneous "unknown flash chip" entries. They badly clash with the "multiple flash chip support" and are no longer required since it's possible to force a flash chip with Peter's last patch.
NACK that part. The functionality provided by the "unknown xy flash chip" entries is still not available otherwise. Removing code just because it does not work perfectly as expected is something you opposed a few days ago.
Regards, Carl-Daniel
Carl-Daniel Hailfinger wrote:
On 27.06.2008 17:44, Stefan Reinauer wrote:
see patch
- drop erroneous "unknown flash chip" entries. They badly clash with the "multiple flash chip support" and are no longer required since it's possible to force a flash chip with Peter's last patch.
NACK that part. The functionality provided by the "unknown xy flash chip" entries is still not available otherwise. Removing code just because it does not work perfectly as expected is something you opposed a few days ago.
And I was convinced that it is ok in case it never worked correctly.
So what do you suggest is the right fix? Dummy entries is not it.
On 27.06.2008 18:53, Stefan Reinauer wrote:
Carl-Daniel Hailfinger wrote:
On 27.06.2008 17:44, Stefan Reinauer wrote:
see patch
- drop erroneous "unknown flash chip" entries. They badly clash with the "multiple flash chip support" and are no longer required since it's possible to force a flash chip with Peter's last patch.
NACK that part. The functionality provided by the "unknown xy flash chip" entries is still not available otherwise. Removing code just because it does not work perfectly as expected is something you opposed a few days ago.
And I was convinced that it is ok in case it never worked correctly.
It did work correctly, but the "multiple flash chip support" patch made it a lot less useful.
So what do you suggest is the right fix? Dummy entries is not it.
Reverting multiple flash chip support would be one of the possible solutions. The preferred way to fix this would be a per-bus probe list, decoding the vendor ID if the device ID is unknown. By the way, that would also be the only correct solution for multiple flash chip recognition.
Regards, Carl-Daniel
On Fri, Jun 27, 2008 at 08:36:38PM +0200, Carl-Daniel Hailfinger wrote:
And I was convinced that it is ok in case it never worked correctly.
It did work correctly, but the "multiple flash chip support" patch made it a lot less useful.
It's not less useful, but it has way more false positives.
So what do you suggest is the right fix? Dummy entries is not it.
Reverting multiple flash chip support would be one of the possible solutions.
I disagree.
The preferred way to fix this would be a per-bus probe list, decoding the vendor ID if the device ID is unknown.
I agree this is something we will have in flashrom, but it will be a big change, long after 1.0.
It will also not neccessarily fix all false positives.
By the way, that would also be the only correct solution for multiple flash chip recognition.
Not neccessarily. (e.g. ID strapping on FWH)
Short term I prefer removing the catchall entries. They will be replaced with code to do the same thing, but in a different way, before 1.0, so rather soon.
The only OK kludge is to not match the catchall entries for anything but the first flash chip, but we might as well just remove them now and not type more text in email but type code instead. :)
//Peter
-
Carl-Daniel Hailfinger -
Peter Stuge -
Stefan Reinauer