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:
> > 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 <sebcape@xxxxxxxxx>
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>
> > ---
> > arch/arm64/include/asm/psci.h | 4 ++
> > arch/arm64/kernel/psci.c | 103 ++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 107 insertions(+)
>
> [...]
>
> > +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.
> > /*
> > * 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.
> > + 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.
> > +
> > + 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
