[PATCH] powerpc/mpc52xx/wdt: Fix 5200 wdt always being used as gpt

[PATCH] powerpc/mpc52xx/wdt: Fix 5200 wdt always being used as gpt

[Albrecht Dreß]

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

In the current code, all MPC5200 timers are registered by the  
mpc52xx_gpt driver, even if gpt0 (the only one with this capability)  
shall be used as hardware watchdog which is indicated by the  
"fsl,has-wdt" or "has-wdt" property in the device tree.  Thus, the  
watchdog driver does never find any watchdog and simply doesn't work.

This trivial patch protects timers with a "(fsl,)?has-wdt" property  
from being probed as gpt's.  The watchdog timer now works just fine.

Tested on a custom (roughly Icecube based) MPC5200B board, with the  
5200 watchdog driver built into the kernel.

Signed-off-by: Albrecht Dreß <albrecht.dress@arcor.de>

---

--- linux-2.6.30.3.orig/arch/powerpc/platforms/52xx/mpc52xx_gpt.c	 
2009-07-24 23:47:51.000000000 +0200
+++ linux-2.6.30.3/arch/powerpc/platforms/52xx/mpc52xx_gpt.c	 
2009-08-03 14:20:10.000000000 +0200
@@ -343,6 +343,14 @@
  {
  	struct mpc52xx_gpt_priv *gpt;

+	/* do not grab devices which shall be used as watchdog */
+	if (of_get_property(ofdev->node, "fsl,has-wdt", NULL) ||
+		of_get_property(ofdev->node, "has-wdt", NULL)) {
+		pr_notice("%s: ignore wdt %s\n", __func__,
+			  ofdev->node->full_name);
+		return -ENODEV;
+	}
+
  	gpt = kzalloc(sizeof *gpt, GFP_KERNEL);
  	if (!gpt)
  		return -ENOMEM;


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

Re: [PATCH] powerpc/mpc52xx/wdt: Fix 5200 wdt always being used as gpt

[Grant Likely]

On Mon, Aug 3, 2009 at 10:40 AM, Albrecht Dre=DF<albrecht.dress@arcor.de> w=
rote:
> In the current code, all MPC5200 timers are registered by the mpc52xx_gpt
> driver, even if gpt0 (the only one with this capability) shall be used as
> hardware watchdog which is indicated by the "fsl,has-wdt" or "has-wdt"
> property in the device tree. =A0Thus, the watchdog driver does never find=
 any
> watchdog and simply doesn't work.
>
> This trivial patch protects timers with a "(fsl,)?has-wdt" property from
> being probed as gpt's. =A0The watchdog timer now works just fine.

Blech.  This does solve one problem, but it causes another.  Just
about all mpc5200 device trees have the fsl,has-wdt property on GPT0
even when it isn't used as a watchdog.  The boards using GPT0 as a
GPIO or timer will be broken by this patch.

I know it is a lot more work, but the correct solution is to merge the
GPT watchdog driver into the regular GPT driver so that we don't have
two device drivers trying to bind against the same device.

g.


--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] powerpc/mpc52xx/wdt: Fix 5200 wdt always being used as gpt

[Albrecht Dreß]

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

Am 03.08.09 19:50 schrieb(en) Grant Likely:
> Just about all mpc5200 device trees have the fsl,has-wdt property on  
> GPT0 even when it isn't used as a watchdog.

Sorry, I do not understand...  The file  
Documentation/powerpc/dts-bindings/fsl/mpc5200.txt says

<snip>
On the mpc5200 and 5200b, GPT0 has a watchdog timer function.  If the  
board design supports the internal wdt, then the device node for GPT0  
should include the empty property 'fsl,has-wdt'.
</snip>

I interpreted this as "if you don't want to have the wdt function of  
gpt0, remove this property".  If this assumption is wrong, how is the  
kernel supposed to decide if a device shall be used as gpt or as wdt?

> The boards using GPT0 as a GPIO or timer will be broken by this patch.

A wdt is a security device which will (IMHO) either not be used as  
such, or it is a *must not* be used for something else on a particular  
board (you may even want u-boot to activate it, e.g. to detect a  
hanging boot process).  In both cases, a "tuned" device tree is needed.

> I know it is a lot more work, but the correct solution is to merge  
> the GPT watchdog driver into the regular GPT driver so that we don't  
> have two device drivers trying to bind against the same device.

I see the benefit of removing some duplicate code and of having gpio  
access in parallel with the wdt, but it wouldn't solve the general  
confusion above!  Will look into that, though...

Best, Albrecht.

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

Re: [PATCH] powerpc/mpc52xx/wdt: Fix 5200 wdt always being used as gpt

[Grant Likely]

On Mon, Aug 3, 2009 at 12:46 PM, Albrecht Dre=DF<albrecht.dress@arcor.de> w=
rote:
> Am 03.08.09 19:50 schrieb(en) Grant Likely:
>>
>> Just about all mpc5200 device trees have the fsl,has-wdt property on GPT=
0
>> even when it isn't used as a watchdog.
>
> Sorry, I do not understand... =A0The file
> Documentation/powerpc/dts-bindings/fsl/mpc5200.txt says
>
> <snip>
> On the mpc5200 and 5200b, GPT0 has a watchdog timer function. =A0If the b=
oard
> design supports the internal wdt, then the device node for GPT0 should
> include the empty property 'fsl,has-wdt'.
> </snip>
>
> I interpreted this as "if you don't want to have the wdt function of gpt0=
,
> remove this property". =A0If this assumption is wrong, how is the kernel
> supposed to decide if a device shall be used as gpt or as wdt?

That just states whether or not the functionality is there.  In fact,
the device can be used as both a WDT and a GPIO pin at the same time.

The kernel should decide how to use it based on what userspace asks it
to do.  If the WDT interface is opened, then it should be used as a
WDT.  If the GPIO interface is opened, then it should be used as a
GPIO (and not disturb the WDT settings).  If the gpt timer API is
called (not yet merged), then it should be used as a timer; but only
if it hasn't already been set up as a WDT.

>
>> The boards using GPT0 as a GPIO or timer will be broken by this patch.
>
> A wdt is a security device which will (IMHO) either not be used as such, =
or
> it is a *must not* be used for something else on a particular board (you =
may
> even want u-boot to activate it, e.g. to detect a hanging boot process). =
=A0In
> both cases, a "tuned" device tree is needed.

There is no property in the current binding that provides that data to
the kernel.  It works for it to be implicit based on how userspace
accesses the device, but if you want added assurance to ensure that
nothing else can use the WDT then you can define a new property to
state that explicitly and inhibit other uses.  That way no older
boards remain broken regardless of how they currently use GPT0

In fact, it probably makes sense to have a property to describe a
timeout value to preload into the WDT at boot time, or respect a
watchdog value already initialized by firmware (so that even if
userspace fails to start the watchdog daemon, the system will still
get reset).

>> I know it is a lot more work, but the correct solution is to merge the G=
PT
>> watchdog driver into the regular GPT driver so that we don't have two de=
vice
>> drivers trying to bind against the same device.
>
> I see the benefit of removing some duplicate code and of having gpio acce=
ss
> in parallel with the wdt, but it wouldn't solve the general confusion abo=
ve!
> =A0Will look into that, though...

I took a look at the code this evening, and it actually shouldn't be
too difficult to rework.  Most of the work would be relocating the
functions in the wdt driver into the gpt driver and wiring it into the
GPT probe/remove functions.

g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.