From: Ulf Hansson <ulf.hansson@linaro.org>
To: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Viresh Kumar <vireshk@kernel.org>, Nishanth Menon <nm@ti.com>,
Stephen Boyd <sboyd@kernel.org>,
Linux PM <linux-pm@vger.kernel.org>,
Vincent Guittot <vincent.guittot@linaro.org>,
Rafael Wysocki <rjw@rjwysocki.net>,
Niklas Cassel <niklas.cassel@linaro.org>,
Rajendra Nayak <rnayak@codeaurora.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH V3 08/10] OPP: Configure all required OPPs
Date: Thu, 25 Oct 2018 12:54:48 +0200 [thread overview]
Message-ID: <CAPDyKFraxWBURAcwMPspkS6OFvXOOeFm3L5G7Pj1tXgX0_A6vQ@mail.gmail.com> (raw)
In-Reply-To: <f49e46f8cd45f7458dfbaf0abac1d2941136ae19.1540446493.git.viresh.kumar@linaro.org>
On 25 October 2018 at 07:52, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> Now that all the infrastructure is in place to support multiple required
> OPPs, lets switch over to using it.
>
> A new internal routine _set_required_opps() takes care of updating
> performance state for all the required OPPs. With this the performance
> state updates are supported even when the end device needs to configure
> regulators as well, that wasn't the case earlier.
>
> The pstates were earlier stored in the end device's OPP structures, that
> also changes now as those values are stored in the genpd's OPP
> structures. And so we switch over to using
> pm_genpd_opp_to_performance_state() instead of
> of_genpd_opp_to_performance_state() to get performance state for the
> genpd OPPs.
>
> The routine _generic_set_opp_domain() is not required anymore and is
> removed.
>
> On errors we don't try to recover by reverting to old settings as things
> are really complex now and the calls here should never really fail
> unless there is a bug. There is no point increasing the complexity, for
> code which will never be executed.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Kind regards
Uffe
> ---
> drivers/opp/core.c | 113 ++++++++++++++++++++++++++-------------------
> drivers/opp/of.c | 5 +-
> 2 files changed, 68 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index cef2ccda355d..0eaa954b3f6c 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -548,44 +548,6 @@ _generic_set_opp_clk_only(struct device *dev, struct clk *clk,
> return ret;
> }
>
> -static inline int
> -_generic_set_opp_domain(struct device *dev, struct clk *clk,
> - unsigned long old_freq, unsigned long freq,
> - unsigned int old_pstate, unsigned int new_pstate)
> -{
> - int ret;
> -
> - /* Scaling up? Scale domain performance state before frequency */
> - if (freq > old_freq) {
> - ret = dev_pm_genpd_set_performance_state(dev, new_pstate);
> - if (ret)
> - return ret;
> - }
> -
> - ret = _generic_set_opp_clk_only(dev, clk, old_freq, freq);
> - if (ret)
> - goto restore_domain_state;
> -
> - /* Scaling down? Scale domain performance state after frequency */
> - if (freq < old_freq) {
> - ret = dev_pm_genpd_set_performance_state(dev, new_pstate);
> - if (ret)
> - goto restore_freq;
> - }
> -
> - return 0;
> -
> -restore_freq:
> - if (_generic_set_opp_clk_only(dev, clk, freq, old_freq))
> - dev_err(dev, "%s: failed to restore old-freq (%lu Hz)\n",
> - __func__, old_freq);
> -restore_domain_state:
> - if (freq > old_freq)
> - dev_pm_genpd_set_performance_state(dev, old_pstate);
> -
> - return ret;
> -}
> -
> static int _generic_set_opp_regulator(const struct opp_table *opp_table,
> struct device *dev,
> unsigned long old_freq,
> @@ -663,6 +625,56 @@ static int _set_opp_custom(const struct opp_table *opp_table,
> return opp_table->set_opp(data);
> }
>
> +/* This is only called for PM domain for now */
> +static int _set_required_opps(struct device *dev,
> + struct opp_table *opp_table,
> + struct dev_pm_opp *opp)
> +{
> + struct opp_table **required_opp_tables = opp_table->required_opp_tables;
> + struct device **genpd_virt_devs = opp_table->genpd_virt_devs;
> + unsigned int pstate;
> + int i, ret = 0;
> +
> + if (!required_opp_tables)
> + return 0;
> +
> + /* Single genpd case */
> + if (!genpd_virt_devs) {
> + pstate = opp->required_opps[0]->pstate;
> + ret = dev_pm_genpd_set_performance_state(dev, pstate);
> + if (ret) {
> + dev_err(dev, "Failed to set performance state of %s: %d (%d)\n",
> + dev_name(dev), pstate, ret);
> + }
> + return ret;
> + }
> +
> + /* Multiple genpd case */
> +
> + /*
> + * Acquire genpd_virt_dev_lock to make sure we don't use a genpd_dev
> + * after it is freed from another thread.
> + */
> + mutex_lock(&opp_table->genpd_virt_dev_lock);
> +
> + for (i = 0; i < opp_table->required_opp_count; i++) {
> + pstate = opp->required_opps[i]->pstate;
> +
> + if (!genpd_virt_devs[i])
> + continue;
> +
> + ret = dev_pm_genpd_set_performance_state(genpd_virt_devs[i], pstate);
> + if (ret) {
> + dev_err(dev, "Failed to set performance rate of %s: %d (%d)\n",
> + dev_name(genpd_virt_devs[i]), pstate, ret);
> + break;
> + }
> + }
> + mutex_unlock(&opp_table->genpd_virt_dev_lock);
> +
> + return ret;
> +}
> +
> /**
> * dev_pm_opp_set_rate() - Configure new OPP based on frequency
> * @dev: device for which we do this operation
> @@ -730,6 +742,13 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
> dev_dbg(dev, "%s: switching OPP: %lu Hz --> %lu Hz\n", __func__,
> old_freq, freq);
>
> + /* Scaling up? Configure required OPPs before frequency */
> + if (freq > old_freq) {
> + ret = _set_required_opps(dev, opp_table, opp);
> + if (ret)
> + goto put_opp;
> + }
> +
> if (opp_table->set_opp) {
> ret = _set_opp_custom(opp_table, dev, old_freq, freq,
> IS_ERR(old_opp) ? NULL : old_opp->supplies,
> @@ -740,19 +759,17 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
> opp->supplies);
> } else {
> /* Only frequency scaling */
> + ret = _generic_set_opp_clk_only(dev, clk, old_freq, freq);
> + }
>
> - /*
> - * We don't support devices with both regulator and
> - * domain performance-state for now.
> - */
> - if (opp_table->genpd_performance_state)
> - ret = _generic_set_opp_domain(dev, clk, old_freq, freq,
> - IS_ERR(old_opp) ? 0 : old_opp->pstate,
> - opp->pstate);
> - else
> - ret = _generic_set_opp_clk_only(dev, clk, old_freq, freq);
> + /* Scaling down? Configure required OPPs after frequency */
> + if (!ret && freq < old_freq) {
> + ret = _set_required_opps(dev, opp_table, opp);
> + if (ret)
> + dev_err(dev, "Failed to set required opps: %d\n", ret);
> }
>
> +put_opp:
> dev_pm_opp_put(opp);
> put_old_opp:
> if (!IS_ERR(old_opp))
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index 71aef28953c2..4e494720ac25 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -595,12 +595,13 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
> if (!of_property_read_u32(np, "clock-latency-ns", &val))
> new_opp->clock_latency_ns = val;
>
> - new_opp->pstate = of_genpd_opp_to_performance_state(dev, np);
> -
> ret = opp_parse_supplies(new_opp, dev, opp_table);
> if (ret)
> goto free_required_opps;
>
> + if (opp_table->is_genpd)
> + new_opp->pstate = pm_genpd_opp_to_performance_state(dev, new_opp);
> +
> ret = _opp_add(dev, new_opp, opp_table, rate_not_available);
> if (ret) {
> /* Don't return error for duplicate OPPs */
> --
> 2.19.1.568.g152ad8e3369a
>
next prev parent reply other threads:[~2018-10-25 10:54 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-10-25 5:52 [PATCH V3 00/10] OPP: Support multiple power-domains per device Viresh Kumar
2018-10-25 5:52 ` [PATCH V3 01/10] PM / Domains: Rename genpd virtual devices as virt_dev Viresh Kumar
2018-10-25 10:54 ` Ulf Hansson
2018-10-25 5:52 ` [PATCH V3 02/10] OPP: Identify and mark genpd OPP tables Viresh Kumar
2018-10-25 5:52 ` [PATCH V3 03/10] OPP: Separate out custom OPP handler specific code Viresh Kumar
2018-10-25 5:52 ` [PATCH V3 04/10] OPP: Populate required opp tables from "required-opps" property Viresh Kumar
2018-10-25 5:52 ` [PATCH V3 05/10] OPP: Populate OPPs " Viresh Kumar
2018-10-25 5:52 ` [PATCH V3 06/10] PM / Domains: Add genpd_opp_to_performance_state() Viresh Kumar
2018-10-25 10:54 ` Ulf Hansson
2018-10-25 5:52 ` [PATCH V3 07/10] OPP: Add dev_pm_opp_{set|put}_genpd_virt_dev() helper Viresh Kumar
2018-10-25 10:54 ` Ulf Hansson
2018-10-25 5:52 ` [PATCH V3 08/10] OPP: Configure all required OPPs Viresh Kumar
2018-10-25 10:54 ` Ulf Hansson [this message]
2018-10-25 5:52 ` [PATCH V3 09/10] OPP: Rename and relocate of_genpd_opp_to_performance_state() Viresh Kumar
2018-10-25 10:54 ` Ulf Hansson
2018-11-02 9:15 ` [PATCH V4 " Viresh Kumar
2018-10-25 5:52 ` [PATCH V3 10/10] OPP: Remove of_dev_pm_opp_find_required_opp() Viresh Kumar
2018-10-25 10:54 ` Ulf Hansson
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=CAPDyKFraxWBURAcwMPspkS6OFvXOOeFm3L5G7Pj1tXgX0_A6vQ@mail.gmail.com \
--to=ulf.hansson@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=niklas.cassel@linaro.org \
--cc=nm@ti.com \
--cc=rjw@rjwysocki.net \
--cc=rnayak@codeaurora.org \
--cc=sboyd@kernel.org \
--cc=vincent.guittot@linaro.org \
--cc=viresh.kumar@linaro.org \
--cc=vireshk@kernel.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).