Re: [flashrom] [PATCH] Intel 28F004/28F400 support

[Carl-Daniel Hailfinger]

On 01.04.2010 00:48, Michael Karcher wrote:
> This should also fix broken write for non-uniform sectored FWH-like chips.
> These are:
>   Intel 28F001
>   Sharp LHF00L04
>   ST M50FW002
>   ST M50LPW116
>
> Signed-off-by: Michael Karcher <[e-mail]>
> ---
>  82802ab.c     |   85 +++++++++++++++++++++++++++++--------------
>  chipdrivers.h |    3 ++
>  flash.h       |    2 +-
>  flashchips.c  |  110 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  flashchips.h  |    4 ++
>  5 files changed, 175 insertions(+), 29 deletions(-)
>
> diff --git a/82802ab.c b/82802ab.c
> index aa7e45e..27acaa3 100644
> --- a/82802ab.c
> +++ b/82802ab.c
> @@ -48,6 +48,12 @@ int probe_82802ab(struct flashchip *flash)
>   chipaddr bios = flash->virtual_memory;
>   uint8_t id1, id2;
>   uint8_t flashcontent1, flashcontent2;
> +	int id2_offset;
> +
> +	if (flash->feature_bits & FEATURE_ADDR_SHIFTED)

Umm... I'm pretty sure that FEATURE_ADDR_SHIFTED has another meaning
(the 0x555 etc shifted left by one bit).
The id2_offset may be needed anyway, but I propose to use a variable
"shifted" which is 0 for all non-shifted chips and 1 for all shifted
chips. It would be used as left shift for every programmatic access to
special addresses.
Such a change would allow us to kill m29f400bt.c which has buggy write
functions anyway.

Example for probing:

/* Issue JEDEC Product ID Entry command */
chip_writeb(0xAA, bios + (0x5555 & mask) << shifted);
chip_writeb(0x55, bios + (0x2AAA & mask) << shifted);
chip_writeb(0x90, bios + (0x5555 & mask) << shifted);
> +		id2_offset = 2;
> +	else
> +		id2_offset = 1;
>
> /* Reset to get a clean state */
>   chip_writeb(0xFF, bios);
> @@ -58,7 +64,7 @@ int probe_82802ab(struct flashchip *flash)
>   programmer_delay(10);
>
> id1 = chip_readb(bios);

Arguably my "shifted" proposal suggests to use the following line for
consistency:

 	id1 = chip_readb(bios + 0x00 << shifted);
> -	id2 = chip_readb(bios + 0x01);
> +	id2 = chip_readb(bios + id2_offset);

