Sridhar Siricilla 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 17:
(6 comments)
https://review.coreboot.org/c/coreboot/+/35226/14/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35226/14/src/soc/intel/common/block... PS14, Line 282: csr |= CSR_READY;
281 and 282 can combine ?
For readability sake, the operations are separated into two statements.
https://review.coreboot.org/c/coreboot/+/35226/14/src/soc/intel/common/block... PS14, Line 289: 15000
same here ?
Done
https://review.coreboot.org/c/coreboot/+/35226/14/src/soc/intel/common/block... PS14, Line 291: 100
can't use macro ?
Done
https://review.coreboot.org/c/coreboot/+/35226/17/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35226/17/src/soc/intel/common/block... PS17, Line 258: set_host_ready
just to maintain naming consistency add cse in function name?
There are two types of CSR register VIZ: HOST_CSR & CSE_CSR register. The function clears reset state in HOST_CSR register. It confuses if I add cse in the function name.
https://review.coreboot.org/c/coreboot/+/35226/17/src/soc/intel/common/block... PS17, Line 525: csr = read_host_csr(); : csr |= (CSR_RESET | CSR_IG); : write_host_csr(csr);
can we create a static helper function ?
can be done.
https://review.coreboot.org/c/coreboot/+/35226/14/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35226/14/src/soc/intel/common/block... PS14, Line 75: /* : * Clears the reset state in the Host CSR : */
doesn't req multi line comments
Done