Change in coreboot[master]: src/soc/tigerlake: Add memory configuration support for Jasper Lake
Meera Ravindranath has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
src/soc/tigerlake: Add memory configuration support for Jasper Lake
BUG=none BRANCH=none TEST=Build and verify boot of WaddleDoo.
Change-Id: I8de502d3f05d52b9dae34e3b013c6d5b1896fa85 Signed-off-by: Meera Ravindranath meera.ravindranath@intel.com --- M src/soc/intel/tigerlake/Makefile.inc A src/soc/intel/tigerlake/include/soc/jsl_memcfg_init.h A src/soc/intel/tigerlake/jsl_memcfg_init.c 3 files changed, 305 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/39135/1
diff --git a/src/soc/intel/tigerlake/Makefile.inc b/src/soc/intel/tigerlake/Makefile.inc index 56119f5..c85f9dc 100644 --- a/src/soc/intel/tigerlake/Makefile.inc +++ b/src/soc/intel/tigerlake/Makefile.inc @@ -26,6 +26,7 @@ romstage-y += espi.c romstage-y += gpio.c romstage-$(CONFIG_SOC_INTEL_TIGERLAKE) += meminit_tgl.c +romstage-$(CONFIG_SOC_INTEL_JASPERLAKE) += jsl_memcfg_init.c romstage-y += reset.c
ramstage-y += acpi.c diff --git a/src/soc/intel/tigerlake/include/soc/jsl_memcfg_init.h b/src/soc/intel/tigerlake/include/soc/jsl_memcfg_init.h new file mode 100644 index 0000000..e04ef85 --- /dev/null +++ b/src/soc/intel/tigerlake/include/soc/jsl_memcfg_init.h @@ -0,0 +1,139 @@ +/* + * This file is part of the coreboot project. + * + * Copyright Intel Corporation 2020. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef _SOC_JASPERLAKE_MEMCFG_INIT_H_ +#define _SOC_JASPERLAKE_MEMCFG_INIT_H_ + +#include <stddef.h> +#include <stdint.h> +#include <fsp/soc_binding.h> + +/* Number of dq bits controlled per dqs */ +#define DQ_BITS_PER_DQS 8 + +/* Number of memory DIMM slots available on Cannonlake board */ +#define NUM_DIMM_SLOT 4 + +/* + * Number of memory packages, where a "package" represents a 64-bit solution. + */ +#define DDR_NUM_PACKAGES 2 + +/* 64-bit Channel identification */ +enum { + DDR_CH0, + DDR_CH1, + DDR_NUM_CHANNELS +}; + +struct spd_by_pointer { + size_t spd_data_len; + uintptr_t spd_data_ptr; +}; + +enum mem_info_read_type { + NOT_EXISTING, /* No memory in this slot */ + READ_SMBUS, /* Read on-module spd by SMBUS. */ + READ_SPD_CBFS, /* Find spd file in CBFS. */ + READ_SPD_MEMPTR /* Find spd data from pointer. */ +}; + +struct spd_info { + enum mem_info_read_type read_type; + union spd_data_by { + /* To read on-module spd when read_type is READ_SMBUS. */ + uint8_t spd_smbus_address; + + /* To identify spd file when read_type is READ_SPD_CBFS. */ + int spd_index; + + /* To find spd data when read_type is READ_SPD_MEMPTR. */ + struct spd_by_pointer spd_data_ptr_info; + } spd_spec; +}; + +/* Board-specific memory dq mapping information */ +struct jsl_mb_cfg { + /* Parameters required to access SPD for CH0D0/CH0D1/CH1D0/CH1D1. */ + struct spd_info spd[NUM_DIMM_SLOT]; + + /* + * For each channel, there are 6 sets of DQ byte mappings, + * where each set has a package 0 and a package 1 value (package 0 + * represents the first 64-bit lpddr4 chip combination, and package 1 + * represents the second 64-bit lpddr4 chip combination). + * The first three sets are for CLK, CMD, and CTL. + * The fsp package actually expects 6 sets, even though the last 3 sets + * are not used in CNL. + * We let the meminit_lpddr4() routine take care of clearing the + * unused fields for the caller. + * Note that dq_map is only used by LPDDR; it does not need to be + * initialized for designs using DDR4. + */ + uint8_t dq_map[DDR_NUM_CHANNELS][6][DDR_NUM_PACKAGES]; + + /* + * DQS CPU<>DRAM map Ch0 and Ch1. Each array entry represents a + * mapping of a dq bit on the CPU to the bit it's connected to on + * the memory part. The array index represents the dqs bit number + * on the memory part, and the values in the array represent which + * pin on the CPU that DRAM pin connects to. + * dqs_map is only used by LPDDR; same comments apply as for dq_map + * above. + */ + 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. + */ + uint16_t rcomp_resistor[3]; + + /* + * Rcomp target values. These will typically be the following + * values for Cannon Lake : { 80, 40, 40, 40, 30 } + */ + uint16_t rcomp_targets[5]; + + /* + * Indicates whether memory is interleaved. + * Set to 1 for an interleaved design, + * set to 0 for non-interleaved design. + */ + uint8_t dq_pins_interleaved; + + /* + * 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. + */ + uint8_t vref_ca_config; + + /* Early Command Training Enabled */ + uint8_t ect; + + /* Board type */ + uint8_t UserBd; +}; + +/* + * Initialize default memory configurations for CannonLake. + */ +void jasperlake_memcfg_init(FSP_M_CONFIG *mem_cfg, + const struct jsl_mb_cfg *jsl_cfg); + +#endif /* _SOC_JASPERLAKE_MEMCFG_INIT_H_ */ diff --git a/src/soc/intel/tigerlake/jsl_memcfg_init.c b/src/soc/intel/tigerlake/jsl_memcfg_init.c new file mode 100644 index 0000000..c8e2c03 --- /dev/null +++ b/src/soc/intel/tigerlake/jsl_memcfg_init.c @@ -0,0 +1,165 @@ +/* + * This file is part of the coreboot project. + * + * Copyright 2017 Google Inc. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ +#include <assert.h> +#include <console/console.h> +#include <fsp/util.h> +#include <soc/jsl_memcfg_init.h> +#include <spd_bin.h> +#include <string.h> + +static void meminit_memcfg(FSP_M_CONFIG *mem_cfg, + const struct jsl_mb_cfg *board_cfg) +{ + /* + * DqByteMapChx expects 12 bytes of data, but the last 6 bytes + * are unused, so client passes in the relevant values and + * we null out the rest of the data. + */ + memset(&mem_cfg->DqByteMapCh0, 0, sizeof(mem_cfg->DqByteMapCh0)); + memcpy(&mem_cfg->DqByteMapCh0, &board_cfg->dq_map[DDR_CH0], + sizeof(board_cfg->dq_map[DDR_CH0])); + + memset(&mem_cfg->DqByteMapCh1, 0, sizeof(mem_cfg->DqByteMapCh1)); + memcpy(&mem_cfg->DqByteMapCh1, &board_cfg->dq_map[DDR_CH1], + sizeof(board_cfg->dq_map[DDR_CH1])); + + memcpy(&mem_cfg->DqsMapCpu2DramCh0, &board_cfg->dqs_map[DDR_CH0], + sizeof(board_cfg->dqs_map[DDR_CH0])); + memcpy(&mem_cfg->DqsMapCpu2DramCh1, &board_cfg->dqs_map[DDR_CH1], + sizeof(board_cfg->dqs_map[DDR_CH1])); + + memcpy(&mem_cfg->RcompResistor, &board_cfg->rcomp_resistor, + sizeof(mem_cfg->RcompResistor)); + + /* Early cannonlake requires rcomp targets to be 0 */ + memcpy(&mem_cfg->RcompTarget, &board_cfg->rcomp_targets, + sizeof(mem_cfg->RcompTarget)); + + mem_cfg->UserBd = board_cfg->UserBd; + +} + +/* + * Initialize default memory settings using spd data contained in a buffer. + */ +static void meminit_spd_data(FSP_M_CONFIG *mem_cfg, uint8_t mem_slot, + size_t spd_data_len, uintptr_t spd_data_ptr) +{ + static size_t last_set_spd_data_len = 0; + + assert(spd_data_ptr != 0 && spd_data_len != 0); + + if (last_set_spd_data_len != 0 && + last_set_spd_data_len != spd_data_len) + die("spd data length disparity among slots"); + + mem_cfg->MemorySpdDataLen = spd_data_len; + last_set_spd_data_len = spd_data_len; + + switch (mem_slot) { + case 0: + mem_cfg->MemorySpdPtr00 = spd_data_ptr; + break; + case 1: + mem_cfg->MemorySpdPtr01 = spd_data_ptr; + break; + case 2: + mem_cfg->MemorySpdPtr10 = spd_data_ptr; + break; + case 3: + mem_cfg->MemorySpdPtr11 = spd_data_ptr; + break; + default: + die("nonexistent memory slot"); + } + printk(BIOS_INFO, "memory slot: %d configuration done.\n", mem_slot); +} + +/* + * Initialize default memory settings using the spd file specified by + * spd_index. The spd_index is an index into the SPD_SOURCES array defined + * in spd/Makefile.inc. + */ +static void meminit_cbfs_spd_index(FSP_M_CONFIG *mem_cfg, + int spd_index, uint8_t mem_slot) +{ + static size_t spd_data_len; + static uintptr_t spd_data_ptr; + static int last_spd_index; + + assert(mem_slot < NUM_DIMM_SLOT); + + if ((spd_data_ptr == 0) || (last_spd_index != spd_index)) { + struct region_device spd_rdev; + + printk(BIOS_DEBUG, "SPD INDEX = %d\n", spd_index); + + if (get_spd_cbfs_rdev(&spd_rdev, spd_index) < 0) + die("spd.bin not found or incorrect index\n"); + + spd_data_len = region_device_sz(&spd_rdev); + + /* Memory leak is ok since we have memory mapped boot media */ + assert(CONFIG(BOOT_DEVICE_MEMORY_MAPPED)); + + spd_data_ptr = (uintptr_t)rdev_mmap_full(&spd_rdev); + last_spd_index = spd_index; + print_spd_info((unsigned char *)spd_data_ptr); + } + + meminit_spd_data(mem_cfg, mem_slot, spd_data_len, spd_data_ptr); +} + +/* Initialize onboard memory configurations for CannonLake */ +void jasperlake_memcfg_init(FSP_M_CONFIG *mem_cfg, + const struct jsl_mb_cfg *jsl_cfg) +{ + const struct spd_info *spdi; + + /* Early Command Training Enabled */ + mem_cfg->ECT = jsl_cfg->ect; + mem_cfg->DqPinsInterleaved = jsl_cfg->dq_pins_interleaved; + mem_cfg->CaVrefConfig = jsl_cfg->vref_ca_config; + + for (int i = 0; i < NUM_DIMM_SLOT; i++) { + spdi = &(jsl_cfg->spd[i]); + switch (spdi->read_type) { + case NOT_EXISTING: + break; + + case READ_SMBUS: + mem_cfg->SpdAddressTable[i] = + spdi->spd_spec.spd_smbus_address; + break; + + case READ_SPD_CBFS: + mem_cfg->SpdAddressTable[i] = 0; + meminit_cbfs_spd_index(mem_cfg, + spdi->spd_spec.spd_index, i); + break; + + case READ_SPD_MEMPTR: + meminit_spd_data(mem_cfg, i, + spdi->spd_spec.spd_data_ptr_info.spd_data_len, + spdi->spd_spec.spd_data_ptr_info.spd_data_ptr); + break; + + default: + die("no valid way to read mem info"); + } + + meminit_memcfg(mem_cfg, jsl_cfg); + } +}
Hello Patrick Rudolph, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39135
to look at the new patch set (#2).
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
src/soc/tigerlake: Add memory configuration support for Jasper Lake
BUG=none BRANCH=none TEST=Build and verify boot of WaddleDoo.
Change-Id: I8de502d3f05d52b9dae34e3b013c6d5b1896fa85 Signed-off-by: Meera Ravindranath meera.ravindranath@intel.com --- M src/soc/intel/tigerlake/Makefile.inc A src/soc/intel/tigerlake/include/soc/jsl_memcfg_init.h A src/soc/intel/tigerlake/jsl_memcfg_init.c 3 files changed, 305 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/39135/2
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 2:
(6 comments)
https://review.coreboot.org/c/coreboot/+/39135/2/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/jsl_memcfg_init.h:
https://review.coreboot.org/c/coreboot/+/39135/2/src/soc/intel/tigerlake/inc... PS2, Line 26: Cannonlake Jasperlake here and in other places in this CL. I see multiple references to Cannonlake and CNL.
https://review.coreboot.org/c/coreboot/+/39135/2/src/soc/intel/tigerlake/inc... PS2, Line 48: READ_SMBUS, Are we planning to read the SPD data through SMBUS or MEMPTR? If they are more for future, can we add them when those use-cases become a reality?
https://review.coreboot.org/c/coreboot/+/39135/2/src/soc/intel/tigerlake/jsl... File src/soc/intel/tigerlake/jsl_memcfg_init.c:
https://review.coreboot.org/c/coreboot/+/39135/2/src/soc/intel/tigerlake/jsl... PS2, Line 51: Nit: Remove this extra line
https://review.coreboot.org/c/coreboot/+/39135/2/src/soc/intel/tigerlake/jsl... PS2, Line 132: mem_cfg->ECT = jsl_cfg->ect; : mem_cfg->DqPinsInterleaved = jsl_cfg->dq_pins_interleaved; : mem_cfg->CaVrefConfig = jsl_cfg->vref_ca_config; Can be assigned as part of meminit_memcfg?
https://review.coreboot.org/c/coreboot/+/39135/2/src/soc/intel/tigerlake/jsl... PS2, Line 139: NOT_EXISTING: Is it valid to have 1 or 3 memory modules populated? My assumption is either 2 or 4 memory modules is a valid configuration.
https://review.coreboot.org/c/coreboot/+/39135/2/src/soc/intel/tigerlake/jsl... PS2, Line 163: meminit_memcfg(mem_cfg, jsl_cfg); Nothing DIMM_SLOT specific. I think it can be moved outside the for loop.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39135/2/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/jsl_memcfg_init.h:
https://review.coreboot.org/c/coreboot/+/39135/2/src/soc/intel/tigerlake/inc... PS2, Line 136: jasperlake Nit: I don't think we need jasperlake or jsl prefix in the structures and functions.
Hello Patrick Rudolph, Karthik Ramasubramanian, Subrata Banik, Aamir Bohra, Rizwan Qureshi, Tim Wawrzynczak, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39135
to look at the new patch set (#3).
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
src/soc/tigerlake: Add memory configuration support for Jasper Lake
BUG=none BRANCH=none TEST=Build and verify boot of WaddleDoo.
Change-Id: I8de502d3f05d52b9dae34e3b013c6d5b1896fa85 Signed-off-by: Meera Ravindranath meera.ravindranath@intel.com --- M src/soc/intel/tigerlake/Makefile.inc A src/soc/intel/tigerlake/include/soc/jsl_memcfg_init.h A src/soc/intel/tigerlake/jsl_memcfg_init.c 3 files changed, 305 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/39135/3
Hello Patrick Rudolph, Karthik Ramasubramanian, Subrata Banik, Aamir Bohra, Rizwan Qureshi, Tim Wawrzynczak, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39135
to look at the new patch set (#4).
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
src/soc/tigerlake: Add memory configuration support for Jasper Lake
BUG=none BRANCH=none TEST=Build and verify boot of WaddleDoo.
Change-Id: I8de502d3f05d52b9dae34e3b013c6d5b1896fa85 Signed-off-by: Meera Ravindranath meera.ravindranath@intel.com --- M src/soc/intel/tigerlake/Makefile.inc A src/soc/intel/tigerlake/include/soc/jsl_memcfg_init.h A src/soc/intel/tigerlake/jsl_memcfg_init.c 3 files changed, 305 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/39135/4
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39135/4/src/soc/intel/tigerlake/jsl... File src/soc/intel/tigerlake/jsl_memcfg_init.c:
https://review.coreboot.org/c/coreboot/+/39135/4/src/soc/intel/tigerlake/jsl... PS4, Line 49: trailing whitespace
Hello Patrick Rudolph, Karthik Ramasubramanian, Subrata Banik, Aamir Bohra, Rizwan Qureshi, Tim Wawrzynczak, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39135
to look at the new patch set (#5).
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
src/soc/tigerlake: Add memory configuration support for Jasper Lake
BUG=none BRANCH=none TEST=Build and verify boot of WaddleDoo.
Change-Id: I8de502d3f05d52b9dae34e3b013c6d5b1896fa85 Signed-off-by: Meera Ravindranath meera.ravindranath@intel.com --- M src/soc/intel/tigerlake/Makefile.inc A src/soc/intel/tigerlake/include/soc/jsl_memcfg_init.h A src/soc/intel/tigerlake/jsl_memcfg_init.c 3 files changed, 305 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/39135/5
Meera Ravindranath has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 3:
(7 comments)
https://review.coreboot.org/c/coreboot/+/39135/2/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/jsl_memcfg_init.h:
https://review.coreboot.org/c/coreboot/+/39135/2/src/soc/intel/tigerlake/inc... PS2, Line 26: Cannonlake
Jasperlake here and in other places in this CL. I see multiple references to Cannonlake and CNL.
Done
https://review.coreboot.org/c/coreboot/+/39135/2/src/soc/intel/tigerlake/inc... PS2, Line 48: READ_SMBUS,
Are we planning to read the SPD data through SMBUS or MEMPTR? If they are more for future, can we ad […]
Since this is SoC code, prefer to have all the methods to provide SPD info. This could be used not only by chrome systems where we are using soldered down memory, but by other board manufacturers which use replaceable DIMMs as well.
https://review.coreboot.org/c/coreboot/+/39135/2/src/soc/intel/tigerlake/inc... PS2, Line 136: jasperlake
Nit: I don't think we need jasperlake or jsl prefix in the structures and functions.
Since this SoC folder supports two SoCs and Tigerlake is using a different way of configuring the DIMMs and these functions are JSL specific, we would like to prefix it with jsl.
https://review.coreboot.org/c/coreboot/+/39135/2/src/soc/intel/tigerlake/jsl... File src/soc/intel/tigerlake/jsl_memcfg_init.c:
https://review.coreboot.org/c/coreboot/+/39135/2/src/soc/intel/tigerlake/jsl... PS2, Line 51:
Nit: Remove this extra line
Done
https://review.coreboot.org/c/coreboot/+/39135/2/src/soc/intel/tigerlake/jsl... PS2, Line 132: mem_cfg->ECT = jsl_cfg->ect; : mem_cfg->DqPinsInterleaved = jsl_cfg->dq_pins_interleaved; : mem_cfg->CaVrefConfig = jsl_cfg->vref_ca_config;
Can be assigned as part of meminit_memcfg?
Ack
https://review.coreboot.org/c/coreboot/+/39135/2/src/soc/intel/tigerlake/jsl... PS2, Line 139: NOT_EXISTING:
Is it valid to have 1 or 3 memory modules populated? My assumption is either 2 or 4 memory modules i […]
Yes, it is possible to have only 1 or 3 DIMMs. Whether such a configuration is optimal or not is debatable.
https://review.coreboot.org/c/coreboot/+/39135/2/src/soc/intel/tigerlake/jsl... PS2, Line 163: meminit_memcfg(mem_cfg, jsl_cfg);
Nothing DIMM_SLOT specific. I think it can be moved outside the for loop.
Ack
Subrata Banik has uploaded a new patch set (#6) to the change originally created by Meera Ravindranath. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
src/soc/tigerlake: Add memory configuration support for Jasper Lake
BUG=none BRANCH=none TEST=Build and verify boot of WaddleDoo.
Change-Id: I8de502d3f05d52b9dae34e3b013c6d5b1896fa85 Signed-off-by: Meera Ravindranath meera.ravindranath@intel.com --- M src/soc/intel/tigerlake/Makefile.inc A src/soc/intel/tigerlake/include/soc/jsl_memcfg_init.h A src/soc/intel/tigerlake/jsl_memcfg_init.c 3 files changed, 305 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/39135/6
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39135/2/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/jsl_memcfg_init.h:
https://review.coreboot.org/c/coreboot/+/39135/2/src/soc/intel/tigerlake/inc... PS2, Line 136: jasperlake
Since this SoC folder supports two SoCs and Tigerlake is using a different way of configuring the DI […]
Even though both SoCs configure memory differently, only one of them will be enabled/used at a time i.e. either jsl_memcfg_init.c or tgl_memcfg_init.c will be enabled and not both. From that perspective we don't need to prefix SoC in the data structures and functions. Also there are discussions to split JSL and TGL SoCs separately.
https://review.coreboot.org/c/coreboot/+/39135/2/src/soc/intel/tigerlake/jsl... File src/soc/intel/tigerlake/jsl_memcfg_init.c:
https://review.coreboot.org/c/coreboot/+/39135/2/src/soc/intel/tigerlake/jsl... PS2, Line 139: NOT_EXISTING:
Yes, it is possible to have only 1 or 3 DIMMs. […]
Ok, I believe that means we don't need to pass the strap information regarding only half the memory modules are populated. It will be automatically inferred based on the number of dimm slots configured. Is that correct?
Subrata Banik has uploaded a new patch set (#7) to the change originally created by Meera Ravindranath. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
src/soc/tigerlake: Add memory configuration support for Jasper Lake
BUG=none BRANCH=none TEST=Build and verify boot of WaddleDoo.
Change-Id: I8de502d3f05d52b9dae34e3b013c6d5b1896fa85 Signed-off-by: Meera Ravindranath meera.ravindranath@intel.com --- M src/soc/intel/tigerlake/Makefile.inc A src/soc/intel/tigerlake/include/soc/jsl_memcfg_init.h A src/soc/intel/tigerlake/jsl_memcfg_init.c 3 files changed, 305 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/39135/7
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 7:
(5 comments)
https://review.coreboot.org/c/coreboot/+/39135/7/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/jsl_memcfg_init.h:
https://review.coreboot.org/c/coreboot/+/39135/7/src/soc/intel/tigerlake/inc... PS7, Line 70: struct spd_info spd[NUM_DIMM_SLOT]; Does JSL support different types of memories for each slot? If not, mainboard can just pass in SPD information which can be used to populate all slots. Please see how this is done on TGL.
https://review.coreboot.org/c/coreboot/+/39135/7/src/soc/intel/tigerlake/inc... PS7, Line 103: uint16_t rcomp_resistor[3]; On TGL, we were told that rcomp_resistor and rcomp_target need to be set to auto configurable by MRC rather than having to pass those in by mainboard unless the board deviates from design guidelines. Is this true for JSL as well? Some details captured here: b/142650263
https://review.coreboot.org/c/coreboot/+/39135/7/src/soc/intel/tigerlake/inc... PS7, Line 116: dq_pins_interleaved Does JSL with LPDDR4 allow interleaved memory configuration? I know with TGL this was not a possibility as per PDG and so interleaved config was not exposed to mainboard.
https://review.coreboot.org/c/coreboot/+/39135/7/src/soc/intel/tigerlake/inc... PS7, Line 136: void jasperlake_memcfg_init(FSP_M_CONFIG *mem_cfg, : const struct jsl_mb_cfg *jsl_cfg); Quick comment: Please take a look at TGL memcfg_init. We intentionally split mb_cfg to separate out mainboard configuration from SPD data and half populated info. It makes it easier to have static const structures in mainboard to pass into memcfg_init().
https://review.coreboot.org/c/coreboot/+/39135/7/src/soc/intel/tigerlake/jsl... File src/soc/intel/tigerlake/jsl_memcfg_init.c:
https://review.coreboot.org/c/coreboot/+/39135/7/src/soc/intel/tigerlake/jsl... PS7, Line 136: for (int i = 0; i < NUM_DIMM_SLOT; i++) { I think some of these can be simplified based on what the memory controller really supports. Please see TGL as reference.
Meera Ravindranath has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 7:
(5 comments)
https://review.coreboot.org/c/coreboot/+/39135/7/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/jsl_memcfg_init.h:
https://review.coreboot.org/c/coreboot/+/39135/7/src/soc/intel/tigerlake/inc... PS7, Line 70: struct spd_info spd[NUM_DIMM_SLOT];
Does JSL support different types of memories for each slot? If not, mainboard can just pass in SPD i […]
No it does not support different memories for each slot. Yes, the implementation like TGL is under progress. Will post a new CL soon.
https://review.coreboot.org/c/coreboot/+/39135/7/src/soc/intel/tigerlake/inc... PS7, Line 103: uint16_t rcomp_resistor[3];
On TGL, we were told that rcomp_resistor and rcomp_target need to be set to auto configurable by MRC […]
In JSL, we can't auto configure it as the board values deviates and need to be passed in mainboard.
https://review.coreboot.org/c/coreboot/+/39135/7/src/soc/intel/tigerlake/inc... PS7, Line 136: void jasperlake_memcfg_init(FSP_M_CONFIG *mem_cfg, : const struct jsl_mb_cfg *jsl_cfg);
Quick comment: Please take a look at TGL memcfg_init. […]
Ack
https://review.coreboot.org/c/coreboot/+/39135/2/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/jsl_memcfg_init.h:
https://review.coreboot.org/c/coreboot/+/39135/2/src/soc/intel/tigerlake/inc... PS2, Line 48: READ_SMBUS,
Since this is SoC code, prefer to have all the methods to provide SPD info. […]
Ack
https://review.coreboot.org/c/coreboot/+/39135/2/src/soc/intel/tigerlake/inc... PS2, Line 136: jasperlake
Even though both SoCs configure memory differently, only one of them will be enabled/used at a time […]
Ack
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39135/7/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/jsl_memcfg_init.h:
https://review.coreboot.org/c/coreboot/+/39135/7/src/soc/intel/tigerlake/inc... PS7, Line 31: */ Nit: Can be a comment in single line.
https://review.coreboot.org/c/coreboot/+/39135/7/src/soc/intel/tigerlake/inc... PS7, Line 50: Nit: Use tab for indentation. I think space is being used.
Hello Patrick Rudolph, Karthik Ramasubramanian, Subrata Banik, Aamir Bohra, Rizwan Qureshi, Tim Wawrzynczak, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39135
to look at the new patch set (#9).
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
src/soc/tigerlake: Add memory configuration support for Jasper Lake
BUG=none BRANCH=none TEST=Build and verify boot of WaddleDoo.
Change-Id: I8de502d3f05d52b9dae34e3b013c6d5b1896fa85 Signed-off-by: Meera Ravindranath meera.ravindranath@intel.com --- M src/soc/intel/tigerlake/Makefile.inc A src/soc/intel/tigerlake/include/soc/meminit_jsl.h A src/soc/intel/tigerlake/jsl_memcfg_init.c A src/soc/intel/tigerlake/meminit_jsl.c 4 files changed, 422 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/39135/9
Hello Patrick Rudolph, Karthik Ramasubramanian, Subrata Banik, Aamir Bohra, Rizwan Qureshi, Tim Wawrzynczak, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39135
to look at the new patch set (#10).
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
src/soc/tigerlake: Add memory configuration support for Jasper Lake
BUG=none BRANCH=none TEST=Build and verify boot of WaddleDoo.
Change-Id: I8de502d3f05d52b9dae34e3b013c6d5b1896fa85 Signed-off-by: Meera Ravindranath meera.ravindranath@intel.com --- M src/soc/intel/tigerlake/Makefile.inc A src/soc/intel/tigerlake/include/soc/meminit_jsl.h A src/soc/intel/tigerlake/meminit_jsl.c 3 files changed, 257 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/39135/10
Hello Patrick Rudolph, Karthik Ramasubramanian, Subrata Banik, Aamir Bohra, Rizwan Qureshi, Tim Wawrzynczak, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39135
to look at the new patch set (#11).
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
src/soc/tigerlake: Add memory configuration support for Jasper Lake
BUG=none BRANCH=none TEST=Build and verify boot of WaddleDoo.
Change-Id: I8de502d3f05d52b9dae34e3b013c6d5b1896fa85 Signed-off-by: Meera Ravindranath meera.ravindranath@intel.com --- M src/soc/intel/tigerlake/Makefile.inc A src/soc/intel/tigerlake/include/soc/meminit_jsl.h A src/soc/intel/tigerlake/meminit_jsl.c 3 files changed, 257 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/39135/11
Meera Ravindranath has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 11:
(4 comments)
https://review.coreboot.org/c/coreboot/+/39135/7/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/jsl_memcfg_init.h:
https://review.coreboot.org/c/coreboot/+/39135/7/src/soc/intel/tigerlake/inc... PS7, Line 31: */
Nit: Can be a comment in single line.
Done
https://review.coreboot.org/c/coreboot/+/39135/7/src/soc/intel/tigerlake/inc... PS7, Line 50:
Nit: Use tab for indentation. I think space is being used.
Done
https://review.coreboot.org/c/coreboot/+/39135/7/src/soc/intel/tigerlake/inc... PS7, Line 70: struct spd_info spd[NUM_DIMM_SLOT];
No it does not support different memories for each slot. […]
Done
https://review.coreboot.org/c/coreboot/+/39135/7/src/soc/intel/tigerlake/inc... PS7, Line 136: void jasperlake_memcfg_init(FSP_M_CONFIG *mem_cfg, : const struct jsl_mb_cfg *jsl_cfg);
Ack
Done
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 11:
(2 comments)
Please address below comments, rest looks good to me
https://review.coreboot.org/c/coreboot/+/39135/11/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_jsl.h:
https://review.coreboot.org/c/coreboot/+/39135/11/src/soc/intel/tigerlake/in... PS11, Line 4: * Copyright Intel Corporation 2020. please use header as you did here
https://review.coreboot.org/c/coreboot/+/39136/9/src/mainboard/google/dedede...
https://review.coreboot.org/c/coreboot/+/39135/11/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_jsl.c:
https://review.coreboot.org/c/coreboot/+/39135/11/src/soc/intel/tigerlake/me... PS11, Line 4: * Copyright Intel Corporation 2020. same here https://review.coreboot.org/c/coreboot/+/39136/9/src/mainboard/google/dedede...
Hello Patrick Rudolph, Karthik Ramasubramanian, Subrata Banik, Aamir Bohra, Rizwan Qureshi, Tim Wawrzynczak, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39135
to look at the new patch set (#12).
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
src/soc/tigerlake: Add memory configuration support for Jasper Lake
BUG=none BRANCH=none TEST=Build and verify boot of WaddleDoo.
Change-Id: I8de502d3f05d52b9dae34e3b013c6d5b1896fa85 Signed-off-by: Meera Ravindranath meera.ravindranath@intel.com --- M src/soc/intel/tigerlake/Makefile.inc A src/soc/intel/tigerlake/include/soc/meminit_jsl.h A src/soc/intel/tigerlake/meminit_jsl.c 3 files changed, 257 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/39135/12
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39135/12/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_jsl.h:
https://review.coreboot.org/c/coreboot/+/39135/12/src/soc/intel/tigerlake/in... PS12, Line 4: Copyright (C) Intel Corporation 2020. Copyright (C) 2020 Intel Corporation.
Hello Patrick Rudolph, Karthik Ramasubramanian, Subrata Banik, Aamir Bohra, Rizwan Qureshi, Tim Wawrzynczak, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39135
to look at the new patch set (#13).
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
src/soc/tigerlake: Add memory configuration support for Jasper Lake
BUG=none BRANCH=none TEST=Build and verify boot of WaddleDoo.
Change-Id: I8de502d3f05d52b9dae34e3b013c6d5b1896fa85 Signed-off-by: Meera Ravindranath meera.ravindranath@intel.com --- M src/soc/intel/tigerlake/Makefile.inc A src/soc/intel/tigerlake/include/soc/meminit_jsl.h A src/soc/intel/tigerlake/meminit_jsl.c 3 files changed, 257 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/39135/13
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 13: Code-Review+2
Ronak Kanabar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 13: Code-Review+2
Meera Ravindranath has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 13:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39135/12/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_jsl.h:
https://review.coreboot.org/c/coreboot/+/39135/12/src/soc/intel/tigerlake/in... PS12, Line 4: Copyright (C) Intel Corporation 2020.
Copyright (C) 2020 Intel Corporation.
Done
https://review.coreboot.org/c/coreboot/+/39135/11/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_jsl.h:
https://review.coreboot.org/c/coreboot/+/39135/11/src/soc/intel/tigerlake/in... PS11, Line 4: * Copyright Intel Corporation 2020.
please use header as you did here […]
Done
https://review.coreboot.org/c/coreboot/+/39135/11/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_jsl.c:
https://review.coreboot.org/c/coreboot/+/39135/11/src/soc/intel/tigerlake/me... PS11, Line 4: * Copyright Intel Corporation 2020.
same here https://review.coreboot.org/c/coreboot/+/39136/9/src/mainboard/google/dedede.... […]
Done
Meera Ravindranath has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 14:
(4 comments)
https://review.coreboot.org/c/coreboot/+/39135/7/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/jsl_memcfg_init.h:
https://review.coreboot.org/c/coreboot/+/39135/7/src/soc/intel/tigerlake/inc... PS7, Line 103: uint16_t rcomp_resistor[3];
In JSL, we can't auto configure it as the board values deviates and need to be passed in mainboard.
https://review.coreboot.org/c/coreboot/+/39135/7/src/soc/intel/tigerlake/inc... PS7, Line 116: dq_pins_interleaved
Does JSL with LPDDR4 allow interleaved memory configuration? I know with TGL this was not a possibil […]
Furquan, i understand your concern that interleaved might not required here as FSP default and coreboot UPD over ride both sets to 0 hence we can avoid the same, but be in safer side, i might need to get this confirmation from HW team. Can we do additional clean up if required to remove interleaved later?
https://review.coreboot.org/c/coreboot/+/39135/2/src/soc/intel/tigerlake/jsl... File src/soc/intel/tigerlake/jsl_memcfg_init.c:
https://review.coreboot.org/c/coreboot/+/39135/2/src/soc/intel/tigerlake/jsl... PS2, Line 139: NOT_EXISTING:
Ok, I believe that means we don't need to pass the strap information regarding only half the memory […]
Yes, This is not applicable to JSL. Hence we would not be using the half populated memory modules type of configuration.
https://review.coreboot.org/c/coreboot/+/39135/7/src/soc/intel/tigerlake/jsl... File src/soc/intel/tigerlake/jsl_memcfg_init.c:
https://review.coreboot.org/c/coreboot/+/39135/7/src/soc/intel/tigerlake/jsl... PS7, Line 136: for (int i = 0; i < NUM_DIMM_SLOT; i++) {
I think some of these can be simplified based on what the memory controller really supports. […]
Done
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 15:
(7 comments)
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_jsl.h:
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/in... PS14, Line 43: READ_SMBUS, Not used anywhere. Can be added later if the use-case arises. Same for NOT_EXISTING.
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/in... PS14, Line 73: meminit_lpddr4() I think it is meminit_dq_dqs_map that is clearing the unused fields. Also I see that it is set to 0 at the mainboard_config
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/in... PS14, Line 117: uint8_t vref_ca_config; vref_ca config is marked as not applicable for LPDDR4x. Do we still need to configure it? Atleast configured in the mainboard config.
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/in... PS14, Line 132: ) We still need to allow half population.
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_jsl.c:
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/me... PS14, Line 61: / Nit: Fix the indentation.
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/me... PS14, Line 82: /* Early Jasper Lake requires rcomp targets to be 0 */ Based on the comment, did you mean memset RcompTarget to 0. Else the RcompTarget is copied as passed by the mainboard and is not 0.
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/me... PS14, Line 93: mem_cfg->MemorySpdPtr01 = 0; No need to set it to spd_data_ptr for LPDDR4x with 4 slots?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 15: -Code-Review
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 15:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_jsl.h:
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/in... PS14, Line 43: READ_SMBUS,
Not used anywhere. Can be added later if the use-case arises. Same for NOT_EXISTING.
this is valid. withdrawn my +2 vote
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/in... PS14, Line 132: )
We still need to allow half population.
i believe we have only 1 memory controller on JSL hence those concepts might not be applicable here
Hello Patrick Rudolph, Karthik Ramasubramanian, Subrata Banik, Ronak Kanabar, Aamir Bohra, Rizwan Qureshi, Tim Wawrzynczak, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39135
to look at the new patch set (#16).
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
src/soc/tigerlake: Add memory configuration support for Jasper Lake
BUG=none BRANCH=none TEST=Build and verify boot of WaddleDoo.
Change-Id: I8de502d3f05d52b9dae34e3b013c6d5b1896fa85 Signed-off-by: Meera Ravindranath meera.ravindranath@intel.com --- M src/soc/intel/tigerlake/Makefile.inc A src/soc/intel/tigerlake/include/soc/meminit_jsl.h A src/soc/intel/tigerlake/meminit_jsl.c 3 files changed, 257 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/39135/16
Hello Patrick Rudolph, Karthik Ramasubramanian, Subrata Banik, Ronak Kanabar, Aamir Bohra, Rizwan Qureshi, Tim Wawrzynczak, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39135
to look at the new patch set (#17).
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
src/soc/tigerlake: Add memory configuration support for Jasper Lake
BUG=none BRANCH=none TEST=Build and verify boot of WaddleDoo.
Change-Id: I8de502d3f05d52b9dae34e3b013c6d5b1896fa85 Signed-off-by: Meera Ravindranath meera.ravindranath@intel.com --- M src/soc/intel/tigerlake/Makefile.inc A src/soc/intel/tigerlake/include/soc/meminit_jsl.h A src/soc/intel/tigerlake/meminit_jsl.c 3 files changed, 245 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/39135/17
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 17: Code-Review+2
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 17:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39135/17/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_jsl.h:
https://review.coreboot.org/c/coreboot/+/39135/17/src/soc/intel/tigerlake/in... PS17, Line 49: /* To read on-module spd when read_type is READ_SMBUS. */ : uint8_t spd_smbus_address; : can remove this as well.
Meera Ravindranath has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 17:
(7 comments)
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_jsl.h:
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/in... PS14, Line 43: READ_SMBUS,
this is valid. […]
Ack
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/in... PS14, Line 73: meminit_lpddr4()
I think it is meminit_dq_dqs_map that is clearing the unused fields.
Ack. Done.
Also I see that it is set to 0 at the mainboard_config
So the the last 6 bytes of dq map for Ch1 and Ch2 corresponds to DIMM1 and DIMM3 respectively which are unused for JSL. Hence the value 0. So in any case we first clear all the 12 bytes of data and then assign the board values.
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/in... PS14, Line 117: uint8_t vref_ca_config;
vref_ca config is marked as not applicable for LPDDR4x.
Need to check on this from the MRC team and get back.
Do we still need to configure it? Atleast configured in the mainboard config.
The default UPD value is 2 and we are passing the same from mainboard. Hence removing it.
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/in... PS14, Line 132: )
i believe we have only 1 memory controller on JSL hence those concepts might not be applicable here
I second Subrata. We do not support 2 memory controllers on JSL and think half population won't be applicable here.
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_jsl.c:
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/me... PS14, Line 61: /
Nit: Fix the indentation.
Done
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/me... PS14, Line 82: /* Early Jasper Lake requires rcomp targets to be 0 */
Based on the comment, did you mean memset RcompTarget to 0. […]
This comment is confusing, hence removed it.And no, we don't memset it to 0. We pass the board values. Thanks for the finding.
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/me... PS14, Line 93: mem_cfg->MemorySpdPtr01 = 0;
No need to set it to spd_data_ptr for LPDDR4x with 4 slots?
LPDDR4 supports 2 channels which means 4 DIMMs can be connected. Since we are connecting the DIMMs only on the 0th and 2nd slot, we don't have to set the spd data pointer to the other two slots.
Hello Patrick Rudolph, Karthik Ramasubramanian, Subrata Banik, Aamir Bohra, Ronak Kanabar, Rizwan Qureshi, Tim Wawrzynczak, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39135
to look at the new patch set (#18).
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
src/soc/tigerlake: Add memory configuration support for Jasper Lake
BUG=none BRANCH=none TEST=Build and verify boot of WaddleDoo.
Change-Id: I8de502d3f05d52b9dae34e3b013c6d5b1896fa85 Signed-off-by: Meera Ravindranath meera.ravindranath@intel.com --- M src/soc/intel/tigerlake/Makefile.inc A src/soc/intel/tigerlake/include/soc/meminit_jsl.h A src/soc/intel/tigerlake/meminit_jsl.c 3 files changed, 242 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/39135/18
Meera Ravindranath has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 18:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39135/17/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_jsl.h:
https://review.coreboot.org/c/coreboot/+/39135/17/src/soc/intel/tigerlake/in... PS17, Line 49: /* To read on-module spd when read_type is READ_SMBUS. */ : uint8_t spd_smbus_address; :
can remove this as well.
Done
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 18: Code-Review+2
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 18:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_jsl.h:
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/in... PS14, Line 132: )
I second Subrata. […]
What if only 2 of the 4 LPDDR4x memory components/slots are populated. Currently we have a strap to identify the number of memory slots populated. For example in CNL, spd[2] is filled if the concerned slot is populated and MemorySpdPtr10 UPD is filled in accordingly. Please bear with me if I am butchering the terminologies.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 18:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_jsl.h:
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/in... PS14, Line 132: )
What if only 2 of the 4 LPDDR4x memory components/slots are populated. […]
Hi Karthik, I understood what you are referring. let me try to explain my understanding. Typically on chrome design we have seen LPDDRx DIMMs are connected at CHO, Slot_0 and CH1, Slot_0. Hence Meera has updated SPD here accordingly
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/me...
So far i haven't seen any alternative to this assumptions.
but your point is valid, what is some variant design of "dedede" baseboard has DIMM connected at CH0, Slot 1 and CH1, Slot 1 then above logic will fail and system will stuck.
In my opinion, we should have below logic to break all assumption and there is no harm of assigning SPD pointer to all FSP UPD. MRC logic is that intelligent to first understand the DIMM present then does the training.
/* Channel 0 */ mem_cfg->MemorySpdPtr00 = spd_data_ptr; mem_cfg->MemorySpdPtr01 = spd_data_ptr;
/* Channel 1 */ mem_cfg->MemorySpdPtr10 = spd_data_ptr; mem_cfg->MemorySpdPtr11 = spd_data_ptr;
Do you agree?
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/in... PS14, Line 132: spd can you please change this to "spd_info" for better readability ?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 18:
(8 comments)
https://review.coreboot.org/c/coreboot/+/39135/7/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/jsl_memcfg_init.h:
https://review.coreboot.org/c/coreboot/+/39135/7/src/soc/intel/tigerlake/inc... PS7, Line 103: uint16_t rcomp_resistor[3];
In JSL, we can't auto configure it as the board values deviates and need to be passed in mainboard […]
Doesn't need to block this change, but can you please provide reference to where this information is captured? Do you have a DQ-DQS sheet for JSL similar to the one that is present for previous platforms?
https://review.coreboot.org/c/coreboot/+/39135/7/src/soc/intel/tigerlake/inc... PS7, Line 116: dq_pins_interleaved
Furquan, i understand your concern that interleaved might not required here as FSP default and core […]
Sure. Can you please file a bug to capture this?
BTW, as per PDG, Chapter 6, Table 9 Notes, it looks like interleaving is not supported.
https://review.coreboot.org/c/coreboot/+/39135/18/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_jsl.h:
https://review.coreboot.org/c/coreboot/+/39135/18/src/soc/intel/tigerlake/in... PS18, Line 73: 6 Why does this not have a macro of its own?
https://review.coreboot.org/c/coreboot/+/39135/18/src/soc/intel/tigerlake/in... PS18, Line 106: Enabled That is not correct. It could be enabled/disabled based on board setting.
https://review.coreboot.org/c/coreboot/+/39135/18/src/soc/intel/tigerlake/in... PS18, Line 110: UserBd Should there be an enum for what FSP expects?
https://review.coreboot.org/c/coreboot/+/39135/18/src/soc/intel/tigerlake/in... PS18, Line 118: const struct mb_cfg *board_cfg, nit: Would this fit on the line above?
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_jsl.h:
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/in... PS14, Line 132: )
Hi Karthik, I understood what you are referring. let me try to explain my understanding. […]
I think what Karthik is referring to here is:
-- JSL has 2 logical channels say Ch0 and Ch1 -- This corresponds to: Logical Ch0: MemorySpdPtr00 --> spd_data_ptr, MemorySpdPtr01 --> NULL Logical Ch1: MemorySpdPtr10 --> spd_data_ptr, MemorySpdPtr11 --> NULL -- When channels are half-populated, then logical Ch1 would be left empty and logical Ch0 is still populated. Thus, the FSP configuration would look like: Logical Ch0: MemorySpdPtr00 --> spd_data_ptr, MemorySpdPtr01 --> NULL Logical Ch1: MemorySpdPtr10 --> NULL, MemorySpdPtr11 --> NULL
https://review.coreboot.org/c/coreboot/+/39135/18/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_jsl.c:
https://review.coreboot.org/c/coreboot/+/39135/18/src/soc/intel/tigerlake/me... PS18, Line 68: sizeof(board_cfg->dq_map[DDR_CH0]) I am confused by the comment above and the memset. Isn't this memcpy actually copying all the 12 bytes?
Meera Ravindranath has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 18:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_jsl.h:
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/in... PS14, Line 132: )
I think what Karthik is referring to here is: […]
Okay. But Furquan, I am not understanding the direction here. Are you suggesting us to have a half populated configuration? If so, what would be the benefit of it?
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/in... PS14, Line 132: spd
can you please change this to "spd_info" for better readability ?
Ack
Hello Patrick Rudolph, Karthik Ramasubramanian, Subrata Banik, Aamir Bohra, Ronak Kanabar, Rizwan Qureshi, Tim Wawrzynczak, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39135
to look at the new patch set (#19).
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
src/soc/tigerlake: Add memory configuration support for Jasper Lake
BUG=none BRANCH=none TEST=Build and verify boot of WaddleDoo.
Change-Id: I8de502d3f05d52b9dae34e3b013c6d5b1896fa85 Signed-off-by: Meera Ravindranath meera.ravindranath@intel.com --- M src/soc/intel/tigerlake/Makefile.inc A src/soc/intel/tigerlake/include/soc/meminit_jsl.h A src/soc/intel/tigerlake/meminit_jsl.c 3 files changed, 238 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/39135/19
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 19:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39135/19/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_jsl.h:
https://review.coreboot.org/c/coreboot/+/39135/19/src/soc/intel/tigerlake/in... PS19, Line 116: void memcfg_init(FSP_M_CONFIG *mem_cfg,const struct mb_cfg *board_cfg, space required after that ',' (ctx:VxV)
Maulik V Vaghela has uploaded a new patch set (#20) to the change originally created by Meera Ravindranath. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
src/soc/tigerlake: Add memory configuration support for Jasper Lake
BUG=none BRANCH=none TEST=Build and verify boot of WaddleDoo.
Change-Id: I8de502d3f05d52b9dae34e3b013c6d5b1896fa85 Signed-off-by: Meera Ravindranath meera.ravindranath@intel.com --- M src/soc/intel/tigerlake/Makefile.inc A src/soc/intel/tigerlake/include/soc/meminit_jsl.h A src/soc/intel/tigerlake/meminit_jsl.c 3 files changed, 238 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/39135/20
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 20:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39135/20/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_jsl.h:
https://review.coreboot.org/c/coreboot/+/39135/20/src/soc/intel/tigerlake/in... PS20, Line 116: void memcfg_init(FSP_M_CONFIG *mem_cfg,const struct mb_cfg *board_cfg, space required after that ',' (ctx:VxV)
Meera Ravindranath has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 20:
(6 comments)
https://review.coreboot.org/c/coreboot/+/39135/18/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_jsl.h:
https://review.coreboot.org/c/coreboot/+/39135/18/src/soc/intel/tigerlake/in... PS18, Line 73: 6
Why does this not have a macro of its own?
Done
https://review.coreboot.org/c/coreboot/+/39135/18/src/soc/intel/tigerlake/in... PS18, Line 106: Enabled
That is not correct. It could be enabled/disabled based on board setting.
Done
https://review.coreboot.org/c/coreboot/+/39135/18/src/soc/intel/tigerlake/in... PS18, Line 110: UserBd
Should there be an enum for what FSP expects?
Yes, Tigerlake defines the enum in src/soc/intel/tigerlake/include/soc/romstage.h and hence we are reusing it instead of redefining it here again.
https://review.coreboot.org/c/coreboot/+/39135/18/src/soc/intel/tigerlake/in... PS18, Line 118: const struct mb_cfg *board_cfg,
nit: Would this fit on the line above?
Done
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_jsl.h:
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/in... PS14, Line 132: spd
Ack
Done
https://review.coreboot.org/c/coreboot/+/39135/18/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_jsl.c:
https://review.coreboot.org/c/coreboot/+/39135/18/src/soc/intel/tigerlake/me... PS18, Line 68: sizeof(board_cfg->dq_map[DDR_CH0])
I am confused by the comment above and the memset. […]
Ack Yes it copies all 12 bytes of data. Since the default UPD is 0, there is no point of the memset above. Removed it.
Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Maulik V Vaghela, Rizwan Qureshi, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Karthik Ramasubramanian, Ronak Kanabar, Tim Wawrzynczak, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39135
to look at the new patch set (#21).
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
src/soc/tigerlake: Add memory configuration support for Jasper Lake
BUG=none BRANCH=none TEST=Build and verify boot of WaddleDoo.
Change-Id: I8de502d3f05d52b9dae34e3b013c6d5b1896fa85 Signed-off-by: Meera Ravindranath meera.ravindranath@intel.com --- M src/soc/intel/tigerlake/Makefile.inc A src/soc/intel/tigerlake/include/soc/meminit_jsl.h A src/soc/intel/tigerlake/meminit_jsl.c 3 files changed, 238 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/39135/21
Meera Ravindranath has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 21:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39135/7/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/jsl_memcfg_init.h:
https://review.coreboot.org/c/coreboot/+/39135/7/src/soc/intel/tigerlake/inc... PS7, Line 103: uint16_t rcomp_resistor[3];
Doesn't need to block this change, but can you please provide reference to where this information is […]
Sure, Furquan. We would raise a crosbug and share the relevant document.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 21:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_jsl.h:
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/in... PS14, Line 132: )
Okay. But Furquan, I am not understanding the direction here. […]
Meera, Furquan articulated exactly what I was referring to. In the past for pricing reasons, memory has been populated on only one channels by OEMs - eg. Octopus and Hatch. The performance was not too bad in those situations.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 21:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39135/21/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_jsl.c:
https://review.coreboot.org/c/coreboot/+/39135/21/src/soc/intel/tigerlake/me... PS21, Line 101: const struct mb_cfg *board_cfg, Nit: It can be accomodated in the previous line as commented by Furquan in the header file.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 21:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39135/7/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/jsl_memcfg_init.h:
https://review.coreboot.org/c/coreboot/+/39135/7/src/soc/intel/tigerlake/inc... PS7, Line 103: uint16_t rcomp_resistor[3];
Sure, Furquan. We would raise a crosbug and share the relevant document.
I have asked for the relevant info for the DQ-DQs mapping in this crosbug - b/150154457 - c#8. You can use that bug.
As for the Rcomp resistor values, it matches with the resistors being populated in the schematics.
Subrata Banik has uploaded a new patch set (#22) to the change originally created by Meera Ravindranath. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
src/soc/tigerlake: Add memory configuration support for Jasper Lake
BUG=none BRANCH=none TEST=Build and verify boot of WaddleDoo.
Change-Id: I8de502d3f05d52b9dae34e3b013c6d5b1896fa85 Signed-off-by: Meera Ravindranath meera.ravindranath@intel.com --- M src/soc/intel/tigerlake/Makefile.inc A src/soc/intel/tigerlake/include/soc/meminit_jsl.h A src/soc/intel/tigerlake/meminit_jsl.c 3 files changed, 242 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/39135/22
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 21:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_jsl.h:
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/in... PS14, Line 132: )
Meera, Furquan articulated exactly what I was referring to. […]
Ack
https://review.coreboot.org/c/coreboot/+/39135/21/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_jsl.c:
https://review.coreboot.org/c/coreboot/+/39135/21/src/soc/intel/tigerlake/me... PS21, Line 101: const struct mb_cfg *board_cfg,
Nit: It can be accomodated in the previous line as commented by Furquan in the header file.
Ack
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 22:
(16 comments)
https://review.coreboot.org/c/coreboot/+/39135/22/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_jsl.c:
https://review.coreboot.org/c/coreboot/+/39135/22/src/soc/intel/tigerlake/me... PS22, Line 24: do { \ code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39135/22/src/soc/intel/tigerlake/me... PS22, Line 24: do { \ please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/39135/22/src/soc/intel/tigerlake/me... PS22, Line 25: memcpy(&_mem_cfg->DqByteMapCh ## _ch, \ code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39135/22/src/soc/intel/tigerlake/me... PS22, Line 25: memcpy(&_mem_cfg->DqByteMapCh ## _ch, \ please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/39135/22/src/soc/intel/tigerlake/me... PS22, Line 26: &_b_cfg->dq_map[_ch], \ code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39135/22/src/soc/intel/tigerlake/me... PS22, Line 26: &_b_cfg->dq_map[_ch], \ please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/39135/22/src/soc/intel/tigerlake/me... PS22, Line 27: sizeof(_b_cfg->dq_map[_ch])); \ code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39135/22/src/soc/intel/tigerlake/me... PS22, Line 27: sizeof(_b_cfg->dq_map[_ch])); \ please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/39135/22/src/soc/intel/tigerlake/me... PS22, Line 28: memcpy(&_mem_cfg->DqsMapCpu2DramCh ## _ch, \ code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39135/22/src/soc/intel/tigerlake/me... PS22, Line 28: memcpy(&_mem_cfg->DqsMapCpu2DramCh ## _ch, \ please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/39135/22/src/soc/intel/tigerlake/me... PS22, Line 29: &_b_cfg->dqs_map[_ch], \ code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39135/22/src/soc/intel/tigerlake/me... PS22, Line 29: &_b_cfg->dqs_map[_ch], \ please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/39135/22/src/soc/intel/tigerlake/me... PS22, Line 30: sizeof(_b_cfg->dqs_map[_ch])); \ code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39135/22/src/soc/intel/tigerlake/me... PS22, Line 30: sizeof(_b_cfg->dqs_map[_ch])); \ please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/39135/22/src/soc/intel/tigerlake/me... PS22, Line 31: } while (0) code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39135/22/src/soc/intel/tigerlake/me... PS22, Line 31: } while (0) please, no spaces at the start of a line
Subrata Banik has uploaded a new patch set (#23) to the change originally created by Meera Ravindranath. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
src/soc/tigerlake: Add memory configuration support for Jasper Lake
BUG=none BRANCH=none TEST=Build and verify boot of WaddleDoo.
Change-Id: I8de502d3f05d52b9dae34e3b013c6d5b1896fa85 Signed-off-by: Meera Ravindranath meera.ravindranath@intel.com --- M src/soc/intel/tigerlake/Makefile.inc A src/soc/intel/tigerlake/include/soc/meminit_jsl.h A src/soc/intel/tigerlake/meminit_jsl.c 3 files changed, 242 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/39135/23
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 23:
(16 comments)
https://review.coreboot.org/c/coreboot/+/39135/22/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_jsl.c:
https://review.coreboot.org/c/coreboot/+/39135/22/src/soc/intel/tigerlake/me... PS22, Line 24: do { \
code indent should use tabs where possible
Ack
https://review.coreboot.org/c/coreboot/+/39135/22/src/soc/intel/tigerlake/me... PS22, Line 24: do { \
please, no spaces at the start of a line
Ack
https://review.coreboot.org/c/coreboot/+/39135/22/src/soc/intel/tigerlake/me... PS22, Line 25: memcpy(&_mem_cfg->DqByteMapCh ## _ch, \
code indent should use tabs where possible
Ack
https://review.coreboot.org/c/coreboot/+/39135/22/src/soc/intel/tigerlake/me... PS22, Line 25: memcpy(&_mem_cfg->DqByteMapCh ## _ch, \
please, no spaces at the start of a line
Ack
https://review.coreboot.org/c/coreboot/+/39135/22/src/soc/intel/tigerlake/me... PS22, Line 26: &_b_cfg->dq_map[_ch], \
please, no spaces at the start of a line
Ack
https://review.coreboot.org/c/coreboot/+/39135/22/src/soc/intel/tigerlake/me... PS22, Line 26: &_b_cfg->dq_map[_ch], \
code indent should use tabs where possible
Ack
https://review.coreboot.org/c/coreboot/+/39135/22/src/soc/intel/tigerlake/me... PS22, Line 27: sizeof(_b_cfg->dq_map[_ch])); \
code indent should use tabs where possible
Ack
https://review.coreboot.org/c/coreboot/+/39135/22/src/soc/intel/tigerlake/me... PS22, Line 27: sizeof(_b_cfg->dq_map[_ch])); \
please, no spaces at the start of a line
Ack
https://review.coreboot.org/c/coreboot/+/39135/22/src/soc/intel/tigerlake/me... PS22, Line 28: memcpy(&_mem_cfg->DqsMapCpu2DramCh ## _ch, \
please, no spaces at the start of a line
Ack
https://review.coreboot.org/c/coreboot/+/39135/22/src/soc/intel/tigerlake/me... PS22, Line 28: memcpy(&_mem_cfg->DqsMapCpu2DramCh ## _ch, \
code indent should use tabs where possible
Ack
https://review.coreboot.org/c/coreboot/+/39135/22/src/soc/intel/tigerlake/me... PS22, Line 29: &_b_cfg->dqs_map[_ch], \
please, no spaces at the start of a line
Ack
https://review.coreboot.org/c/coreboot/+/39135/22/src/soc/intel/tigerlake/me... PS22, Line 29: &_b_cfg->dqs_map[_ch], \
code indent should use tabs where possible
Ack
https://review.coreboot.org/c/coreboot/+/39135/22/src/soc/intel/tigerlake/me... PS22, Line 30: sizeof(_b_cfg->dqs_map[_ch])); \
please, no spaces at the start of a line
Ack
https://review.coreboot.org/c/coreboot/+/39135/22/src/soc/intel/tigerlake/me... PS22, Line 30: sizeof(_b_cfg->dqs_map[_ch])); \
code indent should use tabs where possible
Ack
https://review.coreboot.org/c/coreboot/+/39135/22/src/soc/intel/tigerlake/me... PS22, Line 31: } while (0)
code indent should use tabs where possible
Ack
https://review.coreboot.org/c/coreboot/+/39135/22/src/soc/intel/tigerlake/me... PS22, Line 31: } while (0)
please, no spaces at the start of a line
Ack
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 23:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39135/23/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_jsl.c:
https://review.coreboot.org/c/coreboot/+/39135/23/src/soc/intel/tigerlake/me... PS23, Line 25: memcpy(&_mem_cfg->DqByteMapCh ## _ch, \ : &_b_cfg->dq_map[_ch], \ : sizeof(_b_cfg->dq_map[_ch])); \ : memcpy(&_mem_cfg->DqsMapCpu2DramCh ## _ch, \ : &_b_cfg->dqs_map[_ch], \ : sizeof(_b_cfg->dqs_map[_ch])); \ I really think this looks a lot better as just a little static function. Because it's small enough, the compiler ought to inline it anyway since it's only used in meminit_dq_dqs_map
Subrata Banik has uploaded a new patch set (#24) to the change originally created by Meera Ravindranath. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
src/soc/tigerlake: Add memory configuration support for Jasper Lake
BUG=none BRANCH=none TEST=Build and verify boot of WaddleDoo.
Change-Id: I8de502d3f05d52b9dae34e3b013c6d5b1896fa85 Signed-off-by: Meera Ravindranath meera.ravindranath@intel.com --- M src/soc/intel/tigerlake/Makefile.inc A src/soc/intel/tigerlake/include/soc/meminit_jsl.h A src/soc/intel/tigerlake/meminit_jsl.c 3 files changed, 241 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/39135/24
Subrata Banik has uploaded a new patch set (#25) to the change originally created by Meera Ravindranath. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
src/soc/tigerlake: Add memory configuration support for Jasper Lake
BUG=none BRANCH=none TEST=Build and verify boot of WaddleDoo.
Change-Id: I8de502d3f05d52b9dae34e3b013c6d5b1896fa85 Signed-off-by: Meera Ravindranath meera.ravindranath@intel.com --- M src/soc/intel/tigerlake/Makefile.inc A src/soc/intel/tigerlake/include/soc/meminit_jsl.h A src/soc/intel/tigerlake/meminit_jsl.c 3 files changed, 240 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/39135/25
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 25:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39135/23/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_jsl.c:
https://review.coreboot.org/c/coreboot/+/39135/23/src/soc/intel/tigerlake/me... PS23, Line 25: memcpy(&_mem_cfg->DqByteMapCh ## _ch, \ : &_b_cfg->dq_map[_ch], \ : sizeof(_b_cfg->dq_map[_ch])); \ : memcpy(&_mem_cfg->DqsMapCpu2DramCh ## _ch, \ : &_b_cfg->dqs_map[_ch], \ : sizeof(_b_cfg->dqs_map[_ch])); \
I really think this looks a lot better as just a little static function. […]
tried creating helper function bt looks like macro append won't work with static function. restoring the old method
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 25: Code-Review+2
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 25: Code-Review+2
V Sowmya has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 25: Code-Review+2
Maulik V Vaghela has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 25: Code-Review+2
Ronak Kanabar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 25: Code-Review+2
Subrata Banik has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
src/soc/tigerlake: Add memory configuration support for Jasper Lake
BUG=none BRANCH=none TEST=Build and verify boot of WaddleDoo.
Change-Id: I8de502d3f05d52b9dae34e3b013c6d5b1896fa85 Signed-off-by: Meera Ravindranath meera.ravindranath@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/39135 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Karthik Ramasubramanian kramasub@google.com Reviewed-by: Aamir Bohra aamir.bohra@intel.com Reviewed-by: V Sowmya v.sowmya@intel.com Reviewed-by: Maulik V Vaghela maulik.v.vaghela@intel.com Reviewed-by: Ronak Kanabar ronak.kanabar@intel.com --- M src/soc/intel/tigerlake/Makefile.inc A src/soc/intel/tigerlake/include/soc/meminit_jsl.h A src/soc/intel/tigerlake/meminit_jsl.c 3 files changed, 240 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Aamir Bohra: Looks good to me, approved V Sowmya: Looks good to me, approved Maulik V Vaghela: Looks good to me, approved Karthik Ramasubramanian: Looks good to me, approved Ronak Kanabar: Looks good to me, approved
diff --git a/src/soc/intel/tigerlake/Makefile.inc b/src/soc/intel/tigerlake/Makefile.inc index 56119f5..89ef877 100644 --- a/src/soc/intel/tigerlake/Makefile.inc +++ b/src/soc/intel/tigerlake/Makefile.inc @@ -26,6 +26,7 @@ romstage-y += espi.c romstage-y += gpio.c romstage-$(CONFIG_SOC_INTEL_TIGERLAKE) += meminit_tgl.c +romstage-$(CONFIG_SOC_INTEL_JASPERLAKE) += meminit_jsl.c romstage-y += reset.c
ramstage-y += acpi.c diff --git a/src/soc/intel/tigerlake/include/soc/meminit_jsl.h b/src/soc/intel/tigerlake/include/soc/meminit_jsl.h new file mode 100644 index 0000000..588ad5c --- /dev/null +++ b/src/soc/intel/tigerlake/include/soc/meminit_jsl.h @@ -0,0 +1,119 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2020 Intel Corporation. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef _SOC_JASPERLAKE_MEMCFG_INIT_H_ +#define _SOC_JASPERLAKE_MEMCFG_INIT_H_ + +#include <stddef.h> +#include <stdint.h> +#include <fsp/soc_binding.h> + +/* Number of dq bits controlled per dqs */ +#define DQ_BITS_PER_DQS 8 + +/* Number of memory packages, where a "package" represents a 64-bit solution */ +#define DDR_NUM_PACKAGES 2 + +/* Number of DQ byte mappings */ +#define DDR_NUM_BYTE_MAPPINGS 6 + +/* 64-bit Channel identification */ +enum { + DDR_CH0, + DDR_CH1, + DDR_NUM_CHANNELS +}; + +struct spd_by_pointer { + size_t spd_data_len; + uintptr_t spd_data_ptr; +}; + +enum mem_info_read_type { + READ_SPD_CBFS, /* Find spd file in CBFS. */ + READ_SPD_MEMPTR /* Find spd data from pointer. */ +}; + +struct spd_info { + enum mem_info_read_type read_type; + union spd_data_by { + /* To identify spd file when read_type is READ_SPD_CBFS. */ + int spd_index; + + /* To find spd data when read_type is READ_SPD_MEMPTR. */ + struct spd_by_pointer spd_data_ptr_info; + } spd_spec; +}; + +/* Board-specific memory dq mapping information */ +struct mb_cfg { + + /* + * For each channel, there are 6 sets of DQ byte mappings, + * where each set has a package 0 and a package 1 value (package 0 + * represents the first 64-bit lpddr4 chip combination, and package 1 + * represents the second 64-bit lpddr4 chip combination). + * The first three sets are for CLK, CMD, and CTL. + * The fsp package actually expects 6 sets, even though the last 3 sets + * are not used in JSL. + * We let the meminit_dq_dqs_map routine take care of clearing the + * unused fields for the caller. + * Note that dq_map is only used by LPDDR; it does not need to be + * initialized for designs using DDR4. + */ + uint8_t dq_map[DDR_NUM_CHANNELS][DDR_NUM_BYTE_MAPPINGS][DDR_NUM_PACKAGES]; + + /* + * DQS CPU<>DRAM map Ch0 and Ch1. Each array entry represents a + * mapping of a dq bit on the CPU to the bit it's connected to on + * the memory part. The array index represents the dqs bit number + * on the memory part, and the values in the array represent which + * pin on the CPU that DRAM pin connects to. + * dqs_map is only used by LPDDR; same comments apply as for dq_map + * above. + */ + 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. + */ + uint16_t rcomp_resistor[3]; + + /* + * Rcomp target values. These will typically be the following + * values for Jasper Lake : { 80, 40, 40, 40, 30 } + */ + uint16_t rcomp_targets[5]; + + /* + * Early Command Training Enable/Disable Control + * 1 = enable, 0 = disable + */ + uint8_t ect; + + /* Board type */ + uint8_t UserBd; +}; + +/* + * Initialize default memory configurations for Jasper Lake. + */ + +void memcfg_init(FSP_M_CONFIG *mem_cfg, const struct mb_cfg *board_cfg, + const struct spd_info *spd_info, bool half_populated); + +#endif /* _SOC_JASPERLAKE_MEMCFG_INIT_H_ */ diff --git a/src/soc/intel/tigerlake/meminit_jsl.c b/src/soc/intel/tigerlake/meminit_jsl.c new file mode 100644 index 0000000..f977ce2 --- /dev/null +++ b/src/soc/intel/tigerlake/meminit_jsl.c @@ -0,0 +1,120 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2020 Intel Corporation. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <assert.h> +#include <console/console.h> +#include <fsp/util.h> +#include <soc/meminit_jsl.h> +#include <spd_bin.h> +#include <string.h> + +static void spd_read_from_cbfs(const struct spd_info *spd_info, uintptr_t *spd_data_ptr, + size_t *spd_data_len) +{ + struct region_device spd_rdev; + size_t spd_index = spd_info->spd_spec.spd_index; + + printk(BIOS_DEBUG, "SPD INDEX = %lu\n", spd_index); + if (get_spd_cbfs_rdev(&spd_rdev, spd_index) < 0) + die("spd.bin not found or incorrect index\n"); + + *spd_data_len = region_device_sz(&spd_rdev); + + /* Memory leak is ok since we have memory mapped boot media */ + assert(CONFIG(BOOT_DEVICE_MEMORY_MAPPED)); + + *spd_data_ptr = (uintptr_t)rdev_mmap_full(&spd_rdev); +} + +static void get_spd_data(const struct spd_info *spd_info, uintptr_t *spd_data_ptr, + size_t *spd_data_len) +{ + if (spd_info->read_type == READ_SPD_MEMPTR) { + *spd_data_ptr = spd_info->spd_spec.spd_data_ptr_info.spd_data_ptr; + *spd_data_len = spd_info->spd_spec.spd_data_ptr_info.spd_data_len; + return; + } + + if (spd_info->read_type == READ_SPD_CBFS) { + spd_read_from_cbfs(spd_info, spd_data_ptr, spd_data_len); + return; + } + + die("no valid way to read SPD info"); +} + +static void meminit_dq_dqs_map(FSP_M_CONFIG *mem_cfg, const struct mb_cfg *board_cfg, + bool half_populated) +{ + memcpy(&mem_cfg->RcompResistor, &board_cfg->rcomp_resistor, + sizeof(mem_cfg->RcompResistor)); + + memcpy(&mem_cfg->RcompTarget, &board_cfg->rcomp_targets, + sizeof(mem_cfg->RcompTarget)); + + memcpy(&mem_cfg->DqByteMapCh0, &board_cfg->dq_map[DDR_CH0], + sizeof(board_cfg->dq_map[DDR_CH0])); + + memcpy(&mem_cfg->DqsMapCpu2DramCh0, &board_cfg->dqs_map[DDR_CH0], + sizeof(board_cfg->dqs_map[DDR_CH0])); + + if (half_populated) + return; + + memcpy(&mem_cfg->DqByteMapCh1, &board_cfg->dq_map[DDR_CH1], + sizeof(board_cfg->dq_map[DDR_CH1])); + + memcpy(&mem_cfg->DqsMapCpu2DramCh1, &board_cfg->dqs_map[DDR_CH1], + sizeof(board_cfg->dqs_map[DDR_CH1])); +} + +static void meminit_channels(FSP_M_CONFIG *mem_cfg, const struct mb_cfg *board_cfg, + uintptr_t spd_data_ptr, bool half_populated) +{ + /* Channel 0 */ + mem_cfg->MemorySpdPtr00 = spd_data_ptr; + mem_cfg->MemorySpdPtr01 = 0; + + if (half_populated) { + printk(BIOS_INFO, "%s: DRAM half-populated\n", __func__); + spd_data_ptr = 0; + } + + /* Channel 1 */ + mem_cfg->MemorySpdPtr10 = spd_data_ptr; + mem_cfg->MemorySpdPtr11 = 0; + + meminit_dq_dqs_map(mem_cfg, board_cfg, half_populated); +} + +/* Initialize onboard memory configurations for lpddr4x */ +void memcfg_init(FSP_M_CONFIG *mem_cfg, const struct mb_cfg *board_cfg, + const struct spd_info *spd_info, bool half_populated) +{ + size_t spd_data_len; + uintptr_t spd_data_ptr; + + memset(&mem_cfg->SpdAddressTable, 0, sizeof(mem_cfg->SpdAddressTable)); + get_spd_data(spd_info, &spd_data_ptr, &spd_data_len); + print_spd_info((unsigned char *)spd_data_ptr); + + mem_cfg->MemorySpdDataLen = spd_data_len; + meminit_channels(mem_cfg, board_cfg, spd_data_ptr, half_populated); + + /* Early Command Training Enabled */ + mem_cfg->ECT = board_cfg->ect; + + mem_cfg->UserBd = board_cfg->UserBd; +}
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 26:
Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3 Emulation targets: EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/1047 EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1046 EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1045
Please note: This test is under development and might not be accurate at all!
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 26:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39135/26/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_jsl.h:
https://review.coreboot.org/c/coreboot/+/39135/26/src/soc/intel/tigerlake/in... PS26, Line 76: DDR_NUM_BYTE_MAPPINGS Just a note for future clean up: Since FSP expects only first 3 sets to be valid, I think we should ask mainboard to only provide those and memset last 3 sets to 0.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 26:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39135/26/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_jsl.h:
https://review.coreboot.org/c/coreboot/+/39135/26/src/soc/intel/tigerlake/in... PS26, Line 76: DDR_NUM_BYTE_MAPPINGS
Just a note for future clean up: Since FSP expects only first 3 sets to be valid, I think we should […]
yes make sense for further clean up
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 26:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39135/26/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_jsl.h:
https://review.coreboot.org/c/coreboot/+/39135/26/src/soc/intel/tigerlake/in... PS26, Line 76: DDR_NUM_BYTE_MAPPINGS
yes make sense for further clean up
Ack
-
9elements QA (Code Review) -
Aamir Bohra (Code Review)
-
build bot (Jenkins) (Code Review)
-
Furquan Shaikh (Code Review)
-
Karthik Ramasubramanian (Code Review)
-
Maulik V Vaghela (Code Review)
-
Meera Ravindranath (Code Review)
-
Patrick Georgi (Code Review)
-
Ronak Kanabar (Code Review)
-
Subrata Banik (Code Review)
-
Tim Wawrzynczak (Code Review)
-
V Sowmya (Code Review)