[flashrom] [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