On Mon, Nov 17, 2008 at 9:49 AM, Elia Yehuda z4ziggy@gmail.com wrote:
On Mon, Nov 17, 2008 at 3:58 PM, Stefan Reinauer stepan@coresystems.dewrote:
Elia Yehuda wrote:
Those 2 patches are one step towards having a working Onboard-VGA and 512MB (the max for i810). The raminit.c patch fixes some misconfigurations and probes the DIMMs correctly (all dual-sided are now recognized properly), and also set buffer_strength to handle 2 DIMMs although atm this doesn't work. The i82810/northbridge.c patch takes care of allocating Onboard-VGA memory.
Signed-off-by: Elia Yehuda <z4ziggy@gmail.com <mailto:z4ziggy@gmail.com
Good work!
thanks :)
But no ack yet, because it's ongoing work that also breaks some (potentially) working cases. I have a couple of comments though
for (i = 0; i < DIMM_SOCKETS ; i++) {dimm_size = smbus_read_byte(ctrl->channel0[i], 31);dimm_banks = smbus_read_byte(ctrl->channel0[i], 5);if (dimm_size) {Can this be changed to read the size and banks values from the PCI registers or variables instead of from the smbus? It's a lot of slow, unneeded bus traffic for every ram read. I don't know about the 810, but other Intel chipsets would get confused by that kind of bus traffic and delays at that point.
i had no idea using the smbus is slower. actually my initial hacking was using the PCI but it was a bit more complexed since i had to convert from intel's codes to MBs, and i had a bit of a problem detecting the dimm_bank properly - i will look into that again when i'll wake up...
IIRC there was a "magic table" that did the conversion.
* Rows of Slot 1 are numbed 2 through 3. The* DIMM’s SPD Byte 5 describes the number ofsides in a
* DIMM; SPD Byte 13 provides information onSome weird character sneaked into that comment.
the evil char is now gone...
);
uint8_t val;val = pci_read_config8(ctrl->d0, DRAMT);I suggest using PCI_DEV(0,0,0) instead of the indirection ctrl->d0 all over raminit.c.
That indirection was invented for AMD K8 where in an SMP environment there are several memory controllers with several DIMMs attached to each. But the i810 definitely only supports a single memory controller, and the code can be kept a lot simpler.
actually using ctrl->d0 looks cleaner in the code (rather than using PCI_DEV(0,0,0)) but sure, i'll change it...
if ((smbus_read_byte(ctrl->channel0[i], 127) |0xf) == 0xff) { Same here: Intel boards expect the SPD ROMs at fixed places, unlike the common case on Opteron boards.
will look into that.
Also, your code only treats single channel configurations. Is the i810 capable of dual channel operation?
the i810 supports 2 SDRAM DIMM slots, single and dual sided, up to 512MB total. i have no idea how to treat the 2nd slot, although ive read the datasheet top-to-bottom. any clues would be greatly appreciated...
Dual channel is a DDR thing? I picked up another i810 board a couple days ago, so I'll poke around with a second dimm if you can get one working ;) Please remid me, IIRC i810 can only have 512MB if it has 2 dual-sided 256MB memory sticks, it can't use single-sided 256MB sticks, right?
/* TODO: This needs to be set according to the DRAM tech
/* TODO: This needs to be calculated according to the DRAM techDon't think this can really be calculated. Looking at your findings below, you should probably use a table for this and look up the correct values from the table.
* (x8, x16, or x32). Argh, Intel provides no docs on this! * Currently, it needs to be pulled from the output of * lspci -xxx Rx92
* here are some common results:* value: tom: config:* 0x3356 256MB 1 x 256MB DIMM(s), dual sided* 0x0001 512MB 2 x 256MB DIMM(s), dual sided* 0x77da 128MB 1 x 128MB DIMM(s), single sided* 0x55c6 128MB 2 x 128MB DIMM(s), single sided* 0x3356 128MB 1 x 128MB DIMM(s), dual sided* 0x0001 256MB 2 x 128MB DIMM(s), dual sided */Because:
pci_write_config16(ctrl->d0, BUFF_SC, 0x77da);
/* use 2 dual sided DIMMs - this also works with only 1 DIMM */pci_write_config16(ctrl->d0, BUFF_SC, 0x0001);This fixes one case and breaks another one. Not really an improvement.
not really. using 0x0001 works fine for both 1 DIMM and for 2 DIMMs (currently my i810 is active, using 1 256MB DIMM, with 0x0001 setting). using the 0x77da is worse since it only works with 1 single-sided DIMM. after I'll fix the 2nd DIMM issue (ie, make it work!!) i'll deal with those small issues.
Cool! This is an improvement ;)
/* set the GMCH to prechange all during the service of a page miss*/
precha_r_ge?
yep. prechange. looks weird? talk to the intel ppl - i copied this line from the datasheet...
I'm fairly sure that's a typo in the datasheet, it should be precharge.
/* or we can use sane defaults, but VGA won't work for unknownreason */
//pci_write_config8(ctrl->d0, PAM, 0x41);This will prevent writes to the CSEG, will it? (Didn't check)
yep, from CSEG and FSEG. i tried putting this at the end of sdram_enable() but it always causes the VGA not to work, so i left it for now with the "insane" range for r/w. the original BIOS shows 0x41 at this offset, but i have no idea after which step does it set it to make the VGA still work. maybe im missing someting else?
Weird. Probably some part of the Video BIOS uses that range during init. I think a few K for working onboard VGA is a good tradeoff ;)
-Corey
/* setting Vendor/Device ids - there must be a better way of doing* this...*//* set vendor id */val = pci_read_config16(ctrl->d0, 0);pci_write_config16(ctrl->d0, 0x2c, val);/* set device id */val = pci_read_config16(ctrl->d0, 2);pci_write_config16(ctrl->d0, 0x2e, val);Yes, during pci_set_subsystem_ids in northbridge.c:
static void intel_set_subsystem(device_t dev, unsigned vendor, unsigned device) { u32 pci_id;
printk_debug("Setting PCI bridge subsystem ID\n"); pci_id = pci_read_config32(dev, 0); pci_write_config32(dev, PCI_SUBSYSTEM_VENDOR_ID, pci_id );}
static struct pci_operations intel_pci_ops = { .set_subsystem = intel_set_subsystem, };
static struct device_operations mc_ops = { [..] .ops_pci = &intel_pci_ops, };
many, many thanks :) i'm still learning the coreboot code (im just hacking around for about 2 weeks) and i figured there must be a better way of doing this, just couldnt figure out how...
/* 3. Perform 8 refresh cycles. Wait tRC each time. */ PRINT_DEBUG("RAM Enable 3: CBR\r\n");
do_ram_command(ctrl, RAM_COMMAND_CBR, 0, row_offset); for (i = 0; i < 8; i++) {
do_ram_command(ctrl, RAM_COMMAND_CBR, 0, row_offset);/* FIXME: are those read32() needed? */ read32(0); read32(row_offset); udelay(1);Probably not needed the way you're doing it now. The reads cause a refresh cycle on a given dram rank. do_ram_command does one such read. Since several are needed they were done in a loop.
thank for clearing this out. just a remark - do_ram_command already did read32() commands before my patches, it simply didn't do it on all slots/banks.
Since do_ram_command does more stuff than just those reads, you might cause the controllers state machine to choke.
Basically, doing any reads in do_ram_command() is a bad idea. They should instead be done when/where they need to happen. I suggest looking at the i945 code for an example.
i don't know any other way of doing the do_ram_command() properly. i've used mainly the i830 code as a reference - i can't seems to find any i945 code.
Stefan
again, many thanks for your insights and corrections. and since we're on the subject - can we remove the redundant row_offset which is not used anywhere in the code?
Elia.
-- coresystems GmbH • Brahmsstr. 16 • D-79104 Freiburg i. Br. Tel.: +49 761 7668825 • Fax: +49 761 7664613 Email: info@coresystems.de • http://www.coresystems.de/ Registergericht: Amtsgericht Freiburg • HRB 7656 Geschäftsführer: Stefan Reinauer • Ust-IdNr.: DE245674866
-- coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot