From: Ulf Hansson <ulf.hansson@linaro.org>
To: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Sudeep Holla <sudeep.holla@arm.com>,
"Rafael J . Wysocki" <rjw@rjwysocki.net>,
Daniel Lezcano <daniel.lezcano@linaro.org>,
Mark Rutland <mark.rutland@arm.com>,
Lina Iyer <ilina@codeaurora.org>,
Linux PM <linux-pm@vger.kernel.org>,
Rob Herring <robh+dt@kernel.org>,
Vincent Guittot <vincent.guittot@linaro.org>,
Stephen Boyd <sboyd@kernel.org>, Andy Gross <agross@kernel.org>,
Bjorn Andersson <bjorn.andersson@linaro.org>,
Kevin Hilman <khilman@kernel.org>,
Linux ARM <linux-arm-kernel@lists.infradead.org>,
linux-arm-msm <linux-arm-msm@vger.kernel.org>,
Lina Iyer <lina.iyer@linaro.org>
Subject: Re: [PATCH v2 10/13] cpuidle: psci: Prepare to use OS initiated suspend mode via PM domains
Date: Mon, 25 Nov 2019 11:44:47 +0100 [thread overview]
Message-ID: <CAPDyKFruoj_r3ktAHbVJAnnCZ6EP1dB9sZLE0=BZX=UziUYJag@mail.gmail.com> (raw)
In-Reply-To: <20191122182623.GA8290@e121166-lin.cambridge.arm.com>
On Fri, 22 Nov 2019 at 19:26, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> On Mon, Nov 18, 2019 at 02:37:41PM +0100, Ulf Hansson wrote:
> > On Fri, 15 Nov 2019 at 18:30, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > On Tue, Oct 29, 2019 at 05:44:35PM +0100, Ulf Hansson wrote:
> > > > The per CPU variable psci_power_state, contains an array of fixed values,
> > > > which reflects the corresponding arm,psci-suspend-param parsed from DT, for
> > > > each of the available CPU idle states.
> > > >
> > > > This isn't sufficient when using the hierarchical CPU topology in DT, in
> > > > combination with having PSCI OS initiated (OSI) mode enabled. More
> > > > precisely, in OSI mode, Linux is responsible of telling the PSCI FW what
> > > > idle state the cluster (a group of CPUs) should enter, while in PSCI
> > > > Platform Coordinated (PC) mode, each CPU independently votes for an idle
> > > > state of the cluster.
> > > >
> > > > For this reason, introduce a per CPU variable called domain_state and
> > > > implement two helper functions to read/write its value. Then let the
> > > > domain_state take precedence over the regular selected state, when entering
> > > > and idle state.
> > > >
> > > > Finally, let's also avoid sprinkling the existing non-OSI path with
> > > > operations being specific for OSI.
> > > >
> > >
> > > Mostly looks good.
> >
> > Thanks!
> >
> >
> > > I am still wondering if we can keep all OSI related
> > > info in the newly created structure and have psci_states outside it as
> > > before. And I was think psci_enter_idle_state_pc and psci_enter_idle_state_osi
> > > instead of single psci_enter_idle_state and assign/initialise state->enter
> > > based on the mode chosen. I had to closer look now and looks like enter
> > > is initialised in generic dt_idle_states. That said, what you have in this
> > > patch also looks OK to me, was just trying to avoid access to the new
> > > structure all together and keep the PC mode patch almost same as before
> > > when suspending. I will see what Lorenzo thinks about this.
> >
> > I did explore that approach a bit, but found it easier to go with what
> > I propose here. The most important point, in my view, is that in this
> > suggested approach only one if-check, "if (!data->dev)", is added to
> > the PC mode path compared to the original path. I think this should be
> > fine, right!?
>
> I don't see why we should use data->dev at runtime when we can
> have two separate idle enter functions and the detection can
> be done at probe once for all instead of every idle entry.
>
> The overhead is close to nought but the point is that it is
> really not needed.
Alright, I will adopt our suggestion and override the assigned idle
enter callback, when we succeed to attach the cpu-device to its PM
domain.
Do you have any other thoughts about the series?
Kind regards
Uffe
next prev parent reply other threads:[~2019-11-25 10:45 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-29 16:44 [PATCH v2 00/13] cpuidle: psci: Support hierarchical CPU arrangement Ulf Hansson
2019-10-29 16:44 ` [PATCH v2 01/13] cpuidle: psci: Align psci_power_state count with idle state count Ulf Hansson
2019-10-29 16:44 ` [PATCH v2 02/13] dt: psci: Update DT bindings to support hierarchical PSCI states Ulf Hansson
2019-11-15 17:12 ` Sudeep Holla
2019-11-19 15:15 ` Ulf Hansson
2019-10-29 16:44 ` [PATCH v2 03/13] firmware: psci: Export functions to manage the OSI mode Ulf Hansson
2019-10-29 16:44 ` [PATCH v2 04/13] of: base: Add of_get_cpu_state_node() to get idle states for a CPU node Ulf Hansson
2019-10-29 16:44 ` [PATCH v2 05/13] cpuidle: dt: Support hierarchical CPU idle states Ulf Hansson
2019-10-29 16:44 ` [PATCH v2 06/13] cpuidle: psci: Simplify OF parsing of CPU idle state nodes Ulf Hansson
2019-11-15 17:13 ` Sudeep Holla
2019-10-29 16:44 ` [PATCH v2 07/13] cpuidle: psci: Support hierarchical CPU idle states Ulf Hansson
2019-10-29 16:44 ` [PATCH v2 08/13] cpuidle: psci: Add a helper to attach a CPU to its PM domain Ulf Hansson
2019-11-07 9:13 ` Niklas Cassel
2019-11-07 10:22 ` Ulf Hansson
2019-11-11 11:36 ` Niklas Cassel
2019-11-15 17:13 ` Sudeep Holla
2019-11-18 12:28 ` Ulf Hansson
2019-10-29 16:44 ` [PATCH v2 09/13] cpuidle: psci: Attach CPU devices to their PM domains Ulf Hansson
2019-11-15 17:15 ` Sudeep Holla
2019-11-18 12:50 ` Ulf Hansson
2019-10-29 16:44 ` [PATCH v2 10/13] cpuidle: psci: Prepare to use OS initiated suspend mode via " Ulf Hansson
2019-11-15 17:30 ` Sudeep Holla
2019-11-18 13:37 ` Ulf Hansson
2019-11-22 18:26 ` Lorenzo Pieralisi
2019-11-25 10:44 ` Ulf Hansson [this message]
2019-10-29 16:44 ` [PATCH v2 11/13] cpuidle: psci: Manage runtime PM in the idle path Ulf Hansson
2019-11-15 17:32 ` Sudeep Holla
2019-10-29 16:44 ` [PATCH v2 12/13] cpuidle: psci: Add support for PM domains by using genpd Ulf Hansson
2019-10-29 16:44 ` [PATCH v2 13/13] arm64: dts: Convert to the hierarchical CPU topology layout for MSM8916 Ulf Hansson
2019-11-11 11:00 ` [PATCH v2 00/13] cpuidle: psci: Support hierarchical CPU arrangement Ulf Hansson
2019-11-11 14:31 ` Sudeep Holla
2019-11-22 8:14 ` 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='CAPDyKFruoj_r3ktAHbVJAnnCZ6EP1dB9sZLE0=BZX=UziUYJag@mail.gmail.com' \
--to=ulf.hansson@linaro.org \
--cc=agross@kernel.org \
--cc=bjorn.andersson@linaro.org \
--cc=daniel.lezcano@linaro.org \
--cc=ilina@codeaurora.org \
--cc=khilman@kernel.org \
--cc=lina.iyer@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=lorenzo.pieralisi@arm.com \
--cc=mark.rutland@arm.com \
--cc=rjw@rjwysocki.net \
--cc=robh+dt@kernel.org \
--cc=sboyd@kernel.org \
--cc=sudeep.holla@arm.com \
--cc=vincent.guittot@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).