Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35226 )
Change subject: src/soc/intel/common/block/cse: Make hfsts1 common & add helper functions ......................................................................
Patch Set 25:
(5 comments)
https://review.coreboot.org/c/coreboot/+/35226/25//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35226/25//COMMIT_MSG@9 PS25, Line 9: Host FW status 1 (FWSTS1/HFSTS1) register definition is common across SoCs, Is this true even for small core? If yes, why was APL not updated to use this?
https://review.coreboot.org/c/coreboot/+/35226/25/src/soc/intel/cannonlake/m... File src/soc/intel/cannonlake/me.c:
https://review.coreboot.org/c/coreboot/+/35226/25/src/soc/intel/cannonlake/m... PS25, Line 215: hfsts2 What about the other hfsts? Are they not common across SoCs?
https://review.coreboot.org/c/coreboot/+/35226/25/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35226/25/src/soc/intel/common/block... PS25, Line 266: 'HECI_OP_MODE_SEC_OVERRIDE' for 15 seconds How was the 15 second timeout decided?
https://review.coreboot.org/c/coreboot/+/35226/25/src/soc/intel/common/block... PS25, Line 273: if (stopwatch_expired(&sw)) It would be good to add a print indicating that timeout occurred. Also, it might be helpful to log how much time it typically takes.
https://review.coreboot.org/c/coreboot/+/35226/25/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35226/25/src/soc/intel/common/block... PS25, Line 101: set_host_ready This is a very generic name. Since it is being exposed as an API, it should ideally be named something like cse_set_host_ready()