MV64x60 watchdog timer driver updates

MV64x60 watchdog timer driver updates

[Corey Minyard]

James looked at an earlier version and said it was ok, and suggested
a few changes.  This has been tested against 2.6.14-rc2 on a Katana.

Note that the bus_clk value from the platform information seems to
be in MHZ, but the actual frequency is generally 133,333,333, not
133,000,000.  I don't think the inaccuracy matters here, but it seems
a little odd.

-Corey

----

The mv64x60 watchdog timer driver didn't allow dynamically setting
the timeout value.  The hardware is settable, and setting the value
is rather important (especially with such a small default).  So
this patch adds that function.

It also does the following:
  * Modifies the returned timeout to be seconds, not jiffies,
    as seconds is the standard.
  * Adds a semaphore around the various critical data structures
    and access to the device registers.  The former operations
    could be racy.
  * Adds enable and disable operations.

Signed-off-by: Corey Minyard <cminyard@mvista.com>

Index: linux-2.6.14-rc2/drivers/char/watchdog/mv64x60_wdt.c
===================================================================
--- linux-2.6.14-rc2.orig/drivers/char/watchdog/mv64x60_wdt.c
+++ linux-2.6.14-rc2/drivers/char/watchdog/mv64x60_wdt.c
@@ -25,12 +25,14 @@
 #include <asm/mv64x60.h>
 #include <asm/uaccess.h>
 #include <asm/io.h>
+#include <asm/semaphore.h>
 
 /* MV64x60 WDC (config) register access definitions */
 #define MV64x60_WDC_CTL1_MASK	(3 << 24)
 #define MV64x60_WDC_CTL1(val)	((val & 3) << 24)
 #define MV64x60_WDC_CTL2_MASK	(3 << 26)
 #define MV64x60_WDC_CTL2(val)	((val & 3) << 26)
+#define MV64x60_WDC_ENABLE	(1 << 31)
 
 /* Flags bits */
 #define MV64x60_WDOG_FLAG_OPENED	0
@@ -39,44 +41,77 @@
 static unsigned long wdt_flags;
 static int wdt_status;
 static void __iomem *mv64x60_regs;
+static int mv64x60_bus_clk = 133000000;
 static int mv64x60_wdt_timeout;
+static u32 mv64x60_wdt_control_val;
+
+static DECLARE_MUTEX(mv64x60_sem);
+
+static u32 mv64x60_wdt_read_reg(void)
+{
+	return readl(mv64x60_regs + MV64x60_WDT_WDC);
+}
 
 static void mv64x60_wdt_reg_write(u32 val)
 {
 	/* Allow write only to CTL1 / CTL2 fields, retaining values in
 	 * other fields.
 	 */
-	u32 data = readl(mv64x60_regs + MV64x60_WDT_WDC);
-	data &= ~(MV64x60_WDC_CTL1_MASK | MV64x60_WDC_CTL2_MASK);
-	data |= val;
+	u32 data = mv64x60_wdt_control_val | val;
 	writel(data, mv64x60_regs + MV64x60_WDT_WDC);
 }
 
 static void mv64x60_wdt_service(void)
 {
 	/* Write 01 followed by 10 to CTL2 */
+	down(&mv64x60_sem);
 	mv64x60_wdt_reg_write(MV64x60_WDC_CTL2(0x01));
 	mv64x60_wdt_reg_write(MV64x60_WDC_CTL2(0x02));
+	up(&mv64x60_sem);
+}
+
+static int mv64x60_set_timeout(int timeout)
+{
+	u32 cnt;
+
+	if (timeout > (0xffffffff / mv64x60_bus_clk))
+		return -EINVAL;
+
+	down(&mv64x60_sem);
+	mv64x60_wdt_timeout = timeout;
+
+	/* Put into u32 first so shift will be unsigned. */
+	cnt = timeout * mv64x60_bus_clk;
+	mv64x60_wdt_control_val = cnt >> 8;
+	up(&mv64x60_sem);
+
+	if (test_bit(MV64x60_WDOG_FLAG_ENABLED, &wdt_flags))
+		mv64x60_wdt_service();
+	return 0;
 }
 
 static void mv64x60_wdt_handler_disable(void)
 {
+	down(&mv64x60_sem);
 	if (test_and_clear_bit(MV64x60_WDOG_FLAG_ENABLED, &wdt_flags)) {
 		/* Write 01 followed by 10 to CTL1 */
 		mv64x60_wdt_reg_write(MV64x60_WDC_CTL1(0x01));
 		mv64x60_wdt_reg_write(MV64x60_WDC_CTL1(0x02));
 		printk(KERN_NOTICE "mv64x60_wdt: watchdog deactivated\n");
 	}
+	up(&mv64x60_sem);
 }
 
 static void mv64x60_wdt_handler_enable(void)
 {
+	down(&mv64x60_sem);
 	if (!test_and_set_bit(MV64x60_WDOG_FLAG_ENABLED, &wdt_flags)) {
 		/* Write 01 followed by 10 to CTL1 */
 		mv64x60_wdt_reg_write(MV64x60_WDC_CTL1(0x01));
 		mv64x60_wdt_reg_write(MV64x60_WDC_CTL1(0x02));
 		printk(KERN_NOTICE "mv64x60_wdt: watchdog activated\n");
 	}
+	up(&mv64x60_sem);
 }
 
 static int mv64x60_wdt_open(struct inode *inode, struct file *file)
