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 30:
(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?
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. Why we are hijacking MTRR setup that is done by common code?
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?
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. Otherwise this is a potential hard-hang
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
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
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. Would it be possible this never evaluates to true?
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?
https://review.coreboot.org/c/coreboot/+/38548/29/src/soc/intel/xeon_sp/soc_... PS29, Line 929: sizeof(ids)/sizeof(int); ARRAY_SIZE()
https://review.coreboot.org/c/coreboot/+/38548/29/src/soc/intel/xeon_sp/soc_... PS29, Line 1046: izeof(regs)/sizeof(uint64_t) ditto