Change in ...coreboot[master]: lib/bootblock: Sanitize CMOS after bootblock_*_early_init()

[Thomas Heijligen (Code Review)]

Hello Nico Huber,

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

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

to review the following change.

Change subject: lib/bootblock: Sanitize CMOS after bootblock_*_early_init()
......................................................................

lib/bootblock: Sanitize CMOS after bootblock_*_early_init()

CMOS isn't used that early, but the chipset initialization may be
required to access it.

In one instance, Intel Apollo Lake, the sanitize_cmos() function
seems to hang if called before bootblock_soc_early_init(). The
missing step is fast_spi_early_init(). But even without, one might
expect sanitize_cmos() to return eventually (it didn't within
about 20min).

Change-Id: I6e1a029e4be7e109be43a3dad944bd7e05ea1f02
Signed-off-by: Nico Huber 
---
M src/lib/bootblock.c
1 file changed, 3 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/31349/1

diff --git a/src/lib/bootblock.c b/src/lib/bootblock.c
index f2ada52..6b9ecbf 100644
--- a/src/lib/bootblock.c
+++ b/src/lib/bootblock.c
@@ -41,12 +41,12 @@
    			      timestamps[i].entry_stamp);
    }

-	sanitize_cmos();
-	cmos_post_init();
-
    bootblock_soc_early_init();
    bootblock_mainboard_early_init();

+	sanitize_cmos();
+	cmos_post_init();
+
    if (IS_ENABLED(CONFIG_BOOTBLOCK_CONSOLE)) {
    	console_init();
    	exception_init();

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31349 )

Change subject: lib/bootblock: Sanitize CMOS after bootblock_*_early_init()
......................................................................

Patch Set 1: Code-Review+1

[Thomas Heijligen (Code Review)]

Hello Werner Zeh, Aaron Durbin, Julius Werner, Paul Menzel, build bot (Jenkins), Nico Huber,

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

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

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

Change subject: lib/bootblock: Sanitize CMOS after bootblock_*_early_init()
......................................................................

lib/bootblock: Sanitize CMOS after bootblock_*_early_init()

CMOS isn't used that early, but the chipset initialization may be
required to access it.

In one instance, Intel Apollo Lake, the sanitize_cmos() function
seems to hang if called before bootblock_soc_early_init(). The
missing step is fast_spi_early_init(). But even without, one might
expect sanitize_cmos() to return eventually (it didn't within
about 20min).

Change-Id: I6e1a029e4be7e109be43a3dad944bd7e05ea1f02
Signed-off-by: Nico Huber 
---
M src/lib/bootblock.c
1 file changed, 3 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/31349/3

[Nico Huber (Code Review)]

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

Change subject: lib/bootblock: Sanitize CMOS after bootblock_*_early_init()
......................................................................

Patch Set 3:

I only have one APL system to test with, but it might
just be that CONFIG_OPTION_TABLE is generally broken
on APL.

I'm not 100% confident about changing the order, but
then again it seems we don't need to do it that early.

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

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

Change subject: lib/bootblock: Sanitize CMOS after bootblock_*_early_init()
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/31349/3/src/lib/bootblock.c 
File src/lib/bootblock.c:

https://review.coreboot.org/#/c/31349/3/src/lib/bootblock.c@a45 
PS3, Line 45: 
The call to enable_rtc_upper_bank() were like just the previous executed instructions if running without timestamps? Could that RTC hardware require little delay after its decoding range was extended?

[Nico Huber (Code Review)]

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

Change subject: lib/bootblock: Sanitize CMOS after bootblock_*_early_init()
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/31349/3/src/lib/bootblock.c 
File src/lib/bootblock.c:

https://review.coreboot.org/#/c/31349/3/src/lib/bootblock.c@a45 
PS3, Line 45:
> The call to enable_rtc_upper_bank() were like just the previous executed instructions if running wit […]

Interesting thought, I'll have to check. Altough it's unlikely that I had
timestamps disabled, it's not a huge delay anyway.

[Felix Singer (Code Review)]

Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31349 )

Change subject: lib/bootblock: Sanitize CMOS after bootblock_*_early_init()
......................................................................

Patch Set 3:

On UPsquared board CMOS works with and without this patch. Can I help debugging?

[Alex Thiessen (Code Review)]

Alex Thiessen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31349 )

Change subject: lib/bootblock: Sanitize CMOS after bootblock_*_early_init()
......................................................................

Patch Set 3: Code-Review+2

With 4.8.1 and 4.9, we run into this issue on APL when _not_ using memory mapping to access the SPI flash _and_ using a CMOS table (with RTC set to the `cmos.default` on every boot).

This patch fixes the issue for us (in the bootblock, at least).

I might be wrong, but in our case `sanitize_cmos()` needs the Fast SPI PCI device to be initialized for accessing `cmos.default` in CBFS. The device isn't initialized until `bootblock_soc_early_init()` is run. Thus, there is an implicit dependency between the two calls.

So, bring it on after maybe adding a code comment / commit message on why this order matters.

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31349 )

Change subject: lib/bootblock: Sanitize CMOS after bootblock_*_early_init()
......................................................................

Patch Set 3:
> Patch Set 3: Code-Review+2
>
> With 4.8.1 and 4.9, we run into this issue on APL when _not_ using memory mapping to access the SPI flash _and_ using a CMOS table (with RTC set to the `cmos.default` on every boot).
>
> This patch fixes the issue for us (in the bootblock, at least).
>
> I might be wrong, but in our case `sanitize_cmos()` needs the Fast SPI PCI device to be initialized for accessing `cmos.default` in CBFS. The device isn't initialized until `bootblock_soc_early_init()` is run. Thus, there is an implicit dependency between the two calls.
>
> So, bring it on after maybe adding a code comment / commit message on why this order matters.

I think the explanation makes sense. It could probably be hitting this assert https://review.coreboot.org/cgit/coreboot.git/tree/src/soc/intel/common/bloc... when accessing "cmos.default". Or maybe end up accessing the wrong registers from MMIO space for the fast SPI. And, if this is true, it should actually be seen on more than just APL because other Intel SoCs using the common fast_spi block seem to have very similar dependency.

[Patrick Georgi (Code Review)]

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

Change subject: lib/bootblock: Sanitize CMOS after bootblock_*_early_init()
......................................................................

lib/bootblock: Sanitize CMOS after bootblock_*_early_init()

CMOS isn't used that early, but the chipset initialization may be
required to access it.

In one instance, Intel Apollo Lake, the sanitize_cmos() function
seems to hang if called before bootblock_soc_early_init(). The
missing step is fast_spi_early_init(). But even without, one might
expect sanitize_cmos() to return eventually (it didn't within
about 20min).

Change-Id: I6e1a029e4be7e109be43a3dad944bd7e05ea1f02
Signed-off-by: Nico Huber 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/31349
Tested-by: build bot (Jenkins) 
Reviewed-by: Alex Thiessen 
---
M src/lib/bootblock.c
1 file changed, 3 insertions(+), 3 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Alex Thiessen: Looks good to me, approved

diff --git a/src/lib/bootblock.c b/src/lib/bootblock.c
index 43674ef..c28a67a 100644
--- a/src/lib/bootblock.c
+++ b/src/lib/bootblock.c
@@ -42,12 +42,12 @@
    			      timestamps[i].entry_stamp);
    }

-	sanitize_cmos();
-	cmos_post_init();
-
    bootblock_soc_early_init();
    bootblock_mainboard_early_init();

+	sanitize_cmos();
+	cmos_post_init();
+
    if (CONFIG(BOOTBLOCK_CONSOLE)) {
    	console_init();
    	exception_init();