[flashrom] [PATCH] Intel 28F001BX fix

[Carl-Daniel Hailfinger]

On 21.03.2010 20:52, Sean Nelson wrote:
> On 3/21/10 6:18 AM, Carl-Daniel Hailfinger wrote:
>> According to my datasheet, 28F004S5 can do locking, but it does _not_
>> use registers in a separate address space. Besides that, chips on a
>> Parallel Bus can't do register accesses anyway because the high address
>> bits are not passed through.
>>    
> My fault for skimming over that information in the datasheet. The
> 28F004S5 has a command sequence to unlock blocks; which this patch
> creates unlock_28f004s5. Unlock_28f004s5 checks if the master lock
> isn't set, and then initiates the Clear Block Lock-bits. This might
> (should ?) be split into a printlock and unlock_28f004s5 which should
> only contain Clear Block Lock-bits.

Hm yes. A task for post 0.9.2.


> diff --git a/82802ab.c b/82802ab.c
> index 1316939..d09ab20 100644
> --- a/82802ab.c
> +++ b/82802ab.c
> @@ -195,13 +195,47 @@ int write_82802ab(struct flashchip *flash, uint8_t *buf)
> +int unlock_28f004s5(struct flashrom *flash)
> +{
> +	chipaddr bios = flash->virtual_memory;
> +	uint8_t mcfg, bcfg;
> +
> +	// clear status register
> +	chip_writeb(0x50, bios);
> +
> +	// enter read identifier codes
> +	chip_writeb(0x90, bios);
> +
> +	// read master lock config
> +	mcfg = chip_readb(bios + 3);
> +	
> +	// exit read identifier code
> +	chip_writeb(0xFF, bios);

I'm pretty sure that won't work. You don't have a register space, so you
depend on a special mode to read registers. That special mode is the
read ID mode, and we exit it by writing 0xff.


> +
> +	// unlock: clear block lock-bits, if needed
> +	if (mcfg==0) {
> +		chip_writeb(0x60, bios);
> +		chip_writeb(0xD0, bios);
> +	}

This is dangerous... there's an errata datasheet for that chip which
lists chip errors as a result of unlocking. Found it just now. The
suggested workaround is to just read the block locks, and if they are
unlocked, enable lockdown so nobody sets the locks by accident. Later
revisions of the same chip (of course with the same ID, otherwise it
would be easy) have fully functional locking.
I'd say we just read all locks, and if some are locked, print a warning
at cmsg_info which says that the user may experience a non-working chip,
then try to unlock only locked blocks. For defective unlocked chips and
working locked/unlocked chips this will cause no problems, whereas
people can still change the code if they have a defective chip which is
locked.


> +
> +	chip_writeb(0x90, bios);
> +	for (i = 0; i < flash->total_size * 1024; i+= flash->page_size) {

Sorry, but that won't work. page_size is 256, but lock block size is 64k.


> +		// read block lock config
> +		bcfg = chip_readb(bios + i + 2);
> +		msg_cdbg("block at %06x is %slocked!\n", i, bcfg ? "" : "un");
> +	}
> +	chip_writeb(0xFF, bios);
> +
> +	return 0;
> +}
> diff --git a/flashchips.c b/flashchips.c
> index 86c766b..70d84bf 100644
> --- a/flashchips.c
> +++ b/flashchips.c
> @@ -2358,38 +2358,37 @@ struct flashchip flashchips[] = {
>  		},
>  		.write		= NULL,
>  		.read		= read_memmapped,
>  	},
>  
>  	{
>  		.vendor		= "Intel",
>  		.name		= "28F004S5",
>  		.bustype	= CHIP_BUSTYPE_PARALLEL,
>  		.manufacture_id	= INTEL_ID,
>  		.model_id	= E_28F004S5,
>  		.total_size	= 512,
>  		.page_size	= 256,

It might be an option to increase page_size to 64k.


> -		.feature_bits	= FEATURE_REGISTERMAP,
>  		.tested		= TEST_UNTESTED,
>  		.probe		= probe_82802ab,
>  		.probe_timing	= TIMING_ZERO,	/* Datasheet has no timing info specified */
>  		.block_erasers	=
>  		{
>  			{
>  				.eraseblocks = { {64 * 1024, 8} },
>  				.block_erase = erase_block_82802ab,
>  			},
>  		},
> -		.unlock		= unlock_82802ab,
> +		.unlock		= unlock_28f004s5,
>  		.write		= write_82802ab,
>  		.read		= read_memmapped,
>  	},

 

Regards,
Carl-Daniel

-- 
"I do consider assignment statements and pointer variables to be among
computer science's most valuable treasures."
-- Donald E. Knuth