On Sat, Nov 29, 2008 at 9:15 AM, Carl-Daniel Hailfinger
<[EMAIL PROTECTED]> 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.
> 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. 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. 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.
> 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).
yu ning
--
coreboot mailing list: [email protected]
http://www.coreboot.org/mailman/listinfo/coreboot
