On Wed, Jun 25, 2014 at 05:09:11PM +0100, Mark Rutland wrote:

On Wed, Jun 25, 2014 at 03:10:17PM +0100, Lorenzo Pieralisi wrote:

quoted

This patch implements the cpu_suspend cpu operations method through
the PSCI CPU_SUSPEND API. The PSCI implementation translates the idle state
index passed by the cpu_suspend core call into a valid PSCI state according to
the PSCI states initialized at boot by the PSCI suspend backend.

Entry point is set to cpu_resume physical address, that represents the
default kernel execution address following a CPU reset.

Idle state indices missing a DT node description are initialized to power
state standby WFI so that if called by the idle driver they provide the
default behaviour.

Reviewed-by: Sebastian Capella <redacted>
Signed-off-by: Lorenzo Pieralisi <redacted>
---
 arch/arm64/include/asm/psci.h |   4 ++
 arch/arm64/kernel/psci.c      | 103 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 107 insertions(+)

[...]

quoted

+static void psci_power_state_unpack(u32 power_state,
+				    struct psci_power_state *state)
+{
+	state->id = (power_state & PSCI_0_2_POWER_STATE_ID_MASK) >>
+			PSCI_0_2_POWER_STATE_ID_SHIFT;
+	state->type = (power_state & PSCI_0_2_POWER_STATE_TYPE_MASK) >>
+			PSCI_0_2_POWER_STATE_TYPE_SHIFT;
+	state->affinity_level =
+			(power_state & PSCI_0_2_POWER_STATE_AFFL_MASK) >>
+			PSCI_0_2_POWER_STATE_AFFL_SHIFT;
+}

Is this valid for PSCI versions prior to 0.2?

Yes, it should, as for the packing function.

quoted

 /*
  * The following two functions are invoked via the invoke_psci_fn pointer
  * and will not be inlined, allowing us to piggyback on the AAPCS.

@@ -199,6 +216,77 @@ static int psci_migrate_info_type(void)
 	return err;
 }
 
+int __init psci_dt_register_idle_states(struct cpuidle_driver *drv,
+					struct device_node *state_nodes[])
+{
+	int cpu, i;

Perhaps unsigned int? You print i with %u below.

Yes.

quoted

+	for (i = 0; i < drv->state_count; i++) {
+		u32 psci_power_state;
+
+		if (!state_nodes[i]) {
+			/*
+			 * An index with a missing node pointer falls back to
+			 * simple STANDBYWFI
+			 */
+			psci_states[i].type = PSCI_POWER_STATE_TYPE_STANDBY;
+			continue;
+		}

Does this make sense? Are there any limitations on which state nodes
could be missing?

I think the check is overkill, you are right.

quoted

+
+		if (of_property_read_u32(state_nodes[i], "entry-method-param",
+					 &psci_power_state)) {
+			pr_warn(" * %s missing entry-method-param property\n",
+				state_nodes[i]->full_name);
+			/*
+			 * If entry-method-param property is missing, fall
+			 * back to STANDBYWFI state
+			 */
+			psci_states[i].type = PSCI_POWER_STATE_TYPE_STANDBY;
+			continue;

Surely we want to throw away these states instead?

Otherwise we can get into a mess like:

psci_states[0] => low power state
psci_states[1] => lower power state
psci_states[2] => WFI / not low power
psci_states[3] => lowest power state

Where power usage and latency would jump around rather than follow
monotonic patterns.

I do not think that's a problem by itself, but honestly I think you have
a point. It is better to barf, throw away the states and avoid initializing
CPUidle to force a firmware update than keep going with a state that is
actually not doing what it probably was designed for, I just tried to be too
accommodating on this.

Thanks,
Lorenzo