Change in ...coreboot[master]: nb/intel/i945: Check if interleaved even if rank[4] size is zero

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31007

Change subject: nb/intel/i945: Check if interleaved even if rank[4] size is zero
......................................................................

nb/intel/i945: Check if interleaved even if rank[4] size is zero

With this patch, the board can boot when interleaved but rank4 is 0.
Tested on 945G-M4, it boot when:
DIMM0 + DIMM2 populated: Ok
DIMM0 + DIMM3 populated: Ok
DIMM1 + DIMM2 populated: Ok
DIMM1 + DIMM3 populated: Ok

raminit.c still needs channelA populated. It will not boot if
channel A is not populated.

Change-Id: Ibf130a3d4b6f8fa816f7a5f06822a9b8807be3d4
Signed-off-by: Elyes HAOUAS 
---
M src/northbridge/intel/i945/raminit.c
1 file changed, 3 insertions(+), 3 deletions(-)

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

diff --git a/src/northbridge/intel/i945/raminit.c b/src/northbridge/intel/i945/raminit.c
index 64c87da..a46677b 100644
--- a/src/northbridge/intel/i945/raminit.c
+++ b/src/northbridge/intel/i945/raminit.c
@@ -2551,9 +2551,6 @@
    u32 bankaddr = 0, tmpaddr, mrsaddr = 0;

for (i = 0, nonzero = -1; i < 8; i++) {
-		if (sysinfo->banksize[i]  == 0)
-			continue;
-
    	printk(BIOS_DEBUG, "jedec enable sequence: bank %d\n", i);
    	switch (i) {
    	case 0:
@@ -2576,6 +2573,9 @@
    		bankaddr = 0;
    	}

+		if (sysinfo->banksize[i]  == 0)
+			continue;
+
    	/* We have a bank with a non-zero size.. Remember it
    	 * for the next offset we have to calculate
    	 */

Change in ...coreboot[master]: nb/intel/i945: Check if interleaved even if rank #4 size is zero

[HAOUAS Elyes (Code Review)]

Hello Patrick Rudolph, Arthur Heymans,

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

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

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

Change subject: nb/intel/i945: Check if interleaved even if rank #4 size is zero
......................................................................

nb/intel/i945: Check if interleaved even if rank #4 size is zero

With this patch, the board can boot when interleaved but rank4 is 0.
Tested on 945G-M4, it boot when:
DIMM0 + DIMM2 populated: Ok
DIMM0 + DIMM3 populated: Ok
DIMM1 + DIMM2 populated: Ok
DIMM1 + DIMM3 populated: Ok

raminit.c still needs channelA populated. It will not boot if
channel A is not populated.

Change-Id: Ibf130a3d4b6f8fa816f7a5f06822a9b8807be3d4
Signed-off-by: Elyes HAOUAS 
---
M src/northbridge/intel/i945/raminit.c
1 file changed, 3 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/31007/2

Change in ...coreboot[master]: nb/intel/i945: Check if interleaved even if rank #4 size is zero

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31007 )

Change subject: nb/intel/i945: Check if interleaved even if rank #4 size is zero
......................................................................

Patch Set 4: Code-Review-1

(1 comment)

https://review.coreboot.org/#/c/31007/4/src/northbridge/intel/i945/raminit.c 
File src/northbridge/intel/i945/raminit.c:

https://review.coreboot.org/#/c/31007/4/src/northbridge/intel/i945/raminit.c... 
PS4, Line 2556: 
              : 
              : 
without this, the loop seems useless...

Change in ...coreboot[master]: nb/intel/i945: Check if interleaved even if rank #4 size is zero

[HAOUAS Elyes (Code Review)]

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

Change subject: nb/intel/i945: Check if interleaved even if rank #4 size is zero
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/#/c/31007/4/src/northbridge/intel/i945/raminit.c 
File src/northbridge/intel/i945/raminit.c:

https://review.coreboot.org/#/c/31007/4/src/northbridge/intel/i945/raminit.c... 
PS4, Line 2556: 
              : 
              :
> without this, the loop seems useless...

here if banksize[4]=0, and the "sysinfo->interleaved" is true, will keep using "bankaddr = 0" even for banksize[5,6 and 7]

Change in ...coreboot[master]: nb/intel/i945: Check if interleaved even if rank #4 size is zero

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31007 )

Change subject: nb/intel/i945: Check if interleaved even if rank #4 size is zero
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/#/c/31007/4/src/northbridge/intel/i945/raminit.c 
File src/northbridge/intel/i945/raminit.c:

https://review.coreboot.org/#/c/31007/4/src/northbridge/intel/i945/raminit.c... 
PS4, Line 2559: 		case 0:
              : 			/* Start at address 0 */
              : 			bankaddr = 0;
              : 			break;
you always exit here so this cannot be correct.

Change in ...coreboot[master]: nb/intel/i945: Check if interleaved even if rank #4 size is zero

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31007 )

Change subject: nb/intel/i945: Check if interleaved even if rank #4 size is zero
......................................................................

Patch Set 4: -Code-Review

Change in ...coreboot[master]: nb/intel/i945: Check if interleaved even if rank #4 size is zero

[Nico Huber (Code Review)]

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

Change subject: nb/intel/i945: Check if interleaved even if rank #4 size is zero
......................................................................

Patch Set 4:

(2 comments)

https://review.coreboot.org/#/c/31007/4/src/northbridge/intel/i945/raminit.c 
File src/northbridge/intel/i945/raminit.c:

https://review.coreboot.org/#/c/31007/4/src/northbridge/intel/i945/raminit.c... 
PS4, Line 2572: 					(sysinfo->interleaved ? 26 : 25);
You might hit this multiple times for zero banks. e.g. if you have
0, 1, 4 and 5 populated, it would spuriously increase `bankaddr`
here for 2 and 3.

https://review.coreboot.org/#/c/31007/4/src/northbridge/intel/i945/raminit.c... 
PS4, Line 2580: 			continue;
I would probably write everything from the start of the loop
as follows:

if (sysinfo->banksize[i] == 0)
            continue;

if (nonzero != -1) {
            if (sysinfo->interleaved && nonzero < 4 && i >= 4) {
                    bankaddr = 0x40;
            } else {
                    bankaddr += ...
            }
    }

Reasoning: `bankaddr` is already initialized to `0`, we should
never have to set `0` later. When we find another non-zero bank,
we always add the size of the last non-zero bank, unless we
do interleaving and found the first non-zero bank >= 4.

Change in ...coreboot[master]: nb/intel/i945: Check if interleaved even if rank #4 size is zero

[HAOUAS Elyes (Code Review)]

Hello Patrick Rudolph, Arthur Heymans, build bot (Jenkins),

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

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

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

Change subject: nb/intel/i945: Check if interleaved even if rank #4 size is zero
......................................................................

nb/intel/i945: Check if interleaved even if rank #4 size is zero

With this patch, the board can boot when interleaved but rank4 is 0.
Tested on 945G-M4, it boot when:
DIMM0 + DIMM2 populated: Ok
DIMM0 + DIMM3 populated: Ok
DIMM1 + DIMM2 populated: Ok
DIMM1 + DIMM3 populated: Ok

raminit.c still needs channelA populated. It will not boot if
channel A is not populated.

Change-Id: Ibf130a3d4b6f8fa816f7a5f06822a9b8807be3d4
Signed-off-by: Elyes HAOUAS 
---
M src/northbridge/intel/i945/raminit.c
1 file changed, 6 insertions(+), 18 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/31007/5

Change in ...coreboot[master]: nb/intel/i945: Check if interleaved even if rank #4 size is zero

[build bot (Jenkins) (Code Review)]

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

Change subject: nb/intel/i945: Check if interleaved even if rank #4 size is zero
......................................................................

Patch Set 5:

(2 comments)

https://review.coreboot.org/#/c/31007/5/src/northbridge/intel/i945/raminit.c 
File src/northbridge/intel/i945/raminit.c:

https://review.coreboot.org/#/c/31007/5/src/northbridge/intel/i945/raminit.c... 
PS5, Line 2564:                         } else {
code indent should use tabs where possible

https://review.coreboot.org/#/c/31007/5/src/northbridge/intel/i945/raminit.c... 
PS5, Line 2564:                         } else {
please, no spaces at the start of a line

Change in ...coreboot[master]: nb/intel/i945: Check if interleaved even if rank #4 size is zero

[HAOUAS Elyes (Code Review)]

Hello Patrick Rudolph, Arthur Heymans, build bot (Jenkins),

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

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

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

Change subject: nb/intel/i945: Check if interleaved even if rank #4 size is zero
......................................................................

nb/intel/i945: Check if interleaved even if rank #4 size is zero

Change-Id: Ibf130a3d4b6f8fa816f7a5f06822a9b8807be3d4
Signed-off-by: Elyes HAOUAS 
---
M src/northbridge/intel/i945/raminit.c
1 file changed, 6 insertions(+), 18 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/31007/6

Change in ...coreboot[master]: nb/intel/i945: Check if interleaved even if rank #4 size is zero

[Nico Huber (Code Review)]

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

Change subject: nb/intel/i945: Check if interleaved even if rank #4 size is zero
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/#/c/31007/5/src/northbridge/intel/i945/raminit.c 
File src/northbridge/intel/i945/raminit.c:

https://review.coreboot.org/#/c/31007/5/src/northbridge/intel/i945/raminit.c... 
PS5, Line 2559: 		if (sysinfo->banksize[i] == 0)
              : 			continue;
Please move this back above the printk().

Change in ...coreboot[master]: nb/intel/i945: Check if interleaved even if rank #4 size is zero

[HAOUAS Elyes (Code Review)]

Hello Patrick Rudolph, Arthur Heymans, build bot (Jenkins),

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

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

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

Change subject: nb/intel/i945: Check if interleaved even if rank #4 size is zero
......................................................................

nb/intel/i945: Check if interleaved even if rank #4 size is zero

Tested config:
Interleaved (config; status before, after):
 DIMM{0 + 2}: ok, ok
 DIMM{0 + 3}: Nok, ok
 DIMM{1 + 2}: ok, ok
 DIMM{1 + 3}: Nok, ok
 DIMM{1 + 2 + 3}: ok, ok
 DIMM{0 + 2 + 3}: ok, ok
 DIMM{0 + 1 + 2}: ok, ok
 DIMM{0 + + + 3}: Nok, ok

Not Interleaved:
 DIMM{0 + 1 + 3}: Nok, Nok
 DIMM{0 + 1 + 2}: ok, ok (with single ranked)
 DIMM{0 + 1 + 2}: Nok, Nok (with only dual ranked)
 DIMM{0 + 2 + 3}: Nok, ok
 DIMM{1 + 2 + 3}: ok, ok

If channel0 is not populated, raminit will fail.
If RAM is 533, raminit will fail.
I think that northbridge/intel/i945/rcven.c needs some inverstigation
and probably a fix.

Change-Id: Ibf130a3d4b6f8fa816f7a5f06822a9b8807be3d4
Signed-off-by: Elyes HAOUAS 
---
M src/northbridge/intel/i945/raminit.c
1 file changed, 6 insertions(+), 16 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/31007/7

Change in ...coreboot[master]: nb/intel/i945: Check if interleaved even if rank #4 size is zero

[build bot (Jenkins) (Code Review)]

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

Change subject: nb/intel/i945: Check if interleaved even if rank #4 size is zero
......................................................................

Patch Set 7:

(2 comments)

https://review.coreboot.org/#/c/31007/7/src/northbridge/intel/i945/raminit.c 
File src/northbridge/intel/i945/raminit.c:

https://review.coreboot.org/#/c/31007/7/src/northbridge/intel/i945/raminit.c... 
PS7, Line 2566:                         } else {
code indent should use tabs where possible

https://review.coreboot.org/#/c/31007/7/src/northbridge/intel/i945/raminit.c... 
PS7, Line 2566:                         } else {
please, no spaces at the start of a line

Change in ...coreboot[master]: nb/intel/i945: Check if interleaved even if rank #4 size is zero

[HAOUAS Elyes (Code Review)]

Hello Patrick Rudolph, Arthur Heymans, build bot (Jenkins),

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

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

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

Change subject: nb/intel/i945: Check if interleaved even if rank #4 size is zero
......................................................................

nb/intel/i945: Check if interleaved even if rank #4 size is zero

Tested config:
Interleaved (config; status before, after):
 DIMM{0 + 2}: ok, ok
 DIMM{0 + 3}: Nok, ok
 DIMM{1 + 2}: ok, ok
 DIMM{1 + 3}: Nok, ok
 DIMM{1 + 2 + 3}: ok, ok
 DIMM{0 + 2 + 3}: ok, ok
 DIMM{0 + 1 + 2}: ok, ok
 DIMM{0 + + + 3}: Nok, ok

Not Interleaved:
 DIMM{0 + 1 + 3}: Nok, Nok
 DIMM{0 + 1 + 2}: ok, ok (with single ranked)
 DIMM{0 + 1 + 2}: Nok, Nok (with only dual ranked)
 DIMM{0 + 2 + 3}: Nok, ok
 DIMM{1 + 2 + 3}: ok, ok

If channel0 is not populated, raminit will fail.
If RAM is 533, raminit will fail.
I think that northbridge/intel/i945/rcven.c needs some inverstigation
and probably a fix.

Change-Id: Ibf130a3d4b6f8fa816f7a5f06822a9b8807be3d4
Signed-off-by: Elyes HAOUAS 
---
M src/northbridge/intel/i945/raminit.c
1 file changed, 6 insertions(+), 16 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/31007/8

Change in ...coreboot[master]: nb/intel/i945: Check if interleaved even if rank #4 size is zero

[Nico Huber (Code Review)]

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

Change subject: nb/intel/i945: Check if interleaved even if rank #4 size is zero
......................................................................

Patch Set 8: Code-Review+1

(4 comments)

https://review.coreboot.org/#/c/31007/8//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/31007/8//COMMIT_MSG@18 
PS8, Line 18: +
1?

https://review.coreboot.org/#/c/31007/8//COMMIT_MSG@27 
PS8, Line 27: If channel0 is not populated, raminit will fail.
            : If RAM is 533, raminit will fail.
            : I think that northbridge/intel/i945/rcven.c needs some inverstigation
            : and probably a fix.
That's all unrelated?

https://review.coreboot.org/#/c/31007/8/src/northbridge/intel/i945/raminit.c 
File src/northbridge/intel/i945/raminit.c:

https://review.coreboot.org/#/c/31007/8/src/northbridge/intel/i945/raminit.c... 
PS8, Line 2581: 
Why remove this blank line?

https://review.coreboot.org/#/c/31007/8/src/northbridge/intel/i945/raminit.c... 
PS8, Line 2557: 
Why add a blank line?

Change in ...coreboot[master]: nb/intel/i945: Check if interleaved even if rank #4 size is zero

[HAOUAS Elyes (Code Review)]

Hello Patrick Rudolph, Arthur Heymans, build bot (Jenkins), Nico Huber,

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

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

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

Change subject: nb/intel/i945: Check if interleaved even if rank #4 size is zero
......................................................................

nb/intel/i945: Check if interleaved even if rank #4 size is zero

Tested config:
Interleaved (config; status before, after):
 DIMM{0 + 2}: ok, ok
 DIMM{0 + 3}: Nok, ok
 DIMM{1 + 2}: ok, ok
 DIMM{1 + 3}: Nok, ok
 DIMM{1 + 2 + 3}: ok, ok
 DIMM{0 + 2 + 3}: ok, ok
 DIMM{0 + 1 + 2}: ok, ok
 DIMM{0 + 1 + 3}: Nok, ok

Not Interleaved:
 DIMM{0 + 1 + 3}: Nok, Nok
 DIMM{0 + 1 + 2}: ok, ok (with single ranked)
 DIMM{0 + 1 + 2}: Nok, Nok (with only dual ranked)
 DIMM{0 + 2 + 3}: Nok, ok
 DIMM{1 + 2 + 3}: ok, ok

Change-Id: Ibf130a3d4b6f8fa816f7a5f06822a9b8807be3d4
Signed-off-by: Elyes HAOUAS 
---
M src/northbridge/intel/i945/raminit.c
1 file changed, 5 insertions(+), 15 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/31007/9

Change in ...coreboot[master]: nb/intel/i945: Check if interleaved even if rank #4 size is zero

[Nico Huber (Code Review)]

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

Change subject: nb/intel/i945: Check if interleaved even if rank #4 size is zero
......................................................................

Patch Set 9: Code-Review+2

Change in ...coreboot[master]: nb/intel/i945: Check if interleaved even if rank #4 size is zero

[Patrick Georgi (Code Review)]

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

Change subject: nb/intel/i945: Check if interleaved even if rank #4 size is zero
......................................................................

nb/intel/i945: Check if interleaved even if rank #4 size is zero

Tested config:
Interleaved (config; status before, after):
 DIMM{0 + 2}: ok, ok
 DIMM{0 + 3}: Nok, ok
 DIMM{1 + 2}: ok, ok
 DIMM{1 + 3}: Nok, ok
 DIMM{1 + 2 + 3}: ok, ok
 DIMM{0 + 2 + 3}: ok, ok
 DIMM{0 + 1 + 2}: ok, ok
 DIMM{0 + 1 + 3}: Nok, ok

Not Interleaved:
 DIMM{0 + 1 + 3}: Nok, Nok
 DIMM{0 + 1 + 2}: ok, ok (with single ranked)
 DIMM{0 + 1 + 2}: Nok, Nok (with only dual ranked)
 DIMM{0 + 2 + 3}: Nok, ok
 DIMM{1 + 2 + 3}: ok, ok

Change-Id: Ibf130a3d4b6f8fa816f7a5f06822a9b8807be3d4
Signed-off-by: Elyes HAOUAS 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/31007
Tested-by: build bot (Jenkins) 
Reviewed-by: Nico Huber 
---
M src/northbridge/intel/i945/raminit.c
1 file changed, 5 insertions(+), 15 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, approved

diff --git a/src/northbridge/intel/i945/raminit.c b/src/northbridge/intel/i945/raminit.c
index d41b913..1e8cf65 100644
--- a/src/northbridge/intel/i945/raminit.c
+++ b/src/northbridge/intel/i945/raminit.c
@@ -2546,29 +2546,19 @@
    u32 bankaddr = 0, tmpaddr, mrsaddr = 0;

for (i = 0, nonzero = -1; i < 8; i++) {
-		if (sysinfo->banksize[i]  == 0)
+		if (sysinfo->banksize[i] == 0)
    		continue;

printk(BIOS_DEBUG, "jedec enable sequence: bank %d\n", i);
-		switch (i) {
-		case 0:
-			/* Start at address 0 */
-			bankaddr = 0;
-			break;
-		case 4:
-			if (sysinfo->interleaved) {
+
+		if (nonzero != -1) {
+			if (sysinfo->interleaved && nonzero < 4 && i >= 4) {
    			bankaddr = 0x40;
-				break;
-			}
-		default:
-			if (nonzero != -1) {
+			} else {
    			printk(BIOS_DEBUG, "bankaddr from bank size of rank %d\n", nonzero);
    			bankaddr += sysinfo->banksize[nonzero] <<
    				(sysinfo->interleaved ? 26 : 25);
-				break;
    		}
-			/* No populated bank hit before. Start at address 0 */
-			bankaddr = 0;
    	}

/* We have a bank with a non-zero size.. Remember it