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

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

[Sudeep Holla]

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

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

[Mark Rutland]

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
> 

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

[Koskinen, Aaro (Nokia - FI/Espoo)]

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.

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

[Sudeep Holla]

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

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

[Sudeep Holla]

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

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

[Aaro Koskinen]

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.

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

[Sudeep Holla]

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