On Mon, Nov 17, 2008 at 5:07 PM, Uwe Hermann <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED] <mailto: > [EMAIL PROTECTED]>> > > > > 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 >
-- coreboot mailing list: [email protected] http://www.coreboot.org/mailman/listinfo/coreboot
