[coreboot] [PATCH] v3: CN700 initram support

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Wed Oct 15 00:29:56 CEST 2008


On 14.10.2008 21:40, Corey Osgood wrote:
> On Tue, Oct 14, 2008 at 6:32 AM, Carl-Daniel Hailfinger <
> c-d.hailfinger.devel.2006 at gmx.net> wrote:
>
>   
>> On 14.10.2008 06:21, Corey Osgood wrote:
>>
>>     
> <snip>
>
>   
>>> +/* Is this correct? */
>>> +static void read32(u32 addr)
>>> +{
>>> +     u32 val;
>>> +     volatile unsigned long *x;
>>> +     x = (void *)addr;
>>> +     val = *x;
>>> +}
>>>
>>>       
>> Can't you use readl() instead?
>>     
>
> <snip>
>
>   
>>> +                             read32(0x800);
>>>       
>
> By using readl(), I'm getting
>
> /home/corey/coreboot/coreboot-v3/northbridge/via/cn700/initram.c:69:
> warning: passing argument 1 of 'readl' makes pointer from integer without a
> cast
>
> Where the above line is 69, this pops up on every readl() call. I know it's
> probably something stupid I'm not doing, what is it?
>   

readl() expects a pointer, not an integer. AFAICS you could do
readl((void *)0x800);


>>> +                     } else {
>>> +                             read32(0x121c20);
>>> +                             read32(0x120020);
>>> +                     }
>>> +                     break;
>>>
>>>       
>> The logic above may be correct, but I have no way to figure it out.
>> Could you maybe use symbolic constants for 0x12000, 0x800, 0x121c20,
>> 01120020?
>>     
>
>
> These are MRS/EMRS addresses, the first values are for DLL reset, second is
> for OCD calibration, there's a comment that explains what each offset is for
> now.
>   

Great, thanks for doing this.


>>> +             case RAM_COMMAND_CBR:
>>> +                     for(j = 0; j < 8; j++) {
>>> +                             read32((reg8 << 26));
>>> +                             udelay(100);
>>> +                     }
>>> +                     printk(BIOS_SPEW, "'CBR' to 0x%x", addr_offset);
>>> +                     break;
>>> +     };
>>> +
>>> +     /* NOTE: Dual-sided and multi-dimm ready */
>>> +     read32(0 + addr_offset);
>>> +     for(i = 0; i < (ARRAY_SIZE(dev->spd_channel0) * 2); i++) {
>>> +             reg8 = pci_conf1_read_config8(dev->d0f3, RANK0_START + i);
>>> +             if(reg8) {
>>> +                     read32((reg8 << 26) + addr_offset);
>>> +                     printk(BIOS_SPEW, ", 0x%x", (reg8 << 26) +
>>>       
>> addr_offset);
>>     
>>> +                     if(command == RAM_COMMAND_MRS)
>>> +                     {
>>> +                             if(addr_offset == 0x12000)
>>> +                             {
>>> +                                     read32((reg8 << 26) + 0x800);
>>> +                             } else {
>>> +                                     read32((reg8 << 26) + 0x121c20);
>>> +                                     read32((reg8 << 26) + 0x120020);
>>>
>>>       
>> Same magic values as above.
>>     
>
>
> I didn't bother to re-comment these, it's the same as above.
>   

Sure.


>>> +                             }
>>> +                     } else if(command == RAM_COMMAND_CBR)
>>> +                     {
>>> +                             for(j = 0; j < 8; j++) {
>>> +                                     read32((reg8 << 26));
>>> +                                     udelay(100);
>>> +                             }
>>> +                     }
>>> +             }
>>> +     }
>>> +     printk(BIOS_SPEW, "\n");
>>> +}
>>> +
>>> +/**
>>> + * Configure the bus between the cpu and the northbridge. This might be
>>>       
>> able to
>>     
>>> + * be moved to post-ram code in the future. For the most part, these
>>>       
>> registers
>>     
>>> + * should not be messed around with. These are too complex to explain
>>>       
>> short of
>>     
>>> + * copying the datasheets into the comments, but most of these values
>>>       
>> are from
>>     
>>> + * the BIOS Porting Guide, so they should work on any board. If they
>>>       
>> don't,
>>     
>>> + * try the values from your factory BIOS.
>>>
>>>       
>> Hm. Good leading explanation. If you could add a pointer to which data
>> sheet sections mainly have this info, it would be nice. Your choice.
>> The inline comments are really readable and I like them very much.
>>     
>
>
> The only problem with that is Via has come out with several newer versions
> of the datasheet. If you were to get one from them today, it would likely be
> different section numbers then I've got.
>   

OK, works for me.


> <more snipping>
>
>   
>>> +     /* Set WR=5 and RFC */
>>> +     //pci_conf1_write_config8(dev->d0f3, 0x61, 0x94);//c7
>>>
>>>       
>> And here.
>>
>>     
>>> +     /* Set CAS=5 */
>>> +     //pci_conf1_write_config8(dev->d0f3, 0x62, 0x7a);//af
>>> +     //pci_conf1_write_config8(dev->d0f3, 0x63, 0x00);//ca
>>> +     //pci_conf1_write_config8(dev->d0f3, 0x64, 0x88);
>>>
>>>       
>> And here.
>>     
>
>
> These are "safe" DRAM timings, eventually I'll make a Kconfig option to
> enable them, in case the ones set by SPD turn out to be too tight.
>   

Could you add a comment which tells the reader that these are
failsafe/fallback settings?


>>> +     if(!spd_data || spd_data == 0xff) {
>>> +             printk(BIOS_DEBUG, "No memory in slot %d\n", i);
>>> +             return 0;
>>> +     }
>>> +     else if(spd_data >= 0x10)
>>> +             spd_data = spd_data >> 4;
>>> +     else
>>> +             spd_data = spd_data << 4;
>>>
>>>       
>> Hm. Are you trying to just flip the nibbles or are you selectively
>> picking one nibble? For the former, try this:
>> spd_data = (spd_data >> 4) | (spd_data << 4) & 0xff;
>>     
>
>
> That should work, I'll try it out just to be sure.
>   

Of course, an 8-bit rotate() function would be the thing we want, but I
doubt we're going to add that to our math library.

Regards
Carl-Daniel

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





More information about the coreboot mailing list