From: Rajendra Nayak <rnayak@codeaurora.org>
To: Dmitry Osipenko <digetx@gmail.com>,
ulf.hansson@linaro.org, bjorn.andersson@linaro.org,
viresh.kumar@linaro.org
Cc: linux-pm@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
swboyd@chromium.org, rojay@codeaurora.org, stephan@gerhold.net
Subject: Re: [PATCH v6 1/2] PM / Domains: Add support for 'required-opps' to set default perf state
Date: Wed, 11 Aug 2021 15:15:37 +0530 [thread overview]
Message-ID: <5f676dbe-6b8a-d302-cd81-2d12fe089e5a@codeaurora.org> (raw)
In-Reply-To: <9588f01d-1cb6-7ca3-eb41-74259f1678f6@gmail.com>
On 8/10/2021 5:16 AM, Dmitry Osipenko wrote:
> 10.08.2021 01:44, Dmitry Osipenko пишет:
>> 10.08.2021 01:26, Dmitry Osipenko пишет:
>>> 04.08.2021 13:58, Rajendra Nayak пишет:
>>>> @@ -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)) {
>
> The OF node returned by of_parse_phandle() must be put.
Thanks for spotting that, I will fix it, though I might just drop
the use of of_parse_phandle() and call of_get_required_opp_performance_state()
unconditionally as suggested by Ulf.
>
>>>> + pstate = of_get_required_opp_performance_state(np, index);
>
> If you have more than one power domain, then this will override the
> pstate which was set for a previous domain. This code doesn't feel solid
> to me, at least a clarifying comment is needed about how it's supposed
> to work.
I don't quite understand the concern here, this should work with devices
having multiple power-domains as well. __genpd_dev_pm_attach gets called
once per power-domain, and we use the index above to identify the power-domain.
>>>> + 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);
>>
>> Where is error handling?
Sure, I'll fix that.
>>
>>>> + dev_gpd_data(dev)->default_pstate = pstate;
>>>> + }
>>>> + }
>>>
>>> Why performance state is set after genpd_power_on()?
The requirement is that the domain is operating at a given performance
state before the device becomes operational. The driver should ideally wait
for the attach to complete before expecting the device to be functional.
So the order here should really not matter, or rather the small amount of time
while the power-domain is on but not at the right performance state should
not matter if that's the concern.
>
> Should set_performance_state() be invoked when power_on=false?
>
> I assume it should be:
>
> if (power_on) {
> dev_pm_genpd_set_performance_state(dev, pstate);
> dev_gpd_data(dev)->default_pstate = pstate;
> } else {
> dev_gpd_data(dev)->rpm_pstate = pstate;
> }
>
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
next prev parent reply other threads:[~2021-08-11 9:45 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-04 10:58 [PATCH v6 0/2] PM / Domains: Add support for 'required-opps' to set default perf state 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
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 [this message]
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=5f676dbe-6b8a-d302-cd81-2d12fe089e5a@codeaurora.org \
--to=rnayak@codeaurora.org \
--cc=bjorn.andersson@linaro.org \
--cc=devicetree@vger.kernel.org \
--cc=digetx@gmail.com \
--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 \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.