| Message ID | 1652275016-13423-5-git-send-email-quic_mkshah@quicinc.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | Add APSS RSC to Cluster power domain | expand |
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
On Thu 19 May 07:56 CDT 2022, Ulf Hansson wrote: > 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. > Maulik, it seems the only things remaining for this series it resolve these three comments. Looking forward to be able to merge the next revision. Regards, Bjorn
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. + */ +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; + + 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; /* 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
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(+)