| Message ID | 20191211154343.29765-14-ulf.hansson@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cpuidle: psci: Support hierarchical CPU arrangement | expand |
On Wed, Dec 11, 2019 at 04:43:42PM +0100, Ulf Hansson wrote: > When the hierarchical CPU topology layout is used in DT and the PSCI OSI > mode is supported by the PSCI FW, let's initialize a corresponding PM > domain topology by using genpd. This enables a CPU and a group of CPUs, > when attached to the topology, to be power-managed accordingly. > > To trigger the attempt to initialize the genpd data structures let's use a > subsys_initcall, which should be early enough to allow CPUs, but also other > devices to be attached. > > The initialization consists of parsing the PSCI OF node for the topology > and the "domain idle states" DT bindings. In case the idle states are > compatible with "domain-idle-state", the initialized genpd becomes > responsible of selecting an idle state for the PM domain, via assigning it > a genpd governor. > > Note that, a successful initialization of the genpd data structures, is > followed by a call to psci_set_osi_mode(), as to try to enable the OSI mode > in the PSCI FW. In case this fails, we fall back into a degraded mode > rather than bailing out and returning an error code. > > Co-developed-by: Lina Iyer <lina.iyer@linaro.org> > Signed-off-by: Lina Iyer <lina.iyer@linaro.org> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > --- > > Changes in v4: > - None. > > --- > drivers/cpuidle/cpuidle-psci-domain.c | 267 ++++++++++++++++++++++++++ > drivers/cpuidle/cpuidle-psci.c | 4 +- > drivers/cpuidle/cpuidle-psci.h | 5 + > 3 files changed, 274 insertions(+), 2 deletions(-) > > diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c > index 656ef3d59149..c2f94ba42222 100644 > --- a/drivers/cpuidle/cpuidle-psci-domain.c > +++ b/drivers/cpuidle/cpuidle-psci-domain.c > @@ -7,14 +7,281 @@ > * > */ > > +#define pr_fmt(fmt) "CPUidle PSCI: " fmt > + > #include <linux/cpu.h> > #include <linux/device.h> > #include <linux/kernel.h> > #include <linux/pm_domain.h> > #include <linux/pm_runtime.h> > +#include <linux/psci.h> > +#include <linux/slab.h> > +#include <linux/string.h> > > #include "cpuidle-psci.h" > > +struct psci_pd_provider { > + struct list_head link; > + struct device_node *node; > +}; > + > +static LIST_HEAD(psci_pd_providers); > +static bool osi_mode_enabled; > + > +static int psci_pd_power_off(struct generic_pm_domain *pd) > +{ > + struct genpd_power_state *state = &pd->states[pd->state_idx]; > + u32 *pd_state; > + > + /* If we have failed to enable OSI mode, then abort power off. */ > + if (!osi_mode_enabled) > + return -EBUSY; > + Why is above check needed ? Shouldn't we have disable/remove pd of OSI is not enabled ? > + if (!state->data) > + return 0; > + > + /* OSI mode is enabled, set the corresponding domain state. */ > + pd_state = state->data; > + psci_set_domain_state(*pd_state); > + > + return 0; > +} > + [...] > +static const struct of_device_id psci_of_match[] __initconst = { > + { .compatible = "arm,psci" }, I think we can drop the above one as it's for v0.1 which didn't support OSI. > + { .compatible = "arm,psci-0.2" }, > + { .compatible = "arm,psci-1.0" }, > + {} > +}; > + > +static int __init psci_idle_init_domains(void) > +{ > + struct device_node *np = of_find_matching_node(NULL, psci_of_match); > + struct device_node *node; > + int ret = 0, pd_count = 0; > + > + if (!np) > + return -ENODEV; > + > + /* Currently limit the hierarchical topology to be used in OSI mode. */ > + if (!psci_has_osi_support()) > + goto out; > + > + /* > + * Parse child nodes for the "#power-domain-cells" property and > + * initialize a genpd/genpd-of-provider pair when it's found. > + */ > + for_each_child_of_node(np, node) { > + if (!of_find_property(node, "#power-domain-cells", NULL)) > + continue; > + > + ret = psci_pd_init(node); > + if (ret) > + goto put_node; > + > + pd_count++; > + } > + > + /* Bail out if not using the hierarchical CPU topology. */ > + if (!pd_count) > + goto out; > + > + /* Link genpd masters/subdomains to model the CPU topology. */ > + ret = psci_pd_init_topology(np); > + if (ret) > + goto remove_pd; > + > + /* Try to enable OSI mode. */ > + ret = psci_set_osi_mode(); > + if (ret) > + pr_warn("failed to enable OSI mode: %d\n", ret); Same question as above: shouldn't we disable and goto remove_pd ? -- Regards, Sudeep
On Thu, 19 Dec 2019 at 15:34, Sudeep Holla <sudeep.holla@arm.com> wrote: > > On Wed, Dec 11, 2019 at 04:43:42PM +0100, Ulf Hansson wrote: > > When the hierarchical CPU topology layout is used in DT and the PSCI OSI > > mode is supported by the PSCI FW, let's initialize a corresponding PM > > domain topology by using genpd. This enables a CPU and a group of CPUs, > > when attached to the topology, to be power-managed accordingly. > > > > To trigger the attempt to initialize the genpd data structures let's use a > > subsys_initcall, which should be early enough to allow CPUs, but also other > > devices to be attached. > > > > The initialization consists of parsing the PSCI OF node for the topology > > and the "domain idle states" DT bindings. In case the idle states are > > compatible with "domain-idle-state", the initialized genpd becomes > > responsible of selecting an idle state for the PM domain, via assigning it > > a genpd governor. > > > > Note that, a successful initialization of the genpd data structures, is > > followed by a call to psci_set_osi_mode(), as to try to enable the OSI mode > > in the PSCI FW. In case this fails, we fall back into a degraded mode > > rather than bailing out and returning an error code. > > > > Co-developed-by: Lina Iyer <lina.iyer@linaro.org> > > Signed-off-by: Lina Iyer <lina.iyer@linaro.org> > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > --- > > > > Changes in v4: > > - None. > > > > --- > > drivers/cpuidle/cpuidle-psci-domain.c | 267 ++++++++++++++++++++++++++ > > drivers/cpuidle/cpuidle-psci.c | 4 +- > > drivers/cpuidle/cpuidle-psci.h | 5 + > > 3 files changed, 274 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c > > index 656ef3d59149..c2f94ba42222 100644 > > --- a/drivers/cpuidle/cpuidle-psci-domain.c > > +++ b/drivers/cpuidle/cpuidle-psci-domain.c > > @@ -7,14 +7,281 @@ > > * > > */ > > > > +#define pr_fmt(fmt) "CPUidle PSCI: " fmt > > + > > #include <linux/cpu.h> > > #include <linux/device.h> > > #include <linux/kernel.h> > > #include <linux/pm_domain.h> > > #include <linux/pm_runtime.h> > > +#include <linux/psci.h> > > +#include <linux/slab.h> > > +#include <linux/string.h> > > > > #include "cpuidle-psci.h" > > > > +struct psci_pd_provider { > > + struct list_head link; > > + struct device_node *node; > > +}; > > + > > +static LIST_HEAD(psci_pd_providers); > > +static bool osi_mode_enabled; > > + > > +static int psci_pd_power_off(struct generic_pm_domain *pd) > > +{ > > + struct genpd_power_state *state = &pd->states[pd->state_idx]; > > + u32 *pd_state; > > + > > + /* If we have failed to enable OSI mode, then abort power off. */ > > + if (!osi_mode_enabled) > > + return -EBUSY; > > + > > Why is above check needed ? Shouldn't we have disable/remove pd of > OSI is not enabled ? Well, failing to enable OSI should in practice not happen, while it theoretically it could. My approach to this has been to fall back to use a "degraded mode", which seems quite common for these kind of situations. The degraded mode means, we are preventing domain states from being used. More importantly, it also keeps the code registering the PM domains, a bit simpler. > > > + if (!state->data) > > + return 0; > > + > > + /* OSI mode is enabled, set the corresponding domain state. */ > > + pd_state = state->data; > > + psci_set_domain_state(*pd_state); > > + > > + return 0; > > +} > > + > > [...] > > > +static const struct of_device_id psci_of_match[] __initconst = { > > + { .compatible = "arm,psci" }, > > I think we can drop the above one as it's for v0.1 which didn't support > OSI. Yeah, I do that. > > > + { .compatible = "arm,psci-0.2" }, > > + { .compatible = "arm,psci-1.0" }, > > + {} > > +}; > > + > > +static int __init psci_idle_init_domains(void) > > +{ > > + struct device_node *np = of_find_matching_node(NULL, psci_of_match); > > + struct device_node *node; > > + int ret = 0, pd_count = 0; > > + > > + if (!np) > > + return -ENODEV; > > + > > + /* Currently limit the hierarchical topology to be used in OSI mode. */ > > + if (!psci_has_osi_support()) > > + goto out; > > + > > + /* > > + * Parse child nodes for the "#power-domain-cells" property and > > + * initialize a genpd/genpd-of-provider pair when it's found. > > + */ > > + for_each_child_of_node(np, node) { > > + if (!of_find_property(node, "#power-domain-cells", NULL)) > > + continue; > > + > > + ret = psci_pd_init(node); > > + if (ret) > > + goto put_node; > > + > > + pd_count++; > > + } > > + > > + /* Bail out if not using the hierarchical CPU topology. */ > > + if (!pd_count) > > + goto out; > > + > > + /* Link genpd masters/subdomains to model the CPU topology. */ > > + ret = psci_pd_init_topology(np); > > + if (ret) > > + goto remove_pd; > > + > > + /* Try to enable OSI mode. */ > > + ret = psci_set_osi_mode(); > > + if (ret) > > + pr_warn("failed to enable OSI mode: %d\n", ret); > > Same question as above: shouldn't we disable and goto remove_pd ? See my answer above. Does it satisfy your concern? If so, may I add your reviewed-by tag, for the next re-spin when I have removed one of the compatible strings? Kind regards Uffe
On Thu, Dec 19, 2019 at 04:48:39PM +0100, Ulf Hansson wrote: > On Thu, 19 Dec 2019 at 15:34, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > On Wed, Dec 11, 2019 at 04:43:42PM +0100, Ulf Hansson wrote: > > > When the hierarchical CPU topology layout is used in DT and the PSCI OSI > > > mode is supported by the PSCI FW, let's initialize a corresponding PM > > > domain topology by using genpd. This enables a CPU and a group of CPUs, > > > when attached to the topology, to be power-managed accordingly. > > > > > > To trigger the attempt to initialize the genpd data structures let's use a > > > subsys_initcall, which should be early enough to allow CPUs, but also other > > > devices to be attached. > > > > > > The initialization consists of parsing the PSCI OF node for the topology > > > and the "domain idle states" DT bindings. In case the idle states are > > > compatible with "domain-idle-state", the initialized genpd becomes > > > responsible of selecting an idle state for the PM domain, via assigning it > > > a genpd governor. > > > > > > Note that, a successful initialization of the genpd data structures, is > > > followed by a call to psci_set_osi_mode(), as to try to enable the OSI mode > > > in the PSCI FW. In case this fails, we fall back into a degraded mode > > > rather than bailing out and returning an error code. > > > > > > Co-developed-by: Lina Iyer <lina.iyer@linaro.org> > > > Signed-off-by: Lina Iyer <lina.iyer@linaro.org> > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > > --- > > > > > > Changes in v4: > > > - None. > > > > > > --- > > > drivers/cpuidle/cpuidle-psci-domain.c | 267 ++++++++++++++++++++++++++ > > > drivers/cpuidle/cpuidle-psci.c | 4 +- > > > drivers/cpuidle/cpuidle-psci.h | 5 + > > > 3 files changed, 274 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c > > > index 656ef3d59149..c2f94ba42222 100644 > > > --- a/drivers/cpuidle/cpuidle-psci-domain.c > > > +++ b/drivers/cpuidle/cpuidle-psci-domain.c > > > @@ -7,14 +7,281 @@ > > > * > > > */ > > > > > > +#define pr_fmt(fmt) "CPUidle PSCI: " fmt > > > + > > > #include <linux/cpu.h> > > > #include <linux/device.h> > > > #include <linux/kernel.h> > > > #include <linux/pm_domain.h> > > > #include <linux/pm_runtime.h> > > > +#include <linux/psci.h> > > > +#include <linux/slab.h> > > > +#include <linux/string.h> > > > > > > #include "cpuidle-psci.h" > > > > > > +struct psci_pd_provider { > > > + struct list_head link; > > > + struct device_node *node; > > > +}; > > > + > > > +static LIST_HEAD(psci_pd_providers); > > > +static bool osi_mode_enabled; > > > + > > > +static int psci_pd_power_off(struct generic_pm_domain *pd) > > > +{ > > > + struct genpd_power_state *state = &pd->states[pd->state_idx]; > > > + u32 *pd_state; > > > + > > > + /* If we have failed to enable OSI mode, then abort power off. */ > > > + if (!osi_mode_enabled) > > > + return -EBUSY; > > > + > > > > Why is above check needed ? Shouldn't we have disable/remove pd of > > OSI is not enabled ? > > Well, failing to enable OSI should in practice not happen, while it > theoretically it could. > I won't assume that. Since it's new and not tested yet, I prefer to assume it can fail. > My approach to this has been to fall back to use a "degraded mode", > which seems quite common for these kind of situations. The degraded > mode means, we are preventing domain states from being used. > But why can't we just fail registering or remove if already added. They are useless for "degraded mode" anyways. And it will ensure that data->dev is NULL. Sorry now I see why you said it can be NULL but I would rather not leave those unused genpd in place in case of error. > More importantly, it also keeps the code registering the PM domains, a > bit simpler. > I feel it is simpler other way around especially if I am testing and seeing failures but I see genpd succeeding. That's confusing. -- Regards, Sudeep
On Thu, 19 Dec 2019 at 19:06, Sudeep Holla <sudeep.holla@arm.com> wrote: > > On Thu, Dec 19, 2019 at 04:48:39PM +0100, Ulf Hansson wrote: > > On Thu, 19 Dec 2019 at 15:34, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > > > On Wed, Dec 11, 2019 at 04:43:42PM +0100, Ulf Hansson wrote: > > > > When the hierarchical CPU topology layout is used in DT and the PSCI OSI > > > > mode is supported by the PSCI FW, let's initialize a corresponding PM > > > > domain topology by using genpd. This enables a CPU and a group of CPUs, > > > > when attached to the topology, to be power-managed accordingly. > > > > > > > > To trigger the attempt to initialize the genpd data structures let's use a > > > > subsys_initcall, which should be early enough to allow CPUs, but also other > > > > devices to be attached. > > > > > > > > The initialization consists of parsing the PSCI OF node for the topology > > > > and the "domain idle states" DT bindings. In case the idle states are > > > > compatible with "domain-idle-state", the initialized genpd becomes > > > > responsible of selecting an idle state for the PM domain, via assigning it > > > > a genpd governor. > > > > > > > > Note that, a successful initialization of the genpd data structures, is > > > > followed by a call to psci_set_osi_mode(), as to try to enable the OSI mode > > > > in the PSCI FW. In case this fails, we fall back into a degraded mode > > > > rather than bailing out and returning an error code. > > > > > > > > Co-developed-by: Lina Iyer <lina.iyer@linaro.org> > > > > Signed-off-by: Lina Iyer <lina.iyer@linaro.org> > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > > > --- > > > > > > > > Changes in v4: > > > > - None. > > > > > > > > --- > > > > drivers/cpuidle/cpuidle-psci-domain.c | 267 ++++++++++++++++++++++++++ > > > > drivers/cpuidle/cpuidle-psci.c | 4 +- > > > > drivers/cpuidle/cpuidle-psci.h | 5 + > > > > 3 files changed, 274 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c > > > > index 656ef3d59149..c2f94ba42222 100644 > > > > --- a/drivers/cpuidle/cpuidle-psci-domain.c > > > > +++ b/drivers/cpuidle/cpuidle-psci-domain.c > > > > @@ -7,14 +7,281 @@ > > > > * > > > > */ > > > > > > > > +#define pr_fmt(fmt) "CPUidle PSCI: " fmt > > > > + > > > > #include <linux/cpu.h> > > > > #include <linux/device.h> > > > > #include <linux/kernel.h> > > > > #include <linux/pm_domain.h> > > > > #include <linux/pm_runtime.h> > > > > +#include <linux/psci.h> > > > > +#include <linux/slab.h> > > > > +#include <linux/string.h> > > > > > > > > #include "cpuidle-psci.h" > > > > > > > > +struct psci_pd_provider { > > > > + struct list_head link; > > > > + struct device_node *node; > > > > +}; > > > > + > > > > +static LIST_HEAD(psci_pd_providers); > > > > +static bool osi_mode_enabled; > > > > + > > > > +static int psci_pd_power_off(struct generic_pm_domain *pd) > > > > +{ > > > > + struct genpd_power_state *state = &pd->states[pd->state_idx]; > > > > + u32 *pd_state; > > > > + > > > > + /* If we have failed to enable OSI mode, then abort power off. */ > > > > + if (!osi_mode_enabled) > > > > + return -EBUSY; > > > > + > > > > > > Why is above check needed ? Shouldn't we have disable/remove pd of > > > OSI is not enabled ? > > > > Well, failing to enable OSI should in practice not happen, while it > > theoretically it could. > > > > I won't assume that. Since it's new and not tested yet, I prefer to assume > it can fail. Yes, I agree. Hence the degraded mode. > > > My approach to this has been to fall back to use a "degraded mode", > > which seems quite common for these kind of situations. The degraded > > mode means, we are preventing domain states from being used. > > > > But why can't we just fail registering or remove if already added. We can, but there are more problems with that than leaving this in a degraded mode, I think. See more below. > They are useless for "degraded mode" anyways. And it will ensure that > data->dev is NULL. Sorry now I see why you said it can be NULL but I > would rather not leave those unused genpd in place in case of error. data->dev would not be NULL in this case, because the dev_pm_domain_attach_by_name() which is called when we attach the CPU is going to return an error code, not NULL. That's because the connection is there in the DTB and thus it must fail, in this case it would be with -EPROBE_DEFER (waiting for a genpd provider to be registered). That would then lead to that the entire cpuidle-psci driver would fail to initiate/probe. In my opinion, I think it's better to fall back into a degraded mode, using all the idle states for the CPUs, but just preventing the cluster idle states. Just wanted to make this more clear for you to consider. I am happy to change in any way you suggest, but please confirm that you really want another behaviour than the degraded mode. > > > More importantly, it also keeps the code registering the PM domains, a > > bit simpler. > > > > I feel it is simpler other way around especially if I am testing and > seeing failures but I see genpd succeeding. That's confusing. There is an warning message printed if psci_set_osi_mode() fails, "failed to enable OSI mode", in psci_idle_init_domains(). We could potentially add some more information to that message, that this also leads into using a "degraded mode". Would that be sufficient to satisfy your concerns? Kind regards Uffe
On Thu, Dec 19, 2019 at 11:02:40PM +0100, Ulf Hansson wrote: > On Thu, 19 Dec 2019 at 19:06, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > On Thu, Dec 19, 2019 at 04:48:39PM +0100, Ulf Hansson wrote: > > > On Thu, 19 Dec 2019 at 15:34, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > > > > > On Wed, Dec 11, 2019 at 04:43:42PM +0100, Ulf Hansson wrote: > > > > > When the hierarchical CPU topology layout is used in DT and the PSCI OSI > > > > > mode is supported by the PSCI FW, let's initialize a corresponding PM > > > > > domain topology by using genpd. This enables a CPU and a group of CPUs, > > > > > when attached to the topology, to be power-managed accordingly. > > > > > > > > > > To trigger the attempt to initialize the genpd data structures let's use a > > > > > subsys_initcall, which should be early enough to allow CPUs, but also other > > > > > devices to be attached. > > > > > > > > > > The initialization consists of parsing the PSCI OF node for the topology > > > > > and the "domain idle states" DT bindings. In case the idle states are > > > > > compatible with "domain-idle-state", the initialized genpd becomes > > > > > responsible of selecting an idle state for the PM domain, via assigning it > > > > > a genpd governor. > > > > > > > > > > Note that, a successful initialization of the genpd data structures, is > > > > > followed by a call to psci_set_osi_mode(), as to try to enable the OSI mode > > > > > in the PSCI FW. In case this fails, we fall back into a degraded mode > > > > > rather than bailing out and returning an error code. > > > > > > > > > > Co-developed-by: Lina Iyer <lina.iyer@linaro.org> > > > > > Signed-off-by: Lina Iyer <lina.iyer@linaro.org> > > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > > > > --- > > > > > > > > > > Changes in v4: > > > > > - None. > > > > > > > > > > --- > > > > > drivers/cpuidle/cpuidle-psci-domain.c | 267 ++++++++++++++++++++++++++ > > > > > drivers/cpuidle/cpuidle-psci.c | 4 +- > > > > > drivers/cpuidle/cpuidle-psci.h | 5 + > > > > > 3 files changed, 274 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c > > > > > index 656ef3d59149..c2f94ba42222 100644 > > > > > --- a/drivers/cpuidle/cpuidle-psci-domain.c > > > > > +++ b/drivers/cpuidle/cpuidle-psci-domain.c > > > > > @@ -7,14 +7,281 @@ > > > > > * > > > > > */ > > > > > > > > > > +#define pr_fmt(fmt) "CPUidle PSCI: " fmt > > > > > + > > > > > #include <linux/cpu.h> > > > > > #include <linux/device.h> > > > > > #include <linux/kernel.h> > > > > > #include <linux/pm_domain.h> > > > > > #include <linux/pm_runtime.h> > > > > > +#include <linux/psci.h> > > > > > +#include <linux/slab.h> > > > > > +#include <linux/string.h> > > > > > > > > > > #include "cpuidle-psci.h" > > > > > > > > > > +struct psci_pd_provider { > > > > > + struct list_head link; > > > > > + struct device_node *node; > > > > > +}; > > > > > + > > > > > +static LIST_HEAD(psci_pd_providers); > > > > > +static bool osi_mode_enabled; > > > > > + > > > > > +static int psci_pd_power_off(struct generic_pm_domain *pd) > > > > > +{ > > > > > + struct genpd_power_state *state = &pd->states[pd->state_idx]; > > > > > + u32 *pd_state; > > > > > + > > > > > + /* If we have failed to enable OSI mode, then abort power off. */ > > > > > + if (!osi_mode_enabled) > > > > > + return -EBUSY; > > > > > + > > > > > > > > Why is above check needed ? Shouldn't we have disable/remove pd of > > > > OSI is not enabled ? > > > > > > Well, failing to enable OSI should in practice not happen, while it > > > theoretically it could. > > > > > > > I won't assume that. Since it's new and not tested yet, I prefer to assume > > it can fail. > > Yes, I agree. Hence the degraded mode. > > > > > > My approach to this has been to fall back to use a "degraded mode", > > > which seems quite common for these kind of situations. The degraded > > > mode means, we are preventing domain states from being used. > > > > > > > But why can't we just fail registering or remove if already added. > > We can, but there are more problems with that than leaving this in a > degraded mode, I think. See more below. > > > They are useless for "degraded mode" anyways. And it will ensure that > > data->dev is NULL. Sorry now I see why you said it can be NULL but I > > would rather not leave those unused genpd in place in case of error. > > data->dev would not be NULL in this case, because the > dev_pm_domain_attach_by_name() which is called when we attach the CPU > is going to return an error code, not NULL. > > That's because the connection is there in the DTB and thus it must > fail, in this case it would be with -EPROBE_DEFER (waiting for a genpd > provider to be registered). > > That would then lead to that the entire cpuidle-psci driver would fail > to initiate/probe. In my opinion, I think it's better to fall back > into a degraded mode, using all the idle states for the CPUs, but just > preventing the cluster idle states. > > Just wanted to make this more clear for you to consider. I am happy to > change in any way you suggest, but please confirm that you really want > another behaviour than the degraded mode. > Sorry but if OSI set failed in firmware, it will be operating in default/ PC mode and I *don't* want to create genpd for that. It's confusing. Even if you don't create all these genpd domains, it is still degraded mode and we are anyway not changing that. Let me know if my understanding is wrong here. I am sure, DTB may get copied to different platform and the firmware may not support OSI. I know we have logs, but creating and leaving those genpd domains unused will be just confusing. Please change that. -- Regards, Sudeep
On Fri, 20 Dec 2019 at 11:07, Sudeep Holla <sudeep.holla@arm.com> wrote: > > On Thu, Dec 19, 2019 at 11:02:40PM +0100, Ulf Hansson wrote: > > On Thu, 19 Dec 2019 at 19:06, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > > > On Thu, Dec 19, 2019 at 04:48:39PM +0100, Ulf Hansson wrote: > > > > On Thu, 19 Dec 2019 at 15:34, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > > > > > > > On Wed, Dec 11, 2019 at 04:43:42PM +0100, Ulf Hansson wrote: > > > > > > When the hierarchical CPU topology layout is used in DT and the PSCI OSI > > > > > > mode is supported by the PSCI FW, let's initialize a corresponding PM > > > > > > domain topology by using genpd. This enables a CPU and a group of CPUs, > > > > > > when attached to the topology, to be power-managed accordingly. > > > > > > > > > > > > To trigger the attempt to initialize the genpd data structures let's use a > > > > > > subsys_initcall, which should be early enough to allow CPUs, but also other > > > > > > devices to be attached. > > > > > > > > > > > > The initialization consists of parsing the PSCI OF node for the topology > > > > > > and the "domain idle states" DT bindings. In case the idle states are > > > > > > compatible with "domain-idle-state", the initialized genpd becomes > > > > > > responsible of selecting an idle state for the PM domain, via assigning it > > > > > > a genpd governor. > > > > > > > > > > > > Note that, a successful initialization of the genpd data structures, is > > > > > > followed by a call to psci_set_osi_mode(), as to try to enable the OSI mode > > > > > > in the PSCI FW. In case this fails, we fall back into a degraded mode > > > > > > rather than bailing out and returning an error code. > > > > > > > > > > > > Co-developed-by: Lina Iyer <lina.iyer@linaro.org> > > > > > > Signed-off-by: Lina Iyer <lina.iyer@linaro.org> > > > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > > > > > --- > > > > > > > > > > > > Changes in v4: > > > > > > - None. > > > > > > > > > > > > --- > > > > > > drivers/cpuidle/cpuidle-psci-domain.c | 267 ++++++++++++++++++++++++++ > > > > > > drivers/cpuidle/cpuidle-psci.c | 4 +- > > > > > > drivers/cpuidle/cpuidle-psci.h | 5 + > > > > > > 3 files changed, 274 insertions(+), 2 deletions(-) > > > > > > > > > > > > diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c > > > > > > index 656ef3d59149..c2f94ba42222 100644 > > > > > > --- a/drivers/cpuidle/cpuidle-psci-domain.c > > > > > > +++ b/drivers/cpuidle/cpuidle-psci-domain.c > > > > > > @@ -7,14 +7,281 @@ > > > > > > * > > > > > > */ > > > > > > > > > > > > +#define pr_fmt(fmt) "CPUidle PSCI: " fmt > > > > > > + > > > > > > #include <linux/cpu.h> > > > > > > #include <linux/device.h> > > > > > > #include <linux/kernel.h> > > > > > > #include <linux/pm_domain.h> > > > > > > #include <linux/pm_runtime.h> > > > > > > +#include <linux/psci.h> > > > > > > +#include <linux/slab.h> > > > > > > +#include <linux/string.h> > > > > > > > > > > > > #include "cpuidle-psci.h" > > > > > > > > > > > > +struct psci_pd_provider { > > > > > > + struct list_head link; > > > > > > + struct device_node *node; > > > > > > +}; > > > > > > + > > > > > > +static LIST_HEAD(psci_pd_providers); > > > > > > +static bool osi_mode_enabled; > > > > > > + > > > > > > +static int psci_pd_power_off(struct generic_pm_domain *pd) > > > > > > +{ > > > > > > + struct genpd_power_state *state = &pd->states[pd->state_idx]; > > > > > > + u32 *pd_state; > > > > > > + > > > > > > + /* If we have failed to enable OSI mode, then abort power off. */ > > > > > > + if (!osi_mode_enabled) > > > > > > + return -EBUSY; > > > > > > + > > > > > > > > > > Why is above check needed ? Shouldn't we have disable/remove pd of > > > > > OSI is not enabled ? > > > > > > > > Well, failing to enable OSI should in practice not happen, while it > > > > theoretically it could. > > > > > > > > > > I won't assume that. Since it's new and not tested yet, I prefer to assume > > > it can fail. > > > > Yes, I agree. Hence the degraded mode. > > > > > > > > > My approach to this has been to fall back to use a "degraded mode", > > > > which seems quite common for these kind of situations. The degraded > > > > mode means, we are preventing domain states from being used. > > > > > > > > > > But why can't we just fail registering or remove if already added. > > > > We can, but there are more problems with that than leaving this in a > > degraded mode, I think. See more below. > > > > > They are useless for "degraded mode" anyways. And it will ensure that > > > data->dev is NULL. Sorry now I see why you said it can be NULL but I > > > would rather not leave those unused genpd in place in case of error. > > > > data->dev would not be NULL in this case, because the > > dev_pm_domain_attach_by_name() which is called when we attach the CPU > > is going to return an error code, not NULL. > > > > That's because the connection is there in the DTB and thus it must > > fail, in this case it would be with -EPROBE_DEFER (waiting for a genpd > > provider to be registered). > > > > That would then lead to that the entire cpuidle-psci driver would fail > > to initiate/probe. In my opinion, I think it's better to fall back > > into a degraded mode, using all the idle states for the CPUs, but just > > preventing the cluster idle states. > > > > Just wanted to make this more clear for you to consider. I am happy to > > change in any way you suggest, but please confirm that you really want > > another behaviour than the degraded mode. > > > > Sorry but if OSI set failed in firmware, it will be operating in default/ > PC mode and I *don't* want to create genpd for that. It's confusing. Alright, so that will cause some additional changes - let's agree how to make those. > Even if you don't create all these genpd domains, it is still degraded > mode and we are anyway not changing that. Let me know if my understanding > is wrong here. Your understanding is wrong. If I remove the genpds because psci_set_osi_mode() fails, then in the current suggested initialization path, that will lead to that the entire cpuidle-psci driver will fail to initiate (which is because psci_dt_attach_cpu() returns an error). In other words, only WFI state will be used by cpuidle as there will be no cpuidle driver registered at all. That would not be an acceptable behaviour, as it would make the situation worse than today. What we want in this scenario is to keep using all the idle states for the CPUs, but ignores those for the cluster. That we both agree on, right? > > I am sure, DTB may get copied to different platform and the firmware may > not support OSI. I know we have logs, but creating and leaving those > genpd domains unused will be just confusing. Please change that. We are not creating any genpds unless OSI mode is supported. We do not even try to attach CPUs to the PM domains, unless OSI mode is supported. So this should already work according to your expectations and previous requests. To address your concern about removing genpds when psci_set_osi_mode() fails, we also need to address the problems we get when calling psci_dt_attach_cpu(). There are two viable options as I see it. 1. Prevent calling psci_dt_attach_cpu() altogether when psci_set_osi_mode() failed. This means another function needs to be shared from cpuidle-psci-domain.c to let cpuidle-psci.c know about it. 2. We can let psci_dt_attach_cpu() return NULL, when psci_set_osi_mode() failed - as this information is already known by cpuidle-psci-domain.c. I vote for option 2, but what do you think? Kind regards Uffe
On Fri, Dec 20, 2019 at 12:27:39PM +0100, Ulf Hansson wrote: > On Fri, 20 Dec 2019 at 11:07, Sudeep Holla <sudeep.holla@arm.com> wrote: [...] > > > > Even if you don't create all these genpd domains, it is still degraded > > mode and we are anyway not changing that. Let me know if my understanding > > is wrong here. > > Your understanding is wrong. > > If I remove the genpds because psci_set_osi_mode() fails, then in the > current suggested initialization path, that will lead to that the > entire cpuidle-psci driver will fail to initiate (which is because > psci_dt_attach_cpu() returns an error). In other words, only WFI state > will be used by cpuidle as there will be no cpuidle driver registered > at all. > > That would not be an acceptable behaviour, as it would make the > situation worse than today. > > What we want in this scenario is to keep using all the idle states for > the CPUs, but ignores those for the cluster. That we both agree on, > right? > Yes, I agree and understand that. I was assuming as part of this change you will fixup psci_dt_cpu_init_idle not to return error but just allow CPU level idle. Sorry if that was not clear, I was always assuming that. > > > > I am sure, DTB may get copied to different platform and the firmware may > > not support OSI. I know we have logs, but creating and leaving those > > genpd domains unused will be just confusing. Please change that. > > We are not creating any genpds unless OSI mode is supported. We do not > even try to attach CPUs to the PM domains, unless OSI mode is > supported. So this should already work according to your expectations > and previous requests. > Yes I understand, but checking if "OSI mode is supported" is not same as "setting OSI mode". Until OSI mode is set, it is default/PC mode, so we need to work based on that assumption. > To address your concern about removing genpds when psci_set_osi_mode() > fails, we also need to address the problems we get when calling > psci_dt_attach_cpu(). There are two viable options as I see it. > Shouldn't that fail ? Sorry, I might be missing something. > 1. Prevent calling psci_dt_attach_cpu() altogether when > psci_set_osi_mode() failed. This means another function needs to be > shared from cpuidle-psci-domain.c to let cpuidle-psci.c know about it. > If we don't create any genpd, will psci_dt_attach_cpu fail ? > 2. We can let psci_dt_attach_cpu() return NULL, when > psci_set_osi_mode() failed - as this information is already known by > cpuidle-psci-domain.c. > Yes I was making all the arguments/discussion based on that. Do you see any issues with that ? Any races possible ? > I vote for option 2, but what do you think? > Me too from the time I started the discussion, I assume a lot and don't put this into words in the email. -- Regards, Sudeep
diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c index 656ef3d59149..c2f94ba42222 100644 --- a/drivers/cpuidle/cpuidle-psci-domain.c +++ b/drivers/cpuidle/cpuidle-psci-domain.c @@ -7,14 +7,281 @@ * */ +#define pr_fmt(fmt) "CPUidle PSCI: " fmt + #include <linux/cpu.h> #include <linux/device.h> #include <linux/kernel.h> #include <linux/pm_domain.h> #include <linux/pm_runtime.h> +#include <linux/psci.h> +#include <linux/slab.h> +#include <linux/string.h> #include "cpuidle-psci.h" +struct psci_pd_provider { + struct list_head link; + struct device_node *node; +}; + +static LIST_HEAD(psci_pd_providers); +static bool osi_mode_enabled; + +static int psci_pd_power_off(struct generic_pm_domain *pd) +{ + struct genpd_power_state *state = &pd->states[pd->state_idx]; + u32 *pd_state; + + /* If we have failed to enable OSI mode, then abort power off. */ + if (!osi_mode_enabled) + return -EBUSY; + + if (!state->data) + return 0; + + /* OSI mode is enabled, set the corresponding domain state. */ + pd_state = state->data; + psci_set_domain_state(*pd_state); + + return 0; +} + +static int __init psci_pd_parse_state_nodes(struct genpd_power_state *states, + int state_count) +{ + int i, ret; + u32 psci_state, *psci_state_buf; + + for (i = 0; i < state_count; i++) { + ret = psci_dt_parse_state_node(to_of_node(states[i].fwnode), + &psci_state); + if (ret) + goto free_state; + + psci_state_buf = kmalloc(sizeof(u32), GFP_KERNEL); + if (!psci_state_buf) { + ret = -ENOMEM; + goto free_state; + } + *psci_state_buf = psci_state; + states[i].data = psci_state_buf; + } + + return 0; + +free_state: + i--; + for (; i >= 0; i--) + kfree(states[i].data); + return ret; +} + +static int __init psci_pd_parse_states(struct device_node *np, + struct genpd_power_state **states, int *state_count) +{ + int ret; + + /* Parse the domain idle states. */ + ret = of_genpd_parse_idle_states(np, states, state_count); + if (ret) + return ret; + + /* Fill out the PSCI specifics for each found state. */ + ret = psci_pd_parse_state_nodes(*states, *state_count); + if (ret) + kfree(*states); + + return ret; +} + +static void psci_pd_free_states(struct genpd_power_state *states, + unsigned int state_count) +{ + int i; + + for (i = 0; i < state_count; i++) + kfree(states[i].data); + kfree(states); +} + +static int __init psci_pd_init(struct device_node *np) +{ + struct generic_pm_domain *pd; + struct psci_pd_provider *pd_provider; + struct dev_power_governor *pd_gov; + struct genpd_power_state *states = NULL; + int ret = -ENOMEM, state_count = 0; + + pd = kzalloc(sizeof(*pd), GFP_KERNEL); + if (!pd) + goto out; + + pd_provider = kzalloc(sizeof(*pd_provider), GFP_KERNEL); + if (!pd_provider) + goto free_pd; + + pd->name = kasprintf(GFP_KERNEL, "%pOF", np); + if (!pd->name) + goto free_pd_prov; + + /* + * Parse the domain idle states and let genpd manage the state selection + * for those being compatible with "domain-idle-state". + */ + ret = psci_pd_parse_states(np, &states, &state_count); + if (ret) + goto free_name; + + pd->free_states = psci_pd_free_states; + pd->name = kbasename(pd->name); + pd->power_off = psci_pd_power_off; + pd->states = states; + pd->state_count = state_count; + pd->flags |= GENPD_FLAG_IRQ_SAFE | GENPD_FLAG_CPU_DOMAIN; + + /* Use governor for CPU PM domains if it has some states to manage. */ + pd_gov = state_count > 0 ? &pm_domain_cpu_gov : NULL; + + ret = pm_genpd_init(pd, pd_gov, false); + if (ret) { + psci_pd_free_states(states, state_count); + goto free_name; + } + + ret = of_genpd_add_provider_simple(np, pd); + if (ret) + goto remove_pd; + + pd_provider->node = of_node_get(np); + list_add(&pd_provider->link, &psci_pd_providers); + + pr_debug("init PM domain %s\n", pd->name); + return 0; + +remove_pd: + pm_genpd_remove(pd); +free_name: + kfree(pd->name); +free_pd_prov: + kfree(pd_provider); +free_pd: + kfree(pd); +out: + pr_err("failed to init PM domain ret=%d %pOF\n", ret, np); + return ret; +} + +static void __init psci_pd_remove(void) +{ + struct psci_pd_provider *pd_provider, *it; + struct generic_pm_domain *genpd; + + list_for_each_entry_safe(pd_provider, it, &psci_pd_providers, link) { + of_genpd_del_provider(pd_provider->node); + + genpd = of_genpd_remove_last(pd_provider->node); + if (!IS_ERR(genpd)) + kfree(genpd); + + of_node_put(pd_provider->node); + list_del(&pd_provider->link); + kfree(pd_provider); + } +} + +static int __init psci_pd_init_topology(struct device_node *np) +{ + struct device_node *node; + struct of_phandle_args child, parent; + int ret; + + for_each_child_of_node(np, node) { + if (of_parse_phandle_with_args(node, "power-domains", + "#power-domain-cells", 0, &parent)) + continue; + + child.np = node; + child.args_count = 0; + + ret = of_genpd_add_subdomain(&parent, &child); + of_node_put(parent.np); + if (ret) { + of_node_put(node); + return ret; + } + } + + return 0; +} + +static const struct of_device_id psci_of_match[] __initconst = { + { .compatible = "arm,psci" }, + { .compatible = "arm,psci-0.2" }, + { .compatible = "arm,psci-1.0" }, + {} +}; + +static int __init psci_idle_init_domains(void) +{ + struct device_node *np = of_find_matching_node(NULL, psci_of_match); + struct device_node *node; + int ret = 0, pd_count = 0; + + if (!np) + return -ENODEV; + + /* Currently limit the hierarchical topology to be used in OSI mode. */ + if (!psci_has_osi_support()) + goto out; + + /* + * Parse child nodes for the "#power-domain-cells" property and + * initialize a genpd/genpd-of-provider pair when it's found. + */ + for_each_child_of_node(np, node) { + if (!of_find_property(node, "#power-domain-cells", NULL)) + continue; + + ret = psci_pd_init(node); + if (ret) + goto put_node; + + pd_count++; + } + + /* Bail out if not using the hierarchical CPU topology. */ + if (!pd_count) + goto out; + + /* Link genpd masters/subdomains to model the CPU topology. */ + ret = psci_pd_init_topology(np); + if (ret) + goto remove_pd; + + /* Try to enable OSI mode. */ + ret = psci_set_osi_mode(); + if (ret) + pr_warn("failed to enable OSI mode: %d\n", ret); + else + osi_mode_enabled = true; + + of_node_put(np); + pr_info("Initialized CPU PM domain topology\n"); + return pd_count; + +put_node: + of_node_put(node); +remove_pd: + if (pd_count) + psci_pd_remove(); + pr_err("failed to create CPU PM domains ret=%d\n", ret); +out: + of_node_put(np); + return ret; +} +subsys_initcall(psci_idle_init_domains); + struct device *psci_dt_attach_cpu(int cpu) { struct device *dev; diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c index 34a89d99bb0f..70141090a298 100644 --- a/drivers/cpuidle/cpuidle-psci.c +++ b/drivers/cpuidle/cpuidle-psci.c @@ -34,7 +34,7 @@ static DEFINE_PER_CPU_READ_MOSTLY(struct psci_cpuidle_data, psci_cpuidle_data); static DEFINE_PER_CPU(u32, domain_state); static bool psci_cpuidle_use_cpuhp; -static inline void psci_set_domain_state(u32 state) +void psci_set_domain_state(u32 state) { __this_cpu_write(domain_state, state); } @@ -143,7 +143,7 @@ static const struct of_device_id psci_idle_state_match[] __initconst = { { }, }; -static int __init psci_dt_parse_state_node(struct device_node *np, u32 *state) +int __init psci_dt_parse_state_node(struct device_node *np, u32 *state) { int err = of_property_read_u32(np, "arm,psci-suspend-param", state); diff --git a/drivers/cpuidle/cpuidle-psci.h b/drivers/cpuidle/cpuidle-psci.h index 0cadbb71dc55..d2e55cad9ac6 100644 --- a/drivers/cpuidle/cpuidle-psci.h +++ b/drivers/cpuidle/cpuidle-psci.h @@ -3,6 +3,11 @@ #ifndef __CPUIDLE_PSCI_H #define __CPUIDLE_PSCI_H +struct device_node; + +void psci_set_domain_state(u32 state); +int __init psci_dt_parse_state_node(struct device_node *np, u32 *state); + #ifdef CONFIG_PM_GENERIC_DOMAINS_OF struct device *psci_dt_attach_cpu(int cpu); #else