[PATCH 0/2] Expose reset reason through sysfs

[PATCH 0/2] Expose reset reason through sysfs

[Miquel Raynal]

Hello,

Back in 2019, my colleague Kamel did try to upstream a small change in the at91 reset driver, in order to expose the reset reason through sysfs instead of expecting userland to grep through dmesg to get it. There was basically no strong reason opposed to it, besides minor changes which needed fixing. 4 years ago I am seeing again the need for such exposure, so here is Kamel's patch with the minor comments addressed, as well as a small cleanup just before.

Link: https://lore.kernel.org/lkml/00f4e9a2-f6bd-9242-cafd-9c0c4f4dc619@microchip.com/T/

Cheers,
Miquèl

Changes in v2:
* Collected Nicolas' Acked-by
* Dropped the Xtal frequency information (as this may change between
  platforms of course).

Kamel Bouhara (1):
  power: reset: at91-reset: add sysfs interface to the power on reason

Miquel Raynal (1):
  power: reset: at91-reset: use driver structure as status parameter

 .../testing/sysfs-platform-power-on-reason    | 10 +++++
 drivers/power/reset/at91-reset.c              | 45 +++++++++++++------
 include/linux/power/power_on_reason.h         | 19 ++++++++
 3 files changed, 60 insertions(+), 14 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-platform-power-on-reason
 create mode 100644 include/linux/power/power_on_reason.h

-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/2] power: reset: at91-reset: use driver structure as status parameter

[Miquel Raynal]

It is quite uncommon to use a driver helper with parameters like *pdev
and __iomem *base. It is much cleaner and close to today's standards to
provide the per-device driver structure and then access its
internals. Let's do this with at91_resete_status() before making more
modifications to this helper.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/power/reset/at91-reset.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/power/reset/at91-reset.c b/drivers/power/reset/at91-reset.c
index 741e44a017c3..a8a6f3997768 100644
--- a/drivers/power/reset/at91-reset.c
+++ b/drivers/power/reset/at91-reset.c
@@ -149,11 +149,10 @@ static int at91_reset(struct notifier_block *this, unsigned long mode,
 	return NOTIFY_DONE;
 }
 
