<div dir="ltr"><br><br><div class="gmail_quote">On Mon, Nov 17, 2008 at 5:07 PM, Uwe Hermann <span dir="ltr"><<a href="mailto:uwe@hermann-uwe.de">uwe@hermann-uwe.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 class="Ih2E3d">On Mon, Nov 17, 2008 at 02:58:55PM +0100, Stefan Reinauer wrote:<br>
> 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>
> > Signed-off-by: Elia Yehuda <<a href="mailto:z4ziggy@gmail.com">z4ziggy@gmail.com</a> <mailto:<a href="mailto:z4ziggy@gmail.com">z4ziggy@gmail.com</a>>><br>
><br>
> Good work!<br>
<br>
</div>Yep, indeed. But please split up the patch in multiple ones, each fixing<br>
one issue. This way they can be tested and reviewed more easily. I suggest<br>
at least one patch for supporting multiple DIMMs and one for VGA<br>
stuff, maybe more...</blockquote><div><br>grrr....<br>i'll try :-)<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>
I'll be able to test the patches this evening...</blockquote><div><br>excellent.<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>
<div class="Ih2E3d"><br>
<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.<br>
<br>
</div>Yep, this is probably true for various NBs in v2, e.g. 440BX, i810,<br>
i830, and maybe more. That should be an extra patch though.<br>
<div class="Ih2E3d"><br>
<br>
> Also, your code only treats single channel configurations. Is the i810<br>
> capable of dual channel operation?<br>
><br>
> > -   /* TODO: This needs to be set according to the DRAM tech<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>
<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>
</div>I assume these are gathered from actual hardware on your board? Or did<br>
you find them in the datasheet somewhere?</blockquote><div><br>my own testings.<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>
<div class="Ih2E3d"><br>
<br>
> > +   /* setting Vendor/Device ids - there must be a better way of doing<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>
</div>Yep, this should definately be moved.<br>
</blockquote><div><br>done.<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>
Uwe.</blockquote><div><br>many thanks,<br>Elia.<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>
<a href="http://www.hermann-uwe.de" target="_blank">http://www.hermann-uwe.de</a>  | <a href="http://www.holsham-traders.de" target="_blank">http://www.holsham-traders.de</a><br>
<a href="http://www.crazy-hacks.org" target="_blank">http://www.crazy-hacks.org</a> | <a href="http://www.unmaintained-free-software.org" target="_blank">http://www.unmaintained-free-software.org</a><br>
</font></blockquote></div><br></div>