[coreboot] [commit] r6626 - trunk/src/mainboard/asrock/e350m1

Scott Duplichan scott at notabs.org
Sun Jun 5 09:39:28 CEST 2011


Stefan Reinauer wrote:

]> +  // all cores: set pstate 0 (1600 MHz) early to save a few ms of boot
]time
]> + __writemsr (0xc0010062, 0);
]> +
]
]why not use writemsr instead of __writemsr?

Hello Stefan,

The wrmsr function is certainly OK. I am in the habit us using the
Intel/MS functions because they are built into the Microsoft compiler
and therefore portable between all UEFI vendor code bases and any
other code that uses the MS complier. But portability with UEFI is
not the reason I still sometimes use these function types. Truthfully,
the coreboot wrmsr function seems awkward to me. With the Intel/MS
__writemsr function, any 64-bit value can be written with a single
statement. Coreboot code using wrmsr is often taking 4 statements to
do the same thing. While there is probably little difference in
generated code, using the coreboot wrmsr function does require more
source code typing. I also find it less readable. For example,

// set bit 46 of NB_CFG_MSR using wrmsr function:
msr_t msr;
msr = rdmsr(NB_CFG_MSR);
msr.hi |= (1 << (46 - 32));
wrmsr(NB_CFG_MSR, msr);

// Making msr_t a union that adds a uint64_t would allow:
msr_t msr;
msr = rdmsr(NB_CFG_MSR);
msr.all |= 1 << 46;
wrmsr(NB_CFG_MSR, msr);

// The Intel/MS style uint64_t prototypes allow:
__writemsr (__readmsr (NB_CFG_MSR) | 1 << 46);

A question I have wanted to ask for a while is why coreboot
doesn't use 64 bit integers in situations like this?

Thanks,
Scott





More information about the coreboot mailing list