Patchwork First step in converting W83977TF early serial from included to linked

login
register
about
Submitter Keith Hui
Date 2011-03-01 05:20:07
Message ID <AANLkTik1i0DMaJP2GQDMsm5ycvb+kKBxrRhtwwqjFADi@mail.gmail.com>
Download mbox | patch
Permalink /patch/2715/
State New
Headers show

Comments

Keith Hui - 2011-03-01 05:20:07
And here is the patch. abuild-tested. I will boot test it with P2B-LS
and P3B-F tomorrow but I want this patch out there to generate some
discussions and get more boot test coverage.

This I believe falls under "infrastructure projects" [1].

This patch facilitates, for boards using Winbond W83977TF superio,
dropping early_serial.c from #includes of their romstage, instead
linking to them; and converts 12 of 13 mainboards using this superio
to do exactly this. The lone board out, iei/nova4899r, is a
Geode/CS5530 board that has not yet been converted to CAR or tiny
bootblock. The rest are all slot 1/440BX/i82371eb boards that have
been converted. At this stage I should leave converting CS5530 to tiny
bootblock to someone better versed in this platform.

The pnp_... functions defined in romcc_io.h now have a new home in
arch/x86/lib/pnp.c, and is compiled and linked like any other C files
meant for romstage.

This patch puts the W83977TF early serial code in a state where it can
be incorporated both through the old setup (ie. #included in mainboard
romstage), or be compiled separately and linked into romstage. Once
this conversion is done on all superios and all southbridges/boards
have been converted, the few #ifdefs that made this possible can be
cleaned out.

Signed-off-by: Keith Hui <buurin@gmail.com>

[1] http://www.coreboot.org/Infrastructure_Projects#Remove_.c_includes

On Mon, Feb 28, 2011 at 1:00 PM, Keith Hui <buurin@gmail.com> wrote:
> Hi all,
>
> In my pipeline is something more infrastructural, something different
> from what I usually do -
>
> I am converting early serial code for Winbond W83977TF from code
> included in romstage to compiled unit to be linked into romstage.
>
> 13 boards use this superio, some apparently still not converted to CAR
> (including one Intel server board). I have yet to complete running
> abuild on my change. Should I let it break when I submit it for review
> given earlier buzz suggesting we leave non-CAR boards behind?
>
> The reason I'm doing this is I want to do this for ITE IT8705F, which
> happens to have no board using it at the moment, but I want to test
> this approach on a board that I can boot test right now. Enter the
> P2B-LS and W83977TF. If this proof of concept works, we can start this
> conversion one superio at a time and nail down one more goal of the
> infrastructure refactoring.
>
> Thoughts and comments?
>
Thanks
Keith
Alexandru Gagniuc - 2011-03-01 09:19:09
On 03/01/2011 07:20 AM, Keith Hui wrote:
> And here is the patch. abuild-tested. I will boot test it with P2B-LS
> and P3B-F tomorrow but I want this patch out there to generate some
> discussions and get more boot test coverage.
> 
OK.

> This I believe falls under "infrastructure projects" [1].
> 
Indeed, and for that reason people should be _more_ involved and open to
discuss this issue, not just leave it aside and ignore it. I'm glad
you're at least thinking of that.

> This patch facilitates, for boards using Winbond W83977TF superio,
> dropping early_serial.c from #includes of their romstage, instead
> linking to them; and converts 12 of 13 mainboards using this superio
> to do exactly this. The lone board out, iei/nova4899r, is a
> Geode/CS5530 board that has not yet been converted to CAR or tiny
> bootblock. The rest are all slot 1/440BX/i82371eb boards that have
> been converted. At this stage I should leave converting CS5530 to tiny
> bootblock to someone better versed in this platform.
> 
Definitely leave it out for now. Converting that to CAR is for a
different patch, though I think the tanks will expect you to convert it.

> The pnp_... functions defined in romcc_io.h now have a new home in
> arch/x86/lib/pnp.c, and is compiled and linked like any other C files
> meant for romstage.
> 
I do not like this. I would prefer to have the pnp functions declared
inline in a header file. It's more elegant for functions this small, and
avoids call/ret.

