Re: [RFC] add decode command (SPD EEPROM data decoder)

[Sascha Hauer <s.hauer@pengutronix.de>]

Hi Alexander,

On Fri, Jun 19, 2015 at 06:46:02PM +0300, Alexander Smirnov wrote:
> decode-dimms perl script is used as prototype
> (see https://github.com/groeck/i2c-tools/blob/master/eeprom/decode-dimms).
> 
> Here is a sample decode output:
> 
> barebox@barebox sandbox:/ decode env/crucial_pc2-6400_ddr2
> Decoding EEPROM: env/crucial_pc2-6400_ddr2
> 
> ---=== SPD EEPROM Information ===---
> EEPROM Checksum of bytes 0-62			 OK (0xCA)
> Total number of bytes in EEPROM			 256
> Fundamental Memory type				 DDR2 SDRAM
> SPD Revision					 1.3
> 
> ---=== Memory Characteristics ===---
> Maximum module speed				 800 MHz (PC2-6400)
> Size						 1024 MB
> Banks x Rows x Columns x Bits			 8 x 14 x 10 x 64
> Ranks						 1
> SDRAM Device Width				 8 bits
> Module Height					 30.0 mm
> Module Type					 SO-DIMM (67.6 mm)
> DRAM Package					 Planar
> Voltage Interface Level				 SSTL 1.8V
> Module Configuration Type 			 No Parity
> Refresh Rate					 Reduced (7.8 us) - Self Refresh
> Supported Burst Lengths				 4, 8
> Supported CAS Latencies (tCL)			 6T
> tCL-tRCD-tRP-tRAS				 6-6-6-18 as DDR2-800
> Minimum Cycle Time				 250 (ns*100) at CAS 6
> Maximum Access Time				 4 (ns*10) at CAS 6
> Maximum Cycle Time (tCK max)			 8 ns
> 
> ---=== Timing Parameters ===---
> Address/Command Setup Time Before Clock (tIS)	 17 (ns*100)

Better print these in ns directly?

> +static int do_decode(int argc, char *argv[])
> +{
> +	int highestCAS = 0;
> +	int cas[256];
> +	int i, i_i, k;
> +	int ddrclk, tbits, pcclk;
> +	int trcd, trp, tras;
> +	int ctime;
> +	int fp;
> +	uint8_t record[256];
> +	uint8_t parity;
> +	char *ref;
> +
> +	if (argc != 2) {
> +		printf("Отсутствует или указано больше 1 аргумента\n");

I think most readers can't read this language ;)

> +		return 2;
> +	}
> +
> +	fp = open(argv[1], O_RDONLY);

better 'fd'? It's a file descriptor, not a file pointer.

> +
> +	if (fp == 0) {
> +		perror("Ошибка при работе с файлом");
> +		return 3;
> +	}
> +
> +	read(fp, &record[0], 256);
> +	close(fp);

You should separate the command code from the dispatching/printing code.
let the dispatching/printing code take a void *buf and put it into
common/. This way we can call this not only from the command but also
from C code which makes this more useful.

> +
> +//	dump(&record[0], 256);
> +
> +	printf("Decoding EEPROM: %s\n\n", argv[1]);
> +
> +	printf("---=== SPD EEPROM Information ===---\n");
> +	printf("EEPROM Checksum of bytes 0-62\t\t\t OK (0x%0X)\n", record[63]);
> +	printf("# of bytes written to SDRAM EEPROM\t\t %d\n", record[0]);
> +	printf("Total number of bytes in EEPROM\t\t\t %d\n", 1 << record[1]);
> +
> +	if (record[2] < 11) {
> +		printf("Fundamental Memory type\t\t\t\t %s\n", type_list[record[2]]);
> +	} else {
> +		printf("Warning: unknown memory type (%02x)\n", record[2]);
> +	}

Please skip the braces when they are not needed.

Generally we already have include/ddr_spd.h. If you put this struct over
the buffer then the dispatching should be more straight forward.
Wouldn't that be possible?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox