[LinuxBIOS] [PATCH] SIO PC87338 support, 440bx bug fix, VMWare mainboard support

[Ceri Coburn]

Added initial support for the national semiconductor's PC87338 Super I/O

Fixed bug in northbridge.c for the 440bx - calculating top of RAM was
incorrect

Added initial support for VMWare workstation.

Separated DRB register setting into separate function

Now setting PAM registers for 440bx to exclude majority of BIOS area

VMware Does not fully boot a payload yet

Signed-off-by: Ceri Coburn [e-mail]

---

Note on VMWare -  The build of the vmware mainboard rom will not work
directly out of the box with any release of Workstation because ESCD
configuration information it copied into the loaded ROM overwriting portions
of the bios code.  Ideally a hole needs to be created for this, or an update
from VMWare to allow the position of ESCD config to be specified or turned
off.

I had to separate the DRB register setting from the sdram_set_spd_registers
function because VMWare pre populates the DRB registers within the
configuration space because it hasn't emulated SPD for the RAM correctly (as
far as I can tell).  So any 440bx based

Other issues with VMWare are that it has issues executing the SCSI
controller ROM ("NOT IMPLEMENTED" assertion error within VMWare, i'm
guessing something in the emulator code is executing, which VMWare hasn't
implemented), so remove SCSI controllers from the virtual machine config,
and that FILO times out when reading sector 0 of the IDE disk, could be some
config error in the southbridge config for IDE.

Ceri.

[Corey Osgood]

Just a few comments:

Ceri Coburn wrote:
> Added initial support for the national semiconductor’s PC87338 Super I/O

IMO, 1 patch
> Added initial support for VMWare workstation.

another patch
> Fixed bug in northbridge.c for the 440bx – calculating top of RAM was
> incorrect
>
> Separated DRB register setting into separate function
>
> Now setting PAM registers for 440bx to exclude majority of BIOS area

third patch. I think these patches also break tyan s1846, please check.
Try to avoid breaking that board, as others folks are working on it and
the 440bx.
> Index: src/superio/nsc/pc87338/pc87338.h
>
> --- src/superio/nsc/pc87338/pc87338.h    (revision 0) 
> +++ src/superio/nsc/pc87338/pc87338.h    (revision 0) 
> @@ -0,0 +1,32 @@
> +/*
> This file is part of the LinuxBIOS project.
> Copyright (C) 2006 Uwe Hermann
> Index: src/superio/nsc/pc87338/Config.lb
>
> --- src/superio/nsc/pc87338/Config.lb    (revision 0) 
> +++ src/superio/nsc/pc87338/Config.lb    (revision 0) 
> @@ -0,0 +1,23 @@
> +## 
> +## This file is part of the LinuxBIOS project. 
> +## 
> +## Copyright (C) 2006 Uwe Hermann  
> +##

Why Uwe and not yourself? These are trivial files, just remove uwe and
add yourself.
> Property changes on: src/superio/nsc/pc87338/Config.lb
> ___________________________________________________________________
> Name: svn:executable

Not necessary for any of these files, please fix.
> +/* Special values used for entering MB PnP mode. The first four bytes of
> each line determine the address port, the last four are data. */
>
> +static const uint8_t init_values[] = {
> 0x6A, 0xB5, 0xDA, 0xED, 0xF6, 0xFB, 0x7D, 0xBE,
> 0xDF, 0x6F, 0x37, 0x1B, 0x0D, 0x86, 0xC3, 0x61,
> 0xB0, 0x58, 0x2C, 0x16, 0x8B, 0x45, 0xA2, 0xD1,
> 0xE8, 0x74, 0x3A, 0x9D, 0xCE, 0xE7, 0x00, 0x00
>
> +};

Are these ALL really necessary? Is it just me, or are they not even used?
> /* Sequentially write the 32 special PnP values. */
> //for (i = 0; i < 32; i++) {
> //    outb(init_values[i], NSC87338_CONFIGURATION_PORT);
> //}
> ///* Write out our INDEX/BASE address after the PnP special values */
> //outb( (SIO_BASE & 0xff00) >> 8, NSC87338_CONFIGURATION_PORT);
> //outb(  SIO_BASE & 0xff , NSC87338_CONFIGURATION_PORT);
> /* Set th address of SSC1 */
> //pc87338_sio_write(NSC87338_CONFIG_REG_S1BAH,(iobase & 0xff00) << 8);
> //pc87338_sio_write(NSC87338_CONFIG_REG_S1BAL, iobase & 0xff);
> /* Enable PP, SSC1, SSC2, FDC, FDC Four-Drive encoding */

