intel/intelpmc: Add Intel PMC Core driver

[seuros]

Summary

Add driver for Intel Power Management Controller (PMC) found on Sunrise Point PCH chipsets.
This device exposes S0ix sleep state residency counters and power management status.

Sysctls provided:

dev.intelpmc.0.slp_s0_residency_us - Time in deepest sleep (us)
dev.intelpmc.0.ltr_ignore          - LTR ignore mask
dev.intelpmc.0.pm_cfg              - PM configuration register
dev.intelpmc.0.pm_sts              - PM status register
dev.intelpmc.0.access_denied       - Firmware lock status

Supported devices for now:

• Sunrise Point-LP (0x9D21)
• Sunrise Point-H (0xA121)

Note: Later PCH generations (Cannon Lake, Tiger Lake, ect.) have different PMC register layouts according to the datasheet and would need per-generation tables.
I avoided adding untested hardware in case they have a quirk.

[adrian]

I'm going to land this today unless someone has any last minute objections!

[olce]

In D54881#1289937, @adrian wrote:

I'm going to land this today unless someone has any last minute objections!

Please let me review/test tomorrow and land it. I'm working on testing S0ix on Intel.

[ziaee]

Other than these nits, The manpage looks good to me. Maybe we should alphabetize the sysctls?

share/man/man4/intelpmc.4

1 ↗

