[PATCH v5 0/3] clocksource: fix Tegra234 SoC Watchdog Timer.

[PATCH v5 0/3] clocksource: fix Tegra234 SoC Watchdog Timer.

[Robert Lin]

From: robelin <robelin@nvidia.com>

This set of patches includes a fix for watchdog for it may not bark
due to self-pinging and adds WDIOC_GETTIMELEFT support.

--
V5:
- Print warning message if get unexpected value from the register

V4:
- Improve the precision of timeleft value
- Fix the unused variable warning

V3:
- Improve comment description
- Refactor to fit codeline within 80 columns
- Remove unused if(0) blocks


V2:
- Fix a compilation error, a warning and updates copyright
--

Pohsun Su (2):
  clocksource/drivers/timer-tegra186: add WDIOC_GETTIMELEFT support
  clocksource/drivers/timer-tegra186: fix watchdog self-pinging

robelin (1):
  clocksource/drivers/timer-tegra186: Remove unused bits

 drivers/clocksource/timer-tegra186.c | 94 +++++++++++++++++-----------
 1 file changed, 57 insertions(+), 37 deletions(-)

-- 
2.34.1

[PATCH v5 1/3] clocksource/drivers/timer-tegra186: add WDIOC_GETTIMELEFT support

[Robert Lin]

From: Pohsun Su <pohsuns@nvidia.com>

This change adds support for WDIOC_GETTIMELEFT so userspace
programs can get the number of seconds before system reset by
the watchdog timer via ioctl.

Signed-off-by: Pohsun Su <pohsuns@nvidia.com>
Signed-off-by: Robert Lin <robelin@nvidia.com>
---
 drivers/clocksource/timer-tegra186.c | 58 +++++++++++++++++++++++++++-
 1 file changed, 57 insertions(+), 1 deletion(-)

diff --git a/drivers/clocksource/timer-tegra186.c b/drivers/clocksource/timer-tegra186.c
index ea742889ee06..56d08bf1b6b0 100644
--- a/drivers/clocksource/timer-tegra186.c
+++ b/drivers/clocksource/timer-tegra186.c
@@ -1,8 +1,9 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /*
- * Copyright (c) 2019-2020 NVIDIA Corporation. All rights reserved.
+ * Copyright (c) 2019-2025 NVIDIA Corporation. All rights reserved.
  */
 
+#include <linux/bitfield.h>
 #include <linux/clocksource.h>
 #include <linux/module.h>
 #include <linux/interrupt.h>
@@ -30,6 +31,7 @@
 
 #define TMRSR 0x004
 #define  TMRSR_INTR_CLR BIT(30)
+#define  TMRSR_PCV GENMASK(28, 0)
 
 #define TMRCSSR 0x008
 #define  TMRCSSR_SRC_USEC (0 << 0)
@@ -46,6 +48,9 @@
 #define  WDTCR_TIMER_SOURCE_MASK 0xf
 #define  WDTCR_TIMER_SOURCE(x) ((x) & 0xf)
 
+#define WDTSR 0x004
+#define  WDTSR_CURRENT_EXPIRATION_COUNT GENMASK(14, 12)
+
 #define WDTCMDR 0x008
 #define  WDTCMDR_DISABLE_COUNTER BIT(1)
 #define  WDTCMDR_START_COUNTER BIT(0)
@@ -235,12 +240,63 @@ static int tegra186_wdt_set_timeout(struct watchdog_device *wdd,
 	return 0;
 }
 
