Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230713141738.23970-1-ulf.hansson@linaro.org>
 <20230713141738.23970-11-ulf.hansson@linaro.org> <ZLf4c7ejFBJLH7iN@e120937-lin>
 <CAPDyKFr3ann52GAtOLfnLSGgsdF+EZBNz_apNo_OHzrQ-Hg55Q@mail.gmail.com> <ZME4MQpYd7kJmFzF@e120937-lin>
In-Reply-To: <ZME4MQpYd7kJmFzF@e120937-lin>
From:   Ulf Hansson <ulf.hansson@linaro.org>
Date:   Thu, 27 Jul 2023 13:37:50 +0200
Message-ID: <CAPDyKFou5Vq-7j5HpZ1AT7AF-+y4wgEqs9tLyNWV0uE2isS85w@mail.gmail.com>
Subject: Re: [PATCH v2 10/11] firmware: arm_scmi: Add the SCMI performance domain
To:     Cristian Marussi <cristian.marussi@arm.com>
Cc:     Sudeep Holla <sudeep.holla@arm.com>,
        Viresh Kumar <vireshk@kernel.org>, Nishanth Menon <nm@ti.com>,
        Stephen Boyd <sboyd@kernel.org>,
        Nikunj Kela <nkela@quicinc.com>,
        Prasad Sodagudi <psodagud@quicinc.com>,
        Alexandre Torgue <alexandre.torgue@foss.st.com>,
        linux-pm@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

[...]

> > >
> > > Is it not possible that, once registered, GenPD can decide, at some point
> > > in the future, to try act on some of these domains associated with a CPU ?
> > > (like Clock framework does at the end of boot trying to disable unused
> > >  clocks...not familiar with internals of GenPD, though)
> >
> > The "magic" that exists in genpd is to save/restore the performance
> > state at genpd_runtime_suspend|resume().
> >
> > That means the consumer device needs to be attached and runtime PM
> > enabled, otherwise genpd will just leave the performance level
> > unchanged. In other words, the control is entirely at the consumer
> > driver (scmi cpufreq driver).
> >
>
> Ok, so if the DT is well formed and a CPU-related perf domain is not
> wrongly referred from a driver looking for a device perf-domain, the
> genPD subsystem wont act on the CPUs domains.

Yes, correct!

>
> > >
> > > > +             scmi_pd->domain_id = i;
> > > > +             scmi_pd->perf_ops = perf_ops;
> > > > +             scmi_pd->ph = ph;
> > > > +             scmi_pd->genpd.name = scmi_pd->info->name;
> > > > +             scmi_pd->genpd.flags = GENPD_FLAG_OPP_TABLE_FW;
> > > > +             scmi_pd->genpd.set_performance_state = scmi_pd_set_perf_state;
> > > > +
> > > > +             ret = perf_ops->level_get(ph, i, &perf_level, false);
> > > > +             if (ret) {
> > > > +                     dev_dbg(dev, "Failed to get perf level for %s",
> > > > +                              scmi_pd->genpd.name);
> > > > +                     perf_level = 0;
> > > > +             }
> > > > +
> > > > +             /* Let the perf level indicate the power-state too. */
> > > > +             ret = pm_genpd_init(&scmi_pd->genpd, NULL, perf_level == 0);
> > >
> > > In SCMI world PERF levels should have nothing to do with the Power
> > > state of a domain: you have the POWER protocol for that, so you should
> > > not assume that perf level 0 means OFF, but you can use the POWER protocol
> > > operation .state_get() to lookup the power state. (and you can grab both
> > > perf and power ops from the same driver)
> >
> > Well, I think this may be SCMI FW implementation specific, but
> > honestly I don't know exactly what the spec says about this. In any
> > case, I don't think it's a good idea to mix this with the POWER
> > domain, as that's something that is entirely different. We have no
> > clue of those relationships (domain IDs).
> >
> > My main idea behind this, is just to give the genpd internals a
> > reasonably defined value for its power state.
> >
>
> The thing is that in the SCMI world you cannot assume that perf_level 0
> means powered off, the current SCP/SCMI platform fw, as an example, wont
> advertise a 0-perf-level and wont act on such a request, because you are
> supposed to use POWER protocol to get/set the power-state of a device.

Right, thanks for clarifying this!

>
> So it could be fine, as long as genPD wont try to set the level to 0
> expecting the domain to be as a consequence also powered off and as
> long as it is fine for these genpd domains to be always initialized
> as ON. (since perf_level could never be found as zero..)

Okay, so to me, it sounds like that we should set GENPD_FLAG_ALWAYS_ON
for the genpd. This makes it clear that the domain can't be powered
off.

Moreover, we should prevent genpd internals from setting the
performance state to 0 for the scmi performance domain. Let me look
into how to best deal with that.

Kind regards
Uffe