Re: [flashrom] [PATCH] Intel 28F001BX fix

[Carl-Daniel Hailfinger]

On 22.03.2010 02:31, Sean Nelson wrote:
> On 3/21/10 5:14 PM, Carl-Daniel Hailfinger wrote:
> > 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.
>
> To access/read the lock bits, we use the same mode to read the chip id.
>
> This patch looks into the write situation for the Intel
> 28F001BX-{B,T}. Looks like they're just a 82802ab page write.
>
> Unlock_28f004s5 has been changed to read all the lock bits and if at
> least one of the block lock bits are set, clear them all. If the
> master lock bit is set, we can't do anything about it, so we return.
>
> Signed-off-by: Sean Nelson

Thanks. A few minor comments. If you address them, this is
Acked-by: Carl-Daniel Hailfinger
> diff --git a/82802ab.c b/82802ab.c
> index 1316939..3938106 100644
> --- a/82802ab.c
> +++ b/82802ab.c
> @@ -195,13 +195,55 @@ int write_82802ab(struct flashchip *flash, uint8_t *buf)
>    	/* erase block by block and write block by block; this is the most secure way */
>    	if (erase_block_82802ab(flash, i * page_size, page_size)) {
>    		fprintf(stderr, "ERASE FAILED!\n");
>    		return -1;
>    	}
>    	write_page_82802ab(bios, buf + i * page_size,
>    			   bios + i * page_size, page_size);
>    }
>    printf("\n");
>    free(tmpbuf);
>
> return 0;
>  }
>
> +int unlock_28f004s5(struct flashrom *flash)
> +{
> chipaddr bios = flash->virtual_memory;
> uint8_t mcfg, bcfg, need_unlock = 0;
> // clear status register

Please use classic C /* FOO */ comments instead of // FOO everywhere.
And before Uwe sees this code, may I suggest you change the first word
in each comment to have an uppercase first letter?
> chip_writeb(0x50, bios);
> // enter read identifier codes
> chip_writeb(0x90, bios);
> // read master lock-bit
> mcfg = chip_readb(bios + 0x3);
> msg_cinfo("master lock is ");

Should be msg_cdbg.
> if (mcfg) {
> msg_cinfo("locked!\n");

If we don't return here, we can use msg_cdbg.
> return -1;

We don't exit the ID mode here. Maybe goto chip_reset or goto out? Then
again, even if the master lock is locked, we can write to the chip as
long as the individual locks are not active AFAICS, so we only should
set some variable can_unlock.
> } else {
> msg_cinfo("unlocked!\n");

msg_cdbg("unlocked.\n");
> }
> // read block lock-bits
> for (i = 0; i < flash->total_size * 1024; i+= (64 * 1024)) {
> bcfg = chip_readb(bios + i + 2); // read block lock config
> msg_cinfo("block lock at %06x is %slocked!\n", i, bcfg ? "" : "un");

msg_cdbg
> if (bcfg) {
> 	need_unlock = 1;
> }
> }
> // exit read identifier code
> chip_writeb(0xFF, bios);
> // unlock: clear block lock-bits, if needed
> if (need_unlock) {

Can you do the following instead?
if (can_unlock && need_unlock) {
> chip_writeb(0x60, bios);
> chip_writeb(0xD0, bios);

AFAIK this causes the chip to return status code indefinitely, so
resetting it might be a good idea.
> }

if (!can_unlock && need_unlock) {
    msg_cerr("At least one block is locked and lockdown is active!\n");
    return -1;
}
> return 0;
>
> +}
> diff --git a/flashchips.c b/flashchips.c
> index 86c766b..1d239da 100644
> --- a/flashchips.c
> +++ b/flashchips.c
> @@ -2306,90 +2306,89 @@ struct flashchip flashchips[] = {
>    	},
>    	.write		= write_coreboot_m29f400bt,
>    	.read		= read_memmapped,
>    },
>
> {
>    	.vendor		= "Intel",
>    	.name		= "28F001BX-B",
>    	.bustype	= CHIP_BUSTYPE_PARALLEL,
>    	.manufacture_id	= INTEL_ID,
>    	.model_id	= P28F001BXB,
>    	.total_size	= 128,
>    	.page_size	= 128 * 1024, /* 8k + 2x4k + 112k */
> .tested		= TEST_BAD_WRITE,
> .tested		= TEST_UNTESTED,
> .probe		= probe_jedec,
> .probe_timing	= TIMING_ZERO,	/* Datasheet has no timing info specified */
> .block_erasers	=
> {
> 	{
> 		.eraseblocks = { 
> 			{8 * 1024, 1},
> 			{4 * 1024, 2},
> 			{112 * 1024, 1},
> 		},
> 		.block_erase = erase_block_82802ab,
> 	},
> },
> .write		= NULL,
> .write		= write_82802ab,
>
> .read		= read_memmapped,
> },
>
> {
> .vendor		= "Intel",
> .name		= "28F001BX-T",
> .bustype	= CHIP_BUSTYPE_PARALLEL,
> .manufacture_id	= INTEL_ID,
> .model_id	= P28F001BXT,
> .total_size	= 128,
> .page_size	= 128 * 1024, /* 112k + 2x4k + 8k */
> .tested		= TEST_BAD_WRITE,
> .tested		= TEST_UNTESTED,
> .probe		= probe_jedec,
> .probe_timing	= TIMING_ZERO,	/* Datasheet has no timing info specified */
> .block_erasers	=
> {
> 	{
> 		.eraseblocks = { 
> 			{112 * 1024, 1},
> 			{4 * 1024, 2},
> 			{8 * 1024, 1},
> 		},
> 		.block_erase = erase_block_82802ab,
> 	},
> },
> .write		= NULL,
> .write		= write_82802ab,
>
> .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,
> .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,
> },
>
> {
> .vendor		= "Intel",
> .name		= "82802AB",
> .bustype	= CHIP_BUSTYPE_FWH,
> .manufacture_id	= INTEL_ID,
> .model_id	= I_82802AB,
> .total_size	= 512,
> .page_size	= 64 * 1024,
> .feature_bits	= FEATURE_REGISTERMAP,

These chips are really tricky. Thanks for taking care of them.

Regards,
Carl-Daniel