LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
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

  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] " 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 \
    --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).