<div dir="ltr"><br><br><div class="gmail_quote">On Mon, Nov 17, 2008 at 6:01 PM, Corey Osgood <span dir="ltr"><<a href="mailto:corey.osgood@gmail.com">corey.osgood@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div><div></div><div class="Wj3C7c">On Mon, Nov 17, 2008 at 9:49 AM, Elia Yehuda <span dir="ltr"><<a href="mailto:z4ziggy@gmail.com" target="_blank">z4ziggy@gmail.com</a>></span> wrote:<br></div></div><div class="gmail_quote">
<div><div></div><div class="Wj3C7c"><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div dir="ltr"><br><br><div class="gmail_quote"><div>On Mon, Nov 17, 2008 at 3:58 PM, Stefan Reinauer <span dir="ltr"><<a href="mailto:stepan@coresystems.de" target="_blank">stepan@coresystems.de</a>></span> wrote:<br>

<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div><div></div><div>Elia Yehuda wrote:<br>
> Those 2 patches are one step towards having a working Onboard-VGA<br>
> and 512MB (the max for i810). The raminit.c patch fixes some<br>
> misconfigurations and probes the DIMMs correctly (all dual-sided are<br>
> now recognized properly), and also set buffer_strength to handle 2<br>
> DIMMs although atm this doesn't work. The i82810/northbridge.c patch<br>
> takes care of allocating Onboard-VGA memory.<br>
><br>
</div></div>> Signed-off-by: Elia Yehuda <<a href="mailto:z4ziggy@gmail.com" target="_blank">z4ziggy@gmail.com</a> <mailto:<a href="mailto:z4ziggy@gmail.com" target="_blank">z4ziggy@gmail.com</a>>><br>
<br>
Good work!</blockquote></div><div><br>thanks :)<br> <br></div><div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br>
<br>
But no ack yet, because it's ongoing work that also breaks some<br>
(potentially) working cases. I have a couple of comments though<br>
<br>
> +     for (i = 0; i < DIMM_SOCKETS ; i++) {<br>
> +             dimm_size = smbus_read_byte(ctrl->channel0[i], 31);<br>
> +             dimm_banks = smbus_read_byte(ctrl->channel0[i], 5);<br>
> +             if (dimm_size) {<br>
Can this be changed to read the size and banks values from the PCI<br>
registers or variables instead of from the smbus?<br>
It's a lot of slow, unneeded bus traffic for every ram read. I don't<br>
know about the 810, but other Intel chipsets would get confused by that<br>
kind of bus traffic and delays at that point.<br>
</blockquote></div><div><br>i had no idea using the smbus is slower.<br>actually my initial hacking was using the PCI but it was a bit more complexed<br>since i had to convert from intel's codes to MBs, and i had a bit of a problem<br>


detecting the dimm_bank properly - i will look into that again when i'll wake up...</div></div></div></blockquote></div></div><div><br>IIRC there was a "magic table" that did the conversion.</div></div></blockquote>
<div><br>yep, there is, its still a bit 'messy' comparing to the current (with smbus), but the main<br>issue is getting the dimm_bank properly. ok, i woke up - im on to it... will be updating<br>the list soon.<br>
 </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div class="gmail_quote"><div><br> </div><div class="Ih2E3d"><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">

<div dir="ltr"><div class="gmail_quote"><div><br> <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
> +                      * Rows of Slot 1 are numbed 2 through 3. The<div><br>
> +                      * DIMM’s SPD Byte 5 describes the number of sides in a<br>
> +                      * DIMM; SPD Byte 13 provides information on<br>
Some weird character sneaked into that comment.</div></blockquote><div><br>the evil char is now gone...<br> <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">


<br>
<br>
> );<br>
> +     uint8_t val;<div><br>
> +     val = pci_read_config8(ctrl->d0, DRAMT);<br>
I suggest using PCI_DEV(0,0,0) instead of the indirection ctrl->d0 all<br>
over raminit.c.<br>
<br>
That indirection was invented for AMD K8 where in an SMP environment<br>
there are several memory controllers with several DIMMs attached to<br>
each. But the i810 definitely only supports a single memory controller,<br>
and the code can be kept a lot simpler.</div></blockquote><div><br>actually using ctrl->d0 looks cleaner in the code (rather than using PCI_DEV(0,0,0))<br>but sure, i'll change it...<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">


<br>
<br>
> +                     if ((smbus_read_byte(ctrl->channel0[i], 127) | 0xf) == 0xff) {<div><br>
Same here: Intel boards expect the SPD ROMs at fixed places, unlike the<br>
common case on Opteron boards.</div></blockquote><div><br>will look into that.<br> </div><div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">

<br>
<br>
Also, your code only treats single channel configurations. Is the i810<br>
capable of dual channel operation?</blockquote></div><div><br>the i810 supports 2 SDRAM DIMM slots, single and dual sided, up to 512MB total.<br>i have no idea how to treat the 2nd slot, although ive read the datasheet top-to-bottom.<br>


any clues would be greatly appreciated...</div></div></div></blockquote></div><div><br>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?</div>
</div></blockquote><div><br>you mean SDRAM. and you are correct.<br> <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div class="gmail_quote">
<div><br>
 </div><div><div></div><div class="Wj3C7c"><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div dir="ltr"><div class="gmail_quote"><div><br>
 </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>
<br>
> -     /* TODO: This needs to be set according to the DRAM tech<div><br>
> +     /* TODO: This needs to be calculated according to the DRAM tech<br>
><br>
Don't think this can really be calculated. Looking at your findings<br>
below, you should probably use a table for this and look up the correct<br>
values from the table.<br>
>        * (x8, x16, or x32). Argh, Intel provides no docs on this!<br>
>        * Currently, it needs to be pulled from the output of<br>
>        * lspci -xxx Rx92<br>
> +      * here are some common results:<br>
> +      * value:   tom:    config:<br>
> +      * 0x3356   256MB   1 x 256MB DIMM(s), dual sided<br>
> +      * 0x0001   512MB   2 x 256MB DIMM(s), dual sided<br>
> +      * 0x77da   128MB   1 x 128MB DIMM(s), single sided<br>
> +      * 0x55c6   128MB   2 x 128MB DIMM(s), single sided<br>
> +      * 0x3356   128MB   1 x 128MB DIMM(s), dual sided<br>
> +      * 0x0001   256MB   2 x 128MB DIMM(s), dual sided<br>
>        */<br>
Because:<br>
> -     pci_write_config16(ctrl->d0, BUFF_SC, 0x77da);<br>
> +     /* use 2 dual sided DIMMs - this also works with only 1 DIMM */<br>
> +     pci_write_config16(ctrl->d0, BUFF_SC, 0x0001);<br>
> +<br>
This fixes one case and breaks another one. Not really an improvement.</div></blockquote><div><br>not really. using 0x0001 works fine for both 1 DIMM and for 2 DIMMs (currently<br>my i810 is active, using 1 256MB DIMM, with 0x0001 setting). using the 0x77da<br>


is worse since it only works with 1 single-sided DIMM. after I'll fix the 2nd DIMM<br>issue (ie, make it work!!) i'll deal with those small issues.<br></div></div></div></blockquote></div></div><div><br>Cool! This is an improvement ;)<br>

 </div><div class="Ih2E3d"><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div dir="ltr"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">


<br>
<br>
> +     /* set the GMCH to prechange all during the service of a page miss */<br>
><br>
precha_r_ge?</blockquote><div><br>yep. prechange. looks weird? talk to the intel ppl - i copied this line from the datasheet...</div></div></div></blockquote></div><div><br>I'm fairly sure that's a typo in the datasheet, it should be precharge.</div>
</div></blockquote><div><br>/me not native english speaker... will fix the typo.<br> <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div class="gmail_quote"><div><br>
 </div><div class="Ih2E3d"><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div dir="ltr"><div class="gmail_quote"><div><br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">


<br>
<br>
> +     /* or we can use sane defaults, but VGA won't work for unknown reason */<div><br>
> +     //pci_write_config8(ctrl->d0, PAM, 0x41);<br>
This will prevent writes to the CSEG, will it? (Didn't check)</div></blockquote><div><br>yep, from CSEG and FSEG. i tried putting this at the end of sdram_enable() but<br>it always causes the VGA not to work, so i left it for now with the "insane" range<br>


for r/w.<br>the original BIOS shows 0x41 at this offset, but i have no idea after which step <br>does it set it to make the VGA still work. maybe im missing someting else?</div></div></div></blockquote></div><div><br>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 ;)</div>
</div></blockquote><div><br>im not giving up my working VGA!  :-)<br><br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div class="gmail_quote">
<div><br>
<br>-Corey<br> </div><div><div></div><div class="Wj3C7c"><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div dir="ltr"><div class="gmail_quote">
<div><br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">

