[coreboot-gerrit] [XL] Change in coreboot[main]: soc/intel/ptl: Do initial Panther Lake SoC commit till bootblock

[Saurabh Mishra (Code Review)]

Attention is currently required from: Dinesh Gehlot, Eran Mitrani, Felix Singer, Jakub Czapiga, Kapil Porwal, Saurabh Mishra, Subrata Banik, Tarun.

Saurabh Mishra 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 9:

(73 comments)

File src/soc/intel/pantherlake/Kconfig:

https://review.coreboot.org/c/coreboot/+/83354/comment/4c882c8c_725f16f0?usp... :
PS8, Line 8: 	select INTEL_DESCRIPTOR_MODE_CAPABLE
           : 	select IDT_IN_EVERY_STAGE
> follow order please

Done

https://review.coreboot.org/c/coreboot/+/83354/comment/b0378c49_014a4038?usp... :
PS8, Line 10: 	select CAR_ENHANCED_NEM
> select CPU_INTEL_FIRMWARE_INTERFACE_TABLE […]

Done

https://review.coreboot.org/c/coreboot/+/83354/comment/561e0c5e_8a118ad1?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. […]

Done

https://review.coreboot.org/c/coreboot/+/83354/comment/9d0015f0_83a927c8?usp... :
PS8, Line 23: SOC_INTEL_PANTHERLAKE_A0
> are you suggesting that we shall add newer Kconfig with every SOC steppings ? […]

Done

https://review.coreboot.org/c/coreboot/+/83354/comment/d4bb3a0c_5386d7e5?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  […]

Done

https://review.coreboot.org/c/coreboot/+/83354/comment/33880288_ce8dad5a?usp... :
PS8, Line 44: 0xfa000000
> why you are changing this address ? […]

Due to PTL using config blocks (intermediality, UPD based FSP will be relased soon) to update  to FSP-M/S, hence the size of DCACHE was increased. This setting is reverted back to previous program settings, and validated with LNL-M SOC SKU (FSP-Debug/Non-debug both).

https://review.coreboot.org/c/coreboot/+/83354/comment/96ac6505_8c72e6bd?usp... :
PS8, Line 47: 0x200000
> who needs this much of cache ? this is like 2.5x growth from MTL.

Due to PTL using config blocks (intermediality, UPD based FSP will be relased soon) to update to FSP-M/S, hence the size of DCACHE was increased. This setting is reverted back to previous program settings, and validated with LNL-M SOC SKU (FSP-Debug/Non-debug both).

https://review.coreboot.org/c/coreboot/+/83354/comment/97635cd6_0cabff1a?usp... :
PS8, Line 54: 0x80400
> 0x88000 […]

Acknowledged

https://review.coreboot.org/c/coreboot/+/83354/comment/84bd98f4_c9077894?usp... :
PS8, Line 63: 0x80100
> any reason why so much of memory usage increases between MTL to PTL ? if this some placeholder numbe […]

Due to PTL using config blocks (intermediality, UPD based FSP will be relased soon) to update to FSP-M/S, hence the size of DCACHE was increased. This setting is reverted back to previous program settings, and validated with LNL-M SOC SKU (FSP-Debug/Non-debug both).

https://review.coreboot.org/c/coreboot/+/83354/comment/044b4f2b_f4a6ca3e?usp... :
PS8, Line 71: ifd2
> please use `ptl` as we have landed https://review.coreboot. […]

Done

https://review.coreboot.org/c/coreboot/+/83354/comment/4a74b80e_0d5ca12f?usp... :
PS8, Line 77: config HEAP_SIZE
            : 	hex
            : 	default 0x10000
> any reason why you need to override this from its default value ?

Removed, not needed.

https://review.coreboot.org/c/coreboot/+/83354/comment/b06d3e64_35c7f489?usp... :
PS8, Line 85: sideband bus
> P2SB#1 aka SoC P2SB

Acknowledged

https://review.coreboot.org/c/coreboot/+/83354/comment/fa442f9f_12bf16b0?usp... :
PS8, Line 107: 2
> should be 3

