Andrey Petrov 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 25:
(20 comments)
thanks Jonathan I think the patch has been greatly improved. However I feel we need to better understand the logic around iio/bus resource assignment. It would be great to have at least that bit as a separate patch so we can review and better understand it.
https://review.coreboot.org/c/coreboot/+/38548/21/src/soc/intel/xeon_sp/Kcon... File src/soc/intel/xeon_sp/Kconfig:
https://review.coreboot.org/c/coreboot/+/38548/21/src/soc/intel/xeon_sp/Kcon... PS21, Line 148: config HSUART_DEV : hex : default 0x1a : : config ENABLE_HSUART : depends on NON_LEGACY_UART_MODE : bool "Enable High-speed UART debug port selected by UART_FOR_CONSOLE." : default n : select CONSOLE_SERIAL : select DRIVERS_UART : select DRIVERS_UART_8250MEM : : config CONSOLE_UART_BASE_ADDRESS : depends on ENABLE_HSUART : hex "MMIO base address for UART" : default 0xd4000000 does skylake-sp has HSUART? I belive HSUART is part of some intel true SoC.
https://review.coreboot.org/c/coreboot/+/38548/21/src/soc/intel/xeon_sp/Kcon... PS21, Line 169: config SKYLAKE_SP_CAR_NEM_ENHANCED : bool "Enhanced Non-evict mode" : depends on !FSP_CAR : default y : select SOC_INTEL_COMMON_BLOCK_CAR : select INTEL_CAR_NEM_ENHANCED : help : A current limitation of NEM (Non-Evict mode) is that code and data sizes : are derived from the requirement to not write out any modified cache line. : With NEM, if there is no physical memory behind the cached area, : the modified data will be lost and NEM results will be inconsistent. : ENHANCED NEM guarantees that modified data is always : kept in cache while clean data is replaced. we are using FSP_CAR and FSP-T. Unless there is plan to re-implement FSP-T, this is technically dead code
https://review.coreboot.org/c/coreboot/+/38548/21/src/soc/intel/xeon_sp/chip... File src/soc/intel/xeon_sp/chip.c:
https://review.coreboot.org/c/coreboot/+/38548/21/src/soc/intel/xeon_sp/chip... PS21, Line 526: //show_devs_tree(dev, BIOS_SPEW, 0); please remove commented out code
https://review.coreboot.org/c/coreboot/+/38548/21/src/soc/intel/xeon_sp/chip... File src/soc/intel/xeon_sp/chip.c.bak:
https://review.coreboot.org/c/coreboot/+/38548/21/src/soc/intel/xeon_sp/chip... PS21, Line 1: * please remove this .bak file, I assume it a unwanted hitchhiker
https://review.coreboot.org/c/coreboot/+/38548/21/src/soc/intel/xeon_sp/cpu.... File src/soc/intel/xeon_sp/cpu.c:
https://review.coreboot.org/c/coreboot/+/38548/21/src/soc/intel/xeon_sp/cpu.... PS21, Line 47: //static char processor_name[64]; ditto
https://review.coreboot.org/c/coreboot/+/38548/21/src/soc/intel/xeon_sp/cpu.... PS21, Line 267: //southcluster_smm_enable_smi(); ditto
https://review.coreboot.org/c/coreboot/+/38548/21/src/soc/intel/xeon_sp/incl... File src/soc/intel/xeon_sp/include/soc/bootblock.h:
https://review.coreboot.org/c/coreboot/+/38548/21/src/soc/intel/xeon_sp/incl... PS21, Line 22: void early_uart_init(void); is this function and file used?
https://review.coreboot.org/c/coreboot/+/38548/21/src/soc/intel/xeon_sp/incl... File src/soc/intel/xeon_sp/include/soc/gpio_soc_defs.h:
https://review.coreboot.org/c/coreboot/+/38548/21/src/soc/intel/xeon_sp/incl... PS21, Line 20: /// This header file should be used together with : /// PCH GPIO lib in C and ASL. All defines used : /// must match both ASL/C syntax what is PCH GPIO lib?
https://review.coreboot.org/c/coreboot/+/38548/21/src/soc/intel/xeon_sp/incl... PS21, Line 245: #ifdef PCH_SERVER_BIOS_FLAG this looks unused/unset
https://review.coreboot.org/c/coreboot/+/38548/21/src/soc/intel/xeon_sp/incl... File src/soc/intel/xeon_sp/include/soc/hob_iiouds.h:
https://review.coreboot.org/c/coreboot/+/38548/21/src/soc/intel/xeon_sp/incl... PS21, Line 32: UINT64 lets just use c99 fixed-width integers. What is this file? if it coming from intel, shouldn't it go to vendorcode?
https://review.coreboot.org/c/coreboot/+/38548/21/src/soc/intel/xeon_sp/incl... File src/soc/intel/xeon_sp/include/soc/hob_memmap.h:
https://review.coreboot.org/c/coreboot/+/38548/21/src/soc/intel/xeon_sp/incl... PS21, Line 19: same for this file
https://review.coreboot.org/c/coreboot/+/38548/21/src/soc/intel/xeon_sp/incl... File src/soc/intel/xeon_sp/include/soc/msr.h:
https://review.coreboot.org/c/coreboot/+/38548/21/src/soc/intel/xeon_sp/incl... PS21, Line 22: define RDMSR(id) \ this looks like DUMPMSR rather than RDMSR. rename?
also, by invoking this preprocessor macro we generate extra duplicated code. Why not turn this into a function? we can still have string macro name, just as a parameter
https://review.coreboot.org/c/coreboot/+/38548/21/src/soc/intel/xeon_sp/incl... File src/soc/intel/xeon_sp/include/soc/pcr_ids.h:
https://review.coreboot.org/c/coreboot/+/38548/21/src/soc/intel/xeon_sp/incl... PS21, Line 37: #define PCH_PCR_ADDRESS(Pid, Offset) \ : (P2SB_BAR | ((uint8_t)(Pid) << 16) | (uint16_t)(Offset)) you don't need this
https://review.coreboot.org/c/coreboot/+/38548/21/src/soc/intel/xeon_sp/incl... File src/soc/intel/xeon_sp/include/soc/soc_util.h:
https://review.coreboot.org/c/coreboot/+/38548/21/src/soc/intel/xeon_sp/incl... PS21, Line 44: #define DEV_FUNC_ENTER(dev) \ : printk(BIOS_SPEW, "%s:%s:%d: ENTER (dev: %s)\n", \ : __FILE__, __func__, __LINE__, dev_path(dev)) : : #define DEV_FUNC_EXIT(dev) \ : printk(BIOS_SPEW, "%s:%s:%d: EXIT (dev: %s)\n", __FILE__, \ : __func__, __LINE__, dev_path(dev)) : : #define FUNC_ENTER() \ : printk(BIOS_SPEW, "%s:%s:%d: ENTER\n", __FILE__, __func__, __LINE__) : : #define FUNC_EXIT() \ : printk(BIOS_SPEW, "%s:%s:%d: EXIT\n", __FILE__, __func__, __LINE__) I think we should drop these, now that we are done with debugging
https://review.coreboot.org/c/coreboot/+/38548/21/src/soc/intel/xeon_sp/rese... File src/soc/intel/xeon_sp/reset.c:
https://review.coreboot.org/c/coreboot/+/38548/21/src/soc/intel/xeon_sp/rese... PS21, Line 21: void chipset_handle_reset(uint32_t status) : { : switch (status) { : case FSP_STATUS_RESET_REQUIRED_5: /* Global Reset */ : die("Global Reset not implemented!\n"); : break; : default: : printk(BIOS_ERR, "unhandled reset type %x\n", status); : die("unknown reset type"); : break; : } : } it sounds like we are not trying to take action and actually issue a reset. Why?
https://review.coreboot.org/c/coreboot/+/38548/21/src/soc/intel/xeon_sp/roms... File src/soc/intel/xeon_sp/romstage.c:
https://review.coreboot.org/c/coreboot/+/38548/21/src/soc/intel/xeon_sp/roms... PS21, Line 93: __weak void mainboard_memory_init_params(FSPM_UPD *mupd) : { : /* Do nothing */ : } remove? there is no need to declare empty function and call it here. what gives?
https://review.coreboot.org/c/coreboot/+/38548/21/src/soc/intel/xeon_sp/soc_... File src/soc/intel/xeon_sp/soc_util.c:
https://review.coreboot.org/c/coreboot/+/38548/21/src/soc/intel/xeon_sp/soc_... PS21, Line 170: void scan_dump_pci_devs(void) so what are we trying to achieve here? scan through PCIEX space and sww what is responding? if that is the case please just use pci_mmio_read_configX() variants
https://review.coreboot.org/c/coreboot/+/38548/21/src/soc/intel/xeon_sp/soc_... PS21, Line 180: DEFAULT_PCIEXBAR why we need something other than CONFIG_MMCONF_BASE_ADDRESS?
https://review.coreboot.org/c/coreboot/+/38548/21/src/soc/intel/xeon_sp/soc_... PS21, Line 599: if (total_delay >= max_delay) please use stopwatch_ family of functions (timer.h)
https://review.coreboot.org/c/coreboot/+/38548/21/src/soc/intel/xeon_sp/soc_... PS21, Line 868: void *addr = (void *) PCH_PCR_ADDRESS(PID_ITSS, reg); : uint8_t val = read8(addr); just pcr_read8(PID_ITSS, reg);