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

[Ulf Hansson]

On Thu, 12 Aug 2021 at 03:40, Dmitry Osipenko <digetx@xxxxxxxxx> wrote:
>
> 12.08.2021 01:41, Dmitry Osipenko пишет:
> >>> 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.
>
> GENPD core has these false assumptions:
>
> 1. It assumes that by default all devices are at zero performance level
> at a boot time. This is not true for Tegra because hardware is
> pre-initialized independently from GENPD.

Right, which is similar to other SoCs.

>
> 2. It assumes that nothing depends on performance level and devices can
> operate at any level at any time. Not true for Tegra and other platforms
> where performance level is coupled with clocks state of attached
> devices. OPP framework glues clock and performance level together for
> us, which works good so far.

Right, OPPs need to be managed differently depending on the SoC.
That's why genpd is there to help and to model this as "performance
states" and to allow operations to be set through SoC specific
callabacks, for example.

More importantly, the assumption is that in general, consumer drivers
should use the OPP library to vote/set OPP levels, they shouldn't call
dev_pm_genpd_set_performance_state() - unless they know exactly what
they are doing.

>
> Hence I either need to patch every driver to use dev_pm_opp_set_rate in
> order to sync clk rate with the perf level at device runtime, or I need
> to preset the rpm perf level to allow GENPD core to set the level before
> device is resumed.

When the device is getting attached to its genpd (during the probe
sequence and for a single PM domain case), runtime PM is disabled for
the device. If you would call dev_pm_opp_set_rate() from a genpd
callback during attach (without changing the rate), this means you
would update/synchronize the vote. In this way, the vote is set before
the device is runtime resumed by the driver, right?

On the other hand, patching the driver should also be quite simple and
you need to do that anyways, don't you?

Kind regards
Uffe