[PATCH] firmware/psci: add support for SYSTEM_RESET2

[PATCH] 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(+)

diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
index c80ec1d03274..216b1950bbd5 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;
+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.7.4

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

[Florian Fainelli]

On 05/01/2018 07:51 AM, 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(+)
> 
> diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
> index c80ec1d03274..216b1950bbd5 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;
> +bool psci_system_reset2_supported;

Should this be static? It does not appear to be used in another
translation unit.

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

Missing closing parenthesis

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

-- 
Florian

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

[Sudeep Holla]

On 01/05/18 18:59, Florian Fainelli wrote:
> On 05/01/2018 07:51 AM, 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(+)
>>
>> diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
>> index c80ec1d03274..216b1950bbd5 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;
>> +bool psci_system_reset2_supported;
> 
> Should this be static? It does not appear to be used in another
> translation unit.
> 

Indeed will fix it.

>>
>>  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
> 
> Missing closing parenthesis
>

Yes I saw this soon after I sent out, already fixed locally.

-- 
Regards,
Sudeep

[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(+)

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) &&
+	    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.7.4

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

[Michal Simek]

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

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

[Sudeep Holla]

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

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

[Michal Simek]

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

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

[Sudeep Holla]

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

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

[Michal Simek]

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

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

[Sudeep Holla]

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

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

[Michal Simek]

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