[PATCH] Intel 28F001BX fix

[Sean Nelson]

see patch:

[Sean Nelson]

committed in r960.

[Carl-Daniel Hailfinger]

On 20.03.2010 03:56, Sean Nelson wrote:
> {
>    	.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,

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.

Regards,
Carl-Daniel

[Sean Nelson]

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.

[Sean Nelson]

On 3/21/10 12:52 PM, 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.

Forgot my signoff.
Signed-off-by: Sean Nelson

[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

[Sean Nelson]

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

[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

[Sean Nelson]

On 3/21/10 8:03 PM, Carl-Daniel Hailfinger wrote:
> Thanks. A few minor comments. If you address them, this is
> Acked-by: Carl-Daniel Hailfinger

Updated.

[Sean Nelson]

On 3/21/10 9:37 PM, Sean Nelson wrote:
> On 3/21/10 8:03 PM, Carl-Daniel Hailfinger wrote:
> > Thanks. A few minor comments. If you address them, this is
> > Acked-by: Carl-Daniel Hailfinger
>
> Updated.

Thanks. Committed in r965.

[Sean Nelson]

On 3/21/10 9:40 PM, Sean Nelson wrote:
> On 3/21/10 9:37 PM, Sean Nelson wrote:
> > On 3/21/10 8:03 PM, Carl-Daniel Hailfinger wrote:
> > > Thanks. A few minor comments. If you address them, this is
> > > Acked-by: Carl-Daniel Hailfinger
> >
> > Updated.
>
> Thanks. Committed in r965.

I made a few typos and forgot a variable and a prototype in chipdrivers.h
Fix Committed in r966.