Hello Varun Joshi,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/38500
to review the following change.
Change subject: Common code romstage ......................................................................
Common code romstage
Signed-off-by: Varun Joshi varun.joshi@intel.corp-partner.google.com Change-Id: If949fe60086165f62cb1065c76575825ebc46d15 --- M src/soc/intel/apollolake/romstage.c 1 file changed, 61 insertions(+), 58 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/38500/1
diff --git a/src/soc/intel/apollolake/romstage.c b/src/soc/intel/apollolake/romstage.c index 258f4ff..0d2225a 100644 --- a/src/soc/intel/apollolake/romstage.c +++ b/src/soc/intel/apollolake/romstage.c @@ -70,13 +70,24 @@ #define P2SB_HPTC_ADDRESS_SELECT_2 (2 << 0) #define P2SB_HPTC_ADDRESS_SELECT_3 (3 << 0)
+static const struct romstage_ops s_romstage_ops = { + &aplk_romstage_soc_init, + &romstage_cmn_pch_init, + &romstage_cmn_cpu_init, + &romstage_cmn_is_s3wake, + &aplk_soc_mem_init_params, + &aplk_mainboard_mem_init_params, + &aplk_soc_post_mem_init + &romstage_cmn_mb_post_mem_init +}; + /* * Enables several BARs and devices which are needed for memory init * - MCH_BASE_ADDR is needed in order to talk to the memory controller * - HPET is enabled because FSP wants to store a pointer to global data in the * HPET comparator register */ -static void soc_early_romstage_init(void) +static void aplk_romstage_soc_init(void) { static const struct sa_mmio_descriptor soc_fixed_pci_resources[] = { { MCHBAR, MCH_BASE_ADDRESS, MCH_BASE_SIZE, "MCHBAR" }, @@ -190,36 +201,6 @@ cpu_set_p_state_to_turbo_ratio(); }
-void mainboard_romstage_entry(void) -{ - bool s3wake; - size_t var_size; - struct chipset_power_state *ps = pmc_get_power_state(); - const void *new_var_data; - - soc_early_romstage_init(); - - s3wake = pmc_fill_power_state(ps) == ACPI_S3; - fsp_memory_init(s3wake); - - if (punit_init()) - set_max_freq(); - else - printk(BIOS_DEBUG, "Punit failed to initialize properly\n"); - - /* Stash variable MRC data and let cache system update it later */ - new_var_data = fsp_find_extension_hob_by_guid(hob_variable_guid, - &var_size); - if (new_var_data) - mrc_cache_stash_data(MRC_VARIABLE_DATA, - fsp_version, new_var_data, - var_size); - else - printk(BIOS_ERR, "Failed to determine variable data\n"); - - mainboard_save_dimm_info(); -} - static void fill_console_params(FSPM_UPD *mupd) { if (CONFIG(CONSOLE_SERIAL)) { @@ -263,31 +244,6 @@ } }
-static void soc_memory_init_params(FSPM_UPD *mupd) -{ -#if CONFIG(SOC_INTEL_GLK) - /* Only for GLK */ - FSP_M_CONFIG *m_cfg = &mupd->FspmConfig; - - m_cfg->PrmrrSize = get_prmrr_size(); - - /* - * CpuMemoryTest in FSP tests 0 to 1M of the RAM after MRC init. - * With PAGING_IN_CACHE_AS_RAM enabled for GLK, there was no page - * table entry for this range which caused a page fault. Since this - * test is anyway not exhaustive, skipping the memory test in FSP. - */ - m_cfg->SkipMemoryTestUpd = 1; - - /* - * PCIe power sequence can be done from within FSP when provided - * with the GPIOs used for PERST to FSP. Since this is done in - * coreboot, skipping the PCIe power sequence done by FSP. - */ - m_cfg->SkipPciePowerSequence = 1; -#endif -} - static void parse_devicetree_setting(FSPM_UPD *m_upd) { #if CONFIG(SOC_INTEL_GLK) @@ -299,7 +255,7 @@ #endif }
-void platform_fsp_memory_init_params_cb(FSPM_UPD *mupd, uint32_t version) +static void aplk_soc_mem_init_params(FSPM_UPD *mupd) { struct region_device rdev;
@@ -307,9 +263,35 @@
fill_console_params(mupd);
- if (CONFIG(SOC_INTEL_GLK)) +/*f (CONFIG(SOC_INTEL_GLK)) soc_memory_init_params(mupd); +*/ +#if CONFIG(SOC_INTEL_GLK) + /* Only for GLK */ + FSP_M_CONFIG *m_cfg = &mupd->FspmConfig;
+ m_cfg->PrmrrSize = get_prmrr_size(); + + /* + * CpuMemoryTest in FSP tests 0 to 1M of the RAM after MRC init. + * With PAGING_IN_CACHE_AS_RAM enabled for GLK, there was no page + * table entry for this range which caused a page fault. Since this + * test is anyway not exhaustive, skipping the memory test in FSP. + */ + m_cfg->SkipMemoryTestUpd = 1; + + /* + * PCIe power sequence can be done from within FSP when provided + * with the GPIOs used for PERST to FSP. Since this is done in + * coreboot, skipping the PCIe power sequence done by FSP. + */ + m_cfg->SkipPciePowerSequence = 1; +#endif +} + +static void aplk_mainboard_mem_init_params(FSPM_UPD *mupd) +{ + uint32_t version; mainboard_memory_init_params(mupd);
parse_devicetree_setting(mupd); @@ -351,7 +333,28 @@ }
fsp_version = version; +}
+static void aplk_soc_post_mem_init(void) +{ + struct romstage_ops *rs_ops = soc_get_ops(); + + if (punit_init()) + set_max_freq(); + else + printk(BIOS_DEBUG, "Punit failed to initialize properly\n"); + + /* Stash variable MRC data and let cache system update it later */ + new_var_data = fsp_find_extension_hob_by_guid(hob_variable_guid, + &var_size); + if (new_var_data) + mrc_cache_stash_data(MRC_VARIABLE_DATA, + fsp_version, new_var_data, + var_size); + else + printk(BIOS_ERR, "Failed to determine variable data\n"); + + mainboard_save_dimm_info(); }
__weak
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38500 )
Change subject: Common code romstage ......................................................................
Patch Set 1:
(50 comments)
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... File src/soc/intel/apollolake/romstage.c:
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 270: /* Only for GLK */ code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 271: FSP_M_CONFIG *m_cfg = &mupd->FspmConfig; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 271: FSP_M_CONFIG *m_cfg = &mupd->FspmConfig; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 271: FSP_M_CONFIG *m_cfg = &mupd->FspmConfig; need consistent spacing around '*' (ctx:WxV)
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 273: m_cfg->PrmrrSize = get_prmrr_size(); code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 273: m_cfg->PrmrrSize = get_prmrr_size(); please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 275: /* code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 276: * CpuMemoryTest in FSP tests 0 to 1M of the RAM after MRC init. code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 277: * With PAGING_IN_CACHE_AS_RAM enabled for GLK, there was no page code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 278: * table entry for this range which caused a page fault. Since this code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 279: * test is anyway not exhaustive, skipping the memory test in FSP. code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 280: */ code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 281: m_cfg->SkipMemoryTestUpd = 1; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 281: m_cfg->SkipMemoryTestUpd = 1; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 283: /* code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 284: * PCIe power sequence can be done from within FSP when provided code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 285: * with the GPIOs used for PERST to FSP. Since this is done in code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 286: * coreboot, skipping the PCIe power sequence done by FSP. code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 287: */ code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 288: m_cfg->SkipPciePowerSequence = 1; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 288: m_cfg->SkipPciePowerSequence = 1; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 340: struct romstage_ops *rs_ops = soc_get_ops(); code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 340: struct romstage_ops *rs_ops = soc_get_ops(); please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 342: if (punit_init()) code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 342: if (punit_init()) please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 343: set_max_freq(); code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 343: set_max_freq(); please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 344: else code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 344: else please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 345: printk(BIOS_DEBUG, "Punit failed to initialize properly\n"); code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 345: printk(BIOS_DEBUG, "Punit failed to initialize properly\n"); please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 347: /* Stash variable MRC data and let cache system update it later */ code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 348: new_var_data = fsp_find_extension_hob_by_guid(hob_variable_guid, code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 348: new_var_data = fsp_find_extension_hob_by_guid(hob_variable_guid, please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 349: &var_size); code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 349: &var_size); please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 350: if (new_var_data) code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 350: if (new_var_data) please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 351: mrc_cache_stash_data(MRC_VARIABLE_DATA, code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 351: mrc_cache_stash_data(MRC_VARIABLE_DATA, please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 352: fsp_version, new_var_data, code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 352: fsp_version, new_var_data, please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 353: var_size); code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 353: var_size); please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 354: else code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 354: else please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 355: printk(BIOS_ERR, "Failed to determine variable data\n"); code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 355: printk(BIOS_ERR, "Failed to determine variable data\n"); please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 357: mainboard_save_dimm_info(); code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38500/1/src/soc/intel/apollolake/ro... PS1, Line 357: mainboard_save_dimm_info(); please, no spaces at the start of a line
-
build bot (Jenkins) (Code Review) -
Varun Joshi (Code Review)