Attention is currently required from: Anil Kumar K, Subrata Banik, Paul Menzel, Himanshu Sahdev.
Jérémy Compostella has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74577 )
Change subject: soc/intel/cse_lite: Back up PSR data during CSE FW downgrade ......................................................................
Patch Set 28:
(5 comments)
File src/soc/intel/common/block/cse/cse_lite.c:
https://review.coreboot.org/c/coreboot/+/74577/comment/504baa92_81a962bf PS27, Line 145: struct psr_heci_fw_downgrade_backup_req { : struct psr_heci_header header; : } __packed; Why do you need to define this ?
https://review.coreboot.org/c/coreboot/+/74577/comment/49efbc5a_95267d3b PS27, Line 153: sizeof(struct psr_heci_fw_downgrade_backup_res) `sizeof(*res)`
File src/soc/intel/common/block/cse/cse_lite.c:
https://review.coreboot.org/c/coreboot/+/74577/comment/d37d2c4d_9d7e5d50 PS28, Line 14: #include <pc80/mc146818rtc.h> what is this needed for ?
https://review.coreboot.org/c/coreboot/+/74577/comment/869bc66a_009fd767 PS28, Line 142: static enum cb_err cse_send_backup_psr_cmd(struct psr_heci_fw_downgrade_backup_res *res) I don't think there is a lot of value having this as a dedicated function unless you have plan of using it for other purposes.
https://review.coreboot.org/c/coreboot/+/74577/comment/866eea7c_1d7a04f3 PS28, Line 813: coding style: I don't think there is any rule requiring a blank link at the beginning of a block. And your patch seems to be introducing this.