Change in coreboot[master]: intel/nehalem: Refactor ACPI S3 detection

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35372 )

Change subject: intel/nehalem: Refactor ACPI S3 detection
......................................................................

intel/nehalem: Refactor ACPI S3 detection

Change-Id: Ib405f3c3a6143e972963307eef7371dd43b9b5fc
Signed-off-by: Kyösti Mälkki 
---
M src/northbridge/intel/nehalem/early_init.c
1 file changed, 8 insertions(+), 16 deletions(-)

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

diff --git a/src/northbridge/intel/nehalem/early_init.c b/src/northbridge/intel/nehalem/early_init.c
index a5cac7b..1f413e3 100644
--- a/src/northbridge/intel/nehalem/early_init.c
+++ b/src/northbridge/intel/nehalem/early_init.c
@@ -73,22 +73,7 @@
    pci_write_config8(PCI_DEV(0xff, 0x00, 1), QPD0F1_PAM(5), 0x33);
    pci_write_config8(PCI_DEV(0xff, 0x00, 1), QPD0F1_PAM(6), 0x33);

-#if CONFIG(ELOG_BOOT_COUNT)
-	/* Increment Boot Counter for non-S3 resume */
-	if ((inw(DEFAULT_PMBASE + PM1_STS) & WAK_STS) &&
-	    ((inl(DEFAULT_PMBASE + PM1_CNT) >> 10) & 7) != SLP_TYP_S3)
-		boot_count_increment();
-#endif
-
    printk(BIOS_DEBUG, " done.\n");
-
-#if CONFIG(ELOG_BOOT_COUNT)
-	/* Increment Boot Counter except when resuming from S3 */
-	if ((inw(DEFAULT_PMBASE + PM1_STS) & WAK_STS) &&
-	    ((inl(DEFAULT_PMBASE + PM1_CNT) >> 10) & 7) == SLP_TYP_S3)
-		return;
-	boot_count_increment();
-#endif
 }

static void early_cpu_init (void)
@@ -134,6 +119,7 @@
 {
    u32 capid0_a;
    u8 reg8;
+	int s3_resume;

/* Device ID Override Enable should be done very early */
    capid0_a = pci_read_config32(PCI_DEV(0, 0, 0), 0xe4);
@@ -150,6 +136,12 @@
    /* Setup all BARs required for early PCIe and raminit */
    nehalem_setup_bars();

+	s3_resume = (inw(DEFAULT_PMBASE + PM1_STS) & WAK_STS) &&
+	    (((inl(DEFAULT_PMBASE + PM1_CNT) >> 10) & 7) == SLP_TYP_S3);
+
+	if (CONFIG(ELOG_BOOT_COUNT) && !s3_resume)
+		boot_count_increment();
+
    /* Device Enable */
    pci_write_config32(PCI_DEV(0, 0, 0), D0F0_DEVEN,
    		   DEVEN_IGD | DEVEN_PEG10 | DEVEN_HOST);
@@ -161,7 +153,7 @@
    		   PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY);

/* Magic for S3 resume. Must be done early.  */
-	if (((inl(DEFAULT_PMBASE + PM1_CNT) >> 10) & 7) == SLP_TYP_S3) {
+	if (s3_resume) {
    	MCHBAR32 (0x1e8) = (MCHBAR32(0x1e8) & ~1) | 6;
    	MCHBAR32 (0x1e8) = (MCHBAR32(0x1e8) & ~3) | 4;
    }

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35372 )

Change subject: intel/nehalem: Refactor ACPI S3 detection
......................................................................

Patch Set 1: Code-Review+2

Does nehalem use the elog?

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35372 )

Change subject: intel/nehalem: Refactor ACPI S3 detection
......................................................................

Patch Set 1:
> Patch Set 1: Code-Review+2
>
> Does nehalem use the elog?

Probably just copy-paste from early sandy/ivy.

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35372 )

Change subject: intel/nehalem: Refactor ACPI S3 detection
......................................................................

Patch Set 1:

there are no boards that use elog on nehalem. I'd recommend to remove the whole elog code on this platform instead.

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35372 )

Change subject: intel/nehalem: Refactor ACPI S3 detection
......................................................................

Patch Set 1:
> Patch Set 1:
>
> there are no boards that use elog on nehalem. I'd recommend to remove the whole elog code on this platform instead.

ELOG depends on BOOT_DEVICE_SUPPORTS_WRITES, so everything ELOG -related under southbridge/intel and northbridge/intel never builds. Looks like a regression from CB:16204?

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35372 )

Change subject: intel/nehalem: Refactor ACPI S3 detection
......................................................................

Patch Set 1: Code-Review+2

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35372 )

Change subject: intel/nehalem: Refactor ACPI S3 detection
......................................................................

Patch Set 1:
> Patch Set 1:
> > Patch Set 1:
> >
> > there are no boards that use elog on nehalem. I'd recommend to remove the whole elog code on this platform instead.
>
> ELOG depends on BOOT_DEVICE_SUPPORTS_WRITES, so everything ELOG -related under southbridge/intel and northbridge/intel never builds. Looks like a regression from CB:16204?

