[coreboot-gerrit] Change in coreboot[master]: soc/intel/common/pmc: Avoid unnecessary writes of AFTERG3_EN

[Nico Huber (Code Review)]

Attention is currently required from: Angel Pons.
Hello Angel Pons,

I'd like you to do a code review. Please visit

https://review.coreboot.org/c/coreboot/+/55539

to review the following change.

Change subject: soc/intel/common/pmc: Avoid unnecessary writes of AFTERG3_EN
......................................................................

soc/intel/common/pmc: Avoid unnecessary writes of AFTERG3_EN

pmc_set_power_failure_state() is usually called twice, once upon boot
(with `target_on == true`) and once from SMM when the system is shut
down (with `target_on == false`). Assuming settings didn't change
between these calls, there is only one case where we actually need
to write the register value: when updating the state for the
MAINBOARD_POWER_STATE_PREVIOUS feature.

This suits us well as we want to avoid unnecessary writes so we
don't clobber the value set upon boot from within SMM. Due to
inaccessible option backends, SMM might not know the current
option state.

The assumption above, that the option value didn't change, may not
be true if the user changed the option on purpose. In the future,
one would have to reboot the machine for option changes to take
effect. However, this doesn't seem to make a huge difference: One
already needed a controlled shutdown for the update to take effect
before. A reboot doesn't seem much more expensive.

Change-Id: I58dd74b8b073f4f07db090bf7fddb14b5df8239a
Signed-off-by: Nico Huber 
---
M src/soc/intel/common/block/pmc/pmclib.c
1 file changed, 15 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/55539/1

diff --git a/src/soc/intel/common/block/pmc/pmclib.c b/src/soc/intel/common/block/pmc/pmclib.c
index aa5b362..d3a21a1 100644
--- a/src/soc/intel/common/block/pmc/pmclib.c
+++ b/src/soc/intel/common/block/pmc/pmclib.c
@@ -581,31 +581,38 @@

void pmc_set_power_failure_state(const bool target_on)
 {
-	bool on;
-
    const unsigned int state = get_uint_option("power_on_after_fail",
    				 CONFIG_MAINBOARD_POWER_FAILURE_STATE);

+	/*
+	 * On the shutdown path (target_on == false), we only need to
+	 * update the register for MAINBOARD_POWER_STATE_PREVIOUS. For
+	 * all other cases, we don't write the register to avoid clob-
+	 * bering the value set on the boot path. This is necessary,
+	 * for instance, when we can't access the option backend in SMM.
+	 */
+
    switch (state) {
    case MAINBOARD_POWER_STATE_OFF:
+		if (!target_on)
+			break;
    	printk(BIOS_INFO, "Set power off after power failure.\n");
-		on = false;
+		pmc_soc_set_afterg3_en(false);
    	break;
    case MAINBOARD_POWER_STATE_ON:
+		if (!target_on)
+			break;
    	printk(BIOS_INFO, "Set power on after power failure.\n");
-		on = true;
+		pmc_soc_set_afterg3_en(true);
    	break;
    case MAINBOARD_POWER_STATE_PREVIOUS:
    	printk(BIOS_INFO, "Keep power state after power failure.\n");
-		on = target_on;
+		pmc_soc_set_afterg3_en(target_on);
    	break;
    default:
    	printk(BIOS_WARNING, "WARNING: Unknown power-failure state: %d\n", state);
-		on = false;
    	break;
    }
-
-	pmc_soc_set_afterg3_en(on);
 }

/* This function returns the highest assertion duration of the SLP_Sx assertion widths */