[PATCH] Heterogenous dual channel support, part 2

[Carl-Daniel Hailfinger]

The check for compatible tCL and associated timing after the parameters
have already been determined makes sure we don't drive any DIMM with
incorrect settings.

In a heterogenous dual channel setup, each DIMM must be checked for that
criterion.

Factor out the check to prepare for per-DIMM checks.

I'd appreciate a thorough review, especially the flow of error and
default paths of the code. They should be identical.

Once this is applied, a roughly 10-line change is sufficient to use
DIMMS with compatible, but not identical tCL timings.

Signed-off-by: Carl-Daniel Hailfinger

Index: LinuxBIOSv2-asus_m2a-vm/src/northbridge/amd/amdk8/raminit_f.c
===================================================================
--- LinuxBIOSv2-asus_m2a-vm/src/northbridge/amd/amdk8/raminit_f.c	(Revision 3867)
+++ LinuxBIOSv2-asus_m2a-vm/src/northbridge/amd/amdk8/raminit_f.c	(Arbeitskopie)
@@ -1792,6 +1792,51 @@
    return 0;
 }

+int check_spd_latency_available(u32 spd_device, unsigned min_cycle_time, unsigned min_latency)
+{
+	int latencies;
+	int latency;
+	int index;
+	int value;
+
+	latencies = spd_read_byte(spd_device, SPD_CAS_LAT);
+	if (latencies < 0)
+		return -1;
+	if (latencies == 0)
+		return 1;
+
+	/* Compute the lowest cas latency supported */
+	latency = log2(latencies) -2;
+
+	/* Walk through searching for the selected latency */
+	for (index = 0; index < 3; index++, latency++) {
+		if (!(latencies & (1 << latency))) {
+			continue;
+		}
+		if (latency == min_latency)
+			break;
+	}
+	/* If I can't find the latency or my index is bad error */
+	if ((latency != min_latency) || (index >= 3)) {
+		return -2;
+	}
+
+	/* Read the min_cycle_time for this latency */
+	value = spd_read_byte(spd_device, latency_indicies[index]);
+	if (value < 0)
+		return -1;
+
+	value = convert_to_linear(value);
+	/* All is good if the selected clock speed
+	 * is what I need or slower.
+	 */
+	if (value <= min_cycle_time)
+		return 1;
+
+	/* That didn't work out... */
+	return -2;
+}
+
 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 */
@@ -1862,10 +1907,6 @@
    printk_raminit("3 min_latency: %08x\n", min_latency);

for (i = 0; (i < DIMM_SOCKETS); i++) {
-		int latencies;
-		int latency;
-		int index;
-		int value;
    	u32 spd_device = ctrl->channel0[i];

if (!(meminfo->dimm_mask & (1 << i))) {
@@ -1876,42 +1917,17 @@
    		}
    	}

-		latencies = spd_read_byte(spd_device, SPD_CAS_LAT);
-		if (latencies < 0) goto hw_error;
-		if (latencies == 0) {
+		switch (check_spd_latency_available(spd_device, min_cycle_time, min_latency)) {
+		case -2:
+			/* We have an error, disable the dimm */
+			meminfo->dimm_mask = disable_dimm(ctrl, i, meminfo);
+			break;
+		case -1:
+			goto hw_error;
+			break;
+		case 1:
    		continue;
    	}
-
-		/* Compute the lowest cas latency supported */
-		latency = log2(latencies) -2;
-
-		/* Walk through searching for the selected latency */
-		for (index = 0; index < 3; index++, latency++) {
-			if (!(latencies & (1 << latency))) {
-				continue;
-			}
-			if (latency == min_latency)
-				break;
-		}
-		/* If I can't find the latency or my index is bad error */
-		if ((latency != min_latency) || (index >= 3)) {
-			goto dimm_err;
-		}
-
-		/* Read the min_cycle_time for this latency */
-		value = spd_read_byte(spd_device, latency_indicies[index]);
-		if (value < 0) goto hw_error;
-
-		value = convert_to_linear(value);
-		/* All is good if the selected clock speed
-		 * is what I need or slower.
-		 */
-		if (value <= min_cycle_time) {
-			continue;
-		}
-		/* Otherwise I have an error, disable the dimm */
-	dimm_err:
-		meminfo->dimm_mask = disable_dimm(ctrl, i, meminfo);
    }

printk_raminit("4 min_cycle_time: %08x\n", min_cycle_time);

[Carl-Daniel Hailfinger]

Marc,

I believe you are the person who is most proficient in K8 RAM init (no
offense to anyone else intended). Could you take a look at the patch and
give me a few comments?

Thanks.

Regards,
Carl-Daniel

On 16.01.2009 05:17, Carl-Daniel Hailfinger wrote:
> The check for compatible tCL and associated timing after the parameters
> have already been determined makes sure we don't drive any DIMM with
> incorrect settings.
>
> In a heterogenous dual channel setup, each DIMM must be checked for that
> criterion.
>
> Factor out the check to prepare for per-DIMM checks.
>
> I'd appreciate a thorough review, especially the flow of error and
> default paths of the code. They should be identical.
>
> Once this is applied, a roughly 10-line change is sufficient to use
> DIMMS with compatible, but not identical tCL timings.
>
> Signed-off-by: Carl-Daniel Hailfinger

[Marc Jones]

On Tue, Jan 20, 2009 at 5:32 AM, Carl-Daniel Hailfinger
 wrote:
> Marc,
>
> I believe you are the person who is most proficient in K8 RAM init (no
> offense to anyone else intended). Could you take a look at the patch and
> give me a few comments?

I think that the code flow is equivalent. Maybe add a comment about
the return values for the functions. The case statement could have a
default to catch an unexpected return. Maybe make the hw_error the
default?
> if (latencies == 0) {

That should probably cause a disable dimm.

I think that spd_set_memclk() has the same CAS checking so you could
do the fix their as well. Search for SPD_CAS_LAT.

Marc