[patch] i82810 WIP for fixing VGA and 512MB

[Elia Yehuda]

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

[Joseph Smith]

On Mon, 17 Nov 2008 04:25:34 +0200, "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

Sorry I'm going to have to nack your northbridge.patch.

Your VGA memory has to be subtracted from tomk in kb,like this:

#ifdef CONFIG_VIDEO_MB
    	/* check for VGA reserved memory 
    	 * possible CONFIG_VIDEO_MB values are 512(kb) and 1(mb)
    	 */
    	if (CONFIG_VIDEO_MB == 512) {
    		tomk -= 512;
    	} else if (CONFIG_VIDEO_MB == 1) {
    		tomk -= 1024;
    	} else {
                           /* assume no vga if incorrect value */
    		tomk == tomk;
#endif

Also Make sure you have already set the CONFIG_VIDEO_MB value in your
northbridge vga memory register. I would recomend this earlier in
raminit.c. Clue from i82830 raminit.c:

/* Set the value for GMCH Control Register #1 */ 
         switch (CONFIG_VIDEO_MB) { 
         case 512: /* 512K of memory */ 
                 igd_memory = 0x2; 
                 break; 
         case 1: /* 1M of memory */ 
                 igd_memory = 0x3; 
                 break; 
         case 8: /* 8M of memory */ 
                 igd_memory = 0x4; 
                 break; 
         default: /* No memory */ 
                 pci_write_config16(ctrl->d0, GCC1, 0x0002); 
                 igd_memory = 0x0; 
         }

value = pci_read_config16(ctrl->d0, GCC1); 
         value |= igd_memory << 4; 
         pci_write_config16(ctrl->d0, GCC1, value);

Hope that helps.

[Elia Yehuda]

On Mon, Nov 17, 2008 at 5:30 AM, Joseph Smith  wrote:
> On Mon, 17 Nov 2008 04:25:34 +0200, "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
>
> Sorry I'm going to have to nack your northbridge.patch.
>
> Your VGA memory has to be subtracted from tomk in kb,like this:
>
> #ifdef CONFIG_VIDEO_MB
>                /* check for VGA reserved memory
>                 * possible CONFIG_VIDEO_MB values are 512(kb) and 1(mb)
>                 */
>                if (CONFIG_VIDEO_MB == 512) {
>                        tomk -= 512;
>                } else if (CONFIG_VIDEO_MB == 1) {
>                        tomk -= 1024;
>                } else {
>                           /* assume no vga if incorrect value */
>                        tomk == tomk;
> #endif

the place ive added this is when tomk is still set as MBs. Ofcourse i can
move
this a few lines down after tomk is converted to KBs. Ive just put it there
to be
before the printk_debug("Setting RAM size to %d MB\n", tomk) so we can see
how much MBs are subtracted from tomk.
> Also Make sure you have already set the CONFIG_VIDEO_MB value in your
> northbridge vga memory register. I would recomend this earlier in
> raminit.c. Clue from i82830 raminit.c:
>     /* Set the value for GMCH Control Register #1 */
>     switch (CONFIG_VIDEO_MB) {
>     case 512: /* 512K of memory */
>             igd_memory = 0x2;
>             break;
>     case 1: /* 1M of memory */
>             igd_memory = 0x3;
>             break;
>     case 8: /* 8M of memory */
>             igd_memory = 0x4;
>             break;
>     default: /* No memory */
>             pci_write_config16(ctrl->d0, GCC1, 0x0002);
>             igd_memory = 0x0;
>     }
>
>     value = pci_read_config16(ctrl->d0, GCC1);
>     value |= igd_memory << 4;
>     pci_write_config16(ctrl->d0, GCC1, value);

more of the lines of : ?

/* set System Management RAM Control Register / Graphics Mode Select */
    value = pci_read_config8(ctrl->d0, SMRAM);
    /* Set size for Onboard-VGA framebuffer. */
    switch (CONFIG_VIDEO_MB) {
    case 512: /* 512K of memory */
        val = 0x2;
        break;
    case 1: /* 1M of memory */
        val = 0x3;
        break;
    default: /* No VGA memory */
        /* Preserve bits except for GMS */
        value &= 0x3f;
        pci_write_config16(ctrl->d0, SMRAM, value);
        val = 0x0;
    }
        value |= val << 6;
    /* set AB segment Enabled as SMM RAM */
    value |= 0x0C;
    pci_write_config8(ctrl->d0, SMRAM, value);

do notice ive removed the 8MB option since i810 doesn't support this.
> Hope that helps.

always appreciated.

Elia.
>

[Joseph Smith]

On Mon, 17 Nov 2008 06:13:16 +0200, "Elia Yehuda" 
wrote:
> On Mon, Nov 17, 2008 at 5:30 AM, Joseph Smith

wrote:
> > On Mon, 17 Nov 2008 04:25:34 +0200, "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
> >
> > Sorry I'm going to have to nack your northbridge.patch.
> >
> > Your VGA memory has to be subtracted from tomk in kb,like this:
> >
> > #ifdef CONFIG_VIDEO_MB
> >                /* check for VGA reserved memory
> >                 * possible CONFIG_VIDEO_MB values are 512(kb) and 1(mb)
> >                 */
> >                if (CONFIG_VIDEO_MB == 512) {
> >                        tomk -= 512;
> >                } else if (CONFIG_VIDEO_MB == 1) {
> >                        tomk -= 1024;
> >                } else {
> >                           /* assume no vga if incorrect value */
> >                        tomk == tomk;
> > #endif
>
> the place ive added this is when tomk is still set as MBs. Ofcourse i can
> move
> this a few lines down after tomk is converted to KBs. Ive just put it
> there
> to be
> before the printk_debug("Setting RAM size to %d MB\n", tomk) so we can

see
> how much MBs are subtracted from tomk.

You will have to do that to make the 512 work. Or, what is the point in
showing it in MB's, just for user aesthetics? It would be much more
accurate to show it in kb's correct? Especially if your only subtracting
512kb. I would recomend changing this to kb's in the first place, that is
what tomk stands for (top of memory in kb's).

tomk += ((unsigned long)(translate_i82810_to_mb[drp_value]) * 1024);
>

---------------------------------------------------------------------------------
> /* set System Management RAM Control Register / Graphics Mode Select
>
> */
>     value = pci_read_config8(ctrl->d0, SMRAM);
>     /* Set size for Onboard-VGA framebuffer. */
>     switch (CONFIG_VIDEO_MB) {
>     case 512: /* 512K of memory */
>         val = 0x2;
>         break;
>     case 1: /* 1M of memory */
>         val = 0x3;
>         break;
>     default: /* No VGA memory */
>         /* Preserve bits except for GMS */
>         value &= 0x3f;
>         pci_write_config16(ctrl->d0, SMRAM, value);
>         val = 0x0;
>     }
>         value |= val << 6;
>     /* set AB segment Enabled as SMM RAM */
>     value |= 0x0C;
>     pci_write_config8(ctrl->d0, SMRAM, value);

Yup that looks good, of course I haven't a chance to compaire it to the
datasheet. So make sure you test it. This should go in raminit.c where you
set the SMRAM register.

[Stefan Reinauer]

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] >

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

[Elia Yehuda]

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!

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...
> >                  * 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...
> > /* 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.
> > /* 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...
> > /* 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?
> > /* 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.
>

[Corey Osgood]

On Mon, Nov 17, 2008 at 9:49 AM, Elia Yehuda  wrote:
> 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!
>
> 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.
> > >                  * 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?
> > > /* 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.
> > > /* 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 ;)

-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.
> >

[Elia Yehuda]

On Mon, Nov 17, 2008 at 6:01 PM, Corey Osgood wrote:
> On Mon, Nov 17, 2008 at 9:49 AM, Elia Yehuda  wrote:
> > 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!
> >
> > 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.
> > >

Elia.

[Uwe Hermann]

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 <[e-mail] >
>
> 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.

[Elia Yehuda]

On Mon, Nov 17, 2008 at 5:07 PM, Uwe Hermann  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 <[e-mail] <mailto:
>
> [e-mail]>>
> > 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.
>

[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

[Elia Yehuda]

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  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 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

[Joseph Smith]

On Tue, 18 Nov 2008 00:59:07 +0200, "Elia Yehuda" 
wrote:
> just noticed ive put it in cpu_bus_ops... have a laugh...

No worries, this is how we learn :-)
> but i dont have "static struct device_operations mc_ops" - where should i
> add
> the .ops_pci ?

Little confused about what your trying to do here. This doesn't work??

static const struct pci_driver northbridge_driver __pci_driver = { 
         .ops    = &northbridge_operations, 
         .vendor = PCI_VENDOR_ID_INTEL, 
         .device = 0x7120, 
};

[Elia Yehuda]

On Tue, Nov 18, 2008 at 3:44 AM, Joseph Smith  wrote:
> On Tue, 18 Nov 2008 00:59:07 +0200, "Elia Yehuda" 
> wrote:
> > just noticed ive put it in cpu_bus_ops... have a laugh...
>
> No worries, this is how we learn :-)
> > but i dont have "static struct device_operations mc_ops" - where should i
> > add
> > the .ops_pci ?
>
> Little confused about what your trying to do here. This doesn't work??
>
> static const struct pci_driver northbridge_driver __pci_driver = {
>         .ops    = &northbridge_operations,
>         .vendor = PCI_VENDOR_ID_INTEL,
>         .device = 0x7120,
> };

this works just fine. i simply don't know where to add ".ops_pci" since i
dont have
"static struct device_operations mc_ops" in northbridge.c
>

Elia.

[Joseph Smith]

On Tue, 18 Nov 2008 05:10:48 +0200, "Elia Yehuda" 
wrote:
> this works just fine. i simply don't know where to add ".ops_pci" since i
> dont have
> "static struct device_operations mc_ops" in northbridge.c

I guess I am confused, why do you need .ops_pci? Why can't you use .ops_pci
in:

static struct device_operations northbridge_operations = { 
        .read_resources         = pci_dev_read_resources, 
        .set_resources          = pci_dev_set_resources, 
        .enable_resources       = pci_dev_enable_resources, 
        .init                   = northbridge_init, 
        .enable                 = 0, 
        .ops_pci                = 0, 
};

???

[Corey Osgood]

On Mon, Nov 17, 2008 at 10:10 PM, Elia Yehuda  wrote:
> On Tue, Nov 18, 2008 at 3:44 AM, Joseph Smith  wrote:
> > On Tue, 18 Nov 2008 00:59:07 +0200, "Elia Yehuda" 
> > wrote:
> > > just noticed ive put it in cpu_bus_ops... have a laugh...
> >
> > No worries, this is how we learn :-)
> > > but i dont have "static struct device_operations mc_ops" - where should
> >
> > i
> > > add
> > > the .ops_pci ?
> >
> > Little confused about what your trying to do here. This doesn't work??
> >
> > static const struct pci_driver northbridge_driver __pci_driver = {
> >         .ops    = &northbridge_operations,
> >         .vendor = PCI_VENDOR_ID_INTEL,
> >         .device = 0x7120,
> > };
>
> this works just fine. i simply don't know where to add ".ops_pci" since i
> dont have
> "static struct device_operations mc_ops" in northbridge.c

You should be able to add it to pci_domain_ops, but be sure to check that it
actually runs. Otherwise, you'll need to create that mc_ops driver with just
the .ops_pci and everything else set to NULL, and use this:

static void enable_dev(struct device *dev)
{
    struct device_path path; //unused?

/* Set the operations if it is a special bus type */
    if (dev->path.type == DEVICE_PATH_PCI_DOMAIN) {
        dev->ops = &pci_domain_ops;
        pci_set_method(dev);
    } else if (dev->path.type == DEVICE_PATH_APIC_CLUSTER) {
        dev->ops = &cpu_bus_ops;
    } else if (dev->path.type == DEVICE_PATH_PCI) {
        dev->ops = &mc_ops;
    }
}

I hope you can understand what I'm trying to say, I'm a bit tired right now.

-Corey

[Elia Yehuda]

On Tue, Nov 18, 2008 at 6:04 AM, Corey Osgood wrote:
> On Mon, Nov 17, 2008 at 10:10 PM, Elia Yehuda  wrote:
> > On Tue, Nov 18, 2008 at 3:44 AM, Joseph Smith wrote:
> > > On Tue, 18 Nov 2008 00:59:07 +0200, "Elia Yehuda" 
> > > wrote:
> > > > just noticed ive put it in cpu_bus_ops... have a laugh...
> > >
> > > No worries, this is how we learn :-)
> > > > but i dont have "static struct device_operations mc_ops" - where should
> > >
> > > i
> > > > add
> > > > the .ops_pci ?
> > >
> > > Little confused about what your trying to do here. This doesn't work??
> > >
> > > static const struct pci_driver northbridge_driver __pci_driver = {
> > >         .ops    = &northbridge_operations,
> > >         .vendor = PCI_VENDOR_ID_INTEL,
> > >         .device = 0x7120,
> > > };
> >
> > this works just fine. i simply don't know where to add ".ops_pci" since i
> > dont have
> > "static struct device_operations mc_ops" in northbridge.c
>
> You should be able to add it to pci_domain_ops, but be sure to check that
> it actually runs.

i tried the following, no game :
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 pci_domain_ops = {
    .read_resources        = pci_domain_read_resources,
    .set_resources        = pci_domain_set_resources,
    .enable_resources    = enable_childrens_resources,
    .init            = 0,
    .scan_bus        = pci_domain_scan_bus,
    .ops_pci        = &intel_pci_ops,
};
> Otherwise, you'll need to create that mc_ops driver with just the .ops_pci
> and everything else set to NULL, and use this:
>
> static void enable_dev(struct device *dev)
> {
>     struct device_path path; //unused?
> /* Set the operations if it is a special bus type */
> if (dev->path.type == DEVICE_PATH_PCI_DOMAIN) {
>     dev->ops = &pci_domain_ops;
>     pci_set_method(dev);
> } else if (dev->path.type == DEVICE_PATH_APIC_CLUSTER) {
>     dev->ops = &cpu_bus_ops;
> } else if (dev->path.type == DEVICE_PATH_PCI) {
>     dev->ops = &mc_ops;
> }
>
> }
>
> I hope you can understand what I'm trying to say, I'm a bit tired right
> now.

this doesnt work either :

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 = {
        .read_resources   = 0, //mc_read_resources,
        .set_resources    = 0, //mc_set_resources,
        .enable_resources = 0, //pci_dev_enable_resources,
        .init             = 0,
        .scan_bus         = 0,
        .ops_pci          = &intel_pci_ops,
};

static void enable_dev(struct device *dev)
{
    struct device_path path;

/* Set the operations if it is a special bus type */
    if (dev->path.type == DEVICE_PATH_PCI_DOMAIN) {
        dev->ops = &pci_domain_ops;
        pci_set_method(dev);
    } else if (dev->path.type == DEVICE_PATH_APIC_CLUSTER) {
        dev->ops = &cpu_bus_ops;
    } else if (dev->path.type == DEVICE_PATH_PCI) {
        dev->ops = &mc_ops;
    }
}
> -Corey

Elia.

[Elia Yehuda]

well, i managed to make it work as you initially suggested (adding .ops_pci
to northbridge_operations),
but... i had to change the .device to 0x7124 (which is what lspci shows me)
- are you all sure 82810
device_id is 7120? i use 82810e but according to datasheet it should be the
same.

Elia.

On Tue, Nov 18, 2008 at 6:04 AM, Corey Osgood wrote:
> On Mon, Nov 17, 2008 at 10:10 PM, Elia Yehuda  wrote:
> > On Tue, Nov 18, 2008 at 3:44 AM, Joseph Smith wrote:
> > > On Tue, 18 Nov 2008 00:59:07 +0200, "Elia Yehuda" 
> > > wrote:
> > > > just noticed ive put it in cpu_bus_ops... have a laugh...
> > >
> > > No worries, this is how we learn :-)
> > > > but i dont have "static struct device_operations mc_ops" - where should
> > >
> > > i
> > > > add
> > > > the .ops_pci ?
> > >
> > > Little confused about what your trying to do here. This doesn't work??
> > >
> > > static const struct pci_driver northbridge_driver __pci_driver = {
> > >         .ops    = &northbridge_operations,
> > >         .vendor = PCI_VENDOR_ID_INTEL,
> > >         .device = 0x7120,
> > > };
> >
> > this works just fine. i simply don't know where to add ".ops_pci" since i
> > dont have
> > "static struct device_operations mc_ops" in northbridge.c
>
> You should be able to add it to pci_domain_ops, but be sure to check that
> it actually runs. Otherwise, you'll need to create that mc_ops driver with
> just the .ops_pci and everything else set to NULL, and use this:
>
> static void enable_dev(struct device *dev)
> {
>     struct device_path path; //unused?
> /* Set the operations if it is a special bus type */
> if (dev->path.type == DEVICE_PATH_PCI_DOMAIN) {
>     dev->ops = &pci_domain_ops;
>     pci_set_method(dev);
> } else if (dev->path.type == DEVICE_PATH_APIC_CLUSTER) {
>     dev->ops = &cpu_bus_ops;
> } else if (dev->path.type == DEVICE_PATH_PCI) {
>     dev->ops = &mc_ops;
> }
>
> }
>
> I hope you can understand what I'm trying to say, I'm a bit tired right
> now.
>
> -Corey

[Corey Osgood]

On Mon, Nov 17, 2008 at 11:45 PM, Elia Yehuda  wrote:
> well, i managed to make it work as you initially suggested (adding .ops_pci
> to northbridge_operations),
> but... i had to change the .device to 0x7124 (which is what lspci shows me)
> are you all sure 82810
>
> device_id is 7120? i use 82810e but according to datasheet it should be the
> same.

Nope, here's from my i810 datasheet:

DIDDevice Identification Register (Device 0)
Address Offset: 02–03h
Default Value:  82810 = 7120h
                82810-DC100 = 7122h
Attribute:      Read Only
Size:           16 bits

So, go ahead and do this:

static const struct pci_driver i810_northbridge_driver __pci_driver = {
        .ops    = &northbridge_operations,
        .vendor = PCI_VENDOR_ID_INTEL,
        .device = 0x7120,
};

static const struct pci_driver i810e_northbridge_driver __pci_driver = {
        .ops    = &northbridge_operations,
        .vendor = PCI_VENDOR_ID_INTEL,
        .device = 0x7124,
};

Please do not be tempted to add i810-dc100 in there as well, unless you want
to read through the i810 datasheet and fix any other differences ;)

-Corey
> Elia.
>
> On Tue, Nov 18, 2008 at 6:04 AM, Corey Osgood wrote:
> > On Mon, Nov 17, 2008 at 10:10 PM, Elia Yehuda  wrote:
> > > On Tue, Nov 18, 2008 at 3:44 AM, Joseph Smith wrote:
> > > > On Tue, 18 Nov 2008 00:59:07 +0200, "Elia Yehuda" 
> > > > wrote:
> > > > > just noticed ive put it in cpu_bus_ops... have a laugh...
> > > >
> > > > No worries, this is how we learn :-)
> > > > > but i dont have "static struct device_operations mc_ops" - where
> > > >
> > > > should i
> > > > > add
> > > > > the .ops_pci ?
> > > >
> > > > Little confused about what your trying to do here. This doesn't work??
> > > >
> > > > static const struct pci_driver northbridge_driver __pci_driver = {
> > > >         .ops    = &northbridge_operations,
> > > >         .vendor = PCI_VENDOR_ID_INTEL,
> > > >         .device = 0x7120,
> > > > };
> > >
> > > this works just fine. i simply don't know where to add ".ops_pci" since i
> > > dont have
> > > "static struct device_operations mc_ops" in northbridge.c
> >
> > You should be able to add it to pci_domain_ops, but be sure to check that
> > it actually runs. Otherwise, you'll need to create that mc_ops driver with
> > just the .ops_pci and everything else set to NULL, and use this:
> >
> > static void enable_dev(struct device *dev)
> > {
> >     struct device_path path; //unused?
> > /* Set the operations if it is a special bus type */
> > if (dev->path.type == DEVICE_PATH_PCI_DOMAIN) {
> >     dev->ops = &pci_domain_ops;
> >     pci_set_method(dev);
> > } else if (dev->path.type == DEVICE_PATH_APIC_CLUSTER) {
> >     dev->ops = &cpu_bus_ops;
> > } else if (dev->path.type == DEVICE_PATH_PCI) {
> >     dev->ops = &mc_ops;
> > }
> >
> > }
> >
> > I hope you can understand what I'm trying to say, I'm a bit tired right
> > now.
> >
> > -Corey