Re: [PATCH v7 02/37] soc/tegra: pmc: Implement attach_dev() of power domain drivers

[Dmitry Osipenko <digetx@gmail.com>]

11.08.2021 22:30, Dmitry Osipenko пишет:
> 10.08.2021 13:51, Ulf Hansson пишет:
> ...
>>> +               if (power_on) {
>>> +                       ret = dev_pm_genpd_set_performance_state(dev, default_pstate);
>>
>> However, this is more questionable to me.
>>
>> First, I don't think we should care about whether this is "power_on"
>> or not. At this point, performance states are treated orthogonal to
>> idle states in genpd. We may decide to change that in some way, but
>> that deserves a different change.
> 
> Okay
> 
>> Second, I don't think we should call
>> dev_pm_genpd_set_performance_state() from here. It's probably better
>> handled from the genpd callback itself, if/when needed.
> Indeed
> 
>> That said, perhaps the new callback should just return a regular error
>> code and zero on success, rather than the current performance state.
>> See more below.
>>
>>> +                       if (ret) {
>>> +                               dev_err(dev, "failed to set default performance state %u for PM domain %s: %d\n",
>>> +                                       default_pstate, pd->name, ret);
>>> +                               goto out;
>>> +                       }
>>> +               } else {
>>> +                       dev_gpd_data(dev)->rpm_pstate = default_pstate;
>>
>> No, this isn't the right thing to do.
>>
>> It looks like you are trying to use the ->rpm_pstate for
>> synchronization with runtime PM for consumer drivers. This is fragile
>> as it depends on the runtime PM deployment in the consumer driver. I
>> think you should look at ->rpm_pstate as a variable solely for
>> managing save/restore of the performance state for the device, during
>> runtime suspend/resume in genpd.
>>
>> Synchronization of a vote for a performance state for a device, needs
>> to be managed by calling dev_pm_genpd_set_performance_state() - or by
>> calling an OPP function that calls it, like dev_pm_opp_set_rate(), for
>> example.
> 
> The !power_on case should be skipped at all because common core code
> can't handle it properly, hence rpm_pstate shouldn't be touched. I
> realized it only after sending out this email.
> 
> ...
>>>> Rather than doing the OF parsing above to find out the current state
>>>> for the device, why can't you just call dev_pm_opp_set_rate() to
>>>> initialize a proper vote instead?
>>>>
>>>
>>> For some devices clock rate is either preset by bootloader, or by clk driver, or by assigned-clocks in a device-tree. And then I don't see what's the difference in comparison to initialization for the current rate.
>>>
>>> For some devices, like memory controller, we can't just change the clock rate because it's a complex procedure and some boards will use fixed rate, but the power vote still must be initialized.
>>
>> I am not saying you should change the clock rate. The current code
>> path that runs via devm_tegra_core_dev_init_opp_table() just calls
>> clk_get_rate and then dev_pm_opp_set_rate() with the current rate to
>> vote for the corresponding OPP level. Right?
>>
>> Isn't this exactly what you want? No?
> 
> I see now what you meant, it's actually a simpler variant and it works
> too. Thank you for the suggestion, I'll prepare v8.
> 

My bad, it doesn't work at all. I actually need to use the rpm_pstate or
something else because performance state is coupled with the enable
state of the device. If device is never rpm-suspended by consumer
driver, then the initialized performance state is never dropped. Hence I
want to initialize the state which is set only when device is resumed.

I'll need to think more about it.