[RFC PATCH 0/4] Implement a PSCI SYSTEM_RESET2 reboot-mode driver

[RFC PATCH 0/4] Implement a PSCI SYSTEM_RESET2 reboot-mode driver

[Elliot Berman]

PSCI implements a restart notifier for architectural defined resets.
The SYSTEM_RESET2 call allows vendor firmware to define additional reset
types which could be mapped to the reboot reason.

Implement a driver to wire the reboot-mode framework to make vendor
SYSTEM_RESET2 calls on reboot.

This is a continuation from https://lore.kernel.org/all/4a679542-b48d-7e11-f33a-63535a5c68cb@quicinc.com/

Elliot Berman (4):
  power: reset: reboot-mode: Allow magic to be 0
  power: reset: reboot-mode: Wire reboot_mode enum to magic
  dt-bindings: power: reset: Document arm,psci-vendor-reset
  power: reset: Implement a PSCI SYSTEM_RESET2 reboot-mode driver

 .../power/reset/arm,psci-vendor-reset.yaml    | 35 +++++++++++++
 MAINTAINERS                                   |  2 +
 drivers/firmware/psci/psci.c                  |  9 ++++
 drivers/power/reset/Kconfig                   |  9 ++++
 drivers/power/reset/Makefile                  |  1 +
 drivers/power/reset/psci-vendor-reset.c       | 49 +++++++++++++++++++
 drivers/power/reset/reboot-mode.c             | 44 ++++++++++++-----
 include/linux/psci.h                          |  2 +
 8 files changed, 139 insertions(+), 12 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/power/reset/arm,psci-vendor-reset.yaml
 create mode 100644 drivers/power/reset/psci-vendor-reset.c


base-commit: 6eaae198076080886b9e7d57f4ae06fa782f90ef
-- 
2.41.0

[RFC PATCH 1/4] power: reset: reboot-mode: Allow magic to be 0

[Elliot Berman]

Allow magic from the reboot-mode driver to be defined, but 0. This is
useful when the register/command to trigger reboot needs 0 to be
written. This is the case when the "default" reboot mode is to enter a
crash dump collection mode (e.g. when triggered by a watchdog) and Linux
doing a normal reboot requires the setting to be explicitly reset to
zero.

Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
---
 drivers/power/reset/reboot-mode.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/power/reset/reboot-mode.c b/drivers/power/reset/reboot-mode.c
index b4076b10b893..a646aefa041b 100644
--- a/drivers/power/reset/reboot-mode.c
+++ b/drivers/power/reset/reboot-mode.c
@@ -19,11 +19,10 @@ struct mode_info {
 	struct list_head list;
 };
 
