Nico Huber has posted comments on this change. ( https://review.coreboot.org/21480 )
Change subject: device/dram/ddr2.c: Improve error returning and debug output ......................................................................
Patch Set 2:
(7 comments)
Thanks for taking this up.
https://review.coreboot.org/#/c/21480/2/src/device/dram/ddr2.c File src/device/dram/ddr2.c:
https://review.coreboot.org/#/c/21480/2/src/device/dram/ddr2.c@116 PS2, Line 116: static int spd_decode_tck_time(u8 c, u32 *tck) I'd prefer the pointer to be the first argument (old writel vs write32 debate).
https://review.coreboot.org/#/c/21480/2/src/device/dram/ddr2.c@119 PS2, Line 119: int ret = CB_SUCCESS; We could just bail out directly and drop the variable. Not writing `*tck` isn't bad, IMO. Just a matter of personal preference, I guess.
https://review.coreboot.org/#/c/21480/2/src/device/dram/ddr2.c@155 PS2, Line 155: * Returns cycle time in 1/256th ns. (I hate comments)
https://review.coreboot.org/#/c/21480/2/src/device/dram/ddr2.c@157 PS2, Line 157: u32 nit, er, int ;)
https://review.coreboot.org/#/c/21480/2/src/device/dram/ddr2.c@350 PS2, Line 350: printk(BIOS_WARNING, " Invalid number of memory rows\n"); I don't think, we should keep the indent. With low loglevel, the message would be freestanding. Also we might want to give some context for that case, e.g. "... in SPD" or "SPD decode: ..."?
https://review.coreboot.org/#/c/21480/2/src/device/dram/ddr2.c@440 PS2, Line 440: if (spd_decode_bcd_time(spd[10], &dimm->access_time[cl]) != CB_SUCCESS) { line is too long
https://review.coreboot.org/#/c/21480/2/src/device/dram/ddr2.c@480 PS2, Line 480: printk(BIOS_WARNING," Invalid rank density.\n"); space after comma, no space before the output?