OMAP3:WDT:Enable IVA, SECURE and minor bugfixes

PATCH In review Ulrik Bech Hald 2 participants 12 messages Jun 8, 2009 – Jun 15, 2009

Subsystems (5)

Arm (32-bit) Arm sub-architectures OMAP2+ support Watchdog device drivers +1 more subsystems

lore

Share & tools

lore

Ulrik Bech Hald

author

2009-06-08 21:02 UTC #0 lore raw

cover OMAP3:WDT:Enable IVA, SECURE and minor bugfixes

This patch series enables support for IVA and SECURE WDTs, available on omap34xx.
For omap34xx devices the WDT will be accessible (when present) through:
SECURE: /dev/watchdog1
MPU: /dev/watchdog2
IVA: /dev/watchdog3

For devices older than omap34xx only MPU WDT is present and will be accessible through /dev/watchdog

The series also fixes two bugs:

1) Correct timeout value is not loaded upon opening the watchdog device.

2) clks are not enabled when accessing registers in probe

 b/arch/arm/mach-omap2/clock34xx.c |   12 ++---
 b/arch/arm/plat-omap/devices.c    |   79 ++++++++++++++++++++++++++++++--------
 b/drivers/watchdog/omap_wdt.c     |   42 ++++++++++++++++++----
 3 files changed, 112 insertions(+), 28 deletions(-)

Ulrik Bech Hald

author

2009-06-08 21:02 UTC #1 lore raw

PATCH 1/3 OMAP3:WDT:Register IVA,SECURE, make clks accessible

Enabling registration of IVA and SECURE WDT devices. Making ick and fck for IVA and SECURE WDTs accessible.

Signed-off-by Ulrik Bech Hald <[email protected]>

---
 arch/arm/mach-omap2/clock34xx.c |   12 +++---
 arch/arm/plat-omap/devices.c    |   79 +++++++++++++++++++++++++++++++-------
 2 files changed, 70 insertions(+), 21 deletions(-)

arch/arm/mach-omap2/clock34xx.c +6 -6

