Re: [flashrom] Migrate towards common JEDEC functions

[Michael Karcher]

Am Donnerstag, den 26.11.2009, 18:43 +0100 schrieb Carl-Daniel
Hailfinger:
> You forgot your name in the signoff. The common format is
> Signed-off-by: Firstname Lastname 
> and the email address has to be in <> angle brackets. Our commit hooks
> check for that.

OK, got it now.
> > -/* The EN29F002 chip needs repeated single byte writing, no block writing. */
> > -int write_en29f002a(struct flashchip *flash, uint8_t *buf)
>
> Did you know that replacing this with standard JEDEC is a behaviour
> change? Standard JEDEC ignores write of 0xff, this function does write
> 0xff.

No, didn't think of it.
> If any of the chips using this function have TEST_OK_PREW, please
> change it to TEST_OK_PRE. We want reports that the new version works as
> well. Oh, and please note this in the changelog.

Of course, I will do that.
> > int write_pm29f002(struct flashchip *flash, uint8_t *buf)
> >  {
> >    int i, total_size = flash->total_size * 1024;
> >    chipaddr bios = flash->virtual_memory;
> >    chipaddr dst = bios;
> > /* Pm29F002T/B use the same erase method... */
> > /* Pm29F002T/B use the same erase method...
> >   FIXME: use erase_chip_jedec? */
> > if (erase_29f040b(flash)) {
>
> You could run erase_flash(flash) here. erase_flash calls the standard
> erase function for the chip, and this is hopefully erase_29f040b.

I think calling erase_flash instead of erase_chip_jedec in write_jedec_1
is the most generic solution.
> > ===================================================================
> > --- am29f040b.c	(Revision 784)
> > +++ am29f040b.c	(Arbeitskopie)
> > @@ -20,6 +20,8 @@
> >
> > #include "flash.h"
> >
> > +/* FIMXE: check that the 2 second delay is really needed.
> >      Use erase_sector_jedec if not? */
>
> If the only difference is the delay, you could make this a wrapper for
> erase_sector_jedec with a delay at the end.

Not possible. The delay is between command and starting to poll the
toggle bit. Also, the addresses are cut, that's why there is the
question mark in the second line.
> > +/* FIXME: Use erase_chip_jedec?
> > (does not send 0xF0 (exit ID mode) and uses 0x5555/0x2AAA adresses) */
>
> Please reword that comment a bit. If the addresses are normal, don't
> mention them to avoid confusion.

I wrote what erase_chip_jedec does instead of this function. Of course
the addresses erase_chip_jedec uses are normal.
> > Index: m29f400bt.c
> >
> > --- m29f400bt.c	(Revision 784)
> > +++ m29f400bt.c	(Arbeitskopie)
>
> This file needs special care. It uses 0xAAA,0x555,0xAAA instead of the
> common 0x555,0xAAA,0x555 sequence. Please add that info to the various
> fixme comments in the file to avoid conversion by accident.

Excellent point! I completely overlooked that.
> Overall, I think this needs one more iteration and then it can be
> committed. Feel free to include Sean's Ack in your next patch. That way,
> he doesn't have to ack again.

I hope the next iteration is good.

Regards,
  Michael Karcher