Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45759 )
Change subject: soc/intel/alderlake/ramstage: Do initial SoC commit till ramstage ......................................................................
Patch Set 6:
(13 comments)
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/acp... File src/soc/intel/alderlake/acpi.c:
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/acp... PS3, Line 235: if (tbtbar && tbten) { Should there also be a check here depending on whether or not each device (00:07.0 - 7.3) is enabled ?
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/chi... File src/soc/intel/alderlake/chip.h:
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/chi... PS3, Line 242: /* CNVi */ : uint8_t CnviMode; : uint8_t CnviBtCore; Why did these get removed?
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/cpu... File src/soc/intel/alderlake/cpu.c:
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/cpu... PS3, Line 150: /* TODO(adurbin): This should only be done on a cold boot. Also, some : * of these banks are core vs package scope. For now every CPU clears : * every bank. */ fun fact, AFAICT this TODO has been in the codebase for 7 years
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/esp... File src/soc/intel/alderlake/espi.c:
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/esp... PS3, Line 132: u8 stdint.h
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/fin... File src/soc/intel/alderlake/finalize.c:
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/fin... PS3, Line 57: /* TODO: Add Thermal Configuration */ Is there still a reason that `pch_thermal_configuration()` isn't called here? (should we be calling it in tigerlake too?)
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/fsp... File src/soc/intel/alderlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/fsp... PS3, Line 152: 0xff OC_SKIP
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/fsp... PS3, Line 160: 0xff OC_SKIP
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/inc... File src/soc/intel/alderlake/include/soc/meminit.h:
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/inc... PS3, Line 27: 16 16 DIMM slots?
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/inc... PS3, Line 38: nit:tab
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/me.... File src/soc/intel/alderlake/me.c:
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/me.... PS3, Line 9: /* Host Firmware Status Register 2 */ : union me_hfsts2 { : uint32_t data; : struct { : uint32_t nftp_load_failure : 1; : uint32_t icc_prog_status : 2; : uint32_t invoke_mebx : 1; : uint32_t cpu_replaced : 1; : uint32_t rsvd0 : 1; : uint32_t mfs_failure : 1; : uint32_t warm_reset_rqst : 1; : uint32_t cpu_replaced_valid : 1; : uint32_t low_power_state : 1; : uint32_t me_power_gate : 1; : uint32_t ipu_needed : 1; : uint32_t forced_safe_boot : 1; : uint32_t rsvd1 : 2; : uint32_t listener_change : 1; : uint32_t status_data : 8; : uint32_t current_pmevent : 4; : uint32_t phase : 4; : } __packed fields; : }; : : /* Host Firmware Status Register 4 */ : union me_hfsts4 { : uint32_t data; : struct { : uint32_t rsvd0 : 9; : uint32_t enforcement_flow : 1; : uint32_t sx_resume_type : 1; : uint32_t rsvd1 : 1; : uint32_t tpms_disconnected : 1; : uint32_t rvsd2 : 1; : uint32_t fwsts_valid : 1; : uint32_t boot_guard_self_test : 1; : uint32_t rsvd3 : 16; : } __packed fields; : }; : : /* Host Firmware Status Register 5 */ : union me_hfsts5 { : uint32_t data; : struct { : uint32_t acm_active : 1; : uint32_t valid : 1; : uint32_t result_code_source : 1; : uint32_t error_status_code : 5; : uint32_t acm_done_sts : 1; : uint32_t timeout_count : 7; : uint32_t scrtm_indicator : 1; : uint32_t inc_boot_guard_acm : 4; : uint32_t inc_key_manifest : 4; : uint32_t inc_boot_policy : 4; : uint32_t rsvd0 : 2; : uint32_t start_enforcement : 1; : } __packed fields; : }; : : /* Host Firmware Status Register 6 */ : union me_hfsts6 { : uint32_t data; : struct { : uint32_t force_boot_guard_acm : 1; : uint32_t cpu_debug_disable : 1; : uint32_t bsp_init_disable : 1; : uint32_t protect_bios_env : 1; : uint32_t rsvd0 : 2; : uint32_t error_enforce_policy : 2; : uint32_t measured_boot : 1; : uint32_t verified_boot : 1; : uint32_t boot_guard_acmsvn : 4; : uint32_t kmsvn : 4; : uint32_t bpmsvn : 4; : uint32_t key_manifest_id : 4; : uint32_t boot_policy_status : 1; : uint32_t error : 1; : uint32_t boot_guard_disable : 1; : uint32_t fpf_disable : 1; : uint32_t fpf_soc_lock : 1; : uint32_t txt_support : 1; : } __packed fields; : }; : note to self: convert to masks & shifts, bitfields are not OK
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/pmc... File src/soc/intel/alderlake/pmc.c:
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/pmc... PS3, Line 128: : /* FIXME: Rewrite loop below without this. */ : extern struct chip_operations drivers_intel_pmc_mux_ops; : : /* By default, TGL uses the PMC MUX for all ports, so port_number is unused */ : const struct device *soc_get_pmc_mux_device(int port_number) : { : const struct device *pmc; : struct device *child; : : child = NULL; : pmc = pcidev_path_on_root(PCH_DEVFN_PMC); : if (!pmc || !pmc->link_list) : return NULL; : : while ((child = dev_bus_each_child(pmc->link_list, child)) != NULL) : if (child->chip_ops == &drivers_intel_pmc_mux_ops) : break; : : /* child will either be the correct device or NULL if not found */ : return child; : } Good news, this is being removed! CB:45747, feel free to rebase there or not and I'll clean up my mess later 😄
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/pmu... File src/soc/intel/alderlake/pmutil.c:
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/pmu... PS3, Line 217: /* : * Check for any power failure to determine if this a wake from : * S5 because the PCH does not set the WAK_STS bit when waking : * from a true G3 state. : */ alignment of `*`
https://review.coreboot.org/c/coreboot/+/45759/3/src/soc/intel/alderlake/pmu... PS3, Line 253: ps->tco1_sts, ps->tco2_sts); can fit on previous line