Bernardo Perez Priego has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37628 )
Change subject: soc/intel/common: Add romstage common stage file ......................................................................
soc/intel/common: Add romstage common stage file
This patch will ensures all intel soc is using common stage files to make coreboot design flow align across all socs.
CPU, SA, PCH, MCH programming sequence might be different between socs but the function call should route from same location across all soc.
Signed-off-by: Bernardo Perez Priego bernardo.perez.priego@intel.com Change-Id: I06d43ac29f5e87ce731a470e5e145adea07ece4c --- M src/cpu/intel/car/romstage.c A src/soc/intel/common/basecode/include/intelbasecode/romstage.h A src/soc/intel/common/basecode/romstage/Makefile.inc A src/soc/intel/common/basecode/romstage/romstage.c 4 files changed, 162 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/37628/1
diff --git a/src/cpu/intel/car/romstage.c b/src/cpu/intel/car/romstage.c index 1f8eb9a..55d3c9d 100644 --- a/src/cpu/intel/car/romstage.c +++ b/src/cpu/intel/car/romstage.c @@ -52,9 +52,6 @@ for (i = 0; i < num_guards; i++) stack_base[i] = stack_guard;
- if (CONFIG(VBOOT_EARLY_EC_SYNC)) - vboot_sync_ec(); - mainboard_romstage_entry();
/* Check the stack. */ @@ -64,9 +61,6 @@ printk(BIOS_DEBUG, "Smashed stack detected in romstage!\n"); }
- if (CONFIG(SMM_TSEG)) - smm_list_regions(); - prepare_and_run_postcar(&early_mtrrs); /* We do not return here. */ } diff --git a/src/soc/intel/common/basecode/include/intelbasecode/romstage.h b/src/soc/intel/common/basecode/include/intelbasecode/romstage.h new file mode 100644 index 0000000..0cebbbf --- /dev/null +++ b/src/soc/intel/common/basecode/include/intelbasecode/romstage.h @@ -0,0 +1,37 @@ +/* + * This file is part of the coreboot project. + * + * Copyright 2019 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_INTEL_COMMON_BASECODE_ROMSTAGE_H +#define SOC_INTEL_COMMON_BASECODE_ROMSTAGE_H + +#include <stddef.h> +#include <stdint.h> +#include <fsp/soc_binding.h> + +struct romstage_ops { + void (*soc_early_init)(void); + void (*soc_init)(void); + void (*pch_early_init)(void); + void (*pch_init)(void); + void (*cpu_early_init)(void); + void (*cpu_init)(void); + bool (*is_s3wake)(void); + void (*soc_mem_init_params)(FSP_M_CONFIG *mupd); +}; + +/* SoC Override function */ +struct romstage_ops *soc_get_ops(void); + +#endif diff --git a/src/soc/intel/common/basecode/romstage/Makefile.inc b/src/soc/intel/common/basecode/romstage/Makefile.inc new file mode 100644 index 0000000..29763fb --- /dev/null +++ b/src/soc/intel/common/basecode/romstage/Makefile.inc @@ -0,0 +1 @@ +romstage-y += romstage.c diff --git a/src/soc/intel/common/basecode/romstage/romstage.c b/src/soc/intel/common/basecode/romstage/romstage.c new file mode 100644 index 0000000..55c4ab5 --- /dev/null +++ b/src/soc/intel/common/basecode/romstage/romstage.c @@ -0,0 +1,124 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2019 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 <arch/early_variables.h> +#include <romstage_common.h> +#include <cpu/x86/pae.h> +#include <intelbasecode/romstage.h> +#include <intelblocks/systemagent.h> +#include <soc/iomap.h> +#include <soc/romstage.h> + +static const struct romstage_ops s_romstage_ops; + +__weak struct romstage_ops *soc_get_ops(void) +{ + return (struct romstage_ops *)&s_romstage_ops; +} + +void romstage_early_init(void) +{ + struct romstage_ops *rs_ops = soc_get_ops(); + + rs_ops->soc_early_init(); + rs_ops->pch_early_init(); + rs_ops->cpu_early_init(); +} + +void romstage_init(void) +{ + struct romstage_ops *rs_ops = soc_get_ops(); + + rs_ops->soc_init(); + rs_ops->pch_init(); + rs_ops->cpu_init(); +} + +void romstage_cmn_soc_early_init(void) +{ + systemagent_early_init(); + heci_init(HECI1_BASE_ADDRESS); +} + +void romstage_cmn_soc_init(void) +{ +} + +void romstage_cmn_pch_early_init(void) +{ +} + +void romstage_cmn_cpu_early_init(void) +{ +} + +void romstage_cmn_pch_init(void) +{ +} + +void romstage_cmn_cpu_init(void) +{ +} + +bool romstage_cmn_is_s3wake(void) +{ + struct chipset_power_state *ps = pmc_get_power_state(); + return pmc_fill_power_state(ps) == ACPI_S3; +} + +void romstage_cmn_soc_mem_init_param(FSP_M_CONFIG *m_cfg) +{ +} + +static const struct romstage_ops s_romstage_ops = { + &romstage_cmn_soc_early_init, + &romstage_cmn_soc_init, + &romstage_cmn_pch_early_init, + &romstage_cmn_pch_init, + &romstage_cmn_cpu_early_init, + &romstage_cmn_cpu_init, + &romstage_cmn_is_s3wake, + &romstage_cmn_mb_mem_init_param, + &romstage_cmn_soc_mem_init_param +}; + +/* + Main romstage function +*/ +asmlinkage void mainboard_romstage_entry(void) +{ + if (CONFIG(VBOOT_EARLY_EC_SYNC)) + vboot_sync_ec(rs_ops->fill_power_state()); + + romstage_early_init(); + romstage_init(); + fsp_memory_init(rs_ops->is_s3wake()); + + if (CONFIG(SMM_TSEG)) + smm_list_regions(); +} + +/* + Callback function for FSP memory initialization +*/ +void platform_fsp_memory_init_params_cb(FSPM_UPD *mupd, uint32_t version) +{ + struct romstage_ops *rs_ops = soc_get_ops(); + + FSP_M_CONFIG *m_cfg; + rs_ops->soc_mem_init_params(m_cfg); + mainboard_memory_init_params(m_cfg); +} +
Bernardo Perez Priego has removed Patrick Georgi from this change. ( https://review.coreboot.org/c/coreboot/+/37628 )
Change subject: soc/intel/common: Add romstage common stage file ......................................................................
Removed reviewer Patrick Georgi.
Bernardo Perez Priego has removed Martin Roth from this change. ( https://review.coreboot.org/c/coreboot/+/37628 )
Change subject: soc/intel/common: Add romstage common stage file ......................................................................
Removed reviewer Martin Roth.
Bernardo Perez Priego has removed Patrick Rudolph from this change. ( https://review.coreboot.org/c/coreboot/+/37628 )
Change subject: soc/intel/common: Add romstage common stage file ......................................................................
Removed reviewer Patrick Rudolph.
Hello Bora Guvendik, build bot (Jenkins), Patrick Georgi, Martin Roth, Selma Bensaid, Subrata Banik, Varun Joshi, Usha P, Aamir Bohra, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37628
to look at the new patch set (#18).
Change subject: soc/intel/common: Add romstage common stage file ......................................................................
soc/intel/common: Add romstage common stage file
This patch will ensures all intel soc is using common stage files to make coreboot design flow align across all socs.
CPU, SA, PCH, MCH programming sequence might be different between socs but the function call should route from same location across all soc.
Signed-off-by: Bernardo Perez Priego bernardo.perez.priego@intel.com Change-Id: I06d43ac29f5e87ce731a470e5e145adea07ece4c --- M src/soc/intel/common/Kconfig.common A src/soc/intel/common/basecode/include/intelbasecode/romstage.h A src/soc/intel/common/basecode/romstage/Kconfig A src/soc/intel/common/basecode/romstage/Makefile.inc A src/soc/intel/common/basecode/romstage/romstage.c 5 files changed, 297 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/37628/18
Hello Bora Guvendik, build bot (Jenkins), Patrick Georgi, Martin Roth, Selma Bensaid, Subrata Banik, Varun Joshi, Usha P, Aamir Bohra, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37628
to look at the new patch set (#19).
Change subject: soc/intel/common: Add romstage common stage file ......................................................................
soc/intel/common: Add romstage common stage file
This patch will ensures all intel soc is using common stage files to make coreboot design flow align across all socs.
CPU, SA, PCH, MCH programming sequence might be different between socs but the function call should route from same location across all soc.
Signed-off-by: Bernardo Perez Priego bernardo.perez.priego@intel.com Change-Id: I06d43ac29f5e87ce731a470e5e145adea07ece4c --- M src/soc/intel/common/Kconfig.common A src/soc/intel/common/basecode/include/intelbasecode/romstage.h A src/soc/intel/common/basecode/romstage/Kconfig A src/soc/intel/common/basecode/romstage/Makefile.inc A src/soc/intel/common/basecode/romstage/romstage.c 5 files changed, 302 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/37628/19
Hello Bora Guvendik, build bot (Jenkins), Patrick Georgi, Martin Roth, Selma Bensaid, Subrata Banik, Varun Joshi, Usha P, Aamir Bohra, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37628
to look at the new patch set (#20).
Change subject: soc/intel/common: Add romstage common stage file ......................................................................
soc/intel/common: Add romstage common stage file
This patch will ensures all intel soc is using common stage files to make coreboot design flow align across all socs.
CPU, SA, PCH, MCH programming sequence might be different between socs but the function call should route from same location across all soc.
Signed-off-by: Bernardo Perez Priego bernardo.perez.priego@intel.com Change-Id: I06d43ac29f5e87ce731a470e5e145adea07ece4c --- M src/soc/intel/common/Kconfig.common A src/soc/intel/common/basecode/include/intelbasecode/romstage.h A src/soc/intel/common/basecode/romstage/Kconfig A src/soc/intel/common/basecode/romstage/Makefile.inc A src/soc/intel/common/basecode/romstage/romstage.c 5 files changed, 303 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/37628/20
Attention is currently required from: Furquan Shaikh, Tim Wawrzynczak, Angel Pons, Michael Niewöhner, Bernardo Perez Priego, Aaron Durbin. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37628 )
Change subject: soc/intel/common: Add romstage common stage file ......................................................................
Patch Set 20:
(3 comments)
Patchset:
PS20: Hi Bernardo, I think this is a nice idea. There is a lot of of unnecessary divergence in the individual SoC code.
We need to be very careful with the design, though. We have to avoid the introduction of any patterns that could make future maintenance harder.
I've looked ahead into the next commit and find it quite hard to assess the impact of the common stage file because there's a lot of not directly related cleanup within the patches. It would be nice if any unrelated refactoring could be done in advance. Only when the changes directly related to the common stage file are clearly visible, we can discuss its merits.
File src/soc/intel/common/basecode/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/37628/comment/20f23eef_9167c977 PS20, Line 44: /* Save the DIMM information for SMBIOS table 17 */ : static void save_dimm_info(void) : { : int node, channel, dimm, dimm_max, index; : size_t hob_size; : uint8_t ddr_type; : const CONTROLLER_INFO *ctrlr_info; : const CHANNEL_INFO *channel_info; : const DIMM_INFO *src_dimm; : struct dimm_info *dest_dimm; : struct memory_info *mem_info; : const MEMORY_INFO_DATA_HOB *meminfo_hob; : const uint8_t smbios_memory_info_guid[16] = : FSP_SMBIOS_MEMORY_INFO_GUID; : const uint8_t *serial_num; : const char *dram_part_num = NULL; : size_t dram_part_num_len = 0; : bool is_dram_part_overridden = false; : : /* Locate the memory info HOB, presence validated by raminit */ : meminfo_hob = fsp_find_extension_hob_by_guid( : smbios_memory_info_guid, : &hob_size); : if (meminfo_hob == NULL || hob_size == 0) { : printk(BIOS_ERR, "SMBIOS MEMORY_INFO_DATA_HOB not found\n"); : return; : } : : /* : * Allocate CBMEM area for DIMM information used to populate SMBIOS : * table 17 : */ : mem_info = cbmem_add(CBMEM_ID_MEMINFO, sizeof(*mem_info)); : if (mem_info == NULL) { : printk(BIOS_ERR, "CBMEM entry for DIMM info missing\n"); : return; : } : memset(mem_info, 0, sizeof(*mem_info)); : : /* Allow mainboard to override DRAM part number. */ : dram_part_num = mainboard_get_dram_part_num(); : if (dram_part_num) { : dram_part_num_len = strlen(dram_part_num); : is_dram_part_overridden = true; : } : : /* Save available DIMM information */ : index = 0; : dimm_max = ARRAY_SIZE(mem_info->dimm); : for (node = 0; node < MAX_NODE; node++) { : ctrlr_info = &meminfo_hob->Controller[node]; : for (channel = 0; channel < MAX_CH && index < dimm_max; : channel++) { : channel_info = &ctrlr_info->ChannelInfo[channel]; : if (channel_info->Status != CHANNEL_PRESENT) : continue; : : for (dimm = 0; dimm < MAX_DIMM && index < dimm_max; : dimm++) { : src_dimm = &channel_info->DimmInfo[dimm]; : dest_dimm = &mem_info->dimm[index]; : if (src_dimm->Status != DIMM_PRESENT) : continue; : : switch (meminfo_hob->MemoryType) { : case MRC_DDR_TYPE_DDR4: : ddr_type = MEMORY_TYPE_DDR4; : break; : case MRC_DDR_TYPE_DDR3: : ddr_type = MEMORY_TYPE_DDR3; : break; : case MRC_DDR_TYPE_LPDDR3: : ddr_type = MEMORY_TYPE_LPDDR3; : break; : default: : ddr_type = meminfo_hob->MemoryType; : break; : } : /* If there is no DRAM part number overridden by : * mainboard then use original one. */ : if (!is_dram_part_overridden) { : dram_part_num_len = sizeof(src_dimm->ModulePartNum); : dram_part_num = (const char *) : &src_dimm->ModulePartNum[0]; : } : : u8 memProfNum = meminfo_hob->MemoryProfile; : serial_num = src_dimm->SpdSave + : SPD_SAVE_OFFSET_SERIAL; : : /* Populate the DIMM information */ : dimm_info_fill(dest_dimm, : src_dimm->DimmCapacity, : ddr_type, : meminfo_hob->ConfiguredMemoryClockSpeed, : src_dimm->RankInDimm, : channel_info->ChannelId, : src_dimm->DimmId, : dram_part_num, : dram_part_num_len, : serial_num, : meminfo_hob->DataWidth, : meminfo_hob->VddVoltage[memProfNum], : meminfo_hob->EccSupport, : src_dimm->MfgId, : src_dimm->SpdModuleType); : index++; : } : } : } : mem_info->dimm_cnt = index; : printk(BIOS_DEBUG, "%d DIMMs found\n", mem_info->dimm_cnt); : } Please do this in a separate commit. If it is done ahead, it would make it much easier to assess the impact of the actual topic, the common romstage file. It might be a good idea to put this into its own .c file.
Also, it seems FSP specific. If it is FSP but not chipset specific, it probably belongs into `drivers/intel/fsp*`.
https://review.coreboot.org/c/coreboot/+/37628/comment/7e6fd066_862903f7 PS20, Line 178: romstage_cmn_pch_init(); Non-empty weak functions are generally discouraged.
Attention is currently required from: Furquan Shaikh, Tim Wawrzynczak, Angel Pons, Michael Niewöhner, Bernardo Perez Priego, Aaron Durbin. Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37628 )
Change subject: soc/intel/common: Add romstage common stage file ......................................................................
Patch Set 20:
(1 comment)
File src/soc/intel/common/basecode/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/37628/comment/11dd8d3a_d97828a9 PS18, Line 174: romstage_cmn_pch_init this will be impossible to debug. Either make all __weak function empty or remove the __weak and make it mandatory to implement this function.
Attention is currently required from: Nico Huber, Furquan Shaikh, Tim Wawrzynczak, Angel Pons, Bernardo Perez Priego, Aaron Durbin. Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37628 )
Change subject: soc/intel/common: Add romstage common stage file ......................................................................
Patch Set 20:
(14 comments)
File src/soc/intel/common/basecode/romstage/Kconfig:
https://review.coreboot.org/c/coreboot/+/37628/comment/a748297a_f7846a19 PS20, Line 5: for all when it's "for all", why have a Kconfig for it?
https://review.coreboot.org/c/coreboot/+/37628/comment/2bb0763d_3ec117ef PS20, Line 5: Platform platforms
File src/soc/intel/common/basecode/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/37628/comment/71b4b3bd_97091698 PS20, Line 3: e <acpi/acpi.h> : #include <arch/romstage.h> : #include <intelbasecode/romstage.h> : #include <console/console.h> : #include <intelblocks/cse.h> : #include <intelblocks/pmclib.h> : #include <intelblocks/smbus.h> : #include <soc/iomap.h> : #include <soc/romstage.h> : #include <fsp/util.h> : #include <cbmem.h> : #include <memory_info.h> : #include <soc/intel/common/smbios.h> : #include <soc/pm.h> nit: order
https://review.coreboot.org/c/coreboot/+/37628/comment/b1dfedb7_229b9ecf PS20, Line 18: nit: drop newline
https://review.coreboot.org/c/coreboot/+/37628/comment/49cc45ba_22655a4b PS20, Line 23: } nit: newline
https://review.coreboot.org/c/coreboot/+/37628/comment/2ca593f1_14543d18 PS20, Line 24: /* : Common functions implementation : */ nit: does this comment provide any value? I'd drop it
https://review.coreboot.org/c/coreboot/+/37628/comment/a330b10e_c1aac930 PS20, Line 27: cmn I stumbled upon this and only got what it means when looking at the path again, which won't help when looking at the path where functions may be used. Why not write it out?
https://review.coreboot.org/c/coreboot/+/37628/comment/09663d14_c5154d3c PS20, Line 44: /* Save the DIMM information for SMBIOS table 17 */ : static void save_dimm_info(void) : { : int node, channel, dimm, dimm_max, index; : size_t hob_size; : uint8_t ddr_type; : const CONTROLLER_INFO *ctrlr_info; : const CHANNEL_INFO *channel_info; : const DIMM_INFO *src_dimm; : struct dimm_info *dest_dimm; : struct memory_info *mem_info; : const MEMORY_INFO_DATA_HOB *meminfo_hob; : const uint8_t smbios_memory_info_guid[16] = : FSP_SMBIOS_MEMORY_INFO_GUID; : const uint8_t *serial_num; : const char *dram_part_num = NULL; : size_t dram_part_num_len = 0; : bool is_dram_part_overridden = false; : : /* Locate the memory info HOB, presence validated by raminit */ : meminfo_hob = fsp_find_extension_hob_by_guid( : smbios_memory_info_guid, : &hob_size); : if (meminfo_hob == NULL || hob_size == 0) { : printk(BIOS_ERR, "SMBIOS MEMORY_INFO_DATA_HOB not found\n"); : return; : } : : /* : * Allocate CBMEM area for DIMM information used to populate SMBIOS : * table 17 : */ : mem_info = cbmem_add(CBMEM_ID_MEMINFO, sizeof(*mem_info)); : if (mem_info == NULL) { : printk(BIOS_ERR, "CBMEM entry for DIMM info missing\n"); : return; : } : memset(mem_info, 0, sizeof(*mem_info)); : : /* Allow mainboard to override DRAM part number. */ : dram_part_num = mainboard_get_dram_part_num(); : if (dram_part_num) { : dram_part_num_len = strlen(dram_part_num); : is_dram_part_overridden = true; : } : : /* Save available DIMM information */ : index = 0; : dimm_max = ARRAY_SIZE(mem_info->dimm); : for (node = 0; node < MAX_NODE; node++) { : ctrlr_info = &meminfo_hob->Controller[node]; : for (channel = 0; channel < MAX_CH && index < dimm_max; : channel++) { : channel_info = &ctrlr_info->ChannelInfo[channel]; : if (channel_info->Status != CHANNEL_PRESENT) : continue; : : for (dimm = 0; dimm < MAX_DIMM && index < dimm_max; : dimm++) { : src_dimm = &channel_info->DimmInfo[dimm]; : dest_dimm = &mem_info->dimm[index]; : if (src_dimm->Status != DIMM_PRESENT) : continue; : : switch (meminfo_hob->MemoryType) { : case MRC_DDR_TYPE_DDR4: : ddr_type = MEMORY_TYPE_DDR4; : break; : case MRC_DDR_TYPE_DDR3: : ddr_type = MEMORY_TYPE_DDR3; : break; : case MRC_DDR_TYPE_LPDDR3: : ddr_type = MEMORY_TYPE_LPDDR3; : break; : default: : ddr_type = meminfo_hob->MemoryType; : break; : } : /* If there is no DRAM part number overridden by : * mainboard then use original one. */ : if (!is_dram_part_overridden) { : dram_part_num_len = sizeof(src_dimm->ModulePartNum); : dram_part_num = (const char *) : &src_dimm->ModulePartNum[0]; : } : : u8 memProfNum = meminfo_hob->MemoryProfile; : serial_num = src_dimm->SpdSave + : SPD_SAVE_OFFSET_SERIAL; : : /* Populate the DIMM information */ : dimm_info_fill(dest_dimm, : src_dimm->DimmCapacity, : ddr_type, : meminfo_hob->ConfiguredMemoryClockSpeed, : src_dimm->RankInDimm, : channel_info->ChannelId, : src_dimm->DimmId, : dram_part_num, : dram_part_num_len, : serial_num, : meminfo_hob->DataWidth, : meminfo_hob->VddVoltage[memProfNum], : meminfo_hob->EccSupport, : src_dimm->MfgId, : src_dimm->SpdModuleType); : index++; : } : } : } : mem_info->dimm_cnt = index; : printk(BIOS_DEBUG, "%d DIMMs found\n", mem_info->dimm_cnt); : }
Please do this in a separate commit. If it is done ahead, it would make […]
Yup, looks FSP2 specific to me on a first look. If the code matches for all FSP2.* platforms, I'd move that to `drivers/intel/fsp2_0/memory_init.c`.
https://review.coreboot.org/c/coreboot/+/37628/comment/91627063_087db3bf PS20, Line 169: /* : Weak Functions implementation : */ not sure if that's valid coreboot comment style; also add a newline b/c it's for a whole section, not the function
https://review.coreboot.org/c/coreboot/+/37628/comment/b4de09c6_b814d3ce PS20, Line 209: /* : Initialization functions : */ nit: same as above
https://review.coreboot.org/c/coreboot/+/37628/comment/868628d4_6d02e2fa PS20, Line 227: : /* : Main romstage function : */ nit: _entry already provides that information :)
https://review.coreboot.org/c/coreboot/+/37628/comment/5afa9123_66b32aa0 PS20, Line 235: romstage_init(); : : fsp_memory_init(s3wake); : : romstage_post_mem_init(); nit: why all these newlines?
https://review.coreboot.org/c/coreboot/+/37628/comment/8caff23a_a8e6fa5b PS20, Line 242: /* : Callback function for FSP memory initialization : */ nit: function name already provides that information
https://review.coreboot.org/c/coreboot/+/37628/comment/89b50c01_b37457df PS20, Line 248: nit: I'd drop that newline
Attention is currently required from: Nico Huber, Furquan Shaikh, Tim Wawrzynczak, Angel Pons, Michael Niewöhner, Aaron Durbin. Bernardo Perez Priego has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37628 )
Change subject: soc/intel/common: Add romstage common stage file ......................................................................
Patch Set 21:
(6 comments)
Patchset:
PS20:
Hi Bernardo, I think this is a nice idea. There is a lot of […]
Thanks for the feedback I will address this in upcoming commits.
File src/soc/intel/common/basecode/romstage/Kconfig:
https://review.coreboot.org/c/coreboot/+/37628/comment/14277298_8e9e97f4 PS20, Line 5: Platform
platforms
Ack
https://review.coreboot.org/c/coreboot/+/37628/comment/4607d7e8_bc2b9001 PS20, Line 5: for all
when it's "for all", why have a Kconfig for it?
Ack
File src/soc/intel/common/basecode/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/37628/comment/5fdf1d09_9d4389c5 PS20, Line 27: cmn
I stumbled upon this and only got what it means when looking at the path again, which won't help whe […]
Ack
https://review.coreboot.org/c/coreboot/+/37628/comment/ef9cfe36_20ac6d89 PS20, Line 44: /* Save the DIMM information for SMBIOS table 17 */ : static void save_dimm_info(void) : { : int node, channel, dimm, dimm_max, index; : size_t hob_size; : uint8_t ddr_type; : const CONTROLLER_INFO *ctrlr_info; : const CHANNEL_INFO *channel_info; : const DIMM_INFO *src_dimm; : struct dimm_info *dest_dimm; : struct memory_info *mem_info; : const MEMORY_INFO_DATA_HOB *meminfo_hob; : const uint8_t smbios_memory_info_guid[16] = : FSP_SMBIOS_MEMORY_INFO_GUID; : const uint8_t *serial_num; : const char *dram_part_num = NULL; : size_t dram_part_num_len = 0; : bool is_dram_part_overridden = false; : : /* Locate the memory info HOB, presence validated by raminit */ : meminfo_hob = fsp_find_extension_hob_by_guid( : smbios_memory_info_guid, : &hob_size); : if (meminfo_hob == NULL || hob_size == 0) { : printk(BIOS_ERR, "SMBIOS MEMORY_INFO_DATA_HOB not found\n"); : return; : } : : /* : * Allocate CBMEM area for DIMM information used to populate SMBIOS : * table 17 : */ : mem_info = cbmem_add(CBMEM_ID_MEMINFO, sizeof(*mem_info)); : if (mem_info == NULL) { : printk(BIOS_ERR, "CBMEM entry for DIMM info missing\n"); : return; : } : memset(mem_info, 0, sizeof(*mem_info)); : : /* Allow mainboard to override DRAM part number. */ : dram_part_num = mainboard_get_dram_part_num(); : if (dram_part_num) { : dram_part_num_len = strlen(dram_part_num); : is_dram_part_overridden = true; : } : : /* Save available DIMM information */ : index = 0; : dimm_max = ARRAY_SIZE(mem_info->dimm); : for (node = 0; node < MAX_NODE; node++) { : ctrlr_info = &meminfo_hob->Controller[node]; : for (channel = 0; channel < MAX_CH && index < dimm_max; : channel++) { : channel_info = &ctrlr_info->ChannelInfo[channel]; : if (channel_info->Status != CHANNEL_PRESENT) : continue; : : for (dimm = 0; dimm < MAX_DIMM && index < dimm_max; : dimm++) { : src_dimm = &channel_info->DimmInfo[dimm]; : dest_dimm = &mem_info->dimm[index]; : if (src_dimm->Status != DIMM_PRESENT) : continue; : : switch (meminfo_hob->MemoryType) { : case MRC_DDR_TYPE_DDR4: : ddr_type = MEMORY_TYPE_DDR4; : break; : case MRC_DDR_TYPE_DDR3: : ddr_type = MEMORY_TYPE_DDR3; : break; : case MRC_DDR_TYPE_LPDDR3: : ddr_type = MEMORY_TYPE_LPDDR3; : break; : default: : ddr_type = meminfo_hob->MemoryType; : break; : } : /* If there is no DRAM part number overridden by : * mainboard then use original one. */ : if (!is_dram_part_overridden) { : dram_part_num_len = sizeof(src_dimm->ModulePartNum); : dram_part_num = (const char *) : &src_dimm->ModulePartNum[0]; : } : : u8 memProfNum = meminfo_hob->MemoryProfile; : serial_num = src_dimm->SpdSave + : SPD_SAVE_OFFSET_SERIAL; : : /* Populate the DIMM information */ : dimm_info_fill(dest_dimm, : src_dimm->DimmCapacity, : ddr_type, : meminfo_hob->ConfiguredMemoryClockSpeed, : src_dimm->RankInDimm, : channel_info->ChannelId, : src_dimm->DimmId, : dram_part_num, : dram_part_num_len, : serial_num, : meminfo_hob->DataWidth, : meminfo_hob->VddVoltage[memProfNum], : meminfo_hob->EccSupport, : src_dimm->MfgId, : src_dimm->SpdModuleType); : index++; : } : } : } : mem_info->dimm_cnt = index; : printk(BIOS_DEBUG, "%d DIMMs found\n", mem_info->dimm_cnt); : }
Yup, looks FSP2 specific to me on a first look. If the code matches for all FSP2. […]
Thanks for the feedback I will address this in upcoming commits.
https://review.coreboot.org/c/coreboot/+/37628/comment/b5a4ff47_84545607 PS20, Line 169: /* : Weak Functions implementation : */
not sure if that's valid coreboot comment style; also add a newline b/c it's for a whole section, no […]
Ack
Attention is currently required from: Nico Huber, Furquan Shaikh, Patrick Rudolph, Tim Wawrzynczak, Angel Pons, Michael Niewöhner, Aaron Durbin. Bernardo Perez Priego has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37628 )
Change subject: soc/intel/common: Add romstage common stage file ......................................................................
Patch Set 21:
(1 comment)
File src/soc/intel/common/basecode/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/37628/comment/6ddcd848_75c1c1a3 PS18, Line 174: romstage_cmn_pch_init
this will be impossible to debug. […]
Ack
Attention is currently required from: Nico Huber, Furquan Shaikh, Patrick Rudolph, Tim Wawrzynczak, Angel Pons, Michael Niewöhner, Aaron Durbin. Bernardo Perez Priego has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37628 )
Change subject: soc/intel/common: Add romstage common stage file ......................................................................
Patch Set 21:
(2 comments)
Patchset:
PS21: Thanks for your feedback, please also refer to design document: https://review.coreboot.org/c/coreboot/+/51260
File src/soc/intel/common/basecode/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/37628/comment/c8ff328f_6ae829f9 PS20, Line 44: /* Save the DIMM information for SMBIOS table 17 */ : static void save_dimm_info(void) : { : int node, channel, dimm, dimm_max, index; : size_t hob_size; : uint8_t ddr_type; : const CONTROLLER_INFO *ctrlr_info; : const CHANNEL_INFO *channel_info; : const DIMM_INFO *src_dimm; : struct dimm_info *dest_dimm; : struct memory_info *mem_info; : const MEMORY_INFO_DATA_HOB *meminfo_hob; : const uint8_t smbios_memory_info_guid[16] = : FSP_SMBIOS_MEMORY_INFO_GUID; : const uint8_t *serial_num; : const char *dram_part_num = NULL; : size_t dram_part_num_len = 0; : bool is_dram_part_overridden = false; : : /* Locate the memory info HOB, presence validated by raminit */ : meminfo_hob = fsp_find_extension_hob_by_guid( : smbios_memory_info_guid, : &hob_size); : if (meminfo_hob == NULL || hob_size == 0) { : printk(BIOS_ERR, "SMBIOS MEMORY_INFO_DATA_HOB not found\n"); : return; : } : : /* : * Allocate CBMEM area for DIMM information used to populate SMBIOS : * table 17 : */ : mem_info = cbmem_add(CBMEM_ID_MEMINFO, sizeof(*mem_info)); : if (mem_info == NULL) { : printk(BIOS_ERR, "CBMEM entry for DIMM info missing\n"); : return; : } : memset(mem_info, 0, sizeof(*mem_info)); : : /* Allow mainboard to override DRAM part number. */ : dram_part_num = mainboard_get_dram_part_num(); : if (dram_part_num) { : dram_part_num_len = strlen(dram_part_num); : is_dram_part_overridden = true; : } : : /* Save available DIMM information */ : index = 0; : dimm_max = ARRAY_SIZE(mem_info->dimm); : for (node = 0; node < MAX_NODE; node++) { : ctrlr_info = &meminfo_hob->Controller[node]; : for (channel = 0; channel < MAX_CH && index < dimm_max; : channel++) { : channel_info = &ctrlr_info->ChannelInfo[channel]; : if (channel_info->Status != CHANNEL_PRESENT) : continue; : : for (dimm = 0; dimm < MAX_DIMM && index < dimm_max; : dimm++) { : src_dimm = &channel_info->DimmInfo[dimm]; : dest_dimm = &mem_info->dimm[index]; : if (src_dimm->Status != DIMM_PRESENT) : continue; : : switch (meminfo_hob->MemoryType) { : case MRC_DDR_TYPE_DDR4: : ddr_type = MEMORY_TYPE_DDR4; : break; : case MRC_DDR_TYPE_DDR3: : ddr_type = MEMORY_TYPE_DDR3; : break; : case MRC_DDR_TYPE_LPDDR3: : ddr_type = MEMORY_TYPE_LPDDR3; : break; : default: : ddr_type = meminfo_hob->MemoryType; : break; : } : /* If there is no DRAM part number overridden by : * mainboard then use original one. */ : if (!is_dram_part_overridden) { : dram_part_num_len = sizeof(src_dimm->ModulePartNum); : dram_part_num = (const char *) : &src_dimm->ModulePartNum[0]; : } : : u8 memProfNum = meminfo_hob->MemoryProfile; : serial_num = src_dimm->SpdSave + : SPD_SAVE_OFFSET_SERIAL; : : /* Populate the DIMM information */ : dimm_info_fill(dest_dimm, : src_dimm->DimmCapacity, : ddr_type, : meminfo_hob->ConfiguredMemoryClockSpeed, : src_dimm->RankInDimm, : channel_info->ChannelId, : src_dimm->DimmId, : dram_part_num, : dram_part_num_len, : serial_num, : meminfo_hob->DataWidth, : meminfo_hob->VddVoltage[memProfNum], : meminfo_hob->EccSupport, : src_dimm->MfgId, : src_dimm->SpdModuleType); : index++; : } : } : } : mem_info->dimm_cnt = index; : printk(BIOS_DEBUG, "%d DIMMs found\n", mem_info->dimm_cnt); : }
Thanks for the feedback I will address this in upcoming commits.
Please refer to this CR: https://review.coreboot.org/c/coreboot/+/51105
Attention is currently required from: Nico Huber, Furquan Shaikh, Patrick Rudolph, Tim Wawrzynczak, Angel Pons, Michael Niewöhner, Aaron Durbin. Hello Bora Guvendik, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Selma Bensaid, Angel Pons, Subrata Banik, Varun Joshi, Michael Niewöhner, Aamir Bohra, Aaron Durbin, Patrick Rudolph, Martin Roth, Tim Wawrzynczak, Usha P,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37628
to look at the new patch set (#22).
Change subject: soc/intel/common: Add romstage common stage file ......................................................................
soc/intel/common: Add romstage common stage file
This patch will ensures all intel soc is using common stage files to make coreboot design flow align across all socs.
CPU, SA, PCH, MCH programming sequence might be different between socs but the function call should route from same location across all soc.
Signed-off-by: Bernardo Perez Priego bernardo.perez.priego@intel.com Change-Id: I06d43ac29f5e87ce731a470e5e145adea07ece4c --- M src/soc/intel/common/Kconfig.common A src/soc/intel/common/basecode/include/intelbasecode/romstage.h A src/soc/intel/common/basecode/romstage/Kconfig A src/soc/intel/common/basecode/romstage/Makefile.inc A src/soc/intel/common/basecode/romstage/romstage.c 5 files changed, 174 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/37628/22
Attention is currently required from: Nico Huber, Furquan Shaikh, Patrick Rudolph, Tim Wawrzynczak, Angel Pons, Bernardo Perez Priego, Michael Niewöhner, Aaron Durbin. Hello Bora Guvendik, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Selma Bensaid, Angel Pons, Subrata Banik, Varun Joshi, Michael Niewöhner, Aamir Bohra, Aaron Durbin, Patrick Rudolph, Martin Roth, Tim Wawrzynczak, Usha P,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37628
to look at the new patch set (#24).
Change subject: soc/intel/common: Add romstage common stage file ......................................................................
soc/intel/common: Add romstage common stage file
This patch will ensures all intel soc is using common stage files to make coreboot design flow align across all socs.
CPU, SA, PCH, MCH programming sequence might be different between socs but the function call should route from same location across all soc.
Signed-off-by: Bernardo Perez Priego bernardo.perez.priego@intel.com Change-Id: I06d43ac29f5e87ce731a470e5e145adea07ece4c --- M src/soc/intel/common/Kconfig.common A src/soc/intel/common/basecode/include/intelbasecode/romstage.h A src/soc/intel/common/basecode/romstage/Kconfig A src/soc/intel/common/basecode/romstage/Makefile.inc A src/soc/intel/common/basecode/romstage/romstage.c 5 files changed, 181 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/37628/24
Attention is currently required from: Felix Singer, Nico Huber, Furquan Shaikh, Patrick Rudolph, Tim Wawrzynczak, Angel Pons, Aaron Durbin. Hello Felix Singer, Bora Guvendik, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Selma Bensaid, Angel Pons, Subrata Banik, Varun Joshi, Michael Niewöhner, Aamir Bohra, Aaron Durbin, Patrick Rudolph, Nico Huber, Martin Roth, Tim Wawrzynczak, Usha P,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37628
to look at the new patch set (#25).
Change subject: soc/intel/common: Add romstage common stage file ......................................................................
soc/intel/common: Add romstage common stage file
This patch will ensures all intel soc is using common stage files to make coreboot design flow align across all socs.
CPU, SA, PCH, MCH programming sequence might be different between socs but the function call should route from same location across all soc.
Signed-off-by: Bernardo Perez Priego bernardo.perez.priego@intel.com Change-Id: I06d43ac29f5e87ce731a470e5e145adea07ece4c --- M src/soc/intel/common/Kconfig.common A src/soc/intel/common/basecode/include/intelbasecode/romstage.h A src/soc/intel/common/basecode/romstage/Kconfig A src/soc/intel/common/basecode/romstage/Makefile.inc A src/soc/intel/common/basecode/romstage/romstage.c 5 files changed, 184 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/37628/25
Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/37628?usp=email )
Change subject: soc/intel/common: Add romstage common stage file ......................................................................
Abandoned
This patch has not been touched in over 12 months. Anyone who wants to take over work on this patch, please feel free to restore it and do any work needed to get it merged. If you create a new patch based on this work, please credit the original author.
-
Bernardo Perez Priego (Code Review) -
Martin L Roth (Code Review)
-
Michael Niewöhner (Code Review)
-
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)