Fix ambiguity, if a board is equipped with more than one chip

[Claus Gindhart]

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.

[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

Signed-off-by: Claus Gindhart

---

Mit freundlichen Grüßen / Best regards

Claus Gindhart
 SW R&D 
 Kontron Modular Computers
 phone :++49 (0)8341-803-374
 mailto:[e-mail] 
 http://www.kontron.com

Kontron Modular Computers GmbH
Geschaeftsfuehrer / Managing Directors: Ulrich Gehrmann, Thomas Sabisch 
Sitz der Gesellschaft / Registered Office: Kaufbeuren, Rechtsform / Legal: GmbH 
Amtsgericht / Local District Court Kempten, HRB Nr. / Trade Register No. 6195

The information contained in this document is CONFIDENTIAL and property of Kontron.
Any unauthorized review, use, disclosure or distribution is prohibited without express
written consent of Kontron. If you are not the intended recipient, please contact the
sender and destroy all copies of the original message and enclosed attachments.

Diese E-Mail enthält vertrauliche und/oder rechtlich geschützte Informationen und ist
Eigentum von Kontron. Die Verwendung und Weitergabe von jeglichen Inhalten ist
ohne ausdrückliche schriftliche Genehmigung von Kontron strikt untersagt. Wenn Sie
diese E-Mail irrtümlich erhalten haben, informieren Sie bitte sofort den Absender und
vernichten diese Mail und enthaltene Dokumente.

[Peter Stuge]

Dear Claus, please confirm that you're subscribed to the list.

On Mon, May 05, 2008 at 04:47:29PM +0200, Claus Gindhart wrote:
> 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

Thanks!

I will rework the patch a little and resend later today.

Remember to add yourself or your employer as copyright holder in your
patches, not just add nice new features. That is important too. :)

//Peter

[Claus Gindhart]

Hi Peter,

yes, i am subscribed to the coreboot list.

Please be aware, that Carl-Daniel Hailfinger already provided some senseful 
proposals to my patch and take them into account, when working on it.

By the way: In my application, i replace the flashrom.c by my own frontend, 
because i need a somehow different user interface. My submission work is 
mainly concentrated on low-level HW-functions. I only submitted this patch, 
because without this patch i couldnt do any tests with the original flashrom.

So, i am happy about your proposal, because flashrom.c is not in my focus.

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

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

[Peter Stuge]

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.
> 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.
> What happens if there are multiple flash chips of the same
> technology?

If same name, then same size and at same address => larger problem.
> 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

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

Regards,
Carl-Daniel

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 18:00, Carl-Daniel Hailfinger wrote:
> On 05.05.2008 17:49, Peter Stuge wrote:
> > 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])

Missing break. This loop runs ARRAY_SIZE(flashchips) times even if the
first probe failed.
> > 	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
>
> Regards,
> Carl-Daniel

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

[Peter Stuge]

On Mon, May 05, 2008 at 06:00:13PM +0200, Carl-Daniel Hailfinger wrote:
> > 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).

That future will require many other changes, so I prefer keeping it
simple for now.
> > > 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.

When we do, let's revisit the "unlimited" flash chip count.
> > 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.

Sure. Matter of taste.
> > printf("Flash part is %s (%d KB).\n", flash->name, flash->total_size);
>
> Move that statement into the loop above, please.

No need since the probe() function always prints the name of the
found flash chip.
> Otherwise, this is
> Acked-by: Carl-Daniel Hailfinger

Thanks, but I'm still waiting for Claus to test that it works on his
hardware.

On Mon, May 05, 2008 at 06:06:53PM +0200, Carl-Daniel Hailfinger wrote:
> > > for (i = 0; i < ARRAY_SIZE(flashes); i++) {
> > > flashes[i] = probe_flash(i ? flashes[i - 1] + 1 : flashchips);
> > > if (!flashes[i])
>
> Missing break. This loop runs ARRAY_SIZE(flashchips) times even if the
> first probe failed.
> > > 	for (i++; i < ARRAY_SIZE(flashes); i++)
> > > 		flashes[i] = NULL;

The same counter is used to clear remaining entries so that the outer
loop also is finished after the first failed probe.

//Peter

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 18:29, Peter Stuge wrote:
> On Mon, May 05, 2008 at 06:00:13PM +0200, Carl-Daniel Hailfinger wrote:
> > > printf("Flash part is %s (%d KB).\n", flash->name, flash->total_size);
> >
> > Move that statement into the loop above, please.
>
> No need since the probe() function always prints the name of the
> found flash chip.

Either the statement is redundant and we remove it completely, or we
move it inside the loop. IMO requiring the user to look up flash chip
size from a model number is not the way to go.
> > Otherwise, this is
> > Acked-by: Carl-Daniel Hailfinger
>
> Thanks, but I'm still waiting for Claus to test that it works on his
> hardware.
>
> On Mon, May 05, 2008 at 06:06:53PM +0200, Carl-Daniel Hailfinger wrote:
> > > > for (i = 0; i < ARRAY_SIZE(flashes); i++) {
> > > > flashes[i] = probe_flash(i ? flashes[i - 1] + 1 : flashchips);
> > > > if (!flashes[i])
> >
> > Missing break. This loop runs ARRAY_SIZE(flashchips) times even if the
> > first probe failed.
> > > > 	for (i++; i < ARRAY_SIZE(flashes); i++)
> > > > 		flashes[i] = NULL;
>
> The same counter is used to clear remaining entries so that the outer
> loop also is finished after the first failed probe.

I see.

Regards,
Carl-Daniel

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

[Claus Gindhart]

Kontron Modular Computers GmbH
Geschaeftsfuehrer / Managing Directors: Ulrich Gehrmann, Thomas Sabisch 
Sitz der Gesellschaft / Registered Office: Kaufbeuren, Rechtsform / Legal: GmbH 
Amtsgericht / Local District Court Kempten, HRB Nr. / Trade Register No. 6195

The information contained in this document is CONFIDENTIAL and property of Kontron.
Any unauthorized review, use, disclosure or distribution is prohibited without express
written consent of Kontron. If you are not the intended recipient, please contact the
sender and destroy all copies of the original message and enclosed attachments.

Diese E-Mail enthält vertrauliche und/oder rechtlich geschützte Informationen und ist
Eigentum von Kontron. Die Verwendung und Weitergabe von jeglichen Inhalten ist
ohne ausdrückliche schriftliche Genehmigung von Kontron strikt untersagt. Wenn Sie
diese E-Mail irrtümlich erhalten haben, informieren Sie bitte sofort den Absender und
vernichten diese Mail und enthaltene Dokumente.

-----Ursprüngliche Nachricht-----
Von: Peter Stuge [mailto:[e-mail]]
Gesendet: Mo 5/5/2008 17:49
An: [e-mail]
Cc: Claus Gindhart
Betreff: Re: [coreboot] flashrom: Probe for up to 3 flash chips. [was: Fix ambiguity, if a board is equipped with more than one chip]

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.
> 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.
> What happens if there are multiple flash chips of the same
> technology?

If same name, then same size and at same address => larger problem.
> 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

Hi Peter,

i've tested the patch, it worked; i've acked-by the attached patch.

This was my screen output
------------8-<----------
Multiple flash chips were detected: SST49LF008A M25P16@ICH9
Please specify which chip to use with the -c <chipname> option.
----------8-<----------

About the question "Multiple flash chips of the same technology": In my opinion, this is a problem, which has to be solved in a different way, even in regards, how this could be presented to the user. One possible approach could be the following:

If we have multiple chips of the same type, all will be detected by the same probe-function. If we know, that this technology can support up to n identical chips, we could add n lines to flashchips.c, with the device names
   "chipxxx#1"
   "chipxxx#2"
   ...
   "chipxxx#n"
It's up to the probe function then, to handle this; the flashrom.c would need to be changed.

But for the moment i would recommend to wait, until such a board appears on somebody's desk.