[coreboot] [PATCH][RFC] First step in converting W83977TF early serial from included to linked

[Alex G.]

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