Re: [coreboot] Some typos in raminit_f.c (Re: r3866 - trunk/coreboot-v2/src/northbridge/amd/amdk8)

[Carl-Daniel Hailfinger]

On 16.01.2009 08:38, Paul Menzel wrote:
> Dear list,
>
> this patch fixes two(?) typos, capitalizes dimm in some comments and
> adds some full stops in some comments.
>
> Thanks,
>
> Paul
>
> Signed-off-by: Paul Menzel
> Index: raminit_f.c
>
> --- raminit_f.c	(Revision 3867)
> +++ raminit_f.c	(Arbeitskopie)
> @@ -730,7 +730,7 @@
>    /* Test to see if I am an Opteron.
>     * FIXME Testing dual channel capability is correct for now
>     * but a better test is probably required.
> * m2 and s1g1 support dual channel too. but only support unbuffered dimm
> * m2 and s1g1 support dual channel too. But only support unbuffered DIMM.

m2 should probably be AM2 and s1g1 should probably be S1g1 (notice the
odd capitalization).
> */
>
> #warning "FIXME implement a better test for opterons"
>    uint32_t nbcap;
> @@ -839,30 +839,30 @@
>    uint32_t base0, base1;
>
> /* For each base register.
> * Place the dimm size in 32 MB quantities in the bits 31 - 21.
> * The initialize dimm size is in bits.
> * Place the DIMM size in 32 MB quantities in the bits 31 - 21.
> * The initialize DIMM size is in bits.
> Set the base enable bit0.
>
> */
>
> base0 = base1 = 0;
> /* Make certain side1 of the dimm is at least 128MB */
> /* Make certain side1 of the DIMM is at least 128MB. */
> if (sz->per_rank >= 27) {
> base0 = (1 << ((sz->per_rank - 27 ) + 19)) | 1;
> }
> /* Make certain side2 of the dimm is at least 128MB */
> /* Make certain side2 of the DIMM is at least 128MB. */
> if (sz->rank > 1) { // 2 ranks or 4 ranks
> base1 = (1 << ((sz->per_rank - 27 ) + 19)) | 1;
> }
> /* Double the size if we are using dual channel memory */
> /* Double the size if we are using dual channel memory. */
> if (meminfo->is_Width128) {
> base0 = (base0 << 1) | (base0 & 1);
> base1 = (base1 << 1) | (base1 & 1);
> }
> /* Clear the reserved bits */
> /* Clear the reserved bits. */
> base0 &= ~0xe007fffe;
> base1 &= ~0xe007fffe;
>
> @@ -881,7 +881,7 @@
>  #endif
>    }
> /* Enable the memory clocks for this DIMM by Clear the MemClkDis bit*/
> /* Enable the memory clocks for this DIMM by Clear the MemClkDis bit. */

Maybe reword the sentence above.
> if (base0) {
>    	uint32_t dword;
>    	uint32_t ClkDis0;
> @@ -970,7 +970,7 @@
>    }
>  #endif
> /* Make certain side1 of the dimm is at least 128MB */
> /* Make certain side1 of the DIMM is at least 128MB */

