Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45055 )
Change subject: soc/amd/picasso: Fix TSC frequency calculation ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/45055/2/src/soc/amd/picasso/tsc_fre... File src/soc/amd/picasso/tsc_freq.c:
https://review.coreboot.org/c/coreboot/+/45055/2/src/soc/amd/picasso/tsc_fre... PS2, Line 35: mhz); coreboot's "official" max width is now 96, so in the future I wouldn't hesitate to put this on the line above. (My personal preference isn't to write for 96 but rather use it when it helps readability.)
https://review.coreboot.org/c/coreboot/+/45055/2/src/soc/amd/picasso/tsc_fre... PS2, Line 36: 0x3c Did you find this value any place in the PPR? AFAICS it's reasonably silent on it. 0x2C is the highest value defined; the text says "Core supports DID up to 30h, but L3 must be 2Ch or less (but I've yet to understand how that works)". Since the higher values are all evens, I don't see why 0x3e shouldn't be acceptable. BTW, I would keep the min at 8 since that's VCO/1.
https://review.coreboot.org/c/coreboot/+/45055/2/src/soc/amd/picasso/tsc_fre... PS2, Line 37: mhz = (unsigned long) ((200 * cpufid) / cpudid); Maybe it's just me reading as a reviewer, but this could maybe use a comment to help translate from what the PPR says, i.e. that mhz = (fid * 25) / (did / 8)
https://review.coreboot.org/c/coreboot/+/45055/2/src/soc/amd/picasso/tsc_fre... PS2, Line 39: (unsigned long) You should be able to lose this casts here and on line 37;