[PATCH 0/2] watchdog:OMAP3:Add support for IVA2, SECURE WDTs

[PATCH 0/2] watchdog:OMAP3:Add support for IVA2, SECURE WDTs

[Ulrik Bech Hald]

This patch series enables support for IVA2 and SECURE
WDTs, available on omap34xx.
The WDTs will be accessible (when present on device) through:
MPU:	/dev/watchdog
SECURE:	/dev/watchdog_secure
IVA:	/dev/watchdog_iva2

Signed-off-by: Ulrik Bech Hald <ubh@ti.com>
--- 
This patch set has a dependency on:
[PATCH 1/2] OMAP2/3: SoC IDs: add omap_type() for determining GP/EMU/HS
[PATCH 1/1] watchdog: OMAP fixes: enable clock in probe, trigger timer reload

 arch/arm/mach-omap2/clock34xx.c |   12 ++---
 arch/arm/plat-omap/devices.c    |   81 ++++++++++++++++++++++++++++++++--------
 drivers/watchdog/omap_wdt.c     |   51 +++++++++++++++++++++----
 3 files changed, 114 insertions(+), 30 deletions(-)

[PATCH 1/2] watchdog:OMAP3:Register IVA and SECURE WDT, 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>
---
This patch has a dependency on:
PATCH 1/2] OMAP2/3: SoC IDs: add omap_type() for determining GP/EMU/HS

 arch/arm/mach-omap2/clock34xx.c |   12 +++---
 arch/arm/plat-omap/devices.c    |   81 +++++++++++++++++++++++++++++++--------
 2 files changed, 71 insertions(+), 22 deletions(-)
 mode change 100644 => 100755 arch/arm/plat-omap/devices.c

diff --git a/arch/arm/mach-omap2/clock34xx.c b/arch/arm/mach-omap2/clock34xx.c
index 9e43fe5..933ae9e 100644
--- a/arch/arm/mach-omap2/clock34xx.c
+++ b/arch/arm/mach-omap2/clock34xx.c
@@ -215,11 +215,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),
@@ -241,14 +241,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),
@@ -275,7 +275,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
old mode 100644
new mode 100755
index a64b692..de5182c
--- a/arch/arm/plat-omap/devices.c
+++ b/arch/arm/plat-omap/devices.c
@@ -288,42 +288,91 @@ 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 */
-	else if (cpu_is_omap44xx())
-		wdt_resources[0].start = 0x4a314000;
+		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
 
+
 /*-------------------------------------------------------------------------*/
 
 #if defined(CONFIG_HW_RANDOM_OMAP) || defined(CONFIG_HW_RANDOM_OMAP_MODULE)
-- 
1.5.4.3

[PATCH 2/2] watchdog:OMAP3:Enable support for IVA2 and SECURE

[Ulrik Bech Hald]

This patch enables the IVA2 and SECURE WDTs in the omap_wdt
driver for omap34xx family. SECURE will be available as
/dev/watchdog_secure on HS/EMU devices and IVA2 will be available
as /dev/watchdog_iva2. MPU will still be available as /dev/watchdog

Signed-off-by: Ulrik Bech Hald <ubh@ti.com>
---
This patch has a dependency on:
[PATCH 1/1] watchdog: OMAP fixes: enable clock in probe, trigger timer reload

 drivers/watchdog/omap_wdt.c |   51 ++++++++++++++++++++++++++++++++++++------
 1 files changed, 43 insertions(+), 8 deletions(-)

diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
index f271385..85a92de 100644
--- a/drivers/watchdog/omap_wdt.c
+++ b/drivers/watchdog/omap_wdt.c
@@ -47,7 +47,15 @@
 
 #include "omap_wdt.h"
 
-static struct platform_device *omap_wdt_dev;
+#define NUM_WDTS	3
+
+enum {
+	SECURE_WDT = 1,
+	MPU_WDT,
+	IVA2_WDT,
+};
+
+static struct platform_device *omap_wdt_dev[NUM_WDTS];
 
 static unsigned timer_margin;
 module_param(timer_margin, uint, 0);
