[coreboot] [ patch] v3 Geode graphics init.

[Carl-Daniel Hailfinger]

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.


>>>> +    } 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.
>   

And the new bug I discovered above.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/