[PATCH 0/7] jz4740 watchdog driver & platform cleanups

[PATCH 0/7] jz4740 watchdog driver & platform cleanups

[Paul Cercueil]

Hi,

This patchset is meant to drop the platform code that handles the system
reset, since the watchdog driver can be used for this task.

Some fixes and cleanups are also included.

Thanks,
-Paul Cercueil

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/7] watchdog: JZ4740: Disable clock after stopping counter

[Paul Cercueil]

Previously, the clock was disabled first, which makes the watchdog
component insensitive to register writes.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/watchdog/jz4740_wdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
index 20627f22baf6..6955deb100ef 100644
--- a/drivers/watchdog/jz4740_wdt.c
+++ b/drivers/watchdog/jz4740_wdt.c
@@ -124,8 +124,8 @@ static int jz4740_wdt_stop(struct watchdog_device *wdt_dev)
 {
 	struct jz4740_wdt_drvdata *drvdata = watchdog_get_drvdata(wdt_dev);
 
-	jz4740_timer_disable_watchdog();
 	writeb(0x0, drvdata->base + JZ_REG_WDT_COUNTER_ENABLE);
+	jz4740_timer_disable_watchdog();
 
 	return 0;
 }
-- 
2.11.0

[PATCH 2/7] watchdog: jz4740: Use devm_* functions

[Paul Cercueil]

- Use devm_clk_get instead of clk_get
- Use devm_watchdog_register_device instead of watchdog_register_device

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/watchdog/jz4740_wdt.c | 27 ++++++++-------------------
 1 file changed, 8 insertions(+), 19 deletions(-)

diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
index 6955deb100ef..92d6ca8ceb49 100644
--- a/drivers/watchdog/jz4740_wdt.c
+++ b/drivers/watchdog/jz4740_wdt.c
@@ -178,40 +178,29 @@ static int jz4740_wdt_probe(struct platform_device *pdev)
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	drvdata->base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(drvdata->base)) {
-		ret = PTR_ERR(drvdata->base);
-		goto err_out;
-	}
+	if (IS_ERR(drvdata->base))
+		return PTR_ERR(drvdata->base);
 
-	drvdata->rtc_clk = clk_get(&pdev->dev, "rtc");
+	drvdata->rtc_clk = devm_clk_get(&pdev->dev, "rtc");
 	if (IS_ERR(drvdata->rtc_clk)) {
 		dev_err(&pdev->dev, "cannot find RTC clock\n");
-		ret = PTR_ERR(drvdata->rtc_clk);
-		goto err_out;
+		return PTR_ERR(drvdata->rtc_clk);
 	}
 
-	ret = watchdog_register_device(&drvdata->wdt);
+	ret = devm_watchdog_register_device(&pdev->dev, &drvdata->wdt);
 	if (ret < 0)
-		goto err_disable_clk;
+		return ret;
 
 	platform_set_drvdata(pdev, drvdata);
-	return 0;
 
-err_disable_clk:
-	clk_put(drvdata->rtc_clk);
-err_out:
-	return ret;
+	return 0;
 }
 
 static int jz4740_wdt_remove(struct platform_device *pdev)
 {
 	struct jz4740_wdt_drvdata *drvdata = platform_get_drvdata(pdev);
 
-	jz4740_wdt_stop(&drvdata->wdt);
-	watchdog_unregister_device(&drvdata->wdt);
-	clk_put(drvdata->rtc_clk);
-
-	return 0;
+	return jz4740_wdt_stop(&drvdata->wdt);
 }
 
 static struct platform_driver jz4740_wdt_driver = {
-- 
2.11.0

[PATCH 3/7] watchdog: JZ4740: Register a restart handler

[Paul Cercueil]

The watchdog driver can restart the system by simply configuring the
hardware for a timeout of 0 seconds.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/watchdog/jz4740_wdt.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
index 92d6ca8ceb49..fa7f49a3212c 100644
--- a/drivers/watchdog/jz4740_wdt.c
+++ b/drivers/watchdog/jz4740_wdt.c
@@ -130,6 +130,14 @@ static int jz4740_wdt_stop(struct watchdog_device *wdt_dev)
 	return 0;
 }
 
+static int jz4740_wdt_restart(struct watchdog_device *wdt_dev,
+			      unsigned long action, void *data)
+{
+	wdt_dev->timeout = 0;
+	jz4740_wdt_start(wdt_dev);
+	return 0;
+}
+
 static const struct watchdog_info jz4740_wdt_info = {
 	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
 	.identity = "jz4740 Watchdog",
@@ -141,6 +149,7 @@ static const struct watchdog_ops jz4740_wdt_ops = {
 	.stop = jz4740_wdt_stop,
 	.ping = jz4740_wdt_ping,
 	.set_timeout = jz4740_wdt_set_timeout,
+	.restart = jz4740_wdt_restart,
 };
 
 #ifdef CONFIG_OF
-- 
2.11.0

[PATCH 4/7] MIPS: jz4740: dts: Add bindings for the jz4740-wdt driver

[Paul Cercueil]

Also remove the watchdog platform_device from platform.c, since it
wasn't used anywhere anyway.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 arch/mips/boot/dts/ingenic/jz4740.dtsi |  8 ++++++++
 arch/mips/jz4740/platform.c            | 16 ----------------
 2 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/arch/mips/boot/dts/ingenic/jz4740.dtsi b/arch/mips/boot/dts/ingenic/jz4740.dtsi
index cd5185bb90ae..26c6b561d6f7 100644
--- a/arch/mips/boot/dts/ingenic/jz4740.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4740.dtsi
@@ -45,6 +45,14 @@
 		#clock-cells = <1>;
 	};
 
+	watchdog: watchdog@10002000 {
+		compatible = "ingenic,jz4740-watchdog";
+		reg = <0x10002000 0x10>;
+
+		clocks = <&cgu JZ4740_CLK_RTC>;
+		clock-names = "rtc";
+	};
+
 	rtc_dev: rtc@10003000 {
 		compatible = "ingenic,jz4740-rtc";
 		reg = <0x10003000 0x40>;
diff --git a/arch/mips/jz4740/platform.c b/arch/mips/jz4740/platform.c
index 5b7cdd67a9d9..cbc5f8e87230 100644
--- a/arch/mips/jz4740/platform.c
+++ b/arch/mips/jz4740/platform.c
@@ -233,22 +233,6 @@ struct platform_device jz4740_adc_device = {
 	.resource	= jz4740_adc_resources,
 };
 
-/* Watchdog */
-static struct resource jz4740_wdt_resources[] = {
-	{
-		.start = JZ4740_WDT_BASE_ADDR,
-		.end   = JZ4740_WDT_BASE_ADDR + 0x10 - 1,
-		.flags = IORESOURCE_MEM,
-	},
-};
-
-struct platform_device jz4740_wdt_device = {
-	.name	       = "jz4740-wdt",
-	.id	       = -1,
-	.num_resources = ARRAY_SIZE(jz4740_wdt_resources),
-	.resource      = jz4740_wdt_resources,
-};
-
 /* PWM */
 struct platform_device jz4740_pwm_device = {
 	.name = "jz4740-pwm",
-- 
2.11.0

[PATCH 5/7] MIPS: jz4780: dts: Fix watchdog node

[Paul Cercueil]

- The previous node requested a memory area of 0x100 bytes, while the
  driver only manipulates four registers present in the first 0x10 bytes.

- The driver requests for the "rtc" clock, but the previous node did not
  provide any.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 arch/mips/boot/dts/ingenic/jz4780.dtsi | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi
index 9b5794667aee..a52f59bf58c7 100644
--- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
@@ -221,7 +221,10 @@
 
 	watchdog: watchdog@10002000 {
 		compatible = "ingenic,jz4780-watchdog";
-		reg = <0x10002000 0x100>;
+		reg = <0x10002000 0x10>;
+
+		clocks = <&cgu JZ4780_CLK_RTCLK>;
+		clock-names = "rtc";
 	};
 
 	nemc: nemc@13410000 {
-- 
2.11.0

[PATCH 6/7] MIPS: qi_lb60: Enable the jz4740-wdt driver

[Paul Cercueil]

The watchdog is an useful piece of hardware, so there's no reason not to
enable it.

This commit enables the Kconfig option in the qi_lb60 defconfig.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 arch/mips/configs/qi_lb60_defconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/mips/configs/qi_lb60_defconfig b/arch/mips/configs/qi_lb60_defconfig
index 3f1333517405..ba8e1c56b626 100644
--- a/arch/mips/configs/qi_lb60_defconfig
+++ b/arch/mips/configs/qi_lb60_defconfig
@@ -73,6 +73,8 @@ CONFIG_POWER_SUPPLY=y
 CONFIG_BATTERY_JZ4740=y
 CONFIG_CHARGER_GPIO=y
 # CONFIG_HWMON is not set
+CONFIG_WATCHDOG=y
+CONFIG_JZ4740_WDT=y
 CONFIG_MFD_JZ4740_ADC=y
 CONFIG_REGULATOR=y
 CONFIG_REGULATOR_FIXED_VOLTAGE=y
-- 
2.11.0

[PATCH 7/7] MIPS: jz4740: Drop old platform reset code

[Paul Cercueil]

This work is now performed by the watchdog driver directly.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 arch/mips/jz4740/reset.c | 31 -------------------------------
 1 file changed, 31 deletions(-)

diff --git a/arch/mips/jz4740/reset.c b/arch/mips/jz4740/reset.c
index 67780c4b6573..5bf0cf44b55f 100644
--- a/arch/mips/jz4740/reset.c
+++ b/arch/mips/jz4740/reset.c
@@ -12,18 +12,9 @@
  *
  */
 
-#include <linux/clk.h>
-#include <linux/io.h>
-#include <linux/kernel.h>
-#include <linux/pm.h>
-
 #include <asm/reboot.h>
 
-#include <asm/mach-jz4740/base.h>
-#include <asm/mach-jz4740/timer.h>
-
 #include "reset.h"
-#include "clock.h"
 
 static void jz4740_halt(void)
 {
@@ -36,29 +27,7 @@ static void jz4740_halt(void)
 	}
 }
 
-#define JZ_REG_WDT_DATA 0x00
-#define JZ_REG_WDT_COUNTER_ENABLE 0x04
-#define JZ_REG_WDT_COUNTER 0x08
-#define JZ_REG_WDT_CTRL 0x0c
-
-static void jz4740_restart(char *command)
-{
-	void __iomem *wdt_base = ioremap(JZ4740_WDT_BASE_ADDR, 0x0f);
-
-	jz4740_timer_enable_watchdog();
-
-	writeb(0, wdt_base + JZ_REG_WDT_COUNTER_ENABLE);
-
-	writew(0, wdt_base + JZ_REG_WDT_COUNTER);
-	writew(0, wdt_base + JZ_REG_WDT_DATA);
-	writew(BIT(2), wdt_base + JZ_REG_WDT_CTRL);
-
-	writeb(1, wdt_base + JZ_REG_WDT_COUNTER_ENABLE);
-	jz4740_halt();
-}
-
 void jz4740_reset_init(void)
 {
-	_machine_restart = jz4740_restart;
 	_machine_halt = jz4740_halt;
 }
-- 
2.11.0

Re: [PATCH 5/7] MIPS: jz4780: dts: Fix watchdog node

[Mathieu Malaterre]

Hi Paul,

On Thu, Dec 28, 2017 at 5:29 PM, Paul Cercueil <paul@crapouillou.net> wrote:
> - The previous node requested a memory area of 0x100 bytes, while the
>   driver only manipulates four registers present in the first 0x10 bytes.
>
> - The driver requests for the "rtc" clock, but the previous node did not
>   provide any.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  arch/mips/boot/dts/ingenic/jz4780.dtsi | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi
> index 9b5794667aee..a52f59bf58c7 100644
> --- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
> +++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
> @@ -221,7 +221,10 @@
>
>         watchdog: watchdog@10002000 {
>                 compatible = "ingenic,jz4780-watchdog";
> -               reg = <0x10002000 0x100>;
> +               reg = <0x10002000 0x10>;
> +
> +               clocks = <&cgu JZ4780_CLK_RTCLK>;
> +               clock-names = "rtc";
>         };
>
>         nemc: nemc@13410000 {
> --
> 2.11.0
>
>

Looks good, thanks for fixing my mess. Tested on MIPS Creator CI20:

Dec 28 17:27:50 ci20 watchdog[17531]: starting daemon (5.14):
Dec 28 17:27:50 ci20 watchdog[17531]: int=1s realtime=yes sync=no
soft=no mla=0 mem=0
Dec 28 17:27:50 ci20 watchdog[17531]: ping: no machine to check
Dec 28 17:27:50 ci20 watchdog[17531]: file: no file to check
Dec 28 17:27:50 ci20 watchdog[17531]: pidfile: no server process to check
Dec 28 17:27:50 ci20 watchdog[17531]: interface: no interface to check
Dec 28 17:27:50 ci20 watchdog[17531]: temperature: no sensors to check
Dec 28 17:27:50 ci20 watchdog[17531]: test=none(0) repair=none(0)
alive=/dev/watchdog heartbeat=none to=root no_act=no force=no
Dec 28 17:27:50 ci20 watchdog[17531]: watchdog now set to 60 seconds
Dec 28 17:27:50 ci20 watchdog[17531]: hardware watchdog identity:
jz4740 Watchdog

pkill + reboot = ok

Reviewed-by: Mathieu Malaterre <malat@debian.org>

Re: [PATCH 2/7] watchdog: jz4740: Use devm_* functions

[Guenter Roeck]

On 12/28/2017 08:29 AM, Paul Cercueil wrote:
> - Use devm_clk_get instead of clk_get
> - Use devm_watchdog_register_device instead of watchdog_register_device
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>   drivers/watchdog/jz4740_wdt.c | 27 ++++++++-------------------
>   1 file changed, 8 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
> index 6955deb100ef..92d6ca8ceb49 100644
> --- a/drivers/watchdog/jz4740_wdt.c
> +++ b/drivers/watchdog/jz4740_wdt.c
> @@ -178,40 +178,29 @@ static int jz4740_wdt_probe(struct platform_device *pdev)
>   
>   	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>   	drvdata->base = devm_ioremap_resource(&pdev->dev, res);
> -	if (IS_ERR(drvdata->base)) {
> -		ret = PTR_ERR(drvdata->base);
> -		goto err_out;
> -	}
> +	if (IS_ERR(drvdata->base))
> +		return PTR_ERR(drvdata->base);
>   
> -	drvdata->rtc_clk = clk_get(&pdev->dev, "rtc");
> +	drvdata->rtc_clk = devm_clk_get(&pdev->dev, "rtc");
>   	if (IS_ERR(drvdata->rtc_clk)) {
>   		dev_err(&pdev->dev, "cannot find RTC clock\n");
> -		ret = PTR_ERR(drvdata->rtc_clk);
> -		goto err_out;
> +		return PTR_ERR(drvdata->rtc_clk);
>   	}
>   
> -	ret = watchdog_register_device(&drvdata->wdt);
> +	ret = devm_watchdog_register_device(&pdev->dev, &drvdata->wdt);
>   	if (ret < 0)
> -		goto err_disable_clk;
> +		return ret;
>   
>   	platform_set_drvdata(pdev, drvdata);
> -	return 0;
>   
> -err_disable_clk:
> -	clk_put(drvdata->rtc_clk);
> -err_out:
> -	return ret;
> +	return 0;
>   }
>   
>   static int jz4740_wdt_remove(struct platform_device *pdev)
>   {
>   	struct jz4740_wdt_drvdata *drvdata = platform_get_drvdata(pdev);
>   
> -	jz4740_wdt_stop(&drvdata->wdt);
> -	watchdog_unregister_device(&drvdata->wdt);
> -	clk_put(drvdata->rtc_clk);
> -
> -	return 0;
> +	return jz4740_wdt_stop(&drvdata->wdt);

If the watchdog is running, the module can not be unloaded. Even if that wasn't
the case, this defeats both WDIOF_MAGICCLOSE and watchdog_set_nowayout().
Are you sure this is what you want ? If so, please call
watchdog_stop_on_unregister() before registration; this clarifies that this
is what you want, and you can drop the remove function.

Thanks,
Guenter

>   }
>   
>   static struct platform_driver jz4740_wdt_driver = {
> 

Re: [PATCH 1/7] watchdog: JZ4740: Disable clock after stopping counter

[Guenter Roeck]

On 12/28/2017 08:29 AM, Paul Cercueil wrote:
> Previously, the clock was disabled first, which makes the watchdog
> component insensitive to register writes.
> 
> Signed-off-by: Paul Cercueil <paul-icTtO2rgO2OTuSrc4Mpeew@public.gmane.org>

Reviewed-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>

> ---
>   drivers/watchdog/jz4740_wdt.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
> index 20627f22baf6..6955deb100ef 100644
> --- a/drivers/watchdog/jz4740_wdt.c
> +++ b/drivers/watchdog/jz4740_wdt.c
> @@ -124,8 +124,8 @@ static int jz4740_wdt_stop(struct watchdog_device *wdt_dev)
>   {
>   	struct jz4740_wdt_drvdata *drvdata = watchdog_get_drvdata(wdt_dev);
>   
> -	jz4740_timer_disable_watchdog();
>   	writeb(0x0, drvdata->base + JZ_REG_WDT_COUNTER_ENABLE);
> +	jz4740_timer_disable_watchdog();
>   
>   	return 0;
>   }
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/7] watchdog: jz4740: Use devm_* functions

[Guenter Roeck]

On 12/28/2017 08:29 AM, Paul Cercueil wrote:
> - Use devm_clk_get instead of clk_get
> - Use devm_watchdog_register_device instead of watchdog_register_device
> 
> Signed-off-by: Paul Cercueil <paul-icTtO2rgO2OTuSrc4Mpeew@public.gmane.org>

Reviewed-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>

[ the change I suggested earlier should be made in a separate patch ]

> ---
>   drivers/watchdog/jz4740_wdt.c | 27 ++++++++-------------------
>   1 file changed, 8 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
> index 6955deb100ef..92d6ca8ceb49 100644
> --- a/drivers/watchdog/jz4740_wdt.c
> +++ b/drivers/watchdog/jz4740_wdt.c
> @@ -178,40 +178,29 @@ static int jz4740_wdt_probe(struct platform_device *pdev)
>   
>   	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>   	drvdata->base = devm_ioremap_resource(&pdev->dev, res);
> -	if (IS_ERR(drvdata->base)) {
> -		ret = PTR_ERR(drvdata->base);
> -		goto err_out;
> -	}
> +	if (IS_ERR(drvdata->base))
> +		return PTR_ERR(drvdata->base);
>   
> -	drvdata->rtc_clk = clk_get(&pdev->dev, "rtc");
> +	drvdata->rtc_clk = devm_clk_get(&pdev->dev, "rtc");
>   	if (IS_ERR(drvdata->rtc_clk)) {
>   		dev_err(&pdev->dev, "cannot find RTC clock\n");
> -		ret = PTR_ERR(drvdata->rtc_clk);
> -		goto err_out;
> +		return PTR_ERR(drvdata->rtc_clk);
>   	}
>   
> -	ret = watchdog_register_device(&drvdata->wdt);
> +	ret = devm_watchdog_register_device(&pdev->dev, &drvdata->wdt);
>   	if (ret < 0)
> -		goto err_disable_clk;
> +		return ret;
>   
>   	platform_set_drvdata(pdev, drvdata);
> -	return 0;
>   
> -err_disable_clk:
> -	clk_put(drvdata->rtc_clk);
> -err_out:
> -	return ret;
> +	return 0;
>   }
>   
>   static int jz4740_wdt_remove(struct platform_device *pdev)
>   {
>   	struct jz4740_wdt_drvdata *drvdata = platform_get_drvdata(pdev);
>   
> -	jz4740_wdt_stop(&drvdata->wdt);
> -	watchdog_unregister_device(&drvdata->wdt);
> -	clk_put(drvdata->rtc_clk);
> -
> -	return 0;
> +	return jz4740_wdt_stop(&drvdata->wdt);
>   }
>   
>   static struct platform_driver jz4740_wdt_driver = {
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/7] watchdog: JZ4740: Register a restart handler

[Guenter Roeck]

On 12/28/2017 08:29 AM, Paul Cercueil wrote:
> The watchdog driver can restart the system by simply configuring the
> hardware for a timeout of 0 seconds.
> 
> Signed-off-by: Paul Cercueil <paul-icTtO2rgO2OTuSrc4Mpeew@public.gmane.org>

Reviewed-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>

> ---
>   drivers/watchdog/jz4740_wdt.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
> index 92d6ca8ceb49..fa7f49a3212c 100644
> --- a/drivers/watchdog/jz4740_wdt.c
> +++ b/drivers/watchdog/jz4740_wdt.c
> @@ -130,6 +130,14 @@ static int jz4740_wdt_stop(struct watchdog_device *wdt_dev)
>   	return 0;
>   }
>   
> +static int jz4740_wdt_restart(struct watchdog_device *wdt_dev,
> +			      unsigned long action, void *data)
> +{
> +	wdt_dev->timeout = 0;
> +	jz4740_wdt_start(wdt_dev);
> +	return 0;
> +}
> +
>   static const struct watchdog_info jz4740_wdt_info = {
>   	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
>   	.identity = "jz4740 Watchdog",
> @@ -141,6 +149,7 @@ static const struct watchdog_ops jz4740_wdt_ops = {
>   	.stop = jz4740_wdt_stop,
>   	.ping = jz4740_wdt_ping,
>   	.set_timeout = jz4740_wdt_set_timeout,
> +	.restart = jz4740_wdt_restart,
>   };
>   
>   #ifdef CONFIG_OF
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/7] watchdog: jz4740: Use devm_* functions

[Paul Cercueil]

Hi Guenter,

Le jeu. 28 déc. 2017 à 18:48, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> a 
écrit :
> On 12/28/2017 08:29 AM, Paul Cercueil wrote:
>> - Use devm_clk_get instead of clk_get
>> - Use devm_watchdog_register_device instead of 
>> watchdog_register_device
>> 
>> Signed-off-by: Paul Cercueil <paul-icTtO2rgO2OTuSrc4Mpeew@public.gmane.org>
>> ---
>>   drivers/watchdog/jz4740_wdt.c | 27 ++++++++-------------------
>>   1 file changed, 8 insertions(+), 19 deletions(-)
>> 
>> diff --git a/drivers/watchdog/jz4740_wdt.c 
>> b/drivers/watchdog/jz4740_wdt.c
>> index 6955deb100ef..92d6ca8ceb49 100644
>> --- a/drivers/watchdog/jz4740_wdt.c
>> +++ b/drivers/watchdog/jz4740_wdt.c
>> @@ -178,40 +178,29 @@ static int jz4740_wdt_probe(struct 
>> platform_device *pdev)
>>   \x7f  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>   	drvdata->base = devm_ioremap_resource(&pdev->dev, res);
>> -	if (IS_ERR(drvdata->base)) {
>> -		ret = PTR_ERR(drvdata->base);
>> -		goto err_out;
>> -	}
>> +	if (IS_ERR(drvdata->base))
>> +		return PTR_ERR(drvdata->base);
>>   \x7f-	drvdata->rtc_clk = clk_get(&pdev->dev, "rtc");
>> +	drvdata->rtc_clk = devm_clk_get(&pdev->dev, "rtc");
>>   	if (IS_ERR(drvdata->rtc_clk)) {
>>   		dev_err(&pdev->dev, "cannot find RTC clock\n");
>> -		ret = PTR_ERR(drvdata->rtc_clk);
>> -		goto err_out;
>> +		return PTR_ERR(drvdata->rtc_clk);
>>   	}
>>   \x7f-	ret = watchdog_register_device(&drvdata->wdt);
>> +	ret = devm_watchdog_register_device(&pdev->dev, &drvdata->wdt);
>>   	if (ret < 0)
>> -		goto err_disable_clk;
>> +		return ret;
>>   \x7f  	platform_set_drvdata(pdev, drvdata);
>> -	return 0;
>>   \x7f-err_disable_clk:
>> -	clk_put(drvdata->rtc_clk);
>> -err_out:
>> -	return ret;
>> +	return 0;
>>   }
>>   \x7f  static int jz4740_wdt_remove(struct platform_device *pdev)
>>   {
>>   	struct jz4740_wdt_drvdata *drvdata = platform_get_drvdata(pdev);
>>   \x7f-	jz4740_wdt_stop(&drvdata->wdt);
>> -	watchdog_unregister_device(&drvdata->wdt);
>> -	clk_put(drvdata->rtc_clk);
>> -
>> -	return 0;
>> +	return jz4740_wdt_stop(&drvdata->wdt);
> 
> If the watchdog is running, the module can not be unloaded. Even if 
> that wasn't
> the case, this defeats both WDIOF_MAGICCLOSE and 
> watchdog_set_nowayout().
> Are you sure this is what you want ? If so, please call
> watchdog_stop_on_unregister() before registration; this clarifies 
> that this
> is what you want, and you can drop the remove function.
> 
> Thanks,
> Guenter

This patch does not change the behaviour. We always used that driver 
built-in; what
should the behaviour be when unloading the module? Keep the watchdog 
hardware running
if configured for 'nowayout'?

Thanks,
-Paul

> 
>>   }
>>   \x7f  static struct platform_driver jz4740_wdt_driver = {
>> 
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/7] watchdog: jz4740: Use devm_* functions

[Guenter Roeck]

On 12/28/2017 11:59 AM, Paul Cercueil wrote:
> Hi Guenter,
> 
> Le jeu. 28 déc. 2017 à 18:48, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> a écrit :
>> On 12/28/2017 08:29 AM, Paul Cercueil wrote:
>>> - Use devm_clk_get instead of clk_get
>>> - Use devm_watchdog_register_device instead of watchdog_register_device
>>>
>>> Signed-off-by: Paul Cercueil <paul-icTtO2rgO2OTuSrc4Mpeew@public.gmane.org>
>>> ---
>>>   drivers/watchdog/jz4740_wdt.c | 27 ++++++++-------------------
>>>   1 file changed, 8 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
>>> index 6955deb100ef..92d6ca8ceb49 100644
>>> --- a/drivers/watchdog/jz4740_wdt.c
>>> +++ b/drivers/watchdog/jz4740_wdt.c
>>> @@ -178,40 +178,29 @@ static int jz4740_wdt_probe(struct platform_device *pdev)
>>>   \x7f      res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>       drvdata->base = devm_ioremap_resource(&pdev->dev, res);
>>> -    if (IS_ERR(drvdata->base)) {
>>> -        ret = PTR_ERR(drvdata->base);
>>> -        goto err_out;
>>> -    }
>>> +    if (IS_ERR(drvdata->base))
>>> +        return PTR_ERR(drvdata->base);
>>>   \x7f-    drvdata->rtc_clk = clk_get(&pdev->dev, "rtc");
>>> +    drvdata->rtc_clk = devm_clk_get(&pdev->dev, "rtc");
>>>       if (IS_ERR(drvdata->rtc_clk)) {
>>>           dev_err(&pdev->dev, "cannot find RTC clock\n");
>>> -        ret = PTR_ERR(drvdata->rtc_clk);
>>> -        goto err_out;
>>> +        return PTR_ERR(drvdata->rtc_clk);
>>>       }
>>>   \x7f-    ret = watchdog_register_device(&drvdata->wdt);
>>> +    ret = devm_watchdog_register_device(&pdev->dev, &drvdata->wdt);
>>>       if (ret < 0)
>>> -        goto err_disable_clk;
>>> +        return ret;
>>>   \x7f      platform_set_drvdata(pdev, drvdata);
>>> -    return 0;
>>>   \x7f-err_disable_clk:
>>> -    clk_put(drvdata->rtc_clk);
>>> -err_out:
>>> -    return ret;
>>> +    return 0;
>>>   }
>>>   \x7f  static int jz4740_wdt_remove(struct platform_device *pdev)
>>>   {
>>>       struct jz4740_wdt_drvdata *drvdata = platform_get_drvdata(pdev);
>>>   \x7f-    jz4740_wdt_stop(&drvdata->wdt);
>>> -    watchdog_unregister_device(&drvdata->wdt);
>>> -    clk_put(drvdata->rtc_clk);
>>> -
>>> -    return 0;
>>> +    return jz4740_wdt_stop(&drvdata->wdt);
>>
>> If the watchdog is running, the module can not be unloaded. Even if that wasn't
>> the case, this defeats both WDIOF_MAGICCLOSE and watchdog_set_nowayout().
>> Are you sure this is what you want ? If so, please call
>> watchdog_stop_on_unregister() before registration; this clarifies that this
>> is what you want, and you can drop the remove function.
>>
>> Thanks,
>> Guenter
> 
> This patch does not change the behaviour. We always used that driver built-in; what
> should the behaviour be when unloading the module? Keep the watchdog hardware running
> if configured for 'nowayout'?
> 

One can still unload the driver. If you are fine with bypassing/dfeating nowayout
and WDIOF_MAGICCLOSE, may I ask why those are enabled in the first place ?

Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/7] watchdog: jz4740: Use devm_* functions

[Paul Cercueil]

Le jeu. 28 déc. 2017 à 21:19, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> a 
écrit :
> On 12/28/2017 11:59 AM, Paul Cercueil wrote:
>> Hi Guenter,
>> 
>> Le jeu. 28 déc. 2017 à 18:48, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> a 
>> écrit :
>>> On 12/28/2017 08:29 AM, Paul Cercueil wrote:
>>>> - Use devm_clk_get instead of clk_get
>>>> - Use devm_watchdog_register_device instead of 
>>>> watchdog_register_device
>>>> 
>>>> Signed-off-by: Paul Cercueil <paul-icTtO2rgO2OTuSrc4Mpeew@public.gmane.org>
>>>> ---
>>>>   drivers/watchdog/jz4740_wdt.c | 27 ++++++++-------------------
>>>>   1 file changed, 8 insertions(+), 19 deletions(-)
>>>> 
>>>> diff --git a/drivers/watchdog/jz4740_wdt.c 
>>>> b/drivers/watchdog/jz4740_wdt.c
>>>> index 6955deb100ef..92d6ca8ceb49 100644
>>>> --- a/drivers/watchdog/jz4740_wdt.c
>>>> +++ b/drivers/watchdog/jz4740_wdt.c
>>>> @@ -178,40 +178,29 @@ static int jz4740_wdt_probe(struct 
>>>> platform_device *pdev)
>>>>   \x7f      res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>>       drvdata->base = devm_ioremap_resource(&pdev->dev, res);
>>>> -    if (IS_ERR(drvdata->base)) {
>>>> -        ret = PTR_ERR(drvdata->base);
>>>> -        goto err_out;
>>>> -    }
>>>> +    if (IS_ERR(drvdata->base))
>>>> +        return PTR_ERR(drvdata->base);
>>>>   \x7f-    drvdata->rtc_clk = clk_get(&pdev->dev, "rtc");
>>>> +    drvdata->rtc_clk = devm_clk_get(&pdev->dev, "rtc");
>>>>       if (IS_ERR(drvdata->rtc_clk)) {
>>>>           dev_err(&pdev->dev, "cannot find RTC clock\n");
>>>> -        ret = PTR_ERR(drvdata->rtc_clk);
>>>> -        goto err_out;
>>>> +        return PTR_ERR(drvdata->rtc_clk);
>>>>       }
>>>>   \x7f-    ret = watchdog_register_device(&drvdata->wdt);
>>>> +    ret = devm_watchdog_register_device(&pdev->dev, 
>>>> &drvdata->wdt);
>>>>       if (ret < 0)
>>>> -        goto err_disable_clk;
>>>> +        return ret;
>>>>   \x7f      platform_set_drvdata(pdev, drvdata);
>>>> -    return 0;
>>>>   \x7f-err_disable_clk:
>>>> -    clk_put(drvdata->rtc_clk);
>>>> -err_out:
>>>> -    return ret;
>>>> +    return 0;
>>>>   }
>>>>   \x7f  static int jz4740_wdt_remove(struct platform_device *pdev)
>>>>   {
>>>>       struct jz4740_wdt_drvdata *drvdata = 
>>>> platform_get_drvdata(pdev);
>>>>   \x7f-    jz4740_wdt_stop(&drvdata->wdt);
>>>> -    watchdog_unregister_device(&drvdata->wdt);
>>>> -    clk_put(drvdata->rtc_clk);
>>>> -
>>>> -    return 0;
>>>> +    return jz4740_wdt_stop(&drvdata->wdt);
>>> 
>>> If the watchdog is running, the module can not be unloaded. Even if 
>>> that wasn't
>>> the case, this defeats both WDIOF_MAGICCLOSE and 
>>> watchdog_set_nowayout().
>>> Are you sure this is what you want ? If so, please call
>>> watchdog_stop_on_unregister() before registration; this clarifies 
>>> that this
>>> is what you want, and you can drop the remove function.
>>> 
>>> Thanks,
>>> Guenter
>> 
>> This patch does not change the behaviour. We always used that driver 
>> built-in; what
>> should the behaviour be when unloading the module? Keep the watchdog 
>> hardware running
>> if configured for 'nowayout'?
>> 
> 
> One can still unload the driver. If you are fine with 
> bypassing/dfeating nowayout
> and WDIOF_MAGICCLOSE, may I ask why those are enabled in the first 
> place ?
> 

Who knows? That code is very old :)
I'm fine with removing the remove() function completely, if you think 
it makes more sense.

Thanks,
-Paul

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/7] watchdog: jz4740: Use devm_* functions

[Guenter Roeck]

On 12/28/2017 12:22 PM, Paul Cercueil wrote:
> 
> 
> Le jeu. 28 déc. 2017 à 21:19, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> a écrit :
>> On 12/28/2017 11:59 AM, Paul Cercueil wrote:
>>> Hi Guenter,
>>>
>>> Le jeu. 28 déc. 2017 à 18:48, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> a écrit :
>>>> On 12/28/2017 08:29 AM, Paul Cercueil wrote:
>>>>> - Use devm_clk_get instead of clk_get
>>>>> - Use devm_watchdog_register_device instead of watchdog_register_device
>>>>>
>>>>> Signed-off-by: Paul Cercueil <paul-icTtO2rgO2OTuSrc4Mpeew@public.gmane.org>
>>>>> ---
>>>>>   drivers/watchdog/jz4740_wdt.c | 27 ++++++++-------------------
>>>>>   1 file changed, 8 insertions(+), 19 deletions(-)
>>>>>
>>>>> diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
>>>>> index 6955deb100ef..92d6ca8ceb49 100644
>>>>> --- a/drivers/watchdog/jz4740_wdt.c
>>>>> +++ b/drivers/watchdog/jz4740_wdt.c
>>>>> @@ -178,40 +178,29 @@ static int jz4740_wdt_probe(struct platform_device *pdev)
>>>>>   \x7f      res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>>>       drvdata->base = devm_ioremap_resource(&pdev->dev, res);
>>>>> -    if (IS_ERR(drvdata->base)) {
>>>>> -        ret = PTR_ERR(drvdata->base);
>>>>> -        goto err_out;
>>>>> -    }
>>>>> +    if (IS_ERR(drvdata->base))
>>>>> +        return PTR_ERR(drvdata->base);
>>>>>   \x7f-    drvdata->rtc_clk = clk_get(&pdev->dev, "rtc");
>>>>> +    drvdata->rtc_clk = devm_clk_get(&pdev->dev, "rtc");
>>>>>       if (IS_ERR(drvdata->rtc_clk)) {
>>>>>           dev_err(&pdev->dev, "cannot find RTC clock\n");
>>>>> -        ret = PTR_ERR(drvdata->rtc_clk);
>>>>> -        goto err_out;
>>>>> +        return PTR_ERR(drvdata->rtc_clk);
>>>>>       }
>>>>>   \x7f-    ret = watchdog_register_device(&drvdata->wdt);
>>>>> +    ret = devm_watchdog_register_device(&pdev->dev, &drvdata->wdt);
>>>>>       if (ret < 0)
>>>>> -        goto err_disable_clk;
>>>>> +        return ret;
>>>>>   \x7f      platform_set_drvdata(pdev, drvdata);
>>>>> -    return 0;
>>>>>   \x7f-err_disable_clk:
>>>>> -    clk_put(drvdata->rtc_clk);
>>>>> -err_out:
>>>>> -    return ret;
>>>>> +    return 0;
>>>>>   }
>>>>>   \x7f  static int jz4740_wdt_remove(struct platform_device *pdev)
>>>>>   {
>>>>>       struct jz4740_wdt_drvdata *drvdata = platform_get_drvdata(pdev);
>>>>>   \x7f-    jz4740_wdt_stop(&drvdata->wdt);
>>>>> -    watchdog_unregister_device(&drvdata->wdt);
>>>>> -    clk_put(drvdata->rtc_clk);
>>>>> -
>>>>> -    return 0;
>>>>> +    return jz4740_wdt_stop(&drvdata->wdt);
>>>>
>>>> If the watchdog is running, the module can not be unloaded. Even if that wasn't
>>>> the case, this defeats both WDIOF_MAGICCLOSE and watchdog_set_nowayout().
>>>> Are you sure this is what you want ? If so, please call
>>>> watchdog_stop_on_unregister() before registration; this clarifies that this
>>>> is what you want, and you can drop the remove function.
>>>>
>>>> Thanks,
>>>> Guenter
>>>
>>> This patch does not change the behaviour. We always used that driver built-in; what
>>> should the behaviour be when unloading the module? Keep the watchdog hardware running
>>> if configured for 'nowayout'?
>>>
>>
>> One can still unload the driver. If you are fine with bypassing/dfeating nowayout
>> and WDIOF_MAGICCLOSE, may I ask why those are enabled in the first place ?
>>
> 
> Who knows? That code is very old :)

