Attention is currently required from: Kapil Porwal, Sridhar Siricilla, Subrata Banik.
Dinesh Gehlot has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74995?usp=email )
Change subject: soc/intel/cmd/blk/cse: Implement APIs to access FW versions in CMOS ......................................................................
Patch Set 54:
(3 comments)
File src/soc/intel/common/block/cse/cse_lite_cmos.h:
https://review.coreboot.org/c/coreboot/+/74995/comment/ca042367_3d0b6229 : PS54, Line 12: void cmos_get_curr_cse_rw_fw_version(struct fw_version *cse_version);
wondering if we need dedicated APIs for retrieving each CMOS entries ? […]
Yes, the CMOS API design should be changed to send the entire DS to the user. This will allow the user to retrieve and operate the required CMOS entry. The current CMOS APIs are doing exactly same, but sharing entire DS would be an improvement. I will make the changes shortly.
File src/soc/intel/common/block/cse/cse_lite_cmos.c:
https://review.coreboot.org/c/coreboot/+/74995/comment/559af3d5_fc85bf36 : PS54, Line 74: /* Update checksum over firmware versions */ : info->crc = get_cse_info_crc(info);
shouldn't we update the CRC after the write itself ?
The variable "info" is a temporary variable that stores the data read from the CMOS area. We update the required CSE firmware partition version, calculate the CRC checksum of the updated data, and update the CRC field. Finally, we copy the entire "info" variable to the CMOS area.
So the design is to calculate CRC locally and update it to CMOS.
https://review.coreboot.org/c/coreboot/+/74995/comment/5ca8c442_0a79c3ee : PS54, Line 90: info->crc = get_cse_info_crc(info);
same
The variable "info" is a temporary variable that stores the data read from the CMOS area. We update the required CSE firmware partition version, calculate the CRC checksum of the updated data, and update the CRC field. Finally, we copy the entire "info" variable to the CMOS area.