[coreboot] [patch] i82810 WIP for fixing VGA and 512MB

[Elia Yehuda]

On Mon, Nov 17, 2008 at 5:07 PM, Uwe Hermann <uwe at hermann-uwe.de> wrote:

> On Mon, Nov 17, 2008 at 02:58:55PM +0100, Stefan Reinauer 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 <z4ziggy at gmail.com <mailto:
> z4ziggy at gmail.com>>
> >
> > Good work!
>
> Yep, indeed. But please split up the patch in multiple ones, each fixing
> one issue. This way they can be tested and reviewed more easily. I suggest
> at least one patch for supporting multiple DIMMs and one for VGA
> stuff, maybe more...


grrr....
i'll try :-)


>
>
> I'll be able to test the patches this evening...


excellent.


>
>
>
> > > +   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.
>
> Yep, this is probably true for various NBs in v2, e.g. 440BX, i810,
> i830, and maybe more. That should be an extra patch though.
>
>
> > Also, your code only treats single channel configurations. Is the i810
> > capable of dual channel operation?
> >
> > > -   /* 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
>
> I assume these are gathered from actual hardware on your board? Or did
> you find them in the datasheet somewhere?


my own testings.


>
>
>
> > > +   /* 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:
>
> Yep, this should definately be moved.
>

done.


>
>
> Uwe.


many thanks,
Elia.


>
> --
> http://www.hermann-uwe.de  | http://www.holsham-traders.de
> http://www.crazy-hacks.org | http://www.unmaintained-free-software.org
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20081117/4232ae15/attachment.html>