Re: [PATCHv7 8/8] watchdog: omap_wdt: Convert to use new core extensions

[Timo Kokkonen <timo.kokkonen@offcode.fi>]

On 07.05.2015 10:30, Uwe Kleine-König wrote:
> Hello Timo,
>
> On Thu, May 07, 2015 at 09:42:44AM +0300, Timo Kokkonen wrote:
>> On 04.05.2015 10:04, Uwe Kleine-König wrote:
>>> 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
> s/reads/writes/ in the above line.
>
>>> while the counter is running. Maybe even this could be used to detect a
>>> running timer?:
>>
>> Maybe, but you will get a data abort when the HW doesn't accept a
>> read. How do you recover from that?
>>
>> Also, it seems that for some reason the watchdog HW is very picky
>> about which registers can be read in what condition. If I add the
>> code to read watchdog counter register, I will get a data abort
>> later at the end of the probe when the watchdog revision is read. I
>> don't understand why that happens, this does not seem logica. Maybe
>> it has got something to do with the fact that the watchdog is
>> stopped? If I can't read the counter register, I can't really
>> implement the above heuristic.. Any ideas?
> Which SoC do you use for your tests? Sounds strange. If you provide me
> your wip patch I can take a look.
>

I tried it on BeagleBone which has am35xx

I tried both approaches and neither were working:

diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
index f86775e..b929c83 100644
--- a/drivers/watchdog/omap_wdt.c
+++ b/drivers/watchdog/omap_wdt.c
@@ -41,6 +41,7 @@
  #include <linux/slab.h>
  #include <linux/pm_runtime.h>
  #include <linux/platform_data/omap-wd-timer.h>
+#include <linux/delay.h>

  #include "omap_wdt.h"

@@ -81,6 +82,31 @@ static void omap_wdt_reload(struct omap_wdt_dev *wdev)
  static int omap_wdt_is_running(struct omap_wdt_dev *wdev)
  {
  	void __iomem *base = wdev->base;
+	unsigned long cntrl, counter1, counter2, shift;
+       /*
+	* There is no register that tells us if the timer is running,
+	* so we have to resort to sample twice. Use shortest delay
+	* depending on the actual prescaling value.
+	*
+	* Note! If bootloader configured a very large prescaler
+	* value, we might delay up to 4ms here. If that happens, you
+	* are better to fix your bootloader anyway!
+	*/
+
+	/*
+	cntrl = readl_relaxed(base + OMAP_WATCHDOG_CNTRL);
+	if (cntrl & (1 << 5))
+		shift = (cntrl >> 2) & 0x7;
+	else
+		shift = 0;
+	*/
+
+	counter1 = readl_relaxed(base + OMAP_WATCHDOG_CRR);
+	/*udelay(31 << shift);*/
+	msleep(4);
+	counter2 = readl_relaxed(base + OMAP_WATCHDOG_CRR);
+
+	return counter1 != counter2;

  	return readl_relaxed(base + OMAP_WATCHDOG_SPR) == 0x4444;
  }



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