Closed ermo closed 2 years ago
On family 15h model 38h, the output looks fine, so there's maybe some math difference between model 2h and 38h.
The manual for 2h ([00h-0Fh) says: Core current operating frequency in MHz. CoreCOF = 100 * (MSRC001_00[6B:64][CpuFid] + 10h) / (2^MSRC001_00[6B:64][CpuDid]).
10h-1Fh : Core current operating frequency in MHz. CoreCOF = 100 * (MSRC001_00[6B:64][CpuFid] + 10h) / (2^MSRC001_00[6B:64][CpuDid]).
30h-3Fh : Core current operating frequency in MHz. CoreCOF = 100 * (MSRC001_00[6B:64][CpuFid[5:0]] + 10h) / (2^MSRC001_00[6B:64][CpuDid]).
60h-6Fh: Core current operating frequency in MHz. CoreCOF = 100 * (MSRC001_00[6B:64][CpuFid[5:0]] + 10h) / (2^MSRC001_00[6B:64][CpuDid])
70h-7Fh: Core current operating frequency in MHz. CoreCOF = 100 * (MSRC001_00[6B:64][CpuFid[5:0]] + 10h) / (2^MSRC001_00[6B:64][CpuDid]).
The only difference is it specifices the bit range for CpuFid, but that doesn't make a difference, since CpuFid is at 5:0 if we look down lower.
My guess is the math is done wrong in amdctl then, if we do the math ourselves:
Core current operating frequency in MHz. CoreCOF = 100 * (MSRC001_00[6B:64][CpuFid] + 10h) / (2^MSRC001_00[6B:64][CpuDid]).
100 * (26 + 16) / (2 ^ 0)
=
100 * 42 / 1
=
4200
https://github.com/kevinlekiller/amdctl/blob/master/amdctl.c#L631
Try changing to :
case AMD10H:
case AMD16H:
return ((REFCLK * (CpuFid + 0x10)) >> CpuDid);
case AMD15H:
return (REFCLK * (CpuFid + 0x10) / (int) pow(2.0, (float) CpuDid));
Edit:
Both 16h manuals have the same math: Core current operating frequency in MHz. CoreCOF = 100 * (MSRC001_00[6B:64][CpuFid] + 10h) / (2^MSRC001_00[6B:64][CpuDid]).
10h: CPU COF = 100 MHz * (CpuFid + 10h) / (2^CpuDid).
case AMD10H:
case AMD15H:
case AMD16H:
return (REFCLK * (CpuFid + 0x10) / (int) pow(2.0, (float) CpuDid));
11h is different:
The CPU COF specified by MSRC001_00[6B:64][CpuFid,CpuDid] is ((100 MHz * (CpuFid +08h)) / (2^CpuDid)).
• The frequency specified by (100 MHz * (CpuFid + 08h)) must always be >50% of and <= 100% of the frequency specified by F3xD4[MainPllOpFreqId, MainPllOpFreqIdEn].
So I guess it could be changed to
int getClockSpeed(const int CpuFid, const int CpuDid) {
switch (cpuFamily) {
case AMD10H:
case AMD11H:
case AMD15H:
case AMD16H:
return (REFCLK * (CpuFid + (cpuFamily == AMD11H ? 0x08 : 0x10)) / (int) pow(2.0, (float) CpuDid));
case AMD12H:
return (int) (REFCLK * (CpuFid + 0x10) / getDiv(CpuDid));
case AMD17H:
case AMD19H:
return CpuFid && CpuDid ? (int) (((float)CpuFid / (float)CpuDid) * REFCLK * 2) : 0;
default:
return 0;
}
}
Tried switching out the existing code with the edited function you just pasted and this is what I got (this is a different box than the first one I used -- I suspect this box has a BIOS bug with the CPU-NB P-State settings):
ermo@solbox:~/repos/amdctl ⑂master*
$ git diff
diff --git a/amdctl.c b/amdctl.c
index 4b0182c..08a2bbb 100644
--- a/amdctl.c
+++ b/amdctl.c
@@ -626,11 +626,10 @@ float getCpuMultiplier(const int CpuFid, const int CpuDid) {
int getClockSpeed(const int CpuFid, const int CpuDid) {
switch (cpuFamily) {
case AMD10H:
+ case AMD11H:
case AMD15H:
case AMD16H:
- return ((REFCLK * (CpuFid + 0x10)) >> CpuDid);
- case AMD11H:
- return ((REFCLK * (CpuFid + 0x08)) >> CpuDid);
+ return (REFCLK * (CpuFid + (cpuFamily == AMD11H ? 0x08 : 0x10)) / (int) pow(2.0, (float) CpuDid));
case AMD12H:
return (int) (REFCLK * (CpuFid + 0x10) / getDiv(CpuDid));
case AMD17H:
ermo@solbox:~/repos/amdctl ⑂master*
$ make
gcc -Wall -pedantic -Wextra -std=c99 -O2 -lm amdctl.c -o amdctl
amdctl.c:29:29: warning: Incompatible kernel! MSR access deprecated by your Linux kernel! To use this program, set the kernel parameter msr.allow_writes=on or set /sys/module/msr/parameters/allow_writes to on at runtime.
29 | #pragma GCC warning "Incompatible kernel! MSR access deprecated by your Linux kernel! To use this program, set the kernel parameter msr.allow_writes=on or set /sys/module/msr/parameters/allow_writes to on at runtime."
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
ermo@solbox:~/repos/amdctl ⑂master*
$ sudo ./amdctl -g -c0
Voltage ID encodings: SVI (serial)
Detected CPU model 2h, from family 15h with 8 CPU cores.
Core 0 | P-State Limits (non-turbo): Highest: 1 ; Lowest 5 | Current P-State: 1
Pstate Status CpuFid CpuDid CpuVid CpuMult CpuFreq CpuVolt IddVal IddDiv CpuCurr CpuPower
0 1 28 0 10 22.00x 8800MHz 1425mV 134 10 13.40A 19.09W
1 1 26 0 12 21.00x 8400MHz 1400mV 134 10 13.40A 18.76W
2 1 24 0 16 20.00x 8000MHz 1350mV 119 10 11.90A 16.06W
3 1 18 0 24 17.00x 6800MHz 1250mV 95 10 9.50A 11.88W
4 1 12 0 32 14.00x 5600MHz 1150mV 75 10 7.50A 8.62W
5 1 5 0 42 10.50x 4200MHz 1025mV 54 10 5.40A 5.54W
current 1 12 1 12 7.00x 2800MHz 1400mV
Northbridge:
P-State 0: 29 (vid), 1188mV, 4400MHz
P-State 1: 0 (vid), 1550mV, 1600MHz
ermo@solbox:~/repos/amdctl ⑂master*
$ gcc --version
gcc (Solus) 11.2.0
Copyright (C) 2021 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
Looks like it now doesn't divide by 2 like it's supposed to -- neither in the CPU P-states, nor in the NB P-states?
It seems like it's supposed to be 2^(CpuDid+1), such that it's either dividing by 2 (when CpuDid = 0) or 4 (when CpuDid = 1)?
It looks like this if I try my hand at it:
ermo@solbox:~/repos/amdctl ⑂master*
$ git diff
diff --git a/amdctl.c b/amdctl.c
index 4b0182c..bc42bf9 100644
--- a/amdctl.c
+++ b/amdctl.c
@@ -626,11 +626,10 @@ float getCpuMultiplier(const int CpuFid, const int CpuDid) {
int getClockSpeed(const int CpuFid, const int CpuDid) {
switch (cpuFamily) {
case AMD10H:
+ case AMD11H:
case AMD15H:
case AMD16H:
- return ((REFCLK * (CpuFid + 0x10)) >> CpuDid);
- case AMD11H:
- return ((REFCLK * (CpuFid + 0x08)) >> CpuDid);
+ return (int) (REFCLK * (CpuFid + (cpuFamily == AMD11H ? 0x08 : 0x10)) / pow(2.0, (float) CpuDid+1));
case AMD12H:
return (int) (REFCLK * (CpuFid + 0x10) / getDiv(CpuDid));
case AMD17H:
ermo@solbox:~/repos/amdctl ⑂master*
$ make
gcc -Wall -pedantic -Wextra -std=c99 -O2 -lm amdctl.c -o amdctl
amdctl.c:29:29: warning: Incompatible kernel! MSR access deprecated by your Linux kernel! To use this program, set the kernel parameter msr.allow_writes=on or set /sys/module/msr/parameters/allow_writes to on at runtime.
29 | #pragma GCC warning "Incompatible kernel! MSR access deprecated by your Linux kernel! To use this program, set the kernel parameter msr.allow_writes=on or set /sys/module/msr/parameters/allow_writes to on at runtime."
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
ermo@solbox:~/repos/amdctl ⑂master*
$ sudo ./amdctl -g -c0
Voltage ID encodings: SVI (serial)
Detected CPU model 2h, from family 15h with 8 CPU cores.
Core 0 | P-State Limits (non-turbo): Highest: 1 ; Lowest 5 | Current P-State: 1
Pstate Status CpuFid CpuDid CpuVid CpuMult CpuFreq CpuVolt IddVal IddDiv CpuCurr CpuPower
0 1 28 0 10 22.00x 4400MHz 1425mV 134 10 13.40A 19.09W
1 1 26 0 12 21.00x 4200MHz 1400mV 134 10 13.40A 18.76W
2 1 24 0 16 20.00x 4000MHz 1350mV 119 10 11.90A 16.06W
3 1 18 0 24 17.00x 3400MHz 1250mV 95 10 9.50A 11.88W
4 1 12 0 32 14.00x 2800MHz 1150mV 75 10 7.50A 8.62W
5 1 5 0 42 10.50x 2100MHz 1025mV 54 10 5.40A 5.54W
current 1 26 0 12 21.00x 4200MHz 1400mV
Northbridge:
P-State 0: 29 (vid), 1188mV, 4400MHz
P-State 1: 0 (vid), 1550mV, 1600MHz
ermo@solbox:~/repos/amdctl ⑂master*
$
... but then the CPU-NB frequency shown is wrong.
Thoughts?
EDIT: Given the definition from the manual, it seems like a bitshift of CpuDid
should be just as effective as the (more expensive) pov(2.0, CpuDid)
int -> float -> int casting?
EDIT2: Looks like the REFCLK is changed from the default of 100 to 200 for Fam15 CPUs in the 0x0h to 0x10h range? This could account for the discrepancy I'm seeing? So maybe the frequency should simply be calculated like this for Fam15h:
case AMD15H:
return (REFCLK * getCpuMultiplier(CpuFid, CpuDid));
?
From what I can see getClockSpeed() is only used for printing, so shouldn't affect the NB values?
Maybe it's better to make getClockSpeed() return float and cast the arguments to float, instead of doing a bunch of casts?
Here's the output of the 38h (a10 7850k I think - not my machine, but have SSH access to it) CPU with return (REFCLK * (CpuFid + 0x10) / (int) pow(2.0, (float) CpuDid));
.
Core 3 | P-State Limits (non-turbo): Highest: 1 ; Lowest 7 | Current P-State: 1
Pstate Status CpuFid CpuDid CpuVid CpuMult CpuFreq CpuVolt IddVal IddDiv CpuCurr CpuPower
0 1 25 0 8 20.50x 4100MHz 1500mV 180 10 18.00A 27.00W
1 1 23 0 10 19.50x 3900MHz 1488mV 224 10 22.40A 33.32W
2 1 22 0 12 19.00x 3800MHz 1475mV 212 10 21.20A 31.27W
3 1 21 0 18 18.50x 3700MHz 1438mV 200 10 20.00A 28.75W
4 1 19 0 28 17.50x 3500MHz 1375mV 175 10 17.50A 24.06W
5 1 14 0 52 15.00x 3000MHz 1225mV 125 10 12.50A 15.31W
6 1 8 0 76 12.00x 2400MHz 1075mV 82 10 8.20A 8.81W
7 1 18 1 98 8.50x 1700MHz 938mV 55 10 5.50A 5.16W
current 1 23 0 10 19.50x 3900MHz 1488mV
Northbridge:
P-State 0: 52 (vid), 1225mV, 1800MHz
P-State 1: 60 (vid), 1175mV, 1600MHz
P-State 2: 68 (vid), 1125mV, 1400MHz
P-State 3: 78 (vid), 1062mV, 1100MHz
EDIT2: Looks like the REFCLK is changed from the default of 100 to 200 for Fam15 CPUs in the 0x0h to 0x10h >range? This could account for the discrepancy I'm seeing? So maybe the frequency should simply be calculated >like this for Fam15h:
case AMD15H:
return ((REFCLK * getCpuMultiplier(CpuFid, CpuDid));
?
Here's the output of that on the 15h family 38h model CPU, values are halved:
Core 3 | P-State Limits (non-turbo): Highest: 1 ; Lowest 7 | Current P-State: 1
Pstate Status CpuFid CpuDid CpuVid CpuMult CpuFreq CpuVolt IddVal IddDiv CpuCurr CpuPower
0 1 25 0 8 20.50x 2050MHz 1500mV 180 10 18.00A 27.00W
1 1 23 0 10 19.50x 1950MHz 1488mV 224 10 22.40A 33.32W
2 1 22 0 12 19.00x 1900MHz 1475mV 212 10 21.20A 31.27W
3 1 21 0 18 18.50x 1850MHz 1438mV 200 10 20.00A 28.75W
4 1 19 0 28 17.50x 1750MHz 1375mV 175 10 17.50A 24.06W
5 1 14 0 52 15.00x 1500MHz 1225mV 125 10 12.50A 15.31W
6 1 8 0 76 12.00x 1200MHz 1075mV 82 10 8.20A 8.81W
7 1 18 1 98 8.50x 850MHz 938mV 55 10 5.50A 5.16W
current 1 18 1 98 8.50x 850MHz 938mV
EDIT2: Looks like the REFCLK is changed from the default of 100 to 200 for Fam15 CPUs in the 0x0h to 0x10h >range? This could account for the discrepancy I'm seeing? So maybe the frequency should simply be calculated >like this for Fam15h:
https://github.com/kevinlekiller/amdctl/blob/master/amdctl.c#L430
I'm wondering if that's right, will check the manual.
That's the issue indeed, the reference clock is 200, but the equation doesn't use the reference clock, it's 100: Core current operating frequency in MHz. CoreCOF = 100 * (MSRC001_00[6B:64][CpuFid] + 10h) / (2^MSRC001_00[6B:64][CpuDid]).
So return ((100 * (CpuFid + 0x10)) >> CpuDid);
should work right?
So this ?
int getClockSpeed(const int CpuFid, const int CpuDid) {
switch (cpuFamily) {
case AMD10H:
case AMD11H:
case AMD15H:
case AMD16H:
return ((100 * (CpuFid + (cpuFamily == AMD11H ? 0x08 : 0x10))) >> CpuDid);
case AMD12H:
return (int) (REFCLK * (CpuFid + 0x10) / getDiv(CpuDid));
case AMD17H:
return CpuFid && CpuDid ? (int) (((float)CpuFid / (float)CpuDid) * REFCLK * 2) : 0;
default:
return 0;
}
}
Edit: Although I think it's still better to make that function return float, then remove those casts.
Let's take a step back here. =)
I'm adding some extra printf fields in order to be able to ascertain which invariants the code has for my particular CPU, so you can check that against the specs. I'll paste here when I'm ready.
Sound fair?
I've been looking at the manual but can't find where it says the reference clock is 200 MHz, I did find some posts online saying the 8350 is 200 MHz ; here for example.
It's been a long time, and I could be wrong, but I think the idea of using the reference clock to multiply comes from k10ctl, although that idea obviously wrong, since the 10h manual only says 100 MHz, it never mentions the reference clock, but the reference clock is 100 MHz, so that's probably why the assumption was made?
ermo@solbox:~/repos/amdctl ⑂master*
$ git diff
diff --git a/amdctl.c b/amdctl.c
index 4b0182c..da2b364 100644
--- a/amdctl.c
+++ b/amdctl.c
@@ -248,8 +248,8 @@ int main(int argc, char **argv) {
}
getReg(PSTATE_STATUS);
if (!quiet) {
- printf("\nCore %d | P-State Limits (non-turbo): Highest: %d ; Lowest %d | Current P-State: %d\n", core,
- maxPstate, minPstate, curPstate);
+ printf("\nCore %d | P-State Limits (non-turbo): Highest: %d ; Lowest %d | Current P-State: %d | REFCLK: %d\n", core,
+ maxPstate, minPstate, curPstate, REFCLK);
if (cpuFamily == AMD10H || cpuFamily == AMD11H) {
printf(
"%7s%7s%7s%7s%7s%8s%8s%8s%6s%7s%7s%7s%8s%9s\n",
@@ -350,17 +350,26 @@ void northBridge(const int nvid) {
} else {
return;
}
+ // We need to be able to display the REFCLK used for the calculations
+ int _refclk = REFCLK;
+ if (cpuModel >= 0x00 && cpuModel <= 0x0f)
+ {
+ _refclk = 2 * REFCLK;
+ }
for (int nbpstate = 0; nbpstate < nbpstates; nbpstate++) {
getAddr("18.5", addresses[0][nbpstate]);
nbvid = ((getDec("16:10") + (getDec("21:21") << 7)));
nbfid = getDec("7:7");
nbdid = getDec("6:1");
printf(
- "P-State %d: %d (vid), %5.0fmV, %1.0fMHz\n",
+ "P-State %d: %d (vid), %d (fid), %d (did) %5.0fmV, %1.0fMHz, (REFCLK = %d)\n",
nbpstate,
nbvid,
+ nbfid,
+ nbdid,
vidTomV(nbvid),
- (((cpuModel >= 0x00 && cpuModel <= 0x0f) ? REFCLK * 2 : REFCLK) * (nbdid + 0x4) / pow(2, nbfid))
+ (_refclk * (nbdid + 0x4) / pow(2, nbfid)),
+ _refclk
);
}
break;
ermo@solbox:~/repos/amdctl ⑂master*
$ sudo ./amdctl -g -c0
Voltage ID encodings: SVI (serial)
Detected CPU model 2h, from family 15h with 8 CPU cores.
Core 0 | P-State Limits (non-turbo): Highest: 1 ; Lowest 5 | Current P-State: 1 | REFCLK: 200
Pstate Status CpuFid CpuDid CpuVid CpuMult CpuFreq CpuVolt IddVal IddDiv CpuCurr CpuPower
0 1 28 0 10 22.00x 8800MHz 1425mV 134 10 13.40A 19.09W
1 1 26 0 12 21.00x 8400MHz 1400mV 134 10 13.40A 18.76W
2 1 24 0 16 20.00x 8000MHz 1350mV 119 10 11.90A 16.06W
3 1 18 0 24 17.00x 6800MHz 1250mV 95 10 9.50A 11.88W
4 1 12 0 32 14.00x 5600MHz 1150mV 75 10 7.50A 8.62W
5 1 5 0 42 10.50x 4200MHz 1025mV 54 10 5.40A 5.54W
current 1 12 1 12 7.00x 2800MHz 1400mV
Northbridge:
P-State 0: 29 (vid), 0 (fid), 7 (did) 1188mV, 4400MHz, (REFCLK = 400)
P-State 1: 0 (vid), 0 (fid), 0 (did) 1550mV, 1600MHz, (REFCLK = 400)
ermo@solbox:~/repos/amdctl ⑂master*
$
Looking at the above, it simply seems that my FX-8350 Piledriver (Fam15h model 2h) needs a REFCLK of 100 for CPU frequency calculations and a REFCLK of 200 for the CPU-NB frequency calculations, assuming that the rest of the code is kept as is?
No, the issue is the REFCLK value shouldn't be used here, that line should be changed to return ((100 * (CpuFid + 0x10)) >> CpuDid);
.
No, the issue is the REFCLK value shouldn't be used here, that line should be changed to
return ((100 * (CpuFid + 0x10)) >> CpuDid);
.
I get that.
And for the nortbridge, if the values are double, then this line is wrong, since the REFCLK is already set properly here.
That was precisely the reason I instrumented the code; to get an idea of whether the REFCLK invariant was correct (and it turns out it might not be...)
To me it feels like the issue is now properly described.
I still think it might perhaps be useful for the code to print out the extra info you saw in my diff? I.e. extra CPU-NB register values and (possibly) the correct-for-the-CPU-and-current-function-context REFCLK.
I still think it might perhaps be useful for the code to print out the extra info you saw in my diff? I.e. extra CPU-NB register values and (possibly) the correct-for-the-CPU-and-current-function-context REFCLK.
That's a good idea.
Looking at the various online articles (and the BIOS/UEFI settings for my two AM3+ motherboards), the base clock (REFCLK) does appear to be 200 MHz for AM3+ CPUs.
From the perspective of calculating the CPU frequency from a base clock of REFCLK / 2 = 200 / 2 = 100, isn't it just a question of shifting the frequency calculation an extra bit and writing a comment on why it's done that way and then just set the REFCLK correctly to 200 once?
I.e. something like
int getClockSpeed(const int CpuFid, const int CpuDid) {
switch (cpuFamily) {
case AMD15H:
// Uses a REFCLK of 200, but needs a frequency calculation of 100 * (CpuFid + 0x10)) >> CpuDid
// Let's be cheeky and divide REFCLK by 2 by shifting an extra bit via CpuDid+1
return REFCLK * (CpuFid + 0x10) >> (CpuDid+1)
// (...)
Then the only thing left is fixing the superfluous-REFCLK-doubling bug we found and perhaps adding more register values + REFCLK values to the various printf
s?
Though I do wonder if there's perhaps a way to actually read the base clock (REFCLK) from the motherboard or the CPU. But that's another feature request for another day.
Thanks for engaging so quickly with me! :+1:
Looking at the various online articles (and the BIOS/UEFI settings for my two AM3+ motherboards), the base clock (REFCLK) does appear to be 200 MHz for AM3+ CPUs.
This is only true for the original 15h CPU's, all the later ones (like the A10 7850k) are 100 MHz. Edit: That is FM2, but still 15h however.
But regardless, AMD's own reference manual says to use 100 for all the 15h CPU's for doing the calculations, it never says to use the reference clock, like I wrote in a previous comment, this was probably brought over from k10ctl which was doing it wrong in the first place.
Looking at the various online articles (and the BIOS/UEFI settings for my two AM3+ motherboards), the base clock (REFCLK) does appear to be 200 MHz for AM3+ CPUs.
This is only true for the original 15h CPU's, all the later ones (like the A10 7850k) are 100 MHz.
But regardless, AMD's own reference manual says to use 100 for all the 15h CPU's for doing the calculations, it never says to use the reference clock, like I wrote in a previous comment, this was probably brought over from k10ctl which was doing it wrong in the first place.
Fair enough.
Are you expecting me to contribute a PR or would you rather do the reorganisation + bug fixing + extra printf
info stuff yourself in a way that suits your sensibilities? It kinda sounds like you know exactly how you want it to look... :art: :paintbrush:
If you want to do a PR, that would be appreciated!
Hi There,
Loving
amdctl
and am using it on my old PhII and FX-8350 boxes.I get this output when listing the states:
The CPU-NB frequencies are correct, but the CPU P-state frequencies are doubled?
I took a short look at the code, but it seems that the CPU-NB and the CPU P-state frequencies require different interpretations? I'm happy to try out different approaches per your suggestions and then creating a PR once I get it working correctly FWIW.