Change in coreboot[master]: mb/intel/adlrvp: Add dq_pins_interleaved into 'struct mb_cfg'

[Subrata Banik (Code Review)]

Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46873 )

Change subject: mb/intel/adlrvp: Add dq_pins_interleaved into 'struct mb_cfg'
......................................................................

mb/intel/adlrvp: Add dq_pins_interleaved into 'struct mb_cfg'

List of changes:
1. Split mem_cfg for DDR4 and LPDDR4 as per board_id
2. Move dq_pins_interleaved into board-specific memory configuration
information

TEST=Able to build and boot DDR4 and LPDDR4 ADLRVP SKUs.

Change-Id: I6ef19209767c810426bba0c8bc48178bf2e2a110
Signed-off-by: Subrata Banik 
---
M src/mainboard/intel/adlrvp/romstage_fsp_params.c
M src/mainboard/intel/adlrvp/variants/adlrvp_p/memory.c
M src/soc/intel/alderlake/include/soc/meminit.h
M src/soc/intel/alderlake/meminit.c
4 files changed, 33 insertions(+), 11 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/46873/1

diff --git a/src/mainboard/intel/adlrvp/romstage_fsp_params.c b/src/mainboard/intel/adlrvp/romstage_fsp_params.c
index 9d7cc91..209ee6a 100644
--- a/src/mainboard/intel/adlrvp/romstage_fsp_params.c
+++ b/src/mainboard/intel/adlrvp/romstage_fsp_params.c
@@ -51,12 +51,10 @@
    switch (board_id) {
    case ADL_P_DDR4_1:
    case ADL_P_DDR4_2:
-		mupd->FspmConfig.DqPinsInterleaved = 1;
    	memcfg_init(&mupd->FspmConfig, mem_config, &ddr4_spd_info, half_populated);
    	break;
    case ADL_P_LP4_1:
    case ADL_P_LP4_2:
-		mupd->FspmConfig.DqPinsInterleaved = 0;
    	memcfg_init(&mupd->FspmConfig, mem_config, &lpddr4_spd_info, half_populated);
    	break;
    default:
diff --git a/src/mainboard/intel/adlrvp/variants/adlrvp_p/memory.c b/src/mainboard/intel/adlrvp/variants/adlrvp_p/memory.c
index f8b3660..353bb7f 100644
--- a/src/mainboard/intel/adlrvp/variants/adlrvp_p/memory.c
+++ b/src/mainboard/intel/adlrvp/variants/adlrvp_p/memory.c
@@ -5,7 +5,23 @@
 #include <baseboard/variants.h>
 #include <soc/romstage.h>

-static const struct mb_cfg mem_config = {
+static const struct mb_cfg ddr4_mem_config = {
+	/* Baseboard uses 100, 100 and 100 rcomp resistors */
+	.rcomp_resistor = {100, 100, 100},
+
+	/*
+	 * Baseboard Rcomp target values.
+	 */
+	.rcomp_targets = {40, 30, 33, 33, 30},
+
+	.dq_pins_interleaved = true,
+
+	.ect = true, /* Early Command Training */
+
+	.UserBd = BOARD_TYPE_MOBILE,
+};
+
+static const struct mb_cfg lpddr4_mem_config = {
    /* DQ byte map */
    .dq_map = {
    {   0,  2,   3,  1,   6,   7,   5,   4,    /* Byte 0 */
@@ -33,13 +49,7 @@
    { 0, 1 }, { 1, 0 }, { 1, 0 }, { 0, 1 }
    },

-	/* Baseboard uses only 100ohm Rcomp resistors */
-	.rcomp_resistor = {100, 100, 100},
-
-	/*
-	 * Baseboard Rcomp target values.
-	 */
-	.rcomp_targets = {40, 30, 33, 33, 30},
+	.dq_pins_interleaved = false,

.ect = true, /* Early Command Training */

@@ -48,5 +58,12 @@

const struct mb_cfg *variant_memory_params(void)
 {
-	return &mem_config;
+	int board_id = get_board_id();
+
+	if (board_id == ADL_P_LP4_1 || board_id == ADL_P_LP4_2)
+		return &lpddr4_mem_config;
+	else if (board_id == ADL_P_DDR4_1 || board_id == ADL_P_DDR4_2)
+		return &ddr4_mem_config;
+
+	die("unsupported board id : 0x%x\n", board_id);
 }
diff --git a/src/soc/intel/alderlake/include/soc/meminit.h b/src/soc/intel/alderlake/include/soc/meminit.h
index 76930be..5fed568 100644
--- a/src/soc/intel/alderlake/include/soc/meminit.h
+++ b/src/soc/intel/alderlake/include/soc/meminit.h
@@ -77,6 +77,12 @@
    uint16_t rcomp_targets[5];

/*
+	 * Dqs Pins Interleaved Setting. Enable/Disable Control
+	 * TRUE = enable, FALSE = disable
+	 */
+	bool dq_pins_interleaved;
+
+	/*
     * Early Command Training Enable/Disable Control
     * TRUE = enable, FALSE = disable
     */
diff --git a/src/soc/intel/alderlake/meminit.c b/src/soc/intel/alderlake/meminit.c
index e7084a5..f5f747d 100644
--- a/src/soc/intel/alderlake/meminit.c
+++ b/src/soc/intel/alderlake/meminit.c
@@ -180,4 +180,5 @@

mem_cfg->ECT = board_cfg->ect;
    mem_cfg->UserBd = board_cfg->UserBd;
+	mem_cfg->DqPinsInterleaved = board_cfg->dq_pins_interleaved;
 }

[Subrata Banik (Code Review)]

Hello Patrick Rudolph,

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

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

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

Change subject: mb/intel/adlrvp: Add dq_pins_interleaved into 'struct mb_cfg'
......................................................................

mb/intel/adlrvp: Add dq_pins_interleaved into 'struct mb_cfg'

List of changes:
1. Split mem_cfg for DDR4 and LPDDR4 as per board_id
2. Move dq_pins_interleaved into board-specific memory configuration
information

TEST=Able to build and boot DDR4 and LPDDR4 ADLRVP SKUs.

Change-Id: I6ef19209767c810426bba0c8bc48178bf2e2a110
Signed-off-by: Subrata Banik 
---
M src/mainboard/intel/adlrvp/romstage_fsp_params.c
M src/mainboard/intel/adlrvp/variants/adlrvp_p/memory.c
M src/soc/intel/alderlake/include/soc/meminit.h
M src/soc/intel/alderlake/meminit.c
4 files changed, 31 insertions(+), 11 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/46873/2

[Subrata Banik (Code Review)]

Hello build bot (Jenkins), Patrick Rudolph,

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

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

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

Change subject: mb/intel/adlrvp: Add dq_pins_interleaved into 'struct mb_cfg'
......................................................................

mb/intel/adlrvp: Add dq_pins_interleaved into 'struct mb_cfg'

List of changes:
1. Split mem_cfg for DDR4 and LPDDR4 as per board_id
2. Move dq_pins_interleaved into board-specific memory configuration
information

TEST=Able to build and boot DDR4 and LPDDR4 ADLRVP SKUs.

Change-Id: I6ef19209767c810426bba0c8bc48178bf2e2a110
Signed-off-by: Subrata Banik 
---
M src/mainboard/intel/adlrvp/romstage_fsp_params.c
M src/mainboard/intel/adlrvp/variants/adlrvp_p/memory.c
M src/soc/intel/alderlake/include/soc/meminit.h
M src/soc/intel/alderlake/meminit.c
4 files changed, 31 insertions(+), 11 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/46873/3

[Angel Pons (Code Review)]

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

Change subject: mb/intel/adlrvp: Add dq_pins_interleaved into 'struct mb_cfg'
......................................................................

Patch Set 3:

(3 comments)

https://review.coreboot.org/c/coreboot/+/46873/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/46873/3//COMMIT_MSG@7 
PS3, Line 7: into
nit: to

https://review.coreboot.org/c/coreboot/+/46873/3//COMMIT_MSG@7 
PS3, Line 7: mb/intel/adlrvp
nit: I'd use `soc/intel/alderlake` here

https://review.coreboot.org/c/coreboot/+/46873/3/src/mainboard/intel/adlrvp/... 
File src/mainboard/intel/adlrvp/variants/adlrvp_p/memory.c:

https://review.coreboot.org/c/coreboot/+/46873/3/src/mainboard/intel/adlrvp/... 
PS3, Line 17: 	.ect = true, /* Early Command Training */
I think MRC will disable ECT for DDR4, so this should be false. (AFAIUI, DDR4 PDA is not stable until basic training is done, and at that point one can already perform Late Command Training instead).

This can be addressed in a follow-up.

[Angel Pons (Code Review)]

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

Change subject: mb/intel/adlrvp: Add dq_pins_interleaved into 'struct mb_cfg'
......................................................................

Patch Set 3: Code-Review+2

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46873 )

Change subject: mb/intel/adlrvp: Add dq_pins_interleaved into 'struct mb_cfg'
......................................................................

Patch Set 3:

(3 comments)

https://review.coreboot.org/c/coreboot/+/46873/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/46873/3//COMMIT_MSG@7 
PS3, Line 7: into
> nit: to

Ack

https://review.coreboot.org/c/coreboot/+/46873/3//COMMIT_MSG@7 
PS3, Line 7: mb/intel/adlrvp
> nit: I'd use `soc/intel/alderlake` here

Ack

https://review.coreboot.org/c/coreboot/+/46873/3/src/mainboard/intel/adlrvp/... 
File src/mainboard/intel/adlrvp/variants/adlrvp_p/memory.c:

https://review.coreboot.org/c/coreboot/+/46873/3/src/mainboard/intel/adlrvp/... 
PS3, Line 17: 	.ect = true, /* Early Command Training */
> I think MRC will disable ECT for DDR4, so this should be false. […]

Got it Angel thanks

Change in coreboot[master]: soc/intel/alderlake: Add dq_pins_interleaved to 'struct mb_cfg'

[Subrata Banik (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Angel Pons, Patrick Rudolph,

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

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

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

Change subject: soc/intel/alderlake: Add dq_pins_interleaved to 'struct mb_cfg'
......................................................................

soc/intel/alderlake: Add dq_pins_interleaved to 'struct mb_cfg'

List of changes:
1. Split mem_cfg for DDR4 and LPDDR4 as per board_id
2. Move dq_pins_interleaved into board-specific memory configuration
information

TEST=Able to build and boot DDR4 and LPDDR4 ADLRVP SKUs.

Change-Id: I6ef19209767c810426bba0c8bc48178bf2e2a110
Signed-off-by: Subrata Banik 
---
M src/mainboard/intel/adlrvp/romstage_fsp_params.c
M src/mainboard/intel/adlrvp/variants/adlrvp_p/memory.c
M src/soc/intel/alderlake/include/soc/meminit.h
M src/soc/intel/alderlake/meminit.c
4 files changed, 31 insertions(+), 11 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/46873/4

[Subrata Banik (Code Review)]

Subrata Banik has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46873 )

Change subject: mb/intel/adlrvp: Add dq_pins_interleaved into 'struct mb_cfg'
......................................................................

mb/intel/adlrvp: Add dq_pins_interleaved into 'struct mb_cfg'

List of changes:
1. Split mem_cfg for DDR4 and LPDDR4 as per board_id
2. Move dq_pins_interleaved into board-specific memory configuration
information

TEST=Able to build and boot DDR4 and LPDDR4 ADLRVP SKUs.

Change-Id: I6ef19209767c810426bba0c8bc48178bf2e2a110
Signed-off-by: Subrata Banik 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/46873
Reviewed-by: Angel Pons 
Reviewed-by: Tim Wawrzynczak 
Tested-by: build bot (Jenkins) 
---
M src/mainboard/intel/adlrvp/romstage_fsp_params.c
M src/mainboard/intel/adlrvp/variants/adlrvp_p/memory.c
M src/soc/intel/alderlake/include/soc/meminit.h
M src/soc/intel/alderlake/meminit.c
4 files changed, 31 insertions(+), 11 deletions(-)

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

diff --git a/src/mainboard/intel/adlrvp/romstage_fsp_params.c b/src/mainboard/intel/adlrvp/romstage_fsp_params.c
index 9d7cc91..209ee6a 100644
--- a/src/mainboard/intel/adlrvp/romstage_fsp_params.c
+++ b/src/mainboard/intel/adlrvp/romstage_fsp_params.c
@@ -51,12 +51,10 @@
    switch (board_id) {
    case ADL_P_DDR4_1:
    case ADL_P_DDR4_2:
-		mupd->FspmConfig.DqPinsInterleaved = 1;
    	memcfg_init(&mupd->FspmConfig, mem_config, &ddr4_spd_info, half_populated);
    	break;
    case ADL_P_LP4_1:
    case ADL_P_LP4_2:
-		mupd->FspmConfig.DqPinsInterleaved = 0;
    	memcfg_init(&mupd->FspmConfig, mem_config, &lpddr4_spd_info, half_populated);
    	break;
    default:
diff --git a/src/mainboard/intel/adlrvp/variants/adlrvp_p/memory.c b/src/mainboard/intel/adlrvp/variants/adlrvp_p/memory.c
index f8b3660..c730b99 100644
--- a/src/mainboard/intel/adlrvp/variants/adlrvp_p/memory.c
+++ b/src/mainboard/intel/adlrvp/variants/adlrvp_p/memory.c
@@ -5,7 +5,21 @@
 #include <baseboard/variants.h>
 #include <soc/romstage.h>

-static const struct mb_cfg mem_config = {
+static const struct mb_cfg ddr4_mem_config = {
+	/* Baseboard uses only 100ohm Rcomp resistors */
+	.rcomp_resistor = {100, 100, 100},
+
+	/* Baseboard Rcomp target values */
+	.rcomp_targets = {40, 30, 33, 33, 30},
+
+	.dq_pins_interleaved = true,
+
+	.ect = true, /* Early Command Training */
+
+	.UserBd = BOARD_TYPE_MOBILE,
+};
+
+static const struct mb_cfg lpddr4_mem_config = {
    /* DQ byte map */
    .dq_map = {
    {   0,  2,   3,  1,   6,   7,   5,   4,    /* Byte 0 */
@@ -33,13 +47,7 @@
    { 0, 1 }, { 1, 0 }, { 1, 0 }, { 0, 1 }
    },

-	/* Baseboard uses only 100ohm Rcomp resistors */
-	.rcomp_resistor = {100, 100, 100},
-
-	/*
-	 * Baseboard Rcomp target values.
-	 */
-	.rcomp_targets = {40, 30, 33, 33, 30},
+	.dq_pins_interleaved = false,

.ect = true, /* Early Command Training */

@@ -48,5 +56,12 @@

const struct mb_cfg *variant_memory_params(void)
 {
-	return &mem_config;
+	int board_id = get_board_id();
+
+	if (board_id == ADL_P_LP4_1 || board_id == ADL_P_LP4_2)
+		return &lpddr4_mem_config;
+	else if (board_id == ADL_P_DDR4_1 || board_id == ADL_P_DDR4_2)
+		return &ddr4_mem_config;
+
+	die("unsupported board id : 0x%x\n", board_id);
 }
diff --git a/src/soc/intel/alderlake/include/soc/meminit.h b/src/soc/intel/alderlake/include/soc/meminit.h
index 76930be..5fed568 100644
--- a/src/soc/intel/alderlake/include/soc/meminit.h
+++ b/src/soc/intel/alderlake/include/soc/meminit.h
@@ -77,6 +77,12 @@
    uint16_t rcomp_targets[5];

/*
+	 * Dqs Pins Interleaved Setting. Enable/Disable Control
+	 * TRUE = enable, FALSE = disable
+	 */
+	bool dq_pins_interleaved;
+
+	/*
     * Early Command Training Enable/Disable Control
     * TRUE = enable, FALSE = disable
     */
diff --git a/src/soc/intel/alderlake/meminit.c b/src/soc/intel/alderlake/meminit.c
index e7084a5..f5f747d 100644
--- a/src/soc/intel/alderlake/meminit.c
+++ b/src/soc/intel/alderlake/meminit.c
@@ -180,4 +180,5 @@

mem_cfg->ECT = board_cfg->ect;
    mem_cfg->UserBd = board_cfg->UserBd;
+	mem_cfg->DqPinsInterleaved = board_cfg->dq_pins_interleaved;
 }