[L] Change in coreboot[master]: soc/intel/alderlake/hsphy: Add possibility to cache HSPHY in flash
Attention is currently required from: Tarun Tuli, Martin L Roth, Subrata Banik, Paul Menzel, Krystian Hebel.
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68987 )
Change subject: soc/intel/alderlake/hsphy: Add possibility to cache HSPHY in flash ......................................................................
Patch Set 4:
(7 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/68987/comment/57a18d7e_82840dcf PS3, Line 13: This change allows to : keep PCIe 5.0 functioning even if CSME/HECI is not functional.
Does it still work after adding or removing PCIe devices?
It does not depend on the PCIe presence. Just the root port has to be enabled so that coreboot attempts to load the HSPHY
File src/soc/intel/alderlake/hsphy.c:
https://review.coreboot.org/c/coreboot/+/68987/comment/383efcfd_dd14b829 PS3, Line 222: printk(BIOS_ERR, "Invalid parameters, HSPHY will not be cached in flash.\n");
I think this misses a return.
Indeed, fixed.
https://review.coreboot.org/c/coreboot/+/68987/comment/69e8d5d1_3aea736b PS3, Line 233: if (hsphy_cache_valid(hsphy_fw_cache)) {
Update is skipped if content of `buf` is different than cached version, is this the expected behavio […]
I have changed the flow to try HECI command so we can fetch the newest HSPHY FW and cache it if needed. Also laoding from cache is attempted if any fail in the normal flow occurs.
https://review.coreboot.org/c/coreboot/+/68987/comment/fb525154_858a9745 PS3, Line 234: printk(BIOS_INFO, "HSPHY: HSPHY cache valid, skipping update\n");
How can this happen? In line 313 the firmware is loaded from cache, isn’t it. […]
Indeed, Krystian also pointed it out. I have changed the flow to try HECI command so we can fetch the newest HSPHY FW and cache it if needed. Also laoding from cache is attempted if any fail in the normal flow occurs.
https://review.coreboot.org/c/coreboot/+/68987/comment/794f2cc2_4e002440 PS3, Line 240: printk(BIOS_ERR, "HSPHY: HSPHY_FW region too small\n");
Please also log the size values.
Done
https://review.coreboot.org/c/coreboot/+/68987/comment/12030b22_f4c44399 PS3, Line 246: hsphy_fw_cache = malloc(sizeof(*hsphy_fw_cache));
No `rdev_munmap` before `hsphy_fw_cache` is overwritten.
Yup, fixed
https://review.coreboot.org/c/coreboot/+/68987/comment/ed3a6bc4_dd173636 PS3, Line 314: return;
I’d log the successful loading in this location.
Done
-
Michał Żygowski (Code Review)