-static unsigned int get_reboot_mode_magic(struct reboot_mode_driver *reboot,
-					  const char *cmd)
+static bool get_reboot_mode_magic(struct reboot_mode_driver *reboot,
+					  const char *cmd, unsigned int *magic)
 {
 	const char *normal = "normal";
-	int magic = 0;
 	struct mode_info *info;
 
 	if (!cmd)
@@ -31,12 +30,12 @@ static unsigned int get_reboot_mode_magic(struct reboot_mode_driver *reboot,
 
 	list_for_each_entry(info, &reboot->head, list) {
 		if (!strcmp(info->mode, cmd)) {
-			magic = info->magic;
-			break;
+			*magic = info->magic;
+			return true;
 		}
 	}
 
-	return magic;
+	return false;
 }
 
 static int reboot_mode_notify(struct notifier_block *this,
@@ -46,8 +45,7 @@ static int reboot_mode_notify(struct notifier_block *this,
 	unsigned int magic;
 
 	reboot = container_of(this, struct reboot_mode_driver, reboot_notifier);
-	magic = get_reboot_mode_magic(reboot, cmd);
-	if (magic)
+	if (get_reboot_mode_magic(reboot, cmd, &magic))
 		reboot->write(reboot, magic);
 
 	return NOTIFY_DONE;
-- 
2.41.0

Re: [RFC PATCH 1/4] power: reset: reboot-mode: Allow magic to be 0

[Pavan Kondeti]

On Mon, Jul 24, 2023 at 03:30:51PM -0700, Elliot Berman wrote:
> Allow magic from the reboot-mode driver to be defined, but 0. This is
> useful when the register/command to trigger reboot needs 0 to be
> written. This is the case when the "default" reboot mode is to enter a
> crash dump collection mode (e.g. when triggered by a watchdog) and Linux
> doing a normal reboot requires the setting to be explicitly reset to
> zero.

Sorry, it is not clear to me. The current code does not treat 0 as a
valid value for magic. Since our platform has a meaning for 0 as a magic
value, we are doing this correct?

Basically we are allowing

reboot-mode {
	mode-normal = <0x0>;
	...
}

What is "default" reboot mode you are referring here? are you referring
to

enum reboot_mode reboot_mode DEFAULT_REBOOT_MODE

defined in kernel/reboot.c?

> 
> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
> ---
>  drivers/power/reset/reboot-mode.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/power/reset/reboot-mode.c b/drivers/power/reset/reboot-mode.c
> index b4076b10b893..a646aefa041b 100644
> --- a/drivers/power/reset/reboot-mode.c
> +++ b/drivers/power/reset/reboot-mode.c
> @@ -19,11 +19,10 @@ struct mode_info {
>  	struct list_head list;
>  };
>  
> -static unsigned int get_reboot_mode_magic(struct reboot_mode_driver *reboot,
> -					  const char *cmd)
> +static bool get_reboot_mode_magic(struct reboot_mode_driver *reboot,
> +					  const char *cmd, unsigned int *magic)
>  {
>  	const char *normal = "normal";
> -	int magic = 0;
>  	struct mode_info *info;
>  
>  	if (!cmd)
> @@ -31,12 +30,12 @@ static unsigned int get_reboot_mode_magic(struct reboot_mode_driver *reboot,
>  
>  	list_for_each_entry(info, &reboot->head, list) {
>  		if (!strcmp(info->mode, cmd)) {
> -			magic = info->magic;
> -			break;
> +			*magic = info->magic;
> +			return true;
>  		}
>  	}
>  
> -	return magic;
> +	return false;
>  }
>  
>  static int reboot_mode_notify(struct notifier_block *this,
> @@ -46,8 +45,7 @@ static int reboot_mode_notify(struct notifier_block *this,
>  	unsigned int magic;
>  
>  	reboot = container_of(this, struct reboot_mode_driver, reboot_notifier);
> -	magic = get_reboot_mode_magic(reboot, cmd);
> -	if (magic)
> +	if (get_reboot_mode_magic(reboot, cmd, &magic))
>  		reboot->write(reboot, magic);
>  
>  	return NOTIFY_DONE;
> -- 
> 2.41.0
> 

Thanks,
Pavan

[RFC PATCH 2/4] power: reset: reboot-mode: Wire reboot_mode enum to magic

[Elliot Berman]

Allow the reboot mode type to be wired to magic.

Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
---
 drivers/power/reset/reboot-mode.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/power/reset/reboot-mode.c b/drivers/power/reset/reboot-mode.c
index a646aefa041b..4ea97ccbaf51 100644
--- a/drivers/power/reset/reboot-mode.c
+++ b/drivers/power/reset/reboot-mode.c
@@ -22,12 +22,8 @@ struct mode_info {
 static bool get_reboot_mode_magic(struct reboot_mode_driver *reboot,
 					  const char *cmd, unsigned int *magic)
 {
-	const char *normal = "normal";
 	struct mode_info *info;
 
-	if (!cmd)
-		cmd = normal;
-
 	list_for_each_entry(info, &reboot->head, list) {
 		if (!strcmp(info->mode, cmd)) {
 			*magic = info->magic;
@@ -45,6 +41,32 @@ static int reboot_mode_notify(struct notifier_block *this,
 	unsigned int magic;
 
 	reboot = container_of(this, struct reboot_mode_driver, reboot_notifier);
+
+	if (!cmd) {
+		switch (mode) {
+		case REBOOT_COLD:
+			cmd = "cold";
+			break;
+		case REBOOT_WARM:
+			cmd = "warm";
+			break;
+		case REBOOT_HARD:
+			cmd = "hard";
+			break;
+		case REBOOT_SOFT:
+			cmd = "soft";
+			break;
+		case REBOOT_GPIO:
+			cmd = "gpio";
+			break;
+		}
+		if (get_reboot_mode_magic(reboot, cmd, &magic)) {
+			reboot->write(reboot, magic);
+			return NOTIFY_DONE;
+		}
+		cmd = "normal";
+	}
+
 	if (get_reboot_mode_magic(reboot, cmd, &magic))
 		reboot->write(reboot, magic);
 
-- 
2.41.0

Re: [RFC PATCH 2/4] power: reset: reboot-mode: Wire reboot_mode enum to magic

[Mukesh Ojha]

On 7/25/2023 4:00 AM, Elliot Berman wrote:
> Allow the reboot mode type to be wired to magic.
> 
> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
> ---
>   drivers/power/reset/reboot-mode.c | 30 ++++++++++++++++++++++++++----
>   1 file changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/power/reset/reboot-mode.c b/drivers/power/reset/reboot-mode.c
> index a646aefa041b..4ea97ccbaf51 100644
> --- a/drivers/power/reset/reboot-mode.c
> +++ b/drivers/power/reset/reboot-mode.c
> @@ -22,12 +22,8 @@ struct mode_info {
>   static bool get_reboot_mode_magic(struct reboot_mode_driver *reboot,
>   					  const char *cmd, unsigned int *magic)
>   {
> -	const char *normal = "normal";
>   	struct mode_info *info;
>   
> -	if (!cmd)
> -		cmd = normal;
> -
>   	list_for_each_entry(info, &reboot->head, list) {
>   		if (!strcmp(info->mode, cmd)) {
>   			*magic = info->magic;
> @@ -45,6 +41,32 @@ static int reboot_mode_notify(struct notifier_block *this,
>   	unsigned int magic;
>   
>   	reboot = container_of(this, struct reboot_mode_driver, reboot_notifier);
> +
> +	if (!cmd) {
> +		switch (mode) {

IIUC, mode will be filled up with reboot_mode during restart
notifier and not reboot notifiers ?

> +		case REBOOT_COLD:
> +			cmd = "cold";
> +			break;
> +		case REBOOT_WARM:
> +			cmd = "warm";
> +			break;
> +		case REBOOT_HARD:
> +			cmd = "hard";
> +			break;
> +		case REBOOT_SOFT:
> +			cmd = "soft";
> +			break;
> +		case REBOOT_GPIO:
> +			cmd = "gpio";

These strings are already there kernel/reboot.c
Can it be reused ?

#define REBOOT_COLD_STR         "cold"
#define REBOOT_WARM_STR         "warm"
#define REBOOT_HARD_STR         "hard"
#define REBOOT_SOFT_STR         "soft"
#define REBOOT_GPIO_STR         "gpio"
#define REBOOT_UNDEFINED_STR    "undefined"


> +			break;
> +		}
> +		if (get_reboot_mode_magic(reboot, cmd, &magic)) {

Is info->mode is going to filled up with mode-cold, mode-warm and so
on from DT to compare against cmd?

What if , cmd is not among the one above switch, NULL pointer during
strcmp ?

-Mukesh

> +			reboot->write(reboot, magic);
> +			return NOTIFY_DONE;
> +		}
> +		cmd = "normal";
> +	}
> +
>   	if (get_reboot_mode_magic(reboot, cmd, &magic))
>   		reboot->write(reboot, magic);
>   

Re: [RFC PATCH 2/4] power: reset: reboot-mode: Wire reboot_mode enum to magic

[Elliot Berman]

On 7/25/2023 3:03 AM, Mukesh Ojha wrote:
> 
> 
> On 7/25/2023 4:00 AM, Elliot Berman wrote:
>> Allow the reboot mode type to be wired to magic.
>>
>> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
>> ---
>>   drivers/power/reset/reboot-mode.c | 30 ++++++++++++++++++++++++++----
>>   1 file changed, 26 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/power/reset/reboot-mode.c 
>> b/drivers/power/reset/reboot-mode.c
>> index a646aefa041b..4ea97ccbaf51 100644
>> --- a/drivers/power/reset/reboot-mode.c
>> +++ b/drivers/power/reset/reboot-mode.c
>> @@ -22,12 +22,8 @@ struct mode_info {
>>   static bool get_reboot_mode_magic(struct reboot_mode_driver *reboot,
>>                         const char *cmd, unsigned int *magic)
>>   {
>> -    const char *normal = "normal";
>>       struct mode_info *info;
>> -    if (!cmd)
>> -        cmd = normal;
>> -
>>       list_for_each_entry(info, &reboot->head, list) {
>>           if (!strcmp(info->mode, cmd)) {
>>               *magic = info->magic;
>> @@ -45,6 +41,32 @@ static int reboot_mode_notify(struct notifier_block 
>> *this,
>>       unsigned int magic;
>>       reboot = container_of(this, struct reboot_mode_driver, 
>> reboot_notifier);
>> +
>> +    if (!cmd) {
>> +        switch (mode) {
> 
> IIUC, mode will be filled up with reboot_mode during restart
> notifier and not reboot notifiers ?
> 

Ah, you're correct. I should register a restart notifier and not use 
reboot mode framework. I'll follow similar bindings.

>> +        case REBOOT_COLD:
>> +            cmd = "cold";
>> +            break;
>> +        case REBOOT_WARM:
>> +            cmd = "warm";
>> +            break;
>> +        case REBOOT_HARD:
>> +            cmd = "hard";
>> +            break;
>> +        case REBOOT_SOFT:
>> +            cmd = "soft";
>> +            break;
>> +        case REBOOT_GPIO:
>> +            cmd = "gpio";
> 
> These strings are already there kernel/reboot.c
> Can it be reused ?
> 
> #define REBOOT_COLD_STR         "cold"
> #define REBOOT_WARM_STR         "warm"
> #define REBOOT_HARD_STR         "hard"
> #define REBOOT_SOFT_STR         "soft"
> #define REBOOT_GPIO_STR         "gpio"
> #define REBOOT_UNDEFINED_STR    "undefined"
> 
> 

One set of constants are "binding" for devicetree and the other is for 
sysfs. I think they should be kept separate.

>> +            break;
>> +        }
>> +        if (get_reboot_mode_magic(reboot, cmd, &magic)) {
> 
> Is info->mode is going to filled up with mode-cold, mode-warm and so
> on from DT to compare against cmd?
> 
> What if , cmd is not among the one above switch, NULL pointer during
> strcmp ?
> > -Mukesh
> 
>> +            reboot->write(reboot, magic);
>> +            return NOTIFY_DONE;
>> +        }
>> +        cmd = "normal";
>> +    }
>> +
>>       if (get_reboot_mode_magic(reboot, cmd, &magic))
>>           reboot->write(reboot, magic);

Re: [RFC PATCH 2/4] power: reset: reboot-mode: Wire reboot_mode enum to magic

[Pavan Kondeti]

On Tue, Jul 25, 2023 at 02:04:28PM -0700, Elliot Berman wrote:
> 
> 
> On 7/25/2023 3:03 AM, Mukesh Ojha wrote:
> > 
> > 
> > On 7/25/2023 4:00 AM, Elliot Berman wrote:
> > > Allow the reboot mode type to be wired to magic.
> > > 
> > > Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
> > > ---
> > >   drivers/power/reset/reboot-mode.c | 30 ++++++++++++++++++++++++++----
> > >   1 file changed, 26 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/power/reset/reboot-mode.c
> > > b/drivers/power/reset/reboot-mode.c
> > > index a646aefa041b..4ea97ccbaf51 100644
> > > --- a/drivers/power/reset/reboot-mode.c
> > > +++ b/drivers/power/reset/reboot-mode.c
> > > @@ -22,12 +22,8 @@ struct mode_info {
> > >   static bool get_reboot_mode_magic(struct reboot_mode_driver *reboot,
> > >                         const char *cmd, unsigned int *magic)
> > >   {
> > > -    const char *normal = "normal";
> > >       struct mode_info *info;
> > > -    if (!cmd)
> > > -        cmd = normal;
> > > -
> > >       list_for_each_entry(info, &reboot->head, list) {
> > >           if (!strcmp(info->mode, cmd)) {
> > >               *magic = info->magic;
> > > @@ -45,6 +41,32 @@ static int reboot_mode_notify(struct
> > > notifier_block *this,
> > >       unsigned int magic;
> > >       reboot = container_of(this, struct reboot_mode_driver,
> > > reboot_notifier);
> > > +
> > > +    if (!cmd) {
> > > +        switch (mode) {
> > 
> > IIUC, mode will be filled up with reboot_mode during restart
> > notifier and not reboot notifiers ?
> > 
> 

I went through the patch in isolation and came to the same conclusion on
why you are using mode directly here. Now that it is clarified, why
not use reboot_mode directly instead of introducing restart notifiers
here?

Also you might want to clarify that we are using reboot_mode as fallback
to wire the magic.

Thanks,
Pavan

[RFC PATCH 3/4] dt-bindings: power: reset: Document arm,psci-vendor-reset

[Elliot Berman]

Add devicetree bindings for using PSCI SYSTEM_RESET2 with vendor reset  types.

Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
---
 .../power/reset/arm,psci-vendor-reset.yaml    | 35 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 36 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/reset/arm,psci-vendor-reset.yaml

diff --git a/Documentation/devicetree/bindings/power/reset/arm,psci-vendor-reset.yaml b/Documentation/devicetree/bindings/power/reset/arm,psci-vendor-reset.yaml
new file mode 100644
index 000000000000..18b0b8c167a1
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/reset/arm,psci-vendor-reset.yaml
@@ -0,0 +1,35 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright 2023 Qualcomm Innovation Center, Inc. All Rights Reserved.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/power/reset/arm,psci-vendor-reset.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: PSCI SYSTEM_RESET2 Vendor Resets
+
+maintainers:
+  - Elliot Berman <quic_eberman@quicinc.com>
+
+description: |
+  PSCI SYSTEM_RESET2 supports vendor-defined reset types. This describes
+  the conversion of reboot modes to the reset types.
+
+properties:
+  compatible:
+    const: arm,psci-vendor-reset
+
+allOf:
+  - $ref: reboot-mode.yaml#
+
+additionalProperties: false
+
+examples:
+  - |
+    firmware {
+      psci-vendor-resets {
+        compatible = "arm,psci-vendor-reset";
+        reboot-normal = <0x100>;
+        reboot-bootloader = <0x101>;
+        reboot-fastboot = <0x102>;
+      };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index d516295978a4..2da4c5f1917b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16982,6 +16982,7 @@ M:	Mark Rutland <mark.rutland@arm.com>
 M:	Lorenzo Pieralisi <lpieralisi@kernel.org>
 L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
 S:	Maintained
+F:	Documentation/devicetree/bindings/power/reset/arm,psci-vendor-reset.yaml
 F:	drivers/firmware/psci/
 F:	include/linux/psci.h
 F:	include/uapi/linux/psci.h
-- 
2.41.0

Re: [RFC PATCH 3/4] dt-bindings: power: reset: Document arm,psci-vendor-reset

[Rob Herring]

On Mon, Jul 24, 2023 at 03:30:53PM -0700, Elliot Berman wrote:
> Add devicetree bindings for using PSCI SYSTEM_RESET2 with vendor reset  types.
> 
> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
> ---
>  .../power/reset/arm,psci-vendor-reset.yaml    | 35 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 36 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/reset/arm,psci-vendor-reset.yaml
> 
> diff --git a/Documentation/devicetree/bindings/power/reset/arm,psci-vendor-reset.yaml b/Documentation/devicetree/bindings/power/reset/arm,psci-vendor-reset.yaml
> new file mode 100644
> index 000000000000..18b0b8c167a1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/reset/arm,psci-vendor-reset.yaml
> @@ -0,0 +1,35 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2023 Qualcomm Innovation Center, Inc. All Rights Reserved.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/power/reset/arm,psci-vendor-reset.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: PSCI SYSTEM_RESET2 Vendor Resets
> +
> +maintainers:
> +  - Elliot Berman <quic_eberman@quicinc.com>
> +
> +description: |
> +  PSCI SYSTEM_RESET2 supports vendor-defined reset types. This describes
> +  the conversion of reboot modes to the reset types.
> +
> +properties:
> +  compatible:
> +    const: arm,psci-vendor-reset
> +
> +allOf:
> +  - $ref: reboot-mode.yaml#
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    firmware {
> +      psci-vendor-resets {
> +        compatible = "arm,psci-vendor-reset";

We already have a node for PSCI, we don't need a second one. You can 
have a separate driver without a separate node. 

> +        reboot-normal = <0x100>;

Wouldn't 'normal' be the normal PSCI reset?

> +        reboot-bootloader = <0x101>;
> +        reboot-fastboot = <0x102>;
> +      };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d516295978a4..2da4c5f1917b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -16982,6 +16982,7 @@ M:	Mark Rutland <mark.rutland@arm.com>
>  M:	Lorenzo Pieralisi <lpieralisi@kernel.org>
>  L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
>  S:	Maintained
> +F:	Documentation/devicetree/bindings/power/reset/arm,psci-vendor-reset.yaml
>  F:	drivers/firmware/psci/
>  F:	include/linux/psci.h
>  F:	include/uapi/linux/psci.h
> -- 
> 2.41.0
> 

Re: [RFC PATCH 3/4] dt-bindings: power: reset: Document arm,psci-vendor-reset

[Elliot Berman]

On 7/24/2023 4:23 PM, Rob Herring wrote:
> On Mon, Jul 24, 2023 at 03:30:53PM -0700, Elliot Berman wrote:
>> Add devicetree bindings for using PSCI SYSTEM_RESET2 with vendor reset  types.
>>
>> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
>> ---
>>   .../power/reset/arm,psci-vendor-reset.yaml    | 35 +++++++++++++++++++
>>   MAINTAINERS                                   |  1 +
>>   2 files changed, 36 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/power/reset/arm,psci-vendor-reset.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/power/reset/arm,psci-vendor-reset.yaml b/Documentation/devicetree/bindings/power/reset/arm,psci-vendor-reset.yaml
>> new file mode 100644
>> index 000000000000..18b0b8c167a1
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/reset/arm,psci-vendor-reset.yaml
>> @@ -0,0 +1,35 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +# Copyright 2023 Qualcomm Innovation Center, Inc. All Rights Reserved.
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/power/reset/arm,psci-vendor-reset.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: PSCI SYSTEM_RESET2 Vendor Resets
>> +
>> +maintainers:
>> +  - Elliot Berman <quic_eberman@quicinc.com>
>> +
>> +description: |
>> +  PSCI SYSTEM_RESET2 supports vendor-defined reset types. This describes
>> +  the conversion of reboot modes to the reset types.
>> +
>> +properties:
>> +  compatible:
>> +    const: arm,psci-vendor-reset
>> +
>> +allOf:
>> +  - $ref: reboot-mode.yaml#
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    firmware {
>> +      psci-vendor-resets {
>> +        compatible = "arm,psci-vendor-reset";
> 
> We already have a node for PSCI, we don't need a second one. You can
> have a separate driver without a separate node.
> 

I could also place the reboot-mode functionality straight into 
drivers/firwmare/psci/? I thought that might be more controversial than 
separate driver, but maybe not?

Mark/Loreno, do you have any concerns to add the reboot-mode driver 
functionality directly in drivers/firmware/psci/psci.c?

Sebastian, do you have any concerns to have this reboot-mode driver 
outside drivers/power/reset/?

>> +        reboot-normal = <0x100>;
> 
> Wouldn't 'normal' be the normal PSCI reset?
> 

Ah, right. I had my head buried in the reboot-mode code when creating 
the example. I can remove from the example.

>> +        reboot-bootloader = <0x101>;
>> +        reboot-fastboot = <0x102>;
>> +      };
>> +    };
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index d516295978a4..2da4c5f1917b 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -16982,6 +16982,7 @@ M:	Mark Rutland <mark.rutland@arm.com>
>>   M:	Lorenzo Pieralisi <lpieralisi@kernel.org>
>>   L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
>>   S:	Maintained
>> +F:	Documentation/devicetree/bindings/power/reset/arm,psci-vendor-reset.yaml
>>   F:	drivers/firmware/psci/
>>   F:	include/linux/psci.h
>>   F:	include/uapi/linux/psci.h
>> -- 
>> 2.41.0
>>

Re: [RFC PATCH 3/4] dt-bindings: power: reset: Document arm,psci-vendor-reset

[Elliot Berman]

On 7/25/2023 11:01 AM, Elliot Berman wrote:
> 
> 
> On 7/24/2023 4:23 PM, Rob Herring wrote:
>> On Mon, Jul 24, 2023 at 03:30:53PM -0700, Elliot Berman wrote:
>>> Add devicetree bindings for using PSCI SYSTEM_RESET2 with vendor 
>>> reset  types.
>>>
>>> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
>>> ---
>>>   .../power/reset/arm,psci-vendor-reset.yaml    | 35 +++++++++++++++++++
>>>   MAINTAINERS                                   |  1 +
>>>   2 files changed, 36 insertions(+)
>>>   create mode 100644 
>>> Documentation/devicetree/bindings/power/reset/arm,psci-vendor-reset.yaml
>>>
>>> diff --git 
>>> a/Documentation/devicetree/bindings/power/reset/arm,psci-vendor-reset.yaml b/Documentation/devicetree/bindings/power/reset/arm,psci-vendor-reset.yaml
>>> new file mode 100644
>>> index 000000000000..18b0b8c167a1
>>> --- /dev/null
>>> +++ 
>>> b/Documentation/devicetree/bindings/power/reset/arm,psci-vendor-reset.yaml
>>> @@ -0,0 +1,35 @@
>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>> +# Copyright 2023 Qualcomm Innovation Center, Inc. All Rights Reserved.
>>> +%YAML 1.2
>>> +---
>>> +$id: 
>>> http://devicetree.org/schemas/power/reset/arm,psci-vendor-reset.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: PSCI SYSTEM_RESET2 Vendor Resets
>>> +
>>> +maintainers:
>>> +  - Elliot Berman <quic_eberman@quicinc.com>
>>> +
>>> +description: |
>>> +  PSCI SYSTEM_RESET2 supports vendor-defined reset types. This 
>>> describes
>>> +  the conversion of reboot modes to the reset types.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: arm,psci-vendor-reset
>>> +
>>> +allOf:
>>> +  - $ref: reboot-mode.yaml#
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    firmware {
>>> +      psci-vendor-resets {
>>> +        compatible = "arm,psci-vendor-reset";
>>
>> We already have a node for PSCI, we don't need a second one. You can
>> have a separate driver without a separate node.
>>
> 
> I could also place the reboot-mode functionality straight into 
> drivers/firwmare/psci/? I thought that might be more controversial than 
> separate driver, but maybe not?
> 
> Mark/Loreno, do you have any concerns to add the reboot-mode driver 
> functionality directly in drivers/firmware/psci/psci.c?
> 
> Sebastian, do you have any concerns to have this reboot-mode driver 
> outside drivers/power/reset/?
> 

Sebastian, please disregard this question.

Mukesh pointed out that reboot-mode framework isn't the right option 
here since this driver does the actual reset and, as I understand, 
reboot-mode isn't intended to do actual reset. I'm going to implement 
something similar to what reboot-mode framework does but register 
against the restart_handler_list instead of reboot_notifier_list.

>>> +        reboot-normal = <0x100>;
>>
>> Wouldn't 'normal' be the normal PSCI reset?
>>
> 
> Ah, right. I had my head buried in the reboot-mode code when creating 
> the example. I can remove from the example.
> 
>>> +        reboot-bootloader = <0x101>;
>>> +        reboot-fastboot = <0x102>;
>>> +      };
>>> +    };
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index d516295978a4..2da4c5f1917b 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -16982,6 +16982,7 @@ M:    Mark Rutland <mark.rutland@arm.com>
>>>   M:    Lorenzo Pieralisi <lpieralisi@kernel.org>
>>>   L:    linux-arm-kernel@lists.infradead.org (moderated for 
>>> non-subscribers)
>>>   S:    Maintained
>>> +F:    
>>> Documentation/devicetree/bindings/power/reset/arm,psci-vendor-reset.yaml
>>>   F:    drivers/firmware/psci/
>>>   F:    include/linux/psci.h
>>>   F:    include/uapi/linux/psci.h
>>> -- 
>>> 2.41.0
>>>

Re: [RFC PATCH 3/4] dt-bindings: power: reset: Document arm,psci-vendor-reset

[Rob Herring]

On Tue, Jul 25, 2023 at 12:01 PM Elliot Berman <quic_eberman@quicinc.com> wrote:
>
>
>
> On 7/24/2023 4:23 PM, Rob Herring wrote:
> > On Mon, Jul 24, 2023 at 03:30:53PM -0700, Elliot Berman wrote:
> >> Add devicetree bindings for using PSCI SYSTEM_RESET2 with vendor reset  types.
> >>
> >> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
> >> ---
> >>   .../power/reset/arm,psci-vendor-reset.yaml    | 35 +++++++++++++++++++
> >>   MAINTAINERS                                   |  1 +
> >>   2 files changed, 36 insertions(+)
> >>   create mode 100644 Documentation/devicetree/bindings/power/reset/arm,psci-vendor-reset.yaml
> >>
> >> diff --git a/Documentation/devicetree/bindings/power/reset/arm,psci-vendor-reset.yaml b/Documentation/devicetree/bindings/power/reset/arm,psci-vendor-reset.yaml
> >> new file mode 100644
> >> index 000000000000..18b0b8c167a1
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/power/reset/arm,psci-vendor-reset.yaml
> >> @@ -0,0 +1,35 @@
> >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> >> +# Copyright 2023 Qualcomm Innovation Center, Inc. All Rights Reserved.
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/power/reset/arm,psci-vendor-reset.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: PSCI SYSTEM_RESET2 Vendor Resets
> >> +
> >> +maintainers:
> >> +  - Elliot Berman <quic_eberman@quicinc.com>
> >> +
> >> +description: |
> >> +  PSCI SYSTEM_RESET2 supports vendor-defined reset types. This describes
> >> +  the conversion of reboot modes to the reset types.
> >> +
> >> +properties:
> >> +  compatible:
> >> +    const: arm,psci-vendor-reset
> >> +
> >> +allOf:
> >> +  - $ref: reboot-mode.yaml#
> >> +
> >> +additionalProperties: false
> >> +
> >> +examples:
> >> +  - |
> >> +    firmware {
> >> +      psci-vendor-resets {
> >> +        compatible = "arm,psci-vendor-reset";
> >
> > We already have a node for PSCI, we don't need a second one. You can
> > have a separate driver without a separate node.
> >
>
> I could also place the reboot-mode functionality straight into
> drivers/firwmare/psci/? I thought that might be more controversial than
> separate driver, but maybe not?
>
> Mark/Loreno, do you have any concerns to add the reboot-mode driver
> functionality directly in drivers/firmware/psci/psci.c?

I'm talking about the binding. Why are you talking about driver
design? They are independent.

Rob

Re: [RFC PATCH 3/4] dt-bindings: power: reset: Document arm,psci-vendor-reset

[Elliot Berman]

On 7/26/2023 6:45 AM, Rob Herring wrote:
> On Tue, Jul 25, 2023 at 12:01 PM Elliot Berman <quic_eberman@quicinc.com> wrote:
>>
>>
>>
>> On 7/24/2023 4:23 PM, Rob Herring wrote:
>>> On Mon, Jul 24, 2023 at 03:30:53PM -0700, Elliot Berman wrote:
>>>> Add devicetree bindings for using PSCI SYSTEM_RESET2 with vendor reset  types.
>>>>
>>>> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
>>>> ---
>>>>    .../power/reset/arm,psci-vendor-reset.yaml    | 35 +++++++++++++++++++
>>>>    MAINTAINERS                                   |  1 +
>>>>    2 files changed, 36 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/power/reset/arm,psci-vendor-reset.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/power/reset/arm,psci-vendor-reset.yaml b/Documentation/devicetree/bindings/power/reset/arm,psci-vendor-reset.yaml
>>>> new file mode 100644
>>>> index 000000000000..18b0b8c167a1
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/power/reset/arm,psci-vendor-reset.yaml
>>>> @@ -0,0 +1,35 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>>> +# Copyright 2023 Qualcomm Innovation Center, Inc. All Rights Reserved.
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/power/reset/arm,psci-vendor-reset.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: PSCI SYSTEM_RESET2 Vendor Resets
>>>> +
>>>> +maintainers:
>>>> +  - Elliot Berman <quic_eberman@quicinc.com>
>>>> +
>>>> +description: |
>>>> +  PSCI SYSTEM_RESET2 supports vendor-defined reset types. This describes
>>>> +  the conversion of reboot modes to the reset types.
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    const: arm,psci-vendor-reset
>>>> +
>>>> +allOf:
>>>> +  - $ref: reboot-mode.yaml#
>>>> +
>>>> +additionalProperties: false
>>>> +
>>>> +examples:
>>>> +  - |
>>>> +    firmware {
>>>> +      psci-vendor-resets {
>>>> +        compatible = "arm,psci-vendor-reset";
>>>
>>> We already have a node for PSCI, we don't need a second one. You can
>>> have a separate driver without a separate node.
>>>
>>
>> I could also place the reboot-mode functionality straight into
>> drivers/firwmare/psci/? I thought that might be more controversial than
>> separate driver, but maybe not?
>>
>> Mark/Loreno, do you have any concerns to add the reboot-mode driver
>> functionality directly in drivers/firmware/psci/psci.c?
> 
> I'm talking about the binding. Why are you talking about driver
> design? They are independent.

Apologies, I agree to make it part of the same node. I believe it 
requires some changes to PSCI driver to create/bind the vendor restart 
device. I wanted to see how Mark and Lorenzo wanted to incorporate the 
vendor resets into PSCI driver: maybe I don't even create a 
device/driver and simply incorporate the changes into PSCI driver directly.

Re: [RFC PATCH 3/4] dt-bindings: power: reset: Document arm,psci-vendor-reset

[Mukesh Ojha]

On 7/25/2023 4:00 AM, Elliot Berman wrote:
> Add devicetree bindings for using PSCI SYSTEM_RESET2 with vendor reset  types.
> 
> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
> ---
>   .../power/reset/arm,psci-vendor-reset.yaml    | 35 +++++++++++++++++++
>   MAINTAINERS                                   |  1 +
>   2 files changed, 36 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/power/reset/arm,psci-vendor-reset.yaml
> 
> diff --git a/Documentation/devicetree/bindings/power/reset/arm,psci-vendor-reset.yaml b/Documentation/devicetree/bindings/power/reset/arm,psci-vendor-reset.yaml
> new file mode 100644
> index 000000000000..18b0b8c167a1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/reset/arm,psci-vendor-reset.yaml
> @@ -0,0 +1,35 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2023 Qualcomm Innovation Center, Inc. All Rights Reserved.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/power/reset/arm,psci-vendor-reset.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: PSCI SYSTEM_RESET2 Vendor Resets
> +
> +maintainers:
> +  - Elliot Berman <quic_eberman@quicinc.com>
> +
> +description: |
> +  PSCI SYSTEM_RESET2 supports vendor-defined reset types. This describes
> +  the conversion of reboot modes to the reset types.
> +
> +properties:
> +  compatible:
> +    const: arm,psci-vendor-reset
> +
> +allOf:
> +  - $ref: reboot-mode.yaml#
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    firmware {
> +      psci-vendor-resets {
> +        compatible = "arm,psci-vendor-reset";
> +        reboot-normal = <0x100>;
> +        reboot-bootloader = <0x101>;
> +        reboot-fastboot = <0x102>;

Should it start with mode-* ?

-Mukesh
> +      };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d516295978a4..2da4c5f1917b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -16982,6 +16982,7 @@ M:	Mark Rutland <mark.rutland@arm.com>
>   M:	Lorenzo Pieralisi <lpieralisi@kernel.org>
>   L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
>   S:	Maintained
> +F:	Documentation/devicetree/bindings/power/reset/arm,psci-vendor-reset.yaml
>   F:	drivers/firmware/psci/
>   F:	include/linux/psci.h
>   F:	include/uapi/linux/psci.h

[RFC PATCH 4/4] power: reset: Implement a PSCI SYSTEM_RESET2 reboot-mode driver

[Elliot Berman]

PSCI implements a restart notifier for architectural defined resets.
The SYSTEM_RESET2 allows vendor firmware to define additional reset
types which could be mapped to the reboot reason.

Implement a driver to wire the reboot-mode framework to make vendor
SYSTEM_RESET2 calls on reboot.

Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
---
 MAINTAINERS                             |  1 +
 drivers/firmware/psci/psci.c            |  9 +++++
 drivers/power/reset/Kconfig             |  9 +++++
 drivers/power/reset/Makefile            |  1 +
 drivers/power/reset/psci-vendor-reset.c | 49 +++++++++++++++++++++++++
 include/linux/psci.h                    |  2 +
 6 files changed, 71 insertions(+)
 create mode 100644 drivers/power/reset/psci-vendor-reset.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 2da4c5f1917b..214b14c1da63 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16984,6 +16984,7 @@ L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
 S:	Maintained
 F:	Documentation/devicetree/bindings/power/reset/arm,psci-vendor-reset.yaml
 F:	drivers/firmware/psci/
+F:	drivers/power/reset/psci-vendor-reset.c
 F:	include/linux/psci.h
 F:	include/uapi/linux/psci.h
 
diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
index d9629ff87861..6db73f9d2304 100644
--- a/drivers/firmware/psci/psci.c
+++ b/drivers/firmware/psci/psci.c
@@ -328,6 +328,15 @@ static struct notifier_block psci_sys_reset_nb = {
 	.priority = 129,
 };
 
+void psci_vendor_sys_reset2(u32 reset_type, u32 cookie)
+{
+	if (psci_system_reset2_supported)
+		invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2),
+			reset_type | BIT_ULL(31),
+			cookie, 0);
+}
+EXPORT_SYMBOL_GPL(psci_vendor_sys_reset2);
+
 static void psci_sys_poweroff(void)
 {
 	invoke_psci_fn(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
index fff07b2bd77b..1474b9d51089 100644
--- a/drivers/power/reset/Kconfig
+++ b/drivers/power/reset/Kconfig
@@ -311,4 +311,13 @@ config POWER_MLXBF
 	help
 	  This driver supports reset or low power mode handling for Mellanox BlueField.
 
+config POWER_RESET_PSCI_VENDOR_RESET
+	tristate "PSCI Vendor SYSTEM_RESET2 driver"
+	depends on ARM64 || ARM || COMPILE_TEST
+	select ARM_PSCI_FW
+	select REBOOT_MODE
+	help
+	  Say y/m here to enable driver to use Vendor SYSTEM_RESET2 types on
+	  chips which have firmware implementing custom SYSTEM_RESET2 types.
+
 endif
diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile
index d763e6735ee3..d09243966b74 100644
--- a/drivers/power/reset/Makefile
+++ b/drivers/power/reset/Makefile
@@ -37,3 +37,4 @@ obj-$(CONFIG_SYSCON_REBOOT_MODE) += syscon-reboot-mode.o
 obj-$(CONFIG_POWER_RESET_SC27XX) += sc27xx-poweroff.o
 obj-$(CONFIG_NVMEM_REBOOT_MODE) += nvmem-reboot-mode.o
 obj-$(CONFIG_POWER_MLXBF) += pwr-mlxbf.o
+obj-$(CONFIG_POWER_RESET_PSCI_VENDOR_RESET) += psci-vendor-reset.o
diff --git a/drivers/power/reset/psci-vendor-reset.c b/drivers/power/reset/psci-vendor-reset.c
new file mode 100644
index 000000000000..95d027225185
--- /dev/null
+++ b/drivers/power/reset/psci-vendor-reset.c
@@ -0,0 +1,49 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/**
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/reboot-mode.h>
+#include <linux/psci.h>
+
+static int psci_vendor_system_reset2(struct reboot_mode_driver *reboot, unsigned int magic)
+{
+	psci_vendor_sys_reset2(magic, 0);
+
+	return NOTIFY_DONE;
+}
+
+static int psci_vendor_reset_probe(struct platform_device *pdev)
+{
+	struct reboot_mode_driver *reboot;
+
+	reboot = devm_kzalloc(&pdev->dev, sizeof(*reboot), GFP_KERNEL);
+	if (!reboot)
+		return -ENOMEM;
+
+	reboot->write = psci_vendor_system_reset2;
+
+	return devm_reboot_mode_register(&pdev->dev, reboot);
+}
+
+static const struct of_device_id psci_vendor_reset_id_table[] = {
+	{ .compatible = "arm,psci-vendor-reset" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, psci_vendor_reset_id_table);
+
+static struct platform_driver psci_vendor_reset_driver = {
+	.probe = psci_vendor_reset_probe,
+	.driver = {
+		.name = "psci-vendor-reset",
+		.of_match_table = of_match_ptr(psci_vendor_reset_id_table),
+	},
+};
+module_platform_driver(psci_vendor_reset_driver);
+
+MODULE_DESCRIPTION("PSCI Vendor SYSTEM_RESET2 Driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/psci.h b/include/linux/psci.h
index 4ca0060a3fc4..0c652c12e0a8 100644
--- a/include/linux/psci.h
+++ b/include/linux/psci.h
@@ -21,6 +21,8 @@ bool psci_power_state_is_valid(u32 state);
 int psci_set_osi_mode(bool enable);
 bool psci_has_osi_support(void);
 
+void psci_vendor_sys_reset2(u32 reset_type, u32 cookie);
+
 struct psci_operations {
 	u32 (*get_version)(void);
 	int (*cpu_suspend)(u32 state, unsigned long entry_point);
-- 
2.41.0

Re: [RFC PATCH 4/4] power: reset: Implement a PSCI SYSTEM_RESET2 reboot-mode driver

[Krzysztof Kozlowski]

On 25/07/2023 00:30, Elliot Berman wrote:
> PSCI implements a restart notifier for architectural defined resets.
> The SYSTEM_RESET2 allows vendor firmware to define additional reset
> types which could be mapped to the reboot reason.
> 


> +
> +static const struct of_device_id psci_vendor_reset_id_table[] = {
> +	{ .compatible = "arm,psci-vendor-reset" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, psci_vendor_reset_id_table);
> +
> +static struct platform_driver psci_vendor_reset_driver = {
> +	.probe = psci_vendor_reset_probe,
> +	.driver = {
> +		.name = "psci-vendor-reset",
> +		.of_match_table = of_match_ptr(psci_vendor_reset_id_table),

Drop of_match_ptr() - it is useless and if used, then it must be
balanced with conditional table.

> +	},
> +};
> +module_platform_driver(psci_vendor_reset_driver);
> +

Best regards,
Krzysztof

Re: [RFC PATCH 4/4] power: reset: Implement a PSCI SYSTEM_RESET2 reboot-mode driver

[Pavan Kondeti]

On Mon, Jul 24, 2023 at 03:30:54PM -0700, Elliot Berman wrote:
> PSCI implements a restart notifier for architectural defined resets.
> The SYSTEM_RESET2 allows vendor firmware to define additional reset
> types which could be mapped to the reboot reason.
> 
> Implement a driver to wire the reboot-mode framework to make vendor
> SYSTEM_RESET2 calls on reboot.
> 
> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>

Do we need to skip the PSCI call from the existing PSCI restart notifier
which gets called after your newly introduced callback from reboot mode
notifier?

Thanks,
Pavan

Re: [RFC PATCH 4/4] power: reset: Implement a PSCI SYSTEM_RESET2 reboot-mode driver

[Elliot Berman]

On 7/26/2023 3:41 AM, Pavan Kondeti wrote:
> On Mon, Jul 24, 2023 at 03:30:54PM -0700, Elliot Berman wrote:
>> PSCI implements a restart notifier for architectural defined resets.
>> The SYSTEM_RESET2 allows vendor firmware to define additional reset
>> types which could be mapped to the reboot reason.
>>
>> Implement a driver to wire the reboot-mode framework to make vendor
>> SYSTEM_RESET2 calls on reboot.
>>
>> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
> 
> Do we need to skip the PSCI call from the existing PSCI restart notifier
> which gets called after your newly introduced callback from reboot mode
> notifier?
> 

No need, the vendor SYSTEM_RESET2 call shouldn't return if the call worked.

> Thanks,
> Pavan

Re: [RFC PATCH 4/4] power: reset: Implement a PSCI SYSTEM_RESET2 reboot-mode driver

[Pavan Kondeti]

On Wed, Jul 26, 2023 at 10:19:01AM -0700, Elliot Berman wrote:
> 
> 
> On 7/26/2023 3:41 AM, Pavan Kondeti wrote:
> > On Mon, Jul 24, 2023 at 03:30:54PM -0700, Elliot Berman wrote:
> > > PSCI implements a restart notifier for architectural defined resets.
> > > The SYSTEM_RESET2 allows vendor firmware to define additional reset
> > > types which could be mapped to the reboot reason.
> > > 
> > > Implement a driver to wire the reboot-mode framework to make vendor
> > > SYSTEM_RESET2 calls on reboot.
> > > 
> > > Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
> > 
> > Do we need to skip the PSCI call from the existing PSCI restart notifier
> > which gets called after your newly introduced callback from reboot mode
> > notifier?
> > 
> 
> No need, the vendor SYSTEM_RESET2 call shouldn't return if the call worked.
> 

From the documentation of restart handlers, restarting from reboot
notifiers may not be correct. Since you hooked it up with reboot-mode
driver framework, you may restart the system much early before the
actual machine restart kicks in. Pls check. 

Thanks,
Pavan

Re: [RFC PATCH 0/4] Implement a PSCI SYSTEM_RESET2 reboot-mode driver

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 1008 bytes --]

Hello,

On 7/24/23 15:30, Elliot Berman wrote:
> PSCI implements a restart notifier for architectural defined resets.
> The SYSTEM_RESET2 call allows vendor firmware to define additional reset
> types which could be mapped to the reboot reason.
> 
> Implement a driver to wire the reboot-mode framework to make vendor
> SYSTEM_RESET2 calls on reboot.
> 
> This is a continuation from https://lore.kernel.org/all/4a679542-b48d-7e11-f33a-63535a5c68cb@quicinc.com/

Would appreciate being CC'd on a the non-RFC postings of this patch. 
FWIW, my use case is better described with this earlier submission:

https://lore.kernel.org/lkml/20220122035421.4086618-1-f.fainelli@gmail.com/T/#m74e4243c1af3a8d896e19b573b58f562fa09961d

It would be neat if I could leverage your driver in order to implement 
this custom "reboot powercycle" implementation. Towards that goal, we 
would likely need to specify the desired reboot "sub" operation 
alongside its PSCI SYSTEM_RESET2 reboot type argument?

Thanks!
-- 
Florian


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [RFC PATCH 0/4] Implement a PSCI SYSTEM_RESET2 reboot-mode driver

[Elliot Berman]

On 7/25/2023 12:12 PM, Florian Fainelli wrote:
> Hello,
> 
> On 7/24/23 15:30, Elliot Berman wrote:
>> PSCI implements a restart notifier for architectural defined resets.
>> The SYSTEM_RESET2 call allows vendor firmware to define additional reset
>> types which could be mapped to the reboot reason.
>>
>> Implement a driver to wire the reboot-mode framework to make vendor
>> SYSTEM_RESET2 calls on reboot.
>>
>> This is a continuation from 
>> https://lore.kernel.org/all/4a679542-b48d-7e11-f33a-63535a5c68cb@quicinc.com/
> 
> Would appreciate being CC'd on a the non-RFC postings of this patch. 
> FWIW, my use case is better described with this earlier submission:
> 
> https://lore.kernel.org/lkml/20220122035421.4086618-1-f.fainelli@gmail.com/T/#m74e4243c1af3a8d896e19b573b58f562fa09961d
> 
> It would be neat if I could leverage your driver in order to implement 
> this custom "reboot powercycle" implementation. Towards that goal, we 
> would likely need to specify the desired reboot "sub" operation 
> alongside its PSCI SYSTEM_RESET2 reboot type argument?
> 
> Thanks!

I think you you want to describe the PSCI vendor reset under a warm 
reboot with command "powercycle"? In other words, my series only lets DT 
describe either reboot_mode (warm) or cmd (powercycle) but not both 
simultaneously?

Please correct me if I got it wrong! Otherwise, I can incorporate way to 
describe vendor reset type matching both reboot_mode and cmd in the DT.

- Elliot

Re: [RFC PATCH 0/4] Implement a PSCI SYSTEM_RESET2 reboot-mode driver

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 1756 bytes --]

On 7/25/23 13:27, Elliot Berman wrote:
> 
> 
> On 7/25/2023 12:12 PM, Florian Fainelli wrote:
>> Hello,
>>
>> On 7/24/23 15:30, Elliot Berman wrote:
>>> PSCI implements a restart notifier for architectural defined resets.
>>> The SYSTEM_RESET2 call allows vendor firmware to define additional reset
>>> types which could be mapped to the reboot reason.
>>>
>>> Implement a driver to wire the reboot-mode framework to make vendor
>>> SYSTEM_RESET2 calls on reboot.
>>>
>>> This is a continuation from 
>>> https://lore.kernel.org/all/4a679542-b48d-7e11-f33a-63535a5c68cb@quicinc.com/
>>
>> Would appreciate being CC'd on a the non-RFC postings of this patch. 
>> FWIW, my use case is better described with this earlier submission:
>>
>> https://lore.kernel.org/lkml/20220122035421.4086618-1-f.fainelli@gmail.com/T/#m74e4243c1af3a8d896e19b573b58f562fa09961d
>>
>> It would be neat if I could leverage your driver in order to implement 
>> this custom "reboot powercycle" implementation. Towards that goal, we 
>> would likely need to specify the desired reboot "sub" operation 
>> alongside its PSCI SYSTEM_RESET2 reboot type argument?
>>
>> Thanks!
> 
> I think you you want to describe the PSCI vendor reset under a warm 
> reboot with command "powercycle"? In other words, my series only lets DT 
> describe either reboot_mode (warm) or cmd (powercycle) but not both 
> simultaneously?

I did not give a lot of thoughts into the different types of reboot 
(warm, soft, cold) and just went with an extension of whichever reboot 
type we have to be supplemented by the "powercycle" command. It seems 
like we should support both the reboot type and command, it would be 
fine with me if I had to specify reboot_mode + cmd for each reboot_mode.
-- 
Florian


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]