Change in ...coreboot[master]: soc/intel/cannonlake: Use MEM_CH_SEL to initialize MemorySpdPrts

[Shelley Chen (Code Review)]

Shelley Chen has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31262

Change subject: soc/intel/cannonlake: Use MEM_CH_SEL to initialize MemorySpdPrts
......................................................................

soc/intel/cannonlake: Use MEM_CH_SEL to initialize MemorySpdPrts

MEM_CH_SEL is used to indicate whether we are on a single or dual
channel device, where MEM_CH_SEL = 1 for single channel skus and
MEM_CH_SEL = 0 for dual channel skus.  Initialize MemorySpdPrt
pointers based on the value read from MEM_CH_SEL, which is read from
GPP_F2.  In the first build, we did not use GPP_F2, so we need to add
an internal pulldown as those early devices were all dual channel
devices.

BUG=b:123062346, b:122959294
BRANCH=None
TEST=Boot into current boards and ensure that we have 2 channels as expected
     Also, verify that GPP_F2 is set to 0.

Change-Id: Ice22b103664187834e255d1359bfd9b51993b5b6
Signed-off-by: Shelley Chen 
---
M src/soc/intel/cannonlake/cnl_memcfg_init.c
1 file changed, 12 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/31262/1

diff --git a/src/soc/intel/cannonlake/cnl_memcfg_init.c b/src/soc/intel/cannonlake/cnl_memcfg_init.c
index 4425862..8bd82a6 100644
--- a/src/soc/intel/cannonlake/cnl_memcfg_init.c
+++ b/src/soc/intel/cannonlake/cnl_memcfg_init.c
@@ -15,7 +15,9 @@
 #include <assert.h>
 #include <console/console.h>
 #include <fsp/util.h>
+#include <gpio.h>
 #include <soc/cnl_memcfg_init.h>
+#include <soc/gpio_soc_defs.h>
 #include <spd_bin.h>
 #include <string.h>

@@ -55,8 +57,16 @@
    mem_cfg->MemorySpdDataLen = spd_data_len;
    mem_cfg->MemorySpdPtr00 = spd_data_ptr;

-	/* Use the same spd data for channel 1, Dimm 0 */
-	mem_cfg->MemorySpdPtr10 = mem_cfg->MemorySpdPtr00;
+	/*
+	 * GPP_F2 is the MEM_CH_SEL gpio, which is set to 1 for single
+	 * channel skus and 0 for dual channel skus.
+	 */
+	if (gpio_get(GPP_F2) == 1)
+		mem_cfg->MemorySpdPtr10 = 0;
+	else {
+		/* Use the same spd data for channel 1, Dimm 0 */
+		mem_cfg->MemorySpdPtr10 = mem_cfg->MemorySpdPtr00;
+	}
 }

