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>
 <20230719155920.iuu2ue2co535dfkx@bogus>
In-Reply-To: <20230719155920.iuu2ue2co535dfkx@bogus>
From:   Ulf Hansson <ulf.hansson@linaro.org>
Date:   Wed, 26 Jul 2023 14:01:21 +0200
Message-ID: <CAPDyKFqMXWshKRd-dcETa9SRWFF4w5MFrWBSVkMn80dHg0Cvjg@mail.gmail.com>
Subject: Re: [PATCH v2 10/11] firmware: arm_scmi: Add the SCMI performance domain
To:     Sudeep Holla <sudeep.holla@arm.com>
Cc:     Cristian Marussi <cristian.marussi@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

On Wed, 19 Jul 2023 at 17:59, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Wed, Jul 19, 2023 at 03:51:45PM +0100, Cristian Marussi wrote:
> > On Thu, Jul 13, 2023 at 04:17:37PM +0200, Ulf Hansson wrote:
>
> [...]
>
> > > +   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.
> >
>
> Agreed, I pointed out briefly in the previous patch I think. I am not sure
> how will that work if the performance and power domains are not 1-1 mapping
> or if they are CPUs then this might confusing ? Not sure but looks like
> we might be creating a spaghetti here :(.

I assume the discussions around the DT bindings are making it more
clear on how this should work. The scmi performance-domain and the
scmi power-domain are two separate providers.

>
> > 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.
> >
>
> +1
>
> > 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 ?
>
> IIRC, all unused genpd are turned off right. It may not impact here but still
> super confusing as we will be creating power domains for the set of domains
> actually advertised as power domains by the firmware. This will add another
> set.
>
> > (like Clock framework does at the end of boot trying to disable unused
> >  clocks...not familiar with internals of GenPD, though)
> >
>
> Ah, I am reading too much serialised. Just agreed and wrote the same above.
>
> > > +           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)
> >
> > The tricky part would be to match the PERF domain at hand with the
> > related POWER domain to query the state for, I suppose.
> >
>
> I wanted to ask the same. E.g. on juno, GPU has perf domain 2 and power domain
> 9. It would be good if we can how it would work there ? What is expected
> from the gpu driver in terms of managing perf and power ? Does it need
> to specify 2 power domains now and specify which is perf and which power in
> its bindings ?

Yes, correct.

Note that, we already have plenty of consumer devices/drivers that are
managing multiple PM domains. They use
dev_pm_domain_attach_by_id|name() to attach their devices to their
corresponding domain(s). In addition, they often use device_link_add()
to simplify runtime PM management.

That said, we should expect to see some boilerplate code in consumer
drivers that deals with this attaching/detaching of multiple PM
domains. That's a separate problem we may want to address later on. In
fact, it's already been discussed earlier at LKML (I can't find the
link right now).

[...]

Kind regards
Uffe