Attention is currently required from: Bora Guvendik, Anil Kumar K, Subrata Banik, Tim Wawrzynczak. Jérémy Compostella has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63750 )
Change subject: soc/intel/common: Add Raptor Lake device IDs ......................................................................
Patch Set 1:
(10 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/63750/comment/974282cc_c9eeddbc PS1, Line 9: Add Raptor Lake specific CPU, System Agent, PCH, The line break is unnecessary as the line length is 64 characters.
Regarding the references I would suggest something like:
References: - RaptorLake External Design Specification Volume 1 (640555) - 600/700 Series PCH External Design Specification Volume 1 (626817)
Patchset:
PS1: See comments inline.
File src/include/cpu/intel/cpu_ids.h:
https://review.coreboot.org/c/coreboot/+/63750/comment/733469bd_4a48f461 PS1, Line 62: #define CPUID_RAPTORLAKE_J0 0xb06a2 According the EDS section 15.0, there are two SKUs (P and S) with two different CPUIDs. Shouldn't we create CPUID_RAPTORLAKE_P_J0 and CPUID_RAPTORLAKE_S_J0 ? Or at least name it CPUID_RAPTORLAKE_P_J0 ?
Also, Are we already at J stepping ?
File src/include/device/pci_ids.h:
https://review.coreboot.org/c/coreboot/+/63750/comment/05f52443_0a497d1d PS1, Line 3079: #define PCI_DID_INTEL_RPL_P_ESPI_1 0x519d Where does this second eSPI MC comes from ? I don't find it in the PCH EDS.
https://review.coreboot.org/c/coreboot/+/63750/comment/c2585f9c_ac9ee241 PS1, Line 4149: #define PCI_DID_INTEL_RPL_TCSS_XHCI 0xa71e Where is it defined? I could not find it.
https://review.coreboot.org/c/coreboot/+/63750/comment/fa79ac98_04a38a54 PS1, Line 4350: #define PCI_DID_INTEL_RPL_TBT_RP0 0xa76e Where are they defined? I could not find them.
https://review.coreboot.org/c/coreboot/+/63750/comment/3effaf65_14bbf106 PS1, Line 4394: #define PCI_DID_INTEL_RPL_IPU 0xa75d Where is it defined? I could not find it.
File src/soc/intel/alderlake/bootblock/report_platform.c:
https://review.coreboot.org/c/coreboot/+/63750/comment/9d616722_3b9a9180 PS1, Line 32: { CPUID_RAPTORLAKE_J0, "Raptorlake Platform" }, CPUID_RAPTORLAKE_J0 is actually the Raptor P cpuid.
File src/soc/intel/alderlake/cpu.c:
https://review.coreboot.org/c/coreboot/+/63750/comment/c4d4bb79_a46403ff PS1, Line 262: adl_p_mch_ids should be rpl_p_mch_ids right ?
Also this pattern is repeating over and over, it may be time to introduce a function or a macro?
File src/soc/intel/common/block/dtt/dtt.c:
https://review.coreboot.org/c/coreboot/+/63750/comment/9a375209_5a2aef61 PS1, Line 8: PCI_DID_INTEL_RPL_DTT, Is there any reason why it is being added at the beginning ?