Re: [PATCH v2] platform/x86:intel/pmc: Enable the ACPI PM Timer to be turned off when suspended

["David E. Box" <david.e.box@linux.intel.com>]

Hi Marek. Thanks for the patch.

On Wed, 2024-07-03 at 12:30 -0400, Rajneesh Bhardwaj wrote:
> On Wed, Jul 3, 2024 at 7:39 AM Marek Maslanka <mmaslanka@google.com> wrote:
> > 
> > Allow to disable ACPI PM Timer on suspend and enable on resume. A
> > disabled timer helps optimise power consumption when the system is
> > suspended. On resume the timer is only reactivated if it was activated
> > prior to suspend, so unless the ACPI PM timer is enabled in the BIOS,
> > this won't change anything.
> 
> Back in the days IIRC, it was frowned upon but I am not sure anymore.
> Maybe Rafael or David will have some opinion on this change. Is this
> something that could be done in a platform specific manner such as in
> coreboot?

I discussed with Rafael. This is generally a good idea, but need to ensure that
the ACPI PM Timer isn't being used as a clock source. This could mess with the
timekeeping system. Also, maybe a better idea is to disable it altogether at
probe time if it's not being used as a clock source. This should only be the
case when TSC is unstable and HPET is unavailable, but need to confirm.

David

> 
> > 
> > Signed-off-by: Marek Maslanka <mmaslanka@google.com>
> > ---
> >  drivers/platform/x86/intel/pmc/adl.c  |  2 ++
> >  drivers/platform/x86/intel/pmc/cnp.c  |  2 ++
> >  drivers/platform/x86/intel/pmc/core.c | 37 +++++++++++++++++++++++++++
> >  drivers/platform/x86/intel/pmc/core.h |  8 ++++++
> >  drivers/platform/x86/intel/pmc/icl.c  |  2 ++
> >  drivers/platform/x86/intel/pmc/mtl.c  |  2 ++
> >  drivers/platform/x86/intel/pmc/spt.c  |  2 ++
> >  drivers/platform/x86/intel/pmc/tgl.c  |  2 ++
> >  8 files changed, 57 insertions(+)
> > 
> > diff --git a/drivers/platform/x86/intel/pmc/adl.c
> > b/drivers/platform/x86/intel/pmc/adl.c
> > index e7878558fd909..9d9c07f44ff61 100644
> > --- a/drivers/platform/x86/intel/pmc/adl.c
> > +++ b/drivers/platform/x86/intel/pmc/adl.c
> > @@ -295,6 +295,8 @@ const struct pmc_reg_map adl_reg_map = {
> >         .ppfear_buckets = CNP_PPFEAR_NUM_ENTRIES,
> >         .pm_cfg_offset = CNP_PMC_PM_CFG_OFFSET,
> >         .pm_read_disable_bit = CNP_PMC_READ_DISABLE_BIT,
> > +       .acpi_pm_tmr_ctl_offset = SPT_PMC_ACPI_PM_TMR_CTL_OFFSET,
> > +       .acpi_pm_tmr_disable_bit = SPT_PMC_BIT_ACPI_PM_TMR_DISABLE,
> >         .ltr_ignore_max = ADL_NUM_IP_IGN_ALLOWED,
> >         .lpm_num_modes = ADL_LPM_NUM_MODES,
> >         .lpm_num_maps = ADL_LPM_NUM_MAPS,
> > diff --git a/drivers/platform/x86/intel/pmc/cnp.c
> > b/drivers/platform/x86/intel/pmc/cnp.c
> > index dd72974bf71e2..513c02670c5aa 100644
> > --- a/drivers/platform/x86/intel/pmc/cnp.c
> > +++ b/drivers/platform/x86/intel/pmc/cnp.c
> > @@ -200,6 +200,8 @@ const struct pmc_reg_map cnp_reg_map = {
> >         .ppfear_buckets = CNP_PPFEAR_NUM_ENTRIES,
> >         .pm_cfg_offset = CNP_PMC_PM_CFG_OFFSET,
> >         .pm_read_disable_bit = CNP_PMC_READ_DISABLE_BIT,
> > +       .acpi_pm_tmr_ctl_offset = SPT_PMC_ACPI_PM_TMR_CTL_OFFSET,
> > +       .acpi_pm_tmr_disable_bit = SPT_PMC_BIT_ACPI_PM_TMR_DISABLE,
> >         .ltr_ignore_max = CNP_NUM_IP_IGN_ALLOWED,
> >         .etr3_offset = ETR3_OFFSET,
> >  };
> > diff --git a/drivers/platform/x86/intel/pmc/core.c
> > b/drivers/platform/x86/intel/pmc/core.c
> > index 10c96c1a850af..e97ac7a8a18bc 100644
> > --- a/drivers/platform/x86/intel/pmc/core.c
> > +++ b/drivers/platform/x86/intel/pmc/core.c
> > @@ -1171,6 +1171,35 @@ static bool pmc_core_is_pson_residency_enabled(struct
> > pmc_dev *pmcdev)
> >         return val == 1;
> >  }
> > 
> > +/*
> > + * Enable or disable APCI PM Timer
> > + *
> > + * @return: Previous APCI PM Timer enabled state
> > + */
> > +static bool pmc_core_enable_apci_pm_timer(struct pmc_dev *pmcdev, bool
> > enable)
> > +{
> > +       struct pmc *pmc = pmcdev->pmcs[PMC_IDX_MAIN];
> > +       const struct pmc_reg_map *map = pmc->map;
> > +       bool state;
> > +       u32 reg;
> > +
> > +       if (!map->acpi_pm_tmr_ctl_offset)
> > +               return false;
> > +
> > +       mutex_lock(&pmcdev->lock);
> > +
> > +       reg = pmc_core_reg_read(pmc, map->acpi_pm_tmr_ctl_offset);
> > +       state = !(reg & map->acpi_pm_tmr_disable_bit);
> > +       if (enable)
> > +               reg &= ~map->acpi_pm_tmr_disable_bit;
> > +       else
> > +               reg |= map->acpi_pm_tmr_disable_bit;
> > +       pmc_core_reg_write(pmc, map->acpi_pm_tmr_ctl_offset, reg);
> > +
> > +       mutex_unlock(&pmcdev->lock);
> > +
> > +       return state;
> > +}
> > 
> >  static void pmc_core_dbgfs_unregister(struct pmc_dev *pmcdev)
> >  {
> > @@ -1446,6 +1475,10 @@ static __maybe_unused int pmc_core_suspend(struct
> > device *dev)
> >         if (pmcdev->suspend)
> >                 pmcdev->suspend(pmcdev);
> > 
> > +       /* Disable APCI PM Timer */
> > +       pmcdev->enable_acpi_pm_timer_on_resume =
> > +               pmc_core_enable_apci_pm_timer(pmcdev, false);
> > +
> >         /* Check if the syspend will actually use S0ix */
> >         if (pm_suspend_via_firmware())
> >                 return 0;
> > @@ -1500,6 +1533,10 @@ int pmc_core_resume_common(struct pmc_dev *pmcdev)
> >         int offset = pmc->map->lpm_status_offset;
> >         int i;
> > 
> > +       /* Enable APCI PM Timer */
> > +       if (pmcdev->enable_acpi_pm_timer_on_resume)
> > +               pmc_core_enable_apci_pm_timer(pmcdev, true);
> > +
> >         /* Check if the syspend used S0ix */
> >         if (pm_suspend_via_firmware())
> >                 return 0;
> > diff --git a/drivers/platform/x86/intel/pmc/core.h
> > b/drivers/platform/x86/intel/pmc/core.h
> > index 83504c49a0e31..fe1a94f693b63 100644
> > --- a/drivers/platform/x86/intel/pmc/core.h
> > +++ b/drivers/platform/x86/intel/pmc/core.h
> > @@ -67,6 +67,8 @@ struct telem_endpoint;
> >  #define SPT_PMC_LTR_SCC                                0x3A0
> >  #define SPT_PMC_LTR_ISH                                0x3A4
> > 
> > +#define SPT_PMC_ACPI_PM_TMR_CTL_OFFSET         0x18FC
> > +
> >  /* Sunrise Point: PGD PFET Enable Ack Status Registers */
> >  enum ppfear_regs {
> >         SPT_PMC_XRAM_PPFEAR0A = 0x590,
> > @@ -147,6 +149,8 @@ enum ppfear_regs {
> >  #define SPT_PMC_VRIC1_SLPS0LVEN                        BIT(13)
> >  #define SPT_PMC_VRIC1_XTALSDQDIS               BIT(22)
> > 
> > +#define SPT_PMC_BIT_ACPI_PM_TMR_DISABLE                BIT(1)
> > +
> >  /* Cannonlake Power Management Controller register offsets */
> >  #define CNP_PMC_SLPS0_DBG_OFFSET               0x10B4
> >  #define CNP_PMC_PM_CFG_OFFSET                  0x1818
> > @@ -344,6 +348,8 @@ struct pmc_reg_map {
> >         const u8  *lpm_reg_index;
> >         const u32 pson_residency_offset;
> >         const u32 pson_residency_counter_step;
> > +       const u32 acpi_pm_tmr_ctl_offset;
> > +       const u32 acpi_pm_tmr_disable_bit;
> >  };
> > 
> >  /**
> > @@ -417,6 +423,8 @@ struct pmc_dev {
> >         u32 die_c6_offset;
> >         struct telem_endpoint *punit_ep;
> >         struct pmc_info *regmap_list;
> > +
> > +       bool enable_acpi_pm_timer_on_resume;
> >  };
> > 
> >  enum pmc_index {
> > diff --git a/drivers/platform/x86/intel/pmc/icl.c
> > b/drivers/platform/x86/intel/pmc/icl.c
> > index 71b0fd6cb7d84..cbbd440544688 100644
> > --- a/drivers/platform/x86/intel/pmc/icl.c
> > +++ b/drivers/platform/x86/intel/pmc/icl.c
> > @@ -46,6 +46,8 @@ const struct pmc_reg_map icl_reg_map = {
> >         .ppfear_buckets = ICL_PPFEAR_NUM_ENTRIES,
> >         .pm_cfg_offset = CNP_PMC_PM_CFG_OFFSET,
> >         .pm_read_disable_bit = CNP_PMC_READ_DISABLE_BIT,
> > +       .acpi_pm_tmr_ctl_offset = SPT_PMC_ACPI_PM_TMR_CTL_OFFSET,
> > +       .acpi_pm_tmr_disable_bit = SPT_PMC_BIT_ACPI_PM_TMR_DISABLE,
> >         .ltr_ignore_max = ICL_NUM_IP_IGN_ALLOWED,
> >         .etr3_offset = ETR3_OFFSET,
> >  };
> > diff --git a/drivers/platform/x86/intel/pmc/mtl.c
> > b/drivers/platform/x86/intel/pmc/mtl.c
> > index c7d15d864039d..91f2fa728f5c8 100644
> > --- a/drivers/platform/x86/intel/pmc/mtl.c
> > +++ b/drivers/platform/x86/intel/pmc/mtl.c
> > @@ -462,6 +462,8 @@ const struct pmc_reg_map mtl_socm_reg_map = {
> >         .ppfear_buckets = MTL_SOCM_PPFEAR_NUM_ENTRIES,
> >         .pm_cfg_offset = CNP_PMC_PM_CFG_OFFSET,
> >         .pm_read_disable_bit = CNP_PMC_READ_DISABLE_BIT,
> > +       .acpi_pm_tmr_ctl_offset = SPT_PMC_ACPI_PM_TMR_CTL_OFFSET,
> > +       .acpi_pm_tmr_disable_bit = SPT_PMC_BIT_ACPI_PM_TMR_DISABLE,
> >         .lpm_num_maps = ADL_LPM_NUM_MAPS,
> >         .ltr_ignore_max = MTL_SOCM_NUM_IP_IGN_ALLOWED,
> >         .lpm_res_counter_step_x2 = TGL_PMC_LPM_RES_COUNTER_STEP_X2,
> > diff --git a/drivers/platform/x86/intel/pmc/spt.c
> > b/drivers/platform/x86/intel/pmc/spt.c
> > index ab993a69e33ee..2cd2b3c68e468 100644
> > --- a/drivers/platform/x86/intel/pmc/spt.c
> > +++ b/drivers/platform/x86/intel/pmc/spt.c
> > @@ -130,6 +130,8 @@ const struct pmc_reg_map spt_reg_map = {
> >         .ppfear_buckets = SPT_PPFEAR_NUM_ENTRIES,
> >         .pm_cfg_offset = SPT_PMC_PM_CFG_OFFSET,
> >         .pm_read_disable_bit = SPT_PMC_READ_DISABLE_BIT,
> > +       .acpi_pm_tmr_ctl_offset = SPT_PMC_ACPI_PM_TMR_CTL_OFFSET,
> > +       .acpi_pm_tmr_disable_bit = SPT_PMC_BIT_ACPI_PM_TMR_DISABLE,
> >         .ltr_ignore_max = SPT_NUM_IP_IGN_ALLOWED,
> >         .pm_vric1_offset = SPT_PMC_VRIC1_OFFSET,
> >  };
> > diff --git a/drivers/platform/x86/intel/pmc/tgl.c
> > b/drivers/platform/x86/intel/pmc/tgl.c
> > index e0580de180773..371b4e30f1426 100644
> > --- a/drivers/platform/x86/intel/pmc/tgl.c
> > +++ b/drivers/platform/x86/intel/pmc/tgl.c
> > @@ -197,6 +197,8 @@ const struct pmc_reg_map tgl_reg_map = {
> >         .ppfear_buckets = ICL_PPFEAR_NUM_ENTRIES,
> >         .pm_cfg_offset = CNP_PMC_PM_CFG_OFFSET,
> >         .pm_read_disable_bit = CNP_PMC_READ_DISABLE_BIT,
> > +       .acpi_pm_tmr_ctl_offset = SPT_PMC_ACPI_PM_TMR_CTL_OFFSET,
> > +       .acpi_pm_tmr_disable_bit = SPT_PMC_BIT_ACPI_PM_TMR_DISABLE,
> >         .ltr_ignore_max = TGL_NUM_IP_IGN_ALLOWED,
> >         .lpm_num_maps = TGL_LPM_NUM_MAPS,
> >         .lpm_res_counter_step_x2 = TGL_PMC_LPM_RES_COUNTER_STEP_X2,
> > --
> > 2.45.2.803.g4e1b14247a-goog
> > 
> 
>