Change in ...coreboot[master]: mb/google/hatch: Use MEM_CH_SEL to indicate single_channel sku
Shelley Chen has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31358
Change subject: mb/google/hatch: Use MEM_CH_SEL to indicate single_channel sku ......................................................................
mb/google/hatch: Use MEM_CH_SEL to indicate single_channel sku
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: I89d022793580be603a93d0b177d73ce968529b5c Signed-off-by: Shelley Chen shchen@google.com --- M src/mainboard/google/hatch/romstage.c M src/mainboard/google/hatch/variants/baseboard/gpio.c M src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/hatch/variants/baseboard/memory.c 4 files changed, 47 insertions(+), 26 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/31358/1
diff --git a/src/mainboard/google/hatch/romstage.c b/src/mainboard/google/hatch/romstage.c index 401f41f..b2db7ee 100644 --- a/src/mainboard/google/hatch/romstage.c +++ b/src/mainboard/google/hatch/romstage.c @@ -19,11 +19,14 @@
void mainboard_memory_init_params(FSPM_UPD *memupd) { + struct cnl_mb_cfg memcfg; + const struct spd_info spd = { .spd_by_index = true, .spd_spec.spd_index = variant_memory_sku(), };
+ variant_memory_params(&memcfg); cannonlake_memcfg_init(&memupd->FspmConfig, - variant_memory_params(), &spd); + &memcfg, &spd); } diff --git a/src/mainboard/google/hatch/variants/baseboard/gpio.c b/src/mainboard/google/hatch/variants/baseboard/gpio.c index f8d5d7d..b87cfa8 100644 --- a/src/mainboard/google/hatch/variants/baseboard/gpio.c +++ b/src/mainboard/google/hatch/variants/baseboard/gpio.c @@ -115,6 +115,8 @@ PAD_CFG_NF(GPP_F0, NONE, DEEP, NF1), /* WWAN_RESET_1V8_ODL */ PAD_CFG_GPO(GPP_F1, 1, DEEP), + /* MEM_CH_SEL */ + PAD_CFG_GPI(GPP_F2, DN_20K, PLTRST), /* UART_WWANTX_WLANRX_COEX1 */ PAD_CFG_NF(GPP_F8, NONE, DEEP, NF1), /* UART_WWANRX_WLANTX_COEX2 */ @@ -185,6 +187,8 @@ PAD_CFG_GPI_APIC(GPP_C21, NONE, DEEP, LEVEL, INVERT), /* WLAN_PE_RST# */ PAD_CFG_GPO(GPP_C23, 1, DEEP), + /* MEM_CH_SEL */ + PAD_CFG_GPI(GPP_F2, DN_20K, PLTRST), };
const struct pad_config *__weak variant_early_gpio_table(size_t *num) diff --git a/src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h b/src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h index 038ec6e..aa7c67d 100644 --- a/src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h +++ b/src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h @@ -16,6 +16,7 @@ #ifndef BASEBOARD_VARIANTS_H #define BASEBOARD_VARIANTS_H
+#include <soc/cnl_memcfg_init.h> #include <soc/gpio.h> #include <stdint.h> #include <vendorcode/google/chromeos/chromeos.h> @@ -29,7 +30,7 @@ int variant_memory_sku(void);
/* Return board specific memory configuration */ -const struct cnl_mb_cfg *variant_memory_params(void); +void variant_memory_params(struct cnl_mb_cfg *bcfg);
/* Return ChromeOS gpio table and fill in number of entries. */ const struct cros_gpio *variant_cros_gpios(size_t *num); diff --git a/src/mainboard/google/hatch/variants/baseboard/memory.c b/src/mainboard/google/hatch/variants/baseboard/memory.c index 80f3ba4..977573d 100644 --- a/src/mainboard/google/hatch/variants/baseboard/memory.c +++ b/src/mainboard/google/hatch/variants/baseboard/memory.c @@ -18,33 +18,46 @@ #include <gpio.h> #include <soc/cnl_memcfg_init.h>
-static const struct cnl_mb_cfg baseboard_memcfg = { - /* - * The dqs_map arrays map the ddr4 pins to the SoC pins - * for both channels. - * - * the index = pin number on ddr4 part - * the value = pin number on SoC - */ - .dqs_map[DDR_CH0] = { 0, 1, 4, 5, 2, 3, 6, 7 }, - .dqs_map[DDR_CH1] = { 0, 1, 4, 5, 2, 3, 6, 7 }, - - /* Baseboard uses 120, 81 and 100 rcomp resistors */ - .rcomp_resistor = { 120, 81, 100 }, - - /* Baseboard Rcomp target values */ - .rcomp_targets = { 100, 40, 20, 20, 26 }, - - /* Set CaVref config to 2 */ - .vref_ca_config = 2, - - /* Enable Early Command Training */ - .ect = 1, +/* + * The dqs_map arrays map the ddr4 pins to the SoC pins + * for both channels. + * + * the index = pin number on ddr4 part + * the value = pin number on SoC + */ +static const uint8_t dqs_map_ddr4[][8] = { + { 0, 1, 4, 5, 2, 3, 6, 7 }, + { 0, 1, 4, 5, 2, 3, 6, 7 } }; +/* Baseboard uses 120, 81 and 100 rcomp resistors */ +const static uint16_t rcomp_resistor_ddr4[] = { 120, 81, 100 }; +/* Baseboard Rcomp target values */ +const static uint16_t rcomp_targets_ddr4[] = { 100, 40, 20, 20, 26 };
-const struct cnl_mb_cfg *__weak variant_memory_params(void) +void __weak variant_memory_params(struct cnl_mb_cfg *bcfg) { - return &baseboard_memcfg; + int i; + + for (i = 0; i < sizeof(dqs_map_ddr4[DDR_CH0])/sizeof(uint8_t); i++) { + bcfg->dqs_map[DDR_CH0][i] = dqs_map_ddr4[DDR_CH0][i]; + bcfg->dqs_map[DDR_CH1][i] = dqs_map_ddr4[DDR_CH1][i]; + } + + for (i = 0; i < sizeof(rcomp_resistor_ddr4)/sizeof(uint16_t); i++) + bcfg->rcomp_resistor[i] = rcomp_resistor_ddr4[i]; + + for (i = 0; i < sizeof(rcomp_targets_ddr4)/sizeof(uint16_t); i++) + bcfg->rcomp_targets[i] = rcomp_targets_ddr4[i]; + + bcfg->vref_ca_config = 2; + bcfg->ect = 1; + + /* + * 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) + bcfg->single_channel = 1; }
int __weak variant_memory_sku(void)
Shelley Chen has uploaded a new patch set (#2). ( https://review.coreboot.org/c/coreboot/+/31358 )
Change subject: mb/google/hatch: Use MEM_CH_SEL to indicate single_channel sku ......................................................................
mb/google/hatch: Use MEM_CH_SEL to indicate single_channel sku
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 single_channel field (from GPP_F2), which will in turn initialize MemorySpdPrt pointers in cannonlake soc code. 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: I89d022793580be603a93d0b177d73ce968529b5c Signed-off-by: Shelley Chen shchen@google.com --- M src/mainboard/google/hatch/romstage.c M src/mainboard/google/hatch/variants/baseboard/gpio.c M src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/hatch/variants/baseboard/memory.c 4 files changed, 47 insertions(+), 26 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/31358/2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31358 )
Change subject: mb/google/hatch: Use MEM_CH_SEL to indicate single_channel sku ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/31358/3/src/mainboard/google/hatch/variants/... File src/mainboard/google/hatch/variants/baseboard/memory.c:
https://review.coreboot.org/#/c/31358/3/src/mainboard/google/hatch/variants/... PS3, Line 22: : : : : : : : : : : : : : : : : : : : : Why were these values moved out? They would remain the same irrespective of single or dual channel, right?
Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31358 )
Change subject: mb/google/hatch: Use MEM_CH_SEL to indicate single_channel sku ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/31358/3/src/mainboard/google/hatch/variants/... File src/mainboard/google/hatch/variants/baseboard/memory.c:
https://review.coreboot.org/#/c/31358/3/src/mainboard/google/hatch/variants/... PS3, Line 22: : : : : : : : : : : : : : : : : : : : :
Why were these values moved out? They would remain the same irrespective of single or dual channel, […]
yeah, this was not straightforward. I had to take the const struct out so that I could add the single_channel field, which resulted in a compiler error with illegal global variable basebaord_memcfg.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31358 )
Change subject: mb/google/hatch: Use MEM_CH_SEL to indicate single_channel sku ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/31358/3/src/mainboard/google/hatch/variants/... File src/mainboard/google/hatch/variants/baseboard/memory.c:
https://review.coreboot.org/#/c/31358/3/src/mainboard/google/hatch/variants/... PS3, Line 22: : : : : : : : : : : : : : : : : : : : :
yeah, this was not straightforward. […]
But why not just have:
static struct cnl_mb_cfg baseboard_memcfg = {
...
};
here and then:
const struct cnl_mb_cfg *__weak variant_memory_params(void) { if (gpio_get(GPP_F2) == 1) baseboard_memcfg.single_channel = 1;
return &baseboard_memcfg; }
Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31358 )
Change subject: mb/google/hatch: Use MEM_CH_SEL to indicate single_channel sku ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/31358/3/src/mainboard/google/hatch/variants/... File src/mainboard/google/hatch/variants/baseboard/memory.c:
https://review.coreboot.org/#/c/31358/3/src/mainboard/google/hatch/variants/... PS3, Line 22: : : : : : : : : : : : : : : : : : : : :
But why not just have: […]
Making the struct non-const results in the error that I mentioned before:
Forbidden global variables in romstage: CC firmware/2lib/2crc8.o ffffff30 d _GLOBAL_OFFSET_TABLE_ ffffff00 d baseboard_memcfg CC firmware/lib20/packed_key.o make: *** [src/arch/x86/Makefile.inc:249: build-hatch/cbfs/fallback/romstage.debug] Error 1
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31358 )
Change subject: mb/google/hatch: Use MEM_CH_SEL to indicate single_channel sku ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/#/c/31358/3/src/mainboard/google/hatch/romstage.... File src/mainboard/google/hatch/romstage.c:
https://review.coreboot.org/#/c/31358/3/src/mainboard/google/hatch/romstage.... PS3, Line 22: struct static
https://review.coreboot.org/#/c/31358/3/src/mainboard/google/hatch/romstage.... PS3, Line 24: const static
https://review.coreboot.org/#/c/31358/3/src/mainboard/google/hatch/variants/... File src/mainboard/google/hatch/variants/baseboard/memory.c:
https://review.coreboot.org/#/c/31358/3/src/mainboard/google/hatch/variants/... PS3, Line 22: : : : : : : : : : : : : : : : : : : : :
Making the struct non-const results in the error that I mentioned before: […]
Aah yes!
We can probably do:
void __weak variant_memory_params(struct cnl_mb_cfg *bcfg) { memcpy(bcfg, &baseboard_memcfg, sizeof(baseboard_memcfg)); if (gpio_get(GPP_F2)) bcfg.single_channel = 1; }
That way you don't have to split the above structure and do individual copies.
Hello Aaron Durbin, Aamir Bohra, Duncan Laurie, Rizwan Qureshi, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31358
to look at the new patch set (#4).
Change subject: mb/google/hatch: Use MEM_CH_SEL to indicate single_channel sku ......................................................................
mb/google/hatch: Use MEM_CH_SEL to indicate single_channel sku
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 single_channel field (from GPP_F2), which will in turn initialize MemorySpdPrt pointers in cannonlake soc code. 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: I89d022793580be603a93d0b177d73ce968529b5c Signed-off-by: Shelley Chen shchen@google.com --- M src/mainboard/google/hatch/romstage.c M src/mainboard/google/hatch/variants/baseboard/gpio.c M src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/hatch/variants/baseboard/memory.c 4 files changed, 19 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/31358/4
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31358 )
Change subject: mb/google/hatch: Use MEM_CH_SEL to indicate single_channel sku ......................................................................
Patch Set 4: Code-Review+2
(1 comment)
https://review.coreboot.org/#/c/31358/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31358/4//COMMIT_MSG@12 PS4, Line 12: MemorySpdPrt MemorySpdPtr
Hello Aaron Durbin, Aamir Bohra, Duncan Laurie, Rizwan Qureshi, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31358
to look at the new patch set (#5).
Change subject: mb/google/hatch: Use MEM_CH_SEL to indicate single_channel sku ......................................................................
mb/google/hatch: Use MEM_CH_SEL to indicate single_channel sku
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 single_channel field (from GPP_F2), which will in turn initialize MemorySpdPtr pointers in cannonlake soc code. 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: I89d022793580be603a93d0b177d73ce968529b5c Signed-off-by: Shelley Chen shchen@google.com --- M src/mainboard/google/hatch/romstage.c M src/mainboard/google/hatch/variants/baseboard/gpio.c M src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/hatch/variants/baseboard/memory.c 4 files changed, 19 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/31358/5
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31358 )
Change subject: mb/google/hatch: Use MEM_CH_SEL to indicate single_channel sku ......................................................................
Patch Set 5: Code-Review+2
Hello Aaron Durbin, Aamir Bohra, Duncan Laurie, Rizwan Qureshi, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31358
to look at the new patch set (#6).
Change subject: mb/google/hatch: Use MEM_CH_SEL to indicate single_channel sku ......................................................................
mb/google/hatch: Use MEM_CH_SEL to indicate single_channel sku
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 single_channel field (from GPP_F2), which will in turn initialize MemorySpdPtr pointers in cannonlake soc code. 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: I89d022793580be603a93d0b177d73ce968529b5c Signed-off-by: Shelley Chen shchen@google.com --- M src/mainboard/google/hatch/romstage.c M src/mainboard/google/hatch/variants/baseboard/gpio.c M src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/hatch/variants/baseboard/memory.c 4 files changed, 19 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/31358/6
Hello Aaron Durbin, Aamir Bohra, Duncan Laurie, Rizwan Qureshi, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31358
to look at the new patch set (#7).
Change subject: mb/google/hatch: Use MEM_CH_SEL to indicate single_channel sku ......................................................................
mb/google/hatch: Use MEM_CH_SEL to indicate single_channel sku
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 single_channel field (from GPP_F2), which will in turn initialize MemorySpdPtr pointers in cannonlake soc code. 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: I89d022793580be603a93d0b177d73ce968529b5c Signed-off-by: Shelley Chen shchen@google.com --- M src/mainboard/google/hatch/romstage.c M src/mainboard/google/hatch/variants/baseboard/gpio.c M src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/hatch/variants/baseboard/memory.c 4 files changed, 29 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/31358/7
Hello Aaron Durbin, Aamir Bohra, Duncan Laurie, Rizwan Qureshi, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31358
to look at the new patch set (#9).
Change subject: mb/google/hatch: Use MEM_CH_SEL to indicate single_channel sku ......................................................................
mb/google/hatch: Use MEM_CH_SEL to indicate single_channel sku
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 single_channel field (from GPP_F2), which will in turn initialize MemorySpdPtr pointers in cannonlake soc code. 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: I89d022793580be603a93d0b177d73ce968529b5c Signed-off-by: Shelley Chen shchen@google.com --- M src/mainboard/google/hatch/romstage.c M src/mainboard/google/hatch/variants/baseboard/gpio.c M src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/hatch/variants/baseboard/memory.c 4 files changed, 29 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/31358/9
Hello Aaron Durbin, Aamir Bohra, Duncan Laurie, Rizwan Qureshi, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31358
to look at the new patch set (#12).
Change subject: mb/google/hatch: Use MEM_CH_SEL to indicate single_channel sku ......................................................................
mb/google/hatch: Use MEM_CH_SEL to indicate single_channel sku
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 single_channel field (from GPP_F2), which will in turn initialize MemorySpdPtr pointers in cannonlake soc code. 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: I89d022793580be603a93d0b177d73ce968529b5c Signed-off-by: Shelley Chen shchen@google.com --- M src/mainboard/google/hatch/romstage.c M src/mainboard/google/hatch/variants/baseboard/gpio.c M src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/hatch/variants/baseboard/memory.c 4 files changed, 29 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/31358/12
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31358 )
Change subject: mb/google/hatch: Use MEM_CH_SEL to indicate single_channel sku ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/#/c/31358/12/src/mainboard/google/hatch/variants... File src/mainboard/google/hatch/variants/baseboard/gpio.c:
https://review.coreboot.org/#/c/31358/12/src/mainboard/google/hatch/variants... PS12, Line 119: PAD_CFG_GPI(GPP_F2, DN_20K, PLTRST), Actually, I am unsure about the state of GPP_F2 on hatch_whl. Maybe we should push this change later when adding new variant?
Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31358 )
Change subject: mb/google/hatch: Use MEM_CH_SEL to indicate single_channel sku ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/#/c/31358/12/src/mainboard/google/hatch/variants... File src/mainboard/google/hatch/variants/baseboard/gpio.c:
https://review.coreboot.org/#/c/31358/12/src/mainboard/google/hatch/variants... PS12, Line 119: PAD_CFG_GPI(GPP_F2, DN_20K, PLTRST),
Actually, I am unsure about the state of GPP_F2 on hatch_whl. […]
Ok, we can add this in cml.
Hello Aaron Durbin, Aamir Bohra, Duncan Laurie, Rizwan Qureshi, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31358
to look at the new patch set (#15).
Change subject: mb/google/hatch: Use MEM_CH_SEL to indicate single_channel sku ......................................................................
mb/google/hatch: Use MEM_CH_SEL to indicate single_channel sku
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 single_channel field (from GPP_F2), which will in turn initialize MemorySpdPtr pointers in cannonlake soc code. 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: I89d022793580be603a93d0b177d73ce968529b5c Signed-off-by: Shelley Chen shchen@google.com --- M src/mainboard/google/hatch/romstage.c M src/mainboard/google/hatch/variants/baseboard/gpio.c M src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/hatch/variants/baseboard/memory.c 4 files changed, 27 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/31358/15
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31358 )
Change subject: mb/google/hatch: Use MEM_CH_SEL to indicate single_channel sku ......................................................................
Patch Set 15:
(3 comments)
Don't you also need to update this: https://review.coreboot.org/cgit/coreboot.git/tree/src/mainboard/google/hatc...
https://review.coreboot.org/#/c/31358/15/src/mainboard/google/hatch/variants... File src/mainboard/google/hatch/variants/baseboard/gpio.c:
https://review.coreboot.org/#/c/31358/15/src/mainboard/google/hatch/variants... PS15, Line 284: DN_20K Why is the pull-down required?
https://review.coreboot.org/#/c/31358/15/src/mainboard/google/hatch/variants... PS15, Line 432: DN_20K same here
https://review.coreboot.org/#/c/31358/15/src/mainboard/google/hatch/variants... File src/mainboard/google/hatch/variants/baseboard/memory.c:
https://review.coreboot.org/#/c/31358/15/src/mainboard/google/hatch/variants... PS15, Line 22: baseboard_memcfg Do these params apply to both hatch and hatch_whl?
Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31358 )
Change subject: mb/google/hatch: Use MEM_CH_SEL to indicate single_channel sku ......................................................................
Patch Set 18:
Patch Set 15:
(3 comments)
Don't you also need to update this: https://review.coreboot.org/cgit/coreboot.git/tree/src/mainboard/google/hatc...
I was going to update this in another CL (mainly because this was enabling the MEM_CH_SEL gpio).
Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31358 )
Change subject: mb/google/hatch: Use MEM_CH_SEL to indicate single_channel sku ......................................................................
Patch Set 18:
(1 comment)
https://review.coreboot.org/#/c/31358/15/src/mainboard/google/hatch/variants... File src/mainboard/google/hatch/variants/baseboard/gpio.c:
https://review.coreboot.org/#/c/31358/15/src/mainboard/google/hatch/variants... PS15, Line 284: DN_20K
Why is the pull-down required?
This was originally done to accommodate earlier boards that had this line not connected.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31358 )
Change subject: mb/google/hatch: Use MEM_CH_SEL to indicate single_channel sku ......................................................................
Patch Set 18: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31358 )
Change subject: mb/google/hatch: Use MEM_CH_SEL to indicate single_channel sku ......................................................................
mb/google/hatch: Use MEM_CH_SEL to indicate single_channel sku
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 single_channel field (from GPP_F2), which will in turn initialize MemorySpdPtr pointers in cannonlake soc code. 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: I89d022793580be603a93d0b177d73ce968529b5c Signed-off-by: Shelley Chen shchen@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/31358 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/mainboard/google/hatch/romstage.c M src/mainboard/google/hatch/variants/baseboard/gpio.c M src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/hatch/variants/baseboard/memory.c 4 files changed, 27 insertions(+), 6 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/mainboard/google/hatch/romstage.c b/src/mainboard/google/hatch/romstage.c index 429aa09..bdf951b 100644 --- a/src/mainboard/google/hatch/romstage.c +++ b/src/mainboard/google/hatch/romstage.c @@ -23,13 +23,16 @@
void mainboard_memory_init_params(FSPM_UPD *memupd) { + struct cnl_mb_cfg memcfg; + const struct spd_info spd = { .spd_by_index = true, .spd_spec.spd_index = variant_memory_sku(), };
+ variant_memory_params(&memcfg); cannonlake_memcfg_init(&memupd->FspmConfig, - variant_memory_params(), &spd); + &memcfg, &spd); }
void mainboard_get_dram_part_num(const char **part_num, size_t *len) diff --git a/src/mainboard/google/hatch/variants/baseboard/gpio.c b/src/mainboard/google/hatch/variants/baseboard/gpio.c index 091f6b8..93e0af1 100644 --- a/src/mainboard/google/hatch/variants/baseboard/gpio.c +++ b/src/mainboard/google/hatch/variants/baseboard/gpio.c @@ -281,7 +281,7 @@ /* F1 : WWAN_RESET_1V8_ODL */ PAD_CFG_GPO(GPP_F1, 1, DEEP), /* F2 : MEM_CH_SEL */ - PAD_CFG_GPI(GPP_F2, NONE, PLTRST), + PAD_CFG_GPI(GPP_F2, DN_20K, PLTRST), /* F3 : GPP_F3 ==> NC */ PAD_NC(GPP_F3, NONE), /* F4 : CNV_BRI_DT */ @@ -429,7 +429,7 @@ /* C23 : WLAN_PE_RST# */ PAD_CFG_GPO(GPP_C23, 1, DEEP), /* F2 : MEM_CH_SEL */ - PAD_CFG_GPI(GPP_F2, NONE, PLTRST), + PAD_CFG_GPI(GPP_F2, DN_20K, PLTRST), /* F11 : PCH_MEM_STRAP2 */ PAD_CFG_GPI(GPP_F11, NONE, PLTRST), /* F20 : PCH_MEM_STRAP0 */ diff --git a/src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h b/src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h index 038ec6e..aa7c67d 100644 --- a/src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h +++ b/src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h @@ -16,6 +16,7 @@ #ifndef BASEBOARD_VARIANTS_H #define BASEBOARD_VARIANTS_H
+#include <soc/cnl_memcfg_init.h> #include <soc/gpio.h> #include <stdint.h> #include <vendorcode/google/chromeos/chromeos.h> @@ -29,7 +30,7 @@ int variant_memory_sku(void);
/* Return board specific memory configuration */ -const struct cnl_mb_cfg *variant_memory_params(void); +void variant_memory_params(struct cnl_mb_cfg *bcfg);
/* Return ChromeOS gpio table and fill in number of entries. */ const struct cros_gpio *variant_cros_gpios(size_t *num); diff --git a/src/mainboard/google/hatch/variants/baseboard/memory.c b/src/mainboard/google/hatch/variants/baseboard/memory.c index 80f3ba4..6ca98e4 100644 --- a/src/mainboard/google/hatch/variants/baseboard/memory.c +++ b/src/mainboard/google/hatch/variants/baseboard/memory.c @@ -17,6 +17,7 @@ #include <baseboard/gpio.h> #include <gpio.h> #include <soc/cnl_memcfg_init.h> +#include <string.h>
static const struct cnl_mb_cfg baseboard_memcfg = { /* @@ -42,9 +43,25 @@ .ect = 1, };
-const struct cnl_mb_cfg *__weak variant_memory_params(void) +void __weak variant_memory_params(struct cnl_mb_cfg *bcfg) { - return &baseboard_memcfg; + memcpy(bcfg, &baseboard_memcfg, sizeof(baseboard_memcfg)); + /* + * 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) { + /* + * Single channel config: for Hatch, Channel 0 is + * always populated. + */ + bcfg->channel_empty[0] = 0; + bcfg->channel_empty[1] = 1; + } else { + /* Dual channel config: both channels populated. */ + bcfg->channel_empty[0] = 0; + bcfg->channel_empty[1] = 0; + } }
int __weak variant_memory_sku(void)
-
Furquan Shaikh (Code Review) -
Patrick Georgi (Code Review)
-
Shelley Chen (Code Review)