Change in coreboot[master]: mb/asrock/h110m/romstage.c: Correct FSP-M UPDs

[Angel Pons (Code Review)]

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46246 )

Change subject: mb/asrock/h110m/romstage.c: Correct FSP-M UPDs
......................................................................

mb/asrock/h110m/romstage.c: Correct FSP-M UPDs

The DQ and DQS byte maps do not apply to DDR4 configurations, and the
RCOMP resistor and target values are not correct for SKL-S (or KBL-S).

Drop the byte maps and use RCOMP values for the correct platform type.
These can be found using Intel Document #573387 and the RCOMP resistor
values for SKL-S have been confirmed by measuring them on an i5-6400.

Change-Id: I699d46b9b516be8946367e6d9b24883ae1e78d03
Signed-off-by: Angel Pons 
---
M src/mainboard/asrock/h110m/romstage.c
1 file changed, 13 insertions(+), 46 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/46246/1

diff --git a/src/mainboard/asrock/h110m/romstage.c b/src/mainboard/asrock/h110m/romstage.c
index 7a17915..8e72e8e 100644
--- a/src/mainboard/asrock/h110m/romstage.c
+++ b/src/mainboard/asrock/h110m/romstage.c
@@ -1,61 +1,25 @@
 /* SPDX-License-Identifier: GPL-2.0-only */

+#include <assert.h>
 #include <soc/romstage.h>
+#include <stdint.h>
 #include <string.h>
 #include <spd_bin.h>

-static void mainboard_fill_dq_map_data(void *dq_map_ch0, void *dq_map_ch1)
-{
-	/* DQ byte map */
-	const u8 dq_map[2][12] = {
-		  { 0x0F, 0xF0, 0x00, 0xF0, 0x0F, 0xF0,
-		    0x0F, 0x00, 0xFF, 0x00, 0xFF, 0x00 },
-		  { 0x33, 0xCC, 0x00, 0xCC, 0x33, 0xCC,
-		    0x33, 0x00, 0xFF, 0x00, 0xFF, 0x00 } };
-	memcpy(dq_map_ch0, dq_map[0], sizeof(dq_map[0]));
-	memcpy(dq_map_ch1, dq_map[1], sizeof(dq_map[1]));
-}
-
-static void mainboard_fill_dqs_map_data(void *dqs_map_ch0, void *dqs_map_ch1)
-{
-	/* DQS CPU<>DRAM map */
-	const u8 dqs_map[2][8] = {
-		{ 0, 1, 3, 2, 4, 5, 6, 7 },
-		{ 1, 0, 4, 5, 2, 3, 6, 7 } };
-	memcpy(dqs_map_ch0, dqs_map[0], sizeof(dqs_map[0]));
-	memcpy(dqs_map_ch1, dqs_map[1], sizeof(dqs_map[1]));
-}
-
-static void mainboard_fill_rcomp_res_data(void *rcomp_ptr)
-{
-	/* Rcomp resistor */
-	const u16 RcompResistor[3] = { 200, 81, 162 };
-	memcpy(rcomp_ptr, RcompResistor,
-		 sizeof(RcompResistor));
-}
-
-static void mainboard_fill_rcomp_strength_data(void *rcomp_strength_ptr)
-{
-	/* Rcomp target */
-	static const u16 RcompTarget[5] = {
-		100, 40, 40, 23, 40 };
-	memcpy(rcomp_strength_ptr, RcompTarget, sizeof(RcompTarget));
-}
-
 void mainboard_memory_init_params(FSPM_UPD *mupd)
 {
-	FSP_M_CONFIG *mem_cfg;
+	const u16 rcomp_resistors[3] = { 121, 75, 100 };
+
+	const u16 rcomp_targets[5] = { 50, 26, 20, 20, 26 };
+
+	FSP_M_CONFIG *const mem_cfg = &mupd->FspmConfig;
+
    struct spd_block blk = {
    	.addr_map = { 0x50, 0x51, 0x52, 0x53, },
    };

-	mem_cfg = &mupd->FspmConfig;
-	mainboard_fill_dq_map_data(&mem_cfg->DqByteMapCh0,
-				   &mem_cfg->DqByteMapCh1);
-	mainboard_fill_dqs_map_data(&mem_cfg->DqsMapCpu2DramCh0,
-				    &mem_cfg->DqsMapCpu2DramCh1);
-	mainboard_fill_rcomp_res_data(&mem_cfg->RcompResistor);
-	mainboard_fill_rcomp_strength_data(&mem_cfg->RcompTarget);
+	assert(sizeof(mem_cfg->RcompResistor) == sizeof(rcomp_resistors));
+	assert(sizeof(mem_cfg->RcompTarget)   == sizeof(rcomp_targets));

mem_cfg->DqPinsInterleaved = 1;
    get_spd_smbus(&blk);
@@ -66,6 +30,9 @@
    mem_cfg->MemorySpdPtr11 = (uintptr_t)blk.spd_array[3];
    dump_spd_info(&blk);

+	memcpy(mem_cfg->RcompResistor, rcomp_resistors, sizeof(mem_cfg->RcompResistor));
+	memcpy(mem_cfg->RcompTarget,   rcomp_targets,   sizeof(mem_cfg->RcompTarget));
+
    /* use virtual channel 1 for the dmi interface of the PCH */
    mupd->FspmTestConfig.DmiVc1 = 1;

[build bot (Jenkins) (Code Review)]

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

Change subject: mb/asrock/h110m/romstage.c: Correct FSP-M UPDs
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46246/1/src/mainboard/asrock/h110m/... 
File src/mainboard/asrock/h110m/romstage.c:

https://review.coreboot.org/c/coreboot/+/46246/1/src/mainboard/asrock/h110m/... 
PS1, Line 15: 	FSP_M_CONFIG *const mem_cfg = &mupd->FspmConfig;
need consistent spacing around '*' (ctx:WxV)

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46246 )

