Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38189 )
Change subject: drivers/elog,pc80: Move cmos_post_log() ......................................................................
drivers/elog,pc80: Move cmos_post_log()
Change-Id: I98044a28c29a2b1756fb25fb593f505e914a71c0 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/drivers/elog/elog.c M src/drivers/pc80/rtc/post.c M src/include/console/console.h M src/include/pc80/mc146818rtc.h 4 files changed, 36 insertions(+), 25 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/38189/1
diff --git a/src/drivers/elog/elog.c b/src/drivers/elog/elog.c index 5f11c0c..9bd2a61 100644 --- a/src/drivers/elog/elog.c +++ b/src/drivers/elog/elog.c @@ -758,16 +758,37 @@ #endif }
+/* Check and log POST codes from previous boot */ +static void log_last_boot_post(void) +{ +#if CONFIG(ARCH_X86) + u8 code; + u32 extra; + + if (!CONFIG(CMOS_POST)) + return; + + if (cmos_post_previous(&code, &extra) == 0) + return; + + printk(BIOS_WARNING, "POST: Unexpected post code/extra " + "in previous boot: 0x%02x/0x%04x\n", code, extra); + + if (ENV_RAMSTAGE || CONFIG(ELOG_PRERAM)) { + elog_add_event_word(ELOG_TYPE_LAST_POST_CODE, code); + /* Always zero with !CMOS_POST_EXTRA. */ + if (extra) + elog_add_event_dword(ELOG_TYPE_POST_EXTRA, extra); + } +#endif +} + static void elog_add_boot_count(void) { if (elog_do_add_boot_count()) { elog_add_event_dword(ELOG_TYPE_BOOT, boot_count_read());
-#if CONFIG(ARCH_X86) - /* Check and log POST codes from previous boot */ - if (CONFIG(CMOS_POST)) - cmos_post_log(); -#endif + log_last_boot_post(); } }
diff --git a/src/drivers/pc80/rtc/post.c b/src/drivers/pc80/rtc/post.c index 0d5d0e1..2893a41 100644 --- a/src/drivers/pc80/rtc/post.c +++ b/src/drivers/pc80/rtc/post.c @@ -13,7 +13,6 @@ */
#include <stdint.h> -#include <elog.h> #include <console/console.h> #include <device/device.h> #include <pc80/mc146818rtc.h> @@ -21,45 +20,38 @@
DECLARE_SPIN_LOCK(cmos_post_lock)
-void cmos_post_log(void) +int cmos_post_previous(u8 *code, u32 *extra) { - u8 code = 0; - u32 extra = 0; - spin_lock(&cmos_post_lock);
/* Get post code from other bank */ switch (cmos_read(CMOS_POST_BANK_OFFSET)) { case CMOS_POST_BANK_0_MAGIC: - code = cmos_read(CMOS_POST_BANK_1_OFFSET); + *code = cmos_read(CMOS_POST_BANK_1_OFFSET); if (CONFIG(CMOS_POST_EXTRA)) - extra = cmos_read32(CMOS_POST_BANK_1_EXTRA); + *extra = cmos_read32(CMOS_POST_BANK_1_EXTRA); break; case CMOS_POST_BANK_1_MAGIC: - code = cmos_read(CMOS_POST_BANK_0_OFFSET); + *code = cmos_read(CMOS_POST_BANK_0_OFFSET); if (CONFIG(CMOS_POST_EXTRA)) - extra = cmos_read32(CMOS_POST_BANK_0_EXTRA); + *extra = cmos_read32(CMOS_POST_BANK_0_EXTRA); break; }
spin_unlock(&cmos_post_lock);
/* Check last post code in previous boot against normal list */ - switch (code) { + switch (*code) { case POST_OS_BOOT: case POST_OS_RESUME: case POST_ENTER_ELF_BOOT: case 0: break; default: - printk(BIOS_WARNING, "POST: Unexpected post code " - "in previous boot: 0x%02x\n", code); -#if CONFIG(ELOG) && (ENV_RAMSTAGE || CONFIG(ELOG_PRERAM)) - elog_add_event_word(ELOG_TYPE_LAST_POST_CODE, code); - if (CONFIG(CMOS_POST_EXTRA) && extra) - elog_add_event_dword(ELOG_TYPE_POST_EXTRA, extra); -#endif + return -1; } + + return 0; }
void cmos_post_init(void) diff --git a/src/include/console/console.h b/src/include/console/console.h index 9983cef..9e6c76e 100644 --- a/src/include/console/console.h +++ b/src/include/console/console.h @@ -31,6 +31,7 @@ void post_code(u8 value); void arch_post_code(u8 value); void cmos_post_code(u8 value); +int cmos_post_previous(u8 *code, u32 *extra); #if CONFIG(CMOS_POST_EXTRA) struct device; void post_log_path(const struct device *dev); diff --git a/src/include/pc80/mc146818rtc.h b/src/include/pc80/mc146818rtc.h index b064461..94d5ef5 100644 --- a/src/include/pc80/mc146818rtc.h +++ b/src/include/pc80/mc146818rtc.h @@ -209,9 +209,6 @@
#define CMOS_POST_EXTRA_DEV_PATH 0x01
-void cmos_post_log(void); -#else -static inline void cmos_post_log(void) {} #endif /* CONFIG_CMOS_POST */
#endif /* CONFIG_ARCH_X86 */
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38189 )
Change subject: drivers/elog,pc80: Move cmos_post_log() ......................................................................
Patch Set 4: Code-Review+1
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38189 )
Change subject: drivers/elog,pc80: Move cmos_post_log() ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38189/5/src/drivers/elog/elog.c File src/drivers/elog/elog.c:
https://review.coreboot.org/c/coreboot/+/38189/5/src/drivers/elog/elog.c@779 PS5, Line 779: /* Always zero with !CMOS_POST_EXTRA. */ Is this comment correct with "extra" uninitialized at the top and not assigned anything in cmos_post_previous with !CONFIG_POST_EXTRA?
https://review.coreboot.org/c/coreboot/+/38189/5/src/drivers/pc80/rtc/post.c File src/drivers/pc80/rtc/post.c:
https://review.coreboot.org/c/coreboot/+/38189/5/src/drivers/pc80/rtc/post.c... PS5, Line 23: cmos_post_previous nit: cmos_post_previous_boot?
Hello Karthikeyan Ramasubramanian, Angel Pons, build bot (Jenkins), Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38189
to look at the new patch set (#6).
Change subject: drivers/elog,pc80: Move cmos_post_log() ......................................................................
drivers/elog,pc80: Move cmos_post_log()
Change-Id: I98044a28c29a2b1756fb25fb593f505e914a71c0 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/drivers/elog/elog.c M src/drivers/pc80/rtc/post.c M src/include/console/console.h M src/include/pc80/mc146818rtc.h 4 files changed, 38 insertions(+), 24 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/38189/6
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38189 )
Change subject: drivers/elog,pc80: Move cmos_post_log() ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38189/5/src/drivers/elog/elog.c File src/drivers/elog/elog.c:
https://review.coreboot.org/c/coreboot/+/38189/5/src/drivers/elog/elog.c@779 PS5, Line 779: /* Always zero with !CMOS_POST_EXTRA. */
Is this comment correct with "extra" uninitialized at the top and not assigned anything in cmos_post […]
Done
https://review.coreboot.org/c/coreboot/+/38189/5/src/drivers/pc80/rtc/post.c File src/drivers/pc80/rtc/post.c:
https://review.coreboot.org/c/coreboot/+/38189/5/src/drivers/pc80/rtc/post.c... PS5, Line 23: cmos_post_previous
nit: cmos_post_previous_boot?
Done
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38189 )
Change subject: drivers/elog,pc80: Move cmos_post_log() ......................................................................
Patch Set 6: Code-Review+2
(1 comment)
A commit message would have been nice, but the intention seems clear.
https://review.coreboot.org/c/coreboot/+/38189/6/src/drivers/elog/elog.c File src/drivers/elog/elog.c:
https://review.coreboot.org/c/coreboot/+/38189/6/src/drivers/elog/elog.c@777 PS6, Line 777: if (ENV_RAMSTAGE || CONFIG(ELOG_PRERAM)) { This seems odd, elog_add_boot_count() shouldn't be called otherwise, afaics. It also seems to have fallen out of sync with the introduction of ENV_PAYLOAD_LOADER. Though, unrelated to this change because it just retains current checks.
Hello build bot (Jenkins), Nico Huber, Furquan Shaikh, Patrick Georgi, Angel Pons, Karthikeyan Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38189
to look at the new patch set (#7).
Change subject: drivers/elog,pc80: Move cmos_post_log() ......................................................................
drivers/elog,pc80: Move cmos_post_log()
Do this to remove elog header dependency from pc80/ and remove some preprocessor guards.
Change-Id: I98044a28c29a2b1756fb25fb593f505e914a71c0 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/drivers/elog/elog.c M src/drivers/pc80/rtc/post.c M src/include/console/console.h M src/include/pc80/mc146818rtc.h 4 files changed, 38 insertions(+), 24 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/38189/7
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38189 )
Change subject: drivers/elog,pc80: Move cmos_post_log() ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38189/6/src/drivers/elog/elog.c File src/drivers/elog/elog.c:
https://review.coreboot.org/c/coreboot/+/38189/6/src/drivers/elog/elog.c@777 PS6, Line 777: if (ENV_RAMSTAGE || CONFIG(ELOG_PRERAM)) {
This seems odd, elog_add_boot_count() shouldn't be called otherwise, […]
I agree. You say we can remove the conditionals with follow-up?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38189 )
Change subject: drivers/elog,pc80: Move cmos_post_log() ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38189/6/src/drivers/elog/elog.c File src/drivers/elog/elog.c:
https://review.coreboot.org/c/coreboot/+/38189/6/src/drivers/elog/elog.c@777 PS6, Line 777: if (ENV_RAMSTAGE || CONFIG(ELOG_PRERAM)) {
I agree. […]
Yes, or just do it here if you want. I looked a little closer now, and it turns out that this file is only compiled if the condition is true. So the original intent of the guard was to avoid linking errors.
Hello build bot (Jenkins), Nico Huber, Furquan Shaikh, Patrick Georgi, Angel Pons, Karthikeyan Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38189
to look at the new patch set (#9).
Change subject: drivers/elog,pc80: Move cmos_post_log() ......................................................................
drivers/elog,pc80: Move cmos_post_log()
Do this to remove elog header dependency from pc80/ and remove some preprocessor guards.
Change-Id: I98044a28c29a2b1756fb25fb593f505e914a71c0 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/drivers/elog/elog.c M src/drivers/pc80/rtc/post.c M src/include/console/console.h M src/include/pc80/mc146818rtc.h 4 files changed, 36 insertions(+), 24 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/38189/9
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38189 )
Change subject: drivers/elog,pc80: Move cmos_post_log() ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38189/6/src/drivers/elog/elog.c File src/drivers/elog/elog.c:
https://review.coreboot.org/c/coreboot/+/38189/6/src/drivers/elog/elog.c@777 PS6, Line 777: if (ENV_RAMSTAGE || CONFIG(ELOG_PRERAM)) {
Yes, or just do it here if you want. I looked a little closer now, and […]
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38189 )
Change subject: drivers/elog,pc80: Move cmos_post_log() ......................................................................
Patch Set 9: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38189 )
Change subject: drivers/elog,pc80: Move cmos_post_log() ......................................................................
drivers/elog,pc80: Move cmos_post_log()
Do this to remove elog header dependency from pc80/ and remove some preprocessor guards.
Change-Id: I98044a28c29a2b1756fb25fb593f505e914a71c0 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/38189 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com --- M src/drivers/elog/elog.c M src/drivers/pc80/rtc/post.c M src/include/console/console.h M src/include/pc80/mc146818rtc.h 4 files changed, 36 insertions(+), 24 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved
diff --git a/src/drivers/elog/elog.c b/src/drivers/elog/elog.c index c353d59..ca604dd 100644 --- a/src/drivers/elog/elog.c +++ b/src/drivers/elog/elog.c @@ -748,16 +748,35 @@ #endif }
+/* Check and log POST codes from previous boot */ +static void log_last_boot_post(void) +{ +#if CONFIG(ARCH_X86) + u8 code; + u32 extra; + + if (!CONFIG(CMOS_POST)) + return; + + if (cmos_post_previous_boot(&code, &extra) == 0) + return; + + printk(BIOS_WARNING, "POST: Unexpected post code/extra " + "in previous boot: 0x%02x/0x%04x\n", code, extra); + + elog_add_event_word(ELOG_TYPE_LAST_POST_CODE, code); + /* Always zero with !CMOS_POST_EXTRA. */ + if (extra) + elog_add_event_dword(ELOG_TYPE_POST_EXTRA, extra); +#endif +} + static void elog_add_boot_count(void) { if (elog_do_add_boot_count()) { elog_add_event_dword(ELOG_TYPE_BOOT, boot_count_read());
-#if CONFIG(ARCH_X86) - /* Check and log POST codes from previous boot */ - if (CONFIG(CMOS_POST)) - cmos_post_log(); -#endif + log_last_boot_post(); } }
diff --git a/src/drivers/pc80/rtc/post.c b/src/drivers/pc80/rtc/post.c index 1ac3630..e143674 100644 --- a/src/drivers/pc80/rtc/post.c +++ b/src/drivers/pc80/rtc/post.c @@ -2,7 +2,6 @@ /* This file is part of the coreboot project. */
#include <stdint.h> -#include <elog.h> #include <console/console.h> #include <device/device.h> #include <pc80/mc146818rtc.h> @@ -10,45 +9,41 @@
DECLARE_SPIN_LOCK(cmos_post_lock)
-void cmos_post_log(void) +int cmos_post_previous_boot(u8 *code, u32 *extra) { - u8 code = 0; - u32 extra = 0; + *code = 0; + *extra = 0;
spin_lock(&cmos_post_lock);
/* Get post code from other bank */ switch (cmos_read(CMOS_POST_BANK_OFFSET)) { case CMOS_POST_BANK_0_MAGIC: - code = cmos_read(CMOS_POST_BANK_1_OFFSET); + *code = cmos_read(CMOS_POST_BANK_1_OFFSET); if (CONFIG(CMOS_POST_EXTRA)) - extra = cmos_read32(CMOS_POST_BANK_1_EXTRA); + *extra = cmos_read32(CMOS_POST_BANK_1_EXTRA); break; case CMOS_POST_BANK_1_MAGIC: - code = cmos_read(CMOS_POST_BANK_0_OFFSET); + *code = cmos_read(CMOS_POST_BANK_0_OFFSET); if (CONFIG(CMOS_POST_EXTRA)) - extra = cmos_read32(CMOS_POST_BANK_0_EXTRA); + *extra = cmos_read32(CMOS_POST_BANK_0_EXTRA); break; }
spin_unlock(&cmos_post_lock);
/* Check last post code in previous boot against normal list */ - switch (code) { + switch (*code) { case POST_OS_BOOT: case POST_OS_RESUME: case POST_ENTER_ELF_BOOT: case 0: break; default: - printk(BIOS_WARNING, "POST: Unexpected post code " - "in previous boot: 0x%02x\n", code); -#if CONFIG(ELOG) && (ENV_RAMSTAGE || CONFIG(ELOG_PRERAM)) - elog_add_event_word(ELOG_TYPE_LAST_POST_CODE, code); - if (CONFIG(CMOS_POST_EXTRA) && extra) - elog_add_event_dword(ELOG_TYPE_POST_EXTRA, extra); -#endif + return -1; } + + return 0; }
void cmos_post_init(void) diff --git a/src/include/console/console.h b/src/include/console/console.h index c68caf0..f9e9fe2 100644 --- a/src/include/console/console.h +++ b/src/include/console/console.h @@ -19,6 +19,7 @@ void post_code(u8 value); void arch_post_code(u8 value); void cmos_post_code(u8 value); +int cmos_post_previous_boot(u8 *code, u32 *extra); #if CONFIG(CMOS_POST_EXTRA) struct device; void post_log_path(const struct device *dev); diff --git a/src/include/pc80/mc146818rtc.h b/src/include/pc80/mc146818rtc.h index 20e9639..edaeb1e 100644 --- a/src/include/pc80/mc146818rtc.h +++ b/src/include/pc80/mc146818rtc.h @@ -210,9 +210,6 @@
#define CMOS_POST_EXTRA_DEV_PATH 0x01
-void cmos_post_log(void); -#else -static inline void cmos_post_log(void) {} #endif /* CONFIG_CMOS_POST */
#endif /* CONFIG_ARCH_X86 */
-
Angel Pons (Code Review) -
Karthik Ramasubramanian (Code Review)
-
Kyösti Mälkki (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)