[PATCH 0/3] OMAP3:WDT:Enable IVA, SECURE and minor bugfixes

[PATCH 0/3] OMAP3:WDT:Enable IVA, SECURE and minor bugfixes

[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

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

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

[Ulrik Bech Hald]

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

Signed-off-by: Ulrik Bech Hald <ubh@ti.com>
---
 arch/arm/mach-omap2/clock34xx.c |   12 +++---
 arch/arm/plat-omap/devices.c    |   79 +++++++++++++++++++++++++++++++-------
 2 files changed, 70 insertions(+), 21 deletions(-)

diff --git a/arch/arm/mach-omap2/clock34xx.c b/arch/arm/mach-omap2/clock34xx.c
index ba05aa4..fe43d73 100644
--- a/arch/arm/mach-omap2/clock34xx.c
+++ b/arch/arm/mach-omap2/clock34xx.c
@@ -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 */
diff --git a/arch/arm/plat-omap/devices.c b/arch/arm/plat-omap/devices.c
index 87fb7ff..2a1b806 100644
--- a/arch/arm/plat-omap/devices.c
+++ b/arch/arm/plat-omap/devices.c
@@ -288,36 +288,85 @@ static inline void omap_init_uwire(void) {}
 
 #if	defined(CONFIG_OMAP_WATCHDOG) || defined(CONFIG_OMAP_WATCHDOG_MODULE)
 
-static struct resource wdt_resources[] = {
+#define	OMAP34XX_WDT1_BASE	0x4830c000
+#define	OMAP34XX_WDT2_BASE	0x48314000
+#define	OMAP34XX_WDT3_BASE	0x49030000
+#define	OMAP2430_WDT_BASE	0x49016000
+#define	OMAP2420_WDT_BASE	0x48022000
+#define	OMAP16XX_WDT_BASE	0xfffeb000
+
+static struct resource wdt1_resources[] = {
 	{
-		.flags		= IORESOURCE_MEM,
+		.flags = IORESOURCE_MEM,
 	},
 };
 
-static struct platform_device omap_wdt_device = {
-	.name	   = "omap_wdt",
-	.id	     = -1,
-	.num_resources	= ARRAY_SIZE(wdt_resources),
-	.resource	= wdt_resources,
+static struct resource wdt2_resources[] = {
+	{
+		.flags = IORESOURCE_MEM,
+	},
+};
+
+static struct resource wdt3_resources[] = {
+	{
+		.flags = IORESOURCE_MEM,
+	},
+};
+
+/* SECURE WDT */
+static struct platform_device omap_wdt1_device = {
+	.name = "omap_wdt",
+	.id = 1,
+	.num_resources = ARRAY_SIZE(wdt1_resources),
+	.resource = wdt1_resources,
+};
+
+/* MPU WDT */
+static struct platform_device omap_wdt2_device = {
+	.name = "omap_wdt",
+	.id = 2,
+	.num_resources = ARRAY_SIZE(wdt2_resources),
+	.resource = wdt2_resources,
+};
+
+/* IVA WDT */
+static struct platform_device omap_wdt3_device = {
+	.name = "omap_wdt",
+	.id = 3,
+	.num_resources = ARRAY_SIZE(wdt3_resources),
+	.resource = wdt3_resources,
 };
 
 static void omap_init_wdt(void)
 {
 	if (cpu_is_omap16xx())
-		wdt_resources[0].start = 0xfffeb000;
+		wdt2_resources[0].start = OMAP16XX_WDT_BASE;
 	else if (cpu_is_omap2420())
-		wdt_resources[0].start = 0x48022000; /* WDT2 */
+		wdt2_resources[0].start = OMAP2420_WDT_BASE;
 	else if (cpu_is_omap2430())
-		wdt_resources[0].start = 0x49016000; /* WDT2 */
-	else if (cpu_is_omap343x())
-		wdt_resources[0].start = 0x48314000; /* WDT2 */
+		wdt2_resources[0].start = OMAP2430_WDT_BASE;
+	else if (cpu_is_omap343x()) {
+		wdt1_resources[0].start = OMAP34XX_WDT1_BASE;
+		wdt1_resources[0].end = wdt1_resources[0].start + 0x4f;
+		wdt2_resources[0].start = OMAP34XX_WDT2_BASE;
+		wdt3_resources[0].start = OMAP34XX_WDT3_BASE;
+		wdt3_resources[0].end = wdt3_resources[0].start + 0x4f;
+	}
 	else
 		return;
 
-	wdt_resources[0].end = wdt_resources[0].start + 0x4f;
+	wdt2_resources[0].end = wdt2_resources[0].start + 0x4f;
 
-	(void) platform_device_register(&omap_wdt_device);
-}
+	/* MPU WDT present across omap family */
+	(void) platform_device_register(&omap_wdt2_device);
+
+	if (cpu_is_omap34xx()) {
+		(void) platform_device_register(&omap_wdt3_device);
+		if (omap_type() == OMAP2_DEVICE_TYPE_SEC
+			|| omap_type() == OMAP2_DEVICE_TYPE_EMU)
+			(void) platform_device_register(&omap_wdt1_device);
+		}
+	}
 #else
 static inline void omap_init_wdt(void) {}
 #endif
-- 
1.5.4.3

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

[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 <ubh@ti.com>
---
 drivers/watchdog/omap_wdt.c |   42 +++++++++++++++++++++++++++++++++++-------
 1 files changed, 35 insertions(+), 7 deletions(-)
 mode change 100644 => 100755 drivers/watchdog/omap_wdt.c

diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
old mode 100644
new mode 100755
index f271385..26935c7
--- a/drivers/watchdog/omap_wdt.c
+++ b/drivers/watchdog/omap_wdt.c
@@ -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;
 }
-- 
1.5.4.3

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

[Ulrik Bech Hald]

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 <ubh@ti.com>
---
 drivers/watchdog/omap_wdt.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
index 26935c7..cb580a8 100755
--- a/drivers/watchdog/omap_wdt.c
+++ b/drivers/watchdog/omap_wdt.c
@@ -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;
 
-- 
1.5.4.3

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

[Kevin Hilman]

Ulrik Bech Hald <ubh@ti.com> writes:

> 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 <ubh@ti.com>
> ---
>  drivers/watchdog/omap_wdt.c |   42 +++++++++++++++++++++++++++++++++++-------
>  1 files changed, 35 insertions(+), 7 deletions(-)
>  mode change 100644 => 100755 drivers/watchdog/omap_wdt.c
>
> diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c

[...]

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

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.

>  	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;
> +	}

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

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

[Hald, Ulrik Bech]

> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
> Sent: Tuesday, June 09, 2009 9:47 AM
> To: Hald, Ulrik Bech
> Cc: linux-omap@vger.kernel.org
> Subject: Re: [PATCH 2/3] OMAP3:WDT:Enable support for IVA and SECURE
> 
> Ulrik Bech Hald <ubh@ti.com> writes:
> 
> > 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 <ubh@ti.com>
> > ---
> >  drivers/watchdog/omap_wdt.c |   42 +++++++++++++++++++++++++++++++++++-
> ------
> >  1 files changed, 35 insertions(+), 7 deletions(-)
> >  mode change 100644 => 100755 drivers/watchdog/omap_wdt.c
> >
> > diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
> 
> [...]
> 
> >  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;
> 
> 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.

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().

> 
> >  	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;
> > +	}
> 
> 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.
	
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?

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

> 
> Kevin

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

[Kevin Hilman]

"Hald, Ulrik Bech" <ubh@ti.com> writes:

>> -----Original Message-----
>> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
>> Sent: Tuesday, June 09, 2009 9:47 AM
>> To: Hald, Ulrik Bech
>> Cc: linux-omap@vger.kernel.org
>> Subject: Re: [PATCH 2/3] OMAP3:WDT:Enable support for IVA and SECURE
>> 
>> Ulrik Bech Hald <ubh@ti.com> writes:
>> 
>> > 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 <ubh@ti.com>
>> > ---
>> >  drivers/watchdog/omap_wdt.c |   42 +++++++++++++++++++++++++++++++++++-
>> ------
>> >  1 files changed, 35 insertions(+), 7 deletions(-)
>> >  mode change 100644 => 100755 drivers/watchdog/omap_wdt.c
>> >
>> > diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
>> 
>> [...]
>> 
>> >  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;
>> 
>> 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.
>
> 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.

>> 
>> >  	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;
>> > +	}
>> 
>> 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)

> 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

> ...for all the WDT devices, despite how many are present?
>
>> 
>> 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!
>
>> 
>> Kevin

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

[Hald, Ulrik Bech]

> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
> Sent: Tuesday, June 09, 2009 4:10 PM
> To: Hald, Ulrik Bech
> Cc: linux-omap@vger.kernel.org
> Subject: Re: [PATCH 2/3] OMAP3:WDT:Enable support for IVA and SECURE
> 
> "Hald, Ulrik Bech" <ubh@ti.com> writes:
> 
> >> -----Original Message-----
> >> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
> >> Sent: Tuesday, June 09, 2009 9:47 AM
> >> To: Hald, Ulrik Bech
> >> Cc: linux-omap@vger.kernel.org
> >> Subject: Re: [PATCH 2/3] OMAP3:WDT:Enable support for IVA and SECURE
> >>
> >> Ulrik Bech Hald <ubh@ti.com> writes:
> >>
> >> > 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 <ubh@ti.com>
> >> > ---
> >> >  drivers/watchdog/omap_wdt.c |   42
> +++++++++++++++++++++++++++++++++++-
> >> ------
> >> >  1 files changed, 35 insertions(+), 7 deletions(-)
> >> >  mode change 100644 => 100755 drivers/watchdog/omap_wdt.c
> >> >
> >> > diff --git a/drivers/watchdog/omap_wdt.c
> b/drivers/watchdog/omap_wdt.c
> >>
> >> [...]
> >>
> >> >  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;
> >>
> >> 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.
> >
> > 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.
> 
> >>
> >> >  	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;
> >> > +	}
> >>
> >> 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)

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

