Constify flashchips array.
This moves 99.5% of the .data section to .rodata (which ends up in .text).
Disadvantages: We have one additional memcpy with sizeof(struct flashchip) per executed probe. This is purely startup cost and won't have any impact on operation after probing.
Tests would be appreciated, especially with machines having multiple chips.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-const_flashchips/flash.h =================================================================== --- flashrom-const_flashchips/flash.h (Revision 1252) +++ flashrom-const_flashchips/flash.h (Arbeitskopie) @@ -173,7 +173,7 @@ #define TIMING_IGNORED -1 #define TIMING_ZERO -2
-extern struct flashchip flashchips[]; +extern const struct flashchip flashchips[];
/* print.c */ char *flashbuses_to_text(enum chipbustype bustype); @@ -193,7 +193,7 @@ void map_flash_registers(struct flashchip *flash); int read_memmapped(struct flashchip *flash, uint8_t *buf, int start, int len); int erase_flash(struct flashchip *flash); -struct flashchip *probe_flash(struct flashchip *first_flash, int force); +int probe_flash(int startchip, struct flashchip *fill_flash, int force); int read_flash_to_file(struct flashchip *flash, char *filename); int min(int a, int b); int max(int a, int b); Index: flashrom-const_flashchips/cli_classic.c =================================================================== --- flashrom-const_flashchips/cli_classic.c (Revision 1252) +++ flashrom-const_flashchips/cli_classic.c (Arbeitskopie) @@ -100,7 +100,11 @@ { unsigned long size; /* Probe for up to three flash chips. */ - struct flashchip *flash, *flashes[3]; + const struct flashchip *flash; + struct flashchip flashes[3]; + struct flashchip *fill_flash; + int startchip = 0; + int chipcount = 0; const char *name; int namelen; int opt; @@ -359,49 +363,47 @@ exit(1); }
- /* FIXME: Delay calibration should happen in programmer code. */ for (i = 0; i < ARRAY_SIZE(flashes); i++) { - flashes[i] = - probe_flash(i ? flashes[i - 1] + 1 : flashchips, 0); - if (!flashes[i]) - for (i++; i < ARRAY_SIZE(flashes); i++) - flashes[i] = NULL; + startchip = probe_flash(startchip, &flashes[i], 0); + if (startchip == -1) + break; + chipcount++; }
- if (flashes[1]) { + if (chipcount > 1) { printf("Multiple flash chips were detected:"); - for (i = 0; i < ARRAY_SIZE(flashes) && flashes[i]; i++) - printf(" %s", flashes[i]->name); + for (i = 0; i < chipcount; i++) + printf(" %s", flashes[i].name); printf("\nPlease specify which chip to use with the -c <chipname> option.\n"); programmer_shutdown(); exit(1); - } else if (!flashes[0]) { + } else if (!chipcount) { printf("No EEPROM/flash device found.\n"); if (!force || !chip_to_probe) { printf("Note: flashrom can never write if the flash chip isn't found automatically.\n"); } if (force && read_it && chip_to_probe) { printf("Force read (-f -r -c) requested, pretending the chip is there:\n"); - flashes[0] = probe_flash(flashchips, 1); - if (!flashes[0]) { + startchip = probe_flash(0, &flashes[0], 1); + if (startchip == -1) { printf("Probing for flash chip '%s' failed.\n", chip_to_probe); programmer_shutdown(); exit(1); } printf("Please note that forced reads most likely contain garbage.\n"); - return read_flash_to_file(flashes[0], filename); + return read_flash_to_file(&flashes[0], filename); } // FIXME: flash writes stay enabled! programmer_shutdown(); exit(1); }
- flash = flashes[0]; + fill_flash = &flashes[0];
- check_chip_supported(flash); + check_chip_supported(fill_flash);
- size = flash->total_size * 1024; - if (check_max_decode((buses_supported & flash->bustype), size) && + size = fill_flash->total_size * 1024; + if (check_max_decode((buses_supported & fill_flash->bustype), size) && (!force)) { fprintf(stderr, "Chip is too big for this programmer " "(-V gives details). Use --force to override.\n"); @@ -432,5 +434,5 @@ * Give the chip time to settle. */ programmer_delay(100000); - return doit(flash, force, filename, read_it, write_it, erase_it, verify_it); + return doit(fill_flash, force, filename, read_it, write_it, erase_it, verify_it); } Index: flashrom-const_flashchips/print_wiki.c =================================================================== --- flashrom-const_flashchips/print_wiki.c (Revision 1252) +++ flashrom-const_flashchips/print_wiki.c (Arbeitskopie) @@ -201,7 +201,7 @@ static void print_supported_chips_wiki(int cols) { int i = 0, c = 1, chipcount = 0; - struct flashchip *f, *old = NULL; + const struct flashchip *f, *old = NULL; uint32_t t;
for (f = flashchips; f->name != NULL; f++) Index: flashrom-const_flashchips/flashchips.c =================================================================== --- flashrom-const_flashchips/flashchips.c (Revision 1252) +++ flashrom-const_flashchips/flashchips.c (Arbeitskopie) @@ -32,7 +32,7 @@ * Please keep the list sorted by vendor name and chip name, so that * the output of 'flashrom -L' is alphabetically sorted. */ -struct flashchip flashchips[] = { +const struct flashchip flashchips[] = {
/* * .vendor = Vendor name Index: flashrom-const_flashchips/flashrom.c =================================================================== --- flashrom-const_flashchips/flashrom.c (Revision 1252) +++ flashrom-const_flashchips/flashrom.c (Arbeitskopie) @@ -1123,15 +1123,15 @@ return 1; }
-struct flashchip *probe_flash(struct flashchip *first_flash, int force) +int probe_flash(int startchip, struct flashchip *fill_flash, int force) { - struct flashchip *flash; + const struct flashchip *flash; unsigned long base = 0; uint32_t size; enum chipbustype buses_common; char *tmp;
- for (flash = first_flash; flash && flash->name; flash++) { + for (flash = flashchips + startchip; flash && flash->name; flash++) { if (chip_to_probe && strcmp(flash->name, chip_to_probe) != 0) continue; msg_gdbg("Probing for %s %s, %d KB: ", @@ -1158,25 +1158,35 @@ size = flash->total_size * 1024; check_max_decode(buses_common, size);
+ /* Start filling in the dynamic data. */ + *fill_flash = *flash; + base = flashbase ? flashbase : (0xffffffff - size + 1); - flash->virtual_memory = (chipaddr)programmer_map_flash_region("flash chip", base, size); + fill_flash->virtual_memory = (chipaddr)programmer_map_flash_region("flash chip", base, size);
if (force) break;
- if (flash->probe(flash) != 1) + if (fill_flash->probe(fill_flash) != 1) goto notfound;
- if (first_flash == flashchips - || flash->model_id != GENERIC_DEVICE_ID) + /* If this is the first chip found, accept it. + * If this is not the first chip found, accept it only if it is + * a non-generic match. + * We could either make chipcount global or provide it as + * parameter, or we assume that startchip==0 means this call to + * probe_flash() is the first one and thus no chip has been + * found before. + */ + if (startchip == 0 || fill_flash->model_id != GENERIC_DEVICE_ID) break;
notfound: - programmer_unmap_flash_region((void *)flash->virtual_memory, size); + programmer_unmap_flash_region((void *)fill_flash->virtual_memory, size); }
if (!flash || !flash->name) - return NULL; + return -1;
msg_cinfo("%s chip "%s %s" (%d KB, %s) at physical address 0x%lx.\n", force ? "Assuming" : "Found", @@ -1187,10 +1197,11 @@ * may be stored in registers, so avoid lock info printing. */ if (!force) - if (flash->printlock) - flash->printlock(flash); + if (fill_flash->printlock) + fill_flash->printlock(fill_flash);
- return flash; + /* Return position of matching chip. */ + return flash - flashchips; }
int verify_flash(struct flashchip *flash, uint8_t *buf) @@ -1299,7 +1310,7 @@ * walk_eraseregions(). * Even if an error is found, the function will keep going and check the rest. */ -static int selfcheck_eraseblocks(struct flashchip *flash) +static int selfcheck_eraseblocks(const struct flashchip *flash) { int i, j, k; int ret = 0; @@ -1676,7 +1687,7 @@ int selfcheck(void) { int ret = 0; - struct flashchip *flash; + const struct flashchip *flash;
/* Safety check. Instead of aborting after the first error, check * if more errors exist. @@ -1695,7 +1706,7 @@ return ret; }
-void check_chip_supported(struct flashchip *flash) +void check_chip_supported(const struct flashchip *flash) { if (TEST_OK_MASK != (flash->tested & TEST_OK_MASK)) { msg_cinfo("===\n"); Index: flashrom-const_flashchips/programmer.h =================================================================== --- flashrom-const_flashchips/programmer.h (Revision 1252) +++ flashrom-const_flashchips/programmer.h (Arbeitskopie) @@ -497,7 +497,7 @@ extern struct decode_sizes max_rom_decode; extern int programmer_may_write; extern unsigned long flashbase; -void check_chip_supported(struct flashchip *flash); +void check_chip_supported(const struct flashchip *flash); int check_max_decode(enum chipbustype buses, uint32_t size); char *extract_programmer_param(char *param_name);
Index: flashrom-const_flashchips/print.c =================================================================== --- flashrom-const_flashchips/print.c (Revision 1252) +++ flashrom-const_flashchips/print.c (Arbeitskopie) @@ -78,7 +78,7 @@ { int okcol = 0, pos = 0, i, chipcount = 0; int maxchiplen = 0, maxvendorlen = 0; - struct flashchip *f; + const struct flashchip *f;
for (f = flashchips; f->name != NULL; f++) { /* Ignore "unknown XXXX SPI chip" entries. */
* Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net [110118 21:29]:
Constify flashchips array.
This moves 99.5% of the .data section to .rodata (which ends up in .text).
Disadvantages: We have one additional memcpy with sizeof(struct flashchip) per executed probe. This is purely startup cost and won't have any impact on operation after probing.
Nice!
You could have a "struct flash_instance" which is passed around and contains all the instance specific information of a flash chip plus a pointer to the struct flashchip element. This way you would cleanly separate the "database" from the flash chips found in the system (and could increase the number of flashes to 16 instead of 3). I know it requires an s,flashchip,flashchip_instance, across most of the code and some additional changes, but the readability might be worth the effort.
Stefan
Tests would be appreciated, especially with machines having multiple chips.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-const_flashchips/flash.h
--- flashrom-const_flashchips/flash.h (Revision 1252) +++ flashrom-const_flashchips/flash.h (Arbeitskopie) @@ -173,7 +173,7 @@ #define TIMING_IGNORED -1 #define TIMING_ZERO -2
-extern struct flashchip flashchips[]; +extern const struct flashchip flashchips[];
/* print.c */ char *flashbuses_to_text(enum chipbustype bustype); @@ -193,7 +193,7 @@ void map_flash_registers(struct flashchip *flash); int read_memmapped(struct flashchip *flash, uint8_t *buf, int start, int len); int erase_flash(struct flashchip *flash); -struct flashchip *probe_flash(struct flashchip *first_flash, int force); +int probe_flash(int startchip, struct flashchip *fill_flash, int force); int read_flash_to_file(struct flashchip *flash, char *filename); int min(int a, int b); int max(int a, int b); Index: flashrom-const_flashchips/cli_classic.c =================================================================== --- flashrom-const_flashchips/cli_classic.c (Revision 1252) +++ flashrom-const_flashchips/cli_classic.c (Arbeitskopie) @@ -100,7 +100,11 @@ { unsigned long size; /* Probe for up to three flash chips. */
- struct flashchip *flash, *flashes[3];
- const struct flashchip *flash;
- struct flashchip flashes[3];
- struct flashchip *fill_flash;
- int startchip = 0;
- int chipcount = 0; const char *name; int namelen; int opt;
@@ -359,49 +363,47 @@ exit(1); }
- /* FIXME: Delay calibration should happen in programmer code. */ for (i = 0; i < ARRAY_SIZE(flashes); i++) {
flashes[i] =
probe_flash(i ? flashes[i - 1] + 1 : flashchips, 0);
if (!flashes[i])
for (i++; i < ARRAY_SIZE(flashes); i++)
flashes[i] = NULL;
startchip = probe_flash(startchip, &flashes[i], 0);
if (startchip == -1)
break;
}chipcount++;
- if (flashes[1]) {
- if (chipcount > 1) { printf("Multiple flash chips were detected:");
for (i = 0; i < ARRAY_SIZE(flashes) && flashes[i]; i++)
printf(" %s", flashes[i]->name);
for (i = 0; i < chipcount; i++)
printf("\nPlease specify which chip to use with the -c <chipname> option.\n"); programmer_shutdown(); exit(1);printf(" %s", flashes[i].name);
- } else if (!flashes[0]) {
- } else if (!chipcount) { printf("No EEPROM/flash device found.\n"); if (!force || !chip_to_probe) { printf("Note: flashrom can never write if the flash chip isn't found automatically.\n"); } if (force && read_it && chip_to_probe) { printf("Force read (-f -r -c) requested, pretending the chip is there:\n");
flashes[0] = probe_flash(flashchips, 1);
if (!flashes[0]) {
startchip = probe_flash(0, &flashes[0], 1);
if (startchip == -1) { printf("Probing for flash chip '%s' failed.\n", chip_to_probe); programmer_shutdown(); exit(1); } printf("Please note that forced reads most likely contain garbage.\n");
return read_flash_to_file(flashes[0], filename);
} // FIXME: flash writes stay enabled! programmer_shutdown(); exit(1); }return read_flash_to_file(&flashes[0], filename);
- flash = flashes[0];
- fill_flash = &flashes[0];
- check_chip_supported(flash);
- check_chip_supported(fill_flash);
- size = flash->total_size * 1024;
- if (check_max_decode((buses_supported & flash->bustype), size) &&
- size = fill_flash->total_size * 1024;
- if (check_max_decode((buses_supported & fill_flash->bustype), size) && (!force)) { fprintf(stderr, "Chip is too big for this programmer " "(-V gives details). Use --force to override.\n");
@@ -432,5 +434,5 @@ * Give the chip time to settle. */ programmer_delay(100000);
- return doit(flash, force, filename, read_it, write_it, erase_it, verify_it);
- return doit(fill_flash, force, filename, read_it, write_it, erase_it, verify_it);
} Index: flashrom-const_flashchips/print_wiki.c =================================================================== --- flashrom-const_flashchips/print_wiki.c (Revision 1252) +++ flashrom-const_flashchips/print_wiki.c (Arbeitskopie) @@ -201,7 +201,7 @@ static void print_supported_chips_wiki(int cols) { int i = 0, c = 1, chipcount = 0;
- struct flashchip *f, *old = NULL;
const struct flashchip *f, *old = NULL; uint32_t t;
for (f = flashchips; f->name != NULL; f++)
Index: flashrom-const_flashchips/flashchips.c
--- flashrom-const_flashchips/flashchips.c (Revision 1252) +++ flashrom-const_flashchips/flashchips.c (Arbeitskopie) @@ -32,7 +32,7 @@
- Please keep the list sorted by vendor name and chip name, so that
- the output of 'flashrom -L' is alphabetically sorted.
*/ -struct flashchip flashchips[] = { +const struct flashchip flashchips[] = {
/* * .vendor = Vendor name Index: flashrom-const_flashchips/flashrom.c =================================================================== --- flashrom-const_flashchips/flashrom.c (Revision 1252) +++ flashrom-const_flashchips/flashrom.c (Arbeitskopie) @@ -1123,15 +1123,15 @@ return 1; }
-struct flashchip *probe_flash(struct flashchip *first_flash, int force) +int probe_flash(int startchip, struct flashchip *fill_flash, int force) {
- struct flashchip *flash;
- const struct flashchip *flash; unsigned long base = 0; uint32_t size; enum chipbustype buses_common; char *tmp;
- for (flash = first_flash; flash && flash->name; flash++) {
- for (flash = flashchips + startchip; flash && flash->name; flash++) { if (chip_to_probe && strcmp(flash->name, chip_to_probe) != 0) continue; msg_gdbg("Probing for %s %s, %d KB: ",
@@ -1158,25 +1158,35 @@ size = flash->total_size * 1024; check_max_decode(buses_common, size);
/* Start filling in the dynamic data. */
*fill_flash = *flash;
- base = flashbase ? flashbase : (0xffffffff - size + 1);
flash->virtual_memory = (chipaddr)programmer_map_flash_region("flash chip", base, size);
fill_flash->virtual_memory = (chipaddr)programmer_map_flash_region("flash chip", base, size);
if (force) break;
if (flash->probe(flash) != 1)
if (fill_flash->probe(fill_flash) != 1) goto notfound;
if (first_flash == flashchips
|| flash->model_id != GENERIC_DEVICE_ID)
/* If this is the first chip found, accept it.
* If this is not the first chip found, accept it only if it is
* a non-generic match.
* We could either make chipcount global or provide it as
* parameter, or we assume that startchip==0 means this call to
* probe_flash() is the first one and thus no chip has been
* found before.
*/
if (startchip == 0 || fill_flash->model_id != GENERIC_DEVICE_ID) break;
notfound:
programmer_unmap_flash_region((void *)flash->virtual_memory, size);
programmer_unmap_flash_region((void *)fill_flash->virtual_memory, size);
}
if (!flash || !flash->name)
return NULL;
return -1;
msg_cinfo("%s chip "%s %s" (%d KB, %s) at physical address 0x%lx.\n", force ? "Assuming" : "Found",
@@ -1187,10 +1197,11 @@ * may be stored in registers, so avoid lock info printing. */ if (!force)
if (flash->printlock)
flash->printlock(flash);
if (fill_flash->printlock)
fill_flash->printlock(fill_flash);
- return flash;
- /* Return position of matching chip. */
- return flash - flashchips;
}
int verify_flash(struct flashchip *flash, uint8_t *buf) @@ -1299,7 +1310,7 @@
- walk_eraseregions().
- Even if an error is found, the function will keep going and check the rest.
*/ -static int selfcheck_eraseblocks(struct flashchip *flash) +static int selfcheck_eraseblocks(const struct flashchip *flash) { int i, j, k; int ret = 0; @@ -1676,7 +1687,7 @@ int selfcheck(void) { int ret = 0;
- struct flashchip *flash;
const struct flashchip *flash;
/* Safety check. Instead of aborting after the first error, check
- if more errors exist.
@@ -1695,7 +1706,7 @@ return ret; }
-void check_chip_supported(struct flashchip *flash) +void check_chip_supported(const struct flashchip *flash) { if (TEST_OK_MASK != (flash->tested & TEST_OK_MASK)) { msg_cinfo("===\n"); Index: flashrom-const_flashchips/programmer.h =================================================================== --- flashrom-const_flashchips/programmer.h (Revision 1252) +++ flashrom-const_flashchips/programmer.h (Arbeitskopie) @@ -497,7 +497,7 @@ extern struct decode_sizes max_rom_decode; extern int programmer_may_write; extern unsigned long flashbase; -void check_chip_supported(struct flashchip *flash); +void check_chip_supported(const struct flashchip *flash); int check_max_decode(enum chipbustype buses, uint32_t size); char *extract_programmer_param(char *param_name);
Index: flashrom-const_flashchips/print.c
--- flashrom-const_flashchips/print.c (Revision 1252) +++ flashrom-const_flashchips/print.c (Arbeitskopie) @@ -78,7 +78,7 @@ { int okcol = 0, pos = 0, i, chipcount = 0; int maxchiplen = 0, maxvendorlen = 0;
- struct flashchip *f;
const struct flashchip *f;
for (f = flashchips; f->name != NULL; f++) { /* Ignore "unknown XXXX SPI chip" entries. */
flashrom mailing list flashrom@flashrom.org http://www.flashrom.org/mailman/listinfo/flashrom
Auf 19.01.2011 07:11, Stefan Reinauer schrieb:
- Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net [110118 21:29]:
Constify flashchips array.
This moves 99.5% of the .data section to .rodata (which ends up in .text).
Disadvantages: We have one additional memcpy with sizeof(struct flashchip) per executed probe. This is purely startup cost and won't have any impact on operation after probing.
Nice!
Thanks.
You could have a "struct flash_instance" which is passed around and contains all the instance specific information of a flash chip plus a pointer to the struct flashchip element. This way you would cleanly separate the "database" from the flash chips found in the system (and could increase the number of flashes to 16 instead of 3). I know it requires an s,flashchip,flashchip_instance, across most of the code and some additional changes, but the readability might be worth the effort.
Given that almost all the code works with the instance data, it may be better to use struct flashchip for the instance data and struct flashchip_rodata (or flashchip_def) for the table. If flashchip_rodata is an anonymous struct within flashchip, we have easy copying and can keep all currently untouched code as is. As a nice side effect, the flashrom .text size would shrink by a few percent.
My longterm plans call for programmer info (at least a pointer) in struct flashchip so that we can handle multiple flashchips on sister buses easier (e.g. software-switched "Dual BIOS" solutions) because flashchip contains e.g. a enable_this_chip function pointer.
What do you think?
Regards, Carl-Daniel
* Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net [110119 08:49]:
Auf 19.01.2011 07:11, Stefan Reinauer schrieb:
- Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net [110118 21:29]:
You could have a "struct flash_instance" which is passed around and contains all the instance specific information of a flash chip plus a pointer to the struct flashchip element. This way you would cleanly separate the "database" from the flash chips found in the system (and could increase the number of flashes to 16 instead of 3). I know it requires an s,flashchip,flashchip_instance, across most of the code and some additional changes, but the readability might be worth the effort.
Given that almost all the code works with the instance data, it may be better to use struct flashchip for the instance data and struct flashchip_rodata (or flashchip_def) for the table. If flashchip_rodata is an anonymous struct within flashchip, we have easy copying and can keep all currently untouched code as is. As a nice side effect, the flashrom .text size would shrink by a few percent.
My longterm plans call for programmer info (at least a pointer) in struct flashchip so that we can handle multiple flashchips on sister buses easier (e.g. software-switched "Dual BIOS" solutions) because flashchip contains e.g. a enable_this_chip function pointer.
What do you think?
Sounds great! This is a very good clean up of flashrom's "device model"
Stefan
Auf 19.01.2011 20:00, Stefan Reinauer schrieb:
- Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net [110119 08:49]:
Auf 19.01.2011 07:11, Stefan Reinauer schrieb:
- Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net [110118 21:29]:
You could have a "struct flash_instance" which is passed around and contains all the instance specific information of a flash chip plus a pointer to the struct flashchip element. This way you would cleanly separate the "database" from the flash chips found in the system (and could increase the number of flashes to 16 instead of 3). I know it requires an s,flashchip,flashchip_instance, across most of the code and some additional changes, but the readability might be worth the effort.
Given that almost all the code works with the instance data, it may be better to use struct flashchip for the instance data and struct flashchip_rodata (or flashchip_def) for the table. If flashchip_rodata is an anonymous struct within flashchip, we have easy copying and can keep all currently untouched code as is. As a nice side effect, the flashrom .text size would shrink by a few percent.
Turns out that even gcc 4.5.0 chokes on this (but can support it with a special feature switch), and older gcc versions can't handle it at all. The pointer-to-flashchip_def variant breaks chips with variable size, and even just duplicating struct flashchip into a slightly trimmed down struct flashchip_def causes compilation problems on some targets.
My longterm plans call for programmer info (at least a pointer) in struct flashchip so that we can handle multiple flashchips on sister buses easier (e.g. software-switched "Dual BIOS" solutions) because flashchip contains e.g. a enable_this_chip function pointer.
What do you think?
Sounds great! This is a very good clean up of flashrom's "device model"
My last patch is the only variant I could find which works on all currently supported compilers, and until I've figured out a way to implement something better, this is still a step forward, so I'm going to ask for a code review and an Ack.
Regards, Carl-Daniel
Ping?
The plans to create a more advanced version of the patch will break compilation with gcc 4.5.0 and older, so I'd like to keep the patch as is. Subsequent optimizations can be built on top of this.
Regards, Carl-Daniel
Am 03.03.2011 23:23 schrieb Carl-Daniel Hailfinger:
Auf 19.01.2011 20:00, Stefan Reinauer schrieb:
- Carl-Daniel Hailfingerc-d.hailfinger.devel.2006@gmx.net [110119 08:49]:
Auf 19.01.2011 07:11, Stefan Reinauer schrieb:
- Carl-Daniel Hailfingerc-d.hailfinger.devel.2006@gmx.net [110118 21:29]:
You could have a "struct flash_instance" which is passed around and contains all the instance specific information of a flash chip plus a pointer to the struct flashchip element. This way you would cleanly separate the "database" from the flash chips found in the system (and could increase the number of flashes to 16 instead of 3). I know it requires an s,flashchip,flashchip_instance, across most of the code and some additional changes, but the readability might be worth the effort.
Given that almost all the code works with the instance data, it may be better to use struct flashchip for the instance data and struct flashchip_rodata (or flashchip_def) for the table. If flashchip_rodata is an anonymous struct within flashchip, we have easy copying and can keep all currently untouched code as is. As a nice side effect, the flashrom .text size would shrink by a few percent.
Turns out that even gcc 4.5.0 chokes on this (but can support it with a special feature switch), and older gcc versions can't handle it at all. The pointer-to-flashchip_def variant breaks chips with variable size, and even just duplicating struct flashchip into a slightly trimmed down struct flashchip_def causes compilation problems on some targets.
My longterm plans call for programmer info (at least a pointer) in struct flashchip so that we can handle multiple flashchips on sister buses easier (e.g. software-switched "Dual BIOS" solutions) because flashchip contains e.g. a enable_this_chip function pointer.
What do you think?
Sounds great! This is a very good clean up of flashrom's "device model"
My last patch is the only variant I could find which works on all currently supported compilers, and until I've figured out a way to implement something better, this is still a step forward, so I'm going to ask for a code review and an Ack.
Regards, Carl-Daniel
On 03.05.2011, at 16:11, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Ping?
The plans to create a more advanced version of the patch will break compilation with gcc 4.5.0 and older, so I'd like to keep the patch as is. Subsequent optimizations can be built on top of this.
Sure whatever works is fine.
Regards, Carl-Daniel
Am 03.03.2011 23:23 schrieb Carl-Daniel Hailfinger:
Auf 19.01.2011 20:00, Stefan Reinauer schrieb:
- Carl-Daniel Hailfingerc-d.hailfinger.devel.2006@gmx.net [110119 08:49]:
Auf 19.01.2011 07:11, Stefan Reinauer schrieb:
- Carl-Daniel Hailfingerc-d.hailfinger.devel.2006@gmx.net [110118 21:29]:
You could have a "struct flash_instance" which is passed around and contains all the instance specific information of a flash chip plus a pointer to the struct flashchip element. This way you would cleanly separate the "database" from the flash chips found in the system (and could increase the number of flashes to 16 instead of 3). I know it requires an s,flashchip,flashchip_instance, across most of the code and some additional changes, but the readability might be worth the effort.
Given that almost all the code works with the instance data, it may be better to use struct flashchip for the instance data and struct flashchip_rodata (or flashchip_def) for the table. If flashchip_rodata is an anonymous struct within flashchip, we have easy copying and can keep all currently untouched code as is. As a nice side effect, the flashrom .text size would shrink by a few percent.
Turns out that even gcc 4.5.0 chokes on this (but can support it with a special feature switch), and older gcc versions can't handle it at all. The pointer-to-flashchip_def variant breaks chips with variable size, and even just duplicating struct flashchip into a slightly trimmed down struct flashchip_def causes compilation problems on some targets.
My longterm plans call for programmer info (at least a pointer) in struct flashchip so that we can handle multiple flashchips on sister buses easier (e.g. software-switched "Dual BIOS" solutions) because flashchip contains e.g. a enable_this_chip function pointer.
What do you think?
Sounds great! This is a very good clean up of flashrom's "device model"
My last patch is the only variant I could find which works on all currently supported compilers, and until I've figured out a way to implement something better, this is still a step forward, so I'm going to ask for a code review and an Ack.
Regards, Carl-Daniel
Am 04.05.2011 01:36 schrieb Stefan Reinauer:
On 03.05.2011, at 16:11, Carl-Daniel Hailfingerc-d.hailfinger.devel.2006@gmx.net wrote:
Ping?
The plans to create a more advanced version of the patch will break compilation with gcc 4.5.0 and older, so I'd like to keep the patch as is. Subsequent optimizations can be built on top of this.
Sure whatever works is fine.
Thanks. For the record (and confirmed on IRC) this is
Acked-by: Stefan Reinauerstefan.reinauer@coreboot.org
and committed in r1293.
Regards, Carl-Daniel