Re: [coreboot] Fix ambiguity, if a board is equipped with more than one chip

[Carl-Daniel Hailfinger]

Hi Claus,

thanks for the patch. Comments below.

On 05.05.2008 16:47, Claus Gindhart wrote:
> Currently there is an ongoing technology migration from FWH to SPI chips. For 
> this reason some boards have multiple chips of different technologies 
> onboard. Flashrom will probe for up to 3 chips; if more than one chip is 
> detected, it will stop, and will prompt the user to choose one by the -c 
> option, e.g.

Can we support probing for an arbitrary number of chips instead? 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.
> [root@localhost src]# ./flashrom 
> ...
> Warning: Multiple flash chips detected
> SST49LF008A
> M25P16@ICH9
> Please fix this ambiguity by providing the
>    -c | --chip <chipname>
> command line option

What happens if there are multiple flash chips of the same technology? I
have seen data sheets from Intel which suggest it is possible to have at
least 4 FWH chips on a board (we don't support that right now).
> Signed-off-by: Claus Gindhart
> Index: flashrom.c
>
> --- flashrom.c	(revision 3275)
> +++ flashrom.c	(working copy)
> @@ -3,7 +3,7 @@
> Copyright (C) 2000 Silicon Integrated System Corporation
> Copyright (C) 2004 Tyan Corp
> Copyright (C) 2005-2007 coresystems GmbH
> Copyright (C) 2005-2007 coresystems GmbH
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License as published by
>
> @@ -127,7 +127,7 @@
>    	flash_baseaddr = (0xffffffff - size + 1);
>  #endif
> /* If getpagesize() > size -> 
> /* If getpagesize() > size ->
> "Can't mmap memory using /dev/mem: Invalid argument"
> This should never happen as we don't support any flash chips
> smaller than 4k or 8k (yet).
>
> @@ -246,7 +246,13 @@
>    uint8_t *buf;
>    unsigned long size;
>    FILE *image;
> /* The flash chip to be supported */
> struct flashchip *flash;
> /* Alternative flash chips, if more than one are found */
> struct flashchip *flash2, *flash3;

struct flashchip *flash[]=malloc(sizeof(struct flashchip *));
> int opt;
>    int option_index = 0;
>    int read_it = 0, write_it = 0, erase_it = 0, verify_it = 0;
> @@ -405,12 +411,35 @@
>
> board_flash_enable(lb_vendor, lb_part);
> if ((flash = probe_flash(flashchips)) == NULL) {
> /* Probe for maximum 3 types of onboard flashes */
> flash = probe_flash(flashchips);
> if (flash == NULL) {
> printf("No EEPROM/flash device found.\n");
> // FIXME: flash writes stay enabled!
> exit(1);
> } else {
> flash2 = probe_flash(flash+1);
> if (flash2 != NULL) {
> 	flash3 = probe_flash(flash2+1);
> }
> }

Please rewrite the chunk above as a loop together with realloc().
> /* If more than 1 flash chip is detected, we cannot continue */
> if (flash2)

This check would have to be changed as well. Maybe introduce a separate
flash chip counter?
> {
> printf("Warning: Multiple flash chips detected\n");
> printf("%s\n",flash->name);
> printf("%s\n",flash2->name);
> if (flash3) printf("%s\n",flash3->name);

Use a loop here as well.
> printf(
> 	"Please fix this ambiguity by providing the\n"
> 	"   -c | --chip <chipname>\n"
> 	"command line option\n");
> // FIXME: flash writes stay enabled!

You can drop that FIXME for now. It's in the wrong place and we also
have seen that it can be harmful to restore flash writability to the
state it had before running flashrom.
> exit(1);
> }
> printf("Flash part is %s (%d KB).\n", flash->name, flash->total_size);

Could you move that info printing into the loop above? That would help
users a lot when they try to identify the chip.
> if (!(read_it | write_it | verify_it | erase_it)) {
> @@ -476,7 +505,7 @@
>    }
>
> /* exclude range stuff. Nice idea, but at the moment it is only
> * supported in hardware by the pm49fl004 chips. 
> * supported in hardware by the pm49fl004 chips.
> Instead of implementing this for all chips I suggest advancing
> it to the rom layout feature below and drop exclude range
> completely once all flash chips can do rom layouts. stepan
>
> @@ -496,7 +525,7 @@
>    exclude_end_page = exclude_end_position / flash->page_size;
>    // ////////////////////////////////////////////////////////////
> // This should be moved into each flash part's code to do it
> // This should be moved into each flash part's code to do it
> // cleanly. This does the job.
> handle_romentries(buf, (uint8_t *) flash->virtual_memory);

Overall, I really like what the patch does. I think one more iteration
will be enough to get it merged.

Regards,
Carl-Daniel