This patch implements support for the memory controller and PCIe interface of the Intel EP80579 Integrated Processor. The memory controller code supports only 64-bit-wide DIMMs with x8 devices and ECC. It has been tested on a development board using a single Micron MT9HTF6472PY-667D2 DIMM. Your mileage will definitely vary with other DIMMs.
Signed-off-by: Ed Swierk eswierk@arastra.com
On Wed, Aug 20, 2008 at 09:19:23AM -0700, Ed Swierk wrote:
This patch implements support for the memory controller and PCIe interface of the Intel EP80579 Integrated Processor. The memory controller code supports only 64-bit-wide DIMMs with x8 devices and ECC. It has been tested on a development board using a single Micron MT9HTF6472PY-667D2 DIMM. Your mileage will definitely vary with other DIMMs.
Signed-off-by: Ed Swierk eswierk@arastra.com
Index: coreboot-v2-3363/src/northbridge/intel/i3100/raminit_ep80579.c
--- /dev/null +++ coreboot-v2-3363/src/northbridge/intel/i3100/raminit_ep80579.c @@ -0,0 +1,862 @@ +/*
- This file is part of the coreboot project.
- Copyright (C) 2008 Arastra, Inc.
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License version 2 as
- published by the Free Software Foundation.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program; if not, write to the Free Software
- Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
- */
+/* This code is based on src/northbridge/intel/e7520/raminit.c */
We should research svn history for who wrote the e7520/raminit.c initially (if your code retains substantial parts of the original code), so we can add the proper (C) line...
Same for the stock i3100 raminit.c we have in svn now.
+static void sdram_set_registers(const struct mem_controller *ctrl) +{
- static const unsigned int register_values[] = {
PCI_ADDR(0, 0x00, 0, CKDIS), 0xffff0000, 0x0000ffff,PCI_ADDR(0, 0x00, 0, DEVPRES), 0x00000000, 0x07420801 | DEVPRES_CONFIG,PCI_ADDR(0, 0x00, 0, PAM-1), 0xcccccc7f, 0x33333000,PCI_ADDR(0, 0x00, 0, PAM+3), 0xcccccccc, 0x33333333,PCI_ADDR(0, 0x00, 0, DEVPRES1), 0xffffffff, 0x0040003a,PCI_ADDR(0, 0x00, 0, SMRBASE), 0x00000fff, BAR | 0,- };
- int i;
- int max;
- max = sizeof(register_values)/sizeof(register_values[0]);
ARRAY_SIZE
- for(i = 0; i < max; i += 3) {
device_t dev;unsigned where;unsigned long reg;dev = (register_values[i] & ~0xff) - PCI_DEV(0, 0x00, 0) + ctrl->f0;where = register_values[i] & 0xff;reg = pci_read_config32(dev, where);reg &= register_values[i+1];reg |= register_values[i+2];pci_write_config32(dev, where, reg);- }
+}
+struct dimm_size {
- unsigned long side1;
- unsigned long side2;
+};
This is pretty common, we have multiple copies of this struct definition in v2, can we move it somewhere globally?
+static struct dimm_size spd_get_dimm_size(unsigned device) +{
- /* Calculate the log base 2 size of a DIMM in bits */
- struct dimm_size sz;
- int value, low, ddr2;
- sz.side1 = 0;
- sz.side2 = 0;
- /* Note it might be easier to use byte 31 here, it has the DIMM size as
* a multiple of 4MB. The way we do it now we can size both* sides of an assymetric dimm.*/- value = spd_read_byte(device, 3); /* rows */
Please use the #defines from spd.h (or similar) here, instead of magic values such as the "3" and others below...
- if (value < 0) goto hw_err;
- if ((value & 0xf) == 0) goto val_err;
- sz.side1 += value & 0xf;
- value = spd_read_byte(device, 4); /* columns */
- if (value < 0) goto hw_err;
- if ((value & 0xf) == 0) goto val_err;
- sz.side1 += value & 0xf;
- value = spd_read_byte(device, 17); /* banks */
- if (value < 0) goto hw_err;
- if ((value & 0xff) == 0) goto val_err;
- sz.side1 += log2(value & 0xff);
- /* Get the module data width and convert it to a power of two */
- value = spd_read_byte(device, 7); /* (high byte) */
- if (value < 0) goto hw_err;
- value &= 0xff;
- value <<= 8;
- low = spd_read_byte(device, 6); /* (low byte) */
- if (low < 0) goto hw_err;
- value = value | (low & 0xff);
- if ((value != 72) && (value != 64)) goto val_err;
- sz.side1 += log2(value);
- /* side 2 */
- value = spd_read_byte(device, 5); /* number of ranks */
- if (value < 0) goto hw_err;
- value &= 7;
- value++;
- if (value == 1) goto out;
- if (value != 2) goto val_err;
- /* Start with the symmetrical case */
- sz.side2 = sz.side1;
- value = spd_read_byte(device, 3); /* rows */
- if (value < 0) goto hw_err;
- if ((value & 0xf0) == 0) goto out; /* If symmetrical we are done */
- sz.side2 -= (value & 0x0f); /* Subtract out rows on side 1 */
- sz.side2 += ((value >> 4) & 0x0f); /* Add in rows on side 2 */
- value = spd_read_byte(device, 4); /* columns */
- if (value < 0) goto hw_err;
- if ((value & 0xff) == 0) goto val_err;
- sz.side2 -= (value & 0x0f); /* Subtract out columns on side 1 */
- sz.side2 += ((value >> 4) & 0x0f); /* Add in columns on side 2 */
- goto out;
- val_err:
- die("Bad SPD value\r\n");
- /* If an hw_error occurs report that I have no memory */
- hw_err:
- sz.side1 = 0;
- sz.side2 = 0;
- out:
- print_debug("dimm ");
- print_debug_hex8(device);
- print_debug(" size = ");
- print_debug_hex8(sz.side1);
- print_debug(".");
- print_debug_hex8(sz.side2);
- print_debug("\r\n");
- return sz;
+}
+static long spd_set_ram_size(const struct mem_controller *ctrl, long dimm_mask) +{
- int i;
- int cum;
- for(i = cum = 0; i < DIMM_SOCKETS; i++) {
struct dimm_size sz;if (dimm_mask & (1 << i)) {sz = spd_get_dimm_size(ctrl->channel0[i]);if (sz.side1 < 29) {return -1; /* Report SPD error */}/* convert bits to multiples of 64MB */sz.side1 -= 29;cum += (1 << sz.side1);
/* DRB = 0x60 */
Comment not needed, the point of the DRB #define is that you don't have to know the value here ;-)
pci_write_config8(ctrl->f0, DRB + (i*2), cum);pci_write_config8(ctrl->f0, DRB+1 + (i*2), cum);if (spd_read_byte(ctrl->channel0[i], 5) & 0x1) {cum <<= 1;}}else {pci_write_config8(ctrl->f0, DRB + (i*2), cum);pci_write_config8(ctrl->f0, DRB+1 + (i*2), cum);}- }
- print_debug("DRB = ");
- print_debug_hex32(pci_read_config32(ctrl->f0, DRB));
- print_debug("\r\n");
- cum >>= 1;
- /* set TOM top of memory 0xcc */
- pci_write_config16(ctrl->f0, TOM, cum);
- print_debug("TOM = ");
- print_debug_hex16(cum);
- print_debug("\r\n");
- /* set TOLM top of low memory */
- if(cum > 0x18) {
cum = 0x18;- }
- cum <<= 11;
- /* 0xc4 TOLM */
- pci_write_config16(ctrl->f0, TOLM, cum);
- print_debug("TOLM = ");
- print_debug_hex16(cum);
- print_debug("\r\n");
- return 0;
+}
+static unsigned int spd_detect_dimms(const struct mem_controller *ctrl) +{
- unsigned dimm_mask;
- int i;
- dimm_mask = 0;
- for(i = 0; i < DIMM_SOCKETS; i++) {
int byte;unsigned device;device = ctrl->channel0[i];if (device) {byte = spd_read_byte(device, 2); /* Type */print_debug("spd ");print_debug_hex8(device);print_debug(" = ");print_debug_hex8(byte);print_debug("\r\n");if (byte == 8) {dimm_mask |= (1 << i);}}- }
- return dimm_mask;
+}
+static int spd_set_row_attributes(const struct mem_controller *ctrl,
long dimm_mask)
How long must this mask be? Can we use u64 or maybe u32 here? I assume the length is a fixed, known value?
+{
- int value;
- int i;
- for (i = 0; i < DIMM_SOCKETS; i++) {
uint32_t dra = 0;int reg = 0;if (!(dimm_mask & (1 << i))) {continue;}value = spd_read_byte(ctrl->channel0[i], 3); /* rows */if (value < 0) die("Bad SPD data\r\n");if ((value & 0xf) == 0) die("Invalid # of rows\r\n");dra |= (((value-13) & 0x7) << 23);dra |= (((value-13) & 0x7) << 29);reg += value & 0xf;value = spd_read_byte(ctrl->channel0[i], 4); /* columns */if (value < 0) die("Bad SPD data\r\n");if ((value & 0xf) == 0) die("Invalid # of columns\r\n");dra |= (((value-10) & 0x7) << 20);dra |= (((value-10) & 0x7) << 26);reg += value & 0xf;value = spd_read_byte(ctrl->channel0[i], 17); /* banks */if (value < 0) die("Bad SPD data\r\n");if ((value & 0xff) == 0) die("Invalid # of banks\r\n");reg += log2(value & 0xff);print_debug("dimm ");print_debug_hex8(i);print_debug(" reg = ");print_debug_hex8(reg);print_debug("\r\n");/* set device density */dra |= ((31-reg));dra |= ((31-reg) << 6);/* set device width (x8) */dra |= (1 << 4);dra |= (1 << 10);/* set device type (registered) */dra |= (1 << 14);/* set number of ranks (0=single, 1=dual) */value = spd_read_byte(ctrl->channel0[i], 5);dra |= ((value & 0x1) << 17);print_debug("DRA");print_debug_hex8(i);print_debug(" = ");print_debug_hex32(dra);print_debug("\r\n");/* 0x70 DRA */pci_write_config32(ctrl->f0, DRA + (i*4), dra);- }
- return 0;
+}
+static uint32_t spd_set_drt_attributes(const struct mem_controller *ctrl,
long dimm_mask, uint32_t drc)+{
- int i;
- uint32_t val, val1;
- uint32_t cl;
- uint32_t trc = 0;
- uint32_t trfc = 0;
- uint32_t tras = 0;
- uint32_t trtp = 0;
- uint32_t twtr = 0;
- int index = drc & 0x00000003;
- int ci;
- static const uint8_t latencies[] = { /* 533, 800, 400, 667 */
0x10, 0x60, 0x10, 0x20 };- static const uint32_t drt0[] = { /* 533, 800, 400, 667 */
0x24240002, 0x24360002, 0x24220002, 0x24360002 };- static const uint32_t drt1[] = { /* 533, 800, 400, 667 */
0x00400000, 0x00900000, 0x00200000, 0x00700000 };- static const uint32_t magic[] = { /* 533, 800, 400, 667 */
0x007b8221, 0x00b94331, 0x005ca1a1, 0x009a62b1 };- static const uint32_t mrs[] = { /* 533, 800, 400, 667 */
0x07020000, 0x0b020000, 0x05020000, 0x09020000 };- static const int cycle[] = { /* 533, 800, 400, 667 */
15, 10, 20, 12 }; /* cycle time in 1/4 ns units */- static const int byte40rem[] = {
0, 1, 2, 2, 3, 3, 0, 0 }; /* byte 40 remainder in 1/4 ns units */- /* CAS latency in cycles */
- val = latencies[index];
- for (i = 0; i < DIMM_SOCKETS; i++) {
if (!(dimm_mask & (1 << i)))continue;val &= spd_read_byte(ctrl->channel0[i], 18);- }
- if (val & 0x10)
cl = 4;- else if (val & 0x20)
cl = 5;- else if (val & 0x40)
cl = 6;- else
die("CAS latency mismatch\r\n");- print_debug("cl = ");
- print_debug_hex8(cl);
- print_debug("\r\n");
- ci = cycle[index];
- /* Trc, Trfc in cycles */
- for (i = 0; i < DIMM_SOCKETS; i++) {
if (!(dimm_mask & (1 << i)))continue;val1 = spd_read_byte(ctrl->channel0[i], 40);val = spd_read_byte(ctrl->channel0[i], 41);val <<= 2; /* convert to 1/4 ns */val += byte40rem[(val1 >> 4) & 0x7];val = (val + ci - 1) / ci + 1; /* convert to cycles */if (trc < val)trc = val;val = spd_read_byte(ctrl->channel0[i], 42);val <<= 2; /* convert to 1/4 ns */if (val1 & 0x01)val += 1024;val += byte40rem[(val1 >> 1) & 0x7];val = (val + ci - 1) / ci; /* convert to cycles */if (trfc < val)trfc = val;- }
- print_debug("trc = ");
- print_debug_hex8(trc);
- print_debug("\r\n");
- print_debug("trfc = ");
- print_debug_hex8(trfc);
- print_debug("\r\n");
- /* Tras, Trtp, Twtr in cycles */
- for (i = 0; i < DIMM_SOCKETS; i++) {
if (!(dimm_mask & (1 << i)))continue;val = spd_read_byte(ctrl->channel0[i], 30);val <<= 2; /* convert to 1/4 ns */val = (val + ci - 1) / ci; /* convert to cycles */if (tras < val)tras = val;val = spd_read_byte(ctrl->channel0[i], 38);val = (val + ci - 1) / ci; /* convert to cycles */if (trtp < val)trtp = val;val = spd_read_byte(ctrl->channel0[i], 37);val = (val + ci - 1) / ci; /* convert to cycles */if (twtr < val)twtr = val;- }
- print_debug("tras = ");
- print_debug_hex8(tras);
- print_debug("\r\n");
- print_debug("trtp = ");
- print_debug_hex8(trtp);
- print_debug("\r\n");
- print_debug("twtr = ");
- print_debug_hex8(twtr);
- print_debug("\r\n");
- val = (drt0[index] | ((trc - 11) << 12) | ((cl - 3) << 9)
| ((cl - 3) << 6) | ((cl - 3) << 3));- print_debug("drt0 = ");
- print_debug_hex32(val);
- print_debug("\r\n");
- pci_write_config32(ctrl->f0, DRT0, val);
- val = (drt1[index] | ((tras - 8) << 28) | ((trtp - 2) << 25)
| (twtr << 15));- print_debug("drt1 = ");
- print_debug_hex32(val);
- print_debug("\r\n");
- pci_write_config32(ctrl->f0, DRT1, val);
- val = (magic[index]);
- print_debug("magic = ");
- print_debug_hex32(val);
- print_debug("\r\n");
- pci_write_config32(PCI_DEV(0, 0x08, 0), 0xcc, val);
- val = (mrs[index] | (cl << 20));
- print_debug("mrs = ");
- print_debug_hex32(val);
- print_debug("\r\n");
- return val;
+}
+static int spd_set_dram_controller_mode(const struct mem_controller *ctrl,
long dimm_mask)+{
- int value;
- int drc = 0;
- int i;
- msr_t msr;
- uint8_t cycle = 0x25;
- /* 0x7c DRC */
- for (i = 0; i < DIMM_SOCKETS; i++) {
if (!(dimm_mask & (1 << i)))continue;if ((spd_read_byte(ctrl->channel0[i], 6) & 0xf0) != 0x40)die("ERROR: Only 64-bit DIMMs supported\r\n");if (!(spd_read_byte(ctrl->channel0[i], 11) & 0x02))die("ERROR: Only ECC DIMMs supported\r\n");if (spd_read_byte(ctrl->channel0[i], 13) != 0x08)die("ERROR: Only x8 DIMMs supported\r\n");value = spd_read_byte(ctrl->channel0[i], 9); /* cycle time */if (value > cycle)cycle = value;- }
- print_debug("cycle = ");
- print_debug_hex8(cycle);
- print_debug("\r\n");
- drc |= (1 << 20); /* enable ECC */
- drc |= (3 << 30); /* enable CKE on each dimm */
- drc |= (1 << 4); /* enable CKE globally */
- /* eswierk check: */
Done already? Remove? Turn into "TODO"?
- /* set front side bus speed */
- msr = rdmsr(0xcd); /* returns 0 on Pentium M 90nm */
- print_debug("msr 0xcd = ");
- print_debug_hex32(msr.hi);
- print_debug_hex32(msr.lo);
- print_debug("\r\n");
- /* eswierk check that this msr really indicates fsb speed! */
Ditto.
- if (msr.lo & 0x07) {
print_info("533 MHz FSB\r\n");if (cycle <= 0x25) {drc |= 0x5;print_info("400 MHz DDR\r\n");} else if (cycle <= 0x30) {drc |= 0x7;print_info("333 MHz DDR\r\n");} else if (cycle <= 0x3d) {drc |= 0x4;print_info("266 MHz DDR\r\n");} else {drc |= 0x2;print_info("200 MHz DDR\r\n");}- }
- else {
print_info("400 MHz FSB\r\n");if (cycle <= 0x30) {drc |= 0x7;print_info("333 MHz DDR\r\n");} else if (cycle <= 0x3d) {drc |= 0x0;print_info("266 MHz DDR\r\n");} else {drc |= 0x2;print_info("200 MHz DDR\r\n");}- }
- print_debug("DRC = ");
- print_debug_hex32(drc);
- print_debug("\r\n");
- return drc;
+}
+static void sdram_set_spd_registers(const struct mem_controller *ctrl) +{
- long dimm_mask;
- int i;
- /* Test if we can read the spd */
- dimm_mask = spd_detect_dimms(ctrl);
- if (!(dimm_mask & ((1 << DIMM_SOCKETS) - 1))) {
print_err("No memory for this cpu\r\n");return;- }
- return;
+}
+static void do_delay(void) +{
- int i;
- unsigned char b;
- for(i=0;i<16;i++)
b=inb(0x80);+}
Please use the global delay function, no need for yet another one.
+#define TIMEOUT_LOOPS 300000
+#define DCALCSR 0x040 +#define DCALADDR 0x044 +#define DCALDATA 0x048 +#define MBCSR 0x140 +#define MBADDR 0x144 +#define MBDATA 0x148 +#define DDRIOMC2 0x268
Move to some *.h file?
+static void set_on_dimm_termination_enable(const struct mem_controller *ctrl) +{
- unsigned char c1,c2;
unsigned int dimm,i;unsigned int data32;- unsigned int t4;
- /* Set up northbridge values */
- /* ODT enable */
- pci_write_config32(ctrl->f0, SDRC, 0xa0002c30);
- c1 = pci_read_config8(ctrl->f0, DRB);
- c2 = pci_read_config8(ctrl->f0, DRB+2);
- if (c1 == c2) {
/* 1 single-rank DIMM */data32 = 0x00000010;- }
- else {
/* 2 single-rank DIMMs or 1 double-rank DIMM */data32 = 0x00002010;- }
- print_debug("ODT Value = ");
- print_debug_hex32(data32);
- print_debug("\r\n");
- pci_write_config32(ctrl->f0, DDR2ODTC, data32);
- for(i=0;i<2;i++) {
print_debug("ODT CS");print_debug_hex8(i);print_debug("\r\n");write32(BAR+DCALADDR, 0x0b840001);write32(BAR+DCALCSR, 0x80000003 | ((i+1)<<21));data32 = read32(BAR+DCALCSR);while (data32 & 0x80000000)data32 = read32(BAR+DCALCSR);- }
+}
+static void dump_dcal_regs(void) +{
- int i;
- for (i = 0x0; i < 0x2a0; i += 4) {
if ((i % 16) == 0) {print_debug("\r\n");print_debug_hex16(i);print_debug(": ");}print_debug_hex32(read32(BAR+i));print_debug(" ");- }
- print_debug("\r\n");
+}
+static void sdram_enable(int controllers, const struct mem_controller *ctrl) +{
- int i;
- int cs;
- long mask;
- uint32_t drc;
- uint32_t data32;
- uint32_t mode_reg;
- msr_t msr;
- uint16_t data16;
- mask = spd_detect_dimms(ctrl);
- print_debug("Starting SDRAM Enable\r\n");
- /* set dram type and Front Side Bus freq. */
- drc = spd_set_dram_controller_mode(ctrl, mask);
- if (drc == 0) {
die("Error calculating DRC\r\n");- }
- data32 = drc & ~(3 << 20); /* clear ECC mode */
- data32 = data32 | (3 << 5); /* temp turn off ODT */
- /* Set dram controller mode */
- /* 0x7c DRC */
- pci_write_config32(ctrl->f0, DRC, data32);
/* turn the clocks on */- /* 0x8c CKDIS */
- pci_write_config16(ctrl->f0, CKDIS, 0x0000);
/* program row size DRB */- spd_set_ram_size(ctrl, mask);
/* program row attributes DRA */- spd_set_row_attributes(ctrl, mask);
/* program DRT timing values */- mode_reg = spd_set_drt_attributes(ctrl, mask, drc);
The indentation of the comments here, and the coding style in general are not quite correct in various places. Let's commit this anyway for now, I'll do an 'indent'-run on the code later though, invoked as listed here: http://www.coreboot.org/Development_Guidelines#Coding_Style
- dump_dcal_regs();
- for(cs=0;cs<2;cs++) { /* loop through each dimm to test */
print_debug("NOP CS");print_debug_hex8(cs);print_debug("\r\n");/* Apply NOP */
Wrong comment (or comment in the wrong place)? This only does a delay, no?
do_delay();write32(BAR+DCALCSR, (0x00000000 | ((cs+1)<<21)));write32(BAR+DCALCSR, (0x80000000 | ((cs+1)<<21)));data32 = read32(BAR+DCALCSR);while(data32 & 0x80000000)data32 = read32(BAR+DCALCSR);- }
- /* Apply NOP */
Ditto.
- do_delay();
- for(cs=0;cs<2;cs++) {
print_debug("NOP CS");print_debug_hex8(cs);print_debug("\r\n");write32(BAR + DCALCSR, (0x80000000 | ((cs+1)<<21)));data32 = read32(BAR+DCALCSR);while(data32 & 0x80000000)data32 = read32(BAR+DCALCSR);- }
- /* Precharge all banks */
- do_delay();
- for(cs=0;cs<2;cs++) {
print_debug("Precharge CS");print_debug_hex8(cs);print_debug("\r\n");write32(BAR+DCALADDR, 0x04000000);write32(BAR+DCALCSR, (0x80000002 | ((cs+1)<<21)));data32 = read32(BAR+DCALCSR);while(data32 & 0x80000000)data32 = read32(BAR+DCALCSR);- }
- /* EMRS: Enable DLLs, set OCD calibration mode to default */
- do_delay();
- for(cs=0;cs<2;cs++) {
print_debug("EMRS CS");print_debug_hex8(cs);print_debug("\r\n");write32(BAR+DCALADDR, 0x0b840001);write32(BAR+DCALCSR, (0x80000003 | ((cs+1)<<21)));data32 = read32(BAR+DCALCSR);while(data32 & 0x80000000)data32 = read32(BAR+DCALCSR);- }
- /* MRS: Reset DLLs */
- do_delay();
- for(cs=0;cs<2;cs++) {
print_debug("MRS CS");print_debug_hex8(cs);print_debug("\r\n");write32(BAR+DCALADDR, mode_reg);write32(BAR+DCALCSR, (0x80000003 | ((cs+1)<<21)));data32 = read32(BAR+DCALCSR);while(data32 & 0x80000000)data32 = read32(BAR+DCALCSR);- }
- /* Precharge all banks */
- do_delay();
- do_delay();
- do_delay();
- for(cs=0;cs<2;cs++) {
print_debug("Precharge CS");print_debug_hex8(cs);print_debug("\r\n");write32(BAR+DCALADDR, 0x04000000);write32(BAR+DCALCSR, (0x80000002 | ((cs+1)<<21)));data32 = read32(BAR+DCALCSR);while(data32 & 0x80000000)data32 = read32(BAR+DCALCSR);- }
- /* Do 2 refreshes */
- do_delay();
- for(cs=0;cs<2;cs++) {
print_debug("Refresh CS");print_debug_hex8(cs);print_debug("\r\n");write32(BAR+DCALCSR, (0x80000001 | ((cs+1)<<21)));data32 = read32(BAR+DCALCSR);while(data32 & 0x80000000)data32 = read32(BAR+DCALCSR);- }
- do_delay();
- for(cs=0;cs<2;cs++) {
print_debug("Refresh CS");print_debug_hex8(cs);print_debug("\r\n");write32(BAR+DCALCSR, (0x80000001 | ((cs+1)<<21)));data32 = read32(BAR+DCALCSR);while(data32 & 0x80000000)data32 = read32(BAR+DCALCSR);- }
As far as I can see both blocks are identical? Why not a 'for' loop then? This would also make a smaller coreboot (binary) image (less code/printfs, less strings for the debug output, etc.).
Are there code issues or romcc issues?
- do_delay();
- /* for good luck do 6 more */
Better comment please, e.g. mention the reason why multiple runs are needed and why 6 and not some other number. I assume the SPD/RAM/NB datasheet says so?
- for(cs=0;cs<2;cs++) {
write32(BAR+DCALCSR, (0x80000001 | ((cs+1)<<21)));- }
- do_delay();
- for(cs=0;cs<2;cs++) {
write32(BAR+DCALCSR, (0x80000001 | ((cs+1)<<21)));- }
- do_delay();
- for(cs=0;cs<2;cs++) {
write32(BAR+DCALCSR, (0x80000001 | ((cs+1)<<21)));- }
- do_delay();
- for(cs=0;cs<2;cs++) {
write32(BAR+DCALCSR, (0x80000001 | ((cs+1)<<21)));- }
- do_delay();
- for(cs=0;cs<2;cs++) {
write32(BAR+DCALCSR, (0x80000001 | ((cs+1)<<21)));- }
- do_delay();
- for(cs=0;cs<2;cs++) {
write32(BAR+DCALCSR, (0x80000001 | ((cs+1)<<21)));- }
- do_delay();
Same as above, please pack this into a loop if possible.
- /* MRS: Set DLLs to normal */
- do_delay();
- for(cs=0;cs<2;cs++) {
print_debug("MRS CS");print_debug_hex8(cs);print_debug("\r\n");write32(BAR+DCALADDR, (mode_reg & ~(1<<24)));write32(BAR+DCALCSR, (0x80000003 | ((cs+1)<<21)));data32 = read32(BAR+DCALCSR);while(data32 & 0x80000000)data32 = read32(BAR+DCALCSR);- }
- /* EMRS: Enable DLLs */
- do_delay();
- for(cs=0;cs<2;cs++) {
print_debug("EMRS CS");print_debug_hex8(cs);print_debug("\r\n");write32(BAR+DCALADDR, 0x0b840001);write32(BAR+DCALCSR, (0x80000003 | ((cs+1)<<21)));data32 = read32(BAR+DCALCSR);while(data32 & 0x80000000)data32 = read32(BAR+DCALCSR);}- do_delay();
- /* No command */
- write32(BAR+DCALCSR, 0x0000000f);
- write32(BAR, 0x00100000);
- /* enable on dimm termination */
- set_on_dimm_termination_enable(ctrl);
- dump_dcal_regs();
- /* receive enable calibration */
- do_delay();
- for(cs=0;cs<1;cs++) {
print_debug("receive enable calibration CS");print_debug_hex8(cs);print_debug("\r\n");write32(BAR+DCALCSR, (0x8000000c | ((cs+1)<<21)));data32 = read32(BAR+DCALCSR);while(data32 & 0x80000000)data32 = read32(BAR+DCALCSR);- }
- dump_dcal_regs();
- /* Adjust RCOMP */
- data32 = read32(BAR+DDRIOMC2);
- data32 &= ~(0xf << 16);
- data32 |= (0xb << 16);
- write32(BAR+DDRIOMC2, data32);
- dump_dcal_regs();
- /* 0x7c DRC */
- data32 = drc & ~(3 << 20); /* clear ECC mode */
- pci_write_config32(ctrl->f0, DRC, data32);
- write32(BAR+DCALCSR, 0x0008000f);
- /* Clear memory and init ECC */
- for(cs=0;cs<2;cs++) {
if (!(mask & (1<<cs)))continue;print_debug("clear memory CS");print_debug_hex8(cs);print_debug("\r\n");write32(BAR+MBCSR, 0xa00000f0 | ((cs+1)<<20) | (0<<16));data32 = read32(BAR+MBCSR);while(data32 & 0x80000000)data32 = read32(BAR+MBCSR);if(data32 & 0x40000000)print_debug("failed!\r\n");- }
- /* Clear read/write FIFO pointers */
- print_debug("clear read/write fifo pointers\r\n");
- write32(BAR+DDRIOMC2, read32(BAR+DDRIOMC2) | (1<<15));
- do_delay();
- write32(BAR+DDRIOMC2, read32(BAR+DDRIOMC2) & ~(1<<15));
- do_delay();
- dump_dcal_regs();
- print_debug("Done\r\n");
- /* Set initialization complete */
- /* 0x7c DRC */
- drc |= (1 << 29);
- drc |= (3 << 30);
- data32 = drc & ~(3 << 20); /* clear ECC mode */
- pci_write_config32(ctrl->f0, DRC, data32);
- /* Set the ECC mode */
- pci_write_config32(ctrl->f0, DRC, drc);
- /* The memory is now setup, use it */
- cache_lbmem(MTRR_TYPE_WRBACK);
+}
+static inline int memory_initialized(void) +{
- return pci_read_config32(PCI_DEV(0, 0x00, 0), DRC) & (1 << 29);
+} Index: coreboot-v2-3363/src/northbridge/intel/i3100/raminit_ep80579.h =================================================================== --- /dev/null +++ coreboot-v2-3363/src/northbridge/intel/i3100/raminit_ep80579.h @@ -0,0 +1,33 @@ +/*
- This file is part of the coreboot project.
- Copyright (C) 2008 Arastra, Inc.
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License version 2 as
- published by the Free Software Foundation.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program; if not, write to the Free Software
- Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
- */
+/* This code is based on src/northbridge/intel/e7520/raminit.h */
Nah, drop this, too trivial. This is soleley (C) Arastra, IMHO.
+#ifndef NORTHBRIDGE_INTEL_EP80579_RAMINIT_H +#define NORTHBRIDGE_INTEL_EP80579_RAMINIT_H
NORTHBRIDGE_INTEL_I3100_RAMINIT_EP80579_H for consistency.
+#define DIMM_SOCKETS 2 +struct mem_controller {
- unsigned node_id;
uint16_t or u16 here, as also done below?
- device_t f0;
- uint16_t channel0[DIMM_SOCKETS];
+};
+#endif Index: coreboot-v2-3363/src/northbridge/intel/i3100/ep80579.h =================================================================== --- /dev/null +++ coreboot-v2-3363/src/northbridge/intel/i3100/ep80579.h @@ -0,0 +1,61 @@ +/*
- This file is part of the coreboot project.
- Copyright (C) 2008 Arastra, Inc.
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License version 2 as
- published by the Free Software Foundation.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program; if not, write to the Free Software
- Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
- */
+#ifndef NORTHBRIDGE_INTEL_EP80579_H +#define NORTHBRIDGE_INTEL_EP80579_H
NORTHBRIDGE_INTEL_I3100_EP80579_H for consistency.
+#define VID 0x00 +#define DID 0x02 +#define PCICMD 0x04 +#define PCISTS 0x06 +#define RID 0x08
These five are generic and already in pci.h I think (and not used in your code anyway), I'd drop them.
+#define SMRBASE 0x14 +#define MCHCFG0 0x50 +#define FDHC 0x58 +#define PAM 0x59 +#define DRB 0x60 +#define DRT1 0x64 +#define DRA 0x70 +#define DRT0 0x78 +#define DRC 0x7c +#define ECCDIAG 0x84 +#define SDRC 0x88 +#define CKDIS 0x8c +#define CKEDIS 0x8d +#define DEVPRES 0x9c +#define DEVPRES_D0F0 (1 << 0) +#define DEVPRES_D1F0 (1 << 1) +#define DEVPRES_D2F0 (1 << 2) +#define DEVPRES_D3F0 (1 << 3) +#define DEVPRES_D4F0 (1 << 4) +#define DEVPRES_D10F0 (1 << 5) +#define EXSMRC 0x9d +#define SMRAM 0x9e +#define EXSMRAMC 0x9f +#define DDR2ODTC 0xb0 +#define TOLM 0xc4 +#define REMAPBASE 0xc6 +#define REMAPLIMIT 0xc8 +#define REMAPOFFSET 0xca +#define TOM 0xcc +#define HECBASE 0xce +#define DEVPRES1 0xf4
+#endif
Uwe.
Thanks for the feedback. I have addressed most of your comments; please see the attached patch.
I will address your comment about the duplicate struct dimm_size declaration separately. Would it make sense to put it in a file common to all Intel northbridges, say, src/northbridge/intel/mem.h, and then remove it from the various raminit.c's?
Signed-off-by: Ed Swierk eswierk@arastra.com
--Ed
On Tue, Aug 26, 2008 at 11:05 AM, Uwe Hermann uwe@hermann-uwe.de wrote:
On Wed, Aug 20, 2008 at 09:19:23AM -0700, Ed Swierk wrote:
This patch implements support for the memory controller and PCIe interface of the Intel EP80579 Integrated Processor. The memory controller code supports only 64-bit-wide DIMMs with x8 devices and ECC. It has been tested on a development board using a single Micron MT9HTF6472PY-667D2 DIMM. Your mileage will definitely vary with other DIMMs.
Signed-off-by: Ed Swierk eswierk@arastra.com
Index: coreboot-v2-3363/src/northbridge/intel/i3100/raminit_ep80579.c
--- /dev/null +++ coreboot-v2-3363/src/northbridge/intel/i3100/raminit_ep80579.c @@ -0,0 +1,862 @@ +/*
- This file is part of the coreboot project.
- Copyright (C) 2008 Arastra, Inc.
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License version 2 as
- published by the Free Software Foundation.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program; if not, write to the Free Software
- Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
- */
+/* This code is based on src/northbridge/intel/e7520/raminit.c */
We should research svn history for who wrote the e7520/raminit.c initially (if your code retains substantial parts of the original code), so we can add the proper (C) line...
Same for the stock i3100 raminit.c we have in svn now.
+static void sdram_set_registers(const struct mem_controller *ctrl) +{
static const unsigned int register_values[] = {PCI_ADDR(0, 0x00, 0, CKDIS), 0xffff0000, 0x0000ffff,PCI_ADDR(0, 0x00, 0, DEVPRES), 0x00000000, 0x07420801 | DEVPRES_CONFIG,PCI_ADDR(0, 0x00, 0, PAM-1), 0xcccccc7f, 0x33333000,PCI_ADDR(0, 0x00, 0, PAM+3), 0xcccccccc, 0x33333333,PCI_ADDR(0, 0x00, 0, DEVPRES1), 0xffffffff, 0x0040003a,PCI_ADDR(0, 0x00, 0, SMRBASE), 0x00000fff, BAR | 0,};int i;int max;max = sizeof(register_values)/sizeof(register_values[0]);ARRAY_SIZE
for(i = 0; i < max; i += 3) {device_t dev;unsigned where;unsigned long reg;dev = (register_values[i] & ~0xff) - PCI_DEV(0, 0x00, 0) + ctrl->f0;where = register_values[i] & 0xff;reg = pci_read_config32(dev, where);reg &= register_values[i+1];reg |= register_values[i+2];pci_write_config32(dev, where, reg);}+}
+struct dimm_size {
unsigned long side1;unsigned long side2;+};
This is pretty common, we have multiple copies of this struct definition in v2, can we move it somewhere globally?
+static struct dimm_size spd_get_dimm_size(unsigned device) +{
/* Calculate the log base 2 size of a DIMM in bits */struct dimm_size sz;int value, low, ddr2;sz.side1 = 0;sz.side2 = 0;/* Note it might be easier to use byte 31 here, it has the DIMM size as* a multiple of 4MB. The way we do it now we can size both* sides of an assymetric dimm.*/value = spd_read_byte(device, 3); /* rows */Please use the #defines from spd.h (or similar) here, instead of magic values such as the "3" and others below...
if (value < 0) goto hw_err;if ((value & 0xf) == 0) goto val_err;sz.side1 += value & 0xf;value = spd_read_byte(device, 4); /* columns */if (value < 0) goto hw_err;if ((value & 0xf) == 0) goto val_err;sz.side1 += value & 0xf;value = spd_read_byte(device, 17); /* banks */if (value < 0) goto hw_err;if ((value & 0xff) == 0) goto val_err;sz.side1 += log2(value & 0xff);/* Get the module data width and convert it to a power of two */value = spd_read_byte(device, 7); /* (high byte) */if (value < 0) goto hw_err;value &= 0xff;value <<= 8;low = spd_read_byte(device, 6); /* (low byte) */if (low < 0) goto hw_err;value = value | (low & 0xff);if ((value != 72) && (value != 64)) goto val_err;sz.side1 += log2(value);/* side 2 */value = spd_read_byte(device, 5); /* number of ranks */if (value < 0) goto hw_err;value &= 7;value++;if (value == 1) goto out;if (value != 2) goto val_err;/* Start with the symmetrical case */sz.side2 = sz.side1;value = spd_read_byte(device, 3); /* rows */if (value < 0) goto hw_err;if ((value & 0xf0) == 0) goto out; /* If symmetrical we are done */sz.side2 -= (value & 0x0f); /* Subtract out rows on side 1 */sz.side2 += ((value >> 4) & 0x0f); /* Add in rows on side 2 */value = spd_read_byte(device, 4); /* columns */if (value < 0) goto hw_err;if ((value & 0xff) == 0) goto val_err;sz.side2 -= (value & 0x0f); /* Subtract out columns on side 1 */sz.side2 += ((value >> 4) & 0x0f); /* Add in columns on side 2 */goto out;- val_err:
die("Bad SPD value\r\n");/* If an hw_error occurs report that I have no memory */- hw_err:
sz.side1 = 0;sz.side2 = 0;- out:
print_debug("dimm ");print_debug_hex8(device);print_debug(" size = ");print_debug_hex8(sz.side1);print_debug(".");print_debug_hex8(sz.side2);print_debug("\r\n");return sz;+}
+static long spd_set_ram_size(const struct mem_controller *ctrl, long dimm_mask) +{
int i;int cum;for(i = cum = 0; i < DIMM_SOCKETS; i++) {struct dimm_size sz;if (dimm_mask & (1 << i)) {sz = spd_get_dimm_size(ctrl->channel0[i]);if (sz.side1 < 29) {return -1; /* Report SPD error */}/* convert bits to multiples of 64MB */sz.side1 -= 29;cum += (1 << sz.side1);
/* DRB = 0x60 */Comment not needed, the point of the DRB #define is that you don't have to know the value here ;-)
pci_write_config8(ctrl->f0, DRB + (i*2), cum);pci_write_config8(ctrl->f0, DRB+1 + (i*2), cum);if (spd_read_byte(ctrl->channel0[i], 5) & 0x1) {cum <<= 1;}}else {pci_write_config8(ctrl->f0, DRB + (i*2), cum);pci_write_config8(ctrl->f0, DRB+1 + (i*2), cum);}}print_debug("DRB = ");print_debug_hex32(pci_read_config32(ctrl->f0, DRB));print_debug("\r\n");cum >>= 1;/* set TOM top of memory 0xcc */pci_write_config16(ctrl->f0, TOM, cum);print_debug("TOM = ");print_debug_hex16(cum);print_debug("\r\n");/* set TOLM top of low memory */if(cum > 0x18) {cum = 0x18;}cum <<= 11;/* 0xc4 TOLM */pci_write_config16(ctrl->f0, TOLM, cum);print_debug("TOLM = ");print_debug_hex16(cum);print_debug("\r\n");return 0;+}
+static unsigned int spd_detect_dimms(const struct mem_controller *ctrl) +{
unsigned dimm_mask;int i;dimm_mask = 0;for(i = 0; i < DIMM_SOCKETS; i++) {int byte;unsigned device;device = ctrl->channel0[i];if (device) {byte = spd_read_byte(device, 2); /* Type */print_debug("spd ");print_debug_hex8(device);print_debug(" = ");print_debug_hex8(byte);print_debug("\r\n");if (byte == 8) {dimm_mask |= (1 << i);}}}return dimm_mask;+}
+static int spd_set_row_attributes(const struct mem_controller *ctrl,
long dimm_mask)How long must this mask be? Can we use u64 or maybe u32 here? I assume the length is a fixed, known value?
+{
int value;int i;for (i = 0; i < DIMM_SOCKETS; i++) {uint32_t dra = 0;int reg = 0;if (!(dimm_mask & (1 << i))) {continue;}value = spd_read_byte(ctrl->channel0[i], 3); /* rows */if (value < 0) die("Bad SPD data\r\n");if ((value & 0xf) == 0) die("Invalid # of rows\r\n");dra |= (((value-13) & 0x7) << 23);dra |= (((value-13) & 0x7) << 29);reg += value & 0xf;value = spd_read_byte(ctrl->channel0[i], 4); /* columns */if (value < 0) die("Bad SPD data\r\n");if ((value & 0xf) == 0) die("Invalid # of columns\r\n");dra |= (((value-10) & 0x7) << 20);dra |= (((value-10) & 0x7) << 26);reg += value & 0xf;value = spd_read_byte(ctrl->channel0[i], 17); /* banks */if (value < 0) die("Bad SPD data\r\n");if ((value & 0xff) == 0) die("Invalid # of banks\r\n");reg += log2(value & 0xff);print_debug("dimm ");print_debug_hex8(i);print_debug(" reg = ");print_debug_hex8(reg);print_debug("\r\n");/* set device density */dra |= ((31-reg));dra |= ((31-reg) << 6);/* set device width (x8) */dra |= (1 << 4);dra |= (1 << 10);/* set device type (registered) */dra |= (1 << 14);/* set number of ranks (0=single, 1=dual) */value = spd_read_byte(ctrl->channel0[i], 5);dra |= ((value & 0x1) << 17);print_debug("DRA");print_debug_hex8(i);print_debug(" = ");print_debug_hex32(dra);print_debug("\r\n");/* 0x70 DRA */pci_write_config32(ctrl->f0, DRA + (i*4), dra);}return 0;+}
+static uint32_t spd_set_drt_attributes(const struct mem_controller *ctrl,
long dimm_mask, uint32_t drc)+{
int i;uint32_t val, val1;uint32_t cl;uint32_t trc = 0;uint32_t trfc = 0;uint32_t tras = 0;uint32_t trtp = 0;uint32_t twtr = 0;int index = drc & 0x00000003;int ci;static const uint8_t latencies[] = { /* 533, 800, 400, 667 */0x10, 0x60, 0x10, 0x20 };static const uint32_t drt0[] = { /* 533, 800, 400, 667 */0x24240002, 0x24360002, 0x24220002, 0x24360002 };static const uint32_t drt1[] = { /* 533, 800, 400, 667 */0x00400000, 0x00900000, 0x00200000, 0x00700000 };static const uint32_t magic[] = { /* 533, 800, 400, 667 */0x007b8221, 0x00b94331, 0x005ca1a1, 0x009a62b1 };static const uint32_t mrs[] = { /* 533, 800, 400, 667 */0x07020000, 0x0b020000, 0x05020000, 0x09020000 };static const int cycle[] = { /* 533, 800, 400, 667 */15, 10, 20, 12 }; /* cycle time in 1/4 ns units */static const int byte40rem[] = {0, 1, 2, 2, 3, 3, 0, 0 }; /* byte 40 remainder in 1/4 ns units *//* CAS latency in cycles */val = latencies[index];for (i = 0; i < DIMM_SOCKETS; i++) {if (!(dimm_mask & (1 << i)))continue;val &= spd_read_byte(ctrl->channel0[i], 18);}if (val & 0x10)cl = 4;else if (val & 0x20)cl = 5;else if (val & 0x40)cl = 6;elsedie("CAS latency mismatch\r\n");print_debug("cl = ");print_debug_hex8(cl);print_debug("\r\n");ci = cycle[index];/* Trc, Trfc in cycles */for (i = 0; i < DIMM_SOCKETS; i++) {if (!(dimm_mask & (1 << i)))continue;val1 = spd_read_byte(ctrl->channel0[i], 40);val = spd_read_byte(ctrl->channel0[i], 41);val <<= 2; /* convert to 1/4 ns */val += byte40rem[(val1 >> 4) & 0x7];val = (val + ci - 1) / ci + 1; /* convert to cycles */if (trc < val)trc = val;val = spd_read_byte(ctrl->channel0[i], 42);val <<= 2; /* convert to 1/4 ns */if (val1 & 0x01)val += 1024;val += byte40rem[(val1 >> 1) & 0x7];val = (val + ci - 1) / ci; /* convert to cycles */if (trfc < val)trfc = val;}print_debug("trc = ");print_debug_hex8(trc);print_debug("\r\n");print_debug("trfc = ");print_debug_hex8(trfc);print_debug("\r\n");/* Tras, Trtp, Twtr in cycles */for (i = 0; i < DIMM_SOCKETS; i++) {if (!(dimm_mask & (1 << i)))continue;val = spd_read_byte(ctrl->channel0[i], 30);val <<= 2; /* convert to 1/4 ns */val = (val + ci - 1) / ci; /* convert to cycles */if (tras < val)tras = val;val = spd_read_byte(ctrl->channel0[i], 38);val = (val + ci - 1) / ci; /* convert to cycles */if (trtp < val)trtp = val;val = spd_read_byte(ctrl->channel0[i], 37);val = (val + ci - 1) / ci; /* convert to cycles */if (twtr < val)twtr = val;}print_debug("tras = ");print_debug_hex8(tras);print_debug("\r\n");print_debug("trtp = ");print_debug_hex8(trtp);print_debug("\r\n");print_debug("twtr = ");print_debug_hex8(twtr);print_debug("\r\n");val = (drt0[index] | ((trc - 11) << 12) | ((cl - 3) << 9)| ((cl - 3) << 6) | ((cl - 3) << 3));print_debug("drt0 = ");print_debug_hex32(val);print_debug("\r\n");pci_write_config32(ctrl->f0, DRT0, val);val = (drt1[index] | ((tras - 8) << 28) | ((trtp - 2) << 25)| (twtr << 15));print_debug("drt1 = ");print_debug_hex32(val);print_debug("\r\n");pci_write_config32(ctrl->f0, DRT1, val);val = (magic[index]);print_debug("magic = ");print_debug_hex32(val);print_debug("\r\n");pci_write_config32(PCI_DEV(0, 0x08, 0), 0xcc, val);val = (mrs[index] | (cl << 20));print_debug("mrs = ");print_debug_hex32(val);print_debug("\r\n");return val;+}
+static int spd_set_dram_controller_mode(const struct mem_controller *ctrl,
long dimm_mask)+{
int value;int drc = 0;int i;msr_t msr;uint8_t cycle = 0x25;/* 0x7c DRC */for (i = 0; i < DIMM_SOCKETS; i++) {if (!(dimm_mask & (1 << i)))continue;if ((spd_read_byte(ctrl->channel0[i], 6) & 0xf0) != 0x40)die("ERROR: Only 64-bit DIMMs supported\r\n");if (!(spd_read_byte(ctrl->channel0[i], 11) & 0x02))die("ERROR: Only ECC DIMMs supported\r\n");if (spd_read_byte(ctrl->channel0[i], 13) != 0x08)die("ERROR: Only x8 DIMMs supported\r\n");value = spd_read_byte(ctrl->channel0[i], 9); /* cycle time */if (value > cycle)cycle = value;}print_debug("cycle = ");print_debug_hex8(cycle);print_debug("\r\n");drc |= (1 << 20); /* enable ECC */drc |= (3 << 30); /* enable CKE on each dimm */drc |= (1 << 4); /* enable CKE globally */
/* eswierk check: */Done already? Remove? Turn into "TODO"?
/* set front side bus speed */msr = rdmsr(0xcd); /* returns 0 on Pentium M 90nm */print_debug("msr 0xcd = ");print_debug_hex32(msr.hi);print_debug_hex32(msr.lo);print_debug("\r\n");
/* eswierk check that this msr really indicates fsb speed! */Ditto.
if (msr.lo & 0x07) {print_info("533 MHz FSB\r\n");if (cycle <= 0x25) {drc |= 0x5;print_info("400 MHz DDR\r\n");} else if (cycle <= 0x30) {drc |= 0x7;print_info("333 MHz DDR\r\n");} else if (cycle <= 0x3d) {drc |= 0x4;print_info("266 MHz DDR\r\n");} else {drc |= 0x2;print_info("200 MHz DDR\r\n");}}else {print_info("400 MHz FSB\r\n");if (cycle <= 0x30) {drc |= 0x7;print_info("333 MHz DDR\r\n");} else if (cycle <= 0x3d) {drc |= 0x0;print_info("266 MHz DDR\r\n");} else {drc |= 0x2;print_info("200 MHz DDR\r\n");}}print_debug("DRC = ");print_debug_hex32(drc);print_debug("\r\n");return drc;+}
+static void sdram_set_spd_registers(const struct mem_controller *ctrl) +{
long dimm_mask;int i;/* Test if we can read the spd */dimm_mask = spd_detect_dimms(ctrl);if (!(dimm_mask & ((1 << DIMM_SOCKETS) - 1))) {print_err("No memory for this cpu\r\n");return;}return;+}
+static void do_delay(void) +{
int i;unsigned char b;for(i=0;i<16;i++)b=inb(0x80);+}
Please use the global delay function, no need for yet another one.
+#define TIMEOUT_LOOPS 300000
+#define DCALCSR 0x040 +#define DCALADDR 0x044 +#define DCALDATA 0x048 +#define MBCSR 0x140 +#define MBADDR 0x144 +#define MBDATA 0x148 +#define DDRIOMC2 0x268
Move to some *.h file?
+static void set_on_dimm_termination_enable(const struct mem_controller *ctrl) +{
unsigned char c1,c2;unsigned int dimm,i;unsigned int data32;unsigned int t4;/* Set up northbridge values *//* ODT enable */pci_write_config32(ctrl->f0, SDRC, 0xa0002c30);c1 = pci_read_config8(ctrl->f0, DRB);c2 = pci_read_config8(ctrl->f0, DRB+2);if (c1 == c2) {/* 1 single-rank DIMM */data32 = 0x00000010;}else {/* 2 single-rank DIMMs or 1 double-rank DIMM */data32 = 0x00002010;}print_debug("ODT Value = ");print_debug_hex32(data32);print_debug("\r\n");pci_write_config32(ctrl->f0, DDR2ODTC, data32);for(i=0;i<2;i++) {print_debug("ODT CS");print_debug_hex8(i);print_debug("\r\n");write32(BAR+DCALADDR, 0x0b840001);write32(BAR+DCALCSR, 0x80000003 | ((i+1)<<21));data32 = read32(BAR+DCALCSR);while (data32 & 0x80000000)data32 = read32(BAR+DCALCSR);}+}
+static void dump_dcal_regs(void) +{
int i;for (i = 0x0; i < 0x2a0; i += 4) {if ((i % 16) == 0) {print_debug("\r\n");print_debug_hex16(i);print_debug(": ");}print_debug_hex32(read32(BAR+i));print_debug(" ");}print_debug("\r\n");+}
+static void sdram_enable(int controllers, const struct mem_controller *ctrl) +{
int i;int cs;long mask;uint32_t drc;uint32_t data32;uint32_t mode_reg;msr_t msr;uint16_t data16;mask = spd_detect_dimms(ctrl);print_debug("Starting SDRAM Enable\r\n");/* set dram type and Front Side Bus freq. */drc = spd_set_dram_controller_mode(ctrl, mask);if (drc == 0) {die("Error calculating DRC\r\n");}data32 = drc & ~(3 << 20); /* clear ECC mode */data32 = data32 | (3 << 5); /* temp turn off ODT *//* Set dram controller mode *//* 0x7c DRC */pci_write_config32(ctrl->f0, DRC, data32);/* turn the clocks on *//* 0x8c CKDIS */pci_write_config16(ctrl->f0, CKDIS, 0x0000);/* program row size DRB */spd_set_ram_size(ctrl, mask);/* program row attributes DRA */spd_set_row_attributes(ctrl, mask);/* program DRT timing values */mode_reg = spd_set_drt_attributes(ctrl, mask, drc);The indentation of the comments here, and the coding style in general are not quite correct in various places. Let's commit this anyway for now, I'll do an 'indent'-run on the code later though, invoked as listed here: http://www.coreboot.org/Development_Guidelines#Coding_Style
dump_dcal_regs();for(cs=0;cs<2;cs++) { /* loop through each dimm to test */print_debug("NOP CS");print_debug_hex8(cs);print_debug("\r\n");/* Apply NOP */Wrong comment (or comment in the wrong place)? This only does a delay, no?
do_delay();write32(BAR+DCALCSR, (0x00000000 | ((cs+1)<<21)));write32(BAR+DCALCSR, (0x80000000 | ((cs+1)<<21)));data32 = read32(BAR+DCALCSR);while(data32 & 0x80000000)data32 = read32(BAR+DCALCSR);}/* Apply NOP */Ditto.
do_delay();for(cs=0;cs<2;cs++) {print_debug("NOP CS");print_debug_hex8(cs);print_debug("\r\n");write32(BAR + DCALCSR, (0x80000000 | ((cs+1)<<21)));data32 = read32(BAR+DCALCSR);while(data32 & 0x80000000)data32 = read32(BAR+DCALCSR);}/* Precharge all banks */do_delay();for(cs=0;cs<2;cs++) {print_debug("Precharge CS");print_debug_hex8(cs);print_debug("\r\n");write32(BAR+DCALADDR, 0x04000000);write32(BAR+DCALCSR, (0x80000002 | ((cs+1)<<21)));data32 = read32(BAR+DCALCSR);while(data32 & 0x80000000)data32 = read32(BAR+DCALCSR);}/* EMRS: Enable DLLs, set OCD calibration mode to default */do_delay();for(cs=0;cs<2;cs++) {print_debug("EMRS CS");print_debug_hex8(cs);print_debug("\r\n");write32(BAR+DCALADDR, 0x0b840001);write32(BAR+DCALCSR, (0x80000003 | ((cs+1)<<21)));data32 = read32(BAR+DCALCSR);while(data32 & 0x80000000)data32 = read32(BAR+DCALCSR);}/* MRS: Reset DLLs */do_delay();for(cs=0;cs<2;cs++) {print_debug("MRS CS");print_debug_hex8(cs);print_debug("\r\n");write32(BAR+DCALADDR, mode_reg);write32(BAR+DCALCSR, (0x80000003 | ((cs+1)<<21)));data32 = read32(BAR+DCALCSR);while(data32 & 0x80000000)data32 = read32(BAR+DCALCSR);}/* Precharge all banks */do_delay();do_delay();do_delay();for(cs=0;cs<2;cs++) {print_debug("Precharge CS");print_debug_hex8(cs);print_debug("\r\n");write32(BAR+DCALADDR, 0x04000000);write32(BAR+DCALCSR, (0x80000002 | ((cs+1)<<21)));data32 = read32(BAR+DCALCSR);while(data32 & 0x80000000)data32 = read32(BAR+DCALCSR);}
/* Do 2 refreshes */do_delay();for(cs=0;cs<2;cs++) {print_debug("Refresh CS");print_debug_hex8(cs);print_debug("\r\n");write32(BAR+DCALCSR, (0x80000001 | ((cs+1)<<21)));data32 = read32(BAR+DCALCSR);while(data32 & 0x80000000)data32 = read32(BAR+DCALCSR);}do_delay();for(cs=0;cs<2;cs++) {print_debug("Refresh CS");print_debug_hex8(cs);print_debug("\r\n");write32(BAR+DCALCSR, (0x80000001 | ((cs+1)<<21)));data32 = read32(BAR+DCALCSR);while(data32 & 0x80000000)data32 = read32(BAR+DCALCSR);}As far as I can see both blocks are identical? Why not a 'for' loop then? This would also make a smaller coreboot (binary) image (less code/printfs, less strings for the debug output, etc.).
Are there code issues or romcc issues?
do_delay();/* for good luck do 6 more */Better comment please, e.g. mention the reason why multiple runs are needed and why 6 and not some other number. I assume the SPD/RAM/NB datasheet says so?
for(cs=0;cs<2;cs++) {write32(BAR+DCALCSR, (0x80000001 | ((cs+1)<<21)));}do_delay();for(cs=0;cs<2;cs++) {write32(BAR+DCALCSR, (0x80000001 | ((cs+1)<<21)));}do_delay();for(cs=0;cs<2;cs++) {write32(BAR+DCALCSR, (0x80000001 | ((cs+1)<<21)));}do_delay();for(cs=0;cs<2;cs++) {write32(BAR+DCALCSR, (0x80000001 | ((cs+1)<<21)));}do_delay();for(cs=0;cs<2;cs++) {write32(BAR+DCALCSR, (0x80000001 | ((cs+1)<<21)));}do_delay();for(cs=0;cs<2;cs++) {write32(BAR+DCALCSR, (0x80000001 | ((cs+1)<<21)));}do_delay();Same as above, please pack this into a loop if possible.
/* MRS: Set DLLs to normal */do_delay();for(cs=0;cs<2;cs++) {print_debug("MRS CS");print_debug_hex8(cs);print_debug("\r\n");write32(BAR+DCALADDR, (mode_reg & ~(1<<24)));write32(BAR+DCALCSR, (0x80000003 | ((cs+1)<<21)));data32 = read32(BAR+DCALCSR);while(data32 & 0x80000000)data32 = read32(BAR+DCALCSR);}/* EMRS: Enable DLLs */do_delay();for(cs=0;cs<2;cs++) {print_debug("EMRS CS");print_debug_hex8(cs);print_debug("\r\n");write32(BAR+DCALADDR, 0x0b840001);write32(BAR+DCALCSR, (0x80000003 | ((cs+1)<<21)));data32 = read32(BAR+DCALCSR);while(data32 & 0x80000000)data32 = read32(BAR+DCALCSR);}do_delay();/* No command */write32(BAR+DCALCSR, 0x0000000f);write32(BAR, 0x00100000);/* enable on dimm termination */set_on_dimm_termination_enable(ctrl);dump_dcal_regs();/* receive enable calibration */do_delay();for(cs=0;cs<1;cs++) {print_debug("receive enable calibration CS");print_debug_hex8(cs);print_debug("\r\n");write32(BAR+DCALCSR, (0x8000000c | ((cs+1)<<21)));data32 = read32(BAR+DCALCSR);while(data32 & 0x80000000)data32 = read32(BAR+DCALCSR);}dump_dcal_regs();/* Adjust RCOMP */data32 = read32(BAR+DDRIOMC2);data32 &= ~(0xf << 16);data32 |= (0xb << 16);write32(BAR+DDRIOMC2, data32);dump_dcal_regs();/* 0x7c DRC */data32 = drc & ~(3 << 20); /* clear ECC mode */pci_write_config32(ctrl->f0, DRC, data32);write32(BAR+DCALCSR, 0x0008000f);/* Clear memory and init ECC */for(cs=0;cs<2;cs++) {if (!(mask & (1<<cs)))continue;print_debug("clear memory CS");print_debug_hex8(cs);print_debug("\r\n");write32(BAR+MBCSR, 0xa00000f0 | ((cs+1)<<20) | (0<<16));data32 = read32(BAR+MBCSR);while(data32 & 0x80000000)data32 = read32(BAR+MBCSR);if(data32 & 0x40000000)print_debug("failed!\r\n");}/* Clear read/write FIFO pointers */print_debug("clear read/write fifo pointers\r\n");write32(BAR+DDRIOMC2, read32(BAR+DDRIOMC2) | (1<<15));do_delay();write32(BAR+DDRIOMC2, read32(BAR+DDRIOMC2) & ~(1<<15));do_delay();dump_dcal_regs();print_debug("Done\r\n");/* Set initialization complete *//* 0x7c DRC */drc |= (1 << 29);drc |= (3 << 30);data32 = drc & ~(3 << 20); /* clear ECC mode */pci_write_config32(ctrl->f0, DRC, data32);/* Set the ECC mode */pci_write_config32(ctrl->f0, DRC, drc);/* The memory is now setup, use it */cache_lbmem(MTRR_TYPE_WRBACK);+}
+static inline int memory_initialized(void) +{
return pci_read_config32(PCI_DEV(0, 0x00, 0), DRC) & (1 << 29);+} Index: coreboot-v2-3363/src/northbridge/intel/i3100/raminit_ep80579.h =================================================================== --- /dev/null +++ coreboot-v2-3363/src/northbridge/intel/i3100/raminit_ep80579.h @@ -0,0 +1,33 @@ +/*
- This file is part of the coreboot project.
- Copyright (C) 2008 Arastra, Inc.
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License version 2 as
- published by the Free Software Foundation.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program; if not, write to the Free Software
- Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
- */
+/* This code is based on src/northbridge/intel/e7520/raminit.h */
Nah, drop this, too trivial. This is soleley (C) Arastra, IMHO.
+#ifndef NORTHBRIDGE_INTEL_EP80579_RAMINIT_H +#define NORTHBRIDGE_INTEL_EP80579_RAMINIT_H
NORTHBRIDGE_INTEL_I3100_RAMINIT_EP80579_H for consistency.
+#define DIMM_SOCKETS 2 +struct mem_controller {
unsigned node_id;uint16_t or u16 here, as also done below?
device_t f0;uint16_t channel0[DIMM_SOCKETS];+};
+#endif Index: coreboot-v2-3363/src/northbridge/intel/i3100/ep80579.h =================================================================== --- /dev/null +++ coreboot-v2-3363/src/northbridge/intel/i3100/ep80579.h @@ -0,0 +1,61 @@ +/*
- This file is part of the coreboot project.
- Copyright (C) 2008 Arastra, Inc.
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License version 2 as
- published by the Free Software Foundation.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program; if not, write to the Free Software
- Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
- */
+#ifndef NORTHBRIDGE_INTEL_EP80579_H +#define NORTHBRIDGE_INTEL_EP80579_H
NORTHBRIDGE_INTEL_I3100_EP80579_H for consistency.
+#define VID 0x00 +#define DID 0x02 +#define PCICMD 0x04 +#define PCISTS 0x06 +#define RID 0x08
These five are generic and already in pci.h I think (and not used in your code anyway), I'd drop them.
+#define SMRBASE 0x14 +#define MCHCFG0 0x50 +#define FDHC 0x58 +#define PAM 0x59 +#define DRB 0x60 +#define DRT1 0x64 +#define DRA 0x70 +#define DRT0 0x78 +#define DRC 0x7c +#define ECCDIAG 0x84 +#define SDRC 0x88 +#define CKDIS 0x8c +#define CKEDIS 0x8d +#define DEVPRES 0x9c +#define DEVPRES_D0F0 (1 << 0) +#define DEVPRES_D1F0 (1 << 1) +#define DEVPRES_D2F0 (1 << 2) +#define DEVPRES_D3F0 (1 << 3) +#define DEVPRES_D4F0 (1 << 4) +#define DEVPRES_D10F0 (1 << 5) +#define EXSMRC 0x9d +#define SMRAM 0x9e +#define EXSMRAMC 0x9f +#define DDR2ODTC 0xb0 +#define TOLM 0xc4 +#define REMAPBASE 0xc6 +#define REMAPLIMIT 0xc8 +#define REMAPOFFSET 0xca +#define TOM 0xcc +#define HECBASE 0xce +#define DEVPRES1 0xf4
+#endif
Uwe.
http://www.hermann-uwe.de | http://www.holsham-traders.de http://www.crazy-hacks.org | http://www.unmaintained-free-software.org
On Wed, Sep 3, 2008 at 5:23 PM, Ed Swierk eswierk@arastra.com wrote:
Thanks for the feedback. I have addressed most of your comments; please see the attached patch.
I will address your comment about the duplicate struct dimm_size declaration separately. Would it make sense to put it in a file common to all Intel northbridges, say, src/northbridge/intel/mem.h, and then remove it from the various raminit.c's?
Signed-off-by: Ed Swierk eswierk@arastra.com
Pinged-by: Ed Swierk eswierk@arastra.com
;-)
On Tue, 23 Sep 2008 15:30:53 -0700, "Ed Swierk" eswierk@arastra.com wrote:
On Wed, Sep 3, 2008 at 5:23 PM, Ed Swierk eswierk@arastra.com wrote:
Thanks for the feedback. I have addressed most of your comments; please see the attached patch.
I will address your comment about the duplicate struct dimm_size declaration separately. Would it make sense to put it in a file common to all Intel northbridges, say, src/northbridge/intel/mem.h, and then remove it from the various raminit.c's?
Signed-off-by: Ed Swierk eswierk@arastra.com
Pinged-by: Ed Swierk eswierk@arastra.com
Wait, making a common file for this?
struct dimm_size { unsigned long side1; unsigned long side2; };
Four four lines of code that seems a little silly to me? Can you resend you patch, I don't think I got it.
On Tue, Sep 23, 2008 at 5:09 PM, Joseph Smith joe@settoplinux.org wrote:
Wait, making a common file for this?
struct dimm_size { unsigned long side1; unsigned long side2; };
Four four lines of code that seems a little silly to me?
This was in response to Uwe's suggestion--he would rather move the struct dimm_size to a common header file than duplicate it in numerous intel raminit files. I could go either way; this is pretty minor considering the overall level of redundancy in the northbridge code but I can see not wanting to make things worse.
Can you resend you patch, I don't think I got it.
Here it is (unchanged from my 3 Sept message).
--Ed
On Tue, 23 Sep 2008 17:48:14 -0700, "Ed Swierk" eswierk@arastra.com wrote:
On Tue, Sep 23, 2008 at 5:09 PM, Joseph Smith joe@settoplinux.org
wrote:
Wait, making a common file for this?
struct dimm_size { unsigned long side1; unsigned long side2; };
Four four lines of code that seems a little silly to me?
This was in response to Uwe's suggestion--he would rather move the struct dimm_size to a common header file than duplicate it in numerous intel raminit files. I could go either way; this is pretty minor considering the overall level of redundancy in the northbridge code but I can see not wanting to make things worse.
Can you resend you patch, I don't think I got it.
Here it is (unchanged from my 3 Sept message).
Signed-off-by: Ed Swierk eswierk@arastra.com
Looks good if you have tested it on real hardware:
Acked-by: Joseph Smith joe@settoplinux.org
I don't think we should worry about intel_common.h for now, for four lines of code. I can see simularities in the northbridge.c's but alot of the raminit.c's have the same basic goal but do it very differently. I say we get your code submitted now and worry about code consolidation later.
On Tue, Sep 23, 2008 at 6:38 PM, Joseph Smith joe@settoplinux.org wrote:
Looks good if you have tested it on real hardware:
Acked-by: Joseph Smith joe@settoplinux.org
Yup, I've tested it on the Tolapai development board.
Committed in r3600.
--Ed
On Tue, Sep 23, 2008 at 09:38:59PM -0400, Joseph Smith wrote:
I don't think we should worry about intel_common.h for now, for four lines
Sure, I didn't mean to make an _extra_ file just for this one struct, but rather put it into some already existing file (e.g. spd.h or so). Something along those lines. Not a major issue right now, though.
of code. I can see simularities in the northbridge.c's but alot of the raminit.c's have the same basic goal but do it very differently. I say we get your code submitted now and worry about code consolidation later.
Yes.
Uwe.
-
Ed Swierk -
Joseph Smith -
Uwe Hermann