[coreboot] [ patch] v3 Geode graphics init.

Marc Jones Marc.Jones at AMD.com
Fri Feb 22 00:42:07 CET 2008


Carl-Daniel Hailfinger wrote:
> On 21.02.2008 23:06, Marc Jones wrote:
>> 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:
>>>>>>             
>>>>>>>          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.
>>   
>
> Thanks.
>
>
>>>>> +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 */
>>>>>         
>
> If get_systop really returns kBytes, the comment above (/* 4K */) does 
> not match the code which adds 4*1024 K = 4 M.
Argh! This is all messed up. Something happened from v2 to v3. 
get_systop should return bytes. Not KB. I'll get this sorted out.

Marc





-- 
Marc Jones
Senior Firmware Engineer
(970) 226-9684 Office
mailto:Marc.Jones at amd.com
http://www.amd.com/embeddedprocessors 






More information about the coreboot mailing list