Nikolai Vyssotski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45055 )
Change subject: soc/amd/picasso: Fix TSC frequency calculation ......................................................................
Patch Set 3:
(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 l […]
Thanks for pointing this out. I am glad this is the case. I actually re-read https://www.coreboot.org/Coding_Style to make sure I was doing the right thing with 80. It looks like it needs to be updated.
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. […]
I have been scratching my head on this one from the beginning. In the latest PPR and on our PPR on web I only see values up to 0x30. 0x3c value came from AGESA/AgesaModulePkg/Library/CcxPstatesZenZpLib/CcxPstatesZenZpLib.c. I can't find any history in AGESA log on how this value came to be. I figured it would be safer to have expanded range of valid values than restrict it and possibly fail parts that may have valid divider values in the range of 0x31-0x3C. I can change to 0x30 to match PPR if you think it is a good idea, though.
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 […]
Great point. I am all for more verbose comments. I find CB to be overly concise on the comments - was trying to follow the precedent.
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;
Good point. Missed that one. Thanks.