[coreboot] Some typos in raminit_f.c (Re: r3866 - trunk/coreboot-v2/src/northbridge/amd/amdk8)
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Fri Jan 16 13:34:47 CET 2009
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 <paulepanter at users.sourceforge.net>
>
> 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
--
http://www.hailfinger.org/
More information about the coreboot
mailing list