[v5] power-mgmt : occ : Add 'freq-domain-indicator' DT property

Message ID 1516194276-21515-1-git-send-email-huntbag@linux.vnet.ibm.com
State Changes Requested
Headers show
Series
  • [v5] power-mgmt : occ : Add 'freq-domain-indicator' DT property
Related show

Commit Message

Abhishek Goel Jan. 17, 2018, 1:04 p.m.
Add a new device-tree property freq-domain-indicator to define group of
CPUs which would share same frequency. This property has been added under
power-mgmt node.

It is a bitmask which will have different value depending upon the
generation of the processor. Bitwise AND is taken between this bitmask
value and PIR of cpu. All the CPUs lying in the same frequency domain will
have same result for AND.

For example, for POWER8 0xFFF8 indicates core wide frequency domain.
Taking AND with the PIR of CPUs will yield us a frequency domain which is
core wide distribution as last 3 bits have been masked which represent the
threads.

For POWER9, 0xFFF0 indicates quad wide frequency domain. Taking AND with
the PIR of CPUs will yield us frequency domain which is quad wise
distribution as last 4 bits have been masked which represent the cores.

Signed-off-by: Abhishek Goel <huntbag@linux.vnet.ibm.com>
---

v4->v5 :
 * Added documentation for compatibility flags.
v3->v4 :
 * Added compatibility string "p9-occ-quirk"
v2->v3 :
 * Added compatibility string "freq-domain-v1"
v1->v2 :
 * Handled errors if device tree node creation failed.

 doc/device-tree/ibm,opal/power-mgt/occ.rst | 36 ++++++++++++++++++++++++++++++
 hw/occ.c                                   | 18 +++++++++++++++
 2 files changed, 54 insertions(+)

Comments

Stewart Smith Aug. 7, 2018, 3:49 a.m. | #1
Abhishek Goel <huntbag@linux.vnet.ibm.com> writes:
> Add a new device-tree property freq-domain-indicator to define group of
> CPUs which would share same frequency. This property has been added under
> power-mgmt node.
>
> It is a bitmask which will have different value depending upon the
> generation of the processor. Bitwise AND is taken between this bitmask
> value and PIR of cpu. All the CPUs lying in the same frequency domain will
> have same result for AND.
>
> For example, for POWER8 0xFFF8 indicates core wide frequency domain.
> Taking AND with the PIR of CPUs will yield us a frequency domain which is
> core wide distribution as last 3 bits have been masked which represent the
> threads.
>
> For POWER9, 0xFFF0 indicates quad wide frequency domain. Taking AND with
> the PIR of CPUs will yield us frequency domain which is quad wise
> distribution as last 4 bits have been masked which represent the
> cores.

Sorry it's taken so long to get back to this patch!

I like the idea, I don't think we have a current sensible way to expose
this, and thus the choices left to an OS are either to have this
knowledge baked in for each processor generation, or to try to do things
that can't be done, possibly leading to not as good decisions.

> diff --git a/doc/device-tree/ibm,opal/power-mgt/occ.rst b/doc/device-tree/ibm,opal/power-mgt/occ.rst
> index d13a62b..77d4258 100644
> --- a/doc/device-tree/ibm,opal/power-mgt/occ.rst
> +++ b/doc/device-tree/ibm,opal/power-mgt/occ.rst
> @@ -37,3 +37,39 @@ ibm,pstate-vcss ibm,pstate-vdds
>  These properties list a voltage-identifier of each of the pstates listed in
>  ibm,pstate-ids for the Vcs and Vdd values used for that pstate in that chip.
>  Each VID is a single byte.
> +
> +ibm,opal/power-mgt/ibm,freq-domain-indicator

I think 'frequency-domain-mask' would be better as it's a property so it
doesn't need the 'ibm,' prefix (it's already in the OPAL specific
'ibm,opal' hierarchy).

The definition should be that if present, then an OS can use this as an
indicator of how the hardware/firmware behaves. If not present, then
each *can* be treated separately but this may not reflect what frequency
the hardware runs at. For any new scheme, we can add a property
describing it.


> +--------------------------------------------
> +
> +This property is a bitmask which will have different value depending upon the
> +generation of the processor. Frequency domain would indicate group of CPUs
> +which would share same frequency. Bitwise AND is taken between this bitmask
> +value and PIR of cpu. All the CPUs lying in the same frequency domain will have
> +same result for AND. Thus frequency management can be done based on frequency
> +domain. A frequency domain may be a core or a quad, etc depending upon the
> +generation of the processor.
> +
> +For example, for POWER8 0xFFF8 indicates core wide frequency domain. Taking AND
> +with the PIR of CPUs will yield us a frequency domain which is core wide
> +distribution as last 3 bits have been masked which represent the threads.
> +
> +For POWER9, 0xFFF0 indicates quad wide frequency domain. Taking AND with
> +the PIR of CPUs will yield us frequency domain which is quad wise
> +distribution as last 4 bits have been masked which represent the cores.
> +
> +ibm,opal/power-mgt/compatible
> +--------------------------------------------
> +
> +`p9-occ-quirk`
> +
> +It represents the current DVFS implementation in firmware. To set a frequency
> +in a quad, all cores of the quad need to set frequency in their respective
> +PMCR's. Ideally setting frequency on any of the core of that quad should change
> +frequency for the quad.

I'm not sure if these should be in the compatible property or not... it
doesn't seem quite right, but more that it should be a property in the
power-mgt node.

I don't like the 'p9-occ-quirk' name, perhaps instead we should have a
property with a value indicating what frequency the hardware actually
runs at? 'domain-runs-at' = 'max-in-domain' | 'most-recently-set' or
something? (I don't like those names too much either, so improvements
are welcome). The idea being that how the hardware/firmware behaves is
certainly *one* of these, but never more than.

> +
> +`freq-domain-v1`
> +
> +This compatibility string helps kernel to interpret freq-domain-indicator's
> +format. If in future we have different format for freq-domain-indicator, future
> +firmwares running newer kernel will not interpret freq-domain-indicator
> +incorrectly.

I don't think we need this, it's taken care of by the implicit
presence/absence of the property.

Patch

diff --git a/doc/device-tree/ibm,opal/power-mgt/occ.rst b/doc/device-tree/ibm,opal/power-mgt/occ.rst
index d13a62b..77d4258 100644
--- a/doc/device-tree/ibm,opal/power-mgt/occ.rst
+++ b/doc/device-tree/ibm,opal/power-mgt/occ.rst
@@ -37,3 +37,39 @@  ibm,pstate-vcss ibm,pstate-vdds
 These properties list a voltage-identifier of each of the pstates listed in
 ibm,pstate-ids for the Vcs and Vdd values used for that pstate in that chip.
 Each VID is a single byte.
+
+ibm,opal/power-mgt/ibm,freq-domain-indicator
+--------------------------------------------
+
+This property is a bitmask which will have different value depending upon the
+generation of the processor. Frequency domain would indicate group of CPUs
+which would share same frequency. Bitwise AND is taken between this bitmask
+value and PIR of cpu. All the CPUs lying in the same frequency domain will have
+same result for AND. Thus frequency management can be done based on frequency
+domain. A frequency domain may be a core or a quad, etc depending upon the
+generation of the processor.
+
+For example, for POWER8 0xFFF8 indicates core wide frequency domain. Taking AND
+with the PIR of CPUs will yield us a frequency domain which is core wide
+distribution as last 3 bits have been masked which represent the threads.
+
+For POWER9, 0xFFF0 indicates quad wide frequency domain. Taking AND with
+the PIR of CPUs will yield us frequency domain which is quad wise
+distribution as last 4 bits have been masked which represent the cores.
+
+ibm,opal/power-mgt/compatible
+--------------------------------------------
+
+`p9-occ-quirk`
+
+It represents the current DVFS implementation in firmware. To set a frequency
+in a quad, all cores of the quad need to set frequency in their respective
+PMCR's. Ideally setting frequency on any of the core of that quad should change
+frequency for the quad.
+
+`freq-domain-v1`
+
+This compatibility string helps kernel to interpret freq-domain-indicator's
+format. If in future we have different format for freq-domain-indicator, future
+firmwares running newer kernel will not interpret freq-domain-indicator
+incorrectly.
diff --git a/hw/occ.c b/hw/occ.c
index f3f1231..cdd4928 100644
--- a/hw/occ.c
+++ b/hw/occ.c
@@ -47,6 +47,9 @@ 
 #define MAX_OPAL_CMD_DATA_LENGTH	4090
 #define MAX_OCC_RSP_DATA_LENGTH		8698
 
+#define P8_PIR_CORE_MASK		0xFFF8
+#define P9_PIR_QUAD_MASK		0xFFF0
+
 /**
  * OCC-OPAL Shared Memory Region
  *
@@ -488,6 +491,7 @@  static bool add_cpu_pstate_properties(int *pstate_nom)
 	u8 nr_pstates;
 	bool ultra_turbo_supported;
 	int i;
+	u32 freq_domain_indicator = 0;
 
 	prlog(PR_DEBUG, "OCC: CPU pstate state device tree init\n");
 
@@ -660,6 +664,13 @@  static bool add_cpu_pstate_properties(int *pstate_nom)
 		return false;
 	}
 
+	if (proc_gen == proc_gen_p8)
+		freq_domain_indicator = P8_PIR_CORE_MASK;
+	else if (proc_gen == proc_gen_p9)
+		freq_domain_indicator = P9_PIR_QUAD_MASK;
+	else
+		prerror("OCC: freq-domain-indicator: Processor is not supported\n");
+
 	/* Add the device-tree entries */
 	dt_add_property(power_mgt, "ibm,pstate-ids", dt_id,
 			nr_pstates * sizeof(u32));
@@ -669,6 +680,13 @@  static bool add_cpu_pstate_properties(int *pstate_nom)
 	dt_add_property_cells(power_mgt, "ibm,pstate-nominal", pnom);
 	dt_add_property_cells(power_mgt, "ibm,pstate-max", pmax);
 
+	if (freq_domain_indicator) {
+		dt_add_property_cells(power_mgt, "ibm,freq-domain-indicator",
+						freq_domain_indicator);
+		dt_add_property_strings(power_mgt, "compatible",
+					"freq-domain-v1", "p9-occ-quirk");
+	}
+
 	free(dt_freq);
 	free(dt_id);