[1/4] arm: parse PSCI node from the host device-tree

The availability of a PSCI handler is advertised in the DTB.
Find and parse the node (described in the Linux device-tree binding)
and save the function number for bringing up a CPU for later usage.
We do some sanity checks, especially we deny using HVC as a callling
method, as it does not make much sense currently under Xen.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---
 xen/arch/arm/smpboot.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

On Mon, 2013-11-25 at 13:02 +0100, Andre Przywara wrote:
> The availability of a PSCI handler is advertised in the DTB.
> Find and parse the node (described in the Linux device-tree binding)
> and save the function number for bringing up a CPU for later usage.
> We do some sanity checks, especially we deny using HVC as a callling

Too many l's in callling.

> method, as it does not make much sense currently under Xen.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> ---
>  xen/arch/arm/smpboot.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index 6c90fa6..97bd414 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -89,6 +89,44 @@ smp_clear_cpu_maps (void)
>      cpu_logical_map(0) = READ_SYSREG(MPIDR_EL1) & MPIDR_HWID_MASK;
>  }
>  
> +uint32_t psci_host_cpu_on_nr;

Can we make this static and have a global "psci_enable" flag to use
elsewhere please.

> +static int __init psci_host_init(void)

s/host// I think? Generally we have e.g. gic_init for the host and
vgic_init for the guest.

> +{
> +    struct dt_device_node *psci;
> +    int ret;
> +    const char *prop_str;
> +
> +    psci = dt_find_compatible_node(NULL, NULL, "arm,psci");
> +    if ( !psci )
> +        return 1;

You've made up some return codes here? Please can you return -Efoo
(whichever one(s) seem appropriate).

If you need to distinguish PSCI not present from PSCI present but buggy
then perhaps use ENODEV or something similarly unique+relevant for the
former case?

> +    ret = dt_property_read_string(psci, "method", &prop_str);
> +    if ( ret )
> +    {
> +        printk("/psci node does not provide a method (%d)\n", ret);
> +        return 2;
> +    }
> +
> +    /* Since Xen runs in HYP all of the time, it does not make sense to
> +     * let it call into HYP for PSCI handling, since the handler won't
> +     * just be there. So bail out with an error if "smc" is not used.
> +     */
> +    if ( strcmp(prop_str, "smc") )
> +    {
> +        printk("/psci method must be smc, but is: \"%s\"\n", prop_str);
> +        return 3;
> +    }
> +
> +    if ( !dt_property_read_u32(psci, "cpu_on", &psci_host_cpu_on_nr) )
> +    {
> +        printk("/psci node is missing the \"cpu_on\" property\n");
> +        return 4;
> +    }

http://www.spinics.net/lists/devicetree/msg05348.html updates the
bindings for PSCI 0.2. I suppose Midway must only support 0.1?

I'd be OK with only supporting 0.1 right now, but it would be useful to
comment either in the code or in the commit message.

(nb: I'm not sure of v4 was the final version of that series)

> +
> +    return 0;
> +}
> +
>  /* Parse the device tree and build the logical map array containing
>   * MPIDR values related to logical cpus
>   * Code base on Linux arch/arm/kernel/devtree.c
> @@ -107,6 +145,11 @@ void __init smp_init_cpus(void)
>      bool_t bootcpu_valid = 0;
>      int rc;
>  
> +    if ( psci_host_init() == 0 )
> +    {
> +        printk(XENLOG_INFO "Using PSCI for SMP bringup\n");
> +    }
> +
>      if ( (rc = arch_smp_init()) < 0 )

arch_smp_init is empty on both platforms. arm32 has a comment "TODO:
PSCI" ;-)

I think we can nuke this function while we are here, since it's only
purpose was as a PSCI placehoder.

Ian.

On Tue, 2013-11-26 at 11:12 +0000, Ian Campbell wrote:
> On Mon, 2013-11-25 at 13:02 +0100, Andre Przywara wrote:
> > The availability of a PSCI handler is advertised in the DTB.
> > Find and parse the node (described in the Linux device-tree binding)
> > and save the function number for bringing up a CPU for later usage.
> > We do some sanity checks, especially we deny using HVC as a callling
> 
> Too many l's in callling.
> 
> > method, as it does not make much sense currently under Xen.
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> > ---
> >  xen/arch/arm/smpboot.c | 43 +++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 43 insertions(+)
> > 
> > diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> > index 6c90fa6..97bd414 100644
> > --- a/xen/arch/arm/smpboot.c
> > +++ b/xen/arch/arm/smpboot.c
> > @@ -89,6 +89,44 @@ smp_clear_cpu_maps (void)
> >      cpu_logical_map(0) = READ_SYSREG(MPIDR_EL1) & MPIDR_HWID_MASK;
> >  }
> >  
> > +uint32_t psci_host_cpu_on_nr;
> 
> Can we make this static and have a global "psci_enable" flag to use
> elsewhere please.

I think it will have become clear in my comments on patch #0/#2 but also
can we have this in a separate psci.c please.

It would be useful to have a no-psci command line option too, please.

Ian.

On 11/26/2013 12:12 PM, Ian Campbell wrote:
> On Mon, 2013-11-25 at 13:02 +0100, Andre Przywara wrote:
>> +
>> +    if ( !dt_property_read_u32(psci, "cpu_on", &psci_host_cpu_on_nr) )
>> +    {
>> +        printk("/psci node is missing the \"cpu_on\" property\n");
>> +        return 4;
>> +    }
>
> http://www.spinics.net/lists/devicetree/msg05348.html updates the
> bindings for PSCI 0.2. I suppose Midway must only support 0.1?
>
> I'd be OK with only supporting 0.1 right now, but it would be useful to
> comment either in the code or in the commit message.
>
> (nb: I'm not sure of v4 was the final version of that series)

If I look at http://www.spinics.net/lists/devicetree/msg12293.html
I would keep it as it is - at least until the binding changes. Hopefully 
there will be a compatibility fallback if cpu_on should change to 
cpu_on-{32,64}. I will talk to Rob about this and will later send a fix 
if that is needed.
Midway provides an implementation conforming to PSCI 0.2, however the 
device tree binding is still the one from the current kernel git HEAD.

>
>> +
>> +    return 0;
>> +}
>> +
>>   /* Parse the device tree and build the logical map array containing
>>    * MPIDR values related to logical cpus
>>    * Code base on Linux arch/arm/kernel/devtree.c
>> @@ -107,6 +145,11 @@ void __init smp_init_cpus(void)
>>       bool_t bootcpu_valid = 0;
>>       int rc;
>>
>> +    if ( psci_host_init() == 0 )
>> +    {
>> +        printk(XENLOG_INFO "Using PSCI for SMP bringup\n");
>> +    }
>> +
>>       if ( (rc = arch_smp_init()) < 0 )
>
> arch_smp_init is empty on both platforms. arm32 has a comment "TODO:
> PSCI" ;-)
>
> I think we can nuke this function while we are here, since it's only
> purpose was as a PSCI placehoder.
>

But arch_smp_init() calls platform_smp_init(), which is not empty for 
the Exynos, VExpress and OMAP5 (it contains the native SMP bringup for 
these platforms).
So shall I skip the superfluous arch_smp_init() and call 
platform_smp_init() directly from smpboot.c?
Or keep it as it is and we clean it up later?

Thanks,
Andre.