From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Rafael J. Wysocki" Subject: Re: [PATCH v8 04/26] PM / Domains: Add support for CPU devices to genpd Date: Thu, 19 Jul 2018 12:25:54 +0200 Message-ID: <1678369.6pMUSOvdlV@aspire.rjw.lan> References: <20180620172226.15012-1-ulf.hansson@linaro.org> <20180620172226.15012-5-ulf.hansson@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7Bit Return-path: In-Reply-To: <20180620172226.15012-5-ulf.hansson@linaro.org> Sender: linux-kernel-owner@vger.kernel.org To: Ulf Hansson Cc: Sudeep Holla , Lorenzo Pieralisi , Mark Rutland , linux-pm@vger.kernel.org, Kevin Hilman , Lina Iyer , Lina Iyer , Rob Herring , Daniel Lezcano , Thomas Gleixner , Vincent Guittot , Stephen Boyd , Juri Lelli , Geert Uytterhoeven , linux-arm-kernel@lists.infradead.org, linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org On Wednesday, June 20, 2018 7:22:04 PM CEST Ulf Hansson wrote: > To enable a device belonging to a CPU to be attached to a PM domain managed > by genpd, let's do a few changes to genpd as to make it convenient to > manage the specifics around CPUs. > > First, as to be able to quickly find out what CPUs that are attached to a > genpd, which typically becomes useful from a genpd governor as following > changes is about to show, let's add a cpumask 'cpus' to the struct > generic_pm_domain. > > At the point when a device that belongs to a CPU, is attached/detached to > its corresponding PM domain via genpd_add_device(), let's update the > cpumask in genpd->cpus. Moreover, propagate the update of the cpumask to > the master domains, which makes the genpd->cpus to contain a cpumask that > hierarchically reflect all CPUs for a genpd, including CPUs attached to > subdomains. > > Second, to unconditionally manage CPUs and the cpumask in genpd->cpus, is > unnecessary for cases when only non-CPU devices are parts of a genpd. > Let's avoid this by adding a new configuration bit, GENPD_FLAG_CPU_DOMAIN. > Clients must set the bit before they call pm_genpd_init(), as to instruct > genpd that it shall deal with CPUs and thus manage the cpumask in > genpd->cpus. > > Cc: Lina Iyer > Co-developed-by: Lina Iyer > Signed-off-by: Ulf Hansson > --- > drivers/base/power/domain.c | 69 ++++++++++++++++++++++++++++++++++++- > include/linux/pm_domain.h | 3 ++ > 2 files changed, 71 insertions(+), 1 deletion(-) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 21d298e1820b..6149ce0bfa7b 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -20,6 +20,7 @@ > #include > #include > #include > +#include > > #include "power.h" > > @@ -126,6 +127,7 @@ static const struct genpd_lock_ops genpd_spin_ops = { > #define genpd_is_irq_safe(genpd) (genpd->flags & GENPD_FLAG_IRQ_SAFE) > #define genpd_is_always_on(genpd) (genpd->flags & GENPD_FLAG_ALWAYS_ON) > #define genpd_is_active_wakeup(genpd) (genpd->flags & GENPD_FLAG_ACTIVE_WAKEUP) > +#define genpd_is_cpu_domain(genpd) (genpd->flags & GENPD_FLAG_CPU_DOMAIN) > > static inline bool irq_safe_dev_in_no_sleep_domain(struct device *dev, > const struct generic_pm_domain *genpd) > @@ -1377,6 +1379,62 @@ static void genpd_free_dev_data(struct device *dev, > dev_pm_put_subsys_data(dev); > } > > +static void __genpd_update_cpumask(struct generic_pm_domain *genpd, > + int cpu, bool set, unsigned int depth) > +{ > + struct gpd_link *link; > + > + if (!genpd_is_cpu_domain(genpd)) > + return; > + > + list_for_each_entry(link, &genpd->slave_links, slave_node) { > + struct generic_pm_domain *master = link->master; > + > + genpd_lock_nested(master, depth + 1); > + __genpd_update_cpumask(master, cpu, set, depth + 1); > + genpd_unlock(master); > + } > + > + if (set) > + cpumask_set_cpu(cpu, genpd->cpus); > + else > + cpumask_clear_cpu(cpu, genpd->cpus); > +} As noted elsewhere, there is a concern about the possible weight of this cpumask and I think that it would be good to explicitly put a limit on it. > + > +static void genpd_update_cpumask(struct generic_pm_domain *genpd, > + struct device *dev, bool set) > +{ > + bool is_cpu = false; > + int cpu; > + > + if (!genpd_is_cpu_domain(genpd)) > + return; > + > + for_each_possible_cpu(cpu) { > + if (get_cpu_device(cpu) == dev) { > + is_cpu = true; You may call __genpd_update_cpumask() right here and then you won't need the extra is_cpu variable. > + break; > + } > + } > + > + if (!is_cpu) > + return; > + > + __genpd_update_cpumask(genpd, cpu, set, 0); > +} > + > +static void genpd_set_cpumask(struct generic_pm_domain *genpd, > + struct device *dev) > +{ > + genpd_update_cpumask(genpd, dev, true); > +} > + > +static void genpd_clear_cpumask(struct generic_pm_domain *genpd, > + struct device *dev) > +{ > + genpd_update_cpumask(genpd, dev, false); > +} > + > static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev, > struct gpd_timing_data *td) > { > @@ -1398,6 +1456,8 @@ static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev, > if (ret) > goto out; > > + genpd_set_cpumask(genpd, dev); > + > dev_pm_domain_set(dev, &genpd->domain); > > genpd->device_count++; > @@ -1459,6 +1519,7 @@ static int genpd_remove_device(struct generic_pm_domain *genpd, > if (genpd->detach_dev) > genpd->detach_dev(genpd, dev); > > + genpd_clear_cpumask(genpd, dev); > dev_pm_domain_set(dev, NULL); > > list_del_init(&pdd->list_node); > @@ -1686,11 +1747,16 @@ int pm_genpd_init(struct generic_pm_domain *genpd, > if (genpd_is_always_on(genpd) && !genpd_status_on(genpd)) > return -EINVAL; > > + if (!zalloc_cpumask_var(&genpd->cpus, GFP_KERNEL)) > + return -ENOMEM; > + > /* Use only one "off" state if there were no states declared */ > if (genpd->state_count == 0) { > ret = genpd_set_default_power_state(genpd); > - if (ret) > + if (ret) { > + free_cpumask_var(genpd->cpus); > return ret; > + } > } else if (!gov) { > pr_warn("%s : no governor for states\n", genpd->name); > } > @@ -1736,6 +1802,7 @@ static int genpd_remove(struct generic_pm_domain *genpd) > list_del(&genpd->gpd_list_node); > genpd_unlock(genpd); > cancel_work_sync(&genpd->power_off_work); > + free_cpumask_var(genpd->cpus); > kfree(genpd->free); > pr_debug("%s: removed %s\n", __func__, genpd->name); > > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h > index 27fca748344a..3f67ff0c1c69 100644 > --- a/include/linux/pm_domain.h > +++ b/include/linux/pm_domain.h > @@ -16,12 +16,14 @@ > #include > #include > #include > +#include > > /* Defines used for the flags field in the struct generic_pm_domain */ > #define GENPD_FLAG_PM_CLK (1U << 0) /* PM domain uses PM clk */ > #define GENPD_FLAG_IRQ_SAFE (1U << 1) /* PM domain operates in atomic */ > #define GENPD_FLAG_ALWAYS_ON (1U << 2) /* PM domain is always powered on */ > #define GENPD_FLAG_ACTIVE_WAKEUP (1U << 3) /* Keep devices active if wakeup */ > +#define GENPD_FLAG_CPU_DOMAIN (1U << 4) /* PM domain manages CPUs */ > > enum gpd_status { > GPD_STATE_ACTIVE = 0, /* PM domain is active */ > @@ -68,6 +70,7 @@ struct generic_pm_domain { > unsigned int suspended_count; /* System suspend device counter */ > unsigned int prepared_count; /* Suspend counter of prepared devices */ > unsigned int performance_state; /* Aggregated max performance state */ > + cpumask_var_t cpus; /* A cpumask of the attached CPUs */ > int (*power_off)(struct generic_pm_domain *domain); > int (*power_on)(struct generic_pm_domain *domain); > unsigned int (*opp_to_performance_state)(struct generic_pm_domain *genpd, > From mboxrd@z Thu Jan 1 00:00:00 1970 From: rjw@rjwysocki.net (Rafael J. Wysocki) Date: Thu, 19 Jul 2018 12:25:54 +0200 Subject: [PATCH v8 04/26] PM / Domains: Add support for CPU devices to genpd In-Reply-To: <20180620172226.15012-5-ulf.hansson@linaro.org> References: <20180620172226.15012-1-ulf.hansson@linaro.org> <20180620172226.15012-5-ulf.hansson@linaro.org> Message-ID: <1678369.6pMUSOvdlV@aspire.rjw.lan> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Wednesday, June 20, 2018 7:22:04 PM CEST Ulf Hansson wrote: > To enable a device belonging to a CPU to be attached to a PM domain managed > by genpd, let's do a few changes to genpd as to make it convenient to > manage the specifics around CPUs. > > First, as to be able to quickly find out what CPUs that are attached to a > genpd, which typically becomes useful from a genpd governor as following > changes is about to show, let's add a cpumask 'cpus' to the struct > generic_pm_domain. > > At the point when a device that belongs to a CPU, is attached/detached to > its corresponding PM domain via genpd_add_device(), let's update the > cpumask in genpd->cpus. Moreover, propagate the update of the cpumask to > the master domains, which makes the genpd->cpus to contain a cpumask that > hierarchically reflect all CPUs for a genpd, including CPUs attached to > subdomains. > > Second, to unconditionally manage CPUs and the cpumask in genpd->cpus, is > unnecessary for cases when only non-CPU devices are parts of a genpd. > Let's avoid this by adding a new configuration bit, GENPD_FLAG_CPU_DOMAIN. > Clients must set the bit before they call pm_genpd_init(), as to instruct > genpd that it shall deal with CPUs and thus manage the cpumask in > genpd->cpus. > > Cc: Lina Iyer > Co-developed-by: Lina Iyer > Signed-off-by: Ulf Hansson > --- > drivers/base/power/domain.c | 69 ++++++++++++++++++++++++++++++++++++- > include/linux/pm_domain.h | 3 ++ > 2 files changed, 71 insertions(+), 1 deletion(-) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 21d298e1820b..6149ce0bfa7b 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -20,6 +20,7 @@ > #include > #include > #include > +#include > > #include "power.h" > > @@ -126,6 +127,7 @@ static const struct genpd_lock_ops genpd_spin_ops = { > #define genpd_is_irq_safe(genpd) (genpd->flags & GENPD_FLAG_IRQ_SAFE) > #define genpd_is_always_on(genpd) (genpd->flags & GENPD_FLAG_ALWAYS_ON) > #define genpd_is_active_wakeup(genpd) (genpd->flags & GENPD_FLAG_ACTIVE_WAKEUP) > +#define genpd_is_cpu_domain(genpd) (genpd->flags & GENPD_FLAG_CPU_DOMAIN) > > static inline bool irq_safe_dev_in_no_sleep_domain(struct device *dev, > const struct generic_pm_domain *genpd) > @@ -1377,6 +1379,62 @@ static void genpd_free_dev_data(struct device *dev, > dev_pm_put_subsys_data(dev); > } > > +static void __genpd_update_cpumask(struct generic_pm_domain *genpd, > + int cpu, bool set, unsigned int depth) > +{ > + struct gpd_link *link; > + > + if (!genpd_is_cpu_domain(genpd)) > + return; > + > + list_for_each_entry(link, &genpd->slave_links, slave_node) { > + struct generic_pm_domain *master = link->master; > + > + genpd_lock_nested(master, depth + 1); > + __genpd_update_cpumask(master, cpu, set, depth + 1); > + genpd_unlock(master); > + } > + > + if (set) > + cpumask_set_cpu(cpu, genpd->cpus); > + else > + cpumask_clear_cpu(cpu, genpd->cpus); > +} As noted elsewhere, there is a concern about the possible weight of this cpumask and I think that it would be good to explicitly put a limit on it. > + > +static void genpd_update_cpumask(struct generic_pm_domain *genpd, > + struct device *dev, bool set) > +{ > + bool is_cpu = false; > + int cpu; > + > + if (!genpd_is_cpu_domain(genpd)) > + return; > + > + for_each_possible_cpu(cpu) { > + if (get_cpu_device(cpu) == dev) { > + is_cpu = true; You may call __genpd_update_cpumask() right here and then you won't need the extra is_cpu variable. > + break; > + } > + } > + > + if (!is_cpu) > + return; > + > + __genpd_update_cpumask(genpd, cpu, set, 0); > +} > + > +static void genpd_set_cpumask(struct generic_pm_domain *genpd, > + struct device *dev) > +{ > + genpd_update_cpumask(genpd, dev, true); > +} > + > +static void genpd_clear_cpumask(struct generic_pm_domain *genpd, > + struct device *dev) > +{ > + genpd_update_cpumask(genpd, dev, false); > +} > + > static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev, > struct gpd_timing_data *td) > { > @@ -1398,6 +1456,8 @@ static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev, > if (ret) > goto out; > > + genpd_set_cpumask(genpd, dev); > + > dev_pm_domain_set(dev, &genpd->domain); > > genpd->device_count++; > @@ -1459,6 +1519,7 @@ static int genpd_remove_device(struct generic_pm_domain *genpd, > if (genpd->detach_dev) > genpd->detach_dev(genpd, dev); > > + genpd_clear_cpumask(genpd, dev); > dev_pm_domain_set(dev, NULL); > > list_del_init(&pdd->list_node); > @@ -1686,11 +1747,16 @@ int pm_genpd_init(struct generic_pm_domain *genpd, > if (genpd_is_always_on(genpd) && !genpd_status_on(genpd)) > return -EINVAL; > > + if (!zalloc_cpumask_var(&genpd->cpus, GFP_KERNEL)) > + return -ENOMEM; > + > /* Use only one "off" state if there were no states declared */ > if (genpd->state_count == 0) { > ret = genpd_set_default_power_state(genpd); > - if (ret) > + if (ret) { > + free_cpumask_var(genpd->cpus); > return ret; > + } > } else if (!gov) { > pr_warn("%s : no governor for states\n", genpd->name); > } > @@ -1736,6 +1802,7 @@ static int genpd_remove(struct generic_pm_domain *genpd) > list_del(&genpd->gpd_list_node); > genpd_unlock(genpd); > cancel_work_sync(&genpd->power_off_work); > + free_cpumask_var(genpd->cpus); > kfree(genpd->free); > pr_debug("%s: removed %s\n", __func__, genpd->name); > > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h > index 27fca748344a..3f67ff0c1c69 100644 > --- a/include/linux/pm_domain.h > +++ b/include/linux/pm_domain.h > @@ -16,12 +16,14 @@ > #include > #include > #include > +#include > > /* Defines used for the flags field in the struct generic_pm_domain */ > #define GENPD_FLAG_PM_CLK (1U << 0) /* PM domain uses PM clk */ > #define GENPD_FLAG_IRQ_SAFE (1U << 1) /* PM domain operates in atomic */ > #define GENPD_FLAG_ALWAYS_ON (1U << 2) /* PM domain is always powered on */ > #define GENPD_FLAG_ACTIVE_WAKEUP (1U << 3) /* Keep devices active if wakeup */ > +#define GENPD_FLAG_CPU_DOMAIN (1U << 4) /* PM domain manages CPUs */ > > enum gpd_status { > GPD_STATE_ACTIVE = 0, /* PM domain is active */ > @@ -68,6 +70,7 @@ struct generic_pm_domain { > unsigned int suspended_count; /* System suspend device counter */ > unsigned int prepared_count; /* Suspend counter of prepared devices */ > unsigned int performance_state; /* Aggregated max performance state */ > + cpumask_var_t cpus; /* A cpumask of the attached CPUs */ > int (*power_off)(struct generic_pm_domain *domain); > int (*power_on)(struct generic_pm_domain *domain); > unsigned int (*opp_to_performance_state)(struct generic_pm_domain *genpd, >