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

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