Message-Id: <20170125080900.GA10276@in.ibm.com>
Date:   Wed, 25 Jan 2017 13:39:00 +0530
From:   Gautham R Shenoy <ego@...ux.vnet.ibm.com>
To:     Rob Herring <robh@...nel.org>
Cc:     "Gautham R. Shenoy" <ego@...ux.vnet.ibm.com>,
        Michael Ellerman <mpe@...erman.id.au>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        Michael Neuling <mikey@...ling.org>,
        Vaidyanathan Srinivasan <svaidy@...ux.vnet.ibm.com>,
        "Shreyas B. Prabhu" <shreyasbp@...il.com>,
        Shilpasri G Bhat <shilpa.bhat@...ux.vnet.ibm.com>,
        Stewart Smith <stewart@...ux.vnet.ibm.com>,
        Balbir Singh <bsingharora@...il.com>,
        "Oliver O'Halloran" <oohall@...il.com>,
        linuxppc-dev@...ts.ozlabs.org, linux-kernel@...r.kernel.org,
        linux-pm@...r.kernel.org, devicetree@...r.kernel.org,
        mark.rutland@....com
Subject: Re: [PATCH v5 5/5] Documentation:powerpc: Add device-tree bindings
 for power-mgt

Hello Rob,

Thank you very much for your review. I had missed this mail
and found it while looking at the lkml thread while preparing for the
next iteration.

On Fri, Jan 13, 2017 at 10:57:43AM -0600, Rob Herring wrote:
> On Tue, Jan 10, 2017 at 02:37:04PM +0530, Gautham R. Shenoy wrote:
> > From: "Gautham R. Shenoy" <ego@...ux.vnet.ibm.com>
> > 
> > Document the device-tree bindings defining the the properties under
> > the @power-mgt node in the device tree that describe the idle states
> > for Linux running on baremetal POWER servers.
> > 
> 
> We have "common" idle state bindings. Perhaps some explanation why those 
> can't be used.

Are you referring to
Documentation/devicetree/bindings/power/domain-idle-state.txt ?

On POWER, since POWER8 time, the DVFS states as well as the idle
states were exposed as properties under the common /ibm,opal/power-mgt
node. Since the DVFS states were large in number, creating a separate
node for each one of them didn't seem like a good design. Hence the
DVFS state properties were encoded as property arrays. The same design
was carried forward to the idle-states as well.

Which is why the common idle-state bindings in domain-idle-state.txt
cannot be used since each idle-state is described as a node there.

> 
> > Signed-off-by: Gautham R. Shenoy <ego@...ux.vnet.ibm.com>
> > ---
> > [v4]-> [v5]: Fixed a couple of typos.
> > 
> >  .../devicetree/bindings/powerpc/opal/power-mgt.txt | 125 +++++++++++++++++++++
> >  1 file changed, 125 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/powerpc/opal/power-mgt.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/powerpc/opal/power-mgt.txt b/Documentation/devicetree/bindings/powerpc/opal/power-mgt.txt
> > new file mode 100644
> > index 0000000..4967831
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/powerpc/opal/power-mgt.txt
> > @@ -0,0 +1,125 @@
> > +IBM Power-Management Bindings
> > +=============================
> > +
> > +Linux running on baremetal POWER machines has access to the processor
> > +idle states. The description of these idle states is exposed via the
> > +node @power-mgt in the device-tree by the firmware.
> > +
> > +Definitions:
> > +----------------
> > +Typically each idle state has the following associated properties:
> > +
> > +- name: The name of the idle state as defined by the firmware.
> > +
> > +- flags: indicating some aspects of this idle states such as the
> > +         extent of state-loss, whether timebase is stopped on this
> > +         idle states and so on. The flag bits are as follows:
> > +
> > +- exit-latency: The latency involved in transitioning the state of the
> > +		CPU from idle to running.
> > +
> > +- target-residency: The minimum time that the CPU needs to reside in
> > +		    this idle state in order to accrue power-savings
> > +		    benefit.
> > +
> > +Properties
> > +----------------
> > +The following properties provide details about the idle states. These
> > +properties are optional unless mentioned otherwise below.
> 
> -names is optional but everything else seems to require it. It is not 
> clear what the binding looks like if -names is not present.

The firmware could choose not to expose the idle-states to the kernel,
in which case, it would expose any of these properties.

However, if the firmware chooses to expose idle-states, then the 
ibm,cpu-idle-state-names and ibm,cpu-idle-state-flags properties are
required.

I will reword this as follows:

  The following properties provide details about the idle
  states. These properties are exposed as arrays. Each entry in the
  property array provides the value of that property for the idle
  state associated with the array index of that entry.

  If idle-states are defined, then the properties
  "ibm,cpu-idle-state-names" and "ibm,cpu-idle-state-flags" are
  required. The other properties are required unless mentioned
  otherwise. The length of all the property arrays must be the same.


> 
> > +
> > +- ibm,cpu-idle-state-names:
> > +	Array of strings containing the names of the idle states.
> > +
> > +- ibm,cpu-idle-state-flags:
> > +	Array of unsigned 32-bit values containing the values of the
> > +	flags associated with the the aforementioned idle-states. This
> > +	property is required on POWER9 whenever
> > +	ibm,cpu-idle-state-names is defined and the length of this
> > +	property array should be the same as
> > +	ibm,-cpu-idle-state-names.The flag bits are as follows:
> 
> s/ibm,-cpu/ibm,cpu/
> 
> Needs a space after the period.

Thanks for catching this. Will fix it in the next iteration.

> 
> > +		0x00000001 /* Decrementer would stop */
> > +		0x00000002 /* Needs timebase restore */
> > +		0x00001000 /* Restore GPRs like nap */
> > +		0x00002000 /* Restore hypervisor resource from PACA pointer */
> > +		0x00004000 /* Program PORE to restore PACA pointer */
> > +		0x00010000 /* This is a nap state */
> > +		0x00020000 /* This is a fast-sleep state */
> > +		0x00040000 /* This is a winkle state */
> > +		0x00080000 /* This is a fast-sleep state which requires a */
> > +			   /* software workaround for restoring the timebase*/
> > +		0x00800000 /* This state uses SPR PMICR instruction */
> > +		0x00100000 /* This is a fast stop state */
> > +		0x00200000 /* This is a deep-stop state */
> > +
> > +- ibm,cpu-idle-state-latencies-ns:
> > +	Array of unsigned 32-bit values containing the values of the
> > +	exit-latencies (in ns) for the idle states in
> > +	ibm,cpu-idle-state-names. This property is required whenever
> > +	ibm,cpu-idle-state-names is defined and the length of this
> > +	property array should be the same as
> > +	ibm,-cpu-idle-state-names.
> 
> Same typo.

Will fix this one as well.

> 
> > +
> > +- ibm,cpu-idle-state-residency-ns:
> > +	Array of unsigned 32-bit values containing the values of the
> > +	target-residency (in ns) for the idle states in
> > +	ibm,cpu-idle-state-names. On POWER8 this is an optional
> > +	property. If the property is absent, the target residency for
> > +	the "Nap", "FastSleep" are defined to 10000 and 300000000
> > +	respectively. On POWER9 this property must be defined if
> > +	ibm,cpu-idle-state-names is defined and the length should be
> > +	same as that of ibm,cpu-idle-state-names.
> > +
> > +- ibm,cpu-idle-state-psscr:
> > +	Array of unsigned 64-bit values containing the values for the
> > +	PSSCR for each of the idle states in ibm,cpu-idle-state-names.
> > +	This property is required on POWER9 whenever
> > +	ibm,cpu-idle-state-names is defined and the length of this
> > +	property array should be the same as
> > +	ibm,cpu-idle-state-names.
> > +
> > +- ibm,cpu-idle-state-psscr-mask:
> > +	Array of unsigned 64-bit values containing the masks
> > +	indicating which psscr fields are set in the corresponding
> > +	entries of ibm,cpu-idle-state-psscr.  This property is
> > +	required on POWER9 whenever ibm,cpu-idle-state-names is
> > +	defined and the length of this property array should be the
> > +	same as ibm,cpu-idle-state-names.
> > +
> > +	Whenever the firmware sets an entry in
> > +	ibm,cpu-idle-state-psscr-mask value to 0xf, it implies that
> > +	only the Requested Level (RL) field of the corresponding entry
> > +	in ibm,cpu-idle-state-psscr should be considered by the
> > +	kernel. For such idle states, the kernel would set the
> > +	remaining fields of the psscr to the following sane-default
> > +	values.
> > +
> > +		- ESL and EC bits are to 1. So wakeup from any stop
> > +		  state will be at vector 0x100.
> > +
> > +		- MTL and PSLL are set to the maximum allowed value as
> > +		  per the ISA, i.e. 15.
> > +
> > +		- The Transition Rate, TR is set to the Maximum value
> > +                  3.
> > +
> > +	For all the other values of the entry in
> > +	ibm,cpu-idle-state-psscr-mask, the Kernel expects all the
> > +	psscr fields of the corresponding entry in
> > +	ibm,cpu-idle-state-psscr to be correctly set by the firmware.
> > +
> > +- ibm,cpu-idle-state-pmicr:
> > +	Array of unsigned 64-bit values containing the pmicr values
> > +	for the idle states in ibm,cpu-idle-state-names. This 64-bit
> > +	register value is to be set in pmicr for the corresponding
> > +	state if the flag indicates that pmicr SPR should be set. This
> > +	is an optional property on POWER8 and is absent on
> > +	POWER9. When present on POWER8, the length of this property
> > +	array should be the same as ibm,cpu-idle-state-names.
> > +
> > +- ibm,cpu-idle-state-pmicr:-mask
> > +	Array of unsigned 64-bit values containing the mask indicating
> > +	which of the fields of the PMICR are set in the corresponding
> > +	entries in ibm,cpu-idle-state-pmicr. This is an optional
> > +	property on POWER8 and is absent on POWER9. When present on
> > +	POWER8, the length of this property array should be the same
> > +	as ibm,cpu-idle-state-names.
> > -- 
> > 1.9.4
> > 
> 

Powered by blists - more mailing lists