Paul Menzel has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39534 )
Change subject: nb/intel/i945/raminit: Simplify if condition ......................................................................
nb/intel/i945/raminit: Simplify if condition
Use DeMorgan's law to make the condition simpler by getting rid of the negations.
Change-Id: I041f2740d6991f9b4e6b8f77988b970c028ca512 Signed-off-by: Paul Menzel pmenzel@molgen.mpg.de --- M src/northbridge/intel/i945/raminit.c 1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/39534/1
diff --git a/src/northbridge/intel/i945/raminit.c b/src/northbridge/intel/i945/raminit.c index cb3b943..6321753 100644 --- a/src/northbridge/intel/i945/raminit.c +++ b/src/northbridge/intel/i945/raminit.c @@ -2437,8 +2437,8 @@ reg32 |= (1 << 14) | (1 << 6) | (2 << 16); MCHBAR32(ODTC) = reg32;
- if (!(sysinfo->dimm[0] != SYSINFO_DIMM_NOT_POPULATED && - sysinfo->dimm[1] != SYSINFO_DIMM_NOT_POPULATED)) { + if (sysinfo->dimm[0] == SYSINFO_DIMM_NOT_POPULATED || + sysinfo->dimm[1] == SYSINFO_DIMM_NOT_POPULATED) { printk(BIOS_DEBUG, "one dimm per channel config..\n");
reg32 = MCHBAR32(C0ODT);
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39534 )
Change subject: nb/intel/i945/raminit: Simplify if condition ......................................................................
Patch Set 1: Code-Review-1
what if both are not populated ?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39534 )
Change subject: nb/intel/i945/raminit: Simplify if condition ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
Patch Set 1: Code-Review-1
what if both are not populated ?
This change does not change the behavior of the code.
https://review.coreboot.org/c/coreboot/+/39534/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39534/1//COMMIT_MSG@9 PS1, Line 9: DeMorgan add a space: De Morgan
As to why, Wikipedia has it: https://en.wikipedia.org/wiki/De_Morgan%27s_laws
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39534 )
Change subject: nb/intel/i945/raminit: Simplify if condition ......................................................................
Patch Set 1:
Patch Set 1: Code-Review+2
(1 comment)
Patch Set 1: Code-Review-1
what if both are not populated ?
This change does not change the behavior of the code.
Elyes, that’s indeed the question, but should be fixed either in your change-set I7aec35f45250da554ddc5a68f5add157c313399c or a separate one.
Hello build bot (Jenkins), Patrick Georgi, Angel Pons, Patrick Rudolph, HAOUAS Elyes,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39534
to look at the new patch set (#2).
Change subject: nb/intel/i945/raminit: Simplify if condition ......................................................................
nb/intel/i945/raminit: Simplify if condition
Use De Morgan’s law to simplify the condition by getting rid of the negations.
Change-Id: I041f2740d6991f9b4e6b8f77988b970c028ca512 Signed-off-by: Paul Menzel pmenzel@molgen.mpg.de --- M src/northbridge/intel/i945/raminit.c 1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/39534/2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39534 )
Change subject: nb/intel/i945/raminit: Simplify if condition ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39534/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39534/1//COMMIT_MSG@9 PS1, Line 9: DeMorgan
add a space: De Morgan […]
Done
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39534 )
Change subject: nb/intel/i945/raminit: Simplify if condition ......................................................................
Patch Set 2:
This is true if we have only 2 DIMMs. in other words we MUST have at least one DIMM populated.
if we have 4 DIMMs (at SPD 0x50, 0x51, 0x52 and 0x53), the old and new one are wrong.
Up to you if you want to merge equivalent wrong code.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39534 )
Change subject: nb/intel/i945/raminit: Simplify if condition ......................................................................
Patch Set 2:
Patch Set 2:
This is true if we have only 2 DIMMs. in other words we MUST have at least one DIMM populated.
if we have 4 DIMMs (at SPD 0x50, 0x51, 0x52 and 0x53), the old and new one are wrong.
Up to you if you want to merge equivalent wrong code.
Yes, I know i945 raminit is doing weird things for desktop stuff, but the scope of this change is to not change the behavior of the code. If that makes a mistake much more obvious, great! Another patch can fix it 😄
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39534 )
Change subject: nb/intel/i945/raminit: Simplify if condition ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39534/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39534/2//COMMIT_MSG@11 PS2, Line 11: Could you please add:
Tested with BUILD_TIMELESS=1, getac/p470 remains unchanged.
I have checked that myself, you can re-check if you want to be sure.
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39534 )
Change subject: nb/intel/i945/raminit: Simplify if condition ......................................................................
Patch Set 2:
Patch Set 2: Up to you if you want to merge equivalent wrong code.
As I understand it, the purpose of this change is to land the non-controversial change (that makes the code more easily understandable) while the approach to fix the problem for real can be discussed in your change.
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39534 )
Change subject: nb/intel/i945/raminit: Simplify if condition ......................................................................
Patch Set 2: -Code-Review
Patch Set 2:
Patch Set 2: Up to you if you want to merge equivalent wrong code.
As I understand it, the purpose of this change is to land the non-controversial change (that makes the code more easily understandable) while the approach to fix the problem for real can be discussed in your change.
I am not against, I just prefer to fix this code.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39534 )
Change subject: nb/intel/i945/raminit: Simplify if condition ......................................................................
Patch Set 2:
Patch Set 2: -Code-Review
Patch Set 2:
Patch Set 2: Up to you if you want to merge equivalent wrong code.
As I understand it, the purpose of this change is to land the non-controversial change (that makes the code more easily understandable) while the approach to fix the problem for real can be discussed in your change.
I am not against, I just prefer to fix this code.
Yes, that is my intention.
Hello build bot (Jenkins), Patrick Georgi, Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39534
to look at the new patch set (#3).
Change subject: nb/intel/i945/raminit: Simplify if condition ......................................................................
nb/intel/i945/raminit: Simplify if condition
Use De Morgan’s law to simplify the condition by getting rid of the negations.
TEST=With `make BUILD_TIMELESS=1` getac/p470 remains unchanged. Change-Id: I041f2740d6991f9b4e6b8f77988b970c028ca512 Signed-off-by: Paul Menzel pmenzel@molgen.mpg.de --- M src/northbridge/intel/i945/raminit.c 1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/39534/3
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39534 )
Change subject: nb/intel/i945/raminit: Simplify if condition ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39534/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39534/2//COMMIT_MSG@11 PS2, Line 11:
Could you please add: […]
Done
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39534 )
Change subject: nb/intel/i945/raminit: Simplify if condition ......................................................................
nb/intel/i945/raminit: Simplify if condition
Use De Morgan’s law to simplify the condition by getting rid of the negations.
TEST=With `make BUILD_TIMELESS=1` getac/p470 remains unchanged. Change-Id: I041f2740d6991f9b4e6b8f77988b970c028ca512 Signed-off-by: Paul Menzel pmenzel@molgen.mpg.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/39534 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com --- M src/northbridge/intel/i945/raminit.c 1 file changed, 2 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved
diff --git a/src/northbridge/intel/i945/raminit.c b/src/northbridge/intel/i945/raminit.c index c50b1d8..a92118c 100644 --- a/src/northbridge/intel/i945/raminit.c +++ b/src/northbridge/intel/i945/raminit.c @@ -2437,8 +2437,8 @@ reg32 |= (1 << 14) | (1 << 6) | (2 << 16); MCHBAR32(ODTC) = reg32;
- if (!(sysinfo->dimm[0] != SYSINFO_DIMM_NOT_POPULATED && - sysinfo->dimm[1] != SYSINFO_DIMM_NOT_POPULATED)) { + if (sysinfo->dimm[0] == SYSINFO_DIMM_NOT_POPULATED || + sysinfo->dimm[1] == SYSINFO_DIMM_NOT_POPULATED) { printk(BIOS_DEBUG, "one dimm per channel config..\n");
reg32 = MCHBAR32(C0ODT);
-
Angel Pons (Code Review) -
HAOUAS Elyes (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)