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.

[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

266

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

12

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.

23–31

"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.

33

39

60

60–63

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

94

sys/dev/intel/intel_pmc.c

9–12

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

14–16

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.

18–24

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.

27

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

68

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

87–88

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).

160

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

12

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.

33

ditto.

39

Ditto.

60

ditto.

[olce]

share/man/man4/intel_pmc.4

12

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.

12

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.

33

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

39

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

60

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

[ziaee]

share/man/man4/intel_pmc.4

12

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

95

[olce]

share/man/man4/intel_pmc.4

12

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...

[seuros]

update/rebase

[olce]

Looks really great now! Thanks!

[ziaee]

share/man/man4/intel_pmc.4

14–21

Prose in synopsis is dead. So, I think the other sysctl is read only?

36

No, the driver does not support he entire PCH, right, just the PMC in it. So this should reflect that.

[olce]

share/man/man4/intel_pmc.4

14–21

So, I think the other sysctl is read only?

Yes, it is.

[seuros]

share/man/man4/intel_pmc.4

14–21

Let fix it in the second Diff D59596, there are more generation, each one will mutate this man page slightly.

[ziaee]

share/man/man4/intel_pmc.4

14–21

i dont like that, i prefer that we import according to our spec and then the commit that adds new stuff adds new stuff. its more clean and atomic

[olce]

I'd also advise to disclose the AI used (for anything) with an "Assisted by:" tag.

[olce]

share/man/man4/intel_pmc.4

14–21

(...) its more clean and atomic

👍, especially given it's a very small change to make.

[adrian]

approved; please re-check and fix any issues from ziaee before landing

[seuros]

remove prose

[ziaee]

You forgot to put this in the release notes, and omitted that I worked with you on this in the commit log. Both of these make my life harder. Please do not do this.

[seuros]

i wrote that the driver is still not complete there are many other generations to work on like D59596. < and this is not the last one.

A release note doesn't break the build. We can even add it in a separate commit.

Alternatively, i will refrain from writing any man page and you handle it. wdyt ?

[ziaee]

In D54881#1368942, @seuros wrote:

i wrote that the driver is still not complete there are many other generations to work on like D59596. < and this is not the last one.

A release note doesn't break the build. We can even add it in a separate commit.

Honestly, in terms of things that can go wrong, breaking the build is not that big of a deal.
You rushed and made a mistake. I am pointing it out so that you can take a little more time.
Please add it as you said. Had you done as I asked it would be a lot less work for you and cleaner, and I would not have to be here arguing with you about it.

Alternatively, i will refrain from writing any man page and you handle it. wdyt ?

I think this is a bit rude actually. To me this is reading like you don't want to do it anymore because I asked you to fix a few details, and even showed you how, and you wanted to rush or something. You are writing perfectly fine doc, I just gave you some small editorial corrections so that the doc follows project standards. I think if you do not want to fix these small things, you definitely do not want to spend the much greater amount of time to teach me what your driver is doing so that I can write it for you. Which is also a giant waste of my time honestly. All of the corrections I told you on this review, I told you on previous reviews. And, I wrote them in style guides, and I told you where they were. So, this is all a waste of time, and your proposal sounds like you think I am wasting your time, so you want to waste even more time. It makes me a bit sad.

[seuros]

Please add it as you said. Had you done as I asked it would be a lot less work for you and cleaner, and I would not have to be here arguing with you about it.

I will add it in the other diff.

I think this is a bit rude actually. To me this is reading like you don't want to do it anymore because I asked you to fix a few details, and even showed you how, and you wanted to rush or something. You are writing perfectly fine doc, I just gave you some small editorial corrections so that the doc follows project standards. I think if you do not want to fix these small things, you definitely do not want to spend the much greater amount of time to teach me what your driver is doing so that I can write it for you. Which is also a giant waste of my time honestly. All of the corrections I told you on this review, I told you on previous reviews. And, I wrote them in style guides, and I told you where they were. So, this is all a waste of time, and your proposal sounds like you think I am wasting your time, so you want to waste even more time. It makes me a bit sad.

That was not a statement, some people like to do things on their own. I myself push changes on people PRs in various of the project i maintain, especially when i want them to land exactly like i want.
If you want to arc patch locally , edit it and update it, i'm totally fine with it.

the apple_bce driver shipped without a manpage, and you and ngie authored it. So, i though that maybe i'm stepping on your tasks.
And no, I will spend time to explain you the driver, even give you ssh access or do video call with you if needed so we ship documentation correctly.
My issue is that this driver is super old Jan 26 2026 , i revisited so many time. Also there are 12 generations of on top.. which mean that the narrative and man page is going to change multiple time. The end users are going to read the last version, not the history.
Also for me writing man pages is harder than the driver itself, not because can't write. But because i don't know when to stop explaining things.
So my flow is that i copy a similar man page and edit it.

So i apologies if that sounded like an attack..

Anyway, this driver works continue on D59596, and there will be more. I will resume it later.

[olce]

In D54881#1368936, @ziaee wrote:

You forgot to put this in the release notes, and omitted that I worked with you on this in the commit log. Both of these make my life harder. Please do not do this.

@ziaee,

I'd like to point out first and foremost that the form of this message is counter-productive and for this reason alone should have been avoided. I'm almost certain it would have come across differently if you had said something like: "Release notes are missing, could you please add them? Also, please mention in future commit logs that I reviewed the manual page.". Same for "Had you done as I asked it would be a lot less work for you and cleaner, and I would not have to be here arguing with you about it.".

