Patchwork Locking infrastructure and locking for W39V080FA and PMC 49LF040

2010-02-04 21:11:52

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
\ No newline at end of file

Submitter	Sean Nelson
Date	2010-02-04 21:11:52
Message ID	<4B6B3818.5070401@gmail.com>
Download	mbox \| patch
Permalink	/patch/880/
State	Bitrotted
Headers	show Return-path: <flashrom-bounces@flashrom.org> Envelope-to: patchwork@coreboot.org DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=message-id:date:from:user-agent:mime-version:to:cc:subject :references:in-reply-to:content-type; b=SO6UgmWh255L/84ryGjUiY4jCjtQTTbOvVUOYCwZOxnzs+YL3izog8wjXjzXkNsatE SxK3u1+eyIDDBKy92D6ik3lJyeWhslX4BIrCCJ+GVgxk/4gvFE9n26vNA3mqPqpV3Woh lwCP8VE7cj2Q27aZeUDdtXCNra2YGpjHeNL8k= Message-ID: <4B6B3818.5070401@gmail.com> Date: Thu, 04 Feb 2010 13:11:52 -0800 From: Sean Nelson <audiohacked@gmail.com> User-Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.1.7) Gecko/20100111 Thunderbird/3.0.1 MIME-Version: 1.0 To: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> References: <4B6A6951.8070005@gmail.com> <4B6A9EEE.7060902@gmx.net> In-Reply-To: <4B6A9EEE.7060902@gmx.net> Content-Type: multipart/mixed; boundary="------------060206010301030704010808" Cc: flashrom <flashrom@flashrom.org> Subject: Re: [flashrom] [PATCH] Locking infrastructure and locking for W39V080FA and PMC 49LF040 Precedence: list Sender: flashrom-bounces@flashrom.org Errors-To: flashrom-bounces@flashrom.org

Patchwork Locking infrastructure and locking for W39V080FA and PMC 49LF040

Comments

Patch