Quoting Corey Osgood corey.osgood@gmail.com:
On Mon, 2008-02-25 at 21:20 -0500, joe@smittys.pointclark.net wrote:
Quoting Peter Stuge peter@stuge.se:
On Mon, Feb 25, 2008 at 08:51:00PM -0500, Corey Osgood wrote:
without the ability to try other memory sticks or with real spd data, it might look right on the rm4100, but fail in practice.
The problem is that this is in (supposedly) generic 830 code.
It pretty much is. First of all PC133 SO-DIMMS only come in cas3 or cas2 but setting DRT to cas3 is not hurting anything. So your memory only runs a tiny bit slower, an average user probably wouldn't notice the difference. But it does need to be done eventually (DRT). Besides DRT everything else is spd detectable. Did you check out function Corey and I came up with to get the memory size using spd 31. I still think that is pretty cool.
What he said ;)
Moving on:
+static void spd_set_dram_size(const struct mem_controller *ctrl) +{
- int i, value, drb1, drb2;
- for (i = 0; i < DIMM_SOCKETS; i++) {
struct dimm_size sz;unsigned device;device = ctrl->channel0[i];drb1 = 0;drb2 = 0;/* First check if a DIMM is actually present. */if (spd_read_byte(device, 2) == 0x4) {print_debug("Found DIMM in slot ");print_debug_hex8(i);print_debug("\r\n");sz = spd_get_dimm_size(device);/* WISHLIST: would be nice to display it as decimal? */I saw a small bit of code to do this just a little while ago, if only I could remember where...
That would be cool.
print_debug("DIMM is 0x");print_debug_hex16(sz.side1);print_debug(" on side 1\r\n");print_debug("DIMM is 0x");print_debug_hex16(sz.side2);print_debug(" on side 2\r\n");/* The i82830 can't handle DIMMs smaller than 32MB per* side or larger than 256MB per side. It also can* only support a symmetrical dual-sided dimm.*/if ((sz.side1 < 32)) {print_err("DIMMs smaller than 32MB per side\r\n");print_err("are not supported on this board\r\n");board -> northbridge or chipset, in all of these.
Not sure what you mean here.
die("HALT\r\n");Why die?
Die becaause the memory is not supported, right?
}if ((sz.side2 != 0) && (sz.side2 < 32)) {print_err("DIMMs smaller than 32MB per side\r\n");print_err("are not supported on this board\r\n");die("HALT\r\n");}if ((sz.side1 > 256) || (sz.side2 > 256)) {side1 should always be the larger side, and i830 doesn't support asymetrical dimms, so you can drop the side2 check.
ok will change
print_err("DIMMs larger than 256MB per side\r\n");print_err("are not supported on this board\r\n");die("HALT\r\n");}if ((sz.side2 != 0) && (sz.side1 != sz.side2)) {print_err("This board only supports\r\n");print_err("symmetrical dual-sided DIMMs\r\n");die("HALT\r\n");}This should be moved right into the detection routine. If the northbridge can't handle it, then just die if sz.side1 |= sz.side2. The other option is to set sz.side2 to 0, and try to use it like a single-sided dimm.
Hmm above is saying that IF THERE IS A SECOND SIDE and it does not equal the first side than die, otherwise it is assumed it is single sided and moves on.
"if sz.side1 |= sz.side2" does not work for single sided so-dimms, correct?
/* We need to divide size by 32 to set up the
* DRB registers.*/if (sz.side1 > 0) {if(sz.side1) works as well. unsigned can never be negative.
ok will change
drb1 = sz.side1 >> 5;Please don't use bitwise shifts for multiplication/division, same applies for other places.
}if (sz.side2 > 0) {drb2 = sz.side2 >> 5;}Please excuse me for jumping around, trying to keep this short:
ok will change, but there was quite a discussion about this a while ago and half thought it was ok and half didn't. So which is correct in "C" standards?
+static void do_ram_command(const struct mem_controller *ctrl, uint32_t command, uint32_t addr_offset) +{
<snip> + + /* Read from (DIMM start address + addr_offset). */ + read32(0 + addr_offset); //first offset is always 0 +}
This isn't ready for multiple dimms yet. See the cn700 patch I recently sent (but haven't committed yet, I think it was acked).
ok will do.
I think that's everything, I'm hitting the sack.
-Coreu
Thanks - Joe