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

Elia Yehuda z4ziggy at gmail.com
Mon Nov 17 21:02:52 CET 2008


On Mon, Nov 17, 2008 at 6:01 PM, Corey Osgood <corey.osgood at gmail.com>wrote:

> On Mon, Nov 17, 2008 at 9:49 AM, Elia Yehuda <z4ziggy at gmail.com> wrote:
>
>>
>>
>> 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!
>>
>>
>> thanks :)
>>
>>
>>>
>>>
>>> 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.
>>>
>>
>> i had no idea using the smbus is slower.
>> actually my initial hacking was using the PCI but it was a bit more
>> complexed
>> since i had to convert from intel's codes to MBs, and i had a bit of a
>> problem
>> detecting the dimm_bank properly - i will look into that again when i'll
>> wake up...
>>
>
> IIRC there was a "magic table" that did the conversion.
>

yep, there is, its still a bit 'messy' comparing to the current (with
smbus), but the main
issue is getting the dimm_bank properly. ok, i woke up - im on to it... will
be updating
the list soon.


>
>
>
>>
>>
>>
>>>
>>> > +                      * 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.
>>>
>>
>> the evil char is now gone...
>>
>>
>>>
>>>
>>> > );
>>> > +     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.
>>>
>>
>> actually using ctrl->d0 looks cleaner in the code (rather than using
>> PCI_DEV(0,0,0))
>> but sure, i'll change it...
>>
>>
>>>
>>>
>>> > +                     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.
>>>
>>
>> will look into that.
>>
>>
>>>
>>>
>>> Also, your code only treats single channel configurations. Is the i810
>>> capable of dual channel operation?
>>
>>
>> the i810 supports 2 SDRAM DIMM slots, single and dual sided, up to 512MB
>> total.
>> i have no idea how to treat the 2nd slot, although ive read the datasheet
>> top-to-bottom.
>> any clues would be greatly appreciated...
>>
>
> Dual channel is a DDR thing? I picked up another i810 board a couple days
> ago, so I'll poke around with a second dimm if you can get one working ;)
> Please remid me, IIRC i810 can only have 512MB if it has 2 dual-sided 256MB
> memory sticks, it can't use single-sided 256MB sticks, right?
>

you mean SDRAM. and you are correct.


>
>
>
>>
>>
>>
>>>
>>>
>>> > -     /* 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.
>>>
>>
>> not really. using 0x0001 works fine for both 1 DIMM and for 2 DIMMs
>> (currently
>> my i810 is active, using 1 256MB DIMM, with 0x0001 setting). using the
>> 0x77da
>> is worse since it only works with 1 single-sided DIMM. after I'll fix the
>> 2nd DIMM
>> issue (ie, make it work!!) i'll deal with those small issues.
>>
>
> Cool! This is an improvement ;)
>
>
>>
>>
>>>
>>>
>>> > +     /* set the GMCH to prechange all during the service of a page
>>> miss */
>>> >
>>> precha_r_ge?
>>
>>
>> yep. prechange. looks weird? talk to the intel ppl - i copied this line
>> from the datasheet...
>>
>
> I'm fairly sure that's a typo in the datasheet, it should be precharge.
>

/me not native english speaker... will fix the typo.


>
>
>
>>
>>
>>
>>>
>>>
>>> > +     /* 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)
>>>
>>
>> yep, from CSEG and FSEG. i tried putting this at the end of sdram_enable()
>> but
>> it always causes the VGA not to work, so i left it for now with the
>> "insane" range
>> for r/w.
>> the original BIOS shows 0x41 at this offset, but i have no idea after
>> which step
>> does it set it to make the VGA still work. maybe im missing someting else?
>>
>
> Weird. Probably some part of the Video BIOS uses that range during init. I
> think a few K for working onboard VGA is a good tradeoff ;)
>

im not giving up my working VGA!  :-)


>
> -Corey
>
>
>>
>>
>>
>>>
>>>
>>> > +     /* 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,
>>> };
>>>
>>
>> many, many thanks :) i'm still learning the coreboot code (im just hacking
>> around for
>> about 2 weeks) and i figured there must be a better way of doing this,
>> just couldnt
>> figure out how...
>>
>>
>>>
>>>
>>> >       /* 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.
>>
>>
>> thank for clearing this out. just a remark - do_ram_command already did
>> read32()
>> commands before my patches, it simply didn't do it on all slots/banks.
>>
>>
>
>>
>>
>>>
>>>
>>> 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.
>>
>>
>> i don't know any other way of doing the do_ram_command() properly. i've
>> used mainly
>> the i830 code as a reference - i can't seems to find any i945 code.
>>
>>
>>>
>>>
>>>
>>>
>>> Stefan
>>
>>
>> again, many thanks for your insights and corrections. and since we're on
>> the subject -
>> can we remove the redundant row_offset which is not used anywhere in the
>> code?
>>
>> Elia.
>>
>>
>>>
>>>
>>>
>>> --
>>> 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
>>>
>>>
>>
>> --
>> coreboot mailing list: coreboot at coreboot.org
>> http://www.coreboot.org/mailman/listinfo/coreboot
>>
>
>
Elia.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20081117/3f475a3e/attachment.html>


More information about the coreboot mailing list