See comments below.
-----Original Message----- From: Corey Osgood [mailto:corey_osgood@verizon.net] Sent: 08 May 2007 21:26 To: Ceri Coburn Cc: linuxbios@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@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@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@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