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 33:
(10 comments)
https://review.coreboot.org/c/coreboot/+/38548/29/src/soc/intel/xeon_sp/acpi... File src/soc/intel/xeon_sp/acpi.c:
https://review.coreboot.org/c/coreboot/+/38548/29/src/soc/intel/xeon_sp/acpi... PS29, Line 808: if (size % 0x1000) // 4k align : size = ((MEM_BLK_COUNT * sizeof(MEM_BLK)) & (~0xfff)) + 0x1000; :
can we use ALIGN(), ALIGN_UP() macros here?
Done
https://review.coreboot.org/c/coreboot/+/38548/29/src/soc/intel/xeon_sp/cpu.... File src/soc/intel/xeon_sp/cpu.c:
https://review.coreboot.org/c/coreboot/+/38548/29/src/soc/intel/xeon_sp/cpu.... PS29, Line 233: int fixed_msrs[] = {0x268, 0x269, 0x26a, 0x26b, 0x26c, 0x26d, 0x26e, 0x26f}; : for (int i = 0; i < sizeof(fixed_msrs)/sizeof(int); ++i) { : msr_t msr; : msr.lo = 0x05050505; : msr.hi = 0x05050505; : wrmsr(fixed_msrs[i], msr); : } : : // Patch ME Segment uncachable region 7F00 0000 - 7FFF FFFF : msr_t msr; : msr.lo = 0x7f000000; : msr.hi = 0x0; : wrmsr(0x206, msr); : msr.lo = 0xff000800; : msr.hi = 0x3fff; : wrmsr(0x207, msr);
this looks like a kludge. […]
Done
https://review.coreboot.org/c/coreboot/+/38548/29/src/soc/intel/xeon_sp/cpu.... PS29, Line 233: int fixed_msrs[] = {0x268, 0x269, 0x26a, 0x26b, 0x26c, 0x26d, 0x26e, 0x26f}; : for (int i = 0; i < sizeof(fixed_msrs)/sizeof(int); ++i) { : msr_t msr; : msr.lo = 0x05050505; : msr.hi = 0x05050505; : wrmsr(fixed_msrs[i], msr); : } : : // Patch ME Segment uncachable region 7F00 0000 - 7FFF FFFF : msr_t msr; : msr.lo = 0x7f000000; : msr.hi = 0x0; : wrmsr(0x206, msr); : msr.lo = 0xff000800; : msr.hi = 0x3fff; : wrmsr(0x207, msr);
this looks like a kludge. […]
Done
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))
yeah I commented on that file. […]
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 558: while ((reg & ((u32)1 << (pcode_init_bit-1))) == 0) {
I suggest we use stopwatch() here to abort just in case. […]
Done
https://review.coreboot.org/c/coreboot/+/38548/29/src/soc/intel/xeon_sp/soc_... PS29, Line 579: total_delay += step_delay; : if (total_delay >= max_delay) : break;
same here, but I think I already mentioned this piece
Done
https://review.coreboot.org/c/coreboot/+/38548/29/src/soc/intel/xeon_sp/soc_... PS29, Line 768: if ((leaf_b.ecx & 0xff00) == 0x0200) : break;
I feel a bit concerned for this loop. […]
infinite loop concern is not really valid but that function is not needed, so dropped.
https://review.coreboot.org/c/coreboot/+/38548/29/src/soc/intel/xeon_sp/soc_... PS29, Line 891: //assert(hob != NULL && hob_size != 0 && hob_size == sizeof(IIO_UDS));
remove commented out code?
Done
https://review.coreboot.org/c/coreboot/+/38548/32/src/soc/intel/xeon_sp/unco... File src/soc/intel/xeon_sp/uncore.c:
https://review.coreboot.org/c/coreboot/+/38548/32/src/soc/intel/xeon_sp/unco... PS32, Line 18: include <cbmem.h> : #include <console/console.h> : #include <arch/acpi.h> : #include <arch/io.h> : #include <stdint.h> : #include <delay.h> : #include <device/device.h> : #include <device/pci.h> : #include <device/pci_ids.h> : #include <stdlib.h> : #include <string.h> : #include <romstage_handoff.h> : #include <timer.h> : #include <cpu/x86/lapic.h> : : #include <fsp/util.h> : #include <fsp/memory_init.h> : : #include <soc/soc_util.h> : #include <soc/iomap.h> : #include <soc/pci_devs.h> : #include <soc/ramstage.h> : #include <soc/acpi.h>
please include Only what you use. […]
Done
https://review.coreboot.org/c/coreboot/+/38548/32/src/soc/intel/xeon_sp/unco... PS32, Line 18: include <cbmem.h> : #include <console/console.h> : #include <arch/acpi.h> : #include <arch/io.h> : #include <stdint.h> : #include <delay.h> : #include <device/device.h> : #include <device/pci.h> : #include <device/pci_ids.h> : #include <stdlib.h> : #include <string.h> : #include <romstage_handoff.h> : #include <timer.h> : #include <cpu/x86/lapic.h> : : #include <fsp/util.h> : #include <fsp/memory_init.h> : : #include <soc/soc_util.h> : #include <soc/iomap.h> : #include <soc/pci_devs.h> : #include <soc/ramstage.h> : #include <soc/acpi.h>
please include Only what you use. […]
Done