From: Sudeep Holla <sudeep.holla@arm.com>
To: Maulik Shah <mkshah@codeaurora.org>
Cc: swboyd@chromium.org, agross@kernel.org, david.brown@linaro.org,
Lorenzo.Pieralisi@arm.com, linux-arm-msm@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, bjorn.andersson@linaro.org,
evgreen@chromium.org, dianders@chromium.org,
rnayak@codeaurora.org, ilina@codeaurora.org,
lsrao@codeaurora.org, ulf.hansson@linaro.org, rjw@rjwysocki.net,
Sudeep Holla <sudeep.holla@arm.com>
Subject: Re: [PATCH v3 5/7] drivers: firmware: psci: Add hierarchical domain idle states converter
Date: Mon, 3 Feb 2020 17:08:32 +0000 [thread overview]
Message-ID: <20200203170832.GA38466@bogus> (raw)
In-Reply-To: <1580736940-6985-6-git-send-email-mkshah@codeaurora.org>
On Mon, Feb 03, 2020 at 07:05:38PM +0530, Maulik Shah wrote:
> From: Ulf Hansson <ulf.hansson@linaro.org>
>
> If the hierarchical CPU topology is used, but the OS initiated mode isn't
> supported, we need to rely solely on the regular cpuidle framework to
> manage the idle state selection, rather than using genpd and its
> governor.
>
> For this reason, introduce a new PSCI DT helper function,
> psci_dt_pm_domains_parse_states(), which parses and converts the
> hierarchically described domain idle states from DT, into regular flattened
> cpuidle states. The converted states are added to the existing cpuidle
> driver's array of idle states, which make them available for cpuidle.
>
And what's the main motivation for this if OSI is not supported in the
firmware ?
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> [applied to new path, resolved conflicts]
> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
> ---
> drivers/cpuidle/cpuidle-psci-domain.c | 137 +++++++++++++++++++++++++++++-----
> drivers/cpuidle/cpuidle-psci.c | 41 +++++-----
> drivers/cpuidle/cpuidle-psci.h | 11 +++
> 3 files changed, 153 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
> index 423f03b..3c417f7 100644
> --- a/drivers/cpuidle/cpuidle-psci-domain.c
> +++ b/drivers/cpuidle/cpuidle-psci-domain.c
> @@ -26,13 +26,17 @@ struct psci_pd_provider {
> };
>
> static LIST_HEAD(psci_pd_providers);
> -static bool osi_mode_enabled __initdata;
> +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 ((psci_has_osi_support()) && !osi_mode_enabled)
> + return -EBUSY;
> +
Why is this needed ? IIUC we don't create genpd domains if OSI is not
enabled.
> if (!state->data)
> return 0;
>
> @@ -101,6 +105,105 @@ static void psci_pd_free_states(struct genpd_power_state *states,
> kfree(states);
> }
>
> +static void psci_pd_convert_states(struct cpuidle_state *idle_state,
> + u32 *psci_state, struct genpd_power_state *state)
> +{
> + u32 *state_data = state->data;
> + u64 target_residency_us = state->residency_ns;
> + u64 exit_latency_us = state->power_on_latency_ns +
> + state->power_off_latency_ns;
> +
> + *psci_state = *state_data;
> + do_div(target_residency_us, 1000);
> + idle_state->target_residency = target_residency_us;
> + do_div(exit_latency_us, 1000);
> + idle_state->exit_latency = exit_latency_us;
> + idle_state->enter = &psci_enter_domain_idle_state;
> + idle_state->flags |= CPUIDLE_FLAG_TIMER_STOP;
> +
> + strncpy(idle_state->name, to_of_node(state->fwnode)->name,
> + CPUIDLE_NAME_LEN - 1);
> + strncpy(idle_state->desc, to_of_node(state->fwnode)->name,
> + CPUIDLE_NAME_LEN - 1);
> +}
> +
> +static bool psci_pd_is_provider(struct device_node *np)
> +{
> + struct psci_pd_provider *pd_prov, *it;
> +
> + list_for_each_entry_safe(pd_prov, it, &psci_pd_providers, link) {
> + if (pd_prov->node == np)
> + return true;
> + }
> +
> + return false;
> +}
> +
> +int __init psci_dt_pm_domains_parse_states(struct cpuidle_driver *drv,
> + struct device_node *cpu_node, u32 *psci_states)
> +{
> + struct genpd_power_state *pd_states;
> + struct of_phandle_args args;
> + int ret, pd_state_count, i, state_idx, psci_idx;
> + u32 cpu_psci_state = psci_states[drv->state_count - 1];
> + struct device_node *np = of_node_get(cpu_node);
> +
> + /* Walk the CPU topology to find compatible domain idle states. */
> + while (np) {
> + ret = of_parse_phandle_with_args(np, "power-domains",
> + "#power-domain-cells", 0, &args);
> + of_node_put(np);
> + if (ret)
> + return 0;
> +
> + np = args.np;
> +
> + /* Verify that the node represents a psci pd provider. */
> + if (!psci_pd_is_provider(np)) {
> + of_node_put(np);
> + return 0;
> + }
> +
> + /* Parse for compatible domain idle states. */
> + ret = psci_pd_parse_states(np, &pd_states, &pd_state_count);
> + if (ret) {
> + of_node_put(np);
> + return ret;
> + }
> +
> + i = 0;
> + while (i < pd_state_count) {
> +
> + state_idx = drv->state_count;
> + if (state_idx >= CPUIDLE_STATE_MAX) {
> + pr_warn("exceeding max cpuidle states\n");
> + of_node_put(np);
> + return 0;
> + }
> +
> + psci_idx = state_idx + i;
> + psci_pd_convert_states(&drv->states[state_idx + i],
> + &psci_states[psci_idx], &pd_states[i]);
> +
> + /*
> + * In the hierarchical CPU topology the master PM domain
> + * idle state's DT property, "arm,psci-suspend-param",
> + * don't contain the bits for the idle state of the CPU,
> + * let's add those here.
> + */
> + psci_states[psci_idx] |= cpu_psci_state;
No we can't do that. Refer previous discussions around that.
> + pr_debug("psci-power-state %#x index %d\n",
> + psci_states[psci_idx], psci_idx);
> +
> + drv->state_count++;
> + i++;
> + }
> + psci_pd_free_states(pd_states, pd_state_count);
> + }
> +
> + return 0;
> +}
> +
> static int __init psci_pd_init(struct device_node *np)
> {
> struct generic_pm_domain *pd;
> @@ -125,11 +228,14 @@ static int __init psci_pd_init(struct device_node *np)
> * 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;
> + if (psci_has_osi_support()) {
> + 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;
> @@ -236,10 +342,6 @@ static int __init psci_idle_init_domains(void)
> 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.
> @@ -265,14 +367,16 @@ static int __init psci_idle_init_domains(void)
> 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);
> - psci_pd_remove_topology(np);
> - goto remove_pd;
> + if (psci_has_osi_support()) {
> + ret = psci_set_osi_mode();
> + if (ret) {
> + pr_warn("failed to enable OSI mode: %d\n", ret);
> + psci_pd_remove_topology(np);
> + goto remove_pd;
> + } else
> + osi_mode_enabled = true;
> }
>
> - osi_mode_enabled = true;
> of_node_put(np);
> pr_info("Initialized CPU PM domain topology\n");
> return pd_count;
> @@ -293,9 +397,6 @@ struct device __init *psci_dt_attach_cpu(int cpu)
> {
> struct device *dev;
>
> - if (!osi_mode_enabled)
> - return NULL;
> -
> dev = dev_pm_domain_attach_by_name(get_cpu_device(cpu), "psci");
> if (IS_ERR_OR_NULL(dev))
> return dev;
> diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> index edd7a54..3fa2aee 100644
> --- a/drivers/cpuidle/cpuidle-psci.c
> +++ b/drivers/cpuidle/cpuidle-psci.c
> @@ -49,7 +49,7 @@ static inline int psci_enter_state(int idx, u32 state)
> return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter, idx, state);
> }
>
> -static int psci_enter_domain_idle_state(struct cpuidle_device *dev,
> +int psci_enter_domain_idle_state(struct cpuidle_device *dev,
> struct cpuidle_driver *drv, int idx)
> {
> struct psci_cpuidle_data *data = this_cpu_ptr(&psci_cpuidle_data);
> @@ -193,24 +193,29 @@ static int __init psci_dt_cpu_init_idle(struct cpuidle_driver *drv,
> goto free_mem;
> }
>
> - /* Currently limit the hierarchical topology to be used in OSI mode. */
> - if (psci_has_osi_support()) {
> - data->dev = psci_dt_attach_cpu(cpu);
> - if (IS_ERR(data->dev)) {
> - ret = PTR_ERR(data->dev);
> + if (!psci_has_osi_support()) {
> + ret = psci_dt_pm_domains_parse_states(drv, cpu_node,
> + psci_states);
> + if (ret)
> goto free_mem;
> - }
> -
> - /*
> - * Using the deepest state for the CPU to trigger a potential
> - * selection of a shared state for the domain, assumes the
> - * domain states are all deeper states.
> - */
> - if (data->dev) {
> - drv->states[state_count - 1].enter =
> - psci_enter_domain_idle_state;
> - psci_cpuidle_use_cpuhp = true;
> - }
> + }
> +
> + data->dev = psci_dt_attach_cpu(cpu);
> + if (IS_ERR(data->dev)) {
> + ret = PTR_ERR(data->dev);
> + goto free_mem;
> + }
> +
> + /*
> + * Using the deepest state for the CPU to trigger a potential
> + * selection of a shared state for the domain, assumes the
> + * domain states are all deeper states.
> + */
> +
> + if (data->dev) {
> + drv->states[state_count - 1].enter =
> + psci_enter_domain_idle_state;
> + psci_cpuidle_use_cpuhp = true;
> }
>
> /* Idle states parsed correctly, store them in the per-cpu struct. */
--
Regards,
Sudeep
next prev parent reply other threads:[~2020-02-03 17:08 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-03 13:35 [PATCH v3 0/7] Add RSC power domain support Maulik Shah
2020-02-03 13:35 ` [PATCH v3 1/7] drivers: qcom: rpmh: fix macro to accept NULL argument Maulik Shah
2020-02-03 13:35 ` [PATCH v3 2/7] drivers: qcom: rpmh: remove rpmh_flush export Maulik Shah
2020-02-03 13:35 ` [PATCH v3 3/7] dt-bindings: soc: qcom: Add RSC power domain specifier Maulik Shah
2020-02-03 13:35 ` [PATCH v3 4/7] drivers: qcom: rpmh-rsc: Add RSC power domain support Maulik Shah
2020-02-03 13:35 ` [PATCH v3 5/7] drivers: firmware: psci: Add hierarchical domain idle states converter Maulik Shah
2020-02-03 17:08 ` Sudeep Holla [this message]
2020-02-04 4:52 ` Maulik Shah
2020-02-04 15:21 ` Sudeep Holla
2020-02-05 12:23 ` Maulik Shah
2020-02-05 14:06 ` Sudeep Holla
2020-02-05 15:55 ` Ulf Hansson
2020-02-05 16:18 ` Sudeep Holla
2020-02-06 8:45 ` Ulf Hansson
2020-02-06 20:45 ` Lina Iyer
2020-02-07 11:20 ` Sudeep Holla
2020-02-07 12:32 ` Ulf Hansson
2020-02-07 14:48 ` Lorenzo Pieralisi
2020-02-07 15:52 ` Ulf Hansson
2020-02-07 16:15 ` Sudeep Holla
2020-02-08 10:25 ` Ulf Hansson
2020-02-10 10:31 ` Sudeep Holla
2020-02-07 16:05 ` Sudeep Holla
2020-02-06 21:11 ` Bjorn Andersson
2020-02-07 11:25 ` Sudeep Holla
2020-02-03 13:35 ` [PATCH v3 6/7] arm64: dts: qcom: sc7180: Add cpuidle low power states Maulik Shah
2020-02-04 23:15 ` Matthias Kaehlcke
2020-02-05 12:07 ` Maulik Shah
2020-02-03 13:35 ` [PATCH v3 7/7] arm64: dts: qcom: sc7180: Convert to the hierarchical CPU topology layout Maulik Shah
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200203170832.GA38466@bogus \
--to=sudeep.holla@arm.com \
--cc=Lorenzo.Pieralisi@arm.com \
--cc=agross@kernel.org \
--cc=bjorn.andersson@linaro.org \
--cc=david.brown@linaro.org \
--cc=dianders@chromium.org \
--cc=evgreen@chromium.org \
--cc=ilina@codeaurora.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=lsrao@codeaurora.org \
--cc=mkshah@codeaurora.org \
--cc=rjw@rjwysocki.net \
--cc=rnayak@codeaurora.org \
--cc=swboyd@chromium.org \
--cc=ulf.hansson@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).