Change subject: mb/asrock/h110m/romstage.c: Correct FSP-M UPDs
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46246/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/46246/1//COMMIT_MSG@9 
PS1, Line 9: not apply to DDR4 configurations
does only apply to LPDDR* configurations

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46246 )

Change subject: mb/asrock/h110m/romstage.c: Correct FSP-M UPDs
......................................................................

Patch Set 1: Code-Review+2

[Angel Pons (Code Review)]

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

Change subject: mb/asrock/h110m/romstage.c: Correct FSP-M UPDs
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46246/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/46246/1//COMMIT_MSG@9 
PS1, Line 9: not apply to DDR4 configurations
> does only apply to LPDDR* configurations

I've no idea if DDR5 could use the byte maps. Plus, the board at hand only supports DDR4, so why would I talk about LPDDR and risk being wrong about it? You know, I don't like being mistaken in writings that will eventually get set in stone (e.g. commit messages)... 😆

[Nico Huber (Code Review)]

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

Change subject: mb/asrock/h110m/romstage.c: Correct FSP-M UPDs
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46246/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/46246/1//COMMIT_MSG@13 
PS1, Line 13: can be found using Intel Document #573387
Can they?

The only S target values I can find are for CFL-S 2DPC. If you want this
to be accurate, you could say something like "can be guessed from".

With only that document as reference, there is still a chance that the
first target value should be 60. For the better documented H values, it
started with 50 for 1DPC and 60 for 2DPC, but that's for SO-DIMM. The 60
was later lowered to 50 (CFL-H), but not for regular DIMMs. So there is
a slight chance that regular DIMMs always should have a 60 there. When
they lowered it for the SO-DIMMs, a comment says that the old value
wouldn't hurt.

I have no final suggestion here. A hunch tells me to take the 60 and
comment that 50 is also possible.

One could also ask Intel, ofc.

[Angel Pons (Code Review)]

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

Change subject: mb/asrock/h110m/romstage.c: Correct FSP-M UPDs
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46246/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/46246/1//COMMIT_MSG@13 
PS1, Line 13: can be found using Intel Document #573387
> Can they? […]

Just checked, the last three RCOMP targets appear in the datasheet volume 1. The other 2 are trainable.

[Angel Pons (Code Review)]

Hello build bot (Jenkins), Michael Niewöhner,

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

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

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

Change subject: mb/asrock/h110m/romstage.c: Correct FSP-M UPDs
......................................................................

mb/asrock/h110m/romstage.c: Correct FSP-M UPDs

The DQ and DQS byte maps do not apply to DDR4 configurations, and the
RCOMP resistor and target values are not correct for SKL-S (or KBL-S).
Drop the byte maps and use RCOMP values for the correct platform type.

RCOMP resistor values for all non-socketed platforms are listed in the
Platform Design Guide, and also appear in schematics. For SKL-S, the
RCOMP resistors are on the CPU and their values have been confirmed
by measuring them on an i5-6400, and match the PDG values for SKL-H.

RCOMP target values can be guessed from Intel Document #573387 and some
of them are also present in datasheet volume 1, under DC specifications.

Change-Id: I699d46b9b516be8946367e6d9b24883ae1e78d03
Signed-off-by: Angel Pons 
---
M src/mainboard/asrock/h110m/romstage.c
1 file changed, 13 insertions(+), 46 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/46246/2

[Angel Pons (Code Review)]

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

Change subject: mb/asrock/h110m/romstage.c: Correct FSP-M UPDs
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46246/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/46246/1//COMMIT_MSG@13 
PS1, Line 13: can be found using Intel Document #573387
> Just checked, the last three RCOMP targets appear in the datasheet volume 1. […]

Done

[Michael Niewöhner (Code Review)]

Michael Niewöhner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46246 )

Change subject: mb/asrock/h110m/romstage.c: Correct FSP-M UPDs
......................................................................

mb/asrock/h110m/romstage.c: Correct FSP-M UPDs

The DQ and DQS byte maps do not apply to DDR4 configurations, and the
RCOMP resistor and target values are not correct for SKL-S (or KBL-S).
Drop the byte maps and use RCOMP values for the correct platform type.

