[flashrom] [PATCH] Constify flashchips array

[Stefan Reinauer]

* Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at 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 at 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. */
> 
> 
> -- 
> http://www.hailfinger.org/
> 
> 
> _______________________________________________
> flashrom mailing list
> flashrom at flashrom.org
> http://www.flashrom.org/mailman/listinfo/flashrom
>