Máté Kukri has posted comments on this change by Máté Kukri. ( https://review.coreboot.org/c/coreboot/+/81529?usp=email )
Change subject: mb/dell/optiplex_9020: Implement late HWM initialization ......................................................................
Patch Set 11:
(1 comment)
File src/mainboard/dell/optiplex_9020/mainboard.c:
https://review.coreboot.org/c/coreboot/+/81529/comment/989d1a6f_0e397db8?usp... : PS11, Line 310: / rapl_power_unit I think the low 4 bits of MSR_RAPL_POWER_UNIT are simply never 0 on the hardware in question (And likely all hardware given it being a "unit"). This of course makes the `if` both misleading and redundant.
The first `if` can be removed, but I think the calculation should remain `rapl_power_unit = 1 << (rapl_power_unit - 1)`, which was intentional, and the `- 1` is acceptable if `rapl_power_unit > 0` (which I believe to be a valid assumption).
When the MSR value is > 7, storing the result in the uint8_t makes it 0
This might be a problem (although I haven't seen it in practice) if the low 4 bits can be `> 7` and `<= 0xf`.
Changing both variables to `unsinged int`, and removing the first `if` is what I would suggest as a solution. (Or if there is a desire to guard against initial 0, then the whole function could default to 32 in that case)