[LinuxBIOS] [PATCH] Add K8T890CE support

Rudolf Marek r.marek at assembler.cz
Sun Oct 21 23:20:40 CEST 2007


>> ===================================================================
>> --- Config.lb	(revision 0)
>> +++ Config.lb	(revision 0)
> 
> Missing license header.

Fixed

> 
>> +static void host_enable(struct device *dev)
> 
> Would
> 
>   const struct device *dev
> 
> work? It looks like dev is not being modified(?)
> 
> 

Perhaps yes, but lets change that later.
> 
> One empty line only between functions.

fixed.

> 
> 
>> +void pcie_init(struct device *dev)
>> +{
>> +	u8 reg;
>> +
>> +	printk_debug("Configuring PCIe PEXs\n");
> 
> Is the "official" name PCIe or PCI-E?

Both.

> 
> Looks very similar to above. Does it make sense to merge it somehow?
> Probably not, but I figured I should ask nevertheless, just in case.

Well some bits are different. I will need to fix the init when Training fails so
this will be rewritten later.
>> +static struct pci_driver pcie_drvd3f3 __pci_driver = {
>> +	.ops = &pcie_ops,
>> +	.vendor = PCI_VENDOR_ID_VIA,
>> +	.device = PCI_DEVICE_ID_VIA_K8T890CE_PEX3,
> 
> Do you have the device ID for non-CE K8T890, too? Are they different?
> Would it make sense to add them, i.e. do you expect the code to
> (more or less) work in that case, too?

You mean for the CF? this IDs are 100% same.
> 
> Empty line not needed.

fixed.

> 
> One empty line only.
> 

fixed.


>> +	res->base = K8T890_APIC_BASE;
>> +	res->size = 256;
>> +	res->limit = res->base + res->size - 1;
>> +	res->align = 8;
>> +	res->gran = 8;
>> +	res->flags = IORESOURCE_MEM | IORESOURCE_FIXED |
>> +	    IORESOURCE_STORED | IORESOURCE_ASSIGNED;
>> +
>> +	/* Add a MMCONFIG resource */
>> +	res = new_resource(dev, K8T890_MMCONFIG_MBAR);
>> +	res->size = 256 * 1024 * 1024;
> 
> Why hardcoded? Is it configurable or _must_ it be hardcoded this way?
> (sorry if I already asked that)

What exactly? The BAR addr is computed dynamically.
>> + * Modified for K8T890 ROM strap by Rudolf Marek
> 
> Uh, "trivial" (well, let say "short" ;-) file, so (C) Rudolf Marek should
> suffice, IMHO.
> 

Ok. But I will leave it as it is ;)
>> +	dump_south(dev);
>> +	/* bit 4 is reserved but set by AW
>> +	   set PCI to HT outstanding requests to 3 */
>> +	pci_write_config8(dev, 0xa0, 0x13);
>> +
>> +	/* disable NVRAM and enable non-posted PCI writes */
>> +	pci_write_config8(dev, 0xa1, 0x8e);
>> +
>> +	/* nvram IO base 0xe00-0xeff but it is disabled 
>> +	   some bits are set and reserved */
>> +	pci_write_config8(dev, 0xa2, 0x0e);
>> +	/* Arbitration control, some bits are reserved */
> 
> Can you specify exactly which bits mean what and which are reversed? As
> ost of us will never see the datasheets, this is going to be important
> some day, I guess.

I have in plan to somehow document the code one day. "some bits reserved" means
the the some bits which are not documented are set to 1.
>> +	/* Update the desired HT LNK caps in NB too */
> 
> caps = capabilities?

Ok

>> +	pci_write_config8(dev, 0x80, 0xff);
>> +	pci_write_config8(dev, 0x81, 0xff);
>> +	pci_write_config8(dev, 0x82, 0xff);
>> +	pci_write_config8(dev, 0x83, 0x30);
> 
> Please list the registers and which bits mean what, so we can later
> modify if needed without requiring the datasheet.

Ok I tried to document it somehow.


>> +		regm3 = 0x0;
>> +	/* page F + Memhole copy */
> 
> "Shadow page F" ?

yep fixed.

> 
>> +	regm = pci_read_config8(devfun3, 0x83);
>> +	pci_write_config8(dev, 0x63, regm3 | (regm & 0x3F));
>> +}
>> +
>> +static struct device_operations ctrl_ops = {
>> +	.read_resources = pci_dev_read_resources,
>> +	.set_resources = pci_dev_set_resources,
>> +	.enable_resources = pci_dev_enable_resources,
>> +	.enable = ctrl_enable,
>> +	.ops_pci = 0,
>> +};
>> +
>> +static struct pci_driver northbridge_driver __pci_driver = {
>> +	.ops = &ctrl_ops,
>> +	.vendor = PCI_VENDOR_ID_VIA,
>> +	.device = PCI_DEVICE_ID_VIA_K8T890CE_7,
>> +};
>> Index: k8t890.h
>> ===================================================================
>> --- k8t890.h	(revision 0)
>> +++ k8t890.h	(revision 0)
>> @@ -0,0 +1,32 @@
>> +/*
>> + * This file is part of the LinuxBIOS project.
>> + *
>> + * Copyright (C) 2007 Rudolf Marek <r.marek at assembler.cz>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License v2 as published by
>> + * the Free Software Foundation.
>> + *
>> + * 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
>> + */
>> +
>> +#ifndef SOUTHBRIDGE_VIA_K8T890_H
>> +#define SOUTHBRIDGE_VIA_K8T890_H
> 
> SOUTHBRIDGE_VIA_K8T890_K8T890_H

fixed.

> 
> 
>> +
>> +/* static resources for K8T890 */
> 
> K8T890CE or all versions of it?

I support CE, CF should be very similar.

> 
> 
>> +#define K8T890_APIC_ID 0x3
>> +
>> +/* please check the datasheet and traf_ctrl_enable before change! */
>> +#define K8T890_APIC_BASE 0xfecc0000
> 
> Um, "please check datasheet" is not going to happen without NDA, so
> please explain it here.

Well it cant be changed to arbitrary address, so this is the reason why the reg 
is static.

> Cool stuff. With the above fixes this looks committable to me. If there
> are no further reviews or remarks, I'll commit later this evening or so.

Ok thanks, I'm attaching the fixed version for now. I will do some changes 
later, but all I need now is to have some consistent and working version.

Rudolf


-------------- next part --------------
A non-text attachment was scrubbed...
Name: k8t890_patch.patch
Type: text/x-patch
Size: 27554 bytes
Desc: not available
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20071021/d2c7fb04/attachment.patch>


More information about the coreboot mailing list