Re: [LinuxBIOS] [PATCH] VIA VT8237R support

[Peter Stuge]

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