[flashrom] Support for MX29F001T/B

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Thu Aug 20 15:52:22 CEST 2009


Hi Marko,

On 20.08.2009 11:01, Marko Kraljevic wrote:
> Hi, I've added support for MX29F001T/B.

Nice!

> I'm not sure the proper way to submit a patch, I've just joined the
> mailing list recently.

The following guidelines are for coreboot, but most of them apply to
flashrom as well: http://www.coreboot.org/Development_Guidelines
The really important part is about the Signed-off-by procedure.


> I've only got the -T type, but I just tested it and it works with them.
> The attached files are the patch, the additional .c file (based on
> mx29f002.c, modified based on datasheet for mx29f001. The trial.txt is
> output of probe, read, erase, write on the MX29F001T.

We try to reuse as much code as possible and create new files only if
absolutely needed, so if you find a function somewhere in the tree which
already does what you want (even if it is for a totally different chip),
please use it.


> I compiled and tested this all on an old Soyo SY-6BB [440BX, PIIX4];
> It still boots after a read, erase, blank check, write... So it must
> have worked ;)

Looks good. And for a first patch, you managed to not only write the
code, but also test it. Some hints about proper submission procedure are
mentioned above.

Instead of a full review, I just wrote all this stuff in a special wiki
page so others can benefit from this as well:
http://www.coreboot.org/Flashrom/Random_notes . Please read it and
comment if something is unclear.

Now, on to the code you sent. The review may sound harsh, but please
don't get discouraged. We try to merge patches after one or two iterations.

First, the probe function. If we take into account what the random notes
say about ignored address bits, it makes sense to check the log if any
other probe functions worked as well. Simply grep the log for 0xc2 and
look which functions are listed. I've used the following command line:

# grep 0xc2 flashromlog.txt|sed "s/.*probe/probe/"|sort|uniq
probe_29f001: id1 0xc2, id2 0x18
probe_29f002: id1 0xc2, id2 0x18
probe_29f040b: id1 0xc2, id2 0x18
probe_jedec: id1 0xc2, id2 0x18
probe_stm50flw0x0x: id1 0xc2, id2 0x18
probe_w39v040c: id1 0xc2, id2 0x18
probe_winbond_fwhub: id1 0xc2, id2 0x18

As you can see, there are quite a lot of probe functions which seem to
work fine (and that's mostly because of the ignored address bits).
probe_jedec is the most-used function in our tree, so if the sequence
looks ok, please use that one.

The reset command you've added at the beginning of erase_29f001() makes
sense, but AFAICS it would make sense in the original erase_29f002()
function as well, so please change that one and use it afterwards. The
resent command I'm talking about is
 chip_writeb(0xF0, bios + 0x555);

Last but not least, write_29f001() could maybe be replaced by
write_29f002() as well if the chip ignores the high address bits for
these commands as well.

Rest of the review is in the Random notes wiki page.

If this was a bit confusing, feel free to ask.

Regards,
Carl-Daniel

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





More information about the flashrom mailing list