Patchwork Proposal for dealing with superio *.c includes

login
register
about
Submitter Alexandru Gagniuc
Date 2011-02-26 12:17:37
Message ID <4D68EF61.6080309@gmail.com>
Download mbox | patch
Permalink /patch/2707/
State Superseded
Headers show

Comments

Alexandru Gagniuc - 2011-02-26 12:17:37
Hi guys!

I present a proposal for dealing with the annoying:
#include "superio/vendor/model/early_serial.c"
present in virtually all romstage.c files.

The steps are as follows:
1) Declare a generic prototype: superio_enable_early_serial();
2) Remove #include */early_serial.c from romstage
3) add a romstage-$(CONFIG_THIS_SUPERIO) += early_serial.c to the
superio's Makefile.inc
4) Include the generic "superio/early_serial.h" definition in romstage.c
5) Include the superio specific header in romstage.c
6) Done

I'm demonstrating this with the fintek/f81865f superio, and the
amd/persimmon board. Besides discussion, this patch is also intended for
inclusion.

See patch for more details.
Peter Stuge - 2011-02-26 21:16:56
Alex G. wrote:
> Moves the inclusion of the superio early code from romstage.c in 
> the mainboard directory to Makefile.inc in the superio directory.

Will it work also for boards with more than one superio?


//Peter
Alexandru Gagniuc - 2011-02-26 21:27:47
On 02/26/2011 10:50 PM, Patrick Georgi wrote:
> Am Samstag, 26. Februar 2011, 14:17:37 schrieb Alex G.:
>> 3) add a romstage-$(CONFIG_THIS_SUPERIO) += early_serial.c to the
>> superio's Makefile.inc
> This will fail for romcc boards, as for them, romstage must be compiled by a 
> single romcc invocation.
> 
If we break romcc boards in an effort to efficientize the code base, I
am all for it, just so as long as we plead to fix it not much later down
the road.

> I planned to work on this by having the build system generate a wrapper for 
> romcc boards automatically  (essentially a file in $(obj) with romstage.c and 
> other .c files #included, properly hidden from the dev), but that has some 
> issues with the right order in which things must be included (romcc doesn't 
> support forward declarations).
> 
Why bother holding dear to romcc when the obvious solution is to move
those boards to CAR? We introduce more unneeded complexity, and make it
at least just as hard to phase out romcc.

Alex
Alexandru Gagniuc - 2011-02-26 21:40:28
On 02/26/2011 11:16 PM, Peter Stuge wrote:
> Alex G. wrote:
>> Moves the inclusion of the superio early code from romstage.c in 
>> the mainboard directory to Makefile.inc in the superio directory.
> 
> Will it work also for boards with more than one superio?
> 
I'm very tempted (and sure) to say "no". That's why this is largely a
proposal, and you are welcome to extend it. I can only do one superio at
a time, and I definitely cannot do it all by myself.

A:
One other solution I had considered was to put the include in the
board's Makefile.inc, but, as you might have imagined, proved too messy.

B:
A fuzzy extension of this option is to declare
superio_enable_early_serial() as weak, and have the board select which
superio to use for serial output.

C:
Or, besides SUPERIO_VENDOR_NAME we can also have a Kconfig option
SUPERIO_VENDOR_NAME_HAS_EARLY_SERIAL, and base our decision of including
the early serial code in romstage based on the latter.

======================================================================
config BOARD_SPECIFIC_OPTIONS # dummy
	def_bool y
	[...]
	select SUPERIO_FINTEK_F81865F
	select SUPERIO_OTHERVENDOR_OTHERMODEL
	select SUPERIO_FINTEK_F81865F_HAS_EARLY_SERIAL
	[...]
======================================================================

romstage-$(CONFIG_SUPERIO_FINTEK_F81865F_HAS_EARLY_SERIAL) += early_serial.c

If the board has two identical superios, then the same file will get
included (only once). In this case, handling both superios will have to
be done in mainboard logic anyways.

D:
Just forget about it and never again worry about .c includes.

Alex
Alexandru Gagniuc - 2011-02-26 21:44:00
On 02/26/2011 11:43 PM, Patrick Georgi wrote:
> Am Samstag, 26. Februar 2011, 23:27:47 schrieb Alex G.:
>> Why bother holding dear to romcc when the obvious solution is to move
>> those boards to CAR? We introduce more unneeded complexity, and make it
>> at least just as hard to phase out romcc.
> Those boards carry CPUs without CAR.
> 
Then those boards are most likely old and obsolete. Is it worth still
supporting them? "Support for this board was dropped with revision
'xyzw'". Do we want to make it xyzwq ?

> As for phasing out such support: That would be easy (just drop everything 
> guarded in CONFIG_ROMCC in the build system).
> 
Yeah, wasting countless hours fixing something that should have been
done long ago is fun. Oh wait, you guys are grep/sed masters; nevermind
that. :)

