[PATCH] Constify flashchips array

[Carl-Daniel Hailfinger]

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

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. */

[Stefan Reinauer]

* Carl-Daniel Hailfinger  [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
> 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. */
>
> flashrom mailing list
> [e-mail]
> http://www.flashrom.org/mailman/listinfo/flashrom

[Carl-Daniel Hailfinger]

Auf 19.01.2011 07:11, Stefan Reinauer schrieb:
> Carl-Daniel Hailfinger  [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

[Stefan Reinauer]

* Carl-Daniel Hailfinger  [110119 08:49]:
> Auf 19.01.2011 07:11, Stefan Reinauer schrieb:
> > Carl-Daniel Hailfinger  [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

[Carl-Daniel Hailfinger]

Auf 19.01.2011 20:00, Stefan Reinauer schrieb:
> Carl-Daniel Hailfinger  [110119 08:49]:
> > Auf 19.01.2011 07:11, Stefan Reinauer schrieb:
> > > Carl-Daniel Hailfinger  [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

[Carl-Daniel Hailfinger]

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 Hailfinger  [110119 08:49]:
> > > Auf 19.01.2011 07:11, Stefan Reinauer schrieb:
> > > > Carl-Daniel Hailfinger  [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

[Stefan Reinauer]

On 03.05.2011, at 16:11, Carl-Daniel Hailfinger  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 Hailfinger  [110119 08:49]:
> > > > Auf 19.01.2011 07:11, Stefan Reinauer schrieb:
> > > > > Carl-Daniel Hailfinger  [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

[Carl-Daniel Hailfinger]

Am 04.05.2011 01:36 schrieb Stefan Reinauer:
> On 03.05.2011, at 16:11, Carl-Daniel Hailfinger  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 Reinauer

and committed in r1293.

Regards,
Carl-Daniel