Change in coreboot[master]: soc/intel/elkhartlake: Introduce Intel PSE

[Werner Zeh (Code Review)]

Attention is currently required from: Arthur Heymans, Nico Huber, Maulik V Vaghela, 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 47:

(4 comments)

File src/soc/intel/elkhartlake/Kconfig:

https://review.coreboot.org/c/coreboot/+/55367/comment/116f2844_244cc1c1 
PS47, Line 198: config PSE_IMAGE
I would rename it to something like "ENABLE_PSE" or the like since with this switch you decide whether to enable or disable the PSE globally.

File src/soc/intel/elkhartlake/Makefile.inc:

https://review.coreboot.org/c/coreboot/+/55367/comment/83fc5d03_1fe65859 
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 byte units. This will never fit.
Instead, you need to multiply the CONFIG_PSE_FW_FILE_SIZE by 1024 to get the byte sized buffer. Something the like should work:

ifeq ($(call int-gt,\
    $(call file-size,$(CONFIG_PSE_IMAGE_FILE))\
    $(shell printf "%d" $(call int-shift-left, $(CONFIG_PSE_FW_FILE_SIZE) 10))),\
    1)
$(error PSE binary larger than CONFIG_PSE_FW_FILE_SIZE.)
endif

File src/soc/intel/elkhartlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/55367/comment/e9ba74da_66eaa088 
PS43, Line 420: params->PchPseShellEnabled = config->PseShellEn;
> What's a "PSE shell"?

The PSE firmware can (and do in the provided images) have it's own shell on a given, PSE assigned UART. I guess this is more mainboard related and is fine to have the devicetree entry for as it varies from board to board (which UART is used, is it routed, ...?)

File src/soc/intel/elkhartlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/55367/comment/1a2a347f_37b44829 
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-case, so PSE_BUF_SIZE would be right and easy to read.