+static unsigned int tegra186_wdt_get_timeleft(struct watchdog_device *wdd)
+{
+	struct tegra186_wdt *wdt = to_tegra186_wdt(wdd);
+	u32 timeleft, expiration, val;
+
+	if (!watchdog_active(&wdt->base)) {
+		/* return zero if the watchdog timer is not activated. */
+		return 0;
+	}
+
+	/*
+	 * Reset occurs on the fifth expiration of the
+	 * watchdog timer and so when the watchdog timer is configured,
+	 * the actual value programmed into the counter is 1/5 of the
+	 * timeout value. Once the counter reaches 0, expiration count
+	 * will be increased by 1 and the down counter restarts.
+	 * Hence to get the time left before system reset we must
+	 * combine 2 parts:
+	 * 1. value of the current down counter
+	 * 2. (number of counter expirations remaining) * (timeout/5)
+	 */
+
+	/* Get the current number of counter expirations. Should be a
+	 * value between 0 and 4
+	 */
+	val = readl_relaxed(wdt->regs + WDTSR);
+	expiration = FIELD_GET(WDTSR_CURRENT_EXPIRATION_COUNT, val);
+	if (WARN_ON(expiration > 4))
+             return 0;
+
+	/* Get the current counter value in microsecond.
+	 */
+	val = readl_relaxed(wdt->tmr->regs + TMRSR);
+	timeleft = FIELD_GET(TMRSR_PCV, val);
+
+	/*
+	 * Calculate the time remaining by adding the time for the
+	 * counter value to the time of the counter expirations that
+	 * remain. Do the multiplication first on purpose just to keep
+	 * the precision due to the integer division.
+	 */
+	timeleft += wdt->base.timeout * (4 - expiration) / 5;
+	/*
+	 * Convert the current counter value to seconds,
+	 * rounding up to the nearest second.
+	 */
+	timeleft = (timeleft + USEC_PER_SEC / 2) / USEC_PER_SEC;
+	return timeleft;
+}
+
 static const struct watchdog_ops tegra186_wdt_ops = {
 	.owner = THIS_MODULE,
 	.start = tegra186_wdt_start,
 	.stop = tegra186_wdt_stop,
 	.ping = tegra186_wdt_ping,
 	.set_timeout = tegra186_wdt_set_timeout,
+	.get_timeleft = tegra186_wdt_get_timeleft,
 };
 
 static struct tegra186_wdt *tegra186_wdt_create(struct tegra186_timer *tegra,
-- 
2.34.1

Re: [PATCH v5 1/3] clocksource/drivers/timer-tegra186: add WDIOC_GETTIMELEFT support

[Jon Hunter]

Hi Robert,

On 21/04/2025 11:08, Robert Lin wrote:
> From: Pohsun Su <pohsuns@nvidia.com>
> 
> This change adds support for WDIOC_GETTIMELEFT so userspace
> programs can get the number of seconds before system reset by
> the watchdog timer via ioctl.
> 
> Signed-off-by: Pohsun Su <pohsuns@nvidia.com>
> Signed-off-by: Robert Lin <robelin@nvidia.com>
> ---
>   drivers/clocksource/timer-tegra186.c | 58 +++++++++++++++++++++++++++-
>   1 file changed, 57 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clocksource/timer-tegra186.c b/drivers/clocksource/timer-tegra186.c
> index ea742889ee06..56d08bf1b6b0 100644
> --- a/drivers/clocksource/timer-tegra186.c
> +++ b/drivers/clocksource/timer-tegra186.c
> @@ -1,8 +1,9 @@
>   // SPDX-License-Identifier: GPL-2.0-only
>   /*
> - * Copyright (c) 2019-2020 NVIDIA Corporation. All rights reserved.
> + * Copyright (c) 2019-2025 NVIDIA Corporation. All rights reserved.
>    */
>   
> +#include <linux/bitfield.h>
>   #include <linux/clocksource.h>
>   #include <linux/module.h>
>   #include <linux/interrupt.h>
> @@ -30,6 +31,7 @@
>   
>   #define TMRSR 0x004
>   #define  TMRSR_INTR_CLR BIT(30)
> +#define  TMRSR_PCV GENMASK(28, 0)
>   
>   #define TMRCSSR 0x008
>   #define  TMRCSSR_SRC_USEC (0 << 0)
> @@ -46,6 +48,9 @@
>   #define  WDTCR_TIMER_SOURCE_MASK 0xf
>   #define  WDTCR_TIMER_SOURCE(x) ((x) & 0xf)
>   
> +#define WDTSR 0x004
> +#define  WDTSR_CURRENT_EXPIRATION_COUNT GENMASK(14, 12)
> +
>   #define WDTCMDR 0x008
>   #define  WDTCMDR_DISABLE_COUNTER BIT(1)
>   #define  WDTCMDR_START_COUNTER BIT(0)
> @@ -235,12 +240,63 @@ static int tegra186_wdt_set_timeout(struct watchdog_device *wdd,
>   	return 0;
>   }
>   
> +static unsigned int tegra186_wdt_get_timeleft(struct watchdog_device *wdd)
> +{
> +	struct tegra186_wdt *wdt = to_tegra186_wdt(wdd);
> +	u32 timeleft, expiration, val;
> +
> +	if (!watchdog_active(&wdt->base)) {
> +		/* return zero if the watchdog timer is not activated. */
> +		return 0;
> +	}
> +
> +	/*
> +	 * Reset occurs on the fifth expiration of the
> +	 * watchdog timer and so when the watchdog timer is configured,
> +	 * the actual value programmed into the counter is 1/5 of the
> +	 * timeout value. Once the counter reaches 0, expiration count
> +	 * will be increased by 1 and the down counter restarts.
> +	 * Hence to get the time left before system reset we must
> +	 * combine 2 parts:
> +	 * 1. value of the current down counter
> +	 * 2. (number of counter expirations remaining) * (timeout/5)
> +	 */
> +
> +	/* Get the current number of counter expirations. Should be a
> +	 * value between 0 and 4
> +	 */
> +	val = readl_relaxed(wdt->regs + WDTSR);
> +	expiration = FIELD_GET(WDTSR_CURRENT_EXPIRATION_COUNT, val);
> +	if (WARN_ON(expiration > 4))
> +             return 0;
> +
> +	/* Get the current counter value in microsecond.
> +	 */
> +	val = readl_relaxed(wdt->tmr->regs + TMRSR);
> +	timeleft = FIELD_GET(TMRSR_PCV, val);

So this value is in microseconds.

> +
> +	/*
> +	 * Calculate the time remaining by adding the time for the
> +	 * counter value to the time of the counter expirations that
> +	 * remain. Do the multiplication first on purpose just to keep
> +	 * the precision due to the integer division.
> +	 */
> +	timeleft += wdt->base.timeout * (4 - expiration) / 5;

However, wdt->base.timeout is in seconds. So I don't think we can simply 
add this. Don't we need to ...

timeleft += (wdt->base.timeout * USEC_PER_SEC * (4 - expiration)) / 5;

Given that this could be quite a big number, we probably want to make 
timeleft a 64-bit type too. So we may want to define a 'u64 
timeleft_usecs' and 'u32 timeleft_secs' that we return.

Jon

-- 
nvpublic

RE: [PATCH v5 1/3] clocksource/drivers/timer-tegra186: add WDIOC_GETTIMELEFT support

[Robert Lin]

> -----Original Message-----
> From: Jon Hunter <jonathanh@nvidia.com>
> Sent: Monday, April 28, 2025 10:04 PM
> To: Robert Lin <robelin@nvidia.com>; thierry.reding@gmail.com;
> daniel.lezcano@linaro.org; tglx@linutronix.de; Pohsun Su
> <pohsuns@nvidia.com>
> Cc: linux-kernel@vger.kernel.org; linux-tegra@vger.kernel.org; Sumit Gupta
> <sumitg@nvidia.com>
> Subject: Re: [PATCH v5 1/3] clocksource/drivers/timer-tegra186: add
> WDIOC_GETTIMELEFT support
> 
> Hi Robert,
> 
> On 21/04/2025 11:08, Robert Lin wrote:
> > From: Pohsun Su <pohsuns@nvidia.com>
> >
> > This change adds support for WDIOC_GETTIMELEFT so userspace programs
> > can get the number of seconds before system reset by the watchdog
> > timer via ioctl.
> >
> > Signed-off-by: Pohsun Su <pohsuns@nvidia.com>
> > Signed-off-by: Robert Lin <robelin@nvidia.com>
> > ---
> >   drivers/clocksource/timer-tegra186.c | 58
> +++++++++++++++++++++++++++-
> >   1 file changed, 57 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/clocksource/timer-tegra186.c
> > b/drivers/clocksource/timer-tegra186.c
> > index ea742889ee06..56d08bf1b6b0 100644
> > --- a/drivers/clocksource/timer-tegra186.c
> > +++ b/drivers/clocksource/timer-tegra186.c
> > @@ -1,8 +1,9 @@
> >   // SPDX-License-Identifier: GPL-2.0-only
> >   /*
> > - * Copyright (c) 2019-2020 NVIDIA Corporation. All rights reserved.
> > + * Copyright (c) 2019-2025 NVIDIA Corporation. All rights reserved.
> >    */
> >
> > +#include <linux/bitfield.h>
> >   #include <linux/clocksource.h>
> >   #include <linux/module.h>
> >   #include <linux/interrupt.h>
> > @@ -30,6 +31,7 @@
> >
> >   #define TMRSR 0x004
> >   #define  TMRSR_INTR_CLR BIT(30)
> > +#define  TMRSR_PCV GENMASK(28, 0)
> >
> >   #define TMRCSSR 0x008
> >   #define  TMRCSSR_SRC_USEC (0 << 0)
> > @@ -46,6 +48,9 @@
> >   #define  WDTCR_TIMER_SOURCE_MASK 0xf
> >   #define  WDTCR_TIMER_SOURCE(x) ((x) & 0xf)
> >
> > +#define WDTSR 0x004
> > +#define  WDTSR_CURRENT_EXPIRATION_COUNT GENMASK(14, 12)
> > +
> >   #define WDTCMDR 0x008
> >   #define  WDTCMDR_DISABLE_COUNTER BIT(1)
> >   #define  WDTCMDR_START_COUNTER BIT(0) @@ -235,12 +240,63 @@
> static
> > int tegra186_wdt_set_timeout(struct watchdog_device *wdd,
> >   	return 0;
> >   }
> >
> > +static unsigned int tegra186_wdt_get_timeleft(struct watchdog_device
> > +*wdd) {
> > +	struct tegra186_wdt *wdt = to_tegra186_wdt(wdd);
> > +	u32 timeleft, expiration, val;
> > +
> > +	if (!watchdog_active(&wdt->base)) {
> > +		/* return zero if the watchdog timer is not activated. */
> > +		return 0;
> > +	}
> > +
> > +	/*
> > +	 * Reset occurs on the fifth expiration of the
> > +	 * watchdog timer and so when the watchdog timer is configured,
> > +	 * the actual value programmed into the counter is 1/5 of the
> > +	 * timeout value. Once the counter reaches 0, expiration count
> > +	 * will be increased by 1 and the down counter restarts.
> > +	 * Hence to get the time left before system reset we must
> > +	 * combine 2 parts:
> > +	 * 1. value of the current down counter
> > +	 * 2. (number of counter expirations remaining) * (timeout/5)
> > +	 */
> > +
> > +	/* Get the current number of counter expirations. Should be a
> > +	 * value between 0 and 4
> > +	 */
> > +	val = readl_relaxed(wdt->regs + WDTSR);
> > +	expiration = FIELD_GET(WDTSR_CURRENT_EXPIRATION_COUNT, val);
> > +	if (WARN_ON(expiration > 4))
> > +             return 0;
> > +
> > +	/* Get the current counter value in microsecond.
> > +	 */
> > +	val = readl_relaxed(wdt->tmr->regs + TMRSR);
> > +	timeleft = FIELD_GET(TMRSR_PCV, val);
> 
> So this value is in microseconds.
> 
> > +
> > +	/*
> > +	 * Calculate the time remaining by adding the time for the
> > +	 * counter value to the time of the counter expirations that
> > +	 * remain. Do the multiplication first on purpose just to keep
> > +	 * the precision due to the integer division.
> > +	 */
> > +	timeleft += wdt->base.timeout * (4 - expiration) / 5;
> 
> However, wdt->base.timeout is in seconds. So I don't think we can simply add
> this. Don't we need to ...
> 
> timeleft += (wdt->base.timeout * USEC_PER_SEC * (4 - expiration)) / 5;
> 

You are right. As watchdog timer can still be triggered at the right time, even if get_timeleft callback return the inaccurate value, I thought my change is fine. But it turns out get_timeleft is independent to this.

I'll fix and validate this carefully.

> Given that this could be quite a big number, we probably want to make
> timeleft a 64-bit type too. So we may want to define a 'u64 timeleft_usecs'
> and 'u32 timeleft_secs' that we return.
> 

We can do this.

> Jon
> 
> --
> nvpublic

Re: [PATCH v5 1/3] clocksource/drivers/timer-tegra186: add WDIOC_GETTIMELEFT support

[Daniel Lezcano]

On Mon, Apr 21, 2025 at 06:08:19PM +0800, Robert Lin wrote:
> From: Pohsun Su <pohsuns@nvidia.com>
> 
> This change adds support for WDIOC_GETTIMELEFT so userspace
> programs can get the number of seconds before system reset by
> the watchdog timer via ioctl.
> 
> Signed-off-by: Pohsun Su <pohsuns@nvidia.com>
> Signed-off-by: Robert Lin <robelin@nvidia.com>
> ---

Hi Robert,

I realize that this driver should be split in two and the watchdog part go
under drivers/watchdog.

>  drivers/clocksource/timer-tegra186.c | 58 +++++++++++++++++++++++++++-
>  1 file changed, 57 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clocksource/timer-tegra186.c b/drivers/clocksource/timer-tegra186.c
> index ea742889ee06..56d08bf1b6b0 100644
> --- a/drivers/clocksource/timer-tegra186.c
> +++ b/drivers/clocksource/timer-tegra186.c
> @@ -1,8 +1,9 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /*
> - * Copyright (c) 2019-2020 NVIDIA Corporation. All rights reserved.
> + * Copyright (c) 2019-2025 NVIDIA Corporation. All rights reserved.
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/clocksource.h>
>  #include <linux/module.h>
>  #include <linux/interrupt.h>
> @@ -30,6 +31,7 @@
>  
>  #define TMRSR 0x004
>  #define  TMRSR_INTR_CLR BIT(30)
> +#define  TMRSR_PCV GENMASK(28, 0)
>  
>  #define TMRCSSR 0x008
>  #define  TMRCSSR_SRC_USEC (0 << 0)
> @@ -46,6 +48,9 @@
>  #define  WDTCR_TIMER_SOURCE_MASK 0xf
>  #define  WDTCR_TIMER_SOURCE(x) ((x) & 0xf)
>  
> +#define WDTSR 0x004
> +#define  WDTSR_CURRENT_EXPIRATION_COUNT GENMASK(14, 12)
> +
>  #define WDTCMDR 0x008
>  #define  WDTCMDR_DISABLE_COUNTER BIT(1)
>  #define  WDTCMDR_START_COUNTER BIT(0)
> @@ -235,12 +240,63 @@ static int tegra186_wdt_set_timeout(struct watchdog_device *wdd,
>  	return 0;
>  }
>  
> +static unsigned int tegra186_wdt_get_timeleft(struct watchdog_device *wdd)
> +{
> +	struct tegra186_wdt *wdt = to_tegra186_wdt(wdd);
> +	u32 timeleft, expiration, val;
> +
> +	if (!watchdog_active(&wdt->base)) {
> +		/* return zero if the watchdog timer is not activated. */
> +		return 0;
> +	}
> +
> +	/*
> +	 * Reset occurs on the fifth expiration of the
> +	 * watchdog timer and so when the watchdog timer is configured,
> +	 * the actual value programmed into the counter is 1/5 of the
> +	 * timeout value. Once the counter reaches 0, expiration count
> +	 * will be increased by 1 and the down counter restarts.
> +	 * Hence to get the time left before system reset we must
> +	 * combine 2 parts:
> +	 * 1. value of the current down counter
> +	 * 2. (number of counter expirations remaining) * (timeout/5)
> +	 */
> +
> +	/* Get the current number of counter expirations. Should be a
> +	 * value between 0 and 4
> +	 */
> +	val = readl_relaxed(wdt->regs + WDTSR);
> +	expiration = FIELD_GET(WDTSR_CURRENT_EXPIRATION_COUNT, val);
> +	if (WARN_ON(expiration > 4))
> +             return 0;
> +
> +	/* Get the current counter value in microsecond.
> +	 */
> +	val = readl_relaxed(wdt->tmr->regs + TMRSR);
> +	timeleft = FIELD_GET(TMRSR_PCV, val);
> +
> +	/*
> +	 * Calculate the time remaining by adding the time for the
> +	 * counter value to the time of the counter expirations that
> +	 * remain. Do the multiplication first on purpose just to keep
> +	 * the precision due to the integer division.
> +	 */
> +	timeleft += wdt->base.timeout * (4 - expiration) / 5;
> +	/*
> +	 * Convert the current counter value to seconds,
> +	 * rounding up to the nearest second.
> +	 */
> +	timeleft = (timeleft + USEC_PER_SEC / 2) / USEC_PER_SEC;
> +	return timeleft;
> +}
> +
>  static const struct watchdog_ops tegra186_wdt_ops = {
>  	.owner = THIS_MODULE,
>  	.start = tegra186_wdt_start,
>  	.stop = tegra186_wdt_stop,
>  	.ping = tegra186_wdt_ping,
>  	.set_timeout = tegra186_wdt_set_timeout,
> +	.get_timeleft = tegra186_wdt_get_timeleft,
>  };
>  
>  static struct tegra186_wdt *tegra186_wdt_create(struct tegra186_timer *tegra,
> -- 
> 2.34.1
> 

-- 

 <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 v5 1/3] clocksource/drivers/timer-tegra186: add WDIOC_GETTIMELEFT support

[Jon Hunter]

Hi Daniel,

On 29/04/2025 09:59, Daniel Lezcano wrote:
> On Mon, Apr 21, 2025 at 06:08:19PM +0800, Robert Lin wrote:
>> From: Pohsun Su <pohsuns@nvidia.com>
>>
>> This change adds support for WDIOC_GETTIMELEFT so userspace
>> programs can get the number of seconds before system reset by
>> the watchdog timer via ioctl.
>>
>> Signed-off-by: Pohsun Su <pohsuns@nvidia.com>
>> Signed-off-by: Robert Lin <robelin@nvidia.com>
>> ---
> 
> Hi Robert,
> 
> I realize that this driver should be split in two and the watchdog part go
> under drivers/watchdog.

Are there any other examples you know of where the timer is split in 
this way? It is not clear to me how you propose we do this?

BTW, for this series, I just want to get these updates merged. Any other 
re-factoring we can handle later.

Cheers!
Jon

-- 
nvpublic

Re: [PATCH v5 1/3] clocksource/drivers/timer-tegra186: add WDIOC_GETTIMELEFT support

[Daniel Lezcano]

On 29/04/2025 11:15, Jon Hunter wrote:
> Hi Daniel,
> 
> On 29/04/2025 09:59, Daniel Lezcano wrote:
>> On Mon, Apr 21, 2025 at 06:08:19PM +0800, Robert Lin wrote:
>>> From: Pohsun Su <pohsuns@nvidia.com>
>>>
>>> This change adds support for WDIOC_GETTIMELEFT so userspace
>>> programs can get the number of seconds before system reset by
>>> the watchdog timer via ioctl.
>>>
>>> Signed-off-by: Pohsun Su <pohsuns@nvidia.com>
>>> Signed-off-by: Robert Lin <robelin@nvidia.com>
>>> ---
>>
>> Hi Robert,
>>
>> I realize that this driver should be split in two and the watchdog 
>> part go
>> under drivers/watchdog.
> 
> Are there any other examples you know of where the timer is split in 
> this way? It is not clear to me how you propose we do this?

Just keep the clocksource and move the watchdog code (everything related 
to the watchdog_ops) to a new driver under drivers/watchdog

BTW, there are three clocksources with the same rating, what is the 
point of having them supported ?

Is it not the architected clocksource enough ?

May be the clocksource can be removed and the driver remains a pure 
watchdog driver ?

> BTW, for this series, I just want to get these updates merged. Any other 
> re-factoring we can handle later.
> 
> Cheers!
> Jon
> 


-- 
<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 v5 1/3] clocksource/drivers/timer-tegra186: add WDIOC_GETTIMELEFT support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1814 bytes --]

On Tue, Apr 29, 2025 at 03:19:25PM +0200, Daniel Lezcano wrote:
> On 29/04/2025 11:15, Jon Hunter wrote:
> > Hi Daniel,
> > 
> > On 29/04/2025 09:59, Daniel Lezcano wrote:
> > > On Mon, Apr 21, 2025 at 06:08:19PM +0800, Robert Lin wrote:
> > > > From: Pohsun Su <pohsuns@nvidia.com>
> > > > 
> > > > This change adds support for WDIOC_GETTIMELEFT so userspace
> > > > programs can get the number of seconds before system reset by
> > > > the watchdog timer via ioctl.
> > > > 
> > > > Signed-off-by: Pohsun Su <pohsuns@nvidia.com>
> > > > Signed-off-by: Robert Lin <robelin@nvidia.com>
> > > > ---
> > > 
> > > Hi Robert,
> > > 
> > > I realize that this driver should be split in two and the watchdog
> > > part go
> > > under drivers/watchdog.
> > 
> > Are there any other examples you know of where the timer is split in
> > this way? It is not clear to me how you propose we do this?
> 
> Just keep the clocksource and move the watchdog code (everything related to
> the watchdog_ops) to a new driver under drivers/watchdog

That's a bad idea. This is all a single register space, so we can't have
"proper" drivers (i.e. ones that exclusively request I/O memory regions)
if we split them up.