> This patch puts the W83977TF early serial code in a state where it can
> be incorporated both through the old setup (ie. #included in mainboard
> romstage), or be compiled separately and linked into romstage. Once
> this conversion is done on all superios and all southbridges/boards
> have been converted, the few #ifdefs that made this possible can be
> cleaned out.
> 
Don't worry about new board using this superio. If you broke the one
non-CAR board using this superio, leave it broken and ignore all the
torro-fecal matter surrounding the issue of the old build system. If you
think you can convert that board to CAR (and have the time), definitely
go for it. :)

> 
> Index: src/include/device/device.h
> ===================================================================
> --- src/include/device/device.h	(revision 6411)
> +++ src/include/device/device.h	(working copy)
> @@ -7,7 +7,15 @@
>  
>  
>  struct device;
> +
> +// In ramstage, device_t will be a structure.
> +#if defined(__PRE_RAM__) && !defined(__ROMCC__)
> +typedef unsigned device_t;
OK. I can see where this is going. I'm still not convinced it is the
best option, but I don't have a suggestion on this.

> Index: src/superio/winbond/w83977tf/Makefile.inc
> ===================================================================
> --- src/superio/winbond/w83977tf/Makefile.inc	(revision 6411)
> +++ src/superio/winbond/w83977tf/Makefile.inc	(working copy)
> @@ -22,3 +22,5 @@
>  
>  ramstage-$(CONFIG_SUPERIO_WINBOND_W83977TF) += superio.c
>  
> +romstage-$(CONFIG_SUPERIO_WINBOND_W83977TF) += early_serial.c
> +romstage-$(CONFIG_SUPERIO_WINBOND_W83977TF) += ../../../arch/x86/lib/pnp.c
This last line is ugly. Just keep the functions inlined. pnp.c is
unnecessarry.

> Index: src/superio/winbond/w83977tf/early_serial.c
> ===================================================================
> --- src/superio/winbond/w83977tf/early_serial.c	(revision 6411)
> +++ src/superio/winbond/w83977tf/early_serial.c	(working copy)
> @@ -20,7 +20,12 @@
>   * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
>   */
>  
> +#if defined(__ROMCC__)
>  #include <arch/romcc_io.h>
> +#else
> +#include <device/pnp.h>
> +#include <arch/io.h>
> +#endif
Keith, nothing here for you, move along. For others, why can't we just
get rid of romcc altogether?.


>  
> Index: src/arch/x86/lib/pnp.c
> 
AAAAAAAAAAAAAAAAARGH!

Another thing we need to discuss (THAT MEANS __*EVERYBODY*__, NOT JUST
ME AND KEITH) is whether or not we should declare the
enable_early_serial() in a common place, and just have the superio code
implement it. I prefer common name option, as it makes the superio code
more transparent to the developer.

Alex
zxy__1127 - 2011-03-01 10:36:16
Hi all,

After I add some memory initialisize code, the romstage is over 64K byte,and tne code can't run properly.
Then what should I do when romstage is over 64K byte? Is romstage's size limits to 64KB?

Thanks a lot!
2011-03-01 



zxy__1127

Patch

Index: src/include/device/device.h
===================================================================
--- src/include/device/device.h	(revision 6411)
+++ src/include/device/device.h	(working copy)
@@ -7,7 +7,15 @@ 
 
 
 struct device;
+
+// In ramstage, device_t will be a structure.
+#if defined(__PRE_RAM__) && !defined(__ROMCC__)
+typedef unsigned device_t;
+#else
+// Code compiled with ROMCC should not include this file.
 typedef struct device * device_t;
+#endif
+
 struct pci_operations;
 struct pci_bus_operations;
 struct smbus_bus_operations;
Index: src/include/device/pnp_def.h
===================================================================
--- src/include/device/pnp_def.h	(revision 6411)
+++ src/include/device/pnp_def.h	(working copy)
@@ -13,4 +13,8 @@ 
 #define PNP_IDX_MSC0 0xf0
 #define PNP_IDX_MSC1 0xf1
 
+#ifndef PNP_DEV
+#define PNP_DEV(PORT, FUNC) ((PORT << 8) | (FUNC))
+#endif
+
 #endif /* DEVICE_PNP_DEF_H */
