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}: Make me_hfsts1 structure SoC specific ......................................................................
Patch Set 22:
(15 comments)
https://review.coreboot.org/c/coreboot/+/35546/14/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35546/14/src/soc/intel/common/block... PS14, Line 78: GR_ORIGIN_BIOS_POST
It's not command id or group id. […]
Done
https://review.coreboot.org/c/coreboot/+/35546/14/src/soc/intel/common/block... PS14, Line 84: operation and working modes
Modified the comment in the patch: https://review.coreboot. […]
Done
https://review.coreboot.org/c/coreboot/+/35546/14/src/soc/intel/common/block... PS14, Line 85: HFS
Implemented the comment in the patch - https://review.coreboot. […]
Done
https://review.coreboot.org/c/coreboot/+/35546/14/src/soc/intel/common/block... PS14, Line 86: MODE
Implemented the comment in the patch - https://review.coreboot. […]
Done
https://review.coreboot.org/c/coreboot/+/35546/14/src/soc/intel/common/block... PS14, Line 598: rst_type
Are the different reset types supported by ME firmware for all platforms? For CML, I don't see all t […]
Done
https://review.coreboot.org/c/coreboot/+/35546/14/src/soc/intel/common/block... PS14, Line 606: heci_reset();
heci_reset() is called to reset heci interface to synchronize host and ME for communication.
Done
https://review.coreboot.org/c/coreboot/+/35546/14/src/soc/intel/common/block... PS14, Line 609: memset(&reply, 0, reply_size);
What is the use of setting this to 0?
Not required. Removed the code.
https://review.coreboot.org/c/coreboot/+/35546/14/src/soc/intel/common/block... PS14, Line 650: padding
Rather than padding, can we name this reserved just like it is in BWG?
Done in the patch @ https://review.coreboot.org/c/coreboot/+/37283/13/src/soc/intel/common/block...
https://review.coreboot.org/c/coreboot/+/35546/14/src/soc/intel/common/block... PS14, Line 662: or temporary disable mode
Is this correct? I don't see this in BWG.
Please refer patch: https://review.coreboot.org/c/coreboot/+/37283/13 for latest. Currently, this is not yet documented. We are working internally to document the same.
https://review.coreboot.org/c/coreboot/+/35546/14/src/soc/intel/common/block... PS14, Line 664: hfs1.fields.working_state
Not for this change, but I think we should add helper fucntions for CWS and COP i.e. […]
Helper function wouldn't be unnecessay function call & additional operations to return the bitfield.
https://review.coreboot.org/c/coreboot/+/35546/14/src/soc/intel/common/block... PS14, Line 678: }
comment is addressed in the patch - https://review.coreboot. […]
Done
https://review.coreboot.org/c/coreboot/+/35546/14/src/soc/intel/common/block... PS14, Line 679: return 1;
HMRFPO enabling needs to be followed by a global reset. […]
keeping global reset out of this command handler will help supporting all flows of CSE FW Update.
https://review.coreboot.org/c/coreboot/+/35546/15/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35546/15/src/soc/intel/common/block... PS15, Line 47: #if CONFIG(SOC_INTEL_APOLLOLAKE)
i don't think in common code we had any soc check so far
correct.
https://review.coreboot.org/c/coreboot/+/35546/14/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35546/14/src/soc/intel/common/block... PS14, Line 47: SOC_INTEL_APOLLOLAKE
Moved me_hfsts1 defination into SoC specific code.
Done
https://review.coreboot.org/c/coreboot/+/35546/14/src/soc/intel/common/block... PS14, Line 143:
Implemented in the patch : https://review.coreboot. […]
Done