I understand that it's nice and easy to have things split up along
subsystem boundaries, but sometimes hardware designs just aren't that
cleanly separated.

> BTW, there are three clocksources with the same rating, what is the point of
> having them supported ?
> 
> Is it not the architected clocksource enough ?

The TSC clock source that this driver exposes is different from the
architected timer. It's a SoC-wide clock that is routed to various IP
blocks and used for timestamping events. This clocksource allows these
events to be properly compared, etc.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 1/3] clocksource/drivers/timer-tegra186: add WDIOC_GETTIMELEFT support

[Daniel Lezcano]

On Tue, Apr 29, 2025 at 04:23:22PM +0200, Thierry Reding wrote:
> On Tue, Apr 29, 2025 at 03:19:25PM +0200, Daniel Lezcano wrote:
> > On 29/04/2025 11:15, Jon Hunter wrote:
> > > Hi Daniel,
> > > 
> > > On 29/04/2025 09:59, Daniel Lezcano wrote:
> > > > On Mon, Apr 21, 2025 at 06:08:19PM +0800, Robert Lin wrote:
> > > > > From: Pohsun Su <pohsuns@nvidia.com>
> > > > > 
> > > > > This change adds support for WDIOC_GETTIMELEFT so userspace
> > > > > programs can get the number of seconds before system reset by
> > > > > the watchdog timer via ioctl.
> > > > > 
> > > > > Signed-off-by: Pohsun Su <pohsuns@nvidia.com>
> > > > > Signed-off-by: Robert Lin <robelin@nvidia.com>
> > > > > ---
> > > > 
> > > > Hi Robert,
> > > > 
> > > > I realize that this driver should be split in two and the watchdog
> > > > part go
> > > > under drivers/watchdog.
> > > 
> > > Are there any other examples you know of where the timer is split in
> > > this way? It is not clear to me how you propose we do this?
> > 
> > Just keep the clocksource and move the watchdog code (everything related to
> > the watchdog_ops) to a new driver under drivers/watchdog
> 
> That's a bad idea. This is all a single register space, so we can't have
> "proper" drivers (i.e. ones that exclusively request I/O memory regions)
> if we split them up.
> 
> I understand that it's nice and easy to have things split up along
> subsystem boundaries, but sometimes hardware designs just aren't that
> cleanly separated.