Additionally, personally, I find that the level of animosity conveyed is a bit out of proportion with the actual lapses here. This is my feeling, I'm not in your shoes, I certainly do not have the same context as you and YMMV, but let me explain a bit my point of view as I think it should resonate with you for the most part.

Trying to imagine myself in the shoes of Abdelkader (coming back to you in a moment), this revision was opened in January, was first reviewed almost immediately, but then there was no progress until April, where I did a more thorough review and found an important problem (the driver simply could not work); this was completed by another review in May, and then there was again a gap on my side until beginning of September. In general, and by contrast, Abdelkader responded quite swiftly to all reviews, and also had other work based on this one, which means any change here causes additional work since his stack has to be rebased. So, when I finally accepted the changes, he was probably relieved and eager to commit it, and then came your late intervention, with good points. Abdelkader seemed initially reluctant, and I guess this was because of the history, because the changes requested themselves were in reality tiny, and only the last one (changing the heading before the list of supported PMCs) could percolate through his stack (and even not that far I'd guess). So, you were right about asking for the changes, but also took his reluctance personally whereas it doesn't seem to me that it was because of you personally (again, only my evaluation), and then started to escalate, which could be viewed as heavy and unfair.

Let me add also that we do not necessarily add release notes right away when something new appears, not even manual pages. Of course, you could tell me that this is because of negligence, and you would probably be right most of the time, but I think there are also "legitimate" reasons to prioritize them down, such as when something is under continuous development, with a first version appearing in the tree and then multiple iterations happening on it with significant changes in functionality/support. For example, I (and ShengYi) have been "guilty" of that for the evolution of hwpstate_amd(4) to support CPPC: There's still no manual page text to describe it, and although that's certainly not ideal, it's not a big deal either because the functionality is only in main/-CURRENT and has not been backported, because the sysctl controls are thoroughly documented so it's at least usable even for people not too versed in the topic, and because there's still heavy work on-going behind the scenes on the hwpstate_*(4) drivers (actually, in this precise case, the old developers of hwpstate_amd(4) could also be seen as guilty since there did not write a manual page in 15+ years for the P-state control part, which is why there's not even a manual page to look at still to this day, but again that's probably not a huge deal since that support integrates completely into cpufreq(4), which *has* a manual page). The aim here is just to save on developer *but also* doc reviewer time when we know that a lot of further changes are going to happen. The participating time of all those involved in FreeBSD is precious, for a lot of different reasons, and everybody should keep that in mind at all times, sometimes taking it on themselves to avoid frictions as much as possible (but then, nobody's perfect, mistakes happen, some do a lot of efforts and others do not do enough, and so on and so forth).

From your point of view, I can understand how it feels when trying to move documentation forward and these efforts being met with a lot of resistance and frictions (although I hope that at least a few of them produced actual value). You also pointed out some similar corrections in other reviews with Abdelkader, and are getting tired over that. You're actually saying "all corrections" which looks a bit like an overstatement (I have not personally checked) and possibly is a consequence of you being fed up with repeating the same things in lots of reviews with which Abdelkader has nothing to do. In this context, it's not hard to understand that you can perceive being reluctant to do some small corrections as a low appreciation for the work you are doing. So let me say that we are actually quite blessed that you're actively curating our manual pages (and perhaps other documentation, for all I know), this work is really important to improve our adoption in various audiences. And we are blessed to have Abdelkader too, with that attitude towards hardware support among others. Both code and documentation are important. That said, sometimes, it will happen that code is done first and documentation done later, for lots of reasons (e.g., in addition to the reasons developed above, some developers are terrible writers, etc.) without implying that documentation is less important, or treated as a second-class (or third-class, or worse) citizen. I don't dispute it probably is not treated as seriously as it should in too many cases, it's just I don't think this was the case here, despite some appearances, hence my assessment on your animosity.

[ziaee]

There is no spite or resentment behind my frustrated comment, please do not read into it too deeply. I did take it personally, but I do not agree there was anything escalatory about communicating that, especially after being directly asked what I think. I did not insult anyone, because I do not have any insults to issue. My statement was just, direct. It does make my life harder, and I asked please do not do this. My concern with the doc, as you know from doing release notes, is not that it will take more time later, it is that it will never get done. If it is done on time, it takes everyone an extra 10 minutes. If it is done later, it takes many man hours, starting with a frustrated user report (and who knows how many frustrated users did not bother to file the bug first), many hours of work to figure out what is missing, and so on. Archaeological docs is also the most unglamorous work that could never feed a family, and the damage to our reputation is done when things slip through the cracks and users find them. Similarly with release notes. We can not manually read through the commit log and parse everything that has happened to write release notes, and AI is not at that stage either. If the commit is marked appropriately we can do an acceptable job with only a few hours of work. If not, it takes days, which may or may not happen depending on availability of our teammates.

[olce]

In D54881#1372525, @ziaee wrote:

There is no spite or resentment behind my frustrated comment, please do not read into it too deeply.

Ok, relieved to read that.

I did take it personally, but I do not agree there was anything escalatory about communicating that, especially after being directly asked what I think.

Not questioning that part, I was really talking about earlier reactions.

My concern with the doc, as you know from doing release notes, is not that it will take more time later, it is that it will never get done.

I completely agree in general (and, as you know, I experienced it first hand), but again, I don't think it will be the case here. And I'll guarantee that myself as a last resort.

If the commit is marked appropriately we can do an acceptable job with only a few hours of work. If not, it takes days, which may or may not happen depending on availability of our teammates.

Sure, that commit should have been marked. But we'll have other opportunities, i.e., follow-up commits, to correct that, and anyway we'll update RELNOTES after these follow-ups.