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
--
linuxbios mailing list
linuxbios@linuxbios.org
http://www.linuxbios.org/mailman/listinfo/linuxbios
