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:
(31 comments)
Patchset:
PS38: lso
File src/soc/intel/pantherlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/83798/comment/24b1e1df_809fbcc5?usp... : PS38, Line 190: # - 194 MiB Non-prefetchable memory : # - 448 MiB Prefetchable memory you need to update the comments as well depending upon the hardcoded value
https://review.coreboot.org/c/coreboot/+/83798/comment/71e41ff6_a7b1b924?usp... : PS38, Line 198: config PCIEXP_HOTPLUG_MEM : hex : default 0x6000000 : : config PCIEXP_HOTPLUG_PREFETCH_MEM : hex : default 0x800000000 please specify the source of this magic numbers
https://review.coreboot.org/c/coreboot/+/83798/comment/6242bee7_b7bc76e8?usp... : PS38, Line 292: config VBT_DATA_SIZE_KB : int : default 9 why we need this ?
https://review.coreboot.org/c/coreboot/+/83798/comment/20a0a92d_a2fbed2a?usp... : PS38, Line 314: select VBOOT_X86_RSA_ACCELERATION please follow the order
https://review.coreboot.org/c/coreboot/+/83798/comment/197fafc9_94e02776?usp... : PS38, Line 388: you still need below configs
``` config HAVE_BMP_LOGO_COMPRESS_LZMA default n
# The default offset to store CSE RW FW version information is at 68. # However, in Intel Meteor Lake based systems that use PSR, the additional # size required to keep CSE RW FW version information and PSR back-up status # in adjacent CMOS memory at offset 68 is not available. Therefore, we # override the default offset to 161, which has enough space to keep both # the CSE related information together. config SOC_INTEL_CSE_FW_PARTITION_CMOS_OFFSET int default 161
config SOC_INTEL_COMMON_BLOCK_ACPI_SLP_S0_FREQ_HZ default 0x2005 help slp_s0_residency granularity in 122us ticks (i.e. ~8.2KHz) in Meteor Lake.
config SOC_PHYSICAL_ADDRESS_WIDTH int default 42 ```
File src/soc/intel/pantherlake/Makefile.mk:
https://review.coreboot.org/c/coreboot/+/83798/comment/9cf98677_15468be1?usp... : PS38, Line 39: ramstage-y += me.c do we need this ?
https://review.coreboot.org/c/coreboot/+/83798/comment/b51d198a_7f559fd4?usp... : PS38, Line 61: : CFLAGS_common += -fshort-wchar this has been fixed in MTL, follow https://review.coreboot.org/c/coreboot/+/81192
File src/soc/intel/pantherlake/chip.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/59c496c4_9130d029?usp... : PS38, Line 200: block_gpio_enable(dev);
you need an entry for P2SB#2 (PCI_DEVFN_P2SB2) that sets up ioe_p2sb_ops
``` else if (dev->path.type == DEVICE_PATH_PCI && dev->path.pci.devfn == PCI_DEVFN_P2SB2) dev->ops = &ioe_p2sb_ops; ```
File src/soc/intel/pantherlake/me.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/c4185082_3036ea2f?usp... : PS38, Line 8: follow https://review.coreboot.org/c/coreboot/+/73137
File src/soc/intel/pantherlake/p2sb.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/38e1d388_fb77071b?usp... : PS38, Line 45: you still need below code
``` static void p2sb2_read_resources(struct device *dev) { /* Add the fixed MMIO resource for IOE P2SB */ mmio_range(dev, PCI_BASE_ADDRESS_0, IOE_P2SB_BAR, IOE_P2SB_SIZE); } ```
``` struct device_operations p2sb2_ops = { .read_resources = p2sb2_read_resources, .set_resources = noop_set_resources, .scan_bus = scan_static_bus, }; ```
as you have selected SOC_INTEL_COMMON_BLOCK_IOE_P2SB for 2nd P2SB. look at https://review.coreboot.org/c/coreboot/+/83798/comment/e56c9768_8d228e33/
File src/soc/intel/pantherlake/pcie_rp.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/88985bf2_86981d58?usp... : PS38, Line 8: please add `tbt_rp_groups` as mentioned previously
https://review.coreboot.org/c/coreboot/+/83798/comment/36f32061_f1fdc8f2?usp... : PS38, Line 11: #if CONFIG(SOC_INTEL_PANTHERLAKE_U_H) : { .slot = PCI_DEV_SLOT_PCIE_2, .count = 4, .lcap_port_base = 1 }, : #endif : #if CONFIG(SOC_INTEL_PANTHERLAKE_H) : { .slot = PCI_DEV_SLOT_PCIE_2, .count = 2, .lcap_port_base = 1 }, : #endif ``` #if CONFIG(SOC_INTEL_PANTHERLAKE_U_H) { .slot = PCI_DEV_SLOT_PCIE_2, .count = 4, .lcap_port_base = 1 }, #else { .slot = PCI_DEV_SLOT_PCIE_2, .count = 2, .lcap_port_base = 1 }, #endif ```
File src/soc/intel/pantherlake/pmc.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/7fbf1716_75fbc724?usp... : PS38, Line 76: pmc_dump_soc_qdf_info(); why again ?
https://review.coreboot.org/c/coreboot/+/83798/comment/594f114b_25c52154?usp... : PS38, Line 106: please follow https://review.coreboot.org/c/coreboot/+/69576
https://review.coreboot.org/c/coreboot/+/83798/comment/53a844df_4d6686d1?usp... : PS38, Line 124: lnl_pmc_lpm should be `ptl_pmc_lpm`?
https://review.coreboot.org/c/coreboot/+/83798/comment/0e4ae90a_234d93c9?usp... : PS38, Line 132: lnl_pmc_lpm same
https://review.coreboot.org/c/coreboot/+/83798/comment/8daa8156_023057dc?usp... : PS38, Line 188: soc_acpi_mode_init should be `soc_pmc_init`
https://review.coreboot.org/c/coreboot/+/83798/comment/fa94ccfc_09fcb33c?usp... : PS38, Line 189: pmc_init Why is `pmc_int` for enable function ? ideally you should be using `soc_pmc_enable`
File src/soc/intel/pantherlake/pmutil.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/37d56c0a_72447645?usp... : PS38, Line 15: #include <device/pci_def.h> why we need this ?
File src/soc/intel/pantherlake/retimer.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/ceb8a008_463b4826?usp... : PS38, Line 16: DEV_PTR(tcss_usb3_port2), : }; don't we have 4 TCSS ports
https://review.coreboot.org/c/coreboot/+/83798/comment/3a200610_0db60915?usp... : PS38, Line 20: _MAX_TYPE_C_PORTS why don't you use the macro (MAX_TYPE_C_PORTS) directly coming from TCSS.h
File src/soc/intel/pantherlake/spi.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/358cbd00_e3978e1b?usp... : PS38, Line 12: case PCI_DEVFN_SPI: : return 0; this is fast SPI and doesn't belong here.
https://review.coreboot.org/c/coreboot/+/83798/comment/ba430313_6e928143?usp... : PS38, Line 18: } please add GSPI index 2 here
File src/soc/intel/pantherlake/systemagent.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/c8d3f112_391ced18?usp... : PS38, Line 27: { PCIEXBAR, CONFIG_ECAM_MMCONF_BASE_ADDRESS, CONFIG_ECAM_MMCONF_LENGTH, : "PCIEXBAR" }, https://review.coreboot.org/c/coreboot/+/71114 this CL dropped PCIEXBAR previous as this range already reserved
https://review.coreboot.org/c/coreboot/+/83798/comment/323e98a0_9020075c?usp... : PS38, Line 99: get_mmcfg_size sa_get_mmcfg_size
https://review.coreboot.org/c/coreboot/+/83798/comment/9da2d14c_fe3f1488?usp... : PS38, Line 105: get_dsm_size please look at https://review.coreboot.org/c/coreboot/+/80404 which again tells me that your PTL code is so old and need rebasing
https://review.coreboot.org/c/coreboot/+/83798/comment/cc897b2c_5a1d2a6e?usp... : PS38, Line 163: /* Enable BIOS Reset CPL */ : enable_bios_reset_cpl(); https://review.coreboot.org/c/coreboot/+/70556
https://review.coreboot.org/c/coreboot/+/83798/comment/2b73963a_dffeea21?usp... : PS38, Line 165: : /* Configure turbo power limits 1ms after reset complete bit */ : mdelay(1); : config = config_of_soc(); : : /* Get System Agent PCI ID */ : sa = pcidev_path_on_root(PCI_DEVFN_ROOT); : sa_pci_id = sa ? pci_read_config16(sa, PCI_DEVICE_ID) : 0xFFFF; : : /* Choose a power limits configuration based on the SoC SKU type, : * differentiated here based on SA PCI ID. */ : switch (sa_pci_id) { : case PCI_DID_INTEL_PTL_U_ID_1: : case PCI_DID_INTEL_PTL_H_ID_1: : case PCI_DID_INTEL_PTL_H_ID_2: : soc_config = &config->power_limits_config[PTL_U_15W_POWER_LIMITS]; : break; : default: : printk(BIOS_ERR, "unknown SA ID: 0x%4x, skipping power limits configuration\n", : sa_pci_id); : return; : } : : /* UPDATEME: Need to enable later */ : set_power_limits(MOBILE_SKU_PL1_TIME_SEC, soc_config); follow https://review.coreboot.org/c/coreboot/+/74380
https://review.coreboot.org/c/coreboot/+/83798/comment/4da243c9_71261793?usp... : PS38, Line 206: uint64_t get_mmcfg_size(const struct device *dev) : { : uint32_t pciexbar_reg; : uint64_t mmcfg_length; : : if (!dev) { : printk(BIOS_DEBUG, "%s : device is null\n", __func__); : return 0; : } : : pciexbar_reg = pci_read_config32(dev, PCIEXBAR); : : if (!(pciexbar_reg & (1 << 0))) { : printk(BIOS_DEBUG, "%s : PCIEXBAR disabled\n", __func__); : return 0; : } : : switch ((pciexbar_reg & MASK_PCIEXBAR_LENGTH) >> PCIEXBAR_LENGTH_LSB) { : case PCIEXBAR_LENGTH_4096MB: : mmcfg_length = 4 * ((uint64_t)GiB); : break; : case PCIEXBAR_LENGTH_2048MB: : mmcfg_length = 2 * ((uint64_t)GiB); : break; : case PCIEXBAR_LENGTH_1024MB: : mmcfg_length = 1 * GiB; : break; : case PCIEXBAR_LENGTH_512MB: : mmcfg_length = 512 * MiB; : break; : case PCIEXBAR_LENGTH_256MB: : mmcfg_length = 256 * MiB; : break; : case PCIEXBAR_LENGTH_128MB: : mmcfg_length = 128 * MiB; : break; : case PCIEXBAR_LENGTH_64MB: : mmcfg_length = 64 * MiB; : break; : default: : printk(BIOS_DEBUG, "%s : PCIEXBAR - invalid length (0x%x)\n", __func__, : pciexbar_reg & MASK_PCIEXBAR_LENGTH); : mmcfg_length = 0x0; : break; : } : : return mmcfg_length; : } : : uint64_t get_dsm_size(const struct device *dev) : { : // - size : B0/D0/F0:R 50h [15:8] : uint32_t reg32 = pci_read_config32(dev, GGC); : uint64_t size; : uint32_t size_field = (reg32 & MASK_DSM_LENGTH) >> MASK_DSM_LENGTH_LSB; : if (size_field <= 0x10) { // 0x0 - 0x10 : size = size_field * 32 * MiB; : } else if ((size_field >= 0xF0) && (size_field >= 0xFE)) { : size = ((uint64_t)size_field - 0xEF) * 4 * MiB; : } else { : switch (size_field) { : case 0x20: : size = 1 * GiB; : break; : case 0x30: : size = 1536 * MiB; : break; : case 0x40: : size = 2 * (uint64_t)GiB; : break; : default: : printk(BIOS_DEBUG, "%s : DSM - invalid length (0x%x)\n", : __func__, size_field); : size = 0x0; : break; : } : } : return size; : } : : uint64_t get_gsm_size(const struct device *dev) : { : const u32 gsm_size = pci_read_config32(dev, GGC); : uint64_t size; : uint32_t size_field = (gsm_size & MASK_GSM_LENGTH) >> MASK_GSM_LENGTH_LSB; : switch (size_field) { : case 0x0: : size = 0; : break; : case 0x1: : size = 2 * MiB; : break; : case 0x2: : size = 4 * MiB; : break; : case 0x3: : size = 8 * MiB; : break; : default: : size = 0; : break; : } : return size; : } : uint64_t get_dpr_size(const struct device *dev) : { : uint64_t size; : uint32_t dpr_reg = pci_read_config32(dev, DPR_REG); : uint32_t size_field = (dpr_reg & MASK_DPR_LENGTH) >> MASK_DPR_LENGTH_LSB; : size = (uint64_t)size_field * MiB; : return size; : } please drop
File src/soc/intel/pantherlake/tcss.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/f4f81024_8cba6392?usp... : PS38, Line 6: const struct soc_tcss_ops tcss_ops = { : }; : drop this if we don;t have any need