diff mbox series

[v6,1/2] PM / Domains: Add support for 'required-opps' to set default perf state

Message ID 1628074696-7979-2-git-send-email-rnayak@codeaurora.org
State New
Headers show
Series [v6,1/2] PM / Domains: Add support for 'required-opps' to set default perf state | expand

Commit Message

Rajendra Nayak Aug. 4, 2021, 10:58 a.m. UTC
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(-)

Comments

Ulf Hansson Aug. 6, 2021, 9:32 a.m. UTC | #1
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?

> +               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
Rajendra Nayak Aug. 9, 2021, 11:08 a.m. UTC | #2
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
>
Dmitry Osipenko Aug. 9, 2021, 10:26 p.m. UTC | #3
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)) {
> +		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;
> +		}
> +	}

Why performance state is set after genpd_power_on()?
Dmitry Osipenko Aug. 9, 2021, 10:44 p.m. UTC | #4
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)) {

>> +		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);


Where is error handling?

>> +			dev_gpd_data(dev)->default_pstate = pstate;

>> +		}

>> +	}

> 

> Why performance state is set after genpd_power_on()?

>
Dmitry Osipenko Aug. 9, 2021, 11:46 p.m. UTC | #5
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.

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

>>> +		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?
> 
>>> +			dev_gpd_data(dev)->default_pstate = pstate;
>>> +		}
>>> +	}
>>
>> Why performance state is set after genpd_power_on()?

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;
}
Dmitry Osipenko Aug. 10, 2021, 12:37 a.m. UTC | #6
10.08.2021 02:46, Dmitry Osipenko пишет:
> 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.
> 
>>>> +		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.
> 
>>>> +		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?
>>
>>>> +			dev_gpd_data(dev)->default_pstate = pstate;
>>>> +		}
>>>> +	}
>>>
>>> Why performance state is set after genpd_power_on()?
> 
> 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;
> }
> 

Although, thinking a bit more about this, it should be better to skip
setting perf state in a case of power_on=false and let drivers to handle
it, IMO. Too much trouble with it for the core.
Viresh Kumar Aug. 10, 2021, 2:43 a.m. UTC | #7
On 09-08-21, 16:38, Rajendra Nayak wrote:
> 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?

No.

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

Not really. The function should return failure if the property isn't
there, but it shouldn't be EINVAL but ENODEV.
Rajendra Nayak Aug. 11, 2021, 9:45 a.m. UTC | #8
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
Viresh Kumar Aug. 11, 2021, 10:07 a.m. UTC | #9
On 11-08-21, 15:30, Rajendra Nayak wrote:
> In my case I don't want to error out if the property is missing, I want to error out

> only when the property exists but can't be translated into a performance state.

> 

> So currently I check if the property exists and *only then* try to translate it, Ulf asked

> me to skip the check. If I do that and I call of_get_required_opp_performance_state()

> unconditionally, and if it errors out I will need to put in additional logic (check for

> return value of ENODEV) to distinguish between the property-does-not-exist vs

> property-exists-but-cannot-be-translated case.

> It just seems more straight-forward to call this only when the property exists, Ulf?


The same check will be done by OPP core as well, so it is better to
optimize for the success case here. I will say, don't error out on
ENODEV, rest you know well.

-- 
viresh
Ulf Hansson Aug. 11, 2021, 10:52 a.m. UTC | #10
On Wed, 11 Aug 2021 at 12:07, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>

> On 11-08-21, 15:30, Rajendra Nayak wrote:

> > In my case I don't want to error out if the property is missing, I want to error out

> > only when the property exists but can't be translated into a performance state.

> >

> > So currently I check if the property exists and *only then* try to translate it, Ulf asked

> > me to skip the check. If I do that and I call of_get_required_opp_performance_state()

> > unconditionally, and if it errors out I will need to put in additional logic (check for

> > return value of ENODEV) to distinguish between the property-does-not-exist vs

> > property-exists-but-cannot-be-translated case.

> > It just seems more straight-forward to call this only when the property exists, Ulf?

>

> The same check will be done by OPP core as well, so it is better to

> optimize for the success case here. I will say, don't error out on

> ENODEV, rest you know well.


This should work, while I generally favor not having to parse for
specific return codes.

Another option is to add a new OPP OF helperfunction that just informs
the caller whether the required-opps property exists (instead of
open-coding that part), and if so, the caller can continue with
of_get_required_opp_performance_state() and then expect it to succeed.

I have no strong opinion though! Whatever works for me.

Kind regards
Uffe
Viresh Kumar Aug. 11, 2021, 11:19 a.m. UTC | #11
On 11-08-21, 12:52, Ulf Hansson wrote:
> On Wed, 11 Aug 2021 at 12:07, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 11-08-21, 15:30, Rajendra Nayak wrote:
> > > In my case I don't want to error out if the property is missing, I want to error out
> > > only when the property exists but can't be translated into a performance state.
> > >
> > > So currently I check if the property exists and *only then* try to translate it, Ulf asked
> > > me to skip the check. If I do that and I call of_get_required_opp_performance_state()
> > > unconditionally, and if it errors out I will need to put in additional logic (check for
> > > return value of ENODEV) to distinguish between the property-does-not-exist vs
> > > property-exists-but-cannot-be-translated case.
> > > It just seems more straight-forward to call this only when the property exists, Ulf?
> >
> > The same check will be done by OPP core as well, so it is better to
> > optimize for the success case here. I will say, don't error out on
> > ENODEV, rest you know well.
> 
> This should work, while I generally favor not having to parse for
> specific return codes.
> 
> Another option is to add a new OPP OF helperfunction that just informs
> the caller whether the required-opps property exists (instead of
> open-coding that part), and if so, the caller can continue with
> of_get_required_opp_performance_state() and then expect it to succeed.
> 
> I have no strong opinion though! Whatever works for me.

I would like to work with the existing set of APIs, as the OPP core is
going to do that check anyways, again.
Dmitry Osipenko Aug. 11, 2021, 7:48 p.m. UTC | #12
11.08.2021 12:45, Rajendra Nayak пишет:
>>
>> 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.

The domain core code supports only one domain per device, see what
genpd_set_performance_state() does. This means that the second domain
will set the state of the *first* domain, which doesn't make sense. The
genpd_set_performance_state() will actually fail with -ENODEV for all
domains if you will try to do that.
Dmitry Osipenko Aug. 12, 2021, 2:28 a.m. UTC | #13
11.08.2021 22:48, Dmitry Osipenko пишет:
> 11.08.2021 12:45, Rajendra Nayak пишет:
>>>
>>> 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.
> 
> The domain core code supports only one domain per device, see what
> genpd_set_performance_state() does. This means that the second domain
> will set the state of the *first* domain, which doesn't make sense. The
> genpd_set_performance_state() will actually fail with -ENODEV for all
> domains if you will try to do that.
> 

I confused the base device with the virtual device there, looks like it
should be okay then.
diff mbox series

Patch

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)) {
+		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;