Probably copied from some other driver w/o thinking much about it.

> I'm fine with removing the remove() function completely, if you think it makes more sense.
> 

Yes, I do, but I won't insist on it either.

Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/8] watchdog: JZ4740: Disable clock after stopping counter

[Paul Cercueil]

Previously, the clock was disabled first, which makes the watchdog
component insensitive to register writes.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/jz4740_wdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

 v2: No change

diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
index 20627f22baf6..6955deb100ef 100644
--- a/drivers/watchdog/jz4740_wdt.c
+++ b/drivers/watchdog/jz4740_wdt.c
@@ -124,8 +124,8 @@ static int jz4740_wdt_stop(struct watchdog_device *wdt_dev)
 {
 	struct jz4740_wdt_drvdata *drvdata = watchdog_get_drvdata(wdt_dev);
 
-	jz4740_timer_disable_watchdog();
 	writeb(0x0, drvdata->base + JZ_REG_WDT_COUNTER_ENABLE);
+	jz4740_timer_disable_watchdog();
 
 	return 0;
 }
-- 
2.11.0

[PATCH v2 2/8] watchdog: jz4740: Use devm_* functions

[Paul Cercueil]

- Use devm_clk_get instead of clk_get
- Use devm_watchdog_register_device instead of watchdog_register_device

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/watchdog/jz4740_wdt.c | 27 ++++++++-------------------
 1 file changed, 8 insertions(+), 19 deletions(-)

 v2: No change

diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
index 6955deb100ef..92d6ca8ceb49 100644
--- a/drivers/watchdog/jz4740_wdt.c
+++ b/drivers/watchdog/jz4740_wdt.c
@@ -178,40 +178,29 @@ static int jz4740_wdt_probe(struct platform_device *pdev)
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	drvdata->base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(drvdata->base)) {
-		ret = PTR_ERR(drvdata->base);
-		goto err_out;
-	}
+	if (IS_ERR(drvdata->base))
+		return PTR_ERR(drvdata->base);
 
-	drvdata->rtc_clk = clk_get(&pdev->dev, "rtc");
+	drvdata->rtc_clk = devm_clk_get(&pdev->dev, "rtc");
 	if (IS_ERR(drvdata->rtc_clk)) {
 		dev_err(&pdev->dev, "cannot find RTC clock\n");
-		ret = PTR_ERR(drvdata->rtc_clk);
-		goto err_out;
+		return PTR_ERR(drvdata->rtc_clk);
 	}
 
-	ret = watchdog_register_device(&drvdata->wdt);
+	ret = devm_watchdog_register_device(&pdev->dev, &drvdata->wdt);
 	if (ret < 0)
-		goto err_disable_clk;
+		return ret;
 
 	platform_set_drvdata(pdev, drvdata);
-	return 0;
 
-err_disable_clk:
-	clk_put(drvdata->rtc_clk);
-err_out:
-	return ret;
+	return 0;
 }
 
 static int jz4740_wdt_remove(struct platform_device *pdev)
 {
 	struct jz4740_wdt_drvdata *drvdata = platform_get_drvdata(pdev);
 
-	jz4740_wdt_stop(&drvdata->wdt);
-	watchdog_unregister_device(&drvdata->wdt);
-	clk_put(drvdata->rtc_clk);
-
-	return 0;
+	return jz4740_wdt_stop(&drvdata->wdt);
 }
 
 static struct platform_driver jz4740_wdt_driver = {
-- 
2.11.0

[PATCH v2 3/8] watchdog: JZ4740: Register a restart handler

[Paul Cercueil]

The watchdog driver can restart the system by simply configuring the
hardware for a timeout of 0 seconds.

Signed-off-by: Paul Cercueil <paul-icTtO2rgO2OTuSrc4Mpeew@public.gmane.org>
Reviewed-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
---
 drivers/watchdog/jz4740_wdt.c | 9 +++++++++
 1 file changed, 9 insertions(+)

 v2: No change

diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
index 92d6ca8ceb49..fa7f49a3212c 100644
--- a/drivers/watchdog/jz4740_wdt.c
+++ b/drivers/watchdog/jz4740_wdt.c
@@ -130,6 +130,14 @@ static int jz4740_wdt_stop(struct watchdog_device *wdt_dev)
 	return 0;
 }
 
+static int jz4740_wdt_restart(struct watchdog_device *wdt_dev,
+			      unsigned long action, void *data)
+{
+	wdt_dev->timeout = 0;
+	jz4740_wdt_start(wdt_dev);
+	return 0;
+}
+
 static const struct watchdog_info jz4740_wdt_info = {
 	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
 	.identity = "jz4740 Watchdog",
@@ -141,6 +149,7 @@ static const struct watchdog_ops jz4740_wdt_ops = {
 	.stop = jz4740_wdt_stop,
 	.ping = jz4740_wdt_ping,
 	.set_timeout = jz4740_wdt_set_timeout,
+	.restart = jz4740_wdt_restart,
 };
 
 #ifdef CONFIG_OF
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 4/8] watchdog: JZ4740: Drop module remove function

[Paul Cercueil]

When the watchdog was configured for nowayout, and after the
userspace watchdog daemon closed the dev node without sending the
magic character, unloading this module stopped the watchdog
hardware, which was clearly a problem.

Besides, unloading the module is not possible when the userspace
watchdog daemon is running, so it's safe to assume that we don't
need to stop the watchdog hardware in the jz4740_wdt_remove()
function.

For this reason, the jz4740_wdt_remove() function can then be
dropped alltogether.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/watchdog/jz4740_wdt.c | 8 --------
 1 file changed, 8 deletions(-)

 v2: New patch in this series

diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
index fa7f49a3212c..02b9b8e946a2 100644
--- a/drivers/watchdog/jz4740_wdt.c
+++ b/drivers/watchdog/jz4740_wdt.c
@@ -205,16 +205,8 @@ static int jz4740_wdt_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static int jz4740_wdt_remove(struct platform_device *pdev)
-{
-	struct jz4740_wdt_drvdata *drvdata = platform_get_drvdata(pdev);
-
-	return jz4740_wdt_stop(&drvdata->wdt);
-}
-
 static struct platform_driver jz4740_wdt_driver = {
 	.probe = jz4740_wdt_probe,
-	.remove = jz4740_wdt_remove,
 	.driver = {
 		.name = "jz4740-wdt",
 		.of_match_table = of_match_ptr(jz4740_wdt_of_matches),
-- 
2.11.0

[PATCH v2 5/8] MIPS: jz4740: dts: Add bindings for the jz4740-wdt driver

[Paul Cercueil]

Also remove the watchdog platform_device from platform.c, since it
wasn't used anywhere anyway.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 arch/mips/boot/dts/ingenic/jz4740.dtsi |  8 ++++++++
 arch/mips/jz4740/platform.c            | 16 ----------------
 2 files changed, 8 insertions(+), 16 deletions(-)

 v2: No change

diff --git a/arch/mips/boot/dts/ingenic/jz4740.dtsi b/arch/mips/boot/dts/ingenic/jz4740.dtsi
index cd5185bb90ae..26c6b561d6f7 100644
--- a/arch/mips/boot/dts/ingenic/jz4740.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4740.dtsi
@@ -45,6 +45,14 @@
 		#clock-cells = <1>;
 	};
 
+	watchdog: watchdog@10002000 {
+		compatible = "ingenic,jz4740-watchdog";
+		reg = <0x10002000 0x10>;
+
+		clocks = <&cgu JZ4740_CLK_RTC>;
+		clock-names = "rtc";
+	};
+
 	rtc_dev: rtc@10003000 {
 		compatible = "ingenic,jz4740-rtc";
 		reg = <0x10003000 0x40>;
diff --git a/arch/mips/jz4740/platform.c b/arch/mips/jz4740/platform.c
index 5b7cdd67a9d9..cbc5f8e87230 100644
--- a/arch/mips/jz4740/platform.c
+++ b/arch/mips/jz4740/platform.c
@@ -233,22 +233,6 @@ struct platform_device jz4740_adc_device = {
 	.resource	= jz4740_adc_resources,
 };
 
-/* Watchdog */
-static struct resource jz4740_wdt_resources[] = {
-	{
-		.start = JZ4740_WDT_BASE_ADDR,
-		.end   = JZ4740_WDT_BASE_ADDR + 0x10 - 1,
-		.flags = IORESOURCE_MEM,
-	},
-};
-
-struct platform_device jz4740_wdt_device = {
-	.name	       = "jz4740-wdt",
-	.id	       = -1,
-	.num_resources = ARRAY_SIZE(jz4740_wdt_resources),
-	.resource      = jz4740_wdt_resources,
-};
-
 /* PWM */
 struct platform_device jz4740_pwm_device = {
 	.name = "jz4740-pwm",
-- 
2.11.0

[PATCH v2 6/8] MIPS: jz4780: dts: Fix watchdog node

[Paul Cercueil]

- The previous node requested a memory area of 0x100 bytes, while the
  driver only manipulates four registers present in the first 0x10 bytes.

- The driver requests for the "rtc" clock, but the previous node did not
  provide any.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Reviewed-by: Mathieu Malaterre <malat@debian.org>
---
 arch/mips/boot/dts/ingenic/jz4780.dtsi | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

 v2: No change

diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi
index 9b5794667aee..a52f59bf58c7 100644
--- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
@@ -221,7 +221,10 @@
 
 	watchdog: watchdog@10002000 {
 		compatible = "ingenic,jz4780-watchdog";
-		reg = <0x10002000 0x100>;
+		reg = <0x10002000 0x10>;
+
+		clocks = <&cgu JZ4780_CLK_RTCLK>;
+		clock-names = "rtc";
 	};
 
 	nemc: nemc@13410000 {
-- 
2.11.0

[PATCH v2 7/8] MIPS: qi_lb60: Enable the jz4740-wdt driver

[Paul Cercueil]

The watchdog is an useful piece of hardware, so there's no reason not to
enable it.

This commit enables the Kconfig option in the qi_lb60 defconfig.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 arch/mips/configs/qi_lb60_defconfig | 2 ++
 1 file changed, 2 insertions(+)

 v2: No change

diff --git a/arch/mips/configs/qi_lb60_defconfig b/arch/mips/configs/qi_lb60_defconfig
index 3f1333517405..ba8e1c56b626 100644
--- a/arch/mips/configs/qi_lb60_defconfig
+++ b/arch/mips/configs/qi_lb60_defconfig
@@ -73,6 +73,8 @@ CONFIG_POWER_SUPPLY=y
 CONFIG_BATTERY_JZ4740=y
 CONFIG_CHARGER_GPIO=y
 # CONFIG_HWMON is not set
+CONFIG_WATCHDOG=y
+CONFIG_JZ4740_WDT=y
 CONFIG_MFD_JZ4740_ADC=y
 CONFIG_REGULATOR=y
 CONFIG_REGULATOR_FIXED_VOLTAGE=y
-- 
2.11.0

[PATCH v2 8/8] MIPS: jz4740: Drop old platform reset code

[Paul Cercueil]

This work is now performed by the watchdog driver directly.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 arch/mips/jz4740/reset.c | 31 -------------------------------
 1 file changed, 31 deletions(-)

 v2: No change

diff --git a/arch/mips/jz4740/reset.c b/arch/mips/jz4740/reset.c
index 67780c4b6573..5bf0cf44b55f 100644
--- a/arch/mips/jz4740/reset.c
+++ b/arch/mips/jz4740/reset.c
@@ -12,18 +12,9 @@
  *
  */
 
-#include <linux/clk.h>
-#include <linux/io.h>
-#include <linux/kernel.h>
-#include <linux/pm.h>
-
 #include <asm/reboot.h>
 
-#include <asm/mach-jz4740/base.h>
-#include <asm/mach-jz4740/timer.h>
-
 #include "reset.h"
-#include "clock.h"
 
 static void jz4740_halt(void)
 {
@@ -36,29 +27,7 @@ static void jz4740_halt(void)
 	}
 }
 
-#define JZ_REG_WDT_DATA 0x00
-#define JZ_REG_WDT_COUNTER_ENABLE 0x04
-#define JZ_REG_WDT_COUNTER 0x08
-#define JZ_REG_WDT_CTRL 0x0c
-
-static void jz4740_restart(char *command)
-{
-	void __iomem *wdt_base = ioremap(JZ4740_WDT_BASE_ADDR, 0x0f);
-
-	jz4740_timer_enable_watchdog();
-
-	writeb(0, wdt_base + JZ_REG_WDT_COUNTER_ENABLE);
-
-	writew(0, wdt_base + JZ_REG_WDT_COUNTER);
-	writew(0, wdt_base + JZ_REG_WDT_DATA);
-	writew(BIT(2), wdt_base + JZ_REG_WDT_CTRL);
-
-	writeb(1, wdt_base + JZ_REG_WDT_COUNTER_ENABLE);
-	jz4740_halt();
-}
-
 void jz4740_reset_init(void)
 {
-	_machine_restart = jz4740_restart;
 	_machine_halt = jz4740_halt;
 }
-- 
2.11.0

Re: [PATCH v2 2/8] watchdog: jz4740: Use devm_* functions

[Guenter Roeck]

On 12/30/2017 05:51 AM, Paul Cercueil wrote:
> - Use devm_clk_get instead of clk_get
> - Use devm_watchdog_register_device instead of watchdog_register_device
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/watchdog/jz4740_wdt.c | 27 ++++++++-------------------
>   1 file changed, 8 insertions(+), 19 deletions(-)
> 
>   v2: No change
> 
> diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
> index 6955deb100ef..92d6ca8ceb49 100644
> --- a/drivers/watchdog/jz4740_wdt.c
> +++ b/drivers/watchdog/jz4740_wdt.c
> @@ -178,40 +178,29 @@ static int jz4740_wdt_probe(struct platform_device *pdev)
>   
>   	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>   	drvdata->base = devm_ioremap_resource(&pdev->dev, res);
> -	if (IS_ERR(drvdata->base)) {
> -		ret = PTR_ERR(drvdata->base);
> -		goto err_out;
> -	}
> +	if (IS_ERR(drvdata->base))
> +		return PTR_ERR(drvdata->base);
>   
> -	drvdata->rtc_clk = clk_get(&pdev->dev, "rtc");
> +	drvdata->rtc_clk = devm_clk_get(&pdev->dev, "rtc");
>   	if (IS_ERR(drvdata->rtc_clk)) {
>   		dev_err(&pdev->dev, "cannot find RTC clock\n");
> -		ret = PTR_ERR(drvdata->rtc_clk);
> -		goto err_out;
> +		return PTR_ERR(drvdata->rtc_clk);
>   	}
>   
> -	ret = watchdog_register_device(&drvdata->wdt);
> +	ret = devm_watchdog_register_device(&pdev->dev, &drvdata->wdt);
>   	if (ret < 0)
> -		goto err_disable_clk;
> +		return ret;
>   
>   	platform_set_drvdata(pdev, drvdata);
> -	return 0;
>   
> -err_disable_clk:
> -	clk_put(drvdata->rtc_clk);
> -err_out:
> -	return ret;
> +	return 0;
>   }
>   
>   static int jz4740_wdt_remove(struct platform_device *pdev)
>   {
>   	struct jz4740_wdt_drvdata *drvdata = platform_get_drvdata(pdev);
>   
> -	jz4740_wdt_stop(&drvdata->wdt);
> -	watchdog_unregister_device(&drvdata->wdt);
> -	clk_put(drvdata->rtc_clk);
> -
> -	return 0;
> +	return jz4740_wdt_stop(&drvdata->wdt);
>   }
>   
>   static struct platform_driver jz4740_wdt_driver = {
> 

Re: [PATCH v2 4/8] watchdog: JZ4740: Drop module remove function

[Guenter Roeck]

On 12/30/2017 05:51 AM, Paul Cercueil wrote:
> When the watchdog was configured for nowayout, and after the
> userspace watchdog daemon closed the dev node without sending the
> magic character, unloading this module stopped the watchdog
> hardware, which was clearly a problem.
> 
> Besides, unloading the module is not possible when the userspace
> watchdog daemon is running, so it's safe to assume that we don't
> need to stop the watchdog hardware in the jz4740_wdt_remove()
> function.
> 
> For this reason, the jz4740_wdt_remove() function can then be
> dropped alltogether.
> 
> Signed-off-by: Paul Cercueil <paul-icTtO2rgO2OTuSrc4Mpeew@public.gmane.org>

Reviewed-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>

> ---
>   drivers/watchdog/jz4740_wdt.c | 8 --------
>   1 file changed, 8 deletions(-)
> 
>   v2: New patch in this series
> 
> diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
> index fa7f49a3212c..02b9b8e946a2 100644
> --- a/drivers/watchdog/jz4740_wdt.c
> +++ b/drivers/watchdog/jz4740_wdt.c
> @@ -205,16 +205,8 @@ static int jz4740_wdt_probe(struct platform_device *pdev)
>   	return 0;
>   }
>   
> -static int jz4740_wdt_remove(struct platform_device *pdev)
> -{
> -	struct jz4740_wdt_drvdata *drvdata = platform_get_drvdata(pdev);
> -
> -	return jz4740_wdt_stop(&drvdata->wdt);
> -}
> -
>   static struct platform_driver jz4740_wdt_driver = {
>   	.probe = jz4740_wdt_probe,
> -	.remove = jz4740_wdt_remove,
>   	.driver = {
>   		.name = "jz4740-wdt",
>   		.of_match_table = of_match_ptr(jz4740_wdt_of_matches),
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/8] MIPS: jz4740: dts: Add bindings for the jz4740-wdt driver

[PrasannaKumar Muralidharan]

Hi Paul,

On 30 December 2017 at 19:21, Paul Cercueil <paul-icTtO2rgO2OTuSrc4Mpeew@public.gmane.org> wrote:
> Also remove the watchdog platform_device from platform.c, since it
> wasn't used anywhere anyway.
>
> Signed-off-by: Paul Cercueil <paul-icTtO2rgO2OTuSrc4Mpeew@public.gmane.org>
> ---
>  arch/mips/boot/dts/ingenic/jz4740.dtsi |  8 ++++++++
>  arch/mips/jz4740/platform.c            | 16 ----------------
>  2 files changed, 8 insertions(+), 16 deletions(-)
>
>  v2: No change
>
> diff --git a/arch/mips/boot/dts/ingenic/jz4740.dtsi b/arch/mips/boot/dts/ingenic/jz4740.dtsi
> index cd5185bb90ae..26c6b561d6f7 100644
> --- a/arch/mips/boot/dts/ingenic/jz4740.dtsi
> +++ b/arch/mips/boot/dts/ingenic/jz4740.dtsi
> @@ -45,6 +45,14 @@
>                 #clock-cells = <1>;
>         };
>
> +       watchdog: watchdog@10002000 {
> +               compatible = "ingenic,jz4740-watchdog";
> +               reg = <0x10002000 0x10>;
> +
> +               clocks = <&cgu JZ4740_CLK_RTC>;
> +               clock-names = "rtc";
> +       };
> +

The watchdog driver calls jz4740_timer_enable_watchdog and
jz4740_timer_disable_watchdog which defined in
arch/mips/jz4740/timer.c. It accesses registers iomapped by timer
code. Declaring register size as 0x10 does not show the real picture.
Better use register size as 0x100 and let timer, wdt, pwm drivers to
share them.

Code from one of your branches
(https://github.com/OpenDingux/linux/blob/for-upstream-clocksource/arch/mips/boot/dts/ingenic/jz4740.dtsi)
does it. Can you prepare a patch series and send it?
I have a patch set that moves timer code out of arch/mips/jz4740/ and
does a similar thing for watchdog and pwm. As your new timer driver is
better than the existing one I have not sent my patches yet. I would
like to see it getting mainlined as it paves way for removing most of
code in arch/mips/jz4740.

>         rtc_dev: rtc@10003000 {
>                 compatible = "ingenic,jz4740-rtc";
>                 reg = <0x10003000 0x40>;
> diff --git a/arch/mips/jz4740/platform.c b/arch/mips/jz4740/platform.c
> index 5b7cdd67a9d9..cbc5f8e87230 100644
> --- a/arch/mips/jz4740/platform.c
> +++ b/arch/mips/jz4740/platform.c
> @@ -233,22 +233,6 @@ struct platform_device jz4740_adc_device = {
>         .resource       = jz4740_adc_resources,
>  };
>
> -/* Watchdog */
> -static struct resource jz4740_wdt_resources[] = {
> -       {
> -               .start = JZ4740_WDT_BASE_ADDR,
> -               .end   = JZ4740_WDT_BASE_ADDR + 0x10 - 1,
> -               .flags = IORESOURCE_MEM,
> -       },
> -};
> -
> -struct platform_device jz4740_wdt_device = {
> -       .name          = "jz4740-wdt",
> -       .id            = -1,
> -       .num_resources = ARRAY_SIZE(jz4740_wdt_resources),
> -       .resource      = jz4740_wdt_resources,
> -};
> -
>  /* PWM */
>  struct platform_device jz4740_pwm_device = {
>         .name = "jz4740-pwm",
> --
> 2.11.0
>
>

Regards,
PrasannaKumar
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/8] MIPS: jz4740: dts: Add bindings for the jz4740-wdt driver

[Paul Cercueil]

Hi PrasannaKumar,

Le mar. 2 janv. 2018 à 17:37, PrasannaKumar Muralidharan 
<prasannatsmkumar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> a écrit :
> Hi Paul,
> 
> On 30 December 2017 at 19:21, Paul Cercueil <paul-icTtO2rgO2OTuSrc4Mpeew@public.gmane.org> 
> wrote:
>>  Also remove the watchdog platform_device from platform.c, since it
>>  wasn't used anywhere anyway.
>> 
>>  Signed-off-by: Paul Cercueil <paul-icTtO2rgO2OTuSrc4Mpeew@public.gmane.org>
>>  ---
>>   arch/mips/boot/dts/ingenic/jz4740.dtsi |  8 ++++++++
>>   arch/mips/jz4740/platform.c            | 16 ----------------
>>   2 files changed, 8 insertions(+), 16 deletions(-)
>> 
>>   v2: No change
>> 
>>  diff --git a/arch/mips/boot/dts/ingenic/jz4740.dtsi 
>> b/arch/mips/boot/dts/ingenic/jz4740.dtsi
>>  index cd5185bb90ae..26c6b561d6f7 100644
>>  --- a/arch/mips/boot/dts/ingenic/jz4740.dtsi
>>  +++ b/arch/mips/boot/dts/ingenic/jz4740.dtsi
>>  @@ -45,6 +45,14 @@
>>                  #clock-cells = <1>;
>>          };
>> 
>>  +       watchdog: watchdog@10002000 {
>>  +               compatible = "ingenic,jz4740-watchdog";
>>  +               reg = <0x10002000 0x10>;
>>  +
>>  +               clocks = <&cgu JZ4740_CLK_RTC>;
>>  +               clock-names = "rtc";
>>  +       };
>>  +
> 
> The watchdog driver calls jz4740_timer_enable_watchdog and
> jz4740_timer_disable_watchdog which defined in
> arch/mips/jz4740/timer.c. It accesses registers iomapped by timer
> code. Declaring register size as 0x10 does not show the real picture.
> Better use register size as 0x100 and let timer, wdt, pwm drivers to
> share them.

As you said, it accesses registers iomapped by timer code. So the 
watchdog
driver doesn't need to iomap them.

> Code from one of your branches
> (https://github.com/OpenDingux/linux/blob/for-upstream-clocksource/arch/mips/boot/dts/ingenic/jz4740.dtsi)
> does it. Can you prepare a patch series and send it?
> I have a patch set that moves timer code out of arch/mips/jz4740/ and
> does a similar thing for watchdog and pwm. As your new timer driver is
> better than the existing one I have not sent my patches yet. I would
> like to see it getting mainlined as it paves way for removing most of
> code in arch/mips/jz4740.

The whole 'for-upstream-clocksource' branch is supposed to go upstream,
but I can't do it in one big patchset without having lots of breakages 
with
my other patchsets (jz4770 SoC support, and jz4740 watchdog updates)
currently under review. That also makes it simpler to upstream than 
having
one single patchset that touches 6 different frameworks (MIPS, irq, 
clocks,
clocksource, watchdog, PWM).

So I will submit it in two steps, first the irq/clocks/clocksource 
drivers
(this patchset) hopefully for 4.16, and then the platform/watchdog/PWM 
fixes
for 4.17.

Kind regards,
-Paul

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/8] MIPS: jz4740: dts: Add bindings for the jz4740-wdt driver

[Guenter Roeck]

On 01/02/2018 08:48 AM, Paul Cercueil wrote:
> Hi PrasannaKumar,
> 
> Le mar. 2 janv. 2018 à 17:37, PrasannaKumar Muralidharan <prasannatsmkumar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> a écrit :
>> Hi Paul,
>>
>> On 30 December 2017 at 19:21, Paul Cercueil <paul-icTtO2rgO2OTuSrc4Mpeew@public.gmane.org> wrote:
>>>  Also remove the watchdog platform_device from platform.c, since it
>>>  wasn't used anywhere anyway.
>>>
>>>  Signed-off-by: Paul Cercueil <paul-icTtO2rgO2OTuSrc4Mpeew@public.gmane.org>
>>>  ---
>>>   arch/mips/boot/dts/ingenic/jz4740.dtsi |  8 ++++++++
>>>   arch/mips/jz4740/platform.c            | 16 ----------------
>>>   2 files changed, 8 insertions(+), 16 deletions(-)
>>>
>>>   v2: No change
>>>
>>>  diff --git a/arch/mips/boot/dts/ingenic/jz4740.dtsi b/arch/mips/boot/dts/ingenic/jz4740.dtsi
>>>  index cd5185bb90ae..26c6b561d6f7 100644
>>>  --- a/arch/mips/boot/dts/ingenic/jz4740.dtsi
>>>  +++ b/arch/mips/boot/dts/ingenic/jz4740.dtsi
>>>  @@ -45,6 +45,14 @@
>>>                  #clock-cells = <1>;
>>>          };
>>>
>>>  +       watchdog: watchdog@10002000 {
>>>  +               compatible = "ingenic,jz4740-watchdog";
>>>  +               reg = <0x10002000 0x10>;
>>>  +
>>>  +               clocks = <&cgu JZ4740_CLK_RTC>;
>>>  +               clock-names = "rtc";
>>>  +       };
>>>  +
>>
>> The watchdog driver calls jz4740_timer_enable_watchdog and
>> jz4740_timer_disable_watchdog which defined in
>> arch/mips/jz4740/timer.c. It accesses registers iomapped by timer
>> code. Declaring register size as 0x10 does not show the real picture.
>> Better use register size as 0x100 and let timer, wdt, pwm drivers to
>> share them.
> 
> As you said, it accesses registers iomapped by timer code. So the watchdog
> driver doesn't need to iomap them.
> 
>> Code from one of your branches
>> (https://github.com/OpenDingux/linux/blob/for-upstream-clocksource/arch/mips/boot/dts/ingenic/jz4740.dtsi)
>> does it. Can you prepare a patch series and send it?
>> I have a patch set that moves timer code out of arch/mips/jz4740/ and
>> does a similar thing for watchdog and pwm. As your new timer driver is
>> better than the existing one I have not sent my patches yet. I would
>> like to see it getting mainlined as it paves way for removing most of
>> code in arch/mips/jz4740.
> 
> The whole 'for-upstream-clocksource' branch is supposed to go upstream,
> but I can't do it in one big patchset without having lots of breakages with
> my other patchsets (jz4770 SoC support, and jz4740 watchdog updates)
> currently under review. That also makes it simpler to upstream than having
> one single patchset that touches 6 different frameworks (MIPS, irq, clocks,
> clocksource, watchdog, PWM).
> 
> So I will submit it in two steps, first the irq/clocks/clocksource drivers
> (this patchset) hopefully for 4.16, and then the platform/watchdog/PWM fixes
> for 4.17.
> 

I kind of lost it in this exchange, sorry. At this point I don't know if something
is wrong with the watchdog patches, and I have no clue what the upstream path
is supposed to be. My working assumption is that 1) something may be wrong with
the current version of the patches, and, 2), even if not, none of the patches
is expected to find its way upstream through the watchdog subsystem. Plus, 3),
even if some of the patches are supposed to go upstream through the watchdog
subsystem, that won't happen in 4.16, and the patches will be resubmitted later
when they are ready [and will hopefully marked clearly for submission through
the watchdog subsystem].

