Change in coreboot[master]: intel/i440bx: Resolve long standing raminit TODOs

[Keith Hui (Code Review)]

Keith Hui has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38383 )

Change subject: intel/i440bx: Resolve long standing raminit TODOs
......................................................................

intel/i440bx: Resolve long standing raminit TODOs

Drop DRAMT write as it's only rewriting the power on default.

PMCR write is required. Update comment on its purpose and move to
end of sdram_enable().

Change-Id: I62e8b2531f0f297ffb7db440db89ffa65771b7d5
Signed-off-by: Keith Hui 
---
M src/northbridge/intel/i440bx/raminit.c
1 file changed, 3 insertions(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/38383/1

diff --git a/src/northbridge/intel/i440bx/raminit.c b/src/northbridge/intel/i440bx/raminit.c
index d5c23cf..5ce5e5b 100644
--- a/src/northbridge/intel/i440bx/raminit.c
+++ b/src/northbridge/intel/i440bx/raminit.c
@@ -985,13 +985,6 @@

/* Setup DRAM buffer strength. */
    set_dram_buffer_strength();
-
-	/* TODO: Set PMCR? */
-	// pci_write_config8(NB, PMCR, 0x14);
-	pci_write_config8(NB, PMCR, 0x10);
-
-	/* TODO: This is for EDO memory only. */
-	pci_write_config8(NB, DRAMT, 0x03);
 }

static void sdram_enable(void)
@@ -1034,6 +1027,9 @@
    spd_enable_refresh();
    udelay(1);

+	/* Enable normal refresh with the NB. */
+	pci_write_config8(NB, PMCR, 0x10);
+
    PRINT_DEBUG("Northbridge following SDRAM init:\n");
    DUMPNORTH();
 }

[Paul Menzel (Code Review)]

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

Change subject: intel/i440bx: Resolve long standing raminit TODOs
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38383/1/src/northbridge/intel/i440b... 
File src/northbridge/intel/i440bx/raminit.c:

https://review.coreboot.org/c/coreboot/+/38383/1/src/northbridge/intel/i440b... 
PS1, Line 1030: 	/* Enable normal refresh with the NB. */
“… with the NB.” sounds strange. Just “Enable normal refresh” or “… in the NB”?

[Angel Pons (Code Review)]

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

Change subject: intel/i440bx: Resolve long standing raminit TODOs
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38383/1/src/northbridge/intel/i440b... 
File src/northbridge/intel/i440bx/raminit.c:

https://review.coreboot.org/c/coreboot/+/38383/1/src/northbridge/intel/i440b... 
PS1, Line 1030: 	/* Enable normal refresh with the NB. */
> “… with the NB.” sounds strange. […]

How is this different from the above commented-out write?

[Keith Hui (Code Review)]

Keith Hui has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38383 )

Change subject: intel/i440bx: Resolve long standing raminit TODOs
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38383/1/src/northbridge/intel/i440b... 
File src/northbridge/intel/i440bx/raminit.c:

https://review.coreboot.org/c/coreboot/+/38383/1/src/northbridge/intel/i440b... 
PS1, Line 1030: 	/* Enable normal refresh with the NB. */
> How is this different from the above commented-out write?

The second bit enables clock gating when it senses idle across the board. This was not done with oem.

[Keith Hui (Code Review)]

Hello Patrick Rudolph, build bot (Jenkins),

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

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

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

Change subject: intel/i440bx: Resolve long standing raminit TODOs
......................................................................

intel/i440bx: Resolve long standing raminit TODOs

Drop DRAMT write as it's only rewriting the power on default.

PMCR write is required. Update comment on its purpose and move to
end of sdram_enable().

Change-Id: I62e8b2531f0f297ffb7db440db89ffa65771b7d5
Signed-off-by: Keith Hui 
---
M src/northbridge/intel/i440bx/raminit.c
1 file changed, 1 insertion(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/38383/2

[Angel Pons (Code Review)]

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

Change subject: intel/i440bx: Resolve long standing raminit TODOs
......................................................................

Patch Set 2: Code-Review+2

(2 comments)

https://review.coreboot.org/c/coreboot/+/38383/1/src/northbridge/intel/i440b... 
File src/northbridge/intel/i440bx/raminit.c:

https://review.coreboot.org/c/coreboot/+/38383/1/src/northbridge/intel/i440b... 
PS1, Line 1030: 	/* Enable normal refresh with the NB. */
> The second bit enables clock gating when it senses idle across the board. […]

Well, the commented-out line in line 1026 is the same as line 1031, which is what I was referring to. I don't think clock gating is worth the hassle for such an old platform.

In any case, done.

https://review.coreboot.org/c/coreboot/+/38383/2/src/northbridge/intel/i440b... 
File src/northbridge/intel/i440bx/raminit.c:

https://review.coreboot.org/c/coreboot/+/38383/2/src/northbridge/intel/i440b... 
PS2, Line 361: 	// TODO: Only do this later?
There's a PMCR TODO here!

[Keith Hui (Code Review)]

Keith Hui has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38383 )

Change subject: intel/i440bx: Resolve long standing raminit TODOs
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38383/2/src/northbridge/intel/i440b... 
File src/northbridge/intel/i440bx/raminit.c:

https://review.coreboot.org/c/coreboot/+/38383/2/src/northbridge/intel/i440b... 
PS2, Line 361: 	// TODO: Only do this later?
> There's a PMCR TODO here!

Agreed this should be dropped as it just mirrors power on defaults. Can it wait or should be done now?

[Angel Pons (Code Review)]

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

Change subject: intel/i440bx: Resolve long standing raminit TODOs
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38383/2/src/northbridge/intel/i440b... 
File src/northbridge/intel/i440bx/raminit.c:

https://review.coreboot.org/c/coreboot/+/38383/2/src/northbridge/intel/i440b... 
PS2, Line 361: 	// TODO: Only do this later?
> Agreed this should be dropped as it just mirrors power on defaults. […]

I'd do it on this change. It's at the top of the patch train, so it's easy to remove the comments on lines 361 and 362. I would change the comment on line 360:

Enable normal refresh, but not yet. Done after raminit.

[Keith Hui (Code Review)]

Hello Patrick Rudolph, Angel Pons, build bot (Jenkins),

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

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

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

Change subject: intel/i440bx: Resolve long standing raminit TODOs
......................................................................

intel/i440bx: Resolve long standing raminit TODOs

Drop DRAMT write as it's only rewriting the power on default.

PMCR write is required. Update comment on its purpose and move to
end of sdram_enable().

Change-Id: I62e8b2531f0f297ffb7db440db89ffa65771b7d5
Signed-off-by: Keith Hui 
---
M src/northbridge/intel/i440bx/raminit.c
1 file changed, 2 insertions(+), 12 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/38383/3

[Angel Pons (Code Review)]

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

Change subject: intel/i440bx: Resolve long standing raminit TODOs
......................................................................

Patch Set 3: Code-Review+2

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38383 )

Change subject: intel/i440bx: Resolve long standing raminit TODOs
......................................................................

intel/i440bx: Resolve long standing raminit TODOs

Drop DRAMT write as it's only rewriting the power on default.

PMCR write is required. Update comment on its purpose and move to
end of sdram_enable().

Change-Id: I62e8b2531f0f297ffb7db440db89ffa65771b7d5
Signed-off-by: Keith Hui 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/38383
Tested-by: build bot (Jenkins) 
Reviewed-by: Angel Pons 
---
M src/northbridge/intel/i440bx/raminit.c
1 file changed, 2 insertions(+), 12 deletions(-)

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

diff --git a/src/northbridge/intel/i440bx/raminit.c b/src/northbridge/intel/i440bx/raminit.c
index d5c23cf..0a864e8 100644
--- a/src/northbridge/intel/i440bx/raminit.c
+++ b/src/northbridge/intel/i440bx/raminit.c
@@ -357,10 +357,7 @@
     *         1 = Enable
     *         0 = Disable
     */
-	/* Enable normal refresh and the gated clock. */
-	// TODO: Only do this later?
-	// PMCR, 0x00, 0x14,
-	PMCR, 0x00, 0x00,
+	/* PMCR will be set later. */

/* Enable SCRR.SRRAEN and let BX choose the SRR. */
    SCRR + 1, 0x00, 0x10,
@@ -985,13 +982,6 @@

/* Setup DRAM buffer strength. */
    set_dram_buffer_strength();
-
-	/* TODO: Set PMCR? */
-	// pci_write_config8(NB, PMCR, 0x14);
-	pci_write_config8(NB, PMCR, 0x10);
-
-	/* TODO: This is for EDO memory only. */
-	pci_write_config8(NB, DRAMT, 0x03);
 }

static void sdram_enable(void)
@@ -1030,7 +1020,7 @@

/* 6. Finally enable refresh. */
    PRINT_DEBUG("RAM Enable 6: Enable refresh\n");
-	// pci_write_config8(NB, PMCR, 0x10);
+	pci_write_config8(NB, PMCR, 0x10);
    spd_enable_refresh();
    udelay(1);

[9elements QA (Code Review)]

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38383 )

Change subject: intel/i440bx: Resolve long standing raminit TODOs
......................................................................

Patch Set 4:

Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3
Emulation targets:
EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/288
EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/287
EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/286

Please note: This test is under development and might not be accurate at all!