/*

[Shelley Chen (Code Review)]

Hello Patrick Rudolph, build bot (Jenkins),

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

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

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

Change subject: soc/intel/cannonlake: Use MEM_CH_SEL to initialize MemorySpdPrts
......................................................................

soc/intel/cannonlake: Use MEM_CH_SEL to initialize MemorySpdPrts

MEM_CH_SEL is used to indicate whether we are on a single or dual
channel device, where MEM_CH_SEL = 1 for single channel skus and
MEM_CH_SEL = 0 for dual channel skus.  Initialize MemorySpdPrt
pointers based on the value read from MEM_CH_SEL, which is read from
GPP_F2.  In the first build, we did not use GPP_F2, so we need to add
an internal pulldown as those early devices were all dual channel
devices.

BUG=b:123062346, b:122959294
BRANCH=None
TEST=Boot into current boards and ensure that we have 2 channels as expected
     Also, verify that GPP_F2 is set to 0.

Change-Id: Ice22b103664187834e255d1359bfd9b51993b5b6
Signed-off-by: Shelley Chen 
---
M src/mainboard/google/hatch/variants/baseboard/gpio.c
M src/soc/intel/cannonlake/cnl_memcfg_init.c
2 files changed, 16 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/31262/2

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31262 )

Change subject: soc/intel/cannonlake: Use MEM_CH_SEL to initialize MemorySpdPrts
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/31262/2/src/soc/intel/cannonlake/cnl_memcfg_... 
File src/soc/intel/cannonlake/cnl_memcfg_init.c:

https://review.coreboot.org/#/c/31262/2/src/soc/intel/cannonlake/cnl_memcfg_... 
PS2, Line 64: gpio_get(GPP_F2) == 1)
This is not going to be true for all boards using CNL PCH. You will instead have to add a parameter to cnl_mb_cfg indicating single_channel and use that here.

Change in ...coreboot[master]: soc/intel/cannonlake: Add field to identify single channel skus

[Shelley Chen (Code Review)]

Hello Patrick Rudolph, build bot (Jenkins),

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

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

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

Change subject: soc/intel/cannonlake: Add field to identify single channel skus
......................................................................

soc/intel/cannonlake: Add field to identify single channel skus

Variants of Hatch need to accommodate single channel DDR.  Also,
removing const modifier as we'll need to set these fields
incrementally now.

BUG=b:123062346, b:122959294
BRANCH=None
TEST=Boot into current boards and ensure that we have 2 channels as expected

Change-Id: Ice22b103664187834e255d1359bfd9b51993b5b6
Signed-off-by: Shelley Chen 
---
M src/soc/intel/cannonlake/cnl_memcfg_init.c
M src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h
2 files changed, 23 insertions(+), 10 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/31262/5

Change in ...coreboot[master]: soc/intel/cannonlake: Add field to identify single channel skus

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31262 )

Change subject: soc/intel/cannonlake: Add field to identify single channel skus
......................................................................

Patch Set 5:

(2 comments)

https://review.coreboot.org/#/c/31262/5/src/soc/intel/cannonlake/cnl_memcfg_... 
File src/soc/intel/cannonlake/cnl_memcfg_init.c:

https://review.coreboot.org/#/c/31262/5/src/soc/intel/cannonlake/cnl_memcfg_... 
PS5, Line 18: #include <gpio.h>
Is this required?

https://review.coreboot.org/#/c/31262/5/src/soc/intel/cannonlake/cnl_memcfg_... 
PS5, Line 20: #include <soc/gpio_soc_defs.h>
I think you need to include soc/gpio.h here and that will include the right gpio_soc_defs / gpio_soc_defs_cnp_h header file based on the PCH type.

Change in ...coreboot[master]: soc/intel/cannonlake: Add field to identify single channel skus

[Shelley Chen (Code Review)]

Hello Patrick Rudolph, build bot (Jenkins),

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

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

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

Change subject: soc/intel/cannonlake: Add field to identify single channel skus
......................................................................

soc/intel/cannonlake: Add field to identify single channel skus

Variants of Hatch need to accommodate single channel DDR.  Also,
removing const modifier as we'll need to set these fields
incrementally now.

BUG=b:123062346, b:122959294
BRANCH=None
TEST=Boot into current boards and ensure that we have 2 channels as expected

Change-Id: Ice22b103664187834e255d1359bfd9b51993b5b6
Signed-off-by: Shelley Chen 
---
M src/soc/intel/cannonlake/cnl_memcfg_init.c
M src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h
2 files changed, 22 insertions(+), 10 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/31262/6

Change in ...coreboot[master]: soc/intel/cannonlake: Add field to identify single channel skus

[Shelley Chen (Code Review)]

Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31262 )

Change subject: soc/intel/cannonlake: Add field to identify single channel skus
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/#/c/31262/5/src/soc/intel/cannonlake/cnl_memcfg_... 
File src/soc/intel/cannonlake/cnl_memcfg_init.c:

https://review.coreboot.org/#/c/31262/5/src/soc/intel/cannonlake/cnl_memcfg_... 
PS5, Line 20: #include <soc/gpio_soc_defs.h>
> I think you need to include soc/gpio. […]

Thanks a lot, Furquan.  This fixed things.

Change in ...coreboot[master]: soc/intel/cannonlake: Add field to identify single channel skus

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31262 )

Change subject: soc/intel/cannonlake: Add field to identify single channel skus
......................................................................

Patch Set 6:

(4 comments)

https://review.coreboot.org/#/c/31262/6//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/31262/6//COMMIT_MSG@7 
PS6, Line 7: skus
memory. I don't think SoC cares about the concept of SKUs. It is mostly the mainboard.

https://review.coreboot.org/#/c/31262/6//COMMIT_MSG@8 
PS6, Line 8: 
It would be good to add some comment indicating that SPD data passed to FSP differs based on the single channel flag.

https://review.coreboot.org/#/c/31262/6/src/soc/intel/cannonlake/cnl_memcfg_... 
File src/soc/intel/cannonlake/cnl_memcfg_init.c:

https://review.coreboot.org/#/c/31262/6/src/soc/intel/cannonlake/cnl_memcfg_... 
PS6, Line 59: )
Typically you would use { } for both if and else blocks or none.

https://review.coreboot.org/#/c/31262/6/src/soc/intel/cannonlake/include/soc... 
File src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h:

https://review.coreboot.org/#/c/31262/6/src/soc/intel/cannonlake/include/soc... 
PS6, Line 108: current sku
memory configuration

Change in ...coreboot[master]: soc/intel/cannonlake: Add field to identify single channel memory

[Shelley Chen (Code Review)]

Hello Patrick Rudolph, build bot (Jenkins),

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

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

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

Change subject: soc/intel/cannonlake: Add field to identify single channel memory
......................................................................

soc/intel/cannonlake: Add field to identify single channel memory

Variants of Hatch need to accommodate single channel DDR.  Also,
removing const modifier as we'll need to set these fields
incrementally now.

BUG=b:123062346, b:122959294
BRANCH=None
TEST=Boot into current boards and ensure that we have 2 channels as expected

Change-Id: Ice22b103664187834e255d1359bfd9b51993b5b6
Signed-off-by: Shelley Chen 
---
M src/soc/intel/cannonlake/cnl_memcfg_init.c
M src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h
2 files changed, 23 insertions(+), 10 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/31262/7

Change in ...coreboot[master]: soc/intel/cannonlake: Add field to identify single channel memory

[Shelley Chen (Code Review)]

Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31262 )

Change subject: soc/intel/cannonlake: Add field to identify single channel memory
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/#/c/31262/6//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/31262/6//COMMIT_MSG@8 
PS6, Line 8:
> It would be good to add some comment indicating that SPD data passed to FSP differs based on the sin […]

but the same spd files are used, right?

Change in ...coreboot[master]: soc/intel/cannonlake: Add field to identify single channel memory

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31262 )

Change subject: soc/intel/cannonlake: Add field to identify single channel memory
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/#/c/31262/6//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/31262/6//COMMIT_MSG@8 
PS6, Line 8:
> but the same spd files are used, right?

Correct. I meant indicating that SpdDataPtr gets set differently based on single v/s dual channel.

Change in ...coreboot[master]: soc/intel/cannonlake: Add field to identify single channel memory

[Shelley Chen (Code Review)]

Hello Patrick Rudolph, build bot (Jenkins),

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

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

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

Change subject: soc/intel/cannonlake: Add field to identify single channel memory
......................................................................

soc/intel/cannonlake: Add field to identify single channel memory

Variants of Hatch need to accommodate single channel DDR.  Also,
removing const modifier as we'll need to set these fields
incrementally now.  For the single channel configuration, we set
MemorySpdPtr10 to 0.  For the dual channle configuration, we set
MemorySpdPtr10 to MemorySpdPtr00.

BUG=b:123062346, b:122959294
BRANCH=None
TEST=Boot into current boards and ensure that we have 2 channels as expected

Change-Id: Ice22b103664187834e255d1359bfd9b51993b5b6
Signed-off-by: Shelley Chen 
---
M src/soc/intel/cannonlake/cnl_memcfg_init.c
M src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h
2 files changed, 23 insertions(+), 10 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/31262/8

Change in ...coreboot[master]: soc/intel/cannonlake: Add field to identify single channel memory

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31262 )

Change subject: soc/intel/cannonlake: Add field to identify single channel memory
......................................................................

Patch Set 8: Code-Review+2

Change in ...coreboot[master]: soc/intel/cannonlake: Add field to identify single channel memory

[Patrick Rudolph (Code Review)]

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

Change subject: soc/intel/cannonlake: Add field to identify single channel memory
......................................................................

Patch Set 8:

(6 comments)

The whole cannonlake_memcfg_init() seems to be not designed very well.
Please improve the api.

You could add an enum for each DIMM:
NOT_EXISTING,
READ_SMBUS,
READ_SPD_CBFS,
READ_SPD_MEMPTR

That way the board config would hold all possible combinations.

https://review.coreboot.org/#/c/31262/8/src/soc/intel/cannonlake/cnl_memcfg_... 
File src/soc/intel/cannonlake/cnl_memcfg_init.c:

https://review.coreboot.org/#/c/31262/8/src/soc/intel/cannonlake/cnl_memcfg_... 
PS8, Line 52: static void meminit_memcfg_spd(FSP_M_CONFIG *mem_cfg,
only used once in this file.
Please squash with meminit_spd_data().

https://review.coreboot.org/#/c/31262/8/src/soc/intel/cannonlake/cnl_memcfg_... 
PS8, Line 53: 			const struct cnl_mb_cfg *board_cfg,
why is it board_cfg, but below it's cnl_cfg ?

https://review.coreboot.org/#/c/31262/8/src/soc/intel/cannonlake/cnl_memcfg_... 
PS8, Line 60: 		mem_cfg->MemorySpdPtr10 = 0;
why do you assume channel0 is connected to DRAM ?
That's not documented.

https://review.coreboot.org/#/c/31262/8/src/soc/intel/cannonlake/cnl_memcfg_... 
PS8, Line 103: 			const struct cnl_mb_cfg *cnl_cfg,
why isn't struct spd_info part if struct cnl_mb_cfg ?

https://review.coreboot.org/#/c/31262/8/src/soc/intel/cannonlake/cnl_memcfg_... 
PS8, Line 112: 	/* Spd pointer will only be used if all smbus slave address of memory
why ?

https://review.coreboot.org/#/c/31262/8/src/soc/intel/cannonlake/include/soc... 
File src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h:

https://review.coreboot.org/#/c/31262/8/src/soc/intel/cannonlake/include/soc... 
PS8, Line 108: 	 * Flag to indicate if the memory configuration is single
it's only used for soldered DRAM where SPD resides in CBFS, but that's not documented

Change in ...coreboot[master]: soc/intel/cannonlake: Add field to identify single channel memory

[Shelley Chen (Code Review)]

Hello Patrick Rudolph, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: soc/intel/cannonlake: Add field to identify single channel memory
......................................................................

soc/intel/cannonlake: Add field to identify single channel memory

Variants of Hatch need to accommodate single channel DDR.  Also,
removing const modifier as we'll need to set these fields
incrementally now.  For the single channel configuration, we set
MemorySpdPtr10 to 0.  For the dual channle configuration, we set
MemorySpdPtr10 to MemorySpdPtr00.

BUG=b:123062346, b:122959294
BRANCH=None
TEST=Boot into current boards and ensure that we have 2 channels as expected

Change-Id: Ice22b103664187834e255d1359bfd9b51993b5b6
Signed-off-by: Shelley Chen 
---
M src/soc/intel/cannonlake/cnl_memcfg_init.c
M src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h
2 files changed, 29 insertions(+), 12 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/31262/9

Change in ...coreboot[master]: soc/intel/cannonlake: Add field to identify single channel memory

[Shelley Chen (Code Review)]

Hello Patrick Rudolph, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: soc/intel/cannonlake: Add field to identify single channel memory
......................................................................

soc/intel/cannonlake: Add field to identify single channel memory

Variants of Hatch need to accommodate single channel DDR.  Also,
removing const modifier as we'll need to set these fields
incrementally now.  For the single channel configuration, we set
MemorySpdPtr10 to 0.  For the dual channle configuration, we set
MemorySpdPtr10 to MemorySpdPtr00.

BUG=b:123062346, b:122959294
BRANCH=None
TEST=Boot into current boards and ensure that we have 2 channels as expected

Change-Id: Ice22b103664187834e255d1359bfd9b51993b5b6
Signed-off-by: Shelley Chen 
---
M src/soc/intel/cannonlake/cnl_memcfg_init.c
M src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h
2 files changed, 31 insertions(+), 12 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/31262/10

Change in ...coreboot[master]: soc/intel/cannonlake: Add field to identify single channel memory

[Shelley Chen (Code Review)]

Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31262 )

Change subject: soc/intel/cannonlake: Add field to identify single channel memory
......................................................................

Patch Set 10:

(6 comments)

https://review.coreboot.org/#/c/31262/8/src/soc/intel/cannonlake/cnl_memcfg_... 
File src/soc/intel/cannonlake/cnl_memcfg_init.c:

https://review.coreboot.org/#/c/31262/8/src/soc/intel/cannonlake/cnl_memcfg_... 
PS8, Line 52: static void meminit_memcfg_spd(FSP_M_CONFIG *mem_cfg,
> only used once in this file. […]

I can take a look at this in a follow up cleanup CL.

https://review.coreboot.org/#/c/31262/8/src/soc/intel/cannonlake/cnl_memcfg_... 
PS8, Line 53: 			const struct cnl_mb_cfg *board_cfg,
> why is it board_cfg, but below it's cnl_cfg ?

Ack

https://review.coreboot.org/#/c/31262/8/src/soc/intel/cannonlake/cnl_memcfg_... 
PS8, Line 60: 		mem_cfg->MemorySpdPtr10 = 0;
> why do you assume channel0 is connected to DRAM ? […]

Have updated the flag to accommodate single channel configurations with either channel 0 or 1.

https://review.coreboot.org/#/c/31262/8/src/soc/intel/cannonlake/cnl_memcfg_... 
PS8, Line 103: 			const struct cnl_mb_cfg *cnl_cfg,
> why isn't struct spd_info part if struct cnl_mb_cfg ?

I can look into doing this in a follow up clean up CL.

https://review.coreboot.org/#/c/31262/8/src/soc/intel/cannonlake/cnl_memcfg_... 
PS8, Line 112: 	/* Spd pointer will only be used if all smbus slave address of memory
> why ?

This code will be refactored with the followup clean up CL.  Once we can detect by DRAM type (smbus, spd, etc.) we can check with that enum type instead of whether spd_smbus_address is set to 0.

https://review.coreboot.org/#/c/31262/8/src/soc/intel/cannonlake/include/soc... 
File src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h:

https://review.coreboot.org/#/c/31262/8/src/soc/intel/cannonlake/include/soc... 
PS8, Line 108: 	 * Flag to indicate if the memory configuration is single
> it's only used for soldered DRAM where SPD resides in CBFS, but that's not documented

Added some comments to address this.

Change in ...coreboot[master]: soc/intel/cannonlake: Add field to identify single channel memory

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31262 )

Change subject: soc/intel/cannonlake: Add field to identify single channel memory
......................................................................

Patch Set 10:

(4 comments)

https://review.coreboot.org/#/c/31262/10/src/soc/intel/cannonlake/cnl_memcfg... 
File src/soc/intel/cannonlake/cnl_memcfg_init.c:

https://review.coreboot.org/#/c/31262/10/src/soc/intel/cannonlake/cnl_memcfg... 
PS10, Line 19: #include <soc/gpio.h>
Why is this required?

https://review.coreboot.org/#/c/31262/10/src/soc/intel/cannonlake/cnl_memcfg... 
PS10, Line 53: cnl_cfg
maybe cnl_mb_cfg?

https://review.coreboot.org/#/c/31262/10/src/soc/intel/cannonlake/cnl_memcfg... 
PS10, Line 61: MemorySpdPtr00
Default value of this field is 0. Should be okay to just:

if (cnl_cfg->pop_channel[0])
    mem_cfg->MemorySpdPtr00 = spd_data_ptr;
if (cnl_cfg->pop_channel[1])
    mem_cfg->MemorySpdPtr10 = spd_data_ptr;

https://review.coreboot.org/#/c/31262/10/src/soc/intel/cannonlake/include/so... 
File src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h:

https://review.coreboot.org/#/c/31262/10/src/soc/intel/cannonlake/include/so... 
PS10, Line 108: Flags to indicate which channels are populated
Actually, by doing this, it will break all boards unless you update configuration for each CNL/WHL board to set these fields.

Change in ...coreboot[master]: soc/intel/cannonlake: Add field to identify single channel memory

[Shelley Chen (Code Review)]

Hello Patrick Rudolph, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: soc/intel/cannonlake: Add field to identify single channel memory
......................................................................

soc/intel/cannonlake: Add field to identify single channel memory

Variants of Hatch need to accommodate single channel DDR.  Also,
removing const modifier as we'll need to set these fields
incrementally now.  For the single channel configuration, we set
MemorySpdPtr10 to 0.  For the dual channle configuration, we set
MemorySpdPtr10 to MemorySpdPtr00.

BUG=b:123062346, b:122959294
BRANCH=None
TEST=Boot into current boards and ensure that we have 2 channels as expected

Change-Id: Ice22b103664187834e255d1359bfd9b51993b5b6
Signed-off-by: Shelley Chen 
---
M src/soc/intel/cannonlake/cnl_memcfg_init.c
M src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h
2 files changed, 32 insertions(+), 12 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/31262/11

Change in ...coreboot[master]: soc/intel/cannonlake: Add field to identify single channel memory

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31262 )

Change subject: soc/intel/cannonlake: Add field to identify single channel memory
......................................................................

Patch Set 11:

(3 comments)

https://review.coreboot.org/#/c/31262/11/src/soc/intel/cannonlake/cnl_memcfg... 
File src/soc/intel/cannonlake/cnl_memcfg_init.c:

https://review.coreboot.org/#/c/31262/11/src/soc/intel/cannonlake/cnl_memcfg... 
PS11, Line 61: 		mem_cfg->MemorySpdPtr00 = 0;
Same comment as before. This is the default value. Probably okay to skip?

https://review.coreboot.org/#/c/31262/11/src/soc/intel/cannonlake/include/so... 
File src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h:

https://review.coreboot.org/#/c/31262/11/src/soc/intel/cannonlake/include/so... 
PS11, Line 115: zero to default 0
zero to default 0?

https://review.coreboot.org/#/c/31262/11/src/soc/intel/cannonlake/include/so... 
PS11, Line 116: existing boards like wilco/sarien.
I don't think we need to explicitly mention mainboards here. Also, wilco/sarien doesn't use SPD from CBFS

Change in ...coreboot[master]: soc/intel/cannonlake: Add field to identify single channel memory

[Shelley Chen (Code Review)]

Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31262 )

Change subject: soc/intel/cannonlake: Add field to identify single channel memory
......................................................................

Patch Set 10:

(2 comments)

https://review.coreboot.org/#/c/31262/10/src/soc/intel/cannonlake/cnl_memcfg... 
File src/soc/intel/cannonlake/cnl_memcfg_init.c:

https://review.coreboot.org/#/c/31262/10/src/soc/intel/cannonlake/cnl_memcfg... 
PS10, Line 19: #include <soc/gpio.h>
> Why is this required?

I thought that this was needed for other boards.  You had commented in patchset 5:

I think you need to include soc/gpio.h here and that will include the right gpio_soc_defs / gpio_soc_defs_cnp_h header file based on the PCH type.

https://review.coreboot.org/#/c/31262/10/src/soc/intel/cannonlake/cnl_memcfg... 
PS10, Line 53: cnl_cfg
> maybe cnl_mb_cfg?

I think that Patrick commented that the name should be the same as others below, which used the name cnl_cfg.

Change in ...coreboot[master]: soc/intel/cannonlake: Add field to identify single channel memory

[Shelley Chen (Code Review)]

Hello Patrick Rudolph, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: soc/intel/cannonlake: Add field to identify single channel memory
......................................................................

soc/intel/cannonlake: Add field to identify single channel memory

Variants of Hatch need to accommodate single channel DDR.  Also,
removing const modifier as we'll need to set these fields
incrementally now.  For the single channel configuration, we set
MemorySpdPtr10 to 0.  For the dual channle configuration, we set
MemorySpdPtr10 to MemorySpdPtr00.

BUG=b:123062346, b:122959294
BRANCH=None
TEST=Boot into current boards and ensure that we have 2 channels as expected

Change-Id: Ice22b103664187834e255d1359bfd9b51993b5b6
Signed-off-by: Shelley Chen 
---
M src/soc/intel/cannonlake/cnl_memcfg_init.c
M src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h
2 files changed, 28 insertions(+), 12 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/31262/12

Change in ...coreboot[master]: soc/intel/cannonlake: Add field to identify single channel memory

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31262 )

Change subject: soc/intel/cannonlake: Add field to identify single channel memory
......................................................................

Patch Set 12:

(2 comments)

https://review.coreboot.org/#/c/31262/10/src/soc/intel/cannonlake/cnl_memcfg... 
File src/soc/intel/cannonlake/cnl_memcfg_init.c:

https://review.coreboot.org/#/c/31262/10/src/soc/intel/cannonlake/cnl_memcfg... 
PS10, Line 19: #include <soc/gpio.h>
> I thought that this was needed for other boards.  You had commented in patchset 5: […]

Yeah, that was because you had included gpio_soc_defs.h and so I had indicated that including gpio.h is the right thing to do instead of gpio_soc_defs.h. Looking at the change again, I don't see anything being added here that requires any of those header files. Is it really required?

https://review.coreboot.org/#/c/31262/10/src/soc/intel/cannonlake/cnl_memcfg... 
PS10, Line 53: cnl_cfg
> I think that Patrick commented that the name should be the same as others below, which used the name […]

Oh okay.

Change in ...coreboot[master]: soc/intel/cannonlake: Add field to identify single channel memory

[Shelley Chen (Code Review)]

Hello Patrick Rudolph, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: soc/intel/cannonlake: Add field to identify single channel memory
......................................................................

soc/intel/cannonlake: Add field to identify single channel memory

Variants of Hatch need to accommodate single channel DDR.  Also,
removing const modifier as we'll need to set these fields
incrementally now.  For the single channel configuration, we set
MemorySpdPtr10 to 0.  For the dual channle configuration, we set
MemorySpdPtr10 to MemorySpdPtr00.

BUG=b:123062346, b:122959294
BRANCH=None
TEST=Boot into current boards and ensure that we have 2 channels as expected

Change-Id: Ice22b103664187834e255d1359bfd9b51993b5b6
Signed-off-by: Shelley Chen 
---
M src/soc/intel/cannonlake/cnl_memcfg_init.c
M src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h
2 files changed, 27 insertions(+), 12 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/31262/13

Change in ...coreboot[master]: soc/intel/cannonlake: Add field to identify single channel memory

[Shelley Chen (Code Review)]

Hello Patrick Rudolph, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: soc/intel/cannonlake: Add field to identify single channel memory
......................................................................

soc/intel/cannonlake: Add field to identify single channel memory

Variants of Hatch need to accommodate single channel DDR.  Also,
removing const modifier as we'll need to set these fields
incrementally now.  For the single channel configuration, we set
MemorySpdPtr10 to 0.  For the dual channel configuration, we set
MemorySpdPtr10 to MemorySpdPtr00.

BUG=b:123062346, b:122959294
BRANCH=None
TEST=Boot into current boards and ensure that we have 2 channels as expected

Change-Id: Ice22b103664187834e255d1359bfd9b51993b5b6
Signed-off-by: Shelley Chen 
---
M src/soc/intel/cannonlake/cnl_memcfg_init.c
M src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h
2 files changed, 27 insertions(+), 12 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/31262/14

Change in ...coreboot[master]: soc/intel/cannonlake: Add field to identify single channel memory

[Patrick Rudolph (Code Review)]

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

Change subject: soc/intel/cannonlake: Add field to identify single channel memory
......................................................................

Patch Set 14: Code-Review+1

Change in ...coreboot[master]: soc/intel/cannonlake: Add field to identify single channel memory

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31262 )

Change subject: soc/intel/cannonlake: Add field to identify single channel memory
......................................................................

Patch Set 14: Code-Review+2

Change in ...coreboot[master]: soc/intel/cannonlake: Add field to identify single channel memory

[Patrick Georgi (Code Review)]

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

Change subject: soc/intel/cannonlake: Add field to identify single channel memory
......................................................................

soc/intel/cannonlake: Add field to identify single channel memory

Variants of Hatch need to accommodate single channel DDR.  Also,
removing const modifier as we'll need to set these fields
incrementally now.  For the single channel configuration, we set
MemorySpdPtr10 to 0.  For the dual channel configuration, we set
MemorySpdPtr10 to MemorySpdPtr00.

BUG=b:123062346, b:122959294
BRANCH=None
TEST=Boot into current boards and ensure that we have 2 channels as expected

Change-Id: Ice22b103664187834e255d1359bfd9b51993b5b6
Signed-off-by: Shelley Chen 
Reviewed-on: https://review.coreboot.org/c/31262
Reviewed-by: Patrick Rudolph 
Reviewed-by: Furquan Shaikh 
Tested-by: build bot (Jenkins) 
---
M src/soc/intel/cannonlake/cnl_memcfg_init.c
M src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h
2 files changed, 27 insertions(+), 12 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Furquan Shaikh: Looks good to me, approved
  Patrick Rudolph: Looks good to me, but someone else must approve

diff --git a/src/soc/intel/cannonlake/cnl_memcfg_init.c b/src/soc/intel/cannonlake/cnl_memcfg_init.c
index 4425862..e97b571 100644
--- a/src/soc/intel/cannonlake/cnl_memcfg_init.c
+++ b/src/soc/intel/cannonlake/cnl_memcfg_init.c
@@ -49,14 +49,16 @@
 }

static void meminit_memcfg_spd(FSP_M_CONFIG *mem_cfg,
-			const struct cnl_mb_cfg *board_cfg,
+			const struct cnl_mb_cfg *cnl_cfg,
    		size_t spd_data_len, uintptr_t spd_data_ptr)
 {
    mem_cfg->MemorySpdDataLen = spd_data_len;
-	mem_cfg->MemorySpdPtr00 = spd_data_ptr;

-	/* Use the same spd data for channel 1, Dimm 0 */
-	mem_cfg->MemorySpdPtr10 = mem_cfg->MemorySpdPtr00;
+	if (cnl_cfg->channel_empty[0] == 0)
+		mem_cfg->MemorySpdPtr00 = spd_data_ptr;
+
+	if (cnl_cfg->channel_empty[1] == 0)
+		mem_cfg->MemorySpdPtr10 = spd_data_ptr;
 }

