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

[Saurabh Mishra (Code Review)]

Attention is currently required from: Cliff Huang, Jérémy Compostella, Ravishankar Sarawadi, Subrata Banik.

Saurabh Mishra has posted comments on this change by Saurabh Mishra. ( https://review.coreboot.org/c/coreboot/+/83635?usp=email )

Change subject: soc/intel/ptl: Do initial Panther Lake SoC commit till romstage
......................................................................

Patch Set 66:

(17 comments)

File src/soc/intel/pantherlake/Kconfig:

https://review.coreboot.org/c/coreboot/+/83635/comment/c2e9cfec_ee03673d?usp... :
PS64, Line 102: config MAX_TBT_ROOT_PORTS
              : 	int
              : 	default 4
              : 
              : config MAX_ROOT_PORTS
              : 	int
              : 	default 12
              : 
              : config MAX_PCIE_CLOCK_SRC
              : 	int
              : 	default 9
              :
> redundant entries between line 168-178

Acknowledged

https://review.coreboot.org/c/coreboot/+/83635/comment/cf883933_e9706b98?usp... :
PS64, Line 152: config SOC_INTEL_I3C_DEV_MAX
              : 	int
              : 	default 2
> I don't see any consumer

Acknowledged

https://review.coreboot.org/c/coreboot/+/83635/comment/3f86283b_1c1e138d?usp... :
PS64, Line 227: config FSP_FD_PATH
              : 	string
              : 	depends on FSP_USE_REPO
              : 	default "3rdparty/fsp/PantherLakeFspBinPkg/Fsp.fd"
> you don't need this now. […]

Acknowledged

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

https://review.coreboot.org/c/coreboot/+/83635/comment/e2ce5b7d_d0699332?usp... :
PS64, Line 13: bootblock-y += espi.c
> should be after line 11

Acknowledged

https://review.coreboot.org/c/coreboot/+/83635/comment/218635a8_7837b542?usp... :
PS64, Line 14: p2sb.c
> p2sb.c is a PCI driver, ideally only required in ramstage and smm. […]

Acknowledged

https://review.coreboot.org/c/coreboot/+/83635/comment/08ef74fa_9f997073?usp... :
PS64, Line 19:
> You will be needing below files as well. Please consider adding. […]

Acknowledged

File src/soc/intel/pantherlake/chip.h:

https://review.coreboot.org/c/coreboot/+/83635/comment/aeaabe26_5a12bd16?usp... :
PS64, Line 63: /*
             :  * 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,
             : };
             : 
             : /* Bit values for use in LpmStateEnableMask. */
             : enum lpm_state_mask {
             : 	LPM_S0i2_0 = BIT(0),
             : 	LPM_S0i2_1 = BIT(1),
             : 	LPM_S0i2_2 = BIT(2),
             : 	LPM_S0i3_0 = BIT(3),
             : 	LPM_S0i3_1 = BIT(4),
             : 	LPM_S0i3_2 = BIT(5),
             : 	LPM_S0i3_3 = BIT(6),
             : 	LPM_S0i3_4 = BIT(7),
             : 	LPM_S0iX_ALL = LPM_S0i2_0 | LPM_S0i2_1 | LPM_S0i2_2
             : 		     | LPM_S0i3_0 | LPM_S0i3_1 | LPM_S0i3_2 | LPM_S0i3_3 | LPM_S0i3_4,
             : };
             :
> i don't think these are even needed for romstage code (may be add for ramstage entries)

Ack, moved to Ramstage.

File src/soc/intel/pantherlake/chipset.cb:

https://review.coreboot.org/c/coreboot/+/83635/comment/9fd30112_4d5719f1?usp... :
PS64, Line 5: PTL_H_POWER_LIMITS
> For CrOS side, PTL-UH is POR and not PTL-H hence, please ensure adding correct config

Sure, ack.

https://review.coreboot.org/c/coreboot/+/83635/comment/564782a0_e943db52?usp... :
PS64, Line 6: 9
> is this a 9W silicon ? […]

I have corrected the values. Processor Base Power (PBP) is 15W. Ref.Doc #823589

https://review.coreboot.org/c/coreboot/+/83635/comment/1c66a104_132e0215?usp... :
PS64, Line 7: 		.tdp_pl2_override = 30,
> why not pl4?

Added.

https://review.coreboot.org/c/coreboot/+/83635/comment/7ffb1acb_29c794c5?usp... :
PS64, Line 63:
> why space?

Acknowledged

https://review.coreboot.org/c/coreboot/+/83635/comment/4cb93616_9f947846?usp... :
PS64, Line 107: 					chip drivers/usb/acpi
              : 						device usb 2.8 alias usb2_port9 off end
              : 					end
              : 					chip drivers/usb/acpi
              : 						device usb 2.9 alias usb2_port10 off end
              : 					end
> i thought u have put 8 USB2. […]

Acknowledged

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

https://review.coreboot.org/c/coreboot/+/83635/comment/5e0dded2_29e93207?usp... :
PS64, Line 92: UserBd
> use snakecase and not camel case

Acknowledged

https://review.coreboot.org/c/coreboot/+/83635/comment/e571bd00_a8af46e2?usp... :
PS64, Line 95: CmdMirror
> same

Acknowledged

https://review.coreboot.org/c/coreboot/+/83635/comment/9c800ea5_7a828731?usp... :
PS64, Line 98: LpDdrDqDqsReTraining
> same

Acknowledged

File src/soc/intel/pantherlake/romstage/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/83635/comment/af4d4a7a_ba05a544?usp... :
PS64, Line 135: soc_memory_init_params
> delete line 24-161

Acknowledged

https://review.coreboot.org/c/coreboot/+/83635/comment/e2b7af2c_869d77c6?usp... :
PS64, Line 165: 	const struct soc_intel_pantherlake_config *config;
              : 	FSP_M_CONFIG *m_cfg = &mupd->FspmConfig;
              : 	FSPM_ARCH2_UPD *arch_upd = &mupd->FspmArchUpd;
              : 	if (CONFIG(FSP_USES_CB_DEBUG_EVENT_HANDLER)) {
              : 		if (CONFIG(CONSOLE_SERIAL) && CONFIG(FSP_ENABLE_SERIAL_DEBUG)) {
              : 			enum fsp_log_level log_level = fsp_map_console_log_level();
              : 			arch_upd->FspEventHandler = (uintptr_t)((FSP_EVENT_HANDLER *)
              : 					fsp_debug_event_handler);
              : 			/* Set Serial debug message level */
              : 			(void)log_level; /*TODO: Update with actual implementation */
              : 			/* Set MRC debug level */
              : 		} else {
              : 			/* Disable Serial debug message */
              : 			/* Disable MRC debug message */
              : 		}
              : 	}
              : 	config = config_of_soc();
              : 
              : 	soc_memory_init_params(m_cfg, config);
              : 	mainboard_memory_init_params(mupd);
> doesn't make sense to have multiple TODO w/o actual UPD, better you don't implement this function an […]

Acknowledged