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 26:
(7 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?
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
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
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?
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
It is used. See src/mainboard/ocp/tiogapass/romstage.c.
I do not understand this condition still. Why do we need it? If it is always defined why you need the condition?
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
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. This one is less harmful because it has no side-effect, but I'd really prefer it to be: #define DUMP_UPD(old, new, field);