Re: [PATCH] platform/x86/intel/pmc: Disable ACPI PM timer during suspend on Panther Lake

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Thu, 15 Oct 2026, Tzung-Bi Shih wrote:

> The ACPI PM timer is used to drive the ACPI Watchdog Action Table
> (wdat_wdt) hardware watchdog.  When the ACPI PM timer is enabled by
> firmware on Panther Lake, leaving it running during s2idle prevents the
> SoC from entering its deepest S0i2.2 substate (staying in S0i2.1).
> 
> Since the watchdog is stopped during system suspend, the ACPI PM timer
> does not need to remain active while suspended.  Disable the timer via
> ACPI_TMR_CTL (PWRMBASE + 0x18fc) in ptl_suspend() and re-enable it in
> ptl_resume() only if it was active prior to suspend.
> 
> Unlike commit e86c8186d03a ("platform/x86:intel/pmc: Enable the ACPI PM
> Timer to be turned off when suspended"), which was reverted in commit
> 5fa607880168 because it acquired pmcdev->lock from the acpi_pm
> clocksource suspend callback inside tick_freeze() atomic context, toggle
> the register in ptl_suspend() and ptl_resume() without taking
> pmcdev->lock.
> 
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
> ---
>  drivers/platform/x86/intel/pmc/core.h |  2 ++
>  drivers/platform/x86/intel/pmc/ptl.c  | 35 ++++++++++++++++++++++++++-
>  2 files changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/intel/pmc/core.h b/drivers/platform/x86/intel/pmc/core.h
> index e55164e5807f..a5bd45c4b44e 100644
> --- a/drivers/platform/x86/intel/pmc/core.h
> +++ b/drivers/platform/x86/intel/pmc/core.h
> @@ -487,6 +487,7 @@ struct pmc {
>   * @s0ix_counter:	S0ix residency (step adjusted)
>   * @suspend:		Function to perform platform specific suspend
>   * @resume:		Function to perform platform specific resume
> + * @pm_timer_enabled:	Whether the ACPI PM timer enabled before suspend
>   *
>   * @pkgc_ltr_blocker_counters: Array of PKGC LTR blocker counters
>   * @pkgc_ltr_blocker_offset: Offset to PKGC LTR blockers in telemetry region
> @@ -506,6 +507,7 @@ struct pmc_dev {
>  	u64 s0ix_counter;
>  	void (*suspend)(struct pmc_dev *pmcdev);
>  	int (*resume)(struct pmc_dev *pmcdev);
> +	bool pm_timer_enabled;
>  
>  	u64 *pkgc_res_cnt;
>  	u8 num_of_pkgc;
> diff --git a/drivers/platform/x86/intel/pmc/ptl.c b/drivers/platform/x86/intel/pmc/ptl.c
> index a7d5d4f77934..8c32f9be9302 100644
> --- a/drivers/platform/x86/intel/pmc/ptl.c
> +++ b/drivers/platform/x86/intel/pmc/ptl.c
> @@ -6,6 +6,7 @@
>   * Copyright (c) 2025, Intel Corporation.
>   */
>  
> +#include <linux/io.h>
>  #include <linux/pci.h>
>  
>  #include "core.h"
> @@ -548,6 +549,9 @@ static const u8 ptl_pmc_list[] = {PMC_IDX_MAIN};
>  #define PTL_NPU_PCI_DEV                0xb03e
>  #define PTL_IPU_PCI_DEV                0xb05d
>  
> +#define ACPI_TMR_CTL_OFFSET            0x18fc
> +#define ACPI_TIM_DIS                   BIT(1)

Please add include for this as well.

> +
>  /*
>   * Set power state of select devices that do not have drivers to D3
>   * so that they do not block Package C entry.
> @@ -558,9 +562,38 @@ static void ptl_d3_fixup(void)
>  	pmc_core_set_device_d3(PTL_NPU_PCI_DEV);
>  }
>  
> +static void ptl_set_acpi_pm_timer_on_suspend(struct pmc_dev *pmcdev, bool suspend)
> +{
> +	struct pmc *pmc = pmcdev->pmcs[PMC_IDX_MAIN];
> +	bool enabled;
> +	u8 val;
> +
> +	if (!pmc || !pmc->regbase)
> +		return;
> +
> +	if (!suspend && !pmcdev->pm_timer_enabled)
> +		return;
> +
> +	val = readb(pmc->regbase + ACPI_TMR_CTL_OFFSET);
> +	enabled = !(val & ACPI_TIM_DIS);
> +	if (suspend && enabled)
> +		writeb(val | ACPI_TIM_DIS, pmc->regbase + ACPI_TMR_CTL_OFFSET);
> +	else if (!suspend)
> +		writeb(val & ~ACPI_TIM_DIS, pmc->regbase + ACPI_TMR_CTL_OFFSET);

Change val on a separate line for clarity.

How about using this alternative:

	old_val = val;
	if (...)
		val |= ACPI_TIM_DIS;
	else if (...)
		val &= ~ACPI_TIM_DIS;
	if (val != old_val)
		writeb(...);

?

> +
> +	pmcdev->pm_timer_enabled = suspend && enabled;
> +}
> +
> +static void ptl_suspend(struct pmc_dev *pmcdev)
> +{
> +	ptl_set_acpi_pm_timer_on_suspend(pmcdev, true);
> +	cnl_suspend(pmcdev);
> +}
> +
>  static int ptl_resume(struct pmc_dev *pmcdev)
>  {
>  	ptl_d3_fixup();
> +	ptl_set_acpi_pm_timer_on_suspend(pmcdev, false);
>  	return cnl_resume(pmcdev);
>  }
>  
> @@ -576,7 +609,7 @@ struct pmc_dev_info ptl_pmc_dev = {
>  	.regmap_list = ptl_pmc_info_list,
>  	.map = &ptl_pcdp_reg_map,
>  	.sub_req_show = &pmc_core_substate_blk_req_fops,
> -	.suspend = cnl_suspend,
> +	.suspend = ptl_suspend,
>  	.resume = ptl_resume,
>  	.init = ptl_core_init,
>  	.sub_req = pmc_core_pmt_get_blk_sub_req,
> 

-- 
 i.