[PATCH 1/2] ARM: at91: rm9200 add system timer resources to watchdog

[PATCH 1/2] ARM: at91: rm9200 add system timer resources to watchdog

[Alexandre Belloni]

To be able to make the watchdog driver independent from the mach/ includes, pass
the system timer register space as a resource.

Also, change the name to avoid conflicting with the at91sam9 watchdog driver.

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 arch/arm/mach-at91/at91rm9200_devices.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-at91/at91rm9200_devices.c b/arch/arm/mach-at91/at91rm9200_devices.c
index 74f1eaf97801..4add0481adab 100644
--- a/arch/arm/mach-at91/at91rm9200_devices.c
+++ b/arch/arm/mach-at91/at91rm9200_devices.c
@@ -719,10 +719,19 @@ static void __init at91_add_device_rtc(void) {}
  * -------------------------------------------------------------------- */
 
 #if defined(CONFIG_AT91RM9200_WATCHDOG) || defined(CONFIG_AT91RM9200_WATCHDOG_MODULE)
+static struct resource wdt_resources[] = {
+	[0] = {
+		.start	= AT91RM9200_BASE_ST,
+		.end	= AT91RM9200_BASE_ST + SZ_256 - 1,
+		.flags	= IORESOURCE_MEM,
+	},
+};
+
 static struct platform_device at91rm9200_wdt_device = {
-	.name		= "at91_wdt",
+	.name		= "at91rm9200_wdt",
 	.id		= -1,
-	.num_resources	= 0,
+	.resource	= wdt_resources,
+	.num_resources	= 1,
 };
 
 static void __init at91_add_device_watchdog(void)
-- 
2.1.0

[PATCH 2/2] watchdog: at91rm9200 remove mach/ header dependency

[Alexandre Belloni]

Remove the mach/ header dependency by including the necessary macros and taking
the appropriate resources from the system timer.

