ar7: register watchdog driver only if enabled in hardware configuration

PATCH In review Florian Fainelli 3 participants 7 messages Aug 4, 2009 – Oct 5, 2009 MIPS

Share & tools

Florian Fainelli

author

2009-08-04 21:09 UTC #0 lore raw

PATCH 1/1 ar7: register watchdog driver only if enabled in hardware configuration

This patch checks if the watchdog enable bit is set in the DCL register meaning that the hardware watchdog actually works and if so, register the ar7_wdt platform_device.

Signed-off-by Florian Fainelli <[email protected]>

---

arch/mips/ar7/platform.c +8 -1

index e2278c0..835f3f0 100644
@@ -503,6 +503,7 @@ static int __init ar7_register_devices(void)
 {
 	u16 chip_id;
 	int res;
+	u32 *bootcr, val;
 #ifdef CONFIG_SERIAL_8250
 	static struct uart_port uart_port[2];

@@ -595,7 +596,13 @@ static int __init ar7_register_devices(void)
 
 	ar7_wdt_res.end = ar7_wdt_res.start + 0x20;
 
-	res = platform_device_register(&ar7_wdt);
+	bootcr = (u32 *)ioremap_nocache(AR7_REGS_DCL, 4);
+	val = *bootcr;
+	iounmap(bootcr);
+
+	/* Register watchdog only if enabled in hardware */
+	if (val & AR7_WDT_HW_ENA)
+		res = platform_device_register(&ar7_wdt);
 
 	return res;
 }

arch/mips/include/asm/mach-ar7/ar7.h +3 -0

index de71694..21cbbc7 100644
@@ -78,6 +78,9 @@
 #define AR7_REF_CLOCK	25000000
 #define AR7_XTAL_CLOCK	24000000
 
