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:
(21 comments)
File src/soc/intel/pantherlake/acpi.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/5b8c2f81_6c2c48c8?usp... : PS38, Line 268: current += acpi_create_dmar_ds_pci(current, 0, PCI_DEV_SLOT_IAA, 0); are we always planning to keep IPU enabled ? if not then you also need a `devfn` check for IAA?
File src/soc/intel/pantherlake/chip.h:
https://review.coreboot.org/c/coreboot/+/83798/comment/8a32043a_9a850948?usp... : PS38, Line 63: you might need this now ?
``` /* * The Max Pkg Cstate * Values 0 - C0/C1, 1 - C2, 2 - C3, 3 - C6, 4 - C7, 5 - C7S, 6 - C8, 7 - C9, 8 - C10, * 254 - CPU Default , 255 - Auto. */ enum pkgcstate_limit { LIMIT_C0_C1 = 0, LIMIT_C2 = 1, LIMIT_C3 = 2, LIMIT_C6 = 3, LIMIT_C7 = 4, LIMIT_C7S = 5, LIMIT_C8 = 6, LIMIT_C9 = 7, LIMIT_C10 = 8, LIMIT_CPUDEFAULT = 254, LIMIT_AUTO = 255, }; ```
https://review.coreboot.org/c/coreboot/+/83798/comment/53b1cf9c_e665835a?usp... : PS38, Line 103: TcssD3ColdDisable why camel case ?
https://review.coreboot.org/c/coreboot/+/83798/comment/bd5ac791_c8dcfa38?usp... : PS38, Line 127: SaGv_Disabled, : SaGv_FixedPoint0, : SaGv_FixedPoint1, : SaGv_FixedPoint2, : SaGv_FixedPoint3, : SaGv_Enabled, please use all capital letter for enum
https://review.coreboot.org/c/coreboot/+/83798/comment/1594f9f2_c954aafe?usp... : PS38, Line 212: HeciEnabled why camel case ?
https://review.coreboot.org/c/coreboot/+/83798/comment/f0d2f900_88fd1b2e?usp... : PS38, Line 219: PmTimerDisabled same
https://review.coreboot.org/c/coreboot/+/83798/comment/3b3ac6a5_971c3b53?usp... : PS38, Line 228: uint8_t SerialIoI2cMode[CONFIG_SOC_INTEL_I2C_DEV_MAX]; : uint8_t SerialIoGSpiMode[CONFIG_SOC_INTEL_COMMON_BLOCK_GSPI_MAX]; : uint8_t SerialIoUartMode[CONFIG_SOC_INTEL_UART_DEV_MAX]; same
https://review.coreboot.org/c/coreboot/+/83798/comment/075efb3b_5b27dc2d?usp... : PS38, Line 235: */ : uint8_t SerialIoGSpiCsMode[CONFIG_SOC_INTEL_COMMON_BLOCK_GSPI_MAX]; : /* : * GSPIn Default Chip Select State: : * 0: Low, : * 1: High : */ : uint8_t SerialIoGSpiCsState[CONFIG_SOC_INTEL_COMMON_BLOCK_GSPI_MAX]; same
https://review.coreboot.org/c/coreboot/+/83798/comment/6f938530_49c405cc?usp... : PS38, Line 308: HybridStorageMode same
https://review.coreboot.org/c/coreboot/+/83798/comment/31cb7684_9b122d21?usp... : PS38, Line 327: DmiPwrOptimizeDisable same
https://review.coreboot.org/c/coreboot/+/83798/comment/8d37c95d_909a18ca?usp... : PS38, Line 339: IsaSerialUartBase same
File src/soc/intel/pantherlake/chip.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/4b65160c_4075ce2b?usp... : PS38, Line 70: case PCI_DEVFN_TCSS_XHCI: return "TXHC"; why IGD is missing here from ACPI name space ?
https://review.coreboot.org/c/coreboot/+/83798/comment/608cf646_62738d0a?usp... : PS38, Line 78: NPU0 why NPU0? the limit is 4 char max but you can keep < 4 as well.
there is nothing called NPU1 hence, better to keep NPU as ACPI name space entry
https://review.coreboot.org/c/coreboot/+/83798/comment/a47c25e8_7a341e41?usp... : PS38, Line 79: IPU0 same, why IPU0 and not IPU
https://review.coreboot.org/c/coreboot/+/83798/comment/d604a1fd_07e46dc9?usp... : PS38, Line 80: case PCI_DEVFN_I3C1: return "I3C1"; : case PCI_DEVFN_I3C2: return "I3C2"; as discussed previously, we don't need now
https://review.coreboot.org/c/coreboot/+/83798/comment/01d8e4c1_545cb1d6?usp... : PS38, Line 115: case PCI_DEVFN_IGD: return "GFX0"; why here ?
https://review.coreboot.org/c/coreboot/+/83798/comment/d9059c93_262b93a7?usp... : PS38, Line 122: please use from MTL code
``` #if CONFIG(SOC_INTEL_STORE_ISH_FW_VERSION) /* SoC override API to identify if ISH Firmware existed inside CSE FPT */ bool soc_is_ish_partition_enabled(void) { struct device *ish = pcidev_path_on_root(PCI_DEVFN_ISH); uint16_t ish_pci_id = ish ? pci_read_config16(ish, PCI_DEVICE_ID) : 0xFFFF;
if (ish_pci_id == 0xFFFF) return false;
return true; } #endif ```
https://review.coreboot.org/c/coreboot/+/83798/comment/b30d734d_d8f9276c?usp... : PS38, Line 139: /* Perform silicon specific init. */ looks like you have skipped tbt_authentication and soc_enable_tracehub entries
https://review.coreboot.org/c/coreboot/+/83798/comment/b765aa17_d30fb567?usp... : PS38, Line 148: pcie_rp_update_devicetree(get_pcie_rp_table()); please use
``` /* Swap enabled TBT root ports in device tree if needed. */ pcie_rp_update_devicetree(get_tbt_pcie_rp_table());
```
Also implement Async CSE implementation as we did in MTL
https://review.coreboot.org/c/coreboot/+/83798/comment/e56c9768_8d228e33?usp... : PS38, Line 200: block_gpio_enable(dev); you need an entry for P2SB#2 (PCI_DEVFN_P2SB2) that sets up ioe_p2sb_ops
https://review.coreboot.org/c/coreboot/+/83798/comment/2e52da11_bb5a8179?usp... : PS38, Line 207: }; please implement .final as well.