Attention is currently required from: Arthur Heymans, Maulik V Vaghela, Paul Menzel, Mario Scheithauer, Angel Pons, Subrata Banik, Lean Sheng Tan, Werner Zeh, 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 49:
(18 comments)
Patchset:
PS49:
From a technical point of view this patch looks good so far.
Bold words, made me look ;)
File src/soc/intel/elkhartlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/55367/comment/8a65e7e6_86274cf9 PS49, Line 202: that is designed as an Asymmetric : Multi-Processing (AMP) system AIUI, Intel describes the whole SoC as AMP. Some cores are in the CPU complex and x86 and the PSE is in the PCH and ARM, hence asymmetric. Or did I get it wrong?
https://review.coreboot.org/c/coreboot/+/55367/comment/4e37deea_31cd7260 PS49, Line 203: be *to* be?
https://review.coreboot.org/c/coreboot/+/55367/comment/5515618a_39677622 PS49, Line 204: added to CBFS and loaded by FSP to run PSE. Please add a sentence that this is only a temporary solution.
File src/soc/intel/elkhartlake/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/55367/comment/3435adf3_d40fa05b PS49, Line 52: CONFIG_PSE_IMAGE Did you mean CONFIG_PSE_ENABLE?
https://review.coreboot.org/c/coreboot/+/55367/comment/34037902_bc93f3d1 PS49, Line 61: $(shell printf "%d" $(call int-shift-left,\ This looks like there is no line break necessary?
https://review.coreboot.org/c/coreboot/+/55367/comment/5655ccdf_8192c952 PS49, Line 66: The way this is implemented (if PSE_ENABLE then the file is required) does not allow to enable the PSE and adding the file manually with cbfstool, and neither to build test the option (without the file in the repository).
The latter needs to be fixed.
Also, it would make it impossible for a board port to provide correct Kconfig defaults when, for instance, the PSE is needed to make the MACs work.
The usually solution is to have a separate option to add the file and print a warning at the end of the build when this option wasn't set.
File src/soc/intel/elkhartlake/chip.h:
https://review.coreboot.org/c/coreboot/+/55367/comment/f4250d06_dff6faa4 PS49, Line 482: runtime Runtime of coreboot or the whole time, i.e. OS?
https://review.coreboot.org/c/coreboot/+/55367/comment/c57a5c63_8e14283c PS49, Line 485: enum pse_device_ownership PseDmaOwn[3]; : enum pse_device_ownership PseUartOwn[6]; : enum pse_device_ownership PseHsuartOwn[4]; : enum pse_device_ownership PseQepOwn[4]; : enum pse_device_ownership PseI2cOwn[8]; : enum pse_device_ownership PseI2sOwn[2]; : enum pse_device_ownership PseSpiOwn[4]; : enum pse_device_ownership PseSpiCs0Own[4]; : enum pse_device_ownership PseSpiCs1Own[4]; : enum pse_device_ownership PseCanOwn[2]; : enum pse_device_ownership PsePwmOwn; : enum pse_device_ownership PseAdcOwn; : /* PSE devices sideband interrupt: Disable (0) / Enable (1) */ : bool PseDmaSbIntEn[3]; : bool PseUartSbIntEn[6]; : bool PseQepSbIntEn[4]; : bool PseI2cSbIntEn[8]; : bool PseI2sSbIntEn[2]; : bool PseSpiSbIntEn[4]; : bool PseCanSbIntEn[2]; : bool PseLh2PseSbIntEn; : bool PsePwmSbIntEn; : bool PseAdcSbIntEn; : /* PSE PWM native function: Disable (0) / Enable (1) */ : bool PsePwmPinEn[16]; : /* PSE Console Shell */ : bool PseShellEn; There shouldn't be any CamelCase. But it looks like half of the file does it wrong already? I guess one of the downsides of not having the initial commit reviewed.
File src/soc/intel/elkhartlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/55367/comment/01ba5a62_65bacf72 PS49, Line 383: /* PSE devices config */ I don't understand this comment.
https://review.coreboot.org/c/coreboot/+/55367/comment/a516cce0_7cb492c2 PS49, Line 384: /* PSE DMA */ That's literally what the indentifier says, such a comment is just distraction.
https://review.coreboot.org/c/coreboot/+/55367/comment/dce38d4a_cd1ea496 PS49, Line 385: FSP_ARRAY_LOAD What is this? It hides the implementation. Hiding memcpy() seems dangerous as it discards all type checking.
https://review.coreboot.org/c/coreboot/+/55367/comment/2f604b0d_7005bc8a PS49, Line 423: Nit, missing line break after the /*
https://review.coreboot.org/c/coreboot/+/55367/comment/81ac00e8_a4b3ffe1 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 are overridden here, AFAICS.
Where is this "minimum requirement" documented? Is it specific to the blob you used to test it?
https://review.coreboot.org/c/coreboot/+/55367/comment/2586a640_c62819f1 PS49, Line 436: 0x0 Why is this hex?
https://review.coreboot.org/c/coreboot/+/55367/comment/f99dd5b5_f0aecc8b PS49, Line 438: 0x3 Why is this hex?
https://review.coreboot.org/c/coreboot/+/55367/comment/3c4d08c8_72dfddf3 PS49, Line 438: // Please use common comment style.
File src/soc/intel/elkhartlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/55367/comment/35b12bee_b4218549 PS49, Line 127: m_cfg->PchPseEnable = CONFIG(PSE_ENABLE); Could we check for the file in CBFS here, so we could still boot if it's missing?
-
Nico Huber (Code Review)