Change in coreboot[master]: console/post: Split parts to arch/

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

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

Change subject: console/post: Split parts to arch/
......................................................................

console/post: Split parts to arch/

Both IO port and cmos are currently arch/x86 only features.

Change-Id: I010af3f645c0be38dd856657874c36103aebbdc2
Signed-off-by: Kyösti Mälkki 
---
M src/arch/x86/Makefile.inc
A src/arch/x86/post.c
M src/console/post.c
M src/include/console/console.h
4 files changed, 37 insertions(+), 14 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/38187/1

diff --git a/src/arch/x86/Makefile.inc b/src/arch/x86/Makefile.inc
index 6ed93e5..534f2ce 100644
--- a/src/arch/x86/Makefile.inc
+++ b/src/arch/x86/Makefile.inc
@@ -99,6 +99,7 @@
 all-y += memcpy.c
 all-y += memset.c
 all-y += cpu_common.c
+all-y += post.c

endif

diff --git a/src/arch/x86/post.c b/src/arch/x86/post.c
new file mode 100644
index 0000000..0a20bab
--- /dev/null
+++ b/src/arch/x86/post.c
@@ -0,0 +1,26 @@
+/*
+ * This file is part of the coreboot project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; version 2 of
+ * the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <stdint.h>
+#include <console/console.h>
+#include <arch/io.h>
+
+void arch_post_code(uint8_t value)
+{
+	if (CONFIG(POST_IO))
+		outb(value, CONFIG_POST_IO_PORT);
+
+	if (CONFIG(CMOS_POST))
+		cmos_post_code(value);
+}
diff --git a/src/console/post.c b/src/console/post.c
index b9a7557..33d85e7 100644
--- a/src/console/post.c
+++ b/src/console/post.c
@@ -14,11 +14,9 @@

#include <stdint.h>
 #include <console/console.h>
-#if CONFIG(POST_IO)
-#include <arch/io.h>
-#endif

/* Write POST information */
+void __weak arch_post_code(uint8_t value) { }

/* Some mainboards have very nice features beyond just a simple display.
  * They can override this function.
@@ -27,16 +25,13 @@

void post_code(uint8_t value)
 {
-#if !CONFIG(NO_POST)
-#if CONFIG(CONSOLE_POST)
-	printk(BIOS_EMERG, "POST: 0x%02x\n", value);
-#endif
-#if CONFIG(CMOS_POST)
-	cmos_post_code(value);
-#endif
-#if CONFIG(POST_IO)
-	outb(value, CONFIG_POST_IO_PORT);
-#endif
-#endif
+	if (!CONFIG(NO_POST)) {
+		/* Assume this to be the most reliable and simplest type
+		   for displaying POST so keep it first. */
+		arch_post_code(value);
+
+		if (CONFIG(CONSOLE_POST))
+			printk(BIOS_EMERG, "POST: 0x%02x\n", value);
+	}
    mainboard_post(value);
 }
diff --git a/src/include/console/console.h b/src/include/console/console.h
index d3c1d54..c573123 100644
--- a/src/include/console/console.h
+++ b/src/include/console/console.h
@@ -29,6 +29,7 @@
 #include <console/vtxprintf.h>

void post_code(u8 value);
+void arch_post_code(u8 value);
 void cmos_post_code(u8 value);
 #if CONFIG(CMOS_POST_EXTRA)
 void post_log_extra(u32 value);

[Angel Pons (Code Review)]

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

Change subject: console/post: Split parts to arch/
......................................................................

Patch Set 4: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/38187/4/src/console/post.c 
File src/console/post.c:

https://review.coreboot.org/c/coreboot/+/38187/4/src/console/post.c@36 
PS4, Line 36: mainboard_post(value);
I wonder: shouldn't this be inside the if(!CONFIG(NO_POST)) ?

[Patrick Georgi (Code Review)]

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

Change subject: console/post: Split parts to arch/
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38187/4/src/console/post.c 
File src/console/post.c:

https://review.coreboot.org/c/coreboot/+/38187/4/src/console/post.c@36 
PS4, Line 36: mainboard_post(value);
> I wonder: shouldn't this be inside the if(!CONFIG(NO_POST)) ?

It's used on two boards (see src/mainboard/google/drallion/ec.c and src/mainboard/google/sarien/ec.c) which send the post code to the EC. I guess "no post" is supposed to mean "no post whatsoever" in which case you're right. I'd do it as a separate commit though since that's a change in semantics: https://review.coreboot.org/c/coreboot/+/38408

[Patrick Georgi (Code Review)]

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

Change subject: console/post: Split parts to arch/
......................................................................

console/post: Split parts to arch/

Both IO port and cmos are currently arch/x86 only features.

Change-Id: I010af3f645c0be38dd856657874c36103aebbdc2
Signed-off-by: Kyösti Mälkki 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/38187
Reviewed-by: Angel Pons 
Tested-by: build bot (Jenkins) 
---
M src/arch/x86/Makefile.inc
A src/arch/x86/post.c
M src/console/post.c
M src/include/console/console.h
4 files changed, 37 insertions(+), 14 deletions(-)

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

diff --git a/src/arch/x86/Makefile.inc b/src/arch/x86/Makefile.inc
index 6ed93e5..534f2ce 100644
--- a/src/arch/x86/Makefile.inc
+++ b/src/arch/x86/Makefile.inc
@@ -99,6 +99,7 @@
 all-y += memcpy.c
 all-y += memset.c
 all-y += cpu_common.c
+all-y += post.c

endif

diff --git a/src/arch/x86/post.c b/src/arch/x86/post.c
new file mode 100644
index 0000000..0a20bab
--- /dev/null
+++ b/src/arch/x86/post.c
@@ -0,0 +1,26 @@
+/*
+ * This file is part of the coreboot project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; version 2 of
+ * the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <stdint.h>
+#include <console/console.h>
+#include <arch/io.h>
+
+void arch_post_code(uint8_t value)
+{
+	if (CONFIG(POST_IO))
+		outb(value, CONFIG_POST_IO_PORT);
+
+	if (CONFIG(CMOS_POST))
+		cmos_post_code(value);
+}
diff --git a/src/console/post.c b/src/console/post.c
index b9a7557..33d85e7 100644
--- a/src/console/post.c
+++ b/src/console/post.c
@@ -14,11 +14,9 @@

#include <stdint.h>
 #include <console/console.h>
-#if CONFIG(POST_IO)
-#include <arch/io.h>
-#endif

/* Write POST information */
+void __weak arch_post_code(uint8_t value) { }

/* Some mainboards have very nice features beyond just a simple display.
  * They can override this function.
@@ -27,16 +25,13 @@

void post_code(uint8_t value)
 {
-#if !CONFIG(NO_POST)
-#if CONFIG(CONSOLE_POST)
-	printk(BIOS_EMERG, "POST: 0x%02x\n", value);
-#endif
-#if CONFIG(CMOS_POST)
-	cmos_post_code(value);
-#endif
-#if CONFIG(POST_IO)
-	outb(value, CONFIG_POST_IO_PORT);
-#endif
-#endif
+	if (!CONFIG(NO_POST)) {
+		/* Assume this to be the most reliable and simplest type
+		   for displaying POST so keep it first. */
+		arch_post_code(value);
+
+		if (CONFIG(CONSOLE_POST))
+			printk(BIOS_EMERG, "POST: 0x%02x\n", value);
+	}
    mainboard_post(value);
 }
diff --git a/src/include/console/console.h b/src/include/console/console.h
index d3c1d54..c573123 100644
--- a/src/include/console/console.h
+++ b/src/include/console/console.h
@@ -29,6 +29,7 @@
 #include <console/vtxprintf.h>

void post_code(u8 value);
+void arch_post_code(u8 value);
 void cmos_post_code(u8 value);
 #if CONFIG(CMOS_POST_EXTRA)
 void post_log_extra(u32 value);