Yes, that's true.

The driver has a lot of watchdog code inside and I think it is
possible to move most part of it under drivers/watchdog. Perhaps by
exporting tegra186_wdt_disable() / tegra186_wdt_enable().

Anyway, I understand that is an important change and I don't want to
block this series for this reason. At the first glance, these changes
seem to be fine for me, I'll just do a last review and comment/pick
them.

> > BTW, there are three clocksources with the same rating, what is the point of
> > having them supported ?
> > 
> > Is it not the architected clocksource enough ?
> 
> The TSC clock source that this driver exposes is different from the
> architected timer. It's a SoC-wide clock that is routed to various IP
> blocks and used for timestamping events. This clocksource allows these
> events to be properly compared, etc.

I see, thanks for clarifying

  -- Daniel


-- 

 <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

[PATCH v5 2/3] clocksource/drivers/timer-tegra186: fix watchdog self-pinging

[Robert Lin]

From: Pohsun Su <pohsuns@nvidia.com>

This change removes watchdog self-pinging behavior.

The timer irq handler is triggered due to the 1st expiration,
the handler disables and enables watchdog but also implicitly
clears the expiration count so the count can only be 0 or 1.

Since this watchdog supports opened, configured, or pinged by
systemd, We remove this behavior or the watchdog may not bark
when systemd crashes since the 5th expiration never comes.

