diff mbox series

[RESEND,v2] firmware/psci: add support for SYSTEM_RESET2

Message ID 20190411103346.22462-1-sudeep.holla@arm.com
State Superseded
Headers show
Series [RESEND,v2] firmware/psci: add support for SYSTEM_RESET2 | expand

Commit Message

Sudeep Holla April 11, 2019, 10:33 a.m. UTC 
PSCI v1.1 introduced SYSTEM_RESET2 to allow both architectural resets
where the semantics are described by the PSCI specification itself as
well as vendor-specific resets. Currently only system warm reset
semantics is defined as part of architectural resets by the specification.

This patch implements support for SYSTEM_RESET2 by making using of
reboot_mode passed by the reboot infrastructure in the kernel.

Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

---
 drivers/firmware/psci.c   | 21 +++++++++++++++++++++
 include/uapi/linux/psci.h |  2 ++
 2 files changed, 23 insertions(+)

Resending [1] based on the request. I hope to get some testing this time.
Last time Xilinx asked multiple times but never got any review or testing
https://lore.kernel.org/lkml/1525257003-8608-1-git-send-email-sudeep.holla@arm.com/

--
2.17.1

Comments

Mark Rutland April 11, 2019, 11:03 a.m. UTC | #1 
On Thu, Apr 11, 2019 at 11:33:46AM +0100, Sudeep Holla wrote:
> PSCI v1.1 introduced SYSTEM_RESET2 to allow both architectural resets

> where the semantics are described by the PSCI specification itself as

> well as vendor-specific resets. Currently only system warm reset

> semantics is defined as part of architectural resets by the specification.

> 

> This patch implements support for SYSTEM_RESET2 by making using of

> reboot_mode passed by the reboot infrastructure in the kernel.

> 

> Cc: Mark Rutland <mark.rutland@arm.com>

> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

> ---

>  drivers/firmware/psci.c   | 21 +++++++++++++++++++++

>  include/uapi/linux/psci.h |  2 ++

>  2 files changed, 23 insertions(+)

> 

> Resending [1] based on the request. I hope to get some testing this time.

> Last time Xilinx asked multiple times but never got any review or testing

> https://lore.kernel.org/lkml/1525257003-8608-1-git-send-email-sudeep.holla@arm.com/

> 

> diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c

> index c80ec1d03274..91748725534e 100644

> --- a/drivers/firmware/psci.c

> +++ b/drivers/firmware/psci.c

> @@ -88,6 +88,7 @@ static u32 psci_function_id[PSCI_FN_MAX];

>  				PSCI_1_0_EXT_POWER_STATE_TYPE_MASK)

> 

>  static u32 psci_cpu_suspend_feature;

> +static bool psci_system_reset2_supported;

> 

>  static inline bool psci_has_ext_power_state(void)

>  {

> @@ -253,6 +254,15 @@ static int get_set_conduit_method(struct device_node *np)

> 

>  static void psci_sys_reset(enum reboot_mode reboot_mode, const char *cmd)

>  {

> +	if ((reboot_mode == REBOOT_WARM || reboot_mode == REBOOT_SOFT) &&

> +	    psci_system_reset2_supported)

> +		/*

> +		 * reset_type[31] = 0 (architectural)

> +		 * reset_type[30:0] = 0 (SYSTEM_WARM_RESET)

> +		 * cookie = 0 (ignored by the implementation)

> +		 */

> +		invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2), 0, 0, 0);


Since the comment and invocation span multiple lines, could we please
wrap them in braces?

Other than that, this looks good to me, so:

Acked-by: Mark Rutland <mark.rutland@arm.com>


... I assume that Aaro will give this some testing.

Thanks,
Mark.

> +

>  	invoke_psci_fn(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0);

>  }

> 

> @@ -451,6 +461,16 @@ static const struct platform_suspend_ops psci_suspend_ops = {

>  	.enter          = psci_system_suspend_enter,

>  };

> 

> +static void __init psci_init_system_reset2(void)

> +{

> +	int ret;

> +

> +	ret = psci_features(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2));

> +

> +	if (ret != PSCI_RET_NOT_SUPPORTED)

> +		psci_system_reset2_supported = true;

> +}

> +

>  static void __init psci_init_system_suspend(void)

>  {

>  	int ret;

> @@ -588,6 +608,7 @@ static int __init psci_probe(void)

>  		psci_init_smccc();

>  		psci_init_cpu_suspend();

>  		psci_init_system_suspend();

> +		psci_init_system_reset2();

>  	}

> 

>  	return 0;

> diff --git a/include/uapi/linux/psci.h b/include/uapi/linux/psci.h

> index b3bcabe380da..5b0ba0062541 100644

> --- a/include/uapi/linux/psci.h

> +++ b/include/uapi/linux/psci.h

> @@ -49,8 +49,10 @@

> 

>  #define PSCI_1_0_FN_PSCI_FEATURES		PSCI_0_2_FN(10)

>  #define PSCI_1_0_FN_SYSTEM_SUSPEND		PSCI_0_2_FN(14)

> +#define PSCI_1_1_FN_SYSTEM_RESET2		PSCI_0_2_FN(18)

> 

>  #define PSCI_1_0_FN64_SYSTEM_SUSPEND		PSCI_0_2_FN64(14)

> +#define PSCI_1_1_FN64_SYSTEM_RESET2		PSCI_0_2_FN64(18)

> 

>  /* PSCI v0.2 power state encoding for CPU_SUSPEND function */

>  #define PSCI_0_2_POWER_STATE_ID_MASK		0xffff

> --

> 2.17.1

>
Koskinen, Aaro (Nokia - FI/Espoo) April 11, 2019, 11:42 a.m. UTC | #2 
Hi,

From: Sudeep Holla [sudeep.holla@arm.com]:

>  static void psci_sys_reset(enum reboot_mode reboot_mode, const char *cmd)

>  {

> +       if ((reboot_mode == REBOOT_WARM || reboot_mode == REBOOT_SOFT) &&


I would omit the REBOOT_SOFT here.

> +           psci_system_reset2_supported)

> +               /*

> +                * reset_type[31] = 0 (architectural)

> +                * reset_type[30:0] = 0 (SYSTEM_WARM_RESET)

> +                * cookie = 0 (ignored by the implementation)

> +                */

> +               invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2), 0, 0, 0);

> +

>        invoke_psci_fn(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0);


Use else here, so that we fall back to system halt if SYSTEM_RESET2 fails.

A.
Sudeep Holla April 11, 2019, 4:25 p.m. UTC | #3 
On Thu, Apr 11, 2019 at 12:03:04PM +0100, Mark Rutland wrote:
> On Thu, Apr 11, 2019 at 11:33:46AM +0100, Sudeep Holla wrote:

> > PSCI v1.1 introduced SYSTEM_RESET2 to allow both architectural resets

> > where the semantics are described by the PSCI specification itself as

> > well as vendor-specific resets. Currently only system warm reset

> > semantics is defined as part of architectural resets by the specification.

> > 

> > This patch implements support for SYSTEM_RESET2 by making using of

> > reboot_mode passed by the reboot infrastructure in the kernel.

> > 

> > Cc: Mark Rutland <mark.rutland@arm.com>

> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

> > ---

> >  drivers/firmware/psci.c   | 21 +++++++++++++++++++++

> >  include/uapi/linux/psci.h |  2 ++

> >  2 files changed, 23 insertions(+)

> > 

> > Resending [1] based on the request. I hope to get some testing this time.

> > Last time Xilinx asked multiple times but never got any review or testing

> > https://lore.kernel.org/lkml/1525257003-8608-1-git-send-email-sudeep.holla@arm.com/

> > 

> > diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c

> > index c80ec1d03274..91748725534e 100644

> > --- a/drivers/firmware/psci.c

> > +++ b/drivers/firmware/psci.c

> > @@ -88,6 +88,7 @@ static u32 psci_function_id[PSCI_FN_MAX];

> >  				PSCI_1_0_EXT_POWER_STATE_TYPE_MASK)

> > 

> >  static u32 psci_cpu_suspend_feature;

> > +static bool psci_system_reset2_supported;

> > 

> >  static inline bool psci_has_ext_power_state(void)

> >  {

> > @@ -253,6 +254,15 @@ static int get_set_conduit_method(struct device_node *np)

> > 

> >  static void psci_sys_reset(enum reboot_mode reboot_mode, const char *cmd)

> >  {

> > +	if ((reboot_mode == REBOOT_WARM || reboot_mode == REBOOT_SOFT) &&

> > +	    psci_system_reset2_supported)

> > +		/*

> > +		 * reset_type[31] = 0 (architectural)

> > +		 * reset_type[30:0] = 0 (SYSTEM_WARM_RESET)

> > +		 * cookie = 0 (ignored by the implementation)

> > +		 */

> > +		invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2), 0, 0, 0);

> 

> Since the comment and invocation span multiple lines, could we please

> wrap them in braces?

>


Yes, that would be better, will update it.

> Other than that, this looks good to me, so:

> 

> Acked-by: Mark Rutland <mark.rutland@arm.com>

> 


Thanks.

--
Regards,
Sudeep
Sudeep Holla April 11, 2019, 4:49 p.m. UTC | #4 
On Thu, Apr 11, 2019 at 11:42:28AM +0000, Koskinen, Aaro (Nokia - FI/Espoo) wrote:
> Hi,

> 

> From: Sudeep Holla [sudeep.holla@arm.com]:

> >  static void psci_sys_reset(enum reboot_mode reboot_mode, const char *cmd)

> >  {

> > +       if ((reboot_mode == REBOOT_WARM || reboot_mode == REBOOT_SOFT) &&

> 

> I would omit the REBOOT_SOFT here.

>


I included REBOOT_SOFT for 2 reasons:
1. drivers/firmware/efi/reboot.c - efi_reboot treats WARM and SOFT reboots same
2. If the vendors specific reboots are added and handled in EFI, I assume it
   will be categorised under REBOOT_SOFT.

If that's wrong I can drop REBOOT_SOFT.

> > +           psci_system_reset2_supported)

> > +               /*

> > +                * reset_type[31] = 0 (architectural)

> > +                * reset_type[30:0] = 0 (SYSTEM_WARM_RESET)

> > +                * cookie = 0 (ignored by the implementation)

> > +                */

> > +               invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2), 0, 0, 0);

> > +

> >        invoke_psci_fn(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0);

>

> Use else here, so that we fall back to system halt if SYSTEM_RESET2 fails.

>


Will that not change current behaviour ? IOW, is that expected behaviour ?
I am not sure if halt can be prefer over cold reboot in absence of warm/soft
reboot when the system is request to reboot. From PSCI perspective, since
SYSTEM_RESET is mandatory I prefer that unless Linux has any restriction
on this behaviour.

--
Regards,
Sudeep
Aaro Koskinen April 11, 2019, 6:26 p.m. UTC | #5 
Hi,

On Thu, Apr 11, 2019 at 05:49:36PM +0100, Sudeep Holla wrote:
> On Thu, Apr 11, 2019 at 11:42:28AM +0000, Koskinen, Aaro (Nokia - FI/Espoo) wrote:

> > From: Sudeep Holla [sudeep.holla@arm.com]:

> > >  static void psci_sys_reset(enum reboot_mode reboot_mode, const char *cmd)

> > >  {

> > > +       if ((reboot_mode == REBOOT_WARM || reboot_mode == REBOOT_SOFT) &&

> > 

> > I would omit the REBOOT_SOFT here.

> 

> I included REBOOT_SOFT for 2 reasons:

> 1. drivers/firmware/efi/reboot.c - efi_reboot treats WARM and SOFT reboots same

> 2. If the vendors specific reboots are added and handled in EFI, I assume it

>    will be categorised under REBOOT_SOFT.

> 

> If that's wrong I can drop REBOOT_SOFT.


Not a big issue, but it's just unclear what SOFT means. WARM at least maps
nicely to the PSCI spec.

> > > +           psci_system_reset2_supported)

> > > +               /*

> > > +                * reset_type[31] = 0 (architectural)

> > > +                * reset_type[30:0] = 0 (SYSTEM_WARM_RESET)

> > > +                * cookie = 0 (ignored by the implementation)

> > > +                */

> > > +               invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2), 0, 0, 0);

> > > +

> > >        invoke_psci_fn(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0);

> >

> > Use else here, so that we fall back to system halt if SYSTEM_RESET2 fails.

> 

> Will that not change current behaviour ? IOW, is that expected behaviour ?

> I am not sure if halt can be prefer over cold reboot in absence of warm/soft

> reboot when the system is request to reboot. From PSCI perspective, since

> SYSTEM_RESET is mandatory I prefer that unless Linux has any restriction

> on this behaviour.


Hmm, so does it mean that even if firmware tells that SYSTEM_RESET2
is implemented it does not imply that SYSTEM_WARM_RESET is
available? I.e. the firmware could choose to implement only some
vendor-specific resets but not architectural ones. In that case, could
we fall back to cold reset only if NOT_SUPPORTED is returned? My point
is that if the warm reset fails unexpectedly, we should halt the system
like we do if the cold reset fails.

A.
Sudeep Holla April 12, 2019, 9:55 a.m. UTC | #6 
On Thu, Apr 11, 2019 at 09:26:37PM +0300, Aaro Koskinen wrote:
> Hi,

>

> On Thu, Apr 11, 2019 at 05:49:36PM +0100, Sudeep Holla wrote:

> > On Thu, Apr 11, 2019 at 11:42:28AM +0000, Koskinen, Aaro (Nokia - FI/Espoo) wrote:

> > > From: Sudeep Holla [sudeep.holla@arm.com]:

> > > >  static void psci_sys_reset(enum reboot_mode reboot_mode, const char *cmd)

> > > >  {

> > > > +       if ((reboot_mode == REBOOT_WARM || reboot_mode == REBOOT_SOFT) &&

> > >

> > > I would omit the REBOOT_SOFT here.

> >

> > I included REBOOT_SOFT for 2 reasons:

> > 1. drivers/firmware/efi/reboot.c - efi_reboot treats WARM and SOFT reboots same

> > 2. If the vendors specific reboots are added and handled in EFI, I assume it

> >    will be categorised under REBOOT_SOFT.

> >

> > If that's wrong I can drop REBOOT_SOFT.

>

> Not a big issue, but it's just unclear what SOFT means. WARM at least maps

> nicely to the PSCI spec.

>


OK, I will keep it for now.

> > > > +           psci_system_reset2_supported)

> > > > +               /*

> > > > +                * reset_type[31] = 0 (architectural)

> > > > +                * reset_type[30:0] = 0 (SYSTEM_WARM_RESET)

> > > > +                * cookie = 0 (ignored by the implementation)

> > > > +                */

> > > > +               invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2), 0, 0, 0);

> > > > +

> > > >        invoke_psci_fn(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0);

> > >

> > > Use else here, so that we fall back to system halt if SYSTEM_RESET2 fails.

> >

> > Will that not change current behaviour ? IOW, is that expected behaviour ?

> > I am not sure if halt can be prefer over cold reboot in absence of warm/soft

> > reboot when the system is request to reboot. From PSCI perspective, since

> > SYSTEM_RESET is mandatory I prefer that unless Linux has any restriction

> > on this behaviour.

>

> Hmm, so does it mean that even if firmware tells that SYSTEM_RESET2

> is implemented it does not imply that SYSTEM_WARM_RESET is

> available? I.e. the firmware could choose to implement only some

> vendor-specific resets but not architectural ones. In that case, could

> we fall back to cold reset only if NOT_SUPPORTED is returned? My point

> is that if the warm reset fails unexpectedly, we should halt the system

> like we do if the cold reset fails.

>


OK, I understood. Sorry I was under the assumption that architectural
reset was mandatory if SYSTEM_RESET2 is implemented. I checked the PSCI
specification and I am wrong. So I am happy to add else as per your
suggestion.

--
Regards,
Sudeep
diff mbox series

Patch

diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
index c80ec1d03274..91748725534e 100644
--- a/drivers/firmware/psci.c
+++ b/drivers/firmware/psci.c
@@ -88,6 +88,7 @@  static u32 psci_function_id[PSCI_FN_MAX];
 				PSCI_1_0_EXT_POWER_STATE_TYPE_MASK)

 static u32 psci_cpu_suspend_feature;
+static bool psci_system_reset2_supported;

 static inline bool psci_has_ext_power_state(void)
 {
@@ -253,6 +254,15 @@  static int get_set_conduit_method(struct device_node *np)

 static void psci_sys_reset(enum reboot_mode reboot_mode, const char *cmd)
 {
+	if ((reboot_mode == REBOOT_WARM || reboot_mode == REBOOT_SOFT) &&
+	    psci_system_reset2_supported)
+		/*
+		 * reset_type[31] = 0 (architectural)
+		 * reset_type[30:0] = 0 (SYSTEM_WARM_RESET)
+		 * cookie = 0 (ignored by the implementation)
+		 */
+		invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2), 0, 0, 0);
+
 	invoke_psci_fn(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0);
 }

@@ -451,6 +461,16 @@  static const struct platform_suspend_ops psci_suspend_ops = {
 	.enter          = psci_system_suspend_enter,
 };

+static void __init psci_init_system_reset2(void)
+{
+	int ret;
+
+	ret = psci_features(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2));
+
+	if (ret != PSCI_RET_NOT_SUPPORTED)
+		psci_system_reset2_supported = true;
+}
+
 static void __init psci_init_system_suspend(void)
 {
 	int ret;
@@ -588,6 +608,7 @@  static int __init psci_probe(void)
 		psci_init_smccc();
 		psci_init_cpu_suspend();
 		psci_init_system_suspend();
+		psci_init_system_reset2();
 	}

 	return 0;
diff --git a/include/uapi/linux/psci.h b/include/uapi/linux/psci.h
index b3bcabe380da..5b0ba0062541 100644
--- a/include/uapi/linux/psci.h
+++ b/include/uapi/linux/psci.h
@@ -49,8 +49,10 @@ 

 #define PSCI_1_0_FN_PSCI_FEATURES		PSCI_0_2_FN(10)
 #define PSCI_1_0_FN_SYSTEM_SUSPEND		PSCI_0_2_FN(14)
+#define PSCI_1_1_FN_SYSTEM_RESET2		PSCI_0_2_FN(18)

 #define PSCI_1_0_FN64_SYSTEM_SUSPEND		PSCI_0_2_FN64(14)
+#define PSCI_1_1_FN64_SYSTEM_RESET2		PSCI_0_2_FN64(18)

 /* PSCI v0.2 power state encoding for CPU_SUSPEND function */
 #define PSCI_0_2_POWER_STATE_ID_MASK		0xffff