[flashrom] [PATCH] Add chip definitions for Mosel Vitelic
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Fri Jul 9 17:57:04 CEST 2010
Hi Mattias,
thanks for your patch.
Review follows.
On 09.07.2010 11:36, Mattias Mattsson wrote:
> Add definitions for the following chips from Mosel Vitelic Corporation:
>
> V29C31004B
> V29C31004T
> V29C51000B
> V29C51000T
> V29C51001B
> V29C51001T
> V29C51002B
> V29C51002T
> V29C51004B
> V29C51004T
> V29C51400B
> V29C51400T
> V29LC51000
> V29LC51001
> V29LC51002
>
> This is one of my first patches ever so I am not at all certain it is
> correct.
>
> Datasheets here:
> http://www.ezoflash.com/datasheets/flash/Mosel_Vitelic/V29C31004.pdf
> http://www.ezoflash.com/datasheets/flash/Mosel_Vitelic/V29C51000.pdf
> http://www.ezoflash.com/datasheets/flash/Mosel_Vitelic/V29C51001.pdf
> http://www.ezoflash.com/datasheets/flash/Mosel_Vitelic/V29C51002.pdf
> http://www.ezoflash.com/datasheets/flash/Mosel_Vitelic/V29C51004.pdf
> http://www.ezoflash.com/datasheets/flash/Mosel_Vitelic/V29C51400.pdf
> http://www.ezoflash.com/datasheets/flash/Mosel_Vitelic/V29LC51000.pdf
> http://www.ezoflash.com/datasheets/flash/Mosel_Vitelic/V29LC51001.pdf
> http://www.ezoflash.com/datasheets/flash/Mosel_Vitelic/V29LC51002.pdf
>
> Signed-off-by: Mattias Mattsson <vitplister at gmail.com>
>
OK, you have a good changelog and the formal requirements are met.
> Index: flashrom/flashchips.c
> ===================================================================
> --- flashrom.orig/flashchips.c 2010-07-09 11:07:44.000000000 +0200
> +++ flashrom/flashchips.c 2010-07-09 11:08:01.000000000 +0200
> @@ -3166,6 +3166,422 @@
> },
>
> {
> + .vendor = "Mosel Vitelic",
> + .name = "V29C31004B",
> + .bustype = CHIP_BUSTYPE_PARALLEL,
> + .manufacture_id = MVC_ID,
> + .model_id = MVC_V29C31004B,
> + .total_size = 512,
> + .page_size = 1024,
> + .feature_bits = FEATURE_BYTEWRITES | FEATURE_EITHER_RESET,
>
This is not your fault, but FEATURE_BYTEWRITES is mostly a dead piece of
information. flashrom doesn't use it right now, and once we have byte
write granularity, it will probably be stored differently. I have to
admit that you did read the source code closely. Could you drop
FEATURE_BYTEWRITES for now? Thanks.
> + .tested = TEST_UNTESTED,
> + .probe = probe_jedec,
> + .probe_timing = TIMING_ZERO,
> + .block_erasers =
> + {
> + {
> + .eraseblocks = { {1 * 1024, 512}, },
>
Could you remove the "1*" and the trailing inner comma and change this to
.eraseblocks = { {1024, 512} },
I'll explain why. The idea is to have readable quantities. If something
is bigger than 1024 and a multiple of 1024, write it as a product (with
the second factor being 1024). If it is 1024 or smaller, write it
directly. That allows readers to see e.g. "hey, this chip has 128 kB
erase blocks" which is a bit more readable than "hey, this chip has
131072 B erase blocks... is that even a power of two".
> + .block_erase = erase_sector_jedec,
> + }, {
> + .eraseblocks = { {512 * 1024, 1} },
> + .block_erase = erase_chip_block_jedec,
> + },
> + },
> + .write = write_jedec_1,
> + .read = read_memmapped,
> + },
> +
> + {
> + .vendor = "Mosel Vitelic",
> + .name = "V29C31004T",
> + .bustype = CHIP_BUSTYPE_PARALLEL,
> + .manufacture_id = MVC_ID,
> + .model_id = MVC_V29C31004T,
> + .total_size = 512,
> + .page_size = 1024,
> + .feature_bits = FEATURE_BYTEWRITES | FEATURE_EITHER_RESET,
>
Kill FEATURE_BYTEWRITES...
> + .tested = TEST_UNTESTED,
> + .probe = probe_jedec,
> + .probe_timing = TIMING_ZERO,
> + .block_erasers =
> + {
> + {
> + .eraseblocks = { {1 * 1024, 512}, },
>
Same here.
> + .block_erase = erase_sector_jedec,
> + }, {
> + .eraseblocks = { {512 * 1024, 1} },
> + .block_erase = erase_chip_block_jedec,
> + },
> + },
> + .write = write_jedec_1,
> + .read = read_memmapped,
> + },
> +
> + {
> + .vendor = "Mosel Vitelic",
> + .name = "V29C51000B",
> + .bustype = CHIP_BUSTYPE_PARALLEL,
> + .manufacture_id = MVC_ID,
> + .model_id = MVC_V29C51000B,
> + .total_size = 64,
> + .page_size = 512,
> + .feature_bits = FEATURE_BYTEWRITES | FEATURE_EITHER_RESET,
>
...
> + .tested = TEST_UNTESTED,
> + .probe = probe_jedec,
> + .probe_timing = TIMING_ZERO,
> + .block_erasers =
> + {
> + {
> + .eraseblocks = { {1 * 512, 128}, },
>
.eraseblocks = { {512, 128} },
> + .block_erase = erase_sector_jedec,
> + }, {
> + .eraseblocks = { {128 * 512, 1} },
>
.eraseblocks = { {64 * 1024, 1} },
> + .block_erase = erase_chip_block_jedec,
> + },
> + },
> + .write = write_jedec_1,
> + .read = read_memmapped,
> + },
> +
> + {
> + .vendor = "Mosel Vitelic",
> + .name = "V29C51000T",
> + .bustype = CHIP_BUSTYPE_PARALLEL,
> + .manufacture_id = MVC_ID,
> + .model_id = MVC_V29C51000T,
> + .total_size = 64,
> + .page_size = 512,
> + .feature_bits = FEATURE_BYTEWRITES | FEATURE_EITHER_RESET,
>
...
> + .tested = TEST_UNTESTED,
> + .probe = probe_jedec,
> + .probe_timing = TIMING_ZERO,
> + .block_erasers =
> + {
> + {
> + .eraseblocks = { {1 * 512, 128}, },
>
Same here.
> + .block_erase = erase_sector_jedec,
> + }, {
> + .eraseblocks = { {128 * 512, 1} },
>
{ {64 * 1024}, 1} },
> + .block_erase = erase_chip_block_jedec,
> + },
> + },
> + .write = write_jedec_1,
> + .read = read_memmapped,
> + },
> +
> + {
> + .vendor = "Mosel Vitelic",
> + .name = "V29C51001B",
> + .bustype = CHIP_BUSTYPE_PARALLEL,
> + .manufacture_id = MVC_ID,
> + .model_id = MVC_V29C51001B,
> + .total_size = 128,
> + .page_size = 512,
> + .feature_bits = FEATURE_BYTEWRITES | FEATURE_EITHER_RESET,
>
...
> + .tested = TEST_UNTESTED,
> + .probe = probe_jedec,
> + .probe_timing = TIMING_ZERO,
> + .block_erasers =
> + {
> + {
> + .eraseblocks = { {1 * 512, 256}, },
>
...
> + .block_erase = erase_sector_jedec,
> + }, {
> + .eraseblocks = { {256 * 512, 1} },
>
...
> + .block_erase = erase_chip_block_jedec,
> + },
> + },
> + .write = write_jedec_1,
> + .read = read_memmapped,
> + },
> +
> + {
> + .vendor = "Mosel Vitelic",
> + .name = "V29C51001T",
> + .bustype = CHIP_BUSTYPE_PARALLEL,
> + .manufacture_id = MVC_ID,
> + .model_id = MVC_V29C51001T,
> + .total_size = 128,
> + .page_size = 512,
> + .feature_bits = FEATURE_BYTEWRITES | FEATURE_EITHER_RESET,
> + .tested = TEST_UNTESTED,
> + .probe = probe_jedec,
> + .probe_timing = TIMING_ZERO,
> + .block_erasers =
> + {
> + {
> + .eraseblocks = { {1 * 512, 256}, },
>
...
> + .block_erase = erase_sector_jedec,
> + }, {
> + .eraseblocks = { {256 * 512, 1} },
>
...
> + .block_erase = erase_chip_block_jedec,
> + },
> + },
> + .write = write_jedec_1,
> + .read = read_memmapped,
> + },
> +
> + {
> + .vendor = "Mosel Vitelic",
> + .name = "V29C51002B",
> + .bustype = CHIP_BUSTYPE_PARALLEL,
> + .manufacture_id = MVC_ID,
> + .model_id = MVC_V29C51002B,
> + .total_size = 256,
> + .page_size = 512,
> + .feature_bits = FEATURE_BYTEWRITES | FEATURE_EITHER_RESET,
> + .tested = TEST_UNTESTED,
> + .probe = probe_jedec,
> + .probe_timing = TIMING_ZERO,
> + .block_erasers =
> + {
> + {
> + .eraseblocks = { {1 * 512, 512}, },
> + .block_erase = erase_sector_jedec,
> + }, {
> + .eraseblocks = { {512 * 512, 1} },
> + .block_erase = erase_chip_block_jedec,
> + },
> + },
> + .write = write_jedec_1,
> + .read = read_memmapped,
> + },
> +
> + {
> + .vendor = "Mosel Vitelic",
> + .name = "V29C51002T",
> + .bustype = CHIP_BUSTYPE_PARALLEL,
> + .manufacture_id = MVC_ID,
> + .model_id = MVC_V29C51002T,
> + .total_size = 256,
> + .page_size = 512,
> + .feature_bits = FEATURE_BYTEWRITES | FEATURE_EITHER_RESET,
> + .tested = TEST_UNTESTED,
> + .probe = probe_jedec,
> + .probe_timing = TIMING_ZERO,
> + .block_erasers =
> + {
> + {
> + .eraseblocks = { {1 * 512, 512}, },
> + .block_erase = erase_sector_jedec,
> + }, {
> + .eraseblocks = { {512 * 512, 1} },
> + .block_erase = erase_chip_block_jedec,
> + },
> + },
> + .write = write_jedec_1,
> + .read = read_memmapped,
> + },
> +
> + {
> + .vendor = "Mosel Vitelic",
> + .name = "V29C51004B",
> + .bustype = CHIP_BUSTYPE_PARALLEL,
> + .manufacture_id = MVC_ID,
> + .model_id = MVC_V29C51004B,
> + .total_size = 512,
> + .page_size = 1024,
> + .feature_bits = FEATURE_BYTEWRITES | FEATURE_EITHER_RESET,
> + .tested = TEST_UNTESTED,
> + .probe = probe_jedec,
> + .probe_timing = TIMING_ZERO,
> + .block_erasers =
> + {
> + {
> + .eraseblocks = { {1 * 1024, 512}, },
> + .block_erase = erase_sector_jedec,
> + }, {
> + .eraseblocks = { {512 * 1024, 1} },
> + .block_erase = erase_chip_block_jedec,
> + },
> + },
> + .write = write_jedec_1,
> + .read = read_memmapped,
> + },
> +
> + {
> + .vendor = "Mosel Vitelic",
> + .name = "V29C51004T",
> + .bustype = CHIP_BUSTYPE_PARALLEL,
> + .manufacture_id = MVC_ID,
> + .model_id = MVC_V29C51004T,
> + .total_size = 512,
> + .page_size = 1024,
> + .feature_bits = FEATURE_BYTEWRITES | FEATURE_EITHER_RESET,
> + .tested = TEST_UNTESTED,
> + .probe = probe_jedec,
> + .probe_timing = TIMING_ZERO,
> + .block_erasers =
> + {
> + {
> + .eraseblocks = { {1 * 1024, 512}, },
> + .block_erase = erase_sector_jedec,
> + }, {
> + .eraseblocks = { {512 * 1024, 1} },
> + .block_erase = erase_chip_block_jedec,
> + },
> + },
> + .write = write_jedec_1,
> + .read = read_memmapped,
> + },
> +
> + {
> + .vendor = "Mosel Vitelic",
> + .name = "V29C51400B",
> + .bustype = CHIP_BUSTYPE_PARALLEL,
> + .manufacture_id = MVC_ID,
> + .model_id = MVC_V29C51400B,
> + .total_size = 512,
> + .page_size = 1024,
> + .feature_bits = FEATURE_BYTEWRITES | FEATURE_EITHER_RESET,
> + .tested = TEST_UNTESTED,
> + .probe = probe_jedec,
> + .probe_timing = TIMING_ZERO,
> + .block_erasers =
> + {
> + {
> + .eraseblocks = { {1 * 1024, 512}, },
> + .block_erase = erase_sector_jedec,
> + }, {
> + .eraseblocks = { {512 * 1024, 1} },
> + .block_erase = erase_chip_block_jedec,
> + },
> + },
> + .write = write_jedec_1,
> + .read = read_memmapped,
> + },
> +
> + {
> + .vendor = "Mosel Vitelic",
> + .name = "V29C51400T",
> + .bustype = CHIP_BUSTYPE_PARALLEL,
> + .manufacture_id = MVC_ID,
> + .model_id = MVC_V29C51400T,
> + .total_size = 512,
> + .page_size = 1024,
> + .feature_bits = FEATURE_BYTEWRITES | FEATURE_EITHER_RESET,
> + .tested = TEST_UNTESTED,
> + .probe = probe_jedec,
> + .probe_timing = TIMING_ZERO,
> + .block_erasers =
> + {
> + {
> + .eraseblocks = { {1 * 1024, 512}, },
> + .block_erase = erase_sector_jedec,
> + }, {
> + .eraseblocks = { {512 * 1024, 1} },
> + .block_erase = erase_chip_block_jedec,
> + },
> + },
> + .write = write_jedec_1,
> + .read = read_memmapped,
> + },
> +
> + {
> + .vendor = "Mosel Vitelic",
> + .name = "V29C51400T",
> + .bustype = CHIP_BUSTYPE_PARALLEL,
> + .manufacture_id = MVC_ID,
> + .model_id = MVC_V29C51400T,
> + .total_size = 512,
> + .page_size = 1024,
> + .feature_bits = FEATURE_BYTEWRITES | FEATURE_EITHER_RESET,
> + .tested = TEST_UNTESTED,
> + .probe = probe_jedec,
> + .probe_timing = TIMING_ZERO,
> + .block_erasers =
> + {
> + {
> + .eraseblocks = { {1 * 1024, 512}, },
> + .block_erase = erase_sector_jedec,
> + }, {
> + .eraseblocks = { {512 * 1024, 1} },
> + .block_erase = erase_chip_block_jedec,
> + },
> + },
> + .write = write_jedec_1,
> + .read = read_memmapped,
> + },
> +
> + {
> + .vendor = "Mosel Vitelic",
> + .name = "V29LC51000",
> + .bustype = CHIP_BUSTYPE_PARALLEL,
> + .manufacture_id = MVC_ID,
> + .model_id = MVC_V29LC51000,
> + .total_size = 64,
> + .page_size = 512,
> + .feature_bits = FEATURE_BYTEWRITES | FEATURE_EITHER_RESET,
> + .tested = TEST_UNTESTED,
> + .probe = probe_jedec,
> + .probe_timing = TIMING_ZERO,
> + .block_erasers =
> + {
> + {
> + .eraseblocks = { {1 * 512, 128}, },
> + .block_erase = erase_sector_jedec,
> + }, {
> + .eraseblocks = { {128 * 512, 1} },
> + .block_erase = erase_chip_block_jedec,
> + },
> + },
> + .write = write_jedec_1,
> + .read = read_memmapped,
> + },
> +
> + {
> + .vendor = "Mosel Vitelic",
> + .name = "V29LC51001",
> + .bustype = CHIP_BUSTYPE_PARALLEL,
> + .manufacture_id = MVC_ID,
> + .model_id = MVC_V29LC51001,
> + .total_size = 128,
> + .page_size = 512,
> + .feature_bits = FEATURE_BYTEWRITES | FEATURE_EITHER_RESET,
> + .tested = TEST_UNTESTED,
> + .probe = probe_jedec,
> + .probe_timing = TIMING_ZERO,
> + .block_erasers =
> + {
> + {
> + .eraseblocks = { {1 * 512, 256}, },
> + .block_erase = erase_sector_jedec,
> + }, {
> + .eraseblocks = { {256 * 512, 1} },
> + .block_erase = erase_chip_block_jedec,
> + },
> + },
> + .write = write_jedec_1,
> + .read = read_memmapped,
> + },
> +
> + {
> + .vendor = "Mosel Vitelic",
> + .name = "V29LC51002",
> + .bustype = CHIP_BUSTYPE_PARALLEL,
> + .manufacture_id = MVC_ID,
> + .model_id = MVC_V29LC51002,
> + .total_size = 256,
> + .page_size = 512,
> + .feature_bits = FEATURE_BYTEWRITES | FEATURE_EITHER_RESET,
> + .tested = TEST_UNTESTED,
> + .probe = probe_jedec,
> + .probe_timing = TIMING_ZERO,
> + .block_erasers =
> + {
> + {
> + .eraseblocks = { {1 * 512, 512}, },
> + .block_erase = erase_sector_jedec,
> + }, {
> + .eraseblocks = { {512 * 512, 1} },
> + .block_erase = erase_chip_block_jedec,
> + },
> + },
> + .write = write_jedec_1,
> + .read = read_memmapped,
> + },
> +
> + {
> .vendor = "Numonyx",
> .name = "M25PE10",
> .bustype = CHIP_BUSTYPE_SPI,
>
My comments apply to all the chips above.
For the chips where we already have technically identical entries, we
should probably cross-check those and make sure only one remains.
> Index: flashrom/flashchips.h
> ===================================================================
> --- flashrom.orig/flashchips.h 2010-07-09 11:07:44.000000000 +0200
> +++ flashrom/flashchips.h 2010-07-09 11:08:01.000000000 +0200
> @@ -329,6 +329,23 @@
> #define MX_29SL800CB 0x6B /* Same as MX29SL802CB */
> #define MX_29SL800CT 0xEA /* Same as MX29SL802CT */
>
> +#define MVC_ID 0x40 /* Mosel Vitelic Corp. / SyncMOS */
>
Can you move this to the SyncMOS section?
> +#define MVC_V29C31004B 0x73
> +#define MVC_V29C31004T 0x63 /* Same as S29C31004T */
> +#define MVC_V29C51000B 0xA0
> +#define MVC_V29C51000T 0x00
> +#define MVC_V29C51001B 0xA1
> +#define MVC_V29C51001T 0x01 /* Same as S29C51001T */
> +#define MVC_V29C51002B 0xA2
> +#define MVC_V29C51002T 0x02 /* Same as S29C51002T */
> +#define MVC_V29C51004B 0xA3
> +#define MVC_V29C51004T 0x03 /* Same as S29C51004T */
> +#define MVC_V29C51400B 0xB3
> +#define MVC_V29C51400T 0x13
> +#define MVC_V29LC51000 0x20
> +#define MVC_V29LC51001 0x60
> +#define MVC_V29LC51002 0x82
>
I'm pretty sure that every MVC chip also has a SyncMOS equivalent.
We need a better way to express this than comments in flashchips.h. I
suggest an .alias_name field in struct flashchip. The comments are
useful, though.
> +
> /*
> * Programmable Micro Corp is listed in JEP106W in bank 2, so it should
> * have a 0x7F continuation code prefix.
>
Overall, the patch looks good. You just had the bad luck that you hit a
few places where our policy is not 100% clear.
Please wait a bit with the new patch until we have solved the alias name
issue.
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
More information about the flashrom
mailing list