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

[Uwe Hermann]

See patch for a first attempt to support the Macronix MX28F2000P.

Some TODOs remain in the code, and it will be tested soon by Mete0 (IRC).
However, the chip requires 12V for programming/erasing, we'll see if
that'll work out of the box or maybe requires some board-enable.

Also, there's a "DMI chassis-type is not specific enough.", thus the
laptop=force_I_want_a_brick options is currently needed.

http://paste.flashrom.org/view.php?id=789
http://paste.flashrom.org/view.php?id=787

Uwe.

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

[Uwe Hermann]

On Fri, Sep 02, 2011 at 10:51:43PM +0200, Uwe Hermann wrote:
> See patch for a first attempt to support the Macronix MX28F2000P.

Updated patch which now works.

Thanks to Carl-Daniel for pointing out that you need two 0x90 commands,
one per ID byte you want to read. Thanks Meteo0 on IRC for testing the
patch on his hardware, and verifying that the board always provides 12V
on the VPP pin (which is why write/erase work).

The patch now does the work without major hacks. Unneeded delays are
removed, incorrect comments too.

Uwe.

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

[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.
> .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?
> /* 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.
> {
>    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.
> 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

[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