Index: src/superio/winbond/w83977tf/Makefile.inc
===================================================================
--- src/superio/winbond/w83977tf/Makefile.inc	(revision 6411)
+++ src/superio/winbond/w83977tf/Makefile.inc	(working copy)
@@ -22,3 +22,5 @@ 
 
 ramstage-$(CONFIG_SUPERIO_WINBOND_W83977TF) += superio.c
 
+romstage-$(CONFIG_SUPERIO_WINBOND_W83977TF) += early_serial.c
+romstage-$(CONFIG_SUPERIO_WINBOND_W83977TF) += ../../../arch/x86/lib/pnp.c
Index: src/superio/winbond/w83977tf/w83977tf.h
===================================================================
--- src/superio/winbond/w83977tf/w83977tf.h	(revision 6411)
+++ src/superio/winbond/w83977tf/w83977tf.h	(working copy)
@@ -34,4 +34,8 @@ 
 #define W83977TF_GPIO3            9
 #define W83977TF_ACPI            10
 
+#if defined(__PRE_RAM__) && !defined(__ROMCC__)
+void w83977tf_enable_serial(device_t dev, u16 iobase);
 #endif
+
+#endif
Index: src/superio/winbond/w83977tf/early_serial.c
===================================================================
--- src/superio/winbond/w83977tf/early_serial.c	(revision 6411)
+++ src/superio/winbond/w83977tf/early_serial.c	(working copy)
@@ -20,7 +20,12 @@ 
  * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
  */
 
+#if defined(__ROMCC__)
 #include <arch/romcc_io.h>
+#else
+#include <device/pnp.h>
+#include <arch/io.h>
+#endif
 #include "w83977tf.h"
 
 static void pnp_enter_ext_func_mode(device_t dev)
@@ -36,7 +41,10 @@ 
 	outb(0xaa, port);
 }
 
