Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36786 )
Change subject: soc/intel/common: Add function to wait for CSE to enter Soft Temp Disable mode ......................................................................
Patch Set 38:
(4 comments)
https://review.coreboot.org/c/coreboot/+/36786/16/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/36786/16/src/soc/intel/common/block... PS16, Line 317: uint8_t
Why this data type? Please use CB_SUCCESS and friends.
Since the function return 0 or 1, so uint8_t as return type.
https://review.coreboot.org/c/coreboot/+/36786/36/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/36786/36/src/soc/intel/common/block... PS36, Line 314: HECI_DELAY_READY
Is that the right timeout to use? I believe that is the one used for ensuring HECI is ready.
15 seconds(HECI_DELAY_READY) is not right timeout in this context. Typically, CSE takes less than 1 second to boot from RO_BP1 after a reset. After a CSE reset, I notice CSE takes ~260ms to boot from RO_BP1 as logged by this function. Hence, I set delay time to 5 seconds.
https://review.coreboot.org/c/coreboot/+/36786/36/src/soc/intel/common/block... PS36, Line 317: if (stopwatch_expired(&sw))
I think it would be good to have a print indicating the error?
Done
https://review.coreboot.org/c/coreboot/+/36786/36/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/36786/36/src/soc/intel/common/block... PS36, Line 200: cse_wait_com_soft_temp_disable
I haven't seen the complete use of this yet, but I wonder if this really needs to be exposed out of […]
The usage as you suggested can be seen in https://review.coreboot.org/c/coreboot/+/35403/54/src/soc/intel/common/basec....