Change in flashrom[master]: chipset_enable.c: Disable SPI on ICH7 if booted from LPC

[Angel Pons (Code Review)]

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/40401 )

Change subject: chipset_enable.c: Disable SPI on ICH7 if booted from LPC
......................................................................

chipset_enable.c: Disable SPI on ICH7 if booted from LPC

Commit 92d6a86 ("Refactor Intel Chipset Enables") eliminated a check
to disable SPI when ICH7 has booted from LPC, as the hardware does not
support it. Therefore, when flashrom probes the SPI bus, it times out
waiting for the hardware to react, for each and every SPI flash chip.
This results in very long delays and countless instances of the error:

Error: SCIP never cleared!

To prevent this, bring back part of the lost check. Probing for LPC and
FWH when booted from SPI does not seem to cause any problems on desktop
mainboards with ICH7, so don't disable LPC nor FWH if that is the case.

UNTESTED.

Change-Id: I5e59e66a2dd16b07f2dca410997fce38ab9c8fd1
Signed-off-by: Angel Pons 
---
M chipset_enable.c
1 file changed, 8 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/01/40401/1

diff --git a/chipset_enable.c b/chipset_enable.c
index 36d0dbe..783bb94 100644
--- a/chipset_enable.c
+++ b/chipset_enable.c
@@ -757,6 +757,14 @@
    if (ret_fwh == ERROR_FATAL)
    	return ret_fwh;

+	/*
+	 * It seems that the ICH7 does not support SPI and LPC chips at the same time. When booted
+	 * from LPC, the SCIP bit will never clear, which causes long delays and many error messages.
+	 * To avoid this, we will not enable SPI on ICH7 when the southbridge is strapped to LPC.
+	 */
+	if (ich_generation == CHIPSET_ICH7 && (boot_buses & BUS_LPC))
+		return 0;
+
    /* SPIBAR is at RCRB+0x3020 for ICH[78], Tunnel Creek and Centerton, and RCRB+0x3800 for ICH9. */
    uint16_t spibar_offset;
    switch (ich_generation) {

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40401 )

Change subject: chipset_enable.c: Disable SPI on ICH7 if booted from LPC
......................................................................

Patch Set 1:

getting : 
-D'HAVE_UTSNAME=1' -D'HAVE_CLOCK_GETTIME=1' -D'FLASHROM_VERSION="v1.2-26-ga055d53"' -o libflashrom.o -c libflashrom.c
libflashrom.c: In function 'flashrom_supported_boards':
libflashrom.c:191:32: error: implicit conversion from 'const enum test_state' to 'enum flashrom_test_state' [-Werror=enum-conversion]
  191 |    supported_boards[i].working = binfo[i].working;
      |                                ^
libflashrom.c: In function 'flashrom_supported_chipsets':
libflashrom.c:229:33: error: implicit conversion from 'const enum test_state' to 'enum flashrom_test_state' [-Werror=enum-conversion]
  229 |    supported_chipsets[i].status = chipset[i].status;
      |                                 ^
cc1: all warnings being treated as errors
make: *** [Makefile:1194: libflashrom.o] Error 1

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40401 )

Change subject: chipset_enable.c: Disable SPI on ICH7 if booted from LPC
......................................................................

Patch Set 1:
> Patch Set 1:
>
> getting : 
> -D'HAVE_UTSNAME=1' -D'HAVE_CLOCK_GETTIME=1' -D'FLASHROM_VERSION="v1.2-26-ga055d53"' -o libflashrom.o -c libflashrom.c
> libflashrom.c: In function 'flashrom_supported_boards':
> libflashrom.c:191:32: error: implicit conversion from 'const enum test_state' to 'enum flashrom_test_state' [-Werror=enum-conversion]
>   191 |    supported_boards[i].working = binfo[i].working;
>       |                                ^
> libflashrom.c: In function 'flashrom_supported_chipsets':
> libflashrom.c:229:33: error: implicit conversion from 'const enum test_state' to 'enum flashrom_test_state' [-Werror=enum-conversion]
>   229 |    supported_chipsets[i].status = chipset[i].status;
>       |                                 ^
> cc1: all warnings being treated as errors
> make: *** [Makefile:1194: libflashrom.o] Error 1

using: "make CONFIG_ENABLE_LIBUSB1_PROGRAMMERS=no"

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40401 )

