Change in ...coreboot[master]: soc/intel/braswell/pmutil.c: Use GEN_PMCON1 for RTC status

[Frans Hendriks (Code Review)]

Frans Hendriks has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32724

Change subject: soc/intel/braswell/pmutil.c:  Use GEN_PMCON1 for RTC status
......................................................................

soc/intel/braswell/pmutil.c:  Use GEN_PMCON1 for RTC status

cbmem_find is not available in every stage.
Remove usage of cbmem_find() and use GEN_PMCON1 always.

BUG=NA
TEST=Booting Embedded Linux on Facebook FBG-1701

Change-Id: Id97d57864b3e241e8f046d9b1caebdce199a46b1
Signed-off-by: Frans Hendriks 
---
M src/soc/intel/braswell/pmutil.c
1 file changed, 1 insertion(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/24/32724/1

diff --git a/src/soc/intel/braswell/pmutil.c b/src/soc/intel/braswell/pmutil.c
index 271eefe..5608bf2 100644
--- a/src/soc/intel/braswell/pmutil.c
+++ b/src/soc/intel/braswell/pmutil.c
@@ -18,7 +18,6 @@
 #include <arch/io.h>
 #include <device/mmio.h>
 #include <device/pci_ops.h>
-#include <cbmem.h>
 #include <console/console.h>
 #include <soc/iomap.h>
 #include <soc/lpc.h>
@@ -364,15 +363,9 @@
 {
    uint32_t gen_pmcon1;
    int rtc_fail;
-	struct chipset_power_state *ps = cbmem_find(CBMEM_ID_POWER_STATE);

-	if (ps != NULL)
-		gen_pmcon1 = ps->gen_pmcon1;
-	else
-		gen_pmcon1 = read32((u32 *)(PMC_BASE_ADDRESS + GEN_PMCON1));
-
+	gen_pmcon1 = read32((u32 *)(PMC_BASE_ADDRESS + GEN_PMCON1));
    rtc_fail = !!(gen_pmcon1 & RPS);
-
    if (rtc_fail)
    	printk(BIOS_DEBUG, "RTC failure.\n");

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32724 )

Change subject: soc/intel/braswell/pmutil.c:  Use GEN_PMCON1 for RTC status
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/32724/1/src/soc/intel/braswell/pmutil.c 
File src/soc/intel/braswell/pmutil.c:

https://review.coreboot.org/#/c/32724/1/src/soc/intel/braswell/pmutil.c@367 
PS1, Line 367: 	gen_pmcon1 = read32((u32 *)(PMC_BASE_ADDRESS + GEN_PMCON1));
This reads the current state that may already have been altered by
coreboot. clear_pmc_status() above is called before MP init. I'm not
sure if rtc_failure() is called anywhere after that.

Maybe we could do something like this:

struct chipset_power_state *ps = NULL;
    if (!ENV_BOOTBLOCK && !ENV_VERSTAGE)
        ps = cbmem_find(CBMEM_ID_POWER_STATE);

[Frans Hendriks (Code Review)]

Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32724 )

Change subject: soc/intel/braswell/pmutil.c:  Use GEN_PMCON1 for RTC status
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/32724/1/src/soc/intel/braswell/pmutil.c 
File src/soc/intel/braswell/pmutil.c:

https://review.coreboot.org/#/c/32724/1/src/soc/intel/braswell/pmutil.c@367 
PS1, Line 367: 	gen_pmcon1 = read32((u32 *)(PMC_BASE_ADDRESS + GEN_PMCON1));
> This reads the current state that may already have been altered by […]

rtc_failure() is called by car_soc_post_console_init() and vbnv_cmos_failed().
For non-VBOOT platform I could verify that it works fine.
I noticed that vbnv_cmos_failed() is used in VERSTAGE only.
So this patch set will works in actual situations.

To be safe I will implement your suggestion.

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32724 )

