Attention is currently required from: Arthur Heymans, Nico Huber, Maulik V Vaghela, Paul Menzel, 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:
(6 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/55367/comment/7c98953e_93747eb3 PS43, Line 36: the user could enable : PchPseShellEnabled flag
If the user shall be able to enable it, make it a Kconfig
Answered below, this is a mainboard related config.
File src/soc/intel/elkhartlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/55367/comment/5fff51a1_3f4c783b PS43, Line 216: It is required to allocate at least 512 KiB for PSE FW and 256 KiB for : PSE config data
Please add a dot/period at the end of the sentence.
Done
https://review.coreboot.org/c/coreboot/+/55367/comment/7d2f221e_c0b22b12 PS43, Line 217: FSP will append PSE config data to memory region : right after PSE FW memory region
Please add a dot/period at the end of the sentence.
Done
File src/soc/intel/elkhartlake/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/55367/comment/db7fd54b_ab9aeec5 PS43, Line 57: pse.bin-align := 0x1000 : pse.bin-compression := lzma
We had this discussion already earlier and I do not see a real reason for this alignment as well.
Thanks for the input. Done.
File src/soc/intel/elkhartlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/55367/comment/53bac03c_c4a5c80b PS2, Line 480: : params->PchPseDmaEnable[0] = PSE_Owned; : params->PchPseDmaSbInterruptEnable[0] = 0x0; : params->PchPseUartEnable[2] = PSE_Owned; : params->PchPseHsuartEnable[2] = PSE_Owned; : params->PchPseLogOutputChannel = 0x3; //Set the log output to PSE UART 2 : params->PchPseI2cEnable[2] = PSE_Owned; : params->PchPseTimedGpioEnable[0] = PSE_Owned; : params->PchPseTimedGpioEnable[1] = PSE_Owned; : params->PchPseTimedGpioPinEnable[5] = 1; : params->PchPseTimedGpioPinEnable[6] = 1; : params->PchPseTimedGpioPinAllocation[0] = 0x0; //Set to top 20 TGPIO pins : params->PchPseTimedGpioPinAllocation[1] = 0x0; //Set to top 20 TGPIO pins : : /* : * Set to predefined GPIO Pin Muxing. If the device is disabled, it : * will not be consumed by FSP. : */ : params->PchPseTgpio6PinMux = 0x8B81A203; //GPIO pin mux for GPP_T3 : params->PchPseTgpio7PinMux = 0x8B82A40B; //GPIO pin mux for GPP_T11 : params->PchPseTgpio8PinMux = 0x8B80A607; //GPIO pin mux for GPP_B7 : params->PchPseTgpio9PinMux = 0x8B80A808; //GPIO pin mux for GPP_B8 : params->PchPseTgpio10PinMux = 0x8B86AA07; //GPIO pin mux for GPP_U7 : params->PchPseTgpio11PinMux = 0x8B86AC0B; //GPIO pin mux for GPP_U11 : params->PchPseTgpio12PinMux = 0x8B86AE13; //GPIO pin mux for GPP_U19 : params->PchPseTgpio13PinMux = 0x8B85B00C; //GPIO pin mux for GPP_D12 : params->PchPseTgpio14PinMux = 0x8B90B214; //GPIO pin mux for GPP_E20 : params->PchPseTgpio15PinMux = 0x8B90B403; //GPIO pin mux for GPP_E3 : params->PchPseTgpio16PinMux = 0x8B90B607; //GPIO pin mux for GPP_E7 : params->PchPseTgpio17PinMux = 0x8B90B80F; //GPIO pin mux for GPP_E15 : params->PchPseTgpio18PinMux = 0x8B90BA06; //GPIO pin mux for GPP_E6 : params->PchPseTgpio19PinMux = 0x8B8DBC01; //GPIO pin mux for GPP_C1 : params->PchPsePwmPinMux[8] = 0x7B706604; //GPIO pin mux for GPP_E4 : params->PchPsePwmPinMux[9] = 0x7B706805; //GPIO pin mux for GPP_E5 : params->PchPsePwmPinMux[10] = 0x7B706A06; //GPIO pin mux for GPP_E6 : params->PchPsePwmPinMux[11] = 0x7B706C11; //GPIO pin mux for GPP_E17 : params->PchPsePwmPinMux[12] = 0x7B706E12; //GPIO pin mux for GPP_E18 : params->PchPsePwmPinMux[13] = 0x7B707013; //GPIO pin mux for GPP_E19 : params->PchPsePwmPinMux[14] = 0x4B64720A; //GPIO pin mux for GPP_H10 : params->PchPsePwmPinMux[15] = 0x4B64740B; //GPIO pin mux for GPP_H11 : params->PchPseSpiMosiPinMux[1] = 0x4B852003; //GPIO pin mux for GPP_D3 : params->PchPseSpiMisoPinMux[1] = 0x4B853002; //GPIO pin mux for GPP_D2 : params->PchPseSpiClkPinMux[1] = 0x4B854001; //GPIO pin mux for GPP_D1 : params->PchPseSpiCs0PinMux[1] = 0x4B855000; //GPIO pin mux for GPP_D0 : params->PchPseI2sTxPinMux[0] = 0x1B702610; //GPIO pin mux for GPP_E16 : params->PchPseI2sRxPinMux[0] = 0x1B70160F; //GPIO pin mux for GPP_E15 : params->PchPseI2sSfrmPinMux[0] = 0x1B704614; //GPIO pin mux for GPP_E21 : params->PchPseI2sSclkPinMux[0] = 0x1B703615; //GPIO pin mux for GPP_E20
What about requesting GpioOverride from the FSP team?
The EHL FSP is unfortunately wont have extra features or modifications unless bug fixes :(
File src/soc/intel/elkhartlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/55367/comment/722d11cc_6773e54c PS43, Line 373: CONFIG_PSE_BUFFER_SIZE
beware that the file-size function currently has a bug where it can get stuck when an empty string […]
Wow great input here guys. Thanks Werner, Arther, Felix and Angel for your kind review! :)
-
Lean Sheng Tan (Code Review)