=================================================================== --- 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@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