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 <uwe at hermann-uwe.de>
>> Tested-by: Max Varentsov <admin at phpcode.us> (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
--
http://www.hailfinger.org/