/*
diff --git a/src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h b/src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h
index 245d2cf..c602180 100644
--- a/src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h
+++ b/src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h
@@ -62,7 +62,7 @@
     * and let the meminit_lpddr4() routine take care of clearing the
     * unused fields for the caller.
     */
-	const uint8_t dq_map[DDR_NUM_CHANNELS][3][DDR_NUM_PACKAGES];
+	uint8_t dq_map[DDR_NUM_CHANNELS][3][DDR_NUM_PACKAGES];

/*
     * DQS CPU<>DRAM map Ch0 and Ch1.  Each array entry represents a
@@ -71,38 +71,51 @@
     * on the memory part, and the values in the array represent which
     * pin on the CPU that DRAM pin connects to.
     */
-	const uint8_t dqs_map[DDR_NUM_CHANNELS][DQ_BITS_PER_DQS];
+	uint8_t dqs_map[DDR_NUM_CHANNELS][DQ_BITS_PER_DQS];

/*
     * Rcomp resistor values.  These values represent the resistance in
     * ohms of the three rcomp resistors attached to the DDR_COMP_0,
     * DDR_COMP_1, and DDR_COMP_2 pins on the DRAM.
     */
-	const uint16_t rcomp_resistor[3];
+	uint16_t rcomp_resistor[3];

/*
     * Rcomp target values.  These will typically be the following
     * values for Cannon Lake : { 80, 40, 40, 40, 30 }
     */
-	const uint16_t rcomp_targets[5];
+	uint16_t rcomp_targets[5];

/*
     * Indicates whether memory is interleaved.
     * Set to 1 for an interleaved design,
     * set to 0 for non-interleaved design.
     */
-	const uint8_t dq_pins_interleaved;
+	uint8_t dq_pins_interleaved;

/*
-	 * VREF_CA configuraation.
+	 * VREF_CA configuration.
     * Set to 0 VREF_CA goes to both CH_A and CH_B,
     * set to 1 VREF_CA goes to CH_A and VREF_DQ_A goes to CH_B,
     * set to 2 VREF_CA goes to CH_A and VREF_DQ_B goes to CH_B.
     */
-	const uint8_t vref_ca_config;
+	uint8_t vref_ca_config;

/* Early Command Training Enabled */
-	const uint8_t ect;
+	uint8_t ect;
+
+	/*
+	 * Flags to indicate which channels are populated.  We
+	 * currently support single or dual channel configurations.
+	 * Set 1 to indicate that the channel is not populated Set 0
+	 * to indicate that the channel is populated.  For example,
+	 * dual channel memory configuration would have both
+	 * channel_empty[0] = 0 and channel_empty[1] = 0.  Note that
+	 * this flag is only used for soldered down DRAM where we get
+	 * SPD data from CBFS.  We need the value 0 to default to
+	 * populated in order to support existing boards.
+	 */
+	uint8_t channel_empty[2];
 };

/*