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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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@xxxxxxxxxx>
> ---
>  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.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel