Attention is currently required from: Arthur Heymans, Chen, Gang C, Christian Walter, David Hendricks, Felix Singer, Jincheng Li, Johnny Lin, Jonathan Zhang, Lean Sheng Tan, Nico Huber, Patrick Rudolph, Paul Menzel, TangYiwei, Tim Chu.
Shuo Liu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81316?usp=email )
Change subject: soc/intel/xeon_sp: Add Granite Rapids initial codes ......................................................................
Patch Set 33:
(15 comments)
File src/soc/intel/xeon_sp/Makefile.mk:
https://review.coreboot.org/c/coreboot/+/81316/comment/161745fd_9cc4304c : PS33, Line 8: ebg :
This is confusing. […]
This is due to SPR fork/reuse. Will extend the comment block to clarify.
File src/soc/intel/xeon_sp/chip_gen6.c:
https://review.coreboot.org/c/coreboot/+/81316/comment/a83e38ac_e2abb3cc : PS33, Line 44: IOINDEX
Just use index directly? this one should be deprecated as links are not in struct bus anymore.
Done
File src/soc/intel/xeon_sp/gnr/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/81316/comment/c5d0eeda_d97f6220 : PS33, Line 12: romstage-y += romstage.c soc_util.c : romstage-$(CONFIG_DISPLAY_UPD_DATA) += upd_display.c : : ramstage-y += chip.c cpu.c soc_util.c ramstage.c soc_acpi.c : ramstage-y += ../chip_gen6.c : : CPPFLAGS_common += -I$(src)/soc/intel/xeon_sp/gnr/include -I$(src)/soc/intel/xeon_sp/gnr :
Use one value per line
Done
File src/soc/intel/xeon_sp/gnr/acpi/gpe.asl:
https://review.coreboot.org/c/coreboot/+/81316/comment/f49a38dd_f87deebf : PS33, Line 27: Method (_L62, 0, NotSerialized) : { : DBGO("\_GPE\_L62\n") : SGPC = 0 // clear SWGPE control : SGPS = 1 // clear SWGPE Status : }
What do you need this for?
This is to enable the SWGPE to work, SWGPE is a special sort of GPE, but it can be triggered by software,
The trigger bits is defined in, https://github.com/coreboot/coreboot/blob/main/src/soc/intel/xeon_sp/include...
A similar usage is at, e.g. https://github.com/tianocore/edk2-platforms/blob/master/Platform/Intel/Kabyl...
File src/soc/intel/xeon_sp/gnr/acpi/platform.asl:
https://review.coreboot.org/c/coreboot/+/81316/comment/9315add1_18e5f8b1 : PS33, Line 6: Name (_S0, Package () // mandatory system state : { : 0x00, 0x00, 0x00, 0x00 : }) : : Name (_S5, Package () // mandatory system state : { : 0x07, 0x00, 0x00, 0x00 : })
can you sleepstates. […]
It is possible, but I need to modify the src/southbridge/intel/common/acpi/sleepstates.asl to set SSFG as 0x0 for CONFIG_XEON_SP_COMMON_BASE (to support S0/S5 only). I will have another patch for it.
File src/soc/intel/xeon_sp/gnr/chip.h:
https://review.coreboot.org/c/coreboot/+/81316/comment/b98aa853_3a7e0cd9 : PS33, Line 16: u
Use bool where possible.
Done
https://review.coreboot.org/c/coreboot/+/81316/comment/7ef730b1_841b9be7 : PS33, Line 17: uint8_t x2apic;
unused?
Done
File src/soc/intel/xeon_sp/gnr/cpu.c:
https://review.coreboot.org/c/coreboot/+/81316/comment/f0a70a97_97225a4e : PS33, Line 63: {0, 0},
CPU_TABLE_END
Done
File src/soc/intel/xeon_sp/gnr/include/soc/cpu.h:
https://review.coreboot.org/c/coreboot/+/81316/comment/2d6cb476_f0c55e1c : PS33, Line 6: #include <device/device.h> : #include <cpu/x86/msr.h>
This should not be in this header.
Done
File src/soc/intel/xeon_sp/gnr/include/soc/pci_devs.h:
https://review.coreboot.org/c/coreboot/+/81316/comment/9e411fd3_6e420ac1 : PS33, Line 179: DMIRCBAR
I'm curious. […]
In the initial codes, all register definitions are forked from SPR (Sapphire Rapids) and EBG (Emmisburg PCH)'s codes are reused. (added mentioning in the commit message). Will extend the comment block to clarify.
https://review.coreboot.org/c/coreboot/+/81316/comment/e19a918b_f720f484 : PS33, Line 184: // IIO DFX Global D7F7 registers : #define IIO_DFX_TSWCTL0 0x30c : #define IIO_DFX_LCK_CTL 0x504 : : // XHCI register : #define SYS_BUS_CFG2 0x44 : : /*
Please be consistent with comment style.
Done
File src/soc/intel/xeon_sp/gnr/include/soc/soc_msr.h:
https://review.coreboot.org/c/coreboot/+/81316/comment/0be4a5df_ee9c6416 : PS33, Line 3: // TEMPORARY PLACE HOLDER! DO NOT USE!
Eh?
In the initial codes, all register definitions are forked from SPR (Sapphire Rapids) and EBG (Emmisburg PCH)'s codes are reused. (added mentioning in the commit message). Will extend the comment block to clarify.
File src/soc/intel/xeon_sp/gnr/soc_util.c:
https://review.coreboot.org/c/coreboot/+/81316/comment/85a980ac_9f60e839 : PS33, Line 143: bool is_memtype_reserved(uint16_t mem_type) : { : return FALSE; : } : : bool is_memtype_non_volatile(uint16_t mem_type) : { : return FALSE; : } : : bool is_memtype_processor_attached(uint16_t mem_type) : { : return TRUE; : } :
Please use true/false, the definitions from coreboot.
Done
File src/soc/intel/xeon_sp/uncore.c:
PS33:
The code movements should be separated out into their own commit.
There were a discussion to merge the movement into this one, so I keep them together. The history is at: https://review.coreboot.org/c/coreboot/+/81110.
https://review.coreboot.org/c/coreboot/+/81316/comment/4f643c8e_46a58161 : PS33, Line 35: enum { : TOHM_REG, : MMIOL_REG, : MMCFG_BASE_REG, : MMCFG_LIMIT_REG, : TOLM_REG, : /* NCMEM and ME ranges are mutually exclusive */ : NCMEM_BASE_REG, : NCMEM_LIMIT_REG, : ME_BASE_REG, : ME_LIMIT_REG, : TSEG_BASE_REG, : TSEG_LIMIT_REG, : VTDBAR_REG, : /* Must be last. */ : NUM_MAP_ENTRIES : };
Does it make sense to move this to a header? It's used as index for the array below.
Sure, it should be already at src/soc/intel/xeon_sp/include/soc/chip_common.h.