Attention is currently required from: Máté Kukri.
Nico Huber 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/bf5c3287_680e8e66?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.
If we account for the "the hardware in question", I'm rather sure the unit is always `2^3`. Then we could drop the whole register decoding. I wouldn't be against it if we can find a reference for this. What I wouldn't like is code that looks like it's going by the book but mixes that with assumptions.
0 is a defined value, meaning 1W, so I see no reason to treat it special. Or maybe I started on the wrong food and read the wrong reference (I think it was the SDM), then please tell me.
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).
That's not what the code currently says. It says `2 << (x - 1) ~= 2 * 2^(x-1) = 2^x`. And I think that's actually correct except that it feels like an unnatural way of putting it and has the underflow issue.
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`.
Probably a small-core thing, I think in volume 4 of the SDM it mentioned a 1000b somewhere.
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)
I guess I just don't get why you don't like the 0 :D or I'm really reading the wrong docs.