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.

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?

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!

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/

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)

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.