Attention is currently required from: Tim Wawrzynczak, Krishna P Bhat D. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59685 )
Change subject: soc/intel/common: Add support for CSE IOM/NPHY sub-parition update ......................................................................
Patch Set 16:
(4 comments)
File src/soc/intel/common/block/cse/cse_lite.c:
https://review.coreboot.org/c/coreboot/+/59685/comment/1b531fde_27e58673 PS16, Line 405: : __weak bool skip_cse_sub_part_update(void) : { : return false; : }
nit; I don't think this weak function definition is still required.
I think this makes sense if implementing this function is a rare edge case and most future SoCs using this feature are not expected to need it. My assumption here (without knowing the background) was that the Alderlake thing was just that the earlier stepping doesn't support this because it's the first chip with it or something, but all future SoC generations would want to use it without requiring an extra runtime check. Is that roughly the idea or am I way off?
If we expect that the majority of future chips will need to override this for some reason, I agree it shouldn't be weak.
https://review.coreboot.org/c/coreboot/+/59685/comment/74f42618_d047394d PS16, Line 805: Why not just do rdev_mmap_full() and reuse cse_sub_part_get_source_fw_version() instead of reimplementing everything?
https://review.coreboot.org/c/coreboot/+/59685/comment/f03464a2_b9927256 PS16, Line 959: /* If SOC is not Alderlake A2, skip update */ This is generic code, so it shouldn't reference Alderlake. Just say /* SoCs can implement this if they want to decide whether to apply sub part update at runtime. */ or something.
File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/59685/comment/15920da5_b6a39018 PS16, Line 343: * Returns true if cse sub-parition update is required otherwise false. This should clarify that this is a weak function and what the default behavior is if the platform doesn't implement it.