Change in ...coreboot[master]: nb/amd/amdmct/mct: Deduplicate conditional

[Jacob Garber (Code Review)]

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

Change subject: nb/amd/amdmct/mct: Deduplicate conditional
......................................................................

nb/amd/amdmct/mct: Deduplicate conditional

The pDCTstat->Speed check has already been performed on line 75, so we
can merge the two branches of this conditional.

Change-Id: I41aa19b4b7ed7b1a0e4f83f72e66869760e677dd
Signed-off-by: Jacob Garber 
Found-by: Coverity CID 1229583
---
M src/northbridge/amd/amdmct/mct/mctardk3.c
1 file changed, 4 insertions(+), 15 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/11/33211/1

diff --git a/src/northbridge/amd/amdmct/mct/mctardk3.c b/src/northbridge/amd/amdmct/mct/mctardk3.c
index ce79a52..2d1f900 100644
--- a/src/northbridge/amd/amdmct/mct/mctardk3.c
+++ b/src/northbridge/amd/amdmct/mct/mctardk3.c
@@ -90,21 +90,10 @@
    			valx &= 0xAA;
    			valx >>= 1;
    		}
-			if (mctGet_NVbits(NV_MAX_DIMMS) == 8) {
-				val &= valx;
-				if (val != 0) {
-					pDCTstat->CH_ADDR_TMG[dct] &= 0xFFFF00FF;
-					pDCTstat->CH_ADDR_TMG[dct] |= 0x00002F00;
-				}
-			} else {
-				val &= valx;
-				if (val != 0) {
-					if (pDCTstat->Speed == 3 || pDCTstat->Speed == 3) {
-						pDCTstat->CH_ADDR_TMG[dct] &= 0xFFFF00FF;
-						pDCTstat->CH_ADDR_TMG[dct] |= 0x00002F00;
-					}
-				}
-
+			val &= valx;
+			if (val != 0) {
+				pDCTstat->CH_ADDR_TMG[dct] &= 0xFFFF00FF;
+				pDCTstat->CH_ADDR_TMG[dct] |= 0x00002F00;
    		}
    	}
    }

[HAOUAS Elyes (Code Review)]

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

Change subject: nb/amd/amdmct/mct: Deduplicate conditional
......................................................................

Patch Set 1:

line #75 we have:
if (pDCTstat->Speed == 3 || pDCTstat->Speed == 4)
and line #102 we have:
if (pDCTstat->Speed == 3 || pDCTstat->Speed == 3)

[Angel Pons (Code Review)]

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

Change subject: nb/amd/amdmct/mct: Deduplicate conditional
......................................................................

Patch Set 1: Code-Review-1

(1 comment)

Looks wrong to me.

https://review.coreboot.org/#/c/33211/1/src/northbridge/amd/amdmct/mct/mctar... 
File src/northbridge/amd/amdmct/mct/mctardk3.c:

https://review.coreboot.org/#/c/33211/1/src/northbridge/amd/amdmct/mct/mctar... 
PS1, Line 102: 
How do you know pDCTstat->Speed isn't 4?

[Jacob Garber (Code Review)]

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

Change subject: nb/amd/amdmct/mct: Deduplicate conditional
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/33211/1/src/northbridge/amd/amdmct/mct/mctar... 
File src/northbridge/amd/amdmct/mct/mctardk3.c:

https://review.coreboot.org/#/c/33211/1/src/northbridge/amd/amdmct/mct/mctar... 
PS1, Line 102:
> How do you know pDCTstat->Speed isn't 4?

That's a good point, I had assumed it was a copy-paste error and that 4 should be included too.

[Jacob Garber (Code Review)]

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

Change subject: nb/amd/amdmct/mct: Deduplicate conditional
......................................................................

Patch Set 1:

Hello, this was a long time ago, but do you think that

pDCTstat->Speed == 3 || pDCTstat->Speed == 3

is a copy-paste error?

[HAOUAS Elyes (Code Review)]

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

Change subject: nb/amd/amdmct/mct: Deduplicate conditional
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/33211/1/src/northbridge/amd/amdmct/mct/mctar... 
File src/northbridge/amd/amdmct/mct/mctardk3.c:

https://review.coreboot.org/#/c/33211/1/src/northbridge/amd/amdmct/mct/mctar... 
PS1, Line 84: if (val) {
            : 			//FIXME: skip for Ax
            : 			valx = pDCTstat->DimmDRPresent;
            : 			if (dct == 0) {
            : 				valx &= 0x55;
            : 			} else {
            : 				valx &= 0xAA;
            : 				valx >>= 1;
            : 			}
            : 			val &= valx;
            : 			if (val != 0) {
            : 				pDCTstat->CH_ADDR_TMG[dct] &= 0xFFFF00FF;
            : 				pDCTstat->CH_ADDR_TMG[dct] |= 0x00002F00;
            : 			}
as at line #102, "if (pDCTstat->Speed == 3 || pDCTstat->Speed == 3)" is true and "if (pDCTstat->Speed == 3 || pDCTstat->Speed == 4)" is also true,

would this fix the typo/bug?

if (val) {
                        //FIXME: skip for Ax
                        valx = pDCTstat->DimmDRPresent;
                        if (dct == 0) {
                                valx &= 0x55;
                        } else {
                                valx &= 0xAA;
                                valx >>= 1;
                        }
                        if (mctGet_NVbits(NV_MAX_DIMMS) == 8) {
                                val &= valx;
                                if (val != 0) {
                                        pDCTstat->CH_ADDR_TMG[dct] &= 0xFFFF00FF;
                                        pDCTstat->CH_ADDR_TMG[dct] |= 0x00002F00;
                                }
                        } else {
                                val &= valx;
                                if (val != 0) {
                                        pDCTstat->CH_ADDR_TMG[dct] &= 0xFFFF00FF;
                                        pDCTstat->CH_ADDR_TMG[dct] |= 0x00002F00;
                                }

}

Change in ...coreboot[master]: nb/amd/amdmct/mct: Simplify conditional

[Jacob Garber (Code Review)]

Hello Angel Pons, Marc Jones, build bot (Jenkins),

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

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

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

Change subject: nb/amd/amdmct/mct: Simplify conditional
......................................................................

nb/amd/amdmct/mct: Simplify conditional

These if statements can be combined to merge the two branches of the
conditional and remove the duplicate pDCTstat->Speed == 3 check.

Change-Id: I41aa19b4b7ed7b1a0e4f83f72e66869760e677dd
Signed-off-by: Jacob Garber 
Found-by: Coverity CID 1229583
---
M src/northbridge/amd/amdmct/mct/mctardk3.c
1 file changed, 3 insertions(+), 12 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/11/33211/2

Change in ...coreboot[master]: nb/amd/amdmct/mct: Simplify conditional

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33211 )

Change subject: nb/amd/amdmct/mct: Simplify conditional
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/33211/2/src/northbridge/amd/amdmct/mct/mctar... 
File src/northbridge/amd/amdmct/mct/mctardk3.c:

https://review.coreboot.org/#/c/33211/2/src/northbridge/amd/amdmct/mct/mctar... 
PS2, Line 95: 				if (mctGet_NVbits(NV_MAX_DIMMS) == 8 || pDCTstat->Speed == 3) {
line over 80 characters

Change in ...coreboot[master]: nb/amd/amdmct/mct: Simplify conditional

[Jacob Garber (Code Review)]

Hello Angel Pons, build bot (Jenkins), Marc Jones,

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

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

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

Change subject: nb/amd/amdmct/mct: Simplify conditional
......................................................................

nb/amd/amdmct/mct: Simplify conditional

These if statements can be combined to merge the two branches of the
conditional and remove the duplicate pDCTstat->Speed == 3 check.

Change-Id: I41aa19b4b7ed7b1a0e4f83f72e66869760e677dd
Signed-off-by: Jacob Garber 
Found-by: Coverity CID 1229583
---
M src/northbridge/amd/amdmct/mct/mctardk3.c
1 file changed, 4 insertions(+), 12 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/11/33211/3

Change in ...coreboot[master]: nb/amd/amdmct/mct: Simplify conditional

[Angel Pons (Code Review)]

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

Change subject: nb/amd/amdmct/mct: Simplify conditional
......................................................................

Patch Set 3: Code-Review+2

Should be equivalent.

Change in ...coreboot[master]: nb/amd/amdmct/mct: Simplify conditional

[Nico Huber (Code Review)]

Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33211 )

Change subject: nb/amd/amdmct/mct: Simplify conditional
......................................................................

nb/amd/amdmct/mct: Simplify conditional

These if statements can be combined to merge the two branches of the
conditional and remove the duplicate pDCTstat->Speed == 3 check.

Change-Id: I41aa19b4b7ed7b1a0e4f83f72e66869760e677dd
Signed-off-by: Jacob Garber 
Found-by: Coverity CID 1229583
Reviewed-on: https://review.coreboot.org/c/coreboot/+/33211
Tested-by: build bot (Jenkins) 
Reviewed-by: Angel Pons 
---
M src/northbridge/amd/amdmct/mct/mctardk3.c
1 file changed, 4 insertions(+), 12 deletions(-)

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

diff --git a/src/northbridge/amd/amdmct/mct/mctardk3.c b/src/northbridge/amd/amdmct/mct/mctardk3.c
index ce79a52..4eac790 100644
--- a/src/northbridge/amd/amdmct/mct/mctardk3.c
+++ b/src/northbridge/amd/amdmct/mct/mctardk3.c
@@ -90,21 +90,13 @@
    			valx &= 0xAA;
    			valx >>= 1;
    		}
-			if (mctGet_NVbits(NV_MAX_DIMMS) == 8) {
-				val &= valx;
-				if (val != 0) {
+			val &= valx;
+			if (val != 0) {
+				if (mctGet_NVbits(NV_MAX_DIMMS) == 8 ||
+						pDCTstat->Speed == 3) {
    				pDCTstat->CH_ADDR_TMG[dct] &= 0xFFFF00FF;
    				pDCTstat->CH_ADDR_TMG[dct] |= 0x00002F00;
    			}
-			} else {
-				val &= valx;
-				if (val != 0) {
-					if (pDCTstat->Speed == 3 || pDCTstat->Speed == 3) {
-						pDCTstat->CH_ADDR_TMG[dct] &= 0xFFFF00FF;
-						pDCTstat->CH_ADDR_TMG[dct] |= 0x00002F00;
-					}
-				}
-
    		}
    	}
    }