Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46312 )
Change subject: soc/intel/common: Move CSE RW into new FMAP region to optimize boot time ......................................................................
Patch Set 11:
(3 comments)
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse_lite.c:
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 49: = 1,
True, I assigned numbers manually so as to quickly identify value to enum definition.
Ack.
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 601: : if (source_info->fw_ver.major != cse_rw_ver->major) : return source_info->fw_ver.major - cse_rw_ver->major; : else if (source_info->fw_ver.minor != cse_rw_ver->minor) : return source_info->fw_ver.minor - cse_rw_ver->minor; : else if (source_info->fw_ver.hotfix != cse_rw_ver->hotfix) : return source_info->fw_ver.hotfix - cse_rw_ver->hotfix; : else : return source_info->fw_ver.build - cse_rw_ver->build;
memcmp() does a byte wise comparison.So, it doesn't work here. […]
Ack. My bad.
https://review.coreboot.org/c/coreboot/+/46312/10/src/soc/intel/common/block... PS10, Line 764: *source_info
source_info or source_metadata?
I think it would be good to have "metadata" in the name just to make it easier to relate that this is cse_rw_metadata structure. Maybe go with source_metadata?