diff mbox

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

Message ID 1412193773-31042-3-git-send-email-suravee.suthikulpanit@amd.com
State New
Headers show

Commit Message

Suthikulpanit, Suravee Oct. 1, 2014, 8:02 p.m. UTC 
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(-)

Comments

Stefano Stabellini Oct. 2, 2014, 9:04 a.m. UTC | #1 
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
>
Julien Grall Oct. 2, 2014, 10:54 a.m. UTC | #2 
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,
Suthikulpanit, Suravee Oct. 2, 2014, 7:51 p.m. UTC | #3 
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
Julien Grall Oct. 2, 2014, 9:50 p.m. UTC | #4 
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,
Suthikulpanit, Suravee Oct. 3, 2014, midnight UTC | #5 
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
diff mbox

Patch

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);
     }
 }