Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Date:   Mon, 30 Oct 2023 15:59:44 +0530
From:   Viresh Kumar <viresh.kumar@linaro.org>
To:     Ulf Hansson <ulf.hansson@linaro.org>
Cc:     Stephan Gerhold <stephan@gerhold.net>,
        Viresh Kumar <vireshk@kernel.org>, Nishanth Menon <nm@ti.com>,
        Stephen Boyd <sboyd@kernel.org>, linux-pm@vger.kernel.org,
        Vincent Guittot <vincent.guittot@linaro.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Stephan Gerhold <stephan.gerhold@kernkonzept.com>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] OPP: Use _set_opp_level() for single genpd case
Message-ID: <20231030102944.nrw4bta467zxes5c@vireshk-i7>
References: <cover.1697710527.git.viresh.kumar@linaro.org>
 <f709e9e273004be43efe3a2854a7e7b51a777f99.1697710527.git.viresh.kumar@linaro.org>
 <CAPDyKFqbnsdT0nqKwQhai875CwwpW_vepr816fL+i8yLh=YQhw@mail.gmail.com>
 <20231025065458.z3klmhahrcqh6qyw@vireshk-i7>
 <CAPDyKFr4vdsKVYEx0aF5k_a1bTjp3NzMpNgaXDJOJrvujT7iRg@mail.gmail.com>
 <ZTkciw5AwufxQYnB@gerhold.net>
 <CAPDyKFq+zsoeF-4h5TfT4Z+S46a501_pUq8y2c1x==Tt6EKBGA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAPDyKFq+zsoeF-4h5TfT4Z+S46a501_pUq8y2c1x==Tt6EKBGA@mail.gmail.com>
Precedence: bulk

On 26-10-23, 11:53, Ulf Hansson wrote:
> On Wed, 25 Oct 2023 at 15:49, Stephan Gerhold <stephan@gerhold.net> wrote:
> >  2. The OPP WARNing triggers with both variants because it just checks
> >     if "required-opps" has a single entry. I guess we need extra checks
> >     to exclude the "parent genpd" case compared to the "OPP" case.
> >
> >         [    1.116244] WARNING: CPU: 2 PID: 36 at drivers/opp/of.c:331 _link_required_opps+0x180/0x1cc
> >         [    1.125897] Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT)
> >         [    1.146887] pc : _link_required_opps+0x180/0x1cc
> >         [    1.146902] lr : _link_required_opps+0xdc/0x1cc
> >         [    1.276408] Call trace:
> >         [    1.283519]  _link_required_opps+0x180/0x1cc
> >         [    1.285779]  _of_add_table_indexed+0x61c/0xd40
> >         [    1.290292]  dev_pm_opp_of_add_table+0x10/0x18
> >         [    1.294546]  of_genpd_add_provider_simple+0x80/0x160
> >         [    1.298974]  cpr_probe+0x6a0/0x97c
> >         [    1.304092]  platform_probe+0x64/0xbc
> >
> > It does seem to work correctly, with and without this patch. So I guess
> > another option might be to simply silence this WARN_ON(). :')
> 
> Oh, thanks for pointing this out! This case haven't crossed my mind yet!
> 
> Allow me to think a bit more about it. I will get back to you again
> with a suggestion soon, unless Viresh comes back first. :-)

I have resent the series now.

Stephan, please give it a try again. Thanks.

Regarding this case where a genpd's table points to a parent genpd's table via
the required-opps, it is a bit tricky to solve and the only way around that I
could think of is that someone needs to call dev_pm_opp_set_config() with the
right device pointer, with that we won't hit the warning anymore and things will
work as expected.

In this case the OPP core needs to call dev_pm_domain_set_performance_state()
for device and then its genpd. We need the right device pointers :(

Ulf, also another important thing here is that maybe we would want the genpd
core to not propagate the voting anymore to the parent genpd's ? The
dev_pm_opp_set_opp() call is better placed at handling all things and not just
the performance state, like clk, regulator, bandwidth and so the recursion
should happen at OPP level only. For now my series shouldn't break anything,
just that we will try to set performance state twice for the parent genpd, the
second call should silently return as the target state should be equal to
current state.

-- 
viresh