[LinuxBIOS] SPD Logical and Physical Banks

[Corey Osgood]

Uwe Hermann wrote:
> Hi Corey,
> 
> here are a few more comments on the file/patch:
> 
> On Tue, May 15, 2007 at 04:24:26PM -0400, Corey Osgood wrote:
>> Index: raminit.c
>> ===================================================================
>> --- raminit.c	(Revision 2671)
>> +++ raminit.c	(Arbeitskopie)
>> @@ -68,75 +69,19 @@
>>  
>>  /* Table format: register, bitmask, value. */
>>  static const long register_values[] = {
>> -	/* NBXCFG - NBX Configuration Register
>> -	 * 0x50
>> -	 *
>> -	 * [31:24] SDRAM Row Without ECC
>> -	 *         0 = ECC components are populated in this row
>> -	 *         1 = ECC components are not populated in this row
> [...]
>> -	 * [01:00] Reserved
>> -	 */
>> -	// TODO
>> -	NBXCFG, 0x00000000, 0xff00a00c,
> 
> Hm, please keep these values and comments here for now. They might not
> be needed for every register, but I think I remember that _some_ should
> be set to "sane" defaults even _during_ RAM init. So it might make sense to
> set them twice (here, and later in the code).

Okay. If you notice, I've moved all the comments down into the spd
setup, but the values can stay (here and DRB), at least until I can test
it out. I'll put something in for a comment about it being an initial
value and checking the spd function.

>   
>> @@ -390,7 +311,7 @@
>>  	reg = pci_read_config8(ctrl->d0, DRAMC);
>>  
>>  	for (i = 0; i < DIMM_SOCKETS; i++) {
>> -		value = spd_read_byte(ctrl->channel0[i], SPD_REFRESH);
>> +		value = smbus_read_byte(ctrl->channel0[i], SPD_REFRESH);
> 
> Nope, please use spd_read_byte() everywhere.

spd_read_byte was removed to try an use a few less registers, I'm also
not using sdram_initialize for the same reason. Both got compiling a
little further, but not far enough.

>> +static void spd_set_nbxcfg(const struct mem_controller *ctrl)
> 
> Please don't hardcode register names in function names. Use generic
> function names which explain the step/work the function performs, e.g.
> 
> set_cas_latencies()
> enable_timers()
> set_refresh_rate()
> etc.
> 
> The register might or might not have the same name in a similar chipset,
> but the steps you have to perform are usually the same.
> 
> Also, generic names make the code much better readable and
> understandable for new developers who try to understand it.

Alright, sounds good, will take care of.

>> +	 * [31:24] SDRAM Row Without ECC
>> +	 *         0 = ECC components are populated in this row
>> +	 *         1 = ECC components are not populated in this row */
>> +
>> +
>> +	/* Note: there's no way to tell 440zx from bx via hardware */
>> +	/* 440ZX doesn't do ECC, and needs bits 31-24 set to 0, so skip this */
>> +	#ifndef MODEL_440ZX
> 
> I'd even say you shouldn't worry about ECC all that much right now.
> Just set it to off until everything else works fine. ECC is nice to
> have, but non-essential.

True, but it was only a few minutes of work to implement. ECC shouldn't
actually be enabled unless some other bit is set to 1. I put a comment
in that I wasn't going to mess with it.

>> +	/* Set the DRAM frequency to the lowest of all dimms */
>> +	/* TODO: do this with the previous loop */
>> +	for(i = 0; i < 4; i++)
>> +	{
>> +		spd_data = smbus_read_byte(ctrl->channel0[i], 126);
>> +		/* If we find any 66MHz dimms, set the dimm speed to 66MHz and break */
>> +		/* Note: This depends on the dimm following the Intel SPD standard */
>> +		if(spd_data == 0x66)
>> +		{
>> +			nbxcfg |= 1 << 13;
> 
>> +			i = 6; //just to make sure
> 
> Huh?

To make sure the loop is broken, I wasn't sure if break (which should be
the next line) would break out of the if statement or the for loop, so I
threw that in there (if i=6, i<4). It was very late (2am or so) when I
was writing this, and it shouldn't be necessary.

>> +	nbxcfg |= 0xc; //sets bits 3 and 2 to 1
>> +	PRINT_DEBUG("Value of NBXCFG calculated to 0x");
>> +	PRINT_DEBUG_HEX32(nbxcfg);
>> +	PRINT_DEBUG("/r/n");
> 
> \r\n

good catch, fixed.

> 
> 
>> +	/* Count to 8 since there are 8 possible rows, but count by 2 to check by dimm */
> 
>> +	/* Should 8 be replaced with DIMM_SOCKETS * 2? */
> 
> Yes, probably.
> 
> 
>> +	for(i = 0; i < 8; i = i + 2)
>> +	{
>> +		/* First check if this row is populated with SDR SDRAM */
>> +		if(smbus_read_byte(ctrl->channel0[i/2], 2) == 0x04)
> 
> This is very confusing. I think it would be easier to understand if you
> count from 0..15 in steps of 1 and adapt the number where needed.
> 

Fixed. It now counts 0 to DIMM_SOCKETS in steps of 1, to hit all the dimms.

> 
>> +			dimm_size = rows * row_size * 4;
>> +			print_debug("Found 0x");
>> +			print_debug_hex8(dimm_size);
>> +			print_debug("MB DIMM in slot ");
>> +			print_debug_hex8(i/2);
>> +			print_debug("\r\n");
>> +			
>> +			/* Convert dimms_size to be in 8MB chunks */
>> +			dimm_size = dimm_size >> 3;
> 
> dimm_size /= 8;
> 
> Please don't use bit-manipulations where you really do arithmetic
> operations (just makes the code harder to understand).
> 
> (don't worry about "optimizations" here, that's the job of the compiler)

Fixed. I wasn't even really thinking about readability, just if it would
work. This still needs (IMO) quite a bit of cleanup before sending in a
real patch.

> In general the patch looks quite good. Please feel free to send single
> patches for functions which are finished and usable, so we can merge
> them one by one (and always check if the RAM still verifies).
> No need to wait until all of the functions are finished...

Thanks! I'll get on this soon. I'm going to try to rip out as much of
the complex/non-essential stuff as possible to see how close I can get
to the limitations of romcc. It looks like we will need cache as ram
before we can do the full ram setup in the pre-ram code though.

-Corey