[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