Attention is currently required from: Kapil Porwal, Pranava Y N, Saurabh Mishra, Subrata Banik.
Jérémy Compostella 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 62:
(18 comments)
File src/soc/intel/pantherlake/acpi.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/5e1ada12_0448e0fa?usp... : PS50, Line 120: int size_t
https://review.coreboot.org/c/coreboot/+/83798/comment/2981390b_114293a8?usp... : PS50, Line 126: is_s0ix_enable I don't see the benefit of this intermediary variable.
https://review.coreboot.org/c/coreboot/+/83798/comment/a45e25d5_f83b25e3?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 perform the same operation if the function is called again ?
https://review.coreboot.org/c/coreboot/+/83798/comment/8c9aee35_605f3e67?usp... : PS50, Line 157: 4 sizeof(uint32_t)
File src/soc/intel/pantherlake/chip.h:
https://review.coreboot.org/c/coreboot/+/83798/comment/5f357e23_8de488fd?usp... : PS55, Line 7: #include <device/pci_ids.h> Move line 7 up.
https://review.coreboot.org/c/coreboot/+/83798/comment/529c53fd_6bf26166?usp... : PS55, Line 46: PTL_U_404_4_15W_CORE, /* 4 P-cores +0 E-cores + 4 LP E-cores + 4Xe25W*/
one space after `+`
And one space before ending `*/`
https://review.coreboot.org/c/coreboot/+/83798/comment/129d8878_e932f41d?usp... : PS55, Line 47: PTL_H_484_12_25W_CORE Instead of commenting out every single line, what about something like the following ? ``` /* * Panther Lake power limits naming convention: * PTL_<TYPE>_<PCORE#><ECORE#><LP-ECORE#>_<TDP>W_CORE */ ```
https://review.coreboot.org/c/coreboot/+/83798/comment/6486312a_d1a643fe?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 ?
File src/soc/intel/pantherlake/chip.h:
https://review.coreboot.org/c/coreboot/+/83798/comment/b8a860c1_8212dee6?usp... : PS50, Line 7: #include <device/pci_ids.h> Move that one one line up.
File src/soc/intel/pantherlake/chip.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/b6006751_09cebfd8?usp... : PS55, Line 61: printk(BIOS_DEBUG, "dev->path.type=%x\n", dev->path.usb.port_type); Shouldn't this be an `BIOS_ERR` ? The message is wrong as it print the USB port_type not the path type. Also,shouldn't we print something like `Missing ACPI Name for USB port_type=0x%x\n`
https://review.coreboot.org/c/coreboot/+/83798/comment/40147dac_015c3a13?usp... : PS55, Line 65: printk(BIOS_DEBUG, "dev->path.type=%x\n", dev->path.type); Similar remark here.
https://review.coreboot.org/c/coreboot/+/83798/comment/1c31ff9a_730b42af?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 ?
https://review.coreboot.org/c/coreboot/+/83798/comment/932c4258_d52b059c?usp... : PS55, Line 116: printk(BIOS_DEBUG, "Missing ACPI Name for PCI: 00:%02x.%01x\n", Shouldn't be `BIOS_ERR` ?
File src/soc/intel/pantherlake/cpu.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/4ee3b365_c53e8613?usp... : PS55, Line 59: (1 << 0) BIT(0)
https://review.coreboot.org/c/coreboot/+/83798/comment/a9150677_a0cf2169?usp... : PS55, Line 60: (1 << 3) BIT(3) ...
https://review.coreboot.org/c/coreboot/+/83798/comment/dac546bd_63769f54?usp... : PS55, Line 88: else unnecessary `else` keyword.
File src/soc/intel/pantherlake/crashlog.c:
https://review.coreboot.org/c/coreboot/+/83798/comment/8b91998e_8d67bd05?usp... : PS55, Line 62: sram_bar = res->base; Why do we need this intermediate value ? Can't we just do `return res->base` ?
https://review.coreboot.org/c/coreboot/+/83798/comment/9da269cf_0b34a497?usp... : PS55, Line 98: while (cl_cur && cl_cur->next) { unnecessary brackets