[LinuxBIOS] [PATCH] SIO PC87338 support, 440bx bug fix, VMWare mainboard support
Ceri Coburn
ceri.coburn at googlemail.com
Wed May 9 11:41:32 CEST 2007
See comments below.
> -----Original Message-----
> From: Corey Osgood [mailto:corey_osgood at verizon.net]
> Sent: 08 May 2007 21:26
> To: Ceri Coburn
> Cc: linuxbios at linuxbios.org
> 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 <uwe at hermann-uwe.de>
> > + *
>
> > 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 <uwe at hermann-uwe.de>
> > +##
> > +
>
> 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 <ceri.coburn at gmail.com>
> > + *
>
> 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
More information about the coreboot
mailing list