[XL] Change in coreboot[main]: soc/intel/ptl: Do initial Panther Lake SoC commit till ramstage
Attention is currently required from: Cliff Huang, Kapil Porwal, Pranava Y N, Subrata Banik.
Saurabh Mishra 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 52:
(27 comments)
File src/soc/intel/pantherlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/83798/comment/af284899_a6aa4479?usp... : PS38, Line 2:
Added all mentioned, except "SOC_INTEL_COMMON_BLOCK_ACPI_CPU_HYBRID" […]
Okay, added.
https://review.coreboot.org/c/coreboot/+/83798/comment/4d1f28ac_48aeb548?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
Sure, have updated the comment as per below updated values.
https://review.coreboot.org/c/coreboot/+/83798/comment/76816c24_bae9d5c2?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
Updated the values. There is no change from previous platform.
File src/soc/intel/pantherlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/83798/comment/1ed7f895_3ffb4d51?usp... : PS50, Line 21: FSP_UGOP_EARLY_SIGN_OF_LIFE
do you wish to select this now better add it when you have capable FSP to support uGOP?
Sure, marking it TODO in crosbug.
https://review.coreboot.org/c/coreboot/+/83798/comment/1d7b309b_feac0127?usp... : PS50, Line 69: SOC_INTEL_COMMON_BLOCK_ME_SPEC_18
I don't believe the ME spec is 18 for PTL. Please submit the common code changes for cse_spec. […]
Sure.
File src/soc/intel/pantherlake/chip.h:
https://review.coreboot.org/c/coreboot/+/83798/comment/e057c565_6ce3ddaf?usp... : PS50, Line 43: PTL_U_404_15W_CORE, : PTL_H_484_25W_CORE, : PTL_H_484_45W_CORE,
Why are you changing this macro? Without an explicit comment, it is difficult to follow what the mag […]
Hi Subrata, 404 means =(4 P-cores +0 E-cores + 4 LP E-cores) & 484 mean =(4 P-cores +8 E-cores + 4 LP Ecores).
Is it Ok to carry forwaed this naming? About keeping comment, i have add them. Please let me know if this works.
https://review.coreboot.org/c/coreboot/+/83798/comment/93a5d48a_84270a8d?usp... : PS50, Line 51: TDP_15W = 15
then you should have added other TDP macros as well ? as per my understanding we are actually start […]
Sure, i have added 25W for PTL-UH Sku.
https://review.coreboot.org/c/coreboot/+/83798/comment/bfce8339_355d5c3c?usp... : PS50, Line 60: PCI_DID_INTEL_PTL_U_ID_1
based on my understanding, we will also use `PCI_DID_INTEL_PTL_H_ID_1` for sometime (till next year) […]
Sure, added the corresponding PTL-H amd its support config.
https://review.coreboot.org/c/coreboot/+/83798/comment/21167e8d_e6d72162?usp... : PS50, Line 344: hybrid_storage_mode
this is not applicable since MTL. […]
Removed, no UPD consumer in PTL.
https://review.coreboot.org/c/coreboot/+/83798/comment/63cffe22_6936cad6?usp... : PS50, Line 363: dmi_pwr_optimize_disable
who is the consumer ? (I don't see anything during MTL). […]
Removed, no consumer in PTL as well.
https://review.coreboot.org/c/coreboot/+/83798/comment/ede4b9a7_e6120228?usp... : PS50, Line 408: };
during MTL, we had one useful UPD `lower_basic_mem_test_size` which is now not listed under PTL chip […]
lower_basic_mem_test_size is avaliable in PTL. Added to chip.h.
As suggested, i have added the useful UPDs that existed into MTL. Will capture thise in to TODO crosbug for its updation.
File src/soc/intel/pantherlake/chip.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/35985f94_1ddb32b1?usp... : PS50, Line 12: #include <intelblocks/gpio.h>
do we need this header ?
Not required. Removed.
https://review.coreboot.org/c/coreboot/+/83798/comment/ac17c77c_6d5fc581?usp... : PS50, Line 165: config
don;t we need the NULL check ?
Added the NULL check.
File src/soc/intel/pantherlake/chipset.cb:
https://review.coreboot.org/c/coreboot/+/83798/comment/c710c57f_ad0eb88a?usp... : PS50, Line 10:
can you please add a power limit entry for 25W SoC? we will be using PTL-UH for sometime
Sure, i have added all the power limit entries for all avaliable SKUs.
https://review.coreboot.org/c/coreboot/+/83798/comment/8cb24823_509e59af?usp... : PS50, Line 119: shared_sram
isn't this is still known as `pmc_shared_sram` ?
Changed to pmc_shared_sram.
File src/soc/intel/pantherlake/cpu.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/35307601_a1503a40?usp... : PS50, Line 165: conf
check for the NULL pointer
Sure, added.
File src/soc/intel/pantherlake/elog.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/641c5f0d_b46ff7ff?usp... : PS50, Line 44:
use tab?
Acknowledged
File src/soc/intel/pantherlake/include/soc/crashlog.h:
https://review.coreboot.org/c/coreboot/+/83798/comment/8d763305_e5faa20a?usp... : PS50, Line 11: #define TEL_DVSEC_NEXT_CAP 0x2
same
No consumer. Removed.
https://review.coreboot.org/c/coreboot/+/83798/comment/19e954b9_bfacd934?usp... : PS50, Line 21: #define CRASHLOG_POINTER_SIZE_FIELD_OFFSET 0x04
who is the consumer ?
No consumer. Removed.
File src/soc/intel/pantherlake/include/soc/systemagent.h:
https://review.coreboot.org/c/coreboot/+/83798/comment/218193bf_d50af2f8?usp... : PS50, Line 53: DSM_BASE_ADDR_REG
why we need this when we have below macro in common code systemagent.h […]
Ack, changed to use BDSM.
https://review.coreboot.org/c/coreboot/+/83798/comment/001e101c_64544c2f?usp... : PS50, Line 54: DPR_REG
same true for […]
Ack, Removed.
File src/soc/intel/pantherlake/p2sb.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/1d693119_bfddc721?usp... : PS50, Line 53: soc
as this is local SOC code, can you please check if you can rename `soc_p2sb_ops` to `pcd_p2sb_ops` ?
Sure, updated to pcd_p2sb_ops
File src/soc/intel/pantherlake/pmutil.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/727e255f_0bc2d4f7?usp... : PS50, Line 32:
same
Acknowledged
https://review.coreboot.org/c/coreboot/+/83798/comment/5aade248_417f70ea?usp... : PS50, Line 64:
same
Acknowledged
https://review.coreboot.org/c/coreboot/+/83798/comment/378cca77_a040ef82?usp... : PS50, Line 91:
avoid empty line
Sure, removed.
https://review.coreboot.org/c/coreboot/+/83798/comment/4d8cf747_47c4c143?usp... : PS50, Line 249: return (uint16_t) ACPI_BASE_ADDRESS;
Acknowledged
File src/soc/intel/pantherlake/systemagent.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/14cc3fd2_932a8e7f?usp... : PS50, Line 103: DSM_BASE_ADDR_REG
`BDSM` is enough.
Sure, updated to use BDSM from <intelblocks/systemagent.h>
-
Saurabh Mishra (Code Review)