[PATCH V3 1/2] ARM: SMP_TWD: WDOG: Start registers from 0x00 instead of 0x20

[PATCH V3 1/2] ARM: SMP_TWD: WDOG: Start registers from 0x00 instead of 0x20

[Viresh Kumar]

TWD_WDOG is at offset 0x20 from TWD base address. Current register offsets
contain this extra 0x20 offset, i.e. users were required to pass base address of
TWD instead of WDOG to WDOG driver.

Change this, so that users can pass base address of WDOG to WDOG driver instead
of TWD module. For this, subtract 0x20 from offsets of WDOG registers.

This could break any current users of TWD_WDOG, but i couldn't find any users of
this driver in current Linux tree. So, haven't fixed any platform code. If some
platforms are broken please report to me, so that we can get them fixed in this
patch only.

This also moves these register macros to mpcore_wdt.c, as there wouldn't be any
more users of these macros.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
V2->V3:
- Move register offsets from header file to source file.

 arch/arm/include/asm/smp_twd.h |   19 -------------------
 drivers/watchdog/mpcore_wdt.c  |   24 +++++++++++++++++++++++-
 2 files changed, 23 insertions(+), 20 deletions(-)

diff --git a/arch/arm/include/asm/smp_twd.h b/arch/arm/include/asm/smp_twd.h
index 57857d1..62640cf 100644
--- a/arch/arm/include/asm/smp_twd.h
+++ b/arch/arm/include/asm/smp_twd.h
@@ -6,30 +6,11 @@
 #define TWD_TIMER_CONTROL		0x08
 #define TWD_TIMER_INTSTAT		0x0C
 
-#define TWD_WDOG_LOAD			0x20
-#define TWD_WDOG_COUNTER		0x24
-#define TWD_WDOG_CONTROL		0x28
-#define TWD_WDOG_INTSTAT		0x2C
-#define TWD_WDOG_RESETSTAT		0x30
-#define TWD_WDOG_DISABLE		0x34
-
-#define TWD_WDOG_LOAD_MIN		0x00000000
-#define TWD_WDOG_LOAD_MAX		0xFFFFFFFF
-
 #define TWD_TIMER_CONTROL_ENABLE	(1 << 0)
 #define TWD_TIMER_CONTROL_ONESHOT	(0 << 1)
 #define TWD_TIMER_CONTROL_PERIODIC	(1 << 1)
 #define TWD_TIMER_CONTROL_IT_ENABLE	(1 << 2)
 