index ba05aa4..fe43d73 100644
@@ -212,11 +212,11 @@ static struct omap_clk omap34xx_clks[] = {
 	CLK(NULL,	"gpt1_fck",	&gpt1_fck,	CK_343X),
 	CLK(NULL,	"wkup_32k_fck",	&wkup_32k_fck,	CK_343X),
 	CLK(NULL,	"gpio1_dbck",	&gpio1_dbck,	CK_343X),
-	CLK("omap_wdt",	"fck",		&wdt2_fck,	CK_343X),
+	CLK("omap_wdt.2", "fck",	&wdt2_fck,	CK_343X),
 	CLK(NULL,	"wkup_l4_ick",	&wkup_l4_ick,	CK_343X),
 	CLK(NULL,	"usim_ick",	&usim_ick,	CK_3430ES2),
-	CLK("omap_wdt",	"ick",		&wdt2_ick,	CK_343X),
-	CLK(NULL,	"wdt1_ick",	&wdt1_ick,	CK_343X),
+	CLK("omap_wdt.2", "ick",	&wdt2_ick,	CK_343X),
+	CLK("omap_wdt.1", "ick",	&wdt1_ick,	CK_343X),
 	CLK(NULL,	"gpio1_ick",	&gpio1_ick,	CK_343X),
 	CLK(NULL,	"omap_32ksync_ick", &omap_32ksync_ick, CK_343X),
 	CLK(NULL,	"gpt12_ick",	&gpt12_ick,	CK_343X),

@@ -238,14 +238,14 @@ static struct omap_clk omap34xx_clks[] = {
 	CLK(NULL,	"gpio4_dbck",	&gpio4_dbck,	CK_343X),
 	CLK(NULL,	"gpio3_dbck",	&gpio3_dbck,	CK_343X),
 	CLK(NULL,	"gpio2_dbck",	&gpio2_dbck,	CK_343X),
-	CLK(NULL,	"wdt3_fck",	&wdt3_fck,	CK_343X),
+	CLK("omap_wdt.3", "fck",	&wdt3_fck,	CK_343X),
 	CLK(NULL,	"per_l4_ick",	&per_l4_ick,	CK_343X),
 	CLK(NULL,	"gpio6_ick",	&gpio6_ick,	CK_343X),
 	CLK(NULL,	"gpio5_ick",	&gpio5_ick,	CK_343X),
 	CLK(NULL,	"gpio4_ick",	&gpio4_ick,	CK_343X),
 	CLK(NULL,	"gpio3_ick",	&gpio3_ick,	CK_343X),
 	CLK(NULL,	"gpio2_ick",	&gpio2_ick,	CK_343X),
-	CLK(NULL,	"wdt3_ick",	&wdt3_ick,	CK_343X),
+	CLK("omap_wdt.3", "ick",	&wdt3_ick,	CK_343X),
 	CLK(NULL,	"uart3_ick",	&uart3_ick,	CK_343X),
 	CLK(NULL,	"gpt9_ick",	&gpt9_ick,	CK_343X),
 	CLK(NULL,	"gpt8_ick",	&gpt8_ick,	CK_343X),

@@ -272,7 +272,7 @@ static struct omap_clk omap34xx_clks[] = {
 	CLK(NULL,	"sr_l4_ick",	&sr_l4_ick,	CK_343X),
 	CLK(NULL,	"secure_32k_fck", &secure_32k_fck, CK_343X),
 	CLK(NULL,	"gpt12_fck",	&gpt12_fck,	CK_343X),
-	CLK(NULL,	"wdt1_fck",	&wdt1_fck,	CK_343X),
+	CLK("omap_wdt.1", "fck",	&wdt1_fck,	CK_343X),
 };
 
 /* CM_AUTOIDLE_PLL*.AUTO_* bit values */

arch/arm/plat-omap/devices.c +64 ~~-15~~

100 lines not on this page · raw message

Signature · 1 line

1.5.4.3

Ulrik Bech Hald

author

2009-06-08 21:02 UTC #2 lore raw

PATCH 2/3 OMAP3:WDT:Enable support for IVA and SECURE

This patch enables the IVA and SECURE WDTs in the omap_wdt driver for omap34xx family. SECURE will be available as /dev/watchdog1 HS/EMU devices and IVA will be available as /dev/watchdog3. MPU will be available as /dev/watchdog2
For omap versions earlier than 34xx only MPU watchdog is present and will be available as /dev/watchdog for backwards compatibility.

Signed-off-by Ulrik Bech Hald <[email protected]>

---
 drivers/watchdog/omap_wdt.c |   42 +++++++++++++++++++++++++++++++++++-------
 1 files changed, 35 insertions(+), 7 deletions(-)
 mode change 100644 => 100755 drivers/watchdog/omap_wdt.c

drivers/watchdog/omap_wdt.c +35 -7

old mode 100644
new mode 100755
index f271385..26935c7
@@ -47,7 +47,9 @@
 
 #include "omap_wdt.h"
 
-static struct platform_device *omap_wdt_dev;
+#define NUM_WDTS	3
+
+static struct platform_device *omap_wdt_dev[NUM_WDTS];
 
 static unsigned timer_margin;
 module_param(timer_margin, uint, 0);

@@ -139,8 +141,26 @@ static void omap_wdt_set_timeout(struct omap_wdt_dev *wdev)
  */
 static int omap_wdt_open(struct inode *inode, struct file *file)
 {
-	struct omap_wdt_dev *wdev = platform_get_drvdata(omap_wdt_dev);
-	void __iomem *base = wdev->base;
+	struct omap_wdt_dev *wdev;
+	void __iomem *base;
+
+	/* by default MPU wdt is present across omap family */
+	wdev = platform_get_drvdata(omap_wdt_dev[1]);
+
+	/* if multiple wdts, find match between device node and wdt device */
+	if (cpu_is_omap34xx()) {
+		int i;
+		for (i = 0; i < NUM_WDTS; i++) {
+			if (omap_wdt_dev[i]) {
+				wdev = platform_get_drvdata(omap_wdt_dev[i]);
+				if (iminor(inode)
+					== wdev->omap_wdt_miscdev.minor)
+					break;
+			}
+		}
+	}
+
+	base = wdev->base;
 
 	if (test_and_set_bit(1, (unsigned long *)&(wdev->omap_wdt_users)))
 		return -EBUSY;

@@ -263,6 +283,7 @@ static int __devinit omap_wdt_probe(struct platform_device *pdev)
 	struct resource *res, *mem;
 	struct omap_wdt_dev *wdev;
 	int ret;
+	static char wdt_name[32];
 
 	/* reserve static register mappings */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

@@ -271,7 +292,7 @@ static int __devinit omap_wdt_probe(struct platform_device *pdev)
 		goto err_get_resource;
 	}
 
-	if (omap_wdt_dev) {
+	if (omap_wdt_dev[pdev->id-1]) {
 		ret = -EBUSY;
 		goto err_busy;
 	}

@@ -318,8 +339,14 @@ static int __devinit omap_wdt_probe(struct platform_device *pdev)
 
 	wdev->omap_wdt_miscdev.parent = &pdev->dev;
 	wdev->omap_wdt_miscdev.minor = WATCHDOG_MINOR;
-	wdev->omap_wdt_miscdev.name = "watchdog";
 	wdev->omap_wdt_miscdev.fops = &omap_wdt_fops;
+	wdev->omap_wdt_miscdev.name = "watchdog";
+	if (cpu_is_omap34xx()) {
+		snprintf(wdt_name, sizeof(wdt_name), "watchdog%d", pdev->id);
+		wdev->omap_wdt_miscdev.name = wdt_name;
+		if (pdev->id != 2)
+			wdev->omap_wdt_miscdev.minor = MISC_DYNAMIC_MINOR;
+	}
 
 	ret = misc_register(&(wdev->omap_wdt_miscdev));
 	if (ret)

@@ -332,7 +359,8 @@ static int __devinit omap_wdt_probe(struct platform_device *pdev)
 	/* autogate OCP interface clock */
 	__raw_writel(0x01, wdev->base + OMAP_WATCHDOG_SYS_CONFIG);
 
-	omap_wdt_dev = pdev;
+	/* keep track of the wdt platform devices in local device array */
+	omap_wdt_dev[pdev->id - 1] = pdev;
 
 	return 0;

@@ -384,7 +412,7 @@ static int __devexit omap_wdt_remove(struct platform_device *pdev)
 	iounmap(wdev->base);
 
 	kfree(wdev);
-	omap_wdt_dev = NULL;
+	omap_wdt_dev[pdev->id-1] = NULL;
 
 	return 0;
 }

Signature · 1 line

1.5.4.3

Ulrik Bech Hald

author

2009-06-08 21:02 UTC #3 lore raw

PATCH 3/3 OMAP3:WDT:Enable clk in probe, trigger timer reload

This patch contains two bugfixes:

1)In omap_wdt_probe() the watchdog is reset and disabled. This requires register access and the clks needs to be enabled temporarily

2)In omap_wdt_open() the timer register needs to be reloaded to trigger a new timer value (the default of 60s)

Signed-off-by Ulrik Bech Hald <[email protected]>

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

drivers/watchdog/omap_wdt.c +7 -0

index 26935c7..cb580a8 100755
@@ -179,6 +179,7 @@ static int omap_wdt_open(struct inode *inode, struct file *file)
 	file->private_data = (void *) wdev;
 
 	omap_wdt_set_timeout(wdev);
+	omap_wdt_ping(wdev); /* trigger loading of new timeout value */
 	omap_wdt_enable(wdev);
 
 	return nonseekable_open(inode, file);

@@ -334,6 +335,9 @@ static int __devinit omap_wdt_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, wdev);
 
+	clk_enable(wdev->ick);
+	clk_enable(wdev->fck);
+
 	omap_wdt_disable(wdev);
 	omap_wdt_adjust_timeout(timer_margin);

@@ -359,6 +363,9 @@ static int __devinit omap_wdt_probe(struct platform_device *pdev)
 	/* autogate OCP interface clock */
 	__raw_writel(0x01, wdev->base + OMAP_WATCHDOG_SYS_CONFIG);
 
+	clk_disable(wdev->ick);
+	clk_disable(wdev->fck);
+
 	/* keep track of the wdt platform devices in local device array */
 	omap_wdt_dev[pdev->id - 1] = pdev;

Signature · 1 line

1.5.4.3

Kevin Hilman

2009-06-09 14:47 UTC #4 lore raw

Re: [PATCH 2/3] OMAP3:WDT:Enable support for IVA and SECURE

In reply to Ulrik Bech Hald

Quoted 14 lines · Ulrik Bech Hald

This patch enables the IVA and SECURE WDTs in the omap_wdt driver for omap34xx family. SECURE will be available as /dev/watchdog1 HS/EMU devices and IVA will be available as /dev/watchdog3. MPU will be available as /dev/watchdog2
For omap versions earlier than 34xx only MPU watchdog is present and will be available as /dev/watchdog for backwards compatibility.
Signed-off-by Ulrik Bech Hald <[email protected]>
---
 drivers/watchdog/omap_wdt.c |   42 +++++++++++++++++++++++++++++++++++-------
 1 files changed, 35 insertions(+), 7 deletions(-)
 mode change 100644 => 100755 drivers/watchdog/omap_wdt.c
drivers/watchdog/omap_wdt.c +0 -0

[...]

Quoted 14 lines

static int omap_wdt_open(struct inode *inode, struct file *file) {

drivers/watchdog/omap_wdt.c +10 -2

-	struct omap_wdt_dev *wdev = platform_get_drvdata(omap_wdt_dev);
-	void __iomem *base = wdev->base;
+	struct omap_wdt_dev *wdev;
+	void __iomem *base;
+
+	/* by default MPU wdt is present across omap family */
+	wdev = platform_get_drvdata(omap_wdt_dev[1]);
+
+	/* if multiple wdts, find match between device node and wdt device */
+	if (cpu_is_omap34xx()) {
+		int i;
+		for (i = 0; i < NUM_WDTS; i++) {

drivers/watchdog/omap_wdt.c static void omap_wdt_set_timeout(struct omap_wdt_dev *wdev) · line 163

Quoted 10 lines

drivers/watchdog/omap_wdt.c +10 -0

+			if (omap_wdt_dev[i]) {
+				wdev = platform_get_drvdata(omap_wdt_dev[i]);
+				if (iminor(inode)
+					== wdev->omap_wdt_miscdev.minor)
+					break;
+			}
+		}
+	}
+
+	base = wdev->base;

Hmm, this does not look right to me. Any use of cpu_is_* in driver code usually indicates that something is not quite right. This same watchdog IP is used in some DaVinci family processors and needs to be reused there.

I didn't do the miscdev research, but can't you get the pdev from the miscdev.parent, and from there get the right wdev pointer from the pdev.drvdata.

Otherwise, drop the cpu_is_* and just do inode checking all the time. The way you've done it looks error prone to me.

Quoted 22 lines

drivers/watchdog/omap_wdt.c +2 -1

 	if (test_and_set_bit(1, (unsigned long *)&(wdev->omap_wdt_users)))
 		return -EBUSY;

@@ -263,6 +283,7 @@ static int __devinit omap_wdt_probe(struct platform_device *pdev)
 	struct resource *res, *mem;
 	struct omap_wdt_dev *wdev;
 	int ret;
+	static char wdt_name[32];
 
 	/* reserve static register mappings */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

@@ -271,7 +292,7 @@ static int __devinit omap_wdt_probe(struct platform_device *pdev)
 		goto err_get_resource;
 	}
 
