Attention is currently required from: Arthur Heymans, Martin Roth, Maulik V Vaghela, Mario Scheithauer, Angel Pons, Subrata Banik, Michael Niewöhner, Lean Sheng Tan, Patrick Rudolph, Felix Held. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55367 )
Change subject: soc/intel/elkhartlake: Introduce Intel PSE ......................................................................
Patch Set 58:
(13 comments)
Patchset:
PS58: Sorry for the long silence. I've spotted a few minor things that I think should be addressed.
Irritating comments and everything looking like vendorcode seems to be accepted in soc/intel/elkhartlake so I marked these as resolved (should be fixed but probably with a time machine to not avoid review of the initial bring up).
File src/soc/intel/elkhartlake/chip.h:
https://review.coreboot.org/c/coreboot/+/55367/comment/57a00837_daff0602 PS58, Line 477: hidden from coreboot NB. This is only true if you consider the PSE firmware foreign and not part of coreboot. Which is the current kludge and should be the exception IMHO.
https://review.coreboot.org/c/coreboot/+/55367/comment/305fb7a5_18dadb9f PS58, Line 497: sideband What's sideband in this context?
https://review.coreboot.org/c/coreboot/+/55367/comment/cfd387a2_3785bea1 PS58, Line 497: Disable (0) / Enable (1) disable (false) / enable (true)
File src/soc/intel/elkhartlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/55367/comment/38bead9f_c64fbee9 PS58, Line 86: static char psefwbuf[(CONFIG_PSE_FW_FILE_SIZE + line break looks ugly and unnecessary
https://review.coreboot.org/c/coreboot/+/55367/comment/cfe3d926_f426c520 PS58, Line 87: KiB If these Kconfig values are scaled to KiB, it would be nice to have that in their names to avoid mistakes, e.g. PSE_CONFIG_BUFFER_SIZE_KIB.
https://review.coreboot.org/c/coreboot/+/55367/comment/9a16da08_dec88af5 PS58, Line 91: uint32_t Pointers should always be casted to `uintptr_t` first. The compiler could warn about different sizes otherwise.
This also shows that there is a requirement (by FSP? by the PSE?) to have the firmware in 32-bit space. I guess our CBFS API has no notion for that. So I'd leave a comment at the cbfs_load() call that we have this requirement.
https://review.coreboot.org/c/coreboot/+/55367/comment/169cb7fb_e7205dac PS58, Line 126: * as the settings from devicetree are overwritten here. I still don't understand why we offer the devicetree settings in the first place. This TODO wouldn't be necessary if there was less config clutter.
https://review.coreboot.org/c/coreboot/+/55367/comment/79e3e81e_39726d2e PS58, Line 130: * much better. So it's definitely a requirement by the current PSE FW? If so, why isn't that stated? If so, why does the Kconfig help text not mentioned that a very specific binary is expected?
https://review.coreboot.org/c/coreboot/+/55367/comment/ba6d587a_28c48605 PS58, Line 143: die("PSE enabled but PSE FW not available!\n"); Is it really impossible to go on?
https://review.coreboot.org/c/coreboot/+/55367/comment/b638ca17_f815ce9f PS58, Line 436: && cbfs_file_exists("pse.bin") Isn't this mixed up with the else path inside fill_fsps_pse_params()? The current, hard-to-read flow seems to be this:
if file exists and can be loaded, configure if file exists but can't be loaded, die saying the fw is not available if file doesn't exist, go silently on
Is that the case? Is that intentional?
File src/soc/intel/elkhartlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/55367/comment/9925aff6_03d1c15b PS49, Line 127: m_cfg->PchPseEnable = CONFIG(PSE_ENABLE);
Hi Nico, added the handler cbfs_file_exists() per your advice. […]
Thanks!
File src/soc/intel/elkhartlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/55367/comment/795a9fb8_6dbeee79 PS58, Line 127: !!( What's the !! for? The result of && is already boolean.