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

[Elia Yehuda]

regarding the vendor/device id - ive added the following :

static void intel_set_subsystem(device_t dev, unsigned vendor, unsigned
device)
{
    u32 pci_id;

printk_debug("Setting PCI bridge subsystem ID\n");
    pci_id = pci_read_config32(dev, 0);
    pci_write_config32(dev, PCI_SUBSYSTEM_VENDOR_ID, pci_id );
}

static struct pci_operations intel_pci_ops = {
       .set_subsystem    = intel_set_subsystem,
};

static struct device_operations cpu_bus_ops = {
    .read_resources        = cpu_bus_noop,
    .set_resources        = cpu_bus_noop,
    .enable_resources    = cpu_bus_noop,
    .init            = cpu_bus_init,
    .scan_bus        = 0,
    .ops_pci        = &intel_pci_ops,
};

but it doesnt seems to do the trick. i don't even get "Setting PCI bridge
subsystem ID" - am
I missing something?

Elia.

On Mon, Nov 17, 2008 at 3:58 PM, 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 <[e-mail] <mailto:[e-mail]
> > >
>
> Good work!
>
> But no ack yet, because it's ongoing work that also breaks some
> (potentially) working cases. I have a couple of comments though
> > for (i = 0; i < DIMM_SOCKETS ; i++) {
> >         dimm_size = smbus_read_byte(ctrl->channel0[i], 31);
> >         dimm_banks = smbus_read_byte(ctrl->channel0[i], 5);
> >         if (dimm_size) {
>
> Can this be changed to read the size and banks values from the PCI
> registers or variables instead of from the smbus?
> It's a lot of slow, unneeded bus traffic for every ram read. I don't
> know about the 810, but other Intel chipsets would get confused by that
> kind of bus traffic and delays at that point.
> >                  * Rows of Slot 1 are numbed 2 through 3. The
> >                  * DIMM’s SPD Byte 5 describes the number of
>
> sides in a
> >                  * DIMM; SPD Byte 13 provides information on
>
> Some weird character sneaked into that comment.
> > );
> > uint8_t val;
> > 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.
> >                 if ((smbus_read_byte(ctrl->channel0[i], 127) | 0xf)
>
> == 0xff) {
> Same here: Intel boards expect the SPD ROMs at fixed places, unlike the
> common case on Opteron boards.
>
> 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
> >  */
>
> Because:
> > pci_write_config16(ctrl->d0, BUFF_SC, 0x77da);
> > /* use 2 dual sided DIMMs - this also works with only 1 DIMM */
> > pci_write_config16(ctrl->d0, BUFF_SC, 0x0001);
>
> This fixes one case and breaks another one. Not really an improvement.
> > /* set the GMCH to prechange all during the service of a page miss
>
> */
> >
>
> precha_r_ge?
> > /* or we can use sane defaults, but VGA won't work for unknown
>
> reason */
> > //pci_write_config8(ctrl->d0, PAM, 0x41);
>
> This will prevent writes to the CSEG, will it? (Didn't check)
> > /* 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:
>
> static void intel_set_subsystem(device_t dev, unsigned vendor, unsigned
> device)
> {
>        u32 pci_id;
>    printk_debug("Setting PCI bridge subsystem ID\n");
>    pci_id = pci_read_config32(dev, 0);
>    pci_write_config32(dev, PCI_SUBSYSTEM_VENDOR_ID, pci_id );
>
> }
>
> static struct pci_operations intel_pci_ops = {
>        .set_subsystem    = intel_set_subsystem,
> };
>
> static struct device_operations mc_ops = {
>        [..]
>        .ops_pci          = &intel_pci_ops,
> };
> >   /* 3. Perform 8 refresh cycles. Wait tRC each time. */
> >   PRINT_DEBUG("RAM Enable 3: CBR\r\n");
> > do_ram_command(ctrl, RAM_COMMAND_CBR, 0, row_offset);
> > for (i = 0; i < 8; i++) {
> >         do_ram_command(ctrl, RAM_COMMAND_CBR, 0, row_offset);
> >         /* FIXME: are those read32() needed? */
> >         read32(0);
> >         read32(row_offset);
> >         udelay(1);
>
> Probably not needed the way you're doing it now. The reads cause a
> refresh cycle on a given dram rank. do_ram_command does one such read.
> Since several are needed they were done in a loop.
>
> Since do_ram_command does more stuff than just those reads, you might
> cause the controllers state machine to choke.
>
> Basically, doing any reads in do_ram_command() is a bad idea. They
> should instead be done when/where they need to happen. I suggest looking
> at the i945 code for an example.
>
> Stefan