[LinuxBIOS] [PATCH] VIA VT8237R support

[Rudolf Marek]

> On your special board or on the SB in general?

Asus put Gigabit Ethernet chip. So only on my board.


> 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...

I have there the ISA bridge.


> 
> 
>>>> +	/* address decoding */
>>>> +	/* disable the BARs, we use std ports */
>>> Why? Just curious...
>> I just dropped that.
> 
> The code or the comments? Why? Not needed?

Whole stuff. It will get the ports from resource init.

> 
>>>> +#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?

Yes I noticed that too, and dropped. I think there was some debug stuff which 
was used by me or Corey.

>>>> +	/* 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.

I did

> 
> 
>>>> +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.
> 

Ok later.

> 
>>>> +	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.

Nope really :/

> 
> 
>>> ARRAY_SIZE (later).
>> do we have it in v2?
> 
> We have since r2901 :)
> 
> #include <stdlib.h>

ok.

>>>> +	/* 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?

Yes it does work.

debian:/mnt/disc4/Moje_dilna/LinuxBIOSv2/targets/asus/a8v-e_se/a8v-e_se# 
../../../../util/flashrom/flashrom  -v linuxbios.rom
Calibrating delay loop... ok
Found LinuxBIOS table at: 00000530
vendor id: ASUS part id: A8V-E SE
Found chipset "VT8237": Enabling flash write... OK.
Pm49FL004 found at physical address: 0xfff80000
Flash part is Pm49FL004 (512 KB)
Flash image seems to be a legacy BIOS. Disabling checks.
Verifying flash - VERIFIED

>>> +	/* 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.

ok

> 
> 
>>>> +	/* 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).

aha ok. I think we can leave it now as it is.

> 
> 
>>> 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.

Ok.

Rudolf