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 32:
(13 comments)
https://review.coreboot.org/c/coreboot/+/38548/21/src/soc/intel/xeon_sp/incl... File src/soc/intel/xeon_sp/include/soc/cpu.h:
https://review.coreboot.org/c/coreboot/+/38548/21/src/soc/intel/xeon_sp/incl... PS21, Line 38: /* Latency times in units of 1024ns. */ : #define C_STATE_LATENCY_CONTROL_0_LIMIT 0x4e : #define C_STATE_LATENCY_CONTROL_1_LIMIT 0x76 : #define C_STATE_LATENCY_CONTROL_2_LIMIT 0x94 : #define C_STATE_LATENCY_CONTROL_3_LIMIT 0xfa : #define C_STATE_LATENCY_CONTROL_4_LIMIT 0x14c : #define C_STATE_LATENCY_CONTROL_5_LIMIT 0x3f2 : : /* Power in units of mW */ : #define C1_POWER 0x3e8 : #define C3_POWER 0x1f4 : #define C6_POWER 0x15e : #define C7_POWER 0xc8 : #define C8_POWER 0xc8 : #define C9_POWER 0xc8 : #define C10_POWER 0xc8 : : /* Common Timer Copy (CTC) frequency - 24MHz. */ : #define CTC_FREQ 24000000 : : /* CPU bus clock is fixed at 100MHz */ : #define CPU_BCLK 100 : : #define C_STATE_LATENCY_MICRO_SECONDS(limit, base) \ : (((1 << ((base)*5)) * (limit)) / 1000) : #define C_STATE_LATENCY_FROM_LAT_REG(reg) \ : C_STATE_LATENCY_MICRO_SECONDS(C_STATE_LATENCY_CONTROL_ ##reg## _LIMIT, \ : (IRTL_1024_NS >> 10)) : : int get_cpu_count(void); : void xeon_sp_init_cpus(struct device *dev); : : #endif :
are these used anywhere?
Not right now, they are needed for cstate support. Let's leave them here as they are needed sooner of later.
https://review.coreboot.org/c/coreboot/+/38548/21/src/soc/intel/xeon_sp/incl... File src/soc/intel/xeon_sp/include/soc/gpio_fsp.h:
https://review.coreboot.org/c/coreboot/+/38548/21/src/soc/intel/xeon_sp/incl... PS21, Line 21: #define RShiftU64(Operand, Count) (Operand >> Count) : #define LShiftU64(Operand, Count) (Operand << Count) :
this looks unused
Done
https://review.coreboot.org/c/coreboot/+/38548/21/src/soc/intel/xeon_sp/incl... PS21, Line 28: #define NO_REGISTER_FOR_PROPERTY (~0u)
same here
Done
https://review.coreboot.org/c/coreboot/+/38548/26/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/26/src/soc/intel/xeon_sp/incl... PS26, Line 33: #ifdef PCH_SERVER_BIOS_FLAG
why do we need this condition?
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 245: #ifdef PCH_SERVER_BIOS_FLAG
I do not understand this condition still. […]
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) \
I am just saying your preprocessor macro generates more code than needed. […]
Done
https://review.coreboot.org/c/coreboot/+/38548/21/src/soc/intel/xeon_sp/incl... PS21, Line 24: msr
the macro should not modify variables outside its scope […]
Done
https://review.coreboot.org/c/coreboot/+/38548/26/src/soc/intel/xeon_sp/incl... File src/soc/intel/xeon_sp/include/soc/pci_devs.h:
https://review.coreboot.org/c/coreboot/+/38548/26/src/soc/intel/xeon_sp/incl... PS26, Line 35: define FORM_PCI_ADDR(bus, dev, fun, off) (((PCI_ENABLE)) | \ : ((bus & 0xFF) << 16)| \ : ((dev & 0x1F) << 11)| \ : ((fun & 0x07) << 8) | \ : ((off & 0xFF) << 0))
this is not used, remove
Done
https://review.coreboot.org/c/coreboot/+/38548/29/src/soc/intel/xeon_sp/soc_... File src/soc/intel/xeon_sp/soc_util.c:
https://review.coreboot.org/c/coreboot/+/38548/29/src/soc/intel/xeon_sp/soc_... PS29, Line 290: if (i == 0) : continue;
if we skip i==0, why not start the loop from 1 instead?
Done
https://review.coreboot.org/c/coreboot/+/38548/29/src/soc/intel/xeon_sp/soc_... PS29, Line 587: u32
lets stick to c99 fixed width integers
Done
https://review.coreboot.org/c/coreboot/+/38548/29/src/soc/intel/xeon_sp/soc_... PS29, Line 929: sizeof(ids)/sizeof(int);
ARRAY_SIZE()
Done
https://review.coreboot.org/c/coreboot/+/38548/29/src/soc/intel/xeon_sp/soc_... PS29, Line 1046: izeof(regs)/sizeof(uint64_t)
ditto
Done
https://review.coreboot.org/c/coreboot/+/38548/26/src/soc/intel/xeon_sp/upd_... File src/soc/intel/xeon_sp/upd_display.c:
https://review.coreboot.org/c/coreboot/+/38548/26/src/soc/intel/xeon_sp/upd_... PS26, Line 23: #define DUMP_UPD(field) \
again I think it is generally bad idea for macro to touch variables outside its scope. […]
Done