[coreboot] flashrom: Probe for up to 3 flash chips. [was: Fix ambiguity, if a board is equipped with more than one chip]

[Carl-Daniel Hailfinger]

On 05.05.2008 17:49, Peter Stuge wrote:
> On Mon, May 05, 2008 at 05:07:45PM +0200, Carl-Daniel Hailfinger wrote:
>   
>> Can we support probing for an arbitrary number of chips instead?
>>     
>
> I think that is overly ambitious.
>
> flashrom assumes that flash chips are top-aligned at 4GB so there
> will not be very many flash chips actually connected in a way that
> flashrom understands today.
>   

Yes, but it would be more future-proof(TM).

>> By using an array flash[] instead of flash, flash2 and flash3 this
>> could be done in a loop without limits on the number of detected
>> chips.
>>     
>
> I thought [] too, but I set a fixed size. It's simple and cheap to
> increase the size should there be need, and the code will deal.
>   

OK.

>> What happens if there are multiple flash chips of the same
>> technology?
>>     
>
> If same name, then same size and at same address => larger problem.
>   

Same name, same size, different address... unsupported by flashrom right
now, but we could change that. Easily, even. The only problem is finding
hardware to test.

>> Overall, I really like what the patch does.
>>     
>
> Yes, me too.
>
>
> Claus, can you please test the attached patch and send an Acked-by
> line it if it works, then I'll commit.
>
> Thanks!
>
>
> //Peter
>   
> Index: flashrom.multichip/flashrom.c
> ===================================================================
> --- flashrom.multichip/flashrom.c	(revision 3277)
> +++ flashrom.multichip/flashrom.c	(working copy)
> @@ -246,11 +246,12 @@
>  	uint8_t *buf;
>  	unsigned long size;
>  	FILE *image;
> -	struct flashchip *flash;
> +	/* Probe for up to three flash chips. */
> +	struct flashchip *flash, *flashes[3];
>  	int opt;
>  	int option_index = 0;
>  	int read_it = 0, write_it = 0, erase_it = 0, verify_it = 0;
> -	int ret = 0;
> +	int ret = 0, i;
>  
>  	static struct option long_options[] = {
>  		{"read", 0, 0, 'r'},
> @@ -405,12 +406,27 @@
>  
>  	board_flash_enable(lb_vendor, lb_part);
>  
> -	if ((flash = probe_flash(flashchips)) == NULL) {
> +	for (i = 0; i < ARRAY_SIZE(flashes); i++) {
> +		flashes[i] = probe_flash(i ? flashes[i - 1] + 1 : flashchips);
> +		if (!flashes[i])
> +			for (i++; i < ARRAY_SIZE(flashes); i++)
> +				flashes[i] = NULL;
>   

This for loop could be replaced by an early memset, making the code clearer.

> +	}
> +
> +	if (flashes[1]) {
> +		printf("Multiple flash chips were detected:");
> +		for (i = 0; i < ARRAY_SIZE(flashes) && flashes[i]; i++)
> +			printf(" %s", flashes[i]->name);
> +		printf("\nPlease specify which chip to use with the -c <chipname> option.\n");
> +		exit(1);
> +	} else if (!flashes[0]) {
>  		printf("No EEPROM/flash device found.\n");
>  		// FIXME: flash writes stay enabled!
>  		exit(1);
>  	}
>  
> +	flash = flashes[0];
> +
>  	printf("Flash part is %s (%d KB).\n", flash->name, flash->total_size);
>   

Move that statement into the loop above, please.

>  	if (TEST_OK_MASK != (flash->tested & TEST_OK_MASK)) {
>  		printf("===\n");


Otherwise, this is
Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>


Regards,
Carl-Daniel