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

[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 <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.de  • http://www.coresystems.de/
Registergericht: Amtsgericht Freiburg • HRB 7656
Geschäftsführer: Stefan Reinauer • Ust-IdNr.: DE245674866