[PATCH v2 1/3] watchdog: sama5d4: Cleanup init

[PATCH v2 1/3] watchdog: sama5d4: Cleanup init

[Alexandre Belloni]

.config is used to cache a part of WDT_MR at probe time and is not used
afterwards. Also functions are used while they always return 0. Simplify
the flow by having everything in .probe().

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
Changes in v2:
 - completely get rid of .config instead of caching it
 - merge init functions in probe()

 drivers/watchdog/sama5d4_wdt.c | 92 ++++++++++++++++--------------------------
 1 file changed, 34 insertions(+), 58 deletions(-)

diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
index a49634cdc1cc..2c6f5a70ae67 100644
--- a/drivers/watchdog/sama5d4_wdt.c
+++ b/drivers/watchdog/sama5d4_wdt.c
@@ -28,7 +28,6 @@
 struct sama5d4_wdt {
 	struct watchdog_device	wdd;
 	void __iomem		*reg_base;
-	u32	config;
 };
 
 static int wdt_timeout = WDT_DEFAULT_TIMEOUT;
@@ -128,57 +127,15 @@ static irqreturn_t sama5d4_wdt_irq_handler(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static int of_sama5d4_wdt_init(struct device_node *np, struct sama5d4_wdt *wdt)
-{
-	const char *tmp;
-
-	wdt->config = AT91_WDT_WDDIS;
-
-	if (!of_property_read_string(np, "atmel,watchdog-type", &tmp) &&
-	    !strcmp(tmp, "software"))
-		wdt->config |= AT91_WDT_WDFIEN;
-	else
-		wdt->config |= AT91_WDT_WDRSTEN;
-
-	if (of_property_read_bool(np, "atmel,idle-halt"))
-		wdt->config |= AT91_WDT_WDIDLEHLT;
-
-	if (of_property_read_bool(np, "atmel,dbg-halt"))
-		wdt->config |= AT91_WDT_WDDBGHLT;
-
-	return 0;
-}
-
-static int sama5d4_wdt_init(struct sama5d4_wdt *wdt)
-{
-	struct watchdog_device *wdd = &wdt->wdd;
-	u32 value = WDT_SEC2TICKS(wdd->timeout);
-	u32 reg;
-
-	/*
-	 * Because the fields WDV and WDD must not be modified when the WDDIS
-	 * bit is set, so clear the WDDIS bit before writing the WDT_MR.
-	 */
-	reg = wdt_read(wdt, AT91_WDT_MR);
-	reg &= ~AT91_WDT_WDDIS;
-	wdt_write(wdt, AT91_WDT_MR, reg);
-
-	reg = wdt->config;
-	reg |= AT91_WDT_SET_WDD(value);
-	reg |= AT91_WDT_SET_WDV(value);
-
-	wdt_write(wdt, AT91_WDT_MR, reg);
-
-	return 0;
-}
-
 static int sama5d4_wdt_probe(struct platform_device *pdev)
 {
 	struct watchdog_device *wdd;
 	struct sama5d4_wdt *wdt;
 	struct resource *res;
 	void __iomem *regs;
-	u32 irq = 0;
+	struct device_node *np = pdev->dev.of_node;
+	const char *tmp;
+	u32 mr, reg, irq = 0;
 	int ret;
 
 	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
@@ -201,17 +158,26 @@ static int sama5d4_wdt_probe(struct platform_device *pdev)
 
 	wdt->reg_base = regs;
 
-	if (pdev->dev.of_node) {
-		irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
-		if (!irq)
-			dev_warn(&pdev->dev, "failed to get IRQ from DT\n");
+	irq = irq_of_parse_and_map(np, 0);
+	if (!irq)
+		dev_warn(&pdev->dev, "failed to get IRQ from DT\n");
 
-		ret = of_sama5d4_wdt_init(pdev->dev.of_node, wdt);
-		if (ret)
-			return ret;
-	}
 
-	if ((wdt->config & AT91_WDT_WDFIEN) && irq) {
+	mr = AT91_WDT_WDDIS;
+
+	if (!of_property_read_string(np, "atmel,watchdog-type", &tmp) &&
+	    !strcmp(tmp, "software"))
+		mr |= AT91_WDT_WDFIEN;
+	else
+		mr |= AT91_WDT_WDRSTEN;
+
+	if (of_property_read_bool(np, "atmel,idle-halt"))
+		mr |= AT91_WDT_WDIDLEHLT;
+
+	if (of_property_read_bool(np, "atmel,dbg-halt"))
+		mr |= AT91_WDT_WDDBGHLT;
+
+	if ((mr & AT91_WDT_WDFIEN) && irq) {
 		ret = devm_request_irq(&pdev->dev, irq, sama5d4_wdt_irq_handler,
 				       IRQF_SHARED | IRQF_IRQPOLL |
 				       IRQF_NO_SUSPEND, pdev->name, pdev);
@@ -228,9 +194,19 @@ static int sama5d4_wdt_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	ret = sama5d4_wdt_init(wdt);
-	if (ret)
-		return ret;
+	/*
+	 * WDV and WDD must not be modified when the WDDIS bit is set, so clear
+	 * the WDDIS bit before writing the WDT_MR.
+	 */
+	reg = wdt_read(wdt, AT91_WDT_MR);
+	reg &= ~AT91_WDT_WDDIS;
+	wdt_write(wdt, AT91_WDT_MR, reg);
+
+	reg = mr;
+	reg |= AT91_WDT_SET_WDD(WDT_SEC2TICKS(wdd->timeout));
+	reg |= AT91_WDT_SET_WDV(WDT_SEC2TICKS(wdd->timeout));
+
+	wdt_write(wdt, AT91_WDT_MR, reg);
 
 	watchdog_set_nowayout(wdd, nowayout);
 
-- 
2.11.0

[PATCH v2 2/3] watchdog: sama5d4: Fix setting timeout when watchdog is disabled

[Alexandre Belloni]

The datasheet states: "When setting the WDDIS bit, and while it is set, the
fields WDV and WDD must not be modified."

Ensure WDDIS is not set when changing the timeout and set it afterwards if
the watchdog was disabled.

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
Changes in v2:
 - new patch
 drivers/watchdog/sama5d4_wdt.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
index 2c6f5a70ae67..2a60251806d2 100644
--- a/drivers/watchdog/sama5d4_wdt.c
+++ b/drivers/watchdog/sama5d4_wdt.c
@@ -90,11 +90,18 @@ static int sama5d4_wdt_set_timeout(struct watchdog_device *wdd,
 	u32 reg;
 
 	reg = wdt_read(wdt, AT91_WDT_MR);
+
+	if (reg & AT91_WDT_WDDIS)
+		wdt_write(wdt, AT91_WDT_MR, reg & ~AT91_WDT_WDDIS);
+
 	reg &= ~AT91_WDT_WDV;
 	reg &= ~AT91_WDT_WDD;
 	reg |= AT91_WDT_SET_WDV(value);
 	reg |= AT91_WDT_SET_WDD(value);
-	wdt_write(wdt, AT91_WDT_MR, reg);
+	wdt_write(wdt, AT91_WDT_MR, reg & ~AT91_WDT_WDDIS);
+
+	if (reg & AT91_WDT_WDDIS)
+		wdt_write(wdt, AT91_WDT_MR, reg);
 
 	wdd->timeout = timeout;
 
-- 
2.11.0

[PATCH v2 3/3] watchdog: sama5d4: Implement resume hook

[Alexandre Belloni]

When resuming for the deepest state on sama5d2, it is necessary to restore
MR as the registers are lost.

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
Changes in v2:
 - cache mr beofre suspending

 drivers/watchdog/sama5d4_wdt.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
index 2a60251806d2..5ddeb4803dc3 100644
--- a/drivers/watchdog/sama5d4_wdt.c
+++ b/drivers/watchdog/sama5d4_wdt.c
@@ -28,6 +28,7 @@
 struct sama5d4_wdt {
 	struct watchdog_device	wdd;
 	void __iomem		*reg_base;
+	u32			mr;
 };
 
 static int wdt_timeout = WDT_DEFAULT_TIMEOUT;
@@ -248,11 +249,42 @@ static const struct of_device_id sama5d4_wdt_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, sama5d4_wdt_of_match);
 
+#ifdef CONFIG_PM_SLEEP
+static int sama5d4_wdt_suspend(struct device *dev)
+{
+	struct sama5d4_wdt *wdt = dev_get_drvdata(dev);
+
+	wdt->mr = wdt_read(wdt, AT91_WDT_MR);
+
+	return 0;
+}
+
+static int sama5d4_wdt_resume(struct device *dev)
+{
+	struct sama5d4_wdt *wdt = dev_get_drvdata(dev);
+	u32 reg;
+
+	reg = wdt_read(wdt, AT91_WDT_MR);
+	if (reg & AT91_WDT_WDDIS)
+		wdt_write(wdt, AT91_WDT_MR, reg & ~AT91_WDT_WDDIS);
+
+	wdt_write(wdt, AT91_WDT_MR, wdt->mr & ~AT91_WDT_WDDIS);
+	if (wdt->mr & AT91_WDT_WDDIS)
+		wdt_write(wdt, AT91_WDT_MR, wdt->mr);
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(sama5d4_wdt_pm_ops, sama5d4_wdt_suspend,
+			 sama5d4_wdt_resume);
+
 static struct platform_driver sama5d4_wdt_driver = {
 	.probe		= sama5d4_wdt_probe,
 	.remove		= sama5d4_wdt_remove,
 	.driver		= {
 		.name	= "sama5d4_wdt",
+		.pm	= &sama5d4_wdt_pm_ops,
 		.of_match_table = sama5d4_wdt_of_match,
 	}
 };
-- 
2.11.0

Re: [PATCH v2 2/3] watchdog: sama5d4: Fix setting timeout when watchdog is disabled

[Guenter Roeck]

On 02/16/2017 11:30 AM, Alexandre Belloni wrote:
> The datasheet states: "When setting the WDDIS bit, and while it is set, the
> fields WDV and WDD must not be modified."
>
> Ensure WDDIS is not set when changing the timeout and set it afterwards if
> the watchdog was disabled.
>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> ---
> Changes in v2:
>  - new patch
>  drivers/watchdog/sama5d4_wdt.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> index 2c6f5a70ae67..2a60251806d2 100644
> --- a/drivers/watchdog/sama5d4_wdt.c
> +++ b/drivers/watchdog/sama5d4_wdt.c
> @@ -90,11 +90,18 @@ static int sama5d4_wdt_set_timeout(struct watchdog_device *wdd,
>  	u32 reg;
>
>  	reg = wdt_read(wdt, AT91_WDT_MR);
> +
> +	if (reg & AT91_WDT_WDDIS)
> +		wdt_write(wdt, AT91_WDT_MR, reg & ~AT91_WDT_WDDIS);
> +
>  	reg &= ~AT91_WDT_WDV;
>  	reg &= ~AT91_WDT_WDD;
>  	reg |= AT91_WDT_SET_WDV(value);
>  	reg |= AT91_WDT_SET_WDD(value);
> -	wdt_write(wdt, AT91_WDT_MR, reg);
> +	wdt_write(wdt, AT91_WDT_MR, reg & ~AT91_WDT_WDDIS);
> +
> +	if (reg & AT91_WDT_WDDIS)
> +		wdt_write(wdt, AT91_WDT_MR, reg);
>
That means if the watchdog is running, the timeout would not be updated.
It should be updated no matter if it is running or not.

Guenter

>  	wdd->timeout = timeout;
>
>

Re: [PATCH v2 3/3] watchdog: sama5d4: Implement resume hook

[Guenter Roeck]

On 02/16/2017 11:30 AM, Alexandre Belloni wrote:
> When resuming for the deepest state on sama5d2, it is necessary to restore
> MR as the registers are lost.
>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> ---
> Changes in v2:
>  - cache mr beofre suspending
>
>  drivers/watchdog/sama5d4_wdt.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
>
> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> index 2a60251806d2..5ddeb4803dc3 100644
> --- a/drivers/watchdog/sama5d4_wdt.c
> +++ b/drivers/watchdog/sama5d4_wdt.c
> @@ -28,6 +28,7 @@
>  struct sama5d4_wdt {
>  	struct watchdog_device	wdd;
>  	void __iomem		*reg_base;
> +	u32			mr;

Makes me wonder if we shouldn't just retain the original 'config'
(and maybe rename it to 'mr). After all, it _is_ used now.

>  };
>
>  static int wdt_timeout = WDT_DEFAULT_TIMEOUT;
> @@ -248,11 +249,42 @@ static const struct of_device_id sama5d4_wdt_of_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, sama5d4_wdt_of_match);
>
> +#ifdef CONFIG_PM_SLEEP
> +static int sama5d4_wdt_suspend(struct device *dev)
> +{
> +	struct sama5d4_wdt *wdt = dev_get_drvdata(dev);
> +
> +	wdt->mr = wdt_read(wdt, AT91_WDT_MR);
> +

Wouldn't you want to stop the watchdog here if it is running,
ie set AT91_WDT_WDDIS ?

> +	return 0;
> +}
> +
> +static int sama5d4_wdt_resume(struct device *dev)
> +{
> +	struct sama5d4_wdt *wdt = dev_get_drvdata(dev);
> +	u32 reg;
> +
> +	reg = wdt_read(wdt, AT91_WDT_MR);
> +	if (reg & AT91_WDT_WDDIS)
> +		wdt_write(wdt, AT91_WDT_MR, reg & ~AT91_WDT_WDDIS);
> +
> +	wdt_write(wdt, AT91_WDT_MR, wdt->mr & ~AT91_WDT_WDDIS);

Is that necessary ? Why not just write wdt->mr unconditionally ?

> +	if (wdt->mr & AT91_WDT_WDDIS)
> +		wdt_write(wdt, AT91_WDT_MR, wdt->mr);
> +
> +	return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(sama5d4_wdt_pm_ops, sama5d4_wdt_suspend,
> +			 sama5d4_wdt_resume);
> +
>  static struct platform_driver sama5d4_wdt_driver = {
>  	.probe		= sama5d4_wdt_probe,
>  	.remove		= sama5d4_wdt_remove,
>  	.driver		= {
>  		.name	= "sama5d4_wdt",
> +		.pm	= &sama5d4_wdt_pm_ops,
>  		.of_match_table = sama5d4_wdt_of_match,
>  	}
>  };
>

Re: [PATCH v2 1/3] watchdog: sama5d4: Cleanup init

[Guenter Roeck]

On 02/16/2017 11:30 AM, Alexandre Belloni wrote:
> .config is used to cache a part of WDT_MR at probe time and is not used
> afterwards. Also functions are used while they always return 0. Simplify
> the flow by having everything in .probe().
>

Does that really improve anything ? It makes the code harder to read
and, ultimately, the dropped value in sama5d4_wdt is added back in a
later patch as 'mr'.

Guenter

> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> ---
> Changes in v2:
>  - completely get rid of .config instead of caching it
>  - merge init functions in probe()
>
>  drivers/watchdog/sama5d4_wdt.c | 92 ++++++++++++++++--------------------------
>  1 file changed, 34 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> index a49634cdc1cc..2c6f5a70ae67 100644
> --- a/drivers/watchdog/sama5d4_wdt.c
> +++ b/drivers/watchdog/sama5d4_wdt.c
> @@ -28,7 +28,6 @@
>  struct sama5d4_wdt {
>  	struct watchdog_device	wdd;
>  	void __iomem		*reg_base;
> -	u32	config;
>  };
>
>  static int wdt_timeout = WDT_DEFAULT_TIMEOUT;
> @@ -128,57 +127,15 @@ static irqreturn_t sama5d4_wdt_irq_handler(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>
> -static int of_sama5d4_wdt_init(struct device_node *np, struct sama5d4_wdt *wdt)
> -{
> -	const char *tmp;
> -
> -	wdt->config = AT91_WDT_WDDIS;
> -
> -	if (!of_property_read_string(np, "atmel,watchdog-type", &tmp) &&
> -	    !strcmp(tmp, "software"))
> -		wdt->config |= AT91_WDT_WDFIEN;
> -	else
> -		wdt->config |= AT91_WDT_WDRSTEN;
> -
> -	if (of_property_read_bool(np, "atmel,idle-halt"))
> -		wdt->config |= AT91_WDT_WDIDLEHLT;
> -
> -	if (of_property_read_bool(np, "atmel,dbg-halt"))
> -		wdt->config |= AT91_WDT_WDDBGHLT;
> -
> -	return 0;
> -}
> -
> -static int sama5d4_wdt_init(struct sama5d4_wdt *wdt)
> -{
> -	struct watchdog_device *wdd = &wdt->wdd;
> -	u32 value = WDT_SEC2TICKS(wdd->timeout);
> -	u32 reg;
> -
> -	/*
> -	 * Because the fields WDV and WDD must not be modified when the WDDIS
> -	 * bit is set, so clear the WDDIS bit before writing the WDT_MR.
> -	 */
> -	reg = wdt_read(wdt, AT91_WDT_MR);
> -	reg &= ~AT91_WDT_WDDIS;
> -	wdt_write(wdt, AT91_WDT_MR, reg);
> -
> -	reg = wdt->config;
> -	reg |= AT91_WDT_SET_WDD(value);
> -	reg |= AT91_WDT_SET_WDV(value);
> -
> -	wdt_write(wdt, AT91_WDT_MR, reg);
> -
> -	return 0;
> -}
> -
>  static int sama5d4_wdt_probe(struct platform_device *pdev)
>  {
>  	struct watchdog_device *wdd;
>  	struct sama5d4_wdt *wdt;
>  	struct resource *res;
>  	void __iomem *regs;
> -	u32 irq = 0;
> +	struct device_node *np = pdev->dev.of_node;
> +	const char *tmp;
> +	u32 mr, reg, irq = 0;
>  	int ret;
>
>  	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
> @@ -201,17 +158,26 @@ static int sama5d4_wdt_probe(struct platform_device *pdev)
>
>  	wdt->reg_base = regs;
>
> -	if (pdev->dev.of_node) {
> -		irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
> -		if (!irq)
> -			dev_warn(&pdev->dev, "failed to get IRQ from DT\n");
> +	irq = irq_of_parse_and_map(np, 0);
> +	if (!irq)
> +		dev_warn(&pdev->dev, "failed to get IRQ from DT\n");
>
> -		ret = of_sama5d4_wdt_init(pdev->dev.of_node, wdt);
> -		if (ret)
> -			return ret;
> -	}
>
> -	if ((wdt->config & AT91_WDT_WDFIEN) && irq) {
> +	mr = AT91_WDT_WDDIS;
> +
> +	if (!of_property_read_string(np, "atmel,watchdog-type", &tmp) &&
> +	    !strcmp(tmp, "software"))
> +		mr |= AT91_WDT_WDFIEN;
> +	else
> +		mr |= AT91_WDT_WDRSTEN;
> +
> +	if (of_property_read_bool(np, "atmel,idle-halt"))
> +		mr |= AT91_WDT_WDIDLEHLT;
> +
> +	if (of_property_read_bool(np, "atmel,dbg-halt"))
> +		mr |= AT91_WDT_WDDBGHLT;
> +
> +	if ((mr & AT91_WDT_WDFIEN) && irq) {
>  		ret = devm_request_irq(&pdev->dev, irq, sama5d4_wdt_irq_handler,
>  				       IRQF_SHARED | IRQF_IRQPOLL |
>  				       IRQF_NO_SUSPEND, pdev->name, pdev);
> @@ -228,9 +194,19 @@ static int sama5d4_wdt_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>
> -	ret = sama5d4_wdt_init(wdt);
> -	if (ret)
> -		return ret;
> +	/*
> +	 * WDV and WDD must not be modified when the WDDIS bit is set, so clear
> +	 * the WDDIS bit before writing the WDT_MR.
> +	 */
> +	reg = wdt_read(wdt, AT91_WDT_MR);
> +	reg &= ~AT91_WDT_WDDIS;
> +	wdt_write(wdt, AT91_WDT_MR, reg);
> +
> +	reg = mr;
> +	reg |= AT91_WDT_SET_WDD(WDT_SEC2TICKS(wdd->timeout));
> +	reg |= AT91_WDT_SET_WDV(WDT_SEC2TICKS(wdd->timeout));
> +
> +	wdt_write(wdt, AT91_WDT_MR, reg);
>
>  	watchdog_set_nowayout(wdd, nowayout);
>
>

Re: [PATCH v2 2/3] watchdog: sama5d4: Fix setting timeout when watchdog is disabled

[Alexandre Belloni]

On 17/02/2017 at 06:40:33 -0800, Guenter Roeck wrote:
> On 02/16/2017 11:30 AM, Alexandre Belloni wrote:
> > The datasheet states: "When setting the WDDIS bit, and while it is set, the
> > fields WDV and WDD must not be modified."
> > 
> > Ensure WDDIS is not set when changing the timeout and set it afterwards if
> > the watchdog was disabled.
> > 
> > Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> > ---
> > Changes in v2:
> >  - new patch
> >  drivers/watchdog/sama5d4_wdt.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> > index 2c6f5a70ae67..2a60251806d2 100644
> > --- a/drivers/watchdog/sama5d4_wdt.c
> > +++ b/drivers/watchdog/sama5d4_wdt.c
> > @@ -90,11 +90,18 @@ static int sama5d4_wdt_set_timeout(struct watchdog_device *wdd,
> >  	u32 reg;
> > 
> >  	reg = wdt_read(wdt, AT91_WDT_MR);
> > +
> > +	if (reg & AT91_WDT_WDDIS)
> > +		wdt_write(wdt, AT91_WDT_MR, reg & ~AT91_WDT_WDDIS);
> > +
> >  	reg &= ~AT91_WDT_WDV;
> >  	reg &= ~AT91_WDT_WDD;
> >  	reg |= AT91_WDT_SET_WDV(value);
> >  	reg |= AT91_WDT_SET_WDD(value);
> > -	wdt_write(wdt, AT91_WDT_MR, reg);
> > +	wdt_write(wdt, AT91_WDT_MR, reg & ~AT91_WDT_WDDIS);
> > +
> > +	if (reg & AT91_WDT_WDDIS)
> > +		wdt_write(wdt, AT91_WDT_MR, reg);
> > 
> That means if the watchdog is running, the timeout would not be updated.
> It should be updated no matter if it is running or not.
> 

No, it is enabling the watchdog, then changing WDV and WDD and finally
disabling the watchdog if necessary. So, WDV and WDD are always changed.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v2 3/3] watchdog: sama5d4: Implement resume hook

[Alexandre Belloni]

On 17/02/2017 at 06:47:03 -0800, Guenter Roeck wrote:
> On 02/16/2017 11:30 AM, Alexandre Belloni wrote:
> > When resuming for the deepest state on sama5d2, it is necessary to restore
> > MR as the registers are lost.
> > 
> > Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> > ---
> > Changes in v2:
> >  - cache mr beofre suspending
> > 
> >  drivers/watchdog/sama5d4_wdt.c | 32 ++++++++++++++++++++++++++++++++
> >  1 file changed, 32 insertions(+)
> > 
> > diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> > index 2a60251806d2..5ddeb4803dc3 100644
> > --- a/drivers/watchdog/sama5d4_wdt.c
> > +++ b/drivers/watchdog/sama5d4_wdt.c
> > @@ -28,6 +28,7 @@
> >  struct sama5d4_wdt {
> >  	struct watchdog_device	wdd;
> >  	void __iomem		*reg_base;
> > +	u32			mr;
> 
> Makes me wonder if we shouldn't just retain the original 'config'
> (and maybe rename it to 'mr). After all, it _is_ used now.
> 
> >  };
> > 
> >  static int wdt_timeout = WDT_DEFAULT_TIMEOUT;
> > @@ -248,11 +249,42 @@ static const struct of_device_id sama5d4_wdt_of_match[] = {
> >  };
> >  MODULE_DEVICE_TABLE(of, sama5d4_wdt_of_match);
> > 
> > +#ifdef CONFIG_PM_SLEEP
> > +static int sama5d4_wdt_suspend(struct device *dev)
> > +{
> > +	struct sama5d4_wdt *wdt = dev_get_drvdata(dev);
> > +
> > +	wdt->mr = wdt_read(wdt, AT91_WDT_MR);
> > +
> 
> Wouldn't you want to stop the watchdog here if it is running,
> ie set AT91_WDT_WDDIS ?
> 

Some existing customers want the watchdog to continue to run while the
system is suspended.

> > +	return 0;
> > +}
> > +
> > +static int sama5d4_wdt_resume(struct device *dev)
> > +{
> > +	struct sama5d4_wdt *wdt = dev_get_drvdata(dev);
> > +	u32 reg;
> > +
> > +	reg = wdt_read(wdt, AT91_WDT_MR);
> > +	if (reg & AT91_WDT_WDDIS)
> > +		wdt_write(wdt, AT91_WDT_MR, reg & ~AT91_WDT_WDDIS);
> > +
> > +	wdt_write(wdt, AT91_WDT_MR, wdt->mr & ~AT91_WDT_WDDIS);
> 
> Is that necessary ? Why not just write wdt->mr unconditionally ?
> 

Because you can change WDV and WDD *OR* WDDIS but not both at the same
time.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v2 1/3] watchdog: sama5d4: Cleanup init

[Alexandre Belloni]

On 17/02/2017 at 06:48:36 -0800, Guenter Roeck wrote:
> On 02/16/2017 11:30 AM, Alexandre Belloni wrote:
> > .config is used to cache a part of WDT_MR at probe time and is not used
> > afterwards. Also functions are used while they always return 0. Simplify
> > the flow by having everything in .probe().
> > 
> 
> Does that really improve anything ? It makes the code harder to read
> and, ultimately, the dropped value in sama5d4_wdt is added back in a
> later patch as 'mr'.
> 

I don't find the current code to be particularly easy to read but maybe
it is a matter of taste.

I would still remove the test on pdev->dev.of_node because it will never
be false anyway.


> Guenter
> 
> > Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> > ---
> > Changes in v2:
> >  - completely get rid of .config instead of caching it
> >  - merge init functions in probe()
> > 
> >  drivers/watchdog/sama5d4_wdt.c | 92 ++++++++++++++++--------------------------
> >  1 file changed, 34 insertions(+), 58 deletions(-)
> > 
> > diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
> > index a49634cdc1cc..2c6f5a70ae67 100644
> > --- a/drivers/watchdog/sama5d4_wdt.c
> > +++ b/drivers/watchdog/sama5d4_wdt.c
> > @@ -28,7 +28,6 @@
> >  struct sama5d4_wdt {
> >  	struct watchdog_device	wdd;
> >  	void __iomem		*reg_base;
> > -	u32	config;
> >  };
> > 
> >  static int wdt_timeout = WDT_DEFAULT_TIMEOUT;
> > @@ -128,57 +127,15 @@ static irqreturn_t sama5d4_wdt_irq_handler(int irq, void *dev_id)
> >  	return IRQ_HANDLED;
> >  }
> > 
> > -static int of_sama5d4_wdt_init(struct device_node *np, struct sama5d4_wdt *wdt)
> > -{
> > -	const char *tmp;
> > -
> > -	wdt->config = AT91_WDT_WDDIS;
> > -
> > -	if (!of_property_read_string(np, "atmel,watchdog-type", &tmp) &&
> > -	    !strcmp(tmp, "software"))
> > -		wdt->config |= AT91_WDT_WDFIEN;
> > -	else
> > -		wdt->config |= AT91_WDT_WDRSTEN;
> > -
> > -	if (of_property_read_bool(np, "atmel,idle-halt"))
> > -		wdt->config |= AT91_WDT_WDIDLEHLT;
> > -
> > -	if (of_property_read_bool(np, "atmel,dbg-halt"))
> > -		wdt->config |= AT91_WDT_WDDBGHLT;
> > -
> > -	return 0;
> > -}
> > -
> > -static int sama5d4_wdt_init(struct sama5d4_wdt *wdt)
> > -{
> > -	struct watchdog_device *wdd = &wdt->wdd;
> > -	u32 value = WDT_SEC2TICKS(wdd->timeout);
> > -	u32 reg;
> > -
> > -	/*
> > -	 * Because the fields WDV and WDD must not be modified when the WDDIS
> > -	 * bit is set, so clear the WDDIS bit before writing the WDT_MR.
> > -	 */
> > -	reg = wdt_read(wdt, AT91_WDT_MR);
> > -	reg &= ~AT91_WDT_WDDIS;
> > -	wdt_write(wdt, AT91_WDT_MR, reg);
> > -
> > -	reg = wdt->config;
> > -	reg |= AT91_WDT_SET_WDD(value);
> > -	reg |= AT91_WDT_SET_WDV(value);
> > -
> > -	wdt_write(wdt, AT91_WDT_MR, reg);
> > -
> > -	return 0;
> > -}
> > -
> >  static int sama5d4_wdt_probe(struct platform_device *pdev)
> >  {
> >  	struct watchdog_device *wdd;
> >  	struct sama5d4_wdt *wdt;
> >  	struct resource *res;
> >  	void __iomem *regs;
> > -	u32 irq = 0;
> > +	struct device_node *np = pdev->dev.of_node;
> > +	const char *tmp;
> > +	u32 mr, reg, irq = 0;
> >  	int ret;
> > 
> >  	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
> > @@ -201,17 +158,26 @@ static int sama5d4_wdt_probe(struct platform_device *pdev)
> > 
> >  	wdt->reg_base = regs;
> > 
> > -	if (pdev->dev.of_node) {
> > -		irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
> > -		if (!irq)
> > -			dev_warn(&pdev->dev, "failed to get IRQ from DT\n");
> > +	irq = irq_of_parse_and_map(np, 0);
> > +	if (!irq)
> > +		dev_warn(&pdev->dev, "failed to get IRQ from DT\n");
> > 
> > -		ret = of_sama5d4_wdt_init(pdev->dev.of_node, wdt);
> > -		if (ret)
> > -			return ret;
> > -	}
> > 
> > -	if ((wdt->config & AT91_WDT_WDFIEN) && irq) {
> > +	mr = AT91_WDT_WDDIS;
> > +
> > +	if (!of_property_read_string(np, "atmel,watchdog-type", &tmp) &&
> > +	    !strcmp(tmp, "software"))
> > +		mr |= AT91_WDT_WDFIEN;
> > +	else
> > +		mr |= AT91_WDT_WDRSTEN;
> > +
> > +	if (of_property_read_bool(np, "atmel,idle-halt"))
> > +		mr |= AT91_WDT_WDIDLEHLT;
> > +
> > +	if (of_property_read_bool(np, "atmel,dbg-halt"))
> > +		mr |= AT91_WDT_WDDBGHLT;
> > +
> > +	if ((mr & AT91_WDT_WDFIEN) && irq) {
> >  		ret = devm_request_irq(&pdev->dev, irq, sama5d4_wdt_irq_handler,
> >  				       IRQF_SHARED | IRQF_IRQPOLL |
> >  				       IRQF_NO_SUSPEND, pdev->name, pdev);
> > @@ -228,9 +194,19 @@ static int sama5d4_wdt_probe(struct platform_device *pdev)
> >  		return ret;
> >  	}
> > 
> > -	ret = sama5d4_wdt_init(wdt);
> > -	if (ret)
> > -		return ret;
> > +	/*
> > +	 * WDV and WDD must not be modified when the WDDIS bit is set, so clear
> > +	 * the WDDIS bit before writing the WDT_MR.
> > +	 */
> > +	reg = wdt_read(wdt, AT91_WDT_MR);
> > +	reg &= ~AT91_WDT_WDDIS;
> > +	wdt_write(wdt, AT91_WDT_MR, reg);
> > +
> > +	reg = mr;
> > +	reg |= AT91_WDT_SET_WDD(WDT_SEC2TICKS(wdd->timeout));
> > +	reg |= AT91_WDT_SET_WDV(WDT_SEC2TICKS(wdd->timeout));
> > +
> > +	wdt_write(wdt, AT91_WDT_MR, reg);
> > 
> >  	watchdog_set_nowayout(wdd, nowayout);
> > 
> > 
> 

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v2 1/3] watchdog: sama5d4: Cleanup init

[Guenter Roeck]

On 02/17/2017 07:35 AM, Alexandre Belloni wrote:
> On 17/02/2017 at 06:48:36 -0800, Guenter Roeck wrote:
>> On 02/16/2017 11:30 AM, Alexandre Belloni wrote:
>>> .config is used to cache a part of WDT_MR at probe time and is not used
>>> afterwards. Also functions are used while they always return 0. Simplify
>>> the flow by having everything in .probe().
>>>
>>
>> Does that really improve anything ? It makes the code harder to read
>> and, ultimately, the dropped value in sama5d4_wdt is added back in a
>> later patch as 'mr'.
>>
>
> I don't find the current code to be particularly easy to read but maybe
> it is a matter of taste.
>
I always thought it is commonly accepted that splitting code into smaller
functions tends to improve readability. I for my part still think it does,
and splitting out reading configuration data and initializing the chip
are good candidates.

[ Not that the functions are necessarily named well here, but that is
a different issue ]

> I would still remove the test on pdev->dev.of_node because it will never
> be false anyway.
>
Agreed.

Guenter

>
>> Guenter
>>
>>> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
>>> ---
>>> Changes in v2:
>>>  - completely get rid of .config instead of caching it
>>>  - merge init functions in probe()
>>>
>>>  drivers/watchdog/sama5d4_wdt.c | 92 ++++++++++++++++--------------------------
>>>  1 file changed, 34 insertions(+), 58 deletions(-)
>>>
>>> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
>>> index a49634cdc1cc..2c6f5a70ae67 100644
>>> --- a/drivers/watchdog/sama5d4_wdt.c
>>> +++ b/drivers/watchdog/sama5d4_wdt.c
>>> @@ -28,7 +28,6 @@
>>>  struct sama5d4_wdt {
>>>  	struct watchdog_device	wdd;
>>>  	void __iomem		*reg_base;
>>> -	u32	config;
>>>  };
>>>
>>>  static int wdt_timeout = WDT_DEFAULT_TIMEOUT;
>>> @@ -128,57 +127,15 @@ static irqreturn_t sama5d4_wdt_irq_handler(int irq, void *dev_id)
>>>  	return IRQ_HANDLED;
>>>  }
>>>
>>> -static int of_sama5d4_wdt_init(struct device_node *np, struct sama5d4_wdt *wdt)
>>> -{
>>> -	const char *tmp;
>>> -
>>> -	wdt->config = AT91_WDT_WDDIS;
>>> -
>>> -	if (!of_property_read_string(np, "atmel,watchdog-type", &tmp) &&
>>> -	    !strcmp(tmp, "software"))
>>> -		wdt->config |= AT91_WDT_WDFIEN;
>>> -	else
>>> -		wdt->config |= AT91_WDT_WDRSTEN;
>>> -
>>> -	if (of_property_read_bool(np, "atmel,idle-halt"))
>>> -		wdt->config |= AT91_WDT_WDIDLEHLT;
>>> -
>>> -	if (of_property_read_bool(np, "atmel,dbg-halt"))
>>> -		wdt->config |= AT91_WDT_WDDBGHLT;
>>> -
>>> -	return 0;
>>> -}
>>> -
>>> -static int sama5d4_wdt_init(struct sama5d4_wdt *wdt)
>>> -{
>>> -	struct watchdog_device *wdd = &wdt->wdd;
>>> -	u32 value = WDT_SEC2TICKS(wdd->timeout);
>>> -	u32 reg;
>>> -
>>> -	/*
>>> -	 * Because the fields WDV and WDD must not be modified when the WDDIS
>>> -	 * bit is set, so clear the WDDIS bit before writing the WDT_MR.
>>> -	 */
>>> -	reg = wdt_read(wdt, AT91_WDT_MR);
>>> -	reg &= ~AT91_WDT_WDDIS;
>>> -	wdt_write(wdt, AT91_WDT_MR, reg);
>>> -
>>> -	reg = wdt->config;
>>> -	reg |= AT91_WDT_SET_WDD(value);
>>> -	reg |= AT91_WDT_SET_WDV(value);
>>> -
>>> -	wdt_write(wdt, AT91_WDT_MR, reg);
>>> -
>>> -	return 0;
>>> -}
>>> -
>>>  static int sama5d4_wdt_probe(struct platform_device *pdev)
>>>  {
>>>  	struct watchdog_device *wdd;
>>>  	struct sama5d4_wdt *wdt;
>>>  	struct resource *res;
>>>  	void __iomem *regs;
>>> -	u32 irq = 0;
>>> +	struct device_node *np = pdev->dev.of_node;
>>> +	const char *tmp;
>>> +	u32 mr, reg, irq = 0;
>>>  	int ret;
>>>
>>>  	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
>>> @@ -201,17 +158,26 @@ static int sama5d4_wdt_probe(struct platform_device *pdev)
>>>
>>>  	wdt->reg_base = regs;
>>>
>>> -	if (pdev->dev.of_node) {
>>> -		irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
>>> -		if (!irq)
>>> -			dev_warn(&pdev->dev, "failed to get IRQ from DT\n");
>>> +	irq = irq_of_parse_and_map(np, 0);
>>> +	if (!irq)
>>> +		dev_warn(&pdev->dev, "failed to get IRQ from DT\n");
>>>
>>> -		ret = of_sama5d4_wdt_init(pdev->dev.of_node, wdt);
>>> -		if (ret)
>>> -			return ret;
>>> -	}
>>>
>>> -	if ((wdt->config & AT91_WDT_WDFIEN) && irq) {
>>> +	mr = AT91_WDT_WDDIS;
>>> +
>>> +	if (!of_property_read_string(np, "atmel,watchdog-type", &tmp) &&
>>> +	    !strcmp(tmp, "software"))
>>> +		mr |= AT91_WDT_WDFIEN;
>>> +	else
>>> +		mr |= AT91_WDT_WDRSTEN;
>>> +
>>> +	if (of_property_read_bool(np, "atmel,idle-halt"))
>>> +		mr |= AT91_WDT_WDIDLEHLT;
>>> +
>>> +	if (of_property_read_bool(np, "atmel,dbg-halt"))
>>> +		mr |= AT91_WDT_WDDBGHLT;
>>> +
>>> +	if ((mr & AT91_WDT_WDFIEN) && irq) {
>>>  		ret = devm_request_irq(&pdev->dev, irq, sama5d4_wdt_irq_handler,
>>>  				       IRQF_SHARED | IRQF_IRQPOLL |
>>>  				       IRQF_NO_SUSPEND, pdev->name, pdev);
>>> @@ -228,9 +194,19 @@ static int sama5d4_wdt_probe(struct platform_device *pdev)
>>>  		return ret;
>>>  	}
>>>
>>> -	ret = sama5d4_wdt_init(wdt);
>>> -	if (ret)
>>> -		return ret;
>>> +	/*
>>> +	 * WDV and WDD must not be modified when the WDDIS bit is set, so clear
>>> +	 * the WDDIS bit before writing the WDT_MR.
>>> +	 */
>>> +	reg = wdt_read(wdt, AT91_WDT_MR);
>>> +	reg &= ~AT91_WDT_WDDIS;
>>> +	wdt_write(wdt, AT91_WDT_MR, reg);
>>> +
>>> +	reg = mr;
>>> +	reg |= AT91_WDT_SET_WDD(WDT_SEC2TICKS(wdd->timeout));
>>> +	reg |= AT91_WDT_SET_WDV(WDT_SEC2TICKS(wdd->timeout));
>>> +
>>> +	wdt_write(wdt, AT91_WDT_MR, reg);
>>>
>>>  	watchdog_set_nowayout(wdd, nowayout);
>>>
>>>
>>
>

Re: [PATCH v2 2/3] watchdog: sama5d4: Fix setting timeout when watchdog is disabled

[Guenter Roeck]

On 02/17/2017 07:16 AM, Alexandre Belloni wrote:
> On 17/02/2017 at 06:40:33 -0800, Guenter Roeck wrote:
>> On 02/16/2017 11:30 AM, Alexandre Belloni wrote:
>>> The datasheet states: "When setting the WDDIS bit, and while it is set, the
>>> fields WDV and WDD must not be modified."
>>>
>>> Ensure WDDIS is not set when changing the timeout and set it afterwards if
>>> the watchdog was disabled.
>>>
>>> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
>>> ---
>>> Changes in v2:
>>>  - new patch
>>>  drivers/watchdog/sama5d4_wdt.c | 9 ++++++++-
>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
>>> index 2c6f5a70ae67..2a60251806d2 100644
>>> --- a/drivers/watchdog/sama5d4_wdt.c
>>> +++ b/drivers/watchdog/sama5d4_wdt.c
>>> @@ -90,11 +90,18 @@ static int sama5d4_wdt_set_timeout(struct watchdog_device *wdd,
>>>  	u32 reg;
>>>
>>>  	reg = wdt_read(wdt, AT91_WDT_MR);
>>> +
>>> +	if (reg & AT91_WDT_WDDIS)
>>> +		wdt_write(wdt, AT91_WDT_MR, reg & ~AT91_WDT_WDDIS);
>>> +
>>>  	reg &= ~AT91_WDT_WDV;
>>>  	reg &= ~AT91_WDT_WDD;
>>>  	reg |= AT91_WDT_SET_WDV(value);
>>>  	reg |= AT91_WDT_SET_WDD(value);
>>> -	wdt_write(wdt, AT91_WDT_MR, reg);
>>> +	wdt_write(wdt, AT91_WDT_MR, reg & ~AT91_WDT_WDDIS);
>>> +
>>> +	if (reg & AT91_WDT_WDDIS)
>>> +		wdt_write(wdt, AT91_WDT_MR, reg);
>>>
>> That means if the watchdog is running, the timeout would not be updated.
>> It should be updated no matter if it is running or not.
>>
>
> No, it is enabling the watchdog, then changing WDV and WDD and finally
> disabling the watchdog if necessary. So, WDV and WDD are always changed.
>
You are correct. Sorry for the noise.

Seems odd that the watchdog must be _running_ to change the timeout.
Usually, if there is a restriction, it is the opposite. I hope this
doesn't cause race conditions, where the watchdog fires immediately
after being enabled due to a low timeout.

Guenter

Re: [PATCH v2 3/3] watchdog: sama5d4: Implement resume hook

[Guenter Roeck]

On 02/17/2017 07:22 AM, Alexandre Belloni wrote:
> On 17/02/2017 at 06:47:03 -0800, Guenter Roeck wrote:
>> On 02/16/2017 11:30 AM, Alexandre Belloni wrote:
>>> When resuming for the deepest state on sama5d2, it is necessary to restore
>>> MR as the registers are lost.
>>>
>>> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
>>> ---
>>> Changes in v2:
>>>  - cache mr beofre suspending
>>>
>>>  drivers/watchdog/sama5d4_wdt.c | 32 ++++++++++++++++++++++++++++++++
>>>  1 file changed, 32 insertions(+)
>>>
>>> diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wdt.c
>>> index 2a60251806d2..5ddeb4803dc3 100644
>>> --- a/drivers/watchdog/sama5d4_wdt.c
>>> +++ b/drivers/watchdog/sama5d4_wdt.c
>>> @@ -28,6 +28,7 @@
>>>  struct sama5d4_wdt {
>>>  	struct watchdog_device	wdd;
>>>  	void __iomem		*reg_base;
>>> +	u32			mr;
>>
>> Makes me wonder if we shouldn't just retain the original 'config'
>> (and maybe rename it to 'mr). After all, it _is_ used now.
>>
>>>  };
>>>
>>>  static int wdt_timeout = WDT_DEFAULT_TIMEOUT;
>>> @@ -248,11 +249,42 @@ static const struct of_device_id sama5d4_wdt_of_match[] = {
>>>  };
>>>  MODULE_DEVICE_TABLE(of, sama5d4_wdt_of_match);
>>>
>>> +#ifdef CONFIG_PM_SLEEP
>>> +static int sama5d4_wdt_suspend(struct device *dev)
>>> +{
>>> +	struct sama5d4_wdt *wdt = dev_get_drvdata(dev);
>>> +
>>> +	wdt->mr = wdt_read(wdt, AT91_WDT_MR);
>>> +
>>
>> Wouldn't you want to stop the watchdog here if it is running,
>> ie set AT91_WDT_WDDIS ?
>>
>
> Some existing customers want the watchdog to continue to run while the
> system is suspended.
>
>>> +	return 0;
>>> +}
>>> +
>>> +static int sama5d4_wdt_resume(struct device *dev)
>>> +{
>>> +	struct sama5d4_wdt *wdt = dev_get_drvdata(dev);
>>> +	u32 reg;
>>> +
>>> +	reg = wdt_read(wdt, AT91_WDT_MR);
>>> +	if (reg & AT91_WDT_WDDIS)
>>> +		wdt_write(wdt, AT91_WDT_MR, reg & ~AT91_WDT_WDDIS);
>>> +
>>> +	wdt_write(wdt, AT91_WDT_MR, wdt->mr & ~AT91_WDT_WDDIS);
>>
>> Is that necessary ? Why not just write wdt->mr unconditionally ?
>>
>
> Because you can change WDV and WDD *OR* WDDIS but not both at the same
> time.
>

Odd hardware ;-). Please add a comment explaining why the watchdog isn't
stopped on suspend, and why the AT91_WDT_MR has to be reprogrammed on resume.

Thanks,
Guenter

Re: [PATCH v2 2/3] watchdog: sama5d4: Fix setting timeout when watchdog is disabled

[Alexandre Belloni]

On 19/02/2017 at 08:57:35 -0800, Guenter Roeck wrote:
> > > That means if the watchdog is running, the timeout would not be updated.
> > > It should be updated no matter if it is running or not.
> > > 
> > 
> > No, it is enabling the watchdog, then changing WDV and WDD and finally
> > disabling the watchdog if necessary. So, WDV and WDD are always changed.
> > 
> You are correct. Sorry for the noise.
> 
> Seems odd that the watchdog must be _running_ to change the timeout.
> Usually, if there is a restriction, it is the opposite. I hope this
> doesn't cause race conditions, where the watchdog fires immediately
> after being enabled due to a low timeout.
> 

While it is difficult to reproduce, I can confirm it races and sometimes
reset the SoC without any good reason. It doesn't matter whether it is
disabled or not

I've raised the issue at Atmel last Thursday so I don't have any answer
yet.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v2 2/3] watchdog: sama5d4: Fix setting timeout when watchdog is disabled

[Guenter Roeck]

On 02/19/2017 03:52 PM, Alexandre Belloni wrote:
> On 19/02/2017 at 08:57:35 -0800, Guenter Roeck wrote:
>>>> That means if the watchdog is running, the timeout would not be updated.
>>>> It should be updated no matter if it is running or not.
>>>>
>>>
>>> No, it is enabling the watchdog, then changing WDV and WDD and finally
>>> disabling the watchdog if necessary. So, WDV and WDD are always changed.
>>>
>> You are correct. Sorry for the noise.
>>
>> Seems odd that the watchdog must be _running_ to change the timeout.
>> Usually, if there is a restriction, it is the opposite. I hope this
>> doesn't cause race conditions, where the watchdog fires immediately
>> after being enabled due to a low timeout.
>>
>
> While it is difficult to reproduce, I can confirm it races and sometimes
> reset the SoC without any good reason. It doesn't matter whether it is
> disabled or not
>

Outch :-(.

> I've raised the issue at Atmel last Thursday so I don't have any answer
> yet.
>

Please keep us in the loop.

Thanks,
Guenter

Re: [PATCH v2 2/3] watchdog: sama5d4: Fix setting timeout when watchdog is disabled

[Alexandre Belloni]

Hi,

On 19/02/2017 at 20:46:13 -0800, Guenter Roeck wrote:
> Please keep us in the loop.
> 

I've sent a new series that solves the issues we were seeing. I've tied
to document as much as possible and to make the driver as simple as
possible.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com