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

[Smirnov Alexander <alllecs@yandex.ru>]

Hi Sascha!

22.06.2015, 08:43, "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?

Thanks for your review!
I'll try to rework my patch and resubmit it in a few days.

-- 
С уважением,
 Smirnov Alexander

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