Acknowledged

https://review.coreboot.org/c/coreboot/+/83354/comment/bff11926_8b54b404?usp... :
PS8, Line 113: config SOC_INTEL_I3C_DEV_MAX
             : 	int
             : 	default 2
> no user, drop this

Acknowledged

https://review.coreboot.org/c/coreboot/+/83354/comment/42be0ff2_2240908c?usp... :
PS8, Line 151: 0x4000
> why double ?

Acknowledged

https://review.coreboot.org/c/coreboot/+/83354/comment/d5179c69_92f04e07?usp... :
PS8, Line 152: 
             : config CONSOLE_CBMEM_BUFFER_SIZE
             : 	hex
             : 	default 0x40000
>

Acknowledged

https://review.coreboot.org/c/coreboot/+/83354/comment/1f80b846_db7fc759?usp... :
PS8, Line 156:
> should add below config as well here […]

Acknowledged

File src/soc/intel/pantherlake/Makefile.mk:

https://review.coreboot.org/c/coreboot/+/83354/comment/7f05b5ba_063b807e?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 h […]

Acknowledged

https://review.coreboot.org/c/coreboot/+/83354/comment/bcfcf3be_0e047c83?usp... :
PS8, Line 11: pch
> pcd. […]

Acknowledged

https://review.coreboot.org/c/coreboot/+/83354/comment/15c02c77_3f3178f0?usp... :
PS8, Line 13:
> need below files here as well […]

Acknowledged

File src/soc/intel/pantherlake/bootblock/bootblock.c:

https://review.coreboot.org/c/coreboot/+/83354/comment/3d9f1842_922badd7?usp... :
PS8, Line 14: soc_die
> should be `pcd` or `platform_ctrl_die`

Acknowledged

File src/soc/intel/pantherlake/bootblock/pch.c:

https://review.coreboot.org/c/coreboot/+/83354/comment/8c48e802_6283698a?usp... :
PS8, Line 1: /* SPDX-License-Identifier: GPL-2.0-only */
> why you are not following the SOC proper nomenclature here? […]

Acknowledged

https://review.coreboot.org/c/coreboot/+/83354/comment/2fc364b0_bc95b397?usp... :
PS8, Line 40: soc
> shouldn't this be like `pcd_die_config_pwrmbase`

Acknowledged

https://review.coreboot.org/c/coreboot/+/83354/comment/c3be0b1b_a479a7f2?usp... :
PS8, Line 58: pch
> same feedback as above, please use one nomenclature as applicable and don't mix pch, soc and pcd int […]

Acknowledged

https://review.coreboot.org/c/coreboot/+/83354/comment/7eb00909_0e8a7cf2?usp... :
PS8, Line 70: 	/* Program generic IO Decode Range */
            : 	pch_enable_lpc();
> this function is inside `espi. […]

Acknowledged

https://review.coreboot.org/c/coreboot/+/83354/comment/4e138357_348e1923?usp... :
PS8, Line 74: void bootblock_pch_early_init(void)
> why not static ?

Acknowledged

https://review.coreboot.org/c/coreboot/+/83354/comment/4ade1359_69df3b06?usp... :
PS8, Line 74: pch
> same as above

Acknowledged

https://review.coreboot.org/c/coreboot/+/83354/comment/366c837a_4e05fa0f?usp... :
PS8, Line 76: soc_die_early_sa_init
> you are calling soc_die_early_sa_init() function twice ? […]

Acknowledged

https://review.coreboot.org/c/coreboot/+/83354/comment/d38104ee_79524c5d?usp... :
PS8, Line 89: SoC
> update the comment to fit into existing SoC schema

Acknowledged

https://review.coreboot.org/c/coreboot/+/83354/comment/415d9577_ed7fb92f?usp... :
PS8, Line 95: void soc_die_early_sa_init(void)
> why not static ?

Acknowledged

https://review.coreboot.org/c/coreboot/+/83354/comment/9f146618_1b159d13?usp... :
PS8, Line 107: soc
> same as above

Acknowledged

https://review.coreboot.org/c/coreboot/+/83354/comment/56589d9c_48f4f08c?usp... :
PS8, Line 123: soc
> same as above

Acknowledged

https://review.coreboot.org/c/coreboot/+/83354/comment/1195498f_80d5236f?usp... :
PS8, Line 143: pch
> same as above

Acknowledged

https://review.coreboot.org/c/coreboot/+/83354/comment/f27eafa8_c05a251c?usp... :
PS8, Line 149: soc
> same as above

Acknowledged

File src/soc/intel/pantherlake/bootblock/report_platform.c:

https://review.coreboot.org/c/coreboot/+/83354/comment/40aeb649_0a2b085f?usp... :
PS8, Line 18:
> extra newline

Acknowledged

https://review.coreboot.org/c/coreboot/+/83354/comment/dbbde2c2_3481a0b5?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. […]

Acknowledged

https://review.coreboot.org/c/coreboot/+/83354/comment/42697c22_30e09c16?usp... :
PS8, Line 38: P
> based on my understanding PTL has U/H and H x4e so, technically there is nothing called `PTL-P`? […]

Acknowledged

https://review.coreboot.org/c/coreboot/+/83354/comment/3e15b3d3_0587faac?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

Acknowledged

https://review.coreboot.org/c/coreboot/+/83354/comment/677de259_ce57e8b7?usp... :
PS8, Line 59: P
> same

Acknowledged

https://review.coreboot.org/c/coreboot/+/83354/comment/82923c49_cfff8490?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 redun […]

Acknowledged

https://review.coreboot.org/c/coreboot/+/83354/comment/0e37a1f4_35194256?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`.

Acknowledged

File src/soc/intel/pantherlake/include/soc/bootblock.h:

https://review.coreboot.org/c/coreboot/+/83354/comment/e8bee037_802196ef?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

Acknowledged

https://review.coreboot.org/c/coreboot/+/83354/comment/9cd2b149_e20e200a?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

Acknowledged

File src/soc/intel/pantherlake/include/soc/iomap.h:

https://review.coreboot.org/c/coreboot/+/83354/comment/e16d192b_0d022b2d?usp... :
PS8, Line 27: 
            : //PMC MBAR 64KB
> seems unnecessary

Acknowledged

https://review.coreboot.org/c/coreboot/+/83354/comment/0adf4445_815d868d?usp... :
PS8, Line 36: //VTD BAR 512KB
> seems unnecessary

Acknowledged

https://review.coreboot.org/c/coreboot/+/83354/comment/161c2a1d_0b260148?usp... :
PS8, Line 37: fc800000
> address 0xfed90000 with 8KB

Acknowledged

https://review.coreboot.org/c/coreboot/+/83354/comment/13e1af64_a17533db?usp... :
PS8, Line 37: VTD_BASE_ADDRESS
> this is now called DMI3BAR (as per PTL FAS) […]

Acknowledged

https://review.coreboot.org/c/coreboot/+/83354/comment/2dd7b67f_1b45116b?usp... :
PS8, Line 48: 0xfe03e000
> CONFIG_CONSOLE_UART_BASE_ADDRESS

Acknowledged

https://review.coreboot.org/c/coreboot/+/83354/comment/45d50c37_3854d098?usp... :
PS8, Line 61: REGBAR 128MB
> comment and line #63 are not aligned

Acknowledged

https://review.coreboot.org/c/coreboot/+/83354/comment/98555804_047aa0c6?usp... :
PS8, Line 62: 0xd0000000
> as per FAS this range is 0xf0000000

Acknowledged

https://review.coreboot.org/c/coreboot/+/83354/comment/0c8bab00_a23f5727?usp... :
PS8, Line 63: 256
> should be 128 as per FAS

Acknowledged

https://review.coreboot.org/c/coreboot/+/83354/comment/65ea8e2a_351a7409?usp... :
PS8, Line 68: //PCH P2SB2 256MB
> if you wish to add a comment then follow coreboot style  […]

Acknowledged

https://review.coreboot.org/c/coreboot/+/83354/comment/90d762a3_642b5b59?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.  […]