Their spi flash controller needs to work early on in the bootflow (romstage). In the past nearly all intel spi flash controller implementations only worked in ramstage. But if that CL prevented ramstage elog as well it could certainly be seen as a regression. However, I'm not sure how many mainboards were using elog that leveraged those southbridge/intel and northbridge/intel pieces of code. lynxpoint and something else may have, but I'm not sure the order of operations of refactoring the code that may have busted stuff.

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35372 )

Change subject: intel/nehalem: Refactor ACPI S3 detection
......................................................................

Patch Set 1:
> Patch Set 1:
> > Patch Set 1:
> > > Patch Set 1:
> > >
> > > there are no boards that use elog on nehalem. I'd recommend to remove the whole elog code on this platform instead.
> >
> > ELOG depends on BOOT_DEVICE_SUPPORTS_WRITES, so everything ELOG -related under southbridge/intel and northbridge/intel never builds. Looks like a regression from CB:16204?
>
> Their spi flash controller needs to work early on in the bootflow (romstage). In the past nearly all intel spi flash controller implementations only worked in ramstage. But if that CL prevented ramstage elog as well it could certainly be seen as a regression. However, I'm not sure how many mainboards were using elog that leveraged those southbridge/intel and northbridge/intel pieces of code. lynxpoint and something else may have, but I'm not sure the order of operations of refactoring the code that may have busted stuff.

I think ELOG was always selected with CHROMEOS, but I did not check deeper into this.

Possibly affected:
samsung/lumpy,stumpy (sandy / bd82x6x)
google/butterfly,link,parrot,stout (ivy / c216)
google/beltino,slippy  (haswell / lynxpoint)

[Patrick Georgi (Code Review)]

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

Change subject: intel/nehalem: Refactor ACPI S3 detection
......................................................................

intel/nehalem: Refactor ACPI S3 detection

Change-Id: Ib405f3c3a6143e972963307eef7371dd43b9b5fc
Signed-off-by: Kyösti Mälkki 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/35372
Tested-by: build bot (Jenkins) 
Reviewed-by: Angel Pons 
Reviewed-by: Aaron Durbin 
---
M src/northbridge/intel/nehalem/early_init.c
1 file changed, 8 insertions(+), 16 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Aaron Durbin: Looks good to me, approved
  Angel Pons: Looks good to me, approved

diff --git a/src/northbridge/intel/nehalem/early_init.c b/src/northbridge/intel/nehalem/early_init.c
index a5cac7b..1f413e3 100644
--- a/src/northbridge/intel/nehalem/early_init.c
+++ b/src/northbridge/intel/nehalem/early_init.c
@@ -73,22 +73,7 @@
    pci_write_config8(PCI_DEV(0xff, 0x00, 1), QPD0F1_PAM(5), 0x33);
    pci_write_config8(PCI_DEV(0xff, 0x00, 1), QPD0F1_PAM(6), 0x33);

-#if CONFIG(ELOG_BOOT_COUNT)
-	/* Increment Boot Counter for non-S3 resume */
-	if ((inw(DEFAULT_PMBASE + PM1_STS) & WAK_STS) &&
-	    ((inl(DEFAULT_PMBASE + PM1_CNT) >> 10) & 7) != SLP_TYP_S3)
-		boot_count_increment();
-#endif
-
    printk(BIOS_DEBUG, " done.\n");
-
-#if CONFIG(ELOG_BOOT_COUNT)
-	/* Increment Boot Counter except when resuming from S3 */
-	if ((inw(DEFAULT_PMBASE + PM1_STS) & WAK_STS) &&
-	    ((inl(DEFAULT_PMBASE + PM1_CNT) >> 10) & 7) == SLP_TYP_S3)
-		return;
-	boot_count_increment();
-#endif
 }

static void early_cpu_init (void)
@@ -134,6 +119,7 @@
 {
    u32 capid0_a;
    u8 reg8;
+	int s3_resume;

/* Device ID Override Enable should be done very early */
    capid0_a = pci_read_config32(PCI_DEV(0, 0, 0), 0xe4);
@@ -150,6 +136,12 @@
    /* Setup all BARs required for early PCIe and raminit */
    nehalem_setup_bars();

+	s3_resume = (inw(DEFAULT_PMBASE + PM1_STS) & WAK_STS) &&
+	    (((inl(DEFAULT_PMBASE + PM1_CNT) >> 10) & 7) == SLP_TYP_S3);
+
+	if (CONFIG(ELOG_BOOT_COUNT) && !s3_resume)
+		boot_count_increment();
+
    /* Device Enable */
    pci_write_config32(PCI_DEV(0, 0, 0), D0F0_DEVEN,
    		   DEVEN_IGD | DEVEN_PEG10 | DEVEN_HOST);
@@ -161,7 +153,7 @@
    		   PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY);

/* Magic for S3 resume. Must be done early.  */
-	if (((inl(DEFAULT_PMBASE + PM1_CNT) >> 10) & 7) == SLP_TYP_S3) {
+	if (s3_resume) {
    	MCHBAR32 (0x1e8) = (MCHBAR32(0x1e8) & ~1) | 6;
    	MCHBAR32 (0x1e8) = (MCHBAR32(0x1e8) & ~3) | 4;
    }