[PATCH] drivers: power: reset: at91-reset: Provide reset reason via sysfs (amended with Signed-off-by tag)

[PATCH] drivers: power: reset: at91-reset: Provide reset reason via sysfs (amended with Signed-off-by tag)

[David Mosberger-Tang]

Rather than just printing the reset-reason at boot-time, make it available
via sysfs as attribute "reset_reason" of the reset controller.  For example,
on SAMA5D2 Xplained, this sysfs attribute is available at:

 /sys/devices/platform/ahb/ahb:apb/f8048000.rstc/reset_reason

Signed-off-by: David Mosberger <davidm@egauge.net>
---
 drivers/power/reset/at91-reset.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/power/reset/at91-reset.c b/drivers/power/reset/at91-reset.c
index 264b9b8..184516b 100644
--- a/drivers/power/reset/at91-reset.c
+++ b/drivers/power/reset/at91-reset.c
@@ -45,6 +45,11 @@ enum reset_type {
 	RESET_TYPE_USER		= 4,
 };
 
+static ssize_t reset_reason_show(struct device *,
+				 struct device_attribute *attr,
+				 char *);
+static DEVICE_ATTR(reset_reason, S_IRUSR, reset_reason_show, NULL);
+
 static void __iomem *at91_ramc_base[2], *at91_rstc_base;
 
 /*
@@ -132,7 +137,7 @@ static int sama5d3_restart(struct notifier_block *this, unsigned long mode,
 	return NOTIFY_DONE;
 }
 
-static void __init at91_reset_status(struct platform_device *pdev)
+static const char *at91_reset_reason(struct platform_device *pdev)
 {
 	u32 reg = readl(at91_rstc_base + AT91_RSTC_SR);
 	char *reason;
@@ -157,8 +162,15 @@ static void __init at91_reset_status(struct platform_device *pdev)
 		reason = "unknown reset";
 		break;
 	}
+	return reason;
+}
 
-	pr_info("AT91: Starting after %s\n", reason);
+static ssize_t reset_reason_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	return strcpy(buf, at91_reset_reason(pdev));
 }
 
 static const struct of_device_id at91_ramc_of_match[] = {
@@ -250,7 +262,13 @@ static int at91_reset_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	at91_reset_status(pdev);
+	ret = device_create_file(&pdev->dev, &dev_attr_reset_reason);
+	if (ret) {
+		dev_err(&pdev->dev, "Could not create sysfs entry\n");
+		return ret;
+	}
+
+	pr_info("AT91: Starting after %s\n", at91_reset_reason(pdev));
 
 	return 0;
 }
-- 
1.9.1

[PATCH] drivers: power: reset: at91-reset: Provide reset reason via sysfs (amended with Signed-off-by tag)

[Alexandre Belloni]

Hi,

(Adding Sebastian in Cc)

On 21/01/2016 at 09:48:56 -0700, David Mosberger-Tang wrote :
> Rather than just printing the reset-reason at boot-time, make it available
> via sysfs as attribute "reset_reason" of the reset controller.  For example,
> on SAMA5D2 Xplained, this sysfs attribute is available at:
> 
>  /sys/devices/platform/ahb/ahb:apb/f8048000.rstc/reset_reason
> 
> Signed-off-by: David Mosberger <davidm@egauge.net>
> ---
>  drivers/power/reset/at91-reset.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/power/reset/at91-reset.c b/drivers/power/reset/at91-reset.c
> index 264b9b8..184516b 100644
> --- a/drivers/power/reset/at91-reset.c
> +++ b/drivers/power/reset/at91-reset.c
> @@ -45,6 +45,11 @@ enum reset_type {
>  	RESET_TYPE_USER		= 4,
>  };
>  
> +static ssize_t reset_reason_show(struct device *,
> +				 struct device_attribute *attr,
> +				 char *);
> +static DEVICE_ATTR(reset_reason, S_IRUSR, reset_reason_show, NULL);
> +

All sysfs files have to be documented. Can you add some documentation in
Documentation/ABI ?

Also, I think this should be added as a generic file for the whole subsystem.

>  static void __iomem *at91_ramc_base[2], *at91_rstc_base;
>  
>  /*
> @@ -132,7 +137,7 @@ static int sama5d3_restart(struct notifier_block *this, unsigned long mode,
>  	return NOTIFY_DONE;
>  }
>  
> -static void __init at91_reset_status(struct platform_device *pdev)
> +static const char *at91_reset_reason(struct platform_device *pdev)
>  {
>  	u32 reg = readl(at91_rstc_base + AT91_RSTC_SR);
>  	char *reason;
> @@ -157,8 +162,15 @@ static void __init at91_reset_status(struct platform_device *pdev)
>  		reason = "unknown reset";
>  		break;
>  	}
> +	return reason;
> +}
>  
> -	pr_info("AT91: Starting after %s\n", reason);

I'm pretty sure Nicolas will not want the reset reason to disappear from
the kernel log.

> +static ssize_t reset_reason_show(struct device *dev,
> +				 struct device_attribute *attr,
> +				 char *buf)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	return strcpy(buf, at91_reset_reason(pdev));
>  }
>  
>  static const struct of_device_id at91_ramc_of_match[] = {
> @@ -250,7 +262,13 @@ static int at91_reset_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	at91_reset_status(pdev);
> +	ret = device_create_file(&pdev->dev, &dev_attr_reset_reason);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Could not create sysfs entry\n");
> +		return ret;
> +	}
> +
> +	pr_info("AT91: Starting after %s\n", at91_reset_reason(pdev));
>  
>  	return 0;
>  }

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH] drivers: power: reset: at91-reset: Provide reset reason via sysfs (amended with Signed-off-by tag)

[David Mosberger]

Alexandre,

Thanks for taking a look at the patch.

On Mon, Jan 25, 2016 at 1:55 AM, Alexandre Belloni
<alexandre.belloni@free-electrons.com> wrote:

> All sysfs files have to be documented. Can you add some documentation in
> Documentation/ABI ?

I'm not familiar with out this is organized and what file this would
have to be added to.  Other than that, sure I'd be happy to write
something up.

> Also, I think this should be added as a generic file for the whole subsystem.

A patch that shows what you have in mind would be helpful here.

>> -     pr_info("AT91: Starting after %s\n", reason);

You must have missed:

>> +     pr_info("AT91: Starting after %s\n", at91_reset_reason(pdev));

Best regards,

  --david
-- 
eGauge Systems LLC, http://egauge.net/, 1.877-EGAUGE1, fax 720.545.9768

[PATCH] drivers: power: reset: at91-reset: Provide reset reason via sysfs (amended with Signed-off-by tag)

[Sebastian Reichel]

Hi,

On Mon, Jan 25, 2016 at 09:55:39AM +0100, Alexandre Belloni wrote:
> (Adding Sebastian in Cc)

Thanks :)

> [...]
> > +static ssize_t reset_reason_show(struct device *,
> > +				 struct device_attribute *attr,
> > +				 char *);
> > +static DEVICE_ATTR(reset_reason, S_IRUSR, reset_reason_show, NULL);

IMHO power_on_reason is more fitting, since it also covers things
like "wake-on-rtc" or "power-button".

> All sysfs files have to be documented. Can you add some documentation in
> Documentation/ABI ?
> 
> Also, I think this should be added as a generic file for the whole
> subsystem.

Currently the subsystem does not have any generic files, since the
power/reset drivers just take care of system reset/shutdown. I think
we do not need to introduce it for one generic file, but we should
try to standardize the ABI (including the values).

I can see, that there are currently six different values possible
for at91-reset, but it's unclear to me what some of them mean, so
that should be covered in the ABI documentation. Also it may make
sense to drop the reset suffix for some of them (e.g. "unknown").

    reason = "general reset";

    This is power-on by power-button/applying voltage?

    reason = "wakeup";

    This is power-on by RTC?

    reason = "watchdog reset";

    I gues this is reboot triggered by watchdog timeout?

    reason = "software reset";

    This will be returned if system was started by rebooting via
    "reboot / shutdown -r"?

    reason = "user reset";

    This is returned if the board's reset button was pressed?

    reason = "unknown reset";

    => just use "unknown"

I suggest to create include/linux/power/power_on_reason.h, with
standardized values, that are used by the driver:

#define POWER_ON_REASON_RTC "RTC wakeup"
#define POWER_ON_REASON_WATCHDOG "watchdog timeout"
...

> [...]

-- Sebastian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160127/2972f6b1/attachment.sig>

[PATCH] drivers: power: reset: at91-reset: Provide reset reason via sysfs (amended with Signed-off-by tag)

[Alexandre Belloni]

On 27/01/2016 at 08:14:21 +0100, Sebastian Reichel wrote :
> I can see, that there are currently six different values possible
> for at91-reset, but it's unclear to me what some of them mean, so
> that should be covered in the ABI documentation. Also it may make
> sense to drop the reset suffix for some of them (e.g. "unknown").
> 
>     reason = "general reset";
> 
>     This is power-on by power-button/applying voltage?
> 

Yes

>     reason = "wakeup";
> 
>     This is power-on by RTC?
> 

This can actually be any wakeup, but most likely RTC but it could also
be triggered by an external PMIC. Atmel's definition is general: both
VDDcore and VDDbu are rising, wakeup: only VDDcore is rising

>     reason = "watchdog reset";
> 
>     I gues this is reboot triggered by watchdog timeout?
> 

Yes

>     reason = "software reset";
> 
>     This will be returned if system was started by rebooting via
>     "reboot / shutdown -r"?
> 

This will be the case yes

>     reason = "user reset";
> 
>     This is returned if the board's reset button was pressed?
> 

Reset button or any signal on the reset pin of the SoC


>     reason = "unknown reset";
> 
>     => just use "unknown"
> 

It should not happen anyway :)

> I suggest to create include/linux/power/power_on_reason.h, with
> standardized values, that are used by the driver:
> 
> #define POWER_ON_REASON_RTC "RTC wakeup"
> #define POWER_ON_REASON_WATCHDOG "watchdog timeout"
> ...
> 


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com