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

[Marek Maślanka]

Hi Thomas

On Tue, Jul 30, 2024 at 6:08 PM Thomas Gleixner <tglx@xxxxxxxxxxxxx> 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.