Change in ...coreboot[master]: mb/samsung/lumpy: Fix MRC raminit

[Patrick Rudolph (Code Review)]

Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32682

Change subject: mb/samsung/lumpy: Fix MRC raminit
......................................................................

mb/samsung/lumpy: Fix MRC raminit

Fix onboard SPD config placement for mrc.bin and remove outdated
comment.

ChangeId dedcc78ff44f4eb7c227ade84ee35e007f183a89 removed support for
0xf0 SPD address, but the spd_data index wasn't adjusted.

ChangeId If1b94e050d7e8d0dbd349c0415a182730aa5fa90 missed that too while
fixing the channel order for native raminit.

Change-Id: If1910e82a4bd178c2a6c2991c91e09782122888e
Signed-off-by: Patrick Rudolph 
---
M src/mainboard/samsung/lumpy/romstage.c
1 file changed, 1 insertion(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/32682/1

diff --git a/src/mainboard/samsung/lumpy/romstage.c b/src/mainboard/samsung/lumpy/romstage.c
index 1080689..a77149d 100644
--- a/src/mainboard/samsung/lumpy/romstage.c
+++ b/src/mainboard/samsung/lumpy/romstage.c
@@ -148,7 +148,6 @@
    	die("SPD data not found.");
    if (spd_file_len < (spd_index + 1) * 256)
    	die("Missing SPD data.");
-	// leave onboard dimm address at f0, and copy spd data there.
    return spd_data[spd_index];
 }

@@ -198,8 +197,7 @@
    	},
    };
    *pei_data = pei_data_template;
-	// leave onboard dimm address at f0, and copy spd data there.
-	memcpy(pei_data->spd_data[0], locate_spd(), 256);
+	memcpy(pei_data->spd_data[2], locate_spd(), 256);
 }