RCOMP resistor values for all non-socketed platforms are listed in the
Platform Design Guide, and also appear in schematics. For SKL-S, the
RCOMP resistors are on the CPU and their values have been confirmed
by measuring them on an i5-6400, and match the PDG values for SKL-H.

RCOMP target values can be guessed from Intel Document #573387 and some
of them are also present in datasheet volume 1, under DC specifications.

Change-Id: I699d46b9b516be8946367e6d9b24883ae1e78d03
Signed-off-by: Angel Pons 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/46246
Tested-by: build bot (Jenkins) 
Reviewed-by: Michael Niewöhner 
---
M src/mainboard/asrock/h110m/romstage.c
1 file changed, 13 insertions(+), 46 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Michael Niewöhner: Looks good to me, approved

diff --git a/src/mainboard/asrock/h110m/romstage.c b/src/mainboard/asrock/h110m/romstage.c
index 7a17915..8e72e8e 100644
--- a/src/mainboard/asrock/h110m/romstage.c
+++ b/src/mainboard/asrock/h110m/romstage.c
@@ -1,61 +1,25 @@
 /* SPDX-License-Identifier: GPL-2.0-only */

+#include <assert.h>
 #include <soc/romstage.h>
+#include <stdint.h>
 #include <string.h>
 #include <spd_bin.h>

-static void mainboard_fill_dq_map_data(void *dq_map_ch0, void *dq_map_ch1)
-{
-	/* DQ byte map */
-	const u8 dq_map[2][12] = {
-		  { 0x0F, 0xF0, 0x00, 0xF0, 0x0F, 0xF0,
-		    0x0F, 0x00, 0xFF, 0x00, 0xFF, 0x00 },
-		  { 0x33, 0xCC, 0x00, 0xCC, 0x33, 0xCC,
-		    0x33, 0x00, 0xFF, 0x00, 0xFF, 0x00 } };
-	memcpy(dq_map_ch0, dq_map[0], sizeof(dq_map[0]));
-	memcpy(dq_map_ch1, dq_map[1], sizeof(dq_map[1]));
-}
-
-static void mainboard_fill_dqs_map_data(void *dqs_map_ch0, void *dqs_map_ch1)
-{
-	/* DQS CPU<>DRAM map */
-	const u8 dqs_map[2][8] = {
-		{ 0, 1, 3, 2, 4, 5, 6, 7 },
-		{ 1, 0, 4, 5, 2, 3, 6, 7 } };
-	memcpy(dqs_map_ch0, dqs_map[0], sizeof(dqs_map[0]));
-	memcpy(dqs_map_ch1, dqs_map[1], sizeof(dqs_map[1]));
-}
-
-static void mainboard_fill_rcomp_res_data(void *rcomp_ptr)
-{
-	/* Rcomp resistor */
-	const u16 RcompResistor[3] = { 200, 81, 162 };
-	memcpy(rcomp_ptr, RcompResistor,
-		 sizeof(RcompResistor));
-}
-
-static void mainboard_fill_rcomp_strength_data(void *rcomp_strength_ptr)
-{
-	/* Rcomp target */
-	static const u16 RcompTarget[5] = {
-		100, 40, 40, 23, 40 };
-	memcpy(rcomp_strength_ptr, RcompTarget, sizeof(RcompTarget));
-}
-
 void mainboard_memory_init_params(FSPM_UPD *mupd)
 {
-	FSP_M_CONFIG *mem_cfg;
+	const u16 rcomp_resistors[3] = { 121, 75, 100 };
+
+	const u16 rcomp_targets[5] = { 50, 26, 20, 20, 26 };
+
+	FSP_M_CONFIG *const mem_cfg = &mupd->FspmConfig;
+
    struct spd_block blk = {
    	.addr_map = { 0x50, 0x51, 0x52, 0x53, },
    };

-	mem_cfg = &mupd->FspmConfig;
-	mainboard_fill_dq_map_data(&mem_cfg->DqByteMapCh0,
-				   &mem_cfg->DqByteMapCh1);
-	mainboard_fill_dqs_map_data(&mem_cfg->DqsMapCpu2DramCh0,
-				    &mem_cfg->DqsMapCpu2DramCh1);
-	mainboard_fill_rcomp_res_data(&mem_cfg->RcompResistor);
-	mainboard_fill_rcomp_strength_data(&mem_cfg->RcompTarget);
+	assert(sizeof(mem_cfg->RcompResistor) == sizeof(rcomp_resistors));
+	assert(sizeof(mem_cfg->RcompTarget)   == sizeof(rcomp_targets));

mem_cfg->DqPinsInterleaved = 1;
    get_spd_smbus(&blk);
@@ -66,6 +30,9 @@
    mem_cfg->MemorySpdPtr11 = (uintptr_t)blk.spd_array[3];
    dump_spd_info(&blk);

+	memcpy(mem_cfg->RcompResistor, rcomp_resistors, sizeof(mem_cfg->RcompResistor));
+	memcpy(mem_cfg->RcompTarget,   rcomp_targets,   sizeof(mem_cfg->RcompTarget));
+
    /* use virtual channel 1 for the dmi interface of the PCH */
    mupd->FspmTestConfig.DmiVc1 = 1;