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