With that in mind, I'll mark the series for my reference as "not applicable".
If this is wrong please let me know.

Guenter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/8] MIPS: jz4740: dts: Add bindings for the jz4740-wdt driver

[Paul Cercueil]

Le mer. 3 janv. 2018 à 5:46, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> a 
écrit :
> On 01/02/2018 08:48 AM, Paul Cercueil wrote:
>> Hi PrasannaKumar,
>> 
>> Le mar. 2 janv. 2018 à 17:37, PrasannaKumar Muralidharan 
>> <prasannatsmkumar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> a écrit :
>>> Hi Paul,
>>> 
>>> On 30 December 2017 at 19:21, Paul Cercueil <paul-icTtO2rgO2OTuSrc4Mpeew@public.gmane.org> 
>>> wrote:
>>>>  Also remove the watchdog platform_device from platform.c, since it
>>>>  wasn't used anywhere anyway.
>>>> 
>>>>  Signed-off-by: Paul Cercueil <paul-icTtO2rgO2OTuSrc4Mpeew@public.gmane.org>
>>>>  ---
>>>>   arch/mips/boot/dts/ingenic/jz4740.dtsi |  8 ++++++++
>>>>   arch/mips/jz4740/platform.c            | 16 ----------------
>>>>   2 files changed, 8 insertions(+), 16 deletions(-)
>>>> 
>>>>   v2: No change
>>>> 
>>>>  diff --git a/arch/mips/boot/dts/ingenic/jz4740.dtsi 
>>>> b/arch/mips/boot/dts/ingenic/jz4740.dtsi
>>>>  index cd5185bb90ae..26c6b561d6f7 100644
>>>>  --- a/arch/mips/boot/dts/ingenic/jz4740.dtsi
>>>>  +++ b/arch/mips/boot/dts/ingenic/jz4740.dtsi
>>>>  @@ -45,6 +45,14 @@
>>>>                  #clock-cells = <1>;
>>>>          };
>>>> 
>>>>  +       watchdog: watchdog@10002000 {
>>>>  +               compatible = "ingenic,jz4740-watchdog";
>>>>  +               reg = <0x10002000 0x10>;
>>>>  +
>>>>  +               clocks = <&cgu JZ4740_CLK_RTC>;
>>>>  +               clock-names = "rtc";
>>>>  +       };
>>>>  +
>>> 
>>> The watchdog driver calls jz4740_timer_enable_watchdog and
>>> jz4740_timer_disable_watchdog which defined in
>>> arch/mips/jz4740/timer.c. It accesses registers iomapped by timer
>>> code. Declaring register size as 0x10 does not show the real 
>>> picture.
>>> Better use register size as 0x100 and let timer, wdt, pwm drivers to
>>> share them.
>> 
>> As you said, it accesses registers iomapped by timer code. So the 
>> watchdog
>> driver doesn't need to iomap them.
>> 
>>> Code from one of your branches
>>> (https://github.com/OpenDingux/linux/blob/for-upstream-clocksource/arch/mips/boot/dts/ingenic/jz4740.dtsi)
>>> does it. Can you prepare a patch series and send it?
>>> I have a patch set that moves timer code out of arch/mips/jz4740/ 
>>> and
>>> does a similar thing for watchdog and pwm. As your new timer driver 
>>> is
>>> better than the existing one I have not sent my patches yet. I would
>>> like to see it getting mainlined as it paves way for removing most 
>>> of
>>> code in arch/mips/jz4740.
>> 
>> The whole 'for-upstream-clocksource' branch is supposed to go 
>> upstream,
>> but I can't do it in one big patchset without having lots of 
>> breakages with
>> my other patchsets (jz4770 SoC support, and jz4740 watchdog updates)
>> currently under review. That also makes it simpler to upstream than 
>> having
>> one single patchset that touches 6 different frameworks (MIPS, irq, 
>> clocks,
>> clocksource, watchdog, PWM).
>> 
>> So I will submit it in two steps, first the irq/clocks/clocksource 
>> drivers
>> (this patchset) hopefully for 4.16, and then the 
>> platform/watchdog/PWM fixes
>> for 4.17.
>> 
> 
> I kind of lost it in this exchange, sorry. At this point I don't know 
> if something
> is wrong with the watchdog patches, and I have no clue what the 
> upstream path
> is supposed to be. My working assumption is that 1) something may be 
> wrong with
> the current version of the patches, and, 2), even if not, none of the 
> patches
> is expected to find its way upstream through the watchdog subsystem. 
> Plus, 3),
> even if some of the patches are supposed to go upstream through the 
> watchdog
> subsystem, that won't happen in 4.16, and the patches will be 
> resubmitted later
> when they are ready [and will hopefully marked clearly for submission 
> through
> the watchdog subsystem].
> 
> With that in mind, I'll mark the series for my reference as "not 
> applicable".
> If this is wrong please let me know.
> 
> Guenter

Sorry, my fault, PrasannaKumar mentionned my 'for-upstream-clocksource' 
branch requesting
me to submit it upstream, which I am doing in parallel of this one. I 
thought I was
answering him in the other patchset's thread, hence the confusion.

There is nothing wrong with these watchdog patches. Upstream path is 
through the MIPS tree.

Paul

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/8] MIPS: jz4740: dts: Add bindings for the jz4740-wdt driver

[PrasannaKumar Muralidharan]

Hi Guenter,

On 3 January 2018 at 10:16, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
> On 01/02/2018 08:48 AM, Paul Cercueil wrote:
>>
>> Hi PrasannaKumar,
>>
>> Le mar. 2 janv. 2018 à 17:37, PrasannaKumar Muralidharan
>> <prasannatsmkumar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> a écrit :
>>>
>>> Hi Paul,
>>>
>>> On 30 December 2017 at 19:21, Paul Cercueil <paul-icTtO2rgO2OTuSrc4Mpeew@public.gmane.org> wrote:
>>>>
>>>>  Also remove the watchdog platform_device from platform.c, since it
>>>>  wasn't used anywhere anyway.
>>>>
>>>>  Signed-off-by: Paul Cercueil <paul-icTtO2rgO2OTuSrc4Mpeew@public.gmane.org>
>>>>  ---
>>>>   arch/mips/boot/dts/ingenic/jz4740.dtsi |  8 ++++++++
>>>>   arch/mips/jz4740/platform.c            | 16 ----------------
>>>>   2 files changed, 8 insertions(+), 16 deletions(-)
>>>>
>>>>   v2: No change
>>>>
>>>>  diff --git a/arch/mips/boot/dts/ingenic/jz4740.dtsi
>>>> b/arch/mips/boot/dts/ingenic/jz4740.dtsi
>>>>  index cd5185bb90ae..26c6b561d6f7 100644
>>>>  --- a/arch/mips/boot/dts/ingenic/jz4740.dtsi
>>>>  +++ b/arch/mips/boot/dts/ingenic/jz4740.dtsi
>>>>  @@ -45,6 +45,14 @@
>>>>                  #clock-cells = <1>;
>>>>          };
>>>>
>>>>  +       watchdog: watchdog@10002000 {
>>>>  +               compatible = "ingenic,jz4740-watchdog";
>>>>  +               reg = <0x10002000 0x10>;
>>>>  +
>>>>  +               clocks = <&cgu JZ4740_CLK_RTC>;
>>>>  +               clock-names = "rtc";
>>>>  +       };
>>>>  +
>>>
>>>
>>> The watchdog driver calls jz4740_timer_enable_watchdog and
>>> jz4740_timer_disable_watchdog which defined in
>>> arch/mips/jz4740/timer.c. It accesses registers iomapped by timer
>>> code. Declaring register size as 0x10 does not show the real picture.
>>> Better use register size as 0x100 and let timer, wdt, pwm drivers to
>>> share them.
>>
>>
>> As you said, it accesses registers iomapped by timer code. So the watchdog
>> driver doesn't need to iomap them.
>>
>>> Code from one of your branches
>>>
>>> (https://github.com/OpenDingux/linux/blob/for-upstream-clocksource/arch/mips/boot/dts/ingenic/jz4740.dtsi)
>>> does it. Can you prepare a patch series and send it?
>>> I have a patch set that moves timer code out of arch/mips/jz4740/ and
>>> does a similar thing for watchdog and pwm. As your new timer driver is
>>> better than the existing one I have not sent my patches yet. I would
>>> like to see it getting mainlined as it paves way for removing most of
>>> code in arch/mips/jz4740.
>>
>>
>> The whole 'for-upstream-clocksource' branch is supposed to go upstream,
>> but I can't do it in one big patchset without having lots of breakages
>> with
>> my other patchsets (jz4770 SoC support, and jz4740 watchdog updates)
>> currently under review. That also makes it simpler to upstream than having
>> one single patchset that touches 6 different frameworks (MIPS, irq,
>> clocks,
>> clocksource, watchdog, PWM).
>>
>> So I will submit it in two steps, first the irq/clocks/clocksource drivers
>> (this patchset) hopefully for 4.16, and then the platform/watchdog/PWM
>> fixes
>> for 4.17.
>>
>
> I kind of lost it in this exchange, sorry. At this point I don't know if
> something
> is wrong with the watchdog patches, and I have no clue what the upstream
> path

There is nothing wrong in this watchdog patches.

> is supposed to be. My working assumption is that 1) something may be wrong
> with
> the current version of the patches, and, 2), even if not, none of the
> patches
> is expected to find its way upstream through the watchdog subsystem. Plus,
> 3),
> even if some of the patches are supposed to go upstream through the watchdog
> subsystem, that won't happen in 4.16, and the patches will be resubmitted
> later
> when they are ready [and will hopefully marked clearly for submission
> through
> the watchdog subsystem].
>
> With that in mind, I'll mark the series for my reference as "not
> applicable".
> If this is wrong please let me know.

Paul has patches related to timer code. While sending that he would
send changes to watchdog dts entry also. I was suggesting him to send
timer patches together with watchdog patches as a single patch series
but he prefers to send them as separate patch series.

I would like to reiterate that there is nothing wrong with this
watchdog patches. I should have set the correct context in my previous
email itself. I sincerely apologize for creating this confusion.

Regards,
PrasannaKumar
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/8] watchdog: JZ4740: Register a restart handler

[PrasannaKumar Muralidharan]

Hi Paul,

On 30 December 2017 at 19:21, Paul Cercueil <paul-icTtO2rgO2OTuSrc4Mpeew@public.gmane.org> wrote:
> The watchdog driver can restart the system by simply configuring the
> hardware for a timeout of 0 seconds.
>
> Signed-off-by: Paul Cercueil <paul-icTtO2rgO2OTuSrc4Mpeew@public.gmane.org>
> Reviewed-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
> ---
>  drivers/watchdog/jz4740_wdt.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
>  v2: No change
>
> diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
> index 92d6ca8ceb49..fa7f49a3212c 100644
> --- a/drivers/watchdog/jz4740_wdt.c
> +++ b/drivers/watchdog/jz4740_wdt.c
> @@ -130,6 +130,14 @@ static int jz4740_wdt_stop(struct watchdog_device *wdt_dev)
>         return 0;
>  }
>
> +static int jz4740_wdt_restart(struct watchdog_device *wdt_dev,
> +                             unsigned long action, void *data)
> +{
> +       wdt_dev->timeout = 0;
> +       jz4740_wdt_start(wdt_dev);
> +       return 0;
> +}
> +
>  static const struct watchdog_info jz4740_wdt_info = {
>         .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
>         .identity = "jz4740 Watchdog",
> @@ -141,6 +149,7 @@ static const struct watchdog_ops jz4740_wdt_ops = {
>         .stop = jz4740_wdt_stop,
>         .ping = jz4740_wdt_ping,
>         .set_timeout = jz4740_wdt_set_timeout,
> +       .restart = jz4740_wdt_restart,
>  };
>
>  #ifdef CONFIG_OF
> --
> 2.11.0
>
>

Noticed that min_timeout of the watchdog device is set to 1 but this
function calls start with timeout set to 0. Even though this works I
feel it is better to set min_timeout to 0.

Regards,
PrasannaKumar
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/8] watchdog: JZ4740: Drop module remove function

[PrasannaKumar Muralidharan]

Hi Paul,

On 30 December 2017 at 19:21, Paul Cercueil <paul-icTtO2rgO2OTuSrc4Mpeew@public.gmane.org> wrote:
> When the watchdog was configured for nowayout, and after the
> userspace watchdog daemon closed the dev node without sending the
> magic character, unloading this module stopped the watchdog
> hardware, which was clearly a problem.
>
> Besides, unloading the module is not possible when the userspace
> watchdog daemon is running, so it's safe to assume that we don't
> need to stop the watchdog hardware in the jz4740_wdt_remove()
> function.
>
> For this reason, the jz4740_wdt_remove() function can then be
> dropped alltogether.
>
> Signed-off-by: Paul Cercueil <paul-icTtO2rgO2OTuSrc4Mpeew@public.gmane.org>
> ---
>  drivers/watchdog/jz4740_wdt.c | 8 --------
>  1 file changed, 8 deletions(-)
>
>  v2: New patch in this series
>
> diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
> index fa7f49a3212c..02b9b8e946a2 100644
> --- a/drivers/watchdog/jz4740_wdt.c
> +++ b/drivers/watchdog/jz4740_wdt.c
> @@ -205,16 +205,8 @@ static int jz4740_wdt_probe(struct platform_device *pdev)
>         return 0;
>  }
>
> -static int jz4740_wdt_remove(struct platform_device *pdev)
> -{
> -       struct jz4740_wdt_drvdata *drvdata = platform_get_drvdata(pdev);
> -
> -       return jz4740_wdt_stop(&drvdata->wdt);
> -}
> -
>  static struct platform_driver jz4740_wdt_driver = {
>         .probe = jz4740_wdt_probe,
> -       .remove = jz4740_wdt_remove,
>         .driver = {
>                 .name = "jz4740-wdt",
>                 .of_match_table = of_match_ptr(jz4740_wdt_of_matches),
> --
> 2.11.0
>
>

As ".remove" is removed and wdt is required for restarting the system
I am thinking that stop callback is also not required. If so does it
makes sense to remove the stop callback? I can submit a patch for the
same once this patch series goes in.

Regards,
PrasannaKumar
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/8] watchdog: JZ4740: Register a restart handler

[Guenter Roeck]

On 01/19/2018 11:31 PM, PrasannaKumar Muralidharan wrote:
> Hi Paul,
> 
> On 30 December 2017 at 19:21, Paul Cercueil <paul-icTtO2rgO2OTuSrc4Mpeew@public.gmane.org> wrote:
>> The watchdog driver can restart the system by simply configuring the
>> hardware for a timeout of 0 seconds.
>>
>> Signed-off-by: Paul Cercueil <paul-icTtO2rgO2OTuSrc4Mpeew@public.gmane.org>
>> Reviewed-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
>> ---
>>   drivers/watchdog/jz4740_wdt.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>>   v2: No change
>>
>> diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
>> index 92d6ca8ceb49..fa7f49a3212c 100644
>> --- a/drivers/watchdog/jz4740_wdt.c
>> +++ b/drivers/watchdog/jz4740_wdt.c
>> @@ -130,6 +130,14 @@ static int jz4740_wdt_stop(struct watchdog_device *wdt_dev)
>>          return 0;
>>   }
>>
>> +static int jz4740_wdt_restart(struct watchdog_device *wdt_dev,
>> +                             unsigned long action, void *data)
>> +{
>> +       wdt_dev->timeout = 0;
>> +       jz4740_wdt_start(wdt_dev);
>> +       return 0;
>> +}
>> +
>>   static const struct watchdog_info jz4740_wdt_info = {
>>          .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
>>          .identity = "jz4740 Watchdog",
>> @@ -141,6 +149,7 @@ static const struct watchdog_ops jz4740_wdt_ops = {
>>          .stop = jz4740_wdt_stop,
>>          .ping = jz4740_wdt_ping,
>>          .set_timeout = jz4740_wdt_set_timeout,
>> +       .restart = jz4740_wdt_restart,
>>   };
>>
>>   #ifdef CONFIG_OF
>> --
>> 2.11.0
>>
>>
> 
> Noticed that min_timeout of the watchdog device is set to 1 but this
> function calls start with timeout set to 0. Even though this works I
> feel it is better to set min_timeout to 0.
> 

No. That would be wrong. If you want to be pedantic, write a new function
__jz4740_wdt_set_timeout(u16 clock_div, u16 timeout_value) and call it
instead, but don't mess with min_timeout.

Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/8] watchdog: JZ4740: Drop module remove function

[Guenter Roeck]

