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@xxxxxxxxx>
> ---
> 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@xxxxxxxxx>) 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
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
