Re: [flashrom] [PATCH] Add support for the Macronix MX28F2000P chip (256KB parallel)

[Carl-Daniel Hailfinger]

A few clarifications...

Am 12.09.2011 19:08 schrieb Carl-Daniel Hailfinger:
> Hi Uwe,
>
> Am 11.09.2011 15:15 schrieb Uwe Hermann:
> > Add support for the Macronix MX28F2000P chip (256KB parallel).
> >
> > This 256KB chip has a similar, but not identical, command set to the
> > SST SST28SF040A chip. It features a chip erase, block erase, byte write,
> > chip ID, and reset command.
> >
> > In contrast to the SST28SF040A, the reset command is 0xff, 0xff (instead of
> > only one 0xff), and the write command is 0x40 (instead of 0x10).
> > No locking/unlocking (protect/unprotect) commands are documented, it seems.
> >
> > Finally, unlike the SST28SF040A, the MX28F2000P requires 12V for write and
> > erase operations. Only read/ID is supported with the usual 5V. On the tested
> > board the 12V are always supplied on the VPP pin (confirmed via multimeter)
> > though, so that's not a problem there.
> >
> > All operations were successfully tested, thus mark the chip as OK.
> >
> > Write log:
> > http://paste.flashrom.org/view.php?id=804
>
> Thanks for your patch.
> > Signed-off-by: Uwe Hermann 
> > Tested-by: Max Varentsov  (Meteo0 on IRC)
>
> The coding style is very different from what we use elsewhere. Please
> fix that. See below.
> > Index: flashchips.c
> >
> > --- flashchips.c	(Revision 1435)
> > +++ flashchips.c	(Arbeitskopie)
> > @@ -4458,6 +4458,38 @@
> >
> > {
> >    	.vendor		= "Macronix",
> > .name		= "MX28F2000P",
> > .bustype	= BUS_PARALLEL,
> > .manufacture_id	= MACRONIX_ID,
> > .model_id	= MACRONIX_MX28F2000P,
> > .total_size	= 256,
> > .page_size	= 0, /* unused */
>
> Hm yes, reminds me that we should kill page_size completely.

That comment was more a note-to-self, nothing you have to change.
> > .feature_bits	= 0,
> > .tested		= TEST_OK_PREW,
> > .probe		= probe_mx28f2000p,
> > .probe_timing	= TIMING_ZERO,
> > .block_erasers	=
> > {
> > 	{
> > 		.eraseblocks = {
> > 			{4 * 1024, 4},
> > 			{16 * 1024, 14},
> > 			{4 * 1024, 4},
> > 		},
> > 		.block_erase = erase_sector_mx28f2000p,
> > 	}, {
> > 		.eraseblocks = { {256 * 1024, 1} },
> > 		.block_erase = erase_chip_28sf040,
> > 	}
> > },
> > .unlock		= NULL, /* No locking mechanism available. */
> > .write		= write_mx28f2000p,
> > .read		= read_memmapped,
> > .voltage	= {4500, 5500}, /* Needs 12V for write/erase! */
> > },
> > {
> > .vendor		= "Macronix",
> > .name		= "MX29F001B",
> > .bustype	= BUS_PARALLEL,
> > .manufacture_id	= MACRONIX_ID,
> > Index: flashchips.h
> >
> > --- flashchips.h	(Revision 1435)
> > +++ flashchips.h	(Arbeitskopie)
> > @@ -368,6 +368,7 @@
> >  #define MACRONIX_MX25L1635D	0x2415
> >  #define MACRONIX_MX25L1635E	0x2515	/* MX25L1635{E} */
> >  #define MACRONIX_MX25L3235D	0x5E16	/* MX25L3225D/MX25L3235D/MX25L3237D */
> > +#define MACRONIX_MX28F2000P	0x2a
> >  #define MACRONIX_MX29F001B	0x19
> >  #define MACRONIX_MX29F001T	0x18
> >  #define MACRONIX_MX29F002B	0x34	/* Same as MX29F002NB; N has reset pin n/c. */
> > Index: sst28sf040.c
> > ===================================================================
> > --- sst28sf040.c	(Revision 1435)
> > +++ sst28sf040.c	(Arbeitskopie)
> > @@ -26,6 +26,7 @@
> >  #define AUTO_PG_ERASE1		0x20
> >  #define AUTO_PG_ERASE2		0xD0
> >  #define AUTO_PGRM		0x10
> > +#define AUTO_PGRM2		0x40 /* Used on MX28F2000P */
> >  #define CHIP_ERASE		0x30
> >  #define RESET			0xFF
> >  #define READ_ID			0x90
> > @@ -60,7 +61,8 @@
> >    return 0;
> >  }
> >
> > -int erase_sector_28sf040(struct flashchip *flash, unsigned int address, unsigned int sector_size)
> > +static int erase_sector_28sf040_generic(struct flashchip *flash,
> > unsigned int address, unsigned int sector_size, int toggle_delay)
> >
> > {
> >    chipaddr bios = flash->virtual_memory;
> >
> > @@ -68,38 +70,65 @@
> >    chip_writeb(AUTO_PG_ERASE1, bios);
> >    chip_writeb(AUTO_PG_ERASE2, bios + address);
> > /* wait for Toggle bit ready */
> > programmer_delay(toggle_delay);
> > /* Wait for toggle bit ready. */
> > toggle_ready_jedec(bios);
>
> Can't you achieve the same effect with toggle_ready_jedec_slow?
>
> Besides that, the whole function looks very similar to
> erase_block_82802ab(). Can we just kill one of them?

Would be nice to look at that refactoring possibility, but not a hard
requirement for an ack.
> > /* FIXME: Check the status register for errors. */
> >    return 0;
> >  }
> >
> > +int erase_sector_28sf040(struct flashchip *flash, unsigned int address,
> > 	 unsigned int sector_size)
> >
> > +{
> > return erase_sector_28sf040_generic(flash, address, sector_size, 0);
> >
> > +}
> >
> > +int erase_sector_mx28f2000p(struct flashchip *flash, unsigned int address,
> > 	    unsigned int sector_size)
> >
> > +{
> > /* Needs 200us delay before the toggle bit checking begins. */
> > return erase_sector_28sf040_generic(flash, address, sector_size, 200);
> >
> > +}
> >
> > /* chunksize is 1 */
> > -int write_28sf040(struct flashchip *flash, uint8_t *src, int start, int len)
> > +static int write_28sf040_generic(struct flashchip *flash, uint8_t *src,
> > 		 int start, int len, uint8_t auto_pgrm)
>
> NACK.
> We don't have any erase/write function in flashrom which takes a command
> as parameter, and I'd like to keep it that way.
> If you really insist on command-as-parameter, we have to rewrite every
> single erase/write function in flashrom and while that may be fun for
> some people (after all, you can generate a huge patch with a few lines
> of sed/perl/...), it would increase our patch queue significantly.

