Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35546 )
Change subject: soc/intel/{common,skl,cnl,icl,apl,tgl}: Make me_hfsts1 structure SoC specific ......................................................................
Patch Set 25:
(2 comments)
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
The whole point of having helper functions was to mask the different notions of the hfsts1 among SoC […]
My comment was actually continuation of the one made on cse.h. Please see explanation there.
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; : };
I don't understand your comment, I think this patch is doing what you say here. […]
What I meant is:
1) Move union me_hfsts1 to each SoC me.h file - As you mentioned this is already done by the current CL.
2) Keep ME_HFS1_* macros above in lines 36-42 above here in this file i.e. soc/intel/common/block/include/intelblocks/cse.h
3) Add definitions of helper functions cse_is_cop_normal(), cse_is_cws_normal(), etc. to soc/intel/common/block/cse/cse.c instead of every SoC. This is under the assumption that the fields in hfsts1 that are required in the helper functions in cse.c are named consistently across SoCs. The bit locations can be different, but that wouldn't matter.
Does that make sense?