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 26:
(19 comments)
Thanks for the review, Andrey.
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.
Done
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. […]
Done
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
Done
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 . […]
Done
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
Done
https://review.coreboot.org/c/coreboot/+/38548/21/src/soc/intel/xeon_sp/cpu.... PS21, Line 267: //southcluster_smm_enable_smi();
ditto
Done
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?
Done
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?
Done
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
It is used. See src/mainboard/ocp/tiogapass/romstage.c.
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. […]
This file defined one of the HOB interface. It is not clear if Intel will add such as part of FSP header files. So for now we need to add such.
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
Done
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? […]
Prefer not to put the register name twice.
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
See src/soc/intel/xeon_sp/soc_util.c
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
We are not done with debugging. We should drop such when we finish phase 2.
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. […]
This is a stub code. We are going to implement this in future.
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. […]
They are here, so that mainboard can do some differentiation as needed.
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? […]
Done
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?
Done
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);
Done