Re: [PATCH v3 1/3] PM: domains: Make set_performance_state() callback optional

[Dmitry Osipenko]

18.01.2021 13:59, Ulf Hansson пишет:
> On Mon, 18 Jan 2021 at 08:28, Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
>>
>> On 18-01-21, 04:13, Dmitry Osipenko wrote:
>>> Make set_performance_state() callback optional in order to remove the
>>> need from power domain drivers to implement a dummy callback. If callback
>>> isn't implemented by a GENPD driver, then the performance state is passed
>>> to the parent domain.
>>>
>>> Tested-by: Peter Geis <pgwipeout@xxxxxxxxx>
>>> Tested-by: Nicolas Chauvet <kwizart@xxxxxxxxx>
>>> Tested-by: Matt Merhar <mattmerhar@xxxxxxxxxxxxxx>
>>> Suggested-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
>>> Reviewed-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
>>> Signed-off-by: Dmitry Osipenko <digetx@xxxxxxxxx>
>>> ---
>>>  drivers/base/power/domain.c | 11 +++++------
>>>  1 file changed, 5 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>>> index 9a14eedacb92..a3e1bfc233d4 100644
>>> --- a/drivers/base/power/domain.c
>>> +++ b/drivers/base/power/domain.c
>>> @@ -339,9 +339,11 @@ static int _genpd_set_performance_state(struct generic_pm_domain *genpd,
>>>                       goto err;
>>>       }
>>>
>>> -     ret = genpd->set_performance_state(genpd, state);
>>> -     if (ret)
>>> -             goto err;
>>> +     if (genpd->set_performance_state) {
>>> +             ret = genpd->set_performance_state(genpd, state);
>>> +             if (ret)
>>> +                     goto err;
>>> +     }
>>
>> Earlier in this routine we also have this:
>>
>> if (!parent->set_performance_state)
>>         continue;
>>
>> Should we change that too ?
> 
> Good point! I certainly overlooked that when reviewing. We need to
> reevaluate the new state when propagating to the parent(s).
> 
> To me, it looks like when doing the propagation we must check if the
> parent has the ->set_performance_state() callback assigned. If so, we
> should call dev_pm_opp_xlate_performance_state(), but otherwise just
> use the value of "state", when doing the reevaluation.
> 
> Does it make sense?

I played a tad with the power domains by creating a couple dummy domains
and putting them into a parent->child chain. Yours suggestion works well
for the case where intermediate domain doesn't implement the
set_performance_state() callback, i.e. the performance state is
propagated through the whole chain instead of stopping on the domain
that doesn't implement the callback.

I'll make a v4, thanks.