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:
(3 comments)
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) \
Prefer not to put the register name twice.
I am just saying your preprocessor macro generates more code than needed. you probably want to do something like this:
void my_printmsr(unsigned int id, const char *name) { // your function logic goes here }
#define DUMP_MSR(id) \ { \ my_printmsr(id, #id); \ }
DUMP_MSR(abc); DUMP_MSR(xyz);
this way you will have less generated code and less binary code footprint.
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 also why do not we just print "%08x%08x", msr.hi, msr.lo unconditionally?
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))
See src/soc/intel/xeon_sp/soc_util. […]
yeah I commented on that file. We need to use pcr_write()/pcr_read()