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 -- coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot