Message-ID: <c412f7e2-9f02-5cf6-e094-b5fa9a44169a@gmail.com>
Date:   Thu, 12 Aug 2021 19:24:29 +0300
From:   Dmitry Osipenko <digetx@...il.com>
To:     Ulf Hansson <ulf.hansson@...aro.org>
Cc:     Thierry Reding <thierry.reding@...il.com>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Viresh Kumar <vireshk@...nel.org>,
        Stephen Boyd <sboyd@...nel.org>,
        Peter De Schrijver <pdeschrijver@...dia.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-tegra <linux-tegra@...r.kernel.org>,
        Linux PM <linux-pm@...r.kernel.org>
Subject: Re: [PATCH v7 02/37] soc/tegra: pmc: Implement attach_dev() of power
 domain drivers

12.08.2021 14:17, Ulf Hansson пишет:
> On Thu, 12 Aug 2021 at 03:40, Dmitry Osipenko <digetx@...il.com> 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?

I was doing that previously, but then realized that for some devices the
state needs to be synced only once device is rpm-resumed. Making RPM
driver to perform the syncing worked well.

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

It will be extra boilerplate code in the drivers, but perhaps it's the
best option for today, I'll consider it for v8. Thank you for helping
with the review.

Powered by blists - more mailing lists