Re: [PATCH v6 1/3] PM / Domains: add domain feature flag for next wakeup

[Lina Iyer <ilina@codeaurora.org>]

On Tue, Dec 22 2020 at 06:07 -0700, Ulf Hansson wrote:
>On Mon, 30 Nov 2020 at 23:51, Lina Iyer <ilina@codeaurora.org> wrote:
>>
>> PM domains may support entering multiple power down states when the
>> component devices and sub-domains are suspended. Also, they may specify
>> the residency value for an idle state, only after which the idle state
>> may provide power benefits. If the domain does not specify the residency
>> for any of its idle states, the governor's choice is much simplified.
>>
>> Let's make this optional with the use of a PM domain feature flag.
>>
>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>> ---
>> Changes in v6:
>>         - New patch based on discussions on v5 of the series
>> ---
>>  drivers/base/power/domain.c | 18 ++++++++++++++++++
>>  include/linux/pm_domain.h   | 28 ++++++++++++++++++++++------
>>  2 files changed, 40 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index 1b0c9ccbbe1f..1e6c0bf1c945 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -1748,6 +1748,24 @@ int dev_pm_genpd_remove_notifier(struct device *dev)
>>  }
>>  EXPORT_SYMBOL_GPL(dev_pm_genpd_remove_notifier);
>>
>> +/**
>> + * genpd_enable_next_wakeup - Enable genpd gov to use next_wakeup
>> + *
>> + * @genpd: The genpd to be updated
>> + * @enable: Enable/disable genpd gov to use next wakeup
>> + */
>> +void genpd_enable_next_wakeup(struct generic_pm_domain *genpd, bool enable)
>> +{
>> +       genpd_lock(genpd);
>> +       if (enable)
>> +               genpd->flags |= GENPD_FLAG_GOV_NEXT_WAKEUP;
>> +       else
>> +               genpd->flags &= ~GENPD_FLAG_GOV_NEXT_WAKEUP;
>> +       genpd->next_wakeup = KTIME_MAX;
>> +       genpd_unlock(genpd);
>> +}
>> +EXPORT_SYMBOL_GPL(genpd_enable_next_wakeup);
>> +
>
>Please drop this, as I don't think we need a dedicated function to
>enable this feature.
>
Ah, I responded to the patch #2 regarding this before reading this
email.

>Instead, it seems like a better idea to let the genpd provider set it,
>before it calls pm_genpd_init(), along with its other genpd
>configurations.
>
If we no longer have a need for this feature in the domain, we could
dynamically disable it.

>>  static int genpd_add_subdomain(struct generic_pm_domain *genpd,
>>                                struct generic_pm_domain *subdomain)
>>  {
>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>> index 2ca919ae8d36..1f359bd19f77 100644
>> --- a/include/linux/pm_domain.h
>> +++ b/include/linux/pm_domain.h
>> @@ -55,13 +55,19 @@
>>   *
>>   * GENPD_FLAG_RPM_ALWAYS_ON:   Instructs genpd to always keep the PM domain
>>   *                             powered on except for system suspend.
>> + *
>> + * GENPD_FLAG_GOV_NEXT_WAKEUP: Enable the genpd governor to consider its
>> + *                             components' next wakeup when  determining the
>> + *                             optimal idle state. This is setup only if the
>> + *                             domain's idle state specifies a residency.
>>   */
>> -#define GENPD_FLAG_PM_CLK       (1U << 0)
>> -#define GENPD_FLAG_IRQ_SAFE     (1U << 1)
>> -#define GENPD_FLAG_ALWAYS_ON    (1U << 2)
>> -#define GENPD_FLAG_ACTIVE_WAKEUP (1U << 3)
>> -#define GENPD_FLAG_CPU_DOMAIN   (1U << 4)
>> -#define GENPD_FLAG_RPM_ALWAYS_ON (1U << 5)
>> +#define GENPD_FLAG_PM_CLK         (1U << 0)
>> +#define GENPD_FLAG_IRQ_SAFE       (1U << 1)
>> +#define GENPD_FLAG_ALWAYS_ON      (1U << 2)
>> +#define GENPD_FLAG_ACTIVE_WAKEUP   (1U << 3)
>> +#define GENPD_FLAG_CPU_DOMAIN     (1U << 4)
>> +#define GENPD_FLAG_RPM_ALWAYS_ON   (1U << 5)
>> +#define GENPD_FLAG_GOV_NEXT_WAKEUP (1U << 6)
>
>Nitpick.
>
>To me, the configuration is something that corresponds to the genpd,
>rather than the governor (even if it affects it in this case). That
>said, how about just naming the flag something like
>"GENPD_FLAG_MIN_RESIDENCY", as to indicate that the genpd's power off
>states have minimum residencies values that may deserve to be
>considered, while power off.
>
Hmmm..
>>
>>  enum gpd_status {
>>         GENPD_STATE_ON = 0,     /* PM domain is on */
>> @@ -205,6 +211,11 @@ static inline struct generic_pm_domain_data *dev_gpd_data(struct device *dev)
>>         return to_gpd_data(dev->power.subsys_data->domain_data);
>>  }
>>
>> +static inline bool genpd_may_use_next_wakeup(struct generic_pm_domain *genpd)
>> +{
>> +       return genpd->flags & GENPD_FLAG_GOV_NEXT_WAKEUP;
>> +}
>
>This can probably be moved into drivers/base/power/domain_governor.c.
>
Okay.
>> +
>>  int pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev);
>>  int pm_genpd_remove_device(struct device *dev);
>>  int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
>> @@ -217,6 +228,7 @@ int pm_genpd_remove(struct generic_pm_domain *genpd);
>>  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 genpd_enable_next_wakeup(struct generic_pm_domain *genpd, bool enable);
>>
>>  extern struct dev_power_governor simple_qos_governor;
>>  extern struct dev_power_governor pm_domain_always_on_gov;
>> @@ -275,6 +287,10 @@ static inline int dev_pm_genpd_remove_notifier(struct device *dev)
>>         return -EOPNOTSUPP;
>>  }
>>
>> +static void genpd_enable_next_wakeup(struct generic_pm_domain *genpd,
>> +                                    bool enable)
>> +{ }
>> +
>>  #define simple_qos_governor            (*(struct dev_power_governor *)(NULL))
>>  #define pm_domain_always_on_gov                (*(struct dev_power_governor *)(NULL))
>>  #endif
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>>
>
>Kind regards
>Uffe