-static void __init at91_reset_status(struct platform_device *pdev,
-				     void __iomem *base)
+static void __init at91_reset_status(struct at91_reset *reset)
 {
+	u32 reg = readl(reset->rstc_base + AT91_RSTC_SR);
 	const char *reason;
-	u32 reg = readl(base + AT91_RSTC_SR);
 
 	switch ((reg & AT91_RSTC_RSTTYP) >> 8) {
 	case RESET_TYPE_GENERAL:
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] power: reset: at91-reset: use driver structure as status parameter

[Sebastian Reichel]

[-- Attachment #1.1: Type: text/plain, Size: 1404 bytes --]

Hi,

On Fri, Jun 09, 2023 at 04:39:11PM +0200, Miquel Raynal wrote:
> It is quite uncommon to use a driver helper with parameters like *pdev
> and __iomem *base. It is much cleaner and close to today's standards to
> provide the per-device driver structure and then access its
> internals. Let's do this with at91_resete_status() before making more
> modifications to this helper.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/power/reset/at91-reset.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/power/reset/at91-reset.c b/drivers/power/reset/at91-reset.c
> index 741e44a017c3..a8a6f3997768 100644
> --- a/drivers/power/reset/at91-reset.c
> +++ b/drivers/power/reset/at91-reset.c
> @@ -149,11 +149,10 @@ static int at91_reset(struct notifier_block *this, unsigned long mode,
>  	return NOTIFY_DONE;
>  }
>  
> -static void __init at91_reset_status(struct platform_device *pdev,
> -				     void __iomem *base)
> +static void __init at91_reset_status(struct at91_reset *reset)
>  {
> +	u32 reg = readl(reset->rstc_base + AT91_RSTC_SR);
>  	const char *reason;
> -	u32 reg = readl(base + AT91_RSTC_SR);
>  
>  	switch ((reg & AT91_RSTC_RSTTYP) >> 8) {
>  	case RESET_TYPE_GENERAL:

You also need to update the code calling this functions, otherwise
the series is not bisectable.

-- Sebastian

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] power: reset: at91-reset: use driver structure as status parameter

[Miquel Raynal]

Hi Sebastian,

sebastian.reichel@collabora.com wrote on Sat, 10 Jun 2023 01:14:22
+0200:

> Hi,
> 
> On Fri, Jun 09, 2023 at 04:39:11PM +0200, Miquel Raynal wrote:
> > It is quite uncommon to use a driver helper with parameters like *pdev
> > and __iomem *base. It is much cleaner and close to today's standards to
> > provide the per-device driver structure and then access its
> > internals. Let's do this with at91_resete_status() before making more
> > modifications to this helper.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/power/reset/at91-reset.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/power/reset/at91-reset.c b/drivers/power/reset/at91-reset.c
> > index 741e44a017c3..a8a6f3997768 100644
> > --- a/drivers/power/reset/at91-reset.c
> > +++ b/drivers/power/reset/at91-reset.c
> > @@ -149,11 +149,10 @@ static int at91_reset(struct notifier_block *this, unsigned long mode,
> >  	return NOTIFY_DONE;
> >  }
> >  
> > -static void __init at91_reset_status(struct platform_device *pdev,
> > -				     void __iomem *base)
> > +static void __init at91_reset_status(struct at91_reset *reset)
> >  {
> > +	u32 reg = readl(reset->rstc_base + AT91_RSTC_SR);
> >  	const char *reason;
> > -	u32 reg = readl(base + AT91_RSTC_SR);
> >  
> >  	switch ((reg & AT91_RSTC_RSTTYP) >> 8) {
> >  	case RESET_TYPE_GENERAL:  
> 
> You also need to update the code calling this functions, otherwise
> the series is not bisectable.

Of course, I was not paying enough attention here, sorry about that.

Thanks,
Miquèl

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/2] power: reset: at91-reset: add sysfs interface to the power on reason

[Miquel Raynal]

From: Kamel Bouhara <kamel.bouhara@bootlin.com>

Introduce a list of generic reset sources and use them to export the
power on reason through sysfs. Update the ABI documentation to describe
this new interface.

Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
[Miquel Raynal: Follow-up on Kamel's work, 4 years later]
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 .../testing/sysfs-platform-power-on-reason    | 10 +++++
 drivers/power/reset/at91-reset.c              | 42 +++++++++++++------
 include/linux/power/power_on_reason.h         | 19 +++++++++
 3 files changed, 59 insertions(+), 12 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-platform-power-on-reason
 create mode 100644 include/linux/power/power_on_reason.h

diff --git a/Documentation/ABI/testing/sysfs-platform-power-on-reason b/Documentation/ABI/testing/sysfs-platform-power-on-reason
new file mode 100644
index 000000000000..12020d017543
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-platform-power-on-reason
@@ -0,0 +1,10 @@
+What:		/sys/devices/platform/.../power_on_reason
+Date:		October 2019
+KernelVersion:	5.4
+Contact:	Kamel Bouhara <kamel.bouhara@bootlin.com>
+Description:	This file shows system power on reason. Possible sources are:
+		General system power-on, RTC wakeup, watchdog timeout, software
+		reset, user pressed reset button, CPU clock failure, oscillator
+		failure, low power mode exit, unknown.
+
+		The file is read only.
diff --git a/drivers/power/reset/at91-reset.c b/drivers/power/reset/at91-reset.c
index a8a6f3997768..73cc6839a1c1 100644
--- a/drivers/power/reset/at91-reset.c
+++ b/drivers/power/reset/at91-reset.c
@@ -18,6 +18,7 @@
 #include <linux/platform_device.h>
 #include <linux/reboot.h>
 #include <linux/reset-controller.h>
+#include <linux/power/power_on_reason.h>
 
 #include <soc/at91/at91sam9_ddrsdr.h>
 #include <soc/at91/at91sam9_sdramc.h>
@@ -149,44 +150,54 @@ static int at91_reset(struct notifier_block *this, unsigned long mode,
 	return NOTIFY_DONE;
 }
 
-static void __init at91_reset_status(struct at91_reset *reset)
+static const char *at91_reset_reason(struct at91_reset *reset)
 {
 	u32 reg = readl(reset->rstc_base + AT91_RSTC_SR);
 	const char *reason;
 
 	switch ((reg & AT91_RSTC_RSTTYP) >> 8) {
 	case RESET_TYPE_GENERAL:
-		reason = "general reset";
+		reason = POWER_ON_REASON_GENERAL;
 		break;
 	case RESET_TYPE_WAKEUP:
-		reason = "wakeup";
+		reason = POWER_ON_REASON_RTC;
 		break;
 	case RESET_TYPE_WATCHDOG:
-		reason = "watchdog reset";
+		reason = POWER_ON_REASON_WATCHDOG;
 		break;
 	case RESET_TYPE_SOFTWARE:
-		reason = "software reset";
+		reason = POWER_ON_REASON_SOFTWARE;
 		break;
 	case RESET_TYPE_USER:
-		reason = "user reset";
+		reason = POWER_ON_REASON_USER;
 		break;
 	case RESET_TYPE_CPU_FAIL:
-		reason = "CPU clock failure detection";
+		reason = POWER_ON_REASON_CPU_FAIL;
 		break;
 	case RESET_TYPE_XTAL_FAIL:
-		reason = "32.768 kHz crystal failure detection";
+		reason = POWER_ON_REASON_XTAL_FAIL;
 		break;
 	case RESET_TYPE_ULP2:
-		reason = "ULP2 reset";
+		reason = POWER_ON_REASON_LOW_POWER;
 		break;
 	default:
-		reason = "unknown reset";
+		reason = POWER_ON_REASON_UNKNOWN;
 		break;
 	}
 
-	dev_info(&pdev->dev, "Starting after %s\n", reason);
+	return reason;
 }
 
+static ssize_t power_on_reason_show(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct at91_reset *reset = platform_get_drvdata(pdev);
+
+	return sprintf(buf, "%s\n", at91_reset_reason(reset));
+}
+static DEVICE_ATTR_RO(power_on_reason);
+
 static const struct of_device_id at91_ramc_of_match[] = {
 	{
 		.compatible = "atmel,at91sam9260-sdramc",
@@ -391,7 +402,14 @@ static int __init at91_reset_probe(struct platform_device *pdev)
 	if (ret)
 		goto disable_clk;
 
-	at91_reset_status(pdev, reset->rstc_base);
+	ret = device_create_file(&pdev->dev, &dev_attr_power_on_reason);
+	if (ret) {
+		dev_err(&pdev->dev, "Could not create sysfs entry\n");
+		return ret;
+	}
+
+	dev_info(&pdev->dev, "Starting after %s reset\n",
+		 at91_reset_reason(reset));
 
 	return 0;
 
diff --git a/include/linux/power/power_on_reason.h b/include/linux/power/power_on_reason.h
new file mode 100644
index 000000000000..4b92eb0519c4
--- /dev/null
+++ b/include/linux/power/power_on_reason.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Author: Kamel Bouhra <kamel.bouhara@bootlin.com>
+ */
+
+#ifndef POWER_ON_REASON_H
+#define POWER_ON_REASON_H
+
+#define POWER_ON_REASON_GENERAL "general"
+#define POWER_ON_REASON_RTC "RTC wakeup"
+#define POWER_ON_REASON_WATCHDOG "watchdog timeout"
+#define POWER_ON_REASON_SOFTWARE "software"
+#define POWER_ON_REASON_USER "user"
+#define POWER_ON_REASON_CPU_FAIL "CPU clock failure"
+#define POWER_ON_REASON_XTAL_FAIL "crystal oscillator failure"
+#define POWER_ON_REASON_LOW_POWER "low power exit"
+#define POWER_ON_REASON_UNKNOWN "unknown"
+
+#endif /* POWER_ON_REASON_H */
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] power: reset: at91-reset: add sysfs interface to the power on reason

[Sebastian Reichel]

[-- Attachment #1.1: Type: text/plain, Size: 3190 bytes --]

Hi,

On Fri, Jun 09, 2023 at 04:39:12PM +0200, Miquel Raynal wrote:
> From: Kamel Bouhara <kamel.bouhara@bootlin.com>
> 
> Introduce a list of generic reset sources and use them to export the
> power on reason through sysfs. Update the ABI documentation to describe
> this new interface.
> 
> Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
> [Miquel Raynal: Follow-up on Kamel's work, 4 years later]
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  .../testing/sysfs-platform-power-on-reason    | 10 +++++
>  drivers/power/reset/at91-reset.c              | 42 +++++++++++++------
>  include/linux/power/power_on_reason.h         | 19 +++++++++
>  3 files changed, 59 insertions(+), 12 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-platform-power-on-reason
>  create mode 100644 include/linux/power/power_on_reason.h
> 
> diff --git a/Documentation/ABI/testing/sysfs-platform-power-on-reason b/Documentation/ABI/testing/sysfs-platform-power-on-reason
> new file mode 100644
> index 000000000000..12020d017543
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-platform-power-on-reason
> @@ -0,0 +1,10 @@
> +What:		/sys/devices/platform/.../power_on_reason
> +Date:		October 2019
> +KernelVersion:	5.4

That needs to be updated :)

> +Contact:	Kamel Bouhara <kamel.bouhara@bootlin.com>
> +Description:	This file shows system power on reason. Possible sources are:
> +		General system power-on, RTC wakeup, watchdog timeout, software
> +		reset, user pressed reset button, CPU clock failure, oscillator
> +		failure, low power mode exit, unknown.
> +
> +		The file is read only.

This should list the exact strings generated by the kernel. They are
ABI. Also it should be mentioned, that the list might be extended in
the future.

> diff --git a/drivers/power/reset/at91-reset.c b/drivers/power/reset/at91-reset.c

[...]

> diff --git a/include/linux/power/power_on_reason.h b/include/linux/power/power_on_reason.h
> new file mode 100644
> index 000000000000..4b92eb0519c4
> --- /dev/null
> +++ b/include/linux/power/power_on_reason.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Author: Kamel Bouhra <kamel.bouhara@bootlin.com>
> + */
> +
> +#ifndef POWER_ON_REASON_H
> +#define POWER_ON_REASON_H
> +
> +#define POWER_ON_REASON_GENERAL "general"

What's the difference between "general" and "unknown"?

> +#define POWER_ON_REASON_RTC "RTC wakeup"
> +#define POWER_ON_REASON_WATCHDOG "watchdog timeout"
> +#define POWER_ON_REASON_SOFTWARE "software"

"software reset"

> +#define POWER_ON_REASON_USER "user"

user is quite confusing. This should be something like

#define POWER_ON_REASON_RST_BTN "reset button"

> +#define POWER_ON_REASON_CPU_FAIL "CPU clock failure"

POWER_ON_REASON_CPU_CLK_FAIL

> +#define POWER_ON_REASON_XTAL_FAIL "crystal oscillator failure"
> +#define POWER_ON_REASON_LOW_POWER "low power exit"

when is this reported?

> +#define POWER_ON_REASON_UNKNOWN "unknown"
> +
> +#endif /* POWER_ON_REASON_H */

Greetings,

-- Sebastian

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] power: reset: at91-reset: add sysfs interface to the power on reason

[Miquel Raynal]

Hi Sebastian,

sebastian.reichel@collabora.com wrote on Sat, 10 Jun 2023 01:36:59
+0200:

> Hi,
> 
> On Fri, Jun 09, 2023 at 04:39:12PM +0200, Miquel Raynal wrote:
> > From: Kamel Bouhara <kamel.bouhara@bootlin.com>
> > 
> > Introduce a list of generic reset sources and use them to export the
> > power on reason through sysfs. Update the ABI documentation to describe
> > this new interface.
> > 
> > Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> > Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
> > [Miquel Raynal: Follow-up on Kamel's work, 4 years later]
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  .../testing/sysfs-platform-power-on-reason    | 10 +++++
> >  drivers/power/reset/at91-reset.c              | 42 +++++++++++++------
> >  include/linux/power/power_on_reason.h         | 19 +++++++++
> >  3 files changed, 59 insertions(+), 12 deletions(-)
> >  create mode 100644 Documentation/ABI/testing/sysfs-platform-power-on-reason
> >  create mode 100644 include/linux/power/power_on_reason.h
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-platform-power-on-reason b/Documentation/ABI/testing/sysfs-platform-power-on-reason
> > new file mode 100644
> > index 000000000000..12020d017543
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-platform-power-on-reason
> > @@ -0,0 +1,10 @@
> > +What:		/sys/devices/platform/.../power_on_reason
> > +Date:		October 2019
> > +KernelVersion:	5.4  
> 
> That needs to be updated :)

/o\

> 
> > +Contact:	Kamel Bouhara <kamel.bouhara@bootlin.com>
> > +Description:	This file shows system power on reason. Possible sources are:
> > +		General system power-on, RTC wakeup, watchdog timeout, software
> > +		reset, user pressed reset button, CPU clock failure, oscillator
> > +		failure, low power mode exit, unknown.
> > +
> > +		The file is read only.  
> 
> This should list the exact strings generated by the kernel. They are
> ABI. Also it should be mentioned, that the list might be extended in
> the future.

Ah, ok, sure.

> 
> > diff --git a/drivers/power/reset/at91-reset.c b/drivers/power/reset/at91-reset.c  
> 
> [...]
> 
> > diff --git a/include/linux/power/power_on_reason.h b/include/linux/power/power_on_reason.h
> > new file mode 100644
> > index 000000000000..4b92eb0519c4
> > --- /dev/null
> > +++ b/include/linux/power/power_on_reason.h
> > @@ -0,0 +1,19 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Author: Kamel Bouhra <kamel.bouhara@bootlin.com>
> > + */
> > +
> > +#ifndef POWER_ON_REASON_H
> > +#define POWER_ON_REASON_H
> > +
> > +#define POWER_ON_REASON_GENERAL "general"  
> 
> What's the difference between "general" and "unknown"?

This reset value is returned when the reset controller detects rising
power rails. It is basically a "I just plugged the power connector"
situation.

Maybe I can rename it:

#define POWER_ON_REASON_REGULAR "regular power-up"

?

The "unknown" situation is more like a "it does not match what I
expect/know". Might be because support for a new reason was added to
another SoC where these enums would be used but not added to the driver
(yet).

Questioning the low-power situation remark you made below, we could
also imagine that, under certain brownout conditions, where the status
register might somehow be corrupted (assuming if this happens, the
other reasons will not appear), we might want to return that "unknown"
reason which sounds more like a software thing for handling default
case statements.

> > +#define POWER_ON_REASON_RTC "RTC wakeup"
> > +#define POWER_ON_REASON_WATCHDOG "watchdog timeout"
> > +#define POWER_ON_REASON_SOFTWARE "software"  
> 
> "software reset"

Yes.

> 
> > +#define POWER_ON_REASON_USER "user"  
> 
> user is quite confusing. This should be something like
> 
> #define POWER_ON_REASON_RST_BTN "reset button"

Agreed. Maybe even "reset button action" so it fits better the "Starting
after %s" string, like the other reasons.

> 
> > +#define POWER_ON_REASON_CPU_FAIL "CPU clock failure"  
> 
> POWER_ON_REASON_CPU_CLK_FAIL
> 
> > +#define POWER_ON_REASON_XTAL_FAIL "crystal oscillator failure"
> > +#define POWER_ON_REASON_LOW_POWER "low power exit"  
> 
> when is this reported?

I am currently using a Sama5d3 where this reason does not exist, but
given how are defined the other reasons, I would assume that the reset
controller might be able to monitor the power rails, and report this
condition upon a short brownout, like: "there is temporarily not enough
power => CPU reset", but in the mean time, the capacitors allowed the
hardware to retain that information and this does not appear like a
regular power up situation. Just guessing, I did not find yet a proper
explanation.

Anyhow, I will change the string to "low-power condition" which sounds
better IMHO.

> > +#define POWER_ON_REASON_UNKNOWN "unknown"

And here "unknown reason"

> > +
> > +#endif /* POWER_ON_REASON_H */  
> 
> Greetings,
> 
> -- Sebastian

Thanks,
Miquèl

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel