Attention is currently required from: Arthur Heymans, Nico Huber, Maulik V Vaghela, Mario Scheithauer, Angel Pons, Subrata Banik, Michael Niewöhner, Werner Zeh, Patrick Rudolph, Felix Held. Lean Sheng Tan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55367 )
Change subject: soc/intel/elkhartlake: Introduce Intel PSE ......................................................................
Patch Set 49:
(7 comments)
File src/soc/intel/elkhartlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/55367/comment/de72b8e1_81df7623 PS47, Line 198: config PSE_IMAGE
I would rename it to something like "ENABLE_PSE" or the like since with this switch you decide wheth […]
good suggestion.
https://review.coreboot.org/c/coreboot/+/55367/comment/d76371f7_057a0a11 PS47, Line 211: config PSE_FW_FILE_SIZE : hex "Memory buffer (KiB) for PSE FW image" : default 0x200 : help : It is recommended to allocate at least 512 KiB for PSE FW : : config PSE_CONFIG_BUFFER_SIZE : hex "Memory buffer (KiB) for PSE config data" : default 0x100 : help : It is recommended to allocate at least 256 KiB for PSE config : data (FSP will append PSE config data to memory region right : after PSE FW memory region)
Make these dependent on "PSE_IMAGE so that they disappear if the PSE is disabled.
Done
File src/soc/intel/elkhartlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/55367/comment/92b602d5_fbe64da2 PS48, Line 204: added to CBFS and loaded by FSP to run PSE.
trailing whitespace
Please fix.
File src/soc/intel/elkhartlake/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/55367/comment/33d911fe_20af0d9e PS47, Line 58: ifeq ($(CONFIG_PSE_IMAGE),y) : ifeq ($(call int-gt,\ : $(call file-size,$(CONFIG_PSE_IMAGE_FILE))\ : $(shell printf "%d" $(CONFIG_PSE_FW_FILE_SIZE))),\ : 1)
You are comparing CONFIG_PSE_FW_FILE_SIZE, which in in KB units, to the real size of the PSE file in […]
This is great. Thanks Werner!
File src/soc/intel/elkhartlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/55367/comment/42f43d9c_e44a39d1 PS43, Line 420: params->PchPseShellEnabled = config->PseShellEn;
Well, this is about enabling/disabling it. […]
Thanks Michael. I will still prefer to keep this PSE console shell enabling to mainboard devicetree as this is mainboard related, as some board might choose to build PSE FW without the PSE console shell function.
File src/soc/intel/elkhartlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/55367/comment/b123d673_d36a4a6d PS47, Line 373: #define psebufsize (CONFIG_PSE_FW_FILE_SIZE + CONFIG_PSE_CONFIG_BUFFER_SIZE) * KiB
Move this define somewhere on top of the file? And all defined symbols should be written in upper-ca […]
Removed the define and just added into the line as our conversation suggested.
File src/soc/intel/elkhartlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/55367/comment/c8d639e6_a0b468a6 PS48, Line 373: static char psefwbuf[(CONFIG_PSE_FW_FILE_SIZE +\
Avoid unnecessary line continuations
Please fix.