[M] Change in coreboot[master]: soc/intel/cse_lite: Back up PSR data during CSE FW downgrade
Attention is currently required from: Subrata Banik, Jérémy Compostella, Paul Menzel, Himanshu Sahdev.
Anil Kumar K 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 29:
(4 comments)
File src/soc/intel/common/block/cse/cse_lite.c:
https://review.coreboot.org/c/coreboot/+/74577/comment/5c2b456c_2e9a2393 PS27, Line 153: sizeof(struct psr_heci_fw_downgrade_backup_res)
`sizeof(*res)`
is there any issue using sizeof with structure ?
File src/soc/intel/common/block/cse/cse_lite.c:
https://review.coreboot.org/c/coreboot/+/74577/comment/4d4f3bb3_d7e53a91 PS28, Line 14: #include <pc80/mc146818rtc.h>
what is this needed for ?
this is required for cmos_write and cmos_read functions
https://review.coreboot.org/c/coreboot/+/74577/comment/510baa42_715e8585 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 us […]
at the moment this function is only called during downgrade. i am not sure if this will be called in future from other code paths. so i used a dedicated function
https://review.coreboot.org/c/coreboot/+/74577/comment/2f775e8b_1b1227ff PS28, Line 813:
coding style: I don't think there is any rule requiring a blank link at the beginning of a block. […]
Done
-
Anil Kumar K (Code Review)