Signed-off-by: Pohsun Su <pohsuns@nvidia.com>
Signed-off-by: Robert Lin <robelin@nvidia.com>
---
 drivers/clocksource/timer-tegra186.c | 27 ---------------------------
 1 file changed, 27 deletions(-)

diff --git a/drivers/clocksource/timer-tegra186.c b/drivers/clocksource/timer-tegra186.c
index 56d08bf1b6b0..4f06eb1ad309 100644
--- a/drivers/clocksource/timer-tegra186.c
+++ b/drivers/clocksource/timer-tegra186.c
@@ -175,9 +175,6 @@ static void tegra186_wdt_enable(struct tegra186_wdt *wdt)
 		value &= ~WDTCR_PERIOD_MASK;
 		value |= WDTCR_PERIOD(1);
 
-		/* enable local interrupt for WDT petting */
-		value |= WDTCR_LOCAL_INT_ENABLE;
-
 		/* enable local FIQ and remote interrupt for debug dump */
 		if (0)
 			value |= WDTCR_REMOTE_INT_ENABLE |
@@ -422,23 +419,10 @@ static int tegra186_timer_usec_init(struct tegra186_timer *tegra)
 	return clocksource_register_hz(&tegra->usec, USEC_PER_SEC);
 }
 
-static irqreturn_t tegra186_timer_irq(int irq, void *data)
-{
-	struct tegra186_timer *tegra = data;
-
-	if (watchdog_active(&tegra->wdt->base)) {
-		tegra186_wdt_disable(tegra->wdt);
-		tegra186_wdt_enable(tegra->wdt);
-	}
-
-	return IRQ_HANDLED;
-}
-
 static int tegra186_timer_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct tegra186_timer *tegra;
