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

[Subrata Banik (Code Review)]

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

Subrata Banik 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 64:

(14 comments)

File src/soc/intel/pantherlake/Kconfig:

https://review.coreboot.org/c/coreboot/+/83635/comment/9748759a_c167919b?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

https://review.coreboot.org/c/coreboot/+/83635/comment/2be57812_069006a2?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. folks add that to pull FSP blobs from Intel FSP github post FSI

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

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

https://review.coreboot.org/c/coreboot/+/83635/comment/b2ef0e35_b58bac02?usp... :
PS64, Line 14: p2sb.c
p2sb.c is a PCI driver, ideally only required in ramstage and smm. I don't understand why you have added p2sb.c

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

https://review.coreboot.org/c/coreboot/+/83635/comment/eefe146a_33a70372?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)

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

https://review.coreboot.org/c/coreboot/+/83635/comment/14a0480e_5e1c1e9f?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

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

https://review.coreboot.org/c/coreboot/+/83635/comment/3bff94ea_42749474?usp... :
PS64, Line 63:                                                 
why space?

https://review.coreboot.org/c/coreboot/+/83635/comment/33c0a78b_57f054b8?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.0 port then why adding 10 here ?

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

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

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

https://review.coreboot.org/c/coreboot/+/83635/comment/00d846a3_582f0f04?usp... :
PS64, Line 98: LpDdrDqDqsReTraining
same

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

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

https://review.coreboot.org/c/coreboot/+/83635/comment/e4e49d90_66ce086e?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 and keep just a placeholde.