Attention is currently required from: Cliff Huang, Kapil Porwal, Pranava Y N, Subrata Banik.
Saurabh Mishra 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:
(36 comments)
File src/soc/intel/pantherlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/83798/comment/a0788f16_cea03a73?usp... : PS38, Line 2:
we need to include below configs […]
Added all mentioned, except "SOC_INTEL_COMMON_BLOCK_ACPI_CPU_HYBRID" Moved this config to ACPI CL.
https://review.coreboot.org/c/coreboot/+/83798/comment/a8df2791_9a1164b3?usp... : PS38, Line 86: SOC_INTEL_ENABLE_USB4_PCIE_RESOURCES
this was always a main board selectable item previously. […]
Ack, it should not be selected here. removed from soc/kconfig.
https://review.coreboot.org/c/coreboot/+/83798/comment/88aebd26_b24e464a?usp... : PS38, Line 401: endif
why are you overlooking https://review.coreboot. […]
You mean to say, config "DROP_CPU_FEATURE_PROGRAM_IN_FSP" should be added?
File src/soc/intel/pantherlake/acpi.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/c8dce408_1ee00c0f?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?
Ack, added devfn check for IAA.
File src/soc/intel/pantherlake/chip.h:
https://review.coreboot.org/c/coreboot/+/83798/comment/5343b6a8_9b9ca11c?usp... : PS38, Line 63:
you might need this now ? […]
Ack, added.
https://review.coreboot.org/c/coreboot/+/83798/comment/8463a444_22d04e6d?usp... : PS38, Line 103: TcssD3ColdDisable
why camel case ?
Ack, fixed.
https://review.coreboot.org/c/coreboot/+/83798/comment/34cbe2ed_179b3e32?usp... : PS38, Line 127: SaGv_Disabled, : SaGv_FixedPoint0, : SaGv_FixedPoint1, : SaGv_FixedPoint2, : SaGv_FixedPoint3, : SaGv_Enabled,
please use all capital letter for enum
Acknowledged
https://review.coreboot.org/c/coreboot/+/83798/comment/d2b97ae2_1560b632?usp... : PS38, Line 212: HeciEnabled
why camel case ?
Acknowledged
https://review.coreboot.org/c/coreboot/+/83798/comment/5ed562a7_139052b3?usp... : PS38, Line 219: PmTimerDisabled
same
Acknowledged
https://review.coreboot.org/c/coreboot/+/83798/comment/999d630a_0a61d8ba?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
Acknowledged
https://review.coreboot.org/c/coreboot/+/83798/comment/8462f382_bdea248f?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
Acknowledged
https://review.coreboot.org/c/coreboot/+/83798/comment/25e4bd89_a75ca1e7?usp... : PS38, Line 308: HybridStorageMode
same
Acknowledged
https://review.coreboot.org/c/coreboot/+/83798/comment/5bc49f6f_4b333d0f?usp... : PS38, Line 327: DmiPwrOptimizeDisable
same
Acknowledged
https://review.coreboot.org/c/coreboot/+/83798/comment/34a6a9fc_4e170b20?usp... : PS38, Line 339: IsaSerialUartBase
same
Acknowledged
File src/soc/intel/pantherlake/chip.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/46e82ab3_c5573d46?usp... : PS38, Line 70: case PCI_DEVFN_TCSS_XHCI: return "TXHC";
why IGD is missing here from ACPI name space ?
Added IGD below to PCI_DEVFN_ROOT.
https://review.coreboot.org/c/coreboot/+/83798/comment/7fc0b05d_66a77123?usp... : PS38, Line 78: NPU0
why NPU0? the limit is 4 char max but you can keep < 4 as well. […]
Set to 3 chars.
https://review.coreboot.org/c/coreboot/+/83798/comment/67402328_842d215f?usp... : PS38, Line 79: IPU0
same, why IPU0 and not IPU
Set to 3 chars.
https://review.coreboot.org/c/coreboot/+/83798/comment/ea74d8f8_96388837?usp... : PS38, Line 80: case PCI_DEVFN_I3C1: return "I3C1"; : case PCI_DEVFN_I3C2: return "I3C2";
as discussed previously, we don't need now
Acknowledged, removed.
https://review.coreboot.org/c/coreboot/+/83798/comment/033ffc72_30ac62eb?usp... : PS38, Line 115: case PCI_DEVFN_IGD: return "GFX0";
why here ?
removed from bottom order.
https://review.coreboot.org/c/coreboot/+/83798/comment/6ecb4a9f_f0173da8?usp... : PS38, Line 122:
please use from MTL code […]
Ack, added.
https://review.coreboot.org/c/coreboot/+/83798/comment/45b01146_9817311b?usp... : PS38, Line 148: pcie_rp_update_devicetree(get_pcie_rp_table());
please use […]
Sure, added both get_tbt_pcie_rp_table & Async CSE.
https://review.coreboot.org/c/coreboot/+/83798/comment/f9f2163a_b253fc36?usp... : PS38, Line 200: block_gpio_enable(dev);
you need an entry for P2SB#2 (PCI_DEVFN_P2SB2) that sets up ioe_p2sb_ops
Ack,added.
https://review.coreboot.org/c/coreboot/+/83798/comment/ae74ff8d_5a3a7572?usp... : PS38, Line 207: };
please implement .final as well.
Ack,added.
File src/soc/intel/pantherlake/elog.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/0289524b_520688a4?usp... : PS38, Line 43: { PCI_DEVFN_PCIE6, ELOG_WAKE_SOURCE_PME_PCIE6 },
why RP7 and RP8 are not included in this list?
Added.
https://review.coreboot.org/c/coreboot/+/83798/comment/50cdc563_bb6f6e21?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
Added SOC_INTEL_PANTHERLAKE_U_H guard check to add RP#11, RP#12.
File src/soc/intel/pantherlake/gspi.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/184699dd_899627ca?usp... : PS38, Line 15: }
You have mentioned total 3 GSPI controllers are there then why only entries for 2 of them ? […]
Correct to 2.
File src/soc/intel/pantherlake/include/soc/dptf.h:
https://review.coreboot.org/c/coreboot/+/83798/comment/fd8d8b9c_4eab0bb6?usp... : PS38, Line 14:
don't we need below devices […]
I will verify the values and add it.
File src/soc/intel/pantherlake/include/soc/espi.h:
https://review.coreboot.org/c/coreboot/+/83798/comment/5d5d4002_4a1e2928?usp... : PS38, Line 5:
can you please follow https://review.coreboot. […]
Ack, removed
File src/soc/intel/pantherlake/include/soc/irq.h:
https://review.coreboot.org/c/coreboot/+/83798/comment/1642b3a3_20c4dcc7?usp... : PS38, Line 3: _SOC_IRQ_H_
_SOC_PANTHERLAKE_IRQ_H_ ?
Acknowledged
https://review.coreboot.org/c/coreboot/+/83798/comment/65ce2ef9_b82a64b7?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 .
Removed.
File src/soc/intel/pantherlake/include/soc/me.h:
https://review.coreboot.org/c/coreboot/+/83798/comment/135b267f_f71b8043?usp... : PS38, Line 5:
please follow https://review.coreboot.org/c/coreboot/+/73137/10 (more than one year old code). […]
Ack, updated accordingly.
File src/soc/intel/pantherlake/include/soc/pcie.h:
https://review.coreboot.org/c/coreboot/+/83798/comment/29411986_ac9e7b2d?usp... : PS38, Line 9:
for sure we need get_tbt_pcie_rp_table […]
Sure, added.
File src/soc/intel/pantherlake/include/soc/pmc.h:
https://review.coreboot.org/c/coreboot/+/83798/comment/e87fc873_feda37d6?usp... : PS38, Line 26: (1 << 18)
why ? what is the problem with BIT(18) macro ?
Ack, changed to use BIT() macro.
File src/soc/intel/pantherlake/include/soc/serialio.h:
https://review.coreboot.org/c/coreboot/+/83798/comment/bd336ffe_7aa83d9b?usp... : PS38, Line 23: enum { : PchSerialIoIndexI3C0, : PchSerialIoIndexI3C1 : };
we don't need as mentioned earlier
Acknowledged, removed
File src/soc/intel/pantherlake/include/soc/tcss.h:
https://review.coreboot.org/c/coreboot/+/83798/comment/2a3607e5_89e574e2?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 […]
Acknowledged, removed.
File src/soc/intel/pantherlake/include/soc/usb.h:
https://review.coreboot.org/c/coreboot/+/83798/comment/3da23622_2dd34992?usp... : PS38, Line 34: };
please include https://review.coreboot. […]
Ack, added.