Details

Reviewers

adrian
obiwac
olce

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.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 70462
Build 67345: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

guest-seuros requested review of this revision.Jan 26 2026, 1:54 AM

adrian added a reviewer: obiwac.Jan 26 2026, 1:58 AM

Cool that someone is working on an intel PMC driver!

I don't have time today to do a more in depth review, but here's a couple small comments already. I'll review this more fully this evening or tomorrow

sys/dev/intel/intel_pmc_core.c
2–16	The SPDX identifier should be the first line of the copyright header, and maybe make the driver description a separate comment to the copyright header. Also if you want to you could add your name as such: * This software was developed by Abdelkader Boudih <guest-seuros@freebsd.org> * under sponsorship from the FreeBSD Foundation.
92–103	a lot of this code could be factored out into a function which takes in `oidp`, `req`, and an offset, esp as we add sysctls for a bunch of the other registers.
175–178	I haven't read the docs yet so this might be a dumb question, but what is this register counting exactly? in theory a system using S0ix for suspend is always in S0. Or is this counting how much time the system is residing in _any_ S0ix state? perhaps the description could be updated if so.

obiwac added inline comments.Jan 26 2026, 1:46 PM

sys/dev/intel/intel_pmc_core.c
2–16	sorry, wrongly assumed you were working for the foundation. You can just put your name straight in the copyright notice then: /* * SPDX-License-Identifier: BSD-2-Clause * * Copyright (c) 2026 Abdelkader Boudih <your@email.com> */
70–72
164–166

Add files.x86 and modules entries; align slp_s0_residency sysctl name

Herald added a subscriber: imp. · View Herald TranscriptFeb 4 2026, 2:48 AM

Harbormaster completed remote builds in B70387: Diff 171122.Feb 4 2026, 2:48 AM

obiwac added inline comments.Feb 6 2026, 12:38 AM

sys/conf/files.x86
66	can you order this alphabetically?
sys/dev/intel/intel_pmc_core.c
5–7	perhaps best as separate comment
18–27	please order these alphabetically
39	`SPT_PMC_SLP_S0_RES_COUNTER_STEP` on linux (`drivers/platform/x86/intel/pmc/core.h`) sets this to 0x68, which is actually 104us. This is contrary to what 4.3.53 says, so this is likely a bug in linux. Just noting this here.
53–54	formatting issues here but fixable on commit. Also, do you have a device which matches 8086:a121?
136	i feel like some of these names could be a little less cryptic, esp `ltr_ign`
148	might as well just use `SPT_PMC_SLP_S0_RES_STEP` directly and remove the `slp_s0_step` member no? we'll have to refactor this anyways if we support future Intel PMCs.
159–160	can you point to docs which indicate that a failing read (indicating no S0ix) will return 0xFFFFFFFF?
202	this can never happen
sys/modules/Makefile
317	plz order alphabetically here too

Addressed feedback:

SPDX first line, split copyright/description
Alphabetized includes and build files
Updated counter to 104us (empirical/Linux)
Refactored with pmc_read_safe() helper
Documented S0ix scope
Removed slp_s0_step member

Cannot test 0x8086:a121 without hardware yet. Will add more once i get them,

Harbormaster completed remote builds in B70460: Diff 171314.Feb 6 2026, 3:30 AM

Reverting to datasheet value (100us). Linux's 104us likely quirk on untested vendors.

Harbormaster completed remote builds in B70461: Diff 171317.Feb 6 2026, 3:42 AM

intel/intel_pmc_core: Remove unnecessary NULL check in detach

Harbormaster completed remote builds in B70462: Diff 171318.Feb 6 2026, 3:46 AM

Add man page, fix SPDX header order

Herald added a subscriber: ziaee. · View Herald TranscriptFeb 25 2026, 2:07 AM

Harbormaster completed remote builds in B70973: Diff 172638.Feb 25 2026, 2:07 AM

guest-seuros mentioned this in D55495: intel_pmc_core: Add Intel PMC Core driver.Feb 25 2026, 2:08 AM

Factor reg32 sysctls into single handler, fix SLP_S0 description with reference

Harbormaster completed remote builds in B70974: Diff 172639.Feb 25 2026, 2:14 AM

guest-seuros added inline comments.Mar 11 2026, 3:19 AM

