Re: [PATCH i2c-tools v3 3/8] decode-dimms: Decode timings and other data for DDR5

["Brigham Campbell" <me@brighamcampbell.com>]

On Mon Jun 29, 2026 at 1:10 AM MDT, Stephen Horvath wrote:
> Decode size, timings, and other data for DDR5 based on the JEDEC
> JESD400-5B specification.
>
> @@ -2128,6 +2128,280 @@ sub decode_ddr4_sdram($)
>  	}
>  }
>  
> +# DDR5 Rounding Algorithms
> +sub ddr5_min_round($$)
> +{
> +	my ($tck, $ps) = @_;
> +	my $correction = 3;  # 0.30% per the rounding algorithm
> +	return int(($ps * (1000 - $correction) / $tck + 1000) / 1000);

This subroutine seems to assume that $ps and $tck are passed in as whole
numbers. In the datasheet, these quantities are wrapped in a trucation
operation before they're used in the calculation.

Maybe it's worth wrapping $tck and $ps in int()? However, this is not a
problem if the caller always makes sure the values are truncated.

See "Rounding Algorithms" below section 8.1.20.

> +}
> +
> +sub ddr5_round_twr($$)
> +{
> +	my ($tck, $twr) = @_;
> +	my $correction = 3;  # 0.30% per the rounding algorithm
> +	my $new_twr = $twr * (1000 - $correction);
> +	$tck = ($new_twr / $tck) + 1000;
> +
> +	return $twr / int($tck / 1000);

Is the body of this function equivalent to the following?

	return $twr / ddr5_min_round($tck, $twr);

No need to duplicate logic in ddr5_min_round, unless there's some reason
that's not obvious.

> +# Return combined time in ns
> +sub ddr5_ns($$)
> +{
> +	my ($bytes, $index) = @_;
> +
> +	return (($bytes->[$index + 1] << 8) | $bytes->[$index]) / 1000;
> +}
> +
> +# speed
> +	prints("Memory Characteristics");
> +
> +	$ctime = ddr5_ns($bytes, 20);
> +	$ctime = ddr5_round_twr($ctime, $twr);
> +	$ctime_max = ddr5_ns($bytes, 22);
> +	$ctime_max = ddr5_round_twr($ctime_max, $twr);

The datasheet indicates that these quantities are in
picoseconds, not nanoseconds.

> +
> +	my $ddrclk = 2 * (1000 / $ctime);
> +	my $tbits = 8 << ($bytes->[235] & 7);
> +	my $pcclk = int ($ddrclk * $tbits / 8);
> +	# Round down to comply with Jedec
> +	$pcclk = ($pcclk - ($pcclk % 100)) * 2;
> +	$ddrclk = int ($ddrclk);
> +	printl("Maximum module speed", "$ddrclk MT/s (PC5-${pcclk})");
> +

... I skimmed a little here, rather than verify every single register.
It looks pretty good. :D

> +
> +	my $taa;
> +	my $trcd;
> +	my $trp;
> +	my $tras;
> +
> +	$taa  = ddr5_ns($bytes, 30);
> +	$trcd = ddr5_ns($bytes, 32);
> +	$trp  = ddr5_ns($bytes, 34);
> +	$tras = ddr5_ns($bytes, 36);

These also appear to actually be picoseconds.

> +
> +	printl("CL-RCD-RP-RAS (cycles)",
> +	       ddr5_core_timings(ddr5_min_round($ctime, $taa),
> +						$ctime, $trcd, $trp, $tras));
> +
> +# more timing information
> +	prints("Timing Parameters");
> +
> +	printl("Minimum Cycle Time (tCKmin)", tns3($ctime));
> +	printl("Maximum Cycle Time (tCKmax)", tns3($ctime_max));
> +	printl("Minimum CAS Latency Time (tAA)", tns3($taa));
> +	printl("Minimum Active to Read/Write Delay (tRCD)", tns3($trcd));
> +	printl("Minimum Row Precharge Delay (tRP)", tns3($trp));
> +	printl("Minimum Active to Precharge Delay (tRAS)", tns3($tras));
> +	printl("Minimum Active to Auto-Refresh Delay (tRC)", tns3(ddr5_ns($bytes, 38)));
> +	printl("Minimum Write Recovery Time (tWR)", tns3($twr));
> +	printl("Minimum Normal Recovery Delay (tRFC1)", tns3(ddr5_ns($bytes, 42) * 1000));
> +	printl("Minimum Fine Recovery Delay (tRFC2)", tns3(ddr5_ns($bytes, 44) * 1000));
> +	printl("Minimum Same Bank Recovery Delay (tRFCsb)", tns3(ddr5_ns($bytes, 46) * 1000));

Datasheet reports that bytes 42-54 actually are nanoseconds, so this
usage is correct. When fixing the other measurements, be careful to not
switch these from nanoseconds to picoseconds too!

> +	printl("Minimum Row Active to Row Active Delay (tRRD_L)",
> +	       tns3(ddr5_ns($bytes, 70)) . " / " . $bytes->[72] . " cycles");
> +	printl("Minimum Read to Read Delay (tCCD_L)",
> +	       tns3(ddr5_ns($bytes, 73)) . " / " . $bytes->[75] . " cycles");
> +	printl("Minimum Write to Write Delay (tCCD_L_WR)",
> +	       tns3(ddr5_ns($bytes, 76)) . " / " . $bytes->[78] . " cycles");
> +	printl("Minimum 2nd Write to Write Delay (tCCD_L_WR2)",
> +	       tns3(ddr5_ns($bytes, 79)) . " / " . $bytes->[81] . " cycles");
> +	printl("Minimum Four Activate Window Delay (tFAW)",
> +	       tns3(ddr5_ns($bytes, 82)) . " / " . $bytes->[84] . " cycles");
> +	printl("Minimum Write to Read Delay for Same Bank Group (tCCD_L_WTR)",
> +	       tns3(ddr5_ns($bytes, 85)) . " / " . $bytes->[87] . " cycles");
> +	printl("Minimum Write to Read Delay for Different Bank Group (tCCD_S_WTR)",
> +	       tns3(ddr5_ns($bytes, 88)) . " / " . $bytes->[90] . " cycles");
> +	printl("Minimum Read to Precharge Delay (tRTP)",
> +	       tns3(ddr5_ns($bytes, 91)) . " / " . $bytes->[93] . " cycles");
> +	# Optional? Not stated, but not present in my modules.

Is this "Optional?" comment referring to the following code? Sections
8.1.48-50 don't appear to indicate that the following information is
optional... Maybe you already mentioned this somewhere, but did some
behavior during testing give the impression that these values aren't
present on all DIMMs?

> +	printl_cond($bytes->[96], "Minimum Read to Read Delay Delay for Different Bank in Group (tCCD_M)",
> +		    tns3(ddr5_ns($bytes, 94)) . " / " . $bytes->[96] . " cycles");
> +	printl_cond($bytes->[99], "Minimum Write to Write Delay for Different Bank in Group (tCCD_M_WR)",
> +		    tns3(ddr5_ns($bytes, 97)) . " / " . $bytes->[99] . " cycles");
> +	printl_cond($bytes->[102], "Minimum Write to Read Delay for Different Bank in Group (tCCD_M_WTR)",
> +		    tns3(ddr5_ns($bytes, 100)) . " / " . $bytes->[102] . " cycles");
> +

This patch is looking pretty good! Impressive work.

Cheers,
-- 
Brigham Campbell
https://brighamcampbell.com