const struct southbridge_usb_port mainboard_usb_ports[] = {

[Patrick Rudolph (Code Review)]

Patrick Rudolph has uploaded a new patch set (#2). ( https://review.coreboot.org/c/coreboot/+/32682 )

Change subject: mb/samsung/lumpy: Fix MRC raminit
......................................................................

mb/samsung/lumpy: Fix MRC raminit

Fix onboard SPD config placement for mrc.bin and remove outdated
comment.

ChangeId I79937fd1671af23184ab830d5ba6242c8067d944 removed support for
0xf0 SPD address, but the spd_data index wasn't adjusted.

ChangeId If1b94e050d7e8d0dbd349c0415a182730aa5fa90 missed that too while
fixing the channel order for native raminit.

Change-Id: If1910e82a4bd178c2a6c2991c91e09782122888e
Signed-off-by: Patrick Rudolph 
---
M src/mainboard/samsung/lumpy/romstage.c
1 file changed, 1 insertion(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/32682/2

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

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32682 )

Change subject: mb/samsung/lumpy: Fix MRC raminit
......................................................................

Patch Set 2:

What was broken? Is this change based on hardware testing or review of source code?

You need to remember that somewhere along the way, MRC.bin _and_ version of pei_data changed, that was related to 0xf0 removal from SPD map.

It's not that long ago (maybe 4 months) when I last did fresh build of samsung/lumpy using non-native raminit (I was looking into the ill-located MRC heap) and I am pretty sure I did not have any trouble related to amount of detected memory (2 GiB on-board + 4 GiB SO-DIMM).

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32682 )

Change subject: mb/samsung/lumpy: Fix MRC raminit
......................................................................

Patch Set 2:
> Patch Set 2:
>
> What was broken? Is this change based on hardware testing or review of source code?
>
> You need to remember that somewhere along the way, MRC.bin _and_ version of pei_data changed, that was related to 0xf0 removal from SPD map.
>
> It's not that long ago (maybe 4 months) when I last did fresh build of samsung/lumpy using non-native raminit (I was looking into the ill-located MRC heap) and I am pretty sure I did not have any trouble related to amount of detected memory (2 GiB on-board + 4 GiB SO-DIMM).

It's not broken (yet). I'm migrating PEI data to devicetree: https://review.coreboot.org/q/topic:%22mrc_to_devicetree%22+(status:open%20O...)

https://review.coreboot.org/c/coreboot/+/32069 will set dimm_channel0_disabled/dimm_channel1_disabled by looking at spd_addresses and spd_data and as spd_data[2] isn't set, it will always disable channel1.

Can you test this on real hardware with mrc.bin and confirm that spd_data[2] works, too?

[Nico Huber (Code Review)]

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

Change subject: mb/samsung/lumpy: Fix MRC raminit
......................................................................

Patch Set 2: Code-Review-1

This likely won't work.

If I looked in the correct place, the MRC blob takes
`spd_data[0]` for every first DIMM per channel with
`spd_address == 0`. This would also explain the other-
wise redundant `dimm_channelx_disabled` settings. Note
that this means it ignores `spd_data[1..3]`.

What we could do is adding a special tag for `spd_
addresses` like the gone 0xf0, but this time in the
devicetree interface. e.g. for Lumpy:

register "spd_addresses" = "{ 0x50, 0, SPD_DATA, 0 }"

It would just tell the wrapper code that this "DIMM"
shouldn't be disabled. With SPD_DATA == 0x80, we could
even save us extra code:

for (i = 0; i < ARRAY_SIZE(pei->spd_addresses); ++i)
            pei->spd_addresses[i] = config->spd_addresses[i] << 1;

would make it 0x00 ^^

Change in ...coreboot[master]: mb/samsung/lumpy: Move onboard SPD to second channel

[Patrick Rudolph (Code Review)]

Hello Kyösti Mälkki, Edward O'Callaghan, Matt DeVillier, Stefan Reinauer, build bot (Jenkins), Nico Huber,

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

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

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

Change subject: mb/samsung/lumpy: Move onboard SPD to second channel
......................................................................

mb/samsung/lumpy: Move onboard SPD to second channel

Move the onboard SPD to second channel as native raminit does and workaround
mrc expecations in northbridge code.

Required to move pei data to devicetree and to use the same code for mrc and
native raminit.

Tested on Lenovo T520:
Other fields then spd_data[0] are ignored.

Change-Id: If1910e82a4bd178c2a6c2991c91e09782122888e
Signed-off-by: Patrick Rudolph 
---
M src/mainboard/samsung/lumpy/romstage.c
M src/northbridge/intel/sandybridge/raminit_mrc.c
2 files changed, 14 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/32682/3

Change in ...coreboot[master]: mb/samsung/lumpy: Move onboard SPD to second channel

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32682 )

Change subject: mb/samsung/lumpy: Move onboard SPD to second channel
......................................................................

Patch Set 2:
> Patch Set 2: Code-Review-1
>
> This likely won't work.
>
> If I looked in the correct place, the MRC blob takes
> `spd_data[0]` for every first DIMM per channel with
> `spd_address == 0`. This would also explain the other-
> wise redundant `dimm_channelx_disabled` settings. Note
> that this means it ignores `spd_data[1..3]`.
>
> What we could do is adding a special tag for `spd_
> addresses` like the gone 0xf0, but this time in the
> devicetree interface. e.g. for Lumpy:
> register "spd_addresses" = "{ 0x50, 0, SPD_DATA, 0 }"
>
> It would just tell the wrapper code that this "DIMM"
> shouldn't be disabled. With SPD_DATA == 0x80, we could
> even save us extra code:
> for (i = 0; i < ARRAY_SIZE(pei->spd_addresses); ++i)
>         pei->spd_addresses[i] = config->spd_addresses[i] << 1;
>
> would make it 0x00 ^^

My tests on Lenovo T520 showed that only spd_data[0] is used as you said. I added a fix to northbridge code that doesn't touch spd_addresses.

Change in ...coreboot[master]: mb/samsung/lumpy: Move onboard SPD to second channel

