[XL] Change in coreboot[main]: soc/intel/ptl: Do initial Panther Lake SoC commit till ramstage
Attention is currently required from: Jérémy Compostella, 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 63:
(17 comments)
File src/soc/intel/pantherlake/acpi.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/7b1646ad_26f07d4a?usp... : PS50, Line 120: int
size_t
Acknowledged
https://review.coreboot.org/c/coreboot/+/83798/comment/f331a81c_43a18ada?usp... : PS50, Line 126: is_s0ix_enable
I don't see the benefit of this intermediary variable.
optimized to use "config->s0ix_enable".
https://review.coreboot.org/c/coreboot/+/83798/comment/3b052b2e_635e364f?usp... : PS50, Line 138: map
As this is returning an inner static variable, shouldn't we have a mechanism to make sure we do not […]
If the configuration is static, we can use below code.
const acpi_cstate_t *soc_get_cstate_map(size_t *entries) { static acpi_cstate_t map[MAX(ARRAY_SIZE(cstate_set_s0ix), ARRAY_SIZE(cstate_set_non_s0ix))]; static int initialized = 0; /* Flag to check if map has been initialized */ size_t i;
if (!initialized) { config_t *config = config_of_soc(); if (config == NULL) { printk(BIOS_ERR, "Error: Configuration could not be retrieved.\n"); return NULL; }
int *set; if (config->s0ix_enable) { *entries = ARRAY_SIZE(cstate_set_s0ix); set = cstate_set_s0ix; } else { *entries = ARRAY_SIZE(cstate_set_non_s0ix); set = cstate_set_non_s0ix; }
for (i = 0; i < *entries; i++) { map[i] = cstate_map[set[i]]; map[i].ctype = i + 1; }
initialized = 1; /* Set the flag to indicate initialization is done */ }
return map; }
Let me know, if that's the ask?
File src/soc/intel/pantherlake/chip.h:
https://review.coreboot.org/c/coreboot/+/83798/comment/0f303f3d_39ff7356?usp... : PS55, Line 7: #include <device/pci_ids.h>
Move line 7 up.
Acknowledged
https://review.coreboot.org/c/coreboot/+/83798/comment/4beaba41_5670f895?usp... : PS55, Line 46: PTL_U_404_4_15W_CORE, /* 4 P-cores +0 E-cores + 4 LP E-cores + 4Xe25W*/
And one space before ending `*/`
Acknowledged
https://review.coreboot.org/c/coreboot/+/83798/comment/f20e6338_08a0dacd?usp... : PS55, Line 47: PTL_H_484_12_25W_CORE
Instead of commenting out every single line, what about something like the following ? […]
Looks good to me. Hi Subrata, please advise?
https://review.coreboot.org/c/coreboot/+/83798/comment/6811dde4_de586b37?usp... : PS55, Line 339: uint8_t enable_c6dram; : uint8_t pm_timer_disabled;
following the logic of some fields above, shouldn't these two be booleans ?
Acknowledged
File src/soc/intel/pantherlake/chip.h:
https://review.coreboot.org/c/coreboot/+/83798/comment/d918b972_bc124e81?usp... : PS50, Line 7: #include <device/pci_ids.h>
Move that one one line up.
Acknowledged
File src/soc/intel/pantherlake/chip.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/3f8657b3_845621b1?usp... : PS55, Line 61: printk(BIOS_DEBUG, "dev->path.type=%x\n", dev->path.usb.port_type);
Shouldn't this be an `BIOS_ERR` ? […]
Updated as suggested.
https://review.coreboot.org/c/coreboot/+/83798/comment/693dae30_47935f75?usp... : PS55, Line 65: printk(BIOS_DEBUG, "dev->path.type=%x\n", dev->path.type);
Similar remark here.
Updated as suggested.
https://review.coreboot.org/c/coreboot/+/83798/comment/d422d26f_7191d7da?usp... : PS55, Line 84: case PCI_DEVFN_I2C0: return "I2C0"; : case PCI_DEVFN_I2C1: return "I2C1"; : case PCI_DEVFN_I2C2: return "I2C2"; : case PCI_DEVFN_I2C3: return "I2C3"; : case PCI_DEVFN_I2C4: return "I2C4"; : case PCI_DEVFN_I2C5: return "I2C5"; : case PCI_DEVFN_CSE: return "HECI"; : case PCI_DEVFN_PCIE1: return "RP01"; : case PCI_DEVFN_PCIE2: return "RP02"; : case PCI_DEVFN_PCIE3: return "RP03"; : case PCI_DEVFN_PCIE4: return "RP04"; : case PCI_DEVFN_PCIE5: return "RP05"; : case PCI_DEVFN_PCIE6: return "RP06"; : case PCI_DEVFN_PCIE7: return "RP07"; : case PCI_DEVFN_PCIE8: return "RP08"; : case PCI_DEVFN_PCIE9: return "RP09"; : case PCI_DEVFN_PCIE10: return "RP10"; : case PCI_DEVFN_PCIE11: return "RP11"; : case PCI_DEVFN_PCIE12: return "RP12"; : case PCI_DEVFN_PMC: return "PMC"; : case PCI_DEVFN_UART0: return "UAR0"; : case PCI_DEVFN_UART1: return "UAR1"; : case PCI_DEVFN_UART2: return "UAR2"; : case PCI_DEVFN_GSPI0: return "SPI0"; : case PCI_DEVFN_GSPI1: return "SPI1"; : /* Keeping ACPI device name coherent with ec.asl */ : case PCI_DEVFN_ESPI: return "LPCB"; : case PCI_DEVFN_HDA: return "HDAS"; : case PCI_DEVFN_SMBUS: return "SBUS"; : case PCI_DEVFN_SPI: return "FSPI"; : case PCI_DEVFN_GBE: return "GLAN";
Aren't those missing an extra tab ?
Acknowledged
https://review.coreboot.org/c/coreboot/+/83798/comment/f1c103cf_275e3781?usp... : PS55, Line 116: printk(BIOS_DEBUG, "Missing ACPI Name for PCI: 00:%02x.%01x\n",
Shouldn't be `BIOS_ERR` ?
Acknowledged
File src/soc/intel/pantherlake/cpu.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/367e10d0_b28afda2?usp... : PS55, Line 59: (1 << 0)
BIT(0)
Acknowledged
https://review.coreboot.org/c/coreboot/+/83798/comment/3c945b39_2b8d6275?usp... : PS55, Line 60: (1 << 3)
BIT(3) ...
Acknowledged
https://review.coreboot.org/c/coreboot/+/83798/comment/a6d367e1_989406a9?usp... : PS55, Line 88: else
unnecessary `else` keyword.
Acknowledged
File src/soc/intel/pantherlake/crashlog.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/7dbde098_8df1a279?usp... : PS55, Line 62: sram_bar = res->base;
Why do we need this intermediate value ? Can't we just do `return res->base` ?
Acknowledged
https://review.coreboot.org/c/coreboot/+/83798/comment/5498d191_6dbc73ff?usp... : PS55, Line 98: while (cl_cur && cl_cur->next) {
unnecessary brackets
It would be good to have brackets, as it improve readability.
-
Saurabh Mishra (Code Review)