Change subject: soc/intel/braswell/pmutil.c:  Use GEN_PMCON1 for RTC status
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/32724/1/src/soc/intel/braswell/pmutil.c 
File src/soc/intel/braswell/pmutil.c:

https://review.coreboot.org/#/c/32724/1/src/soc/intel/braswell/pmutil.c@367 
PS1, Line 367: 	gen_pmcon1 = read32((u32 *)(PMC_BASE_ADDRESS + GEN_PMCON1));
> rtc_failure() is called by car_soc_post_console_init() and vbnv_cmos_failed(). […]

Hmm, in that case, we could also drop the cbmem_find() as you did and
try to blacklist this function in ramstage. e.g.

#include <assert.h>

/* not usable in ramstage as GEN_PMCON1 gets reset */
    if (ENV_RAMSTAGE)
            dead_code();

Your call.

[Frans Hendriks (Code Review)]

Hello Patrick Rudolph, Matt DeVillier, build bot (Jenkins), Nico Huber, Michał Żygowski,

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

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

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

Change subject: soc/intel/braswell/pmutil.c:  Use GEN_PMCON1 for RTC status
......................................................................

soc/intel/braswell/pmutil.c:  Use GEN_PMCON1 for RTC status

cbmem_find is not available in every stage.
Remove usage of cbmem_find() and use GEN_PMCON1 always.

BUG=NA
TEST=Booting Embedded Linux on Facebook FBG-1701

Change-Id: Id97d57864b3e241e8f046d9b1caebdce199a46b1
Signed-off-by: Frans Hendriks 
---
M src/soc/intel/braswell/pmutil.c
1 file changed, 6 insertions(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/24/32724/2

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32724 )

Change subject: soc/intel/braswell/pmutil.c:  Use GEN_PMCON1 for RTC status
......................................................................

Patch Set 2: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32724 )

Change subject: soc/intel/braswell/pmutil.c:  Use GEN_PMCON1 for RTC status
......................................................................

soc/intel/braswell/pmutil.c:  Use GEN_PMCON1 for RTC status

cbmem_find is not available in every stage.
Remove usage of cbmem_find() and use GEN_PMCON1 always.

BUG=NA
TEST=Booting Embedded Linux on Facebook FBG-1701

Change-Id: Id97d57864b3e241e8f046d9b1caebdce199a46b1
Signed-off-by: Frans Hendriks 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/32724
Tested-by: build bot (Jenkins) 
Reviewed-by: Nico Huber 
---
M src/soc/intel/braswell/pmutil.c
1 file changed, 6 insertions(+), 7 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, approved

diff --git a/src/soc/intel/braswell/pmutil.c b/src/soc/intel/braswell/pmutil.c
index 271eefe..4bc621b 100644
--- a/src/soc/intel/braswell/pmutil.c
+++ b/src/soc/intel/braswell/pmutil.c
@@ -16,9 +16,9 @@

#include <arch/acpi.h>
 #include <arch/io.h>
+#include <assert.h>
 #include <device/mmio.h>
 #include <device/pci_ops.h>
-#include <cbmem.h>
 #include <console/console.h>
 #include <soc/iomap.h>
 #include <soc/lpc.h>
@@ -364,15 +364,14 @@
 {
    uint32_t gen_pmcon1;
    int rtc_fail;
-	struct chipset_power_state *ps = cbmem_find(CBMEM_ID_POWER_STATE);

-	if (ps != NULL)
-		gen_pmcon1 = ps->gen_pmcon1;
-	else
-		gen_pmcon1 = read32((u32 *)(PMC_BASE_ADDRESS + GEN_PMCON1));
+	/* not usable in ramstage as GEN_PMCON1 gets reset */
+	if (ENV_RAMSTAGE)
+		dead_code();
+
+	gen_pmcon1 = read32((u32 *)(PMC_BASE_ADDRESS + GEN_PMCON1));

rtc_fail = !!(gen_pmcon1 & RPS);
-
    if (rtc_fail)
    	printk(BIOS_DEBUG, "RTC failure.\n");