11.08.2021 22:30, Dmitry Osipenko пишет:

10.08.2021 13:51, Ulf Hansson пишет:
...

quoted

quoted

+               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

quoted

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

quoted

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.

quoted

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

...

quoted

quoted

quoted

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.