Message-ID: <ZME4MQpYd7kJmFzF@e120937-lin>
Date:   Wed, 26 Jul 2023 16:13:53 +0100
From:   Cristian Marussi <cristian.marussi@....com>
To:     Ulf Hansson <ulf.hansson@...aro.org>
Cc:     Sudeep Holla <sudeep.holla@....com>,
        Viresh Kumar <vireshk@...nel.org>, Nishanth Menon <nm@...com>,
        Stephen Boyd <sboyd@...nel.org>,
        Nikunj Kela <nkela@...cinc.com>,
        Prasad Sodagudi <psodagud@...cinc.com>,
        Alexandre Torgue <alexandre.torgue@...s.st.com>,
        linux-pm@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 10/11] firmware: arm_scmi: Add the SCMI performance
 domain

On Fri, Jul 21, 2023 at 05:19:51PM +0200, Ulf Hansson wrote:
> Hi Cristian,
> 

Hi,

> On Wed, 19 Jul 2023 at 16:51, Cristian Marussi <cristian.marussi@....com> wrote:
> >
> > On Thu, Jul 13, 2023 at 04:17:37PM +0200, Ulf Hansson wrote:
> > > To enable support for performance scaling (DVFS) for generic devices with
> > > the SCMI performance protocol, let's add an SCMI performance domain. This
> > > is being modelled as a genpd provider, with support for performance scaling
> > > through genpd's ->set_performance_state() callback.
> > >
> > > Note that, this adds the initial support that allows consumer drivers for
> > > attached devices, to vote for a new performance state via calling the
> > > dev_pm_genpd_set_performance_state(). However, this should be avoided as
> > > it's in most cases preferred to use the OPP library to vote for a new OPP
> > > instead. The support using the OPP library isn't part of this change, but
> > > needs to be implemented from subsequent changes.
> > >
> >
> > Hi Ulf,
> >
> > a couple of remarks down below.
> >
> > > Signed-off-by: Ulf Hansson <ulf.hansson@...aro.org>
> > > ---
> > >
> > > Changes in v2:
> > >       - Converted to use the new ->domain_info_get() callback.
> > >
> > > ---
> > >  drivers/firmware/arm_scmi/Kconfig            |  12 ++
> > >  drivers/firmware/arm_scmi/Makefile           |   1 +
> > >  drivers/firmware/arm_scmi/scmi_perf_domain.c | 155 +++++++++++++++++++
> > >  3 files changed, 168 insertions(+)
> > >  create mode 100644 drivers/firmware/arm_scmi/scmi_perf_domain.c
> >
> > [snip]
> >
> > > +static int scmi_perf_domain_probe(struct scmi_device *sdev)
> > > +{
> > > +     struct device *dev = &sdev->dev;
> > > +     const struct scmi_handle *handle = sdev->handle;
> > > +     const struct scmi_perf_proto_ops *perf_ops;
> > > +     struct scmi_protocol_handle *ph;
> > > +     struct scmi_perf_domain *scmi_pd;
> > > +     struct genpd_onecell_data *scmi_pd_data;
> > > +     struct generic_pm_domain **domains;
> > > +     int num_domains, i, ret = 0;
> > > +     u32 perf_level;
> > > +
> > > +     if (!handle)
> > > +             return -ENODEV;
> > > +
> > > +     /* The OF node must specify us as a power-domain provider. */
> > > +     if (!of_find_property(dev->of_node, "#power-domain-cells", NULL))
> > > +             return 0;
> > > +
> > > +     perf_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_PERF, &ph);
> > > +     if (IS_ERR(perf_ops))
> > > +             return PTR_ERR(perf_ops);
> > > +
> > > +     num_domains = perf_ops->num_domains_get(ph);
> > > +     if (num_domains < 0) {
> > > +             dev_warn(dev, "Failed with %d when getting num perf domains\n",
> > > +                      num_domains);
> > > +             return num_domains;
> > > +     } else if (!num_domains) {
> > > +             return 0;
> > > +     }
> > > +
> > > +     scmi_pd = devm_kcalloc(dev, num_domains, sizeof(*scmi_pd), GFP_KERNEL);
> > > +     if (!scmi_pd)
> > > +             return -ENOMEM;
> > > +
> > > +     scmi_pd_data = devm_kzalloc(dev, sizeof(*scmi_pd_data), GFP_KERNEL);
> > > +     if (!scmi_pd_data)
> > > +             return -ENOMEM;
> > > +
> > > +     domains = devm_kcalloc(dev, num_domains, sizeof(*domains), GFP_KERNEL);
> > > +     if (!domains)
> > > +             return -ENOMEM;
> > > +
> > > +     for (i = 0; i < num_domains; i++, scmi_pd++) {
> > > +             scmi_pd->info = perf_ops->domain_info_get(ph, i);
> >
> > So here you are grabbing all the performance domains exposed by the
> > platform via PERF protocol and then a few lines down below you are
> > registering them with pm_genpd_init(), but the list of domains obtained
> > from the platform will contain NOT only devices but also CPUs possibly,
> > already managed by the SCMI CPUFreq driver.
> 
> Correct.
> 
> >
> > In fact the SCMI CPUFreq driver, on his side, takes care to pick only
> > domains that are bound in the DT to a CPU (via scmi_cpu_domain_id DT
> > parsing) but here you are registering all domains with GenPD upfront.
> 
> Right, genpds are acting as providers, which need to be registered
> upfront to allow consumer devices to be attached when they get probed.
> 
> This isn't specific to this case, but how the genpd infrastructure is
> working per design.
> 
> >
> > 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.

> >
> > > +             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.

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..)

Thanks,
Cristian

Powered by blists - more mailing lists