Angel Pons 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: Code-Review+1
(8 comments)
https://review.coreboot.org/c/coreboot/+/45759/5/src/soc/intel/alderlake/esp... File src/soc/intel/alderlake/espi.c:
https://review.coreboot.org/c/coreboot/+/45759/5/src/soc/intel/alderlake/esp... PS5, Line 27: */
Its same Angel. its 32MB in size […]
Ack, thanks for confirming
https://review.coreboot.org/c/coreboot/+/45759/5/src/soc/intel/alderlake/esp... PS5, Line 192: i8259_configure_irq_trigger(9, 1);
9 refer as interrupt number and 1 refer as level trigger for 8259
I see.
https://review.coreboot.org/c/coreboot/+/45759/5/src/soc/intel/alderlake/fsp... File src/soc/intel/alderlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/45759/5/src/soc/intel/alderlake/fsp... PS5, Line 219: else
Didn't get this Angel
If devicetree has enabled DITO config for this SATA port (config->SataPortsEnableDitoConfig[i]), it should provide valid values for DITO multiplier and DITO. If it doesn't, it's a bug in the devicetree.
Yesterday, I thought about printing a message here stating that the devicetree value is invalid, but I doubt anyone will notice. Instead, we may drop the non-zero check and always write the devicetree value:
if (config->SataPortsEnableDitoConfig[i]) { params->SataPortsDmVal[i] = config->SataPortsDmVal[i]; params->SataPortsDitoVal[i] = config->SataPortsDitoVal[i]; }
Note that the FSP UPDs are left untouched when `config->SataPortsEnableDitoConfig[i]` is false.
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
Why are bitfields not OK? IMHO, bitfields provide type safety and overflow checking.
CB:40981 patched some values that would result in a build failure when the temporary `IOSAV_SUBSEQUENCE` macro is redefined to use structs instead of masks and shifts.
Don't forget about CB:45713 either! 😄
https://review.coreboot.org/c/coreboot/+/45759/5/src/soc/intel/alderlake/pmc... File src/soc/intel/alderlake/pmc.c:
https://review.coreboot.org/c/coreboot/+/45759/5/src/soc/intel/alderlake/pmc... PS5, Line 130: /* FIXME: Rewrite loop below without this. */
Answer should be yes as this is getting called from ec_lpc.c as part of acpi_fill_ssdt.
You can try adding printk's when this code runs to see if they are initialized, then check those fields instead of comparing the ops. This would need to be done for TGL too.
Or we can go with CB:45747 instead.
https://review.coreboot.org/c/coreboot/+/45759/5/src/soc/intel/alderlake/pmc... PS5, Line 133: /* By default, TGL uses the PMC MUX for all ports, so port_number is unused */
my bad, TGL and ADL should use the same PMC MUX programming for Type-C
Ack
https://review.coreboot.org/c/coreboot/+/45759/5/src/soc/intel/alderlake/smm... File src/soc/intel/alderlake/smmrelocate.c:
PS5:
Honestly this file a good candidate for common code, many function can be moved into common, i belie […]
Sounds good. I'll be happy to review. Some exceptions to account for:
- Bay Trail, Braswell and Apollo Lake need special handling as they are using a different SMM save state. - I plan on refactoring Haswell and Broadwell at some point, and will move Broadwell out of soc/intel.
SKL, CFL and newer should be good to unify.
https://review.coreboot.org/c/coreboot/+/45759/5/src/soc/intel/alderlake/smm... PS5, Line 58: * According the BWG the IEDBASE MSR is in bits 63:32. It's
Yes. […]
Ack