Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <1652275016-13423-1-git-send-email-quic_mkshah@quicinc.com> <1652275016-13423-5-git-send-email-quic_mkshah@quicinc.com>
In-Reply-To: <1652275016-13423-5-git-send-email-quic_mkshah@quicinc.com>
From:   Ulf Hansson <ulf.hansson@linaro.org>
Date:   Thu, 19 May 2022 14:56:44 +0200
Message-ID: <CAPDyKFp+38XR9osX91quAMG6CbLYaAcetgPqFM53rinfT4ar+Q@mail.gmail.com>
Subject: Re: [PATCH v2 4/6] PM: domains: Store the closest hrtimer event of
 the domain CPUs
To:     Maulik Shah <quic_mkshah@quicinc.com>
Cc:     bjorn.andersson@linaro.org, linux-arm-msm@vger.kernel.org,
        linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org,
        rafael@kernel.org, daniel.lezcano@linaro.org,
        quic_lsrao@quicinc.com, quic_rjendra@quicinc.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, 11 May 2022 at 15:17, Maulik Shah <quic_mkshah@quicinc.com> wrote:
>
> The arch timer can not wake up the Qualcomm Technologies, Inc. (QTI)
> SoCs when the deepest CPUidle modes results in the SoC also to enter
> the low power mode.
>
> RSC is part of CPU subsystem and APSS rsc device is attached to cluster
> power domain. RSC has to setup next hrtimer wakeup in CONTROL_TCS which
> can wakeup the SoC from deepest low power states. The CONTROL_TCS does
> this by writing next wakeup in always on domain timer when the SoC is
> entering the low power state.
>
> Add dev_pm_genpd_get_next_hrtimer() to get the genpd wakeup time.
>
> Signed-off-by: Maulik Shah <quic_mkshah@quicinc.com>
> ---
>  drivers/base/power/domain.c          | 24 ++++++++++++++++++++++++
>  drivers/base/power/domain_governor.c |  1 +
>  include/linux/pm_domain.h            |  7 +++++++
>  3 files changed, 32 insertions(+)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 18cd796..f0d70d0 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -487,6 +487,29 @@ void dev_pm_genpd_set_next_wakeup(struct device *dev, ktime_t next)
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_genpd_set_next_wakeup);
>
> +/**
> + * dev_pm_genpd_get_next_hrtimer - Return genpd domain next_hrtimer.
> + *
> + * @dev: Device to handle
> + *
> + * Returns the aggregated domain wakeup time for CPU PM domain
> + * when all the subdomains are off.

To further clarify when this function should be used, I think that we
should state that it should typically be called from a consumer of a
genpd on/off-notifier at GENPD_NOTIFY_PRE_OFF. This also means that
the genpd's lock is being held across the function.

> + */
> +ktime_t dev_pm_genpd_get_next_hrtimer(struct device *dev)
> +{
> +       struct generic_pm_domain *genpd;
> +
> +       genpd = dev_to_genpd_safe(dev);
> +       if (!genpd)
> +               return KTIME_MAX;
> +
> +       if (atomic_read(&genpd->sd_count) > 0)
> +               return KTIME_MAX;

This above isn't needed, assuming we clarify the description of the
function and when it should be called.

> +
> +       return genpd->next_hrtimer;
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_genpd_get_next_hrtimer);
> +
>  static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
>  {
>         unsigned int state_idx = genpd->state_idx;
> @@ -1998,6 +2021,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
>         genpd->max_off_time_changed = true;
>         genpd->provider = NULL;
>         genpd->has_provider = false;
> +       genpd->next_hrtimer = KTIME_MAX;
>         genpd->accounting_time = ktime_get_mono_fast_ns();
>         genpd->domain.ops.runtime_suspend = genpd_runtime_suspend;
>         genpd->domain.ops.runtime_resume = genpd_runtime_resume;
> diff --git a/drivers/base/power/domain_governor.c b/drivers/base/power/domain_governor.c
> index cd08c58..a4c7dd8 100644
> --- a/drivers/base/power/domain_governor.c
> +++ b/drivers/base/power/domain_governor.c
> @@ -363,6 +363,7 @@ static bool cpu_power_down_ok(struct dev_pm_domain *pd)
>                                 domain_wakeup = next_hrtimer;
>                 }
>         }
> +       genpd->next_hrtimer = domain_wakeup;

There should be no point to set this, unless cpu_power_down_ok() are
returning true. Therefore I suggest you move this a few lines further
down, where cpu_power_down_ok() actually returns true.

>
>         /* The minimum idle duration is from now - until the next wakeup. */
>         idle_duration_ns = ktime_to_ns(ktime_sub(domain_wakeup, now));
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 043d48e..6d9fb79 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -17,6 +17,7 @@
>  #include <linux/notifier.h>
>  #include <linux/spinlock.h>
>  #include <linux/cpumask.h>
> +#include <linux/time64.h>
>
>  /*
>   * Flags to control the behaviour of a genpd.
> @@ -136,6 +137,7 @@ struct generic_pm_domain {
>         struct gpd_dev_ops dev_ops;
>         s64 max_off_time_ns;    /* Maximum allowed "suspended" time. */
>         ktime_t next_wakeup;    /* Maintained by the domain governor */
> +       ktime_t next_hrtimer;   /* Next hrtimer for the CPU PM domain */
>         bool max_off_time_changed;
>         bool cached_power_down_ok;
>         bool cached_power_down_state_idx;
> @@ -228,6 +230,7 @@ int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state);
>  int dev_pm_genpd_add_notifier(struct device *dev, struct notifier_block *nb);
>  int dev_pm_genpd_remove_notifier(struct device *dev);
>  void dev_pm_genpd_set_next_wakeup(struct device *dev, ktime_t next);
> +ktime_t dev_pm_genpd_get_next_hrtimer(struct device *dev);
>
>  extern struct dev_power_governor simple_qos_governor;
>  extern struct dev_power_governor pm_domain_always_on_gov;
> @@ -289,6 +292,10 @@ static inline int dev_pm_genpd_remove_notifier(struct device *dev)
>  static inline void dev_pm_genpd_set_next_wakeup(struct device *dev, ktime_t next)
>  { }
>
> +static inline ktime_t dev_pm_genpd_get_next_hrtimer(struct device *dev)
> +{
> +       return KTIME_MAX;
> +}
>  #define simple_qos_governor            (*(struct dev_power_governor *)(NULL))
>  #define pm_domain_always_on_gov                (*(struct dev_power_governor *)(NULL))
>  #endif
> --
> 2.7.4
>

Kind regards
Uffe