[PATCH v2 0/3] tegra30 watchdog support

[PATCH v2 0/3] tegra30 watchdog support

[Andrew Chew]

This patch series ultimately adds watchdog support for tegra30 and later
chips.

The existing tegra clocksource driver (drivers/clocksource/tegra20_timer.c)
sadly does not distinguish between tegra20 and tegra30 (and later), which
it should have done since the contents of the timer register base have
changed significantly.  In particular, tegra30 (and later) has more timers,
and also hardware watchdog registers.

The first patch adds nvidia,tegra30-timer to the list of compatibilty
strings for the tegra timer device tree node, so that we can distinguish
between tegra20 and tegra30 (and later).

The second patch separates out some macros that are interesting to other
drivers (in particular, the tegra watchdog driver), and also adds the
the missing timers that are present in tegra30 and later.

The third patch adds the actual watchdog driver.  This driver configures
a single watchdog (watchdog 0), pairs it with timer 5
(defined as TEGRA30_TIMER_WDT_* in the shared header file from the previous
patch), and sets it up so that upon timer expiration, will cause the target
system to reset.

I've decided to encapsulate all related changes into one patch series, since
I did not modify any device tree bindings and therefore don't need to review
dt changes separately.  This way, everything can be seen within its complete
context.

Andrew Chew (3):
  clocksource: tegra: Add nvidia,tegra30-timer compat
  clocksource: tegra: Define timer bases in header file
  watchdog: Add tegra watchdog

 Documentation/watchdog/watchdog-parameters.txt |   5 +
 drivers/clocksource/tegra20_timer.c            |  16 +-
 drivers/watchdog/Kconfig                       |  11 +
 drivers/watchdog/Makefile                      |   1 +
 drivers/watchdog/tegra_wdt.c                   | 372 +++++++++++++++++++++++++
 include/clocksource/tegra_timer.h              |  43 +++
 6 files changed, 439 insertions(+), 9 deletions(-)
 create mode 100644 drivers/watchdog/tegra_wdt.c
 create mode 100644 include/clocksource/tegra_timer.h

-- 
1.8.1.5

[PATCH v2 1/3] clocksource: tegra: Add nvidia,tegra30-timer compat

[Andrew Chew]

There are some differences between tegra20's timer registers and tegra30's
(and later).  For example, tegra30 has more timers.  In addition, watchdogs
are not present in tegra20.

Add this compatibility string in order to be able to distinguish
whether the additional timers and watchdogs are there or not.

Signed-off-by: Andrew Chew <achew@nvidia.com>
Acked-by: Stephen Warren <swarren@nvidia.com>
---
 drivers/clocksource/tegra20_timer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clocksource/tegra20_timer.c b/drivers/clocksource/tegra20_timer.c
index d1869f0..73cfa56 100644
--- a/drivers/clocksource/tegra20_timer.c
+++ b/drivers/clocksource/tegra20_timer.c
@@ -218,6 +218,7 @@ static void __init tegra20_init_timer(struct device_node *np)
 					0x1, 0x1fffffff);
 }
 CLOCKSOURCE_OF_DECLARE(tegra20_timer, "nvidia,tegra20-timer", tegra20_init_timer);