@@ -139,8 +147,23 @@ 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]);
+
+	/* Find match between device node and wdt device */
+	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;
@@ -271,7 +294,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;
 	}
@@ -317,10 +340,21 @@ static int __devinit omap_wdt_probe(struct platform_device *pdev)
 	omap_wdt_adjust_timeout(timer_margin);
 
 	wdev->omap_wdt_miscdev.parent = &pdev->dev;
-	wdev->omap_wdt_miscdev.minor = WATCHDOG_MINOR;
-	wdev->omap_wdt_miscdev.name = "watchdog";
+	wdev->omap_wdt_miscdev.minor = MISC_DYNAMIC_MINOR;
 	wdev->omap_wdt_miscdev.fops = &omap_wdt_fops;
 
+	switch (pdev->id) {
+	case SECURE_WDT:
+		wdev->omap_wdt_miscdev.name = "watchdog_secure";
+		break;
+	case IVA2_WDT:
+		wdev->omap_wdt_miscdev.name = "watchdog_iva2";
+		break;
+	case MPU_WDT:
+	default:
+		wdev->omap_wdt_miscdev.name = "watchdog";
+	}
+
 	ret = misc_register(&(wdev->omap_wdt_miscdev));
 	if (ret)
 		goto err_misc;
@@ -332,7 +366,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 +419,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

Re: [PATCH 1/2] watchdog:OMAP3:Register IVA and SECURE WDT, make clks accessible

[Kevin Hilman]

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

> 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>
> ---
> This patch has a dependency on:
> PATCH 1/2] OMAP2/3: SoC IDs: add omap_type() for determining GP/EMU/HS
>
>  arch/arm/mach-omap2/clock34xx.c |   12 +++---
>  arch/arm/plat-omap/devices.c    |   81 +++++++++++++++++++++++++++++++--------
>  2 files changed, 71 insertions(+), 22 deletions(-)
>  mode change 100644 => 100755 arch/arm/plat-omap/devices.c
>
> diff --git a/arch/arm/mach-omap2/clock34xx.c b/arch/arm/mach-omap2/clock34xx.c
> index 9e43fe5..933ae9e 100644
> --- a/arch/arm/mach-omap2/clock34xx.c
> +++ b/arch/arm/mach-omap2/clock34xx.c
> @@ -215,11 +215,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),
> @@ -241,14 +241,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),
> @@ -275,7 +275,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
> old mode 100644
> new mode 100755
> index a64b692..de5182c
> --- a/arch/arm/plat-omap/devices.c
> +++ b/arch/arm/plat-omap/devices.c
> @@ -288,42 +288,91 @@ 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,
>  };

Rather than use the /* SECURE WDT */ comments, why not
just name the platform_device instead of using numbers.

For example, use:

  struct platform_device mpu_wdt_device;
  struct platform_device secure_wdt_device;
  struct platform_device iva_wdt_device;

and same for the struct resources:

  struct resource mpu_wdt_resource;
  ...

That way the numbering issues below will not be so confusing.

Kevin

>  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 */
> -	else if (cpu_is_omap44xx())
> -		wdt_resources[0].start = 0x4a314000;
> +		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
>  
> +
>  /*-------------------------------------------------------------------------*/
>  
>  #if defined(CONFIG_HW_RANDOM_OMAP) || defined(CONFIG_HW_RANDOM_OMAP_MODULE)
> -- 
> 1.5.4.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] watchdog:OMAP3:Enable support for IVA2 and SECURE

[Kevin Hilman]

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

> This patch enables the IVA2 and SECURE WDTs in the omap_wdt
> driver for omap34xx family. SECURE will be available as
> /dev/watchdog_secure on HS/EMU devices and IVA2 will be available
> as /dev/watchdog_iva2. MPU will still be available as /dev/watchdog
>
> Signed-off-by: Ulrik Bech Hald <ubh@ti.com>
> ---
> This patch has a dependency on:
> [PATCH 1/1] watchdog: OMAP fixes: enable clock in probe, trigger timer reload
>
>  drivers/watchdog/omap_wdt.c |   51 ++++++++++++++++++++++++++++++++++++------
>  1 files changed, 43 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
> index f271385..85a92de 100644
> --- a/drivers/watchdog/omap_wdt.c
> +++ b/drivers/watchdog/omap_wdt.c
> @@ -47,7 +47,15 @@
>  
>  #include "omap_wdt.h"
>  
> -static struct platform_device *omap_wdt_dev;
> +#define NUM_WDTS	3
> +
> +enum {
> +	SECURE_WDT = 1,
> +	MPU_WDT,
> +	IVA2_WDT,
> +};
> +
> +static struct platform_device *omap_wdt_dev[NUM_WDTS];