Also allow to compile both the at91rm920 and at91sam9 watchdog drivers at the
same time.

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 drivers/watchdog/Kconfig          |  4 ++--
 drivers/watchdog/at91rm9200_wdt.c | 45 +++++++++++++++++++++++++++++++++++++--
 2 files changed, 45 insertions(+), 4 deletions(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index d0107d424ee4..0592db68f3a8 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -154,14 +154,14 @@ config ARM_SP805_WATCHDOG
 
 config AT91RM9200_WATCHDOG
 	tristate "AT91RM9200 watchdog"
-	depends on ARCH_AT91RM9200
+	depends on ARCH_AT91 || COMPILE_TEST
 	help
 	  Watchdog timer embedded into AT91RM9200 chips. This will reboot your
 	  system when the timeout is reached.
 
 config AT91SAM9X_WATCHDOG
 	tristate "AT91SAM9X / AT91CAP9 watchdog"
-	depends on ARCH_AT91 && !ARCH_AT91RM9200
+	depends on ARCH_AT91
 	select WATCHDOG_CORE
 	help
 	  Watchdog timer embedded into AT91SAM9X and AT91CAP9 chips. This will
diff --git a/drivers/watchdog/at91rm9200_wdt.c b/drivers/watchdog/at91rm9200_wdt.c
index dee6cc21d270..776cb3b313d9 100644
--- a/drivers/watchdog/at91rm9200_wdt.c
+++ b/drivers/watchdog/at91rm9200_wdt.c
@@ -26,11 +26,28 @@
 #include <linux/uaccess.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
-#include <mach/at91_st.h>
+#include <linux/of_address.h>
 
 #define WDT_DEFAULT_TIME	5	/* seconds */
 #define WDT_MAX_TIME		256	/* seconds */
 
+static void __iomem *at91_st_base;
+
+#define at91_st_read(field) \
+	readl(at91_st_base + field)
+
+#define at91_st_write(field, value) \
+	writel(value, at91_st_base + field)
+
+#define AT91_ST_CR	0x00		/* Control Register */
+#define AT91_ST_WDRST	(1 << 0)	/* Watchdog Timer Restart */
+
+#define AT91_ST_WDMR	0x08		/* Watchdog Mode Register */
+#define AT91_ST_WDV	(0xffff <<  0)	/* Watchdog Counter Value */
+#define AT91_ST_RSTEN	(1	<< 16)	/* Reset Enable */
+#define AT91_ST_EXTEN	(1	<< 17)	/* External Signal Assertion Enable */
+
+
 static int wdt_time = WDT_DEFAULT_TIME;
 static bool nowayout = WATCHDOG_NOWAYOUT;
 
@@ -201,14 +218,38 @@ static struct miscdevice at91wdt_miscdev = {
 	.fops		= &at91wdt_fops,
 };
 
+static struct of_device_id at91rm9200_st_timer_ids[] = {
+	{ .compatible = "atmel,at91rm9200-st" },
+	{ /* sentinel */ }
+};
+
 static int at91wdt_probe(struct platform_device *pdev)
 {
+	struct resource *regs;
+	struct resource _regs;
 	int res;
 
 	if (at91wdt_miscdev.parent)
 		return -EBUSY;
 	at91wdt_miscdev.parent = &pdev->dev;
 
+	if (pdev->dev.of_node) {
+		struct device_node *np;
+
+		np = of_find_matching_node(NULL, at91rm9200_st_timer_ids);
+		if (!np)
+			return -ENXIO;
+		if (of_address_to_resource(np, 0, &_regs))
+			return -ENXIO;
+		regs = &_regs;
+	} else {
+		regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	}
+
+	at91_st_base = devm_ioremap_resource(&pdev->dev, regs);
+	if (!at91_st_base)
+		return -ENXIO;
+
 	res = misc_register(&at91wdt_miscdev);
 	if (res)
 		return res;
@@ -267,7 +308,7 @@ static struct platform_driver at91wdt_driver = {
 	.suspend	= at91wdt_suspend,
 	.resume		= at91wdt_resume,
 	.driver		= {
-		.name	= "at91_wdt",
+		.name	= "at91rm9200_wdt",
 		.owner	= THIS_MODULE,
 		.of_match_table = at91_wdt_dt_ids,
 	},
-- 
2.1.0

[PATCH 1/2] ARM: at91: rm9200 add system timer resources to watchdog

[Arnd Bergmann]

On Friday 31 October 2014 21:45:58 Alexandre Belloni wrote:
> To be able to make the watchdog driver independent from the mach/ includes, pass
> the system timer register space as a resource.
> 
> Also, change the name to avoid conflicting with the at91sam9 watchdog driver.
> 
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> 

Doing this change as a separate patch breaks bisection because now the device
name no longer matches untile the other patch is applied too.

	Arnd

[PATCH 2/2] watchdog: at91rm9200 remove mach/ header dependency

[Arnd Bergmann]

On Friday 31 October 2014 21:45:59 Alexandre Belloni wrote:
> Remove the mach/ header dependency by including the necessary macros and taking
> the appropriate resources from the system timer.
> 
> Also allow to compile both the at91rm920 and at91sam9 watchdog drivers at the
> same time.
> 
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>

Please list me in the changelog as well, either in free-form or as
Suggested-by.
>  
> +static struct of_device_id at91rm9200_st_timer_ids[] = {
> +	{ .compatible = "atmel,at91rm9200-st" },
> +	{ /* sentinel */ }
> +};
> +
>  static int at91wdt_probe(struct platform_device *pdev)
>  {
> +	struct resource *regs;
> +	struct resource _regs;
>  	int res;
>  
>  	if (at91wdt_miscdev.parent)
>  		return -EBUSY;
>  	at91wdt_miscdev.parent = &pdev->dev;
>  
> +	if (pdev->dev.of_node) {
> +		struct device_node *np;
> +
> +		np = of_find_matching_node(NULL, at91rm9200_st_timer_ids);
> +		if (!np)
> +			return -ENXIO;
> +		if (of_address_to_resource(np, 0, &_regs))
> +			return -ENXIO;
> +		regs = &_regs;
> +	} else {
> +		regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	}

What was wrong with my approach of binding the driver to the
"atmel,at91rm9200-st" node?

	Arnd

[PATCH 1/2] ARM: at91: rm9200 add system timer resources to watchdog

[Alexandre Belloni]

On 31/10/2014 at 21:50:05 +0100, Arnd Bergmann wrote :
> On Friday 31 October 2014 21:45:58 Alexandre Belloni wrote:
> > To be able to make the watchdog driver independent from the mach/ includes, pass
> > the system timer register space as a resource.
> > 
> > Also, change the name to avoid conflicting with the at91sam9 watchdog driver.
> > 
> > Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> > 
> 
> Doing this change as a separate patch breaks bisection because now the device
> name no longer matches untile the other patch is applied too.
> 

Yeah, I was not sure how important that was as there is no user of the
watchdog in the kernel. My thinking was that both patch can then go
through different trees.

I can definitely squash them.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 2/2] watchdog: at91rm9200 remove mach/ header dependency

[Alexandre Belloni]

On 31/10/2014 at 21:53:03 +0100, Arnd Bergmann wrote :
> On Friday 31 October 2014 21:45:59 Alexandre Belloni wrote:
> > Remove the mach/ header dependency by including the necessary macros and taking
> > the appropriate resources from the system timer.
> > 
> > Also allow to compile both the at91rm920 and at91sam9 watchdog drivers at the
> > same time.
> > 
> > Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> 
> Please list me in the changelog as well, either in free-form or as
> Suggested-by.

Sure, I was thinking about putting your sob there too and forgot...

> >  
> > +static struct of_device_id at91rm9200_st_timer_ids[] = {
> > +	{ .compatible = "atmel,at91rm9200-st" },
> > +	{ /* sentinel */ }
> > +};
> > +
> >  static int at91wdt_probe(struct platform_device *pdev)
> >  {
> > +	struct resource *regs;
> > +	struct resource _regs;
> >  	int res;
> >  
> >  	if (at91wdt_miscdev.parent)
> >  		return -EBUSY;
> >  	at91wdt_miscdev.parent = &pdev->dev;
> >  
> > +	if (pdev->dev.of_node) {
> > +		struct device_node *np;
> > +
> > +		np = of_find_matching_node(NULL, at91rm9200_st_timer_ids);
> > +		if (!np)
> > +			return -ENXIO;
> > +		if (of_address_to_resource(np, 0, &_regs))
> > +			return -ENXIO;
> > +		regs = &_regs;
> > +	} else {
> > +		regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	}
> 
> What was wrong with my approach of binding the driver to the
> "atmel,at91rm9200-st" node?
> 

I would enable and start the watchdog on all the rm9200 based platform,
something that we probably don't want.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 1/2] ARM: at91: rm9200 add system timer resources to watchdog

[Arnd Bergmann]

On Friday 31 October 2014 21:57:56 Alexandre Belloni wrote:
> On 31/10/2014 at 21:50:05 +0100, Arnd Bergmann wrote :
> > On Friday 31 October 2014 21:45:58 Alexandre Belloni wrote:
> > > To be able to make the watchdog driver independent from the mach/ includes, pass
> > > the system timer register space as a resource.
> > > 
> > > Also, change the name to avoid conflicting with the at91sam9 watchdog driver.
> > > 
> > > Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> > > 
> > 
> > Doing this change as a separate patch breaks bisection because now the device
> > name no longer matches untile the other patch is applied too.
> > 
> 
> Yeah, I was not sure how important that was as there is no user of the
> watchdog in the kernel. My thinking was that both patch can then go
> through different trees.
> 
> I can definitely squash them.

AFAICT, arch/arm/configs/at91rm9200_defconfig enables the device and it
gets registered through at91_add_standard_devices. You definitely have
my Ack to merge the mach-at91 patch through the watchdog tree.

	Arnd

[PATCH 1/2] ARM: at91: rm9200 add system timer resources to watchdog

[Alexandre Belloni]

On 31/10/2014 at 22:36:55 +0100, Arnd Bergmann wrote :
> On Friday 31 October 2014 21:57:56 Alexandre Belloni wrote:
> > On 31/10/2014 at 21:50:05 +0100, Arnd Bergmann wrote :
> > > On Friday 31 October 2014 21:45:58 Alexandre Belloni wrote:
> > > > To be able to make the watchdog driver independent from the mach/ includes, pass
> > > > the system timer register space as a resource.
> > > > 
> > > > Also, change the name to avoid conflicting with the at91sam9 watchdog driver.
> > > > 
> > > > Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> > > > 
> > > 
> > > Doing this change as a separate patch breaks bisection because now the device
> > > name no longer matches untile the other patch is applied too.
> > > 
> > 
> > Yeah, I was not sure how important that was as there is no user of the
> > watchdog in the kernel. My thinking was that both patch can then go
> > through different trees.
> > 
> > I can definitely squash them.
> 
> AFAICT, arch/arm/configs/at91rm9200_defconfig enables the device and it
> gets registered through at91_add_standard_devices. You definitely have
> my Ack to merge the mach-at91 patch through the watchdog tree.
> 

You're right, I missed that one. I was expecting it to be called from
board files.

So, I'll squash both patches, add your SoB and your Ack and get it
merged through the watchdog tree, tell me if that is not what you
expect.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 1/2] ARM: at91: rm9200 add system timer resources to watchdog

[Arnd Bergmann]

On Tuesday 04 November 2014 23:41:26 Alexandre Belloni wrote:
> On 31/10/2014 at 22:36:55 +0100, Arnd Bergmann wrote :
> > On Friday 31 October 2014 21:57:56 Alexandre Belloni wrote:
> > > On 31/10/2014 at 21:50:05 +0100, Arnd Bergmann wrote :
> > > > On Friday 31 October 2014 21:45:58 Alexandre Belloni wrote:
> > > > > To be able to make the watchdog driver independent from the mach/ includes, pass
> > > > > the system timer register space as a resource.
> > > > > 
> > > > > Also, change the name to avoid conflicting with the at91sam9 watchdog driver.
> > > > > 
> > > > > Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> > > > > 
> > > > 
> > > > Doing this change as a separate patch breaks bisection because now the device
> > > > name no longer matches untile the other patch is applied too.
> > > > 
> > > 
> > > Yeah, I was not sure how important that was as there is no user of the
> > > watchdog in the kernel. My thinking was that both patch can then go
> > > through different trees.
> > > 
> > > I can definitely squash them.
> > 
> > AFAICT, arch/arm/configs/at91rm9200_defconfig enables the device and it
> > gets registered through at91_add_standard_devices. You definitely have
> > my Ack to merge the mach-at91 patch through the watchdog tree.
> > 
> 
> You're right, I missed that one. I was expecting it to be called from
> board files.
> 
> So, I'll squash both patches, add your SoB and your Ack and get it
> merged through the watchdog tree, tell me if that is not what you
> expect.

I definitely *don't* expect you to add my Signed-off-by, that would
be against the procedures we have in Documentation/SubmittingPatches.

Other than that, it sounds good, thanks!

	Arnd