From: Gautham R Shenoy <ego@linux.vnet.ibm.com>
To: Michael Ellerman <mpe@ellerman.id.au>
Cc: Akshay Adiga <akshay.adiga@linux.vnet.ibm.com>,
linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
npiggin@gmail.com, benh@kernel.crashing.org,
ego@linux.vnet.ibm.com, huntbag@linux.vnet.ibm.com
Subject: Re: [RFC PATCH 0/3] New device-tree format and Opal based idle save-restore
Date: Wed, 8 Aug 2018 11:32:00 +0530 [thread overview]
Message-ID: <20180808060200.GA17634@in.ibm.com> (raw)
In-Reply-To: <87zhxybceu.fsf@concordia.ellerman.id.au>
Hello Michael,
On Tue, Aug 07, 2018 at 10:15:37PM +1000, Michael Ellerman wrote:
> > Skiboot patch-set for device-tree is posted here :
> > https://patchwork.ozlabs.org/project/skiboot/list/?series=58934
>
> I don't see a device tree binding documented anywhere?
>
> There is an existing binding defined for ARM chips, presumably it
> doesn't do everything we need. But are there good reasons why we are not
> using it as a base?
>
> See: Documentation/devicetree/bindings/arm/idle-states.txt
>
In case of ARM, the idle-states node is a child of cpus node. Each
child of the idle-states node is a node describing that particular
idle state.
idle-states {
entry-method = "psci";
CPU_RETENTION_0_0: cpu-retention-0-0 {
compatible = "arm,idle-state";
arm,psci-suspend-param = <0x0010000>;
entry-latency-us = <20>;
exit-latency-us = <40>;
min-residency-us = <80>;
status = "disabled"
};
CPU_SLEEP_0_0: cpu-sleep-0-0 {
compatible = "arm,idle-state";
local-timer-stop;
arm,psci-suspend-param = <0x0010000>;
entry-latency-us = <250>;
exit-latency-us = <500>;
min-residency-us = <950>;
status = "okay"
};
.
.
.
}
Furthermore, each CPU can have a different set of cpu-idle states
due to the asymmetric nature of the processors units on the board.
Thus, there is an additional property for each cpu called
cpu-idle-states which points to the containers of the idle states
themselves.
cpus {
#size-cells = <0>;
#address-cells = <2>;
CPU0: cpu@0 {
device_type = "cpu";
compatible = "arm,cortex-a57";
reg = <0x0 0x0>;
enable-method = "psci";
cpu-idle-states = <&CPU_RETENTION_0_0 &CPU_SLEEP_0_0
&CLUSTER_RETENTION_0 &CLUSTER_SLEEP_0>;
};
. . .
. . .
. . .
. . .
CPU8: cpu@100000000 {
device_type = "cpu";
compatible = "arm,cortex-a53";
reg = <0x1 0x0>;
enable-method = "psci";
cpu-idle-states = <&CPU_RETENTION_1_0 &CPU_SLEEP_1_0
&CLUSTER_RETENTION_1 &CLUSTER_SLEEP_1>;
};
In our case, we already have an "ibm,opal/power-mgt/" node in the
device tree where we have defined the idle state so far. This was the
reason to stick the new device tree format under this existing node
that has been specially earmarked for power management related bits,
instead of defining the new format under the cpus node.
Also, in our case, since all the CPU nodes are symmetric they will
have the same set of idle states. Hence, we wouldn't need the
"cpu-idle-states" property for each CPU.
As for the properties of idle states themselves, the only common
things between the ARM idle-states and our case are the compatible,
exit-latency-us, min-residency-us. In addition to this we need the
flags which indicate the nature of the resource loss (Hypervisors
state loss, Timebase loss, etc..) , the psscr_val and the psscr_mask
corresponding to the stop states which the ARM device-tree doesn't
provide.
For this reason we have opted for a new bindings since the overlap
between these two platforms is minimal.
>
> The way you're using compatible is not really consistent with its
> traditional meaning.
>
> eg, you have multiple states with:
>
> compatible = "ibm,state-v1",
> "cpuoffline",
> "opal-supported";
>
>
> This would typically mean that all those state are all "compatible" with
> some semantics defined by the name "ibm,state-v1". What you're trying to
> say (I think) is that each state is "version 1" of *that state*. And
> only kernels that understand version 1 should use the state.
Ok, I see what you mean here. Perhaps, we should have had something
like "ibm,stop0-v1" , "ibm,stop1-v2", "ibm,stop2-v2" etc, where
version1, version2 etc, pertains to the versions of those specific
states.
Thus a kernel that knows about "version 1" of stop0 and stop2 and
"version 2" of stop1 will end up using only stop0 and stop1 since it
doesn't know "version 2" of stop2.
In such a case, kernel should fallback to OPAL for stop2. Does this
make sense ?
>
> And "cpuoffline" and "opal-supported" definitely don't belong in
> compatible AFAICS, they should simply be boolean properties of the
> node.
I agree. These should be flags.
>
> cheers
>
prev parent reply other threads:[~2018-08-08 6:02 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-08-02 4:51 [RFC PATCH 0/3] New device-tree format and Opal based idle save-restore Akshay Adiga
2018-08-02 4:51 ` [RFC PATCH 1/3] cpuidle/powernv: Add support for states with ibm, cpuidle-state-v1 Akshay Adiga
2018-08-02 4:51 ` [RFC PATCH 2/3] powernv/cpuidle: Pass pointers instead of values to stop loop Akshay Adiga
2018-08-02 4:51 ` [RFC PATCH 3/3] cpuidle/powernv: Conditionally save-restore sprs using opal Akshay Adiga
2018-08-02 14:05 ` Nicholas Piggin
2018-08-08 15:41 ` Gautham R Shenoy
2018-08-11 5:54 ` Nicholas Piggin
2018-08-07 12:15 ` [RFC PATCH 0/3] New device-tree format and Opal based idle save-restore Michael Ellerman
2018-08-08 6:02 ` Gautham R Shenoy [this message]
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=20180808060200.GA17634@in.ibm.com \
--to=ego@linux.vnet.ibm.com \
--cc=akshay.adiga@linux.vnet.ibm.com \
--cc=benh@kernel.crashing.org \
--cc=huntbag@linux.vnet.ibm.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=mpe@ellerman.id.au \
--cc=npiggin@gmail.com \
/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).