+/* DCL */
+#define AR7_WDT_HW_ENA	0x10
+
 struct plat_cpmac_data {
 	int reset_bit;
 	int power_bit;

Alexander Clouter

2009-08-04 23:08 UTC #1 lore raw

Quoted 22 lines · Florian Fainelli

This patch checks if the watchdog enable bit is set in the DCL register meaning that the hardware watchdog actually works and if so, register the ar7_wdt platform_device.
Signed-off-by Florian Fainelli <[email protected]>
---
arch/mips/ar7/platform.c +1 -0
index e2278c0..835f3f0 100644
@@ -503,6 +503,7 @@ static int __init ar7_register_devices(void)
 {
       u16 chip_id;
       int res;
+       u32 *bootcr, val;
 #ifdef CONFIG_SERIAL_8250
       static struct uart_port uart_port[2];
 
@@ -595,7 +596,13 @@ static int __init ar7_register_devices(void)
 
       ar7_wdt_res.end = ar7_wdt_res.start + 0x20;

arch/mips/ar7/platform.c static int __init ar7_register_devices(void) · line 606

Quoted 10 lines

arch/mips/ar7/platform.c +7 -1

 
-       res = platform_device_register(&ar7_wdt);
+       bootcr = (u32 *)ioremap_nocache(AR7_REGS_DCL, 4);
+       val = *bootcr;
+       iounmap(bootcr);
+
+       /* Register watchdog only if enabled in hardware */
+       if (val & AR7_WDT_HW_ENA)
+               res = platform_device_register(&ar7_wdt);

I think the 'correct' way to do this is:
---
void __iomem *bootcr;
u32 val;

...

bootcr = ioremap_nocache(AR7_REGS_DCL, 4);
val = *bootcr;
iounmap(bootcr);
---

I'm betting this could be reduced to:
---
if (ioread32(AR7_REGS_DCL) & AR7_WDT_HW_ENA)

	res = platform_device_register(&ar7_wdt);

---

However without the hardware...I cannot test this.

Cheers

Signature · 3 lines

Alexander Clouter
.sigmonster says: Accuracy, n.:
                  	The vice of being right

Sergei Shtylyov

2009-08-05 08:19 UTC #2 lore raw

Hello.

Quoted 37 lines · Alexander Clouter

Quoted 22 lines
This patch checks if the watchdog enable bit is set in the DCL register meaning that the hardware watchdog actually works and if so, register the ar7_wdt platform_device.
Signed-off-by Florian Fainelli <[email protected]>
---
arch/mips/ar7/platform.c +1 -0
index e2278c0..835f3f0 100644
@@ -503,6 +503,7 @@ static int __init ar7_register_devices(void)
 {
       u16 chip_id;
       int res;
+       u32 *bootcr, val;
 #ifdef CONFIG_SERIAL_8250
       static struct uart_port uart_port[2];
 
@@ -595,7 +596,13 @@ static int __init ar7_register_devices(void)
 
       ar7_wdt_res.end = ar7_wdt_res.start + 0x20;
 
arch/mips/ar7/platform.c static int __init ar7_register_devices(void) · line 607
Quoted 10 lines
arch/mips/ar7/platform.c +7 -1
-       res = platform_device_register(&ar7_wdt);
+       bootcr = (u32 *)ioremap_nocache(AR7_REGS_DCL, 4);
+       val = *bootcr;
+       iounmap(bootcr);
+
+       /* Register watchdog only if enabled in hardware */
+       if (val & AR7_WDT_HW_ENA)
+               res = platform_device_register(&ar7_wdt);
 
 
I think the 'correct' way to do this is:
---
void __iomem *bootcr;
u32 val;

Quoted 5 lines

...
bootcr = ioremap_nocache(AR7_REGS_DCL, 4);
val = *bootcr;

Wait, you can't dereference a pointer to void...

WBR, Sergei

Alexander Clouter

2009-08-05 08:34 UTC #3 lore raw

Hi,

Quoted 10 lines · Sergei Shtylyov

Hello.
Quoted 5 lines · Alexander Clouter
I think the 'correct' way to do this is:
---
void __iomem *bootcr;
u32 val;

Quoted 8 lines

Quoted 5 lines
...
bootcr = ioremap_nocache(AR7_REGS_DCL, 4);
val = *bootcr;
Wait, you can't dereference a pointer to void...

bah,
----
val = __raw_readl(bootcr);
----

Cheers

Signature · 2 lines

Alexander Clouter
.sigmonster says: If you keep anything long enough, you can throw it away.

Alexander Clouter

2009-08-05 08:08 UTC #4 lore raw

In reply to Florian Fainelli

Quoted 25 lines · Florian Fainelli

This patch checks if the watchdog enable bit is set in the DCL register meaning that the hardware watchdog actually works and if so, register the ar7_wdt platform_device.
Signed-off-by Florian Fainelli <[email protected]>
---
arch/mips/ar7/platform.c +1 -1
index e2278c0..835f3f0 100644
@@ -503,6 +503,7 @@ static int __init ar7_register_devices(void)
 {
       u16 chip_id;
       int res;
+       u32 *bootcr, val;
 #ifdef CONFIG_SERIAL_8250
       static struct uart_port uart_port[2];
 
@@ -595,7 +596,13 @@ static int __init ar7_register_devices(void)
 
       ar7_wdt_res.end = ar7_wdt_res.start + 0x20;
 
-       res = platform_device_register(&ar7_wdt);

arch/mips/ar7/platform.c static int __init ar7_register_devices(void) · line 608

Quoted 10 lines

arch/mips/ar7/platform.c +7 -0

+       bootcr = (u32 *)ioremap_nocache(AR7_REGS_DCL, 4);
+       val = *bootcr;
+       iounmap(bootcr);
+
+       /* Register watchdog only if enabled in hardware */
+       if (val & AR7_WDT_HW_ENA)
+               res = platform_device_register(&ar7_wdt);
 
       return res;
 }

'res' can now return NULL[1]. Solved if you do:
----
int res = -ENODEV;
----

I'm guessing this is the most apprioate?

Cheers

[1] I cannot see the full file annoyingly, it's not in my linux-mips git tree.

Signature · 2 lines

Alexander Clouter
.sigmonster says: It doesn't matter whether you win or lose -- until you lose.

Florian Fainelli

author

2009-08-07 17:27 UTC #5 lore raw

Hey Alexander,

Le Wednesday 05 August 2009 10:00:35 Alexander Clouter, vous avez écrit :

Quoted 40 lines

Quoted 23 lines · Florian Fainelli
This patch checks if the watchdog enable bit is set in the DCL register meaning that the hardware watchdog actually works and if so, register the ar7_wdt platform_device.
Signed-off-by Florian Fainelli <[email protected]>
---
arch/mips/ar7/platform.c +1 -1
index e2278c0..835f3f0 100644
@@ -503,6 +503,7 @@ static int __init ar7_register_devices(void)
 {
       u16 chip_id;
       int res;
+       u32 *bootcr, val;
 #ifdef CONFIG_SERIAL_8250
       static struct uart_port uart_port[2];
 
@@ -595,7 +596,13 @@ static int __init ar7_register_devices(void)
 
       ar7_wdt_res.end = ar7_wdt_res.start + 0x20;
 
-       res = platform_device_register(&ar7_wdt);
arch/mips/ar7/platform.c static int __init ar7_register_devices(void) · line 608
Quoted 10 lines
arch/mips/ar7/platform.c +7 -0
+       bootcr = (u32 *)ioremap_nocache(AR7_REGS_DCL, 4);
+       val = *bootcr;
+       iounmap(bootcr);
+
+       /* Register watchdog only if enabled in hardware */
+       if (val & AR7_WDT_HW_ENA)
+               res = platform_device_register(&ar7_wdt);
 
       return res;
 }
'res' can now return NULL[1]. Solved if you do:
----
int res = -ENODEV;
----

Quoted 1 line

I'm guessing this is the most apprioate?

I prefer letting this as-is, since if the watchdog was not enabled in hardware, we will not register the watchdog driver, and return the last platform_device_register call.

Quoted 5 lines

Cheers
[1] I cannot see the full file annoyingly, it's not in my linux-mips git tree.

Not sure which tree you checked out, but it is in linux-queue:
http://www.linux-mips.org/git?p=linux-queue.git;a=tree;f=arch/mips/ar7;h=40ee7382dfabf05ee6c967016e42f94992655c20;hb=HEAD

Signature · 5 lines

Best regards, Florian Fainelli
Email: [email protected]
Web: http://openwrt.org
IRC: [florian] on irc.freenode.net
-------------------------------

Florian Fainelli

author

2009-10-05 09:41 UTC #6 lore raw

In reply to Florian Fainelli

Ralf,

Any comments on that patch ?

Thanks !

Quoted 47 lines · On Tuesday 04 August 2009 23:09:36 Florian Fainelli wrote:

This patch checks if the watchdog enable bit is set in the DCL register meaning that the hardware watchdog actually works and if so, register the ar7_wdt platform_device.

Signed-off-by Florian Fainelli <[email protected]>

---

arch/mips/ar7/platform.c +8 -1

index e2278c0..835f3f0 100644
@@ -503,6 +503,7 @@ static int __init ar7_register_devices(void)
 {
 	u16 chip_id;
 	int res;
+	u32 *bootcr, val;
 #ifdef CONFIG_SERIAL_8250
 	static struct uart_port uart_port[2];

@@ -595,7 +596,13 @@ static int __init ar7_register_devices(void)
 
 	ar7_wdt_res.end = ar7_wdt_res.start + 0x20;
 
-	res = platform_device_register(&ar7_wdt);
+	bootcr = (u32 *)ioremap_nocache(AR7_REGS_DCL, 4);
+	val = *bootcr;
+	iounmap(bootcr);
+
+	/* Register watchdog only if enabled in hardware */
+	if (val & AR7_WDT_HW_ENA)
+		res = platform_device_register(&ar7_wdt);
 
 	return res;
 }

(quoted diff) +0 -0

b/arch/mips/include/asm/mach-ar7/ar7.h index de71694..21cbbc7 100644

arch/mips/include/asm/mach-ar7/ar7.h +3 -0

@@ -78,6 +78,9 @@
 #define AR7_REF_CLOCK	25000000
 #define AR7_XTAL_CLOCK	24000000
 
+/* DCL */
+#define AR7_WDT_HW_ENA	0x10
+
 struct plat_cpmac_data {
 	int reset_bit;
 	int power_bit;