Hi Marek,

On 11.12.18 13:15, Marek Behún wrote:
Hi Stefan,

I forgot to answer you to this email.
Guenter pointed out that in the previous implementation of the watchdog
there was a tiny period of time when the watchdog was disabled (during
pinging - disable, set timeout, enable) which made the system
vulnerable.
I therefore changed the watchdog to use 2 counters:
Counter 1 is the watchdog counter - on expiry, the system is reset.
Counter 0 is used to reset Counter 1 to start counting from the set
timeout again. So Counter 1 is set to be reset on Counter 0 expiry
event and pinging is done by forcing immediate expiry event on Counter
0.

Thanks for explanation. Could you please re-submit and add this
description to the commit text, so that this change is more clear?

Thanks,
Stefan
Marek

On Thu, 29 Nov 2018 14:03:27 +0100
Stefan Roese <[email protected]> wrote:

On 20.11.18 13:04, Marek Behún wrote:
The Armada 37xx watchdog driver was recently accepted for mainline
kernel by watchdog subsystem maintainer, but the driver works a
little different than the one in U-Boot. This patch fixes this.

Out of curiosity: What is different and why does the "old"
implementation not work any more?
Signed-off-by: Marek Behún <[email protected]>
---
   drivers/watchdog/armada-37xx-wdt.c | 109
++++++++++++++++++----------- 1 file changed, 67 insertions(+), 42
deletions(-)

diff --git a/drivers/watchdog/armada-37xx-wdt.c
b/drivers/watchdog/armada-37xx-wdt.c index 0fa4fda4fc..91cd8a6e6a
100644 --- a/drivers/watchdog/armada-37xx-wdt.c
+++ b/drivers/watchdog/armada-37xx-wdt.c
@@ -22,42 +22,63 @@ struct a37xx_wdt {
   };
/*
- * We use Counter 1 for watchdog timer, because so does Marvell's
Linux by
- * default.
+ * We use Counter 1 as watchdog timer, and Counter 0 for
re-triggering Counter 1 */
-#define CNTR_CTRL 0x10
+#define CNTR_CTRL(id)                  ((id) * 0x10)
   #define CNTR_CTRL_ENABLE             0x0001
   #define CNTR_CTRL_ACTIVE             0x0002
   #define CNTR_CTRL_MODE_MASK          0x000c
   #define CNTR_CTRL_MODE_ONESHOT               0x0000
+#define CNTR_CTRL_MODE_HWSIG           0x000c
+#define CNTR_CTRL_TRIG_SRC_MASK                0x00f0
+#define CNTR_CTRL_TRIG_SRC_PREV_CNTR   0x0050
   #define CNTR_CTRL_PRESCALE_MASK              0xff00
   #define CNTR_CTRL_PRESCALE_MIN               2
   #define CNTR_CTRL_PRESCALE_SHIFT     8
-#define CNTR_COUNT_LOW 0x14
-#define CNTR_COUNT_HIGH                        0x18
+#define CNTR_COUNT_LOW(id)             (CNTR_CTRL(id) + 0x4)
+#define CNTR_COUNT_HIGH(id)            (CNTR_CTRL(id) + 0x8)
-static void set_counter_value(struct a37xx_wdt *priv)
+static void set_counter_value(struct a37xx_wdt *priv, int id, u64
val) {
-       writel(priv->timeout & 0xffffffff, priv->reg +
CNTR_COUNT_LOW);
-       writel(priv->timeout >> 32, priv->reg + CNTR_COUNT_HIGH);
+       writel(val & 0xffffffff, priv->reg + CNTR_COUNT_LOW(id));
+       writel(val >> 32, priv->reg + CNTR_COUNT_HIGH(id));
   }
-static void a37xx_wdt_enable(struct a37xx_wdt *priv)
+static void counter_enable(struct a37xx_wdt *priv, int id)
   {
-       u32 reg = readl(priv->reg + CNTR_CTRL);
+       setbits_le32(priv->reg + CNTR_CTRL(id), CNTR_CTRL_ENABLE);
+}
- reg |= CNTR_CTRL_ENABLE;
-       writel(reg, priv->reg + CNTR_CTRL);
+static void counter_disable(struct a37xx_wdt *priv, int id)
+{
+       clrbits_le32(priv->reg + CNTR_CTRL(id), CNTR_CTRL_ENABLE);
   }
