Change in ...coreboot[master]: util/inteltool: Error if first channel is not found

[Jacob Garber (Code Review)]

Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32713

Change subject: util/inteltool: Error if first channel is not found
......................................................................

util/inteltool: Error if first channel is not found

The first channel is needed to initialize the timing
arrays. If it is not present there will be undefined
reads in the 'print_time' function calls starting on
line 182.

Found-by: Coverity Scan #1370{584,585,588,589,590-596,600}
Signed-off-by: Jacob Garber 
Change-Id: I6b59989242e498474782876302e0850e3e4cf2d3
---
M util/inteltool/ivy_memory.c
1 file changed, 7 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/32713/1

diff --git a/util/inteltool/ivy_memory.c b/util/inteltool/ivy_memory.c
index 8ce8611..c079071 100644
--- a/util/inteltool/ivy_memory.c
+++ b/util/inteltool/ivy_memory.c
@@ -97,7 +97,13 @@
    	rankmap[channel] = read_mchbar32(0xc14 + 0x100 * channel) >> 24;
    }

-	two_channels = rankmap[0] && rankmap[1];
+	// The second channel is optional, but the first channel is not
+	if (rankmap[0] == 0) {
+		fputs("Error: first channel not found\n", stderr);
+		exit(1);
+	}
+
+	two_channels = rankmap[1];

mr0[0] = read_mchbar32(0x0004);
    mr1[0] = read_mchbar32(0x0008);

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32713 )

Change subject: util/inteltool: Error if first channel is not found
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/32713/1/util/inteltool/ivy_memory.c 
File util/inteltool/ivy_memory.c:

https://review.coreboot.org/#/c/32713/1/util/inteltool/ivy_memory.c@100 
PS1, Line 100: 	// The second channel is optional, but the first channel is not
that's not true.

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32713 )

Change subject: util/inteltool: Error if first channel is not found
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/#/c/32713/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32713/1//COMMIT_MSG@7 
PS1, Line 7: util/inteltool: Error if first channel is not found
Please make this a statement by adding a verb (in imperative mood).
> Error out if …
> Print error …

https://review.coreboot.org/#/c/32713/1//COMMIT_MSG@9 
PS1, Line 9: The first channel is needed to initialize the timing
           : arrays. If it is not present there will be undefined
           : reads in the 'print_time' function calls starting on
           : line 182.
Please use the fully allowed text width.

[Jacob Garber (Code Review)]

Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32713 )

Change subject: util/inteltool: Error if first channel is not found
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/32713/1/util/inteltool/ivy_memory.c 
File util/inteltool/ivy_memory.c:

https://review.coreboot.org/#/c/32713/1/util/inteltool/ivy_memory.c@100 
PS1, Line 100: 	// The second channel is optional, but the first channel is not
> that's not true.

Are they both optional then?

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32713 )

Change subject: util/inteltool: Error if first channel is not found
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/32713/1/util/inteltool/ivy_memory.c 
File util/inteltool/ivy_memory.c:

https://review.coreboot.org/#/c/32713/1/util/inteltool/ivy_memory.c@100 
PS1, Line 100: 	// The second channel is optional, but the first channel is not
> Are they both optional then?

No only one, doesn't matter which

Change in ...coreboot[master]: util/inteltool: Use appropriate channel for printing timings

[Jacob Garber (Code Review)]

Hello David Hendricks, Stefan Reinauer, build bot (Jenkins), Patrick Georgi, Martin Roth,

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

https://review.coreboot.org/c/coreboot/+/32713

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

Change subject: util/inteltool: Use appropriate channel for printing timings
......................................................................

util/inteltool: Use appropriate channel for printing timings

At least one channel must be present, so print an error if there is
not. However, we cannot always assume it will be the first channel,
so make the appropriate selection when printing the timings.

Found-by: Coverity Scan #1370{584,585,588,589,590-596,600}
Signed-off-by: Jacob Garber 
Change-Id: I6b59989242e498474782876302e0850e3e4cf2d3
---
M util/inteltool/ivy_memory.c
1 file changed, 20 insertions(+), 12 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/32713/2

Change in ...coreboot[master]: util/inteltool: Use appropriate channel for printing timings

[Jacob Garber (Code Review)]

Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32713 )

Change subject: util/inteltool: Use appropriate channel for printing timings
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/#/c/32713/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32713/1//COMMIT_MSG@7 
PS1, Line 7: util/inteltool: Error if first channel is not found
> Please make this a statement by adding a verb (in imperative mood). […]

Done

https://review.coreboot.org/#/c/32713/1//COMMIT_MSG@9 
PS1, Line 9: The first channel is needed to initialize the timing
           : arrays. If it is not present there will be undefined
           : reads in the 'print_time' function calls starting on
           : line 182.
> Please use the fully allowed text width.

Done

Change in ...coreboot[master]: util/inteltool: Use appropriate channel for printing timings

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32713 )

Change subject: util/inteltool: Use appropriate channel for printing timings
......................................................................

Patch Set 2: Code-Review+2

Change in ...coreboot[master]: util/inteltool: Use appropriate channel for printing timings

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32713 )

