On 2/4/10 2:18 AM, Carl-Daniel Hailfinger wrote:
On 04.02.2010 07:29, Sean Nelson wrote:
Locking infrastructure Add lock-status command option. Winbond W39V080FA has 64k lock blocks. PMC Pm49FL004 has 64k lock blocks. Tested, but write/erase fails.
Hm. That's bad. Does the code work in an older revision (e.g. 0.9.1)?
The oldest revision I could compile was r714, but I still couldn't erase or write. Previous revisions had a "rdmsr" compile problem. The board may need a board enable.
+#define NUM_LOCKFUNCTIONS 3
Excessive? I'd say we restrict that to 2 (or maybe even 1) until there is a need for more.
Semantics... if we have multiple lock printing functions, which result do we trust?
Well...for example, the Winbond W39V080FA has two write protect status functions, one for #TBL/#WP and one for Block Lock status.
{ uint8_t *buf; unsigned long numbytes; FILE *image; int ret = 0; unsigned long size;
size = flash->total_size * 1024; buf = (uint8_t *) calloc(size, sizeof(char));
- unlock_flash(flash);
Bad idea. We don't want to unlock the flash so early, especially if we're only planning to read.
Some of the chips have higher lock states like locking-out reading. Also, we would be calling unlock twice otherwise since we need to lock and unlock for erase/write. Some of the chipdrivers unlock, erase/write, and lock for each erase or write function.
print_lock_status_flash(flash);- }
- if (erase_it) { if (flash->tested& TEST_BAD_ERASE) { fprintf(stderr, "Erase is not working on this chip. "); if (!force) { fprintf(stderr, "Aborting.\n"); programmer_shutdown(); return 1; } else { fprintf(stderr, "Continuing anyway.\n"); } } if (erase_flash(flash)) { emergency_help_message(); programmer_shutdown(); return 1; } } else if (read_it) { if (read_flash(flash, filename)) { programmer_shutdown(); return 1; }
- } else {
- } else if (write_it | verify_it) {
This chance can die as well.
The only times the following code is used is for verifying and writing, I don't like using a simple else; but that's just my opinion.
struct stat image_stat; if (flash->tested& TEST_BAD_ERASE) { fprintf(stderr, "Erase is not working on this chip " "and erase is needed for write. "); if (!force) { fprintf(stderr, "Aborting.\n"); programmer_shutdown(); return 1; } else { fprintf(stderr, "Continuing anyway.\n"); } }@@ -1196,26 +1382,28 @@ int doit(struct flashchip *flash, int force, char *filename, int read_it, int wr return 1; } ret = flash->write(flash, buf); if (ret) { fprintf(stderr, "FAILED!\n"); emergency_help_message(); programmer_shutdown(); return 1; } else { printf("COMPLETE.\n"); } }
- lock_flash(flash);
We probably don't want this either. For starters, it means that a previously unlocked chip will now be unusable for all older flashrom versions.
I've removed it but older flashrom versions also did the same locking and unlock sequence for chipdrivers.
if (verify_it) { /* Work around chips which need some time to calm down. */ if (write_it) programmer_delay(1000*1000); ret = verify_flash(flash, buf); /* If we tried to write, and verification now fails, we
- might have an emergency situation.
*/ if (ret&& write_it) emergency_help_message(); }
programmer_shutdown();
diff --git a/jedec.c b/jedec.c index 055e910..2002fd6 100644 --- a/jedec.c +++ b/jedec.c @@ -485,13 +485,33 @@ int erase_block_jedec(struct flashchip *flash, unsigned int page, unsigned int s int mask;
mask = getaddrmask(flash); return erase_block_jedec_common(flash, page, size, mask); }
int erase_chip_jedec(struct flashchip *flash) { int mask;
mask = getaddrmask(flash); return erase_chip_jedec_common(flash, mask); }
+int unlock_block_jedec(struct flashchip *flash, unsigned int block) +{
- chip_writeb(0, flash->virtual_registers + block + 2);
- return 0;
+}
+int lock_block_jedec(struct flashchip *flash, unsigned int block) +{
- chip_writeb(1, flash->virtual_registers + block + 2);
- return 0;
+}
+int lock_status_block_jedec(struct flashchip *flash, unsigned int block) +{
- int lock;
- lock = chip_readb(flash->virtual_registers + block + 2);
- return lock& 0x7;
+}
I've expanded the lock_status_* functions so themselves print the status.
Assuming there are multiple lock functions, which one is correct? If they all have the same results anyway, there's no point in having more than one set of lock functions. If they may have differing results, either something is totally broken or there are different types of locks (and then we'd have to lock and unlock _all_ of them, not just the first one that is successful).
There is another conceptual problem. How do you differentiate between
- temporary reversible software locks
- temporary irreversible software locks (irreversible until next reset etc.)
- permanent software locks (write-once for the whole chip lifetime)
- unchangeable hardware locks (WP#, BPL#)
- locks which lock locks (once that "master" lock is set, other locks
may not be changed)
- read locks (i.e. chip region will return 0xff)
- write locks
All of the above exists in some flash chips supported by flashrom.
I just considered the general case with multiple lock states. Non-SPI is only being considered in this patch. The Lock functions can/should deal with the different lock concepts. You are more than welcome to take my patch and extend it, I only wrote this patch as a starting point for us and was actually expecting you and Uwe to expand on it. From what I could gleam from the chipdrivers and datasheets is we can condense the lock code into my defined lock/unlock/status functions.
Regards, Carl-Daniel
~ Sean Nelson