-static void a37xx_wdt_disable(struct a37xx_wdt *priv)
+static int init_counter(struct a37xx_wdt *priv, int id, u32 mode,
u32 trig_src) {
-       u32 reg = readl(priv->reg + CNTR_CTRL);
+       u32 reg;
+
+       reg = readl(priv->reg + CNTR_CTRL(id));
+       if (reg & CNTR_CTRL_ACTIVE)
+               return -EBUSY;
+
+       reg &= ~(CNTR_CTRL_MODE_MASK | CNTR_CTRL_PRESCALE_MASK |
+                CNTR_CTRL_TRIG_SRC_MASK);
+
+       /* set mode */
+       reg |= mode;
+
+       /* set prescaler to the min value */
+       reg |= CNTR_CTRL_PRESCALE_MIN << CNTR_CTRL_PRESCALE_SHIFT;
+
+       /* set trigger source */
+       reg |= trig_src;
- reg &= ~CNTR_CTRL_ENABLE;
-       writel(reg, priv->reg + CNTR_CTRL);
+       writel(reg, priv->reg + CNTR_CTRL(id));
+
+       return 0;
   }
static int a37xx_wdt_reset(struct udevice *dev)
@@ -67,9 +88,9 @@ static int a37xx_wdt_reset(struct udevice *dev)
        if (!priv->timeout)
                return -EINVAL;
- a37xx_wdt_disable(priv);
-       set_counter_value(priv);
-       a37xx_wdt_enable(priv);
+       /* counter 1 is retriggered by forcing end count on
counter 0 */
+       counter_disable(priv, 0);
+       counter_enable(priv, 0);
return 0;
   }
@@ -78,10 +99,14 @@ static int a37xx_wdt_expire_now(struct udevice
*dev, ulong flags) {
        struct a37xx_wdt *priv = dev_get_priv(dev);
- a37xx_wdt_disable(priv);
-       priv->timeout = 0;
-       set_counter_value(priv);
-       a37xx_wdt_enable(priv);
+       /* first we set timeout to 0 */
+       counter_disable(priv, 1);
+       set_counter_value(priv, 1, 0);
+       counter_enable(priv, 1);
+
+       /* and then we start counter 1 by forcing end count on
counter 0 */
+       counter_disable(priv, 0);
+       counter_enable(priv, 0);
return 0;
   }
@@ -89,26 +114,25 @@ static int a37xx_wdt_expire_now(struct udevice
*dev, ulong flags) static int a37xx_wdt_start(struct udevice *dev,
u64 ms, ulong flags) {
        struct a37xx_wdt *priv = dev_get_priv(dev);
-       u32 reg;
-
-       reg = readl(priv->reg + CNTR_CTRL);
-
-       if (reg & CNTR_CTRL_ACTIVE)
-               return -EBUSY;
+       int err;
- /* set mode */
-       reg = (reg & ~CNTR_CTRL_MODE_MASK) |
CNTR_CTRL_MODE_ONESHOT;
+       err = init_counter(priv, 0, CNTR_CTRL_MODE_ONESHOT, 0);
+       if (err < 0)
+               return err;
- /* set prescaler to the min value */
-       reg &= ~CNTR_CTRL_PRESCALE_MASK;
-       reg |= CNTR_CTRL_PRESCALE_MIN << CNTR_CTRL_PRESCALE_SHIFT;
+       err = init_counter(priv, 1, CNTR_CTRL_MODE_HWSIG,
+                          CNTR_CTRL_TRIG_SRC_PREV_CNTR);
+       if (err < 0)
+               return err;
priv->timeout = ms * priv->clk_rate / 1000 /
CNTR_CTRL_PRESCALE_MIN;
-       writel(reg, priv->reg + CNTR_CTRL);
+       set_counter_value(priv, 0, 0);
+       set_counter_value(priv, 1, priv->timeout);
+       counter_enable(priv, 1);
- set_counter_value(priv);
-       a37xx_wdt_enable(priv);
+       /* we have to force end count on counter 0 to start
counter 1 */
+       counter_enable(priv, 0);
return 0;
   }
@@ -117,7 +141,9 @@ static int a37xx_wdt_stop(struct udevice *dev)
   {
        struct a37xx_wdt *priv = dev_get_priv(dev);
- a37xx_wdt_disable(priv);
+       counter_disable(priv, 1);
+       counter_disable(priv, 0);
+       writel(0, priv->sel_reg);
return 0;
   }
@@ -139,11 +165,10 @@ static int a37xx_wdt_probe(struct udevice
*dev)
        priv->clk_rate = (ulong)get_ref_clk() * 1000000;
- a37xx_wdt_disable(priv);
-
        /*
-        * We use timer 1 as watchdog timer (because Marvell's
Linux uses that
-        * timer as default), therefore we only set bit
TIMER1_IS_WCHDOG_TIMER.
+        * We use counter 1 as watchdog timer, therefore we only
set bit
+        * TIMER1_IS_WCHDOG_TIMER. Counter 0 is only used to force
re-trigger on
+        * counter 1.
         */
        writel(1 << 1, priv->sel_reg);

Viele Grüße,
Stefan



Viele Grüße,
Stefan

--
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: [email protected]
_______________________________________________
U-Boot mailing list
[email protected]
https://lists.denx.de/listinfo/u-boot

Reply via email to