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 <[email protected]> > 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/ _______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
