[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