If you're going to have shared numbers here, they should be shared and
also used by the SoC init code.  IOW, the id's you're using in the
devices.c file affects the numbering here.

Also, why not use zero-based numbering here and in devices.c, then
you can avoide the 'pdev->id - 1' below.

>  static unsigned timer_margin;
>  module_param(timer_margin, uint, 0);
> @@ -139,8 +147,23 @@ 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]);

Drop this and just use the inode match.

> +	/* Find match between device node and wdt device */
> +	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;
> +		}
> +	}

You should check for a valid match here.

> +	base = wdev->base;
>  
>  	if (test_and_set_bit(1, (unsigned long *)&(wdev->omap_wdt_users)))
>  		return -EBUSY;
> @@ -271,7 +294,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]) {

With zero-based numbering, you can drop the -1.

>  		ret = -EBUSY;
>  		goto err_busy;
>  	}
> @@ -317,10 +340,21 @@ static int __devinit omap_wdt_probe(struct platform_device *pdev)
>  	omap_wdt_adjust_timeout(timer_margin);
>  
>  	wdev->omap_wdt_miscdev.parent = &pdev->dev;
> -	wdev->omap_wdt_miscdev.minor = WATCHDOG_MINOR;
> -	wdev->omap_wdt_miscdev.name = "watchdog";
> +	wdev->omap_wdt_miscdev.minor = MISC_DYNAMIC_MINOR;
>  	wdev->omap_wdt_miscdev.fops = &omap_wdt_fops;
>  
> +	switch (pdev->id) {
> +	case SECURE_WDT:
> +		wdev->omap_wdt_miscdev.name = "watchdog_secure";
> +		break;
> +	case IVA2_WDT:
> +		wdev->omap_wdt_miscdev.name = "watchdog_iva2";
> +		break;
> +	case MPU_WDT:
> +	default:
> +		wdev->omap_wdt_miscdev.name = "watchdog";
> +	}
> +

The more think about this, the more I don't like this pdev->id
switching in the driver.  The only thing it is needed for
is to set the name of the node.

Instead, why not set the name in devices.c and pass it in
using platform_data.

Then you can drop the enum and the pdev->id switching.

>  	ret = misc_register(&(wdev->omap_wdt_miscdev));
>  	if (ret)
>  		goto err_misc;
> @@ -332,7 +366,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 +419,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
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 2/2] watchdog:OMAP3:Enable support for IVA2 and SECURE

[Hald, Ulrik Bech]

> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
> Sent: Tuesday, June 16, 2009 10:27 AM
> To: Hald, Ulrik Bech
> Cc: linux-omap@vger.kernel.org
> Subject: Re: [PATCH 2/2] watchdog:OMAP3:Enable support for IVA2 and SECURE
> 
> Ulrik Bech Hald <ubh@ti.com> writes:
> 
> > This patch enables the IVA2 and SECURE WDTs in the omap_wdt
> > driver for omap34xx family. SECURE will be available as
> > /dev/watchdog_secure on HS/EMU devices and IVA2 will be available
> > as /dev/watchdog_iva2. MPU will still be available as /dev/watchdog
> >
> > Signed-off-by: Ulrik Bech Hald <ubh@ti.com>
> > ---
> > This patch has a dependency on:
> > [PATCH 1/1] watchdog: OMAP fixes: enable clock in probe, trigger timer
> reload
> >
> >  drivers/watchdog/omap_wdt.c |   51
> ++++++++++++++++++++++++++++++++++++------
> >  1 files changed, 43 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
> > index f271385..85a92de 100644
> > --- a/drivers/watchdog/omap_wdt.c
> > +++ b/drivers/watchdog/omap_wdt.c
> > @@ -47,7 +47,15 @@
> >
> >  #include "omap_wdt.h"
> >
> > -static struct platform_device *omap_wdt_dev;
> > +#define NUM_WDTS	3
> > +
> > +enum {
> > +	SECURE_WDT = 1,
> > +	MPU_WDT,
> > +	IVA2_WDT,
> > +};
> > +
> > +static struct platform_device *omap_wdt_dev[NUM_WDTS];
> 
> If you're going to have shared numbers here, they should be shared and
> also used by the SoC init code.  IOW, the id's you're using in the
> devices.c file affects the numbering here.
> 
> Also, why not use zero-based numbering here and in devices.c, then
> you can avoide the 'pdev->id - 1' below.
> 
> >  static unsigned timer_margin;
> >  module_param(timer_margin, uint, 0);
> > @@ -139,8 +147,23 @@ 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]);
> 
> Drop this and just use the inode match.
> 
Was considering that, but ended up defaulting value instead of error checking later. I'll change the above.

> > +	/* Find match between device node and wdt device */
> > +	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;
> > +		}
> > +	}
> 
> You should check for a valid match here.
> 
The sanity check I would choose here is

struct omap_wdt_dev *wdev = NULL;
...
<inode matching>
...
if(!wdev)
	return -ENODEV; 

> > +	base = wdev->base;
> >
> >  	if (test_and_set_bit(1, (unsigned long *)&(wdev->omap_wdt_users)))
> >  		return -EBUSY;
> > @@ -271,7 +294,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]) {
> 
> With zero-based numbering, you can drop the -1.
> 
> >  		ret = -EBUSY;
> >  		goto err_busy;
> >  	}
> > @@ -317,10 +340,21 @@ static int __devinit omap_wdt_probe(struct
> platform_device *pdev)
> >  	omap_wdt_adjust_timeout(timer_margin);
> >
> >  	wdev->omap_wdt_miscdev.parent = &pdev->dev;
> > -	wdev->omap_wdt_miscdev.minor = WATCHDOG_MINOR;
> > -	wdev->omap_wdt_miscdev.name = "watchdog";
> > +	wdev->omap_wdt_miscdev.minor = MISC_DYNAMIC_MINOR;
> >  	wdev->omap_wdt_miscdev.fops = &omap_wdt_fops;
> >
> > +	switch (pdev->id) {
> > +	case SECURE_WDT:
> > +		wdev->omap_wdt_miscdev.name = "watchdog_secure";
> > +		break;
> > +	case IVA2_WDT:
> > +		wdev->omap_wdt_miscdev.name = "watchdog_iva2";
> > +		break;
> > +	case MPU_WDT:
> > +	default:
> > +		wdev->omap_wdt_miscdev.name = "watchdog";
> > +	}
> > +
> 
> The more think about this, the more I don't like this pdev->id
> switching in the driver.  The only thing it is needed for
> is to set the name of the node.
> 
> Instead, why not set the name in devices.c and pass it in
> using platform_data.
> 
> Then you can drop the enum and the pdev->id switching.
> 

That does simplify things a bit. Had overlooked that way of sharing info between device and driver.
So, devices.c would have something like:

static struct platform_device omap_secure_wdt_device = {
	.name = "omap_wdt",
	.id = 1,
	.num_resources = ARRAY_SIZE(secure_wdt_resources),
	.resource = secure_wdt_resources,
	.dev = {
		.platform_data = "watchdog_secure",
	},
};

And omap_wdt.c would have in probe():

wdev->omap_wdt_miscdev.name = (char *) pdev->dev.platform_data;

Is that more like what you had in mind?

/Ulrik
> >  	ret = misc_register(&(wdev->omap_wdt_miscdev));
> >  	if (ret)
> >  		goto err_misc;
> > @@ -332,7 +366,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 +419,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
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] watchdog:OMAP3:Enable support for IVA2 and SECURE

[Kevin Hilman]

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

[...]

