[coreboot-gerrit] Change in coreboot[master]: nb/intel/x4x/raminit: Refactor receive enable calibration

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/18692 )

Change subject: nb/intel/x4x/raminit: Refactor receive enable calibration
......................................................................


Patch Set 10:

(7 comments)

https://review.coreboot.org/#/c/18692/10/src/northbridge/intel/x4x/rcven.c
File src/northbridge/intel/x4x/rcven.c:

Line 192: 		timing->tap--;
> else?
I think this is some guard against overflowing but it should not happen that tap is 0 here.

else path could be a warning?


PS10, Line 219: mchbar
> Please rename to `foobar`. When I read the old code, I found this
ok


Line 231: 	program_timing(timing, channel, lane);
> Missing overflow check here. You could do it in program_timing()
I'll add the check. I've never seen it even close to overflowing so maybe just a /* TODO: ... */ comment is more appropriate?


Line 252: void rcven(struct sysinfo *s)
> Nit, `const struct ...`
ok.


Line 265: 		mincoarse = 0xff;
> It wasn't reset per channel in the old version (obviously by
ok


Line 266: 		for (i = 0; !RANK_IS_POPULATED(s->dimms, channel, i); i++)
> maybe add ` && i < RANKS_PER_CHANNEL`? It shouldn't be needed but
There are those weird boards that support only 2 ranks per channel and have 2 slots for that channel. (SPD map probably needs to be fixed in that case)


PS10, Line 267: 1024 * 1024
> MiB, #include <commonlib/helpers.h>
ok.


-- 
To view, visit https://review.coreboot.org/18692
To unsubscribe, visit https://review.coreboot.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0c970455e609d3ce96a262cbf110336a2079da4d
Gerrit-PatchSet: 10
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Arthur Heymans <arthur at aheymans.xyz>
Gerrit-Reviewer: Arthur Heymans <arthur at aheymans.xyz>
Gerrit-Reviewer: Damien Zammit <damien at zamaudio.com>
Gerrit-Reviewer: Nico Huber <nico.h at gmx.de>
Gerrit-Reviewer: Patrick Rudolph <siro at das-labor.org>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: Philippe Mathieu-Daudé <philippe.mathieu.daude at gmail.com>
Gerrit-Reviewer: build bot (Jenkins)
Gerrit-Reviewer: coreboot org <coreboot.org at gmail.com>
Gerrit-HasComments: Yes