Hi Sascha! 22.06.2015, 08:43, "Sascha Hauer" <s.hauer@xxxxxxxxxxxxxx>: > 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@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/barebox
