| Message ID | 1453394936-16616-1-git-send-email-davidm@egauge.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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, 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
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
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" > ... >
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; }
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(-)