Change in flashrom[master]: allow 0x34 as ICCRIBA for CHIPSET_C620_SERIES_LEWISBURG

[Jonathan Zhang (Code Review)]

Jonathan Zhang has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/44621 )

Change subject: allow 0x34 as ICCRIBA for CHIPSET_C620_SERIES_LEWISBURG
......................................................................

allow 0x34 as ICCRIBA for CHIPSET_C620_SERIES_LEWISBURG

Intel C621A Lewisburg PCH belongs to C620 series, it has 0x34 as ICCRIBA.

Fix guess_ich_chipset_from_content() accordingly.

Print error status info for read_ich_descriptors_from_dump().

Change-Id: I363aaccfb90e0a127c0f0bb0072e9e85c210b669
Signed-off-by: Jonathan Zhang 
---
M ich_descriptors.c
1 file changed, 5 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/21/44621/1

diff --git a/ich_descriptors.c b/ich_descriptors.c
index 120d3fe..cd8e13a 100644
--- a/ich_descriptors.c
+++ b/ich_descriptors.c
@@ -932,7 +932,7 @@
    		return CHIPSET_8_SERIES_LYNX_POINT;
    	msg_pwarn("Peculiar firmware descriptor, assuming Wildcat Point compatibility.\n");
    	return CHIPSET_9_SERIES_WILDCAT_POINT;
-	} else if (content->ICCRIBA < 0x34) {
+	} else if (content->ICCRIBA <= 0x34) {
    	if (content->NM == 6)
    		return CHIPSET_C620_SERIES_LEWISBURG;
    	else
@@ -1243,8 +1243,11 @@

struct ich_descriptors desc;
    enum ich_chipset cs = CHIPSET_ICH_UNKNOWN;
-	if (read_ich_descriptors_from_dump(dump, len, &cs, &desc))
+	int ret = read_ich_descriptors_from_dump(dump, len, &cs, &desc);
+	if (ret) {
+		msg_cerr("__FUNC__, __LINE__ Failed with value %d.\n", ret);
    	return 1;
+	}

memset(layout, 0x00, sizeof(*layout));

[Angel Pons (Code Review)]

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

Change subject: allow 0x34 as ICCRIBA for CHIPSET_C620_SERIES_LEWISBURG
......................................................................

Patch Set 1: Code-Review+1

(2 comments)

https://review.coreboot.org/c/flashrom/+/44621/1/ich_descriptors.c 
File ich_descriptors.c:

https://review.coreboot.org/c/flashrom/+/44621/1/ich_descriptors.c@1248 
PS1, Line 1248: "__FUNC__, __LINE__ Failed with value %d.\n", ret
I don't think we're supposed to print anything in here, but let the callers handle errors instead. Plus, it could happen that a descriptor isn't present, which isn't an error.

In any case, this will print `__FUNC__, __LINE__` literally. If anything, I'd print something more user-friendly instead of function and line numbers.

https://review.coreboot.org/c/flashrom/+/44621/1/ich_descriptors.c@1248 
PS1, Line 1248: msg_cerr
msg_perr

[Jonathan Zhang (Code Review)]

Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/44621 )

Change subject: allow 0x34 as ICCRIBA for CHIPSET_C620_SERIES_LEWISBURG
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/flashrom/+/44621/1/ich_descriptors.c 
File ich_descriptors.c:

https://review.coreboot.org/c/flashrom/+/44621/1/ich_descriptors.c@1248 
PS1, Line 1248: msg_cerr
> msg_perr

Done

https://review.coreboot.org/c/flashrom/+/44621/1/ich_descriptors.c@1248 
PS1, Line 1248: "__FUNC__, __LINE__ Failed with value %d.\n", ret
> I don't think we're supposed to print anything in here, but let the callers handle errors instead. […]

The problem is that when read_ich_descriptors_from_dump() returns non-successful status, layout_from_ich_descriptors() just return 1.

In a failure scenario, what the user gets:
Reading ich descriptor... done.
Couldn't parse the descriptor!

With adding of this print, the user gets:
Reading ich descriptor... done.
layout_from_ich_descriptors():1249, returned with value -4.
Couldn't parse the descriptor!

[Jonathan Zhang (Code Review)]

Hello build bot (Jenkins), Nico Huber, Maxim Polyakov, Johnny Lin, David Hendricks, Angel Pons, Bryant Ou,

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

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

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

