Change in ...coreboot[master]: nb/intel/pineview: Remove dead code in switch

[Jacob Garber (Code Review)]

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

Change subject: nb/intel/pineview: Remove dead code in switch
......................................................................

nb/intel/pineview: Remove dead code in switch

This switch was likely copy-pasted from the one right above it. However,
the MEM_CLOCK_800MHz case isn't needed, since that is explicitly checked
and avoided before the while loop. With that gone, only the
667MHz/default case is left, which we don't need to switch over anymore.

Change-Id: Idfb9cc27dd8718f627d15ba92a9c74c51c2c1c2d
Signed-off-by: Jacob Garber 
Found-by: Coverity CID 1347372
---
M src/northbridge/intel/pineview/raminit.c
1 file changed, 6 insertions(+), 22 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/33407/1

diff --git a/src/northbridge/intel/pineview/raminit.c b/src/northbridge/intel/pineview/raminit.c
index 72063cb..cefa13c 100644
--- a/src/northbridge/intel/pineview/raminit.c
+++ b/src/northbridge/intel/pineview/raminit.c
@@ -498,28 +498,12 @@
    	lowcas = lsbp;
    	while (cas == 0 && highcas >= lowcas) {
    		FOR_EACH_POPULATED_DIMM(s->dimms, i) {
-				switch (freq) {
-				case MEM_CLOCK_800MHz:
-					if ((s->dimms[i].spd_data[9] > 0x25) ||
-					    (s->dimms[i].spd_data[10] > 0x40)) {
-						// CAS too fast, lower it
-						highcas--;
-						break;
-					} else {
-						cas = highcas;
-					}
-					break;
-				case MEM_CLOCK_667MHz:
-				default:
-					if ((s->dimms[i].spd_data[9] > 0x30) ||
-					    (s->dimms[i].spd_data[10] > 0x45)) {
-						// CAS too fast, lower it
-						highcas--;
-						break;
-					} else {
-						cas = highcas;
-					}
-					break;
+				if ((s->dimms[i].spd_data[9] > 0x30) ||
+				    (s->dimms[i].spd_data[10] > 0x45)) {
+					// CAS too fast, lower it
+					highcas--;
+				} else {
+					cas = highcas;
    			}
    		}
    	}

[Martin Roth (Code Review)]

Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33407 )

Change subject: nb/intel/pineview: Remove dead code in switch
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/33407/1/src/northbridge/intel/pineview/ramin... 
File src/northbridge/intel/pineview/raminit.c:

https://review.coreboot.org/#/c/33407/1/src/northbridge/intel/pineview/ramin... 
PS1, Line 490: if (freq == MEM_CLOCK_800MHz) {
Unrelated to the change, but shouldn't there be an if (freq == MEM_CLOCK_667MHz) {} here as well?

[Jacob Garber (Code Review)]

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

Change subject: nb/intel/pineview: Remove dead code in switch
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/33407/1/src/northbridge/intel/pineview/ramin... 
File src/northbridge/intel/pineview/raminit.c:

https://review.coreboot.org/#/c/33407/1/src/northbridge/intel/pineview/ramin... 
PS1, Line 490: if (freq == MEM_CLOCK_800MHz) {
> Unrelated to the change, but shouldn't there be an if (freq == MEM_CLOCK_667MHz) {} here as well?

Hmmm, if to avoid the die(), then probably yes

[HAOUAS Elyes (Code Review)]

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

Change subject: nb/intel/pineview: Remove dead code in switch
......................................................................

Patch Set 1: Code-Review+1

Change in coreboot[master]: nb/intel/pineview: Remove dead code in switch

[David Hendricks (Code Review)]

David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33407 )

Change subject: nb/intel/pineview: Remove dead code in switch
......................................................................

Patch Set 1: Code-Review+2

Looks good, but we should follow-up with Martin's suggestion.

Change in coreboot[master]: nb/intel/pineview: Remove dead code in switch

[Angel Pons (Code Review)]

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

Change subject: nb/intel/pineview: Remove dead code in switch
......................................................................

Patch Set 1: Code-Review+2

(2 comments)
> Patch Set 1: Code-Review+2
>
> Looks good, but we should follow-up with Martin's suggestion.

I think that's intended, see comment in the file.

Patch looks good though.

https://review.coreboot.org/c/coreboot/+/33407/1/src/northbridge/intel/pinev... 
File src/northbridge/intel/pineview/raminit.c:

https://review.coreboot.org/c/coreboot/+/33407/1/src/northbridge/intel/pinev... 
PS1, Line 469: break
This and the other 'break' on the following case block can be omitted (just like you did on the code you changed. Can be on a separate patch.

https://review.coreboot.org/c/coreboot/+/33407/1/src/northbridge/intel/pinev... 
PS1, Line 490: if (freq == MEM_CLOCK_800MHz) {
> Hmmm, if to avoid the die(), then probably yes

Nope.

The block before this one tries to find a value for CAS latency, at either 800 MHz or 667 MHz. If it is invalid (highcas < lowcas), it reduces the speed to 667 MHz and tries again. It is pointless to lower the speed if it is 667 MHz already, so that hits the die().

Change in coreboot[master]: nb/intel/pineview: Remove dead code in switch

[Jacob Garber (Code Review)]

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

Change subject: nb/intel/pineview: Remove dead code in switch
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/33407/1/src/northbridge/intel/pinev... 
File src/northbridge/intel/pineview/raminit.c:

https://review.coreboot.org/c/coreboot/+/33407/1/src/northbridge/intel/pinev... 
PS1, Line 469: break
> This and the other 'break' on the following case block can be omitted (just like you did on the code […]

Ack

https://review.coreboot.org/c/coreboot/+/33407/1/src/northbridge/intel/pinev... 
PS1, Line 490: if (freq == MEM_CLOCK_800MHz) {
> Nope. […]

Ok, let's leave it as-is then.

Change in coreboot[master]: nb/intel/pineview: Remove dead code in switch

[Patrick Georgi (Code Review)]

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

Change subject: nb/intel/pineview: Remove dead code in switch
......................................................................

nb/intel/pineview: Remove dead code in switch

This switch was likely copy-pasted from the one right above it. However,
the MEM_CLOCK_800MHz case isn't needed, since that is explicitly checked
and avoided before the while loop. With that gone, only the
667MHz/default case is left, which we don't need to switch over anymore.

Change-Id: Idfb9cc27dd8718f627d15ba92a9c74c51c2c1c2d
Signed-off-by: Jacob Garber 
Found-by: Coverity CID 1347372
Reviewed-on: https://review.coreboot.org/c/coreboot/+/33407
Tested-by: build bot (Jenkins) 
Reviewed-by: HAOUAS Elyes 
Reviewed-by: David Hendricks 
Reviewed-by: Angel Pons 
---
M src/northbridge/intel/pineview/raminit.c
1 file changed, 6 insertions(+), 22 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  David Hendricks: Looks good to me, approved
  HAOUAS Elyes: Looks good to me, but someone else must approve
  Angel Pons: Looks good to me, approved

diff --git a/src/northbridge/intel/pineview/raminit.c b/src/northbridge/intel/pineview/raminit.c
index 1d24ea2..5cece41 100644
--- a/src/northbridge/intel/pineview/raminit.c
+++ b/src/northbridge/intel/pineview/raminit.c
@@ -498,28 +498,12 @@
    	lowcas = lsbp;
    	while (cas == 0 && highcas >= lowcas) {
    		FOR_EACH_POPULATED_DIMM(s->dimms, i) {
-				switch (freq) {
-				case MEM_CLOCK_800MHz:
-					if ((s->dimms[i].spd_data[9] > 0x25) ||
-					    (s->dimms[i].spd_data[10] > 0x40)) {
-						// CAS too fast, lower it
-						highcas--;
-						break;
-					} else {
-						cas = highcas;
-					}
-					break;
-				case MEM_CLOCK_667MHz:
-				default:
-					if ((s->dimms[i].spd_data[9] > 0x30) ||
-					    (s->dimms[i].spd_data[10] > 0x45)) {
-						// CAS too fast, lower it
-						highcas--;
-						break;
-					} else {
-						cas = highcas;
-					}
-					break;
+				if ((s->dimms[i].spd_data[9] > 0x30) ||
+				    (s->dimms[i].spd_data[10] > 0x45)) {
+					// CAS too fast, lower it
+					highcas--;
+				} else {
+					cas = highcas;
    			}
    		}
    	}