Attention is currently required from: Felix Singer, Saurabh Mishra.
Subrata Banik has posted comments on this change by Saurabh Mishra. ( https://review.coreboot.org/c/coreboot/+/83354?usp=email )
Change subject: soc/intel/ptl: Do initial Panther Lake SoC commit till bootblock ......................................................................
Patch Set 8:
(75 comments)
File src/soc/intel/pantherlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/83354/comment/749f4da0_3937159a?usp... : PS8, Line 8: select INTEL_DESCRIPTOR_MODE_CAPABLE : select IDT_IN_EVERY_STAGE follow order please
https://review.coreboot.org/c/coreboot/+/83354/comment/d2883339_91bd0032?usp... : PS8, Line 10: select CAR_ENHANCED_NEM select CPU_INTEL_FIRMWARE_INTERFACE_TABLE select FAST_SPI_SUPPORTS_EXT_BIOS_WINDOW
https://review.coreboot.org/c/coreboot/+/83354/comment/6bb25290_5a4846f9?usp... : PS8, Line 21: select UDELAY_TSC should enable x86_64 support from mb to soc as x86_64 is the only option for PTL. ``` select HAVE_X86_64_SUPPORT select USE_X86_64_SUPPORT ```
https://review.coreboot.org/c/coreboot/+/83354/comment/a9843689_42714467?usp... : PS8, Line 23: SOC_INTEL_PANTHERLAKE_A0 are you suggesting that we shall add newer Kconfig with every SOC steppings ?
if not then please keep the name as
``` SOC_INTEL_PANTHERLAKE_U_H ```
https://review.coreboot.org/c/coreboot/+/83354/comment/b8ce104a_8d49f615?usp... : PS8, Line 27: Choose this option if your mainboard has a PTL A0 SoC. please use some meaningful comment like what we did for MTL
``` Choose this option if your mainboard has a MTL-U (9W or 15W) or MTL-H (28W or 45W) SoC.
Note, the MTL-U/H-Processor Line offered in a 1-Chip Platform that includes the Compute, SOC, GT, and IOE tile on the same package. ```
https://review.coreboot.org/c/coreboot/+/83354/comment/bfa6b9c0_c100b61c?usp... : PS8, Line 44: 0xfa000000 why you are changing this address ?
ideally this range has calculated from (4GB - 16MB) upto 1MB.
https://review.coreboot.org/c/coreboot/+/83354/comment/81643d1a_3f5f390b?usp... : PS8, Line 47: 0x200000 who needs this much of cache ? this is like 2.5x growth from MTL.
https://review.coreboot.org/c/coreboot/+/83354/comment/d5ada188_5a5dc8cf?usp... : PS8, Line 54: 0x80400 0x88000
(512KB for FSP + 32KB for coreboot)
https://review.coreboot.org/c/coreboot/+/83354/comment/23bfc8ef_fdbd726b?usp... : PS8, Line 63: 0x80100 any reason why so much of memory usage increases between MTL to PTL ? if this some placeholder number ?
https://review.coreboot.org/c/coreboot/+/83354/comment/78315348_13cb7cff?usp... : PS8, Line 71: ifd2 please use `ptl` as we have landed https://review.coreboot.org/c/coreboot/+/83141
https://review.coreboot.org/c/coreboot/+/83354/comment/4cba5f61_8fa04a0a?usp... : PS8, Line 77: config HEAP_SIZE : hex : default 0x10000 any reason why you need to override this from its default value ?
https://review.coreboot.org/c/coreboot/+/83354/comment/9e270528_cca86af8?usp... : PS8, Line 85: sideband bus P2SB#1 aka SoC P2SB
https://review.coreboot.org/c/coreboot/+/83354/comment/9b232826_b9ca7a2a?usp... : PS8, Line 107: 2 should be 3
https://review.coreboot.org/c/coreboot/+/83354/comment/7443943a_7af15fe6?usp... : PS8, Line 113: config SOC_INTEL_I3C_DEV_MAX : int : default 2 no user, drop this
https://review.coreboot.org/c/coreboot/+/83354/comment/9352fdf1_af325ea0?usp... : PS8, Line 151: 0x4000 why double ?
https://review.coreboot.org/c/coreboot/+/83354/comment/ebd8bebd_c3127eef?usp... : PS8, Line 152: : config CONSOLE_CBMEM_BUFFER_SIZE : hex : default 0x40000 ``` config CONSOLE_CBMEM_BUFFER_SIZE hex default 0x100000 if BUILDING_WITH_DEBUG_FSP default 0x40000 ```
https://review.coreboot.org/c/coreboot/+/83354/comment/9ed5d167_5928a5d1?usp... : PS8, Line 156: should add below config as well here
1. SOC_PHYSICAL_ADDRESS_WIDTH 2. VBOOT 3. P2SB_2_PCR_BASE_ADDRESS <--- this should assign to P2SB2_BAR in iomap.h 4.
File src/soc/intel/pantherlake/Makefile.mk:
https://review.coreboot.org/c/coreboot/+/83354/comment/bdf68fe2_6710c6a4?usp... : PS8, Line 7: subdirs-y += ../../../cpu/x86/mtrr : subdirs-y += ../../../cpu/x86/tsc why we need this for PTL ? traditionally, I have never seen those sub-directory are being included here
https://review.coreboot.org/c/coreboot/+/83354/comment/d86decc7_e2f7c616?usp... : PS8, Line 11: pch pcd.c
https://review.coreboot.org/c/coreboot/+/83354/comment/4911c740_88284e2b?usp... : PS8, Line 13: need below files here as well
``` bootblock-y += espi.c bootblock-y += soc_info.c ```
File src/soc/intel/pantherlake/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/83354/comment/37b2dbbb_a91fa68c?usp... : PS8, Line 14: soc_die should be `pcd` or `platform_ctrl_die`
File src/soc/intel/pantherlake/bootblock/pch.c:
https://review.coreboot.org/c/coreboot/+/83354/comment/1efdc727_fe79840a?usp... : PS8, Line 1: /* SPDX-License-Identifier: GPL-2.0-only */ why you are not following the SOC proper nomenclature here?
- ADL had PCH die - MTL had SoC and IOE die - PTL will have PCD die
there is nothing called pch die in PTL hence, this file name should be pcd.c
https://review.coreboot.org/c/coreboot/+/83354/comment/aeedb1a8_e5e5e1c3?usp... : PS8, Line 40: soc shouldn't this be like `pcd_die_config_pwrmbase`
https://review.coreboot.org/c/coreboot/+/83354/comment/6f63222a_9392c07b?usp... : PS8, Line 58: pch same feedback as above, please use one nomenclature as applicable and don't mix pch, soc and pcd interchangeably.
https://review.coreboot.org/c/coreboot/+/83354/comment/d8ea7e4a_ddba5779?usp... : PS8, Line 70: /* Program generic IO Decode Range */ : pch_enable_lpc(); this function is inside `espi.c` without adding this file how are you able to make a call into pch_enable_lpc() ?
https://review.coreboot.org/c/coreboot/+/83354/comment/e78e17dd_ee7be5fc?usp... : PS8, Line 74: pch same as above
https://review.coreboot.org/c/coreboot/+/83354/comment/bf4ed8bd_a75e5bc9?usp... : PS8, Line 74: void bootblock_pch_early_init(void) why not static ?
https://review.coreboot.org/c/coreboot/+/83354/comment/d1b7821f_ed511c1c?usp... : PS8, Line 76: soc_die_early_sa_init you are calling soc_die_early_sa_init() function twice ?
1. inside bootblock_pch_early_init() 2. inside bootblock_soc_die_early_init()
ideally the flow should be
- bootblock.c has called into `bootblock_platform_ctrl_die_early_init`/`bootblock_pcd_early_init` - `bootblock_platform_ctrl_die_early_init` then call into `platform_ctrl_die_early_sa_init` and `platform_ctrl_die_early_ip_init`
Rather this code makes soc_die_early_sa_init() getting called by both platform_ctrl_die_early_sa_init and platform_ctrl_die_early_sa_init
https://review.coreboot.org/c/coreboot/+/83354/comment/fe556e70_95a4cdbb?usp... : PS8, Line 89: SoC update the comment to fit into existing SoC schema
https://review.coreboot.org/c/coreboot/+/83354/comment/521785fd_97374dc0?usp... : PS8, Line 95: void soc_die_early_sa_init(void) why not static ?
https://review.coreboot.org/c/coreboot/+/83354/comment/8499854f_fe00e657?usp... : PS8, Line 107: soc same as above
https://review.coreboot.org/c/coreboot/+/83354/comment/a859d376_41ba4ad7?usp... : PS8, Line 123: soc same as above
https://review.coreboot.org/c/coreboot/+/83354/comment/a711dc7a_5f6af1f6?usp... : PS8, Line 143: pch same as above
https://review.coreboot.org/c/coreboot/+/83354/comment/7dbf07b3_ed294ca4?usp... : PS8, Line 149: soc same as above
File src/soc/intel/pantherlake/bootblock/report_platform.c:
https://review.coreboot.org/c/coreboot/+/83354/comment/7dc4aaf7_29e30785?usp... : PS8, Line 18: extra newline
https://review.coreboot.org/c/coreboot/+/83354/comment/96b4f68c_65919469?usp... : PS8, Line 19: enum core_type { : CPUID_RESERVED_1 = 0x10, : CPUID_CORE_TYPE_INTEL_ATOM = 0x20, : CPUID_RESERVED_2 = 0x30, : CPUID_CORE_TYPE_INTEL_CORE = 0x40, : CPUID_UNKNOWN = 0xff, : }; already existed inside https://github.com/coreboot/coreboot/blob/main/src/soc/intel/common/block/in...
https://review.coreboot.org/c/coreboot/+/83354/comment/bf45535c_5638a245?usp... : PS8, Line 38: P based on my understanding PTL has U/H and H x4e so, technically there is nothing called `PTL-P`?
ideally this should be `U`
https://review.coreboot.org/c/coreboot/+/83354/comment/b047a854_e0db27d9?usp... : PS8, Line 46: { PCI_DID_INTEL_PTL_ESPI_1, "Pantherlake SOC-P SuperSKU" }, : { PCI_DID_INTEL_PTL_ESPI_2, "Pantherlake SOC-P Premium" }, : { PCI_DID_INTEL_PTL_ESPI_3, "Pantherlake SOC-P Base" }, should be `U` at least
https://review.coreboot.org/c/coreboot/+/83354/comment/085dfb84_c1442ecf?usp... : PS8, Line 59: P same
https://review.coreboot.org/c/coreboot/+/83354/comment/ade73909_8af569a5?usp... : PS8, Line 87: static enum core_type get_soc_cpu_type(void) : { : if (cpu_is_hybrid_supported()) : return cpu_get_cpu_type(); : else : return CPUID_CORE_TYPE_INTEL_CORE; : } i don't see any value here as PTL is hybrid core hence, additional conditional operation seems redundant
https://review.coreboot.org/c/coreboot/+/83354/comment/afa5fa0c_75abe1eb?usp... : PS8, Line 118: get_soc_cpu_type what is the value of printing this information as we know for PTL the BSP would be always `core`.
File src/soc/intel/pantherlake/include/soc/bootblock.h:
https://review.coreboot.org/c/coreboot/+/83354/comment/834fad30_f7aa2be8?usp... : PS8, Line 7: void bootblock_soc_die_early_init(void); : void bootblock_pch_early_init(void); : please update the function name as per feedback from other file
https://review.coreboot.org/c/coreboot/+/83354/comment/89225387_4a2379b4?usp... : PS8, Line 15: //_SOC_PANTHERLAKE_BOOTBLOCK_H_ seems redundant as the file size is comparatively smaller hence, this comment doesn't make any sense
File src/soc/intel/pantherlake/include/soc/iomap.h:
https://review.coreboot.org/c/coreboot/+/83354/comment/bcec15c0_0f8dbd28?usp... : PS8, Line 27: : //PMC MBAR 64KB seems unnecessary
https://review.coreboot.org/c/coreboot/+/83354/comment/06067fd0_1292f27b?usp... : PS8, Line 36: //VTD BAR 512KB seems unnecessary
https://review.coreboot.org/c/coreboot/+/83354/comment/f0531ec8_9bc8ece1?usp... : PS8, Line 37: fc800000 address 0xfed90000 with 8KB
https://review.coreboot.org/c/coreboot/+/83354/comment/f059f551_86582c8b?usp... : PS8, Line 37: VTD_BASE_ADDRESS this is now called DMI3BAR (as per PTL FAS)
FAS also talks abut something called VTD BAR (so not very sure which one has updated as per PTL)
https://review.coreboot.org/c/coreboot/+/83354/comment/4ceb7bdd_b2dfe9a6?usp... : PS8, Line 48: 0xfe03e000 CONFIG_CONSOLE_UART_BASE_ADDRESS
https://review.coreboot.org/c/coreboot/+/83354/comment/e19bb23f_969b4ae0?usp... : PS8, Line 61: REGBAR 128MB comment and line #63 are not aligned
https://review.coreboot.org/c/coreboot/+/83354/comment/f5a8b472_db9507f1?usp... : PS8, Line 62: 0xd0000000 as per FAS this range is 0xf0000000
https://review.coreboot.org/c/coreboot/+/83354/comment/9f2add58_72efa983?usp... : PS8, Line 63: 256 should be 128 as per FAS
https://review.coreboot.org/c/coreboot/+/83354/comment/ca1d8326_8b79b0bf?usp... : PS8, Line 68: //PCH P2SB2 256MB if you wish to add a comment then follow coreboot style
``` /* ... */ ```
https://review.coreboot.org/c/coreboot/+/83354/comment/7024ef1c_3710b9f9?usp... : PS8, Line 72: #define PCH_SECOND_PCR_ABOVE_4G_BASE_ADDR 0x3fff0000000 I don't see anything belongs to this range and please provide this.
Based on my understanding you are trying to follow the same style of MTL where P2SB#2 was known as IOE P2SB. If we are still keen on leveraging the P2SB#2 as IOE P2SB (to use ioe_p2sb.c). Then I would say lets create something as below
``` #define IOE_P2SB_BAR P2SB2_BAR #define IOE_P2SB_SIZE P2SB2_SIZE ```
``` config SOC_INTEL_COMMON_BLOCK_P2SB_2 bool select SOC_INTEL_COMMON_BLOCK_IOE_P2SB help Intel Processor common P2SB#2 driver inside PCD die ```
https://review.coreboot.org/c/coreboot/+/83354/comment/a1cb4040_19e9f901?usp... : PS8, Line 74: 0x3fff0800000 possible to reflect this range also inside FAS in upcoming revision ?
https://review.coreboot.org/c/coreboot/+/83354/comment/abdca47a_ec8a36dc?usp... : PS8, Line 78: /* : * I/O port address space : */ nit,
``` /* I/O port address space */ ```
File src/soc/intel/pantherlake/include/soc/p2sb.h:
https://review.coreboot.org/c/coreboot/+/83354/comment/45d5aaf1_c9e9ddb5?usp... : PS8, Line 11: extern struct device_operations soc_p2sb_ops; I still believe we need to use P2SB# hence, better we define it here
``` extern struct device_operations soc_p2sb_2_ops;
```
File src/soc/intel/pantherlake/include/soc/pci_devs.h:
https://review.coreboot.org/c/coreboot/+/83354/comment/e67d44d5_b3db9ede?usp... : PS8, Line 3: _SOC_PANTHERLAKE_PCI_DEVS_H_ follow one standard in all other header file but don't use interchangeably
``` _SOC_PANTHERLAKE_PCI_DEVS_H_ ```
or
``` SOC_PANTHERLAKE_PCI_DEVS_H ```
https://review.coreboot.org/c/coreboot/+/83354/comment/7cec0971_60661d2e?usp... : PS8, Line 34: ``` #define PCI_DEV_SLOT_PCIE_2 0x6 #define PCI_DEVFN_PCIE9 _PCI_DEVFN(PCIE_2, 0) #define PCI_DEVFN_PCIE10 _PCI_DEVFN(PCIE_2, 1) #define PCI_DEVFN_PCIE11 _PCI_DEVFN(PCIE_2, 2) #define PCI_DEVFN_PCIE12 _PCI_DEVFN(PCIE_2, 3) #define PCI_DEV_PCIE9 _PCI_DEV(PCIE_2, 0) #define PCI_DEV_PCIE10 _PCI_DEV(PCIE_2, 1) #define PCI_DEV_PCIE11 _PCI_DEV(PCIE_2, 2) #define PCI_DEV_PCIE12 _PCI_DEV(PCIE_2, 3) ```
https://review.coreboot.org/c/coreboot/+/83354/comment/bc09e800_8892d96f?usp... : PS8, Line 46: #define PCI_DEV_SLOT_TELEMETRY 0x0a #define PCI_DEVFN_TELEMETRY _PCI_DEVFN(TELEMETRY, 0) #define PCI_DEV_TELEMETRY _PCI_DEV(TELEMETRY, 0)
https://review.coreboot.org/c/coreboot/+/83354/comment/a3c5e23b_d2421ec7?usp... : PS8, Line 48: VPU NPU
https://review.coreboot.org/c/coreboot/+/83354/comment/e234f86a_f3cf1bdb?usp... : PS8, Line 50: please add IAA as well the SOC IP for CrOS
https://review.coreboot.org/c/coreboot/+/83354/comment/64de0c4b_d4f4367b?usp... : PS8, Line 63: #define PCI_DEV_SLOT_VMD 0x0e : #define PCI_DEVFN_VMD _PCI_DEVFN(VMD, 0) : #define PCI_DEV_VMD _PCI_DEV(VMD, 0) not present in EDS#1
https://review.coreboot.org/c/coreboot/+/83354/comment/bf5aabe0_692bcf05?usp... : PS8, Line 84: should add GSPI#2 as well
https://review.coreboot.org/c/coreboot/+/83354/comment/4e09c527_ce591e52?usp... : PS8, Line 90: #define PCI_DEVFN_IEH _PCI_DEVFN(XHCI, 5) should be IEH_0
and add IEH_1 above as well for 0x12
https://review.coreboot.org/c/coreboot/+/83354/comment/17ae3222_2c806a36?usp... : PS8, Line 122: #define PCI_DEV_CSE_WLAN _PCI_DEV(CSE, 7) what is CSE WLAN ?
https://review.coreboot.org/c/coreboot/+/83354/comment/ea2c9c11_1665d922?usp... : PS8, Line 159: #define PCI_DEV_SLOT_PCIE_2 0x6 : #define PCI_DEVFN_PCIE9 _PCI_DEVFN(PCIE_2, 0) : #define PCI_DEVFN_PCIE10 _PCI_DEVFN(PCIE_2, 1) : #define PCI_DEV_PCIE9 _PCI_DEV(PCIE_2, 0) : #define PCI_DEV_PCIE10 _PCI_DEV(PCIE_2, 1) move this as per order
https://review.coreboot.org/c/coreboot/+/83354/comment/ad85eb75_5efc82db?usp... : PS8, Line 170: #define PCI_DEVFN_TSN1 _PCI_DEVFN(SIO2, 4) : #define PCI_DEVFN_TSN2 _PCI_DEVFN(SIO2, 5) i don't see these PCI IDs in EDS
https://review.coreboot.org/c/coreboot/+/83354/comment/570f55dc_7593798b?usp... : PS8, Line 219: PCH_DEV_P2SB2 PCI_DEV_IOE_P2SB
https://review.coreboot.org/c/coreboot/+/83354/comment/6ca1afc3_ae224fbc?usp... : PS8, Line 219: PCH_DEV_P2SB2 this name doesn't exist in common code rather you have to use PCI_DEV_IOE_P2SB to rename for PCI_DEV_P2SB2
File src/soc/intel/pantherlake/include/soc/pcr_ids.h:
https://review.coreboot.org/c/coreboot/+/83354/comment/4d1420e9_54269d4b?usp... : PS8, Line 5: /* start after one new line
https://review.coreboot.org/c/coreboot/+/83354/comment/096d7712_8e0ad1f9?usp... : PS8, Line 20: #define PID_PSF0 0xB5 don't we need to use other PIDs? between PID_PSF1-3 ?
https://review.coreboot.org/c/coreboot/+/83354/comment/0ab08a00_aca1094e?usp... : PS8, Line 21: why gap?
https://review.coreboot.org/c/coreboot/+/83354/comment/3b62bad8_913cbde8?usp... : PS8, Line 32: //SOC_PANTHERLAKE_PCR_H not needed
File src/soc/intel/pantherlake/include/soc/pm.h:
https://review.coreboot.org/c/coreboot/+/83354/comment/63985123_bf5c743d?usp... : PS8, Line 3: _SOC_PM_H_ please use PTL specific header
File src/soc/intel/pantherlake/include/soc/smbus.h:
https://review.coreboot.org/c/coreboot/+/83354/comment/dc4f8002_681be6d1?usp... : PS8, Line 3: _SOC_SMBUS_H_ same here