On 29.11.2008 13:23, FENG Yu Ning wrote:
On Sat, Nov 29, 2008 at 9:15 AM, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
This is a patch which stores eraseblock sizes in struct flashchip. I decided to fill in the info for a few chips to illustrate how this works both for uniform and non-uniform sector sizes.
This is to unify different kinds of things.
Yes.
Index: flashrom-eraseblocks/flashchips.c
...
{"AMIC", "A29002B", AMIC_ID_NOPREFIX, AMIC_A29002B, 256, 64 * 1024, {{16384,1},{8192,2},{32768,1},{65536,3}},TEST_UNTESTED, probe_29f002, erase_29f002, NULL, write_29f002},{"AMIC", "A29002T", AMIC_ID_NOPREFIX, AMIC_A29002T, 256, 64 * 1024, {{65536,3},{32768,1},{8192,2},{16384,1}},TEST_OK_PREW, probe_29f002, erase_29f002, NULL, write_29f002},...
{"EON", "EN29F002(A)(N)B", EON_ID, EN_29F002B, 256, 256, {{16384,1},{8192,2},{32768,1},{65536,3}},TEST_UNTESTED, probe_jedec, erase_chip_jedec, NULL, write_en29f002a},{"EON", "EN29F002(A)(N)T", EON_ID, EN_29F002T, 256, 256, {{65536,3},{32768,1},{8192,2},{16384,1}},TEST_OK_PREW, probe_jedec, erase_chip_jedec, NULL, write_en29f002a},{"Fujitsu", "MBM29F004BC", FUJITSU_ID, MBM29F004BC, 512, 64 * 1024, {{16384,1},{8192,2},{32768,1},{65536,7}},TEST_UNTESTED, probe_jedec, NULL, NULL, NULL},{"Fujitsu", "MBM29F004TC", FUJITSU_ID, MBM29F004TC, 512, 64 * 1024, {{65536,7},{32768,1},{8192,2},{16384,1}},TEST_UNTESTED, probe_jedec, NULL, NULL, NULL},{"Fujitsu", "MBM29F400BC", FUJITSU_ID, MBM29F400BC, 512, 64 * 1024, {{16384,1},{8192,2},{32768,1},{65536,7}},TEST_UNTESTED, probe_m29f400bt, erase_m29f400bt, NULL, write_coreboot_m29f400bt},{"Fujitsu", "MBM29F400TC", FUJITSU_ID, MBM29F400TC, 512, 64 * 1024, {{65536,7},{32768,1},{8192,2},{16384,1}},TEST_UNTESTED, probe_m29f400bt, erase_m29f400bt, NULL, write_coreboot_m29f400bt},...
{"Macronix", "MX25L512", MX_ID, MX_25L512, 64, 256, {{4096,16}}, TEST_UNTESTED, probe_spi_rdid, spi_chip_erase_c7, NULL, spi_chip_write, spi_chip_read},{"Macronix", "MX25L1005", MX_ID, MX_25L1005, 128, 256, {{4096,32}}, TEST_UNTESTED, probe_spi_rdid, spi_chip_erase_c7, NULL, spi_chip_write, spi_chip_read},{"Macronix", "MX25L2005", MX_ID, MX_25L2005, 256, 256, {{4096,64}}, TEST_UNTESTED, probe_spi_rdid, spi_chip_erase_c7, NULL, spi_chip_write, spi_chip_read},{"Macronix", "MX25L4005", MX_ID, MX_25L4005, 512, 256, {{4096,128}}, TEST_OK_PREW, probe_spi_rdid, spi_chip_erase_c7, NULL, spi_chip_write, spi_chip_read},{"Macronix", "MX25L8005", MX_ID, MX_25L8005, 1024, 256, {{4096,256}}, TEST_OK_PREW, probe_spi_rdid, spi_chip_erase_c7, NULL, spi_chip_write, spi_chip_read},{"Macronix", "MX25L1605", MX_ID, MX_25L1605, 2048, 256, {{4096,512}}, TEST_UNTESTED, probe_spi_rdid, spi_chip_erase_c7, NULL, spi_chip_write, spi_chip_read},{"Macronix", "MX25L3205", MX_ID, MX_25L3205, 4096, 256, {{4096,1024}}, TEST_OK_PREW, probe_spi_rdid, spi_chip_erase_c7, NULL, spi_chip_write, spi_chip_read},{"Macronix", "MX25L6405", MX_ID, MX_25L6405, 8192, 256, {{4096,2048}}, TEST_UNTESTED, probe_spi_rdid, spi_chip_erase_c7, NULL, spi_chip_write, spi_chip_read},{"Macronix", "MX29F002B", MX_ID, MX_29F002B, 256, 64 * 1024, {{16384,1},{8192,2},{32768,1},{65536,3}},TEST_UNTESTED, probe_29f002, erase_29f002, NULL, write_29f002},{"Macronix", "MX29F002T", MX_ID, MX_29F002T, 256, 64 * 1024, {{65536,3},{32768,1},{8192,2},{16384,1}},TEST_UNTESTED, probe_29f002, erase_29f002, NULL, write_29f002},...
From the flash chip names, I guess the chips with uniform erase block
size are serial flash chips, and the ones with non-uniform size are parallel flash chips.
We have serial flash chips with non-uniform sector size and parallel flash chips with uniform sector size. We even have some flash chips which have uniform sector size for one erase command and non-uniform sector size for another erase command.
I propose a new design the 'flashchip' structure. In this new design, serial and parallel flash chip information is stored in 'serial_flashchip' and 'parallel_flashchip' seperately, with a common 'flashchip_common' internal struct as their first field.
serial_flashchip +------------------+------------------------------+ | flashchip_common | sector_size, block_size, ... | +------------------+------------------------------+
parallel_flashchip +------------------+------------------------------+ | flashchip_common | erase_block | +------------------+------------------------------+
'flashchip_common' contains names, id's and common operation functions.
The design above looks clearer.
Sorry, this will not work. See above.
And There is another advantage.
I guess(confirm and disproof welcome) one chipset only provides one of the two flash interface. When a chipset is detected, we can probe only one kind of them, (hopefully) faster and safer. It is ok if we want to probe the other kind.
ICH7,ICH8,ICH9,ICH10 and some VIA chipsets can provide SPI and LPC at the same time. (ICH7 has some problems if SPI and FWH are attached, but the other chipsets can do it.)
Index: flashrom-eraseblocks/flashrom.c
--- flashrom-eraseblocks/flashrom.c (Revision 3776) +++ flashrom-eraseblocks/flashrom.c (Arbeitskopie) @@ -534,11 +534,33 @@
if (erase_it) { printf("Erasing flash chip.\n");
if (!flash->erase) {fprintf(stderr, "Error: flashrom has no erase function for this flash chip.\n");
if (!flash->block_erase && flash->eraseblocks[0].count) {fprintf(stderr, "Hint: flashrom knows the eraseblock ""layout, but there is no blockwise erase ""function for this flash chip. ""Using whole-chip erase.\n");}if (flash->block_erase && !flash->eraseblocks[0].count) {fprintf(stderr, "Hint: flashrom has a blockwise erase ""function for this flash chip, but the ""eraseblock layout is unknown. ""Using whole-chip erase.\n");}if (flash->block_erase && flash->eraseblocks[0].count) {unsigned long done = 0;int i, j;for (i = 0; done < flash->total_size * 1024; i++) {for (j = 0; j < flash->eraseblocks[i].count; j++) {flash->block_erase(flash, done + flash->eraseblocks[i].size * j);}done += flash->eraseblocks[i].count * flash->eraseblocks[i].size;}} else if (flash->erase) {flash->erase(flash);} else {fprintf(stderr, "Error: flashrom has no chip erase function for this flash chip.\n"); return 1; }
flash->erase(flash); exit(0); } else if (read_it) { if ((image = fopen(filename, "w")) == NULL) {No comment to the logic. But the new code has a different degree of detail compared to other 'if (do_it)' blocks(i.e., the for loop).
Yes, I can move that out to a separate function.
Regards, Carl-Daniel