<br>
<br>
> +     /* setting Vendor/Device ids - there must be a better way of doing<div><br>
> +      * this...<br>
> +      */<br>
> +     /* set vendor id */<br>
> +     val = pci_read_config16(ctrl->d0, 0);<br>
> +     pci_write_config16(ctrl->d0, 0x2c, val);<br>
> +     /* set device id */<br>
> +     val = pci_read_config16(ctrl->d0, 2);<br>
> +     pci_write_config16(ctrl->d0, 0x2e, val);<br>
Yes, during pci_set_subsystem_ids in northbridge.c:<br>
<br>
static void intel_set_subsystem(device_t dev, unsigned vendor, unsigned<br>
device)<br>
{<br>
        u32 pci_id;<br>
<br>
        printk_debug("Setting PCI bridge subsystem ID\n");<br>
        pci_id = pci_read_config32(dev, 0);<br>
        pci_write_config32(dev, PCI_SUBSYSTEM_VENDOR_ID, pci_id );<br>
}<br>
<br>
static struct pci_operations intel_pci_ops = {<br>
        .set_subsystem    = intel_set_subsystem,<br>
};<br>
<br>
static struct device_operations mc_ops = {<br>
        [..]<br>
        .ops_pci          = &intel_pci_ops,<br>
};</div></blockquote><div><br>many, many thanks :) i'm still learning the coreboot code (im just hacking around for <br>about 2 weeks) and i figured there must be a better way of doing this, just couldnt<br>figure out how...<br>


 </div><div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br>
<br>
>       /* 3. Perform 8 refresh cycles. Wait tRC each time. */<br>
>       PRINT_DEBUG("RAM Enable 3: CBR\r\n");<br>
> -     do_ram_command(ctrl, RAM_COMMAND_CBR, 0, row_offset);<br>
>       for (i = 0; i < 8; i++) {<br>
> +             do_ram_command(ctrl, RAM_COMMAND_CBR, 0, row_offset);<br>
> +             /* FIXME: are those read32() needed? */<br>
>               read32(0);<br>
>               read32(row_offset);<br>
>               udelay(1);<br>
Probably not needed the way you're doing it now. The reads cause a<br>
refresh cycle on a given dram rank. do_ram_command does one such read.<br>
Since several are needed they were done in a loop.</blockquote></div><div><br>thank for clearing this out. just a remark - do_ram_command already did read32()<br>commands before my patches, it simply didn't do it on all slots/banks.<br>

 </div></div></div></blockquote></div></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div><div></div><div class="Wj3C7c"><div dir="ltr">
<div class="gmail_quote"><div><br>
 </div><div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br>
<br>
Since do_ram_command does more stuff than just those reads, you might<br>
cause the controllers state machine to choke.<br>
<br>
Basically, doing any reads in do_ram_command() is a bad idea. They<br>
should instead be done when/where they need to happen. I suggest looking<br>
at the i945 code for an example.</blockquote></div><div><br>i don't know any other way of doing the do_ram_command() properly. i've used mainly<br>the i830 code as a reference - i can't seems to find any i945 code.<br>


 </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br>
<br>
<br>
<br>
Stefan</blockquote><div><br>again, many thanks for your insights and corrections. and since we're on the subject -<br>can we remove the redundant row_offset which is not used anywhere in the code?<br><br>Elia.<br> <br>


</div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br>
<font color="#888888"><br>
<br>
--<div><br>
coresystems GmbH • Brahmsstr. 16 • D-79104 Freiburg i. Br.<br>
      Tel.: +49 761 7668825 • Fax: +49 761 7664613<br>
Email: <a href="mailto:info@coresystems.de" target="_blank">info@coresystems.de</a>  • <a href="http://www.coresystems.de/" target="_blank">http://www.coresystems.de/</a><br>
Registergericht: Amtsgericht Freiburg • HRB 7656<br>
Geschäftsführer: Stefan Reinauer • Ust-IdNr.: DE245674866<br>
<br>
</div></font></blockquote></div><br></div>
<br></div></div>--<br>
coreboot mailing list: <a href="mailto:coreboot@coreboot.org" target="_blank">coreboot@coreboot.org</a><br>
<a href="http://www.coreboot.org/mailman/listinfo/coreboot" target="_blank">http://www.coreboot.org/mailman/listinfo/coreboot</a><br></blockquote></div><br>
</blockquote></div><br>Elia.<br></div>