Change in ...coreboot[master]: soc/intel/denverton_ns: Rewrite pmutil using pmclib

[Patrick Georgi (Code Review)]

Hello Julien Viard de Galbert,

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

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

to review the following change.

Change subject: soc/intel/denverton_ns: Rewrite pmutil using pmclib
......................................................................

soc/intel/denverton_ns: Rewrite pmutil using pmclib

Change-Id: I13ab94e7440886f77e3091b16e50e48b17151751
Signed-off-by: Julien Viard de Galbert 
---
M src/soc/intel/denverton_ns/include/soc/pm.h
M src/soc/intel/denverton_ns/pmutil.c
M src/soc/intel/denverton_ns/smihandler.c
M src/soc/intel/denverton_ns/smm.c
4 files changed, 33 insertions(+), 184 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/31072/1

diff --git a/src/soc/intel/denverton_ns/include/soc/pm.h b/src/soc/intel/denverton_ns/include/soc/pm.h
index 09ce158..0aab0e2 100644
--- a/src/soc/intel/denverton_ns/include/soc/pm.h
+++ b/src/soc/intel/denverton_ns/include/soc/pm.h
@@ -47,18 +47,6 @@
 struct chipset_power_state *fill_power_state(void);

/* Power Management Utility Functions. */
-uint32_t clear_smi_status(void);
-uint16_t clear_pm1_status(void);
-uint32_t clear_tco_status(void);
-uint32_t clear_gpe_status(void);
 void clear_pmc_status(void);
-void enable_smi(uint32_t mask);
-void disable_smi(uint32_t mask);
-void enable_pm1(uint16_t events);
-void enable_pm1_control(uint32_t mask);
-void disable_pm1_control(uint32_t mask);
-void enable_gpe(uint32_t mask);
-void disable_gpe(uint32_t mask);
-void disable_all_gpe(void);

#endif /* _DENVERTON_NS_PM_H_ */
diff --git a/src/soc/intel/denverton_ns/pmutil.c b/src/soc/intel/denverton_ns/pmutil.c
index ccf0d95..344f8b9 100644
--- a/src/soc/intel/denverton_ns/pmutil.c
+++ b/src/soc/intel/denverton_ns/pmutil.c
@@ -19,29 +19,12 @@
 #include <arch/io.h>
 #include <console/console.h>

+#include <intelblocks/pmclib.h>
 #include <soc/iomap.h>
 #include <soc/soc_util.h>
 #include <soc/pm.h>

-static void print_num_status_bits(int num_bits, uint32_t status,
-				  const char *const bit_names[])
-{
-	int i;
-
-	if (!status)
-		return;
-
-	for (i = num_bits - 1; i >= 0; i--) {
-		if (status & (1 << i)) {
-			if (bit_names[i])
-				printk(BIOS_DEBUG, "%s ", bit_names[i]);
-			else
-				printk(BIOS_DEBUG, "BIT%d ", i);
-		}
-	}
-}
-
-static uint32_t print_smi_status(uint32_t smi_sts)
+const char *const *soc_smi_sts_array(size_t *a)
 {
    static const char *const smi_sts_bits[] = {
    	[2] = "BIOS",
@@ -67,93 +50,11 @@
    	[31] = "LEGACY_USB3",
    };

-	if (!smi_sts)
-		return 0;
-
-	printk(BIOS_DEBUG, "SMI_STS: ");
-	print_num_status_bits(ARRAY_SIZE(smi_sts_bits), smi_sts, smi_sts_bits);
-	printk(BIOS_DEBUG, "\n");
-
-	return smi_sts;
+	*a = ARRAY_SIZE(smi_sts_bits);
+	return smi_sts_bits;
 }

