Marc Jones wrote:
> 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.
>>>>>
>>>
...
>>>>> @@ -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)
>
>
>
There is a bug here. get_systop returns KB. It is already in KB so it
doesn't need the /1024 and the 1MB doesn't need * 1024.
The correct line is :
ram_resource(dev, idx++, 1024,
(get_systop(nb_pci) - 1024);
I will check v2.
>>
>>> +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.
>
>
The bug here isn't MB->KB. get_systop returns KB. The bug is the
additional 1024 taken off the end.
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
