[PATCH 1/3] MIPS: Use do_kernel_restart() as the default restart handler

[PATCH 1/3] MIPS: Use do_kernel_restart() as the default restart handler

[Lars-Peter Clausen]

Use the recently introduced do_kernel_restart() function as the default restart
handler if the platform did not explicitly provide a restart handler. This
allows use restart handler that have been registered by device drivers to
restart the machine.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 arch/mips/kernel/reset.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/mips/kernel/reset.c b/arch/mips/kernel/reset.c
index 07fc524..36cd80c 100644
--- a/arch/mips/kernel/reset.c
+++ b/arch/mips/kernel/reset.c
@@ -19,7 +19,7 @@
  * So handle all using function pointers to machine specific
  * functions.
  */
-void (*_machine_restart)(char *command);
+void (*_machine_restart)(char *command) = do_kernel_restart;
 void (*_machine_halt)(void);
 void (*pm_power_off)(void);
 
-- 
1.7.10.4

[PATCH 2/3] MIPS: qi_lb60: Register watchdog device

[Lars-Peter Clausen]

The next commit will move the restart code to the watchdog driver. So for
restart to continue to work we need to register the watchdog device.

Also enable the watchdog driver in the default config.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 arch/mips/configs/qi_lb60_defconfig |    2 ++
 arch/mips/jz4740/board-qi_lb60.c    |    1 +
 2 files changed, 3 insertions(+)

diff --git a/arch/mips/configs/qi_lb60_defconfig b/arch/mips/configs/qi_lb60_defconfig
index 2b96547..ce06a92 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
diff --git a/arch/mips/jz4740/board-qi_lb60.c b/arch/mips/jz4740/board-qi_lb60.c
index c454525..a00f134 100644
--- a/arch/mips/jz4740/board-qi_lb60.c
+++ b/arch/mips/jz4740/board-qi_lb60.c
@@ -436,6 +436,7 @@ static struct gpiod_lookup_table qi_lb60_audio_gpio_table = {
 };
 
 static struct platform_device *jz_platform_devices[] __initdata = {
+	&jz4740_wdt_device,
 	&jz4740_udc_device,
 	&jz4740_udc_xceiv_device,
 	&jz4740_mmc_device,
-- 
1.7.10.4

Re: [PATCH 2/3] MIPS: qi_lb60: Register watchdog device

[Guenter Roeck]

On 01/10/2015 10:29 AM, Lars-Peter Clausen wrote:
> The next commit will move the restart code to the watchdog driver. So for
> restart to continue to work we need to register the watchdog device.
>
> Also enable the watchdog driver in the default config.
>
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> ---
>   arch/mips/configs/qi_lb60_defconfig |    2 ++
>   arch/mips/jz4740/board-qi_lb60.c    |    1 +
>   2 files changed, 3 insertions(+)
>
> diff --git a/arch/mips/configs/qi_lb60_defconfig b/arch/mips/configs/qi_lb60_defconfig
> index 2b96547..ce06a92 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

Shouldn't JZ4740_WDT be selected from MACH_JZ4740 ?

Guess it doesn't really matter if there is just one board, but if there
is ever another board the reset handler would not automatically be enabled.

Thanks,
Guenter

>   CONFIG_MFD_JZ4740_ADC=y
>   CONFIG_REGULATOR=y
>   CONFIG_REGULATOR_FIXED_VOLTAGE=y
> diff --git a/arch/mips/jz4740/board-qi_lb60.c b/arch/mips/jz4740/board-qi_lb60.c
> index c454525..a00f134 100644
> --- a/arch/mips/jz4740/board-qi_lb60.c
> +++ b/arch/mips/jz4740/board-qi_lb60.c
> @@ -436,6 +436,7 @@ static struct gpiod_lookup_table qi_lb60_audio_gpio_table = {
>   };
>
>   static struct platform_device *jz_platform_devices[] __initdata = {
> +	&jz4740_wdt_device,
>   	&jz4740_udc_device,
>   	&jz4740_udc_xceiv_device,
>   	&jz4740_mmc_device,
>

Re: [PATCH 2/3] MIPS: qi_lb60: Register watchdog device

[Maarten ter Huurne]

On Saturday 10 January 2015 17:24:53 Guenter Roeck wrote:
> On 01/10/2015 10:29 AM, Lars-Peter Clausen wrote:
> > The next commit will move the restart code to the watchdog driver. So
> > for
> > restart to continue to work we need to register the watchdog device.
> > 
> > Also enable the watchdog driver in the default config.
> > 
> > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> > ---
> > 
> >   arch/mips/configs/qi_lb60_defconfig |    2 ++
> >   arch/mips/jz4740/board-qi_lb60.c    |    1 +
> >   2 files changed, 3 insertions(+)
> > 
> > diff --git a/arch/mips/configs/qi_lb60_defconfig
> > b/arch/mips/configs/qi_lb60_defconfig index 2b96547..ce06a92 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
> 
> Shouldn't JZ4740_WDT be selected from MACH_JZ4740 ?
> 
> Guess it doesn't really matter if there is just one board, but if there
> is ever another board the reset handler would not automatically be
> enabled.

There is only one board in the mainline kernel, but Paul Cercueil and me did 
maintain a kernel for the Dingoo A320 for a while which is also JZ4740-
based. Also, a lot of the JZ4740 drivers can be used as-is or with small 
adaptations on later SoC generations. For example, we're using a slightly 
modified version of this watchdog driver on the JZ4770-based GCW Zero.

Bye,
		Maarten

Re: [PATCH 2/3] MIPS: qi_lb60: Register watchdog device

[Guenter Roeck]

On 01/10/2015 05:37 PM, Maarten ter Huurne wrote:
> On Saturday 10 January 2015 17:24:53 Guenter Roeck wrote:
>> On 01/10/2015 10:29 AM, Lars-Peter Clausen wrote:
>>> The next commit will move the restart code to the watchdog driver. So
>>> for
>>> restart to continue to work we need to register the watchdog device.
>>>
>>> Also enable the watchdog driver in the default config.
>>>
>>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>>> ---
>>>
>>>    arch/mips/configs/qi_lb60_defconfig |    2 ++
>>>    arch/mips/jz4740/board-qi_lb60.c    |    1 +
>>>    2 files changed, 3 insertions(+)
>>>
>>> diff --git a/arch/mips/configs/qi_lb60_defconfig
>>> b/arch/mips/configs/qi_lb60_defconfig index 2b96547..ce06a92 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
>>
>> Shouldn't JZ4740_WDT be selected from MACH_JZ4740 ?
>>
>> Guess it doesn't really matter if there is just one board, but if there
>> is ever another board the reset handler would not automatically be
>> enabled.
>
> There is only one board in the mainline kernel, but Paul Cercueil and me did
> maintain a kernel for the Dingoo A320 for a while which is also JZ4740-
> based. Also, a lot of the JZ4740 drivers can be used as-is or with small
> adaptations on later SoC generations. For example, we're using a slightly
> modified version of this watchdog driver on the JZ4770-based GCW Zero.
>

Where does that leave us ? Should CONFIG_JZ4740_WDT be auto-selected ?

Thanks,
Guenter

[PATCH 3/3] MIPS: jz4740: Move reset code to the watchdog driver

[Lars-Peter Clausen]

On JZ4740 reset is handled by the watchdog peripheral. This patch moves the
reset handler code from a architecture specific file to the watchdog peripheral
driver and registers it as a generic reset handler. This will allow it to be
reused by other SoCs that use the same watchdog peripheral.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 arch/mips/jz4740/reset.c      |   22 ----------------------
 drivers/watchdog/jz4740_wdt.c |   34 ++++++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 22 deletions(-)

diff --git a/arch/mips/jz4740/reset.c b/arch/mips/jz4740/reset.c
index b6c6343..0871b94 100644
--- a/arch/mips/jz4740/reset.c
+++ b/arch/mips/jz4740/reset.c
@@ -35,27 +35,6 @@ 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();
-}
-
 #define JZ_REG_RTC_CTRL			0x00
 #define JZ_REG_RTC_HIBERNATE		0x20
 #define JZ_REG_RTC_WAKEUP_FILTER	0x24
@@ -112,7 +91,6 @@ static void jz4740_power_off(void)
 
 void jz4740_reset_init(void)
 {
-	_machine_restart = jz4740_restart;
 	_machine_halt = jz4740_halt;
 	pm_power_off = jz4740_power_off;
 }
diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
index 18e41af..86a4c55 100644
--- a/drivers/watchdog/jz4740_wdt.c
+++ b/drivers/watchdog/jz4740_wdt.c
@@ -24,6 +24,7 @@
 #include <linux/clk.h>
 #include <linux/slab.h>
 #include <linux/err.h>
+#include <linux/reboot.h>
 
 #include <asm/mach-jz4740/timer.h>
 
@@ -65,6 +66,8 @@ struct jz4740_wdt_drvdata {
 	struct watchdog_device wdt;
 	void __iomem *base;
 	struct clk *rtc_clk;
+
+	struct notifier_block restart_handler;
 };
 
 static int jz4740_wdt_ping(struct watchdog_device *wdt_dev)
@@ -142,6 +145,25 @@ static const struct watchdog_ops jz4740_wdt_ops = {
 	.set_timeout = jz4740_wdt_set_timeout,
 };
 
+static int jz4740_wdt_restart(struct notifier_block *nb,
+	unsigned long mode, void *cmd)
+{
+	struct jz4740_wdt_drvdata *drvdata = container_of(nb,
+		struct jz4740_wdt_drvdata, restart_handler);
+
+	jz4740_timer_enable_watchdog();
+
+	writeb(0, drvdata->base + JZ_REG_WDT_COUNTER_ENABLE);
+
+	writew(0, drvdata->base + JZ_REG_WDT_TIMER_COUNTER);
+	writew(0, drvdata->base + JZ_REG_WDT_TIMER_DATA);
+	writew(JZ_WDT_CLOCK_EXT, drvdata->base + JZ_REG_WDT_TIMER_CONTROL);
+
+	writeb(1, drvdata->base + JZ_REG_WDT_COUNTER_ENABLE);
+
+	return NOTIFY_DONE;
+}
+
 static int jz4740_wdt_probe(struct platform_device *pdev)
 {
 	struct jz4740_wdt_drvdata *drvdata;
@@ -186,9 +208,20 @@ static int jz4740_wdt_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto err_disable_clk;
 
+	drvdata->restart_handler.notifier_call = jz4740_wdt_restart;
+	drvdata->restart_handler.priority = 128;
+	ret = register_restart_handler(&drvdata->restart_handler);
+	if (ret) {
+		dev_err(&pdev->dev, "cannot register restart handler, %d\n",
+			ret);
+		goto err_unregister_watchdog;
+	}
+
 	platform_set_drvdata(pdev, drvdata);
 	return 0;
 
+err_unregister_watchdog:
+	watchdog_unregister_device(&drvdata->wdt);
 err_disable_clk:
 	clk_put(drvdata->rtc_clk);
 err_out:
@@ -199,6 +232,7 @@ static int jz4740_wdt_remove(struct platform_device *pdev)
 {
 	struct jz4740_wdt_drvdata *drvdata = platform_get_drvdata(pdev);
 
+	unregister_restart_handler(&drvdata->restart_handler);
 	jz4740_wdt_stop(&drvdata->wdt);
 	watchdog_unregister_device(&drvdata->wdt);
 	clk_put(drvdata->rtc_clk);
-- 
1.7.10.4

Re: [PATCH 3/3] MIPS: jz4740: Move reset code to the watchdog driver

[Guenter Roeck]

On 01/10/2015 10:29 AM, Lars-Peter Clausen wrote:
> On JZ4740 reset is handled by the watchdog peripheral. This patch moves the
> reset handler code from a architecture specific file to the watchdog peripheral
> driver and registers it as a generic reset handler. This will allow it to be
> reused by other SoCs that use the same watchdog peripheral.
>
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> ---
>   arch/mips/jz4740/reset.c      |   22 ----------------------
>   drivers/watchdog/jz4740_wdt.c |   34 ++++++++++++++++++++++++++++++++++
>   2 files changed, 34 insertions(+), 22 deletions(-)
>
> diff --git a/arch/mips/jz4740/reset.c b/arch/mips/jz4740/reset.c
> index b6c6343..0871b94 100644
> --- a/arch/mips/jz4740/reset.c
> +++ b/arch/mips/jz4740/reset.c
> @@ -35,27 +35,6 @@ 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();
> -}
> -
>   #define JZ_REG_RTC_CTRL			0x00
>   #define JZ_REG_RTC_HIBERNATE		0x20
>   #define JZ_REG_RTC_WAKEUP_FILTER	0x24
> @@ -112,7 +91,6 @@ static void jz4740_power_off(void)
>
>   void jz4740_reset_init(void)
>   {
> -	_machine_restart = jz4740_restart;
>   	_machine_halt = jz4740_halt;
>   	pm_power_off = jz4740_power_off;
>   }
> diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
> index 18e41af..86a4c55 100644
> --- a/drivers/watchdog/jz4740_wdt.c
> +++ b/drivers/watchdog/jz4740_wdt.c
> @@ -24,6 +24,7 @@
>   #include <linux/clk.h>
>   #include <linux/slab.h>
>   #include <linux/err.h>
> +#include <linux/reboot.h>
>
>   #include <asm/mach-jz4740/timer.h>
>
> @@ -65,6 +66,8 @@ struct jz4740_wdt_drvdata {
>   	struct watchdog_device wdt;
>   	void __iomem *base;
>   	struct clk *rtc_clk;
> +
> +	struct notifier_block restart_handler;
>   };
>
>   static int jz4740_wdt_ping(struct watchdog_device *wdt_dev)
> @@ -142,6 +145,25 @@ static const struct watchdog_ops jz4740_wdt_ops = {
>   	.set_timeout = jz4740_wdt_set_timeout,
>   };
>
> +static int jz4740_wdt_restart(struct notifier_block *nb,
> +	unsigned long mode, void *cmd)
> +{
> +	struct jz4740_wdt_drvdata *drvdata = container_of(nb,
> +		struct jz4740_wdt_drvdata, restart_handler);
> +
> +	jz4740_timer_enable_watchdog();
> +
> +	writeb(0, drvdata->base + JZ_REG_WDT_COUNTER_ENABLE);
> +
> +	writew(0, drvdata->base + JZ_REG_WDT_TIMER_COUNTER);
> +	writew(0, drvdata->base + JZ_REG_WDT_TIMER_DATA);
> +	writew(JZ_WDT_CLOCK_EXT, drvdata->base + JZ_REG_WDT_TIMER_CONTROL);
> +
> +	writeb(1, drvdata->base + JZ_REG_WDT_COUNTER_ENABLE);
> +
> +	return NOTIFY_DONE;
> +}
> +
>   static int jz4740_wdt_probe(struct platform_device *pdev)
>   {
>   	struct jz4740_wdt_drvdata *drvdata;
> @@ -186,9 +208,20 @@ static int jz4740_wdt_probe(struct platform_device *pdev)
>   	if (ret < 0)
>   		goto err_disable_clk;
>
> +	drvdata->restart_handler.notifier_call = jz4740_wdt_restart;
> +	drvdata->restart_handler.priority = 128;
> +	ret = register_restart_handler(&drvdata->restart_handler);
> +	if (ret) {
> +		dev_err(&pdev->dev, "cannot register restart handler, %d\n",
> +			ret);
> +		goto err_unregister_watchdog;

Are you sure you want to abort in this case ?
After all, the watchdog would still work.

This is pretty theoretic, since the registration does in practice never fail,
so

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

Thanks,
Guenter

> +	}
> +
>   	platform_set_drvdata(pdev, drvdata);
>   	return 0;
>
> +err_unregister_watchdog:
> +	watchdog_unregister_device(&drvdata->wdt);
>   err_disable_clk:
>   	clk_put(drvdata->rtc_clk);
>   err_out:
> @@ -199,6 +232,7 @@ static int jz4740_wdt_remove(struct platform_device *pdev)
>   {
>   	struct jz4740_wdt_drvdata *drvdata = platform_get_drvdata(pdev);
>
> +	unregister_restart_handler(&drvdata->restart_handler);
>   	jz4740_wdt_stop(&drvdata->wdt);
>   	watchdog_unregister_device(&drvdata->wdt);
>   	clk_put(drvdata->rtc_clk);
>

Re: [PATCH 3/3] MIPS: jz4740: Move reset code to the watchdog driver

[Maarten ter Huurne]

On Saturday 10 January 2015 17:18:03 Guenter Roeck wrote:
> On 01/10/2015 10:29 AM, Lars-Peter Clausen wrote:
> > @@ -186,9 +208,20 @@ static int jz4740_wdt_probe(struct platform_device
> > *pdev)> 
> >   	if (ret < 0)
> >   		goto err_disable_clk;
> > 
> > +	drvdata->restart_handler.notifier_call = jz4740_wdt_restart;
> > +	drvdata->restart_handler.priority = 128;
> > +	ret = register_restart_handler(&drvdata->restart_handler);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "cannot register restart handler, %d\n",
> > +			ret);
> > +		goto err_unregister_watchdog;
> 
> Are you sure you want to abort in this case ?
> After all, the watchdog would still work.

That raises a similar question: what about the opposite case, where the 
watchdog registration fails? If the resource acquisition part of the probe 
fails, neither the watchdog nor the restart functionality is going to work, 
but if the call to watchdog_register_device() fails, the restart handler 
would still work.

Bye,
		Maarten

Re: [PATCH 3/3] MIPS: jz4740: Move reset code to the watchdog driver

[Lars-Peter Clausen]

On 01/11/2015 02:43 AM, Maarten ter Huurne wrote:
> On Saturday 10 January 2015 17:18:03 Guenter Roeck wrote:
>> On 01/10/2015 10:29 AM, Lars-Peter Clausen wrote:
>>> @@ -186,9 +208,20 @@ static int jz4740_wdt_probe(struct platform_device
>>> *pdev)>
>>>    	if (ret < 0)
>>>    		goto err_disable_clk;
>>>
>>> +	drvdata->restart_handler.notifier_call = jz4740_wdt_restart;
>>> +	drvdata->restart_handler.priority = 128;
>>> +	ret = register_restart_handler(&drvdata->restart_handler);
>>> +	if (ret) {
>>> +		dev_err(&pdev->dev, "cannot register restart handler, %d\n",
>>> +			ret);
>>> +		goto err_unregister_watchdog;
>>
>> Are you sure you want to abort in this case ?
>> After all, the watchdog would still work.
>
> That raises a similar question: what about the opposite case, where the
> watchdog registration fails? If the resource acquisition part of the probe
> fails, neither the watchdog nor the restart functionality is going to work,
> but if the call to watchdog_register_device() fails, the restart handler
> would still work.

I think this is fine, if either the watchdog or the restart handler 
registration fail then the system is probably already in a rather unusable 
state.

But that got me thinking, maybe instead of having each watchdog driver 
register and implement its own restart handler we should maybe add this as a 
functionality to the watchdog framework. Something along the lines off.

watchdog_set_timeout(wdt, wdt->min_timeout);
watchdog_start(wdt);
mdelay(wdt->min_timeout * 2000);

- Lars

Re: [PATCH 3/3] MIPS: jz4740: Move reset code to the watchdog driver

[Guenter Roeck]

On 01/11/2015 01:41 AM, Lars-Peter Clausen wrote:
> On 01/11/2015 02:43 AM, Maarten ter Huurne wrote:
>> On Saturday 10 January 2015 17:18:03 Guenter Roeck wrote:
>>> On 01/10/2015 10:29 AM, Lars-Peter Clausen wrote:
>>>> @@ -186,9 +208,20 @@ static int jz4740_wdt_probe(struct platform_device
>>>> *pdev)>
>>>>        if (ret < 0)
>>>>            goto err_disable_clk;
>>>>
>>>> +    drvdata->restart_handler.notifier_call = jz4740_wdt_restart;
>>>> +    drvdata->restart_handler.priority = 128;
>>>> +    ret = register_restart_handler(&drvdata->restart_handler);
>>>> +    if (ret) {
>>>> +        dev_err(&pdev->dev, "cannot register restart handler, %d\n",
>>>> +            ret);
>>>> +        goto err_unregister_watchdog;
>>>
>>> Are you sure you want to abort in this case ?
>>> After all, the watchdog would still work.
>>
>> That raises a similar question: what about the opposite case, where the
>> watchdog registration fails? If the resource acquisition part of the probe
>> fails, neither the watchdog nor the restart functionality is going to work,
>> but if the call to watchdog_register_device() fails, the restart handler
>> would still work.
>
> I think this is fine, if either the watchdog or the restart handler registration fail then the system is probably already in a rather unusable state.
>
> But that got me thinking, maybe instead of having each watchdog driver register and implement its own restart handler we should maybe add this as a functionality to the watchdog framework. Something along the lines off.
>
> watchdog_set_timeout(wdt, wdt->min_timeout);
> watchdog_start(wdt);
> mdelay(wdt->min_timeout * 2000);
>

Agreed, something like that might be useful as fallback mechanism.

Guenter

Re: [PATCH 1/3] MIPS: Use do_kernel_restart() as the default restart handler

[Måns Rullgård]

Lars-Peter Clausen <lars@metafoo.de> writes:

> Use the recently introduced do_kernel_restart() function as the default restart
> handler if the platform did not explicitly provide a restart handler. This
> allows use restart handler that have been registered by device drivers to
> restart the machine.
>
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> ---
>  arch/mips/kernel/reset.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/mips/kernel/reset.c b/arch/mips/kernel/reset.c
> index 07fc524..36cd80c 100644
> --- a/arch/mips/kernel/reset.c
> +++ b/arch/mips/kernel/reset.c
> @@ -19,7 +19,7 @@
>   * So handle all using function pointers to machine specific
>   * functions.
>   */
> -void (*_machine_restart)(char *command);
> +void (*_machine_restart)(char *command) = do_kernel_restart;
>  void (*_machine_halt)(void);
>  void (*pm_power_off)(void);

There is already a similar patch posted by Kevin Cernekee:
http://www.linux-mips.org/archives/linux-mips/2014-12/msg00410.html

-- 
Måns Rullgård
mans@mansr.com

Re: [PATCH 1/3] MIPS: Use do_kernel_restart() as the default restart handler

[Guenter Roeck]

On 01/10/2015 12:08 PM, Måns Rullgård wrote:
> Lars-Peter Clausen <lars@metafoo.de> writes:
>
>> Use the recently introduced do_kernel_restart() function as the default restart
>> handler if the platform did not explicitly provide a restart handler. This
>> allows use restart handler that have been registered by device drivers to
>> restart the machine.
>>
>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>> ---
>>   arch/mips/kernel/reset.c |    2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/mips/kernel/reset.c b/arch/mips/kernel/reset.c
>> index 07fc524..36cd80c 100644
>> --- a/arch/mips/kernel/reset.c
>> +++ b/arch/mips/kernel/reset.c
>> @@ -19,7 +19,7 @@
>>    * So handle all using function pointers to machine specific
>>    * functions.
>>    */
>> -void (*_machine_restart)(char *command);
>> +void (*_machine_restart)(char *command) = do_kernel_restart;
>>   void (*_machine_halt)(void);
>>   void (*pm_power_off)(void);
>
> There is already a similar patch posted by Kevin Cernekee:
> http://www.linux-mips.org/archives/linux-mips/2014-12/msg00410.html
>
Personally I prefer the earlier patch, though I guess that is personal preference.

Guenter

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

Re: [PATCH 1/3] MIPS: Use do_kernel_restart() as the default restart handler

[Måns Rullgård]

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

> On 01/10/2015 12:08 PM, Måns Rullgård wrote:
>> Lars-Peter Clausen <lars@metafoo.de> writes:
>>
>>> Use the recently introduced do_kernel_restart() function as the default restart
>>> handler if the platform did not explicitly provide a restart handler. This
>>> allows use restart handler that have been registered by device drivers to
>>> restart the machine.
>>>
>>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>>> ---
>>>   arch/mips/kernel/reset.c |    2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/mips/kernel/reset.c b/arch/mips/kernel/reset.c
>>> index 07fc524..36cd80c 100644
>>> --- a/arch/mips/kernel/reset.c
>>> +++ b/arch/mips/kernel/reset.c
>>> @@ -19,7 +19,7 @@
>>>    * So handle all using function pointers to machine specific
>>>    * functions.
>>>    */
>>> -void (*_machine_restart)(char *command);
>>> +void (*_machine_restart)(char *command) = do_kernel_restart;
>>>   void (*_machine_halt)(void);
>>>   void (*pm_power_off)(void);
>>
>> There is already a similar patch posted by Kevin Cernekee:
>> http://www.linux-mips.org/archives/linux-mips/2014-12/msg00410.html
>>
> Personally I prefer the earlier patch, though I guess that is personal
> preference.

They both achieve the same thing, though Kevin's is more in line with
what ARM does.  Missing from both is a fallback while(1) loop in case no
restart handlers are registered.  With the restart moved to the watchdog
driver, there's a possibility that this might happen.

-- 
Måns Rullgård
mans@mansr.com

Re: [PATCH 1/3] MIPS: Use do_kernel_restart() as the default restart handler

[Lars-Peter Clausen]

On 01/11/2015 01:15 PM, Måns Rullgård wrote:
> Guenter Roeck <linux@roeck-us.net> writes:
>
>> On 01/10/2015 12:08 PM, Måns Rullgård wrote:
>>> Lars-Peter Clausen <lars@metafoo.de> writes:
>>>
>>>> Use the recently introduced do_kernel_restart() function as the default restart
>>>> handler if the platform did not explicitly provide a restart handler. This
>>>> allows use restart handler that have been registered by device drivers to
>>>> restart the machine.
>>>>
>>>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>>>> ---
>>>>    arch/mips/kernel/reset.c |    2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/mips/kernel/reset.c b/arch/mips/kernel/reset.c
>>>> index 07fc524..36cd80c 100644
>>>> --- a/arch/mips/kernel/reset.c
>>>> +++ b/arch/mips/kernel/reset.c
>>>> @@ -19,7 +19,7 @@
>>>>     * So handle all using function pointers to machine specific
>>>>     * functions.
>>>>     */
>>>> -void (*_machine_restart)(char *command);
>>>> +void (*_machine_restart)(char *command) = do_kernel_restart;
>>>>    void (*_machine_halt)(void);
>>>>    void (*pm_power_off)(void);
>>>
>>> There is already a similar patch posted by Kevin Cernekee:
>>> http://www.linux-mips.org/archives/linux-mips/2014-12/msg00410.html
>>>
>> Personally I prefer the earlier patch, though I guess that is personal
>> preference.
>
> They both achieve the same thing, though Kevin's is more in line with
> what ARM does.  Missing from both is a fallback while(1) loop in case no
> restart handlers are registered.  With the restart moved to the watchdog
> driver, there's a possibility that this might happen.
>

In my opinion if such a fallback is needed it should be put into the kernel 
core reboot implementation and not into individual restart handler 
implementations.

My first version of this patch was do_kernel_restart() followed by a 
machine_halt() (so it goes to sleep instead of busy looping) as a fallback. 
But I couldn't find a good reason why that should be done at the individual 
restart handler level, so I dropped it.

- Lars

Re: [PATCH 1/3] MIPS: Use do_kernel_restart() as the default restart handler

[Måns Rullgård]

Lars-Peter Clausen <lars@metafoo.de> writes:

> On 01/11/2015 01:15 PM, Måns Rullgård wrote:
>> Guenter Roeck <linux@roeck-us.net> writes:
>>
>>> On 01/10/2015 12:08 PM, Måns Rullgård wrote:
>>>> Lars-Peter Clausen <lars@metafoo.de> writes:
>>>>
>>>>> Use the recently introduced do_kernel_restart() function as the default restart
>>>>> handler if the platform did not explicitly provide a restart handler. This
>>>>> allows use restart handler that have been registered by device drivers to
>>>>> restart the machine.
>>>>>
>>>>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>>>>> ---
>>>>>    arch/mips/kernel/reset.c |    2 +-
>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/arch/mips/kernel/reset.c b/arch/mips/kernel/reset.c
>>>>> index 07fc524..36cd80c 100644
>>>>> --- a/arch/mips/kernel/reset.c
>>>>> +++ b/arch/mips/kernel/reset.c
>>>>> @@ -19,7 +19,7 @@
>>>>>     * So handle all using function pointers to machine specific
>>>>>     * functions.
>>>>>     */
>>>>> -void (*_machine_restart)(char *command);
>>>>> +void (*_machine_restart)(char *command) = do_kernel_restart;
>>>>>    void (*_machine_halt)(void);
>>>>>    void (*pm_power_off)(void);
>>>>
>>>> There is already a similar patch posted by Kevin Cernekee:
>>>> http://www.linux-mips.org/archives/linux-mips/2014-12/msg00410.html
>>>>
>>> Personally I prefer the earlier patch, though I guess that is personal
>>> preference.
>>
>> They both achieve the same thing, though Kevin's is more in line with
>> what ARM does.  Missing from both is a fallback while(1) loop in case no
>> restart handlers are registered.  With the restart moved to the watchdog
>> driver, there's a possibility that this might happen.
>>
>
> In my opinion if such a fallback is needed it should be put into the
> kernel core reboot implementation and not into individual restart
> handler implementations.
>
> My first version of this patch was do_kernel_restart() followed by a
> machine_halt() (so it goes to sleep instead of busy looping) as a
> fallback. But I couldn't find a good reason why that should be done at
> the individual restart handler level, so I dropped it.

ARM does it its arch machine_restart() function.  MIPS should probably
follow suit.

-- 
Måns Rullgård
mans@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] MIPS: Use do_kernel_restart() as the default restart handler

[Lars-Peter Clausen]

On 01/11/2015 01:30 PM, Måns Rullgård wrote:
> Lars-Peter Clausen <lars@metafoo.de> writes:
>
>> On 01/11/2015 01:15 PM, Måns Rullgård wrote:
>>> Guenter Roeck <linux@roeck-us.net> writes:
>>>
>>>> On 01/10/2015 12:08 PM, Måns Rullgård wrote:
>>>>> Lars-Peter Clausen <lars@metafoo.de> writes:
>>>>>
>>>>>> Use the recently introduced do_kernel_restart() function as the default restart
>>>>>> handler if the platform did not explicitly provide a restart handler. This
>>>>>> allows use restart handler that have been registered by device drivers to
>>>>>> restart the machine.
>>>>>>
>>>>>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>>>>>> ---
>>>>>>     arch/mips/kernel/reset.c |    2 +-
>>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/arch/mips/kernel/reset.c b/arch/mips/kernel/reset.c
>>>>>> index 07fc524..36cd80c 100644
>>>>>> --- a/arch/mips/kernel/reset.c
>>>>>> +++ b/arch/mips/kernel/reset.c
>>>>>> @@ -19,7 +19,7 @@
>>>>>>      * So handle all using function pointers to machine specific
>>>>>>      * functions.
>>>>>>      */
>>>>>> -void (*_machine_restart)(char *command);
>>>>>> +void (*_machine_restart)(char *command) = do_kernel_restart;
>>>>>>     void (*_machine_halt)(void);
>>>>>>     void (*pm_power_off)(void);
>>>>>
>>>>> There is already a similar patch posted by Kevin Cernekee:
>>>>> http://www.linux-mips.org/archives/linux-mips/2014-12/msg00410.html
>>>>>
>>>> Personally I prefer the earlier patch, though I guess that is personal
>>>> preference.
>>>
>>> They both achieve the same thing, though Kevin's is more in line with
>>> what ARM does.  Missing from both is a fallback while(1) loop in case no
>>> restart handlers are registered.  With the restart moved to the watchdog
>>> driver, there's a possibility that this might happen.
>>>
>>
>> In my opinion if such a fallback is needed it should be put into the
>> kernel core reboot implementation and not into individual restart
>> handler implementations.
>>
>> My first version of this patch was do_kernel_restart() followed by a
>> machine_halt() (so it goes to sleep instead of busy looping) as a
>> fallback. But I couldn't find a good reason why that should be done at
>> the individual restart handler level, so I dropped it.
>
> ARM does it its arch machine_restart() function.  MIPS should probably
> follow suit.
>

That's just cargo culting...

Either it is required, then it should probably be moved into the core 
kernel, or it is not required in which case it can be removed from the ARM 
machine_restart() implementation.

Re: [PATCH 1/3] MIPS: Use do_kernel_restart() as the default restart handler

[Måns Rullgård]

Lars-Peter Clausen <lars@metafoo.de> writes:

> On 01/11/2015 01:30 PM, Måns Rullgård wrote:
>> Lars-Peter Clausen <lars@metafoo.de> writes:
>>
>>> On 01/11/2015 01:15 PM, Måns Rullgård wrote:
>>>> Guenter Roeck <linux@roeck-us.net> writes:
>>>>
>>>>> On 01/10/2015 12:08 PM, Måns Rullgård wrote:
>>>>>> Lars-Peter Clausen <lars@metafoo.de> writes:
>>>>>>
>>>>>>> Use the recently introduced do_kernel_restart() function as the default restart
>>>>>>> handler if the platform did not explicitly provide a restart handler. This
>>>>>>> allows use restart handler that have been registered by device drivers to
>>>>>>> restart the machine.
>>>>>>>
>>>>>>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>>>>>>> ---
>>>>>>>     arch/mips/kernel/reset.c |    2 +-
>>>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/arch/mips/kernel/reset.c b/arch/mips/kernel/reset.c
>>>>>>> index 07fc524..36cd80c 100644
>>>>>>> --- a/arch/mips/kernel/reset.c
>>>>>>> +++ b/arch/mips/kernel/reset.c
>>>>>>> @@ -19,7 +19,7 @@
>>>>>>>      * So handle all using function pointers to machine specific
>>>>>>>      * functions.
>>>>>>>      */
>>>>>>> -void (*_machine_restart)(char *command);
>>>>>>> +void (*_machine_restart)(char *command) = do_kernel_restart;
>>>>>>>     void (*_machine_halt)(void);
>>>>>>>     void (*pm_power_off)(void);
>>>>>>
>>>>>> There is already a similar patch posted by Kevin Cernekee:
>>>>>> http://www.linux-mips.org/archives/linux-mips/2014-12/msg00410.html
>>>>>>
>>>>> Personally I prefer the earlier patch, though I guess that is personal
>>>>> preference.
>>>>
>>>> They both achieve the same thing, though Kevin's is more in line with
>>>> what ARM does.  Missing from both is a fallback while(1) loop in case no
>>>> restart handlers are registered.  With the restart moved to the watchdog
>>>> driver, there's a possibility that this might happen.
>>>>
>>>
>>> In my opinion if such a fallback is needed it should be put into the
>>> kernel core reboot implementation and not into individual restart
>>> handler implementations.
>>>
>>> My first version of this patch was do_kernel_restart() followed by a
>>> machine_halt() (so it goes to sleep instead of busy looping) as a
>>> fallback. But I couldn't find a good reason why that should be done at
>>> the individual restart handler level, so I dropped it.
>>
>> ARM does it its arch machine_restart() function.  MIPS should probably
>> follow suit.
>>
>
> That's just cargo culting...

True.

> Either it is required, then it should probably be moved into the core
> kernel, or it is not required in which case it can be removed from the
> ARM machine_restart() implementation.

The quickest way to find out is probably to send a patch adding such
code to the end of kernel_restart().

-- 
Måns Rullgård
mans@mansr.com

Re: [PATCH 1/3] MIPS: Use do_kernel_restart() as the default restart handler

[Guenter Roeck]

On 01/11/2015 04:25 AM, Lars-Peter Clausen wrote:
> On 01/11/2015 01:15 PM, Måns Rullgård wrote:
>> Guenter Roeck <linux@roeck-us.net> writes:
>>
>>> On 01/10/2015 12:08 PM, Måns Rullgård wrote:
>>>> Lars-Peter Clausen <lars@metafoo.de> writes:
>>>>
>>>>> Use the recently introduced do_kernel_restart() function as the default restart
>>>>> handler if the platform did not explicitly provide a restart handler. This
>>>>> allows use restart handler that have been registered by device drivers to
>>>>> restart the machine.
>>>>>
>>>>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>>>>> ---
>>>>>    arch/mips/kernel/reset.c |    2 +-
>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/arch/mips/kernel/reset.c b/arch/mips/kernel/reset.c
>>>>> index 07fc524..36cd80c 100644
>>>>> --- a/arch/mips/kernel/reset.c
>>>>> +++ b/arch/mips/kernel/reset.c
>>>>> @@ -19,7 +19,7 @@
>>>>>     * So handle all using function pointers to machine specific
>>>>>     * functions.
>>>>>     */
>>>>> -void (*_machine_restart)(char *command);
>>>>> +void (*_machine_restart)(char *command) = do_kernel_restart;
>>>>>    void (*_machine_halt)(void);
>>>>>    void (*pm_power_off)(void);
>>>>
>>>> There is already a similar patch posted by Kevin Cernekee:
>>>> http://www.linux-mips.org/archives/linux-mips/2014-12/msg00410.html
>>>>
>>> Personally I prefer the earlier patch, though I guess that is personal
>>> preference.
>>
>> They both achieve the same thing, though Kevin's is more in line with
>> what ARM does.  Missing from both is a fallback while(1) loop in case no
>> restart handlers are registered.  With the restart moved to the watchdog
>> driver, there's a possibility that this might happen.
>>
>
> In my opinion if such a fallback is needed it should be put into the kernel core reboot implementation and not into individual restart handler implementations.
>
Agreed.

> My first version of this patch was do_kernel_restart() followed by a machine_halt() (so it goes to sleep instead of busy looping) as a fallback. But I couldn't find a good reason why that should be done at the individual restart handler level, so I dropped it.
>
That should probably be added to do_kernel_restart().

Guenter