[coreboot] [PATCH] v3: CN700 initram support

[Corey Osgood]

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?


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


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


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

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


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

Thanks,
Corey
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20081014/e78196f8/attachment.html>