Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46324 )
Change subject: soc/intel/broadwell: Revise SA lockdown sequence ......................................................................
Patch Set 10: Code-Review+2
(3 comments)
TBH, it's a mess. I don't understand the reasons to not follow the order of the spec. The added comment makes a few things better, others worse, IMHO. But this review is too annoying and I don't see it breaking anything. I'll not look into it again.
https://review.coreboot.org/c/coreboot/+/46324/6/src/soc/intel/broadwell/fin... File src/soc/intel/broadwell/finalize.c:
https://review.coreboot.org/c/coreboot/+/46324/6/src/soc/intel/broadwell/fin... PS6, Line 16: /* : * 16.6 System Agent Configuration Locking : * "5th Generation Intel Core Processor Family BIOS Specification" : * Document Number 535094 : * Revision 2.2.0, August 2014 : */
Alternatively we could follow the manual wrt. the order. I don't think it's […]
Done
https://review.coreboot.org/c/coreboot/+/46324/10/src/soc/intel/broadwell/fi... File src/soc/intel/broadwell/finalize.c:
https://review.coreboot.org/c/coreboot/+/46324/10/src/soc/intel/broadwell/fi... PS10, Line 37: REG_MMIO_OR32(MCH_BASE_ADDRESS + 0x50fc, 0x8f), /* MC */ It's actually not literally what the spec says. We're supposed to set bits 7..0, so should `& ~0xff` first. But let's ignore that for now, as it was like this already.
https://review.coreboot.org/c/coreboot/+/46324/10/src/soc/intel/broadwell/fi... PS10, Line 39: REG_MMIO_OR32(MCH_BASE_ADDRESS + 0x5880, 1 << 5), /* DDR PTM */ Not covered by the comment above this function, AFAICS.