[U-Boot] [PATCH u-boot-marvell v3 06/10] watchdog: armada_37xx: Fix compliance with kernel's driver

[Stefan Roese <sr@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 <sr@denx.de> 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 <marek.behun@nic.cz>
>>> ---
>>>    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