linux-kernel - Re: [PATCH 1/2] OPP: Use _set_opp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZTk_M0JFdAg7FR7E@gerhold.net>
Date:   Wed, 25 Oct 2023 18:16:23 +0200
From:   Stephan Gerhold <stephan@...hold.net>
To:     Viresh Kumar <viresh.kumar@...aro.org>
Cc:     Ulf Hansson <ulf.hansson@...aro.org>,
        Viresh Kumar <vireshk@...nel.org>, Nishanth Menon <nm@...com>,
        Stephen Boyd <sboyd@...nel.org>, linux-pm@...r.kernel.org,
        Vincent Guittot <vincent.guittot@...aro.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Stephan Gerhold <stephan.gerhold@...nkonzept.com>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] OPP: Use _set_opp_level() for single genpd case

On Wed, Oct 25, 2023 at 08:54:31PM +0530, Viresh Kumar wrote:
> On 25-10-23, 15:47, Stephan Gerhold wrote:
> > FWIW I'm hitting this WARNing when trying to set up the parent domain
> > setup for CPR->RPMPD(MX) on MSM8916 that I discussed with Uffe recently
> > [1]. I know, me and all my weird OPP setups. :'D
> > 
> > Basically, I have cpufreq voting for performance states of the CPR genpd
> > (via required-opps). CPR is supposed to have <&rpmpd MSM8916_VDDMX_AO>
> > as parent genpd and translates to the parent performance state using the
> > "required-opps" in the *CPR* OPP table:
> > 
> > 	cpr: power-controller@...8000 {
> > 		compatible = "qcom,msm8916-cpr", "qcom,cpr";
> > 		reg = <0x0b018000 0x1000>;
> > 		/* ... */
> > 		#power-domain-cells = <0>;
> > 		operating-points-v2 = <&cpr_opp_table>;
> > 		/* Supposed to be parent domain, not consumer */
> > 		power-domains = <&rpmpd MSM8916_VDDMX_AO>;
> > 
> > 		cpr_opp_table: opp-table {
> > 			compatible = "operating-points-v2-qcom-level";
> > 
> > 			cpr_opp1: opp1 {
> > 				opp-level = <1>;
> > 				qcom,opp-fuse-level = <1>;
> > 				required-opps = <&rpmpd_opp_svs_soc>;
> > 			};
> > 			cpr_opp2: opp2 {
> > 				opp-level = <2>;
> > 				qcom,opp-fuse-level = <2>;
> > 				required-opps = <&rpmpd_opp_nom>;
> > 			};
> > 			cpr_opp3: opp3 {
> > 				opp-level = <3>;
> > 				qcom,opp-fuse-level = <3>;
> > 				required-opps = <&rpmpd_opp_super_turbo>;
> > 			};
> > 		};
> > 	};
> 
> I have forgotten a bit about this usecase. How exactly does the
> configurations work currently for this ? I mean genpd core must be
> setting the vote finally for only one of them or something else ?
> 

I'm not sure if I understand your question correctly. Basically, setting
<&rpmpd MSM8916_VDDMX_AO> as "parent genpd" of <&cpr> is supposed to
describe that there is a direct relationship between the performance
states of CPR and VDDMX. When changing the CPR performance state, VDDMX
should also be adjusted accordingly.

This is implemented in the genpd core in _genpd_set_performance_state().
It loops over the parent genpds, and re-evaluates the performance states
of each of them. Translation happens using genpd_xlate_performance_state()
which is just a direct call to dev_pm_opp_xlate_performance_state().
This will look up the required-opps from the OPP table above. However,
the genpd core calls ->set_performance_state() on the parent genpd
directly, so dev_pm_opp_set_opp() isn't involved in this case.

Overall the call sequence for a CPUfreq switch will look something like:

 - cpu0: dev_pm_opp_set_rate(998.4 MHz)
  - cpu0: _set_required_opps(opp-998400000)
   - genpd:1:cpu0: dev_pm_opp_set_opp(&cpr_opp3)
    - genpd:1:cpu0: _set_opp_level(&cpr_opp3)
     - cpr: _genpd_set_performance_state(3)

      # genpd: translate & adjust parent performance states
      - cpr: genpd_xlate_performance_state(parent=VDDMX_AO)
             => &rpmpd_opp_super_turbo = 6
       - VDDMX_AO: _genpd_set_performance_state(6)
        - rpmpd: ->set_performance_state(VDDMX_AO, 6)

      # genpd: change actual performance state
      - cpr: ->set_performance_state(cpr, 3)

Before the discussion with Uffe I did not describe this relationship
between CPR<->VDDMX as parent-child, I just had them as two separate
power domains in the CPU OPP table. That worked fine too but Uffe
suggested the parent-child representation might be better.
   
Does that help or were you looking for something else? :D

Thanks,
Stephan