Attention is currently required from: Cliff Huang, Kapil Porwal, Pranava Y N, Saurabh Mishra.
Subrata Banik has posted comments on this change by Saurabh Mishra. ( https://review.coreboot.org/c/coreboot/+/83798?usp=email )
Change subject: soc/intel/ptl: Do initial Panther Lake SoC commit till ramstage ......................................................................
Patch Set 38:
(24 comments)
File src/soc/intel/pantherlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/83798/comment/13704454_68bad09c?usp... : PS38, Line 2: we need to include below configs
1. FSP_UGOP_EARLY_SIGN_OF_LIFE 2. HAVE_INTEL_COMPLIANCE_TEST_MODE 3. PCIE_CLOCK_CONTROL_THROUGH_P2SB 4. SOC_INTEL_COMMON_BLOCK_ACPI_CPU_HYBRID 5. SOC_INTEL_CRASHLOG 6. X86_INIT_NEED_1_SIPI 7. INTEL_KEYLOCKER
https://review.coreboot.org/c/coreboot/+/83798/comment/e64a2db7_b597cc59?usp... : PS38, Line 86: SOC_INTEL_ENABLE_USB4_PCIE_RESOURCES this was always a main board selectable item previously. any reason to modify that ?
https://review.coreboot.org/c/coreboot/+/83798/comment/eff184ef_d484eac2?usp... : PS38, Line 401: endif why are you overlooking https://review.coreboot.org/c/coreboot/+/74167
File src/soc/intel/pantherlake/cpu.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/2e7b9cdc_4c8712d1?usp... : PS38, Line 78: msr.lo |= (1 << 23); /* Lock it */ wondering why did you overlooked https://review.coreboot.org/c/coreboot/+/74155 CL (which was present inside MTL but missed in PTL copy CL).
again this is difficult for me to follow unless you do a comparison yourself while submitting changes for PTL against MTL to reflect the delta.
I have confirmed looking at PTL spec that bit 18 of MSR 0x1fc is required
https://review.coreboot.org/c/coreboot/+/83798/comment/984e435f_61d56bd2?usp... : PS38, Line 123: Why are you overlooking https://review.coreboot.org/c/coreboot/+/74154 as well ?
https://review.coreboot.org/c/coreboot/+/83798/comment/0811774a_9382c6d7?usp... : PS38, Line 129: } you have also overlooked all below CLs for PTL
1. https://review.coreboot.org/c/coreboot/+/74159/5 2. https://review.coreboot.org/c/coreboot/+/74159/5 3. https://review.coreboot.org/c/coreboot/+/74161 4. https://review.coreboot.org/c/coreboot/+/74162 5. https://review.coreboot.org/c/coreboot/+/74168
https://review.coreboot.org/c/coreboot/+/83798/comment/79a2e72b_b659de7e?usp... : PS38, Line 175: one more
https://review.coreboot.org/c/coreboot/+/80567
File src/soc/intel/pantherlake/crashlog.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/f104b0e6_8424b3b2?usp... : PS38, Line 15: please adopt 64-bit support for Crashlog like Intel added sometime back
https://review.coreboot.org/c/coreboot/+/83106
As I have stated on several occasions, your code is not up-to-date and you are attempting to submit an outdated fork of MTL in the form of a PTL. Please refrain from doing so.
https://review.coreboot.org/c/coreboot/+/83798/comment/be26f9ac_ff692505?usp... : PS38, Line 59: u32 this has been fixed in past using https://review.coreboot.org/c/coreboot/+/83106
File src/soc/intel/pantherlake/elog.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/baada89a_fd4da7bb?usp... : PS38, Line 43: { PCI_DEVFN_PCIE6, ELOG_WAKE_SOURCE_PME_PCIE6 }, why RP7 and RP8 are not included in this list?
https://review.coreboot.org/c/coreboot/+/83798/comment/6ec3bedd_374d7a98?usp... : PS38, Line 46: { PCI_DEVFN_PCIE11, ELOG_WAKE_SOURCE_PME_PCIE11 }, : { PCI_DEVFN_PCIE12, ELOG_WAKE_SOURCE_PME_PCIE12 }, don't you need SOC_INTEL_PANTHERLAKE_H guard to avoid > 10 RP inclusion
File src/soc/intel/pantherlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/5b7d28d7_bfb499a5?usp... : PS38, Line 2: please follow something like you did for romstage/fsp_params.c. Unless required, you just don't add dummy functions
File src/soc/intel/pantherlake/gspi.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/8f9a13fb_73475dd6?usp... : PS38, Line 15: } You have mentioned total 3 GSPI controllers are there then why only entries for 2 of them ?
``` config SOC_INTEL_COMMON_BLOCK_GSPI_MAX int default 3 ```
File src/soc/intel/pantherlake/include/soc/crashlog.h:
https://review.coreboot.org/c/coreboot/+/83798/comment/28c92ffd_5b20a8bc?usp... : PS38, Line 22: please follow https://review.coreboot.org/c/coreboot/+/77239
File src/soc/intel/pantherlake/include/soc/dptf.h:
https://review.coreboot.org/c/coreboot/+/83798/comment/264a2e03_9c7df0a4?usp... : PS38, Line 14: don't we need below devices
``` DPTF_TPCH_DEVICE DPTF_TPWR_DEVICE DPTF_BAT1_DEVICE ```
File src/soc/intel/pantherlake/include/soc/espi.h:
https://review.coreboot.org/c/coreboot/+/83798/comment/91ba76dd_6a7486bd?usp... : PS38, Line 5: can you please follow https://review.coreboot.org/c/coreboot/+/83487/4 where we have switched to common eSPI.h over soc specific ones
File src/soc/intel/pantherlake/include/soc/irq.h:
https://review.coreboot.org/c/coreboot/+/83798/comment/f3627b41_30e11d88?usp... : PS38, Line 3: _SOC_IRQ_H_ _SOC_PANTHERLAKE_IRQ_H_ ?
https://review.coreboot.org/c/coreboot/+/83798/comment/f6055416_b03dbed5?usp... : PS38, Line 12: #define LPSS_UART0_IRQ 16 : #define LPSS_UART1_IRQ 17 : #define LPSS_UART2_IRQ 31 I don't see a consumer .
File src/soc/intel/pantherlake/include/soc/me.h:
https://review.coreboot.org/c/coreboot/+/83798/comment/74641f16_ac3fb122?usp... : PS38, Line 5: please follow https://review.coreboot.org/c/coreboot/+/73137/10 (more than one year old code). not sure how old is the base source of PTL
File src/soc/intel/pantherlake/include/soc/pcie.h:
https://review.coreboot.org/c/coreboot/+/83798/comment/d6f14914_d165ab6c?usp... : PS38, Line 9: for sure we need get_tbt_pcie_rp_table
please refer to https://review.coreboot.org/c/coreboot/+/81841
File src/soc/intel/pantherlake/include/soc/pmc.h:
https://review.coreboot.org/c/coreboot/+/83798/comment/a751b018_72331077?usp... : PS38, Line 26: (1 << 18) why ? what is the problem with BIT(18) macro ?
File src/soc/intel/pantherlake/include/soc/serialio.h:
https://review.coreboot.org/c/coreboot/+/83798/comment/405c0769_b3bfd973?usp... : PS38, Line 23: enum { : PchSerialIoIndexI3C0, : PchSerialIoIndexI3C1 : }; we don't need as mentioned earlier
File src/soc/intel/pantherlake/include/soc/tcss.h:
https://review.coreboot.org/c/coreboot/+/83798/comment/d8357e44_fd54e9a2?usp... : PS38, Line 16: /* : * The PCI-SIG engineering change requirement provides the ACPI additions for firmware latency : * optimization. Both of FW_RESET_TIME and FW_D3HOT_TO_D0_TIME are applicable to the upstream : * port of the USB4/TBT topology. : */ : /* Number of microseconds to wait after a conventional reset */ : #define FW_RESET_TIME 50000 : : /* Number of microseconds to wait after data link layer active report */ : #define FW_DL_UP_TIME 1 : : /* Number of microseconds to wait after a function level reset */ : #define FW_FLR_RESET_TIME 1 : : /* Number of microseconds to wait from D3 hot to D0 transition */ : #define FW_D3HOT_TO_D0_TIME 50000 : : /* Number of microseconds to wait after setting the VF enable bit */ : #define FW_VF_ENABLE_TIME 1 already existed in common code https://github.com/coreboot/coreboot/blob/main/src/soc/intel/common/block/in...
File src/soc/intel/pantherlake/include/soc/usb.h:
https://review.coreboot.org/c/coreboot/+/83798/comment/b9d141f4_ddedf0d7?usp... : PS38, Line 34: }; please include https://review.coreboot.org/c/coreboot/+/82730