Acknowledged, will be using exisitng MTL based P2SB (IOE Die based function) for PTL P2SB2 (>4GB)

https://review.coreboot.org/c/coreboot/+/83354/comment/f2d14872_89d6bb2d?usp... :
PS8, Line 74: 0x3fff0800000
> possible to reflect this range also inside FAS in upcoming revision ?

IOM should be anywhere of size 64KB.

https://review.coreboot.org/c/coreboot/+/83354/comment/d4eddbc5_28333d51?usp... :
PS8, Line 78: /*
            :  * I/O port address space
            :  */
> nit,  […]

Done

File src/soc/intel/pantherlake/include/soc/p2sb.h:

https://review.coreboot.org/c/coreboot/+/83354/comment/8aa423f5_526a9146?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 […]

Done

File src/soc/intel/pantherlake/include/soc/pci_devs.h:

https://review.coreboot.org/c/coreboot/+/83354/comment/19b4e5df_5d44f4df?usp... :
PS8, Line 3: _SOC_PANTHERLAKE_PCI_DEVS_H_
> follow one standard in all other header file but don't use interchangeably  […]

Done

https://review.coreboot.org/c/coreboot/+/83354/comment/2d1536b0_5f7a6514?usp... :
PS8, Line 34:
>

Done

https://review.coreboot.org/c/coreboot/+/83354/comment/2df11921_3848d491?usp... :
PS8, Line 46:
> #define PCI_DEV_SLOT_TELEMETRY	0x0a […]

Done

https://review.coreboot.org/c/coreboot/+/83354/comment/d9e17e43_6d003133?usp... :
PS8, Line 48: VPU
> NPU

Done

https://review.coreboot.org/c/coreboot/+/83354/comment/f7eec910_eb0424a7?usp... :
PS8, Line 50:
> please add IAA as well the SOC IP for CrOS

Done

https://review.coreboot.org/c/coreboot/+/83354/comment/e5732fb8_77c55ace?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

Done

https://review.coreboot.org/c/coreboot/+/83354/comment/1299dd0a_92a81bba?usp... :
PS8, Line 84:
> should add GSPI#2 as well

Done

https://review.coreboot.org/c/coreboot/+/83354/comment/6bc006c9_2c80f7b0?usp... :
PS8, Line 90: #define  PCI_DEVFN_IEH		_PCI_DEVFN(XHCI, 5)
> should be IEH_0  […]

Done

https://review.coreboot.org/c/coreboot/+/83354/comment/03d2eb36_fa63b00c?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

Done

https://review.coreboot.org/c/coreboot/+/83354/comment/1e98f848_8eba823b?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

Done

https://review.coreboot.org/c/coreboot/+/83354/comment/6f90103f_cb84e802?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 […]

Done

https://review.coreboot.org/c/coreboot/+/83354/comment/b99333a9_2938ca2d?usp... :
PS8, Line 219: PCH_DEV_P2SB2
> PCI_DEV_IOE_P2SB

Done

File src/soc/intel/pantherlake/include/soc/pcr_ids.h:

https://review.coreboot.org/c/coreboot/+/83354/comment/5802a90b_5e0a367d?usp... :
PS8, Line 5: /*
> start after one new line

Done

https://review.coreboot.org/c/coreboot/+/83354/comment/34ed568f_134a09c0?usp... :
PS8, Line 21:
> why gap?

Done

https://review.coreboot.org/c/coreboot/+/83354/comment/c178db73_b55466b1?usp... :
PS8, Line 32:  //SOC_PANTHERLAKE_PCR_H
> not needed

Done

File src/soc/intel/pantherlake/include/soc/pm.h:

https://review.coreboot.org/c/coreboot/+/83354/comment/3fbcbcc3_bca828aa?usp... :
PS8, Line 3: _SOC_PM_H_
> please use PTL specific header

Done

File src/soc/intel/pantherlake/include/soc/smbus.h:

https://review.coreboot.org/c/coreboot/+/83354/comment/f50c0db9_4087aa82?usp... :
PS8, Line 3: _SOC_SMBUS_H_
> same here

Done