David Hendricks 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:
(57 comments)
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 4MiB). Since this is kept away from the OS we should probably be conservative.
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
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?
https://review.coreboot.org/c/coreboot/+/38548/41/src/soc/intel/xeon_sp/cpu.... PS41, Line 93: */ formatting issue
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?
https://review.coreboot.org/c/coreboot/+/38548/41/src/soc/intel/xeon_sp/cpu.... PS41, Line 251: /* formatting issue
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
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.
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 as advertised anyway.
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 some kind here.
https://review.coreboot.org/c/coreboot/+/38548/41/src/soc/intel/xeon_sp/hob_... PS41, Line 76: size_t hob_size; initialize to 0
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);
https://review.coreboot.org/c/coreboot/+/38548/41/src/soc/intel/xeon_sp/hob_... PS41, Line 104: size_t hob_size; initialize to zero
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... File src/soc/intel/xeon_sp/include/fsp/FspmUpd.h:
PS42:
These files probably belong somewhere in src/vendorcode/intel/fsp/fsp2_0/?
Ideally yes, however this is kind of a pre-release version. Once we have an actual FSP for Xeon SP then it will definitely go in src/vendorcode, but until then we thought it will be best to avoid confusing this with whatever comes out in the (hopefully near) future.
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... File src/soc/intel/xeon_sp/include/soc/gpe.h:
PS42: It seems that nothing in here is actually used, though the header is included by chip.h and uncore.asl. I think we should get rid of this file for now, and use the common Lewisburg PCH code (with CB:35031 applied).
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?
(side note, we usually use __packed in each packed struct's definition to be explicit, but since this seems to come from UEFI code I suppose it's fine to stick with their convention)
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... File src/soc/intel/xeon_sp/include/soc/irq.h:
PS42: Are these #defines used anywhere? I see this headed included by chip.h and uncore.asl. It seems this file was copied from somewhere, so to avoid confusion it might be better to either remove it entirely or remove the #defines to make it a stub that we can add later.
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.
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.
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?
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?
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.
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: PID_GPIOCOM4 0xAB
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... PS42, Line 24: needs a tab?
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/incl... PS42, Line 29: #define PID_SCS 0xC0 Not on Lewisburg?
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?
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?
(BTW, I'm just going by the public C620-series datasheet: https://www3.intel.com/content/dam/www/public/us/en/documents/datasheets/c62...)
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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.h, maybe this should move into that file? Or just remove it for now if it's unused. (There's some other interesting stuff in those GLBRST registers that we may want to use later on, so we can deal with it then)
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... may as well just define it as such (#define ACTL PMC_ACPI_CNT)
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
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
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 PER_SMI_SEL at bit 0 PER_SMI_SEL_MASK 0x3
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 PME_B0_S5_DIS at bit 15
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) SWSMI_RATESEL_1_5MS (0 << 6) SWSMI_RATESEL_16MS (1 << 6) SWSMI_RATESEL_32MS (2 << 6) SWSMI_RATESEL_64MS (3 << 6)
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.h
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
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
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
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
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/southcluster.c) will allow the compiler to give us a useful error if somebody tries to pass in the wrong type.
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/lpc.... File src/soc/intel/xeon_sp/lpc.c:
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/lpc.... PS42, Line 24: static const struct lpc_mmio_range xeon_lpc_fixed_mmio_ranges[] = { Do the ranges mentioned in chapter 7.3 in the LBG EDS (targeting LPC/eSPI) need to be here?
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/rese... File src/soc/intel/xeon_sp/reset.c:
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/rese... PS42, Line 23: die("Reset not implemented!\n"); Perhaps we can use the implementation in src/soc/intel/common/reset.c... but I suppose we can think about that a bit more later.
(marking this comment as resolved)
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/spi.... File src/soc/intel/xeon_sp/spi.c:
https://review.coreboot.org/c/coreboot/+/38548/42/src/soc/intel/xeon_sp/spi.... PS42, Line 26: case PCH_DEVFN_GSPI0: : return 1; : case PCH_DEVFN_GSPI1: : return 2; Are these used for anything? I don't think we have them on LBG - we only have one SPI bus AFAICT.