Hi Peter,
The second line is very long.
Ok.
I think these should be a doxygen comment around the declaration of the fn_ctrl_lo/hi members in src/southbridge/via/vt8237r/chip.h.
Ok.
- /* TODO: if SATA is disabled, move IDE to fn0 to conform PCI specs */
How is that done?
Some bits in chipset.
- printk_info("%s IDE interface %s\n", "Primary",
sb->ide0_enable ? "enabled" : "disabled");- printk_info("%s IDE interface %s\n", "Secondary",
sb->ide1_enable ? "enabled" : "disabled");Looks like size-optimizing? Are you sure this is good? It's a little more difficult to read. (Might just be copypaste and not your idea?)
copypaste ;)
- printk_debug("enables in reg 0x40 read back as 0x%x\n", enables);
I want 0x%02x but that may just be me.
yep good idea.
- /* standard bios sets master bit. */
- enables = pci_read_config8(dev, PCI_COMMAND);
- enables |= PCI_COMMAND_MASTER | PCI_COMMAND_IO;
Discussion please? Try to find out why this is a good idea. Is this enabling bus mastering? Why would that be a bad idea? Please try to not use factory BIOS as justification until we've run out of ideas completely.
Well the older VIA driver in LB does it too. I think linux will switch bus master when needed. I think we can drop this.
+#if DEBUG_SMBUS == 1 +#define PRINT_DEBUG(x) print_debug(x) +#define PRINT_DEBUG_HEX16(x) print_debug_hex16(x) +#else +#define PRINT_DEBUG(x) +#define PRINT_DEBUG_HEX16(x) +#endif
..
- PRINT_DEBUG("After reset status: ");
- PRINT_DEBUG_HEX16(inb(SMBHSTSTAT));
- PRINT_DEBUG("\r\n");
+}
+/* Public functions */ +u8 smbus_read_byte(u32 dimm, u32 offset) +{
- u32 val;
- print_debug("DIMM ");
- print_debug_hex8(dimm);
- print_debug(" OFFSET ");
- print_debug_hex8(offset);
- print_debug("\r\n");
..this is confusing. Are macros case insensitive? Please unify this a little.
This is because most of file can switch debug off but here it is wrong. I will fix.
Please improve the _cable name. ide0_80pincable or something. No need for comment then, and more clear code. :)
Ok.
+static void br_enable(struct device *dev) +{
- print_debug("B188 device dump\n");
- /* VIA recommends this, sorry no known info */
- writeback(dev, 0x40, 0x91);
- writeback(dev, 0x41, 0x40);
- writeback(dev, 0x43, 0x44);
- writeback(dev, 0x44, 0x31);
- writeback(dev, 0x45, 0x3a);
- writeback(dev, 0x46, 0x88); /* PCI ID lo */
- writeback(dev, 0x47, 0xb1); /* PCI ID hi */
- writeback(dev, 0x3e, 0x16); /* bridge control */
- dump_south(dev);
+}
Ok. Can you add a reference? Maybe a page or section in the data sheet?
I can't. I have no information on this of any kind. Except those values.
+static struct ioapicreg ioapicregvalues[] = {
..
- {23, DISABLED, NONE},
- /* Be careful and don't write past the end... */
Please clarify what this comment means.
Well this is cut and paste and it means that you may get into trouble if you go past the address space.
if ((i == 0) && (value_low == 0xffffffff)) {printk_warning("IO APIC not responding.\n");How does this work? The code seems to just do some memory accesses?
yes
Is the IO APIC memory mapped or how does the query/response work?
You have there two pair of regs. Addr and data. Perhaps if you read back 0xffffffff you get in trouble... This is also from other VIA SB code.
+#define PCI_DEVICE_ID_VIA_K8T890CE_0 0x0238 +#define PCI_DEVICE_ID_VIA_K8T890CE_1 0x1238 +#define PCI_DEVICE_ID_VIA_K8T890CE_2 0x2238 +#define PCI_DEVICE_ID_VIA_K8T890CE_3 0x3238 +#define PCI_DEVICE_ID_VIA_K8T890CE_4 0x4238 +#define PCI_DEVICE_ID_VIA_K8T890CE_5 0x5238 +#define PCI_DEVICE_ID_VIA_K8T890CE_7 0x7238 +#define PCI_DEVICE_ID_VIA_K8T890CE_PEG 0xa238 +#define PCI_DEVICE_ID_VIA_K8T890CE_PEX0 0xc238 +#define PCI_DEVICE_ID_VIA_K8T890CE_PEX1 0xd238 +#define PCI_DEVICE_ID_VIA_K8T890CE_PEX2 0xe238 +#define PCI_DEVICE_ID_VIA_K8T890CE_PEX3 0xf238 +#define PCI_DEVICE_ID_VIA_K8T890CE_BR 0xb188
Do these really go in this patch?
Well I forgot last time ;) But some of those belong here.
I will try to fix this and generate new patch.
Looks good otherwise!
Good to hear, I spent quite a lot of time fixing it ;)
Rudolf