Alex

Patch

Moves the inclusion of the superio early code from romstage.c in 
the mainboard directory to Makefile.inc in the superio directory.
Adapted amd/persimmon board to fit this model.

Designed to serve as an example on how to deal with all superio
#include "*.c" directives in romstage.c

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
Here's what I did to my svn:
svn add src/include/superio
svn mv src/superio/fintek/f81865/f81865f_early_serial.c src/superio/fintek/f81865/early_serial.c

Index: src/include/superio/early_serial.h
===================================================================
--- src/include/superio/early_serial.h	(revision 0)
+++ src/include/superio/early_serial.h	(revision 0)
@@ -0,0 +1,39 @@ 
+/*
+ * This file is part of the coreboot project.
+ *
+ * Copyright (C) 2011  Alexandru Gagniuc <mr.nuke.me@gmail.com>
+ *
+ * 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, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * 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.
+ * 
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+#ifndef SUPERIO_EARLY_SERIAL_H
+#define SUPERIO_EARLY_SERIAL_H
+
+#include <arch/romcc_io.h>
+#include <stdint.h>
+
+/**
+ * \brief Pre-RAM (romstage) serial port initialization
+ * 
+ * Initializes the serial port early in the boot sequence.
+ * \n
+ * The actual definition is in each superio chip's early_serial.c.
+ * early_serial.c should be included by the Makefile.inc of the respective
+ * superio See \n
+ * src/superio/fintek/f81865/Makefile.inc \n
+ * for details
+ * 
+ */
+void superio_enable_early_serial(device_t dev, u16 iobase);
+
+#endif /*  SUPERIO_EARLY_SERIAL_H */
\ No newline at end of file
Index: src/superio/fintek/f81865f/f81865f_early_serial.c
===================================================================
--- src/superio/fintek/f81865f/f81865f_early_serial.c	(revision 6380)
+++ src/superio/fintek/f81865f/f81865f_early_serial.c	(working copy)
@@ -1,47 +0,0 @@ 
-/*
- * This file is part of the coreboot project.
- *
- * Copyright (C) 2011 Advanced Micro Devices, Inc.
- *
- * 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; either version 2 of the License, or
- * (at your option) any later version.
- *
- * 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.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
- */
-
-/* Pre-RAM driver for the Fintek F81865F/FG Super I/O chip. */
-
-#include <arch/romcc_io.h>
-#include "f81865f.h"
-
-static void pnp_enter_conf_state(device_t dev)
-{
-	u16 port = dev >> 8;
-	outb(0x87, port);
-	outb(0x87, port);
-}
-
-static void pnp_exit_conf_state(device_t dev)
-{
-	u16 port = dev >> 8;
-	outb(0xaa, port);
-}
-
-static void f81865f_enable_serial(device_t dev, u16 iobase)
-{
-	pnp_enter_conf_state(dev);
-	pnp_set_logical_device(dev);
-	pnp_set_enable(dev, 0);
-	pnp_set_iobase(dev, PNP_IDX_IO0, iobase);
-	pnp_set_enable(dev, 1);
-	pnp_exit_conf_state(dev);
-}
Index: src/superio/fintek/f81865f/Makefile.inc
===================================================================
--- src/superio/fintek/f81865f/Makefile.inc	(revision 6380)
+++ src/superio/fintek/f81865f/Makefile.inc	(working copy)
@@ -2,6 +2,7 @@ 
 ## This file is part of the coreboot project.
 ##
 ## Copyright (C) 2011 Advanced Micro Devices, Inc.
