From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefan Roese Date: Tue, 11 Dec 2018 15:31:00 +0100 Subject: [U-Boot] [PATCH u-boot-marvell v3 06/10] watchdog: armada_37xx: Fix compliance with kernel's driver In-Reply-To: <20181211131516.1b5942e8@dellmb.labs.office.nic.cz> References: <20181120120409.12822-1-marek.behun@nic.cz> <20181120120409.12822-6-marek.behun@nic.cz> <92820b97-bbfb-1cdc-0d82-4fd52a59f10e@denx.de> <20181211131516.1b5942e8@dellmb.labs.office.nic.cz> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de 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 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 >>> --- >>> 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: sr at denx.de