Carl-Daniel Hailfinger wrote:
> 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 <[EMAIL PROTECTED]>
>>>>
>>>> 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.
>
>
Right, this will give you the size of RAM in this case for Geode. Other
architectures would have to do the right thing.
>> ...
>>
>>
>>>> @@ -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?
>
>
systop-1M is the size.
static void ram_resource(device_t dev, unsigned long index,
unsigned long basek, unsigned long sizek)
>>>> +
>>>> + /* 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)
>
>
ok.
>> ...
>>
>>>> +#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.
Real bug. Fortunately we never take that path. I will fix it in this
patch for v3 and submit a patch for v2.
Thanks,
Marc
--
Marc Jones
Senior Firmware Engineer
(970) 226-9684 Office
mailto:[EMAIL PROTECTED]
http://www.amd.com/embeddedprocessors
--
coreboot mailing list
coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot
