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.
