Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38548 )
Change subject: soc/intel: Add Intel Xeon Scalable Processor support ......................................................................
Patch Set 42:
(54 comments)
Thanks for the review. Uploading a new version with majority of comments done, Please review the new version and replies. Will upload another version addressing the rest.
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/Kcon... File src/soc/intel/xeon_sp/Kconfig:
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/Kcon... PS42, Line 135: config SMM_TSEG_SIZE : hex : default 0x10000000 : : config SMM_RESERVED_SIZE : hex : default 0x8000000
Is there a particular reason for making these so large? Usually they're a few megabytes (IED wants 4 […]
Done
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/acpi... File src/soc/intel/xeon_sp/acpi.c:
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/acpi... PS42, Line 68: #if CONFIG(CONSOLE_CBMEM)
use if instead of #if?
Done
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/acpi... PS42, Line 157: bux
bus?
Done
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/acpi... PS42, Line 224: idel
idle
Done
https://review.coreboot.org/c/coreboot/+/38548/41/src/soc/intel/xeon_sp/chip... File src/soc/intel/xeon_sp/chip.c:
https://review.coreboot.org/c/coreboot/+/38548/41/src/soc/intel/xeon_sp/chip... PS41, Line 184: 8_t al
tab in between type and variable name
Done
https://review.coreboot.org/c/coreboot/+/38548/41/src/soc/intel/xeon_sp/cpu.... File src/soc/intel/xeon_sp/cpu.c:
https://review.coreboot.org/c/coreboot/+/38548/41/src/soc/intel/xeon_sp/cpu.... PS41, Line 45: num_banks = msr.lo & IA32_MCG_CAP_COUNT_MASK;
num_banks does not appear to be used anywhere?
Done
https://review.coreboot.org/c/coreboot/+/38548/41/src/soc/intel/xeon_sp/cpu.... PS41, Line 93: */
formatting issue
Done
https://review.coreboot.org/c/coreboot/+/38548/41/src/soc/intel/xeon_sp/cpu.... PS41, Line 224: */
Did you intend to enable SMIs in this or a follow-up patch?
In a follow-up patch.
https://review.coreboot.org/c/coreboot/+/38548/41/src/soc/intel/xeon_sp/cpu.... PS41, Line 251: /*
formatting issue
Done
https://review.coreboot.org/c/coreboot/+/38548/41/src/soc/intel/xeon_sp/cpu.... PS41, Line 260: /* calls src/cpu/x86/mp_init.c */
formatting issue
Done
https://review.coreboot.org/c/coreboot/+/38548/21/src/soc/intel/xeon_sp/hob_... File src/soc/intel/xeon_sp/hob_display.c:
https://review.coreboot.org/c/coreboot/+/38548/21/src/soc/intel/xeon_sp/hob_... PS21, Line 114: printk(BIOS_DEBUG, "SYSTEM_STATUS: 0x%lx, PLATFORM_DATA: 0x%lx, IIO_RESOURCE_INSTANCE: 0x%lx, STACK_RES: 0x%lx, " : "IIO_DMI_PCIE_INFO: 0x%lx, QPI_IIO_DATA: 0x%lx, QPI_CPU_DATA: 0x%lx, QPI_PEER_DATA: 0x%lx, " : "IIO_PORT_INFO: 0x%lx, UINT64_STRUCT: 0x%lx\n", : sizeof(SYSTEM_STATUS), sizeof(PLATFORM_DATA), sizeof(IIO_RESOURCE_INSTANCE), : sizeof(STACK_RES), sizeof(IIO_DMI_PCIE_INFO), sizeof(QPI_IIO_DATA), : sizeof(QPI_CPU_DATA), sizeof(QPI_PEER_DATA), : sizeof(IIO_PORT_INFO), sizeof(UINT64_STRUCT));
What is this supposed to do? I'd expect to see the value of these fields, not the size.
Done
https://review.coreboot.org/c/coreboot/+/38548/41/src/soc/intel/xeon_sp/hob_... File src/soc/intel/xeon_sp/hob_display.c:
https://review.coreboot.org/c/coreboot/+/38548/41/src/soc/intel/xeon_sp/hob_... PS41, Line 40: return NULL;
I don't see where this function is used, so probably better to get rid of it since it doesn't behave […]
Done
https://review.coreboot.org/c/coreboot/+/38548/41/src/soc/intel/xeon_sp/hob_... PS41, Line 59: res = fsp_hob_header_to_resource(hob);
Since this is dereferenced later on in the function there should probably be a die() or assertion of […]
Done
https://review.coreboot.org/c/coreboot/+/38548/41/src/soc/intel/xeon_sp/hob_... PS41, Line 76: size_t hob_size;
initialize to 0
Done
https://review.coreboot.org/c/coreboot/+/38548/41/src/soc/intel/xeon_sp/hob_... PS41, Line 82: hob, hob_size, sizeof(struct SystemMemoryMapHob), MAX_SOCKET, SAD_RULES);
Check the HOB before using it below: assert(hob != NULL && hob_size != 0);
Done
https://review.coreboot.org/c/coreboot/+/38548/41/src/soc/intel/xeon_sp/hob_... PS41, Line 104: size_t hob_size;
initialize to zero
Done
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... File src/soc/intel/xeon_sp/include/soc/hob_iiouds.h:
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... PS42, Line 44: #pragma pack(1)
Does this need to go above the other structs so they get packed as well? […]
Done
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... File src/soc/intel/xeon_sp/include/soc/itss.h:
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... PS42, Line 32: #define PCR_ITSS_PIR05 0x314A : #define PCR_ITSS_PIR06 0x314C : #define PCR_ITSS_PIR07 0x314E : #define PCR_ITSS_PIR08 0x3150 : #define PCR_ITSS_PIR09 0x3152 : #define PCR_ITSS_PIR10 0x3154 : #define PCR_ITSS_PIR11 0x3156 : #define PCR_ITSS_PIR12 0x3158
I don't think we have PIR05-12 on this platform.
Done
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... File src/soc/intel/xeon_sp/include/soc/p2sb.h:
PS42:
Is this file used? Maybe we can get drop it.
Yes, this file is used by src/soc/intel/common/block/p2sb/p2sb.c .
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... PS42, Line 20: #define HPTC_OFFSET 0x60
Prefix with PCH_P2SB?
This macro is used in src/soc/intel/common/block/p2sb/p2sb.c to configure HPET.
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... PS42, Line 28: #define PCH_PWRM_ACPI_TMR_CTL 0xFC
I don't see this in the P2SB config registers, it might be leftover?
Done
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... File src/soc/intel/xeon_sp/include/soc/pcr_ids.h:
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... PS42, Line 23: #define PID_PSTH 0x89
What is this supposed to be? The LBG EDS says HSIO strap configuration.
This is for IO Trap PCRs. I am not sure what PSTH stands for, existing codebase expects it, see src/soc/intel/common/block/smm/smitraphandler.c
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... PS42, Line 24: #define PID_GPIOCOM3 0xAC
While we're at it, may as well add: […]
Done
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... PS42, Line 24:
needs a tab?
Done
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... PS42, Line 29: #define PID_SCS 0xC0
Not on Lewisburg?
Done
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... PS42, Line 33: #define PID_SERIALIO 0xCB : #define PID_DMI 0xEF
Not on Lewisburg?
Done
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... File src/soc/intel/xeon_sp/include/soc/pm.h:
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... PS42, Line 41: #define PM1_CNT 0x04
For completeness, maybe add SLP_TYP and SLP_EN? […]
SLP_EN and SLP_TYP are already defined in src/arch/x86/include/arch/acpi.h
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... PS42, Line 43: #define BM_RLD (1 << 1)
Not on Lewisburg
Done
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... PS42, Line 48: #define ME_SMI_EN (1 << 30)
Add SERIAL_IO_SMI_EN at bit 29
Done
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... PS42, Line 50: #define GPIO_UNLOCK_SMI_EN (1 << 27)
For completeness, bits 26, 25, 23 should also be in here
Done
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... PS42, Line 51: #define INTEL_USB2_EN (1 << 18) : #define LEGACY_USB2_EN (1 << 17)
Bits 15-22 are reserved on LBG
Done
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... PS42, Line 72: #define SCC_SMI_STS_BIT 25
Add #define for TCO_SMI_EN at bit 23
Done
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... PS42, Line 89: #define UPWRC 0x3c : #define UPWRC_WS (1 << 8) : #define UPWRC_WE (1 << 1) : #define UPWRC_SMI (1 << 0)
Not on LBG
Done
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... PS42, Line 93: #define GPE_CNTL 0x42
This is 0x40 on LBG
Done
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... PS42, Line 94: #define SWGPE_CTRL (1 << 1)
This is bit 17 on LBG
Done
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... PS42, Line 107: #define LAN_WAK_STS (1 << 16)
Not on LBG
Done
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... PS42, Line 113: #define BATLOW_STS (1 << 10)
Not on LBG
Done
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... PS42, Line 116: #define TCOSCI_STS (1 << 6)
Add IE_SCI_STS at bit 3
Done
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... PS42, Line 121: #define LAN_WAK_EN (1 << 16)
Not on LBG
Done
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... PS42, Line 127: #define BATLOW_EN (1 << 10)
Not on LBG
Done
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... PS42, Line 128: #define PCI_EXP_EN (1 << 9)
Add RI_EN on bit 8
Done
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... PS42, Line 133: #define GBLRST_CAUSE0_THERMTRIP (1 << 5)
The register offsets for this are defined in pmc. […]
Done
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... File src/soc/intel/xeon_sp/include/soc/pmc.h:
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... PS42, Line 24: 0x44
It looks like this is intended to be the same as PMC_ACPI_CNT... […]
Done
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... PS42, Line 42: #define SX_PP_EN (1 << 27)
NON_LEG_PCH_MODE at bit 25
Done
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... PS42, Line 54: #define ALLOW_L1LOW_OPI_ON (1 << 6)
ALLOW_L1LOW_BCLKREQ_ON at bit 5
Done
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... PS42, Line 55: #define SMI_LOCK (1 << 4)
ESPI_SMI_LOCK at bit 3 […]
Done
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... PS42, Line 58: #define ACPI_BASE_LOCK (1 << 17)
PM_DATA_BAR_DIS at bit 16 […]
Done
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... PS42, Line 67: #define HOST_RST_STS (1 << 9)
SWSMI_RATESEL_MASK (0x3 << 6) […]
Done
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... PS42, Line 77: #define ETR3 0xac : #define ETR3_CF9LOCK (1 << 31) : #define ETR3_CF9GR (1 << 20)
These are defined in pm. […]
Done
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... PS42, Line 96: #define PMSYNC_TPR_CFG 0xc4 : #define PMSYNC_LOCK (1 << 31)
Not on LBG
Done
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... PS42, Line 103: #define CIR31C 0x31c : #define XTALSDQDIS (1 << 22)
Not on LBG, but we do have CIR324, CIR328, and CIR32C at offsets 0x324, 0x328, and 0x32C
Done
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... File src/soc/intel/xeon_sp/include/soc/smbus.h:
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... PS42, Line 35: #define SMBUS_SLAVE_ADDR 0x24
This should be 0x44 for LGB
Done
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... File src/soc/intel/xeon_sp/include/soc/soc_config.h:
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... PS42, Line 26: MaxIIO
nit: ALLCAPS for #defines
Done
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... File src/soc/intel/xeon_sp/include/soc/soc_util.h:
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... PS42, Line 38: mmio_resource(dev, index, (uint64_t) ((uint64_t)base >> 10), \ : (uint64_t) ((uint64_t)size >> 10)); \ : LOG_MEM_RESOURCE("mmio", dev, index, (uint64_t) ((uint64_t)base >> 10), \ : (uint64_t) ((uint64_t)size >> 10)); \
Are the casts necessary? An inline (for example add_mmio_resource() in src/soc/intel/braswell/southc […]
Done