[coreboot] [patch] fix intel 82810 onboard VGA and SDRAM functions

Elia Yehuda z4ziggy at gmail.com
Wed May 13 13:04:00 CEST 2009


On Tue, May 12, 2009 at 4:57 PM, Uwe Hermann <uwe at hermann-uwe.de> wrote:
> Quick review below, I didn't yet find the time to test on hardware, will
> hopefully be able to do that today.
>
>
>> +     val = 0;
>> +     val = pci_read_config8(PCI_DEV(0, 0, 0), MISSC2);
>
> The 'val = 0' should no be needed if you do pci_read_config8()
> right after that.

agreed. its just a leftover. the 'val=0' can be removed.

>
>
>> +     val |= 0x06;
>> +
>> +     val |= 0xc6;
>
> Why this? Only the last line alone should also do?

same here - the 'val += 0x06' is a leftover from the original code. it
can be removed.

>
>
>> +#ifdef CONFIG_VIDEO_MB
>> +             /* check for VGA reserved memory
>> +             * possible CONFIG_VIDEO_MB values are 512(kb) and 1(mb)
>> +             */
>> +             if (CONFIG_VIDEO_MB == 512) {
>> +                     tomk -= 512;
>> +                     printk_debug("Allocating 512KB of RAM for VGA\n");
>> +             } else if (CONFIG_VIDEO_MB == 1) {
>> +                     tomk -= 1024 ;
>> +                     printk_debug("Allocating 1MB of RAM for VGA\n");
>
>> +             } else {
>> +                     /* assume no vga if incorrect value */
>> +                     tomk == tomk;
>
> Isn't this is no-op? Or am I missing something?

no, you're not missing a thing. i actually copied this from other
northbridge code and i liked
this no-op because its a cleaner code. from a programmer side of view,
it is redundant...

>
>
> Uwe.
> --
> http://www.hermann-uwe.de  | http://www.holsham-traders.de
> http://www.crazy-hacks.org | http://www.unmaintained-free-software.org
>

I hope you'll get a chance to play with the patch and maybe fix the
HIGH_TABLE issue.

Blessings,
Elia.




More information about the coreboot mailing list