diff mbox series

[v2] firmware/psci: add support for SYSTEM_RESET2

Message ID 1525257003-8608-1-git-send-email-sudeep.holla@arm.com
State Superseded
Headers show
Series [v2] firmware/psci: add support for SYSTEM_RESET2 | expand

Commit Message

Sudeep Holla May 2, 2018, 10:30 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(+)

v1->v2:
	- Added mising static anotation to psci_system_reset2_supported
	- Added missing ')' in the comment

-- 
2.7.4

Comments

Michal Simek July 9, 2018, 12:17 p.m. UTC | #1 
Hi Sudeep,

On 2.5.2018 12:30, 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(+)

> 

> v1->v2:

> 	- Added mising static anotation to psci_system_reset2_supported

> 	- Added missing ')' in the comment

> 

> 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) &&


I am curious about this reboot_mode setup. I have grepped the kernel and
reboot_mode is setup via kernel parameters at boot time.
Shouldn't user decide what reboot type wants to do?

Thanks,
Michal
Sudeep Holla July 9, 2018, 12:36 p.m. UTC | #2 
On 09/07/18 13:17, Michal Simek wrote:
> Hi Sudeep,

> 

> On 2.5.2018 12:30, 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(+)

>>

>> v1->v2:

>> 	- Added mising static anotation to psci_system_reset2_supported

>> 	- Added missing ')' in the comment

>>

>> 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) &&

> 

> I am curious about this reboot_mode setup. I have grepped the kernel and

> reboot_mode is setup via kernel parameters at boot time.

> Shouldn't user decide what reboot type wants to do?

> 


I have almost forgotten how I tested this. IIRC and looking quickly @
kernel/reboot.c and __setup(reboot=,...), I recall passing reboot=w
while testing this. We can do something like what efi_reboot, but not
sure if we need that yet if users need too decide after boot.

-- 
Regards,
Sudeep
Michal Simek July 9, 2018, 1:13 p.m. UTC | #3 
On 9.7.2018 14:36, Sudeep Holla wrote:
> 

> 

> On 09/07/18 13:17, Michal Simek wrote:

>> Hi Sudeep,

>>

>> On 2.5.2018 12:30, 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(+)

>>>

>>> v1->v2:

>>> 	- Added mising static anotation to psci_system_reset2_supported

>>> 	- Added missing ')' in the comment

>>>

>>> 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) &&

>>

>> I am curious about this reboot_mode setup. I have grepped the kernel and

>> reboot_mode is setup via kernel parameters at boot time.

>> Shouldn't user decide what reboot type wants to do?

>>

> 

> I have almost forgotten how I tested this. IIRC and looking quickly @

> kernel/reboot.c and __setup(reboot=,...), I recall passing reboot=w

> while testing this.


I would expect that too. It means static setting at boot time.

> We can do something like what efi_reboot, but not

> sure if we need that yet if users need too decide after boot.



I am checking with colleague about all xilinx use cases but I would be
surprised if static configuration at run time is enough.

Also not quite sure about mixing reboot_warm and reboot_soft cases
together. They shouldn't be the same.

Based on psci 1.0 spec there are more options especially for bit[31]
which should be likely setup via DT(no experience with EFI) with names
and codes which should be passed to firmware but still the same issue
remains how to setup reboot type.

Thanks,
Michal
Sudeep Holla July 9, 2018, 1:21 p.m. UTC | #4 
On 09/07/18 14:13, Michal Simek wrote:
> On 9.7.2018 14:36, Sudeep Holla wrote:

>>

>>

>> On 09/07/18 13:17, Michal Simek wrote:

>>> Hi Sudeep,

>>>

>>> On 2.5.2018 12:30, 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(+)

>>>>

>>>> v1->v2:

>>>> 	- Added mising static anotation to psci_system_reset2_supported

>>>> 	- Added missing ')' in the comment

>>>>

>>>> 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) &&

>>>

>>> I am curious about this reboot_mode setup. I have grepped the kernel and

>>> reboot_mode is setup via kernel parameters at boot time.

>>> Shouldn't user decide what reboot type wants to do?

>>>

>>

>> I have almost forgotten how I tested this. IIRC and looking quickly @

>> kernel/reboot.c and __setup(reboot=,...), I recall passing reboot=w

>> while testing this.

> 

> I would expect that too. It means static setting at boot time.

> 

>> We can do something like what efi_reboot, but not

>> sure if we need that yet if users need too decide after boot.

> 

> 

> I am checking with colleague about all xilinx use cases but I would be

> surprised if static configuration at run time is enough.

> 

> Also not quite sure about mixing reboot_warm and reboot_soft cases

> together. They shouldn't be the same.

> 

> Based on psci 1.0 spec there are more options especially for bit[31]

> which should be likely setup via DT(no experience with EFI) with names

> and codes which should be passed to firmware but still the same issue

> remains how to setup reboot type.

> 


I think with efi_reboot, if user execute the command "reboot warm", it
will set REBOOT_WARM. I was thinking something similar for psci too. I
can drop REBOOT_SOFT if that's not correct.

-- 
Regards,
Sudeep
Michal Simek July 19, 2018, 10:47 a.m. UTC | #5 
On 9.7.2018 15:21, Sudeep Holla wrote:
> 

> 

> On 09/07/18 14:13, Michal Simek wrote:

>> On 9.7.2018 14:36, Sudeep Holla wrote:

>>>

>>>

>>> On 09/07/18 13:17, Michal Simek wrote:

>>>> Hi Sudeep,

>>>>

>>>> On 2.5.2018 12:30, 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(+)

>>>>>

>>>>> v1->v2:

>>>>> 	- Added mising static anotation to psci_system_reset2_supported

>>>>> 	- Added missing ')' in the comment

>>>>>

>>>>> 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) &&

>>>>

>>>> I am curious about this reboot_mode setup. I have grepped the kernel and

>>>> reboot_mode is setup via kernel parameters at boot time.

>>>> Shouldn't user decide what reboot type wants to do?

>>>>

>>>

>>> I have almost forgotten how I tested this. IIRC and looking quickly @

>>> kernel/reboot.c and __setup(reboot=,...), I recall passing reboot=w

>>> while testing this.

>>

>> I would expect that too. It means static setting at boot time.

>>

>>> We can do something like what efi_reboot, but not

>>> sure if we need that yet if users need too decide after boot.

>>

>>

>> I am checking with colleague about all xilinx use cases but I would be

>> surprised if static configuration at run time is enough.

>>

>> Also not quite sure about mixing reboot_warm and reboot_soft cases

>> together. They shouldn't be the same.

>>

>> Based on psci 1.0 spec there are more options especially for bit[31]

>> which should be likely setup via DT(no experience with EFI) with names

>> and codes which should be passed to firmware but still the same issue

>> remains how to setup reboot type.

>>

> 

> I think with efi_reboot, if user execute the command "reboot warm", it

> will set REBOOT_WARM. I was thinking something similar for psci too. I

> can drop REBOOT_SOFT if that's not correct.


Have you found more details about efi_reboot and passing information in
case of reboot warm?

Thanks,
Michal
Sudeep Holla July 26, 2018, 1:29 p.m. UTC | #6 
Hi Michal,

Sorry somehow missed this email.

On 19/07/18 11:47, Michal Simek wrote:
> On 9.7.2018 15:21, Sudeep Holla wrote:


[...]

>>

>> I think with efi_reboot, if user execute the command "reboot warm", it

>> will set REBOOT_WARM. I was thinking something similar for psci too. I

>> can drop REBOOT_SOFT if that's not correct.

> 

> Have you found more details about efi_reboot and passing information in

> case of reboot warm?

> 


I was just referring to drivers/firmware/efi/reboot.c- function efi_reboot.

Anyways the function reboot_setup in kernel/reboot.c sets the reboot
mode parsing the string passed by the user(e.g.: warm, cold, hard, gpio,
soft,..etc it just scans the first character TBH)

-- 
Regards,
Sudeep
Michal Simek July 30, 2018, 8:59 a.m. UTC | #7 
Hi Sudeep,

On 26.7.2018 15:29, Sudeep Holla wrote:
> Hi Michal,

> 

> Sorry somehow missed this email.

> 

> On 19/07/18 11:47, Michal Simek wrote:

>> On 9.7.2018 15:21, Sudeep Holla wrote:

> 

> [...]

> 

>>>

>>> I think with efi_reboot, if user execute the command "reboot warm", it

>>> will set REBOOT_WARM. I was thinking something similar for psci too. I

>>> can drop REBOOT_SOFT if that's not correct.

>>

>> Have you found more details about efi_reboot and passing information in

>> case of reboot warm?

>>

> 

> I was just referring to drivers/firmware/efi/reboot.c- function efi_reboot.

> 

> Anyways the function reboot_setup in kernel/reboot.c sets the reboot

> mode parsing the string passed by the user(e.g.: warm, cold, hard, gpio,

> soft,..etc it just scans the first character TBH)


ok. I see. It is just reusing that reboot=X passed via kernel command line.

Thanks,
Michal
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