Please avoid including commented code in patches.

A few of your files (failover.c, etc) still need GPL headers.
> +static void enable_shadow_ram(void) 
> +{
> device_t dev = 0; /* no need to look up 0:0.0 */
> unsigned char shadowreg;
> /* dev 0 for southbridge */
> shadowreg = pci_read_config8(dev, 0x63);
> /* 0xf0000-0xfffff */
> shadowreg |= 0x30;
> pci_write_config8(dev, 0x63, shadowreg);
>
> +}

This function is incorrect, it's a result of the code copy from via
epia. On 440bx, register 0x63 is actually a DRB, so setting it here
might seriously screw things up, unless it's reset during sdram_init.
Here's a correct version:

static void enable_shadow_ram(void)
{
    uint8_t shadowreg = pci_read_config8(0, 0x59);
    shadowreg |= 0x30;
    pci_write_config8(0, 0x59, shadowreg);
}
> Index: src/mainboard/emulation/vmware/mainboard.c
>
> --- src/mainboard/emulation/vmware/mainboard.c    (revision 0) 
> +++ src/mainboard/emulation/vmware/mainboard.c    (revision 0) 
> @@ -0,0 +1,40 @@
> +/*
> This file is part of the LinuxBIOS project.
> Copyright (C) 2007 Ceri Coburn

just one empty line here, please
> 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
> */
>
> +#include <device/device.h> 
> +#include "chip.h" 
> +#include <pc80/keyboard.h> 
> +#include <console/console.h>
>
> +static void vmware_init(struct device* dev) 
> +{
> printk_debug("Initializing VMWare keyboard\n");
> init_pc_keyboard(0x60,0x64,0);
>
> +}
>
> +struct chip_operations mainboard_emulation_vmware_ops = {
> CHIP_NAME("VMWare Workstation")
> .enable_dev = vmware_init
>
> +};

Very annoying whitespace issues here!
> Index: src/northbridge/intel/i440bx/raminit.c
>
> ---src/northbridge/intel/i440bx/raminit.c    (revision 2639) 
> +++ src/northbridge/intel/i440bx/raminit.c    (working copy) 
> @@ -181,13 +181,13 @@
>       * 11 = Read/Write (all access goes to DRAM)
>       */
>      // TODO
> PAM0, 0x00000000, 0x00,
> PAM1, 0x00000000, 0x00,
> PAM2, 0x00000000, 0x00,
> PAM0, 0x00000000, 0x10,

Why set PAM0 to 0x10? Don't we want 0F0000h – 0FFFFFh to be RW (0x30)?
> static void sdram_set_spd_registers(const struct mem_controller *ctrl)
>  {
>      /* TODO: Don't hardcode the values here, get info via SPD. */
> //set PAM maps
> pci_write_config8(ctrl->d0, PAM0, 0x30);
> pci_write_config8(ctrl->d0, PAM1, 0x33);
> pci_write_config8(ctrl->d0, PAM2, 0x33);
> pci_write_config8(ctrl->d0, PAM3, 0x33);
> pci_write_config8(ctrl->d0, PAM4, 0x33);
> pci_write_config8(ctrl->d0, PAM5, 0);
> pci_write_config8(ctrl->d0, PAM6, 0);

PAM registers are getting set twice? Please only set them in one
function, or at least keep consistent
> /* TODO: Set DRB0-DRB7. */
>
> pci_write_config8(ctrl->d0, DRB0, 0x08);
>
> pci_write_config8(ctrl->d0, DRB1, 0x08);
>
> pci_write_config8(ctrl->d0, DRB2, 0x08);
>
> pci_write_config8(ctrl->d0, DRB3, 0x08);
>
> pci_write_config8(ctrl->d0, DRB4, 0x08);
>
> pci_write_config8(ctrl->d0, DRB5, 0x08);
>
> pci_write_config8(ctrl->d0, DRB6, 0x08);
>
> pci_write_config8(ctrl->d0, DRB7, 0x08);
>
> /* TODO: Set DRAMC. Don't enable refresh for now. */
>    pci_write_config8(ctrl->d0, DRAMC, 0x08);
>
> /* TODO: Set RPS. */
>
> pci_write_config16(ctrl->d0, RPS, 0x0001);
>
> pci_write_config16(ctrl->d0, RPS, 0x0000);
>
> /* TODO: Set SDRAMC. */
> // pci_write_config16(ctrl->d0, SDRAMC, 0x0000);
>
> /* TODO: Set PGPOL. */
> pci_write_config16(ctrl->d0, PGPOL, 0x0107);
>
> pci_write_config16(ctrl->d0, PGPOL, 0xff00);
>
> /* TODO: Set NBXCFG. */
> // pci_write_config32(ctrl->d0, NBXCFG, 0x0100220c);
>
> pci_write_config32(ctrl->d0, NBXCFG, 0xff00a00c);
>
> /* TODO: Set RPS. */
>
> pci_write_config8(ctrl->d0, SMRAM, 0x8);
>
> //pci_write_config8(ctrl->d0,ATTBASE,0x3);
>
> /* TODO: Set PMCR? */
>    // pci_write_config8(ctrl->d0, PMCR, 0x14);
>    // pci_write_config8(ctrl->d0, PMCR, 0x10);
>
> /* TODO? */
>    // pci_write_config8(ctrl->d0, MLT, 0x40);
> // pci_write_config8(ctrl->d0, DRAMT, 0x03);
> //pci_write_config8(ctrl->d0, DRAMT, 0x03); 
> // pci_write_config8(ctrl->d0, MBSC, 0x03);
> // pci_write_config8(ctrl->d0, SCRR, 0x38);
>
> }