>> 
>> Drop this and just use the inode match.
>> 
> Was considering that, but ended up defaulting value instead of error checking later. I'll change the above.
>
>> > +	/* Find match between device node and wdt device */
>> > +	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;
>> > +		}
>> > +	}
>> 
>> You should check for a valid match here.
>> 
> The sanity check I would choose here is
>
> struct omap_wdt_dev *wdev = NULL;
> ...
> <inode matching>
> ...
> if(!wdev)
> 	return -ENODEV; 
>

Looks fine.

[...]

>> 
>> The more think about this, the more I don't like this pdev->id
>> switching in the driver.  The only thing it is needed for
>> is to set the name of the node.
>> 
>> Instead, why not set the name in devices.c and pass it in
>> using platform_data.
>> 
>> Then you can drop the enum and the pdev->id switching.
>> 
>
> That does simplify things a bit. Had overlooked that way of sharing info between device and driver.
> So, devices.c would have something like:
>
> static struct platform_device omap_secure_wdt_device = {
> 	.name = "omap_wdt",
> 	.id = 1,
> 	.num_resources = ARRAY_SIZE(secure_wdt_resources),
> 	.resource = secure_wdt_resources,
> 	.dev = {
> 		.platform_data = "watchdog_secure",
> 	},
> };
>
> And omap_wdt.c would have in probe():
>
> wdev->omap_wdt_miscdev.name = (char *) pdev->dev.platform_data;
>
> Is that more like what you had in mind?
>

Exactly.

This is typically done with a platform_data struct, and a pointer to
the struct is passed as the platform_data, but in this case since the
struct would only have one field for the name pointer, this should be
fine.

Kevin

[PATCH 2/2] watchdog:OMAP3:Enable support for IVA2 and SECURE

[Ulrik Bech Hald]

This patch adds support for IVA2 and SECURE WDTs in the omap_wdt
driver for omap34xx family. SECURE will be available as
/dev/watchdog_secure on HS/EMU devices and IVA2 will be available
as /dev/watchdog_iva2. MPU will still be available as /dev/watchdog

Tested on Zoom1 OMAP3 platform

Signed-off-by: Ulrik Bech Hald <ubh@ti.com>
Reviewed-by: Kevin Hilman <khilman@deeprootsystems.com>
---
 drivers/watchdog/omap_wdt.c |   34 ++++++++++++++++++++++++++--------
 1 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
index 3ed571a..12409fe 100644
--- 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,23 @@ 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 = NULL;
+	void __iomem *base;
+
+	/* Find match between device node and wdt device */
+	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;
+		}
+	}
+
+	if (!wdev)
+		return -ENODEV;
+
+	base = wdev->base;
 
 	if (test_and_set_bit(1, (unsigned long *)&(wdev->omap_wdt_users)))
 		return -EBUSY;
@@ -272,7 +289,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]) {
 		ret = -EBUSY;
 		goto err_busy;
 	}
@@ -321,9 +338,9 @@ static int __devinit omap_wdt_probe(struct platform_device *pdev)
 	omap_wdt_adjust_timeout(timer_margin);
 
 	wdev->omap_wdt_miscdev.parent = &pdev->dev;
-	wdev->omap_wdt_miscdev.minor = WATCHDOG_MINOR;
-	wdev->omap_wdt_miscdev.name = "watchdog";
+	wdev->omap_wdt_miscdev.minor = MISC_DYNAMIC_MINOR;
 	wdev->omap_wdt_miscdev.fops = &omap_wdt_fops;
+	wdev->omap_wdt_miscdev.name = (char *) pdev->dev.platform_data;
 
 	ret = misc_register(&(wdev->omap_wdt_miscdev));
 	if (ret)
@@ -339,7 +356,8 @@ static int __devinit omap_wdt_probe(struct platform_device *pdev)
 	clk_disable(wdev->ick);
 	clk_disable(wdev->fck);
 
-	omap_wdt_dev = pdev;
+	/* keep track of the wdt platform devices in local device array */
+	omap_wdt_dev[pdev->id] = pdev;
 
 	return 0;
 
@@ -391,7 +409,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] = NULL;
 
 	return 0;
 }
-- 
1.5.4.3