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
- Use devm_clk_get instead of clk_get
- Use devm_watchdog_register_device instead of watchdog_register_device
Signed-off-by: Paul Cercueil <[email protected]>
---
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
This work is now performed by the watchdog driver directly.
Signed-off-by: Paul Cercueil <[email protected]>
---
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
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 <[email protected]>
---
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
- 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 <[email protected]>
---
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
Also remove the watchdog platform_device from platform.c, since it
wasn't used anywhere anyway.
Signed-off-by: Paul Cercueil <[email protected]>
---
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
The watchdog driver can restart the system by simply configuring the
hardware for a timeout of 0 seconds.
Signed-off-by: Paul Cercueil <[email protected]>
---
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
Previously, the clock was disabled first, which makes the watchdog
component insensitive to register writes.
Signed-off-by: Paul Cercueil <[email protected]>
---
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
Hi Paul,
On Thu, Dec 28, 2017 at 5:29 PM, Paul Cercueil <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
> ---
> 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 = {
>
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 <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
> ---
> 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;
> }
>
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 <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
[ 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 = {
>
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 <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
> ---
> 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
>
Hi Guenter,
Le jeu. 28 d?c. 2017 ? 18:48, Guenter Roeck <[email protected]> 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 <[email protected]>
>> ---
>> 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
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
>
>> }
>> static struct platform_driver jz4740_wdt_driver = {
>>
>
On 12/28/2017 11:59 AM, Paul Cercueil wrote:
> Hi Guenter,
>
> Le jeu. 28 déc. 2017 à 18:48, Guenter Roeck <[email protected]> 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 <[email protected]>
>>> ---
>>> 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
>
> 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
Le jeu. 28 d?c. 2017 ? 21:19, Guenter Roeck <[email protected]> a
?crit :
> On 12/28/2017 11:59 AM, Paul Cercueil wrote:
>> Hi Guenter,
>>
>> Le jeu. 28 d?c. 2017 ? 18:48, Guenter Roeck <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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
>>
>> 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
On 12/28/2017 12:22 PM, Paul Cercueil wrote:
>
>
> Le jeu. 28 déc. 2017 à 21:19, Guenter Roeck <[email protected]> a écrit :
>> On 12/28/2017 11:59 AM, Paul Cercueil wrote:
>>> Hi Guenter,
>>>
>>> Le jeu. 28 déc. 2017 à 18:48, Guenter Roeck <[email protected]> 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 <[email protected]>
>>>>> ---
>>>>> 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
>>>
>>> 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
- Use devm_clk_get instead of clk_get
- Use devm_watchdog_register_device instead of watchdog_register_device
Signed-off-by: Paul Cercueil <[email protected]>
---
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
This work is now performed by the watchdog driver directly.
Signed-off-by: Paul Cercueil <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
- 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 <[email protected]>
Reviewed-by: Mathieu Malaterre <[email protected]>
---
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
The watchdog driver can restart the system by simply configuring the
hardware for a timeout of 0 seconds.
Signed-off-by: Paul Cercueil <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
---
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
Also remove the watchdog platform_device from platform.c, since it
wasn't used anywhere anyway.
Signed-off-by: Paul Cercueil <[email protected]>
---
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
Previously, the clock was disabled first, which makes the watchdog
component insensitive to register writes.
Signed-off-by: Paul Cercueil <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
> ---
> 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 = {
>
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 <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
> ---
> 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),
>
Hi Paul,
On 30 December 2017 at 19:21, Paul Cercueil <[email protected]> wrote:
> Also remove the watchdog platform_device from platform.c, since it
> wasn't used anywhere anyway.
>
> Signed-off-by: Paul Cercueil <[email protected]>
> ---
> 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
Hi PrasannaKumar,
Le mar. 2 janv. 2018 ? 17:37, PrasannaKumar Muralidharan
<[email protected]> a ?crit :
> Hi Paul,
>
> On 30 December 2017 at 19:21, Paul Cercueil <[email protected]>
> wrote:
>> Also remove the watchdog platform_device from platform.c, since it
>> wasn't used anywhere anyway.
>>
>> Signed-off-by: Paul Cercueil <[email protected]>
>> ---
>> 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
On 01/02/2018 08:48 AM, Paul Cercueil wrote:
> Hi PrasannaKumar,
>
> Le mar. 2 janv. 2018 à 17:37, PrasannaKumar Muralidharan <[email protected]> a écrit :
>> Hi Paul,
>>
>> On 30 December 2017 at 19:21, Paul Cercueil <[email protected]> wrote:
>>> Also remove the watchdog platform_device from platform.c, since it
>>> wasn't used anywhere anyway.
>>>
>>> Signed-off-by: Paul Cercueil <[email protected]>
>>> ---
>>> 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
Le mer. 3 janv. 2018 ? 5:46, Guenter Roeck <[email protected]> a
?crit :
> On 01/02/2018 08:48 AM, Paul Cercueil wrote:
>> Hi PrasannaKumar,
>>
>> Le mar. 2 janv. 2018 ? 17:37, PrasannaKumar Muralidharan
>> <[email protected]> a ?crit :
>>> Hi Paul,
>>>
>>> On 30 December 2017 at 19:21, Paul Cercueil <[email protected]>
>>> wrote:
>>>> Also remove the watchdog platform_device from platform.c, since it
>>>> wasn't used anywhere anyway.
>>>>
>>>> Signed-off-by: Paul Cercueil <[email protected]>
>>>> ---
>>>> 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
Hi Guenter,
On 3 January 2018 at 10:16, Guenter Roeck <[email protected]> wrote:
> On 01/02/2018 08:48 AM, Paul Cercueil wrote:
>>
>> Hi PrasannaKumar,
>>
>> Le mar. 2 janv. 2018 à 17:37, PrasannaKumar Muralidharan
>> <[email protected]> a écrit :
>>>
>>> Hi Paul,
>>>
>>> On 30 December 2017 at 19:21, Paul Cercueil <[email protected]> wrote:
>>>>
>>>> Also remove the watchdog platform_device from platform.c, since it
>>>> wasn't used anywhere anyway.
>>>>
>>>> Signed-off-by: Paul Cercueil <[email protected]>
>>>> ---
>>>> 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
Hi Paul,
On 30 December 2017 at 19:21, Paul Cercueil <[email protected]> wrote:
> The watchdog driver can restart the system by simply configuring the
> hardware for a timeout of 0 seconds.
>
> Signed-off-by: Paul Cercueil <[email protected]>
> Reviewed-by: Guenter Roeck <[email protected]>
> ---
> 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
Hi Paul,
On 30 December 2017 at 19:21, Paul Cercueil <[email protected]> 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 <[email protected]>
> ---
> 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
On 01/19/2018 11:31 PM, PrasannaKumar Muralidharan wrote:
> Hi Paul,
>
> On 30 December 2017 at 19:21, Paul Cercueil <[email protected]> wrote:
>> The watchdog driver can restart the system by simply configuring the
>> hardware for a timeout of 0 seconds.
>>
>> Signed-off-by: Paul Cercueil <[email protected]>
>> Reviewed-by: Guenter Roeck <[email protected]>
>> ---
>> 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
On 01/19/2018 11:41 PM, PrasannaKumar Muralidharan wrote:
> Hi Paul,
>
> On 30 December 2017 at 19:21, Paul Cercueil <[email protected]> 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 <[email protected]>
>> ---
>> 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
Hi Guenter,
On 20 January 2018 at 21:20, Guenter Roeck <[email protected]> wrote:
> On 01/19/2018 11:41 PM, PrasannaKumar Muralidharan wrote:
>>
>> Hi Paul,
>>
>> On 30 December 2017 at 19:21, Paul Cercueil <[email protected]> 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 <[email protected]>
>>> ---
>>> 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
Hi Guenter,
On 20 January 2018 at 21:15, Guenter Roeck <[email protected]> wrote:
> On 01/19/2018 11:31 PM, PrasannaKumar Muralidharan wrote:
>>
>> Hi Paul,
>>
>> On 30 December 2017 at 19:21, Paul Cercueil <[email protected]> wrote:
>>>
>>> The watchdog driver can restart the system by simply configuring the
>>> hardware for a timeout of 0 seconds.
>>>
>>> Signed-off-by: Paul Cercueil <[email protected]>
>>> Reviewed-by: Guenter Roeck <[email protected]>
>>> ---
>>> 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
On 01/20/2018 08:04 AM, PrasannaKumar Muralidharan wrote:
> Hi Guenter,
>
> On 20 January 2018 at 21:15, Guenter Roeck <[email protected]> wrote:
>> On 01/19/2018 11:31 PM, PrasannaKumar Muralidharan wrote:
>>>
>>> Hi Paul,
>>>
>>> On 30 December 2017 at 19:21, Paul Cercueil <[email protected]> wrote:
>>>>
>>>> The watchdog driver can restart the system by simply configuring the
>>>> hardware for a timeout of 0 seconds.
>>>>
>>>> Signed-off-by: Paul Cercueil <[email protected]>
>>>> Reviewed-by: Guenter Roeck <[email protected]>
>>>> ---
>>>> 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
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 <[email protected]>
Cheers
James
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 <[email protected]>
The change looks good to me though
Acked-by: James Hogan <[email protected]>
Cheers
James
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 <[email protected]>
Acked-by: James Hogan <[email protected]>
Cheers
James