-	if (omap_wdt_dev) {
+	if (omap_wdt_dev[pdev->id-1]) {
 		ret = -EBUSY;
 		goto err_busy;
 	}

@@ -318,8 +339,14 @@ static int __devinit omap_wdt_probe(struct platform_device *pdev)
 
 	wdev->omap_wdt_miscdev.parent = &pdev->dev;

drivers/watchdog/omap_wdt.c static int __devinit omap_wdt_probe(struct platform_device *pdev) · line 349

Quoted 10 lines

drivers/watchdog/omap_wdt.c +7 -1

 	wdev->omap_wdt_miscdev.minor = WATCHDOG_MINOR;
-	wdev->omap_wdt_miscdev.name = "watchdog";
 	wdev->omap_wdt_miscdev.fops = &omap_wdt_fops;
+	wdev->omap_wdt_miscdev.name = "watchdog";
+	if (cpu_is_omap34xx()) {
+		snprintf(wdt_name, sizeof(wdt_name), "watchdog%d", pdev->id);
+		wdev->omap_wdt_miscdev.name = wdt_name;
+		if (pdev->id != 2)
+			wdev->omap_wdt_miscdev.minor = MISC_DYNAMIC_MINOR;
+	}

Again, red flag: cpu_is_*

It might be more consistent to use pdev->dev->name which is already of the form "watchdog.%d".

Any reason not to use MISC_DYNAMIC_MINOR for all of them?

Kevin

Hald, Ulrik Bech

author

2009-06-09 19:28 UTC #5 lore raw

RE: [PATCH 2/3] OMAP3:WDT:Enable support for IVA and SECURE

Quoted 58 lines

-----Original Message-----
From: Kevin Hilman [mailto:[email protected]]
Sent: Tuesday, June 09, 2009 9:47 AM
To: Hald, Ulrik Bech
Cc: [email protected]
Subject: Re: [PATCH 2/3] OMAP3:WDT:Enable support for IVA and SECURE
Quoted 10 lines · Ulrik Bech Hald
This patch enables the IVA and SECURE WDTs in the omap_wdt driver for omap34xx family. SECURE will be available as /dev/watchdog1 HS/EMU devices and IVA will be available as /dev/watchdog3. MPU will be available as /dev/watchdog2
For omap versions earlier than 34xx only MPU watchdog is present and will be available as /dev/watchdog for backwards compatibility.
Signed-off-by Ulrik Bech Hald <[email protected]>
---
 drivers/watchdog/omap_wdt.c |   42 +++++++++++++++++++++++++++++++++++-
------
Quoted 4 lines
 1 files changed, 35 insertions(+), 7 deletions(-)
 mode change 100644 => 100755 drivers/watchdog/omap_wdt.c
drivers/watchdog/omap_wdt.c +0 -0
[...]
drivers/watchdog/omap_wdt.c static void omap_wdt_set_timeout(struct omap_wdt_dev *wdev)
Quoted 11 lines
static int omap_wdt_open(struct inode *inode, struct file *file) {
drivers/watchdog/omap_wdt.c +7 -2
-	struct omap_wdt_dev *wdev = platform_get_drvdata(omap_wdt_dev);
-	void __iomem *base = wdev->base;
+	struct omap_wdt_dev *wdev;
+	void __iomem *base;
+
+	/* by default MPU wdt is present across omap family */
+	wdev = platform_get_drvdata(omap_wdt_dev[1]);
+
+	/* if multiple wdts, find match between device node and wdt device
*/
drivers/watchdog/omap_wdt.c static void omap_wdt_set_timeout(struct omap_wdt_dev *wdev) · line 163
Quoted 13 lines
drivers/watchdog/omap_wdt.c +13 -0
+	if (cpu_is_omap34xx()) {
+		int i;
+		for (i = 0; i < NUM_WDTS; i++) {
+			if (omap_wdt_dev[i]) {
+				wdev = platform_get_drvdata(omap_wdt_dev[i]);
+				if (iminor(inode)
+					== wdev->omap_wdt_miscdev.minor)
+					break;
+			}
+		}
+	}
+
+	base = wdev->base;
Hmm, this does not look right to me. Any use of cpu_is_* in driver code usually indicates that something is not quite right. This same watchdog IP is used in some DaVinci family processors and needs to be reused there.

Quoted 6 lines

I didn't do the miscdev research, but can't you get the pdev from the miscdev.parent, and from there get the right wdev pointer from the pdev.drvdata.
Otherwise, drop the cpu_is_* and just do inode checking all the time. The way you've done it looks error prone to me.

I can't really see a to way pair the driver and device other than doing inode/minor number checking , so I'll just go for the inode checking without any dependency on cpu_is_xxx().

Quoted 39 lines

drivers/watchdog/omap_wdt.c static int __devinit omap_wdt_probe(struct
Quoted 4 lines
drivers/watchdog/omap_wdt.c +0 -0
 	if (test_and_set_bit(1, (unsigned long *)&(wdev->omap_wdt_users)))
 		return -EBUSY;
@@ -263,6 +283,7 @@ static int __devinit omap_wdt_probe(struct
platform_device *pdev)
drivers/watchdog/omap_wdt.c static int __devinit omap_wdt_probe(struct
Quoted 8 lines
drivers/watchdog/omap_wdt.c +1 -0
 	struct resource *res, *mem;
 	struct omap_wdt_dev *wdev;
 	int ret;
+	static char wdt_name[32];
 
 	/* reserve static register mappings */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -271,7 +292,7 @@ static int __devinit omap_wdt_probe(struct
platform_device *pdev)
drivers/watchdog/omap_wdt.c static int __devinit omap_wdt_probe(struct
Quoted 9 lines
drivers/watchdog/omap_wdt.c +1 -1
 		goto err_get_resource;
 	}
 
-	if (omap_wdt_dev) {
+	if (omap_wdt_dev[pdev->id-1]) {
 		ret = -EBUSY;
 		goto err_busy;
 	}
@@ -318,8 +339,14 @@ static int __devinit omap_wdt_probe(struct
platform_device *pdev)
drivers/watchdog/omap_wdt.c static int __devinit omap_wdt_probe(struct platform_device *pdev) · line 349
Quoted 12 lines
drivers/watchdog/omap_wdt.c +7 -1
 	wdev->omap_wdt_miscdev.parent = &pdev->dev;
 	wdev->omap_wdt_miscdev.minor = WATCHDOG_MINOR;
-	wdev->omap_wdt_miscdev.name = "watchdog";
 	wdev->omap_wdt_miscdev.fops = &omap_wdt_fops;
+	wdev->omap_wdt_miscdev.name = "watchdog";
+	if (cpu_is_omap34xx()) {
+		snprintf(wdt_name, sizeof(wdt_name), "watchdog%d", pdev->id);
+		wdev->omap_wdt_miscdev.name = wdt_name;
+		if (pdev->id != 2)
+			wdev->omap_wdt_miscdev.minor = MISC_DYNAMIC_MINOR;
+	}
Again, red flag: cpu_is_*

Quoted 2 lines

It might be more consistent to use pdev->dev->name which is already of the form "watchdog.%d".

Not sure I follow. I don't see a "name" field in struct device, only init_name.

Do you mean just using:
snprintf(wdt_name, sizeof(wdt_name), "watchdog%d", pdev->id); wdev->omap_wdt_miscdev.name = wdt_name;

...for all the WDT devices, despite how many are present?

Quoted 2 lines

Any reason not to use MISC_DYNAMIC_MINOR for all of them?

No particular reason other than I thought that retaining the use of WATCHDOG_MINOR for the MPU WDT case would be appropriate. But it does make the code cleaner to use only MISC_DYNAMIC_MINOR. I'll change it.

Thanks for reviewing!

Quoted 2 lines

Kevin

Kevin Hilman

2009-06-09 21:10 UTC #6 lore raw

Re: [PATCH 2/3] OMAP3:WDT:Enable support for IVA and SECURE

Quoted 62 lines · Ulrik Bech

Quoted 62 lines
-----Original Message-----
From: Kevin Hilman [mailto:[email protected]]
Sent: Tuesday, June 09, 2009 9:47 AM
To: Hald, Ulrik Bech
Cc: [email protected]
Subject: Re: [PATCH 2/3] OMAP3:WDT:Enable support for IVA and SECURE
Quoted 10 lines · Ulrik Bech Hald
This patch enables the IVA and SECURE WDTs in the omap_wdt driver for omap34xx family. SECURE will be available as /dev/watchdog1 HS/EMU devices and IVA will be available as /dev/watchdog3. MPU will be available as /dev/watchdog2
For omap versions earlier than 34xx only MPU watchdog is present and will be available as /dev/watchdog for backwards compatibility.
Signed-off-by Ulrik Bech Hald <[email protected]>
---
 drivers/watchdog/omap_wdt.c |   42 +++++++++++++++++++++++++++++++++++-
------
Quoted 4 lines
 1 files changed, 35 insertions(+), 7 deletions(-)
 mode change 100644 => 100755 drivers/watchdog/omap_wdt.c
drivers/watchdog/omap_wdt.c +0 -0
[...]
drivers/watchdog/omap_wdt.c static void omap_wdt_set_timeout(struct omap_wdt_dev *wdev)
Quoted 11 lines
static int omap_wdt_open(struct inode *inode, struct file *file) {
drivers/watchdog/omap_wdt.c +7 -2
-	struct omap_wdt_dev *wdev = platform_get_drvdata(omap_wdt_dev);
-	void __iomem *base = wdev->base;
+	struct omap_wdt_dev *wdev;
+	void __iomem *base;
+
+	/* by default MPU wdt is present across omap family */
+	wdev = platform_get_drvdata(omap_wdt_dev[1]);
+
+	/* if multiple wdts, find match between device node and wdt device
*/
drivers/watchdog/omap_wdt.c static void omap_wdt_set_timeout(struct omap_wdt_dev *wdev) · line 163
Quoted 13 lines
drivers/watchdog/omap_wdt.c +13 -0
+	if (cpu_is_omap34xx()) {
+		int i;
+		for (i = 0; i < NUM_WDTS; i++) {
+			if (omap_wdt_dev[i]) {
+				wdev = platform_get_drvdata(omap_wdt_dev[i]);
+				if (iminor(inode)
+					== wdev->omap_wdt_miscdev.minor)
+					break;
+			}
+		}
+	}
+
+	base = wdev->base;
Hmm, this does not look right to me. Any use of cpu_is_* in driver code usually indicates that something is not quite right. This same watchdog IP is used in some DaVinci family processors and needs to be reused there.
I didn't do the miscdev research, but can't you get the pdev from the miscdev.parent, and from there get the right wdev pointer from the pdev.drvdata.

Quoted 6 lines

Quoted 2 lines
Otherwise, drop the cpu_is_* and just do inode checking all the time. The way you've done it looks error prone to me.
I can't really see a to way pair the driver and device other than doing inode/minor number checking , so I'll just go for the inode checking without any dependency on cpu_is_xxx().

OK.

Quoted 39 lines

Quoted 39 lines
drivers/watchdog/omap_wdt.c static int __devinit omap_wdt_probe(struct
Quoted 4 lines
drivers/watchdog/omap_wdt.c +0 -0
 	if (test_and_set_bit(1, (unsigned long *)&(wdev->omap_wdt_users)))
 		return -EBUSY;
@@ -263,6 +283,7 @@ static int __devinit omap_wdt_probe(struct
platform_device *pdev)
drivers/watchdog/omap_wdt.c static int __devinit omap_wdt_probe(struct
Quoted 8 lines
drivers/watchdog/omap_wdt.c +1 -0
 	struct resource *res, *mem;
 	struct omap_wdt_dev *wdev;
 	int ret;
+	static char wdt_name[32];
 
 	/* reserve static register mappings */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -271,7 +292,7 @@ static int __devinit omap_wdt_probe(struct
platform_device *pdev)
drivers/watchdog/omap_wdt.c static int __devinit omap_wdt_probe(struct
Quoted 9 lines
drivers/watchdog/omap_wdt.c +1 -1
 		goto err_get_resource;
 	}
 
-	if (omap_wdt_dev) {
+	if (omap_wdt_dev[pdev->id-1]) {
 		ret = -EBUSY;
 		goto err_busy;
 	}
@@ -318,8 +339,14 @@ static int __devinit omap_wdt_probe(struct
platform_device *pdev)
drivers/watchdog/omap_wdt.c static int __devinit omap_wdt_probe(struct platform_device *pdev) · line 349
Quoted 12 lines
drivers/watchdog/omap_wdt.c +7 -1
 	wdev->omap_wdt_miscdev.parent = &pdev->dev;
 	wdev->omap_wdt_miscdev.minor = WATCHDOG_MINOR;
-	wdev->omap_wdt_miscdev.name = "watchdog";
 	wdev->omap_wdt_miscdev.fops = &omap_wdt_fops;
+	wdev->omap_wdt_miscdev.name = "watchdog";
+	if (cpu_is_omap34xx()) {
+		snprintf(wdt_name, sizeof(wdt_name), "watchdog%d", pdev->id);
+		wdev->omap_wdt_miscdev.name = wdt_name;
+		if (pdev->id != 2)
+			wdev->omap_wdt_miscdev.minor = MISC_DYNAMIC_MINOR;
+	}
Again, red flag: cpu_is_*

Quoted 4 lines

Quoted 2 lines
It might be more consistent to use pdev->dev->name which is already of the form "watchdog.%d".
Not sure I follow. I don't see a "name" field in struct device, only init_name.

Sorry, I meant dev->bus_id. Look at dev_set_name() which creates a bus_id of the form ("%s.%d", pdev->name, pdev->id)

Quoted 3 lines

Do you mean just using:
snprintf(wdt_name, sizeof(wdt_name), "watchdog%d", pdev->id); wdev->omap_wdt_miscdev.name = wdt_name;

Using pdev->id is ok, but that's what dev_set_name() does.

Note that if you decide to go this route, note that wdt_name doesn't need to be static. The driver core will copy the name field the misc_device. If it wasn't copying, you'd have all of the miscdev.name fields ending up being the same thing since they are all assigned the same pointer.

Kevin

Quoted 11 lines

...for all the WDT devices, despite how many are present?
Quoted 2 lines
Any reason not to use MISC_DYNAMIC_MINOR for all of them?
No particular reason other than I thought that retaining the use of WATCHDOG_MINOR for the MPU WDT case would be appropriate. But it does make the code cleaner to use only MISC_DYNAMIC_MINOR. I'll change it.
Thanks for reviewing!
Quoted 2 lines
Kevin

Hald, Ulrik Bech

author

2009-06-15 16:19 UTC #7 lore raw

RE: [PATCH 2/3] OMAP3:WDT:Enable support for IVA and SECURE

Quoted 127 lines

-----Original Message-----
From: Kevin Hilman [mailto:[email protected]]
Sent: Tuesday, June 09, 2009 4:10 PM
To: Hald, Ulrik Bech
Cc: [email protected]
Subject: Re: [PATCH 2/3] OMAP3:WDT:Enable support for IVA and SECURE
Quoted 11 lines · Ulrik Bech
Quoted 11 lines
-----Original Message-----
From: Kevin Hilman [mailto:[email protected]]
Sent: Tuesday, June 09, 2009 9:47 AM
To: Hald, Ulrik Bech
Cc: [email protected]
Subject: Re: [PATCH 2/3] OMAP3:WDT:Enable support for IVA and SECURE
Quoted 2 lines · Ulrik Bech Hald
This patch enables the IVA and SECURE WDTs in the omap_wdt driver for omap34xx family. SECURE will be available as
/dev/watchdog1
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
HS/EMU devices and IVA will be available as /dev/watchdog3. MPU will be available as /dev/watchdog2
For omap versions earlier than 34xx only MPU watchdog is present and will be available as /dev/watchdog for backwards compatibility.
Signed-off-by Ulrik Bech Hald <[email protected]>
---
 drivers/watchdog/omap_wdt.c |   42
+++++++++++++++++++++++++++++++++++-
Quoted 5 lines
Quoted 5 lines
------
Review
Quoted 4 lines
 1 files changed, 35 insertions(+), 7 deletions(-)
 mode change 100644 => 100755 drivers/watchdog/omap_wdt.c
(quoted diff) +0 -0
b/drivers/watchdog/omap_wdt.c
Quoted 6 lines
Quoted 6 lines
[...]
Review
Quoted 3 lines
static int omap_wdt_open(struct inode *inode, struct file *file) {
(quoted diff) +0 -1
-	struct omap_wdt_dev *wdev =
platform_get_drvdata(omap_wdt_dev);
Quoted 8 lines
Quoted 8 lines
drivers/watchdog/omap_wdt.c static void omap_wdt_set_timeout(struct omap_wdt_dev *wdev)
Quoted 8 lines
drivers/watchdog/omap_wdt.c +7 -1
-	void __iomem *base = wdev->base;
+	struct omap_wdt_dev *wdev;
+	void __iomem *base;
+
+	/* by default MPU wdt is present across omap family */
+	wdev = platform_get_drvdata(omap_wdt_dev[1]);
+
+	/* if multiple wdts, find match between device node and wdt
device
Quoted 6 lines
Quoted 6 lines
*/
drivers/watchdog/omap_wdt.c static void omap_wdt_set_timeout(struct omap_wdt_dev *wdev)
Quoted 5 lines
drivers/watchdog/omap_wdt.c +5 -0
+	if (cpu_is_omap34xx()) {
+		int i;
+		for (i = 0; i < NUM_WDTS; i++) {
+			if (omap_wdt_dev[i]) {
+				wdev =
platform_get_drvdata(omap_wdt_dev[i]);
Quoted 18 lines
Quoted 18 lines
drivers/watchdog/omap_wdt.c static void omap_wdt_set_timeout(struct omap_wdt_dev *wdev) · line 163
Quoted 8 lines
drivers/watchdog/omap_wdt.c +8 -0
+				if (iminor(inode)
+					== wdev->omap_wdt_miscdev.minor)
+					break;
+			}
+		}
+	}
+
+	base = wdev->base;
Hmm, this does not look right to me. Any use of cpu_is_* in driver code usually indicates that something is not quite right. This same watchdog IP is used in some DaVinci family processors and needs to be reused there.
I didn't do the miscdev research, but can't you get the pdev from the miscdev.parent, and from there get the right wdev pointer from the pdev.drvdata.
Quoted 6 lines
Quoted 2 lines
Otherwise, drop the cpu_is_* and just do inode checking all the time. The way you've done it looks error prone to me.
I can't really see a to way pair the driver and device other than doing inode/minor number checking , so I'll just go for the inode checking without any dependency on cpu_is_xxx().
OK.
Quoted 41 lines
Quoted 2 lines
Quoted 2 lines
if (test_and_set_bit(1, (unsigned long *)&(wdev-
omap_wdt_users)))
Quoted 23 lines
drivers/watchdog/omap_wdt.c static int __devinit omap_wdt_probe(struct
Quoted 2 lines
drivers/watchdog/omap_wdt.c +0 -0
 		return -EBUSY;
@@ -263,6 +283,7 @@ static int __devinit omap_wdt_probe(struct
platform_device *pdev)
drivers/watchdog/omap_wdt.c static int __devinit omap_wdt_probe(struct
Quoted 8 lines
drivers/watchdog/omap_wdt.c +1 -0
 	struct resource *res, *mem;
 	struct omap_wdt_dev *wdev;
 	int ret;
+	static char wdt_name[32];
 
 	/* reserve static register mappings */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -271,7 +292,7 @@ static int __devinit omap_wdt_probe(struct
platform_device *pdev)
drivers/watchdog/omap_wdt.c static int __devinit omap_wdt_probe(struct
Quoted 9 lines
drivers/watchdog/omap_wdt.c +1 -1
 		goto err_get_resource;
 	}
 
-	if (omap_wdt_dev) {
+	if (omap_wdt_dev[pdev->id-1]) {
 		ret = -EBUSY;
 		goto err_busy;
 	}
@@ -318,8 +339,14 @@ static int __devinit omap_wdt_probe(struct
platform_device *pdev)
Quoted 1 line
Quoted 7 lines
drivers/watchdog/omap_wdt.c static int __devinit omap_wdt_probe(struct platform_device *pdev)
Quoted 7 lines
drivers/watchdog/omap_wdt.c +3 -1
 	wdev->omap_wdt_miscdev.parent = &pdev->dev;
 	wdev->omap_wdt_miscdev.minor = WATCHDOG_MINOR;
-	wdev->omap_wdt_miscdev.name = "watchdog";
 	wdev->omap_wdt_miscdev.fops = &omap_wdt_fops;
+	wdev->omap_wdt_miscdev.name = "watchdog";
+	if (cpu_is_omap34xx()) {
+		snprintf(wdt_name, sizeof(wdt_name), "watchdog%d", pdev-
id);
Quoted 7 lines
drivers/watchdog/omap_wdt.c static int __devinit omap_wdt_probe(struct platform_device *pdev) · line 349
Quoted 4 lines
drivers/watchdog/omap_wdt.c +4 -0
+		wdev->omap_wdt_miscdev.name = wdt_name;
+		if (pdev->id != 2)
+			wdev->omap_wdt_miscdev.minor = MISC_DYNAMIC_MINOR;
+	}
Again, red flag: cpu_is_*

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
It might be more consistent to use pdev->dev->name which is already of the form "watchdog.%d".
Not sure I follow. I don't see a "name" field in struct device, only
init_name.
Sorry, I meant dev->bus_id. Look at dev_set_name() which creates a bus_id of the form ("%s.%d", pdev->name, pdev->id)

As far as I can see, dev->bus_id has been removed from the device struct as per 1fa5ae857bb14f6046205171d98506d8112dd74e.
Furthermore, the pdev->name is "omap_wdt", which should be presented as "watchdog" in wdev->omap_wdt_miscdev.name anyway, so I think I'll just go with the sprintf() approach below, keeping your wdt_name comments in mind.

/Ulrik

Quoted 28 lines

Quoted 4 lines
Do you mean just using:
snprintf(wdt_name, sizeof(wdt_name), "watchdog%d", pdev->id); wdev->omap_wdt_miscdev.name = wdt_name;
Using pdev->id is ok, but that's what dev_set_name() does.
Note that if you decide to go this route, note that wdt_name doesn't need to be static. The driver core will copy the name field the misc_device. If it wasn't copying, you'd have all of the miscdev.name fields ending up being the same thing since they are all assigned the same pointer.
Kevin
Quoted 6 lines
...for all the WDT devices, despite how many are present?
Quoted 2 lines
Any reason not to use MISC_DYNAMIC_MINOR for all of them?
No particular reason other than I thought that retaining the use of
WATCHDOG_MINOR for the MPU WDT case would be appropriate. But it does make the code cleaner to use only MISC_DYNAMIC_MINOR. I'll change it.
Quoted 5 lines
Thanks for reviewing!
Quoted 2 lines
Kevin

Kevin Hilman

2009-06-15 16:26 UTC #8 lore raw

Re: [PATCH 2/3] OMAP3:WDT:Enable support for IVA and SECURE

"Hald, Ulrik Bech" <[email protected]> writes:

[...]

Quoted 12 lines

Quoted 11 lines
Quoted 7 lines
Quoted 5 lines
Again, red flag: cpu_is_*
It might be more consistent to use pdev->dev->name which is already of the form "watchdog.%d".
Not sure I follow. I don't see a "name" field in struct device, only
init_name.
Sorry, I meant dev->bus_id. Look at dev_set_name() which creates a bus_id of the form ("%s.%d", pdev->name, pdev->id)

Quoted 6 lines

As far as I can see, dev->bus_id has been removed from the device struct as per 1fa5ae857bb14f6046205171d98506d8112dd74e. Furthermore, the pdev->name is "omap_wdt", which should be presented as "watchdog" in wdev->omap_wdt_miscdev.name anyway, so I think I'll just go with the sprintf() approach below, keeping your wdt_name comments in mind.

Kevin

Kevin Hilman

2009-06-09 14:50 UTC #9 lore raw

In reply to Ulrik Bech Hald

Quoted 10 lines · Ulrik Bech Hald

This patch series enables support for IVA and SECURE WDTs, available on omap34xx.
For omap34xx devices the WDT will be accessible (when present) through:
SECURE: /dev/watchdog1
MPU: /dev/watchdog2
IVA: /dev/watchdog3
For devices older than omap34xx only MPU WDT is present and will be accessible through /dev/watchdog

I think you should make the MPU WDT the first one since it will always be present.

Quoted 6 lines

The series also fixes two bugs:
1) Correct timeout value is not loaded upon opening the watchdog device.
2) clks are not enabled when accessing registers in probe

Can you fix these existing bugs in a separate patch before you add the new features.

Kevin

Hald, Ulrik Bech

author

2009-06-09 18:02 UTC #10 lore raw

Quoted 17 lines

-----Original Message-----
From: Kevin Hilman [mailto:[email protected]]
Sent: Tuesday, June 09, 2009 9:45 AM
To: Hald, Ulrik Bech
Cc: [email protected]
Subject: Re: [PATCH 0/3] OMAP3:WDT:Enable IVA, SECURE and minor bugfixes
Quoted 8 lines · Ulrik Bech Hald
This patch series enables support for IVA and SECURE WDTs, available on omap34xx.
For omap34xx devices the WDT will be accessible (when present) through:
SECURE: /dev/watchdog1
MPU: /dev/watchdog2
IVA: /dev/watchdog3

Quoted 5 lines

Quoted 2 lines
For devices older than omap34xx only MPU WDT is present and will be accessible through /dev/watchdog
I think you should make the MPU WDT the first one since it will always be present.

The reason, why I numbered them as above, is to make them match the OMAP34xx TRM WDT numbering scheme, where SECURE WDT=WDT1, MPU WDT=WDT2 and IVA WDT=WDT3. My thought was that it would introduce more confusion to change the numbers in /dev/ to something else, although I did consider your point. Do you still think I should change the numbers?

Quoted 10 lines

Quoted 7 lines
The series also fixes two bugs:
1) Correct timeout value is not loaded upon opening the watchdog device.
2) clks are not enabled when accessing registers in probe
Can you fix these existing bugs in a separate patch before you add the new features.

Sure, no problem.

Quoted 2 lines

Kevin

Kevin Hilman

2009-06-09 18:16 UTC #11 lore raw

Quoted 17 lines · Ulrik Bech

Quoted 17 lines
-----Original Message-----
From: Kevin Hilman [mailto:[email protected]]
Sent: Tuesday, June 09, 2009 9:45 AM
To: Hald, Ulrik Bech
Cc: [email protected]
Subject: Re: [PATCH 0/3] OMAP3:WDT:Enable IVA, SECURE and minor bugfixes
Quoted 8 lines · Ulrik Bech Hald
This patch series enables support for IVA and SECURE WDTs, available on omap34xx.
For omap34xx devices the WDT will be accessible (when present) through:
SECURE: /dev/watchdog1
MPU: /dev/watchdog2
IVA: /dev/watchdog3

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
For devices older than omap34xx only MPU WDT is present and will be accessible through /dev/watchdog
I think you should make the MPU WDT the first one since it will always be present.
The reason, why I numbered them as above, is to make them match the OMAP34xx TRM WDT numbering scheme, where SECURE WDT=WDT1, MPU WDT=WDT2 and IVA WDT=WDT3. My thought was that it would introduce more confusion to change the numbers in /dev/ to something else, although I did consider your point. Do you still think I should change the numbers?

Yes. Personally, I don't think the TRM should influence userspace visible nodes in this case.

I would rather see the watchdog that exists on all platforms be the first one.

Kevin