Match the platform by CPU model and attach on acpi with PWRM at
0xfe000000; the PMC is not PCI-enumerable on these platforms.
SLP_S0_RES offsets and granularities follow each generation's PCH
Family Datasheet Volume 2. A mismatch between the LPIT residency
counter address and SLP_S0_RES is reported on attach.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
manpage changes looks good. i suppose i can test this now that it supports alder lake.
| share/man/man4/intel_pmc.4 | ||
|---|---|---|
| 13–14 | ||
Comment Actions
Yes! Your Alder lake should attach with this diff.
@dteske also has AL from Framework.
| share/man/man4/intel_pmc.4 | ||
|---|---|---|
| 13–14 | this diff need a rebase, that verbose code don't exist in the Diff that was merged. | |
This comment was removed by seuros.
Comment Actions
Generally, you have to test if the driver is already attached. Here, the driver could attach through PCI or through ACPI, but it shouldn't attach through both. This is not supposed to happen, but better be safe than sorry, especially when dealing with hardware. So please add a check to both probe methods that device_get_unit(dev) is 0, else bail out (because there's already one driver/device). Check on acpi_get_handle(dev) still looks like a good idea, even if it does not handle the redundancy anymore.
I'll do an extra pass tomorrow morning.
(As I said in my latest mail, I wanted to see if we could do something cleaner and forward-proof. The majority of this day was spent for this, and unfortunately, it does not appear we can do better. Intel PCH documentation appears to be quite flawed with respect to the PMC: There's no PCI device on one of my laptops (Comet Lake) although there should be according to various datasheets, including those for the chipset that should be in that mobile processor (AFAICT after some deep online research). For future generations, we'll want to infer PWRMBASE from the Shared SRAM device. I checked that there's indeed such a device even on Comet Lake, and dumped the content of the BAR areas, without finding the 0xfe000000 base; that one still can be deduced from the LPIT table. The datasheet descriptions of the BARs appear to be wrong (but have been corrected in later generations).)
| share/man/man4/intel_pmc.4 | ||
|---|---|---|
| 23 | ||
| 71 | long-uptime systems looks too vague. I'd specify the order of magnitude. | |
| sys/dev/intel/intel_pmc.c | ||
| 22–26 | Suggested style changes. Also, Linux has code to switch to the Coffee Lake registers if the Kaby Lake CPU ID is found but the 0x9d21 PCI device is not, so the PMC is probably not PCI-enumerable on a Coffee Lake machine too (if you have such a machine and time and will to check). | |
| 243–252 | I can imagine why you're doing that. I'll later propose some complement to it to avoid problems in virtual machines. | |
| 326–384 | This is a kind of layer violation, parsing of LPIT should go into ACPI proper. But that's fine for now, I'll refactor this later. | |
| 416–420 | So, here, you're giving priority to INTEL_PMC_PWRM_BASE_DEFAULT (0xfe000000), whereas Linux uses this as a last resort if there's a residency counter address from the LPIT. Is that intended? If you prefer to keep it like that, the check should probably consider all valid LPI entries before deciding there's a mismatch. | |
Comment Actions
Note: Ideally, at some point, we should add some way to print the IP blocks per generation (separate change/revision).
| sys/dev/intel/intel_pmc.c | ||
|---|---|---|
| 181 | I guess you don't have the hardware for this one? | |
Comment Actions
I tested this on real hardware, with the exception of a few -* variants, but Linux handles them the same way. (I can also get that hardware in the next days.)
There are more generations described by either Linux or the specs, but I omitted them due to lack of hardware for testing.
The specialised platforms are out of scope except Bartlett Lake which i bought for the RAM not as a daily driver.
| share/man/man4/intel_pmc.4 | ||
|---|---|---|
| 23 | ack | |
| 71 | ack | |
| sys/dev/intel/intel_pmc.c | ||
| 181 | Bartlett Lake ? i do for now. But maybe we should remove it, the motherboard is not generic (DFI). wdyt ? | |
| 416–420 | Yes. That was only way to work on every generation listed here. We could have it like linux way as a fallback( there is a small hang in unsupported platform), or i will could try to find proper way auto detect correct way. I have no preference in driver work. | |
| sys/dev/intel/intel_pmc.c | ||
|---|---|---|
| 181 | Mmm, I'd say, let's keep it, especially if you have the hardware. If someone uses FreeBSD on this one, they might be pleasantly surprised, or in the worst case it crashes and hopefully we will hear about it. It looks like a niche anyway. | |
| 416–420 | Just to check I'm understanding correctly, you're saying that there's at least one generation that needs our static PWRMBASE default in priority over the LPIT content, else it does not work? What is the "small hang" you experienced, and in which context? Not sure how to auto-detect if INTEL_PMC_PWRM_BASE_DEFAULT and the LPIT table are not enough. Do you have an idea in mind? | |
Comment Actions
Ping? If you need more info or more generally some help from me, don't hesitate to say so.
Comment Actions
Nothing needed on your side, thanks. It just needs a retest on hardware, which I will do this weekend.
I have been busy with NVMe lately, and most of the drives are in 2-3 machines.
Intel_PMC has to be tested across the whole fleet before I can give you a correct answer.
I don't want to assume anything, given what happened with the Qotom board, which turned out to be the only one that needed a quirk.
I wish there is was an ack button in pharb.
Comment Actions
okay i am testing this now and nothing exploded. or well i thought i was but
$ sysctl dev.intel_pmc sysctl: unknown oid 'dev.intel_pmc'
let me build all this stuff again sorry.
| share/man/man4/intel_pmc.4 | ||
|---|---|---|
| 19 | this is great improvement | |
| 48–49 | the new inconsistency is one of many reasons i do not recommend pciids in the hardware section. how is this sorted now? can you add that other stuff or can we remove them from these? | |
| 71 | +1, but better if we can make it even more terse. people specifically say they want terse docs instead of llm long docs. is it really "platform dependent" since this is only applies to amd64 and specifically only intel? Also, we should show what is going on to the reader first, then explain why if at all. Example, "The counter will wrap after N.N days because it is 100us/32bit". | |
| share/man/man4/intel_pmc.4 | ||
|---|---|---|
| 71 | Yes, it depends on the CPU/chipset generation. Terse docs do not mean not having intelligible sentences. "The counter will wrap after N.N days because it is 100us/32bit.": A counter "is" not "100us", that does not make any sense.
Exactly, and that's why "This value wraps after a few days" comes first in the proposal. A more terse version of the proposal above: | |
Comment Actions
Ping? If you do not have time to work on it in the very short term, I'd like to take it over as I need to make progress and implement newer generations here.
Comment Actions
Address concerns. Retested in all the fleet.
Sorry for delay @olce. My fleet is back online so we can resume testing.
When you will augment this, i can rerun it or just give you ssh (whatever easier for you).
Comment Actions
Are you really sure about the change for Alder Lake? Looking at Linux sources, I see #define TGL_PMC_SLP_S0_RES_COUNTER_STEP 0x7A. I also see #define TGL_PMC_LPM_RES_COUNTER_STEP_X2 61 /* 30.5us * 2 */, so 30500 looks like a confusion. If that is indeed the case, then reverting to milliseconds instead of microseconds is probably better.
The rest looks okay (as per our previous discussions). One more suggestion in an inline comment.
| sys/dev/intel/intel_pmc.c | ||
|---|---|---|
| 363–366 | Not practically needed for now, but logically I'd put the write to *first first, so that it is always filled as long as returned value is not ENOENT. | |
| share/man/man4/intel_pmc.4 | ||
|---|---|---|
| 26–27 | Suggestion to match the new intel_pmc_lpit_match() behavior. In any case, the original "window" should be replaced with "base". | |
Comment Actions
So I found pmc_core_adjust_slp_s0_step() in Linux... Goodness... Seems you're right to do that, even if I do not fully understand the comment there (they use ADL_LPM_RESIDENCY_OFFSET (0x17a4) for LPM residency, while for S0 they use 0x1098).
If that is indeed the case, then reverting to milliseconds instead of microseconds is probably better.
I meant "microseconds", not milliseconds, but anyway if keeping 30500 this becomes obsolete.
The rest looks okay (as per our previous discussions). One more suggestion in an inline comment.
So seems it's ready modulo a few minor stuff (added another inline comment).
Comment Actions
I'm going to land this shortly, i'm just need to rebuild it on top of main and retry it on all the fleet.
Comment Actions
Thanks!
No problem.
When you will augment this, i can rerun it or just give you ssh (whatever easier for you).
I'm interested, yes! I'll ping you back.