[PATCH v3 0/2] Expose reset reason through sysfs

[PATCH v3 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 v3:
* Made the series bisectable.
* Updated the date and kernel version for this new feature.
* Changed a few definitions as discussed with Sebastian.

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: change the power on reason prototype

 .../testing/sysfs-platform-power-on-reason    | 12 +++++
 drivers/power/reset/at91-reset.c              | 44 +++++++++++++------
 include/linux/power/power_on_reason.h         | 19 ++++++++
 3 files changed, 61 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

[PATCH v3 1/2] power: reset: at91-reset: change the power on reason prototype

[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 the helper which returns the power on
reason. While we change the parameters, we can as well rename the
function from at91_reset_status() to at91_reset_reason() to be more
accurate with what the helper actually does, and finally because we don't
really need the pdev argument in this helper besides for printing the
reset reason, we can move the dev_info() call into the probe.

All these modifications prepare the introduction of a sysfs entry to
access this information. This way the diff will be much smaller. Thus,
there is no intended functional change.

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

diff --git a/drivers/power/reset/at91-reset.c b/drivers/power/reset/at91-reset.c
index 741e44a017c3..d6884841a6dc 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 const char * __init at91_reset_reason(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:
@@ -185,7 +184,7 @@ static void __init at91_reset_status(struct platform_device *pdev,
 		break;
 	}
 
-	dev_info(&pdev->dev, "Starting after %s\n", reason);
+	return reason;
 }
 
 static const struct of_device_id at91_ramc_of_match[] = {
@@ -392,7 +391,7 @@ static int __init at91_reset_probe(struct platform_device *pdev)
 	if (ret)
 		goto disable_clk;
 
-	at91_reset_status(pdev, reset->rstc_base);
+	dev_info(&pdev->dev, "Starting after %s\n", at91_reset_reason(reset));
 
 	return 0;
 
-- 
2.34.1

Re: [PATCH v3 1/2] power: reset: at91-reset: change the power on reason prototype

[Nicolas Ferre]

On 16/06/2023 at 15:52, Miquel Raynal wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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 the helper which returns the power on
> reason. While we change the parameters, we can as well rename the
> function from at91_reset_status() to at91_reset_reason() to be more
> accurate with what the helper actually does, and finally because we don't
> really need the pdev argument in this helper besides for printing the
> reset reason, we can move the dev_info() call into the probe.
> 
> All these modifications prepare the introduction of a sysfs entry to
> access this information. This way the diff will be much smaller. Thus,
> there is no intended functional change.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Thanks Miquel:
Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>

Best regards,
   Nicolas

> ---
>   drivers/power/reset/at91-reset.c | 9 ++++-----
>   1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/power/reset/at91-reset.c b/drivers/power/reset/at91-reset.c
> index 741e44a017c3..d6884841a6dc 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 const char * __init at91_reset_reason(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:
> @@ -185,7 +184,7 @@ static void __init at91_reset_status(struct platform_device *pdev,
>                  break;
>          }
> 
> -       dev_info(&pdev->dev, "Starting after %s\n", reason);
> +       return reason;
>   }
> 
>   static const struct of_device_id at91_ramc_of_match[] = {
> @@ -392,7 +391,7 @@ static int __init at91_reset_probe(struct platform_device *pdev)
>          if (ret)
>                  goto disable_clk;
> 
> -       at91_reset_status(pdev, reset->rstc_base);
> +       dev_info(&pdev->dev, "Starting after %s\n", at91_reset_reason(reset));
> 
>          return 0;
> 
> --
> 2.34.1
> 

-- 
Nicolas Ferre

Re: [PATCH v3 1/2] power: reset: at91-reset: change the power on reason prototype

[Sebastian Reichel]

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

Hi,

On Mon, Jun 19, 2023 at 10:10:05AM +0200, Nicolas Ferre wrote:
> On 16/06/2023 at 15:52, Miquel Raynal wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > 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 the helper which returns the power on
> > reason. While we change the parameters, we can as well rename the
> > function from at91_reset_status() to at91_reset_reason() to be more
> > accurate with what the helper actually does, and finally because we don't
> > really need the pdev argument in this helper besides for printing the
> > reset reason, we can move the dev_info() call into the probe.
> > 
> > All these modifications prepare the introduction of a sysfs entry to
> > access this information. This way the diff will be much smaller. Thus,
> > there is no intended functional change.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> 
> Thanks Miquel:
> Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>

Thanks, queued.

-- Sebastian

> 
> Best regards,
>   Nicolas
> 
> > ---
> >   drivers/power/reset/at91-reset.c | 9 ++++-----
> >   1 file changed, 4 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/power/reset/at91-reset.c b/drivers/power/reset/at91-reset.c
> > index 741e44a017c3..d6884841a6dc 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 const char * __init at91_reset_reason(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:
> > @@ -185,7 +184,7 @@ static void __init at91_reset_status(struct platform_device *pdev,
> >                  break;
> >          }
> > 
> > -       dev_info(&pdev->dev, "Starting after %s\n", reason);
> > +       return reason;
> >   }
> > 
> >   static const struct of_device_id at91_ramc_of_match[] = {
> > @@ -392,7 +391,7 @@ static int __init at91_reset_probe(struct platform_device *pdev)
> >          if (ret)
> >                  goto disable_clk;
> > 
> > -       at91_reset_status(pdev, reset->rstc_base);
> > +       dev_info(&pdev->dev, "Starting after %s\n", at91_reset_reason(reset));
> > 
> >          return 0;
> > 
> > --
> > 2.34.1
> > 
> 
> -- 
> Nicolas Ferre
> 

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

[PATCH v3 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    | 12 ++++++
 drivers/power/reset/at91-reset.c              | 37 ++++++++++++++-----
 include/linux/power/power_on_reason.h         | 19 ++++++++++
 3 files changed, 58 insertions(+), 10 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..060d6c43f192
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-platform-power-on-reason
@@ -0,0 +1,12 @@
+What:		/sys/devices/platform/.../power_on_reason
+Date:		June 2023
+KernelVersion:	6.5
+Contact:	Kamel Bouhara <kamel.bouhara@bootlin.com>
+Description:	Shows system power on reason. The following strings/reasons can
+		be read (the list can be extended):
+		"regular power-up", "RTC wakeup", "watchdog timeout",
+		"software reset", "reset button action", "CPU clock failure",
+		"crystal oscillator failure", "low-power condition",
+		"unknown reason".
+
+		The file is read only.
diff --git a/drivers/power/reset/at91-reset.c b/drivers/power/reset/at91-reset.c
index d6884841a6dc..e42c0aa18966 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 const char * __init at91_reset_reason(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_REGULAR;
 		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_RST_BTN;
 		break;
 	case RESET_TYPE_CPU_FAIL:
-		reason = "CPU clock failure detection";
+		reason = POWER_ON_REASON_CPU_CLK_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;
 	}
 
 	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,6 +402,12 @@ static int __init at91_reset_probe(struct platform_device *pdev)
 	if (ret)
 		goto disable_clk;
 
+	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\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..a59d035f1a77
--- /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_REGULAR "regular power-up"
+#define POWER_ON_REASON_RTC "RTC wakeup"
+#define POWER_ON_REASON_WATCHDOG "watchdog timeout"
+#define POWER_ON_REASON_SOFTWARE "software reset"
+#define POWER_ON_REASON_RST_BTN "reset button action"
+#define POWER_ON_REASON_CPU_CLK_FAIL "CPU clock failure"
+#define POWER_ON_REASON_XTAL_FAIL "crystal oscillator failure"
+#define POWER_ON_REASON_LOW_POWER "low-power condition"
+#define POWER_ON_REASON_UNKNOWN "unknown reason"
+
+#endif /* POWER_ON_REASON_H */
-- 
2.34.1

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

[Sebastian Reichel]

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

Hi,

On Fri, Jun 16, 2023 at 03:52:52PM +0200, Miquel Raynal wrote:
> ...
> diff --git a/include/linux/power/power_on_reason.h b/include/linux/power/power_on_reason.h
> new file mode 100644
> index 000000000000..a59d035f1a77
> --- /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_REGULAR "regular power-up"
> +#define POWER_ON_REASON_RTC "RTC wakeup"
> +#define POWER_ON_REASON_WATCHDOG "watchdog timeout"
> +#define POWER_ON_REASON_SOFTWARE "software reset"
> +#define POWER_ON_REASON_RST_BTN "reset button action"
> +#define POWER_ON_REASON_CPU_CLK_FAIL "CPU clock failure"
> +#define POWER_ON_REASON_XTAL_FAIL "crystal oscillator failure"
> +#define POWER_ON_REASON_LOW_POWER "low-power condition"

IIUIC from previous discussion this should be:

#define POWER_ON_REASON_BROWN_OUT "brown-out reset"

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

Otherwise LGTM.

-- Sebastian

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