[coreboot] [ patch] v3 Geode graphics init.

[Marc Jones]

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:Marc.Jones at amd.com
http://www.amd.com/embeddedprocessors