| Message ID | 20240730120546.1042515-1-mmaslanka@google.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v3] platform/x86:intel/pmc: Enable the ACPI PM Timer to be turned off when suspended | expand |
On Tue, 30 Jul 2024, Marek Maslanka 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. > > Signed-off-by: Marek Maslanka <mmaslanka@google.com> > > --- > Changes in v3: > - Add the clocksource_current_cs_name and clocksource_suspend_cs_name to the clocksource.c > - Do not disable the ACPI PM timer if it's selected as the clocksource > - Link to v2: https://lore.kernel.org/lkml/20240703113850.2726539-1-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 | 47 +++++++++++++++++++++++++++ > 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 ++ > include/linux/clocksource.h | 2 ++ > kernel/time/clocksource.c | 22 +++++++++++++ > 10 files changed, 91 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..1a435f5ca08c5 100644 > --- a/drivers/platform/x86/intel/pmc/core.c > +++ b/drivers/platform/x86/intel/pmc/core.c > @@ -12,6 +12,7 @@ > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > #include <linux/bitfield.h> > +#include <linux/clocksource.h> > #include <linux/debugfs.h> > #include <linux/delay.h> > #include <linux/dmi.h> > @@ -1171,6 +1172,44 @@ 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; > + char cs_name[32]; > + bool state; > + u32 reg; > + > + if (!map->acpi_pm_tmr_ctl_offset) > + return false; > + > + clocksource_current_cs_name(cs_name, sizeof(cs_name)); > + if (strncmp(cs_name, "acpi_pm", sizeof(cs_name)) == 0) If you want to do a prefix check, there's str_has_prefix() for that. Otherwise, I don't understand why you need the n variant? > + return false; > + > + clocksource_suspend_cs_name(cs_name, sizeof(cs_name)); > + if (strncmp(cs_name, "acpi_pm", sizeof(cs_name)) == 0) > + 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 +1485,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 +1543,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, > diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h > index 0ad8b550bb4b4..f1953c5687683 100644 > --- a/include/linux/clocksource.h > +++ b/include/linux/clocksource.h > @@ -305,5 +305,7 @@ static inline unsigned int clocksource_get_max_watchdog_retry(void) > } > > void clocksource_verify_percpu(struct clocksource *cs); > +void clocksource_current_cs_name(char *buf, size_t buf_size); > +void clocksource_suspend_cs_name(char *buf, size_t buf_size); > > #endif /* _LINUX_CLOCKSOURCE_H */ > diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c > index e5b260aa0e02c..e2e2609f7f4b2 100644 > --- a/kernel/time/clocksource.c > +++ b/kernel/time/clocksource.c > @@ -1320,6 +1320,28 @@ int clocksource_unregister(struct clocksource *cs) > } > EXPORT_SYMBOL(clocksource_unregister); > > +void clocksource_suspend_cs_name(char *buf, size_t buf_size) > +{ > + mutex_lock(&clocksource_mutex); > + > + buf[0] = '\0'; > + if (suspend_clocksource) > + strscpy(buf, suspend_clocksource->name, buf_size); > + > + mutex_unlock(&clocksource_mutex); > +} > + > +void clocksource_current_cs_name(char *buf, size_t buf_size) > +{ > + mutex_lock(&clocksource_mutex); > + > + buf[0] = '\0'; > + if (curr_clocksource) > + strscpy(buf, curr_clocksource->name, buf_size); > + > + mutex_unlock(&clocksource_mutex); > +} Have you tested allmodconfig build? These functions are not exported so I'd expect it to fail... One could use guard() for the mutex.
Marek! On Tue, Jul 30 2024 at 12:05, Marek Maslanka 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. > > include/linux/clocksource.h | 2 ++ > kernel/time/clocksource.c | 22 +++++++++++++ The changelog is completely silent about the core code change. That's not how it works. Add the core code change as a separate patch with a proper justification and not hide it in the pile of the PMC changes without cc'ing the relevant maintainers. It's documented how this works, no? > +/* > + * 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; > + char cs_name[32]; > + bool state; > + u32 reg; > + > + if (!map->acpi_pm_tmr_ctl_offset) > + return false; > + > + clocksource_current_cs_name(cs_name, sizeof(cs_name)); > + if (strncmp(cs_name, "acpi_pm", sizeof(cs_name)) == 0) > + return false; > + > + clocksource_suspend_cs_name(cs_name, sizeof(cs_name)); > + if (strncmp(cs_name, "acpi_pm", sizeof(cs_name)) == 0) > + return false; How would ACPI/PM ever be selected as a suspend clocksource? It's not marked CLOCK_SOURCE_SUSPEND_NONSTOP. There is a reason why clocksources have suspend/resume and enable/disable callbacks. The latter allow you to turn it completely off when it is not in use. Something like the below should work. It's uncompiled, but you get the idea. Thanks, tglx --- --- a/drivers/clocksource/acpi_pm.c +++ b/drivers/clocksource/acpi_pm.c @@ -63,12 +63,40 @@ static u64 acpi_pm_read(struct clocksour return (u64)read_pmtmr(); } +static bool acpi_pm_enabled; + +static void (*enable_callback)(bool enable); + +bool acpi_pm_register_enable_callback(void (*cb)(bool enable)) +{ + enable_callback = cb; + if (cb) + cb(acpi_pm_enabled); +} + +static int acpi_pm_enable(struct clocksource *cs) +{ + acpi_pm_enabled = true; + if (enable_callback) + enable_callback(true); + return 0; +} + +static void acpi_pm_disable(struct clocksource *cs) +{ + acpi_pm_enabled = false; + if (enable_callback) + enable_callback(false); +} + static struct clocksource clocksource_acpi_pm = { .name = "acpi_pm", .rating = 200, .read = acpi_pm_read, .mask = (u64)ACPI_PM_MASK, .flags = CLOCK_SOURCE_IS_CONTINUOUS, + .enable = acpi_pm_enable, + .disable = acpi_pm_disable, };
Hi Thomas On Tue, Jul 30, 2024 at 6:08 PM Thomas Gleixner <tglx@linutronix.de> wrote: > > Marek! > > On Tue, Jul 30 2024 at 12:05, Marek Maslanka 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. > > > > include/linux/clocksource.h | 2 ++ > > kernel/time/clocksource.c | 22 +++++++++++++ > > The changelog is completely silent about the core code change. That's > not how it works. > > Add the core code change as a separate patch with a proper justification > and not hide it in the pile of the PMC changes without cc'ing the > relevant maintainers. It's documented how this works, no? Ok > > > +/* > > + * 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; > > + char cs_name[32]; > > + bool state; > > + u32 reg; > > + > > + if (!map->acpi_pm_tmr_ctl_offset) > > + return false; > > + > > + clocksource_current_cs_name(cs_name, sizeof(cs_name)); > > + if (strncmp(cs_name, "acpi_pm", sizeof(cs_name)) == 0) > > + return false; > > + > > + clocksource_suspend_cs_name(cs_name, sizeof(cs_name)); > > + if (strncmp(cs_name, "acpi_pm", sizeof(cs_name)) == 0) > > + return false; > > How would ACPI/PM ever be selected as a suspend clocksource? It's not > marked CLOCK_SOURCE_SUSPEND_NONSTOP. > > There is a reason why clocksources have suspend/resume and > enable/disable callbacks. The latter allow you to turn it completely off > when it is not in use. > > Something like the below should work. It's uncompiled, but you get the > idea. > > Thanks, > > tglx > --- > --- a/drivers/clocksource/acpi_pm.c > +++ b/drivers/clocksource/acpi_pm.c > @@ -63,12 +63,40 @@ static u64 acpi_pm_read(struct clocksour > return (u64)read_pmtmr(); > } > > +static bool acpi_pm_enabled; > + > +static void (*enable_callback)(bool enable); > + > +bool acpi_pm_register_enable_callback(void (*cb)(bool enable)) > +{ > + enable_callback = cb; > + if (cb) > + cb(acpi_pm_enabled); > +} > + > +static int acpi_pm_enable(struct clocksource *cs) > +{ > + acpi_pm_enabled = true; > + if (enable_callback) > + enable_callback(true); > + return 0; > +} > + > +static void acpi_pm_disable(struct clocksource *cs) > +{ > + acpi_pm_enabled = false; > + if (enable_callback) > + enable_callback(false); > +} > + > static struct clocksource clocksource_acpi_pm = { > .name = "acpi_pm", > .rating = 200, > .read = acpi_pm_read, > .mask = (u64)ACPI_PM_MASK, > .flags = CLOCK_SOURCE_IS_CONTINUOUS, > + .enable = acpi_pm_enable, > + .disable = acpi_pm_disable, > }; > Thanks. I'll try do this in that way. But I need to disable/enable ACPI PM timer only on suspend/resume, so I'll use suspend/resume callbacks.
Marek! On Wed, Jul 31 2024 at 16:44, Marek Maślanka wrote: > On Tue, Jul 30, 2024 at 6:08 PM Thomas Gleixner <tglx@linutronix.de> wrote: >> On Tue, Jul 30 2024 at 12:05, Marek Maslanka wrote: >> +static void acpi_pm_disable(struct clocksource *cs) >> +{ >> + acpi_pm_enabled = false; >> + if (enable_callback) >> + enable_callback(false); >> +} >> + >> static struct clocksource clocksource_acpi_pm = { >> .name = "acpi_pm", >> .rating = 200, >> .read = acpi_pm_read, >> .mask = (u64)ACPI_PM_MASK, >> .flags = CLOCK_SOURCE_IS_CONTINUOUS, >> + .enable = acpi_pm_enable, >> + .disable = acpi_pm_disable, >> }; >> > Thanks. I'll try do this in that way. But I need to disable/enable > ACPI PM timer only on suspend/resume, so I'll use suspend/resume > callbacks. Why? What's the point of keeping it running when nothing uses it? Thanks, tglx
On Wed, Jul 31, 2024 at 6:33 PM Thomas Gleixner <tglx@linutronix.de> wrote: > > Marek! > > On Wed, Jul 31 2024 at 16:44, Marek Maślanka wrote: > > On Tue, Jul 30, 2024 at 6:08 PM Thomas Gleixner <tglx@linutronix.de> wrote: > >> On Tue, Jul 30 2024 at 12:05, Marek Maslanka wrote: > >> +static void acpi_pm_disable(struct clocksource *cs) > >> +{ > >> + acpi_pm_enabled = false; > >> + if (enable_callback) > >> + enable_callback(false); > >> +} > >> + > >> static struct clocksource clocksource_acpi_pm = { > >> .name = "acpi_pm", > >> .rating = 200, > >> .read = acpi_pm_read, > >> .mask = (u64)ACPI_PM_MASK, > >> .flags = CLOCK_SOURCE_IS_CONTINUOUS, > >> + .enable = acpi_pm_enable, > >> + .disable = acpi_pm_disable, > >> }; > >> > > Thanks. I'll try do this in that way. But I need to disable/enable > > ACPI PM timer only on suspend/resume, so I'll use suspend/resume > > callbacks. > > Why? What's the point of keeping it running when nothing uses it? > > Thanks, > > tglx In case of Intel CPUs the watchdog (iTCO/wdat_wdt) is driven by ACPI PM Timer. But it may also be used by others that I don't know about, so I don't want to disable it. Best Marek
On Wed, Jul 31 2024 at 23:41, Marek Maślanka wrote: > On Wed, Jul 31, 2024 at 6:33 PM Thomas Gleixner <tglx@linutronix.de> wrote: >> On Wed, Jul 31 2024 at 16:44, Marek Maślanka wrote: >> > Thanks. I'll try do this in that way. But I need to disable/enable >> > ACPI PM timer only on suspend/resume, so I'll use suspend/resume >> > callbacks. >> >> Why? What's the point of keeping it running when nothing uses it? >> > In case of Intel CPUs the watchdog (iTCO/wdat_wdt) is driven by ACPI PM > Timer. But it may also be used by others that I don't know about, so I don't > want to disable it. Fair enough.
On Wed, 31 Jul 2024, Marek Maślanka wrote: > On Wed, Jul 31, 2024 at 6:33 PM Thomas Gleixner <tglx@linutronix.de> wrote: > > > > Marek! > > > > On Wed, Jul 31 2024 at 16:44, Marek Maślanka wrote: > > > On Tue, Jul 30, 2024 at 6:08 PM Thomas Gleixner <tglx@linutronix.de> wrote: > > >> On Tue, Jul 30 2024 at 12:05, Marek Maslanka wrote: > > >> +static void acpi_pm_disable(struct clocksource *cs) > > >> +{ > > >> + acpi_pm_enabled = false; > > >> + if (enable_callback) > > >> + enable_callback(false); > > >> +} > > >> + > > >> static struct clocksource clocksource_acpi_pm = { > > >> .name = "acpi_pm", > > >> .rating = 200, > > >> .read = acpi_pm_read, > > >> .mask = (u64)ACPI_PM_MASK, > > >> .flags = CLOCK_SOURCE_IS_CONTINUOUS, > > >> + .enable = acpi_pm_enable, > > >> + .disable = acpi_pm_disable, > > >> }; > > >> > > > Thanks. I'll try do this in that way. But I need to disable/enable > > > ACPI PM timer only on suspend/resume, so I'll use suspend/resume > > > callbacks. > > > > Why? What's the point of keeping it running when nothing uses it? > > > > Thanks, > > > > tglx > > In case of Intel CPUs the watchdog (iTCO/wdat_wdt) is driven by ACPI PM > Timer. But it may also be used by others that I don't know about, so I don't > want to disable it. Hi Marek, This kind of non-obvious information should be put into the changelog because it helps if after ten years somebody is looking into this change and asks similar why questions.
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..1a435f5ca08c5 100644 --- a/drivers/platform/x86/intel/pmc/core.c +++ b/drivers/platform/x86/intel/pmc/core.c @@ -12,6 +12,7 @@ #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt #include <linux/bitfield.h> +#include <linux/clocksource.h> #include <linux/debugfs.h> #include <linux/delay.h> #include <linux/dmi.h> @@ -1171,6 +1172,44 @@ 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; + char cs_name[32]; + bool state; + u32 reg; + + if (!map->acpi_pm_tmr_ctl_offset) + return false; + + clocksource_current_cs_name(cs_name, sizeof(cs_name)); + if (strncmp(cs_name, "acpi_pm", sizeof(cs_name)) == 0) + return false; + + clocksource_suspend_cs_name(cs_name, sizeof(cs_name)); + if (strncmp(cs_name, "acpi_pm", sizeof(cs_name)) == 0) + 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 +1485,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 +1543,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, diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h index 0ad8b550bb4b4..f1953c5687683 100644 --- a/include/linux/clocksource.h +++ b/include/linux/clocksource.h @@ -305,5 +305,7 @@ static inline unsigned int clocksource_get_max_watchdog_retry(void) } void clocksource_verify_percpu(struct clocksource *cs); +void clocksource_current_cs_name(char *buf, size_t buf_size); +void clocksource_suspend_cs_name(char *buf, size_t buf_size); #endif /* _LINUX_CLOCKSOURCE_H */ diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c index e5b260aa0e02c..e2e2609f7f4b2 100644 --- a/kernel/time/clocksource.c +++ b/kernel/time/clocksource.c @@ -1320,6 +1320,28 @@ int clocksource_unregister(struct clocksource *cs) } EXPORT_SYMBOL(clocksource_unregister); +void clocksource_suspend_cs_name(char *buf, size_t buf_size) +{ + mutex_lock(&clocksource_mutex); + + buf[0] = '\0'; + if (suspend_clocksource) + strscpy(buf, suspend_clocksource->name, buf_size); + + mutex_unlock(&clocksource_mutex); +} + +void clocksource_current_cs_name(char *buf, size_t buf_size) +{ + mutex_lock(&clocksource_mutex); + + buf[0] = '\0'; + if (curr_clocksource) + strscpy(buf, curr_clocksource->name, buf_size); + + mutex_unlock(&clocksource_mutex); +} + #ifdef CONFIG_SYSFS /** * current_clocksource_show - sysfs interface for current clocksource
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. Signed-off-by: Marek Maslanka <mmaslanka@google.com> --- Changes in v3: - Add the clocksource_current_cs_name and clocksource_suspend_cs_name to the clocksource.c - Do not disable the ACPI PM timer if it's selected as the clocksource - Link to v2: https://lore.kernel.org/lkml/20240703113850.2726539-1-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 | 47 +++++++++++++++++++++++++++ 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 ++ include/linux/clocksource.h | 2 ++ kernel/time/clocksource.c | 22 +++++++++++++ 10 files changed, 91 insertions(+)