side1 -> side 1
Maybe full stop at the end?
> if (sz->per_rank >= 27) {
>    	unsigned temp_map;
>    	temp_map = cs_map_aaa[(sz->bank-2)*3*4 + (sz->rows - 13)*3 + (sz->col - 9) ];
> @@ -1349,7 +1349,7 @@
>    		return -1;
>    	}
> /* Registered dimm ? */
> /* Registered DIMM? */
> value &= 0x3f;
> if ((value == SPD_DIMM_TYPE_RDIMM) || (value == SPD_DIMM_TYPE_mRDIMM)) {
> 	//check SPD_MOD_ATTRIB to verify it is SPD_MOD_ATTRIB_REGADC (0x11)?
>
> @@ -1361,9 +1361,9 @@
>  #if 0
>    	if ( registered != (meminfo->dimm_mask & ((1<<DIMM_SOCKETS)-1)) ) {
>    		meminfo->dimm_mask &= (registered | (registered << DIMM_SOCKETS) ); //disable unbuffed dimm
> -//			die("Mixed buffered and registered dimms not supported");
> +//			die("Mixed buffered and registered DIMMs not supported.");
>    	}
> //By yhlu for debug M2, s1g1 can do dual channel, but it use unbuffer DIMM
> // By yhlu for debug M2, s1g1 can do dual channel, but it uses unbuffer DIMM.

Probably AM2 again.
unbuffer -> unbuffered
Maybe reword the sentence above.
> if (!registered) {
> 	die("Unbuffered Dimms not supported on Opteron");

DIMMs?
> }
>
> @@ -1464,7 +1464,7 @@
>    u8 mux_cap = 0;
>  #endif
> /* If the dimms are not in pairs do not do dual channels */
> /* If the DIMMs are not in pairs do not do dual channels. */
> if ((meminfo->dimm_mask & ((1 << DIMM_SOCKETS) - 1)) !=
> ((meminfo->dimm_mask >> DIMM_SOCKETS) & ((1 << DIMM_SOCKETS) - 1))) {
> goto single_channel;
>
> @@ -1522,11 +1522,11 @@
>    meminfo->is_Width128 = 0;
>    meminfo->is_64MuxMode = 0;
> /* single dimm */
> /* single DIMM */
> if ((meminfo->dimm_mask & ((1 << DIMM_SOCKETS) - 1)) !=
>  ((meminfo->dimm_mask >> DIMM_SOCKETS) & ((1 << DIMM_SOCKETS) - 1))) {
> if (((meminfo->dimm_mask >> DIMM_SOCKETS) & ((1 << DIMM_SOCKETS) - 1))) {
> 	/* mux capable and single dimm in channelB */
> 	/* mux capable and single DIMM in channelB */
> if (mux_cap) {
> 	printk_spew("Enable 64MuxMode & BurstLength32\n");
> 	dcm = pci_read_config32(ctrl->f2, DRAM_CTRL_MISC);
>
> @@ -1540,10 +1540,10 @@
>    			meminfo->dimm_mask &= ~((1 << (DIMM_SOCKETS * 2)) - (1 << DIMM_SOCKETS));
>    		}
>    	}
> } else { /* unmatched dual dimms ? */
> /* unmatched dual dimms not supported by meminit code. Use single channelA dimm. */
> } else { /* Unmatched dual DIMMs ? */
> /* Unmatched dual DIMMs not supported by meminit code. Use single channelA DIMMs. */
> meminfo->dimm_mask &= ~((1 << (DIMM_SOCKETS * 2)) - (1 << DIMM_SOCKETS));
> printk_spew("Unmatched dual dimms. Use single channelA dimm.\n");
> printk_spew("Unmatched dual DIMMs. Use single channelA DIMM.\n");
> }
> return meminfo->dimm_mask;
>
> }
> @@ -1729,7 +1729,7 @@
>    /* Compute the lowest cas latency which can be expressed in this
>     * particular SPD EEPROM. You can store at most settings for 3
>     * contiguous CAS latencies, so by taking the highest CAS
> * latency maked as supported in the SPD and subtracting 2 you
> * latency marked as supported in the SPD and subtracting 2 you
> get the lowest expressable CAS latency. That latency is not
> necessarily supported, but a (maybe invalid) entry exists
> for it.
>
> @@ -1794,7 +1794,7 @@
>
> static struct spd_set_memclk_result spd_set_memclk(const struct mem_controller *ctrl, struct mem_info *meminfo)
>  {
> /* Compute the minimum cycle time for these dimms */
> /* Compute the minimum cycle time for these DIMMs. */
> struct spd_set_memclk_result result;
> unsigned min_cycle_time, min_latency, bios_cycle_time;
> int i;
>
> @@ -1820,7 +1820,7 @@
>    printk_raminit("1 min_cycle_time: %08x\n", min_cycle_time);
>
> /* Compute the least latency with the fastest clock supported
> * by both the memory controller and the dimms.
> * by both the memory controller and the DIMMs.
> */
>  for (i = 0; i < DIMM_SOCKETS; i++) {
>   u32 spd_device;
>
> @@ -1854,7 +1854,7 @@
>    	}
>    	
>    }
> /* Make a second pass through the dimms and disable
> /* Make a second pass through the DIMMs and disable
> any that cannot support the selected memclk and cas latency.
> */
>
> @@ -1882,10 +1882,10 @@
>    		continue;
>    	}
> /* Compute the lowest cas latency supported */
> /* Compute the lowest cas latency supported. */
> latency = log2(latencies) -2;
> /* Walk through searching for the selected latency */
> /* Walk through searching for the selected latency. */
> for (index = 0; index < 3; index++, latency++) {
> 	if (!(latencies & (1 << latency))) {
> 		continue;
>
> @@ -1893,12 +1893,12 @@
>    		if (latency == min_latency)
>    			break;
>    	}
> /* If I can't find the latency or my index is bad error */
> /* If I can't find the latency or my index is bad error. */

"error" is a verb in the sentence above and it sounds strange. Maybe add
a comma before error.
> if ((latency != min_latency) || (index >= 3)) {
> 	goto dimm_err;
> }
>
> /* Read the min_cycle_time for this latency */
> /* Read the min_cycle_time for this latency. */
> value = spd_read_byte(spd_device, latency_indicies[index]);
> if (value < 0) goto hw_error;
>
> @@ -1909,14 +1909,14 @@
>    	if (value <= min_cycle_time) {
>    		continue;
>    	}
> /* Otherwise I have an error, disable the dimm */
> /* Otherwise I have an error, disable the DIMM. */

Can you leave that change out? I have rewritten that comment in my other
patch and this would collide with my changes.
> dimm_err:
>    	meminfo->dimm_mask = disable_dimm(ctrl, i, meminfo);
>    }
>
> printk_raminit("4 min_cycle_time: %08x\n", min_cycle_time);
> /* Now that I know the minimum cycle time lookup the memory parameters */
> /* Now that I know the minimum cycle time lookup the memory parameters. */

lookup -> look up
> result.param = get_mem_param(min_cycle_time);
>
> /* Update DRAM Config High with our selected memory speed */
> @@ -1928,7 +1928,7 @@
>
> printk_debug("%s\n", result.param->name);
> /* Update DRAM Timing Low with our selected cas latency */
> /* Update DRAM Timing Low with our selected cas latency. */
> value = pci_read_config32(ctrl->f2, DRAM_TIMING_LOW);
> value &= ~(DTL_TCL_MASK << DTL_TCL_SHIFT);
> value |= (min_latency - DTL_TCL_BASE)  << DTL_TCL_SHIFT;
>
> @@ -1947,7 +1947,7 @@
>    static const uint8_t fraction[] = { 0, 1, 2, 2, 3, 3, 0 };
>    unsigned valuex;
> /* We need to convert value to more readable */
> /* We need to convert value to more readable. */

Maybe reword the sentence above.
> valuex =  fraction [value & 0x7];
>    return valuex;
>  }

Since you're already going through this file, how about changing comment
delimiters from // to /**/?

Regards,
Carl-Daniel