On Mon, Nov 17, 2008 at 9:49 AM, Elia Yehuda <[EMAIL PROTECTED]> wrote:
> > > On Mon, Nov 17, 2008 at 3:58 PM, Stefan Reinauer <[EMAIL PROTECTED]>wrote: > >> 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 <[EMAIL PROTECTED] <mailto:[EMAIL PROTECTED] >> >> >> >> 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 of >> sides in a >> > + * DIMM; SPD Byte 13 provides information on >> Some 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 tech >> > >> Don'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 unknown >> reason */ >> > + //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: [EMAIL PROTECTED] • http://www.coresystems.de/ >> Registergericht: Amtsgericht Freiburg • HRB 7656 >> Geschäftsführer: Stefan Reinauer • Ust-IdNr.: DE245674866 >> >> > > -- > coreboot mailing list: [email protected] > http://www.coreboot.org/mailman/listinfo/coreboot >
-- coreboot mailing list: [email protected] http://www.coreboot.org/mailman/listinfo/coreboot
