On Sat, Oct 27, 2007 at 01:16:49AM +0200, Rudolf Marek wrote:
> +/*
> + * Datasheets: http://www.via.com.tw/en/downloads/datasheets/chipsets/
> + *
> VT8237R_SouthBridge_Revision2.06_Lead-Free.zip
> +*/
The second line is very long.
> +static void vt8237r_enable(struct device *dev)
> +{
> + struct southbridge_via_vt8237r_config *sb =
> + (struct southbridge_via_vt8237r_config *) dev->chip_info;
> +
> + /* function disable 1=disabled
> + 7 Dev 17 fn 6 MC97
> + 6 Dev 17 fn 5 AC97
> + 5 Dev 16 fn 1 USB 1.1 UHCI Ports 2-3
> + 4 Dev 16 fn 0 USB 1.1 UHCI Ports 0-1
> + 3 Dev 15 fn 0 Serial ATA
> + 2 Dev 16 fn 2 USB 1.1 UHCI Ports 4-5
> + 1 Dev 16 fn 4 USB 2.0 EHCI
> + 0 Dev 16 fn 3 USB 1.1 UHCI Ports 6-7
> + */
> +
> + pci_write_config8(dev, 0x50, sb->fn_ctrl_lo);
> +
> + /*
> + 7 USB Device Mode 1=dis
> + 6 Reserved
> + 5 Internal LAN Controller Clock Gating 1=gated
> + 4 Internal LAN Controller 1=di
> + 3 Internal RTC 1=en
> + 2 Internal PS2 Mouse 1=en
> + 1 Internal KBC Configuration 0=dis ports 0x2e/0x2f off 0xe0-0xef
> + 0 Internal Keyboard Controller 1=en
> + */
> +
> + pci_write_config8(dev, 0x51, sb->fn_ctrl_hi);
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.
> + /* TODO: if SATA is disabled, move IDE to fn0 to conform PCI specs */
How is that done?
> + 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?)
> + printk_debug("enables in reg 0x40 read back as 0x%x\n", enables);
I want 0x%02x but that may just be me.
> + /* 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.
> +#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.
> +struct southbridge_via_vt8237r_config {
> +
> + /* function enable bits, check vt8237r.c for details */
> + unsigned int fn_ctrl_lo;
> + unsigned int fn_ctrl_hi;
> + int ide0_enable:1;
> + int ide1_enable:1;
> + /* 1 = 80-pin cable */
> + int ide0_cable:1;
> + int ide1_cable:1;
Please improve the _cable name. ide0_80pincable or something. No need
for comment then, and more clear code. :)
> +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?
> +static struct ioapicreg ioapicregvalues[] = {
..
> + {23, DISABLED, NONE},
> + /* Be careful and don't write past the end... */
Please clarify what this comment means.
> + 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?
Is the IO APIC memory mapped or how does the query/response work?
> +#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?
Looks good otherwise!
//Peter
--
linuxbios mailing list
linuxbios@linuxbios.org
http://www.linuxbios.org/mailman/listinfo/linuxbios
