Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35228 )
Change subject: src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common ......................................................................
Patch Set 25:
(6 comments)
How much of this applies to small core? In all of this common code movement, I am afraid things are being generalized without looking at all platforms.
https://review.coreboot.org/c/coreboot/+/35228/25/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35228/25/src/soc/intel/common/block... PS25, Line 83: /* HECI Message Header */ : struct mkhi_hdr { : uint8_t group_id; : uint8_t command:7; : uint8_t is_resp:1; : uint8_t rsvd; : uint8_t result; : } __packed; If this is common across SoCs and has the same format, why not get rid of various definitions of this sprinkled across different SoCs?
https://review.coreboot.org/c/coreboot/+/35228/25/src/soc/intel/common/block... PS25, Line 577: The reset type can be GLOBAL_RESET/ : * HOST_RESET_ONLY/CSE_RESET_ONLY. Are these supported by ME on all platforms? I am looking at a spec which does not provide these different options at all
https://review.coreboot.org/c/coreboot/+/35228/25/src/soc/intel/common/block... PS25, Line 601: -1 All other functions return 0 on error and 1 on success. Why is this function different?
https://review.coreboot.org/c/coreboot/+/35228/25/src/soc/intel/common/block... PS25, Line 609: if (rst_type == CSE_RESET_ONLY) Why is the response type different for the reset types? Can you please point me to the spec#?
https://review.coreboot.org/c/coreboot/+/35228/25/src/soc/intel/common/block... PS25, Line 610: heci_send_receive(&msg, sizeof(msg), NULL, 0); Why not use heci_send()?
https://review.coreboot.org/c/coreboot/+/35228/25/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35228/25/src/soc/intel/common/block... PS25, Line 114: rst_type There should be a comment indicating that this rst_type is one of the definitions below. It might make sense to define a enum for these values. It will be easier to reference.