[coreboot] v2[PATCH]RCA RM4100 i82830 support

[joe at smittys.pointclark.net]

Quoting Corey Osgood <corey.osgood at gmail.com>:

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