+CLOCKSOURCE_OF_DECLARE(tegra30_timer, "nvidia,tegra30-timer", tegra20_init_timer);
 
 static void __init tegra20_init_rtc(struct device_node *np)
 {
-- 
1.8.1.5

Re: [PATCH v2 1/3] clocksource: tegra: Add nvidia,tegra30-timer compat

[Stephen Warren]

On 02/03/2014 05:17 PM, Andrew Chew wrote:
> There are some differences between tegra20's timer registers and tegra30's
> (and later).  For example, tegra30 has more timers.  In addition, watchdogs
> are not present in tegra20.
> 
> Add this compatibility string in order to be able to distinguish
> whether the additional timers and watchdogs are there or not.

> diff --git a/drivers/clocksource/tegra20_timer.c b/drivers/clocksource/tegra20_timer.c

>  CLOCKSOURCE_OF_DECLARE(tegra20_timer, "nvidia,tegra20-timer", tegra20_init_timer);
> +CLOCKSOURCE_OF_DECLARE(tegra30_timer, "nvidia,tegra30-timer", tegra20_init_timer);

Thinking about this more, nothing in this driver actually cares about
Tegra20 vs. Tegra30+, since the timer that's used is present in all
chips. Hence, this patch isn't needed.

RE: [PATCH v2 1/3] clocksource: tegra: Add nvidia,tegra30-timer compat

[Andrew Chew]

> On 02/03/2014 05:17 PM, Andrew Chew wrote:
> > There are some differences between tegra20's timer registers and
> > tegra30's (and later).  For example, tegra30 has more timers.  In
> > addition, watchdogs are not present in tegra20.
> >
> > Add this compatibility string in order to be able to distinguish
> > whether the additional timers and watchdogs are there or not.
> 
> > diff --git a/drivers/clocksource/tegra20_timer.c
> > b/drivers/clocksource/tegra20_timer.c
> 
> >  CLOCKSOURCE_OF_DECLARE(tegra20_timer, "nvidia,tegra20-timer",
> > tegra20_init_timer);
> > +CLOCKSOURCE_OF_DECLARE(tegra30_timer, "nvidia,tegra30-timer",
> > +tegra20_init_timer);
> 
> Thinking about this more, nothing in this driver actually cares about
> Tegra20 vs. Tegra30+, since the timer that's used is present in all chips.
> Hence, this patch isn't needed.

Don't I need to add nvidia,tegra30-timer so that the tegra WDT driver
can match against it?  It would be weird to have the tegra WDT driver
bind against nvidia,tegra20-timer when the tegra WDT driver as it is won't
work at all on tegra20.

Re: [PATCH v2 1/3] clocksource: tegra: Add nvidia,tegra30-timer compat

[Stephen Warren]

On 02/05/2014 01:06 PM, Andrew Chew wrote:
>> On 02/03/2014 05:17 PM, Andrew Chew wrote:
>>> There are some differences between tegra20's timer registers and
>>> tegra30's (and later).  For example, tegra30 has more timers.  In
>>> addition, watchdogs are not present in tegra20.
>>>
>>> Add this compatibility string in order to be able to distinguish
>>> whether the additional timers and watchdogs are there or not.
>>
>>> diff --git a/drivers/clocksource/tegra20_timer.c
>>> b/drivers/clocksource/tegra20_timer.c
>>
>>>  CLOCKSOURCE_OF_DECLARE(tegra20_timer, "nvidia,tegra20-timer",
>>> tegra20_init_timer);
>>> +CLOCKSOURCE_OF_DECLARE(tegra30_timer, "nvidia,tegra30-timer",
>>> +tegra20_init_timer);
>>
>> Thinking about this more, nothing in this driver actually cares about
>> Tegra20 vs. Tegra30+, since the timer that's used is present in all chips.
>> Hence, this patch isn't needed.
> 
> Don't I need to add nvidia,tegra30-timer so that the tegra WDT driver
> can match against it?  It would be weird to have the tegra WDT driver
> bind against nvidia,tegra20-timer when the tegra WDT driver as it is won't
> work at all on tegra20.

The DT files need to contain all of:

* The specific SoC (this is already present)
* nvidia,tegra30-timer for SoCs >= Tegra30 (this is missing)
* nvidia,tegra20-timer for all SoCs (this is already present)

However, since all DTs will contain nvidia,tegra20-timer, since all HW
is backwards-compatible IIUC, any code that only cares about the parts
that have existed since Tegra20 only need match against the Tegra20
compatible value.

RE: [PATCH v2 1/3] clocksource: tegra: Add nvidia,tegra30-timer compat

[Andrew Chew]

> On 02/05/2014 01:06 PM, Andrew Chew wrote:
> >> On 02/03/2014 05:17 PM, Andrew Chew wrote:
> >>> There are some differences between tegra20's timer registers and
> >>> tegra30's (and later).  For example, tegra30 has more timers.  In
> >>> addition, watchdogs are not present in tegra20.
> >>>
> >>> Add this compatibility string in order to be able to distinguish
> >>> whether the additional timers and watchdogs are there or not.
> >>
> >>> diff --git a/drivers/clocksource/tegra20_timer.c
> >>> b/drivers/clocksource/tegra20_timer.c
> >>
> >>>  CLOCKSOURCE_OF_DECLARE(tegra20_timer, "nvidia,tegra20-timer",
> >>> tegra20_init_timer);
> >>> +CLOCKSOURCE_OF_DECLARE(tegra30_timer, "nvidia,tegra30-timer",
> >>> +tegra20_init_timer);
> >>
> >> Thinking about this more, nothing in this driver actually cares about
> >> Tegra20 vs. Tegra30+, since the timer that's used is present in all chips.
> >> Hence, this patch isn't needed.
> >
> > Don't I need to add nvidia,tegra30-timer so that the tegra WDT driver
> > can match against it?  It would be weird to have the tegra WDT driver
> > bind against nvidia,tegra20-timer when the tegra WDT driver as it is
> > won't work at all on tegra20.
> 
> The DT files need to contain all of:
> 
> * The specific SoC (this is already present)
> * nvidia,tegra30-timer for SoCs >= Tegra30 (this is missing)
> * nvidia,tegra20-timer for all SoCs (this is already present)
> 
> However, since all DTs will contain nvidia,tegra20-timer, since all HW is
> backwards-compatible IIUC, any code that only cares about the parts that
> have existed since Tegra20 only need match against the Tegra20 compatible
> value.

Okay, I think I get what you're saying.  So I'll drop this patch.  The tegra WDT
driver will still match against nvidia,tegra30-timer, and device trees for SoCs
that expect to use this WDT driver will have to have nvidia,tegra30-timer.

[PATCH v2 2/3] clocksource: tegra: Define timer bases in header file

[Andrew Chew]

Added timers that are present in tegra30 and later, that are NOT in tegra20.

Also, some of these timer bases are needed in the tegra watchdog driver, so
separate them out into a header file that both the clocksource driver and
the watchdog driver can share them.

Signed-off-by: Andrew Chew <achew@nvidia.com>
---
 drivers/clocksource/tegra20_timer.c | 15 ++++++-------
 include/clocksource/tegra_timer.h   | 43 +++++++++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+), 9 deletions(-)
 create mode 100644 include/clocksource/tegra_timer.h

diff --git a/drivers/clocksource/tegra20_timer.c b/drivers/clocksource/tegra20_timer.c
index 73cfa56..2c49643 100644
--- a/drivers/clocksource/tegra20_timer.c
+++ b/drivers/clocksource/tegra20_timer.c
@@ -28,6 +28,8 @@
 #include <linux/of_irq.h>
 #include <linux/sched_clock.h>
 
+#include <clocksource/tegra_timer.h>
+
 #include <asm/mach/time.h>
 #include <asm/smp_twd.h>
 
@@ -39,11 +41,6 @@
 #define TIMERUS_USEC_CFG 0x14
 #define TIMERUS_CNTR_FREEZE 0x4c
 
-#define TIMER1_BASE 0x0
-#define TIMER2_BASE 0x8
-#define TIMER3_BASE 0x50
-#define TIMER4_BASE 0x58
-
 #define TIMER_PTV 0x0
 #define TIMER_PCR 0x4
 