Change subject: chipset_enable.c: Disable SPI on ICH7 if booted from LPC
......................................................................

Patch Set 1:
> Patch Set 1:
> > Patch Set 1:
> >
> > getting : 
> > -D'HAVE_UTSNAME=1' -D'HAVE_CLOCK_GETTIME=1' -D'FLASHROM_VERSION="v1.2-26-ga055d53"' -o libflashrom.o -c libflashrom.c
> > libflashrom.c: In function 'flashrom_supported_boards':
> > libflashrom.c:191:32: error: implicit conversion from 'const enum test_state' to 'enum flashrom_test_state' [-Werror=enum-conversion]
> >   191 |    supported_boards[i].working = binfo[i].working;
> >       |                                ^
> > libflashrom.c: In function 'flashrom_supported_chipsets':
> > libflashrom.c:229:33: error: implicit conversion from 'const enum test_state' to 'enum flashrom_test_state' [-Werror=enum-conversion]
> >   229 |    supported_chipsets[i].status = chipset[i].status;
> >       |                                 ^
> > cc1: all warnings being treated as errors
> > make: *** [Makefile:1194: libflashrom.o] Error 1
>
> using: "make CONFIG_ENABLE_LIBUSB1_PROGRAMMERS=no"

oops sorry I'm using gcc-10 :)

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40401 )

Change subject: chipset_enable.c: Disable SPI on ICH7 if booted from LPC
......................................................................

Patch Set 1: Code-Review+2

flashrom# time ./flashrom -p internal 
flashrom v1.2-26-ga055d53 on Linux 5.4.0-4-amd64 (x86_64)
flashrom is free software, get the source code at https://flashrom.org

Using clock_gettime for delay loops (clk_id: 1, resolution: 1ns).
Found chipset "Intel ICH7/ICH7R".
Enabling flash write... OK.
Found SST flash chip "SST49LF004A/B" (512 kB, FWH) mapped at physical address 0x00000000fff80000.
No operations were specified.

real	0m0,013s
user	0m0,000s
sys	0m0,013s

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40401 )

Change subject: chipset_enable.c: Disable SPI on ICH7 if booted from LPC
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/flashrom/+/40401/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/40401/1//COMMIT_MSG@21 
PS1, Line 21: UNTESTED.
tested :
flashrom# time ./flashrom -p internal 
flashrom v1.2-26-ga055d53 on Linux 5.4.0-4-amd64 (x86_64)
flashrom is free software, get the source code at https://flashrom.org

Using clock_gettime for delay loops (clk_id: 1, resolution: 1ns).
Found chipset "Intel ICH7/ICH7R".
Enabling flash write... OK.
Found SST flash chip "SST49LF004A/B" (512 kB, FWH) mapped at physical address 0x00000000fff80000.
No operations were specified.

real	0m0,013s
user	0m0,000s
sys	0m0,013s

[Angel Pons (Code Review)]

Hello build bot (Jenkins), Nico Huber, HAOUAS Elyes,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/flashrom/+/40401

to look at the new patch set (#2).

Change subject: chipset_enable.c: Disable SPI on ICH7 if booted from LPC
......................................................................

chipset_enable.c: Disable SPI on ICH7 if booted from LPC

Commit 92d6a86 ("Refactor Intel Chipset Enables") eliminated a check
to disable SPI when ICH7 has booted from LPC, as the hardware does not
support it. Therefore, when flashrom probes the SPI bus, it times out
waiting for the hardware to react, for each and every SPI flash chip.
This results in very long delays and countless instances of the error:

Error: SCIP never cleared!

To prevent this, bring back part of the lost check. Probing for LPC and
FWH when booted from SPI does not seem to cause any problems on desktop
mainboards with ICH7, so don't disable LPC nor FWH if that is the case.

Tested on ECS 945G-M4 (ICH7, boots from LPC), works without errors.

Change-Id: I5e59e66a2dd16b07f2dca410997fce38ab9c8fd1
Signed-off-by: Angel Pons 
---
M chipset_enable.c
1 file changed, 8 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/01/40401/2

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/40401 )

Change subject: chipset_enable.c: Disable SPI on ICH7 if booted from LPC
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/flashrom/+/40401/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/40401/1//COMMIT_MSG@21 
PS1, Line 21: UNTESTED.
> tested : […]

Thanks!