[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