[U-Boot-Users] [PATCH] Fix "i2c sdram" command for DDR2 DIMMs

[Jerry Van Baren <gerald.vanbaren@ge.com>]

Larry Johnson wrote:
> Many of the SPD bytes for DDR2 SDRAM are not interpreted correctly by the
> "i2c sdram" command.  This patch provides correct alternative
> interpretations when DDR2 memory is detected.
> 
> Signed-off-by: Larry Johnson <lrj@acm.org>

Thanks for adding the DDR2 decoding, it is very valuable.

[snip]

>  common/cmd_i2c.c |  615 +++++++++++++++++++++++++++++++++++++++++++-----------
>  1 files changed, 496 insertions(+), 119 deletions(-)

[snip]

> +
> +	switch (type) {
> +	case DDR2:
> +		puts ("CAS latency(s)              ");
> +		if (data[18] & 0x83) puts (" TBD");
> +		if (data[18] & 0x40) puts (" 6");
> +		if (data[18] & 0x20) puts (" 5");
> +		if (data[18] & 0x10) puts (" 4");
> +		if (data[18] & 0x08) puts (" 3");
> +		if (data[18] & 0x04) puts (" 2");
> +		putc ('\n');
> +		break;
> +	default:
> +		puts ("CAS latency(s)              ");
> +		if (data[18] & 0x80) puts (" TBD");
> +		if (data[18] & 0x40) puts (" 7");
> +		if (data[18] & 0x20) puts (" 6");
> +		if (data[18] & 0x10) puts (" 5");
> +		if (data[18] & 0x08) puts (" 4");
> +		if (data[18] & 0x04) puts (" 3");
> +		if (data[18] & 0x02) puts (" 2");
> +		if (data[18] & 0x01) puts (" 1");
> +		putc ('\n');
> +		break;
> +	}
> +
> +	if (DDR2 != type) {
> +		puts ("CS latency(s)               ");
> +		if (data[19] & 0x80) puts (" TBD");
> +		if (data[19] & 0x40) puts (" 6");
> +		if (data[19] & 0x20) puts (" 5");
> +		if (data[19] & 0x10) puts (" 4");
> +		if (data[19] & 0x08) puts (" 3");
> +		if (data[19] & 0x04) puts (" 2");
> +		if (data[19] & 0x02) puts (" 1");
> +		if (data[19] & 0x01) puts (" 0");
> +		putc ('\n');
> +	}
> +
> +	if (DDR2 != type) {
> +		puts ("WE latency(s)               ");
> +		if (data[20] & 0x80) puts (" TBD");
> +		if (data[20] & 0x40) puts (" 6");
> +		if (data[20] & 0x20) puts (" 5");
> +		if (data[20] & 0x10) puts (" 4");
> +		if (data[20] & 0x08) puts (" 3");
> +		if (data[20] & 0x04) puts (" 2");
> +		if (data[20] & 0x02) puts (" 1");
> +		if (data[20] & 0x01) puts (" 0");
> +		putc ('\n');
> +	}


I don't know if it is worth the effort in terms of code size weighed 
against obscuring the actual code, but there are a lot of bit decoding 
to strings going on (and, yes, if you look at the history, it is 
probably my fault).  The following concept should work...

static char *decodeDDR2[] = {
	" TBD",
	" TBD",
	" 2",
	" 3",
	" 4",
	" 5",
	" 6",
	" TBD",
};

static char *decode06[] = {
	" 0",
	" 1",
	" 2",
	" 3",
	" 4",
	" 5",
	" 6",
	" TBD",
};

static char *decode17[] = {
	" 1",
	" 2",
	" 3",
	" 4",
	" 5",
	" 6",
	" 7",
	" TBD",
};

void decodebits(int n, char *str[])
{
	int j, k;

	for (k = 0, j = 0x80; j != 0; j >> 1, k++) {
		if (n & j)
			puts(str[k]);
	}
}

...and then the example usage would be:

	if (DDR2 != type) {
		puts ("WE latency(s)               ");
		bits2string(data[20], decode06);
		putc ('\n');
	}

If we aren't concerned with TBDs for undefined bits, the above lists 
could be condensed into one list that is " 0" through " 8" - one decode 
is 1..8 which could be handled with a decode of 0..7 and a shift.

If you think the decodebits() decoding is worth while, I would strongly 
advocate dropping the "TBDs" and do the following:

static char *decode08[] = {
	" 0",
	" 1",
	" 2",
	" 3",
	" 4",
	" 5",
	" 6",
	" 7",
	" 8",
};

Example for 1..8 decoding:
	default:
		puts ("CAS latency(s)              ");
		decodebits((int)data[18] << 1, decode08);
		putc ('\n');
		break;


There are a couple of decodes that '0' means one thing and '1' means 
another (handled by if/else statements) that the above decode wouldn't 
handle (could, but the penalty would probably be bigger than the gain).

[snip]

> +	switch (type) {
> +	case DDR2:
> +		if (data[22] & 0x80) puts ("  TBD (bit 7)\n");
> +		if (data[22] & 0x40) puts ("  TBD (bit 6)\n");
> +		if (data[22] & 0x20) puts ("  TBD (bit 5)\n");
> +		if (data[22] & 0x10) puts ("  TBD (bit 4)\n");
> +		if (data[22] & 0x08) puts ("  TBD (bit 3)\n");
> +		if (data[22] & 0x04)
> +			puts ("  Supports parital array self refresh\n");

Typo: "partial"

[snip]

> +	switch (type) {
> +	case DDR2:
> +		printf("SDRAM cycle time (2nd highest CAS latency)        %d.",
> +		       (data[23] >> 4) & 0x0F);
> +
> +		switch (data[23] & 0x0F) {
> +		case 0x0:
> +		case 0x1:
> +		case 0x2:
> +		case 0x3:
> +		case 0x4:
> +		case 0x5:
> +		case 0x6:
> +		case 0x7:
> +		case 0x8:
> +		case 0x9:
> +			printf("%d ns\n", data[23] & 0x0F);
> +			break;
> +		case 0xA:
> +			puts("25 ns\n");
> +			break;
> +		case 0xB:
> +			puts("33 ns\n");
> +			break;
> +		case 0xC:
> +			puts("66 ns\n");
> +			break;
> +		case 0xD:
> +			puts("75 ns\n");
> +			break;
> +		default:
> +			puts("?? ns\n");
> +			break;

We saw this code before, could refactor into a subroutine that does the 
decoding and printing.

[snip]

> +	switch (type) {
> +	case DDR2:
> +		printf("SDRAM cycle time (3rd highest CAS latency)        %d.",
> +		       (data[25] >> 4) & 0x0F);
> +
> +		switch (data[25] & 0x0F) {
> +		case 0x0:
> +		case 0x1:
> +		case 0x2:
> +		case 0x3:
> +		case 0x4:
> +		case 0x5:
> +		case 0x6:
> +		case 0x7:
> +		case 0x8:
> +		case 0x9:
> +			printf("%d ns\n", data[25] & 0x0F);
> +			break;
> +		case 0xA:
> +			puts("25 ns\n");
> +			break;
> +		case 0xB:
> +			puts("33 ns\n");
> +			break;
> +		case 0xC:
> +			puts("66 ns\n");
> +			break;
> +		case 0xD:
> +			puts("75 ns\n");
> +			break;
> +		default:
> +			puts("?? ns\n");
> +			break;
> +		}
> +		break;

Hey, deja vue all over again.  :-)

[snip]

> +	switch (type) {
> +	case DDR2:
> +		printf("Minimum row precharge        %d", data[27] >> 2);
> +		switch (data[27] & 0x03) {
> +			case 0x0: puts(".00 ns\n"); break;
> +			case 0x1: puts(".25 ns\n"); break;
> +			case 0x2: puts(".50 ns\n"); break;
> +			case 0x3: puts(".75 ns\n"); break;
> +		}
> +		break;
> +	default:
> +		printf("Minimum row precharge        %d nS\n", data[27]);
> +		break;
> +	}
> +
> +	switch (type) {
> +	case DDR2:
> +		printf("Row active to row active min %d", data[28] >> 2);
> +		switch (data[28] & 0x03) {
> +			case 0x0: puts(".00 ns\n"); break;
> +			case 0x1: puts(".25 ns\n"); break;
> +			case 0x2: puts(".50 ns\n"); break;
> +			case 0x3: puts(".75 ns\n"); break;
> +		}
> +		break;
> +	default:
> +		printf("Row active to row active min %d nS\n", data[28]);
> +		break;
> +	}
> +
> +	switch (type) {
> +	case DDR2:
> +		printf("RAS to CAS delay min         %d", data[29] >> 2);
> +		switch (data[29] & 0x03) {
> +			case 0x0: puts(".00 ns\n"); break;
> +			case 0x1: puts(".25 ns\n"); break;
> +			case 0x2: puts(".50 ns\n"); break;
> +			case 0x3: puts(".75 ns\n"); break;
> +		}
> +		break;

Hmm, another troika that could be refactored into a helper subroutine. 
On second thought, this is probably not worth a helper routine (too 
trivial?): replacing the switch statements with arithmetic is going to 
be slightly less speed efficient, but quite a bit more code efficient:

	printf(".%02d ns\n", (data[29] & 0x03) * 25);

[snip]

Thanks again,
gvb