Attention is currently required from: Arthur Heymans. Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52922 )
Change subject: nb/amd/{agesa,pi}: Avoid overflows during DRAM calculation ......................................................................
Patch Set 4:
(1 comment)
Patchset:
PS4:
Thanks for splitting the patch. It's still a bit big, though. Took me almost 3 hours to read through the first two platforms (fam14 and fam15tn). The new functions look similar enough to extract common code. What generally works well to get a break during review is to add common code in one commit, and then update the individual platforms one-by-one in separate commits.
I will try doing better in that matter. Thanks for the advise.
The code changes look sane, alas the comments and the commit message don't. Without kowing further details, it seems possible that the change of the granularity for the newer platforms results in further problems. And this change is not even mentioned in the commit message.
IMHO, a simple comment like
/* The upper half of each register provides the DRAM base/limit in 16MiB steps. */
could replace all the error-prone and redundant (considering that the documentation is public) comments about bit offsets.
That bit offset error is my bad, indeed it is documented in public BKDGs, although I thought it gives more readability.
I did not see any overflowing shift operations. Though, it looks like the original code treated the limit as exclusive, while it wasn't.
I didn't dig that deep where the data is lost in limit calculation, but certainly on a 2GB platform, where the limit should be 0x7f000000, it calculated 0x78000000 (112MB lost). I have implemented it in a way that is more understandable to me and does less bitshifts than this:
d.mask = ((temp & 0xfff80000)>>(8+3)); // mask out DramMask [26:24] too limit_k = ((resource_t)(((d.mask & ~1) + 0x000FF) & 0x1fffff00)) << 9;
But if we look closer the `// mask out DramMask [26:24] too` may be the case why we lose 112MB, that's exactly those 3 least significant bits.
I will address your comments in a new patch