Change subject: allow 0x34 as ICCRIBA for CHIPSET_C620_SERIES_LEWISBURG
......................................................................

allow 0x34 as ICCRIBA for CHIPSET_C620_SERIES_LEWISBURG

Intel C621A Lewisburg PCH belongs to C620 series, it has 0x34 as ICCRIBA.

Fix guess_ich_chipset_from_content() accordingly.

Print status info for read_ich_descriptors_from_dump().

TESTED=run flashrom successfully from OCP Yosemite V3 DeltaLake server.

Change-Id: I363aaccfb90e0a127c0f0bb0072e9e85c210b669
Signed-off-by: Jonathan Zhang 
---
M ich_descriptors.c
1 file changed, 6 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/21/44621/2

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/44621 )

Change subject: allow 0x34 as ICCRIBA for CHIPSET_C620_SERIES_LEWISBURG
......................................................................

Patch Set 2:

(1 comment)

I guess Intel updated the chipset without changing the marketing
name (C620 series). You should check the whole ich_descriptors
code if it applies correctly to your descriptor. If it does,
CHIPSET_C620_SERIES_LEWISBURG is the right choice, but if
it doesn't, we should add a new name. In any case, the detection
needs to distinct more accurately between your chipset and
300 series.

https://review.coreboot.org/c/flashrom/+/44621/2/ich_descriptors.c 
File ich_descriptors.c:

https://review.coreboot.org/c/flashrom/+/44621/2/ich_descriptors.c@a943 
PS2, Line 943: 		return CHIPSET_300_SERIES_CANNON_POINT;
The default path for 0x34 (without warning) leads here. If you change that,
300-series detection will be broken.

[Jonathan Zhang (Code Review)]

Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/44621 )

Change subject: allow 0x34 as ICCRIBA for CHIPSET_C620_SERIES_LEWISBURG
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/flashrom/+/44621/2/ich_descriptors.c 
File ich_descriptors.c:

https://review.coreboot.org/c/flashrom/+/44621/2/ich_descriptors.c@a943 
PS2, Line 943: 		return CHIPSET_300_SERIES_CANNON_POINT;
> The default path for 0x34 (without warning) leads here. If you change that, […]

Unfortunately I have not found any documentation related to ICCRIBA.
On DeltaLake DVT server, ICCRIBA is 0x34. When it uses original default (CHIPSET_300_SERIES_CANNON_POINT), flashrom fails, not able to parse the descriptor. But when CHIPSET_C620_SERIES_LEWISBURG is chosen, it works well.

I wonder if any PCH of CHIPSET_300_SERIES_CANNON_POINT has ICCRIBA as 0x34.

[Angel Pons (Code Review)]

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

Change subject: allow 0x34 as ICCRIBA for CHIPSET_C620_SERIES_LEWISBURG
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/flashrom/+/44621/2/ich_descriptors.c 
File ich_descriptors.c:

https://review.coreboot.org/c/flashrom/+/44621/2/ich_descriptors.c@a943 
PS2, Line 943: 		return CHIPSET_300_SERIES_CANNON_POINT;
> Unfortunately I have not found any documentation related to ICCRIBA. […]

Something that could help is checking the number of PCH straps. It should be different between Cannon Point and Lewisburg (or whatever the new server PCHs are called)

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/44621 )

Change subject: allow 0x34 as ICCRIBA for CHIPSET_C620_SERIES_LEWISBURG
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/flashrom/+/44621/2/ich_descriptors.c 
File ich_descriptors.c:

https://review.coreboot.org/c/flashrom/+/44621/2/ich_descriptors.c@a943 
PS2, Line 943: 		return CHIPSET_300_SERIES_CANNON_POINT;
> Unfortunately I have not found any documentation related to ICCRIBA.

It used to give some offset, but newer SPI guides just list it as reserved
or reserved with a specific value.
> I wonder if any PCH of CHIPSET_300_SERIES_CANNON_POINT has ICCRIBA as 0x34.

I guess all of them. The SPI guides say "Reserved. Set this field to 34h.".

It seems the original C620 models and Sunrise Point PCHs had the same issue,
hence the check for `NM` above. Maybe that's all we need on this path
too?

[Jonathan Zhang (Code Review)]

Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/44621 )

Change subject: allow 0x34 as ICCRIBA for CHIPSET_C620_SERIES_LEWISBURG
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/flashrom/+/44621/2/ich_descriptors.c 
File ich_descriptors.c:

https://review.coreboot.org/c/flashrom/+/44621/2/ich_descriptors.c@a943 
PS2, Line 943: 		return CHIPSET_300_SERIES_CANNON_POINT;
> > Unfortunately I have not found any documentation related to ICCRIBA. […]

The C621A PCH (used in DeltaLake DVT server) has ICCRIBA as 0x34 and NM as 6. With this code change, flashrom guessed it correctly as CHIPSET_C620_SERIES_LEWISBURG.
If CHIPSET_300_SERIES_CANNON_POINT has 0x34 but NM is never 6, I will use "NM == 6" test to differentiate between CHIPSET_300_SERIES_CANNON_POINT and CHIPSET_C620_SERIES_LEWISBURG..

[Jonathan Zhang (Code Review)]

Hello build bot (Jenkins), Nico Huber, Maxim Polyakov, Johnny Lin, David Hendricks, Angel Pons, Bryant Ou,

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

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

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

Change subject: allow 0x34 as ICCRIBA for CHIPSET_C620_SERIES_LEWISBURG
......................................................................

allow 0x34 as ICCRIBA for CHIPSET_C620_SERIES_LEWISBURG

Intel C621A Lewisburg PCH belongs to C620 series, it has 0x34 as ICCRIBA.

Fix guess_ich_chipset_from_content() accordingly.

Print status info for read_ich_descriptors_from_dump().

TESTED=run flashrom successfully from OCP Yosemite V3 DeltaLake server.

Change-Id: I363aaccfb90e0a127c0f0bb0072e9e85c210b669
Signed-off-by: Jonathan Zhang 
---
M ich_descriptors.c
1 file changed, 12 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/21/44621/3

[Angel Pons (Code Review)]

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

Change subject: allow 0x34 as ICCRIBA for CHIPSET_C620_SERIES_LEWISBURG
......................................................................

Patch Set 4: Code-Review+2

(1 comment)

https://review.coreboot.org/c/flashrom/+/44621/4//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/44621/4//COMMIT_MSG@13 
PS4, Line 13: Print status info for read_ich_descriptors_from_dump().
nit: remove?

[Jonathan Zhang (Code Review)]

Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/44621 )

Change subject: allow 0x34 as ICCRIBA for CHIPSET_C620_SERIES_LEWISBURG
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/flashrom/+/44621/4//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/44621/4//COMMIT_MSG@13 
PS4, Line 13: Print status info for read_ich_descriptors_from_dump().
> nit: remove?

I did add this code change in layout_from_ich_descriptors(). Without this change, it does not give any hint upon failure. With this change, read_ich_descriptors_from_dump() status info is printed upon failure.

[Jonathan Zhang (Code Review)]

Hello build bot (Jenkins), Nico Huber, Maxim Polyakov, Johnny Lin, David Hendricks, Angel Pons, Bryant Ou,

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

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

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

Change subject: allow 0x34 as ICCRIBA for CHIPSET_C620_SERIES_LEWISBURG
......................................................................

allow 0x34 as ICCRIBA for CHIPSET_C620_SERIES_LEWISBURG

Intel C621A Lewisburg PCH belongs to C620 series, it has 0x34 as ICCRIBA.

Fix guess_ich_chipset_from_content() accordingly.

Print status info of read_ich_descriptors_from_dump() to facilitate
debugging upon failure.

TESTED=run flashrom successfully from OCP Yosemite V3 DeltaLake server.

Change-Id: I363aaccfb90e0a127c0f0bb0072e9e85c210b669
Signed-off-by: Jonathan Zhang 
---
M ich_descriptors.c
1 file changed, 12 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/21/44621/5

[Jonathan Zhang (Code Review)]

Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/44621 )

Change subject: allow 0x34 as ICCRIBA for CHIPSET_C620_SERIES_LEWISBURG
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/flashrom/+/44621/5/ich_descriptors.c 
File ich_descriptors.c:

https://review.coreboot.org/c/flashrom/+/44621/5/ich_descriptors.c@940 
PS5, Line 940: 	} else if (content->ICCRIBA == 0x34) {
Some feedback from Intel: Based on the PCH EDS 3.0 and the information you provided, the ICCRIBA should be the ICC initialization register ( the ICC stands for Intel Microcode technology).
Due to the different SKUs of PCH parts used on DeltaLake (EVT) and (DVT), the initialization base address of the Microprocessor register is assigned by PCH to different offset address as the 4-KB boundary rules must meet, if the Program Register type. ( section 27.7.1.4.2, page 1914). It turns out that the 0x34 is the value of the Initial Base Address (offset) of the Microprocess register for A1CB QS or PRQ C621A DVT part.

[Angel Pons (Code Review)]

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

Change subject: allow 0x34 as ICCRIBA for CHIPSET_C620_SERIES_LEWISBURG
......................................................................

Patch Set 5: Code-Review+1

(1 comment)

https://review.coreboot.org/c/flashrom/+/44621/5/ich_descriptors.c 
File ich_descriptors.c:

https://review.coreboot.org/c/flashrom/+/44621/5/ich_descriptors.c@945 
PS5, Line 945: // content->ICCRIBA > 0x34
I don't think this comment adds any value

[Jonathan Zhang (Code Review)]

Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/44621 )

Change subject: allow 0x34 as ICCRIBA for CHIPSET_C620_SERIES_LEWISBURG
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/flashrom/+/44621/5/ich_descriptors.c 
File ich_descriptors.c:

https://review.coreboot.org/c/flashrom/+/44621/5/ich_descriptors.c@945 
PS5, Line 945: // content->ICCRIBA > 0x34
> I don't think this comment adds any value

Done

[Jonathan Zhang (Code Review)]

Hello build bot (Jenkins), Nico Huber, Maxim Polyakov, Johnny Lin, David Hendricks, Angel Pons, Bryant Ou,

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

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

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

Change subject: allow 0x34 as ICCRIBA for CHIPSET_C620_SERIES_LEWISBURG
......................................................................

allow 0x34 as ICCRIBA for CHIPSET_C620_SERIES_LEWISBURG

Intel C621A Lewisburg PCH belongs to C620 series, it has 0x34 as ICCRIBA.

Fix guess_ich_chipset_from_content() accordingly.

Print status info of read_ich_descriptors_from_dump() to facilitate
debugging upon failure.

TESTED=run flashrom successfully from OCP Yosemite V3 DeltaLake server.

Change-Id: I363aaccfb90e0a127c0f0bb0072e9e85c210b669
Signed-off-by: Jonathan Zhang 
---
M ich_descriptors.c
1 file changed, 11 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/21/44621/6

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/44621 )

Change subject: allow 0x34 as ICCRIBA for CHIPSET_C620_SERIES_LEWISBURG
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/flashrom/+/44621/6/ich_descriptors.c 
File ich_descriptors.c:

https://review.coreboot.org/c/flashrom/+/44621/6/ich_descriptors.c@1253 
PS6, Line 1253: 			__func__, __LINE__, ret);
This looks like a debug message and nothing that would be useful to a user. So the
message level should be dbg or maybe even dbg2.

Or maybe a user-readable message in the caller would be better?

[Jonathan Zhang (Code Review)]

Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/44621 )

Change subject: allow 0x34 as ICCRIBA for CHIPSET_C620_SERIES_LEWISBURG
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/flashrom/+/44621/6/ich_descriptors.c 
File ich_descriptors.c:

https://review.coreboot.org/c/flashrom/+/44621/6/ich_descriptors.c@1253 
PS6, Line 1253: 			__func__, __LINE__, ret);
> This looks like a debug message and nothing that would be useful to a user. So the […]

Done

[Jonathan Zhang (Code Review)]

Hello build bot (Jenkins), Nico Huber, Maxim Polyakov, Johnny Lin, David Hendricks, Angel Pons, Bryant Ou,

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

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

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

Change subject: allow 0x34 as ICCRIBA for CHIPSET_C620_SERIES_LEWISBURG
......................................................................

allow 0x34 as ICCRIBA for CHIPSET_C620_SERIES_LEWISBURG

Intel C621A Lewisburg PCH belongs to C620 series, it has 0x34 as ICCRIBA.

Fix guess_ich_chipset_from_content() accordingly.

Print status info of read_ich_descriptors_from_dump() to facilitate
debugging upon failure.

TESTED=run flashrom successfully from OCP Yosemite V3 DeltaLake server.

Change-Id: I363aaccfb90e0a127c0f0bb0072e9e85c210b669
Signed-off-by: Jonathan Zhang 
---
M ich_descriptors.c
1 file changed, 11 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/21/44621/7

[Angel Pons (Code Review)]

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

Change subject: allow 0x34 as ICCRIBA for CHIPSET_C620_SERIES_LEWISBURG
......................................................................

Patch Set 7: Code-Review+2

(3 comments)

https://review.coreboot.org/c/flashrom/+/44621/4//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/44621/4//COMMIT_MSG@13 
PS4, Line 13: Print status info for read_ich_descriptors_from_dump().
> I did add this code change in layout_from_ich_descriptors(). […]

Ack

https://review.coreboot.org/c/flashrom/+/44621/1/ich_descriptors.c 
File ich_descriptors.c:

https://review.coreboot.org/c/flashrom/+/44621/1/ich_descriptors.c@1248 
PS1, Line 1248: "__FUNC__, __LINE__ Failed with value %d.\n", ret
> The problem is that when read_ich_descriptors_from_dump() returns non-successful status, layout_from […]

Ack

https://review.coreboot.org/c/flashrom/+/44621/2/ich_descriptors.c 
File ich_descriptors.c:

https://review.coreboot.org/c/flashrom/+/44621/2/ich_descriptors.c@a943 
PS2, Line 943: 		return CHIPSET_300_SERIES_CANNON_POINT;
> The C621A PCH (used in DeltaLake DVT server) has ICCRIBA as 0x34 and NM as 6. […]

Done

[David Hendricks (Code Review)]

David Hendricks has submitted this change. ( https://review.coreboot.org/c/flashrom/+/44621 )

Change subject: allow 0x34 as ICCRIBA for CHIPSET_C620_SERIES_LEWISBURG
......................................................................

allow 0x34 as ICCRIBA for CHIPSET_C620_SERIES_LEWISBURG

Intel C621A Lewisburg PCH belongs to C620 series, it has 0x34 as ICCRIBA.

Fix guess_ich_chipset_from_content() accordingly.

Print status info of read_ich_descriptors_from_dump() to facilitate
debugging upon failure.

TESTED=run flashrom successfully from OCP Yosemite V3 DeltaLake server.

Change-Id: I363aaccfb90e0a127c0f0bb0072e9e85c210b669
Signed-off-by: Jonathan Zhang 
Reviewed-on: https://review.coreboot.org/c/flashrom/+/44621
Tested-by: build bot (Jenkins) 
Reviewed-by: Angel Pons 
---
M ich_descriptors.c
1 file changed, 11 insertions(+), 3 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Angel Pons: Looks good to me, approved

diff --git a/ich_descriptors.c b/ich_descriptors.c
index 120d3fe..7409a22 100644
--- a/ich_descriptors.c
+++ b/ich_descriptors.c
@@ -937,9 +937,13 @@
    		return CHIPSET_C620_SERIES_LEWISBURG;
    	else
    		return CHIPSET_100_SERIES_SUNRISE_POINT;
+	} else if (content->ICCRIBA == 0x34) {
+		if (content->NM == 6)
+			return CHIPSET_C620_SERIES_LEWISBURG;
+		else
+			return CHIPSET_300_SERIES_CANNON_POINT;
    } else {
-		if (content->ICCRIBA > 0x34)
-			msg_pwarn("Unknown firmware descriptor, assuming 300 series compatibility.\n");
+		msg_pwarn("Unknown firmware descriptor, assuming 300 series compatibility.\n");
    	return CHIPSET_300_SERIES_CANNON_POINT;
    }
 }
@@ -1243,8 +1247,12 @@

struct ich_descriptors desc;
    enum ich_chipset cs = CHIPSET_ICH_UNKNOWN;
-	if (read_ich_descriptors_from_dump(dump, len, &cs, &desc))
+	int ret = read_ich_descriptors_from_dump(dump, len, &cs, &desc);
+	if (ret) {
+		msg_pdbg("%s():%d, returned with value %d.\n",
+			__func__, __LINE__, ret);
    	return 1;
+	}

memset(layout, 0x00, sizeof(*layout));