Here you're changing a bunch of register values that might be working
for other people working on the real 440bx. Please document (in
comments) why you are changing them and what they do, and possibly the
old values as well. Do we need to create some new files or functions to
separate the "real" 440bx from the vmware? Should we anyways and merge
the two back together later?

-Corey

[Ceri Coburn]

See comments below.
> -----Original Message-----
> From: Corey Osgood [mailto:[e-mail]]
> Sent: 08 May 2007 21:26
> To: Ceri Coburn
> Cc: [e-mail]
> Subject: Re: [LinuxBIOS] [PATCH] SIO PC87338 support, 440bx bug fix,
> VMWare mainboard support
>
> Just a few comments:
>
> Ceri Coburn wrote:
> > Added initial support for the national semiconductor's PC87338 Super
>
> I/O
> >
>
> IMO, 1 patch
> > Added initial support for VMWare workstation.
>
> another patch
> > Fixed bug in northbridge.c for the 440bx - calculating top of RAM was
> > incorrect
> >
> > Separated DRB register setting into separate function
> >
> > Now setting PAM registers for 440bx to exclude majority of BIOS area
>
> third patch. I think these patches also break tyan s1846, please check.
> Try to avoid breaking that board, as others folks are working on it and
> the 440bx

No problem, I already asked Stefan about this, and he recommended the same.
One thing though, some of the patches (SIO for instance) are required for
the others to function.  Should I just split them up but say that they
require the previous?
> > Index: src/superio/nsc/pc87338/pc87338.h
> >
> > --- src/superio/nsc/pc87338/pc87338.h    (revision 0)
> > +++ src/superio/nsc/pc87338/pc87338.h    (revision 0)
> > @@ -0,0 +1,32 @@
> > +/*
> > This file is part of the LinuxBIOS project.
> > Copyright (C) 2006 Uwe Hermann
> > Index: src/superio/nsc/pc87338/Config.lb
> >
> > --- src/superio/nsc/pc87338/Config.lb    (revision 0)
> > +++ src/superio/nsc/pc87338/Config.lb    (revision 0)
> > @@ -0,0 +1,23 @@
> > +##
> > +## This file is part of the LinuxBIOS project.
> > +##
> > +## Copyright (C) 2006 Uwe Hermann 
> > +##
>
> Why Uwe and not yourself? These are trivial files, just remove uwe and
> add yourself.

Well some of the files where copied and modified from work that Uwe had
done, so I kept the original persons and added myself. Do you recommend I
just include myself, or leave it as it is?
> > Property changes on: src/superio/nsc/pc87338/Config.lb
> > ___________________________________________________________________
> > Name: svn:executable
>
> Not necessary for any of these files, please fix.
> > +/* Special values used for entering MB PnP mode. The first four
>
> bytes of
> > each line determine the address port, the last four are data. */
> >
> > +static const uint8_t init_values[] = {
> > 0x6A, 0xB5, 0xDA, 0xED, 0xF6, 0xFB, 0x7D, 0xBE,
> > 0xDF, 0x6F, 0x37, 0x1B, 0x0D, 0x86, 0xC3, 0x61,
> > 0xB0, 0x58, 0x2C, 0x16, 0x8B, 0x45, 0xA2, 0xD1,
> > 0xE8, 0x74, 0x3A, 0x9D, 0xCE, 0xE7, 0x00, 0x00
> >
> > +};
>
> Are these ALL really necessary? Is it just me, or are they not even
> used?

These have been added for good reason, if the SIO chip was to run in PnP
mode then this part would be required, but VMWare seems to use a fixed
address of 0x2e for it.  I have left it in there just in case someone else
does further work on the PC87338 and has access to another mb that uses PnP
mode for the address.
> > /* Sequentially write the 32 special PnP values. */
> > //for (i = 0; i < 32; i++) {
> > //    outb(init_values[i], NSC87338_CONFIGURATION_PORT);
> > //}
> > ///* Write out our INDEX/BASE address after the PnP special
>
> values */
> > //outb( (SIO_BASE & 0xff00) >> 8, NSC87338_CONFIGURATION_PORT);
> > //outb(  SIO_BASE & 0xff , NSC87338_CONFIGURATION_PORT);
> > /* Set th address of SSC1 */
> > //pc87338_sio_write(NSC87338_CONFIG_REG_S1BAH,(iobase & 0xff00)
>
> << 8);
> > //pc87338_sio_write(NSC87338_CONFIG_REG_S1BAL, iobase & 0xff);
> > /* Enable PP, SSC1, SSC2, FDC, FDC Four-Drive encoding */
>
> Please avoid including commented code in patches.
>
> A few of your files (failover.c, etc) still need GPL headers.

No problem, I can add the GPL headers to them
> > +static void enable_shadow_ram(void)
> > +{
> > device_t dev = 0; /* no need to look up 0:0.0 */
> > unsigned char shadowreg;
> > /* dev 0 for southbridge */
> > shadowreg = pci_read_config8(dev, 0x63);
> > /* 0xf0000-0xfffff */
> > shadowreg |= 0x30;
> > pci_write_config8(dev, 0x63, shadowreg);
> >
> > +}
>
> This function is incorrect, it's a result of the code copy from via
> epia. On 440bx, register 0x63 is actually a DRB, so setting it here
> might seriously screw things up, unless it's reset during sdram_init.
> Here's a correct version:
>
> static void enable_shadow_ram(void)
> {
>    uint8_t shadowreg = pci_read_config8(0, 0x59);
>    shadowreg |= 0x30;
>    pci_write_config8(0, 0x59, shadowreg);
> }

Yea, my bad.  I think what happened here is that I copied from a board that
was supposed to be using the 440bx, but that board was actually code copied
from the via epia.  I will make the change accordingly.
> > Index: src/mainboard/emulation/vmware/mainboard.c
> >
> > --- src/mainboard/emulation/vmware/mainboard.c    (revision 0)
> > +++ src/mainboard/emulation/vmware/mainboard.c    (revision 0)
> > @@ -0,0 +1,40 @@
> > +/*
> > This file is part of the LinuxBIOS project.
> > Copyright (C) 2007 Ceri Coburn
>
> just one empty line here, please

Will do
> > 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
> > */
> >
> > +#include <device/device.h>
> > +#include "chip.h"
> > +#include <pc80/keyboard.h>
> > +#include <console/console.h>
> >
> > +static void vmware_init(struct device* dev)
> > +{
> > printk_debug("Initializing VMWare keyboard\n");
> > init_pc_keyboard(0x60,0x64,0);
> >
> > +}
> >
> > +struct chip_operations mainboard_emulation_vmware_ops = {
> > CHIP_NAME("VMWare Workstation")
> > .enable_dev = vmware_init
> >
> > +};
>
> Very annoying whitespace issues here!
> > Index: src/northbridge/intel/i440bx/raminit.c
> >
> > ---src/northbridge/intel/i440bx/raminit.c    (revision 2639)
> > +++ src/northbridge/intel/i440bx/raminit.c    (working copy)
> > @@ -181,13 +181,13 @@
> >       * 11 = Read/Write (all access goes to DRAM)
> >       */
> >      // TODO
> > PAM0, 0x00000000, 0x00,
> > PAM1, 0x00000000, 0x00,
> > PAM2, 0x00000000, 0x00,
> > PAM0, 0x00000000, 0x10,
>
> Why set PAM0 to 0x10? Don't we want 0F0000h - 0FFFFFh to be RW (0x30)?
> > static void sdram_set_spd_registers(const struct mem_controller
>
> *ctrl)
> > {
> >      /* TODO: Don't hardcode the values here, get info via SPD. */
> > //set PAM maps
> > pci_write_config8(ctrl->d0, PAM0, 0x30);
> > pci_write_config8(ctrl->d0, PAM1, 0x33);
> > pci_write_config8(ctrl->d0, PAM2, 0x33);
> > pci_write_config8(ctrl->d0, PAM3, 0x33);
> > pci_write_config8(ctrl->d0, PAM4, 0x33);
> > pci_write_config8(ctrl->d0, PAM5, 0);
> > pci_write_config8(ctrl->d0, PAM6, 0);
>
> PAM registers are getting set twice? Please only set them in one
> function, or at least keep consistent
> > /* TODO: Set DRB0-DRB7. */
> >
> > pci_write_config8(ctrl->d0, DRB0, 0x08);
> >
> > pci_write_config8(ctrl->d0, DRB1, 0x08);
> >
> > pci_write_config8(ctrl->d0, DRB2, 0x08);
> >
> > pci_write_config8(ctrl->d0, DRB3, 0x08);
> >
> > pci_write_config8(ctrl->d0, DRB4, 0x08);
> >
> > pci_write_config8(ctrl->d0, DRB5, 0x08);
> >
> > pci_write_config8(ctrl->d0, DRB6, 0x08);
> >
> > pci_write_config8(ctrl->d0, DRB7, 0x08);
> >
> > /* TODO: Set DRAMC. Don't enable refresh for now. */
> >    pci_write_config8(ctrl->d0, DRAMC, 0x08);
> >
> > /* TODO: Set RPS. */
> >
> > pci_write_config16(ctrl->d0, RPS, 0x0001);
> >
> > pci_write_config16(ctrl->d0, RPS, 0x0000);
> >
> > /* TODO: Set SDRAMC. */
> > // pci_write_config16(ctrl->d0, SDRAMC, 0x0000);
> >
> > /* TODO: Set PGPOL. */
> > pci_write_config16(ctrl->d0, PGPOL, 0x0107);
> >
> > pci_write_config16(ctrl->d0, PGPOL, 0xff00);
> >
> > /* TODO: Set NBXCFG. */
> > // pci_write_config32(ctrl->d0, NBXCFG, 0x0100220c);
> >
> > pci_write_config32(ctrl->d0, NBXCFG, 0xff00a00c);
> >
> > /* TODO: Set RPS. */
> >
> > pci_write_config8(ctrl->d0, SMRAM, 0x8);
> >
> > //pci_write_config8(ctrl->d0,ATTBASE,0x3);
> >
> > /* TODO: Set PMCR? */
> >    // pci_write_config8(ctrl->d0, PMCR, 0x14);
> >    // pci_write_config8(ctrl->d0, PMCR, 0x10);
> >
> > /* TODO? */
> >    // pci_write_config8(ctrl->d0, MLT, 0x40);
> > // pci_write_config8(ctrl->d0, DRAMT, 0x03);
> > //pci_write_config8(ctrl->d0, DRAMT, 0x03);
> > // pci_write_config8(ctrl->d0, MBSC, 0x03);
> > // pci_write_config8(ctrl->d0, SCRR, 0x38);
> >
> > }
>
> Here you're changing a bunch of register values that might be working
> for other people working on the real 440bx. Please document (in
> comments) why you are changing them and what they do, and possibly the
> old values as well. Do we need to create some new files or functions to
> separate the "real" 440bx from the vmware? Should we anyways and merge
> the two back together later?

Well these where values pulled from a VMWare machine that the VMWare BIOS
has set, so I made sure that mine matched to minimise problems getting it to
work, since all these registers had TODO comments on them, I assumed they
were not being set correctly at the moment anyway.  I will take a look at
them again and see if the VMWare machine will boot with the original values.

I don't think that we would require a separate 440bx implementation for
VMWare once the 440bx has been finalised.  The only requirement that I can
see that we need for the 440bx is that the DRB registers are not set at all
based off values calculated from SPD for VMWare, because SPD values for the
RAM doesn't seem to be emulated correctly.  The DRB's are pre populated
before the entry point to the BIOS is executed.  Perhaps what I can do is
add an extra function to the raminit.c for the 440bx to read the DRB
registers and set the appropriate DRB registers within the register_values
table, so this way they will be set like they normally are within the
sdram_set_registers function.  Then all that would be required is that the
VMWare auto.c calls this function before sdram_set_registers is called.
Real hardware mb's can call the SPD version of the function before
sdram_set_registers is called.  What do you think?

Ceri
> -Corey