[XL] Change in coreboot[main]: soc/intel/ptl: Do initial Panther Lake SoC commit till bootblock
Attention is currently required from: Ashish Kumar Mishra, Dinesh Gehlot, Elyes Haouas, Eran Mitrani, Felix Singer, Jakub Czapiga, Jérémy Compostella, Kapil Porwal, Ravishankar Sarawadi, Saurabh Mishra, Tarun.
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 51:
(14 comments)
File src/soc/intel/pantherlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/83354/comment/122cedc9_c9e6eb69?usp... : PS51, Line 43: GFx GFX ?
File src/soc/intel/pantherlake/bootblock/pcd.c:
https://review.coreboot.org/c/coreboot/+/83354/comment/240b62b4_97a26e78?usp... : PS51, Line 17: #include <soc/bootblock.h> please follow order
https://review.coreboot.org/c/coreboot/+/83354/comment/fb3d3e9c_42cc3ff6?usp... : PS51, Line 26: #define PCR_PSF8_TO_SHDW_PMC_REG_BASE 0xA80 : #define PCR_PSFX_TO_SHDW_BAR0 0 : #define PCR_PSFX_TO_SHDW_BAR1 0x4 : #define PCR_PSFX_TO_SHDW_BAR2 0x8 : #define PCR_PSFX_TO_SHDW_BAR3 0xC : #define PCR_PSFX_TO_SHDW_BAR4 0x10 : #define PCR_PSFX_TO_SHDW_PCIEN_IOEN 0x01 : #define PCR_PSFX_T0_SHDW_PCIEN 0x1C ```suggestion #define PCR_PSF8_TO_SHDW_PMC_REG_BASE 0xA80 #define PCR_PSFX_TO_SHDW_BAR0 0 #define PCR_PSFX_TO_SHDW_BAR1 0x4 #define PCR_PSFX_TO_SHDW_BAR2 0x8 #define PCR_PSFX_TO_SHDW_BAR3 0xC #define PCR_PSFX_TO_SHDW_BAR4 0x10 #define PCR_PSFX_TO_SHDW_PCIEN_IOEN 0x01 #define PCR_PSFX_T0_SHDW_PCIEN 0x1C ```
https://review.coreboot.org/c/coreboot/+/83354/comment/0927f00b_d1fe67fd?usp... : PS51, Line 35: #define PCR_DMI_ACPIBA 0x27B4 : #define PCR_DMI_ACPIBDID 0x27B8 : #define PCR_DMI_PMBASEA 0x27AC : #define PCR_DMI_PMBASEC 0x27B0 remove these macros, not in use
https://review.coreboot.org/c/coreboot/+/83354/comment/ca280315_1c5020df?usp... : PS51, Line 130: /* PCR_PSF8_TO_SHDW_PMC_REG_BASE is not proper or needs reprogramming */ why we need this ?
https://review.coreboot.org/c/coreboot/+/83354/comment/5fa22c76_e5233525?usp... : PS51, Line 131: printk(BIOS_WARNING, "PMC PCR_PSF8 not properly aligned: pmc_reg_value=%x\n", pmc_reg_value); ```suggestion printk(BIOS_WARNING, "PCR_PSFX_TO_SHDW_BAR4 has not been programmed. \n"); ```
File src/soc/intel/pantherlake/bootblock/report_platform.c:
https://review.coreboot.org/c/coreboot/+/83354/comment/a6fae239_f18163ab?usp... : PS51, Line 109: GT2_1 ```suggestion { PCI_DID_INTEL_PTL_U_GT2_1, "Pantherlake-U GT2" }, ```
https://review.coreboot.org/c/coreboot/+/83354/comment/9fba6331_e77f5d0c?usp... : PS51, Line 110: GT2_1 same as above
File src/soc/intel/pantherlake/include/soc/iomap.h:
https://review.coreboot.org/c/coreboot/+/83354/comment/fca66e0d_a8dad49d?usp... : PS51, Line 19: #define SAF_BASE_ADDRESS 0x3ffe000000 : #define SAF_BASE_SIZE 0x2000000 I thought you told me this address has now moved within < 4GB range but you have still kept a > 4GB value. is that to address my review comment ?
File src/soc/intel/pantherlake/include/soc/pcr_ids.h:
https://review.coreboot.org/c/coreboot/+/83354/comment/23cf7b63_475e6848?usp... : PS51, Line 14: #define PID_PSF15 0xB4 : #define PID_PSF14 0xB3 : #define PID_PSF8 0xB2 : #define PID_PSF6 0xB1 : #define PID_PSF4 0xB0 : #define PID_PSF0 0xB5 : #define PID_CSME0 0x40 : #define PID_PSTH 0x6A : #define PID_ITSS 0x69 : #define PID_RTC 0x6C : #define PID_ISCLK 0x72 : #define PID_IOM 0x80 : #define PID_DMI 0x2F : #define PID_NPK 0x8C : #define PID_XHCI 0x3A ```suggestion #define PID_DMI 0x2F #define PID_XHCI 0x3A #define PID_CSME0 0x40 #define PID_GPIOCOM0 0x59 #define PID_GPIOCOM1 0x5A #define PID_GPIOCOM3 0x5B #define PID_GPIOCOM4 0x5C #define PID_GPIOCOM5 0x5D #define PID_ITSS 0x69 #define PID_PSTH 0x6A #define PID_RTC 0x6C #define PID_ISCLK 0x72 #define PID_IOM 0x80 #define PID_NPK 0x8C #define PID_PSF4 0xB0 #define PID_PSF6 0xB1 #define PID_PSF8 0xB2 #define PID_PSF14 0xB3 #define PID_PSF15 0xB4 #define PID_PSF0 0xB5 ```
File src/soc/intel/pantherlake/include/soc/pm.h:
https://review.coreboot.org/c/coreboot/+/83354/comment/d590bb97_80c15d52?usp... : PS51, Line 76: #define GPE_31_0 0 /* 0x60/0x70 = GPE[31:0] */ : #define GPE_63_32 1 /* 0x64/0x74 = GPE[63:32] */ : #define GPE_95_64 2 /* 0x68/0x78 = GPE[95:64] */ : #define GPE_STD 3 /* 0x6c/0x7c = Standard GPE */ : #define GPE_STS_RSVD GPE_STD : #define WADT_STS BIT(18) : #define GPIO_T2_STS BIT(15) : #define ESPI_STS BIT(14) : #define PME_B0_STS BIT(13) : #define ME_SCI_STS BIT(12) : #define PME_STS BIT(11) : #define BATLOW_STS BIT(10) : #define PCI_EXP_STS BIT(9) : #define SMB_WAK_STS BIT(7) : #define TCOSCI_STS BIT(6) : #define SWGPE_STS BIT(2) : #define HOT_PLUG_STS BIT(1) ```suggestion #define GPE_31_0 0 /* 0x60/0x70 = GPE[31:0] */ #define GPE_63_32 1 /* 0x64/0x74 = GPE[63:32] */ #define GPE_95_64 2 /* 0x68/0x78 = GPE[95:64] */ #define GPE_STD 3 /* 0x6c/0x7c = Standard GPE */ #define GPE_STS_RSVD GPE_STD #define WADT_STS BIT(18) #define GPIO_T2_STS BIT(15) #define ESPI_STS BIT(14) #define PME_B0_STS BIT(13) #define ME_SCI_STS BIT(12) #define PME_STS BIT(11) #define BATLOW_STS BIT(10) #define PCI_EXP_STS BIT(9) #define SMB_WAK_STS BIT(7) #define TCOSCI_STS BIT(6) #define SWGPE_STS BIT(2) #define HOT_PLUG_STS BIT(1) ```
https://review.coreboot.org/c/coreboot/+/83354/comment/385b86cb_15273a04?usp... : PS51, Line 94: #define WADT_EN BIT(18) : #define GPIO_T2_EN BIT(15) : #define ESPI_EN BIT(14) : #define PME_B0_EN_BIT 13 : #define PME_B0_EN BIT(PME_B0_EN_BIT) : #define ME_SCI_EN BIT(12) : #define PME_EN BIT(11) : #define BATLOW_EN BIT(10) : #define PCI_EXP_EN BIT(9) : #define TCOSCI_EN BIT(6) : #define SWGPE_EN BIT(2) : #define HOT_PLUG_EN BIT(1) ```suggestion #define WADT_EN BIT(18) #define GPIO_T2_EN BIT(15) #define ESPI_EN BIT(14) #define PME_B0_EN_BIT 13 #define PME_B0_EN BIT(PME_B0_EN_BIT) #define ME_SCI_EN BIT(12) #define PME_EN BIT(11) #define BATLOW_EN BIT(10) #define PCI_EXP_EN BIT(9) #define TCOSCI_EN BIT(6) #define SWGPE_EN BIT(2) #define HOT_PLUG_EN BIT(1) ```
https://review.coreboot.org/c/coreboot/+/83354/comment/bcd73280_0bde859e?usp... : PS51, Line 106: : #define EN_BLOCK 3 there is no user i assume
File src/soc/intel/pantherlake/soc_info.c:
https://review.coreboot.org/c/coreboot/+/83354/comment/6a9e0864_1f276911?usp... : PS51, Line 26: CONFIG_MAX_TBT_ROOT_PORTS If you have added this Kconfig as part of this CL, then the default value should also be present in the same CL, correct?
the same logic applies to others as well
-
Subrata Banik (Code Review)