-#define TWD_WDOG_CONTROL_ENABLE		(1 << 0)
-#define TWD_WDOG_CONTROL_IRQ_ENABLE	(1 << 2)
-#define TWD_WDOG_CONTROL_WDT_MODE	(1 << 3)
-#define TWD_WDOG_CONTROL_WDT_PRESCALE(x)	((x) << 8)
-#define TWD_WDOG_CONTROL_PRESCALE_MIN	0x00
-#define TWD_WDOG_CONTROL_PRESCALE_MAX	0xFF
-
-#define TWD_WDOG_RESETSTAT_MASK		0x1
-
 #include <linux/ioport.h>
 
 struct twd_local_timer {
diff --git a/drivers/watchdog/mpcore_wdt.c b/drivers/watchdog/mpcore_wdt.c
index 7c00455..13197c7 100644
--- a/drivers/watchdog/mpcore_wdt.c
+++ b/drivers/watchdog/mpcore_wdt.c
@@ -34,7 +34,29 @@
 #include <linux/types.h>
 #include <linux/watchdog.h>
 
-#include <asm/smp_twd.h>
+/*
+ * TWD_WDOG is at offset 0x20 from TWD base address. Following register offsets
+ * doesn't contain this extra 0x20 offset, i.e. users of TWD_WDOG must pass base
+ * address of WDOG to WDOG driver instead of TWD module.
+ */
+#define TWD_WDOG_LOAD				0x00
+#define TWD_WDOG_COUNTER			0x04
+#define TWD_WDOG_CONTROL			0x08
+#define TWD_WDOG_INTSTAT			0x0C
+#define TWD_WDOG_RESETSTAT			0x10
+#define TWD_WDOG_DISABLE			0x14
+
+#define TWD_WDOG_LOAD_MIN			0x00000000
+#define TWD_WDOG_LOAD_MAX			0xFFFFFFFF
+
+#define TWD_WDOG_CONTROL_ENABLE			(1 << 0)
+#define TWD_WDOG_CONTROL_IRQ_ENABLE		(1 << 2)
+#define TWD_WDOG_CONTROL_WDT_MODE		(1 << 3)
+#define TWD_WDOG_CONTROL_WDT_PRESCALE(x)	((x) << 8)
+#define TWD_WDOG_CONTROL_PRESCALE_MIN		0x00
+#define TWD_WDOG_CONTROL_PRESCALE_MAX		0xFF
+
+#define TWD_WDOG_RESETSTAT_MASK			0x1
 
 struct mpcore_wdt {
 	struct watchdog_device wdd;
-- 
1.7.9

[PATCH V3 1/2] ARM: SMP_TWD: WDOG: Start registers from 0x00 instead of 0x20

[Viresh Kumar]

TWD_WDOG is at offset 0x20 from TWD base address. Current register offsets
contain this extra 0x20 offset, i.e. users were required to pass base address of
TWD instead of WDOG to WDOG driver.

Change this, so that users can pass base address of WDOG to WDOG driver instead
of TWD module. For this, subtract 0x20 from offsets of WDOG registers.

This could break any current users of TWD_WDOG, but i couldn't find any users of
this driver in current Linux tree. So, haven't fixed any platform code. If some
platforms are broken please report to me, so that we can get them fixed in this
patch only.

This also moves these register macros to mpcore_wdt.c, as there wouldn't be any
more users of these macros.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
V2->V3:
- Move register offsets from header file to source file.

 arch/arm/include/asm/smp_twd.h |   19 -------------------
 drivers/watchdog/mpcore_wdt.c  |   24 +++++++++++++++++++++++-
 2 files changed, 23 insertions(+), 20 deletions(-)

diff --git a/arch/arm/include/asm/smp_twd.h b/arch/arm/include/asm/smp_twd.h
index 57857d1..62640cf 100644
--- a/arch/arm/include/asm/smp_twd.h
+++ b/arch/arm/include/asm/smp_twd.h
@@ -6,30 +6,11 @@
 #define TWD_TIMER_CONTROL		0x08
 #define TWD_TIMER_INTSTAT		0x0C
 
-#define TWD_WDOG_LOAD			0x20
-#define TWD_WDOG_COUNTER		0x24
-#define TWD_WDOG_CONTROL		0x28
-#define TWD_WDOG_INTSTAT		0x2C
-#define TWD_WDOG_RESETSTAT		0x30
-#define TWD_WDOG_DISABLE		0x34
-
-#define TWD_WDOG_LOAD_MIN		0x00000000
-#define TWD_WDOG_LOAD_MAX		0xFFFFFFFF
-
 #define TWD_TIMER_CONTROL_ENABLE	(1 << 0)
 #define TWD_TIMER_CONTROL_ONESHOT	(0 << 1)
 #define TWD_TIMER_CONTROL_PERIODIC	(1 << 1)
 #define TWD_TIMER_CONTROL_IT_ENABLE	(1 << 2)
 
-#define TWD_WDOG_CONTROL_ENABLE		(1 << 0)
-#define TWD_WDOG_CONTROL_IRQ_ENABLE	(1 << 2)
-#define TWD_WDOG_CONTROL_WDT_MODE	(1 << 3)
-#define TWD_WDOG_CONTROL_WDT_PRESCALE(x)	((x) << 8)
-#define TWD_WDOG_CONTROL_PRESCALE_MIN	0x00
-#define TWD_WDOG_CONTROL_PRESCALE_MAX	0xFF
-
-#define TWD_WDOG_RESETSTAT_MASK		0x1
-
 #include <linux/ioport.h>
 
 struct twd_local_timer {
diff --git a/drivers/watchdog/mpcore_wdt.c b/drivers/watchdog/mpcore_wdt.c
index 7c00455..13197c7 100644
--- a/drivers/watchdog/mpcore_wdt.c
+++ b/drivers/watchdog/mpcore_wdt.c
@@ -34,7 +34,29 @@
 #include <linux/types.h>
 #include <linux/watchdog.h>
 
-#include <asm/smp_twd.h>
+/*
+ * TWD_WDOG is at offset 0x20 from TWD base address. Following register offsets
+ * doesn't contain this extra 0x20 offset, i.e. users of TWD_WDOG must pass base
+ * address of WDOG to WDOG driver instead of TWD module.
+ */
+#define TWD_WDOG_LOAD				0x00
+#define TWD_WDOG_COUNTER			0x04
+#define TWD_WDOG_CONTROL			0x08
+#define TWD_WDOG_INTSTAT			0x0C
+#define TWD_WDOG_RESETSTAT			0x10
+#define TWD_WDOG_DISABLE			0x14
+
+#define TWD_WDOG_LOAD_MIN			0x00000000
+#define TWD_WDOG_LOAD_MAX			0xFFFFFFFF
+
+#define TWD_WDOG_CONTROL_ENABLE			(1 << 0)
+#define TWD_WDOG_CONTROL_IRQ_ENABLE		(1 << 2)
+#define TWD_WDOG_CONTROL_WDT_MODE		(1 << 3)
+#define TWD_WDOG_CONTROL_WDT_PRESCALE(x)	((x) << 8)
+#define TWD_WDOG_CONTROL_PRESCALE_MIN		0x00
+#define TWD_WDOG_CONTROL_PRESCALE_MAX		0xFF
+
+#define TWD_WDOG_RESETSTAT_MASK			0x1
 
 struct mpcore_wdt {
 	struct watchdog_device wdd;
-- 
1.7.9

[PATCH V3 2/2] watchdog: mpcore: Add DT probing support for ARM mpcore watchdog

[Viresh Kumar]

This patch adds Device tree probing support for ARM Mpcore watchdog. Its binding
were already documented in Documentation/devicetree/bindings/arm/twd.txt.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
V2->V3:
- Nothing. Just resend it.

 drivers/watchdog/mpcore_wdt.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/drivers/watchdog/mpcore_wdt.c b/drivers/watchdog/mpcore_wdt.c
index 13197c7..3690af3 100644
--- a/drivers/watchdog/mpcore_wdt.c
+++ b/drivers/watchdog/mpcore_wdt.c
@@ -27,6 +27,7 @@
 #include <linux/io.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/pm.h>
 #include <linux/reboot.h>
@@ -424,6 +425,16 @@ static SIMPLE_DEV_PM_OPS(mpcore_wdt_dev_pm_ops, mpcore_wdt_suspend,
 /* work with hotplug and coldplug */
 MODULE_ALIAS("platform:mpcore_wdt");
 
+#ifdef CONFIG_OF
+static const struct of_device_id mpcore_wdt_id_table[] = {
+	{ .compatible = "arm,cortex-a9-twd-wdt" },
+	{ .compatible = "arm,cortex-a5-twd-wdt" },
+	{ .compatible = "arm,arm11mp-twd-wdt" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, mpcore_wdt_id_table);
+#endif
+
 static struct platform_driver mpcore_wdt_driver = {
 	.probe		= mpcore_wdt_probe,
 	.remove		= __devexit_p(mpcore_wdt_remove),
@@ -432,6 +443,7 @@ static struct platform_driver mpcore_wdt_driver = {
 		.owner	= THIS_MODULE,
 		.name	= "mpcore_wdt",
 		.pm	= &mpcore_wdt_dev_pm_ops,
+		.of_match_table = of_match_ptr(mpcore_wdt_id_table),
 	},
 };
 
-- 
1.7.9

[PATCH V3 2/2] watchdog: mpcore: Add DT probing support for ARM mpcore watchdog

[Viresh Kumar]

This patch adds Device tree probing support for ARM Mpcore watchdog. Its binding
were already documented in Documentation/devicetree/bindings/arm/twd.txt.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
V2->V3:
- Nothing. Just resend it.

 drivers/watchdog/mpcore_wdt.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/drivers/watchdog/mpcore_wdt.c b/drivers/watchdog/mpcore_wdt.c
index 13197c7..3690af3 100644
--- a/drivers/watchdog/mpcore_wdt.c
+++ b/drivers/watchdog/mpcore_wdt.c
@@ -27,6 +27,7 @@
 #include <linux/io.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/pm.h>
 #include <linux/reboot.h>
@@ -424,6 +425,16 @@ static SIMPLE_DEV_PM_OPS(mpcore_wdt_dev_pm_ops, mpcore_wdt_suspend,
 /* work with hotplug and coldplug */
 MODULE_ALIAS("platform:mpcore_wdt");
 
+#ifdef CONFIG_OF
+static const struct of_device_id mpcore_wdt_id_table[] = {
+	{ .compatible = "arm,cortex-a9-twd-wdt" },
+	{ .compatible = "arm,cortex-a5-twd-wdt" },
+	{ .compatible = "arm,arm11mp-twd-wdt" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, mpcore_wdt_id_table);
+#endif
+
 static struct platform_driver mpcore_wdt_driver = {
 	.probe		= mpcore_wdt_probe,
 	.remove		= __devexit_p(mpcore_wdt_remove),
@@ -432,6 +443,7 @@ static struct platform_driver mpcore_wdt_driver = {
 		.owner	= THIS_MODULE,
 		.name	= "mpcore_wdt",
 		.pm	= &mpcore_wdt_dev_pm_ops,
+		.of_match_table = of_match_ptr(mpcore_wdt_id_table),
 	},
 };
 
-- 
1.7.9

Re: [PATCH V3 2/2] watchdog: mpcore: Add DT probing support for ARM mpcore watchdog

[Marc Zyngier]

On 24/04/12 06:50, Viresh Kumar wrote:
> This patch adds Device tree probing support for ARM Mpcore watchdog. Its binding
> were already documented in Documentation/devicetree/bindings/arm/twd.txt.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
> ---
> V2->V3:
> - Nothing. Just resend it.

I'm sorry, but I really have to ask: What is the point of adding DT
support to this driver when it is obvious that it is already broken?

As mentioned yesterday, interrupt request doesn't use the right API,
which makes it very unlikely that the probe function will succeed.

Furthermore, I cannot see anything in this driver that ensures the
userspace ioctl() will end-up kicking the watchdog on the right CPU.

So if you really care about this driver, please have a look at the above
issues. Otherwise this is only pointless churn.

Thanks,

	M.

>  drivers/watchdog/mpcore_wdt.c |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/watchdog/mpcore_wdt.c b/drivers/watchdog/mpcore_wdt.c
> index 13197c7..3690af3 100644
> --- a/drivers/watchdog/mpcore_wdt.c
> +++ b/drivers/watchdog/mpcore_wdt.c
> @@ -27,6 +27,7 @@
>  #include <linux/io.h>
>  #include <linux/module.h>
>  #include <linux/moduleparam.h>
> +#include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm.h>
>  #include <linux/reboot.h>
> @@ -424,6 +425,16 @@ static SIMPLE_DEV_PM_OPS(mpcore_wdt_dev_pm_ops, mpcore_wdt_suspend,
>  /* work with hotplug and coldplug */
>  MODULE_ALIAS("platform:mpcore_wdt");
>  
> +#ifdef CONFIG_OF
> +static const struct of_device_id mpcore_wdt_id_table[] = {
> +	{ .compatible = "arm,cortex-a9-twd-wdt" },
> +	{ .compatible = "arm,cortex-a5-twd-wdt" },
> +	{ .compatible = "arm,arm11mp-twd-wdt" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, mpcore_wdt_id_table);
> +#endif
> +
>  static struct platform_driver mpcore_wdt_driver = {
>  	.probe		= mpcore_wdt_probe,
>  	.remove		= __devexit_p(mpcore_wdt_remove),
> @@ -432,6 +443,7 @@ static struct platform_driver mpcore_wdt_driver = {
>  		.owner	= THIS_MODULE,
>  		.name	= "mpcore_wdt",
>  		.pm	= &mpcore_wdt_dev_pm_ops,
> +		.of_match_table = of_match_ptr(mpcore_wdt_id_table),
>  	},
>  };
>  


-- 
Jazz is not dead. It just smells funny...

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

[PATCH V3 2/2] watchdog: mpcore: Add DT probing support for ARM mpcore watchdog

[Marc Zyngier]

On 24/04/12 06:50, Viresh Kumar wrote:
> This patch adds Device tree probing support for ARM Mpcore watchdog. Its binding
> were already documented in Documentation/devicetree/bindings/arm/twd.txt.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
> ---
> V2->V3:
> - Nothing. Just resend it.

I'm sorry, but I really have to ask: What is the point of adding DT
support to this driver when it is obvious that it is already broken?

As mentioned yesterday, interrupt request doesn't use the right API,
which makes it very unlikely that the probe function will succeed.

Furthermore, I cannot see anything in this driver that ensures the
userspace ioctl() will end-up kicking the watchdog on the right CPU.

So if you really care about this driver, please have a look at the above
issues. Otherwise this is only pointless churn.

Thanks,

	M.

>  drivers/watchdog/mpcore_wdt.c |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/watchdog/mpcore_wdt.c b/drivers/watchdog/mpcore_wdt.c
> index 13197c7..3690af3 100644
> --- a/drivers/watchdog/mpcore_wdt.c
> +++ b/drivers/watchdog/mpcore_wdt.c
> @@ -27,6 +27,7 @@
>  #include <linux/io.h>
>  #include <linux/module.h>
>  #include <linux/moduleparam.h>
> +#include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm.h>
>  #include <linux/reboot.h>
> @@ -424,6 +425,16 @@ static SIMPLE_DEV_PM_OPS(mpcore_wdt_dev_pm_ops, mpcore_wdt_suspend,
>  /* work with hotplug and coldplug */
>  MODULE_ALIAS("platform:mpcore_wdt");
>  
> +#ifdef CONFIG_OF
> +static const struct of_device_id mpcore_wdt_id_table[] = {
> +	{ .compatible = "arm,cortex-a9-twd-wdt" },
> +	{ .compatible = "arm,cortex-a5-twd-wdt" },
> +	{ .compatible = "arm,arm11mp-twd-wdt" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, mpcore_wdt_id_table);
> +#endif
> +
>  static struct platform_driver mpcore_wdt_driver = {
>  	.probe		= mpcore_wdt_probe,
>  	.remove		= __devexit_p(mpcore_wdt_remove),
> @@ -432,6 +443,7 @@ static struct platform_driver mpcore_wdt_driver = {
>  		.owner	= THIS_MODULE,
>  		.name	= "mpcore_wdt",
>  		.pm	= &mpcore_wdt_dev_pm_ops,
> +		.of_match_table = of_match_ptr(mpcore_wdt_id_table),
>  	},
>  };
>  


-- 
Jazz is not dead. It just smells funny...

Re: [PATCH V3 2/2] watchdog: mpcore: Add DT probing support for ARM mpcore watchdog

[Viresh Kumar]

On 4/24/2012 2:56 PM, Marc Zyngier wrote:
> I'm sorry, but I really have to ask: What is the point of adding DT
> support to this driver when it is obvious that it is already broken?

Interrupt is only for testing. And that's why it worked for me earlier,
as i didn't passed irq from DT.

So, i believe this patch still makes sense. Obviously we can have another
patch to get interrupts fixed.

-- 
viresh

[PATCH V3 2/2] watchdog: mpcore: Add DT probing support for ARM mpcore watchdog

[Viresh Kumar]

On 4/24/2012 2:56 PM, Marc Zyngier wrote:
> I'm sorry, but I really have to ask: What is the point of adding DT
> support to this driver when it is obvious that it is already broken?

Interrupt is only for testing. And that's why it worked for me earlier,
as i didn't passed irq from DT.

So, i believe this patch still makes sense. Obviously we can have another
patch to get interrupts fixed.

-- 
viresh

Re: [PATCH V3 2/2] watchdog: mpcore: Add DT probing support for ARM mpcore watchdog

[Marc Zyngier]

On 24/04/12 11:35, Viresh Kumar wrote:
> On 4/24/2012 2:56 PM, Marc Zyngier wrote:
>> I'm sorry, but I really have to ask: What is the point of adding DT
>> support to this driver when it is obvious that it is already broken?
> 
> Interrupt is only for testing. And that's why it worked for me earlier,
> as i didn't passed irq from DT.
> 
> So, i believe this patch still makes sense. Obviously we can have another
> patch to get interrupts fixed.

That's exactly what I object to. You're giving a false sense of
usability ("see, this driver is maintained, it even has DT support!"),
while the damn thing has not worked for quite a long time. And I don't
mean only the interrupt. The driver is not working properly on an SMP
system.

At that point, and unless someone is willing to step up and fix it, I'd
rather make it depend on CONFIG_BROKEN.

	M.
-- 
Jazz is not dead. It just smells funny...

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

[PATCH V3 2/2] watchdog: mpcore: Add DT probing support for ARM mpcore watchdog

[Marc Zyngier]

On 24/04/12 11:35, Viresh Kumar wrote:
> On 4/24/2012 2:56 PM, Marc Zyngier wrote:
>> I'm sorry, but I really have to ask: What is the point of adding DT
>> support to this driver when it is obvious that it is already broken?
> 
> Interrupt is only for testing. And that's why it worked for me earlier,
> as i didn't passed irq from DT.
> 
> So, i believe this patch still makes sense. Obviously we can have another
> patch to get interrupts fixed.

That's exactly what I object to. You're giving a false sense of
usability ("see, this driver is maintained, it even has DT support!"),
while the damn thing has not worked for quite a long time. And I don't
mean only the interrupt. The driver is not working properly on an SMP
system.

At that point, and unless someone is willing to step up and fix it, I'd
rather make it depend on CONFIG_BROKEN.

	M.
-- 
Jazz is not dead. It just smells funny...