[flashrom] [PATCH] Locking infrastructure and locking for W39V080FA and PMC 49LF040
Sean Nelson
audiohacked at gmail.com
Thu Feb 4 22:11:52 CET 2010
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
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: locking_infrastructure_v2.diff
URL: <http://www.flashrom.org/pipermail/flashrom/attachments/20100204/a590371e/attachment.ksh>
More information about the flashrom
mailing list