+## Copyright (C) 2011 Alexandru Gagniuc <mr.nuke.me@gmail.com>
 ##
 ## 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
@@ -19,3 +20,8 @@ 
 ##
 
 ramstage-$(CONFIG_SUPERIO_FINTEK_F81865F) += superio.c
+
+# Include early_serial.c automatically in romstage, so that including it in
+# romstage.c as a "#include .c" is no longer necesarry
+
+romstage-$(CONFIG_SUPERIO_FINTEK_F81865F) += early_serial.c
Index: src/superio/fintek/f81865f/early_serial.c
===================================================================
--- src/superio/fintek/f81865f/early_serial.c	(revision 6380)
+++ src/superio/fintek/f81865f/early_serial.c	(working copy)
@@ -19,10 +19,14 @@ 
  */
 
 /* Pre-RAM driver for the Fintek F81865F/FG Super I/O chip. */
+#include <arch/io.h>	/* <== Must be included before early_serial.h */
+#include <superio/early_serial.h>
 
-#include <arch/romcc_io.h>
+#include <device/pnp_def.h>
+
 #include "f81865f.h"
 
+
 static void pnp_enter_conf_state(device_t dev)
 {
 	u16 port = dev >> 8;
@@ -36,7 +40,7 @@ 
 	outb(0xaa, port);
 }
 
-static void f81865f_enable_serial(device_t dev, u16 iobase)
+void superio_enable_early_serial(device_t dev, u16 iobase)
 {
 	pnp_enter_conf_state(dev);
 	pnp_set_logical_device(dev);
Index: src/mainboard/amd/persimmon/romstage.c
===================================================================
--- src/mainboard/amd/persimmon/romstage.c	(revision 6380)
+++ src/mainboard/amd/persimmon/romstage.c	(working copy)
@@ -2,6 +2,7 @@ 
  * This file is part of the coreboot project.
  *
  * Copyright (C) 2011 Advanced Micro Devices, Inc.
+ * Copyright (C) 2011 Alexandru Gagniuc <mr.nuke.me@gmail.com>
  *
  * 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
@@ -31,7 +32,8 @@ 
 #include <console/loglevel.h>
 #include "agesawrapper.h"
 #include "cpu/x86/bist.h"
-#include "superio/fintek/f81865f/f81865f_early_serial.c"
+#include <superio/early_serial.h>
+#include "superio/fintek/f81865f/f81865f.h"
 #include "cpu/x86/lapic/boot_cpu.c"
 #include "pc80/i8254.c"
 #include "pc80/i8259.c"
@@ -52,7 +54,7 @@ 
     sb_poweron_init();
 
     post_code(0x31);
-    f81865f_enable_serial(SERIAL_DEV, CONFIG_TTYS0_BASE);
+    superio_enable_early_serial(SERIAL_DEV, CONFIG_TTYS0_BASE);
     uart_init();
     console_init();
   }
Index: README
===================================================================
--- README	(revision 6380)
+++ README	(working copy)
@@ -1,3 +1,6 @@ 
+svn add src/include/superio
+svn mv src/superio/fintek/f81865/f81865f_early_serial.c src/superio/fintek/f81865/early_serial.c
+
 -------------------------------------------------------------------------------
 coreboot README
 -------------------------------------------------------------------------------