@@ -64,7 +61,7 @@ static int tegra_timer_set_next_event(unsigned long cycles,
 	u32 reg;
 
 	reg = 0x80000000 | ((cycles > 1) ? (cycles-1) : 0);
-	timer_writel(reg, TIMER3_BASE + TIMER_PTV);
+	timer_writel(reg, TEGRA20_TIMER3_BASE + TIMER_PTV);
 
 	return 0;
 }
@@ -74,12 +71,12 @@ static void tegra_timer_set_mode(enum clock_event_mode mode,
 {
 	u32 reg;
 
-	timer_writel(0, TIMER3_BASE + TIMER_PTV);
+	timer_writel(0, TEGRA20_TIMER3_BASE + TIMER_PTV);
 
 	switch (mode) {
 	case CLOCK_EVT_MODE_PERIODIC:
 		reg = 0xC0000000 | ((1000000/HZ)-1);
-		timer_writel(reg, TIMER3_BASE + TIMER_PTV);
+		timer_writel(reg, TEGRA20_TIMER3_BASE + TIMER_PTV);
 		break;
 	case CLOCK_EVT_MODE_ONESHOT:
 		break;
@@ -142,7 +139,7 @@ static void tegra_read_persistent_clock(struct timespec *ts)
 static irqreturn_t tegra_timer_interrupt(int irq, void *dev_id)
 {
 	struct clock_event_device *evt = (struct clock_event_device *)dev_id;
-	timer_writel(1<<30, TIMER3_BASE + TIMER_PCR);
+	timer_writel(1<<30, TEGRA20_TIMER3_BASE + TIMER_PCR);
 	evt->event_handler(evt);
 	return IRQ_HANDLED;
 }
diff --git a/include/clocksource/tegra_timer.h b/include/clocksource/tegra_timer.h
new file mode 100644
index 0000000..ea0bc8b
--- /dev/null
+++ b/include/clocksource/tegra_timer.h
@@ -0,0 +1,43 @@
+/*
+ * Copyright (C) 2010 Google, Inc.
+ *
+ * Author:
+ *	Colin Cross <ccross@google.com>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#ifndef __CLOCKSOURCE_TEGRA_TIMER_H
+#define __CLOCKSOURCE_TEGRA_TIMER_H
+
+/* Tegra 20 timers */
+#define TEGRA20_TIMER1_BASE	0x0
+#define TEGRA20_TIMER2_BASE	0x8
+#define TEGRA20_TIMER3_BASE	0x50
+#define TEGRA20_TIMER4_BASE	0x58
+
+/* Tegra 30 timers */
+#define TEGRA30_TIMER1_BASE	TEGRA20_TIMER1_BASE
+#define TEGRA30_TIMER2_BASE	TEGRA20_TIMER2_BASE
+#define TEGRA30_TIMER3_BASE	TEGRA20_TIMER3_BASE
+#define TEGRA30_TIMER4_BASE	TEGRA20_TIMER4_BASE
+#define TEGRA30_TIMER5_BASE	0x60
+#define TEGRA30_TIMER6_BASE	0x68
+#define TEGRA30_TIMER7_BASE	0x70
+#define TEGRA30_TIMER8_BASE	0x78
+#define TEGRA30_TIMER9_BASE	0x80
+#define TEGRA30_TIMER0_BASE	0x88
+
+/* Used by the tegra watchdog timer */
+#define TEGRA30_TIMER_WDT_BASE	TEGRA30_TIMER5_BASE
+#define TEGRA30_TIMER_WDT_ID	5
+
+#endif /* __CLOCKSOURCE_TEGRA_TIMER_H */
-- 
1.8.1.5

Re: [PATCH v2 2/3] clocksource: tegra: Define timer bases in header file

[Daniel Lezcano]

On 02/04/2014 01:17 AM, Andrew Chew wrote:
> Added timers that are present in tegra30 and later, that are NOT in tegra20.
>
> Also, some of these timer bases are needed in the tegra watchdog driver, so
> separate them out into a header file that both the clocksource driver and
> the watchdog driver can share them.
>
> Signed-off-by: Andrew Chew <achew@nvidia.com>

When reading the patch 3/3, I don't see any define reused from this 
header except TEGRA30_TIMER_WDT_BASE which is only used for the 
watchdog. May be I missed something but I don't see any definition 
shared and thus I don't see the point of creating this header file.

> ---
>   drivers/clocksource/tegra20_timer.c | 15 ++++++-------
>   include/clocksource/tegra_timer.h   | 43 +++++++++++++++++++++++++++++++++++++
>   2 files changed, 49 insertions(+), 9 deletions(-)
>   create mode 100644 include/clocksource/tegra_timer.h
>
> diff --git a/drivers/clocksource/tegra20_timer.c b/drivers/clocksource/tegra20_timer.c
> index 73cfa56..2c49643 100644
> --- a/drivers/clocksource/tegra20_timer.c
> +++ b/drivers/clocksource/tegra20_timer.c
> @@ -28,6 +28,8 @@
>   #include <linux/of_irq.h>
>   #include <linux/sched_clock.h>
>
> +#include <clocksource/tegra_timer.h>
> +
>   #include <asm/mach/time.h>
>   #include <asm/smp_twd.h>
>
> @@ -39,11 +41,6 @@
>   #define TIMERUS_USEC_CFG 0x14
>   #define TIMERUS_CNTR_FREEZE 0x4c
>
> -#define TIMER1_BASE 0x0
> -#define TIMER2_BASE 0x8
> -#define TIMER3_BASE 0x50
> -#define TIMER4_BASE 0x58
> -
>   #define TIMER_PTV 0x0
>   #define TIMER_PCR 0x4
>
> @@ -64,7 +61,7 @@ static int tegra_timer_set_next_event(unsigned long cycles,
>   	u32 reg;
>
>   	reg = 0x80000000 | ((cycles > 1) ? (cycles-1) : 0);
> -	timer_writel(reg, TIMER3_BASE + TIMER_PTV);
> +	timer_writel(reg, TEGRA20_TIMER3_BASE + TIMER_PTV);
>
>   	return 0;
>   }
> @@ -74,12 +71,12 @@ static void tegra_timer_set_mode(enum clock_event_mode mode,
>   {
>   	u32 reg;
>
> -	timer_writel(0, TIMER3_BASE + TIMER_PTV);
> +	timer_writel(0, TEGRA20_TIMER3_BASE + TIMER_PTV);
>
>   	switch (mode) {
>   	case CLOCK_EVT_MODE_PERIODIC:
>   		reg = 0xC0000000 | ((1000000/HZ)-1);
> -		timer_writel(reg, TIMER3_BASE + TIMER_PTV);
> +		timer_writel(reg, TEGRA20_TIMER3_BASE + TIMER_PTV);
>   		break;
>   	case CLOCK_EVT_MODE_ONESHOT:
>   		break;
> @@ -142,7 +139,7 @@ static void tegra_read_persistent_clock(struct timespec *ts)
>   static irqreturn_t tegra_timer_interrupt(int irq, void *dev_id)
>   {
>   	struct clock_event_device *evt = (struct clock_event_device *)dev_id;
> -	timer_writel(1<<30, TIMER3_BASE + TIMER_PCR);
> +	timer_writel(1<<30, TEGRA20_TIMER3_BASE + TIMER_PCR);
>   	evt->event_handler(evt);
>   	return IRQ_HANDLED;
>   }
> diff --git a/include/clocksource/tegra_timer.h b/include/clocksource/tegra_timer.h
> new file mode 100644
> index 0000000..ea0bc8b
> --- /dev/null
> +++ b/include/clocksource/tegra_timer.h
> @@ -0,0 +1,43 @@
> +/*
> + * Copyright (C) 2010 Google, Inc.
> + *
> + * Author:
> + *	Colin Cross <ccross@google.com>

          ^^^^^^^^^^^^^^^^^^
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#ifndef __CLOCKSOURCE_TEGRA_TIMER_H
> +#define __CLOCKSOURCE_TEGRA_TIMER_H
> +
> +/* Tegra 20 timers */
> +#define TEGRA20_TIMER1_BASE	0x0
> +#define TEGRA20_TIMER2_BASE	0x8
> +#define TEGRA20_TIMER3_BASE	0x50
> +#define TEGRA20_TIMER4_BASE	0x58
> +
> +/* Tegra 30 timers */
> +#define TEGRA30_TIMER1_BASE	TEGRA20_TIMER1_BASE
> +#define TEGRA30_TIMER2_BASE	TEGRA20_TIMER2_BASE
> +#define TEGRA30_TIMER3_BASE	TEGRA20_TIMER3_BASE
> +#define TEGRA30_TIMER4_BASE	TEGRA20_TIMER4_BASE
> +#define TEGRA30_TIMER5_BASE	0x60
> +#define TEGRA30_TIMER6_BASE	0x68
> +#define TEGRA30_TIMER7_BASE	0x70
> +#define TEGRA30_TIMER8_BASE	0x78
> +#define TEGRA30_TIMER9_BASE	0x80
> +#define TEGRA30_TIMER0_BASE	0x88
> +
> +/* Used by the tegra watchdog timer */
> +#define TEGRA30_TIMER_WDT_BASE	TEGRA30_TIMER5_BASE
> +#define TEGRA30_TIMER_WDT_ID	5
> +
> +#endif /* __CLOCKSOURCE_TEGRA_TIMER_H */
>


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

RE: [PATCH v2 2/3] clocksource: tegra: Define timer bases in header file

[Andrew Chew]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2543 bytes --]

> From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org]
> Sent: Monday, February 03, 2014 11:55 PM
> To: Andrew Chew; tglx@linutronix.de; swarren@wwwdotorg.org;
> thierry.reding@gmail.com; rob@landley.net; grant.likely@linaro.org;
> robh+dt@kernel.org; abrestic@chromium.org; dgreid@chromium.org;
> katierh@chromium.org
> Cc: linux-kernel@vger.kernel.org; linux-tegra@vger.kernel.org; linux-
> watchdog@vger.kernel.org; linux-doc@vger.kernel.org
> Subject: Re: [PATCH v2 2/3] clocksource: tegra: Define timer bases in header
> file
> 
> On 02/04/2014 01:17 AM, Andrew Chew wrote:
> > Added timers that are present in tegra30 and later, that are NOT in tegra20.
> >
> > Also, some of these timer bases are needed in the tegra watchdog
> > driver, so separate them out into a header file that both the
> > clocksource driver and the watchdog driver can share them.
> >
> > Signed-off-by: Andrew Chew <achew@nvidia.com>
> 
> When reading the patch 3/3, I don't see any define reused from this header
> except TEGRA30_TIMER_WDT_BASE which is only used for the watchdog.
> May be I missed something but I don't see any definition shared and thus I
> don't see the point of creating this header file.

I guess the point is to make other potential drivers that make use of the timer
registers aware that this particular timer (timer 5) is provisioned for the watchdog
driver.  Right now, you're right, there really isn't much that's shared...the only
thing really is the base address of timer 5 (and its associated ID)...the timer bases
are kind of all over the place, so it's not straightforward to calculate the ID from the
base address.

But I believe the thought at the time (from Stephen Warren's suggestion) is to have
a central place where this stuff is defined so that it's more clear what timers are
provisioned for what purposes.  It just happens that today, the only users of
the timers, other than clocksource, is watchdog, as far as I can tell.

> > +++ b/include/clocksource/tegra_timer.h
> > @@ -0,0 +1,43 @@
> > +/*
> > + * Copyright (C) 2010 Google, Inc.
> > + *
> > + * Author:
> > + *	Colin Cross <ccross@google.com>

The contents of this new header file are largely just a cut and paste of the corresponding
snippet in tegra20-timer.c, so I thought I'd carry over the license verbatim.  I didn't author
this really...I just moved it around.  What should I do instead?
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH v2 2/3] clocksource: tegra: Define timer bases in header file

[Stephen Warren]

On 02/03/2014 05:17 PM, Andrew Chew wrote:
> Added timers that are present in tegra30 and later, that are NOT in tegra20.
> 
> Also, some of these timer bases are needed in the tegra watchdog driver, so
> separate them out into a header file that both the clocksource driver and
> the watchdog driver can share them.

> diff --git a/include/clocksource/tegra_timer.h b/include/clocksource/tegra_timer.h

> +/* Tegra 20 timers */
> +#define TEGRA20_TIMER1_BASE	0x0
> +#define TEGRA20_TIMER2_BASE	0x8
> +#define TEGRA20_TIMER3_BASE	0x50
> +#define TEGRA20_TIMER4_BASE	0x58
> +
> +/* Tegra 30 timers */
> +#define TEGRA30_TIMER1_BASE	TEGRA20_TIMER1_BASE
> +#define TEGRA30_TIMER2_BASE	TEGRA20_TIMER2_BASE
> +#define TEGRA30_TIMER3_BASE	TEGRA20_TIMER3_BASE
> +#define TEGRA30_TIMER4_BASE	TEGRA20_TIMER4_BASE
> +#define TEGRA30_TIMER5_BASE	0x60
> +#define TEGRA30_TIMER6_BASE	0x68
> +#define TEGRA30_TIMER7_BASE	0x70
> +#define TEGRA30_TIMER8_BASE	0x78
> +#define TEGRA30_TIMER9_BASE	0x80
> +#define TEGRA30_TIMER0_BASE	0x88

Why put the SoC name in the define names? Why not just have
TIMER1_BASE..TIMER10_BASE (that should be 10 not 0 as in your patch,
right?) and have the driver know that 1..4 are valid on Tegra20, and
1..10 are valid on later chips.

I guess if the defines are moved into a header file, adding a TEGRA_
prefix does make sense.

But I wonder if it wouldn't be simpler for the Tegra WDT driver to just
call a function on the Tegra clocksource driver to find out which timer
ID(s) to avoid using? Even simpler would be to just put a comment in the
WDT driver saying that timer 5 was chosen arbitrarily, but if it's
changed make sure not to conflict with the clocksource driver (and an
equivalent change to the clocksource driver).

RE: [PATCH v2 2/3] clocksource: tegra: Define timer bases in header file

[Andrew Chew]

> > +/* Tegra 20 timers */
> > +#define TEGRA20_TIMER1_BASE	0x0
> > +#define TEGRA20_TIMER2_BASE	0x8
> > +#define TEGRA20_TIMER3_BASE	0x50
> > +#define TEGRA20_TIMER4_BASE	0x58
> > +
> > +/* Tegra 30 timers */
> > +#define TEGRA30_TIMER1_BASE	TEGRA20_TIMER1_BASE
> > +#define TEGRA30_TIMER2_BASE	TEGRA20_TIMER2_BASE
> > +#define TEGRA30_TIMER3_BASE	TEGRA20_TIMER3_BASE
> > +#define TEGRA30_TIMER4_BASE	TEGRA20_TIMER4_BASE
> > +#define TEGRA30_TIMER5_BASE	0x60
> > +#define TEGRA30_TIMER6_BASE	0x68
> > +#define TEGRA30_TIMER7_BASE	0x70
> > +#define TEGRA30_TIMER8_BASE	0x78
> > +#define TEGRA30_TIMER9_BASE	0x80
> > +#define TEGRA30_TIMER0_BASE	0x88
> 
> Why put the SoC name in the define names? Why not just have
> TIMER1_BASE..TIMER10_BASE (that should be 10 not 0 as in your patch,
> right?) and have the driver know that 1..4 are valid on Tegra20, and
> 1..10 are valid on later chips.
> 
> I guess if the defines are moved into a header file, adding a TEGRA_ prefix
> does make sense.
> 
> But I wonder if it wouldn't be simpler for the Tegra WDT driver to just call a
> function on the Tegra clocksource driver to find out which timer
> ID(s) to avoid using? Even simpler would be to just put a comment in the
> WDT driver saying that timer 5 was chosen arbitrarily, but if it's changed make
> sure not to conflict with the clocksource driver (and an equivalent change to
> the clocksource driver).

Alright, I think I'll just go with putting a comment in the WDT driver then, so that
We don't need to add this new header file.

[PATCH v2 3/3] watchdog: Add tegra watchdog

[Andrew Chew]

Add a driver for the hardware watchdogs in NVIDIA Tegra SoCs (tegra30 and
later).  This driver will configure one watchdog timer that will reset the
system in the case of a watchdog timeout.

This driver binds to the nvidia,tegra30-timer device node and gets its
register base from there.

Signed-off-by: Andrew Chew <achew@nvidia.com>
---
 Documentation/watchdog/watchdog-parameters.txt |   5 +
 drivers/watchdog/Kconfig                       |  11 +
 drivers/watchdog/Makefile                      |   1 +
 drivers/watchdog/tegra_wdt.c                   | 372 +++++++++++++++++++++++++
 4 files changed, 389 insertions(+)
 create mode 100644 drivers/watchdog/tegra_wdt.c

diff --git a/Documentation/watchdog/watchdog-parameters.txt b/Documentation/watchdog/watchdog-parameters.txt
index f9492fe..b39f355 100644
--- a/Documentation/watchdog/watchdog-parameters.txt
+++ b/Documentation/watchdog/watchdog-parameters.txt
@@ -325,6 +325,11 @@ soft_noboot: Softdog action, set to 1 to ignore reboots, 0 to reboot
 stmp3xxx_wdt:
 heartbeat: Watchdog heartbeat period in seconds from 1 to 4194304, default 19
 -------------------------------------------------
+tegra_wdt:
+heartbeat: Watchdog heartbeats in seconds. (default = 120)
+nowayout: Watchdog cannot be stopped once started
+	(default=kernel config parameter)
+-------------------------------------------------
 ts72xx_wdt:
 timeout: Watchdog timeout in seconds. (1 <= timeout <= 8, default=8)
 nowayout: Disable watchdog shutdown on close
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 4c4c566..2852447 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -420,6 +420,17 @@ config SIRFSOC_WATCHDOG
 	  Support for CSR SiRFprimaII and SiRFatlasVI watchdog. When
 	  the watchdog triggers the system will be reset.
 
+config TEGRA_WATCHDOG
+	tristate "Tegra watchdog"
+	depends on ARCH_TEGRA
+	select WATCHDOG_CORE
+	help
+	  Say Y here to include support for the watchdog timer
+	  embedded in NVIDIA Tegra SoCs.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called tegra_wdt.
+
 # AVR32 Architecture
 
 config AT32AP700X_WDT
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 985a66c..1b5f3d5 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -58,6 +58,7 @@ obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o
 obj-$(CONFIG_MOXART_WDT) += moxart_wdt.o
 obj-$(CONFIG_SIRFSOC_WATCHDOG) += sirfsoc_wdt.o
 obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o
+obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o
 
 # AVR32 Architecture
 obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
diff --git a/drivers/watchdog/tegra_wdt.c b/drivers/watchdog/tegra_wdt.c
new file mode 100644
index 0000000..eebe7fc
--- /dev/null
+++ b/drivers/watchdog/tegra_wdt.c
@@ -0,0 +1,372 @@
+/*
+ * Copyright (c) 2013, NVIDIA CORPORATION.  All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/miscdevice.h>
+#include <linux/watchdog.h>
+
+#include <clocksource/tegra_timer.h>
+
+/* minimum and maximum watchdog trigger timeout, in seconds */
+#define MIN_WDT_TIMEOUT			1
+#define MAX_WDT_TIMEOUT			255
+
+/*
+ * Base of the WDT registers, from the timer base address.  There are
+ * actually 5 watchdogs that can be configured (by pairing with an available
+ * timer), at bases 0x100 + (WDT ID) * 0x20, where WDT ID is 0 through 4.
+ * This driver only configures the first watchdog (WDT ID 0).
+ */
+#define WDT_BASE			0x100
+#define WDT_ID				0
+#define WDT_TIMER_BASE			TEGRA30_TIMER_WDT_BASE
+#define WDT_TIMER_ID			TEGRA30_TIMER_WDT_ID
+
+/* WDT registers */
+#define WDT_CFG				0x0
+#define WDT_CFG_PERIOD_SHIFT		4
+#define WDT_CFG_PERIOD_MASK		0xff
+#define WDT_CFG_INT_EN			(1 << 12)
+#define WDT_CFG_PMC2CAR_RST_EN		(1 << 15)
+#define WDT_STS				0x4
+#define WDT_STS_COUNT_SHIFT		4
+#define WDT_STS_COUNT_MASK		0xff
+#define WDT_STS_EXP_SHIFT		12
+#define WDT_STS_EXP_MASK		0x3
+#define WDT_CMD				0x8
+#define WDT_CMD_START_COUNTER		(1 << 0)
+#define WDT_CMD_DISABLE_COUNTER		(1 << 1)
+#define WDT_UNLOCK			(0xc)
+#define WDT_UNLOCK_PATTERN		(0xc45a << 0)
+
+/* Timer registers */
+#define TIMER_PTV			0x0
+#define TIMER_EN			(1 << 31)
+#define TIMER_PERIODIC			(1 << 30)
+
+struct tegra_wdt {
+	struct watchdog_device	wdd;
+	struct kref		kref;
+	void __iomem		*wdt_regs;
+	void __iomem		*tmr_regs;
+};
+
+#define WDT_HEARTBEAT 120
+static int heartbeat = WDT_HEARTBEAT;
+module_param(heartbeat, int, 0);
+MODULE_PARM_DESC(heartbeat, "Watchdog heartbeats in seconds. "
+	"(default = " __MODULE_STRING(WDT_HEARTBEAT) ")");
+
+static bool nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, bool, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
+	"(default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+static void tegra_wdt_release_resources(struct kref *r)
+{
+}
+
+static int tegra_wdt_start(struct watchdog_device *wdd)
+{
+	struct tegra_wdt *wdt = watchdog_get_drvdata(wdd);
+	u32 val;
+
+	/* This thing has a fixed 1MHz clock.  Normally, we would set the
+	 * period to 1 second by writing 1000000ul, but the watchdog system
+	 * reset actually occurs on the 4th expiration of this counter,
+	 * so we set the period to 1/4 of this amount.
+	 */
+	val = 1000000ul / 4;
+	val |= (TIMER_EN | TIMER_PERIODIC);
+	writel(val, wdt->tmr_regs + TIMER_PTV);
+
+	/*
+	 * Set number of periods and start counter.
+	 *
+	 * Interrupt handler is not required for user space
+	 * WDT accesses, since the caller is responsible to ping the
+	 * WDT to reset the counter before expiration, through ioctls.
+	 */
+	val = WDT_TIMER_ID |
+	      (wdd->timeout << WDT_CFG_PERIOD_SHIFT) |
+	      WDT_CFG_PMC2CAR_RST_EN;
+	writel(val, wdt->wdt_regs + WDT_CFG);
+
+	writel(WDT_CMD_START_COUNTER, wdt->wdt_regs + WDT_CMD);
+
+	return 0;
+}
+
+static int tegra_wdt_stop(struct watchdog_device *wdd)
+{
+	struct tegra_wdt *wdt = watchdog_get_drvdata(wdd);
+
+	writel(WDT_UNLOCK_PATTERN, wdt->wdt_regs + WDT_UNLOCK);
+	writel(WDT_CMD_DISABLE_COUNTER, wdt->wdt_regs + WDT_CMD);
+	writel(0, wdt->tmr_regs + TIMER_PTV);
+
+	return 0;
+}
+
+static int tegra_wdt_ping(struct watchdog_device *wdd)
+{
+	struct tegra_wdt *wdt = watchdog_get_drvdata(wdd);
+
+	writel(WDT_CMD_START_COUNTER, wdt->wdt_regs + WDT_CMD);
+
+	return 0;
+}
+
+static int tegra_wdt_set_timeout(struct watchdog_device *wdd,
+				 unsigned int timeout)
+{
+	int ret;
+
+	ret = tegra_wdt_stop(wdd);
+	if (ret)
+		return ret;
+
+	wdd->timeout = clamp_t(int, timeout, MIN_WDT_TIMEOUT, MAX_WDT_TIMEOUT);
+
+	if (watchdog_active(wdd))
+		return tegra_wdt_start(wdd);
+
+	return 0;
+}
+
+static unsigned int tegra_wdt_get_timeleft(struct watchdog_device *wdd)
+{
+	struct tegra_wdt *wdt = watchdog_get_drvdata(wdd);
+	u32 val;
+	int count;
+	int exp;
+
+	val = readl(wdt->wdt_regs + WDT_STS);
+
+	/* Current countdown (from timeout) */
+	count = (val >> WDT_STS_COUNT_SHIFT) & WDT_STS_COUNT_MASK;
+
+	/* Number of expirations (we are waiting for the 4th expiration) */
+	exp = (val >> WDT_STS_EXP_SHIFT) & WDT_STS_EXP_MASK;
+
+	/*
+	 * The entire thing is divided by 4 because we are ticking down 4 times
+	 * faster due to needing to wait for the 4th expiration.
+	 */
+	return (((3 - exp) * wdd->timeout) + count) / 4;
+}
+
+static void tegra_wdt_ref(struct watchdog_device *wdd)
+{
+	struct tegra_wdt *wdt = watchdog_get_drvdata(wdd);
+
+	kref_get(&wdt->kref);
+}
+
+static void tegra_wdt_unref(struct watchdog_device *wdd)
+{
+	struct tegra_wdt *wdt = watchdog_get_drvdata(wdd);
+
+	kref_put(&wdt->kref, tegra_wdt_release_resources);
+}
+
+static const struct watchdog_info tegra_wdt_info = {
+	.options	= WDIOF_SETTIMEOUT |
+			  WDIOF_MAGICCLOSE |
+			  WDIOF_KEEPALIVEPING,
+	.firmware_version = 0,
+	.identity	= "Tegra Watchdog",
+};
+
+static struct watchdog_ops tegra_wdt_ops = {
+	.owner = THIS_MODULE,
+	.start = tegra_wdt_start,
+	.stop = tegra_wdt_stop,
+	.ping = tegra_wdt_ping,
+	.set_timeout = tegra_wdt_set_timeout,
+	.get_timeleft = tegra_wdt_get_timeleft,
+	.ref = tegra_wdt_ref,
+	.ref = tegra_wdt_unref,
+};
+
+static int tegra_wdt_probe(struct platform_device *pdev)
+{
+	struct watchdog_device *wdd;
+	struct tegra_wdt *wdt;
+	struct resource *res;
+	struct resource *mem;
+	void __iomem *regs;
+	int ret;
+
+	/* This is the timer base. */
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "incorrect resources\n");
+		return -ENOENT;
+	}
+
+	mem = devm_request_mem_region(&pdev->dev, res->start,
+				      resource_size(res),
+				      pdev->name);
+	if (!mem) {
+		dev_err(&pdev->dev, "unable to request memory resources\n");
+		return -EBUSY;
+	}
+
+	regs = devm_ioremap(&pdev->dev, mem->start,
+			    resource_size(mem));
+	if (!regs) {
+		dev_err(&pdev->dev, "unable to map registers\n");
+		return -ENOMEM;
+	}
+
+	/*
+	 * Allocate our watchdog driver data, which has the
+	 * struct watchdog_device nested within it.
+	 */
+	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
+	if (!wdt) {
+		dev_err(&pdev->dev, "out of memory\n");
+		return -ENOMEM;
+	}
+
+	/* Initialize struct tegra_wdt. */
+	wdt->wdt_regs = regs + WDT_BASE;
+	wdt->tmr_regs = regs + WDT_TIMER_BASE;
+
+	/* Initialize struct watchdog_device. */
+	wdd = &wdt->wdd;
+	wdd->timeout = heartbeat;
+	wdd->info = &tegra_wdt_info;
+	wdd->ops = &tegra_wdt_ops;
+	wdd->min_timeout = MIN_WDT_TIMEOUT;
+	wdd->max_timeout = MAX_WDT_TIMEOUT;
+
+	watchdog_set_drvdata(wdd, wdt);
+
+	kref_init(&wdt->kref);
+
+	ret = tegra_wdt_stop(wdd);
+	if (ret) {
+		dev_err(&pdev->dev,
+			"failed to stop watchdog device\n");
+		return ret;
+	}
+
+	watchdog_set_nowayout(wdd, nowayout);
+
+	ret = watchdog_register_device(wdd);
+	if (ret) {
+		dev_err(&pdev->dev,
+			"failed to register watchdog device\n");
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, wdt);
+
+	dev_info(&pdev->dev,
+		 "initialized wdt %d, using timer %d\n", WDT_ID, WDT_TIMER_ID);
+	dev_info(&pdev->dev,
+		 "enabled wdt %d (heartbeat = %d sec, nowayout = %d)\n",
+		 WDT_ID, heartbeat, nowayout);
+
+	return 0;
+}
+
+static int tegra_wdt_remove(struct platform_device *pdev)
+{
+	struct tegra_wdt *wdt = platform_get_drvdata(pdev);
+	int ret;
+
+	ret = tegra_wdt_stop(&wdt->wdd);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to stop watchdog %d\n", WDT_ID);
+		return ret;
+	}
+
+	watchdog_unregister_device(&wdt->wdd);
+
+	kref_put(&wdt->kref, tegra_wdt_release_resources);
+
+	platform_set_drvdata(pdev, NULL);
+
+	dev_info(&pdev->dev, "removed wdt %d\n", WDT_ID);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int tegra_wdt_suspend(struct platform_device *pdev, pm_message_t state)
+{
+	struct tegra_wdt *wdt = platform_get_drvdata(pdev);
+	int ret;
+
+	ret = tegra_wdt_stop(&wdt->wdd);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to stop watchdog %d\n", WDT_ID);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int tegra_wdt_resume(struct platform_device *pdev)
+{
+	struct tegra_wdt *wdt = platform_get_drvdata(pdev);
+	int ret;
+
+	if (watchdog_active(&wdt->wdd)) {
+		ret = tegra_wdt_start(&wdt->wdd);
+		if (ret) {
+			dev_err(&pdev->dev,
+				"failed to stop watchdog %d\n", WDT_ID);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+#endif
+
+static const struct of_device_id tegra_wdt_of_match[] = {
+	{ .compatible = "nvidia,tegra30-timer", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, tegra_wdt_of_match);
+
+static struct platform_driver tegra_wdt_driver = {
+	.probe		= tegra_wdt_probe,
+	.remove		= tegra_wdt_remove,
+#ifdef CONFIG_PM
+	.suspend	= tegra_wdt_suspend,
+	.resume		= tegra_wdt_resume,
+#endif
+	.driver		= {
+		.owner	= THIS_MODULE,
+		.name	= "tegra-wdt",
+		.of_match_table = tegra_wdt_of_match,
+	},
+};
+
+module_platform_driver(tegra_wdt_driver);
+
+MODULE_AUTHOR("NVIDIA Corporation");
+MODULE_DESCRIPTION("Tegra Watchdog Driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);
+MODULE_ALIAS("platform:tegra-wdt");
-- 
1.8.1.5

Re: [PATCH v2 3/3] watchdog: Add tegra watchdog

[Stephen Warren]

On 02/03/2014 05:17 PM, Andrew Chew wrote:
> Add a driver for the hardware watchdogs in NVIDIA Tegra SoCs (tegra30 and

s/tegra30/Tegra30/

> later).  This driver will configure one watchdog timer that will reset the
> system in the case of a watchdog timeout.
> 
> This driver binds to the nvidia,tegra30-timer device node and gets its
> register base from there.

> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig

> +config TEGRA_WATCHDOG
> +	tristate "Tegra watchdog"
> +	depends on ARCH_TEGRA

|| COMPILE_TEST ?

> diff --git a/drivers/watchdog/tegra_wdt.c b/drivers/watchdog/tegra_wdt.c

> +static int tegra_wdt_probe(struct platform_device *pdev)

> +	/* This is the timer base. */
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
...
> +	mem = devm_request_mem_region(&pdev->dev, res->start,
> +				      resource_size(res),
> +				      pdev->name);
> +	if (!mem) {
...
> +	regs = devm_ioremap(&pdev->dev, mem->start,
> +			    resource_size(mem));
> +	if (!regs) {
...

I forget exactly which of those, but I think at least one/some of those
don't need to be error-checked, since the later functions will detect an
error input, and return an appropriate error code, so they can be
chained together while only error-checking the final result.

> +static int tegra_wdt_remove(struct platform_device *pdev)

> +	platform_set_drvdata(pdev, NULL);

There's no need to do that; the value of drvdata is irrelevant when the
device isn't probed, so there's no need for it to be NULL.

> +static struct platform_driver tegra_wdt_driver = {
> +	.probe		= tegra_wdt_probe,
> +	.remove		= tegra_wdt_remove,
> +#ifdef CONFIG_PM
> +	.suspend	= tegra_wdt_suspend,
> +	.resume		= tegra_wdt_resume,
> +#endif

I think for suspend/resume, you'd usually set it up like:

static const struct dev_pm_ops tegra20_i2s_pm_ops = {
        SET_RUNTIME_PM_OPS(tegra20_i2s_runtime_suspend,
                           tegra20_i2s_runtime_resume, NULL)
};

static struct platform_driver tegra20_i2s_driver = {
        .driver = {
...
               .pm = &tegra20_i2s_pm_ops,
        },
...
};

> +	.driver		= {
> +		.owner	= THIS_MODULE,
> +		.name	= "tegra-wdt",
> +		.of_match_table = tegra_wdt_of_match,
> +	},
> +};
> +
> +module_platform_driver(tegra_wdt_driver);

No need for the blank line before module_platform_driver().

> +MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);
> +MODULE_ALIAS("platform:tegra-wdt");

You don't need at least that second alias; everything binds to the
MODULE_DEVICE_TABLE() now with DT.