| Message ID | 1444220352-20548-2-git-send-email-harald@ccbib.org |
|---|---|
| State | Not Applicable |
| Headers | show |
On Wed, Oct 07, 2015 at 12:19:12PM +0000, Harald Geyer wrote: > This device doesn't provide any information about boot status. > As workaround we use a persitent bit to track watchdog activity. Hmm, so you set a bit in a persistent register iff the watchdog is running. And if that bit is set during probe you assume the watchdog reset the machine. But this also happens, when the user presses the reset button which makes the detection unreliable. > > Signed-off-by: Harald Geyer <harald@ccbib.org> > --- > Changes since v2: > * make code ordering more consistent > * move part of the commit message to a code comment > * rewrite the commit message > > Changes since v1: > * make code formatting more consistent with the rest of the driver > * Cc some people who might have better documentation then I do > > Changes since initially posting this patch on 08/04/2015: > * fix a spelling error in the commit message > * rebase to a recent version > > drivers/rtc/rtc-stmp3xxx.c | 37 +++++++++++++++++++++++++++++++++++ > drivers/watchdog/stmp3xxx_rtc_wdt.c | 6 +++++- > include/linux/stmp3xxx_rtc_wdt.h | 2 ++ > 3 files changed, 44 insertions(+), 1 deletion(-) > > diff --git a/drivers/rtc/rtc-stmp3xxx.c b/drivers/rtc/rtc-stmp3xxx.c > index ca54d03..47947ea 100644 > --- a/drivers/rtc/rtc-stmp3xxx.c > +++ b/drivers/rtc/rtc-stmp3xxx.c > @@ -30,6 +30,7 @@ > #include <linux/of.h> > #include <linux/stmp_device.h> > #include <linux/stmp3xxx_rtc_wdt.h> > +#include <linux/watchdog.h> > > #define STMP3XXX_RTC_CTRL 0x0 > #define STMP3XXX_RTC_CTRL_ALARM_IRQ_EN 0x00000001 > @@ -62,6 +63,9 @@ > /* missing bitmask in headers */ > #define STMP3XXX_RTC_PERSISTENT1_FORCE_UPDATER 0x80000000 > > +#define STMP3XXX_RTC_PERSISTENT2 0x80 > +#define STMP3XXX_RTC_PERSISTENT2_WDT_ACTIVE 0x00000001 > + > struct stmp3xxx_rtc_data { > struct rtc_device *rtc; > void __iomem *io; > @@ -81,6 +85,20 @@ struct stmp3xxx_rtc_data { > * The watchdog driver is passed the below accessor function via platform_data > * to configure the watchdog. Locking is not needed because accessing SET/CLR > * registers is atomic. > + * > + * Since this device doesn't report the cause of the last reset, we use > + * a persistent bit to track watchdog activity. The code from the Freescale > + * BSP uses the STMP3XXX_RTC_PERSISTENT1 register, which is dedicated to > + * controlling the boot ROM, for this purpose. However it seems the bit > + * there can't be cleared once it has been set. So we are using > + * STMP3XXX_RTC_PERSISTENT2 instead, which is the first register available > + * for "software use" without restriction. Adding a link to the vendor kernel might be interesting here. > + * I (Harald Geyer <harald@ccbib.org>) don't know if the code touching the > + * STMP3XXX_RTC_PERSISTENT1 register is doing anything useful. Maybe this > + * is just a leftover from the BSP code, but then maybe there is something > + * in the boot ROM or in some bootloader that is using this. Hard to tell > + * without proper documentation about this register. Fabio: anything you can do here to help? > */ > > static void stmp3xxx_wdt_set_timeout(struct device *dev, u32 timeout) > @@ -93,16 +111,30 @@ static void stmp3xxx_wdt_set_timeout(struct device *dev, u32 timeout) > rtc_data->io + STMP3XXX_RTC_CTRL + STMP_OFFSET_REG_SET); > writel(STMP3XXX_RTC_PERSISTENT1_FORCE_UPDATER, > rtc_data->io + STMP3XXX_RTC_PERSISTENT1 + STMP_OFFSET_REG_SET); > + writel(STMP3XXX_RTC_PERSISTENT2_WDT_ACTIVE, > + rtc_data->io + STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_SET); > } else { > writel(STMP3XXX_RTC_CTRL_WATCHDOGEN, > rtc_data->io + STMP3XXX_RTC_CTRL + STMP_OFFSET_REG_CLR); > writel(STMP3XXX_RTC_PERSISTENT1_FORCE_UPDATER, > rtc_data->io + STMP3XXX_RTC_PERSISTENT1 + STMP_OFFSET_REG_CLR); > + writel(STMP3XXX_RTC_PERSISTENT2_WDT_ACTIVE, > + rtc_data->io + STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_CLR); checkpatch tells: WARNING: line over 80 characters #131: FILE: drivers/rtc/rtc-stmp3xxx.c:115: + rtc_data->io + STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_SET); WARNING: line over 80 characters #138: FILE: drivers/rtc/rtc-stmp3xxx.c:122: + rtc_data->io + STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_CLR); > } > } > > +static void stmp3xxx_wdt_clear_bootstatus(struct device *dev) > +{ > + struct stmp3xxx_rtc_data *rtc_data = dev_get_drvdata(dev); > + > + writel(STMP3XXX_RTC_PERSISTENT2_WDT_ACTIVE, > + rtc_data->io + STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_CLR); > +} > + > static struct stmp3xxx_wdt_pdata wdt_pdata = { > .wdt_set_timeout = stmp3xxx_wdt_set_timeout, > + .wdt_clear_bootstatus = stmp3xxx_wdt_clear_bootstatus, > + .bootstatus = 0, > }; This is out of the scope of your patch, but IMHO wdt_pdata should be const in case it is used for >1 device. Best regards Uwe
Hi Uwe, thanks for looking into this. Uwe Kleine-König writes: > On Wed, Oct 07, 2015 at 12:19:12PM +0000, Harald Geyer wrote: > > This device doesn't provide any information about boot status. > > As workaround we use a persitent bit to track watchdog activity. > > Hmm, so you set a bit in a persistent register iff the watchdog is > running. And if that bit is set during probe you assume the watchdog > reset the machine. But this also happens, when the user presses the > reset button which makes the detection unreliable. Yes, kind of. I'm using this driver on an imx233-olinuxino, where the reset button causes the device to power cycle and thus clears the persitent bit. So I can't test your case, but surely there are some paths into reset, that don't clear the bit and cause false positives. I have considered gathering some additional information from the power subsystem to refine the result, but decided that it's not worth the complexity. Since the power system is a different silicon block, we can't rely on it being available and would need to provide a fall back implementation anyway. I believe my patch does all that can be done with only the registers, that belong to the device, but if you have any idea how to make it more reliable, then I'd be very interested in that. > > Signed-off-by: Harald Geyer <harald@ccbib.org> > > --- > > Changes since v2: > > * make code ordering more consistent > > * move part of the commit message to a code comment > > * rewrite the commit message > > > > Changes since v1: > > * make code formatting more consistent with the rest of the driver > > * Cc some people who might have better documentation then I do > > > > Changes since initially posting this patch on 08/04/2015: > > * fix a spelling error in the commit message > > * rebase to a recent version > > > > drivers/rtc/rtc-stmp3xxx.c | 37 +++++++++++++++++++++++++++++++++++ > > drivers/watchdog/stmp3xxx_rtc_wdt.c | 6 +++++- > > include/linux/stmp3xxx_rtc_wdt.h | 2 ++ > > 3 files changed, 44 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/rtc/rtc-stmp3xxx.c b/drivers/rtc/rtc-stmp3xxx.c > > index ca54d03..47947ea 100644 > > --- a/drivers/rtc/rtc-stmp3xxx.c > > +++ b/drivers/rtc/rtc-stmp3xxx.c > > @@ -30,6 +30,7 @@ > > #include <linux/of.h> > > #include <linux/stmp_device.h> > > #include <linux/stmp3xxx_rtc_wdt.h> > > +#include <linux/watchdog.h> > > > > #define STMP3XXX_RTC_CTRL 0x0 > > #define STMP3XXX_RTC_CTRL_ALARM_IRQ_EN 0x00000001 > > @@ -62,6 +63,9 @@ > > /* missing bitmask in headers */ > > #define STMP3XXX_RTC_PERSISTENT1_FORCE_UPDATER 0x80000000 > > > > +#define STMP3XXX_RTC_PERSISTENT2 0x80 > > +#define STMP3XXX_RTC_PERSISTENT2_WDT_ACTIVE 0x00000001 > > + > > struct stmp3xxx_rtc_data { > > struct rtc_device *rtc; > > void __iomem *io; > > @@ -81,6 +85,20 @@ struct stmp3xxx_rtc_data { > > * The watchdog driver is passed the below accessor function via platform_data > > * to configure the watchdog. Locking is not needed because accessing SET/CLR > > * registers is atomic. > > + * > > + * Since this device doesn't report the cause of the last reset, we use > > + * a persistent bit to track watchdog activity. The code from the Freescale > > + * BSP uses the STMP3XXX_RTC_PERSISTENT1 register, which is dedicated to > > + * controlling the boot ROM, for this purpose. However it seems the bit > > + * there can't be cleared once it has been set. So we are using > > + * STMP3XXX_RTC_PERSISTENT2 instead, which is the first register available > > + * for "software use" without restriction. > > Adding a link to the vendor kernel might be interesting here. Ok, I'll try to dig it up again. > > + * I (Harald Geyer <harald@ccbib.org>) don't know if the code touching the > > + * STMP3XXX_RTC_PERSISTENT1 register is doing anything useful. Maybe this > > + * is just a leftover from the BSP code, but then maybe there is something > > + * in the boot ROM or in some bootloader that is using this. Hard to tell > > + * without proper documentation about this register. > > Fabio: anything you can do here to help? > > > */ > > > > static void stmp3xxx_wdt_set_timeout(struct device *dev, u32 timeout) > > @@ -93,16 +111,30 @@ static void stmp3xxx_wdt_set_timeout(struct device *dev, u32 timeout) > > rtc_data->io + STMP3XXX_RTC_CTRL + STMP_OFFSET_REG_SET); > > writel(STMP3XXX_RTC_PERSISTENT1_FORCE_UPDATER, > > rtc_data->io + STMP3XXX_RTC_PERSISTENT1 + STMP_OFFSET_REG_SET); > > + writel(STMP3XXX_RTC_PERSISTENT2_WDT_ACTIVE, > > + rtc_data->io + STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_SET); > > } else { > > writel(STMP3XXX_RTC_CTRL_WATCHDOGEN, > > rtc_data->io + STMP3XXX_RTC_CTRL + STMP_OFFSET_REG_CLR); > > writel(STMP3XXX_RTC_PERSISTENT1_FORCE_UPDATER, > > rtc_data->io + STMP3XXX_RTC_PERSISTENT1 + STMP_OFFSET_REG_CLR); > > + writel(STMP3XXX_RTC_PERSISTENT2_WDT_ACTIVE, > > + rtc_data->io + STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_CLR); > > checkpatch tells: > > WARNING: line over 80 characters > #131: FILE: drivers/rtc/rtc-stmp3xxx.c:115: > + rtc_data->io + STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_SET); > > WARNING: line over 80 characters > #138: FILE: drivers/rtc/rtc-stmp3xxx.c:122: > + rtc_data->io + STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_CLR); I tried to follow the style of the surrounding code. If there is a clear statement about the preferred style for this driver from whoever is responsible, I will happily change my patch to whatever it is. > > } > > } > > > > +static void stmp3xxx_wdt_clear_bootstatus(struct device *dev) > > +{ > > + struct stmp3xxx_rtc_data *rtc_data = dev_get_drvdata(dev); > > + > > + writel(STMP3XXX_RTC_PERSISTENT2_WDT_ACTIVE, > > + rtc_data->io + STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_CLR); > > +} > > + > > static struct stmp3xxx_wdt_pdata wdt_pdata = { > > .wdt_set_timeout = stmp3xxx_wdt_set_timeout, > > + .wdt_clear_bootstatus = stmp3xxx_wdt_clear_bootstatus, > > + .bootstatus = 0, > > }; > > This is out of the scope of your patch, but IMHO wdt_pdata should be > const in case it is used for >1 device. Hm, if you want to change wdt_pdata to const, then I can't add the bootstatus field. But I think you can't change this to const anyway, because the platform_data pointer of struct device is not a const void pointer. However didn't look into this in detail, so maybe I'm missing something ... I guess we could move wdt_pdata into rtc_data, but this would entangle rtc and watchdog drivers even more, so I'd rather not do that. What do you think? Thanks, Harald
Hello Harald, On Thu, Nov 26, 2015 at 02:39:45PM +0100, Harald Geyer wrote: > Uwe Kleine-König writes: > > On Wed, Oct 07, 2015 at 12:19:12PM +0000, Harald Geyer wrote: > > > This device doesn't provide any information about boot status. > > > As workaround we use a persitent bit to track watchdog activity. > > > > Hmm, so you set a bit in a persistent register iff the watchdog is > > running. And if that bit is set during probe you assume the watchdog > > reset the machine. But this also happens, when the user presses the > > reset button which makes the detection unreliable. > > Yes, kind of. I'm using this driver on an imx233-olinuxino, where the > reset button causes the device to power cycle and thus clears the > persitent bit. So I can't test your case, but surely there are some paths So the rtc isn't properly battery backed? AFAIUI the persistent bits should survive a power cycle?! I think somewhere in our hardware repository at Pengutronix we have a machine where I can test that, I will check that. > into reset, that don't clear the bit and cause false positives. Best regards Uwe
Hi Uwe, Uwe Kleine-König writes: > On Thu, Nov 26, 2015 at 02:39:45PM +0100, Harald Geyer wrote: > > Uwe Kleine-König writes: > > > Hmm, so you set a bit in a persistent register iff the watchdog is > > > running. And if that bit is set during probe you assume the watchdog > > > reset the machine. But this also happens, when the user presses the > > > reset button which makes the detection unreliable. > > > > Yes, kind of. I'm using this driver on an imx233-olinuxino, where the > > reset button causes the device to power cycle and thus clears the > > persitent bit. So I can't test your case, but surely there are some paths > > So the rtc isn't properly battery backed? Yes, since the mainline kernel can't do anything useful with the battery other than reporting it's voltage, nobody uses imx233-olinuxino with mainline kernel and battery. > AFAIUI the persistent bits should survive a power cycle?! I only tested without battery, and in my case they definitely don't. > I think somewhere in our hardware > repository at Pengutronix we have a machine where I can test that, I > will check that. Thanks! Best regards, Harald > > into reset, that don't clear the bit and cause false positives. > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | http://www.pengutronix.de/ |
On Thu, Nov 26, 2015 at 02:39:45PM +0100, Harald Geyer wrote: > Hi Uwe, > > thanks for looking into this. > > Uwe Kleine-König writes: > > On Wed, Oct 07, 2015 at 12:19:12PM +0000, Harald Geyer wrote: > > > This device doesn't provide any information about boot status. > > > As workaround we use a persitent bit to track watchdog activity. > > > > Hmm, so you set a bit in a persistent register iff the watchdog is > > running. And if that bit is set during probe you assume the watchdog > > reset the machine. But this also happens, when the user presses the > > reset button which makes the detection unreliable. > > Yes, kind of. I'm using this driver on an imx233-olinuxino, where the > reset button causes the device to power cycle and thus clears the > persitent bit. So I can't test your case, but surely there are some paths > into reset, that don't clear the bit and cause false positives. OK, I found a i.MX28 machine in our repository: barebox# mw 0x80056080 0xbabecafe barebox# boot ... linux# reboot barebox# md 0x80056080+4 80056080: babecafe After a power cycle the value disappears (I didn't check the details, if there is a battery it might be empty). > I have considered gathering some additional information from the > power subsystem to refine the result, but decided that it's not > worth the complexity. Since the power system is a different silicon > block, we can't rely on it being available and would need to provide > a fall back implementation anyway. It would be really nice to get some details about boot rom usage of the persistent flags in use. I'd expect that these allow to determine the boot source. > > > */ > > > > > > static void stmp3xxx_wdt_set_timeout(struct device *dev, u32 timeout) > > > @@ -93,16 +111,30 @@ static void stmp3xxx_wdt_set_timeout(struct device *dev, u32 timeout) > > > rtc_data->io + STMP3XXX_RTC_CTRL + STMP_OFFSET_REG_SET); > > > writel(STMP3XXX_RTC_PERSISTENT1_FORCE_UPDATER, > > > rtc_data->io + STMP3XXX_RTC_PERSISTENT1 + STMP_OFFSET_REG_SET); > > > + writel(STMP3XXX_RTC_PERSISTENT2_WDT_ACTIVE, > > > + rtc_data->io + STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_SET); > > > } else { > > > writel(STMP3XXX_RTC_CTRL_WATCHDOGEN, > > > rtc_data->io + STMP3XXX_RTC_CTRL + STMP_OFFSET_REG_CLR); > > > writel(STMP3XXX_RTC_PERSISTENT1_FORCE_UPDATER, > > > rtc_data->io + STMP3XXX_RTC_PERSISTENT1 + STMP_OFFSET_REG_CLR); > > > + writel(STMP3XXX_RTC_PERSISTENT2_WDT_ACTIVE, > > > + rtc_data->io + STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_CLR); > > > > checkpatch tells: > > > > WARNING: line over 80 characters > > #131: FILE: drivers/rtc/rtc-stmp3xxx.c:115: > > + rtc_data->io + STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_SET); > > > > WARNING: line over 80 characters > > #138: FILE: drivers/rtc/rtc-stmp3xxx.c:122: > > + rtc_data->io + STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_CLR); > > I tried to follow the style of the surrounding code. If there is a clear > statement about the preferred style for this driver from whoever is > responsible, I will happily change my patch to whatever it is. Yeah, I don't care much. I would have broken the newly introduced lines, but I'd not go so far to nack your patch when you don't. > > > } > > > } > > > > > > +static void stmp3xxx_wdt_clear_bootstatus(struct device *dev) > > > +{ > > > + struct stmp3xxx_rtc_data *rtc_data = dev_get_drvdata(dev); > > > + > > > + writel(STMP3XXX_RTC_PERSISTENT2_WDT_ACTIVE, > > > + rtc_data->io + STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_CLR); > > > +} > > > + > > > static struct stmp3xxx_wdt_pdata wdt_pdata = { > > > .wdt_set_timeout = stmp3xxx_wdt_set_timeout, > > > + .wdt_clear_bootstatus = stmp3xxx_wdt_clear_bootstatus, > > > + .bootstatus = 0, > > > }; > > > > This is out of the scope of your patch, but IMHO wdt_pdata should be > > const in case it is used for >1 device. > > Hm, if you want to change wdt_pdata to const, then I can't add the > bootstatus field. But I think you can't change this to const anyway, > because the platform_data pointer of struct device is not a const void > pointer. However didn't look into this in detail, so maybe I'm missing > something ... Conceptually platform data should be const for a driver, not necessarily for the code that sets it up. So the right thing to do is to add a const to struct device::platform_data. That is usually not in the way during creation of the device, but stops drivers to modify it. Then you can do in stmp3xxx_wdt_register: struct ... pdata = { .wdt_set_timeout = stmp3xxx_wdt_set_timeout, .bootstatus = whatever; }; struct platform_device_info pdevinfo = { .parent = &rtc_pdev->dev; .data = pdata; .size_data = sizeof(pdata); } platform_device_register_full(&pdevinfo); Best regards Uwe
Hello Harald, On Fri, Nov 27, 2015 at 11:02:54AM +0100, Uwe Kleine-König wrote: > On Thu, Nov 26, 2015 at 02:39:45PM +0100, Harald Geyer wrote: > > Uwe Kleine-König writes: > > > On Wed, Oct 07, 2015 at 12:19:12PM +0000, Harald Geyer wrote: > > > > This device doesn't provide any information about boot status. > > > > As workaround we use a persitent bit to track watchdog activity. > > > > > > Hmm, so you set a bit in a persistent register iff the watchdog is > > > running. And if that bit is set during probe you assume the watchdog > > > reset the machine. But this also happens, when the user presses the > > > reset button which makes the detection unreliable. > > > > Yes, kind of. I'm using this driver on an imx233-olinuxino, where the > > reset button causes the device to power cycle and thus clears the > > persitent bit. So I can't test your case, but surely there are some paths > > into reset, that don't clear the bit and cause false positives. > > OK, I found a i.MX28 machine in our repository: > > barebox# mw 0x80056080 0xbabecafe > barebox# boot > ... > linux# reboot > barebox# md 0x80056080+4 > 80056080: babecafe > > After a power cycle the value disappears (I didn't check the details, if > there is a battery it might be empty). > > > I have considered gathering some additional information from the > > power subsystem to refine the result, but decided that it's not > > worth the complexity. Since the power system is a different silicon > > block, we can't rely on it being available and would need to provide > > a fall back implementation anyway. > > It would be really nice to get some details about boot rom usage of the > persistent flags in use. I'd expect that these allow to determine the > boot source. A colleague just hinted me to http://git.pengutronix.de/?p=barebox.git;a=blob;f=drivers/watchdog/im28wd.c;h=3510776a3afc8aaf113eee995900b099c12c2be9;hb=HEAD which contains a few bit definitions for the i.MX28. Maybe they can help you to understand your machine better?! At a quick glance there is nothing that helps you to determine the boot reason, but still it might contain a few interesting things. Best regards Uwe
On 27/11/2015 at 11:02:54 +0100, Uwe Kleine-König wrote : > OK, I found a i.MX28 machine in our repository: > > barebox# mw 0x80056080 0xbabecafe > barebox# boot > ... > linux# reboot > barebox# md 0x80056080+4 > 80056080: babecafe > > After a power cycle the value disappears (I didn't check the details, if > there is a battery it might be empty). > Yeah, you'd have to ensure that the crystal power domain (VDDXTAL) stays powered.
Hi Uwe! On 27.11.2015 11:44, Uwe Kleine-König wrote: > Hello Harald, > > On Fri, Nov 27, 2015 at 11:02:54AM +0100, Uwe Kleine-König wrote: >> On Thu, Nov 26, 2015 at 02:39:45PM +0100, Harald Geyer wrote: >> > Uwe Kleine-König writes: >> > > On Wed, Oct 07, 2015 at 12:19:12PM +0000, Harald Geyer wrote: >> > > > This device doesn't provide any information about boot status. >> > > > As workaround we use a persitent bit to track watchdog >> activity. >> > > >> > > Hmm, so you set a bit in a persistent register iff the watchdog >> is >> > > running. And if that bit is set during probe you assume the >> watchdog >> > > reset the machine. But this also happens, when the user presses >> the >> > > reset button which makes the detection unreliable. >> > >> > Yes, kind of. I'm using this driver on an imx233-olinuxino, where >> the >> > reset button causes the device to power cycle and thus clears the >> > persitent bit. So I can't test your case, but surely there are >> some paths >> > into reset, that don't clear the bit and cause false positives. >> >> OK, I found a i.MX28 machine in our repository: >> >> barebox# mw 0x80056080 0xbabecafe >> barebox# boot >> ... >> linux# reboot >> barebox# md 0x80056080+4 >> 80056080: babecafe >> >> After a power cycle the value disappears (I didn't check the >> details, if >> there is a battery it might be empty). Are there any concerns left on your side? Ie do you want me to do any additional tests before sending v4 of this patch with the whitespace changes and added URL to the freescale kernel? >> > I have considered gathering some additional information from the >> > power subsystem to refine the result, but decided that it's not >> > worth the complexity. Since the power system is a different >> silicon >> > block, we can't rely on it being available and would need to >> provide >> > a fall back implementation anyway. >> >> It would be really nice to get some details about boot rom usage of >> the >> persistent flags in use. I'd expect that these allow to determine >> the >> boot source. > > A colleague just hinted me to > > > http://git.pengutronix.de/?p=barebox.git;a=blob;f=drivers/watchdog/im28wd.c;h=3510776a3afc8aaf113eee995900b099c12c2be9;hb=HEAD > > which contains a few bit definitions for the i.MX28. Maybe they can > help > you to understand your machine better?! At a quick glance there is > nothing that helps you to determine the boot reason, but still it > might > contain a few interesting things. Thanks. Unfortunately about the bit that we set in mainline kernel for unknown reasons, it also only says: /* dubious meaning from inside the SoC's firmware ROM */ # define MXS_RTC_PERSISTENT1_FORCE_UPDATER (1 << 31) best regards, Harald
Hello Harald, On Sun, Nov 29, 2015 at 11:41:22AM +0100, Harald Geyer wrote: > On 27.11.2015 11:44, Uwe Kleine-König wrote: > >Hello Harald, > > > >On Fri, Nov 27, 2015 at 11:02:54AM +0100, Uwe Kleine-König wrote: > >>On Thu, Nov 26, 2015 at 02:39:45PM +0100, Harald Geyer wrote: > >>> Uwe Kleine-König writes: > >>> > On Wed, Oct 07, 2015 at 12:19:12PM +0000, Harald Geyer wrote: > >>> > > This device doesn't provide any information about boot status. > >>> > > As workaround we use a persitent bit to track watchdog activity. > >>> > > >>> > Hmm, so you set a bit in a persistent register iff the watchdog is > >>> > running. And if that bit is set during probe you assume the > >>watchdog > >>> > reset the machine. But this also happens, when the user presses the > >>> > reset button which makes the detection unreliable. > >>> > >>> Yes, kind of. I'm using this driver on an imx233-olinuxino, where the > >>> reset button causes the device to power cycle and thus clears the > >>> persitent bit. So I can't test your case, but surely there are some > >>paths > >>> into reset, that don't clear the bit and cause false positives. > >> > >>OK, I found a i.MX28 machine in our repository: > >> > >> barebox# mw 0x80056080 0xbabecafe > >> barebox# boot > >> ... > >> linux# reboot > >> barebox# md 0x80056080+4 > >> 80056080: babecafe > >> > >>After a power cycle the value disappears (I didn't check the details, if > >>there is a battery it might be empty). > > Are there any concerns left on your side? Ie do you want me to do > any additional tests before sending v4 of this patch with the > whitespace changes and added URL to the freescale kernel? I just notice that I failed to do the proper test. I.e. I reset the machine using "reboot" instead of a reset button. But I'd expect that doing the latter also keeps the value of the persistant register, which would make your patch wrong here. I will check that tomorrow when I can make someone do this test for me. (I'm 600 km from the machine above away.) Best regards Uwe
diff --git a/drivers/rtc/rtc-stmp3xxx.c b/drivers/rtc/rtc-stmp3xxx.c index ca54d03..47947ea 100644 --- a/drivers/rtc/rtc-stmp3xxx.c +++ b/drivers/rtc/rtc-stmp3xxx.c @@ -30,6 +30,7 @@ #include <linux/of.h> #include <linux/stmp_device.h> #include <linux/stmp3xxx_rtc_wdt.h> +#include <linux/watchdog.h> #define STMP3XXX_RTC_CTRL 0x0 #define STMP3XXX_RTC_CTRL_ALARM_IRQ_EN 0x00000001 @@ -62,6 +63,9 @@ /* missing bitmask in headers */ #define STMP3XXX_RTC_PERSISTENT1_FORCE_UPDATER 0x80000000 +#define STMP3XXX_RTC_PERSISTENT2 0x80 +#define STMP3XXX_RTC_PERSISTENT2_WDT_ACTIVE 0x00000001 + struct stmp3xxx_rtc_data { struct rtc_device *rtc; void __iomem *io; @@ -81,6 +85,20 @@ struct stmp3xxx_rtc_data { * The watchdog driver is passed the below accessor function via platform_data * to configure the watchdog. Locking is not needed because accessing SET/CLR * registers is atomic. + * + * Since this device doesn't report the cause of the last reset, we use + * a persistent bit to track watchdog activity. The code from the Freescale + * BSP uses the STMP3XXX_RTC_PERSISTENT1 register, which is dedicated to + * controlling the boot ROM, for this purpose. However it seems the bit + * there can't be cleared once it has been set. So we are using + * STMP3XXX_RTC_PERSISTENT2 instead, which is the first register available + * for "software use" without restriction. + * + * I (Harald Geyer <harald@ccbib.org>) don't know if the code touching the + * STMP3XXX_RTC_PERSISTENT1 register is doing anything useful. Maybe this + * is just a leftover from the BSP code, but then maybe there is something + * in the boot ROM or in some bootloader that is using this. Hard to tell + * without proper documentation about this register. */ static void stmp3xxx_wdt_set_timeout(struct device *dev, u32 timeout) @@ -93,16 +111,30 @@ static void stmp3xxx_wdt_set_timeout(struct device *dev, u32 timeout) rtc_data->io + STMP3XXX_RTC_CTRL + STMP_OFFSET_REG_SET); writel(STMP3XXX_RTC_PERSISTENT1_FORCE_UPDATER, rtc_data->io + STMP3XXX_RTC_PERSISTENT1 + STMP_OFFSET_REG_SET); + writel(STMP3XXX_RTC_PERSISTENT2_WDT_ACTIVE, + rtc_data->io + STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_SET); } else { writel(STMP3XXX_RTC_CTRL_WATCHDOGEN, rtc_data->io + STMP3XXX_RTC_CTRL + STMP_OFFSET_REG_CLR); writel(STMP3XXX_RTC_PERSISTENT1_FORCE_UPDATER, rtc_data->io + STMP3XXX_RTC_PERSISTENT1 + STMP_OFFSET_REG_CLR); + writel(STMP3XXX_RTC_PERSISTENT2_WDT_ACTIVE, + rtc_data->io + STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_CLR); } } +static void stmp3xxx_wdt_clear_bootstatus(struct device *dev) +{ + struct stmp3xxx_rtc_data *rtc_data = dev_get_drvdata(dev); + + writel(STMP3XXX_RTC_PERSISTENT2_WDT_ACTIVE, + rtc_data->io + STMP3XXX_RTC_PERSISTENT2 + STMP_OFFSET_REG_CLR); +} + static struct stmp3xxx_wdt_pdata wdt_pdata = { .wdt_set_timeout = stmp3xxx_wdt_set_timeout, + .wdt_clear_bootstatus = stmp3xxx_wdt_clear_bootstatus, + .bootstatus = 0, }; static void stmp3xxx_wdt_register(struct platform_device *rtc_pdev) @@ -110,6 +142,8 @@ static void stmp3xxx_wdt_register(struct platform_device *rtc_pdev) struct platform_device *wdt_pdev = platform_device_alloc("stmp3xxx_rtc_wdt", rtc_pdev->id); + stmp3xxx_wdt_clear_bootstatus(&rtc_pdev->dev); + if (wdt_pdev) { wdt_pdev->dev.parent = &rtc_pdev->dev; wdt_pdev->dev.platform_data = &wdt_pdata; @@ -357,6 +391,9 @@ static int stmp3xxx_rtc_probe(struct platform_device *pdev) return err; } + if (readl(STMP3XXX_RTC_PERSISTENT2 + rtc_data->io) & + STMP3XXX_RTC_PERSISTENT2_WDT_ACTIVE) + wdt_pdata.bootstatus |= WDIOF_CARDRESET; stmp3xxx_wdt_register(pdev); return 0; } diff --git a/drivers/watchdog/stmp3xxx_rtc_wdt.c b/drivers/watchdog/stmp3xxx_rtc_wdt.c index e09a01f..7609f78 100644 --- a/drivers/watchdog/stmp3xxx_rtc_wdt.c +++ b/drivers/watchdog/stmp3xxx_rtc_wdt.c @@ -52,7 +52,8 @@ static int wdt_set_timeout(struct watchdog_device *wdd, unsigned new_timeout) } static const struct watchdog_info stmp3xxx_wdt_ident = { - .options = WDIOF_MAGICCLOSE | WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING, + .options = WDIOF_MAGICCLOSE | WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | + WDIOF_CARDRESET, .identity = "STMP3XXX RTC Watchdog", }; @@ -79,6 +80,7 @@ static int wdt_notify_sys(struct notifier_block *nb, unsigned long code, switch (code) { case SYS_DOWN: /* keep enabled, system might crash while going down */ + pdata->wdt_clear_bootstatus(dev->parent); break; case SYS_HALT: /* allow the system to actually halt */ case SYS_POWER_OFF: @@ -95,12 +97,14 @@ static struct notifier_block wdt_notifier = { static int stmp3xxx_wdt_probe(struct platform_device *pdev) { + struct stmp3xxx_wdt_pdata *pdata = dev_get_platdata(&pdev->dev); int ret; watchdog_set_drvdata(&stmp3xxx_wdd, &pdev->dev); stmp3xxx_wdd.timeout = clamp_t(unsigned, heartbeat, 1, STMP3XXX_MAX_TIMEOUT); stmp3xxx_wdd.parent = &pdev->dev; + stmp3xxx_wdd.bootstatus = pdata->bootstatus; ret = watchdog_register_device(&stmp3xxx_wdd); if (ret < 0) { diff --git a/include/linux/stmp3xxx_rtc_wdt.h b/include/linux/stmp3xxx_rtc_wdt.h index 1dd12c9..62dd9e6 100644 --- a/include/linux/stmp3xxx_rtc_wdt.h +++ b/include/linux/stmp3xxx_rtc_wdt.h @@ -10,6 +10,8 @@ struct stmp3xxx_wdt_pdata { void (*wdt_set_timeout)(struct device *dev, u32 timeout); + void (*wdt_clear_bootstatus)(struct device *dev); + unsigned int bootstatus; }; #endif /* __LINUX_STMP3XXX_RTC_WDT_H */
This device doesn't provide any information about boot status. As workaround we use a persitent bit to track watchdog activity. Signed-off-by: Harald Geyer <harald@ccbib.org> --- Changes since v2: * make code ordering more consistent * move part of the commit message to a code comment * rewrite the commit message Changes since v1: * make code formatting more consistent with the rest of the driver * Cc some people who might have better documentation then I do Changes since initially posting this patch on 08/04/2015: * fix a spelling error in the commit message * rebase to a recent version drivers/rtc/rtc-stmp3xxx.c | 37 +++++++++++++++++++++++++++++++++++ drivers/watchdog/stmp3xxx_rtc_wdt.c | 6 +++++- include/linux/stmp3xxx_rtc_wdt.h | 2 ++ 3 files changed, 44 insertions(+), 1 deletion(-)