[Nico Huber (Code Review)]

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

Change subject: mb/samsung/lumpy: Move onboard SPD to second channel
......................................................................

Patch Set 3: Code-Review+2

(1 comment)

https://review.coreboot.org/#/c/32682/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32682/3//COMMIT_MSG@13 
PS3, Line 13: native raminit.
Please adhere to the line length limit in the future.

Change in ...coreboot[master]: mb/samsung/lumpy: Move onboard SPD to second channel

[Patrick Georgi (Code Review)]

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

Change subject: mb/samsung/lumpy: Move onboard SPD to second channel
......................................................................

mb/samsung/lumpy: Move onboard SPD to second channel

Move the onboard SPD to second channel as native raminit does and workaround
mrc expecations in northbridge code.

Required to move pei data to devicetree and to use the same code for mrc and
native raminit.

Tested on Lenovo T520:
Other fields then spd_data[0] are ignored.

Change-Id: If1910e82a4bd178c2a6c2991c91e09782122888e
Signed-off-by: Patrick Rudolph 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/32682
Tested-by: build bot (Jenkins) 
Reviewed-by: Nico Huber 
---
M src/mainboard/samsung/lumpy/romstage.c
M src/northbridge/intel/sandybridge/raminit_mrc.c
2 files changed, 14 insertions(+), 1 deletion(-)

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

diff --git a/src/mainboard/samsung/lumpy/romstage.c b/src/mainboard/samsung/lumpy/romstage.c
index 26b9dcc..a77149d 100644
--- a/src/mainboard/samsung/lumpy/romstage.c
+++ b/src/mainboard/samsung/lumpy/romstage.c
@@ -197,7 +197,7 @@
    	},
    };
    *pei_data = pei_data_template;
-	memcpy(pei_data->spd_data[0], locate_spd(), 256);
+	memcpy(pei_data->spd_data[2], locate_spd(), 256);
 }

const struct southbridge_usb_port mainboard_usb_ports[] = {
diff --git a/src/northbridge/intel/sandybridge/raminit_mrc.c b/src/northbridge/intel/sandybridge/raminit_mrc.c
index ea3590f..1c9e021 100644
--- a/src/northbridge/intel/sandybridge/raminit_mrc.c
+++ b/src/northbridge/intel/sandybridge/raminit_mrc.c
@@ -288,6 +288,19 @@
    mainboard_fill_pei_data(&pei_data);

post_code(0x3a);
+
+	/* Fix spd_data. MRC only uses spd_data[0] and ignores the other */
+	for (size_t i = 1; i < ARRAY_SIZE(pei_data.spd_data); i++) {
+		if (pei_data.spd_data[i][0] && !pei_data.spd_data[0][0]) {
+			memcpy(pei_data.spd_data[0], pei_data.spd_data[i],
+			       sizeof(pei_data.spd_data[0]));
+		} else if (pei_data.spd_data[i][0] && pei_data.spd_data[0][0]) {
+			if (memcmp(pei_data.spd_data[i], pei_data.spd_data[0],
+			    sizeof(pei_data.spd_data[0])) != 0)
+				die("Onboard SPDs must match each other");
+		}
+	}
+
    pei_data.boot_mode = s3resume ? 2 : 0;
    timestamp_add_now(TS_BEFORE_INITRAM);
    sdram_initialize(&pei_data);

Change in coreboot[master]: mb/samsung/lumpy: Move onboard SPD to second channel

[Angel Pons (Code Review)]

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

Change subject: mb/samsung/lumpy: Move onboard SPD to second channel
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/32682/4/src/mainboard/samsung/lumpy... 
File src/mainboard/samsung/lumpy/romstage.c:

https://review.coreboot.org/c/coreboot/+/32682/4/src/mainboard/samsung/lumpy... 
PS4, Line 200: 	memcpy(pei_data->spd_data[2], locate_spd(), 256);
What is intended with this change? Breaking samsung/lumpy?