-static uint32_t reset_smi_status(void)
-{
-	uint16_t pmbase = get_pmbase();
-	uint32_t smi_sts = inl((uint16_t)(pmbase + SMI_STS));
-	outl(smi_sts, (uint16_t)(pmbase + SMI_STS));
-	return smi_sts;
-}
-
-uint32_t clear_smi_status(void) { return print_smi_status(reset_smi_status()); }
-
-void enable_smi(uint32_t mask)
-{
-	uint16_t pmbase = get_pmbase();
-	uint32_t smi_en = inl((uint16_t)(pmbase + SMI_EN));
-	smi_en |= mask;
-	outl(smi_en, (uint16_t)(pmbase + SMI_EN));
-}
-
-void disable_smi(uint32_t mask)
-{
-	uint16_t pmbase = get_pmbase();
-	uint32_t smi_en = inl((uint16_t)(pmbase + SMI_EN));
-	smi_en &= ~mask;
-	outl(smi_en, (uint16_t)(pmbase + SMI_EN));
-}
-
-void enable_pm1_control(uint32_t mask)
-{
-	uint16_t pmbase = get_pmbase();
-	uint32_t pm1_cnt = inl((uint16_t)(pmbase + PM1_CNT));
-	pm1_cnt |= mask;
-	outl(pm1_cnt, (uint16_t)(pmbase + PM1_CNT));
-}
-
-void disable_pm1_control(uint32_t mask)
-{
-	uint16_t pmbase = get_pmbase();
-	uint32_t pm1_cnt = inl((uint16_t)(pmbase + PM1_CNT));
-	pm1_cnt &= ~mask;
-	outl(pm1_cnt, (uint16_t)(pmbase + PM1_CNT));
-}
-
-static uint16_t reset_pm1_status(void)
-{
-	uint16_t pmbase = get_pmbase();
-	uint16_t pm1_sts = inw((uint16_t)(pmbase + PM1_STS));
-	outw(pm1_sts, (uint16_t)(pmbase + PM1_STS));
-	return pm1_sts;
-}
-
-static uint16_t print_pm1_status(uint16_t pm1_sts)
-{
-	static const char *const pm1_sts_bits[] = {
-		[0] = "TMROF",  [4] = "BM",   [5] = "GBL",
-		[8] = "PWRBTN", [10] = "RTC", [11] = "PRBTNOR",
-		[15] = "WAK",
-	};
-
-	if (!pm1_sts)
-		return 0;
-
-	printk(BIOS_SPEW, "PM1_STS: ");
-	print_num_status_bits(ARRAY_SIZE(pm1_sts_bits), pm1_sts, pm1_sts_bits);
-	printk(BIOS_SPEW, "\n");
-
-	return pm1_sts;
-}
-
-uint16_t clear_pm1_status(void) { return print_pm1_status(reset_pm1_status()); }
-
-void enable_pm1(uint16_t events)
-{
-	uint16_t pmbase = get_pmbase();
-	outw(events, (uint16_t)(pmbase + PM1_EN));
-}
-
-static uint32_t print_tco_status(uint32_t tco_sts)
+const char *const *soc_tco_sts_array(size_t *a)
 {
    static const char *const tco_sts_bits[] = {
    	[0] = "NMI2SMI",     [1] = "OS_TCO_SMI",
@@ -164,17 +65,11 @@
    	[17] = "SECOND_TO",  [20] = "SMLINK_SLV_SMI",
    };

-	if (!tco_sts)
-		return 0;
-
-	printk(BIOS_DEBUG, "TCO_STS: ");
-	print_num_status_bits(ARRAY_SIZE(tco_sts_bits), tco_sts, tco_sts_bits);
-	printk(BIOS_DEBUG, "\n");
-
-	return tco_sts;
+	*a = ARRAY_SIZE(tco_sts_bits);
+	return tco_sts_bits;
 }

-static uint32_t reset_tco_status(void)
+uint32_t soc_reset_tco_status(void)
 {
    uint16_t tcobase = get_tcobase();
    uint32_t tco_sts = inl((uint16_t)(tcobase + TCO1_STS));
@@ -184,35 +79,7 @@
    return tco_sts & tco_en;
 }

-uint32_t clear_tco_status(void) { return print_tco_status(reset_tco_status()); }
-
-void enable_gpe(uint32_t mask)
-{
-	uint16_t pmbase = get_pmbase();
-	uint32_t gpe0_en = inl((uint16_t)(pmbase + GPE0_EN(GPE_STD)));
-	gpe0_en |= mask;
-	outl(gpe0_en, (uint16_t)(pmbase + GPE0_EN(GPE_STD)));
-}
-
-void disable_gpe(uint32_t mask)
-{
-	uint16_t pmbase = get_pmbase();
-	uint32_t gpe0_en = inl((uint16_t)(pmbase + GPE0_EN(GPE_STD)));
-	gpe0_en &= ~mask;
-	outl(gpe0_en, (uint16_t)(pmbase + GPE0_EN(GPE_STD)));
-}
-
-void disable_all_gpe(void) { disable_gpe(~0); }
-
-static uint32_t reset_gpe_status(void)
-{
-	uint16_t pmbase = get_pmbase();
-	uint32_t gpe_sts = inl((uint16_t)(pmbase + GPE0_STS(GPE_STD)));
-	outl(gpe_sts, (uint16_t)(pmbase + GPE0_STS(GPE_STD)));
-	return gpe_sts;
-}
-
-static uint32_t print_gpe_sts(uint32_t gpe_sts)
+const char *const *soc_std_gpe_sts_array(size_t *a)
 {
    static const char *const gpe_sts_bits[] = {
    	[0] = "GPIO_0", [1] = "GPIO_1",
@@ -233,16 +100,8 @@
    	[30] = "GPIO_30", [31] = "GPIO_31",
    };

-	if (!gpe_sts)
-		return gpe_sts;
-
-	printk(BIOS_DEBUG, "GPE0a_STS: ");
-	print_num_status_bits(ARRAY_SIZE(gpe_sts_bits), gpe_sts, gpe_sts_bits);
-	printk(BIOS_DEBUG, "\n");
-
-	return gpe_sts;
+	*a = ARRAY_SIZE(gpe_sts_bits);
+	return gpe_sts_bits;
 }

-uint32_t clear_gpe_status(void) { return print_gpe_sts(reset_gpe_status()); }
-
 void clear_pmc_status(void) { /* TODO */ }
diff --git a/src/soc/intel/denverton_ns/smihandler.c b/src/soc/intel/denverton_ns/smihandler.c
index 85f99d7..e1e7bc4 100644
--- a/src/soc/intel/denverton_ns/smihandler.c
+++ b/src/soc/intel/denverton_ns/smihandler.c
@@ -25,6 +25,7 @@
 #include <device/pci_def.h>
 #include <elog.h>
 #include <intelblocks/fast_spi.h>
+#include <intelblocks/pmclib.h>
 #include <spi-generic.h>
 #include <soc/iomap.h>
 #include <soc/soc_util.h>
@@ -52,7 +53,7 @@
    return 0;
 }

-void southbridge_smi_set_eos(void) { enable_smi(EOS); }
+void southbridge_smi_set_eos(void) { pmc_enable_smi(EOS); }

global_nvs_t *smm_get_gnvs(void) { return gnvs; }

@@ -98,7 +99,7 @@
    uint16_t pmbase = get_pmbase();

/* First, disable further SMIs */
-	disable_smi(SLP_SMI_EN);
+	pmc_disable_smi(SLP_SMI_EN);

/* Figure out SLP_TYP */
    reg32 = inl((uint16_t)(pmbase + PM1_CNT));
@@ -131,7 +132,7 @@
    	printk(BIOS_DEBUG, "SMI#: Entering S5 (Soft Power off)\n");

/* Disable all GPE */
-		disable_all_gpe();
+		pmc_disable_all_gpe();

/* also iterates over all bridges on bus 0 */
    	busmaster_disable_on_bus(0);
@@ -145,7 +146,7 @@
     * event again. We need to set BIT13 (SLP_EN) though to make the
     * sleep happen.
     */
-	enable_pm1_control(SLP_EN);
+	pmc_enable_pm1_control(SLP_EN);

/* Make sure to stop executing code here for S3/S4/S5 */
    if (slp_typ > 1)
@@ -158,7 +159,7 @@
    reg32 = inl((uint16_t)(pmbase + PM1_CNT));
    if (reg32 & SCI_EN) {
    	/* The OS is not an ACPI OS, so we set the state to S0 */
-		disable_pm1_control(SLP_EN | SLP_TYP);
+		pmc_disable_pm1_control(SLP_EN | SLP_TYP);
    }
 }

@@ -237,11 +238,11 @@
    	printk(BIOS_DEBUG, "P-state control\n");
    	break;
    case APM_CNT_ACPI_DISABLE:
-		disable_pm1_control(SCI_EN);
+		pmc_disable_pm1_control(SCI_EN);
    	printk(BIOS_DEBUG, "SMI#: ACPI disabled.\n");
    	break;
    case APM_CNT_ACPI_ENABLE:
-		enable_pm1_control(SCI_EN);
+		pmc_enable_pm1_control(SCI_EN);
    	printk(BIOS_DEBUG, "SMI#: ACPI enabled.\n");
    	break;
    case APM_CNT_FINALIZE:
@@ -268,23 +269,23 @@

static void southbridge_smi_pm1(void)
 {
-	uint16_t pm1_sts = clear_pm1_status();
+	uint16_t pm1_sts = pmc_clear_pm1_status();

/* While OSPM is not active, poweroff immediately
     * on a power button event.
     */
    if (pm1_sts & PWRBTN_STS) {
    	// power button pressed
-		disable_pm1_control(-1UL);
-		enable_pm1_control(SLP_EN | (SLP_TYP_S5 << SLP_TYP_SHIFT));
+		pmc_disable_pm1_control(-1UL);
+		pmc_enable_pm1_control(SLP_EN | (SLP_TYP_S5 << SLP_TYP_SHIFT));
    }
 }

-static void southbridge_smi_gpe0(void) { clear_gpe_status(); }
+static void southbridge_smi_gpe0(void) { pmc_clear_all_gpe_status(); }

static void southbridge_smi_tco(void)
 {
-	uint32_t tco_sts = clear_tco_status();
+	uint32_t tco_sts = pmc_clear_tco_status();

/* Any TCO event? */
    if (!tco_sts)
@@ -354,7 +355,7 @@
    /* We need to clear the SMI status registers, or we won't see what's
     * happening in the following calls.
     */
-	smi_sts = clear_smi_status();
+	smi_sts = pmc_clear_smi_status();

/* Call SMI sub handler for each of the status bits */
    for (i = 0; i < ARRAY_SIZE(southbridge_smi); i++) {
diff --git a/src/soc/intel/denverton_ns/smm.c b/src/soc/intel/denverton_ns/smm.c
index 9d3fa75..65d2499 100644
--- a/src/soc/intel/denverton_ns/smm.c
+++ b/src/soc/intel/denverton_ns/smm.c
@@ -23,6 +23,7 @@
 #include <cpu/x86/smm.h>
 #include <string.h>

+#include <intelblocks/pmclib.h>
 #include <soc/iomap.h>
 #include <soc/soc_util.h>
 #include <soc/pm.h>
@@ -48,10 +49,10 @@
    }

/* Dump and clear status registers */
-	clear_smi_status();
-	clear_pm1_status();
-	clear_tco_status();
-	clear_gpe_status();
+	pmc_clear_smi_status();
+	pmc_clear_pm1_status();
+	pmc_clear_tco_status();
+	pmc_clear_all_gpe_status();
    clear_pmc_status();
 }

@@ -60,8 +61,8 @@

printk(BIOS_DEBUG, "Enabling SMIs.\n");
    /* Configure events Disable pcie wake. */
-	enable_pm1(PWRBTN_EN | GBL_EN | PCIEXPWAK_DIS);
-	disable_gpe(PME_B0_EN);
+	pmc_enable_pm1(PWRBTN_EN | GBL_EN | PCIEXPWAK_DIS);
+	pmc_disable_std_gpe(PME_B0_EN);

/* Enable SMI generation:
     *  - on APMC writes (io 0xb2)
@@ -71,7 +72,7 @@
     *  - on TCO events
     *  - on microcontroller writes (io 0x62/0x66)
     */
-	enable_smi(APMC_EN | SLP_SMI_EN | GBL_SMI_EN | EOS);
+	pmc_enable_smi(APMC_EN | SLP_SMI_EN | GBL_SMI_EN | EOS);
 }

void smm_setup_structures(void *gnvs, void *tcg, void *smi1)

Change in coreboot[master]: soc/intel/denverton_ns: Rewrite pmutil using pmclib

[David Guckian (Code Review)]

David Guckian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31072 )

Change subject: soc/intel/denverton_ns: Rewrite pmutil using pmclib
......................................................................

Patch Set 8: Code-Review+1

Change in coreboot[master]: soc/intel/denverton_ns: Rewrite pmutil using pmclib

[Patrick Georgi (Code Review)]

Hello build bot (Jenkins), David Guckian, Julien Viard de Galbert, Vanessa Eusebio, Patrick Rudolph, David Guckian,

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#9).

Change subject: soc/intel/denverton_ns: Rewrite pmutil using pmclib
......................................................................

soc/intel/denverton_ns: Rewrite pmutil using pmclib

Change-Id: I13ab94e7440886f77e3091b16e50e48b17151751
Signed-off-by: Julien Viard de Galbert 
---
M src/soc/intel/denverton_ns/include/soc/pm.h
M src/soc/intel/denverton_ns/pmutil.c
M src/soc/intel/denverton_ns/smihandler.c
M src/soc/intel/denverton_ns/smm.c
4 files changed, 33 insertions(+), 184 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/31072/9

Change in coreboot[master]: soc/intel/denverton_ns: Rewrite pmutil using pmclib

[Patrick Georgi (Code Review)]

Hello build bot (Jenkins), David Guckian, Julien Viard de Galbert, Vanessa Eusebio, Patrick Rudolph, David Guckian,

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#12).

Change subject: soc/intel/denverton_ns: Rewrite pmutil using pmclib
......................................................................

soc/intel/denverton_ns: Rewrite pmutil using pmclib

Change-Id: I13ab94e7440886f77e3091b16e50e48b17151751
Signed-off-by: Julien Viard de Galbert 
---
M src/soc/intel/denverton_ns/include/soc/pm.h
M src/soc/intel/denverton_ns/pmutil.c
M src/soc/intel/denverton_ns/smihandler.c
M src/soc/intel/denverton_ns/smm.c
4 files changed, 33 insertions(+), 184 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/31072/12

Change in coreboot[master]: soc/intel/denverton_ns: Rewrite pmutil using pmclib

[Julien Viard de Galbert (Code Review)]

Julien Viard de Galbert has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31072 )

Change subject: soc/intel/denverton_ns: Rewrite pmutil using pmclib
......................................................................

Patch Set 12: Code-Review-1

This needs more change, TCO now require SMBUS which also need to be enabled for denverton.

Change in coreboot[master]: soc/intel/denverton_ns: Rewrite pmutil using pmclib

[Patrick Georgi (Code Review)]

Attention is currently required from: Mariusz Szafrański, Suresh Bellampalli, Michal Motyl.
Hello build bot (Jenkins), David Guckian, Julien Viard de Galbert, Vanessa Eusebio, Patrick Rudolph, David Guckian,

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#13).

Change subject: soc/intel/denverton_ns: Rewrite pmutil using pmclib
......................................................................

soc/intel/denverton_ns: Rewrite pmutil using pmclib

Change-Id: I13ab94e7440886f77e3091b16e50e48b17151751
Signed-off-by: Julien Viard de Galbert 
---
M src/soc/intel/denverton_ns/include/soc/pm.h
M src/soc/intel/denverton_ns/pmutil.c
M src/soc/intel/denverton_ns/smihandler.c
M src/soc/intel/denverton_ns/smm.c
4 files changed, 33 insertions(+), 184 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/31072/13

Change in coreboot[master]: soc/intel/denverton_ns: Rewrite pmutil using pmclib

[Angel Pons (Code Review)]

Attention is currently required from: Mariusz Szafrański, Suresh Bellampalli, Michal Motyl.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31072 )

Change subject: soc/intel/denverton_ns: Rewrite pmutil using pmclib
......................................................................

Patch Set 13: Code-Review+1

Change in coreboot[master]: soc/intel/denverton_ns: Rewrite pmutil using pmclib

[King Sumo (Code Review)]

Attention is currently required from: Mariusz Szafrański, Suresh Bellampalli, Michal Motyl.
King Sumo has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31072 )

Change subject: soc/intel/denverton_ns: Rewrite pmutil using pmclib
......................................................................

Patch Set 13:

(1 comment)

Patchset:

PS13: 
Do we have any pending implementation for this patch?

Change in coreboot[master]: soc/intel/denverton_ns: Rewrite pmutil using pmclib

[Patrick Georgi (Code Review)]

Patrick Georgi has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/31072 )

Change subject: soc/intel/denverton_ns: Rewrite pmutil using pmclib
......................................................................

Abandoned