[PATCH 0/2] Introduce PSCI-0.2 supports

[PATCH 0/2] Introduce PSCI-0.2 supports

[suravee.suthikulpanit]

From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>

This patch series introduce PSCI-0.2 supports for:
  * PSCI_VERSION
  * CPU_ON
  * SYSTEM_OFF
  * SYSTEM_RESET

Then, it modifies the shutdown/restart logic to use PSCI-0.2 interface
by default when available.

Suravee Suthikulpanit (2):
  xen/arm: Initial support for PSCI-0.2 (cpu_on, system_off,
    system_reset)
  xen/arm: Use PSCI-0.2 for machine_halt/restart by default

 xen/arch/arm/arm64/smpboot.c |  2 +-
 xen/arch/arm/platform.c      |  2 +-
 xen/arch/arm/psci.c          | 72 +++++++++++++++++++++++++++++++++++++++-----
 xen/arch/arm/shutdown.c      | 23 ++++++++++----
 xen/include/asm-arm/psci.h   |  4 ++-
 5 files changed, 86 insertions(+), 17 deletions(-)

-- 
1.9.3

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

[suravee.suthikulpanit]

From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>

This patch adds support for PSCI-0.2 PSCI_VERSION, CPU_ON,
SYSTEM_OFF, and SYSTEM_RESET functions.

For PSCI-0.2, the new DT binding "arm,psci-0.2" is used for matching.
The psci_init() is now refactored to handle both PSCI-0.1 and PSCI-0.2.

Also, to add support for PSCI_VERSION, this patch replaces the "bool_t psci_available"
variable with "int psci_ver", which contains the PSCI_VERSION as described in the
PSCI-0.2 spec. For v0.1, this psci_ver is 1.

Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
---
 xen/arch/arm/arm64/smpboot.c |  2 +-
 xen/arch/arm/platform.c      |  2 +-
 xen/arch/arm/psci.c          | 72 +++++++++++++++++++++++++++++++++++++++-----
 xen/include/asm-arm/psci.h   |  4 ++-
 4 files changed, 69 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/arm64/smpboot.c b/xen/arch/arm/arm64/smpboot.c
index 9146476..341cc77 100644
--- a/xen/arch/arm/arm64/smpboot.c
+++ b/xen/arch/arm/arm64/smpboot.c
@@ -54,7 +54,7 @@ static void __init smp_spin_table_init(int cpu, struct dt_device_node *dn)
 
 static int __init smp_psci_init(int cpu)
 {
-    if ( !psci_available )
+    if ( !psci_ver )
     {
         printk("CPU%d asks for PSCI, but DTB has no PSCI node\n", cpu);
         return -ENODEV;
diff --git a/xen/arch/arm/platform.c b/xen/arch/arm/platform.c
index 74c3328..cb4cda8 100644
--- a/xen/arch/arm/platform.c
+++ b/xen/arch/arm/platform.c
@@ -110,7 +110,7 @@ int __init platform_specific_mapping(struct domain *d)
 #ifdef CONFIG_ARM_32
 int __init platform_cpu_up(int cpu)
 {
-    if ( psci_available )
+    if ( psci_ver )
         return call_psci_cpu_on(cpu);
 
     if ( platform && platform->cpu_up )
diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
index b6360d5..874917e 100644
--- a/xen/arch/arm/psci.c
+++ b/xen/arch/arm/psci.c
@@ -23,7 +23,7 @@
 #include <xen/smp.h>
 #include <asm/psci.h>
 
-bool_t psci_available;
+int psci_ver;
 
 #ifdef CONFIG_ARM_32
 #define REG_PREFIX "r"
@@ -58,16 +58,23 @@ int call_psci_cpu_on(int cpu)
                                 cpu_logical_map(cpu), __pa(init_secondary), 0);
 }
 
-int __init psci_init(void)
+void call_psci_system_off(void)
+{
+    if ( psci_ver > 2 )
+        __invoke_psci_fn_smc(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
+}
+
+void call_psci_system_reset(void)
+{
+    if ( psci_ver > 2 )
+        __invoke_psci_fn_smc(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0);
+}
+
+int __init psci_is_smc_method(const struct dt_device_node *psci)
 {
-    const struct dt_device_node *psci;
     int ret;
     const char *prop_str;
 
-    psci = dt_find_compatible_node(NULL, NULL, "arm,psci");
-    if ( !psci )
-        return -ENODEV;
-
     ret = dt_property_read_string(psci, "method", &prop_str);
     if ( ret )
     {
@@ -85,19 +92,68 @@ int __init psci_init(void)
         return -EINVAL;
     }
 
+    return 0;
+}
+
+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");
 
     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);
+        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);
+
+    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;
 
 int psci_init(void);
 int call_psci_cpu_on(int cpu);
+void call_psci_system_off(void);
+void call_psci_system_reset(void);
 
 /* functions to handle guest PSCI requests */
 int32_t do_psci_cpu_on(uint32_t vcpuid, register_t entry_point);
-- 
1.9.3

[PATCH 2/2] xen/arm: Use PSCI-0.2 for machine_halt/restart by default

[suravee.suthikulpanit]

From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>

"machine_halt()" and "machine_restart()" are modified to use PSCI interface
by default if PSCI-0.2 is supported. The "raw_machine_reset()" is also removed
since this is unnecessary.

For non-PSCI, platform_poweroff() and platform_reset() are used instead.

Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
---
 xen/arch/arm/shutdown.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/shutdown.c b/xen/arch/arm/shutdown.c
index adc0529..2289ad1 100644
--- a/xen/arch/arm/shutdown.c
+++ b/xen/arch/arm/shutdown.c
@@ -5,11 +5,7 @@
 #include <xen/lib.h>
 #include <xen/smp.h>
 #include <asm/platform.h>
-
-static void raw_machine_reset(void)
-{
-    platform_reset();
-}
+#include <asm/psci.h>
 
 static void noreturn halt_this_cpu(void *arg)
 {
@@ -18,10 +14,22 @@ static void noreturn halt_this_cpu(void *arg)
 
 void machine_halt(void)
 {
+    int timeout = 10;
+
     watchdog_disable();
     console_start_sync();
     local_irq_enable();
     smp_call_function(halt_this_cpu, NULL, 0);
+    local_irq_disable();
+
+    /* Wait at most another 10ms for all other CPUs to go offline. */
+    while ( (num_online_cpus() > 1) && (timeout-- > 0) )
+        mdelay(1);
+
+    /* Not return if success */
+    call_psci_system_off();
+
+    platform_poweroff();
     halt_this_cpu(NULL);
 }
 
@@ -39,9 +47,12 @@ void machine_restart(unsigned int delay_millisecs)
     while ( (num_online_cpus() > 1) && (timeout-- > 0) )
         mdelay(1);
 
+    /* Not return if success */
+    call_psci_system_reset();
+
     while ( 1 )
     {
-        raw_machine_reset();
+        platform_reset();
         mdelay(100);
     }
 }
-- 
1.9.3

Re: [PATCH 2/2] xen/arm: Use PSCI-0.2 for machine_halt/restart by default

[Stefano Stabellini]

On Wed, 1 Oct 2014, suravee.suthikulpanit@amd.com wrote:
> From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> 
> "machine_halt()" and "machine_restart()" are modified to use PSCI interface
> by default if PSCI-0.2 is supported. The "raw_machine_reset()" is also removed
> since this is unnecessary.
> 
> For non-PSCI, platform_poweroff() and platform_reset() are used instead.
> 
> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


>  xen/arch/arm/shutdown.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/arm/shutdown.c b/xen/arch/arm/shutdown.c
> index adc0529..2289ad1 100644
> --- a/xen/arch/arm/shutdown.c
> +++ b/xen/arch/arm/shutdown.c
> @@ -5,11 +5,7 @@
>  #include <xen/lib.h>
>  #include <xen/smp.h>
>  #include <asm/platform.h>
> -
> -static void raw_machine_reset(void)
> -{
> -    platform_reset();
> -}
> +#include <asm/psci.h>
>  
>  static void noreturn halt_this_cpu(void *arg)
>  {
> @@ -18,10 +14,22 @@ static void noreturn halt_this_cpu(void *arg)
>  
>  void machine_halt(void)
>  {
> +    int timeout = 10;
> +
>      watchdog_disable();
>      console_start_sync();
>      local_irq_enable();
>      smp_call_function(halt_this_cpu, NULL, 0);
> +    local_irq_disable();
> +
> +    /* Wait at most another 10ms for all other CPUs to go offline. */
> +    while ( (num_online_cpus() > 1) && (timeout-- > 0) )
> +        mdelay(1);
> +
> +    /* Not return if success */
> +    call_psci_system_off();
> +
> +    platform_poweroff();
>      halt_this_cpu(NULL);
>  }
>  
> @@ -39,9 +47,12 @@ void machine_restart(unsigned int delay_millisecs)
>      while ( (num_online_cpus() > 1) && (timeout-- > 0) )
>          mdelay(1);
>  
> +    /* Not return if success */
> +    call_psci_system_reset();
> +
>      while ( 1 )
>      {
> -        raw_machine_reset();
> +        platform_reset();
>          mdelay(100);
>      }
>  }
> -- 
> 1.9.3
> 

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

[Stefano Stabellini]

On Wed, 1 Oct 2014, suravee.suthikulpanit@amd.com wrote:
> From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> 
> This patch adds support for PSCI-0.2 PSCI_VERSION, CPU_ON,
> SYSTEM_OFF, and SYSTEM_RESET functions.
> 
> For PSCI-0.2, the new DT binding "arm,psci-0.2" is used for matching.
> The psci_init() is now refactored to handle both PSCI-0.1 and PSCI-0.2.
> 
> Also, to add support for PSCI_VERSION, this patch replaces the "bool_t psci_available"
> variable with "int psci_ver", which contains the PSCI_VERSION as described in the
> PSCI-0.2 spec. For v0.1, this psci_ver is 1.
> 
> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>

The support to PSCI 0.2 added by this patch is a bit incomplete, but on
the other hand the patch is very simple so it might just be the only
thing we can actually accept for this release.

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


>  xen/arch/arm/arm64/smpboot.c |  2 +-
>  xen/arch/arm/platform.c      |  2 +-
>  xen/arch/arm/psci.c          | 72 +++++++++++++++++++++++++++++++++++++++-----
>  xen/include/asm-arm/psci.h   |  4 ++-
>  4 files changed, 69 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/smpboot.c b/xen/arch/arm/arm64/smpboot.c
> index 9146476..341cc77 100644
> --- a/xen/arch/arm/arm64/smpboot.c
> +++ b/xen/arch/arm/arm64/smpboot.c
> @@ -54,7 +54,7 @@ static void __init smp_spin_table_init(int cpu, struct dt_device_node *dn)
>  
>  static int __init smp_psci_init(int cpu)
>  {
> -    if ( !psci_available )
> +    if ( !psci_ver )
>      {
>          printk("CPU%d asks for PSCI, but DTB has no PSCI node\n", cpu);
>          return -ENODEV;
> diff --git a/xen/arch/arm/platform.c b/xen/arch/arm/platform.c
> index 74c3328..cb4cda8 100644
> --- a/xen/arch/arm/platform.c
> +++ b/xen/arch/arm/platform.c
> @@ -110,7 +110,7 @@ int __init platform_specific_mapping(struct domain *d)
>  #ifdef CONFIG_ARM_32
>  int __init platform_cpu_up(int cpu)
>  {
> -    if ( psci_available )
> +    if ( psci_ver )
>          return call_psci_cpu_on(cpu);
>  
>      if ( platform && platform->cpu_up )
> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
> index b6360d5..874917e 100644
> --- a/xen/arch/arm/psci.c
> +++ b/xen/arch/arm/psci.c
> @@ -23,7 +23,7 @@
>  #include <xen/smp.h>
>  #include <asm/psci.h>
>  
> -bool_t psci_available;
> +int psci_ver;
>  
>  #ifdef CONFIG_ARM_32
>  #define REG_PREFIX "r"
> @@ -58,16 +58,23 @@ int call_psci_cpu_on(int cpu)
>                                  cpu_logical_map(cpu), __pa(init_secondary), 0);
>  }
>  
> -int __init psci_init(void)
> +void call_psci_system_off(void)
> +{
> +    if ( psci_ver > 2 )
> +        __invoke_psci_fn_smc(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
> +}
> +
> +void call_psci_system_reset(void)
> +{
> +    if ( psci_ver > 2 )
> +        __invoke_psci_fn_smc(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0);
> +}
> +
> +int __init psci_is_smc_method(const struct dt_device_node *psci)
>  {
> -    const struct dt_device_node *psci;
>      int ret;
>      const char *prop_str;
>  
> -    psci = dt_find_compatible_node(NULL, NULL, "arm,psci");
> -    if ( !psci )
> -        return -ENODEV;
> -
>      ret = dt_property_read_string(psci, "method", &prop_str);
>      if ( ret )
>      {
> @@ -85,19 +92,68 @@ int __init psci_init(void)
>          return -EINVAL;
>      }
>  
> +    return 0;
> +}
> +
> +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");
>  
>      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);
> +        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);
> +
> +    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;
>  
>  int psci_init(void);
>  int call_psci_cpu_on(int cpu);
> +void call_psci_system_off(void);
> +void call_psci_system_reset(void);
>  
>  /* functions to handle guest PSCI requests */
>  int32_t do_psci_cpu_on(uint32_t vcpuid, register_t entry_point);
> -- 
> 1.9.3
> 

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

[Julien Grall]

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

Re: [PATCH 2/2] xen/arm: Use PSCI-0.2 for machine_halt/restart by default

[Julien Grall]

Hi Suravee,

On 10/01/2014 09:02 PM, suravee.suthikulpanit@amd.com wrote:
>  void machine_halt(void)
>  {
> +    int timeout = 10;
> +
>      watchdog_disable();
>      console_start_sync();
>      local_irq_enable();
>      smp_call_function(halt_this_cpu, NULL, 0);
> +    local_irq_disable();
> +
> +    /* Wait at most another 10ms for all other CPUs to go offline. */
> +    while ( (num_online_cpus() > 1) && (timeout-- > 0) )
> +        mdelay(1);
> +

This doesn't look like PSCI specific. Can you explain in the commit
message why you need to add this new block of code?

Regards,

-- 
Julien Grall

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

[Ian Campbell]

On Thu, 2014-10-02 at 11:51 +0100, Julien Grall wrote:

> > +    if ( psci_ver != 2 )
> > +    {
> > +        printk("Error: The retrieved PSCI version (%#x) does not support.\n", psci_ver);
> 
> NIT: s/does/is/ ?

along with "supported", yes. I'd probably simplify it to "PSCI version %
#x is not supported".

> 
> > +        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.

Yes, please.

This may require also falling back to 0.1 if psci_init_0_2 fails?

> > +    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.

unsigned, to match the psci spec, would be sufficient. The code should
also use the existing XEN_PSCI_V_0_1 and XEN_PSCI_V_0_2 where
appropriate.

Ian.

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

[Ian Campbell]

On Wed, 2014-10-01 at 15:02 -0500, suravee.suthikulpanit@amd.com wrote:
> -int __init psci_init(void)
> +void call_psci_system_off(void)
> +{
> +    if ( psci_ver > 2 )

Doesn't this need to be >= to do anything on a 0.2 system? (Likewise in
the fn below)

> +        __invoke_psci_fn_smc(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);

Should we print a message in the case that we aren't able to call the
0.2 handler? (again, likewise below)

> +}

Ian.

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

[Suravee Suthikulpanit]

On 10/02/2014 09:21 AM, Ian Campbell wrote:
> On Wed, 2014-10-01 at 15:02 -0500, suravee.suthikulpanit@amd.com wrote:
>> -int __init psci_init(void)
>> +void call_psci_system_off(void)
>> +{
>> +    if ( psci_ver > 2 )
>
> Doesn't this need to be >= to do anything on a 0.2 system? (Likewise in
> the fn below)

Ah yes. Thanks.
>
>> +        __invoke_psci_fn_smc(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
>
> Should we print a message in the case that we aren't able to call the
> 0.2 handler? (again, likewise below)
>

I feel that this will be too verbose for the PSCI-0.1 case since this 
function always get called in the arch/arm/shutdown.c

Suravee

Re: [PATCH 2/2] xen/arm: Use PSCI-0.2 for machine_halt/restart by default

[Suravee Suthikulpanit]

On 10/02/2014 05:54 AM, Julien Grall wrote:
> Hi Suravee,
>
> On 10/01/2014 09:02 PM, suravee.suthikulpanit@amd.com wrote:
>>   void machine_halt(void)
>>   {
>> +    int timeout = 10;
>> +
>>       watchdog_disable();
>>       console_start_sync();
>>       local_irq_enable();
>>       smp_call_function(halt_this_cpu, NULL, 0);
>> +    local_irq_disable();
>> +
>> +    /* Wait at most another 10ms for all other CPUs to go offline. */
>> +    while ( (num_online_cpus() > 1) && (timeout-- > 0) )
>> +        mdelay(1);
>> +
>
> This doesn't look like PSCI specific. Can you explain in the commit
> message why you need to add this new block of code?
>
> Regards,
>

It's not PSCI specific. I was just trying to handle the graceful 
shutdown similar to the code flow in machine_restart. If you think it's 
not necessary, I can take this out.

Suravee

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

[Suravee Suthikulpanit]

On 10/02/2014 09:19 AM, Ian Campbell wrote:
>>> +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.
> Yes, please.
>
> This may require also falling back to 0.1 if psci_init_0_2 fails?
>

Good point. I just realized that DT could have:

    compatible = "arm,psci-0.2", "arm,psci"

Suravee

Re: [PATCH 2/2] xen/arm: Use PSCI-0.2 for machine_halt/restart by default

[Julien Grall]

On 02/10/2014 20:51, Suravee Suthikulpanit wrote:
>
>
> On 10/02/2014 05:54 AM, Julien Grall wrote:
>> Hi Suravee,
>>
>> On 10/01/2014 09:02 PM, suravee.suthikulpanit@amd.com wrote:
>>>   void machine_halt(void)
>>>   {
>>> +    int timeout = 10;
>>> +
>>>       watchdog_disable();
>>>       console_start_sync();
>>>       local_irq_enable();
>>>       smp_call_function(halt_this_cpu, NULL, 0);
>>> +    local_irq_disable();
>>> +
>>> +    /* Wait at most another 10ms for all other CPUs to go offline. */
>>> +    while ( (num_online_cpus() > 1) && (timeout-- > 0) )
>>> +        mdelay(1);
>>> +
>>
>> This doesn't look like PSCI specific. Can you explain in the commit
>> message why you need to add this new block of code?
>>
>> Regards,
>>
>
> It's not PSCI specific. I was just trying to handle the graceful
> shutdown similar to the code flow in machine_restart. If you think it's
> not necessary, I can take this out.

I'm fine with keeping this code in the patch. I was requesting to 
specify in the commit message that you are adding this code for...

Regards,


-- 
Julien Grall

Re: [PATCH 2/2] xen/arm: Use PSCI-0.2 for machine_halt/restart by default

[Suravee Suthikulpanit]

On 10/02/2014 04:50 PM, Julien Grall wrote:
>
>
> On 02/10/2014 20:51, Suravee Suthikulpanit wrote:
>>
>>
>> On 10/02/2014 05:54 AM, Julien Grall wrote:
>>> Hi Suravee,
>>>
>>> On 10/01/2014 09:02 PM, suravee.suthikulpanit@amd.com wrote:
>>>>   void machine_halt(void)
>>>>   {
>>>> +    int timeout = 10;
>>>> +
>>>>       watchdog_disable();
>>>>       console_start_sync();
>>>>       local_irq_enable();
>>>>       smp_call_function(halt_this_cpu, NULL, 0);
>>>> +    local_irq_disable();
>>>> +
>>>> +    /* Wait at most another 10ms for all other CPUs to go offline. */
>>>> +    while ( (num_online_cpus() > 1) && (timeout-- > 0) )
>>>> +        mdelay(1);
>>>> +
>>>
>>> This doesn't look like PSCI specific. Can you explain in the commit
>>> message why you need to add this new block of code?
>>>
>>> Regards,
>>>
>>
>> It's not PSCI specific. I was just trying to handle the graceful
>> shutdown similar to the code flow in machine_restart. If you think it's
>> not necessary, I can take this out.
>
> I'm fine with keeping this code in the patch. I was requesting to
> specify in the commit message that you are adding this code for...
>
> Regards,
>
>

Ok, I'll resend w/ the update commit message.

Thanks,

Suravee