[coreboot] [ patch] v3 Geode graphics init.

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Thu Feb 21 03:27:34 CET 2008


On 21.02.2008 02:54, Marc Jones wrote:
>
>
> 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.

Great. Please do note that I forgot to mention the somewhat grim reality 
of 64bit machines with more than 4 GB of RAM where we have two different 
notions of top of RAM: The address where the 32bit hardware decoded 
space begins and the real top of RAM, which is not equal to the size of 
installed RAM due to the hole below 4 GB.


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

My comment above referred to your comment above in conjunction with your 
code below.


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

Sorry, I still don't understand although I'm really trying hard. Maybe 
my comment was misplaced and thus misunderstood.
The first ram_resource claims 0 - 640k.
The second ram_resource claims 1M - (systop-1M). Where's that hardcoded 
missing 1 MB at the top of RAM going?


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

Thanks for the info.

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

Suggestion:
Value in 7:0 looks like (video_mb & ~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.

Thanks.

One more complaint (probably a genuine bug):

> +u32 get_systop(struct northbridge_amd_geodelx_pci_config *nb_pci)
> +{
> +	struct gliutable *gl = NULL;
> +	u32 systop;
> +	struct msr msr;
> +	int i;
> +
> +	for (i = 0; gliu0table[i].desc_name != GL_END; i++) {
> +		if (gliu0table[i].desc_type == R_SYSMEM) {
> +			gl = &gliu0table[i];
> +			break;
> +		}
> +	}
> +	if (gl) {
> +		msr = rdmsr(gl->desc_name);
> +		systop = ((msr.hi & 0xFF) << 24) | ((msr.lo & 0xFFF00000) >> 8);
> +		systop += 4 * 1024;	/* 4K */
> +	} else {
> +		systop =

systop is the address of top of usable RAM in Bytes.

> +		    ((sizeram() - nb_pci->geode_video_mb) * 1024) - SMM_SIZE - 1024;

sizeram() and geode_video_mb are in MBytes, yet they only get multiplied by 1024.
A factor of 1024 is missing here.

> +	}
> +
> +	return systop;
> +}

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/





More information about the coreboot mailing list