[coreboot] [ patch] v3 Geode graphics init.

[Marc Jones]

Carl-Daniel Hailfinger wrote:
> On 21.02.2008 01:09, Marc Jones wrote:
>> This patch assumes the vsa/idt patch is in place.
> Comments inline.
> 
>> Removed CONFIG_VIDE0_MB in Kconfig and set the Geode
>> graphics memory size through the Geode northbridge pci dts.
>> This is a better way to handle a cpu/mainboard specific value.
>>
>> Signed-off-by: Marc Jones <marc.jones at amd.com>
>>
>> Index: coreboot-v3/northbridge/amd/geodelx/geodelx.c
>> ===================================================================
>> --- coreboot-v3.orig/northbridge/amd/geodelx/geodelx.c    2008-02-20 
>> 10:07:27.000000000 -0700

>> +extern void graphics_init(struct northbridge_amd_geodelx_pci_config 
>> *nb_pci);
>> +extern int sizeram(void);
> 
> Do we want sizeram() as a function on all subarches? If yes, we have
> to either call it something like sizeram_4G() or we have to change the
> return type to u64.
>

I don't know if any other subarchs will implement this but I like the 
idea. I will make it u64.

...


>> @@ -127,7 +162,7 @@
>>          ram_resource(dev, idx++, 0, 640);
>>          /* 1 MB .. (Systop - 1 MB) (converted to KB) */
> 
> You subtract only 1 MB here.
> 
>>          ram_resource(dev, idx++, 1024,
>> -                 (get_systop() - (1 * 1024 * 1024)) / 1024);
>> +                 (get_systop(nb_pci) - (1 * 1024 * 1024)) / 1024);
>>      }
>>   

As the comment say. 0-640K is claimed prior to the 1MB to systop.


>> +
>> +    /* SoftVG initialization */
> 
> That's scary. Do we really need VSA support even if there is a native
> geodelx framebuffer/X driver?
> 
Not scary. Geode needs VSA for the PCI headers. VSA needs to know how 
much memory is claimed to report it in the BAR.

...

>> +    /*
>> +     * Graphics Driver Enabled (13)            0, NO (lets BIOS 
>> controls the GP)
>> +     * External Monochrome Card Support(12)        0, NO
>> +     * Controller Priority Select(11)        1, Primary
>> +     * Display Select(10:8)                0x0, CRT
>> +     * Graphics Memory Size(7:1)            CONFIG_VIDEO_MB >> 1,
>> +     *    defined in mainboard/../dts
>> +     * PLL Reference Clock Bypass(0)        0, Default
>> +     */
>> +
>> +    /* Video RAM has to be given in 2MB chunks
>> +     *   the value is read @ 7:1 (value in 7:0 looks like /2)
> 
> Typo?
> 
> + * the value is read @ 7:1 (value in 7:0 looks like *2)
> 
> Besides that, being limited to multiples of 2MB for VRAM seems not to mix
> well with subtracting only 1MB from systop.
> 
> 

Maybe that needs a better explanation. Bit 0 is used but video_mb is not 
shifted. Or you could look at it as video_mb/2 << 1.


...

>> +#ifndef GEODELINK_H
>> +#define GEODELINK_H
>> +
>> +struct gliutable {
>> +    unsigned long desc_name;
>> +    unsigned short desc_type;
>> +    unsigned long hi, lo;
>> +};
>> +
>> +static struct gliutable gliu0table[] = {
>> +    /* 0-7FFFF to MC */
>> +    {.desc_name = MSR_GLIU0_BASE1,.desc_type = BM,.hi = MSR_MC + 0x0,
>> +     .lo = 0x0FFF80},
>> +    /* 80000-9FFFF to MC */
>> +    {.desc_name = MSR_GLIU0_BASE2,.desc_type = BM,.hi = MSR_MC + 0x0,
>> +     .lo = (0x80 << 20) + 0x0FFFE0},
>> +    /* C0000-FFFFF split to MC and PCI (sub decode) A0000-BFFFF
>> +     * handled by SoftVideo.
>> +     */
>> +    {.desc_name = MSR_GLIU0_SHADOW,.desc_type = SC_SHADOW,
>> +     .hi = MSR_MC + 0x0,.lo = 0x03},
>> +    /* Catch and fix dynamically. */
>> +    {.desc_name = MSR_GLIU0_SYSMEM,.desc_type = R_SYSMEM,
>> +     .hi = MSR_MC,.lo = 0x0},
>> +    /* Catch and fix dynamically. */
>> +    {.desc_name = MSR_GLIU0_SMM,.desc_type = BMO_SMM,
>> +     .hi = MSR_MC,.lo = 0x0},
>> +    {.desc_name = GLIU0_GLD_MSR_COH,.desc_type = OTHER,
>> +     .hi = 0x0,.lo = GL0_CPU},
>> +    {.desc_name = GL_END,.desc_type = GL_END,.hi = 0x0,.lo = 0x0},
>> +};
>> +
>> +static struct gliutable gliu1table[] = {
>> +    /* 0-7FFFF to MC */
>> +    {.desc_name = MSR_GLIU1_BASE1,.desc_type = BM,.hi = MSR_GL0 + 0x0,
>> +     .lo = 0x0FFF80},
>> +    /* 80000-9FFFF to MC */
>> +    {.desc_name = MSR_GLIU1_BASE2,.desc_type = BM,.hi = MSR_GL0 + 0x0,
>> +     .lo = (0x80 << 20) + 0x0FFFE0},
>> +    /* C0000-Fffff split to MC and PCI (sub decode) */
>> +    {.desc_name = MSR_GLIU1_SHADOW,.desc_type = SC_SHADOW,
>> +     .hi = MSR_GL0 + 0x0,.lo = 0x03},
>> +    /* Catch and fix dynamically. */
>> +    {.desc_name = MSR_GLIU1_SYSMEM,.desc_type = R_SYSMEM,
>> +     .hi = MSR_GL0,.lo = 0x0},
>> +    /* Catch and fix dynamically. */
>> +    {.desc_name = MSR_GLIU1_SMM,.desc_type = BM_SMM,
>> +     .hi = MSR_GL0,.lo = 0x0},
>> +    {.desc_name = GLIU1_GLD_MSR_COH,.desc_type = OTHER,
>> +     .hi = 0x0,.lo = GL1_GLIU0},
>> +    /* FooGlue FPU 0xF0 */
>> +    {.desc_name = MSR_GLIU1_FPU_TRAP,.desc_type = SCIO,
>> +     .hi = (GL1_GLCP << 29) + 0x0,.lo = 0x033000F0},
>> +    {.desc_name = GL_END,.desc_type = GL_END,.hi = 0x0,.lo = 0x0},
>> +};
> 
> It's debatable whether gliu0table and gliu1table should really be in a
> header file as opposed to a normal .c code file. This max cause additional
> warnings from gcc.
> If any of the tables are constant, please mark them as const.

These need to be marked const. I will think about if this is the right 
way to do this.

...

> 
> With my comments addressed, the patch is
> Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>
> 
> Regards,
> Carl-Daniel
> 


Thanks, I'll work on these things.
Marc

-- 
Marc Jones
Senior Firmware Engineer
(970) 226-9684 Office
mailto:Marc.Jones at amd.com
http://www.amd.com/embeddedprocessors