Attention is currently required from: Keith Hui, Nico Huber, Paul Menzel.
Angel Pons has posted comments on this change by Keith Hui. ( https://review.coreboot.org/c/coreboot/+/82632?usp=email )
Change subject: sio/nuvoton: Implement a common ramstage ACPI LDN helper ......................................................................
Patch Set 6:
(3 comments)
File src/superio/nuvoton/common/common.c:
https://review.coreboot.org/c/coreboot/+/82632/comment/fd50952e_60683c9a?usp... : PS6, Line 66: & 3
I originally do this so I can force any out-of-range value to be within range, but it leaves one val […]
Problem is, an out-of-range value is not a valid value. By masking the result, a value of 4 (invalid) would incorrectly be treated as 0 (off). That's why I suggested removing the mask: if the value is out-of-range (i.e. `power_status <= 2` is not true), then do nothing.
Note that "user-define" isn't actually another mode. It's the same as "keep".
As for the "keep" option: it works by setting the `Power-loss Last State Flag` bit in the other register. I'm not sure if that flag is honoured in the other two modes ("off" and "on").
https://review.coreboot.org/c/coreboot/+/82632/comment/99769cbb_ed21fbeb?usp... : PS6, Line 79: * Clear case open pin 0 status.
Page 81 of datasheet: […]
For now I would omit this code.
https://review.coreboot.org/c/coreboot/+/82632/comment/2e9f4053_8b9bde9b?usp... : PS6, Line 80: Set user-defined power state to "off" just in case.
Page 371 of NCT6779D datasheet: […]
Yes, that's how it should be handled. I believe coreboot already does this for the equivalent bit in the southbridge.
-
Angel Pons (Code Review)