[LinuxBIOS] [PATCH] VIA VT8237R support

Rudolf Marek r.marek at assembler.cz
Sat Oct 27 17:21:17 CEST 2007


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




More information about the coreboot mailing list