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

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Thu Apr 1 12:56:10 CEST 2010


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/





More information about the flashrom mailing list