On 14.10.2008 21:40, Corey Osgood wrote: > On Tue, Oct 14, 2008 at 6:32 AM, Carl-Daniel Hailfinger < > [EMAIL PROTECTED]> 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/ -- coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot