| Message ID | 1412150877-4090-4-git-send-email-suravee.suthikulpanit@amd.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 1 Oct 2014, suravee.suthikulpanit@amd.com wrote: > From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> > > This patch adds SMC calls to suport PSCI-0.2 system_off and system_reset, > It also adds a call to platform_power_off in machine_halt(). This would > allow platform, which implements PSCI-0.2 to properly handle system off > and reset. > > Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> > --- > xen/arch/arm/psci.c | 10 ++++++++++ > xen/arch/arm/shutdown.c | 16 ++++++++++------ > xen/include/asm-arm/psci.h | 2 ++ > 3 files changed, 22 insertions(+), 6 deletions(-) > > diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c > index b6360d5..b0d94a8 100644 > --- a/xen/arch/arm/psci.c > +++ b/xen/arch/arm/psci.c > @@ -58,6 +58,16 @@ int call_psci_cpu_on(int cpu) > cpu_logical_map(cpu), __pa(init_secondary), 0); > } > > +void call_psci_system_off(void) > +{ > + __invoke_psci_fn_smc(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0); > +} > + > +void call_psci_system_reset(void) > +{ > + __invoke_psci_fn_smc(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0); > +} This assumes PSCI 0.2 but actually the cpu_on function is still reading the id from device tree. Could you please change that too for uniformity? > int __init psci_init(void) > { > const struct dt_device_node *psci; > diff --git a/xen/arch/arm/shutdown.c b/xen/arch/arm/shutdown.c > index adc0529..2f63674 100644 > --- a/xen/arch/arm/shutdown.c > +++ b/xen/arch/arm/shutdown.c > @@ -6,11 +6,6 @@ > #include <xen/smp.h> > #include <asm/platform.h> > > -static void raw_machine_reset(void) > -{ > - platform_reset(); > -} Please mention this change in the commit message. > static void noreturn halt_this_cpu(void *arg) > { > stop_cpu(); > @@ -18,10 +13,19 @@ 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); > + > + platform_poweroff(); > halt_this_cpu(NULL); > } > > @@ -41,7 +45,7 @@ void machine_restart(unsigned int delay_millisecs) > > while ( 1 ) > { > - raw_machine_reset(); > + platform_reset(); > mdelay(100); > } > } > diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h > index 9777c03..de00ee2 100644 > --- a/xen/include/asm-arm/psci.h > +++ b/xen/include/asm-arm/psci.h > @@ -17,6 +17,8 @@ extern bool_t psci_available; > > 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 >
On Wed, 2014-10-01 at 11:07 +0100, Stefano Stabellini wrote: > > diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c > > index b6360d5..b0d94a8 100644 > > --- a/xen/arch/arm/psci.c > > +++ b/xen/arch/arm/psci.c > > @@ -58,6 +58,16 @@ int call_psci_cpu_on(int cpu) > > cpu_logical_map(cpu), __pa(init_secondary), 0); > > } > > > > +void call_psci_system_off(void) > > +{ > > + __invoke_psci_fn_smc(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0); > > +} > > + > > +void call_psci_system_reset(void) > > +{ > > + __invoke_psci_fn_smc(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0); > > +} > > This assumes PSCI 0.2 but actually the cpu_on function is still reading > the id from device tree. > Could you please change that too for uniformity? There's actually quite a bit more required, detecting the DT compat string for v0.2 and following the updated bindings for that case. Having done that we need to use the 0.2 function ids for the existing calls too. And these new calls need to check that 0.2 is present and fail on 0.1. Ian.
On Wed, 2014-10-01 at 11:07 +0100, Stefano Stabellini wrote: > > diff --git a/xen/arch/arm/shutdown.c b/xen/arch/arm/shutdown.c > > index adc0529..2f63674 100644 > > --- a/xen/arch/arm/shutdown.c > > +++ b/xen/arch/arm/shutdown.c > > @@ -6,11 +6,6 @@ > > #include <xen/smp.h> > > #include <asm/platform.h> > > > > -static void raw_machine_reset(void) > > -{ > > - platform_reset(); > > -} > > Please mention this change in the commit message. Actually, since this is going to need a freeze exception I think it would be good to split into individual commits which separately: Refactor raw_machine_reset (Re)Implement machine_halt Introduce whatever PSCI stuff is needed. That'll make it easier to reason about and make an argument for. BTW I'm in favour of trying to get this stuff in for 4.5. Ian.
On 10/01/2014 05:26 AM, Ian Campbell wrote: > On Wed, 2014-10-01 at 11:07 +0100, Stefano Stabellini wrote: >>> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c >>> index b6360d5..b0d94a8 100644 >>> --- a/xen/arch/arm/psci.c >>> +++ b/xen/arch/arm/psci.c >>> @@ -58,6 +58,16 @@ int call_psci_cpu_on(int cpu) >>> cpu_logical_map(cpu), __pa(init_secondary), 0); >>> } >>> >>> +void call_psci_system_off(void) >>> +{ >>> + __invoke_psci_fn_smc(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0); >>> +} >>> + >>> +void call_psci_system_reset(void) >>> +{ >>> + __invoke_psci_fn_smc(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0); >>> +} >> >> This assumes PSCI 0.2 but actually the cpu_on function is still reading >> the id from device tree. >> Could you please change that too for uniformity? Sure, I can add that. > > There's actually quite a bit more required, detecting the DT compat > string for v0.2 and following the updated bindings for that case. > > Having done that we need to use the 0.2 function ids for the existing > calls too. And these new calls need to check that 0.2 is present and > fail on 0.1. > > Ian. > So, it seems that minimally, we would need support for all PSCI-0.2 functions except MIGRATION. Also, once the system uses "arm,psci-0.2", the power_off and reset code path should hook into the PSCI interface by default.Does this sound right? However, for Seattle, I would still need to provide the "platform.[reset|poweroff] ops since it's not full PSCI-0.2 (yet). Suravee
On Wed, 2014-10-01 at 09:15 -0500, Suravee Suthikulpanit wrote: > > On 10/01/2014 05:26 AM, Ian Campbell wrote: > > On Wed, 2014-10-01 at 11:07 +0100, Stefano Stabellini wrote: > >>> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c > >>> index b6360d5..b0d94a8 100644 > >>> --- a/xen/arch/arm/psci.c > >>> +++ b/xen/arch/arm/psci.c > >>> @@ -58,6 +58,16 @@ int call_psci_cpu_on(int cpu) > >>> cpu_logical_map(cpu), __pa(init_secondary), 0); > >>> } > >>> > >>> +void call_psci_system_off(void) > >>> +{ > >>> + __invoke_psci_fn_smc(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0); > >>> +} > >>> + > >>> +void call_psci_system_reset(void) > >>> +{ > >>> + __invoke_psci_fn_smc(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0); > >>> +} > >> > >> This assumes PSCI 0.2 but actually the cpu_on function is still reading > >> the id from device tree. > >> Could you please change that too for uniformity? > > Sure, I can add that. > > > > > There's actually quite a bit more required, detecting the DT compat > > string for v0.2 and following the updated bindings for that case. > > > > Having done that we need to use the 0.2 function ids for the existing > > calls too. And these new calls need to check that 0.2 is present and > > fail on 0.1. > > > > Ian. > > > > So, it seems that minimally, we would need support for all PSCI-0.2 > functions except MIGRATION. Also, once the system uses "arm,psci-0.2", > the power_off and reset code path should hook into the PSCI interface by > default.Does this sound right? Yes. > However, for Seattle, I would still need to provide the > "platform.[reset|poweroff] ops since it's not full PSCI-0.2 (yet). OOI is it also PSCI-0.1 compatible? Implementing the full PSCI 0.2 support is the thing I'm more worried about wrt the release, since it is likely to be the large bit of new work and it might not meet with Release Manager approval. Ian.
On 10/1/2014 9:46 AM, Ian Campbell wrote: > On Wed, 2014-10-01 at 09:15 -0500, Suravee Suthikulpanit wrote: >> >> On 10/01/2014 05:26 AM, Ian Campbell wrote: >>> On Wed, 2014-10-01 at 11:07 +0100, Stefano Stabellini wrote: >>>>> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c >>>>> index b6360d5..b0d94a8 100644 >>>>> --- a/xen/arch/arm/psci.c >>>>> +++ b/xen/arch/arm/psci.c >>>>> @@ -58,6 +58,16 @@ int call_psci_cpu_on(int cpu) >>>>> cpu_logical_map(cpu), __pa(init_secondary), 0); >>>>> } >>>>> >>>>> +void call_psci_system_off(void) >>>>> +{ >>>>> + __invoke_psci_fn_smc(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0); >>>>> +} >>>>> + >>>>> +void call_psci_system_reset(void) >>>>> +{ >>>>> + __invoke_psci_fn_smc(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0); >>>>> +} >>>> >>>> This assumes PSCI 0.2 but actually the cpu_on function is still reading >>>> the id from device tree. >>>> Could you please change that too for uniformity? >> >> Sure, I can add that. >> >>> >>> There's actually quite a bit more required, detecting the DT compat >>> string for v0.2 and following the updated bindings for that case. >>> >>> Having done that we need to use the 0.2 function ids for the existing >>> calls too. And these new calls need to check that 0.2 is present and >>> fail on 0.1. >>> >>> Ian. >>> >> >> So, it seems that minimally, we would need support for all PSCI-0.2 >> functions except MIGRATION. Also, once the system uses "arm,psci-0.2", >> the power_off and reset code path should hook into the PSCI interface by >> default.Does this sound right? > > Yes. > >> However, for Seattle, I would still need to provide the >> "platform.[reset|poweroff] ops since it's not full PSCI-0.2 (yet). > > OOI is it also PSCI-0.1 compatible? > > Implementing the full PSCI 0.2 support is the thing I'm more worried > about wrt the release, since it is likely to be the large bit of new > work and it might not meet with Release Manager approval. > > Ian. > No, the only functions the firmware handles are just the system_off / system_reset (work in progress). If you concern about changes to the PSCI subsystem, I could probably just making SMC call directly the platforms/seattle.c, until we have full PSCI-0.2 support ready in the arch/arm/psci.c Suravee
On Wed, 2014-10-01 at 12:58 -0500, Suravee Suthikulanit wrote: > No, the only functions the firmware handles are just the system_off / > system_reset (work in progress). > > If you concern about changes to the PSCI subsystem, I could probably > just making SMC call directly the platforms/seattle.c, until we have > full PSCI-0.2 support ready in the arch/arm/psci.c Having glanced at your newer series with stuff split out I'm much less worried than I was. I'll try and have a proper look today. Ian.
diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c index b6360d5..b0d94a8 100644 --- a/xen/arch/arm/psci.c +++ b/xen/arch/arm/psci.c @@ -58,6 +58,16 @@ int call_psci_cpu_on(int cpu) cpu_logical_map(cpu), __pa(init_secondary), 0); } +void call_psci_system_off(void) +{ + __invoke_psci_fn_smc(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0); +} + +void call_psci_system_reset(void) +{ + __invoke_psci_fn_smc(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0); +} + int __init psci_init(void) { const struct dt_device_node *psci; diff --git a/xen/arch/arm/shutdown.c b/xen/arch/arm/shutdown.c index adc0529..2f63674 100644 --- a/xen/arch/arm/shutdown.c +++ b/xen/arch/arm/shutdown.c @@ -6,11 +6,6 @@ #include <xen/smp.h> #include <asm/platform.h> -static void raw_machine_reset(void) -{ - platform_reset(); -} - static void noreturn halt_this_cpu(void *arg) { stop_cpu(); @@ -18,10 +13,19 @@ 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); + + platform_poweroff(); halt_this_cpu(NULL); } @@ -41,7 +45,7 @@ void machine_restart(unsigned int delay_millisecs) while ( 1 ) { - raw_machine_reset(); + platform_reset(); mdelay(100); } } diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h index 9777c03..de00ee2 100644 --- a/xen/include/asm-arm/psci.h +++ b/xen/include/asm-arm/psci.h @@ -17,6 +17,8 @@ extern bool_t psci_available; 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);