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... I'll be able to test the patches this evening... > > + 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? > > + /* 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. Uwe. -- 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