On 01/19/2018 11:41 PM, PrasannaKumar Muralidharan wrote:
> Hi Paul,
> 
> On 30 December 2017 at 19:21, Paul Cercueil <paul-icTtO2rgO2OTuSrc4Mpeew@public.gmane.org> wrote:
>> When the watchdog was configured for nowayout, and after the
>> userspace watchdog daemon closed the dev node without sending the
>> magic character, unloading this module stopped the watchdog
>> hardware, which was clearly a problem.
>>
>> Besides, unloading the module is not possible when the userspace
>> watchdog daemon is running, so it's safe to assume that we don't
>> need to stop the watchdog hardware in the jz4740_wdt_remove()
>> function.
>>
>> For this reason, the jz4740_wdt_remove() function can then be
>> dropped alltogether.
>>
>> Signed-off-by: Paul Cercueil <paul-icTtO2rgO2OTuSrc4Mpeew@public.gmane.org>
>> ---
>>   drivers/watchdog/jz4740_wdt.c | 8 --------
>>   1 file changed, 8 deletions(-)
>>
>>   v2: New patch in this series
>>
>> diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
>> index fa7f49a3212c..02b9b8e946a2 100644
>> --- a/drivers/watchdog/jz4740_wdt.c
>> +++ b/drivers/watchdog/jz4740_wdt.c
>> @@ -205,16 +205,8 @@ static int jz4740_wdt_probe(struct platform_device *pdev)
>>          return 0;
>>   }
>>
>> -static int jz4740_wdt_remove(struct platform_device *pdev)
>> -{
>> -       struct jz4740_wdt_drvdata *drvdata = platform_get_drvdata(pdev);
>> -
>> -       return jz4740_wdt_stop(&drvdata->wdt);
>> -}
>> -
>>   static struct platform_driver jz4740_wdt_driver = {
>>          .probe = jz4740_wdt_probe,
>> -       .remove = jz4740_wdt_remove,
>>          .driver = {
>>                  .name = "jz4740-wdt",
>>                  .of_match_table = of_match_ptr(jz4740_wdt_of_matches),
>> --
>> 2.11.0
>>
>>
> 
> As ".remove" is removed and wdt is required for restarting the system
> I am thinking that stop callback is also not required. If so does it
> makes sense to remove the stop callback? I can submit a patch for the
> same once this patch series goes in.
> 
The remove function was removed because it would otherwise be an empty
function. Since it is optional, it can and should be removed if it does not
do anything. If the stop function is removed, it is no longer possible
to stop the watchdog. Why would this make sense, and why would it make sense
to drop the stop function if there is no remove function ?

Guenter

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/8] watchdog: JZ4740: Drop module remove function

[PrasannaKumar Muralidharan]

Hi Guenter,

On 20 January 2018 at 21:20, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
> On 01/19/2018 11:41 PM, PrasannaKumar Muralidharan wrote:
>>
>> Hi Paul,
>>
>> On 30 December 2017 at 19:21, Paul Cercueil <paul-icTtO2rgO2OTuSrc4Mpeew@public.gmane.org> wrote:
>>>
>>> When the watchdog was configured for nowayout, and after the
>>> userspace watchdog daemon closed the dev node without sending the
>>> magic character, unloading this module stopped the watchdog
>>> hardware, which was clearly a problem.
>>>
>>> Besides, unloading the module is not possible when the userspace
>>> watchdog daemon is running, so it's safe to assume that we don't
>>> need to stop the watchdog hardware in the jz4740_wdt_remove()
>>> function.
>>>
>>> For this reason, the jz4740_wdt_remove() function can then be
>>> dropped alltogether.
>>>
>>> Signed-off-by: Paul Cercueil <paul-icTtO2rgO2OTuSrc4Mpeew@public.gmane.org>
>>> ---
>>>   drivers/watchdog/jz4740_wdt.c | 8 --------
>>>   1 file changed, 8 deletions(-)
>>>
>>>   v2: New patch in this series
>>>
>>> diff --git a/drivers/watchdog/jz4740_wdt.c
>>> b/drivers/watchdog/jz4740_wdt.c
>>> index fa7f49a3212c..02b9b8e946a2 100644
>>> --- a/drivers/watchdog/jz4740_wdt.c
>>> +++ b/drivers/watchdog/jz4740_wdt.c
>>> @@ -205,16 +205,8 @@ static int jz4740_wdt_probe(struct platform_device
>>> *pdev)
>>>          return 0;
>>>   }
>>>
>>> -static int jz4740_wdt_remove(struct platform_device *pdev)
>>> -{
>>> -       struct jz4740_wdt_drvdata *drvdata = platform_get_drvdata(pdev);
>>> -
>>> -       return jz4740_wdt_stop(&drvdata->wdt);
>>> -}
>>> -
>>>   static struct platform_driver jz4740_wdt_driver = {
>>>          .probe = jz4740_wdt_probe,
>>> -       .remove = jz4740_wdt_remove,
>>>          .driver = {
>>>                  .name = "jz4740-wdt",
>>>                  .of_match_table = of_match_ptr(jz4740_wdt_of_matches),
>>> --
>>> 2.11.0
>>>
>>>
>>
>> As ".remove" is removed and wdt is required for restarting the system
>> I am thinking that stop callback is also not required. If so does it
>> makes sense to remove the stop callback? I can submit a patch for the
>> same once this patch series goes in.
>>
> The remove function was removed because it would otherwise be an empty
> function. Since it is optional, it can and should be removed if it does not
> do anything. If the stop function is removed, it is no longer possible
> to stop the watchdog. Why would this make sense, and why would it make sense
> to drop the stop function if there is no remove function ?
>
> Guenter
>

I missed the fact that stopping is watchdog is possible. Sorry for the noise.

Thanks,
PrasannaKumar
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/8] watchdog: JZ4740: Register a restart handler

[PrasannaKumar Muralidharan]

Hi Guenter,

On 20 January 2018 at 21:15, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
> On 01/19/2018 11:31 PM, PrasannaKumar Muralidharan wrote:
>>
>> Hi Paul,
>>
>> On 30 December 2017 at 19:21, Paul Cercueil <paul-icTtO2rgO2OTuSrc4Mpeew@public.gmane.org> wrote:
>>>
>>> The watchdog driver can restart the system by simply configuring the
>>> hardware for a timeout of 0 seconds.
>>>
>>> Signed-off-by: Paul Cercueil <paul-icTtO2rgO2OTuSrc4Mpeew@public.gmane.org>
>>> Reviewed-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
>>> ---
>>>   drivers/watchdog/jz4740_wdt.c | 9 +++++++++
>>>   1 file changed, 9 insertions(+)
>>>
>>>   v2: No change
>>>
>>> diff --git a/drivers/watchdog/jz4740_wdt.c
>>> b/drivers/watchdog/jz4740_wdt.c
>>> index 92d6ca8ceb49..fa7f49a3212c 100644
>>> --- a/drivers/watchdog/jz4740_wdt.c
>>> +++ b/drivers/watchdog/jz4740_wdt.c
>>> @@ -130,6 +130,14 @@ static int jz4740_wdt_stop(struct watchdog_device
>>> *wdt_dev)
>>>          return 0;
>>>   }
>>>
>>> +static int jz4740_wdt_restart(struct watchdog_device *wdt_dev,
>>> +                             unsigned long action, void *data)
>>> +{
>>> +       wdt_dev->timeout = 0;
>>> +       jz4740_wdt_start(wdt_dev);
>>> +       return 0;
>>> +}
>>> +
>>>   static const struct watchdog_info jz4740_wdt_info = {
>>>          .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
>>> WDIOF_MAGICCLOSE,
>>>          .identity = "jz4740 Watchdog",
>>> @@ -141,6 +149,7 @@ static const struct watchdog_ops jz4740_wdt_ops = {
>>>          .stop = jz4740_wdt_stop,
>>>          .ping = jz4740_wdt_ping,
>>>          .set_timeout = jz4740_wdt_set_timeout,
>>> +       .restart = jz4740_wdt_restart,
>>>   };
>>>
>>>   #ifdef CONFIG_OF
>>> --
>>> 2.11.0
>>>
>>>
>>
>> Noticed that min_timeout of the watchdog device is set to 1 but this
>> function calls start with timeout set to 0. Even though this works I
>> feel it is better to set min_timeout to 0.
>>
>
> No. That would be wrong. If you want to be pedantic, write a new function
> __jz4740_wdt_set_timeout(u16 clock_div, u16 timeout_value) and call it
> instead, but don't mess with min_timeout.
>
> Guenter

What is the effect of changing min_timeout? I could see only
validation checks with it.

Thanks,
PrasannaKumar
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/8] watchdog: JZ4740: Register a restart handler

[Guenter Roeck]

On 01/20/2018 08:04 AM, PrasannaKumar Muralidharan wrote:
> Hi Guenter,
> 
> On 20 January 2018 at 21:15, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
>> On 01/19/2018 11:31 PM, PrasannaKumar Muralidharan wrote:
>>>
>>> Hi Paul,
>>>
>>> On 30 December 2017 at 19:21, Paul Cercueil <paul-icTtO2rgO2OTuSrc4Mpeew@public.gmane.org> wrote:
>>>>
>>>> The watchdog driver can restart the system by simply configuring the
>>>> hardware for a timeout of 0 seconds.
>>>>
>>>> Signed-off-by: Paul Cercueil <paul-icTtO2rgO2OTuSrc4Mpeew@public.gmane.org>
>>>> Reviewed-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
>>>> ---
>>>>    drivers/watchdog/jz4740_wdt.c | 9 +++++++++
>>>>    1 file changed, 9 insertions(+)
>>>>
>>>>    v2: No change
>>>>
>>>> diff --git a/drivers/watchdog/jz4740_wdt.c
>>>> b/drivers/watchdog/jz4740_wdt.c
>>>> index 92d6ca8ceb49..fa7f49a3212c 100644
>>>> --- a/drivers/watchdog/jz4740_wdt.c
>>>> +++ b/drivers/watchdog/jz4740_wdt.c
>>>> @@ -130,6 +130,14 @@ static int jz4740_wdt_stop(struct watchdog_device
>>>> *wdt_dev)
>>>>           return 0;
>>>>    }
>>>>
>>>> +static int jz4740_wdt_restart(struct watchdog_device *wdt_dev,
>>>> +                             unsigned long action, void *data)
>>>> +{
>>>> +       wdt_dev->timeout = 0;
>>>> +       jz4740_wdt_start(wdt_dev);
>>>> +       return 0;
>>>> +}
>>>> +
>>>>    static const struct watchdog_info jz4740_wdt_info = {
>>>>           .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
>>>> WDIOF_MAGICCLOSE,
>>>>           .identity = "jz4740 Watchdog",
>>>> @@ -141,6 +149,7 @@ static const struct watchdog_ops jz4740_wdt_ops = {
>>>>           .stop = jz4740_wdt_stop,
>>>>           .ping = jz4740_wdt_ping,
>>>>           .set_timeout = jz4740_wdt_set_timeout,
>>>> +       .restart = jz4740_wdt_restart,
>>>>    };
>>>>
>>>>    #ifdef CONFIG_OF
>>>> --
>>>> 2.11.0
>>>>
>>>>
>>>
>>> Noticed that min_timeout of the watchdog device is set to 1 but this
>>> function calls start with timeout set to 0. Even though this works I
>>> feel it is better to set min_timeout to 0.
>>>
>>
>> No. That would be wrong. If you want to be pedantic, write a new function
>> __jz4740_wdt_set_timeout(u16 clock_div, u16 timeout_value) and call it
>> instead, but don't mess with min_timeout.
>>
>> Guenter
> 
> What is the effect of changing min_timeout? I could see only
> validation checks with it.
> 
Let me ask you - you want to let userspace set a timeout of 0, which would
effectively reboot the system through the backdoor. Why ?

Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 6/8] MIPS: jz4780: dts: Fix watchdog node

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 658 bytes --]

On Sat, Dec 30, 2017 at 02:51:06PM +0100, Paul Cercueil wrote:
> diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi
> index 9b5794667aee..a52f59bf58c7 100644
> --- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
> +++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
> @@ -221,7 +221,10 @@
>  
>  	watchdog: watchdog@10002000 {
>  		compatible = "ingenic,jz4780-watchdog";
> -		reg = <0x10002000 0x100>;
> +		reg = <0x10002000 0x10>;

Should the example in
Documentation/devicetree/bindings/watchdog/ingenic,jz4740-wdt.txt be
updated too?

Regardless,
Acked-by: James Hogan <jhogan@kernel.org>

Cheers
James


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 7/8] MIPS: qi_lb60: Enable the jz4740-wdt driver

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 533 bytes --]

On Sat, Dec 30, 2017 at 02:51:07PM +0100, Paul Cercueil wrote:
> The watchdog is an useful piece of hardware, so there's no reason not to
> enable it.

Presumably this is important for restart to work after the change in the
next patch? Probably worth mentioning that too if thats the case.

> 
> This commit enables the Kconfig option in the qi_lb60 defconfig.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>

The change looks good to me though

Acked-by: James Hogan <jhogan@kernel.org>

Cheers
James
 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 8/8] MIPS: jz4740: Drop old platform reset code

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 248 bytes --]

On Sat, Dec 30, 2017 at 02:51:08PM +0100, Paul Cercueil wrote:
> This work is now performed by the watchdog driver directly.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>

Acked-by: James Hogan <jhogan@kernel.org>

Cheers
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]