Nikolai Vyssotski has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45055 )
Change subject: soc/amd/picasso: Fix TSC frequency calculation ......................................................................
soc/amd/picasso: Fix TSC frequency calculation
Fix TSC frequency calculation per Picasso PPR. This code was copied from Stoney and was incorrect for Picasso.
BUG=b:163423984 TEST=verify Dalboz TSC to be 1GHz BRANCH=zork
Change-Id: Ibe3f49c7d295e7336ee042da2b94823171b6eb55 Signed-off-by: Nikolai Vyssotski nikolai.vyssotski@amd.corp-partner.google.com --- M src/soc/amd/picasso/tsc_freq.c 1 file changed, 11 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/45055/1
diff --git a/src/soc/amd/picasso/tsc_freq.c b/src/soc/amd/picasso/tsc_freq.c index a86080c..c863657 100644 --- a/src/soc/amd/picasso/tsc_freq.c +++ b/src/soc/amd/picasso/tsc_freq.c @@ -22,9 +22,17 @@ if (!(msr.hi & 0x80000000)) die("Unknown error: cannot determine P-state 0\n");
- cpufid = (msr.lo & 0x3f); - cpudid = (msr.lo & 0x1c0) >> 6; + cpufid = (msr.lo & 0xff); + cpudid = (msr.lo & 0x3f00) >> 8;
- mhz = (100 * (cpufid + 0x10)) / (0x01 << cpudid); + if (!cpudid) + mhz = 0; + else if ((cpudid >= 8) && (cpudid < 0x3c)) + mhz = (unsigned long) ((200 * cpufid) / cpudid); + else { + printk(BIOS_INFO, "Incorrect core frequency divisor %x, assume 1\n", cpudid); + mhz = (unsigned long) (25 * cpufid); + } + return mhz; }
Kangheui Won has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45055 )
Change subject: soc/amd/picasso: Fix TSC frequency calculation ......................................................................
Patch Set 1: Code-Review+1
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45055 )
Change subject: soc/amd/picasso: Fix TSC frequency calculation ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45055/1/src/soc/amd/picasso/tsc_fre... File src/soc/amd/picasso/tsc_freq.c:
https://review.coreboot.org/c/coreboot/+/45055/1/src/soc/amd/picasso/tsc_fre... PS1, Line 29: mhz = 0; Is this possible? Print an error?
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 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45055/1/src/soc/amd/picasso/tsc_fre... File src/soc/amd/picasso/tsc_freq.c:
https://review.coreboot.org/c/coreboot/+/45055/1/src/soc/amd/picasso/tsc_fre... PS1, Line 29: mhz = 0;
Is this possible? Print an error?
It should not really happen. But 0 is a valid value which indicates that clock is off. We should not be able to run in this case. Do you think we still need to add printk or die here?
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45055 )
Change subject: soc/amd/picasso: Fix TSC frequency calculation ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45055/1/src/soc/amd/picasso/tsc_fre... File src/soc/amd/picasso/tsc_freq.c:
https://review.coreboot.org/c/coreboot/+/45055/1/src/soc/amd/picasso/tsc_fre... PS1, Line 29: mhz = 0;
It should not really happen. But 0 is a valid value which indicates that clock is off. […]
If it’s an condition, that shouldn’t happen, I think at least a warning or notice should be printed.
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 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45055/1/src/soc/amd/picasso/tsc_fre... File src/soc/amd/picasso/tsc_freq.c:
https://review.coreboot.org/c/coreboot/+/45055/1/src/soc/amd/picasso/tsc_fre... PS1, Line 29: mhz = 0;
If it’s an condition, that shouldn’t happen, I think at least a warning or notice should be printed.
I suspect it could theoretically be possible in pre-production silicon, given a broken part, the right fuse recipe, etc., and that's why AGESA builds in certain contingencies.
I recommend calculating the correct frequency. And if that's not possible (for any reason) print an appropriate message and return a conservative number, i.e. still attempt to boot.
Hello build bot (Jenkins), Furquan Shaikh, Kangheui Won, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45055
to look at the new patch set (#2).
Change subject: soc/amd/picasso: Fix TSC frequency calculation ......................................................................
soc/amd/picasso: Fix TSC frequency calculation
Fix TSC frequency calculation per Picasso PPR. This code was copied from Stoney and was incorrect for Picasso.
BUG=b:163423984 TEST=verify Dalboz TSC to be 1GHz BRANCH=zork
Change-Id: Ibe3f49c7d295e7336ee042da2b94823171b6eb55 Signed-off-by: Nikolai Vyssotski nikolai.vyssotski@amd.corp-partner.google.com --- M src/soc/amd/picasso/tsc_freq.c 1 file changed, 18 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/45055/2
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 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45055/1/src/soc/amd/picasso/tsc_fre... File src/soc/amd/picasso/tsc_freq.c:
https://review.coreboot.org/c/coreboot/+/45055/1/src/soc/amd/picasso/tsc_fre... PS1, Line 29: mhz = 0;
I suspect it could theoretically be possible in pre-production silicon, given a broken part, the rig […]
Done
Aaron Durbin 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: Code-Review+2
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;
Hello build bot (Jenkins), Furquan Shaikh, Kangheui Won, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45055
to look at the new patch set (#3).
Change subject: soc/amd/picasso: Fix TSC frequency calculation ......................................................................
soc/amd/picasso: Fix TSC frequency calculation
Fix TSC frequency calculation per Picasso PPR. This code was copied from Stoney and was incorrect for Picasso.
BUG=b:163423984 TEST=verify Dalboz TSC to be 1GHz BRANCH=zork
Change-Id: Ibe3f49c7d295e7336ee042da2b94823171b6eb55 Signed-off-by: Nikolai Vyssotski nikolai.vyssotski@amd.corp-partner.google.com --- M src/soc/amd/picasso/tsc_freq.c 1 file changed, 17 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/45055/3
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.
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 3:
(2 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 36: 0x3c
I have been scratching my head on this one from the beginning. […]
Meh, I'd probably lean the other way and go with 0x3e.
https://review.coreboot.org/c/coreboot/+/45055/2/src/soc/amd/picasso/tsc_fre... PS2, Line 37: mhz = (unsigned long) ((200 * cpufid) / cpudid);
Great point. I am all for more verbose comments. […]
It just took me a bit to understand what you were doing because it didn't match the PPR. Works out the same, of course.
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 3: Code-Review+2
Aaron Durbin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45055 )
Change subject: soc/amd/picasso: Fix TSC frequency calculation ......................................................................
soc/amd/picasso: Fix TSC frequency calculation
Fix TSC frequency calculation per Picasso PPR. This code was copied from Stoney and was incorrect for Picasso.
BUG=b:163423984 TEST=verify Dalboz TSC to be 1GHz BRANCH=zork
Change-Id: Ibe3f49c7d295e7336ee042da2b94823171b6eb55 Signed-off-by: Nikolai Vyssotski nikolai.vyssotski@amd.corp-partner.google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/45055 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/soc/amd/picasso/tsc_freq.c 1 file changed, 17 insertions(+), 3 deletions(-)
Approvals: build bot (Jenkins): Verified Marshall Dawson: Looks good to me, approved
diff --git a/src/soc/amd/picasso/tsc_freq.c b/src/soc/amd/picasso/tsc_freq.c index a86080c..8a541fc 100644 --- a/src/soc/amd/picasso/tsc_freq.c +++ b/src/soc/amd/picasso/tsc_freq.c @@ -7,6 +7,10 @@
static unsigned long mhz;
+/* Use this default TSC frequency when it can not be correctly calculated. + Higher numbers are safer as it will result in longer delays using TSC */ +#define TSC_DEFAULT_FREQ_MHZ 4000 + unsigned long tsc_freq_mhz(void) { msr_t msr; @@ -22,9 +26,19 @@ if (!(msr.hi & 0x80000000)) die("Unknown error: cannot determine P-state 0\n");
- cpufid = (msr.lo & 0x3f); - cpudid = (msr.lo & 0x1c0) >> 6; + cpufid = (msr.lo & 0xff); + cpudid = (msr.lo & 0x3f00) >> 8;
- mhz = (100 * (cpufid + 0x10)) / (0x01 << cpudid); + /* normally core frequency is calculated as (fid * 25) / (did / 8) */ + if (!cpudid) { + mhz = TSC_DEFAULT_FREQ_MHZ; + printk(BIOS_ERR, "Invalid divisor, set TSC frequency to %ldMHz\n", mhz); + } else if ((cpudid >= 8) && (cpudid < 0x3c)) { + mhz = (200 * cpufid) / cpudid; + } else { + mhz = 25 * cpufid; + printk(BIOS_ERR, "Invalid frequency divisor 0x%x, assume 1\n", cpudid); + } + return mhz; }
-
Aaron Durbin (Code Review) -
Kangheui Won (Code Review)
-
Marshall Dawson (Code Review)
-
Nikolai Vyssotski (Code Review)
-
Paul Menzel (Code Review)