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

PATCH New David Mosberger-Tang 3 participants 5 messages Jan 21, 2016 – Feb 15, 2016

Subsystems (2)

Arm/Microchip (AT91) SoC support System reset/shutdown drivers

lore

Share & tools

lore

David Mosberger-Tang

author

2016-01-21 16:48 UTC #0 lore raw

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

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 <[email protected]>

---
 drivers/power/reset/at91-reset.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

drivers/power/reset/at91-reset.c +21 -3

index 264b9b8..184516b 100644
@@ -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;
 }

Signature · 1 line

1.9.1

Alexandre Belloni

2016-01-25 08:55 UTC #1 lore raw

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

Hi,

(Adding Sebastian in Cc)

Quoted 16 lines · 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 <[email protected]>
---
 drivers/power/reset/at91-reset.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)
drivers/power/reset/at91-reset.c +0 -0
index 264b9b8..184516b 100644

drivers/power/reset/at91-reset.c enum reset_type { · line 52

Quoted 8 lines

drivers/power/reset/at91-reset.c +5 -0

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

Quoted 13 lines

drivers/power/reset/at91-reset.c +1 -1

 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;

drivers/power/reset/at91-reset.c static void __init at91_reset_status(struct platform_device *pdev) · line 161

Quoted 7 lines

drivers/power/reset/at91-reset.c +2 -1

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

Quoted 24 lines

drivers/power/reset/at91-reset.c +13 -1

+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;
 }

Signature · 3 lines

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

David Mosberger

author

2016-01-25 16:17 UTC #2 lore raw

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

Alexandre,

Thanks for taking a look at the patch.

Quoted 2 lines · Alexandre Belloni

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.

Quoted 1 line

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.

Quoted 1 line

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

You must have missed:

Quoted 1 line

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

Best regards,

--david

Signature · 1 line

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

Sebastian Reichel

maintainer

2016-01-27 07:14 UTC #3 lore raw

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

In reply to Alexandre Belloni

Hi,

Quoted 1 line · Alexandre Belloni

(Adding Sebastian in Cc)

Thanks :)

Quoted 5 lines

[...]
drivers/power/reset/at91-reset.c enum reset_type { · line 51
Quoted 4 lines
drivers/power/reset/at91-reset.c +4 -0
+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".

Quoted 5 lines

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

Quoted 1 line

[...]

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

Alexandre Belloni

2016-02-15 19:47 UTC #4 lore raw

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

Quoted 9 lines · Sebastian Reichel

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

Quoted 4 lines

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

Quoted 4 lines

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

Yes

Quoted 5 lines

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

This will be the case yes

Quoted 4 lines

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

Quoted 4 lines

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

It should not happen anyway :)

Quoted 7 lines

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

Signature · 3 lines

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