From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Timo Kokkonen <timo.kokkonen@offcode.fi>
Cc: linux-arm-kernel@lists.infradead.org,
linux-watchdog@vger.kernel.org,
boris.brezillon@free-electrons.com, nicolas.ferre@atmel.com,
alexandre.belloni@free-electrons.com, Wenyou.Yang@atmel.com
Subject: Re: [PATCHv7 8/8] watchdog: omap_wdt: Convert to use new core extensions
Date: Mon, 4 May 2015 09:04:52 +0200 [thread overview]
Message-ID: <20150504070452.GE25193@pengutronix.de> (raw)
In-Reply-To: <55470AA7.9050605@offcode.fi>
Hello Timo,
On Mon, May 04, 2015 at 08:59:03AM +0300, Timo Kokkonen wrote:
> Hi, 03.05.2015 21:56, Uwe Kleine-König wrote:
> >On Wed, Apr 22, 2015 at 02:11:42PM +0300, Timo Kokkonen wrote:
> >>+static int omap_wdt_is_running(struct omap_wdt_dev *wdev)
> >>+{
> >>+ void __iomem *base = wdev->base;
> >>+
> >>+ return readl_relaxed(base + OMAP_WATCHDOG_SPR) == 0x4444;
> >>+}
> >This isn't reliable. The sequence needed to enable the watchdog is
> > writel(0xbbbb, base + OMAP_WATCHDOG_SPR);
> > writel(0x4444, base + OMAP_WATCHDOG_SPR);
> >
> >The sequence to stop is:
> > writel(0xaaaa, base + OMAP_WATCHDOG_SPR);
> > writel(0x5555, base + OMAP_WATCHDOG_SPR);
> >
> >But:
> >
> >barebox@TI AM335x BeagleBone black:/ md 0x44e35048+4
> >44e35048: 00005555 UU..
> >barebox@TI AM335x BeagleBone black:/ mw 0x44e35048 0x4444
> >barebox@TI AM335x BeagleBone black:/ md 0x44e35048+4
> >44e35048: 00004444 DD..
> >
> >So the register contains 0x4444 but the timer doesn't run. So at best
> >testing for 0x4444 is a good heuristic.
>
> Yeah.. I don't think we can get any better than that. Unless we
> start checking the counter register and see whether it really counts
> or not, and I think that's a bit overkill.. So I'd say we should be
> safe when assuming bootloader is doing things correctly. Although,
> we could add a comment to the code that the test may not be 100%
> reliable in case the start sequence have not been issued properly.
>
> Thanks for pointing this out!
It doesn't seem to much overhead to do:
/*
* There is no register that tells us if the timer is running,
* so we have to resort to sample twice. The minimal frequency
* is 256 Hz (32768 Hz prescaled with 2**7).
*/
counter1 = readl(base + OMAP_WATCHDOG_CCR);
mdelay(4);
counter2 = readl(base + OMAP_WATCHDOG_CCR);
return counter1 != counter2;
I'd say it's even worth to do:
cntrl = readl(base + OMAP_WATCHDOG_CNTRL);
if (cntrl & (1 << 5))
shift = (cntrl >> 2) & 0x7;
else
shift = 0;
counter1 = readl(base + OMAP_WATCHDOG_CCR);
udelay(31 << shift);
counter2 = readl(base + OMAP_WATCHDOG_CCR);
return counter1 != counter2;
For some bonus points add some defines for the magic constants.
This is save as the OMAP_WATCHDOG_CNTRL doesn't seem to accept reads
while the counter is running. Maybe even this could be used to detect a
running timer?:
- enable timer:
barebox@TI AM335x BeagleBone black:/ mw 0x44e35048 0xbbbb
barebox@TI AM335x BeagleBone black:/ mw 0x44e35048 0x4444
- read out WCLR
barebox@TI AM335x BeagleBone black:/ md 0x44e35024+4
44e35024: 00000020 ...
- write to WCLR
barebox@TI AM335x BeagleBone black:/ mw 0x44e35024 0x0
- check result; didn't work
barebox@TI AM335x BeagleBone black:/ md 0x44e35024+4
44e35024: 00000020 ...
- stop timer
barebox@TI AM335x BeagleBone black:/ mw 0x44e35048 0xaaaa
barebox@TI AM335x BeagleBone black:/ mw 0x44e35048 0x5555
- recheck WCLR
barebox@TI AM335x BeagleBone black:/ md 0x44e35024+4
44e35024: 00000020 ...
- write to WCLR
barebox@TI AM335x BeagleBone black:/ mw 0x44e35024 0x0
- check result; write succeeded
barebox@TI AM335x BeagleBone black:/ md 0x44e35024+4
44e35024: 00000000 ....
(This is was tested on an AM335x.)
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@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
WARNING: multiple messages have this Message-ID (diff)
From: u.kleine-koenig@pengutronix.de (Uwe Kleine-König)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCHv7 8/8] watchdog: omap_wdt: Convert to use new core extensions
Date: Mon, 4 May 2015 09:04:52 +0200 [thread overview]
Message-ID: <20150504070452.GE25193@pengutronix.de> (raw)
In-Reply-To: <55470AA7.9050605@offcode.fi>
Hello Timo,
On Mon, May 04, 2015 at 08:59:03AM +0300, Timo Kokkonen wrote:
> Hi, 03.05.2015 21:56, Uwe Kleine-K?nig wrote:
> >On Wed, Apr 22, 2015 at 02:11:42PM +0300, Timo Kokkonen wrote:
> >>+static int omap_wdt_is_running(struct omap_wdt_dev *wdev)
> >>+{
> >>+ void __iomem *base = wdev->base;
> >>+
> >>+ return readl_relaxed(base + OMAP_WATCHDOG_SPR) == 0x4444;
> >>+}
> >This isn't reliable. The sequence needed to enable the watchdog is
> > writel(0xbbbb, base + OMAP_WATCHDOG_SPR);
> > writel(0x4444, base + OMAP_WATCHDOG_SPR);
> >
> >The sequence to stop is:
> > writel(0xaaaa, base + OMAP_WATCHDOG_SPR);
> > writel(0x5555, base + OMAP_WATCHDOG_SPR);
> >
> >But:
> >
> >barebox at TI AM335x BeagleBone black:/ md 0x44e35048+4
> >44e35048: 00005555 UU..
> >barebox at TI AM335x BeagleBone black:/ mw 0x44e35048 0x4444
> >barebox at TI AM335x BeagleBone black:/ md 0x44e35048+4
> >44e35048: 00004444 DD..
> >
> >So the register contains 0x4444 but the timer doesn't run. So at best
> >testing for 0x4444 is a good heuristic.
>
> Yeah.. I don't think we can get any better than that. Unless we
> start checking the counter register and see whether it really counts
> or not, and I think that's a bit overkill.. So I'd say we should be
> safe when assuming bootloader is doing things correctly. Although,
> we could add a comment to the code that the test may not be 100%
> reliable in case the start sequence have not been issued properly.
>
> Thanks for pointing this out!
It doesn't seem to much overhead to do:
/*
* There is no register that tells us if the timer is running,
* so we have to resort to sample twice. The minimal frequency
* is 256 Hz (32768 Hz prescaled with 2**7).
*/
counter1 = readl(base + OMAP_WATCHDOG_CCR);
mdelay(4);
counter2 = readl(base + OMAP_WATCHDOG_CCR);
return counter1 != counter2;
I'd say it's even worth to do:
cntrl = readl(base + OMAP_WATCHDOG_CNTRL);
if (cntrl & (1 << 5))
shift = (cntrl >> 2) & 0x7;
else
shift = 0;
counter1 = readl(base + OMAP_WATCHDOG_CCR);
udelay(31 << shift);
counter2 = readl(base + OMAP_WATCHDOG_CCR);
return counter1 != counter2;
For some bonus points add some defines for the magic constants.
This is save as the OMAP_WATCHDOG_CNTRL doesn't seem to accept reads
while the counter is running. Maybe even this could be used to detect a
running timer?:
- enable timer:
barebox at TI AM335x BeagleBone black:/ mw 0x44e35048 0xbbbb
barebox at TI AM335x BeagleBone black:/ mw 0x44e35048 0x4444
- read out WCLR
barebox at TI AM335x BeagleBone black:/ md 0x44e35024+4
44e35024: 00000020 ...
- write to WCLR
barebox at TI AM335x BeagleBone black:/ mw 0x44e35024 0x0
- check result; didn't work
barebox at TI AM335x BeagleBone black:/ md 0x44e35024+4
44e35024: 00000020 ...
- stop timer
barebox at TI AM335x BeagleBone black:/ mw 0x44e35048 0xaaaa
barebox at TI AM335x BeagleBone black:/ mw 0x44e35048 0x5555
- recheck WCLR
barebox at TI AM335x BeagleBone black:/ md 0x44e35024+4
44e35024: 00000020 ...
- write to WCLR
barebox at TI AM335x BeagleBone black:/ mw 0x44e35024 0x0
- check result; write succeeded
barebox@TI AM335x BeagleBone black:/ md 0x44e35024+4
44e35024: 00000000 ....
(This is was tested on an AM335x.)
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
next prev parent reply other threads:[~2015-05-04 7:04 UTC|newest]
Thread overview: 66+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-04-22 11:11 [PATCHv7 0/8] watchdog: Extend kernel API and add early_timeout_sec feature Timo Kokkonen
2015-04-22 11:11 ` Timo Kokkonen
2015-04-22 11:11 ` [PATCHv7 1/8] watchdog: Extend kernel API to know about HW limitations Timo Kokkonen
2015-04-22 11:11 ` Timo Kokkonen
2015-04-24 17:08 ` Guenter Roeck
2015-04-24 17:08 ` Guenter Roeck
2015-04-27 5:41 ` Timo Kokkonen
2015-04-27 5:41 ` Timo Kokkonen
2015-05-04 7:58 ` Uwe Kleine-König
2015-05-04 7:58 ` Uwe Kleine-König
2015-05-04 9:40 ` Timo Kokkonen
2015-05-04 9:40 ` Timo Kokkonen
2015-05-04 15:43 ` Guenter Roeck
2015-05-04 15:43 ` Guenter Roeck
2015-05-05 6:26 ` Timo Kokkonen
2015-05-05 6:26 ` Timo Kokkonen
2015-05-04 21:17 ` Marc Kleine-Budde
2015-05-04 21:17 ` Marc Kleine-Budde
2015-04-22 11:11 ` [PATCHv7 2/8] watchdog: Allow watchdog to reset device at early boot Timo Kokkonen
2015-04-22 11:11 ` Timo Kokkonen
2015-04-22 11:11 ` [PATCHv7 3/8] devicetree: Document generic watchdog properties Timo Kokkonen
2015-04-22 11:11 ` Timo Kokkonen
2015-04-22 11:11 ` [PATCHv7 4/8] Documentation/watchdog: watchdog-test.c: Add support for changing timeout Timo Kokkonen
2015-04-22 11:11 ` Timo Kokkonen
2015-04-22 11:11 ` [PATCHv7 5/8] watchdog: at91sam9_wdt: Convert to use new watchdog core extensions Timo Kokkonen
2015-04-22 11:11 ` Timo Kokkonen
2015-04-22 11:11 ` [PATCHv7 6/8] watchdog: imx2_wdt: Convert to use new " Timo Kokkonen
2015-04-22 11:11 ` Timo Kokkonen
2015-05-05 8:11 ` Marc Kleine-Budde
2015-05-05 8:11 ` Marc Kleine-Budde
2015-05-05 8:31 ` Marc Kleine-Budde
2015-05-05 8:31 ` Marc Kleine-Budde
2015-05-05 9:07 ` Timo Kokkonen
2015-05-05 9:07 ` Timo Kokkonen
2015-04-22 11:11 ` [PATCHv7 7/8] watchdog: omap_wdt: Fix memory leak on probe fail Timo Kokkonen
2015-04-22 11:11 ` Timo Kokkonen
2015-04-26 15:32 ` Guenter Roeck
2015-04-26 15:32 ` Guenter Roeck
2015-04-27 5:50 ` Timo Kokkonen
2015-04-27 5:50 ` Timo Kokkonen
2015-04-22 11:11 ` [PATCHv7 8/8] watchdog: omap_wdt: Convert to use new core extensions Timo Kokkonen
2015-04-22 11:11 ` Timo Kokkonen
2015-05-03 18:56 ` Uwe Kleine-König
2015-05-03 18:56 ` Uwe Kleine-König
2015-05-04 5:59 ` Timo Kokkonen
2015-05-04 5:59 ` Timo Kokkonen
2015-05-04 7:04 ` Uwe Kleine-König [this message]
2015-05-04 7:04 ` Uwe Kleine-König
2015-05-04 10:06 ` Timo Kokkonen
2015-05-04 10:06 ` Timo Kokkonen
2015-05-07 6:42 ` Timo Kokkonen
2015-05-07 6:42 ` Timo Kokkonen
2015-05-07 7:30 ` Uwe Kleine-König
2015-05-07 7:30 ` Uwe Kleine-König
2015-05-07 7:39 ` Timo Kokkonen
2015-05-07 7:39 ` Timo Kokkonen
2015-05-04 16:08 ` Guenter Roeck
2015-05-04 16:08 ` Guenter Roeck
2015-05-05 13:50 ` [PATCHv7 0/8] watchdog: Extend kernel API and add early_timeout_sec feature Uwe Kleine-König
2015-05-05 13:50 ` Uwe Kleine-König
2015-05-06 7:26 ` Timo Kokkonen
2015-05-06 7:26 ` Timo Kokkonen
2015-05-06 7:48 ` Uwe Kleine-König
2015-05-06 7:48 ` Uwe Kleine-König
2015-05-06 8:23 ` Timo Kokkonen
2015-05-06 8:23 ` Timo Kokkonen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150504070452.GE25193@pengutronix.de \
--to=u.kleine-koenig@pengutronix.de \
--cc=Wenyou.Yang@atmel.com \
--cc=alexandre.belloni@free-electrons.com \
--cc=boris.brezillon@free-electrons.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-watchdog@vger.kernel.org \
--cc=nicolas.ferre@atmel.com \
--cc=timo.kokkonen@offcode.fi \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.