Am 09.05.2014 18:33 schrieb Stefan Tauner:
Subject: [PATCH] Add two new states to enum test_state and use it for flashchips.
The new enum test_state looks like this: enum test_state { OK = 0, NT = 1, /* Not tested */ BAD, /* Known to not work */ DEP, /* Support depends on configuration (e.g. Intel flash descriptor) */ NA, /* Not applicable (e.g. write support on ROM chips) */ };
The second new state 'NA' is introduced, among other things, to indicate the erase and write states of real ROMs correctly. This is also implemented by this patch and required to exchange the previous bit mask in struct flashchip with a new struct containing an enum test_state for each operation. The -L output is changed accordingly to print '-' in the case of an N/A state and the wiki output uses a new template producing a greyed out cell. Previous users of enum test_state are not affected by this change (yet).
Signed-off-by: Stefan Tauner stefan.tauner@alumni.tuwien.ac.at
Excellent work, thank you! I particularly like the #define TEST_* trickery to keep flashchips.c readable and relatively unchanged. Except for some corner cases of NA handling as explained below, this is
Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
diff --git a/flashrom.c b/flashrom.c index c20461a..4956ded 100644 --- a/flashrom.c +++ b/flashrom.c @@ -1794,32 +1794,38 @@ void check_chip_supported(const struct flashchip *chip) "clone the contents of this chip (see man page for " "details).\n"); }
AFAICS the test for OTP memory should be extended with a check for ROM chips (i.e. erase/write == NA) where writing isn't possible. Given that this function has informational purposes and does not return any value, it does serve a different purpose compared to chip_safety_check(). It will get called even if none of read/erase/write is specified. My opinion on this is not cast in stone, though.
- if (TEST_OK_MASK != (chip->tested & TEST_OK_MASK)) {
- if ((chip->tested.probe == BAD) || (chip->tested.probe == NT) ||
(chip->tested.read == BAD) || (chip->tested.read == NT) ||(chip->tested.erase == BAD) || (chip->tested.erase == NT) ||(chip->tested.write == BAD) || (chip->tested.write == NT)){
if (chip->tested & TEST_BAD_MASK) {
if ((chip->tested.probe == BAD) ||(chip->tested.read == BAD) ||(chip->tested.erase == BAD) ||(chip->tested.write == BAD)) { msg_cinfo("This flash part has status NOT WORKING for operations:");
if (chip->tested & TEST_BAD_PROBE)
if (chip->tested.probe == BAD) msg_cinfo(" PROBE");
if (chip->tested & TEST_BAD_READ)
if (chip->tested.read == BAD) msg_cinfo(" READ");
if (chip->tested & TEST_BAD_ERASE)
if (chip->tested.erase == BAD) msg_cinfo(" ERASE");
if (chip->tested & TEST_BAD_WRITE)
if (chip->tested.write == BAD) msg_cinfo(" WRITE"); msg_cinfo("\n");
if ((!(chip->tested & TEST_BAD_PROBE) && !(chip->tested & TEST_OK_PROBE)) ||(!(chip->tested & TEST_BAD_READ) && !(chip->tested & TEST_OK_READ)) ||(!(chip->tested & TEST_BAD_ERASE) && !(chip->tested & TEST_OK_ERASE)) ||(!(chip->tested & TEST_BAD_WRITE) && !(chip->tested & TEST_OK_WRITE))) {
if ((chip->tested.probe == NT) ||(chip->tested.read == NT) ||(chip->tested.erase == NT) ||(chip->tested.write == NT)) { msg_cinfo("This flash part has status UNTESTED for operations:");
if (!(chip->tested & TEST_BAD_PROBE) && !(chip->tested & TEST_OK_PROBE))
if (chip->tested.probe == NT) msg_cinfo(" PROBE");
if (!(chip->tested & TEST_BAD_READ) && !(chip->tested & TEST_OK_READ))
if (chip->tested.read == NT) msg_cinfo(" READ");
if (!(chip->tested & TEST_BAD_ERASE) && !(chip->tested & TEST_OK_ERASE))
if (chip->tested.erase == NT) msg_cinfo(" ERASE");
if (!(chip->tested & TEST_BAD_WRITE) && !(chip->tested & TEST_OK_WRITE))
if (chip->tested.write == NT) msg_cinfo(" WRITE"); msg_cinfo("\n");@@ -1862,7 +1868,7 @@ int chip_safety_check(const struct flashctx *flash, int force, int read_it, int
if (read_it || erase_it || write_it || verify_it) { /* Everything needs read. */
if (chip->tested & TEST_BAD_READ) {
if (chip->tested.read == BAD) { msg_cerr("Read is not working on this chip. "); if (!force) return 1;
We need an additional check for NA with message "This chip does not support read", "This chip has no read functionality" or something similar. We should try to have the messages for NA and BAD differ in a way which makes it easy for users (or at least for us) to understand the difference. Same for the cases below.
@@ -1876,7 +1882,7 @@ int chip_safety_check(const struct flashctx *flash, int force, int read_it, int } if (erase_it || write_it) { /* Write needs erase. */
if (chip->tested & TEST_BAD_ERASE) {
if (chip->tested.erase == BAD) { msg_cerr("Erase is not working on this chip. "); if (!force) return 1;@@ -1889,7 +1895,7 @@ int chip_safety_check(const struct flashctx *flash, int force, int read_it, int } } if (write_it) {
if (chip->tested & TEST_BAD_WRITE) {
if (chip->tested.write == BAD) { msg_cerr("Write is not working on this chip. "); if (!force) return 1;diff --git a/ichspi.c b/ichspi.c index 6c394db..c12ad0d 100644 --- a/ichspi.c +++ b/ichspi.c @@ -1236,7 +1236,10 @@ static int ich_hwseq_probe(struct flashctx *flash) msg_cdbg("In that range are %d erase blocks with %d B each.\n", size_high / erase_size_high, erase_size_high); }
- flash->chip->tested = TEST_OK_PREW;
- flash->chip->tested.probe = OK;
- flash->chip->tested.read = OK;
- flash->chip->tested.erase = OK;
- flash->chip->tested.write = OK;
AFAICS a compound literal should have worked here. flash->chip->tested = (struct tested)TEST_OK_PREW;
return 1; }
diff --git a/print_wiki.c b/print_wiki.c index 5dcc4b6..2dc8156 100644 --- a/print_wiki.c +++ b/print_wiki.c @@ -287,7 +286,35 @@ static void print_supported_chips_wiki(int cols) c = !c;
old = f;
t = f->tested;
const char *probe, *read, *write, *erase;switch (f->tested.probe) {case OK: probe = "OK"; break;case BAD: probe = "No"; break;case NA: probe = "NA"; break;
Could you please print "N/A" here and in the cases below for consistency? We already print "N/A" if the voltage is unknown.
case DEP: probe = "Dep"; break;default: probe = "?3"; break;}switch (f->tested.read) {case OK: read = "OK"; break;case BAD: read = "No"; break;case NA: read = "NA"; break;case DEP: read = "Dep"; break;default: read = "?3"; break;}switch (f->tested.erase) {case OK: erase = "OK"; break;case BAD: erase = "No"; break;case NA: erase = "NA"; break;case DEP: erase = "Dep"; break;default: erase = "?3"; break;}switch (f->tested.write) {case OK: write = "OK"; break;case BAD: write = "No"; break;case NA: write = "NA"; break;case DEP: write = "Dep"; break;default: write = "?3"; break;}