Attention is currently required from: Cliff Huang, Jérémy Compostella, Kapil Porwal, Krishna P Bhat D, Pranava Y N, Saurabh Mishra.
Subrata Banik 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 93:
(26 comments)
File src/soc/intel/pantherlake/Makefile.mk:
https://review.coreboot.org/c/coreboot/+/83798/comment/c24c5f21_aecb670a?usp... : PS93, Line 21: bootblock-y += gpio.c can u please drop this now ? knowing line 15 already added those files ?
https://review.coreboot.org/c/coreboot/+/83798/comment/87528a33_68f01851?usp... : PS93, Line 26: romstage-y += gpio.c can u please drop this now ? knowing line 15 already added those files ?
File src/soc/intel/pantherlake/acpi.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/cc54038b_69b9dc64?usp... : PS93, Line 25: #define GFXVT_ENABLED BIT(0) : #define NONGFXVT_ENABLED BIT(1) : #define IOCVT_ENABLED BIT(2) please move macro definitions under `soc/intel/pantherlake/include/soc/systemagent.h`
``` #define GFXVTBAR VTDBAR #define GFXVT_ENABLED BIT(0) #define NONGFXVT_ENABLED BIT(1) #define IOCVT_ENABLED BIT(2) ```
https://review.coreboot.org/c/coreboot/+/83798/comment/c48e67e3_cf7f2bd1?usp... : PS93, Line 123: initialized nit: then you don't need to use a comment to explain (line 123 and 148)
`c_state_initialized`
https://review.coreboot.org/c/coreboot/+/83798/comment/d3ff48bf_1e958c8e?usp... : PS93, Line 123: int why not `bool`?
https://review.coreboot.org/c/coreboot/+/83798/comment/fbf579af_e11053a5?usp... : PS93, Line 190: tab
File src/soc/intel/pantherlake/chip.h:
https://review.coreboot.org/c/coreboot/+/83798/comment/82b4021f_608382ed?usp... : PS93, Line 339: heci_enabled who is the consumer or who is expected to be the consumer of this code?
https://review.coreboot.org/c/coreboot/+/83798/comment/c5399c77_ec21bfc8?usp... : PS93, Line 522: pch_usb_oc_enable who is the consumer?
File src/soc/intel/pantherlake/chip.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/d245fd28_bcf2346a?usp... : PS93, Line 21: #include <soc/itss.h> why we need this ?
refer to https://review.coreboot.org/c/coreboot/+/84183
https://review.coreboot.org/c/coreboot/+/83798/comment/a312b72a_041d2895?usp... : PS93, Line 115: case PCI_DEVFN_SPI: return "FSPI"; is this required ?
File src/soc/intel/pantherlake/chipset.cb:
https://review.coreboot.org/c/coreboot/+/83798/comment/a5190198_f433684f?usp... : PS93, Line 12: please be consistent with tab or space
https://review.coreboot.org/c/coreboot/+/83798/comment/bd650f14_2b8a819e?usp... : PS93, Line 18: same as above
File src/soc/intel/pantherlake/cpu.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/21df6bc6_0599806d?usp... : PS93, Line 59: /* Fast String enable */ with introduction of the macro, I felt these comments are redundant ones,
https://review.coreboot.org/c/coreboot/+/83798/comment/5f58e8e3_6757d2af?usp... : PS93, Line 126: const config_t *conf = config_of_soc(); : if (conf == NULL) { : printk(BIOS_ERR, "Error: Configuration could not be retrieved.\n"); : return; : } : /* Set energy-performance preference */ : if (conf->enable_energy_perf_pref) : if (check_energy_perf_cap()) : set_energy_perf_pref(conf->energy_perf_pref_value); : looks like returning from this function won't be appropriate looking at remaining operations like enabling turbo etc. (those are not dependent over config). Therefore, I would suggest using below logic.
``` const config_t *conf = config_of_soc(); /* Set energy-performance preference */ if (conf != NULL && conf->enable_energy_perf_pref) { if (check_energy_perf_cap()) set_energy_perf_pref(conf->energy_perf_pref_value); } ```
https://review.coreboot.org/c/coreboot/+/83798/comment/5ffd4617_e9fa9856?usp... : PS93, Line 144: empty line.
https://review.coreboot.org/c/coreboot/+/83798/comment/59c1eb2b_83765cde?usp... : PS93, Line 144: Add a TODO to add support for DROP_CPU_FEATURE_PROGRAM_IN_FSP
File src/soc/intel/pantherlake/crashlog.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/cc71be2d_f0a33767?usp... : PS93, Line 18: SoC-N there is no SoC die. Please confirm if this node count is appropriate for PTL
File src/soc/intel/pantherlake/elog.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/34130392_497fb58c?usp... : PS93, Line 12: #include <stdint.h> drop this , seems not required
File src/soc/intel/pantherlake/finalize.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/9b92bd59_0f626725?usp... : PS93, Line 7: #include <delay.h> do we need this ?
https://review.coreboot.org/c/coreboot/+/83798/comment/e937687c_6f180997?usp... : PS93, Line 28: static void pch_handle_sideband(config_t *config) : { : : } can you please follow https://review.coreboot.org/c/coreboot/+/84189 ?
File src/soc/intel/pantherlake/include/soc/dptf.h:
https://review.coreboot.org/c/coreboot/+/83798/comment/1bbc7c29_46624ae9?usp... : PS93, Line 6: /* Below are the unique ACPI Device IDs for thermal/dptf on SoC. */ this is not a complete sentence IMO. can you please help to rewrite ?
File src/soc/intel/pantherlake/include/soc/itss.h:
https://review.coreboot.org/c/coreboot/+/83798/comment/7fbc2bde_e00b1aac?usp... : PS93, Line 3: TSS_H please follow https://review.coreboot.org/c/coreboot/+/84183
File src/soc/intel/pantherlake/pcie_rp.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/d71d322c_a83c458f?usp... : PS93, Line 3: #include <console/console.h> i don't see a need for console here ?
File src/soc/intel/pantherlake/pmc.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/cd132a89_4ee25c2e?usp... : PS93, Line 27: Pmc PMC in all capital
https://review.coreboot.org/c/coreboot/+/83798/comment/7c8baebb_61b171eb?usp... : PS93, Line 27: mmio MMIO
File src/soc/intel/pantherlake/retimer.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/fb8c46dc_6a1ef1d6?usp... : PS93, Line 7: #include <soc/soc_info.h> drop this, not needed it seems