LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Rajendra Nayak <rnayak@codeaurora.org>
To: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>,
	Viresh Kumar <viresh.kumar@linaro.org>,
	Linux PM <linux-pm@vger.kernel.org>,
	DTML <devicetree@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	Stephen Boyd <swboyd@chromium.org>,
	Roja Rani Yarubandi <rojay@codeaurora.org>,
	Stephan Gerhold <stephan@gerhold.net>
Subject: Re: [PATCH v6 1/2] PM / Domains: Add support for 'required-opps' to set default perf state
Date: Mon, 9 Aug 2021 16:38:50 +0530	[thread overview]
Message-ID: <2afd0fac-ed28-c090-a345-3fd4284b4125@codeaurora.org> (raw)
In-Reply-To: <CAPDyKFrebwt5=S7hqXvcqRvt+-EXLcVmRSRZt1uPf-9n7_pRDg@mail.gmail.com>



On 8/6/2021 3:02 PM, Ulf Hansson wrote:
> On Wed, 4 Aug 2021 at 12:58, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>>
>> Some devices within power domains with performance states do not
>> support DVFS, but still need to vote on a default/static state
>> while they are active. They can express this using the 'required-opps'
>> property in device tree, which points to the phandle of the OPP
>> supported by the corresponding power-domains.
>>
>> Add support to parse this information from DT and then set the
>> specified performance state during attach and drop it on detach.
>> runtime suspend/resume callbacks already have logic to drop/set
>> the vote as needed and should take care of dropping the default
>> perf state vote on runtime suspend and restore it back on runtime
>> resume.
>>
>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>> ---
>>   drivers/base/power/domain.c | 28 ++++++++++++++++++++++++++--
>>   include/linux/pm_domain.h   |  1 +
>>   2 files changed, 27 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index a934c67..b9b5a9b 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -2598,6 +2598,12 @@ static void genpd_dev_pm_detach(struct device *dev, bool power_off)
>>
>>          dev_dbg(dev, "removing from PM domain %s\n", pd->name);
>>
>> +       /* Drop the default performance state */
>> +       if (dev_gpd_data(dev)->default_pstate) {
>> +               dev_pm_genpd_set_performance_state(dev, 0);
>> +               dev_gpd_data(dev)->default_pstate = 0;
>> +       }
>> +
>>          for (i = 1; i < GENPD_RETRY_MAX_MS; i <<= 1) {
>>                  ret = genpd_remove_device(pd, dev);
>>                  if (ret != -EAGAIN)
>> @@ -2637,6 +2643,8 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
>>   {
>>          struct of_phandle_args pd_args;
>>          struct generic_pm_domain *pd;
>> +       struct device_node *np;
>> +       int pstate;
>>          int ret;
>>
>>          ret = of_parse_phandle_with_args(dev->of_node, "power-domains",
>> @@ -2675,10 +2683,26 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
>>                  genpd_unlock(pd);
>>          }
>>
>> -       if (ret)
>> +       if (ret) {
>>                  genpd_remove_device(pd, dev);
>> +               return -EPROBE_DEFER;
>> +       }
>> +
>> +       /* Set the default performance state */
>> +       np = dev->of_node;
>> +       if (of_parse_phandle(np, "required-opps", index)) {
> 
> Looks like Viresh thinks it's a good idea to drop the error print in
> of_get_required_opp_performance_state() when there is no
> "required-opps" specifier.
> 
> Would you mind folding in a patch for that in the series, so this code
> can be simplified according to our earlier discussions?

Sure, I can do that, apart from the error print, the function currently also
returns a -EINVAL in case of the missing 'required-opps', are we suggesting
we change that to not return an error also?

Since this is completely optional in the device node, we would want the function to
ideally not return error and only do so in case 'required-opps' exists and the
translation to performance state fails.
I am not sure that's the behavior we expect in case of 'required-opps' in the OPP
tables also, Viresh?

> 
>> +               pstate = of_get_required_opp_performance_state(np, index);
>> +               if (pstate < 0) {
>> +                       ret = pstate;
>> +                       dev_err(dev, "failed to set required performance state for power-domain %s: %d\n",
>> +                               pd->name, ret);
>> +               } else {
>> +                       dev_pm_genpd_set_performance_state(dev, pstate);
>> +                       dev_gpd_data(dev)->default_pstate = pstate;
>> +               }
>> +       }
>>
>> -       return ret ? -EPROBE_DEFER : 1;
>> +       return ret ? ret : 1;
>>   }
>>
>>   /**
>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>> index 21a0577..67017c9 100644
>> --- a/include/linux/pm_domain.h
>> +++ b/include/linux/pm_domain.h
>> @@ -198,6 +198,7 @@ struct generic_pm_domain_data {
>>          struct notifier_block *power_nb;
>>          int cpu;
>>          unsigned int performance_state;
>> +       unsigned int default_pstate;
>>          unsigned int rpm_pstate;
>>          ktime_t next_wakeup;
>>          void *data;
> 
> Other than the above, this looks good to me!
> 
> Kind regards
> Uffe
> 

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

  reply	other threads:[~2021-08-09 11:09 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-04 10:58 [PATCH v6 0/2] " Rajendra Nayak
2021-08-04 10:58 ` [PATCH v6 1/2] " Rajendra Nayak
2021-08-06  9:32   ` Ulf Hansson
2021-08-09 11:08     ` Rajendra Nayak [this message]
2021-08-10  2:43       ` Viresh Kumar
2021-08-11 10:00         ` Rajendra Nayak
2021-08-11 10:07           ` Viresh Kumar
2021-08-11 10:52             ` Ulf Hansson
2021-08-11 11:19               ` Viresh Kumar
2021-08-09 22:26   ` Dmitry Osipenko
2021-08-09 22:44     ` Dmitry Osipenko
2021-08-09 23:46       ` Dmitry Osipenko
2021-08-10  0:37         ` Dmitry Osipenko
2021-08-11  9:45         ` Rajendra Nayak
2021-08-11 19:48           ` Dmitry Osipenko
2021-08-12  2:28             ` Dmitry Osipenko
2021-08-04 10:58 ` [PATCH v6 2/2] arm64: dts: sc7180: Add required-opps for i2c Rajendra Nayak

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=2afd0fac-ed28-c090-a345-3fd4284b4125@codeaurora.org \
    --to=rnayak@codeaurora.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=devicetree@vger.kernel.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=rojay@codeaurora.org \
    --cc=stephan@gerhold.net \
    --cc=swboyd@chromium.org \
    --cc=ulf.hansson@linaro.org \
    --cc=viresh.kumar@linaro.org \
    --subject='Re: [PATCH v6 1/2] PM / Domains: Add support for '\''required-opps'\'' to set default perf state' \
    /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

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).