-static void w83977tf_enable_serial(device_t dev, u16 iobase)
+#if defined(__ROMCC__)
+static 
+#endif
+void w83977tf_enable_serial(device_t dev, u16 iobase)
 {
 	pnp_enter_ext_func_mode(dev);
 	pnp_set_logical_device(dev);
Index: src/mainboard/a-trend/atc-6220/romstage.c
===================================================================
--- src/mainboard/a-trend/atc-6220/romstage.c	(revision 6411)
+++ src/mainboard/a-trend/atc-6220/romstage.c	(working copy)
@@ -21,8 +21,7 @@ 
 #include <stdint.h>
 #include <device/pci_def.h>
 #include <arch/io.h>
-#include <device/pnp_def.h>
-#include <arch/romcc_io.h>
+#include <device/pnp.h>
 #include <arch/hlt.h>
 #include <stdlib.h>
 #include <console/console.h>
@@ -31,7 +30,7 @@ 
 #include "pc80/udelay_io.c"
 #include "lib/delay.c"
 #include "cpu/x86/bist.h"
-#include "superio/winbond/w83977tf/early_serial.c"
+#include "superio/winbond/w83977tf/w83977tf.h"
 #include <lib.h>
 
 #define SERIAL_DEV PNP_DEV(0x3f0, W83977TF_SP1)
Index: src/mainboard/azza/pt-6ibd/romstage.c
===================================================================
--- src/mainboard/azza/pt-6ibd/romstage.c	(revision 6411)
+++ src/mainboard/azza/pt-6ibd/romstage.c	(working copy)
@@ -21,8 +21,7 @@ 
 #include <stdint.h>
 #include <device/pci_def.h>
 #include <arch/io.h>
-#include <device/pnp_def.h>
-#include <arch/romcc_io.h>
+#include <device/pnp.h>
 #include <arch/hlt.h>
 #include <stdlib.h>
 #include <console/console.h>
@@ -32,7 +31,7 @@ 
 #include "lib/delay.c"
 #include "cpu/x86/bist.h"
 /* FIXME: It's a Winbond W83977EF, actually. */
-#include "superio/winbond/w83977tf/early_serial.c"
+#include "superio/winbond/w83977tf/w83977tf.h"
 #include <lib.h>
 
 /* FIXME: It's a Winbond W83977EF, actually. */
Index: src/mainboard/abit/be6-ii_v2_0/romstage.c
===================================================================
--- src/mainboard/abit/be6-ii_v2_0/romstage.c	(revision 6411)
+++ src/mainboard/abit/be6-ii_v2_0/romstage.c	(working copy)
@@ -22,8 +22,7 @@ 
 #include <stdlib.h>
 #include <device/pci_def.h>
 #include <arch/io.h>
-#include <device/pnp_def.h>
-#include <arch/romcc_io.h>
+#include <device/pnp.h>
 #include <arch/hlt.h>
 #include <console/console.h>
 #include "southbridge/intel/i82371eb/i82371eb.h"
@@ -32,7 +31,7 @@ 
 #include "lib/delay.c"
 #include "cpu/x86/bist.h"
 /* FIXME: It's a Winbond W83977EF, actually. */
-#include "superio/winbond/w83977tf/early_serial.c"
+#include "superio/winbond/w83977tf/w83977tf.h"
 #include <lib.h>
 
 /* FIXME: It's a Winbond W83977EF, actually. */
Index: src/mainboard/msi/ms6119/romstage.c
===================================================================
--- src/mainboard/msi/ms6119/romstage.c	(revision 6411)
+++ src/mainboard/msi/ms6119/romstage.c	(working copy)
@@ -22,8 +22,7 @@ 
 #include <stdlib.h>
 #include <device/pci_def.h>
 #include <arch/io.h>
-#include <device/pnp_def.h>
-#include <arch/romcc_io.h>
+#include <device/pnp.h>
 #include <arch/hlt.h>
 #include <console/console.h>
 #include "southbridge/intel/i82371eb/i82371eb.h"
@@ -31,7 +30,7 @@ 
 #include "pc80/udelay_io.c"
 #include "lib/delay.c"
 #include "cpu/x86/bist.h"
-#include "superio/winbond/w83977tf/early_serial.c"
+#include "superio/winbond/w83977tf/w83977tf.h"
 #include <lib.h>
 
 #define SERIAL_DEV PNP_DEV(0x3f0, W83977TF_SP1)
Index: src/mainboard/msi/ms6147/romstage.c
===================================================================
--- src/mainboard/msi/ms6147/romstage.c	(revision 6411)
+++ src/mainboard/msi/ms6147/romstage.c	(working copy)
@@ -22,8 +22,7 @@ 
 #include <stdlib.h>
 #include <device/pci_def.h>
 #include <arch/io.h>
-#include <device/pnp_def.h>
-#include <arch/romcc_io.h>
+#include <device/pnp.h>
 #include <arch/hlt.h>
 #include <console/console.h>
 #include "southbridge/intel/i82371eb/i82371eb.h"
@@ -31,7 +30,7 @@ 
 #include "pc80/udelay_io.c"
 #include "lib/delay.c"
 #include <cpu/x86/bist.h>
-#include "superio/winbond/w83977tf/early_serial.c"
+#include "superio/winbond/w83977tf/w83977tf.h"
 #include <lib.h>
 
 #define SERIAL_DEV PNP_DEV(0x3f0, W83977TF_SP1)
Index: src/mainboard/msi/ms6156/romstage.c
===================================================================
--- src/mainboard/msi/ms6156/romstage.c	(revision 6411)
+++ src/mainboard/msi/ms6156/romstage.c	(working copy)
@@ -22,8 +22,7 @@ 
 #include <stdlib.h>
 #include <device/pci_def.h>
 #include <arch/io.h>
-#include <device/pnp_def.h>
-#include <arch/romcc_io.h>
+#include <device/pnp.h>
 #include <arch/hlt.h>
 #include <console/console.h>
 #include "southbridge/intel/i82371eb/i82371eb.h"
@@ -31,7 +30,7 @@ 
 #include "pc80/udelay_io.c"
 #include "lib/delay.c"
 #include "cpu/x86/bist.h"
-#include "superio/winbond/w83977tf/early_serial.c"
+#include "superio/winbond/w83977tf/w83977tf.h"
 #include <lib.h>
 
 #define SERIAL_DEV PNP_DEV(0x3f0, W83977TF_SP1)
Index: src/mainboard/asus/p2b/romstage.c
===================================================================
--- src/mainboard/asus/p2b/romstage.c	(revision 6411)
+++ src/mainboard/asus/p2b/romstage.c	(working copy)
@@ -21,8 +21,7 @@ 
 #include <stdint.h>
 #include <device/pci_def.h>
 #include <arch/io.h>
-#include <device/pnp_def.h>
-#include <arch/romcc_io.h>
+#include <device/pnp.h>
 #include <arch/hlt.h>
 #include <stdlib.h>
 #include <console/console.h>
@@ -31,7 +30,7 @@ 
 #include "pc80/udelay_io.c"
 #include "lib/delay.c"
 #include "cpu/x86/bist.h"
-#include "superio/winbond/w83977tf/early_serial.c"
+#include "superio/winbond/w83977tf/w83977tf.h"
 #include <lib.h>
 
 #define SERIAL_DEV PNP_DEV(0x3f0, W83977TF_SP1)
Index: src/mainboard/asus/p2b-ds/romstage.c
===================================================================
--- src/mainboard/asus/p2b-ds/romstage.c	(revision 6411)
+++ src/mainboard/asus/p2b-ds/romstage.c	(working copy)
@@ -21,8 +21,7 @@ 
 #include <stdint.h>
 #include <device/pci_def.h>
 #include <arch/io.h>
-#include <device/pnp_def.h>
-#include <arch/romcc_io.h>
+#include <device/pnp.h>
 #include <arch/hlt.h>
 #include <stdlib.h>
 #include <console/console.h>
@@ -31,7 +30,7 @@ 
 #include "pc80/udelay_io.c"
 #include "lib/delay.c"
 #include "cpu/x86/bist.h"
-#include "superio/winbond/w83977tf/early_serial.c"
+#include "superio/winbond/w83977tf/w83977tf.h"
 #include <lib.h>
 
 #define SERIAL_DEV PNP_DEV(0x3f0, W83977TF_SP1)
Index: src/mainboard/asus/p2b-ls/romstage.c
===================================================================
--- src/mainboard/asus/p2b-ls/romstage.c	(revision 6411)
+++ src/mainboard/asus/p2b-ls/romstage.c	(working copy)
@@ -21,8 +21,7 @@ 
 #include <stdint.h>
 #include <device/pci_def.h>
 #include <arch/io.h>
-#include <device/pnp_def.h>
-#include <arch/romcc_io.h>
+#include <device/pnp.h>
 #include <arch/hlt.h>
 #include <stdlib.h>
 #include <console/console.h>
@@ -31,8 +30,8 @@ 
 #include "pc80/udelay_io.c"
 #include "lib/delay.c"
 #include "cpu/x86/bist.h"
 /* FIXME: The ASUS P2B-LS has a Winbond W83977EF, actually. */
-#include "superio/winbond/w83977tf/early_serial.c"
+#include "superio/winbond/w83977tf/w83977tf.h"
 #include <lib.h>
 
 #define SERIAL_DEV PNP_DEV(0x3f0, W83977TF_SP1)
Index: src/mainboard/asus/p2b-d/romstage.c
===================================================================
--- src/mainboard/asus/p2b-d/romstage.c	(revision 6411)
+++ src/mainboard/asus/p2b-d/romstage.c	(working copy)
@@ -21,8 +21,7 @@ 
 #include <stdint.h>
 #include <device/pci_def.h>
 #include <arch/io.h>
-#include <device/pnp_def.h>
-#include <arch/romcc_io.h>
+#include <device/pnp.h>
 #include <arch/hlt.h>
 #include <stdlib.h>
 #include <console/console.h>
@@ -31,7 +30,7 @@ 
 #include "pc80/udelay_io.c"
 #include "lib/delay.c"
 #include "cpu/x86/bist.h"
-#include "superio/winbond/w83977tf/early_serial.c"
+#include "superio/winbond/w83977tf/w83977tf.h"
 #include <lib.h>
 
 #define SERIAL_DEV PNP_DEV(0x3f0, W83977TF_SP1)
Index: src/mainboard/asus/p2b-f/romstage.c
===================================================================
--- src/mainboard/asus/p2b-f/romstage.c	(revision 6411)
+++ src/mainboard/asus/p2b-f/romstage.c	(working copy)
@@ -21,8 +21,7 @@ 
 #include <stdint.h>
 #include <device/pci_def.h>
 #include <arch/io.h>
-#include <device/pnp_def.h>
-#include <arch/romcc_io.h>
+#include <device/pnp.h>
 #include <arch/hlt.h>
 #include <stdlib.h>
 #include <console/console.h>
@@ -32,7 +31,7 @@ 
 #include "lib/delay.c"
 #include "cpu/x86/bist.h"
 /* FIXME: The ASUS P2B-F has a Winbond W83977EF, actually. */
-#include "superio/winbond/w83977tf/early_serial.c"
+#include "superio/winbond/w83977tf/w83977tf.h"
 #include <lib.h>
 
 /* FIXME: The ASUS P2B-F has a Winbond W83977EF, actually. */
Index: src/mainboard/asus/p3b-f/romstage.c
===================================================================
--- src/mainboard/asus/p3b-f/romstage.c	(revision 6411)
+++ src/mainboard/asus/p3b-f/romstage.c	(working copy)
@@ -21,8 +21,7 @@ 
 #include <stdint.h>
 #include <device/pci_def.h>
 #include <arch/io.h>
-#include <device/pnp_def.h>
-#include <arch/romcc_io.h>
+#include <device/pnp.h>
 #include <arch/hlt.h>
 #include <stdlib.h>
 #include <console/console.h>
@@ -32,7 +31,7 @@ 
 #include "lib/delay.c"
 #include "cpu/x86/bist.h"
 /* FIXME: The ASUS P3B-F has a Winbond W83977EF, actually. */
-#include "superio/winbond/w83977tf/early_serial.c"
+#include "superio/winbond/w83977tf/w83977tf.h"
 #include <lib.h>
 
 /* FIXME: The ASUS P3B-F has a Winbond W83977EF, actually. */
Index: src/arch/x86/include/arch/romcc_io.h
===================================================================
--- src/arch/x86/include/arch/romcc_io.h	(revision 6411)
+++ src/arch/x86/include/arch/romcc_io.h	(working copy)
@@ -52,8 +52,9 @@ 
 #define PCI_ID(VENDOR_ID, DEVICE_ID) \
 	((((DEVICE_ID) & 0xFFFF) << 16) | ((VENDOR_ID) & 0xFFFF))
 
-
+#ifndef PNP_DEV
 #define PNP_DEV(PORT, FUNC) (((PORT) << 8) | (FUNC))
+#endif
 
 typedef unsigned device_t; /* pci and pci_mmio need to have different ways to have dev */
 
Index: src/arch/x86/lib/pnp.c
===================================================================
--- src/arch/x86/lib/pnp.c	(revision 0)
+++ src/arch/x86/lib/pnp.c	(revision 0)
@@ -0,0 +1,57 @@ 
+#include <stddef.h>
+#include <console/console.h>
+#include <arch/io.h>
+#include <device/pnp.h>
+
+/* Generic functions for pnp devices */
+void pnp_write_config(device_t dev, u8 reg, u8 value)
+{
+	unsigned port = dev >> 8;
+	outb(reg, port );
+	outb(value, port +1);
+}
+
+u8 pnp_read_config(device_t dev, u8 reg)
+{
+	unsigned port = dev >> 8;
+	outb(reg, port);
+	return inb(port +1);
+}
+
+void pnp_set_logical_device(device_t dev)
+{
+	unsigned device = dev & 0xff;
+	pnp_write_config(dev, 0x07, device);
+}
+
+void pnp_set_enable(device_t dev, int enable)
+{
+	pnp_write_config(dev, 0x30, enable?0x1:0x0);
+}
+
+int pnp_read_enable(device_t dev)
+{
+	return !!pnp_read_config(dev, 0x30);
+}
+
+void pnp_set_iobase(device_t dev, u8 index, u16 iobase)
+{
+	pnp_write_config(dev, index + 0, (iobase >> 8) & 0xff);
+	pnp_write_config(dev, index + 1, iobase & 0xff);
+}
+/*
+u16 pnp_read_iobase(device_t dev, unsigned index)
+{
+	return ((u16)(pnp_read_config(dev, index)) << 8) | pnp_read_config(dev, index + 1);
+}
+*/
+void pnp_set_irq(device_t dev, u8 index, u8 irq)
+{
+	pnp_write_config(dev, index, irq);
+}
+
+void pnp_set_drq(device_t dev, u8 index, u8 drq)
+{
+	pnp_write_config(dev, index, drq);
+}
+