@@ -119,6 +154,7 @@ static int mv64x60_wdt_ioctl(struct inod
 			     unsigned int cmd, unsigned long arg)
 {
 	int timeout;
+	int val;
 	static struct watchdog_info info = {
 		.options = WDIOF_KEEPALIVEPING,
 		.firmware_version = 0,
@@ -142,7 +178,14 @@ static int mv64x60_wdt_ioctl(struct inod
 		return -EOPNOTSUPP;
 
 	case WDIOC_SETOPTIONS:
-		return -EOPNOTSUPP;
+		if (copy_from_user(&val, (int *) arg, sizeof(int)))
+			return -EFAULT;
+		if (val & WDIOS_DISABLECARD)
+			mv64x60_wdt_handler_disable();
+
+		if (val & WDIOS_ENABLECARD)
+			mv64x60_wdt_handler_enable();
+		return 0;
 
 	case WDIOC_KEEPALIVE:
 		mv64x60_wdt_service();
@@ -150,13 +193,15 @@ static int mv64x60_wdt_ioctl(struct inod
 		break;
 
 	case WDIOC_SETTIMEOUT:
-		return -EOPNOTSUPP;
+  		if (get_user(timeout, (int *)arg))
+  			return -EFAULT;
+		return mv64x60_set_timeout(timeout);
 
 	case WDIOC_GETTIMEOUT:
-		timeout = mv64x60_wdt_timeout * HZ;
-		if (put_user(timeout, (int *)arg))
-			return -EFAULT;
-		break;
+		timeout = mv64x60_wdt_timeout;
+  		if (put_user(timeout, (int *)arg))
+  			return -EFAULT;
+  		break;
 
 	default:
 		return -ENOIOCTLCMD;
@@ -184,18 +229,25 @@ static int __devinit mv64x60_wdt_probe(s
 {
 	struct platform_device *pd = to_platform_device(dev);
 	struct mv64x60_wdt_pdata *pdata = pd->dev.platform_data;
-	int bus_clk = 133;
+	int timeout = 10;
 
-	mv64x60_wdt_timeout = 10;
 	if (pdata) {
-		mv64x60_wdt_timeout = pdata->timeout;
-		bus_clk = pdata->bus_clk;
+		timeout = pdata->timeout;
+		mv64x60_bus_clk = pdata->bus_clk * 1000000;
+	}
+	if (mv64x60_set_timeout(timeout)) {
+		printk("mv64x60_wdt: Default timeout too large, setting to"
+		       " maximum value\n");
+		mv64x60_set_timeout(0xffffffff / mv64x60_bus_clk);
 	}
 
 	mv64x60_regs = mv64x60_get_bridge_vbase();
 
-	writel((mv64x60_wdt_timeout * (bus_clk * 1000000)) >> 8,
-	       mv64x60_regs + MV64x60_WDT_WDC);
+	if (mv64x60_wdt_read_reg() & MV64x60_WDC_ENABLE) {
+		/* Watchdog was already running, disable it */
+		set_bit(MV64x60_WDOG_FLAG_ENABLED, &wdt_flags);
+		mv64x60_wdt_handler_disable();
+	}
 
 	return misc_register(&mv64x60_wdt_miscdev);
 }

Re: MV64x60 watchdog timer driver updates

[Mark A. Greer]

On Thu, Sep 29, 2005 at 01:32:21PM -0500, Corey Minyard wrote:
> James looked at an earlier version and said it was ok, and suggested
> a few changes.  This has been tested against 2.6.14-rc2 on a Katana.
> 
> Note that the bus_clk value from the platform information seems to
> be in MHZ, but the actual frequency is generally 133,333,333, not
> 133,000,000.  I don't think the inaccuracy matters here, but it seems
> a little odd.

Yes, it makes more sense to me to pass the actual frequency and not do
the '* 1000000'.

Also--I should have thought of this earlier--there should probably be a
patch to add a default platform_data entry in arch/ppc/syslib/mv64x60.c
and a patch for katana.c if the defaults in mv64x60.c aren't correct
for the katana.  At least, that's how things are done now.

Mark

Re: MV64x60 watchdog timer driver updates

[Corey Minyard]

Mark A. Greer wrote:

>On Thu, Sep 29, 2005 at 01:32:21PM -0500, Corey Minyard wrote:
>  
>
>>James looked at an earlier version and said it was ok, and suggested
>>a few changes.  This has been tested against 2.6.14-rc2 on a Katana.
>>
>>Note that the bus_clk value from the platform information seems to
>>be in MHZ, but the actual frequency is generally 133,333,333, not
>>133,000,000.  I don't think the inaccuracy matters here, but it seems
>>a little odd.
>>    
>>
>
>Yes, it makes more sense to me to pass the actual frequency and not do
>the '* 1000000'.
>  
>
The only trouble is if  you go over 4GHZ...  Maybe it should be in KHZ 
instead of MHZ?  or 64-bits?  Or maybe we don't worry about >4GHZ busses?

Also, how would one go about changing this?  Would you need something like:

if (bus_clk < 1000)
  bus_clk *= 1000000

for backwards compatability?

>Also--I should have thought of this earlier--there should probably be a
>patch to add a default platform_data entry in arch/ppc/syslib/mv64x60.c
>and a patch for katana.c if the defaults in mv64x60.c aren't correct
>for the katana.  At least, that's how things are done now.
>  
>
I'm not sure I understand completely.  How would you go about setting 
the platform data?

Thanks,

-Corey

Re: MV64x60 watchdog timer driver updates

[Geert Uytterhoeven]

On Fri, 30 Sep 2005, Corey Minyard wrote:
> Mark A. Greer wrote:
> > On Thu, Sep 29, 2005 at 01:32:21PM -0500, Corey Minyard wrote:
> > > James looked at an earlier version and said it was ok, and suggested
> > > a few changes.  This has been tested against 2.6.14-rc2 on a Katana.
> > > 
> > > Note that the bus_clk value from the platform information seems to
> > > be in MHZ, but the actual frequency is generally 133,333,333, not
> > > 133,000,000.  I don't think the inaccuracy matters here, but it seems
> > > a little odd.
> > >    
> > 
> > Yes, it makes more sense to me to pass the actual frequency and not do
> > the '* 1000000'.
> >  
> The only trouble is if  you go over 4GHZ...  Maybe it should be in KHZ instead
> of MHZ?  or 64-bits?  Or maybe we don't worry about >4GHZ busses?
> 
> Also, how would one go about changing this?  Would you need something like:
> 
> if (bus_clk < 1000)
>  bus_clk *= 1000000
> 
> for backwards compatability?

Hey, don't touch the i2c! ;-)

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: MV64x60 watchdog timer driver updates

[Mark A. Greer]

On Fri, Sep 30, 2005 at 10:10:52AM -0500, Corey Minyard wrote:
> Mark A. Greer wrote:
> 
> >On Thu, Sep 29, 2005 at 01:32:21PM -0500, Corey Minyard wrote:
> > 
> >
> >>James looked at an earlier version and said it was ok, and suggested
> >>a few changes.  This has been tested against 2.6.14-rc2 on a Katana.
> >>
> >>Note that the bus_clk value from the platform information seems to
> >>be in MHZ, but the actual frequency is generally 133,333,333, not
> >>133,000,000.  I don't think the inaccuracy matters here, but it seems
> >>a little odd.
> >>   
> >>
> >
> >Yes, it makes more sense to me to pass the actual frequency and not do
> >the '* 1000000'.
> > 
> >
> The only trouble is if  you go over 4GHZ...  Maybe it should be in KHZ 
> instead of MHZ?  or 64-bits?  Or maybe we don't worry about >4GHZ busses?

My vote is to worry about it when the time comes.  IIRC, the fastest a
Marvell bridge part can be clocked right now is 200 MHz so there's some time.

> Also, how would one go about changing this?  Would you need something like:
> 
> if (bus_clk < 1000)
>  bus_clk *= 1000000
> 
> for backwards compatability?

When I didn't see anyone passing that value in via platform_data so
there's no compatibility issue.  I could have missed something though...

> >Also--I should have thought of this earlier--there should probably be a
> >patch to add a default platform_data entry in arch/ppc/syslib/mv64x60.c
> >and a patch for katana.c if the defaults in mv64x60.c aren't correct
> >for the katana.  At least, that's how things are done now.
> > 
> >
> I'm not sure I understand completely.  How would you go about setting 
> the platform data?

Look near the top of arch/ppc/syslib/mv64x60.c and you will see a bunch
of default platform_data being set up for marvell bridges.  They can then
be tweaked by the platform specific code via the platform_notify() hook
(see arch/ppc/platforms/katana.c:katana_platform_notify(), et. al.).

Mark