From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
To: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Sudeep Holla <sudeep.holla@arm.com>,
Mark Rutland <mark.rutland@arm.com>,
Linux ARM <linux-arm-kernel@lists.infradead.org>,
"Rafael J . Wysocki" <rjw@rjwysocki.net>,
Daniel Lezcano <daniel.lezcano@linaro.org>,
"Raju P . L . S . S . S . N" <rplsssn@codeaurora.org>,
Amit Kucheria <amit.kucheria@linaro.org>,
Bjorn Andersson <bjorn.andersson@linaro.org>,
Stephen Boyd <sboyd@kernel.org>,
Niklas Cassel <niklas.cassel@linaro.org>,
Tony Lindgren <tony@atomide.com>,
Kevin Hilman <khilman@kernel.org>,
Lina Iyer <ilina@codeaurora.org>,
Viresh Kumar <viresh.kumar@linaro.org>,
Vincent Guittot <vincent.guittot@linaro.org>,
Geert Uytterhoeven <geert+renesas@glider.be>,
Souvik Chakravarty <souvik.chakravarty@arm.com>,
Linux PM <linux-pm@vger.kernel.org>,
linux-arm-msm <linux-arm-msm@vger.kernel.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 10/18] drivers: firmware: psci: Add hierarchical domain idle states converter
Date: Tue, 16 Jul 2019 15:51:29 +0100 [thread overview]
Message-ID: <20190716145121.GA32490@e121166-lin.cambridge.arm.com> (raw)
In-Reply-To: <CAPDyKFpd5Jip+ZDENXVC115JJgCReJa4G92aKa45Lna6BeV5-g@mail.gmail.com>
On Tue, Jul 16, 2019 at 10:45:49AM +0200, Ulf Hansson wrote:
[...]
> > > +static void psci_pd_convert_states(struct cpuidle_state *idle_state,
> > > + u32 *psci_state, struct genpd_power_state *state)
> > > +{
> > > + u32 *state_data = state->data;
> > > + u64 target_residency_us = state->residency_ns;
> > > + u64 exit_latency_us = state->power_on_latency_ns +
> > > + state->power_off_latency_ns;
> > > +
> > > + *psci_state = *state_data;
> > > + do_div(target_residency_us, 1000);
> > > + idle_state->target_residency = target_residency_us;
> > > + do_div(exit_latency_us, 1000);
> > > + idle_state->exit_latency = exit_latency_us;
> > > + idle_state->enter = &psci_pd_enter_pc;
> > > + idle_state->enter_s2idle = &psci_pd_enter_s2idle_pc;
> > > + idle_state->flags |= CPUIDLE_FLAG_TIMER_STOP;
> >
> > This is arbitrary and not necessarily true.
>
> The arbitrary thing you refer to here, is that the
> CPUIDLE_FLAG_TIMER_STOP? Or are you referring to the complete function
> psci_pd_convert_states()?
I refer to CPUIDLE_FLAG_TIMER_STOP. I think that on platform coordinated
system we should not bother about the hierarchical representation of the
states (I understand I asked you to make it work but it has become too
complex, I would rather focus on making the hierarchical representation
work for all idle states combination in OSI mode).
Plus side, another level of complexity removed.
> > I think that this patch is useful to represent my reservations about the
> > current approach. As a matter of fact, idle state entry will always be a
> > CPUidle decision.
> >
> > You only need PM domain information to understand when all CPUs
> > in a power domain are actually idle but that's all genPD can do
> > in this respect.
> >
> > I think this patchset would be much simpler if both CPUidle and
> > genPD governor would work on *one* set of idle states, globally
> > indexed (and that would be true for PSCI suspend parameters too).
> >
> > To work with a unified set of idle states between CPUidle and genPD
> > (tossing some ideas around):
> >
> > - We can implement a genPD CPUidle governor that in its select method
> > takes into account genPD information (for instance by avoiding
> > selection of idle states that require multiple cpus to be in idle
> > to be effectively active)
> > - We can use genPD to enable/disable CPUidle states through runtime
> > PM information
>
> I don't understand how to make this work.
>
> The CPUidle governor works on per CPU basis. The genpd governor works
> on per PM domain basis, which typically can be a group of CPUs (and
> even other devices) via subdomains, for example.
>
> 1.
> In case of Linux being in *charge* of what idle state to pick for a
> group of CPUs, that decision is best done by the genpd governor as it
> operates on "groups" of CPUs. This is used for PSCI OSI mode. Of
> course, there are idle states also per CPU, which potentially could be
> managed by the genpd governor as well, but at this point I decided to
> re-use the cpuidle governor as it's already doing the job.
>
> 2. In case the decision of what idle state to enter for the group is
> done by the FW, we can rely solely on the cpuidle governor and let it
> select states per CPU basis. This is used for PSCI PC mode.
>
> >
> > There may be other ways. My point is that current code, with two (or
> > more if the hierarchy grows) sets of idle states across two subsystems
> > (CPUidle and genPD) is not very well defined and honestly very hard to
> > grasp and prone to errors.
>
> The complexity is there, I admit that.
>
> I guess some deeper insight about genpd+its governor+runtime PM are
> needed when reviewing this, of course. As an option, you may also have
> a look at my slides [1] from OSPM (Pisa) in May this year, which via
> flow charts try to describes things in more detail.
>
> In our offlist meeting, we discussed that perhaps moving some of the
> new PSCI code introduced in this series, into a cpuidle driver
> instead, could make things more clear. For sure, I can explore that
> option, but before I go there, I think we should agree on it publicly.
I will do it but given that the generic idle infrastructure basically
is there for PSCI and:
drivers/soc/qcom/spm.c
if we create a PSCI CPUidle driver we can write one for qcom-spm and
remove the generic idle infrastructure, there would not be much
point in keeping it in the kernel; at least on ARM64 not using
PSCI for CPUidle is not even an option.
> In principle what it means is to invent a special cpuidle driver for
> PSCI, so we would need access to some of the PSCI internal functions,
> for example.
Yes.
> One thing though, the initialization of the PSCI PM domain topology is
> a separate step, managed via the new initcall, psci_dt_topology_init()
> (introduced in patch 11). That part still seems to be belong to the
> PSCI code, don't you think?
Yes but at least we can call it from a sensible place (well, sensible,
most likely from an initcall given how idle drivers are initialized).
> > > + strncpy(idle_state->name, to_of_node(state->fwnode)->name,
> > > + CPUIDLE_NAME_LEN - 1);
> > > + strncpy(idle_state->desc, to_of_node(state->fwnode)->name,
> > > + CPUIDLE_NAME_LEN - 1);
> > > +}
> > > +
> > > +static bool psci_pd_is_provider(struct device_node *np)
> > > +{
> > > + struct psci_pd_provider *pd_prov, *it;
> > > +
> > > + list_for_each_entry_safe(pd_prov, it, &psci_pd_providers, link) {
> > > + if (pd_prov->node == np)
> > > + return true;
> > > + }
> > > +
> > > + return false;
> > > +}
> > > +
> > > static int psci_pd_init(struct device_node *np)
> > > {
> > > struct generic_pm_domain *pd;
> > > @@ -265,4 +316,71 @@ int psci_dt_init_pm_domains(struct device_node *np)
> > > pr_err("failed to create CPU PM domains ret=%d\n", ret);
> > > return ret;
> > > }
> > > +
> > > +int psci_dt_pm_domains_parse_states(struct cpuidle_driver *drv,
> > > + struct device_node *cpu_node, u32 *psci_states)
> > > +{
> > > + struct genpd_power_state *pd_states;
> > > + struct of_phandle_args args;
> > > + int ret, pd_state_count, i, state_idx, psci_idx;
> > > + u32 cpu_psci_state = psci_states[drv->state_count - 2];
> >
> > This (-2) is very dodgy and I doubt it would work on hierarchies going
> > above "cluster" level.
> >
> > As I say above, I think we should work towards a single array of
> > idle states to be selected by a CPUidle governor using genPD
> > runtime information to bias the results according to the number
> > of CPUs in a genPD that entered/exit idle.
> >
> > To be more precise, all idles states should be "domain-idle-state"
> > compatible, even the CPU ones, the distinction between what CPUidle
> > and genPD manage is a bit stretched IMO in this patchset.
> >
> > We will have a chance to talk about this but I thought I would
> > comment publically if anyone else is willing to chime in, this
> > is not a PSCI problem at all, it is a CPUidle/genPD coexistence
> > design problem which is much broader.
>
> To move this forward, I think we need to move from vague ideas to
> clear and distinct plans. Whatever that means. :-)
See above.
> I understand you are concerned about the level of changes introduced
> to the PSCI code. As I stated somewhere in earlier replies, I picked
> that approach as I thought it was better to implement things in a PSCI
> specific manner to start with, then we could move things around, when
> we realize that it make sense.
I am also concerned about how the idle states are managed in
this patchset and I am pretty certain it will break when we
move away from a simple hierarchy with one CPU state and one
cluster state, we will comment on the specifics.
Moving PSCI code into a CPUidle driver will cater for the rest.
> Anyway, as a suggestion to address your concern, how about this:
>
> 1. Move some things out to a PSCI cpuidle driver. We need to decide
> more exactly on what to move and find the right level for the
> interfaces.
I will do it and post patches asap.
> 2. Don't attach the CPU to the PM domain topology in case the PSCI PC
> mode is used. I think this makes it easier, at least as a first step,
> to understand when runtime PM needs to be used/enabled.
In the PSCI CPUidle driver we can have two distinct struct
cpuidle_state->enter functions for PC and OSI, no overhead
for PC, runtime PM for OSI, decoupling done.
We can choose one or the other depending on whether:
OSI iff:
- OSI is available
- hierarchical idle states are present in DT
otherwise PC.
That's what this patch does but we will do it in a unified file.
> 3. Would it help if I volunteer to help you guys as a maintainer for
> PSCI. At least for the part of the new code that becomes introduced?
We will do as described above if that makes sense.
Thanks,
Lorenzo
next prev parent reply other threads:[~2019-07-16 14:51 UTC|newest]
Thread overview: 68+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-05-13 19:22 [PATCH 00/18] ARM/ARM64: Support hierarchical CPU arrangement for PSCI Ulf Hansson
2019-05-13 19:22 ` [PATCH 01/18] dt: psci: Update DT bindings to support hierarchical PSCI states Ulf Hansson
2019-07-19 11:29 ` Lorenzo Pieralisi
2019-05-13 19:22 ` [PATCH 02/18] of: base: Add of_get_cpu_state_node() to get idle states for a CPU node Ulf Hansson
2019-05-13 19:22 ` [PATCH 03/18] cpuidle: dt: Support hierarchical CPU idle states Ulf Hansson
2019-05-13 19:22 ` [PATCH 04/18] ARM/ARM64: cpuidle: Let back-end init ops take the driver as input Ulf Hansson
2019-06-07 15:00 ` Sudeep Holla
2019-06-10 10:20 ` Ulf Hansson
2019-05-13 19:22 ` [PATCH 05/18] drivers: firmware: psci: Simplify state node parsing Ulf Hansson
2019-06-07 15:01 ` Sudeep Holla
2019-05-13 19:22 ` [PATCH 06/18] drivers: firmware: psci: Support hierarchical CPU idle states Ulf Hansson
2019-06-07 15:03 ` Sudeep Holla
2019-05-13 19:22 ` [PATCH 07/18] drivers: firmware: psci: Prepare to use OS initiated suspend mode Ulf Hansson
2019-06-07 15:17 ` Sudeep Holla
2019-06-10 10:21 ` Ulf Hansson
2019-06-10 10:42 ` Sudeep Holla
2019-07-16 14:53 ` Sudeep Holla
2019-05-13 19:22 ` [PATCH 08/18] drivers: firmware: psci: Prepare to support PM domains Ulf Hansson
2019-06-07 15:21 ` Sudeep Holla
2019-05-13 19:22 ` [PATCH 09/18] drivers: firmware: psci: Add support for PM domains using genpd Ulf Hansson
2019-06-07 15:27 ` Sudeep Holla
2019-06-10 10:21 ` Ulf Hansson
2019-06-10 10:59 ` Sudeep Holla
2019-07-16 15:05 ` Sudeep Holla
2019-07-18 11:04 ` Ulf Hansson
2019-07-18 13:19 ` Sudeep Holla
2019-07-18 17:57 ` Lina Iyer
2019-07-19 9:45 ` Sudeep Holla
2019-05-13 19:22 ` [PATCH 10/18] drivers: firmware: psci: Add hierarchical domain idle states converter Ulf Hansson
2019-07-09 15:31 ` Lorenzo Pieralisi
2019-07-16 8:45 ` Ulf Hansson
2019-07-16 14:51 ` Lorenzo Pieralisi [this message]
2019-07-18 11:43 ` Ulf Hansson
2019-07-18 13:36 ` Lorenzo Pieralisi
2019-05-13 19:22 ` [PATCH 11/18] drivers: firmware: psci: Introduce psci_dt_topology_init() Ulf Hansson
2019-05-13 19:22 ` [PATCH 12/18] drivers: firmware: psci: Add a helper to attach a CPU to its PM domain Ulf Hansson
2019-05-13 19:22 ` [PATCH 13/18] drivers: firmware: psci: Attach the CPU's device " Ulf Hansson
2019-05-13 19:22 ` [PATCH 14/18] drivers: firmware: psci: Manage runtime PM in the idle path for CPUs Ulf Hansson
2019-07-16 15:53 ` Lorenzo Pieralisi
2019-07-18 10:35 ` Ulf Hansson
2019-07-18 13:30 ` Lorenzo Pieralisi
2019-07-18 16:54 ` Ulf Hansson
2019-07-18 17:41 ` Lina Iyer
2019-07-18 21:49 ` Ulf Hansson
2019-07-19 10:02 ` Lorenzo Pieralisi
2019-05-13 19:22 ` [PATCH 15/18] drivers: firmware: psci: Support CPU hotplug for the hierarchical model Ulf Hansson
2019-06-07 15:31 ` Sudeep Holla
2019-06-10 10:21 ` Ulf Hansson
2019-06-10 11:02 ` Sudeep Holla
2019-05-13 19:22 ` [PATCH 16/18] arm64: kernel: Respect the hierarchical CPU topology in DT for PSCI Ulf Hansson
2019-05-13 19:22 ` [PATCH 17/18] arm64: dts: Convert to the hierarchical CPU topology layout for MSM8916 Ulf Hansson
2019-07-16 14:47 ` Sudeep Holla
2019-07-16 20:36 ` Lina Iyer
2019-07-17 17:18 ` Sudeep Holla
2019-05-13 19:23 ` [PATCH 18/18] arm64: dts: hikey: Convert to the hierarchical CPU topology layout Ulf Hansson
2019-07-16 14:47 ` Sudeep Holla
2019-07-18 10:48 ` Ulf Hansson
2019-07-18 13:11 ` Sudeep Holla
2019-05-14 8:08 ` [PATCH 00/18] ARM/ARM64: Support hierarchical CPU arrangement for PSCI Rafael J. Wysocki
2019-05-14 8:58 ` Ulf Hansson
2019-06-07 15:42 ` Sudeep Holla
2019-06-07 19:34 ` Bjorn Andersson
2019-06-10 10:32 ` Sudeep Holla
2019-06-10 15:54 ` Ulf Hansson
2019-06-10 17:16 ` Lorenzo Pieralisi
2019-06-10 18:57 ` Ulf Hansson
2019-06-18 11:56 ` Ulf Hansson
2019-06-07 11:19 ` Ulf Hansson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190716145121.GA32490@e121166-lin.cambridge.arm.com \
--to=lorenzo.pieralisi@arm.com \
--cc=amit.kucheria@linaro.org \
--cc=bjorn.andersson@linaro.org \
--cc=daniel.lezcano@linaro.org \
--cc=geert+renesas@glider.be \
--cc=ilina@codeaurora.org \
--cc=khilman@kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=niklas.cassel@linaro.org \
--cc=rjw@rjwysocki.net \
--cc=rplsssn@codeaurora.org \
--cc=sboyd@kernel.org \
--cc=souvik.chakravarty@arm.com \
--cc=sudeep.holla@arm.com \
--cc=tony@atomide.com \
--cc=ulf.hansson@linaro.org \
--cc=vincent.guittot@linaro.org \
--cc=viresh.kumar@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).