> 
> > 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
> 
> > ...for all the WDT devices, despite how many are present?
> >
> >>
> >> 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!
> >
> >>
> >> Kevin

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

[Kevin Hilman]

"Hald, Ulrik Bech" <ubh@ti.com> writes:

[...]

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

OK

Kevin

Re: [PATCH 0/3] OMAP3:WDT:Enable IVA, SECURE and minor bugfixes

[Kevin Hilman]

Ulrik Bech Hald <ubh@ti.com> writes:

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

> 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

RE: [PATCH 0/3] OMAP3:WDT:Enable IVA, SECURE and minor bugfixes

[Hald, Ulrik Bech]

> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
> Sent: Tuesday, June 09, 2009 9:45 AM
> To: Hald, Ulrik Bech
> Cc: linux-omap@vger.kernel.org
> Subject: Re: [PATCH 0/3] OMAP3:WDT:Enable IVA, SECURE and minor bugfixes
> 
> Ulrik Bech Hald <ubh@ti.com> writes:
> 
> > 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.

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?

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

> 
> Kevin

Re: [PATCH 0/3] OMAP3:WDT:Enable IVA, SECURE and minor bugfixes

[Kevin Hilman]

"Hald, Ulrik Bech" <ubh@ti.com> writes:

>> -----Original Message-----
>> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
>> Sent: Tuesday, June 09, 2009 9:45 AM
>> To: Hald, Ulrik Bech
>> Cc: linux-omap@vger.kernel.org
>> Subject: Re: [PATCH 0/3] OMAP3:WDT:Enable IVA, SECURE and minor bugfixes
>> 
>> Ulrik Bech Hald <ubh@ti.com> writes:
>> 
>> > 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.
>
> 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