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

Elia Yehuda z4ziggy at gmail.com
Mon Nov 17 23:59:07 CET 2008


just noticed ive put it in cpu_bus_ops... have a laugh...
but i dont have "static struct device_operations mc_ops" - where should i
add
the .ops_pci ?

Elia.

On Tue, Nov 18, 2008 at 12:46 AM, Elia Yehuda <z4ziggy at gmail.com> wrote:

> 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 <stepan at coresystems.de>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!
>>
>> 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
>>
>>
>> --
>> coresystems GmbH • Brahmsstr. 16 • D-79104 Freiburg i. Br.
>>      Tel.: +49 761 7668825 • Fax: +49 761 7664613
>> Email: info at coresystems.dehttp://www.coresystems.de/
>> Registergericht: Amtsgericht Freiburg • HRB 7656
>> Geschäftsführer: Stefan Reinauer • Ust-IdNr.: DE245674866
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20081118/ee018456/attachment.html>


More information about the coreboot mailing list