[LinuxBIOS] [PATCH] VIA VT8237R support

Peter Stuge peter at stuge.se
Sat Oct 27 16:25:41 CEST 2007


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




More information about the coreboot mailing list