-	unsigned int irq;
 	int err;
 
 	tegra = devm_kzalloc(dev, sizeof(*tegra), GFP_KERNEL);
@@ -457,8 +441,6 @@ static int tegra186_timer_probe(struct platform_device *pdev)
 	if (err < 0)
 		return err;
 
-	irq = err;
-
 	/* create a watchdog using a preconfigured timer */
 	tegra->wdt = tegra186_wdt_create(tegra, 0);
 	if (IS_ERR(tegra->wdt)) {
@@ -485,17 +467,8 @@ static int tegra186_timer_probe(struct platform_device *pdev)
 		goto unregister_osc;
 	}
 
-	err = devm_request_irq(dev, irq, tegra186_timer_irq, 0,
-			       "tegra186-timer", tegra);
-	if (err < 0) {
-		dev_err(dev, "failed to request IRQ#%u: %d\n", irq, err);
-		goto unregister_usec;
-	}
-
 	return 0;
 
-unregister_usec:
-	clocksource_unregister(&tegra->usec);
 unregister_osc:
 	clocksource_unregister(&tegra->osc);
 unregister_tsc:
-- 
2.34.1

[PATCH v5 3/3] clocksource/drivers/timer-tegra186: Remove unused bits

[Robert Lin]

From: robelin <robelin@nvidia.com>

The intention to keep the unsed if(0) block is gone now. Remove
them for clean codes.

Signed-off-by: robelin <robelin@nvidia.com>
---
 drivers/clocksource/timer-tegra186.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/clocksource/timer-tegra186.c b/drivers/clocksource/timer-tegra186.c
index 4f06eb1ad309..1f4bde813a72 100644
--- a/drivers/clocksource/timer-tegra186.c
+++ b/drivers/clocksource/timer-tegra186.c
@@ -175,15 +175,6 @@ static void tegra186_wdt_enable(struct tegra186_wdt *wdt)
 		value &= ~WDTCR_PERIOD_MASK;
 		value |= WDTCR_PERIOD(1);
 
-		/* enable local FIQ and remote interrupt for debug dump */
-		if (0)
-			value |= WDTCR_REMOTE_INT_ENABLE |
-				 WDTCR_LOCAL_FIQ_ENABLE;
-
-		/* enable system debug reset (doesn't properly reboot) */
-		if (0)
-			value |= WDTCR_SYSTEM_DEBUG_RESET_ENABLE;
-
 		/* enable system POR reset */
 		value |= WDTCR_SYSTEM_POR_RESET_ENABLE;
 
-- 
2.34.1