Re: [PATCH v6 20/25] drivers: firmware: psci: Introduce psci_dt_topology_init()

[Mark Rutland]

On Tue, Apr 10, 2018 at 09:19:26AM +0200, Ulf Hansson wrote:
> On 14 March 2018 at 18:38, Mark Rutland <mark.rutland@xxxxxxx> wrote:
> > On Wed, Mar 14, 2018 at 05:58:30PM +0100, Ulf Hansson wrote:
> >> In case the hierarchical layout is used in DT, we want to initialize the
> >> corresponding PM domain topology for the CPUs, by using the generic PM
> >> domain (aka genpd) infrastructure.
> >>
> >> At first glance, it may seem feasible to hook into the existing
> >> psci_dt_init() function, although because it's called quite early in the
> >> boot sequence, allocating the dynamic data structure for a genpd doesn't
> >> work.
> >>
> >> Therefore, let's export a new init function for PSCI,
> >> psci_dt_topology_init(), which the ARM machine code should call from a
> >> suitable initcall.
> >>
> >> Succeeding to initialize the PM domain topology, which means at least one
> >> instance of a genpd becomes created, allows us to continue to enable the
> >> PSCI OS initiated mode for the platform. If everything turns out fine,
> >> let's print a message in log to inform the user about the changed mode.
> >>
> >> In case of any failures, we stick to the default PSCI Platform Coordinated
> >> mode.
> >
> > For kexec/kdump we'll need to explicitly set the suspend mode to
> > platform coordinated if for whatever reason we choose not to use OSI.
> 
> Could you please elaborate on this? I am not really understanding what
> you are suggesting me to do and why.

Sorry for not being clear.

What I'd like to see is that we always call SET_SUSPEND_MODE before
invoking CPU_SUSPEND. Either deciding early if we can use OSI, or always
setting it to platform co-ordinated early on in boot and later switching
it over.

That way we can be sure of the suspend mode, even if we've kexec'd from
a kernel that had fiddled with it.

> >> Cc: Lina Iyer <ilina@xxxxxxxxxxxxxx>
> >> Co-developed-by: Lina Iyer <lina.iyer@xxxxxxxxxx>
> >> Signed-off-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
> >> ---
> >>  drivers/firmware/psci.c | 31 +++++++++++++++++++++++++++++++
> >>  include/linux/psci.h    |  2 ++
> >>  2 files changed, 33 insertions(+)
> >>
> >> diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
> >> index 463f78c..45d55fc 100644
> >> --- a/drivers/firmware/psci.c
> >> +++ b/drivers/firmware/psci.c
> >> @@ -723,6 +723,37 @@ int __init psci_dt_init(void)
> >>       return ret;
> >>  }
> >>
> >> +int __init psci_dt_topology_init(void)
> >> +{
> >> +     struct device_node *np;
> >> +     int ret;
> >> +
> >> +     if (!psci_has_osi_support())
> >> +             return 0;
> >> +
> >> +     np = of_find_matching_node_and_match(NULL, psci_of_match, NULL);
> >> +     if (!np)
> >> +             return -ENODEV;
> >> +
> >> +     /* Initialize the CPU PM domains based on topology described in DT. */
> >> +     ret = psci_dt_init_pm_domains(np);
> >> +     if (ret <= 0)
> >> +             goto out;
> >> +
> >> +     /* Enable OSI mode. */
> >> +     ret = invoke_psci_fn(PSCI_1_0_FN_SET_SUSPEND_MODE,
> >> +                          PSCI_1_0_SUSPEND_MODE_OSI, 0, 0);
> >
> > IIUC, this patch will need to be moved after the subsequent two patches
> > to ensure that this series bisects cleanly.
> 
> Actually not.
> 
> $subject patch adds a new init function, which purpose is to
> initialize the PSCI PM domains. At this point none is calling it.

Ah. I missed the fact there were no callers. Sorry for the noise!

Thanks,
Mark.