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

Corey Osgood corey_osgood at verizon.net
Tue May 8 22:26:19 CEST 2007


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 <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.

> 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 <ceri.coburn at gmail.com> 
> + * 

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





More information about the coreboot mailing list