Re: [PATCH 1/2] xen/arm: Add support of PSCI v1.0 for the host

[Ian Campbell <ian.campbell@citrix.com>]

On Thu, 2015-10-08 at 19:45 +0100, Julien Grall wrote:
> From Xen point of view, PSCI v0.2 and PSCI v1.0 are very similar. All
> the PSCI calls used within Xen (PSCI_VERSION, CPU_ON, SYSTEM_OFF and
> SYSTEM_RESET) behaves exactly the same.
> 
> While there is no compatible string to represent PSCI v1.0 in the DT,
> it's possible to detect it using the function PSCI_VERSION.
> 
> The compatible string is now used to detect if the platform may support
> PSCI v0.2 or higher.

The actual implementation here looks for precisely 0.2 or 1.0, not >= 0.2
as suggested by this statement.

The PSCI 1.0 spec says (section 5.3.1, intended use of PSCI_VERSION) that
for any 1.y version must be compatible with 1.x when y>x (for those
functions which existed in 1.x, y might have more).

IOW an OS supporting 1.0 should work with any 1.x.

(which begs the question why there is not a "arm,psci-1.x" compat string,
Mark/Andre?)

> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> 
> ---
> 
> Cc: Andre Przywara <andre.przywara@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> ---
>  xen/arch/arm/psci.c        |  9 +++++----
>  xen/include/asm-arm/psci.h | 13 +++++++++++++
>  2 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
> index 172c6e7..53ee2e4 100644
> --- a/xen/arch/arm/psci.c
> +++ b/xen/arch/arm/psci.c
> @@ -122,15 +122,16 @@ int __init psci_init_0_2(void)
>  
>      psci_ver = call_smc(PSCI_0_2_FN_PSCI_VERSION, 0, 0, 0);
>  
> -    if ( psci_ver != XEN_PSCI_V_0_2 )
> +    if ( psci_ver != PSCI_VERSION(0, 2) && psci_ver != PSCI_VERSION(1, 0) )

Based on the above I think this should read:

    if ( psci_ver != PSCI_VERSION(0, 2) && PSCI_MAJOR_VERSION(psci_ver) != 1 )

>      {
> -        printk("Error: PSCI version %#x is not supported.\n", psci_ver);
> -        return -EOPNOTSUPP;
> +        printk("Error: Conflicting PSCI version detected (%#x)\n", psci_ver);

Conflicting with what?

I think perhaps you meant "Unrecognised" or "Unsupported"?

Also please format the version like you did below with %u.%u.

>      }
>  
>      psci_cpu_on_nr = PSCI_0_2_FN_NATIVE(CPU_ON);
>  
> -    printk(XENLOG_INFO "Using PSCI-0.2 for SMP bringup\n");
> +    printk(XENLOG_INFO "Using PSCI-%u.%u for SMP bringup\n",
> +           PSCI_VERSION_MAJOR(psci_ver), PSCI_VERSION_MINOR(psci_ver));
>  
>      return 0;
>  }