Change in coreboot[master]: soc/intel/{common,skl,cnl,icl,apl,tgl}: Move HFSTS1 register definiti...
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35546 )
Change subject: soc/intel/{common,skl,cnl,icl,apl,tgl}: Move HFSTS1 register definition to SoC ......................................................................
Patch Set 27:
(6 comments)
https://review.coreboot.org/c/coreboot/+/35546/25//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35546/25//COMMIT_MSG@9 PS25, Line 9: Move me_hfsts1 structure from common code to SoC specific.
It would be good to document the reasoning behind this change here.
Done
https://review.coreboot.org/c/coreboot/+/35546/25//COMMIT_MSG@9 PS25, Line 9: Move me_hfsts1 structure from common code to SoC specific.
This patch is doing much more than just moving the hfsts1 to SoC. […]
Done
https://review.coreboot.org/c/coreboot/+/35546/25/src/soc/intel/apollolake/i... File src/soc/intel/apollolake/include/soc/me.h:
https://review.coreboot.org/c/coreboot/+/35546/25/src/soc/intel/apollolake/i... PS25, Line 56: cse_is_hfs1_cws_normal
do we have to keep the hfs1 here? the common code is looking for current working state of the csme t […]
Done
https://review.coreboot.org/c/coreboot/+/35546/25/src/soc/intel/apollolake/i... PS25, Line 52: /* : * Checks current working operation state is normal or not. : * Returns true if CSE's current working state is normal, otherwise false. : */ : bool cse_is_hfs1_cws_normal(void); : : /* : * Checks CSE's current operation mode is normal or not. : * Returns true if CSE's current operation mode is normal, otherwise false. : */ : bool cse_is_hfs1_com_normal(void); : : /* : * Checks CSE's current operation mode is SECOVER_MEI_MSG or not. : * Returns true if CSE's current operation mode is SECOVER_MEI_MSG, otherwise false. : */ : bool cse_is_hfs1_com_secover_mei_msg(void); : : /* : * Checks CSE's current operation mode is Soft Disable Mode or not. : * Returns true if CSE's current operation mode is Soft Disable Mode, otherwise false. : */ : bool cse_is_hfs1_com_soft_temp_disable(void);
you have multiple declarations of these functions in each SoC. […]
Done
https://review.coreboot.org/c/coreboot/+/35546/25/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35546/25/src/soc/intel/common/block... PS25, Line 630: cse_is_hfs1_cws_normal
My comment was actually continuation of the one made on cse.h. Please see explanation there.
Done
https://review.coreboot.org/c/coreboot/+/35546/25/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35546/25/src/soc/intel/common/block... PS25, Line 54: /* ME Host Firmware Status register 1 */ : union me_hfsts1 { : u32 data; : struct { : u32 working_state: 4; : u32 mfg_mode: 1; : u32 fpt_bad: 1; : u32 operation_state: 3; : u32 fw_init_complete: 1; : u32 ft_bup_ld_flr: 1; : u32 update_in_progress: 1; : u32 error_code: 4; : u32 operation_mode: 4; : u32 reset_count: 4; : u32 boot_options_present: 1; : u32 reserved1: 1; : u32 bist_test_state: 1; : u32 bist_reset_request: 1; : u32 current_power_source: 2; : u32 d3_support_valid: 1; : u32 d0i3_support_valid: 1; : } __packed fields; : };
What I meant is: […]
Yes, it make sense, and implemented the code accordingly.
-
Sridhar Siricilla (Code Review)