sys/dev/intel/intel_pmc_core.c
159–160	0xFFFFFFFF is the standard PCIe "Unsupported Request" completion response, when the PMC MMIO BAR is locked or inaccessible (firmware disabled S0ix), the root complex returns all-ones for any read. I did read it from https://docs.kernel.org/PCI/pci-error-recovery.html Linux do the same : https://raw.githubusercontent.com/torvalds/linux/refs/heads/master/drivers/platform/x86/intel/pmc/core.c

olce added a reviewer: olce.Thu, Apr 9, 7:02 PM

Hi Abdelkader,

Could you provide the output of acpidump -dt on your test machine? If that's too big for here or your prefer not to display it publicly, could you sent it to me (at olce@)?

Thanks.

The output is 125kb!

To avoid spamming everybody, i did send you it by email as requested.

Fell free to post relevant parts for others.

I know linux calls it a PMC core, but searching the web shows the rest of the world, including intel datasheets, does not use the term core. Searching the web for intel pmc core only shows unrelated things from intel, and the linux driver.

Does it make sense to maybe name it maybe something shorter and more descriptive like intel_pch_pmc? A more traditional BSD driver name might be pchpmc. Also, searching the web for pch pmc actually shows relevant datasheets from intel.

Is this driver different from what linux is doing? e.g. would it only confuse people looking for external resources if it had the same name?

OpenBSD calls their driver intelpmc. NetBSD doesn't appear to have one. I really like when it doesn't have underscores, most BSD drivers don't.

In D54881#1289335, @ziaee wrote:

OpenBSD calls their driver intelpmc. NetBSD doesn't appear to have one. I really like when it doesn't have underscores, most BSD drivers don't.

Fair! intelpmc then ?

I have more parts for this driver (i added new generations), but i want to have this landed before i diff the others.

Rename intel_pmc_core to intelpmc (no underscores, matches OpenBSD). Prefix static functions with intelpmc_. Update man page and build files.

Harbormaster completed remote builds in B72150: Diff 175336.Sun, Apr 12, 10:47 AM

Fix man page: .Dt INTELPMC, drop 'Core' from description

Harbormaster completed remote builds in B72151: Diff 175337.Sun, Apr 12, 10:49 AM

guest-seuros retitled this revision from intel/intel_pmc_core: Add Intel PMC Core driver to intel/intelpmc: Add Intel PMC Core driver.Sun, Apr 12, 12:07 PM

guest-seuros edited the summary of this revision. (Show Details)

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

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.

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 accepted this revision.Sun, Apr 12, 7:02 PM

This revision is now accepted and ready to land.Sun, Apr 12, 7:02 PM

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!

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

Ok, ignore me. Lets just merge it.

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.

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

Address review feedback.

This revision now requires review to proceed.Mon, Apr 13, 1:01 AM

Harbormaster completed remote builds in B72162: Diff 175391.Mon, Apr 13, 1:01 AM

guest-seuros updated this revision to Diff 175392.Mon, Apr 13, 1:04 AM

This comment was removed by guest-seuros.

Harbormaster completed remote builds in B72163: Diff 175392.Mon, Apr 13, 1:04 AM

guest-seuros marked 3 inline comments as done.Mon, Apr 13, 1:05 AM

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.

Address review feedback

Harbormaster completed remote builds in B72167: Diff 175397.Mon, Apr 13, 2:05 AM

Fix man page: SPDX before copyright

Harbormaster completed remote builds in B72168: Diff 175398.Mon, Apr 13, 2:07 AM

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.

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.

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

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/

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.

This revision now requires changes to proceed.Tue, Apr 14, 8:35 PM

guest-seuros mentioned this in D54882: intelthermal: Add Intel Processor Thermal driver.Thu, Apr 16, 11:37 AM

intel/intelpmc: Add Intel PMC Core driver
Needs RevisionPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 171318

sys/conf/files.x86

sys/dev/intel/intel_pmc_core.c

sys/modules/Makefile

sys/modules/intel_pmc_core/Makefile

intel/intelpmc: Add Intel PMC Core driverNeeds RevisionPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 171318

sys/conf/files.x86

sys/dev/intel/intel_pmc_core.c

sys/modules/Makefile

sys/modules/intel_pmc_core/Makefile

intel/intelpmc: Add Intel PMC Core driver
Needs RevisionPublic
Actions

Revision Contents
Changeset List