* [PATCH v5 1/4] dt-bindings: timer: via,vt8500-timer: Convert to YAML
2025-05-21 13:00 [PATCH v5 0/4] clocksource/drivers/timer-vt8500: clean up and add watchdog function Alexey Charkov
@ 2025-05-21 13:00 ` Alexey Charkov
2025-07-23 3:43 ` Rob Herring
2025-05-21 13:00 ` [PATCH v5 2/4] clocksource/drivers/timer-vt8500: Add defines for magic constants Alexey Charkov
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Alexey Charkov @ 2025-05-21 13:00 UTC (permalink / raw)
To: Krzysztof Kozlowski, Daniel Lezcano, Thomas Gleixner, Rob Herring,
Conor Dooley, Krzysztof Kozlowski, Wim Van Sebroeck,
Guenter Roeck
Cc: linux-arm-kernel, linux-kernel, devicetree, linux-watchdog,
Alexey Charkov
Rewrite the textual description for the VIA/WonderMedia timer
as YAML schema.
The IP can generate up to four interrupts from four respective match
registers, so reflect that in the schema.
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Alexey Charkov <alchark@gmail.com>
---
.../devicetree/bindings/timer/via,vt8500-timer.txt | 15 -------
.../bindings/timer/via,vt8500-timer.yaml | 51 ++++++++++++++++++++++
MAINTAINERS | 1 +
3 files changed, 52 insertions(+), 15 deletions(-)
diff --git a/Documentation/devicetree/bindings/timer/via,vt8500-timer.txt b/Documentation/devicetree/bindings/timer/via,vt8500-timer.txt
deleted file mode 100644
index 901c73f0d8ef05fb54d517b807d04f80eef2e736..0000000000000000000000000000000000000000
--- a/Documentation/devicetree/bindings/timer/via,vt8500-timer.txt
+++ /dev/null
@@ -1,15 +0,0 @@
-VIA/Wondermedia VT8500 Timer
------------------------------------------------------
-
-Required properties:
-- compatible : "via,vt8500-timer"
-- reg : Should contain 1 register ranges(address and length)
-- interrupts : interrupt for the timer
-
-Example:
-
- timer@d8130100 {
- compatible = "via,vt8500-timer";
- reg = <0xd8130100 0x28>;
- interrupts = <36>;
- };
diff --git a/Documentation/devicetree/bindings/timer/via,vt8500-timer.yaml b/Documentation/devicetree/bindings/timer/via,vt8500-timer.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..e748149948f3140d4a158f800b91e70bf9c4f042
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/via,vt8500-timer.yaml
@@ -0,0 +1,51 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/timer/via,vt8500-timer.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: VIA/Wondermedia VT8500 Timer
+
+description:
+ This is the timer block that is a standalone part of the system power
+ management controller on VIA/WonderMedia SoCs (VIA VT8500 and alike).
+ The hardware has a single 32-bit counter running at 3 MHz and four match
+ registers, each of which is associated with a dedicated match interrupt,
+ and the first of which can also serve as the system watchdog (if the
+ watchdog function is enabled, it will reset the system upon match instead
+ of triggering its respective interrupt)
+
+maintainers:
+ - Alexey Charkov <alchark@gmail.com>
+
+properties:
+ compatible:
+ const: via,vt8500-timer
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ minItems: 1
+ items:
+ - description: Channel 0 match. Note that if the watchdog function
+ is enabled, this interrupt will not fire and the system will
+ reboot instead once the counter reaches match register 0 value
+ - description: Channel 1 match
+ - description: Channel 2 match
+ - description: Channel 3 match
+
+required:
+ - compatible
+ - reg
+ - interrupts
+
+additionalProperties: false
+
+examples:
+ - |
+ timer@d8130100 {
+ compatible = "via,vt8500-timer";
+ reg = <0xd8130100 0x28>;
+ interrupts = <36>;
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index 3563492e4eba48b9e9389687fc9ac2a881c47ddf..783e5ee6854b69cca87b6f0763844d28b4b2213f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3435,6 +3435,7 @@ M: Krzysztof Kozlowski <krzk@kernel.org>
L: linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
S: Odd Fixes
F: Documentation/devicetree/bindings/i2c/i2c-wmt.txt
+F: Documentation/devicetree/bindings/timer/via,vt8500-timer.yaml
F: arch/arm/boot/dts/vt8500/
F: arch/arm/mach-vt8500/
F: drivers/clocksource/timer-vt8500.c
--
2.49.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v5 1/4] dt-bindings: timer: via,vt8500-timer: Convert to YAML
2025-05-21 13:00 ` [PATCH v5 1/4] dt-bindings: timer: via,vt8500-timer: Convert to YAML Alexey Charkov
@ 2025-07-23 3:43 ` Rob Herring
0 siblings, 0 replies; 10+ messages in thread
From: Rob Herring @ 2025-07-23 3:43 UTC (permalink / raw)
To: Alexey Charkov
Cc: Krzysztof Kozlowski, Daniel Lezcano, Thomas Gleixner,
Conor Dooley, Krzysztof Kozlowski, Wim Van Sebroeck,
Guenter Roeck, linux-arm-kernel, linux-kernel, devicetree,
linux-watchdog
On Wed, May 21, 2025 at 05:00:09PM +0400, Alexey Charkov wrote:
> Rewrite the textual description for the VIA/WonderMedia timer
> as YAML schema.
>
> The IP can generate up to four interrupts from four respective match
> registers, so reflect that in the schema.
>
> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> Signed-off-by: Alexey Charkov <alchark@gmail.com>
> ---
> .../devicetree/bindings/timer/via,vt8500-timer.txt | 15 -------
> .../bindings/timer/via,vt8500-timer.yaml | 51 ++++++++++++++++++++++
> MAINTAINERS | 1 +
> 3 files changed, 52 insertions(+), 15 deletions(-)
There's no reason for this to wait on discussions on the driver, so I
applied it.
Rob
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v5 2/4] clocksource/drivers/timer-vt8500: Add defines for magic constants
2025-05-21 13:00 [PATCH v5 0/4] clocksource/drivers/timer-vt8500: clean up and add watchdog function Alexey Charkov
2025-05-21 13:00 ` [PATCH v5 1/4] dt-bindings: timer: via,vt8500-timer: Convert to YAML Alexey Charkov
@ 2025-05-21 13:00 ` Alexey Charkov
2025-05-21 13:00 ` [PATCH v5 3/4] clocksource/drivers/timer-vt8500: Prepare for watchdog functionality Alexey Charkov
2025-05-21 13:00 ` [PATCH v5 4/4] watchdog: Add support for VIA/WonderMedia SoC " Alexey Charkov
3 siblings, 0 replies; 10+ messages in thread
From: Alexey Charkov @ 2025-05-21 13:00 UTC (permalink / raw)
To: Krzysztof Kozlowski, Daniel Lezcano, Thomas Gleixner, Rob Herring,
Conor Dooley, Krzysztof Kozlowski, Wim Van Sebroeck,
Guenter Roeck
Cc: linux-arm-kernel, linux-kernel, devicetree, linux-watchdog,
Alexey Charkov
Add defines for all known registers and their bits to make the code more
self-explanatory. While at that, replace _VAL suffixes with more
intuitive _REG suffixes on register offsets.
No functional changes.
Signed-off-by: Alexey Charkov <alchark@gmail.com>
---
drivers/clocksource/timer-vt8500.c | 65 ++++++++++++++++++++++++--------------
1 file changed, 42 insertions(+), 23 deletions(-)
diff --git a/drivers/clocksource/timer-vt8500.c b/drivers/clocksource/timer-vt8500.c
index a469b1b5f97233202bf01298b9f612e07026c20c..9f28f30dcaf83ab4e9c89952175b0d4c75bd6b40 100644
--- a/drivers/clocksource/timer-vt8500.c
+++ b/drivers/clocksource/timer-vt8500.c
@@ -24,15 +24,31 @@
#define VT8500_TIMER_OFFSET 0x0100
#define VT8500_TIMER_HZ 3000000
-#define TIMER_MATCH_VAL 0x0000
-#define TIMER_COUNT_VAL 0x0010
-#define TIMER_STATUS_VAL 0x0014
-#define TIMER_IER_VAL 0x001c /* interrupt enable */
-#define TIMER_CTRL_VAL 0x0020
-#define TIMER_AS_VAL 0x0024 /* access status */
-#define TIMER_COUNT_R_ACTIVE (1 << 5) /* not ready for read */
-#define TIMER_COUNT_W_ACTIVE (1 << 4) /* not ready for write */
-#define TIMER_MATCH_W_ACTIVE (1 << 0) /* not ready for write */
+
+#define TIMER_MATCH_REG(x) (4 * (x))
+#define TIMER_COUNT_REG 0x0010 /* clocksource counter */
+
+#define TIMER_STATUS_REG 0x0014
+#define TIMER_STATUS_MATCH(x) BIT((x))
+#define TIMER_STATUS_CLEARALL (TIMER_STATUS_MATCH(0) | \
+ TIMER_STATUS_MATCH(1) | \
+ TIMER_STATUS_MATCH(2) | \
+ TIMER_STATUS_MATCH(3))
+
+#define TIMER_WATCHDOG_EN_REG 0x0018
+#define TIMER_WD_EN BIT(0)
+
+#define TIMER_INT_EN_REG 0x001c /* interrupt enable */
+#define TIMER_INT_EN_MATCH(x) BIT((x))
+
+#define TIMER_CTRL_REG 0x0020
+#define TIMER_CTRL_ENABLE BIT(0) /* enable clocksource counter */
+#define TIMER_CTRL_RD_REQ BIT(1) /* request counter read */
+
+#define TIMER_ACC_STS_REG 0x0024 /* access status */
+#define TIMER_ACC_WR_MATCH(x) BIT((x)) /* writing Match (x) value */
+#define TIMER_ACC_WR_COUNTER BIT(4) /* writing clocksource counter */
+#define TIMER_ACC_RD_COUNTER BIT(5) /* reading clocksource counter */
#define msecs_to_loops(t) (loops_per_jiffy / 1000 * HZ * t)
@@ -43,11 +59,12 @@ static void __iomem *regbase;
static u64 vt8500_timer_read(struct clocksource *cs)
{
int loops = msecs_to_loops(10);
- writel(3, regbase + TIMER_CTRL_VAL);
- while ((readl((regbase + TIMER_AS_VAL)) & TIMER_COUNT_R_ACTIVE)
- && --loops)
+
+ writel(TIMER_CTRL_ENABLE | TIMER_CTRL_RD_REQ, regbase + TIMER_CTRL_REG);
+ while (readl(regbase + TIMER_ACC_STS_REG) & TIMER_ACC_RD_COUNTER
+ && --loops)
cpu_relax();
- return readl(regbase + TIMER_COUNT_VAL);
+ return readl(regbase + TIMER_COUNT_REG);
}
static struct clocksource clocksource = {
@@ -63,23 +80,25 @@ static int vt8500_timer_set_next_event(unsigned long cycles,
{
int loops = msecs_to_loops(10);
u64 alarm = clocksource.read(&clocksource) + cycles;
- while ((readl(regbase + TIMER_AS_VAL) & TIMER_MATCH_W_ACTIVE)
- && --loops)
+
+ while (readl(regbase + TIMER_ACC_STS_REG) & TIMER_ACC_WR_MATCH(0)
+ && --loops)
cpu_relax();
- writel((unsigned long)alarm, regbase + TIMER_MATCH_VAL);
+ writel((unsigned long)alarm, regbase + TIMER_MATCH_REG(0));
if ((signed)(alarm - clocksource.read(&clocksource)) <= MIN_OSCR_DELTA)
return -ETIME;
- writel(1, regbase + TIMER_IER_VAL);
+ writel(TIMER_INT_EN_MATCH(0), regbase + TIMER_INT_EN_REG);
return 0;
}
static int vt8500_shutdown(struct clock_event_device *evt)
{
- writel(readl(regbase + TIMER_CTRL_VAL) | 1, regbase + TIMER_CTRL_VAL);
- writel(0, regbase + TIMER_IER_VAL);
+ writel(readl(regbase + TIMER_CTRL_REG) | TIMER_CTRL_ENABLE,
+ regbase + TIMER_CTRL_REG);
+ writel(0, regbase + TIMER_INT_EN_REG);
return 0;
}
@@ -95,7 +114,7 @@ static struct clock_event_device clockevent = {
static irqreturn_t vt8500_timer_interrupt(int irq, void *dev_id)
{
struct clock_event_device *evt = dev_id;
- writel(0xf, regbase + TIMER_STATUS_VAL);
+ writel(TIMER_STATUS_CLEARALL, regbase + TIMER_STATUS_REG);
evt->event_handler(evt);
return IRQ_HANDLED;
@@ -119,9 +138,9 @@ static int __init vt8500_timer_init(struct device_node *np)
return -EINVAL;
}
- writel(1, regbase + TIMER_CTRL_VAL);
- writel(0xf, regbase + TIMER_STATUS_VAL);
- writel(~0, regbase + TIMER_MATCH_VAL);
+ writel(TIMER_CTRL_ENABLE, regbase + TIMER_CTRL_REG);
+ writel(TIMER_STATUS_CLEARALL, regbase + TIMER_STATUS_REG);
+ writel(~0, regbase + TIMER_MATCH_REG(0));
ret = clocksource_register_hz(&clocksource, VT8500_TIMER_HZ);
if (ret) {
--
2.49.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v5 3/4] clocksource/drivers/timer-vt8500: Prepare for watchdog functionality
2025-05-21 13:00 [PATCH v5 0/4] clocksource/drivers/timer-vt8500: clean up and add watchdog function Alexey Charkov
2025-05-21 13:00 ` [PATCH v5 1/4] dt-bindings: timer: via,vt8500-timer: Convert to YAML Alexey Charkov
2025-05-21 13:00 ` [PATCH v5 2/4] clocksource/drivers/timer-vt8500: Add defines for magic constants Alexey Charkov
@ 2025-05-21 13:00 ` Alexey Charkov
2025-05-21 17:24 ` Daniel Lezcano
2025-05-21 13:00 ` [PATCH v5 4/4] watchdog: Add support for VIA/WonderMedia SoC " Alexey Charkov
3 siblings, 1 reply; 10+ messages in thread
From: Alexey Charkov @ 2025-05-21 13:00 UTC (permalink / raw)
To: Krzysztof Kozlowski, Daniel Lezcano, Thomas Gleixner, Rob Herring,
Conor Dooley, Krzysztof Kozlowski, Wim Van Sebroeck,
Guenter Roeck
Cc: linux-arm-kernel, linux-kernel, devicetree, linux-watchdog,
Alexey Charkov
VIA/WonderMedia system timer can generate a watchdog reset when its
clocksource counter matches the value in the match register 0 and
watchdog function is enabled. For this to work, obvously the clock event
device must use a different match register (1~3) and respective interrupt.
Check if at least two interrupts are provided by the device tree, then use
match register 1 for system clock events and reserve match register 0 for
the watchdog. Instantiate an auxiliary device for the watchdog
Signed-off-by: Alexey Charkov <alchark@gmail.com>
---
MAINTAINERS | 1 +
drivers/clocksource/Kconfig | 1 +
drivers/clocksource/timer-vt8500.c | 111 ++++++++++++++++++++++++++++++++++---
include/linux/vt8500-timer.h | 18 ++++++
4 files changed, 122 insertions(+), 9 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index 783e5ee6854b69cca87b6f0763844d28b4b2213f..5362095240627f613638197fda275db6edc16cf7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3447,6 +3447,7 @@ F: drivers/tty/serial/vt8500_serial.c
F: drivers/video/fbdev/vt8500lcdfb.*
F: drivers/video/fbdev/wm8505fb*
F: drivers/video/fbdev/wmt_ge_rops.*
+F: include/linux/vt8500-timer.h
ARM/ZYNQ ARCHITECTURE
M: Michal Simek <michal.simek@amd.com>
diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 487c8525996724fbf9c6e9726dabb478d86513b9..92f071aade10b7c0f0bba4b47dc6228a5e50360f 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -178,6 +178,7 @@ config TEGRA186_TIMER
config VT8500_TIMER
bool "VT8500 timer driver" if COMPILE_TEST
depends on HAS_IOMEM
+ select AUXILIARY_BUS
help
Enables support for the VT8500 driver.
diff --git a/drivers/clocksource/timer-vt8500.c b/drivers/clocksource/timer-vt8500.c
index 9f28f30dcaf83ab4e9c89952175b0d4c75bd6b40..cdea5245f8e41d65b8b9bebad3fe3a55f43a18fa 100644
--- a/drivers/clocksource/timer-vt8500.c
+++ b/drivers/clocksource/timer-vt8500.c
@@ -11,6 +11,7 @@
* Alexey Charkov. Minor changes have been made for Device Tree Support.
*/
+#include <linux/auxiliary_bus.h>
#include <linux/io.h>
#include <linux/irq.h>
#include <linux/interrupt.h>
@@ -22,9 +23,6 @@
#include <linux/of_address.h>
#include <linux/of_irq.h>
-#define VT8500_TIMER_OFFSET 0x0100
-#define VT8500_TIMER_HZ 3000000
-
#define TIMER_MATCH_REG(x) (4 * (x))
#define TIMER_COUNT_REG 0x0010 /* clocksource counter */
@@ -53,8 +51,14 @@
#define msecs_to_loops(t) (loops_per_jiffy / 1000 * HZ * t)
#define MIN_OSCR_DELTA 16
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/vt8500-timer.h>
static void __iomem *regbase;
+static unsigned int sys_timer_ch; /* which match register to use
+ * for the system timer
+ */
static u64 vt8500_timer_read(struct clocksource *cs)
{
@@ -75,21 +79,26 @@ static struct clocksource clocksource = {
.flags = CLOCK_SOURCE_IS_CONTINUOUS,
};
+static u64 vt8500_timer_next(u64 cycles)
+{
+ return clocksource.read(&clocksource) + cycles;
+}
+
static int vt8500_timer_set_next_event(unsigned long cycles,
struct clock_event_device *evt)
{
int loops = msecs_to_loops(10);
- u64 alarm = clocksource.read(&clocksource) + cycles;
+ u64 alarm = vt8500_timer_next(cycles);
- while (readl(regbase + TIMER_ACC_STS_REG) & TIMER_ACC_WR_MATCH(0)
+ while (readl(regbase + TIMER_ACC_STS_REG) & TIMER_ACC_WR_MATCH(sys_timer_ch)
&& --loops)
cpu_relax();
- writel((unsigned long)alarm, regbase + TIMER_MATCH_REG(0));
+ writel((unsigned long)alarm, regbase + TIMER_MATCH_REG(sys_timer_ch));
if ((signed)(alarm - clocksource.read(&clocksource)) <= MIN_OSCR_DELTA)
return -ETIME;
- writel(TIMER_INT_EN_MATCH(0), regbase + TIMER_INT_EN_REG);
+ writel(TIMER_INT_EN_MATCH(sys_timer_ch), regbase + TIMER_INT_EN_REG);
return 0;
}
@@ -131,7 +140,9 @@ static int __init vt8500_timer_init(struct device_node *np)
return -ENXIO;
}
- timer_irq = irq_of_parse_and_map(np, 0);
+ sys_timer_ch = of_irq_count(np) > 1 ? 1 : 0;
+
+ timer_irq = irq_of_parse_and_map(np, sys_timer_ch);
if (!timer_irq) {
pr_err("%s: Missing irq description in Device Tree\n",
__func__);
@@ -140,7 +151,7 @@ static int __init vt8500_timer_init(struct device_node *np)
writel(TIMER_CTRL_ENABLE, regbase + TIMER_CTRL_REG);
writel(TIMER_STATUS_CLEARALL, regbase + TIMER_STATUS_REG);
- writel(~0, regbase + TIMER_MATCH_REG(0));
+ writel(~0, regbase + TIMER_MATCH_REG(sys_timer_ch));
ret = clocksource_register_hz(&clocksource, VT8500_TIMER_HZ);
if (ret) {
@@ -166,4 +177,86 @@ static int __init vt8500_timer_init(struct device_node *np)
return 0;
}
+static void vt8500_timer_aux_uninit(void *data)
+{
+ auxiliary_device_uninit(data);
+}
+
+static void vt8500_timer_aux_delete(void *data)
+{
+ auxiliary_device_delete(data);
+}
+
+static void vt8500_timer_aux_release(struct device *dev)
+{
+ struct auxiliary_device *aux;
+
+ aux = container_of(dev, struct auxiliary_device, dev);
+ kfree(aux);
+}
+
+/*
+ * This probe gets called after the timer is already up and running. This will
+ * create the watchdog device as a child since the registers are shared.
+ */
+static int vt8500_timer_probe(struct platform_device *pdev)
+{
+ struct vt8500_wdt_info *wdt_info;
+ struct device *dev = &pdev->dev;
+ int ret;
+
+ if (!sys_timer_ch) {
+ dev_info(dev, "Not enabling watchdog: only one irq was given");
+ return 0;
+ }
+
+ if (!regbase)
+ return dev_err_probe(dev, -ENOMEM,
+ "Timer not initialized, cannot create watchdog");
+
+ wdt_info = kzalloc(sizeof(*wdt_info), GFP_KERNEL);
+ if (!wdt_info)
+ return dev_err_probe(dev, -ENOMEM,
+ "Failed to allocate vt8500-wdt info");
+
+ wdt_info->timer_next = &vt8500_timer_next;
+ wdt_info->wdt_en = regbase + TIMER_WATCHDOG_EN_REG;
+ wdt_info->wdt_match = regbase + TIMER_MATCH_REG(0);
+ wdt_info->auxdev.name = "vt8500-wdt";
+ wdt_info->auxdev.dev.parent = dev;
+ wdt_info->auxdev.dev.release = &vt8500_timer_aux_release;
+
+ ret = auxiliary_device_init(&wdt_info->auxdev);
+ if (ret) {
+ kfree(wdt_info);
+ return ret;
+ }
+ ret = devm_add_action_or_reset(dev, vt8500_timer_aux_uninit,
+ &wdt_info->auxdev);
+ if (ret)
+ return ret;
+
+ ret = auxiliary_device_add(&wdt_info->auxdev);
+ if (ret)
+ return ret;
+ return devm_add_action_or_reset(dev, vt8500_timer_aux_delete,
+ &wdt_info->auxdev);
+}
+
+static const struct of_device_id vt8500_timer_of_match[] = {
+ { .compatible = "via,vt8500-timer", },
+ {},
+};
+
+static struct platform_driver vt8500_timer_driver = {
+ .probe = vt8500_timer_probe,
+ .driver = {
+ .name = "vt8500-timer",
+ .of_match_table = vt8500_timer_of_match,
+ .suppress_bind_attrs = true,
+ },
+};
+
+builtin_platform_driver(vt8500_timer_driver);
+
TIMER_OF_DECLARE(vt8500, "via,vt8500-timer", vt8500_timer_init);
diff --git a/include/linux/vt8500-timer.h b/include/linux/vt8500-timer.h
new file mode 100644
index 0000000000000000000000000000000000000000..b8e9000495c509e9c8e8f4098d6bd33de27b3ec4
--- /dev/null
+++ b/include/linux/vt8500-timer.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef LINUX_VT8500_TIMER_H_
+#define LINUX_VT8500_TIMER_H_
+
+#include <linux/auxiliary_bus.h>
+#include <linux/io.h>
+#include <linux/types.h>
+
+#define VT8500_TIMER_HZ 3000000
+
+struct vt8500_wdt_info {
+ struct auxiliary_device auxdev;
+ u64 (*timer_next)(u64 cycles);
+ void __iomem *wdt_en;
+ void __iomem *wdt_match;
+};
+
+#endif /* LINUX_VT8500_TIMER_H_ */
--
2.49.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v5 3/4] clocksource/drivers/timer-vt8500: Prepare for watchdog functionality
2025-05-21 13:00 ` [PATCH v5 3/4] clocksource/drivers/timer-vt8500: Prepare for watchdog functionality Alexey Charkov
@ 2025-05-21 17:24 ` Daniel Lezcano
2025-05-22 5:34 ` Alexey Charkov
0 siblings, 1 reply; 10+ messages in thread
From: Daniel Lezcano @ 2025-05-21 17:24 UTC (permalink / raw)
To: Alexey Charkov
Cc: Krzysztof Kozlowski, Thomas Gleixner, Rob Herring, Conor Dooley,
Krzysztof Kozlowski, Wim Van Sebroeck, Guenter Roeck,
linux-arm-kernel, linux-kernel, devicetree, linux-watchdog
On Wed, May 21, 2025 at 05:00:11PM +0400, Alexey Charkov wrote:
> VIA/WonderMedia system timer can generate a watchdog reset when its
> clocksource counter matches the value in the match register 0 and
> watchdog function is enabled. For this to work, obvously the clock event
> device must use a different match register (1~3) and respective interrupt.
>
> Check if at least two interrupts are provided by the device tree, then use
> match register 1 for system clock events and reserve match register 0 for
> the watchdog. Instantiate an auxiliary device for the watchdog
>
> Signed-off-by: Alexey Charkov <alchark@gmail.com>
> ---
> MAINTAINERS | 1 +
> drivers/clocksource/Kconfig | 1 +
> drivers/clocksource/timer-vt8500.c | 111 ++++++++++++++++++++++++++++++++++---
> include/linux/vt8500-timer.h | 18 ++++++
It should endup in include/clocksource/vt8500-timer.h
> 4 files changed, 122 insertions(+), 9 deletions(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 783e5ee6854b69cca87b6f0763844d28b4b2213f..5362095240627f613638197fda275db6edc16cf7 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3447,6 +3447,7 @@ F: drivers/tty/serial/vt8500_serial.c
> F: drivers/video/fbdev/vt8500lcdfb.*
> F: drivers/video/fbdev/wm8505fb*
> F: drivers/video/fbdev/wmt_ge_rops.*
> +F: include/linux/vt8500-timer.h
>
> ARM/ZYNQ ARCHITECTURE
> M: Michal Simek <michal.simek@amd.com>
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index 487c8525996724fbf9c6e9726dabb478d86513b9..92f071aade10b7c0f0bba4b47dc6228a5e50360f 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -178,6 +178,7 @@ config TEGRA186_TIMER
> config VT8500_TIMER
> bool "VT8500 timer driver" if COMPILE_TEST
> depends on HAS_IOMEM
> + select AUXILIARY_BUS
> help
> Enables support for the VT8500 driver.
>
> diff --git a/drivers/clocksource/timer-vt8500.c b/drivers/clocksource/timer-vt8500.c
> index 9f28f30dcaf83ab4e9c89952175b0d4c75bd6b40..cdea5245f8e41d65b8b9bebad3fe3a55f43a18fa 100644
> --- a/drivers/clocksource/timer-vt8500.c
> +++ b/drivers/clocksource/timer-vt8500.c
> @@ -11,6 +11,7 @@
> * Alexey Charkov. Minor changes have been made for Device Tree Support.
> */
>
> +#include <linux/auxiliary_bus.h>
> #include <linux/io.h>
> #include <linux/irq.h>
> #include <linux/interrupt.h>
> @@ -22,9 +23,6 @@
> #include <linux/of_address.h>
> #include <linux/of_irq.h>
>
> -#define VT8500_TIMER_OFFSET 0x0100
> -#define VT8500_TIMER_HZ 3000000
> -
> #define TIMER_MATCH_REG(x) (4 * (x))
> #define TIMER_COUNT_REG 0x0010 /* clocksource counter */
>
> @@ -53,8 +51,14 @@
> #define msecs_to_loops(t) (loops_per_jiffy / 1000 * HZ * t)
>
> #define MIN_OSCR_DELTA 16
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/vt8500-timer.h>
>
> static void __iomem *regbase;
> +static unsigned int sys_timer_ch; /* which match register to use
> + * for the system timer
> + */
The comment format is a bit odd. It would be nicer on top of the
variable.
/*
* Which match register to use for the system timer
*/
> static u64 vt8500_timer_read(struct clocksource *cs)
> {
> @@ -75,21 +79,26 @@ static struct clocksource clocksource = {
> .flags = CLOCK_SOURCE_IS_CONTINUOUS,
> };
>
> +static u64 vt8500_timer_next(u64 cycles)
> +{
> + return clocksource.read(&clocksource) + cycles;
> +}
> +
> static int vt8500_timer_set_next_event(unsigned long cycles,
> struct clock_event_device *evt)
> {
> int loops = msecs_to_loops(10);
> - u64 alarm = clocksource.read(&clocksource) + cycles;
> + u64 alarm = vt8500_timer_next(cycles);
>
> - while (readl(regbase + TIMER_ACC_STS_REG) & TIMER_ACC_WR_MATCH(0)
> + while (readl(regbase + TIMER_ACC_STS_REG) & TIMER_ACC_WR_MATCH(sys_timer_ch)
> && --loops)
> cpu_relax();
> - writel((unsigned long)alarm, regbase + TIMER_MATCH_REG(0));
> + writel((unsigned long)alarm, regbase + TIMER_MATCH_REG(sys_timer_ch));
>
> if ((signed)(alarm - clocksource.read(&clocksource)) <= MIN_OSCR_DELTA)
> return -ETIME;
>
> - writel(TIMER_INT_EN_MATCH(0), regbase + TIMER_INT_EN_REG);
> + writel(TIMER_INT_EN_MATCH(sys_timer_ch), regbase + TIMER_INT_EN_REG);
>
> return 0;
> }
> @@ -131,7 +140,9 @@ static int __init vt8500_timer_init(struct device_node *np)
> return -ENXIO;
> }
>
> - timer_irq = irq_of_parse_and_map(np, 0);
It may be worth to repeat part of what is said in the changelog
> + sys_timer_ch = of_irq_count(np) > 1 ? 1 : 0;
> +
> + timer_irq = irq_of_parse_and_map(np, sys_timer_ch);
> if (!timer_irq) {
> pr_err("%s: Missing irq description in Device Tree\n",
> __func__);
> @@ -140,7 +151,7 @@ static int __init vt8500_timer_init(struct device_node *np)
>
> writel(TIMER_CTRL_ENABLE, regbase + TIMER_CTRL_REG);
> writel(TIMER_STATUS_CLEARALL, regbase + TIMER_STATUS_REG);
> - writel(~0, regbase + TIMER_MATCH_REG(0));
> + writel(~0, regbase + TIMER_MATCH_REG(sys_timer_ch));
>
> ret = clocksource_register_hz(&clocksource, VT8500_TIMER_HZ);
> if (ret) {
> @@ -166,4 +177,86 @@ static int __init vt8500_timer_init(struct device_node *np)
> return 0;
> }
>
> +static void vt8500_timer_aux_uninit(void *data)
> +{
> + auxiliary_device_uninit(data);
> +}
> +
> +static void vt8500_timer_aux_delete(void *data)
> +{
> + auxiliary_device_delete(data);
> +}
> +
> +static void vt8500_timer_aux_release(struct device *dev)
> +{
> + struct auxiliary_device *aux;
> +
> + aux = container_of(dev, struct auxiliary_device, dev);
> + kfree(aux);
That will result in a double kfree because the data belongs to the
wdt_info structure. It is not a pointer allocated. So when the
wdt_info will be freed, it will free the area already freed by this
function.
Please note, a timer should never be unloaded, so not sure if the wdt
should handle the case.
> +}
> +
> +/*
> + * This probe gets called after the timer is already up and running. This will
> + * create the watchdog device as a child since the registers are shared.
> + */
> +static int vt8500_timer_probe(struct platform_device *pdev)
> +{
> + struct vt8500_wdt_info *wdt_info;
> + struct device *dev = &pdev->dev;
> + int ret;
>>>>>
> + if (!sys_timer_ch) {
> + dev_info(dev, "Not enabling watchdog: only one irq was given");
> + return 0;
> + }
> +
> + if (!regbase)
> + return dev_err_probe(dev, -ENOMEM,
> + "Timer not initialized, cannot create watchdog");
The block above seems to be a bit wobbly as it relies on
vt8500_timer_init() to have succeeded.
Why not have vt8500_timer_probe() called by vt8500_timer_init() (with
a proper name like vt8500_timer_wdt_init()) ?
<<<<<
> + wdt_info = kzalloc(sizeof(*wdt_info), GFP_KERNEL);
devm_kzalloc()
> + if (!wdt_info)
> + return dev_err_probe(dev, -ENOMEM,
> + "Failed to allocate vt8500-wdt info");
Is it possible kzalloc to return -EPROBE_DEFER ?
> +
> + wdt_info->timer_next = &vt8500_timer_next;
> + wdt_info->wdt_en = regbase + TIMER_WATCHDOG_EN_REG;
> + wdt_info->wdt_match = regbase + TIMER_MATCH_REG(0);
The two fields above can be merged into one : wdt_info->regbase
Move TIMER_WATCHDOG_EN_REG to the watchdog driver code.
And as TIMER_MATCH_REG(__channel) == 4 * (__channel),
then TIMER_MATCH_REG == 0, so regbase + 0 == regbase
> + wdt_info->auxdev.name = "vt8500-wdt";
> + wdt_info->auxdev.dev.parent = dev;
> + wdt_info->auxdev.dev.release = &vt8500_timer_aux_release;
> +
> + ret = auxiliary_device_init(&wdt_info->auxdev);
> + if (ret) {
> + kfree(wdt_info);
Remove kfree because of devm_kzalloc
> + return ret;
> + }
nit: add line
> + ret = devm_add_action_or_reset(dev, vt8500_timer_aux_uninit,
> + &wdt_info->auxdev);
> + if (ret)
> + return ret;
> +
> + ret = auxiliary_device_add(&wdt_info->auxdev);
> + if (ret)
> + return ret;
nit: add line
> + return devm_add_action_or_reset(dev, vt8500_timer_aux_delete,
> + &wdt_info->auxdev);
> +}
> +
> +static const struct of_device_id vt8500_timer_of_match[] = {
> + { .compatible = "via,vt8500-timer", },
> + {},
> +};
> +
> +static struct platform_driver vt8500_timer_driver = {
> + .probe = vt8500_timer_probe,
> + .driver = {
> + .name = "vt8500-timer",
> + .of_match_table = vt8500_timer_of_match,
> + .suppress_bind_attrs = true,
> + },
> +};
> +
> +builtin_platform_driver(vt8500_timer_driver);
>
>
> TIMER_OF_DECLARE(vt8500, "via,vt8500-timer", vt8500_timer_init);
> diff --git a/include/linux/vt8500-timer.h b/include/linux/vt8500-timer.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..b8e9000495c509e9c8e8f4098d6bd33de27b3ec4
> --- /dev/null
> +++ b/include/linux/vt8500-timer.h
> @@ -0,0 +1,18 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef LINUX_VT8500_TIMER_H_
> +#define LINUX_VT8500_TIMER_H_
> +
> +#include <linux/auxiliary_bus.h>
> +#include <linux/io.h>
> +#include <linux/types.h>
> +
> +#define VT8500_TIMER_HZ 3000000
> +
> +struct vt8500_wdt_info {
> + struct auxiliary_device auxdev;
> + u64 (*timer_next)(u64 cycles);
> + void __iomem *wdt_en;
> + void __iomem *wdt_match;
> +};
> +
> +#endif /* LINUX_VT8500_TIMER_H_ */
>
> --
> 2.49.0
>
--
<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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v5 3/4] clocksource/drivers/timer-vt8500: Prepare for watchdog functionality
2025-05-21 17:24 ` Daniel Lezcano
@ 2025-05-22 5:34 ` Alexey Charkov
0 siblings, 0 replies; 10+ messages in thread
From: Alexey Charkov @ 2025-05-22 5:34 UTC (permalink / raw)
To: Daniel Lezcano
Cc: Krzysztof Kozlowski, Thomas Gleixner, Rob Herring, Conor Dooley,
Krzysztof Kozlowski, Wim Van Sebroeck, Guenter Roeck,
linux-arm-kernel, linux-kernel, devicetree, linux-watchdog
On Wed, May 21, 2025 at 9:25 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On Wed, May 21, 2025 at 05:00:11PM +0400, Alexey Charkov wrote:
> > VIA/WonderMedia system timer can generate a watchdog reset when its
> > clocksource counter matches the value in the match register 0 and
> > watchdog function is enabled. For this to work, obvously the clock event
> > device must use a different match register (1~3) and respective interrupt.
> >
> > Check if at least two interrupts are provided by the device tree, then use
> > match register 1 for system clock events and reserve match register 0 for
> > the watchdog. Instantiate an auxiliary device for the watchdog
> >
> > Signed-off-by: Alexey Charkov <alchark@gmail.com>
> > ---
> > MAINTAINERS | 1 +
> > drivers/clocksource/Kconfig | 1 +
> > drivers/clocksource/timer-vt8500.c | 111 ++++++++++++++++++++++++++++++++++---
> > include/linux/vt8500-timer.h | 18 ++++++
>
> It should endup in include/clocksource/vt8500-timer.h
Noted, will move.
> > 4 files changed, 122 insertions(+), 9 deletions(-)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 783e5ee6854b69cca87b6f0763844d28b4b2213f..5362095240627f613638197fda275db6edc16cf7 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -3447,6 +3447,7 @@ F: drivers/tty/serial/vt8500_serial.c
> > F: drivers/video/fbdev/vt8500lcdfb.*
> > F: drivers/video/fbdev/wm8505fb*
> > F: drivers/video/fbdev/wmt_ge_rops.*
> > +F: include/linux/vt8500-timer.h
> >
> > ARM/ZYNQ ARCHITECTURE
> > M: Michal Simek <michal.simek@amd.com>
> > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> > index 487c8525996724fbf9c6e9726dabb478d86513b9..92f071aade10b7c0f0bba4b47dc6228a5e50360f 100644
> > --- a/drivers/clocksource/Kconfig
> > +++ b/drivers/clocksource/Kconfig
> > @@ -178,6 +178,7 @@ config TEGRA186_TIMER
> > config VT8500_TIMER
> > bool "VT8500 timer driver" if COMPILE_TEST
> > depends on HAS_IOMEM
> > + select AUXILIARY_BUS
> > help
> > Enables support for the VT8500 driver.
> >
> > diff --git a/drivers/clocksource/timer-vt8500.c b/drivers/clocksource/timer-vt8500.c
> > index 9f28f30dcaf83ab4e9c89952175b0d4c75bd6b40..cdea5245f8e41d65b8b9bebad3fe3a55f43a18fa 100644
> > --- a/drivers/clocksource/timer-vt8500.c
> > +++ b/drivers/clocksource/timer-vt8500.c
> > @@ -11,6 +11,7 @@
> > * Alexey Charkov. Minor changes have been made for Device Tree Support.
> > */
> >
> > +#include <linux/auxiliary_bus.h>
> > #include <linux/io.h>
> > #include <linux/irq.h>
> > #include <linux/interrupt.h>
> > @@ -22,9 +23,6 @@
> > #include <linux/of_address.h>
> > #include <linux/of_irq.h>
> >
> > -#define VT8500_TIMER_OFFSET 0x0100
> > -#define VT8500_TIMER_HZ 3000000
> > -
> > #define TIMER_MATCH_REG(x) (4 * (x))
> > #define TIMER_COUNT_REG 0x0010 /* clocksource counter */
> >
> > @@ -53,8 +51,14 @@
> > #define msecs_to_loops(t) (loops_per_jiffy / 1000 * HZ * t)
> >
> > #define MIN_OSCR_DELTA 16
> > +#include <linux/of_platform.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/vt8500-timer.h>
> >
> > static void __iomem *regbase;
> > +static unsigned int sys_timer_ch; /* which match register to use
> > + * for the system timer
> > + */
>
> The comment format is a bit odd. It would be nicer on top of the
> variable.
>
> /*
> * Which match register to use for the system timer
> */
Indeed. Will reformat, thanks!
> > static u64 vt8500_timer_read(struct clocksource *cs)
> > {
> > @@ -75,21 +79,26 @@ static struct clocksource clocksource = {
> > .flags = CLOCK_SOURCE_IS_CONTINUOUS,
> > };
> >
> > +static u64 vt8500_timer_next(u64 cycles)
> > +{
> > + return clocksource.read(&clocksource) + cycles;
> > +}
> > +
> > static int vt8500_timer_set_next_event(unsigned long cycles,
> > struct clock_event_device *evt)
> > {
> > int loops = msecs_to_loops(10);
> > - u64 alarm = clocksource.read(&clocksource) + cycles;
> > + u64 alarm = vt8500_timer_next(cycles);
> >
> > - while (readl(regbase + TIMER_ACC_STS_REG) & TIMER_ACC_WR_MATCH(0)
> > + while (readl(regbase + TIMER_ACC_STS_REG) & TIMER_ACC_WR_MATCH(sys_timer_ch)
> > && --loops)
> > cpu_relax();
> > - writel((unsigned long)alarm, regbase + TIMER_MATCH_REG(0));
> > + writel((unsigned long)alarm, regbase + TIMER_MATCH_REG(sys_timer_ch));
> >
> > if ((signed)(alarm - clocksource.read(&clocksource)) <= MIN_OSCR_DELTA)
> > return -ETIME;
> >
> > - writel(TIMER_INT_EN_MATCH(0), regbase + TIMER_INT_EN_REG);
> > + writel(TIMER_INT_EN_MATCH(sys_timer_ch), regbase + TIMER_INT_EN_REG);
> >
> > return 0;
> > }
> > @@ -131,7 +140,9 @@ static int __init vt8500_timer_init(struct device_node *np)
> > return -ENXIO;
> > }
> >
> > - timer_irq = irq_of_parse_and_map(np, 0);
>
> It may be worth to repeat part of what is said in the changelog
Will do, thanks!
> > + sys_timer_ch = of_irq_count(np) > 1 ? 1 : 0;
> > +
> > + timer_irq = irq_of_parse_and_map(np, sys_timer_ch);
> > if (!timer_irq) {
> > pr_err("%s: Missing irq description in Device Tree\n",
> > __func__);
> > @@ -140,7 +151,7 @@ static int __init vt8500_timer_init(struct device_node *np)
> >
> > writel(TIMER_CTRL_ENABLE, regbase + TIMER_CTRL_REG);
> > writel(TIMER_STATUS_CLEARALL, regbase + TIMER_STATUS_REG);
> > - writel(~0, regbase + TIMER_MATCH_REG(0));
> > + writel(~0, regbase + TIMER_MATCH_REG(sys_timer_ch));
> >
> > ret = clocksource_register_hz(&clocksource, VT8500_TIMER_HZ);
> > if (ret) {
> > @@ -166,4 +177,86 @@ static int __init vt8500_timer_init(struct device_node *np)
> > return 0;
> > }
> >
> > +static void vt8500_timer_aux_uninit(void *data)
> > +{
> > + auxiliary_device_uninit(data);
> > +}
> > +
> > +static void vt8500_timer_aux_delete(void *data)
> > +{
> > + auxiliary_device_delete(data);
> > +}
> > +
> > +static void vt8500_timer_aux_release(struct device *dev)
> > +{
> > + struct auxiliary_device *aux;
> > +
> > + aux = container_of(dev, struct auxiliary_device, dev);
> > + kfree(aux);
>
> That will result in a double kfree because the data belongs to the
> wdt_info structure. It is not a pointer allocated. So when the
> wdt_info will be freed, it will free the area already freed by this
> function.
Hmm, it will probably even work still, due to the fact that auxdev is
the first member of wdt_info. But at least a container_of is required
as long as the wdt_info struct is allocated with plain kzalloc not its
devm_* sibling.
> Please note, a timer should never be unloaded, so not sure if the wdt
> should handle the case.
I think this function is rather meant for freeing the parent allocated
resources when the child unregisters from the auxiliary bus, which may
happen earlier than the parent itself unloading (at least that's what
I'm getting from the auxiliary bus documentation). The auxiliary bus
doesn't even allow a child device to be added without specifying the
release callback, which leads me to believe that manual object
lifecycle management is preferred over devm managed one [1].
[1] https://elixir.bootlin.com/linux/v6.14.6/source/include/linux/auxiliary_bus.h#L22
But it does make me wonder if e.g. unloading the wdt module would
trigger a release here, which might get messy upon loading it again.
> > +}
> > +
> > +/*
> > + * This probe gets called after the timer is already up and running. This will
> > + * create the watchdog device as a child since the registers are shared.
> > + */
> > +static int vt8500_timer_probe(struct platform_device *pdev)
> > +{
> > + struct vt8500_wdt_info *wdt_info;
> > + struct device *dev = &pdev->dev;
> > + int ret;
>
> >>>>>
>
> > + if (!sys_timer_ch) {
> > + dev_info(dev, "Not enabling watchdog: only one irq was given");
> > + return 0;
> > + }
> > +
> > + if (!regbase)
> > + return dev_err_probe(dev, -ENOMEM,
> > + "Timer not initialized, cannot create watchdog");
>
> The block above seems to be a bit wobbly as it relies on
> vt8500_timer_init() to have succeeded.
>
> Why not have vt8500_timer_probe() called by vt8500_timer_init() (with
> a proper name like vt8500_timer_wdt_init()) ?
I need a struct device to hang all the devm_*, dev_* and auxiliary bus
stuff on to. I couldn't find anything readymade in the timers
framework, thus the platform probe function to put something on a bus
first and get a valid dev pointer.
What are the chances of reaching the point of platform devices probing
without a successfully initialized system timer? I have a strong
suspicion that the system will be unusable anyway if vt8500_timer_init
is unsuccessful, given that it's the only clocksource on VT8500. In
which case being unable to initialize a watchdog would be the least of
my concerns :)
> <<<<<
>
> > + wdt_info = kzalloc(sizeof(*wdt_info), GFP_KERNEL);
>
> devm_kzalloc()
I tried to find examples of other auxiliary_device structures
allocated with the managed functions and could only find plain
kzalloc, which makes me wonder if it's appropriate here?
> > + if (!wdt_info)
> > + return dev_err_probe(dev, -ENOMEM,
> > + "Failed to allocate vt8500-wdt info");
>
> Is it possible kzalloc to return -EPROBE_DEFER ?
I don't think so, but I like how dev_err_probe formats its output
including the textual representation of the errno, and also its inline
return. The docs also say it's fine to use even if -EPROBE_DEFER is
not expected [2]
[2] https://elixir.bootlin.com/linux/v6.14.6/source/drivers/base/core.c#L5057
> > +
> > + wdt_info->timer_next = &vt8500_timer_next;
> > + wdt_info->wdt_en = regbase + TIMER_WATCHDOG_EN_REG;
> > + wdt_info->wdt_match = regbase + TIMER_MATCH_REG(0);
>
> The two fields above can be merged into one : wdt_info->regbase
>
> Move TIMER_WATCHDOG_EN_REG to the watchdog driver code.
>
> And as TIMER_MATCH_REG(__channel) == 4 * (__channel),
> then TIMER_MATCH_REG == 0, so regbase + 0 == regbase
Could do that, but frankly I find it neat that the watchdog driver
doesn't need to do offsets into the parent's MMIO registers and just
uses descriptive names for the two registers it actually needs access
to.
> > + wdt_info->auxdev.name = "vt8500-wdt";
> > + wdt_info->auxdev.dev.parent = dev;
> > + wdt_info->auxdev.dev.release = &vt8500_timer_aux_release;
> > +
> > + ret = auxiliary_device_init(&wdt_info->auxdev);
> > + if (ret) {
> > + kfree(wdt_info);
>
> Remove kfree because of devm_kzalloc
>
> > + return ret;
> > + }
>
> nit: add line
>
> > + ret = devm_add_action_or_reset(dev, vt8500_timer_aux_uninit,
> > + &wdt_info->auxdev);
> > + if (ret)
> > + return ret;
> > +
> > + ret = auxiliary_device_add(&wdt_info->auxdev);
> > + if (ret)
> > + return ret;
>
> nit: add line
Thanks for your review Daniel!
Best regards,
Alexey
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v5 4/4] watchdog: Add support for VIA/WonderMedia SoC watchdog functionality
2025-05-21 13:00 [PATCH v5 0/4] clocksource/drivers/timer-vt8500: clean up and add watchdog function Alexey Charkov
` (2 preceding siblings ...)
2025-05-21 13:00 ` [PATCH v5 3/4] clocksource/drivers/timer-vt8500: Prepare for watchdog functionality Alexey Charkov
@ 2025-05-21 13:00 ` Alexey Charkov
2025-05-21 17:24 ` Daniel Lezcano
3 siblings, 1 reply; 10+ messages in thread
From: Alexey Charkov @ 2025-05-21 13:00 UTC (permalink / raw)
To: Krzysztof Kozlowski, Daniel Lezcano, Thomas Gleixner, Rob Herring,
Conor Dooley, Krzysztof Kozlowski, Wim Van Sebroeck,
Guenter Roeck
Cc: linux-arm-kernel, linux-kernel, devicetree, linux-watchdog,
Alexey Charkov
VIA/WonderMedia SoCs can use their system timer's first channel as a
watchdog device which will reset the system if the clocksource counter
matches the value given in its match register 0 and if the watchdog
function is enabled.
Since the watchdog function is tightly coupled to the timer itself, it
is implemented as an auxiliary device of the timer device
Signed-off-by: Alexey Charkov <alchark@gmail.com>
---
MAINTAINERS | 1 +
drivers/watchdog/Kconfig | 15 ++++++++
drivers/watchdog/Makefile | 1 +
drivers/watchdog/vt8500-wdt.c | 88 +++++++++++++++++++++++++++++++++++++++++++
4 files changed, 105 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 5362095240627f613638197fda275db6edc16cf7..97d1842625dbdf7fdca3556260662dab469ed091 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3447,6 +3447,7 @@ F: drivers/tty/serial/vt8500_serial.c
F: drivers/video/fbdev/vt8500lcdfb.*
F: drivers/video/fbdev/wm8505fb*
F: drivers/video/fbdev/wmt_ge_rops.*
+F: drivers/watchdog/vt8500-wdt.c
F: include/linux/vt8500-timer.h
ARM/ZYNQ ARCHITECTURE
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 0d8d37f712e8cfb4bf8156853baa13c23a57d6d9..2e59303306feba7e15a015c2fce25b1290dc4cbc 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1115,6 +1115,21 @@ config SUNPLUS_WATCHDOG
To compile this driver as a module, choose M here: the
module will be called sunplus_wdt.
+config VT8500_WATCHDOG
+ tristate "VIA/WonderMedia VT8500 watchdog support"
+ depends on ARCH_VT8500 || COMPILE_TEST
+ select WATCHDOG_CORE
+ select AUXILIARY_BUS
+ help
+ VIA/WonderMedia SoCs can use their system timer as a hardware
+ watchdog, as long as the first timer channel is free from other
+ uses and respective function is enabled in its registers. To
+ make use of it, say Y here and ensure that the device tree
+ lists at least two interrupts for the VT8500 timer device.
+
+ To compile this driver as a module, choose M here.
+ The module will be called vt8500-wdt.
+
# X86 (i386 + ia64 + x86_64) Architecture
config ACQUIRE_WDT
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index c9482904bf870a085c7fce2a439ac5089b6e6fee..3072786bf226c357102be3734fe6e701f753d45b 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -101,6 +101,7 @@ obj-$(CONFIG_MSC313E_WATCHDOG) += msc313e_wdt.o
obj-$(CONFIG_APPLE_WATCHDOG) += apple_wdt.o
obj-$(CONFIG_SUNPLUS_WATCHDOG) += sunplus_wdt.o
obj-$(CONFIG_MARVELL_GTI_WDT) += marvell_gti_wdt.o
+obj-$(CONFIG_VT8500_WATCHDOG) += vt8500-wdt.o
# X86 (i386 + ia64 + x86_64) Architecture
obj-$(CONFIG_ACQUIRE_WDT) += acquirewdt.o
diff --git a/drivers/watchdog/vt8500-wdt.c b/drivers/watchdog/vt8500-wdt.c
new file mode 100644
index 0000000000000000000000000000000000000000..a47ee714e7c0172e89a31b0d6c064fff338bd5b6
--- /dev/null
+++ b/drivers/watchdog/vt8500-wdt.c
@@ -0,0 +1,88 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (C) 2025 Alexey Charkov <alchark@gmail.com */
+
+#include <linux/auxiliary_bus.h>
+#include <linux/container_of.h>
+#include <linux/io.h>
+#include <linux/limits.h>
+#include <linux/minmax.h>
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/watchdog.h>
+#include <linux/vt8500-timer.h>
+
+static int vt8500_watchdog_start(struct watchdog_device *wdd)
+{
+ struct vt8500_wdt_info *info = watchdog_get_drvdata(wdd);
+ u32 deadline = min(wdd->timeout * 1000, wdd->max_hw_heartbeat_ms);
+
+ /* The deadline is matched against the hardware clocksource counter,
+ * which is a u32 value incrementing at VT8500_TIMER_HZ and continuing
+ * past wraparound. When the return value of timer_next is greater than
+ * U32_MAX then the match should occur after the hardware counter wraps
+ * around, thus we take only the lower 32 bits of timer_next return val
+ */
+ deadline = info->timer_next((u64)deadline * (VT8500_TIMER_HZ / 1000));
+ writel(deadline, info->wdt_match);
+ writel(1, info->wdt_en);
+ return 0;
+}
+
+static int vt8500_watchdog_stop(struct watchdog_device *wdd)
+{
+ struct vt8500_wdt_info *info = watchdog_get_drvdata(wdd);
+
+ writel(0, info->wdt_en);
+ return 0;
+}
+
+static const struct watchdog_ops vt8500_watchdog_ops = {
+ .start = vt8500_watchdog_start,
+ .stop = vt8500_watchdog_stop,
+};
+
+static const struct watchdog_info vt8500_watchdog_info = {
+ .identity = "VIA VT8500 watchdog",
+ .options = WDIOF_MAGICCLOSE |
+ WDIOF_KEEPALIVEPING |
+ WDIOF_SETTIMEOUT,
+};
+
+static int vt8500_wdt_probe(struct auxiliary_device *auxdev,
+ const struct auxiliary_device_id *id)
+{
+ struct vt8500_wdt_info *info;
+ struct watchdog_device *wdd;
+
+ wdd = devm_kzalloc(&auxdev->dev, sizeof(*wdd), GFP_KERNEL);
+ if (!wdd)
+ return -ENOMEM;
+
+ wdd->info = &vt8500_watchdog_info;
+ wdd->ops = &vt8500_watchdog_ops;
+ wdd->max_hw_heartbeat_ms = U32_MAX / (VT8500_TIMER_HZ / 1000);
+ wdd->parent = &auxdev->dev;
+
+ info = container_of(auxdev, struct vt8500_wdt_info, auxdev);
+ watchdog_set_drvdata(wdd, info);
+
+ return devm_watchdog_register_device(&auxdev->dev, wdd);
+}
+
+static const struct auxiliary_device_id vt8500_wdt_ids[] = {
+ { .name = "timer_vt8500.vt8500-wdt" },
+ {},
+};
+
+MODULE_DEVICE_TABLE(auxiliary, my_auxiliary_id_table);
+
+static struct auxiliary_driver vt8500_wdt_driver = {
+ .name = "vt8500-wdt",
+ .probe = vt8500_wdt_probe,
+ .id_table = vt8500_wdt_ids,
+};
+module_auxiliary_driver(vt8500_wdt_driver);
+
+MODULE_AUTHOR("Alexey Charkov <alchark@gmail.com>");
+MODULE_DESCRIPTION("Driver for the VIA VT8500 watchdog timer");
+MODULE_LICENSE("GPL");
--
2.49.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v5 4/4] watchdog: Add support for VIA/WonderMedia SoC watchdog functionality
2025-05-21 13:00 ` [PATCH v5 4/4] watchdog: Add support for VIA/WonderMedia SoC " Alexey Charkov
@ 2025-05-21 17:24 ` Daniel Lezcano
2025-05-22 7:45 ` Alexey Charkov
0 siblings, 1 reply; 10+ messages in thread
From: Daniel Lezcano @ 2025-05-21 17:24 UTC (permalink / raw)
To: Alexey Charkov
Cc: Krzysztof Kozlowski, Thomas Gleixner, Rob Herring, Conor Dooley,
Krzysztof Kozlowski, Wim Van Sebroeck, Guenter Roeck,
linux-arm-kernel, linux-kernel, devicetree, linux-watchdog
On Wed, May 21, 2025 at 05:00:12PM +0400, Alexey Charkov wrote:
> VIA/WonderMedia SoCs can use their system timer's first channel as a
> watchdog device which will reset the system if the clocksource counter
> matches the value given in its match register 0 and if the watchdog
> function is enabled.
>
> Since the watchdog function is tightly coupled to the timer itself, it
> is implemented as an auxiliary device of the timer device
>
> Signed-off-by: Alexey Charkov <alchark@gmail.com>
> ---
> MAINTAINERS | 1 +
> drivers/watchdog/Kconfig | 15 ++++++++
> drivers/watchdog/Makefile | 1 +
> drivers/watchdog/vt8500-wdt.c | 88 +++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 105 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5362095240627f613638197fda275db6edc16cf7..97d1842625dbdf7fdca3556260662dab469ed091 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3447,6 +3447,7 @@ F: drivers/tty/serial/vt8500_serial.c
> F: drivers/video/fbdev/vt8500lcdfb.*
> F: drivers/video/fbdev/wm8505fb*
> F: drivers/video/fbdev/wmt_ge_rops.*
> +F: drivers/watchdog/vt8500-wdt.c
> F: include/linux/vt8500-timer.h
>
> ARM/ZYNQ ARCHITECTURE
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 0d8d37f712e8cfb4bf8156853baa13c23a57d6d9..2e59303306feba7e15a015c2fce25b1290dc4cbc 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1115,6 +1115,21 @@ config SUNPLUS_WATCHDOG
> To compile this driver as a module, choose M here: the
> module will be called sunplus_wdt.
>
> +config VT8500_WATCHDOG
> + tristate "VIA/WonderMedia VT8500 watchdog support"
> + depends on ARCH_VT8500 || COMPILE_TEST
> + select WATCHDOG_CORE
> + select AUXILIARY_BUS
> + help
> + VIA/WonderMedia SoCs can use their system timer as a hardware
> + watchdog, as long as the first timer channel is free from other
> + uses and respective function is enabled in its registers. To
> + make use of it, say Y here and ensure that the device tree
> + lists at least two interrupts for the VT8500 timer device.
> +
> + To compile this driver as a module, choose M here.
> + The module will be called vt8500-wdt.
Module is not supported by the timers. That will change in a very near
future but unloading won't be supported, you should consider tying the
wdt life cycle with the subsystem it is connected to.
> # X86 (i386 + ia64 + x86_64) Architecture
>
> config ACQUIRE_WDT
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index c9482904bf870a085c7fce2a439ac5089b6e6fee..3072786bf226c357102be3734fe6e701f753d45b 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -101,6 +101,7 @@ obj-$(CONFIG_MSC313E_WATCHDOG) += msc313e_wdt.o
> obj-$(CONFIG_APPLE_WATCHDOG) += apple_wdt.o
> obj-$(CONFIG_SUNPLUS_WATCHDOG) += sunplus_wdt.o
> obj-$(CONFIG_MARVELL_GTI_WDT) += marvell_gti_wdt.o
> +obj-$(CONFIG_VT8500_WATCHDOG) += vt8500-wdt.o
>
> # X86 (i386 + ia64 + x86_64) Architecture
> obj-$(CONFIG_ACQUIRE_WDT) += acquirewdt.o
> diff --git a/drivers/watchdog/vt8500-wdt.c b/drivers/watchdog/vt8500-wdt.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..a47ee714e7c0172e89a31b0d6c064fff338bd5b6
> --- /dev/null
> +++ b/drivers/watchdog/vt8500-wdt.c
> @@ -0,0 +1,88 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (C) 2025 Alexey Charkov <alchark@gmail.com */
> +
> +#include <linux/auxiliary_bus.h>
> +#include <linux/container_of.h>
> +#include <linux/io.h>
> +#include <linux/limits.h>
> +#include <linux/minmax.h>
> +#include <linux/module.h>
> +#include <linux/types.h>
> +#include <linux/watchdog.h>
> +#include <linux/vt8500-timer.h>
> +
> +static int vt8500_watchdog_start(struct watchdog_device *wdd)
> +{
> + struct vt8500_wdt_info *info = watchdog_get_drvdata(wdd);
> + u32 deadline = min(wdd->timeout * 1000, wdd->max_hw_heartbeat_ms);
> +
> + /* The deadline is matched against the hardware clocksource counter,
> + * which is a u32 value incrementing at VT8500_TIMER_HZ and continuing
> + * past wraparound. When the return value of timer_next is greater than
> + * U32_MAX then the match should occur after the hardware counter wraps
> + * around, thus we take only the lower 32 bits of timer_next return val
> + */
> + deadline = info->timer_next((u64)deadline * (VT8500_TIMER_HZ / 1000));
> + writel(deadline, info->wdt_match);
> + writel(1, info->wdt_en);
> + return 0;
> +}
> +
> +static int vt8500_watchdog_stop(struct watchdog_device *wdd)
> +{
> + struct vt8500_wdt_info *info = watchdog_get_drvdata(wdd);
> +
> + writel(0, info->wdt_en);
> + return 0;
> +}
> +
> +static const struct watchdog_ops vt8500_watchdog_ops = {
> + .start = vt8500_watchdog_start,
> + .stop = vt8500_watchdog_stop,
> +};
> +
> +static const struct watchdog_info vt8500_watchdog_info = {
> + .identity = "VIA VT8500 watchdog",
> + .options = WDIOF_MAGICCLOSE |
> + WDIOF_KEEPALIVEPING |
> + WDIOF_SETTIMEOUT,
> +};
> +
> +static int vt8500_wdt_probe(struct auxiliary_device *auxdev,
> + const struct auxiliary_device_id *id)
> +{
> + struct vt8500_wdt_info *info;
> + struct watchdog_device *wdd;
> +
> + wdd = devm_kzalloc(&auxdev->dev, sizeof(*wdd), GFP_KERNEL);
> + if (!wdd)
> + return -ENOMEM;
> +
> + wdd->info = &vt8500_watchdog_info;
> + wdd->ops = &vt8500_watchdog_ops;
> + wdd->max_hw_heartbeat_ms = U32_MAX / (VT8500_TIMER_HZ / 1000);
> + wdd->parent = &auxdev->dev;
> +
> + info = container_of(auxdev, struct vt8500_wdt_info, auxdev);
> + watchdog_set_drvdata(wdd, info);
> +
> + return devm_watchdog_register_device(&auxdev->dev, wdd);
> +}
> +
> +static const struct auxiliary_device_id vt8500_wdt_ids[] = {
> + { .name = "timer_vt8500.vt8500-wdt" },
> + {},
> +};
> +
> +MODULE_DEVICE_TABLE(auxiliary, my_auxiliary_id_table);
> +
> +static struct auxiliary_driver vt8500_wdt_driver = {
> + .name = "vt8500-wdt",
> + .probe = vt8500_wdt_probe,
> + .id_table = vt8500_wdt_ids,
> +};
> +module_auxiliary_driver(vt8500_wdt_driver);
> +
> +MODULE_AUTHOR("Alexey Charkov <alchark@gmail.com>");
> +MODULE_DESCRIPTION("Driver for the VIA VT8500 watchdog timer");
> +MODULE_LICENSE("GPL");
>
> --
> 2.49.0
>
--
<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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v5 4/4] watchdog: Add support for VIA/WonderMedia SoC watchdog functionality
2025-05-21 17:24 ` Daniel Lezcano
@ 2025-05-22 7:45 ` Alexey Charkov
0 siblings, 0 replies; 10+ messages in thread
From: Alexey Charkov @ 2025-05-22 7:45 UTC (permalink / raw)
To: Daniel Lezcano
Cc: Krzysztof Kozlowski, Thomas Gleixner, Rob Herring, Conor Dooley,
Krzysztof Kozlowski, Wim Van Sebroeck, Guenter Roeck,
linux-arm-kernel, linux-kernel, devicetree, linux-watchdog
On Wed, May 21, 2025 at 9:24 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On Wed, May 21, 2025 at 05:00:12PM +0400, Alexey Charkov wrote:
> > VIA/WonderMedia SoCs can use their system timer's first channel as a
> > watchdog device which will reset the system if the clocksource counter
> > matches the value given in its match register 0 and if the watchdog
> > function is enabled.
> >
> > Since the watchdog function is tightly coupled to the timer itself, it
> > is implemented as an auxiliary device of the timer device
> >
> > Signed-off-by: Alexey Charkov <alchark@gmail.com>
> > ---
> > MAINTAINERS | 1 +
> > drivers/watchdog/Kconfig | 15 ++++++++
> > drivers/watchdog/Makefile | 1 +
> > drivers/watchdog/vt8500-wdt.c | 88 +++++++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 105 insertions(+)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 5362095240627f613638197fda275db6edc16cf7..97d1842625dbdf7fdca3556260662dab469ed091 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -3447,6 +3447,7 @@ F: drivers/tty/serial/vt8500_serial.c
> > F: drivers/video/fbdev/vt8500lcdfb.*
> > F: drivers/video/fbdev/wm8505fb*
> > F: drivers/video/fbdev/wmt_ge_rops.*
> > +F: drivers/watchdog/vt8500-wdt.c
> > F: include/linux/vt8500-timer.h
> >
> > ARM/ZYNQ ARCHITECTURE
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index 0d8d37f712e8cfb4bf8156853baa13c23a57d6d9..2e59303306feba7e15a015c2fce25b1290dc4cbc 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -1115,6 +1115,21 @@ config SUNPLUS_WATCHDOG
> > To compile this driver as a module, choose M here: the
> > module will be called sunplus_wdt.
> >
> > +config VT8500_WATCHDOG
> > + tristate "VIA/WonderMedia VT8500 watchdog support"
> > + depends on ARCH_VT8500 || COMPILE_TEST
> > + select WATCHDOG_CORE
> > + select AUXILIARY_BUS
> > + help
> > + VIA/WonderMedia SoCs can use their system timer as a hardware
> > + watchdog, as long as the first timer channel is free from other
> > + uses and respective function is enabled in its registers. To
> > + make use of it, say Y here and ensure that the device tree
> > + lists at least two interrupts for the VT8500 timer device.
> > +
> > + To compile this driver as a module, choose M here.
> > + The module will be called vt8500-wdt.
>
> Module is not supported by the timers. That will change in a very near
> future but unloading won't be supported, you should consider tying the
> wdt life cycle with the subsystem it is connected to.
But there's an auxiliary bus between the timer and this module, so it
should be possible to boot a system with the timer initialized as
usual, and then load the watchdog if/when needed. Which also saves a
bit of space in the main kernel image.
Or am I missing anything here?
Best regards,
Alexey
^ permalink raw reply [flat|nested] 10+ messages in thread