id2 = chip_readb(bios + 0x01 << shifted);
> /* Leave ID mode */
>   chip_writeb(0xFF, bios);
> @@ -72,7 +78,7 @@ int probe_82802ab(struct flashchip *flash)
>
> /* Read the product ID location again. We should now see normal flash contents. */
>   flashcontent1 = chip_readb(bios);
> -	flashcontent2 = chip_readb(bios + 0x01);
> +	flashcontent2 = chip_readb(bios + id2_offset);
>
> if (id1 == flashcontent1)
>       msg_cdbg(", id1 is normal flash content");
> @@ -177,41 +183,64 @@ void write_page_82802ab(chipaddr bios, uint8_t *src,
>
> int write_82802ab(struct flashchip *flash, uint8_t *buf)
>  {
> -	int i;
> -	int total_size = flash->total_size * 1024;
> +	int i, j, blocknum;
> +	int blockoffset = 0;
>   int page_size = flash->page_size;
>   chipaddr bios = flash->virtual_memory;
>   uint8_t *tmpbuf = malloc(page_size);
> +	struct eraseblock * blocks;
> +	int (*erase) (struct flashchip *flash, unsigned int blockaddr,
> +		      unsigned int blocklen);
> +	
> +	/* find first erase layout with a working erase function */
> +	for (i = 0; flash->block_erasers[i].block_erase == NULL && 
> +	            i < NUM_ERASEFUNCTIONS; i++);
> +
> +	if (i == NUM_ERASEFUNCTIONS)
> +	{
> +		msg_perr("No working erase function found - can't write\n");
> +		return -1;
> +	}
> +	msg_pdbg("Chosing block layout #%d\n", i);
> +	blocks = flash->block_erasers[i].eraseblocks;
> +	erase = flash->block_erasers[i].block_erase;

That's too clever to be risked for 0.9.2. I can't see obvious bugs, but
this code triggers my "might break if included directly before a
release" fear.
> if (!tmpbuf) {
>       msg_cerr("Could not allocate memory!\n");
>       exit(1);
>   }
> -	msg_cinfo("Programming page: \n");
> -	for (i = 0; i < total_size / page_size; i++) {
> -		msg_cinfo("\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b");
> -		msg_cinfo("%04d at address: 0x%08x", i, i * page_size);
> -
> -		/* Auto Skip Blocks, which already contain the desired data
> -		 * Faster, because we only write, what has changed
> -		 * More secure, because blocks, which are excluded
> -		 * (with the exclude or layout feature)
> -		 * or not erased and rewritten; their data is retained also in
> -		 * sudden power off situations
> -		 */
> -		chip_readn(tmpbuf, bios + i * page_size, page_size);
> -		if (!memcmp((void *)(buf + i * page_size), tmpbuf, page_size)) {
> -			msg_cdbg("SKIPPED\n");
> -			continue;
> -		}
> -
> -		/* 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)) {
> -			msg_cerr("ERASE FAILED!\n");
> -			return -1;
> +	msg_cinfo("Programming block: \n");
> +	blocknum = 0;
> +	for (i = 0; blocks[i].size != 0; i++) {

This is guaranteed access random data if a chip has NUM_ERASEREGIONS
erase regions for a particular erase function. Either check for
i<NUM_ERASEREGIONS as well or use the chip size as upper limit. The chip
size check is probably more difficult here.
> +		for(j = 0; j < blocks[i].count; j++) {
> +			msg_cinfo("\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b"
> +			          "\b\b\b\b\b\b\b\b\b\b\b");
> +			msg_cinfo("%04d at address: 0x%08x", blocknum,
> +			           blockoffset);
> +
> +			/* Auto Skip Blocks, which already contain the
> +			 * desired data:
> +			 * Faster, because we only write, what has changed
> +			 * More secure, because blocks, which are excluded
> +			 * (with the exclude or layout feature)
> +			 * or not erased and rewritten; their data is
> +			 * retained also in sudden power off situations
> +			 */
> +			chip_readn(tmpbuf, bios + blockoffset, blocks[i].size);
> +			if (!memcmp((void *)(buf + i * page_size), tmpbuf, 
> +				    blocks[i].size)) {
> +				msg_cdbg("SKIPPED\n");
> +			} else {
> +				if (erase(flash, blockoffset, page_size)) {
> +					msg_cerr("ERASE FAILED!\n");
> +					return -1;
> +				}
> +				write_page_82802ab(bios, buf + blockoffset,
> +					bios + blockoffset, blocks[i].size);
> +			}
> +			blockoffset += blocks[i].size;
> +			blocknum++;
>       }
> -		write_page_82802ab(bios, buf + i * page_size,
> -				   bios + i * page_size, page_size);

To be honest, I think the current code in write_82802ab() is something
that should be ripped out and destroyed. It is essentially
special-casing partial writes for a family of chips instead of doing it
correctly in the generic code. The conversion to partial writes will
happen post 0.9.2 anyway, and then this code will be ripped out completely.
Your changes, on the other hand, beat at least some sanity into the
code, so it would be desirable to have them in generic code post-0.9.2.
Please coordinate with David Hendricks who is working on partial flashing.
> }
>   msg_cinfo("DONE!\n");
>   free(tmpbuf);
> diff --git a/chipdrivers.h b/chipdrivers.h
> index 6d5cef0..816088a 100644
> --- a/chipdrivers.h
> +++ b/chipdrivers.h
> @@ -53,6 +53,9 @@ int spi_nbyte_read(int addr, uint8_t *bytes, int len);
>  int spi_read_chunked(struct flashchip *flash, uint8_t *buf, int start, int len, int chunksize);
>  int spi_aai_write(struct flashchip *flash, uint8_t *buf);
>
> +/* i28f00x.c */
> +int write_28f00x(struct flashchip *flash, uint8_t *buf);

I don't see that function anywhere. Did you forget to svn add? Or was
this some aborted attempt?
> +
>  /* 82802ab.c */
>  uint8_t wait_82802ab(chipaddr bios);
>  int probe_82802ab(struct flashchip *flash);
> diff --git a/flashchips.c b/flashchips.c
> index 2dbc1e0..f838ab2 100644
> --- a/flashchips.c
> +++ b/flashchips.c
> @@ -2385,6 +2385,116 @@ struct flashchip flashchips[] = {
>
> {
>       .vendor		= "Intel",
> +		.name		= "28F004BV/BE-B",
> +		.bustype	= CHIP_BUSTYPE_PARALLEL,
> +	},
> +
> +	{
> +		.vendor		= "Intel",
> +		.name		= "28F004BV/BE-T",
> +		.bustype	= CHIP_BUSTYPE_PARALLEL,
> +	},
> +
> +	{
> +		.vendor		= "Intel",
> +		.name		= "28F400BV/CV/CE-B",
> +		.bustype	= CHIP_BUSTYPE_PARALLEL,
> +		.feature_bits	= FEATURE_ADDR_SHIFTED,
> +	},
> +
> +	{
> +		.vendor		= "Intel",
> +		.name		= "28F004BV/BE-T",

That name is identical to the one two chips above AFAICS.
> +		.bustype	= CHIP_BUSTYPE_PARALLEL,
> +		.feature_bits	= FEATURE_ADDR_SHIFTED,

So the bottom two chips have the address left-shifted by 1 bit, but use
full address size?

Suggestions:
How about pointing the FEATURE_ADDR_SHIFTED chips to a to-be-fixed
m29f400bt.c for now? It would reduce jedec.c changes a bit.
How about using erase_flash() in write_82802ab() and dropping any
rolling reflash mechanism until post 0.9.2? It should solve all
non-uniform sector stuff. While that's a fear-reaching change as well, I
believe it makes the code more reviewable, and does allow us to kill
more duplicated code. As an added benefit, conversion to partial write
will be easier.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/

[PATCH] Intel 28F004/28F400 support

[Michael Karcher]

On Thu, Apr 01, 2010 at 01:45:33AM +0200, Carl-Daniel Hailfinger wrote:
> > @@ -48,6 +48,12 @@ int probe_82802ab(struct flashchip *flash)
> >   chipaddr bios = flash->virtual_memory;
> >   uint8_t id1, id2;
> >   uint8_t flashcontent1, flashcontent2;
> > +	int id2_offset;
> > +
> > +	if (flash->feature_bits & FEATURE_ADDR_SHIFTED)
>
> Umm... I'm pretty sure that FEATURE_ADDR_SHIFTED has another meaning
> (the 0x555 etc shifted left by one bit).
> FEATURE_ADDR_SHIFTED was not used up to now. It's meaning is that the
> chip uses word-wide addressing for its commands, but runs in a bytewise
> mode. For JEDEC chips, that means shifting the 0x555 *and* shifting the
> ID offsets. As FWH-like chips don't have magic addresses, on these chips
> shifting addresses only means the ID offset.
> The id2_offset may be needed anyway, but I propose to use a variable
> "shifted" which is 0 for all non-shifted chips and 1 for all shifted
> chips. It would be used as left shift for every programmatic access to
> special addresses.
> This is fine, of course.
> Such a change would allow us to kill m29f400bt.c which has buggy write
> functions anyway.
>
> /* Issue JEDEC Product ID Entry command */
> chip_writeb(0xAA, bios + (0x5555 & mask) << shifted);
> chip_writeb(0x55, bios + (0x2AAA & mask) << shifted);
> chip_writeb(0x90, bios + (0x5555 & mask) << shifted);
> Correct, but implemenenting this in jedec.c is out of scope for this patch.
> > id1 = chip_readb(bios);
> > Arguably my "shifted" proposal suggests to use the following line for
> > consistency:
> >   id1 = chip_readb(bios + 0x00 << shifted);
> > -	id2 = chip_readb(bios + 0x01);
> > +	id2 = chip_readb(bios + id2_offset);
> > id2 = chip_readb(bios + 0x01 << shifted);
> > No problem to change it to that way.
> > +	struct eraseblock * blocks;
> > +	int (*erase) (struct flashchip *flash, unsigned int blockaddr,
> > +		      unsigned int blocklen);
> > +	
> > +	/* find first erase layout with a working erase function */
> > +	for (i = 0; flash->block_erasers[i].block_erase == NULL && 
> > +	            i < NUM_ERASEFUNCTIONS; i++);
> > +
> > +	if (i == NUM_ERASEFUNCTIONS)
> > +	{
> > +		msg_perr("No working erase function found - can't write\n");
> > +		return -1;
> > +	}
> > +	msg_pdbg("Chosing block layout #%d\n", i);
> > +	blocks = flash->block_erasers[i].eraseblocks;
> > +	erase = flash->block_erasers[i].block_erase;
> > That's too clever to be risked for 0.9.2. I can't see obvious bugs, but
> > this code triggers my "might break if included directly before a
> > release" fear.
> > Definitely this patch in its entirety is a post-0.9.2 patch. But on the
> > other hand we have some broken chips now, as the old write_82802ab function
> > does not handle chips with non-uniform block sizes, which were listed in my
> > commit message. And I already found a flaw:
> > if (!tmpbuf) {
> >       msg_cerr("Could not allocate memory!\n");
> >       exit(1);
> >   }
> > tmpbuf is sized to flash->page_size. It would be better to look for the
> > maximum block size in block_erasers[i] yourself, especially as I didn't
> > check the page size for the chips referred above. The next iteration of
> > this patch should fix this by either checking and adjusting page sizes
> > or removing the use of the dreaded page_size variable. I prefer the
> > former approach.
> > +	msg_cinfo("Programming block: \n");
> > +	blocknum = 0;
> > +	for (i = 0; blocks[i].size != 0; i++) {
> > This is guaranteed access random data if a chip has NUM_ERASEREGIONS
> > erase regions for a particular erase function. Either check for
> > i<NUM_ERASEREGIONS as well or use the chip size as upper limit. The chip
> > size check is probably more difficult here.
> > i < NUM_ERASEREGIONS and skipping regions with size==0 seems to be the
> > correct answer.
> To be honest, I think the current code in write_82802ab() is something
> that should be ripped out and destroyed. It is essentially
> special-casing partial writes for a family of chips instead of doing it
> correctly in the generic code.
> We can agree on that.
> The conversion to partial writes will
> happen post 0.9.2 anyway, and then this code will be ripped out completely.
> I would be really happy to see that!
> Your changes, on the other hand, beat at least some sanity into the
> code, so it would be desirable to have them in generic code post-0.9.2.
> Please coordinate with David Hendricks who is working on partial flashing.
> Of course.
> > diff --git a/chipdrivers.h b/chipdrivers.h
> > index 6d5cef0..816088a 100644
> > --- a/chipdrivers.h
> > +++ b/chipdrivers.h
> > @@ -53,6 +53,9 @@ int spi_nbyte_read(int addr, uint8_t *bytes, int len);
> >  int spi_read_chunked(struct flashchip *flash, uint8_t *buf, int start, int len, int chunksize);
> >  int spi_aai_write(struct flashchip *flash, uint8_t *buf);
> >
> > +/* i28f00x.c */
> > +int write_28f00x(struct flashchip *flash, uint8_t *buf);
> > I don't see that function anywhere. Did you forget to svn add? Or was
> > this some aborted attempt?
> > This hunk is bogus. write_28f00x was essentially a clone of write_82802ab
> > walking along the erase blocks, but I folded that back into 82802ab.c
> > directly.
> > {
> >       .vendor		= "Intel",
> > +		.name		= "28F004BV/BE-B",
> > +		.bustype	= CHIP_BUSTYPE_PARALLEL,
> > +	},
> > +
> > +	{
> > +		.vendor		= "Intel",
> > +		.name		= "28F004BV/BE-T",
> > +		.bustype	= CHIP_BUSTYPE_PARALLEL,
> > +	},
> > +
> > +	{
> > +		.vendor		= "Intel",
> > +		.name		= "28F400BV/CV/CE-B",
> > +		.bustype	= CHIP_BUSTYPE_PARALLEL,
> > +		.feature_bits	= FEATURE_ADDR_SHIFTED,
> > +	},
> > +
> > +	{
> > +		.vendor		= "Intel",
> > +		.name		= "28F004BV/BE-T",
>
> That name is identical to the one two chips above AFAICS.
> Yeah, copy/pasto. Shoud read "28F400BV/CV/CE-T"
> > +		.bustype	= CHIP_BUSTYPE_PARALLEL,
> > +		.feature_bits	= FEATURE_ADDR_SHIFTED,
> > So the bottom two chips have the address left-shifted by 1 bit, but use
> > full address size?
> > They are not JEDEC-like but FWH-like, so the concept of address sizes
> > doesn't apply.
> Suggestions:
> How about pointing the FEATURE_ADDR_SHIFTED chips to a to-be-fixed
> m29f400bt.c for now?
> m29f400bt.c is/would be for FEATURE_ADDR_SHIFTED JEDEC chips, not
> FEATURE_ADDR_SHIFTED FWH-like chips, so it's not directly connected
> to this patch, as far as I see.
> It would reduce jedec.c changes a bit.
> How about using erase_flash() in write_82802ab() and dropping any
> rolling reflash mechanism until post 0.9.2?
> As I really like the rolling reflash stuff, I don't feel like ripping
> it out. On the other hand, you are right. This code is broken (as the
> FWH unification applied it to non-uniform-sized chips) right now, and
> that should be fixed before release. Having it work correctly is more
> important than the rolling reflash stuff, so I can resubmit a version
> that rips out the blockwise erase/write.
> It should solve all non-uniform sector stuff.
> Would a patch removing the rolling reflash be accepted before 0.9.2?

Thank you very much for your review!

Regards,
  Michael Karcher

[PATCH] Intel 28F004/28F400 support

[Carl-Daniel Hailfinger]

On 01.04.2010 07:59, Michael Karcher wrote:
> On Thu, Apr 01, 2010 at 01:45:33AM +0200, Carl-Daniel Hailfinger wrote:
> > > @@ -48,6 +48,12 @@ int probe_82802ab(struct flashchip *flash)
> > >   chipaddr bios = flash->virtual_memory;
> > >   uint8_t id1, id2;
> > >   uint8_t flashcontent1, flashcontent2;
> > > +	int id2_offset;
> > > +
> > > +	if (flash->feature_bits & FEATURE_ADDR_SHIFTED)
> >
> > Umm... I'm pretty sure that FEATURE_ADDR_SHIFTED has another meaning
> > (the 0x555 etc shifted left by one bit).
>
> FEATURE_ADDR_SHIFTED was not used up to now. It's meaning is that the
> chip uses word-wide addressing for its commands, but runs in a bytewise
> mode. For JEDEC chips, that means shifting the 0x555 *and* shifting the
> ID offsets. As FWH-like chips don't have magic addresses, on these chips
> shifting addresses only means the ID offset.

Right. My bad. I had misread the file name and looked at jedec.c.
> > The id2_offset may be needed anyway, but I propose to use a variable
> > "shifted" which is 0 for all non-shifted chips and 1 for all shifted
> > chips. It would be used as left shift for every programmatic access to
> > special addresses.
>
> This is fine, of course.
> > Such a change would allow us to kill m29f400bt.c which has buggy write
> > functions anyway.
> >
> > /* Issue JEDEC Product ID Entry command */
> > chip_writeb(0xAA, bios + (0x5555 & mask) << shifted);
> > chip_writeb(0x55, bios + (0x2AAA & mask) << shifted);
> > chip_writeb(0x90, bios + (0x5555 & mask) << shifted);
>
> Correct, but implemenenting this in jedec.c is out of scope for this patch.

Yes.
> > > id1 = chip_readb(bios);
> >
> > Arguably my "shifted" proposal suggests to use the following line for
> > consistency:
> >   id1 = chip_readb(bios + 0x00 << shifted);
> > > -	id2 = chip_readb(bios + 0x01);
> > > +	id2 = chip_readb(bios + id2_offset);
> >
> > id2 = chip_readb(bios + 0x01 << shifted);
>
> No problem to change it to that way.

Cool.
> > > +	struct eraseblock * blocks;
> > > +	int (*erase) (struct flashchip *flash, unsigned int blockaddr,
> > > +		      unsigned int blocklen);
> > > +	
> > > +	/* find first erase layout with a working erase function */
> > > +	for (i = 0; flash->block_erasers[i].block_erase == NULL && 
> > > +	            i < NUM_ERASEFUNCTIONS; i++);
> > > +
> > > +	if (i == NUM_ERASEFUNCTIONS)
> > > +	{
> > > +		msg_perr("No working erase function found - can't write\n");
> > > +		return -1;
> > > +	}
> > > +	msg_pdbg("Chosing block layout #%d\n", i);
> > > +	blocks = flash->block_erasers[i].eraseblocks;
> > > +	erase = flash->block_erasers[i].block_erase;
> >
> > That's too clever to be risked for 0.9.2. I can't see obvious bugs, but
> > this code triggers my "might break if included directly before a
> > release" fear.
>
> Definitely this patch in its entirety is a post-0.9.2 patch. But on the
> other hand we have some broken chips now, as the old write_82802ab function
> does not handle chips with non-uniform block sizes, which were listed in my
> commit message. And I already found a flaw:
> > > if (!tmpbuf) {
> > >       msg_cerr("Could not allocate memory!\n");
> > >       exit(1);
> > >   }
> >
> > tmpbuf is sized to flash->page_size. It would be better to look for the
> > maximum block size in block_erasers[i] yourself, especially as I didn't
> > check the page size for the chips referred above. The next iteration of
> > this patch should fix this by either checking and adjusting page sizes
> > or removing the use of the dreaded page_size variable. I prefer the
> > former approach.

page_size needs to die soon because it is ill-defined. Some chips use it
as max write size, some use it as eraseblock size, others use it as max
read size. Killing page_size is post 0.9.2, though.
> > > +	msg_cinfo("Programming block: \n");
> > > +	blocknum = 0;
> > > +	for (i = 0; blocks[i].size != 0; i++) {
> >
> > This is guaranteed access random data if a chip has NUM_ERASEREGIONS
> > erase regions for a particular erase function. Either check for
> > i<NUM_ERASEREGIONS as well or use the chip size as upper limit. The chip
> > size check is probably more difficult here.
>
> i < NUM_ERASEREGIONS and skipping regions with size==0 seems to be the
> correct answer.
> > To be honest, I think the current code in write_82802ab() is something
> > that should be ripped out and destroyed. It is essentially
> > special-casing partial writes for a family of chips instead of doing it
> > correctly in the generic code.
>
> We can agree on that.
> > The conversion to partial writes will
> > happen post 0.9.2 anyway, and then this code will be ripped out completely.
>
> I would be really happy to see that!
> > Your changes, on the other hand, beat at least some sanity into the
> > code, so it would be desirable to have them in generic code post-0.9.2.
> > Please coordinate with David Hendricks who is working on partial flashing.
>
> Of course.
> > > diff --git a/chipdrivers.h b/chipdrivers.h
> > > index 6d5cef0..816088a 100644
> > > --- a/chipdrivers.h
> > > +++ b/chipdrivers.h
> > > @@ -53,6 +53,9 @@ int spi_nbyte_read(int addr, uint8_t *bytes, int len);
> > >  int spi_read_chunked(struct flashchip *flash, uint8_t *buf, int start, int len, int chunksize);
> > >  int spi_aai_write(struct flashchip *flash, uint8_t *buf);
> > >
> > > +/* i28f00x.c */
> > > +int write_28f00x(struct flashchip *flash, uint8_t *buf);
> >
> > I don't see that function anywhere. Did you forget to svn add? Or was
> > this some aborted attempt?
>
> This hunk is bogus. write_28f00x was essentially a clone of write_82802ab
> walking along the erase blocks, but I folded that back into 82802ab.c
> directly.
> > > {
> > >       .vendor		= "Intel",
> > > +		.name		= "28F004BV/BE-B",
> > > +		.bustype	= CHIP_BUSTYPE_PARALLEL,
> > > +	},
> > > +
> > > +	{
> > > +		.vendor		= "Intel",
> > > +		.name		= "28F004BV/BE-T",
> > > +		.bustype	= CHIP_BUSTYPE_PARALLEL,
> > > +	},
> > > +
> > > +	{
> > > +		.vendor		= "Intel",
> > > +		.name		= "28F400BV/CV/CE-B",
> > > +		.bustype	= CHIP_BUSTYPE_PARALLEL,
> > > +		.feature_bits	= FEATURE_ADDR_SHIFTED,
> > > +	},
> > > +
> > > +	{
> > > +		.vendor		= "Intel",
> > > +		.name		= "28F004BV/BE-T",
> >
> > That name is identical to the one two chips above AFAICS.
>
> Yeah, copy/pasto. Shoud read "28F400BV/CV/CE-T"
> > > +		.bustype	= CHIP_BUSTYPE_PARALLEL,
> > > +		.feature_bits	= FEATURE_ADDR_SHIFTED,
> >
> > So the bottom two chips have the address left-shifted by 1 bit, but use
> > full address size?
>
> They are not JEDEC-like but FWH-like, so the concept of address sizes
> doesn't apply.
> > Suggestions:
> > How about pointing the FEATURE_ADDR_SHIFTED chips to a to-be-fixed
> > m29f400bt.c for now?
>
> m29f400bt.c is/would be for FEATURE_ADDR_SHIFTED JEDEC chips, not
> FEATURE_ADDR_SHIFTED FWH-like chips, so it's not directly connected
> to this patch, as far as I see.

Right. Sorry for the mis-review (is that even a word?) and thanks for
your detailed response.
> > It would reduce jedec.c changes a bit.
> > How about using erase_flash() in write_82802ab() and dropping any
> > rolling reflash mechanism until post 0.9.2?
>
> As I really like the rolling reflash stuff, I don't feel like ripping
> it out. On the other hand, you are right. This code is broken (as the
> FWH unification applied it to non-uniform-sized chips) right now, and
> that should be fixed before release. Having it work correctly is more
> important than the rolling reflash stuff, so I can resubmit a version
> that rips out the blockwise erase/write.
> > It should solve all non-uniform sector stuff.
>
> Would a patch removing the rolling reflash be accepted before 0.9.2?

If you can get it tested on one previously supported 82802ab-style chip
and on one 82802ab-style chip with non-uniform sector sizes, sure.

Some people might complain about a feature regression, but to be honest
this code is broken for some cases right now and sometimes radical
surgery is initially painful but crucial for long-term viability. We're
definitely shooting for long-term support and development of flashrom,
and everything which makes continued development easier will get a
thumbs up from me.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/

[Request for testers] Intel 28F004/28F400 support

[Michael Karcher]

Am Donnerstag, den 01.04.2010, 12:56 +0200 schrieb Carl-Daniel
Hailfinger:
> > As I really like the rolling reflash stuff, I don't feel like ripping
> > it out. On the other hand, you are right. This code is broken (as the
> > FWH unification applied it to non-uniform-sized chips) right now, and
> > that should be fixed before release. Having it work correctly is more
> > important than the rolling reflash stuff, so I can resubmit a version
> > that rips out the blockwise erase/write.
> > > It should solve all non-uniform sector stuff.
> > > Would a patch removing the rolling reflash be accepted before 0.9.2?
> > > If you can get it tested on one previously supported 82802ab-style chip
> > > and on one 82802ab-style chip with non-uniform sector sizes, sure.
> > > I don't have access to a previously supported 82802ab chip (with uniform
> > > sector size). Any testers?
> > > Hint: Chips that are OK for testing are:
> > >  Intel 28F004S5 (the 'S' is important here)
> > >  Intel 82802AB/AC
> > >  ST M50FLW040A/B
> > >  ST M50FLW080A/B
> > >  ST M50FW016
> > >  ST M50FW040
> > >  ST M50FW080

If I get my Thinkpad T20 to boot, I can try with the 28F004 in it. It's
a non-uniform flashchip supported by this patch.
> Some people might complain about a feature regression, but to be honest
> this code is broken for some cases right now and sometimes radical
> surgery is initially painful but crucial for long-term viability. We're
> definitely shooting for long-term support and development of flashrom,
> and everything which makes continued development easier will get a
> thumbs up from me.
> OK, I get you right that I should resubmit the patch without the
> incremental write and with "<< shifted", and have your Ack? Of course
> without the bogus "i28f00x" line in chipdrivers.h

Regards,
  Michael Karcher

[Request for testers] Intel 28F004/28F400 support

[Carl-Daniel Hailfinger]

On 01.04.2010 16:31, Michael Karcher wrote:
> Am Donnerstag, den 01.04.2010, 12:56 +0200 schrieb Carl-Daniel
> Hailfinger:
> > > As I really like the rolling reflash stuff, I don't feel like ripping
> > > it out. On the other hand, you are right. This code is broken (as the
> > > FWH unification applied it to non-uniform-sized chips) right now, and
> > > that should be fixed before release. Having it work correctly is more
> > > important than the rolling reflash stuff, so I can resubmit a version
> > > that rips out the blockwise erase/write.
> > > > It should solve all non-uniform sector stuff.
> > >
> > > Would a patch removing the rolling reflash be accepted before 0.9.2?
> >
> > If you can get it tested on one previously supported 82802ab-style chip
> > and on one 82802ab-style chip with non-uniform sector sizes, sure.
>
> I don't have access to a previously supported 82802ab chip (with uniform
> sector size). Any testers?
> Hint: Chips that are OK for testing are:
>  Intel 28F004S5 (the 'S' is important here)
>  Intel 82802AB/AC
>  ST M50FLW040A/B
>  ST M50FLW080A/B
>  ST M50FW016
>  ST M50FW040
>  ST M50FW080
>
> If I get my Thinkpad T20 to boot, I can try with the 28F004 in it. It's
> a non-uniform flashchip supported by this patch.

Cool, but please don't risk death of that laptop.
> > Some people might complain about a feature regression, but to be honest
> > this code is broken for some cases right now and sometimes radical
> > surgery is initially painful but crucial for long-term viability. We're
> > definitely shooting for long-term support and development of flashrom,
> > and everything which makes continued development easier will get a
> > thumbs up from me.
>
> OK, I get you right that I should resubmit the patch without the
> incremental write and with "<< shifted", and have your Ack? Of course
> without the bogus "i28f00x" line in chipdrivers.h

"without the incremental write" -> "killing the incremental write".
But yes, I will review and ack such a patch.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/

[PATCH, v2] Intel 28F004/28F400 support

[Michael Karcher]

This patch removes blockwise write for i82802ab chips. It will be
reintroduced in post-0.9.2 in a generic way. It is needed to fix
FWH-like chips with non-uniform sectors. It adds IDs for 28F004/28F400

These are:
  Intel 28F001
  Sharp LHF00L04
  ST M50FW002
  ST M50LPW116

Signed-off-by: Michael Karcher <[e-mail]>
---
 82802ab.c    |   54 +++++++++-------------------
 flash.h      |    2 +-
 flashchips.c |  110 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 flashchips.h |    4 ++
 4 files changed, 133 insertions(+), 37 deletions(-)
 create mode 100755 dmidecode

diff --git a/82802ab.c b/82802ab.c
index aa7e45e..0d1bd1a 100644
--- a/82802ab.c
+++ b/82802ab.c
@@ -48,6 +48,7 @@ int probe_82802ab(struct flashchip *flash)
 	chipaddr bios = flash->virtual_memory;
 	uint8_t id1, id2;
 	uint8_t flashcontent1, flashcontent2;
+	int shifted = (flash->feature_bits & FEATURE_ADDR_SHIFTED) != 0;
 
 	/* Reset to get a clean state */
 	chip_writeb(0xFF, bios);
@@ -57,8 +58,8 @@ int probe_82802ab(struct flashchip *flash)
 	chip_writeb(0x90, bios);
 	programmer_delay(10);
 
-	id1 = chip_readb(bios);
-	id2 = chip_readb(bios + 0x01);
+	id1 = chip_readb(bios + (0x00 << shifted));
+	id2 = chip_readb(bios + (0x01 << shifted));
 
 	/* Leave ID mode */
 	chip_writeb(0xFF, bios);
@@ -71,8 +72,8 @@ int probe_82802ab(struct flashchip *flash)
 		msg_cdbg(", id1 parity violation");
 
 	/* Read the product ID location again. We should now see normal flash contents. */
-	flashcontent1 = chip_readb(bios);
-	flashcontent2 = chip_readb(bios + 0x01);
+	flashcontent1 = chip_readb(bios + (0x00 << shifted));
+	flashcontent2 = chip_readb(bios + (0x01 << shifted));
 
 	if (id1 == flashcontent1)
 		msg_cdbg(", id1 is normal flash content");
@@ -178,44 +179,25 @@ void write_page_82802ab(chipaddr bios, uint8_t *src,
 int write_82802ab(struct flashchip *flash, uint8_t *buf)
 {
 	int i;
-	int total_size = flash->total_size * 1024;
-	int page_size = flash->page_size;
 	chipaddr bios = flash->virtual_memory;
-	uint8_t *tmpbuf = malloc(page_size);
 
-	if (!tmpbuf) {
-		msg_cerr("Could not allocate memory!\n");
-		exit(1);
+	if (erase_flash(flash)) {
+		msg_cerr("ERASE FAILED!\n");
+		return -1;
 	}
-	msg_cinfo("Programming page: \n");
-	for (i = 0; i < total_size / page_size; i++) {
-		msg_cinfo("\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b");
-		msg_cinfo("%04d at address: 0x%08x", i, i * page_size);
-
-		/* Auto Skip Blocks, which already contain the desired data
-		 * Faster, because we only write, what has changed
-		 * More secure, because blocks, which are excluded
-		 * (with the exclude or layout feature)
-		 * or not erased and rewritten; their data is retained also in
-		 * sudden power off situations
-		 */
-		chip_readn(tmpbuf, bios + i * page_size, page_size);
-		if (!memcmp((void *)(buf + i * page_size), tmpbuf, page_size)) {
-			msg_cdbg("SKIPPED\n");
-			continue;
-		}
 
-		/* 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)) {
-			msg_cerr("ERASE FAILED!\n");
-			return -1;
-		}
-		write_page_82802ab(bios, buf + i * page_size,
-				   bios + i * page_size, page_size);
+	msg_cinfo("Programming page: ");
+	for (i = 0; i < flash->total_size; i++) {
+		if ((i & 0x3) == 0)
+			msg_cinfo("address: 0x%08lx", (unsigned long)i * 1024);
+
+                write_page_82802ab(bios, buf + i * 1024, bios + i * 1024, 1024);
+
+		if ((i & 0x3) == 0)
+			msg_cinfo("\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b");
 	}
-	msg_cinfo("DONE!\n");
-	free(tmpbuf);
 
+	msg_cinfo("DONE!\n");
 	return 0;
 }
 
diff --git a/dmidecode b/dmidecode
new file mode 100755
index 0000000..e69de29
diff --git a/flash.h b/flash.h
index aee95a3..0a74363 100644
--- a/flash.h
+++ b/flash.h
@@ -166,7 +166,7 @@ enum chipbustype {
 #define FEATURE_ADDR_MASK	(3 << 2)
 #define FEATURE_ADDR_2AA	(1 << 2)
 #define FEATURE_ADDR_AAA	(2 << 2)
-#define FEATURE_ADDR_SHIFTED	0
+#define FEATURE_ADDR_SHIFTED	(1 << 5)
 
 struct flashchip {
 	const char *vendor;
diff --git a/flashchips.c b/flashchips.c
index 2dbc1e0..a9b6551 100644
--- a/flashchips.c
+++ b/flashchips.c
@@ -2385,6 +2385,116 @@ struct flashchip flashchips[] = {
 
 	{
 		.vendor		= "Intel",
+		.name		= "28F004BV/BE-B",
+		.bustype	= CHIP_BUSTYPE_PARALLEL,
+		.manufacture_id	= INTEL_ID,
+		.model_id	= P28F004BB,
+		.total_size	= 512,
+		.page_size	= 128 * 1024, /* maximal block size */
+		.tested		= TEST_UNTESTED,
+		.probe		= probe_82802ab,
+		.probe_timing	= TIMING_ZERO,	/* Datasheet has no timing info specified */
+		.block_erasers	=
+		{
+			{
+				.eraseblocks = { 
+					{16 * 1024, 1},
+					{8 * 1024, 2},
+					{96 * 1024, 1},
+					{128 * 1024, 3},
+				},
+				.block_erase = erase_block_82802ab,
+			},
+		},
+		.write		= write_82802ab,
+		.read		= read_memmapped,
+	},
+
+	{
+		.vendor		= "Intel",
+		.name		= "28F004BV/BE-T",
+		.bustype	= CHIP_BUSTYPE_PARALLEL,
+		.manufacture_id	= INTEL_ID,
+		.model_id	= P28F004BT,
+		.total_size	= 512,
+		.page_size	= 128 * 1024, /* maximal block size */
+		.tested		= TEST_UNTESTED,
+		.probe		= probe_82802ab,
+		.probe_timing	= TIMING_ZERO,	/* Datasheet has no timing info specified */
+		.block_erasers	=
+		{
+			{
+				.eraseblocks = { 
+					{128 * 1024, 3},
+					{96 * 1024, 1},
+					{8 * 1024, 2},
+					{16 * 1024, 1},
+				},
+				.block_erase = erase_block_82802ab,
+			},
+		},
+		.write		= write_82802ab,
+		.read		= read_memmapped,
+	},
+
+	{
+		.vendor		= "Intel",
+		.name		= "28F400BV/CV/CE-B",
+		.bustype	= CHIP_BUSTYPE_PARALLEL,
+		.manufacture_id	= INTEL_ID,
+		.model_id	= P28F400BB,
+		.total_size	= 512,
+		.page_size	= 128 * 1024, /* maximal block size */
+		.feature_bits	= FEATURE_ADDR_SHIFTED,
+		.tested		= TEST_UNTESTED,
+		.probe		= probe_82802ab,
+		.probe_timing	= TIMING_ZERO,	/* Datasheet has no timing info specified */
+		.block_erasers	=
+		{
+			{
+				.eraseblocks = { 
+					{16 * 1024, 1},
+					{8 * 1024, 2},
+					{96 * 1024, 1},
+					{128 * 1024, 3},
+				},
+				.block_erase = erase_block_82802ab,
+			},
+		},
+		.write		= write_82802ab,
+		.read		= read_memmapped,
+	},
+
+	{
+		.vendor		= "Intel",
+		.name		= "28F400BV/CV/CE-B",
+		.bustype	= CHIP_BUSTYPE_PARALLEL,
+		.manufacture_id	= INTEL_ID,
+		.model_id	= P28F400BT,
+		.total_size	= 512,
+		.page_size	= 128 * 1024, /* maximal block size */
+		.feature_bits	= FEATURE_ADDR_SHIFTED,
+		.tested		= TEST_UNTESTED,
+		.probe		= probe_82802ab,
+		.probe_timing	= TIMING_ZERO,	/* Datasheet has no timing info specified */
+		.block_erasers	=
+		{
+			{
+				.eraseblocks = { 
+					{128 * 1024, 3},
+					{96 * 1024, 1},
+					{8 * 1024, 2},
+					{16 * 1024, 1},
+				},
+				.block_erase = erase_block_82802ab,
+			},
+		},
+		.write		= write_82802ab,
+		.read		= read_memmapped,
+	},
+
+	{
+		.vendor		= "Intel",
 		.name		= "82802AB",
 		.bustype	= CHIP_BUSTYPE_FWH,
 		.manufacture_id	= INTEL_ID,
diff --git a/flashchips.h b/flashchips.h
index 4337e52..b6c2e8b 100644
--- a/flashchips.h
+++ b/flashchips.h
@@ -259,6 +259,10 @@
 #define E_28F016S5		0xAA
 #define P28F001BXT		0x94	/* 28F001BX-T */
 #define P28F001BXB		0x95	/* 28F001BX-B */
+#define P28F004BT		0x78	/* 28F004BV/BE-T */
+#define P28F004BB		0x79	/* 28F004BV/BE-B */
+#define P28F400BT		0x70	/* 28F400BV/CV/CE-T */
+#define P28F400BB		0x71	/* 28F400BV/CV/CE-B */
 #define SHARP_LH28F008SA	0xA2	/* Sharp chip, Intel Vendor ID */
 #define SHARP_LH28F008SC	0xA6	/* Sharp chip, Intel Vendor ID */
 
-- 
1.7.0