Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37283 )
Change subject: soc/intel/common/block/cse: Modify handling of HMRFPO_ENABLE command ......................................................................
Patch Set 22:
(12 comments)
https://review.coreboot.org/c/coreboot/+/37283/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37283/5//COMMIT_MSG@11 PS5, Line 11: operation mode is Temp Disable Mode
Why? Is this a new requirement for a specific ME firmware version? Or […]
cse_check_hmrfpo_enable_prerequisites() function takes care prerequisites of original flow, and latest fw (Custom SKU) as well.
https://review.coreboot.org/c/coreboot/+/37283/5//COMMIT_MSG@21 PS5, Line 21: opmode Temp Disable Mode.
Thanks for the explanation. […]
Instead of MRP, it checks ME SKU type. If ME SKU type is Custom, then code will check required for current working state Normal and operation mode "soft temporary disable". Otherwise, it checks for current operation mode and operation mode is 'normal'.
https://review.coreboot.org/c/coreboot/+/37283/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37283/7//COMMIT_MSG@10 PS7, Line 10: Allow execution of HMRFPO_ENABLE command only when CSE's : operation mode is Temp Disable Mode
Why? This does not match my reading of BWG. […]
Addendum will be available soon.
https://review.coreboot.org/c/coreboot/+/37283/7//COMMIT_MSG@19 PS7, Line 19: MRP
MRP is ... […]
MRP refers Minimal Recovery Partition.
https://review.coreboot.org/c/coreboot/+/37283/7//COMMIT_MSG@20 PS7, Line 20: Ensure CSE boots from BP1. When CSE boots from BP1, it will have : opmode Temp Disable Mode.
Where is this captured?
Planned Addendum will cover it.
https://review.coreboot.org/c/coreboot/+/37283/20//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37283/20//COMMIT_MSG@36 PS20, Line 36: Existing CSE Image Layout without MRP feature: : = BP2 + BP3 + DATA PART
In this case, what is the expectation for the command? Do the lines 20-23 still hold true?
No, lines 20-23 are not applicable in this case. In this scenario, after issuing HMRFPO_ENABLE command to CSE, a GLOBAL_RESET command must be issued to place CSE in HMRFPO mode.
https://review.coreboot.org/c/coreboot/+/37283/7/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/37283/7/src/soc/intel/common/block/... PS7, Line 667: if (hfs1.fields.operation_mode != ME_HFS_MODE_TEMP_DISABLE) { : printk(BIOS_ERR, "HECI: ME is not in expected mode/state(0x%x)\n", hfs1.data); : goto failed; : }
This does not match my reading of CML BWG. Please point me to the doc where this is captured.
The change is made considering ME Custom SKU only. I have addressed prerequisite for this command in the latest patch.
https://review.coreboot.org/c/coreboot/+/37283/20/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/37283/20/src/soc/intel/common/block... PS20, Line 669: !cse_is_hfs1_com_soft_temp_disable()
This doesn't match BWG for CML or TGL. I see expected Current Operating Mode as Normal. […]
Yes you are right. A separate Addendum will address the prerequisite for this command considering a new case as well( when CSE boots from BP1_RO).
https://review.coreboot.org/c/coreboot/+/37283/20/src/soc/intel/common/block... PS20, Line 687:
This command requires a global reset after it is issued(that is what I see in BWG). […]
Slight change in HMRFPO mode implementation as defined in current BWG. Hence, GLOBAL RESET is not issued in the handler. This will be covered in our planned document( Addendum to BWG).
https://review.coreboot.org/c/coreboot/+/37283/20/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/37283/20/src/soc/intel/common/block... PS20, Line 131: When MRP feature is enabled,
Is this command supposed to have different conditions based on whether MRP is enabled or not?
Yes, Command has support different conditions based on the SKU type(MRP which is a default feature in Custom SKU).
https://review.coreboot.org/c/coreboot/+/37283/7/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/37283/7/src/soc/intel/common/block/... PS7, Line 133: MRP
What is MRP?
MRP refers Minimal Recovery Partition. This is implemented in Custom SKU.With this feature, ME will boot in Soft Disable Mode from BP1(RO), and allows update of BP2(RW) via HMRFPO mode as explained in the commit message.
https://review.coreboot.org/c/coreboot/+/37283/7/src/soc/intel/common/block/... PS7, Line 134: 1. Ensure CSE boots from BP1(RO). : * - Send set_next_boot_partition(BP1) : * - Issue CSE Only Reset : * 2. Send HMRFPO_ENABLE command to CSE. Further, no reset is required.
Please point me to the doc # that captures this.
This info is not yet captured in any doc as of now. We are woring on the documentation. It will be available soon.