Re: [PATCH 1/2] xen/arm: Initial support for PSCI-0.2

[Julien Grall <julien.grall@linaro.org>]

Hi Suravee,

On 10/01/2014 09:02 PM, suravee.suthikulpanit@amd.com wrote:
> +int __init psci_init_0_1(const struct dt_device_node *psci)
> +{
> +    int ret;
> +
> +    ret = psci_is_smc_method(psci);
> +    if ( ret )
> +        return -EINVAL;
> +
>      if ( !dt_property_read_u32(psci, "cpu_on", &psci_cpu_on_nr) )
>      {
>          printk("/psci node is missing the \"cpu_on\" property\n");
>          return -ENOENT;
>      }
>  
> -    psci_available = 1;
> +    psci_ver = 1;
>  
>      printk(XENLOG_INFO "Using PSCI for SMP bringup\n");

I would modify this printk into "Using PSCI 0.1...".

>  
>      return 0;
>  }
>  
> +int __init psci_init_0_2(const struct dt_device_node *psci)
> +{
> +    int ret;
> +
> +    ret = psci_is_smc_method(psci);
> +    if ( ret )
> +        return -EINVAL;
> +
> +    psci_ver = __invoke_psci_fn_smc(PSCI_0_2_FN_PSCI_VERSION, 0, 0, 0);
> +
> +    if ( psci_ver != 2 )
> +    {
> +        printk("Error: The retrieved PSCI version (%#x) does not support.\n", psci_ver);

NIT: s/does/is/ ?

> +        return -EOPNOTSUPP;
> +    }
> +
> +    psci_cpu_on_nr = PSCI_0_2_FN_CPU_ON;
> +
> +    printk(XENLOG_INFO "Using PSCI-0.2 for SMP bringup\n");
> +
> +    return 0;
> +}
> +
> +int __init psci_init(void)
> +{
> +    const struct dt_device_node *psci;
> +
> +    psci = dt_find_compatible_node(NULL, NULL, "arm,psci");
> +    if ( psci )
> +        return psci_init_0_1(psci);
> +
> +    psci = dt_find_compatible_node(NULL, NULL, "arm,psci-0.2");
> +    if ( psci )
> +        return psci_init_0_2(psci);
> +

I think we need to prefer PSCI 0.2 if the platform supports the both
version of PSCI.


> +    return -ENODEV;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h
> index 9777c03..ab37984 100644
> --- a/xen/include/asm-arm/psci.h
> +++ b/xen/include/asm-arm/psci.h
> @@ -13,10 +13,12 @@
>  #define PSCI_DISABLED               -8
>  
>  /* availability of PSCI on the host for SMP bringup */
> -extern bool_t psci_available;
> +extern int psci_ver;

I would use unsigned int here, or even better an enum to describe the
different version of PSCI.

Regards,

-- 
Julien Grall