Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42677 )
Change subject: soc/intel/tigerlake: Run pmc_set_acpi_mode() during .init in pmc_ops ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42677/5/src/soc/intel/tigerlake/pmc... File src/soc/intel/tigerlake/pmc.c:
https://review.coreboot.org/c/coreboot/+/42677/5/src/soc/intel/tigerlake/pmc... PS5, Line 146: PMC initialization happens earlier for this SoC
This PMC device is intentionally hide to avoid PCI enumeration. […]
Subrata, I should have added you to the reviews for https://review.coreboot.org/41384, sorry about that. The PMC device is now declared 'hidden' in the devicetree, which means that it is not enumerated, but rather assumed to exist as described and *not* probed. This allows us to do 2 things: 1) publish the resources under the PMC device (https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src...) 2) SSDT generation (https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src...)
William, sure, here's what I would write:
pmc_set_acpi_mode() should be delayed until BS_DEV_INIT in order to ensure the ordering does not break the assumptions that other drivers make about ACPI mode (e.g. Chrome EC). Since it disables ACPI mode, other drivers may take different actions based on this (e.g. Chrome EC will flush any pending hostevent bits). Because TGL has its PMC device available for device_operations, it can be done from the "ops->init" callback.
https://review.coreboot.org/c/coreboot/+/42677/5/src/soc/intel/tigerlake/pmc... PS5, Line 158: P.S.: This cannot be done as part of pmc_soc_init as PMC device is : * hidden and hence the PMC driver never gets enumerated and so init is : * not called for it.
Is this really correct?
suggested rewrite above.