Change subject: util/inteltool: Use appropriate channel for printing timings
......................................................................

util/inteltool: Use appropriate channel for printing timings

At least one channel must be present, so print an error if there is
not. However, we cannot always assume it will be the first channel,
so make the appropriate selection when printing the timings.

Found-by: Coverity Scan #1370{584,585,588,589,590-596,600}
Signed-off-by: Jacob Garber 
Change-Id: I6b59989242e498474782876302e0850e3e4cf2d3
Reviewed-on: https://review.coreboot.org/c/coreboot/+/32713
Tested-by: build bot (Jenkins) 
Reviewed-by: Patrick Rudolph 
---
M util/inteltool/ivy_memory.c
1 file changed, 20 insertions(+), 12 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Patrick Rudolph: Looks good to me, approved

diff --git a/util/inteltool/ivy_memory.c b/util/inteltool/ivy_memory.c
index 8ce8611..16c1d89 100644
--- a/util/inteltool/ivy_memory.c
+++ b/util/inteltool/ivy_memory.c
@@ -97,6 +97,11 @@
    	rankmap[channel] = read_mchbar32(0xc14 + 0x100 * channel) >> 24;
    }

+	if ((rankmap[0] == 0) && (rankmap[1] == 0)) {
+		fputs("Error: no memory channels found\n", stderr);
+		exit(1);
+	}
+
    two_channels = rankmap[0] && rankmap[1];

mr0[0] = read_mchbar32(0x0004);
@@ -172,49 +177,52 @@

printf(".reg_4004_b30 = { %d, %d },\n", reg_4004_b30[0],
           reg_4004_b30[1]);
+
+	channel = (rankmap[0] != 0) ? 0 : 1;
+
    if (two_channels && tFAW[0] != tFAW[1])
    	printf("/* tFAW mismatch: %d, %d */\n", tFAW[0], tFAW[1]);
-	print_time("tFAW", tFAW[0], tCK);
+	print_time("tFAW", tFAW[channel], tCK);
    if (two_channels && tWTR[0] != tWTR[1])
    	printf("/* tWTR mismatch: %d, %d */\n", tWTR[0], tWTR[1]);
-	print_time("tWTR", tWTR[0], tCK);
+	print_time("tWTR", tWTR[channel], tCK);
    if (two_channels && tCKE[0] != tCKE[1])
    	printf("/* tCKE mismatch: %d, %d */\n", tCKE[0], tCKE[1]);
-	print_time("tCKE", tCKE[0], tCK);
+	print_time("tCKE", tCKE[channel], tCK);
    if (two_channels && tRTP[0] != tRTP[1])
    	printf("/* tRTP mismatch: %d, %d */\n", tRTP[0], tRTP[1]);
-	print_time("tRTP", tRTP[0], tCK);
+	print_time("tRTP", tRTP[channel], tCK);
    if (two_channels && tRRD[0] != tRRD[1])
    	printf("/* tRRD mismatch: %d, %d */\n", tRRD[0], tRRD[1]);
-	print_time("tRRD", tRRD[0], tCK);
+	print_time("tRRD", tRRD[channel], tCK);

if (two_channels && tRAS[0] != tRAS[1])
    	printf("/* tRAS mismatch: %d, %d */\n", tRAS[0], tRAS[1]);
-	print_time("tRAS", tRAS[0], tCK);
+	print_time("tRAS", tRAS[channel], tCK);

if (two_channels && tCWL[0] != tCWL[1])
    	printf("/* tCWL mismatch: %d, %d */\n", tCWL[0], tCWL[1]);
-	print_time("tCWL", tCWL[0], tCK);
+	print_time("tCWL", tCWL[channel], tCK);

if (two_channels && tRP[0] != tRP[1])
    	printf("/* tRP mismatch: %d, %d */\n", tRP[0], tRP[1]);
-	print_time("tRP", tRP[0], tCK);
+	print_time("tRP", tRP[channel], tCK);

if (two_channels && tRCD[0] != tRCD[1])
    	printf("/* tRCD mismatch: %d, %d */\n", tRCD[0], tRCD[1]);
-	print_time("tRCD", tRCD[0], tCK);
+	print_time("tRCD", tRCD[channel], tCK);

if (two_channels && tXPDLL[0] != tXPDLL[1])
    	printf("/* tXPDLL mismatch: %d, %d */\n", tXPDLL[0], tXPDLL[1]);
-	print_time("tXPDLL", tXPDLL[0], tCK);
+	print_time("tXPDLL", tXPDLL[channel], tCK);

if (two_channels && tXP[0] != tXP[1])
    	printf("/* tXP mismatch: %d, %d */\n", tXP[0], tXP[1]);
-	print_time("tXP", tXP[0], tCK);
+	print_time("tXP", tXP[channel], tCK);

if (two_channels && tAONPD[0] != tAONPD[1])
    	printf("/* tAONPD mismatch: %d, %d */\n", tAONPD[0], tAONPD[1]);
-	print_time("tAONPD", tAONPD[0], tCK);
+	print_time("tAONPD", tAONPD[channel], tCK);

reg = read_mchbar32(0x4298);
    if (reg != read_mchbar32(0x4698) && two_channels)