[coreboot] v2[PATCH]RCA RM4100 i82830 support

Corey Osgood corey.osgood at gmail.com
Tue Feb 26 04:18:01 CET 2008


On Mon, 2008-02-25 at 21:20 -0500, joe at smittys.pointclark.net wrote:
> Quoting Peter Stuge <peter at 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...

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

> +				die("HALT\r\n");

Why die? 

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

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


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

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


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

I think that's everything, I'm hitting the sack.

-Coreu





More information about the coreboot mailing list