Attention is currently required from: Arthur Heymans, Nico Huber, Maulik V Vaghela, Paul Menzel, Mario Scheithauer, Subrata Banik, Michael Niewöhner, Lean Sheng Tan, Werner Zeh, Patrick Rudolph, Felix Held. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55367 )
Change subject: soc/intel/elkhartlake: Introduce Intel PSE ......................................................................
Patch Set 52:
(7 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/55367/comment/702c102c_a06cfac8 PS52, Line 9: The Intel® Programmable Services Engine (Intel® PSE) is a : dedicated offload engine for IoT functions powered by an ARM : Cortex-M7 microcontroller. It provides independent, low-DMIPS : computing and low-speed I/Os for IoT applications, plus : dedicated services for real-time computing and time-sensitive : synchronization. : : The PSE hosts new functions, including remote out-of-band : device management, network proxy, embedded controller lite : and sensor hub. : : This CL enables the user to provide the base address of the : PSE FW blob which will then be loaded by the FSP-S onto the : ARM controller. PSE FW will do the initialization work of : PSE controller and it's peripherals. The loading of PSE FW : should have negligible impact on boot time unless PSE : controller could not locate PSE FW and FSP will attempt to : redo PSE FW loading and wait for PSE handshake until it times : out. Once PSE controller locate PSE FW, it will do initialization : concurrently by itself with coreboot booting. : : It also adds PSE related FSP-S UPD settings which enables the : setup of peripheral ownership (assigned to the PSE or x86 : subsystem) and interrupts. These assignments need to take : place at a given point in the boot process and cannot be : changed later. : : To verify if PSE FW is loaded properly, the user could enable : PchPseShellEnabled flag and the log will be printed at PSE UART 2. : : For further info please refer to doc #611825 (for HW overview) : and #614110 (for PSE EDS).
Please reflow for 75 characters per line.
Isn't the limit 72 characters per line?
File src/soc/intel/elkhartlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/55367/comment/bcdba45c_6ea46d09 PS52, Line 202: atom nit: Capitalize `Atom`
https://review.coreboot.org/c/coreboot/+/55367/comment/31adf9fd_f526141e PS52, Line 212: c typo: remove extra `c` in `ex*ecuted`
https://review.coreboot.org/c/coreboot/+/55367/comment/b85651a2_70de4565 PS52, Line 220: config PSE_FW_FILE_SIZE If these values are in KiB, how about adding a `_KIB` suffix to their names? I'd also use the `int` type for these options, as using hex values to specify sizes in KiB feels rather confusing to me. Alternatively, just specify the sizes in bytes using hex values.
File src/soc/intel/elkhartlake/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/55367/comment/208a3f34_2af2746a PS2, Line 58: pse.bin-align := 0x1000
Fine with me!
Looks like FSP copies the PSE FW to RAM, so the alignment of the file in CBFS shouldn't matter. Also, the current code copies the CBFS file into an array, which discards the alignment.
File src/soc/intel/elkhartlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/55367/comment/2d022a1e_e636372c PS52, Line 94: %08x You can use `z` to print a `size_t` variable without any casts. In this case, you can use `%08zx` and drop the `uint32_t` cast before `psefwsize`.
https://review.coreboot.org/c/coreboot/+/55367/comment/38fbe2a2_3db18fb0 PS52, Line 129: /* Set the ownership of these devices to PSE */ : params->PchPseDmaEnable[0] = PSE_Owned; : params->PchPseUartEnable[2] = PSE_Owned; : params->PchPseHsuartEnable[2] = PSE_Owned; : params->PchPseI2cEnable[2] = PSE_Owned; : params->PchPseTimedGpioEnable[0] = PSE_Owned; : params->PchPseTimedGpioEnable[1] = PSE_Owned; : /* Disable PSE DMA Sideband Interrupt for DMA 0 */ : params->PchPseDmaSbInterruptEnable[0] = 0; : /* Set the log output to PSE UART 2 */ : params->PchPseLogOutputChannel = 3; Why does PSE init require these settings? Are they always required independently of the mainboard and PSE firmware being loaded?