On Sat, Oct 27, 2007 at 09:23:19PM +0200, Rudolf Marek wrote:
+config chip.h +driver vt8237r.o +driver vt8237r_ide.o +driver vt8237r_lpc.o +driver vt8237r_sata.o +driver vt8237r_bridge.o
UHCI? EHCI? AC97 audio / modem? LAN? What's the status there?
I have LAN disabled, cause it is not connected to any connector.
On your special board or on the SB in general?
AC97/UHCI/EHCI works, M97 should too. No settings necessary so far.
Possible, yes. I don't know if it wouldn't be better to at least "hook it up" via an ".enable = xyz_init" line (for each PCI ID)? Maybe some init is done by the generic code even if xyx_init is empty? I'm just guessing here, maybe someone else can say more?
I just recently noticed that it _does_ make a big difference whether the PCI ID lines are there, at least for the (ISA) bridge. If it's not listed (no matter if the function it calls is empty or not), the stuff "below" won't be initialized, in my case the superio with keyboard, com ports, parallel port and so on...
- /* address decoding */
- /* disable the BARs, we use std ports */
Why? Just curious...
I just dropped that.
The code or the comments? Why? Not needed?
+#define DIMM_BASE (0x50<<1)
Is this really needed in vt8237r_early_smbus.c? I don't think so. This address is dependent on the board anyway, correct?
hmm this is first DIMM address, which is allowed in general. No eeprom will occupy lower (except for the write protect control)
Hm, but you never use DIMM_BASE in the SB code. Why is it defined at all?
- /* actual addr to reg format */
- dimm = (dimm << 1);
- dimm |= 1;
- outb(dimm, SMBXMITADD);
- outb(offset, SMBHSTCMD);
- outb(0x48, SMBHSTCTL);
What is 0x48? What effect does it have (should be a comment)?
It tells to start the byte data transaction, whit start bit set.
OK, make that a comment please.
+struct southbridge_via_vt8237r_config {
- /* function enable bits, check vt8237r.c for details */
- unsigned int fn_ctrl_lo;
u16?
If it works then why not. I was not sure if I can use it here.
Probably needs to include arch/i386/include/stdint.h, so:
#include <stdint.h>
But I'm not so sure I like this anyway. It's ok for now, but these variables (ide0_enable etc) will become run-time changeable via CMOS settings in v3 (I hope!) so it's probably better to split this up per bit. I.e.
enable_ac97 enable_mc97 enable_rtc enable_lan etc.
- 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;
+};
- print_debug("B188 device dump\n");
B188?
it is the device ID. I dont know what it does....
Hm, ok.
ARRAY_SIZE (later).
do we have it in v2?
We have since r2901 :)
#include <stdlib.h>
- /* FIXME Map 4MB of FLASH into the address space,
this should be in CAR call */Am I reading this correctly? This SB supports 4MB ROM chips then?
Yes - FFFFFFFFh to FFC00000h. But the memory space it decodes it strapped. So perhaps for LB I should set in CAR to maximum value.
Btw, do we have flashrom support for VT8237R already? Does it work on your board?
- /* pci_write_config8(dev, 0x41, 0x7f); */
- /* Set bit 6 of 0x40, because Award does it (IO recovery time)
"because AWARD does it" no good...
Hmm this comment was not written by me, but this nbit should be set.
OK, then replace the comment with something meaningful please.
- /* enable HPET at: */
- pci_write_config32(dev, 0x68, (VT8237R_HPET_ADDR | 0x80));
Maybe make it configurable via an hpet_enable variable?
could be, but this is the default HPET address for all x86.
Yep. What I meant is to make it configurable whether to enable HPET or not (similar to ide0_enable etc).
Hm, this is superio stuff usually. As the VT8237R integrates these kinds of functions I'm not sure how to procede. Extract the Superio-related stuff into src/superio/via/vt8237r/* or leave it here? Opinions?
it has just RTC and keyboard. No other stuff.
Ah, ok. In that case that stuff is better placed here, indeed.
Uwe.