Re: [LinuxBIOS] [PATCH] flashrom: Support EON EN29F002AT, JEDEC continuation IDs

[Carl-Daniel Hailfinger]

On 30.12.2007 05:40, Corey Osgood wrote:
> >   /* Check if it is a continuation ID, this should be a while
> >
> > loop. */
> >   if (id1 == 0x7F) {
> >           largeid1 <<= 8;
> >           id1 = *(volatile uint8_t *)(bios + 0x100);
> >           largeid1 |= id1;
> >   }
> >   if (id2 == 0x7F) {
> >           largeid2 <<= 8;
> >           id2 = *(volatile uint8_t *)(bios + 0x101);
> >           largeid2 |= id2;
> >   }
>
> are you sure this is right? The IDs are, at most, just 0x7f7f7fXX? Just

Actually, the code above does not go further than checking for IDs of
the type 0x7fXX, but does this for vendor and product ID. The current
published JEDEC spec has a list where the largest vendor ID is 7 bytes
long, but all leading bytes are 0x7f. The list will grow in the future,
and using a 64bit variable will not be enough anymore.
Suggestion for a new encoding:
Use a two-byte data type for the ID, the lower byte contains the only
non-0x7f byte, the upper byte contains the number of 0x7f bytes used as
prefix (which is the "page" number the vendor ID appears in.
> doesn't seem quite right, but it might be (yes, I realize it means 4x
> more IDs). We also break the ability to use IMT flash chips (which
> someone may, eventually...). The other things is, if it should be a
> while loop, why isn't it?

Oh, I had not seen the IMT entry in flash.h. It is obviously wrong. With
the current code, at least EON and IMT will collide, and neither have a
real vendor ID of 0x7f. I'll dig out the real IMT vendor ID.
> You also forgot your email in the copyright header in flashchips.c.

That is intentional.

Thanks for your review!

Regards,
Carl-Daniel