Attention is currently required from: Arthur Heymans, Nico Huber, Maulik V Vaghela, Paul Menzel, Mario Scheithauer, Angel Pons, Subrata Banik, Michael Niewöhner, Lean Sheng Tan, Patrick Rudolph, Felix Held. Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55367 )
Change subject: soc/intel/elkhartlake: Introduce Intel PSE ......................................................................
Patch Set 49:
(4 comments)
File src/soc/intel/elkhartlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/55367/comment/0c7d60a7_624557cc PS49, Line 204: added to CBFS and loaded by FSP to run PSE.
Please add a sentence that this is only a temporary solution.
"A PSE FW binary is required be added to CBFS and loaded by FSP to run PSE." Why would that be a temporary solution? If you need to use some of the mentioned IPs, loading PSE is not an option. And it does not matter where the image comes from (closed blob or fully open source), it needs to be loaded if you need the peripherals in question!
I would better see that that context is added here instead of calling it "temporary".
File src/soc/intel/elkhartlake/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/55367/comment/f4201795_dc010c17 PS49, Line 66:
The way this is implemented (if PSE_ENABLE then the file is required) does […]
The issue with your approach would be the size checking. Right now we do check the size of the real firmware image against the reserved space at build time and throw an error if it does not fit. How should this be handled if we add the image later on with cbfstool?
File src/soc/intel/elkhartlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/55367/comment/88addc41_095067d4 PS49, Line 385: FSP_ARRAY_LOAD
What is this? It hides the implementation. Hiding memcpy() seems dangerous […]
I do not see exactly how the macro itself hides the types from memcpy(). From gcc's point of view, after the preprocessor has done its job, the code looks like it was without the macro. And in addition we have '-Werror=sizeof-pointer-div' enabled so that using just two pointers as parameters to the macro will be caught.
Could you please elaborate your concerns here a bit more so that it is more clear here?
https://review.coreboot.org/c/coreboot/+/55367/comment/dc3f943a_2c140e5d PS49, Line 426: * as it still allows user to change these params from devicetree.
That's not true. The devicetree can't change it because the devicetree values […]
Yes, this could be written more precise her. What actually is meant, and is my concern as well, is that the user could set the parameters in the devicetree and they will be overwritten here. So we need a better way (other than ripping off the arrays) to do it here, or at least throw a warning (compile-time would be preferred) that a value set up in devicetree is overwritten here. This is the TODO: here.
-
Werner Zeh (Code Review)