(On Diff #175337)

This hyphen was for a long abandoned parser. I believe we have removed it from all our style guides, please let me know if this is not the case.

13–25 ↗

(On Diff #175337)

Optional: I proposed standardizing on this format on arch@ and in a BSDCan talk. We have nearly 200 manuals using this format, and it is more correct and consistent with the rest of the manual, as well as other BSDs, reinforcing the definition of SYNOPSIS to readers.

45 ↗

(On Diff #175337)

This section will appear on it's own in the hardware release note, so it is important that we add the context of what the list we're starting is.

[adrian]

oh one final comment - please put the code in sys/dev/intelpmc/, matching sys/modules/intelpmc/ .

If we end up with a lot of little intel drivers then we could relocate them all into sys/dev/intel/<driver>/, but I'm not so worried about that today.

Then fix ordering in modules/Makefile, then i'll land!

[adrian]

wait ignore me. There already IS stuff in sys/dev/intel/

Ok, ignore me. Lets just merge it.

[olce]

In D54881#1290039, @adrian wrote:

wait ignore me. There already IS stuff in sys/dev/intel/

Ok, ignore me. Lets just merge it.

Hello?

In D54881#1290030, @olce wrote:

Please let me review/test tomorrow and land it. I'm working on testing S0ix on Intel.

[adrian]

oh oops! ok sorry yes please test it. i missed that!

[seuros]

Address review feedback.

[seuros]

This comment was removed by seuros.

[ziaee]

share/man/man4/intelpmc.4

1–4 ↗

(On Diff #175392)

Please consider using the consistent style in all our style guides which is blank comment, copyright, blank comment, spdx, blank comment.

sys/dev/intel/intelpmc.c

1–5 ↗

(On Diff #175392)

copyright comes first, and no hyphen at the beginning.

[seuros]

Address review feedback

[seuros]

Fix man page: SPDX before copyright

[olce]

Sorry, a bit late, (hopefully final) review tomorrow. In the meantime, it seems that a couple of suggestions by ziaee@ are still to be applied (although, personally, I wouldn't move the SPDX tag; I've never understood why it's not systematically in the first line, where it is easy to find to know the license without reading the text; having it after the text but before other comments looks to me as an unnecessary and inconvenient complication).

Some notes related to this work (no action required/requested).

I spent some time last Friday on similar topics, and asked Abdelkader his ACPI dump as I wanted to see if we could get the S0ix residency time in a uniform manner with a single manner, and avoid having a driver per PCH generation as Linux does (Linux seems to support a lot more functionality than just S0ix-related stuff) just to have this information. Also checked on some machines myself. Conclusion is: We probably could but it would be somewhat ad-hoc as Intel's LPIT specification doesn't seem to be fully respected in practice, with no Entry Trigger field matching a register in _CST; however, the GAS for the residency counter, when of a valid type, rightfully points to the actual memory where the SLP_S0 register is mapped (and on the machines I examined there is only one such GAS even when there are multiple LPIT entries). Anyway, having a driver per PCH generation is not a bad idea per se, we'll see what useful functionality other than S0ix information it can bring.

Also while I agree that we tend to have a style of not having underscores in driver names, and intelpmc exists on OpenBSD, I think this convention is really bad and looks a lot like cargo-culting (as I really do not see any significant advantage of it). Incidentally, we already have hwpstate_intel(4) and hwpstate_amd(4), and I'd wish all future drivers to rather use that kind of convention, which makes names easy to read.

[ziaee]

In D54881#1290568, @olce wrote:

although, personally, I wouldn't move the SPDX tag; I've never understood why it's not systematically in the first line, where it is easy to find to know the license without reading the text; having it after the text but before other comments looks to me as an unnecessary and inconvenient complication).

This is not my idea, it's the recommendation of the style guide. (if it was my idea, no one would listen :P) -- https://docs.freebsd.org/en/articles/license-guide/

Also while I agree that we tend to have a style of not having underscores in driver names, and intelpmc exists on OpenBSD, I think this convention is really bad and looks a lot like cargo-culting (as I really do not see any significant advantage of it). Incidentally, we already have hwpstate_intel(4) and hwpstate_amd(4), and I'd wish all future drivers to rather use that kind of convention, which makes names easy to read.

For me, I just prefer we do whatever is most consistent. I like the traditional convention because its terse and consistent. I proposed several options, but I don't like using the exact name as the different linux driver, especially when it has an unnecessary and seemingly word at the end, and it's actively counterproductive in a google search because it doesn't use the same language as the datasheets, and produces entirely unrelated results except the confusingly similarly named linux driver.

[seuros]

@ziaee, the reason I put it on the second line was to keep it grepable. If you run head -n3 on driver files, you almost always land in the license header instead of anything useful, for example:
here and sys/fs/ext2fs/ext2_dir.h and many others.

Some files don't even have a license header at all, e.g. sys/arm64/arm64/bus_machdep.c.

PS: there are more files with the SPDX header , and my IDE show me warning because it don't follow the conventions most codebase had
coreboot
linux

The copyright line becomes problematic when there are multiple authors, since the license block keeps getting pushed further down.

The FreeBSD Project aims to produce a complete, BSD-licensed operating system allowing consumers of the system to produce derivative products without constraint or further license obligations.

A fork will inevitably change the structure once they start adding more contributors. Checking if someone altered the version, is not diffable.

---

I will update this diff and thermal once i get the naming is final.

[ziaee]

In D54881#1290624, @guest-seuros wrote:

@ziaee, the reason I put it on the second line was to keep it grepable. If you run head -n3 on driver files, you almost always land in the license header instead of anything useful, for example:
here and sys/fs/ext2fs/ext2_dir.h and many others.

Some files don't even have a license header at all, e.g. sys/arm64/arm64/bus_machdep.c.

PS: there are more files with the SPDX header , and my IDE show me warning because it don't follow the conventions most codebase had
coreboot
linux

The copyright line becomes problematic when there are multiple authors, since the license block keeps getting pushed further down.

The FreeBSD Project aims to produce a complete, BSD-licensed operating system allowing consumers of the system to produce derivative products without constraint or further license obligations.

A fork will inevitably change the structure once they start adding more contributors. Checking if someone altered the version, is not diffable.

---

I will update this diff and thermal once i get the naming is final.

Again, I did not come up with this and I am not giving legal advice, I am just pointing out what is in style.9 and the license guide. All our docs request to use this style. Here are some links for your convenience.

https://man.freebsd.org/cgi/man.cgi?query=style&apropos=0&sektion=0&manpath=FreeBSD+16.0-CURRENT&format=html

https://docs.freebsd.org/en/articles/license-guide/

[olce]

See inline comment, unless I'm somehow mistaken, register reading is wrong.

Also, what is the point of exposing PM_CFG? It does not seem to contain useful data with respect to S0ix, and in general not useful data at all as far as the OS is concerned. PM_STS is not even documented, and it's apparently used in Linux just to acknowledge messages sent to MTPMC (which we don't do), so I'd just remove it.

Finally, LTR_IGNORE is not documented either. It seems Linux only touches some bit there. It might be that some of the bits in there are of interest to know which S0ix are available, but am not sure. Do you have any documentation or pointers for that?

sys/dev/intel/intelpmc.c

130–148 ↗

(On Diff #175398)

This block, and in general, reads of the target registers look wrong.

Taking the example of SLP_S0_RES, according to the documentation, it is part of the "PMC Memory Mapped Registers" range, whose address is to be obtained by looking up the PWRMBASE value in the device's PCI configuration space. Here, IIUC, you're directly applying the SLP_S0_RES offset to the PCI configuration space.

[seuros]

.

[seuros]

remove sys/param.h accidentally added by IDE

[seuros]

rename to intelpmc for consistency with hwpmc/intelspi

[seuros]

for LTR_IGNORE.. each bit maps to an IP block (SPT has 18: SPA, SPB, SATA, ect).
A clear bit means that agent is asserting LTR requirements that can block S0ix entry.
When slp_s0_residency stops incrementing, the register tells you which device is in charge.

Linux exposes the same data in debugfs via pmc_core_ltr_show() for exactly this reason. I will add a comment in the source with the SPT bit map.

PS: I have other generations's support already tested. I will submit additional diffs once this driver land. (extra code used there was removed from this diff)

[olce]

In D54881#1294250, @guest-seuros wrote:

rename to intelpmc for consistency with hwpmc/intelspi

I'd like moving into the easier-to-read direction. Could you please rename it back to intel_pmc? That's also more consistent with acpi_spmc(4).

In D54881#1294254, @guest-seuros wrote:

for LTR_IGNORE.. each bit maps to an IP block (SPT has 18: SPA, SPB, SATA, ect).
A clear bit means that agent is asserting LTR requirements that can block S0ix entry.

From my reading of Linux code, not exactly. It rather means that the corresponding IP *can* block S0ix entry. Do you confirm?

When slp_s0_residency stops incrementing, the register tells you which device is in charge.

From my reading of Linux code, it doesn't do that. It's rather that, if you don't get into S0ix, you can try to ignore IPs that would prevent from doing that.

So it seems to me that exposing LTR_IGNORE doesn't really make sense without write support (you'll have to change the corresponding sysctl).

PS: I have other generations's support already tested. I will submit additional diffs once this driver land. (extra code used there was removed from this diff)

I'm eager to see that, but first of all, let's make sure this base part is completely correct and usable.

share/man/man4/Makefile

258

Leakage of unrelated code.

share/man/man4/intelpmc.4

64–66 ↗

(On Diff #175955)

Intel's documentation is less affirmative/detailed: SLP_S0# is asserted "when PCH is idle and processor is in C10 state", so I'd employ instead their exact terms, especially for the manual page.

sys/dev/intel/intelpmc.c

28–33 ↗

(On Diff #175955)

Where do all these come from? From my own research, you could reference Linux commit 9c2ee19987ef02fe3dbe507d81ff5c7dd5bb4f21.

But that is not enough: What does these mean? You could take inspiration from 2eb150558bb79ee01c39b64c2868216c0be2904f, at a minimum for documentation purposes.

Reading them carefully, you'll then discover that this list is likely slightly wrong (11 is reserved, the rest is shifted, and AGG is not an hardware component and ignoring it makes no sense).

60 ↗

(On Diff #175955)

The mask is likely wrong (which shouldn't have consequences according to the doc, since the BAR size should be 0). Please check it again.

[olce]

sys/dev/intel/intelpmc.c

149 ↗

(On Diff #175955)

How could this lockdown happen?

In the documentation, I see a "Power Management Data BAR Disable (PM_DATA_BAR_DIS)" bit in the documentation, with the indication that once locked reads to the MMIO area will always return 0 (and writes will be ignored). That seems at odds with testing for -1.

Testing whether both registers are 0 could be deceiving, as both registers could perfectly be 0. Perhaps we could test directly for the bit I've just mentioned. Alternative: Read another MMIO location which we know cannot contain 0 (unless the MMIO region is locked).

130–148 ↗

(On Diff #175398)

Access to SLP_S0 now looks OK.

Do not hesitate to mark the previous comment (and this one) as "Done" in Phabricator, that makes it easier to keep track of changes still to do.

[seuros]

Address review: rename to intel_pmc, SPDX first, LTR_IGNORE writable, fix bit assignments, update man page, Note: early tests were done in vendor bios, the current tests are done in coreboot

[ziaee]

The manual looks great to me. Please use Relnotes: yes since this is a new driver, and tag me when this is ready to test on Alder Lake!

[olce]

Sorry for the long delay before coming back to this. I'm now quite in a hurry to get this in, so that I can work on some other CPU/chipset generation. A comment a while ago indicated that you had more generations in stock, let's also examine that just after this one is done. I can help polishing and committing them if you do not have time to handle them.

A number of earlier inline comments, which for some reason were not added to the then current diff (probable pilot error on my side), have not been handled. I've added them back (except the mask one, where apparently I was wrong).

Please also see the new inline comments.

I do not really understand why you added "firmware access lock" detection. To my knowledge, after consulting the doc, there is no real lock mechanism for the PMC memory map, and no global firmware mechanism to read lock (there is the possibility for the BIOS to block reading 2 8-byte ranges, but this is designed to prevent reading password and other system sensitive info, and arguably it would be a bug if this used to block reading SLP_S0_RES or anything else exposed by the PMC). And there's nothing similar in Linux code either (contrary to what you stated in comment #1276647; AI hallucination?). Do you have any concrete evidence that this mechanism is really needed? Barring that, I'd just drop it.

Hopefully, after this new round, we will be there, or almost.

share/man/man4/intel_pmc.4

11

I'd remove "PCH". While this is what this initial driver is currently about, I intend intel_pmc(4) to become the umbrella for all generations (at least as many as we can/is useful/is practical), and later generations do not have a "PCH" anymore (the separate PCH is integrated into the CPU, and Intel has given up the "PCH" terminology). Also, the PMC acronym means "Performance Management Configuration", "Controller" is not part of it.

22–30

"expose (...) LTR block status" is too vague and misleading.

The rest looks good to describe what this driver currently does, but is too specific for what we would like intel_pmc(4) to become. So, before this paragraph, I'd add a more general one.

Some end result proposal:

The
.Nm
driver exposes information and controls from the Performance Monitoring Configuration (PMC) Controllers of Intel platforms.  Presently, it supports the Intel Sunrise Point platform (Skylake CPU generation), providing access to debug information and controls related to S0ix (Modern Standby).
.Pp
The cumulative time spent in low power mode while in S0 is reported, and the administrator can force the platform to ignore IP blocks whose Latency Tolerance Reporting (LTR) prevents the platform from entering S0ix.

32

38

59

59–62

I'll probably add an explicit warning about fiddling with these bits once this revision is ready and committed.

93

sys/dev/intel/intel_pmc.c

8–11

See comments for the manual page description. Text here to be kept in sync. In particular, "LTR block status" here is wrong.

13–15

Before this, please mention that only Sunrise Point PCH is currently supported (try to mimic the beginning of the manual page), and that all the information below covers that hardware only. Use the opportunity to introduce the SPT abbreviation.

17–23

Please cite the source here, as this is not exactly what the official documentation says, which is "When PCH is idle and processor is in C10 state, this pin
will assert to indicate VR controller can go into a light load mode.".

The only source I could find is b740d2e9233cb33626d3b62210bcfc6a34baa839 in the Linux repository. That's where the "in general" suggested addition comes from.

26

Since this is already approximate, given the time scale, what about rounding that to the simpler "5 days"?

67

LTR_IGNORE is not referenced in there. Could you please add a source? Suggestion: Linux's commit 9c2ee19987ef02fe3dbe50.

86–87

I can only find 0xa121 in Intel's Gen 6 doc, and 0x9d21 in Linux code (without any mention of 0xa121). Could you elaborate on the provenance of these, and in particular where the descriptions come from? Well, I'm also finding 0x9d21 in pci_vendors FWIW (mentioning that would be great, in absence of any other source).

159

Please replace by the most appropriate test: If bit 8 in register at offset 0x44 in the PCI config space is 0, then PWRMBASE is not configured, else just proceed. (Sorry that I missed that in the first review.)

[ziaee]

share/man/man4/intel_pmc.4

11

the PMC acronym means "Performance Management Configuration", "Controller" is not part of it.

In that case, it is an important detail that controller should begin with a lowercase c.

32

ditto.

38

Ditto.

59

ditto.

[olce]

share/man/man4/intel_pmc.4

11

Also, the PMC acronym means "Performance Management Configuration", "Controller" is not part of it.

Forget about it. I found "Performance Management Configuration" in some places, but those in the ToC and acronym explanations also indicate "Performance Management Controller". It looks like "PMC Controller" (also in the official doc) is at fault.

And thus the "C" of "Controller" should be uppercase.

11

In that case, it is an important detail that controller should begin with a lowercase c.

Why is that? Asking to check whether I'm missing something.

I don't have a strong opinion in general, but think we should follow as much as possible upstream terminology and typography, so that of the vendor when writing a driver (unless we have stronger reasons not to).

The capital "C" was proposed because this is what I had seen in the official doc (although a re-check seems to show the only occurrences are in titles, where such a word is supposed to always be capitalized). That looks irrelevant now anyway since I think we should stick with "Performance Management Configuration" as the meaning of PMC (see above).

Whatever way is chosen, consistency throughout the manual page and code comments is indeed paramount, and the same choice must be applied everywhere.

32

Please forget about this comment (mark it as done).

38

Please forget about this comment (mark it as done).

59

Please forget about this comment (mark it as done).

[ziaee]

share/man/man4/intel_pmc.4

11

Why is that? Asking to check whether I'm missing something.

Nd is not title case, so if we're spelling out PMC then that is title case, but the rest of Nd should not be.

we should follow as much as possible upstream terminology and typography,

+1

Whatever way is chosen, consistency throughout the manual page and code comments is indeed paramount, and the same choice must be applied everywhere.

+1

[olce]

share/man/man4/intel_pmc.4

94

[olce]

share/man/man4/intel_pmc.4

11

That looks irrelevant now anyway since I think we should stick with "Performance Management Configuration" as the meaning of PMC (see above).

Should read "Performance Management Controller", obviously...