All unknown SPI chips claim to have status UNTESTED for probe/read/erase/write. That's incorrect. Since the chips are unknown, read/erase/write are unavailable for them. And if probe worked, they wouldn't have needed the generic vendor match in the first place. Mark those chips as BAD for probe/read/erase/write.
That change revealed another bug: If a chip has any TEST_BAD_* flag set, we don't even list the unsupported functions, giving the user the impression that the unsupported functions are tested. Fix that bug as well.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-test_bad/flash.h =================================================================== --- flashrom-test_bad/flash.h (Revision 3754) +++ flashrom-test_bad/flash.h (Arbeitskopie) @@ -95,7 +95,9 @@ #define TEST_BAD_READ (1<<5) #define TEST_BAD_ERASE (1<<6) #define TEST_BAD_WRITE (1<<7) +#define TEST_BAD_PREW (TEST_BAD_PROBE|TEST_BAD_READ|TEST_BAD_ERASE|TEST_BAD_WRITE) #define TEST_BAD_MASK 0xf0 +#define TEST_BAD_SHIFT 4
extern struct flashchip flashchips[];
@@ -106,7 +108,7 @@ * * All LPC/FWH parts (parallel flash) have 8-bit device IDs if there is no * continuation code. - * All SPI parts have 16-bit device IDs. + * SPI parts have 16-bit device IDs if they support RDID. */
#define GENERIC_DEVICE_ID 0xffff /* Only match the vendor ID */ Index: flashrom-test_bad/flashchips.c =================================================================== --- flashrom-test_bad/flashchips.c (Revision 3754) +++ flashrom-test_bad/flashchips.c (Arbeitskopie) @@ -182,12 +182,12 @@ {"Winbond", "W39V080FA", WINBOND_ID, W_39V080FA, 1024, 64*1024, TEST_OK_PREW, 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},
- {"Atmel", "unknown Atmel SPI chip",ATMEL_ID, GENERIC_DEVICE_ID, 0, 256, TEST_UNTESTED, probe_spi_rdid, NULL, NULL}, - {"EON", "unknown EON SPI chip", EON_ID_NOPREFIX,GENERIC_DEVICE_ID, 0, 256, TEST_UNTESTED, probe_spi_rdid, NULL, NULL}, - {"Macronix", "unknown Macronix SPI chip", MX_ID, GENERIC_DEVICE_ID, 0, 256, TEST_UNTESTED, probe_spi_rdid, NULL, NULL}, - {"PMC", "unknown PMC SPI chip", PMC_ID, GENERIC_DEVICE_ID, 0, 256, TEST_UNTESTED, probe_spi_rdid, NULL, NULL}, - {"SST", "unknown SST SPI chip", SST_ID, GENERIC_DEVICE_ID, 0, 256, TEST_UNTESTED, probe_spi_rdid, NULL, NULL}, - {"ST", "unknown ST SPI chip", ST_ID, GENERIC_DEVICE_ID, 0, 256, TEST_UNTESTED, probe_spi_rdid, NULL, NULL}, + {"Atmel", "unknown Atmel SPI chip",ATMEL_ID, GENERIC_DEVICE_ID, 0, 256, TEST_BAD_PREW, probe_spi_rdid, NULL, NULL}, + {"EON", "unknown EON SPI chip", EON_ID_NOPREFIX,GENERIC_DEVICE_ID, 0, 256, TEST_BAD_PREW, probe_spi_rdid, NULL, NULL}, + {"Macronix", "unknown Macronix SPI chip", MX_ID, GENERIC_DEVICE_ID, 0, 256, TEST_BAD_PREW, probe_spi_rdid, NULL, NULL}, + {"PMC", "unknown PMC SPI chip", PMC_ID, GENERIC_DEVICE_ID, 0, 256, TEST_BAD_PREW, probe_spi_rdid, NULL, NULL}, + {"SST", "unknown SST SPI chip", SST_ID, GENERIC_DEVICE_ID, 0, 256, TEST_BAD_PREW, probe_spi_rdid, NULL, NULL}, + {"ST", "unknown ST SPI chip", ST_ID, GENERIC_DEVICE_ID, 0, 256, TEST_BAD_PREW, probe_spi_rdid, NULL, NULL},
{NULL,} }; Index: flashrom-test_bad/flashrom.c =================================================================== --- flashrom-test_bad/flashrom.c (Revision 3753) +++ flashrom-test_bad/flashrom.c (Arbeitskopie) @@ -263,6 +263,7 @@ int force = 0; int read_it = 0, write_it = 0, erase_it = 0, verify_it = 0; int ret = 0, i; + uint32_t untested; #ifdef __FreeBSD__ int io_fd; #endif @@ -499,15 +500,18 @@ if (flash->tested & TEST_BAD_WRITE) printf(" WRITE"); printf("\n"); - } else { + } + untested = ((flash->tested & TEST_BAD_MASK) >> TEST_BAD_SHIFT) | + (flash->tested & TEST_OK_MASK); + if (untested) { printf("This flash part has status UNTESTED for operations:"); - if (!(flash->tested & TEST_OK_PROBE)) + if (!(untested & TEST_OK_PROBE)) printf(" PROBE"); - if (!(flash->tested & TEST_OK_READ)) + if (!(untested & TEST_OK_READ)) printf(" READ"); - if (!(flash->tested & TEST_OK_ERASE)) + if (!(untested & TEST_OK_ERASE)) printf(" ERASE"); - if (!(flash->tested & TEST_OK_WRITE)) + if (!(untested & TEST_OK_WRITE)) printf(" WRITE"); printf("\n"); }
On 15.11.2008 16:51, Carl-Daniel Hailfinger wrote:
All unknown SPI chips claim to have status UNTESTED for probe/read/erase/write. That's incorrect. Since the chips are unknown, read/erase/write are unavailable for them. And if probe worked, they wouldn't have needed the generic vendor match in the first place. Mark those chips as BAD for probe/read/erase/write.
That change revealed another bug: If a chip has any TEST_BAD_* flag set, we don't even list the unsupported functions, giving the user the impression that the unsupported functions are tested. Fix that bug as well.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Thanks to Uwe for testing and finding a bug.
Index: flashrom-test_bad/flash.h =================================================================== --- flashrom-test_bad/flash.h (Revision 3754) +++ flashrom-test_bad/flash.h (Arbeitskopie) @@ -95,7 +95,9 @@ #define TEST_BAD_READ (1<<5) #define TEST_BAD_ERASE (1<<6) #define TEST_BAD_WRITE (1<<7) +#define TEST_BAD_PREW (TEST_BAD_PROBE|TEST_BAD_READ|TEST_BAD_ERASE|TEST_BAD_WRITE) #define TEST_BAD_MASK 0xf0 +#define TEST_BAD_SHIFT 4
extern struct flashchip flashchips[];
@@ -106,7 +108,7 @@ * * All LPC/FWH parts (parallel flash) have 8-bit device IDs if there is no * continuation code. - * All SPI parts have 16-bit device IDs. + * SPI parts have 16-bit device IDs if they support RDID. */
#define GENERIC_DEVICE_ID 0xffff /* Only match the vendor ID */ Index: flashrom-test_bad/flashchips.c =================================================================== --- flashrom-test_bad/flashchips.c (Revision 3754) +++ flashrom-test_bad/flashchips.c (Arbeitskopie) @@ -182,12 +182,12 @@ {"Winbond", "W39V080FA", WINBOND_ID, W_39V080FA, 1024, 64*1024, TEST_OK_PREW, 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},
- {"Atmel", "unknown Atmel SPI chip",ATMEL_ID, GENERIC_DEVICE_ID, 0, 256, TEST_UNTESTED, probe_spi_rdid, NULL, NULL}, - {"EON", "unknown EON SPI chip", EON_ID_NOPREFIX,GENERIC_DEVICE_ID, 0, 256, TEST_UNTESTED, probe_spi_rdid, NULL, NULL}, - {"Macronix", "unknown Macronix SPI chip", MX_ID, GENERIC_DEVICE_ID, 0, 256, TEST_UNTESTED, probe_spi_rdid, NULL, NULL}, - {"PMC", "unknown PMC SPI chip", PMC_ID, GENERIC_DEVICE_ID, 0, 256, TEST_UNTESTED, probe_spi_rdid, NULL, NULL}, - {"SST", "unknown SST SPI chip", SST_ID, GENERIC_DEVICE_ID, 0, 256, TEST_UNTESTED, probe_spi_rdid, NULL, NULL}, - {"ST", "unknown ST SPI chip", ST_ID, GENERIC_DEVICE_ID, 0, 256, TEST_UNTESTED, probe_spi_rdid, NULL, NULL}, + {"Atmel", "unknown Atmel SPI chip",ATMEL_ID, GENERIC_DEVICE_ID, 0, 256, TEST_BAD_PREW, probe_spi_rdid, NULL, NULL}, + {"EON", "unknown EON SPI chip", EON_ID_NOPREFIX,GENERIC_DEVICE_ID, 0, 256, TEST_BAD_PREW, probe_spi_rdid, NULL, NULL}, + {"Macronix", "unknown Macronix SPI chip", MX_ID, GENERIC_DEVICE_ID, 0, 256, TEST_BAD_PREW, probe_spi_rdid, NULL, NULL}, + {"PMC", "unknown PMC SPI chip", PMC_ID, GENERIC_DEVICE_ID, 0, 256, TEST_BAD_PREW, probe_spi_rdid, NULL, NULL}, + {"SST", "unknown SST SPI chip", SST_ID, GENERIC_DEVICE_ID, 0, 256, TEST_BAD_PREW, probe_spi_rdid, NULL, NULL}, + {"ST", "unknown ST SPI chip", ST_ID, GENERIC_DEVICE_ID, 0, 256, TEST_BAD_PREW, probe_spi_rdid, NULL, NULL},
{NULL,} }; Index: flashrom-test_bad/flashrom.c =================================================================== --- flashrom-test_bad/flashrom.c (Revision 3753) +++ flashrom-test_bad/flashrom.c (Arbeitskopie) @@ -263,6 +263,7 @@ int force = 0; int read_it = 0, write_it = 0, erase_it = 0, verify_it = 0; int ret = 0, i; + uint32_t untested; #ifdef __FreeBSD__ int io_fd; #endif @@ -499,15 +500,18 @@ if (flash->tested & TEST_BAD_WRITE) printf(" WRITE"); printf("\n"); - } else { + } + untested = ((flash->tested & TEST_BAD_MASK) >> TEST_BAD_SHIFT) | + (flash->tested & TEST_OK_MASK); + if (TEST_OK_MASK != (untested & TEST_OK_MASK)) { printf("This flash part has status UNTESTED for operations:"); - if (!(flash->tested & TEST_OK_PROBE)) + if (!(untested & TEST_OK_PROBE)) printf(" PROBE"); - if (!(flash->tested & TEST_OK_READ)) + if (!(untested & TEST_OK_READ)) printf(" READ"); - if (!(flash->tested & TEST_OK_ERASE)) + if (!(untested & TEST_OK_ERASE)) printf(" ERASE"); - if (!(flash->tested & TEST_OK_WRITE)) + if (!(untested & TEST_OK_WRITE)) printf(" WRITE"); printf("\n"); }
Carl-Daniel Hailfinger wrote:
All unknown SPI chips claim to have status UNTESTED for probe/read/erase/write. That's incorrect. Since the chips are unknown, read/erase/write are unavailable for them. And if probe worked, they wouldn't have needed the generic vendor match in the first place. Mark those chips as BAD for probe/read/erase/write.
That change revealed another bug: If a chip has any TEST_BAD_* flag set, we don't even list the unsupported functions, giving the user the impression that the unsupported functions are tested. Fix that bug as well.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Thanks to Uwe for testing and finding a bug.
Nak.
Unknown chips can not be tested and known to be bad, which is what TEST_BAD_ means.
If anything, please remove the unknown chips.
The TEST_BAD_ logic improvement is good, but I would like it to explicitly test each flag instead of assuming that TEST_OK_ and TEST_BAD_ flags will overlap. That may not always be the case. Also the _SHIFT define isn't so nice imo. Fix those and that part is:
Acked-by: Peter Stuge peter@stuge.se
On 16.11.2008 06:17, Peter Stuge wrote:
Carl-Daniel Hailfinger wrote:
All unknown SPI chips claim to have status UNTESTED for probe/read/erase/write. That's incorrect. Since the chips are unknown, read/erase/write are unavailable for them. And if probe worked, they wouldn't have needed the generic vendor match in the first place. Mark those chips as BAD for probe/read/erase/write.
That change revealed another bug: If a chip has any TEST_BAD_* flag set, we don't even list the unsupported functions, giving the user the impression that the unsupported functions are tested. Fix that bug as well.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Thanks to Uwe for testing and finding a bug.
Nak.
Unknown chips can not be tested and known to be bad, which is what TEST_BAD_ means.
If anything, please remove the unknown chips.
The TEST_BAD_ logic improvement is good, but I would like it to explicitly test each flag instead of assuming that TEST_OK_ and TEST_BAD_ flags will overlap. That may not always be the case. Also the _SHIFT define isn't so nice imo. Fix those and that part is:
Acked-by: Peter Stuge peter@stuge.se
Thanks, r3780.
Regards, Carl-Daniel
-
Carl-Daniel Hailfinger -
Peter Stuge