Apparently the comment above was unclear. The NACK only refers to the
auto_pgrm parameter. Kill it and duplicate the function instead so you
have one function with hardcoded AUTO_PGRM or AUTO_PGRM2 each.
> > {
> >    int i;
> >    chipaddr bios = flash->virtual_memory;
> >    chipaddr dst = flash->virtual_memory + start;
> >
> > for (i = 0; i < len; i++) {
> > /* transfer data from source to destination */
> > /* Transfer data from source to destination. */
> > if (*src == 0xFF) {
> > 	dst++, src++;
> > 	/* If the data is 0xFF, don't program it */
> > 	/* If the data is 0xFF, don't program it. */
> > continue;
> > }
> > /*issue AUTO PROGRAM command */
> > chip_writeb(AUTO_PGRM, dst);
> > /* Issue auto program command. */
> > chip_writeb(auto_pgrm, dst);
> > chip_writeb(*src++, dst++);
> > /* wait for Toggle bit ready */
> > /* Wait for Toggle bit ready. */
> >
> > toggle_ready_jedec(bios);
> > }
> >
> > return 0;
> >
> > }
> >
> > +int write_28sf040(struct flashchip *flash, uint8_t *src, int start, int len)
> > +{
> > return write_28sf040_generic(flash, src, start, len, AUTO_PGRM);
> >
> > +}
> >
> > +int write_mx28f2000p(struct flashchip *flash, uint8_t *src, int start, int len)
> > +{
> > return write_28sf040_generic(flash, src, start, len, AUTO_PGRM2);
> >
> > +}
> >
> > static int erase_28sf040(struct flashchip *flash)
> >  {
> >    chipaddr bios = flash->virtual_memory;
> > @@ -123,3 +152,30 @@
> >    }
> >    return erase_28sf040(flash);
> >  }
> >
> > +int probe_mx28f2000p(struct flashchip *flash)
> > +{
> > chipaddr bios = flash->virtual_memory;
> > uint8_t id1, id2;
> > /* Reset to get a clean state. */
> > chip_writeb(0xff, bios);
> > chip_writeb(0xff, bios);
> > /* Get the vendor and device ID. One 0x90 command per byte needed! */
> > chip_writeb(0x90, bios);
> > id1 = chip_readb(bios);
> > chip_writeb(0x90, bios);
> > id2 = chip_readb(bios + 0x01);
>
> This needs the same "normal flash content" check as probe_jedec.

I hope this comment is clear. That check is how I figured out that the
earlier probe function was incorrect.
> > msg_cdbg("%s: id1 0x%02x, id2 0x%02x", __func__, id1, id2);
> > if (!oddparity(id1))
> > msg_cdbg(", id1 parity violation");
> > msg_cdbg("\n");
> > if (id1 != flash->manufacture_id || id2 != flash->model_id)
> > return 0;
> > return 1;
> >
> > +}

Regards,
Carl-Daniel