All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/3] MIPS: Use do_kernel_restart() as the default restart handler
@ 2015-01-10 18:29 Lars-Peter Clausen
  2015-01-10 18:29 ` [PATCH 2/3] MIPS: qi_lb60: Register watchdog device Lars-Peter Clausen
                   ` (2 more replies)
  0 siblings, 3 replies; 21+ messages in thread
From: Lars-Peter Clausen @ 2015-01-10 18:29 UTC (permalink / raw)
  To: Ralf Baechle
  Cc: Guenter Roeck, Wim Van Sebroeck, Paul Burton, Paul Cercueil,
	Maarten ter Huurne, linux-mips, linux-watchdog,
	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

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 2/3] MIPS: qi_lb60: Register watchdog device
  2015-01-10 18:29 [PATCH 1/3] MIPS: Use do_kernel_restart() as the default restart handler Lars-Peter Clausen
@ 2015-01-10 18:29 ` Lars-Peter Clausen
  2015-01-11  1:24   ` Guenter Roeck
  2015-01-10 18:29 ` [PATCH 3/3] MIPS: jz4740: Move reset code to the watchdog driver Lars-Peter Clausen
  2015-01-10 20:08   ` Måns Rullgård
  2 siblings, 1 reply; 21+ messages in thread
From: Lars-Peter Clausen @ 2015-01-10 18:29 UTC (permalink / raw)
  To: Ralf Baechle
  Cc: Guenter Roeck, Wim Van Sebroeck, Paul Burton, Paul Cercueil,
	Maarten ter Huurne, linux-mips, linux-watchdog,
	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

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 3/3] MIPS: jz4740: Move reset code to the watchdog driver
  2015-01-10 18:29 [PATCH 1/3] MIPS: Use do_kernel_restart() as the default restart handler Lars-Peter Clausen
  2015-01-10 18:29 ` [PATCH 2/3] MIPS: qi_lb60: Register watchdog device Lars-Peter Clausen
@ 2015-01-10 18:29 ` Lars-Peter Clausen
  2015-01-11  1:18   ` Guenter Roeck
  2015-01-10 20:08   ` Måns Rullgård
  2 siblings, 1 reply; 21+ messages in thread
From: Lars-Peter Clausen @ 2015-01-10 18:29 UTC (permalink / raw)
  To: Ralf Baechle
  Cc: Guenter Roeck, Wim Van Sebroeck, Paul Burton, Paul Cercueil,
	Maarten ter Huurne, linux-mips, linux-watchdog,
	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

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/3] MIPS: Use do_kernel_restart() as the default restart handler
  2015-01-10 18:29 [PATCH 1/3] MIPS: Use do_kernel_restart() as the default restart handler Lars-Peter Clausen
@ 2015-01-10 20:08   ` Måns Rullgård
  2015-01-10 18:29 ` [PATCH 3/3] MIPS: jz4740: Move reset code to the watchdog driver Lars-Peter Clausen
  2015-01-10 20:08   ` Måns Rullgård
  2 siblings, 0 replies; 21+ messages in thread
From: Måns Rullgård @ 2015-01-10 20:08 UTC (permalink / raw)
  To: Lars-Peter Clausen
  Cc: Ralf Baechle, Guenter Roeck, Wim Van Sebroeck, Paul Burton,
	Paul Cercueil, Maarten ter Huurne, linux-mips, linux-watchdog

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/3] MIPS: Use do_kernel_restart() as the default restart handler
@ 2015-01-10 20:08   ` Måns Rullgård
  0 siblings, 0 replies; 21+ messages in thread
From: Måns Rullgård @ 2015-01-10 20:08 UTC (permalink / raw)
  To: Lars-Peter Clausen
  Cc: Ralf Baechle, Guenter Roeck, Wim Van Sebroeck, Paul Burton,
	Paul Cercueil, Maarten ter Huurne, linux-mips, linux-watchdog

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 3/3] MIPS: jz4740: Move reset code to the watchdog driver
  2015-01-10 18:29 ` [PATCH 3/3] MIPS: jz4740: Move reset code to the watchdog driver Lars-Peter Clausen
@ 2015-01-11  1:18   ` Guenter Roeck
  2015-01-11  1:43     ` Maarten ter Huurne
  0 siblings, 1 reply; 21+ messages in thread
From: Guenter Roeck @ 2015-01-11  1:18 UTC (permalink / raw)
  To: Lars-Peter Clausen, Ralf Baechle
  Cc: Wim Van Sebroeck, Paul Burton, Paul Cercueil, Maarten ter Huurne,
	linux-mips, linux-watchdog

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);
>


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/3] MIPS: Use do_kernel_restart() as the default restart handler
  2015-01-10 20:08   ` Måns Rullgård
@ 2015-01-11  1:19     ` Guenter Roeck
  -1 siblings, 0 replies; 21+ messages in thread
From: Guenter Roeck @ 2015-01-11  1:19 UTC (permalink / raw)
  To: Måns Rullgård, Lars-Peter Clausen
  Cc: Ralf Baechle, Wim Van Sebroeck, Paul Burton, Paul Cercueil,
	Maarten ter Huurne, linux-mips, linux-watchdog

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/3] MIPS: Use do_kernel_restart() as the default restart handler
@ 2015-01-11  1:19     ` Guenter Roeck
  0 siblings, 0 replies; 21+ messages in thread
From: Guenter Roeck @ 2015-01-11  1:19 UTC (permalink / raw)
  To: Måns Rullgård, Lars-Peter Clausen
  Cc: Ralf Baechle, Wim Van Sebroeck, Paul Burton, Paul Cercueil,
	Maarten ter Huurne, linux-mips, linux-watchdog

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/3] MIPS: qi_lb60: Register watchdog device
  2015-01-10 18:29 ` [PATCH 2/3] MIPS: qi_lb60: Register watchdog device Lars-Peter Clausen
@ 2015-01-11  1:24   ` Guenter Roeck
  2015-01-11  1:37     ` Maarten ter Huurne
  0 siblings, 1 reply; 21+ messages in thread
From: Guenter Roeck @ 2015-01-11  1:24 UTC (permalink / raw)
  To: Lars-Peter Clausen, Ralf Baechle
  Cc: Wim Van Sebroeck, Paul Burton, Paul Cercueil, Maarten ter Huurne,
	linux-mips, linux-watchdog

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,
>


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/3] MIPS: qi_lb60: Register watchdog device
  2015-01-11  1:24   ` Guenter Roeck
@ 2015-01-11  1:37     ` Maarten ter Huurne
  2015-01-11 16:17       ` Guenter Roeck
  0 siblings, 1 reply; 21+ messages in thread
From: Maarten ter Huurne @ 2015-01-11  1:37 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Lars-Peter Clausen, Ralf Baechle, Wim Van Sebroeck, Paul Burton,
	Paul Cercueil, linux-mips, linux-watchdog

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 3/3] MIPS: jz4740: Move reset code to the watchdog driver
  2015-01-11  1:18   ` Guenter Roeck
@ 2015-01-11  1:43     ` Maarten ter Huurne
  2015-01-11  9:41       ` Lars-Peter Clausen
  0 siblings, 1 reply; 21+ messages in thread
From: Maarten ter Huurne @ 2015-01-11  1:43 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Lars-Peter Clausen, Ralf Baechle, Wim Van Sebroeck, Paul Burton,
	Paul Cercueil, linux-mips, linux-watchdog

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 3/3] MIPS: jz4740: Move reset code to the watchdog driver
  2015-01-11  1:43     ` Maarten ter Huurne
@ 2015-01-11  9:41       ` Lars-Peter Clausen
  2015-01-11 16:16         ` Guenter Roeck
  0 siblings, 1 reply; 21+ messages in thread
From: Lars-Peter Clausen @ 2015-01-11  9:41 UTC (permalink / raw)
  To: Maarten ter Huurne, Guenter Roeck
  Cc: Ralf Baechle, Wim Van Sebroeck, Paul Burton, Paul Cercueil,
	linux-mips, linux-watchdog

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/3] MIPS: Use do_kernel_restart() as the default restart handler
  2015-01-11  1:19     ` Guenter Roeck
  (?)
@ 2015-01-11 12:15     ` Måns Rullgård
  2015-01-11 12:25       ` Lars-Peter Clausen
  -1 siblings, 1 reply; 21+ messages in thread
From: Måns Rullgård @ 2015-01-11 12:15 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Lars-Peter Clausen, Ralf Baechle, Wim Van Sebroeck, Paul Burton,
	Paul Cercueil, Maarten ter Huurne, linux-mips, linux-watchdog

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/3] MIPS: Use do_kernel_restart() as the default restart handler
  2015-01-11 12:15     ` Måns Rullgård
@ 2015-01-11 12:25       ` Lars-Peter Clausen
  2015-01-11 12:30           ` Måns Rullgård
  2015-01-11 16:13         ` Guenter Roeck
  0 siblings, 2 replies; 21+ messages in thread
From: Lars-Peter Clausen @ 2015-01-11 12:25 UTC (permalink / raw)
  To: Måns Rullgård, Guenter Roeck
  Cc: Ralf Baechle, Wim Van Sebroeck, Paul Burton, Paul Cercueil,
	Maarten ter Huurne, linux-mips, linux-watchdog

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/3] MIPS: Use do_kernel_restart() as the default restart handler
  2015-01-11 12:25       ` Lars-Peter Clausen
@ 2015-01-11 12:30           ` Måns Rullgård
  2015-01-11 16:13         ` Guenter Roeck
  1 sibling, 0 replies; 21+ messages in thread
From: Måns Rullgård @ 2015-01-11 12:30 UTC (permalink / raw)
  To: Lars-Peter Clausen
  Cc: Guenter Roeck, Ralf Baechle, Wim Van Sebroeck, Paul Burton,
	Paul Cercueil, Maarten ter Huurne, linux-mips, linux-watchdog

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/3] MIPS: Use do_kernel_restart() as the default restart handler
@ 2015-01-11 12:30           ` Måns Rullgård
  0 siblings, 0 replies; 21+ messages in thread
From: Måns Rullgård @ 2015-01-11 12:30 UTC (permalink / raw)
  To: Lars-Peter Clausen
  Cc: Guenter Roeck, Ralf Baechle, Wim Van Sebroeck, Paul Burton,
	Paul Cercueil, Maarten ter Huurne, linux-mips, linux-watchdog

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/3] MIPS: Use do_kernel_restart() as the default restart handler
  2015-01-11 12:30           ` Måns Rullgård
  (?)
@ 2015-01-11 12:41           ` Lars-Peter Clausen
  2015-01-11 12:45             ` Måns Rullgård
  -1 siblings, 1 reply; 21+ messages in thread
From: Lars-Peter Clausen @ 2015-01-11 12:41 UTC (permalink / raw)
  To: Måns Rullgård
  Cc: Guenter Roeck, Ralf Baechle, Wim Van Sebroeck, Paul Burton,
	Paul Cercueil, Maarten ter Huurne, linux-mips, linux-watchdog

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.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/3] MIPS: Use do_kernel_restart() as the default restart handler
  2015-01-11 12:41           ` Lars-Peter Clausen
@ 2015-01-11 12:45             ` Måns Rullgård
  0 siblings, 0 replies; 21+ messages in thread
From: Måns Rullgård @ 2015-01-11 12:45 UTC (permalink / raw)
  To: Lars-Peter Clausen
  Cc: Guenter Roeck, Ralf Baechle, Wim Van Sebroeck, Paul Burton,
	Paul Cercueil, Maarten ter Huurne, linux-mips, linux-watchdog

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/3] MIPS: Use do_kernel_restart() as the default restart handler
  2015-01-11 12:25       ` Lars-Peter Clausen
  2015-01-11 12:30           ` Måns Rullgård
@ 2015-01-11 16:13         ` Guenter Roeck
  1 sibling, 0 replies; 21+ messages in thread
From: Guenter Roeck @ 2015-01-11 16:13 UTC (permalink / raw)
  To: Lars-Peter Clausen, Måns Rullgård
  Cc: Ralf Baechle, Wim Van Sebroeck, Paul Burton, Paul Cercueil,
	Maarten ter Huurne, linux-mips, linux-watchdog

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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 3/3] MIPS: jz4740: Move reset code to the watchdog driver
  2015-01-11  9:41       ` Lars-Peter Clausen
@ 2015-01-11 16:16         ` Guenter Roeck
  0 siblings, 0 replies; 21+ messages in thread
From: Guenter Roeck @ 2015-01-11 16:16 UTC (permalink / raw)
  To: Lars-Peter Clausen, Maarten ter Huurne
  Cc: Ralf Baechle, Wim Van Sebroeck, Paul Burton, Paul Cercueil,
	linux-mips, linux-watchdog

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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/3] MIPS: qi_lb60: Register watchdog device
  2015-01-11  1:37     ` Maarten ter Huurne
@ 2015-01-11 16:17       ` Guenter Roeck
  0 siblings, 0 replies; 21+ messages in thread
From: Guenter Roeck @ 2015-01-11 16:17 UTC (permalink / raw)
  To: Maarten ter Huurne
  Cc: Lars-Peter Clausen, Ralf Baechle, Wim Van Sebroeck, Paul Burton,
	Paul Cercueil, linux-mips, linux-watchdog

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


^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2015-01-11 16:17 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-10 18:29 [PATCH 1/3] MIPS: Use do_kernel_restart() as the default restart handler Lars-Peter Clausen
2015-01-10 18:29 ` [PATCH 2/3] MIPS: qi_lb60: Register watchdog device Lars-Peter Clausen
2015-01-11  1:24   ` Guenter Roeck
2015-01-11  1:37     ` Maarten ter Huurne
2015-01-11 16:17       ` Guenter Roeck
2015-01-10 18:29 ` [PATCH 3/3] MIPS: jz4740: Move reset code to the watchdog driver Lars-Peter Clausen
2015-01-11  1:18   ` Guenter Roeck
2015-01-11  1:43     ` Maarten ter Huurne
2015-01-11  9:41       ` Lars-Peter Clausen
2015-01-11 16:16         ` Guenter Roeck
2015-01-10 20:08 ` [PATCH 1/3] MIPS: Use do_kernel_restart() as the default restart handler Måns Rullgård
2015-01-10 20:08   ` Måns Rullgård
2015-01-11  1:19   ` Guenter Roeck
2015-01-11  1:19     ` Guenter Roeck
2015-01-11 12:15     ` Måns Rullgård
2015-01-11 12:25       ` Lars-Peter Clausen
2015-01-11 12:30         ` Måns Rullgård
2015-01-11 12:30           ` Måns Rullgård
2015-01-11 12:41           ` Lars-Peter Clausen
2015-01-11 12:45             ` Måns Rullgård
2015-01-11 16:13         ` Guenter Roeck

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.