linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/4] clocksource/drivers/timer-vt8500: clean up and add watchdog function
@ 2025-05-15 18:55 Alexey Charkov
  2025-05-15 18:55 ` [PATCH v3 1/4] dt-bindings: timer: via,vt8500-timer: Convert to YAML Alexey Charkov
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Alexey Charkov @ 2025-05-15 18:55 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 named defines for all registers and bits in timer-vt8500.
Move the system events timer from channel 0 to channel 1 when enough
information is provided by the device tree (i.e. more than one IRQ).
Use channel 0 for the system watchdog

Signed-off-by: Alexey Charkov <alchark@gmail.com>
---
Changes in v3:
- Dropped the DTS patch already applied by Krzysztof
- Rebased onto v6.15-rc5 as requested by Daniel
- Split out the watchdog code into a dedicated platform driver, like
  timer-gxp does (thanks Daniel)
- Link to v2: https://lore.kernel.org/r/20250507-vt8500-timer-updates-v2-0-65e5d1b0855e@gmail.com

Changes in v2:
- Included the previously reviewed binding change that is directly related
  to this series as the first patch here (thanks Krzysztof)
- Created a separate config symbol for the watchdog function to let users
  build a kernel without forcing watchdog functionality upon them
  (thanks Krzysztof)
- Link to the previous binding submission: https://lore.kernel.org/all/20250506-via_vt8500_timer_binding-v3-1-88450907503f@gmail.com/
- Link to v1: https://lore.kernel.org/r/20250507-vt8500-timer-updates-v1-0-6b76f7f340a6@gmail.com

---
Alexey Charkov (4):
      dt-bindings: timer: via,vt8500-timer: Convert to YAML
      clocksource/drivers/timer-vt8500: Add defines for magic constants
      clocksource/drivers/timer-vt8500: Prepare for watchdog functionality
      watchdog: Add support for VIA/WonderMedia SoC watchdog functionality

 .../devicetree/bindings/timer/via,vt8500-timer.txt |  15 ---
 .../bindings/timer/via,vt8500-timer.yaml           |  51 +++++++++
 MAINTAINERS                                        |   2 +
 drivers/clocksource/timer-vt8500.c                 | 125 +++++++++++++++++----
 drivers/watchdog/Kconfig                           |  14 +++
 drivers/watchdog/Makefile                          |   1 +
 drivers/watchdog/vt8500-wdt.c                      | 116 +++++++++++++++++++
 7 files changed, 285 insertions(+), 39 deletions(-)
---
base-commit: 92a09c47464d040866cf2b4cd052bc60555185fb
change-id: 20250506-vt8500-timer-updates-44a0d22cd720

Best regards,
-- 
Alexey Charkov <alchark@gmail.com>



^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v3 1/4] dt-bindings: timer: via,vt8500-timer: Convert to YAML
  2025-05-15 18:55 [PATCH v3 0/4] clocksource/drivers/timer-vt8500: clean up and add watchdog function Alexey Charkov
@ 2025-05-15 18:55 ` Alexey Charkov
  2025-05-15 18:55 ` [PATCH v3 2/4] clocksource/drivers/timer-vt8500: Add defines for magic constants Alexey Charkov
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 11+ messages in thread
From: Alexey Charkov @ 2025-05-15 18:55 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 69511c3b2b76fb7090a2a550f4c59a7daf188493..b8b2843491c47182a4fc6c76f5c29b6c411830a6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3434,6 +3434,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] 11+ messages in thread

* [PATCH v3 2/4] clocksource/drivers/timer-vt8500: Add defines for magic constants
  2025-05-15 18:55 [PATCH v3 0/4] clocksource/drivers/timer-vt8500: clean up and add watchdog function Alexey Charkov
  2025-05-15 18:55 ` [PATCH v3 1/4] dt-bindings: timer: via,vt8500-timer: Convert to YAML Alexey Charkov
@ 2025-05-15 18:55 ` Alexey Charkov
  2025-05-15 18:55 ` [PATCH v3 3/4] clocksource/drivers/timer-vt8500: Prepare for watchdog functionality Alexey Charkov
  2025-05-15 18:55 ` [PATCH v3 4/4] watchdog: Add support for VIA/WonderMedia SoC " Alexey Charkov
  3 siblings, 0 replies; 11+ messages in thread
From: Alexey Charkov @ 2025-05-15 18:55 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] 11+ messages in thread

* [PATCH v3 3/4] clocksource/drivers/timer-vt8500: Prepare for watchdog functionality
  2025-05-15 18:55 [PATCH v3 0/4] clocksource/drivers/timer-vt8500: clean up and add watchdog function Alexey Charkov
  2025-05-15 18:55 ` [PATCH v3 1/4] dt-bindings: timer: via,vt8500-timer: Convert to YAML Alexey Charkov
  2025-05-15 18:55 ` [PATCH v3 2/4] clocksource/drivers/timer-vt8500: Add defines for magic constants Alexey Charkov
@ 2025-05-15 18:55 ` Alexey Charkov
  2025-05-18  1:23   ` kernel test robot
  2025-05-15 18:55 ` [PATCH v3 4/4] watchdog: Add support for VIA/WonderMedia SoC " Alexey Charkov
  3 siblings, 1 reply; 11+ messages in thread
From: Alexey Charkov @ 2025-05-15 18:55 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 a platform device for the watchdog and give it
access to the MMIO registers base of the timer through platform data.

Signed-off-by: Alexey Charkov <alchark@gmail.com>
---
 drivers/clocksource/timer-vt8500.c | 68 +++++++++++++++++++++++++++++++++++---
 1 file changed, 63 insertions(+), 5 deletions(-)

diff --git a/drivers/clocksource/timer-vt8500.c b/drivers/clocksource/timer-vt8500.c
index 9f28f30dcaf83ab4e9c89952175b0d4c75bd6b40..122cc046140d5e9edff20ff41a49b4e62064dd40 100644
--- a/drivers/clocksource/timer-vt8500.c
+++ b/drivers/clocksource/timer-vt8500.c
@@ -21,6 +21,8 @@
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
 
 #define VT8500_TIMER_OFFSET	0x0100
 #define VT8500_TIMER_HZ		3000000
@@ -55,6 +57,9 @@
 #define MIN_OSCR_DELTA		16
 
 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)
 {
@@ -81,15 +86,15 @@ 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_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 +136,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 +147,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 +173,55 @@ static int __init vt8500_timer_init(struct device_node *np)
 	return 0;
 }
 
+/*
+ * 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 platform_device *vt8500_watchdog_device;
+	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");
+
+	vt8500_watchdog_device = platform_device_alloc("vt8500-wdt", -1);
+	if (!vt8500_watchdog_device)
+		return dev_err_probe(dev, -ENOMEM,
+			"Failed to allocate vt8500-wdt");
+
+	/* Pass the base address as platform data and nothing else */
+	vt8500_watchdog_device->dev.platform_data = regbase;
+	vt8500_watchdog_device->dev.parent = dev;
+
+	ret = platform_device_add(vt8500_watchdog_device);
+	if (ret)
+		platform_device_put(vt8500_watchdog_device);
+
+	return ret;
+}
+
+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);

-- 
2.49.0



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v3 4/4] watchdog: Add support for VIA/WonderMedia SoC watchdog functionality
  2025-05-15 18:55 [PATCH v3 0/4] clocksource/drivers/timer-vt8500: clean up and add watchdog function Alexey Charkov
                   ` (2 preceding siblings ...)
  2025-05-15 18:55 ` [PATCH v3 3/4] clocksource/drivers/timer-vt8500: Prepare for watchdog functionality Alexey Charkov
@ 2025-05-15 18:55 ` Alexey Charkov
  2025-05-16  6:55   ` kernel test robot
  3 siblings, 1 reply; 11+ messages in thread
From: Alexey Charkov @ 2025-05-15 18:55 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 a child platform device of the timer device, and just
reuses the MMIO registers already remapped by the timer driver. This is
similar to the approach taken by gxp-wdt.c

Signed-off-by: Alexey Charkov <alchark@gmail.com>
---
 MAINTAINERS                   |   1 +
 drivers/watchdog/Kconfig      |  14 +++++
 drivers/watchdog/Makefile     |   1 +
 drivers/watchdog/vt8500-wdt.c | 116 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 132 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index b8b2843491c47182a4fc6c76f5c29b6c411830a6..d809643b76ad299ca1c08804540b08cc4a7184a6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3446,6 +3446,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
 
 ARM/ZYNQ ARCHITECTURE
 M:	Michal Simek <michal.simek@amd.com>
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 0d8d37f712e8cfb4bf8156853baa13c23a57d6d9..429e4e775f1f458fd457067a3a039a5d544b4818 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -2003,6 +2003,20 @@ config PIC32_DMT
 	  To compile this driver as a loadable module, choose M here.
 	  The module will be called pic32-dmt.
 
+config VT8500_WATCHDOG
+	tristate "VIA/WonderMedia VT8500 watchdog support"
+	depends on ARCH_VT8500 || COMPILE_TEST
+	select WATCHDOG_CORE
+	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.
+
 # PARISC Architecture
 
 # POWERPC Architecture
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..8e605c850dd9f42c90104f362b74adac63dd9fdb
--- /dev/null
+++ b/drivers/watchdog/vt8500-wdt.c
@@ -0,0 +1,116 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (C) 2025 Alexey Charkov <alchark@gmail.com */
+
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/types.h>
+#include <linux/watchdog.h>
+
+#define TIMER_MATCH_REG(x)	(4 * (x))
+#define TIMER_COUNT_REG		0x0010	 /* clocksource counter */
+
+#define TIMER_WATCHDOG_EN_REG	0x0018
+#define TIMER_WD_EN		BIT(0)
+
+#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 VT8500_TIMER_HZ		3000000
+#define msecs_to_loops(t) (loops_per_jiffy / 1000 * HZ * t)
+
+struct vt8500_wdt {
+	void __iomem *regbase;
+	struct watchdog_device wdd;
+};
+
+static u64 vt8500_timer_read(void __iomem *regbase)
+{
+	int loops = msecs_to_loops(10);
+
+	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_REG);
+}
+
+static int vt8500_watchdog_start(struct watchdog_device *wdd)
+{
+	struct vt8500_wdt *drvdata = watchdog_get_drvdata(wdd);
+	u64 cycles = wdd->timeout * VT8500_TIMER_HZ;
+	u64 deadline = vt8500_timer_read(drvdata->regbase) + cycles;
+
+	writel((u32)deadline, drvdata->regbase + TIMER_MATCH_REG(0));
+	writel(TIMER_WD_EN, drvdata->regbase + TIMER_WATCHDOG_EN_REG);
+	return 0;
+}
+
+static int vt8500_watchdog_stop(struct watchdog_device *wdd)
+{
+	struct vt8500_wdt *drvdata = watchdog_get_drvdata(wdd);
+
+	writel(0, drvdata->regbase + TIMER_WATCHDOG_EN_REG);
+	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 platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct vt8500_wdt *drvdata;
+
+	drvdata = devm_kzalloc(dev, sizeof(struct vt8500_wdt), GFP_KERNEL);
+	if (!drvdata)
+		return -ENOMEM;
+
+	/*
+	 * The register area where the timer and watchdog reside is disarranged.
+	 * Hence mapping individual register blocks for the timer and watchdog
+	 * is not recommended as they would have access to each others
+	 * registers. The timer driver creates the watchdog as a child device.
+	 * During the watchdogs creation, the timer driver passes the base
+	 * address to the watchdog over the private interface.
+	 */
+
+	drvdata->regbase = (void __iomem *)dev->platform_data;
+
+	drvdata->wdd.info = &vt8500_watchdog_info;
+	drvdata->wdd.ops = &vt8500_watchdog_ops;
+	drvdata->wdd.max_hw_heartbeat_ms = -1UL / (VT8500_TIMER_HZ / 1000);
+	drvdata->wdd.parent = dev;
+
+	watchdog_set_drvdata(&drvdata->wdd, drvdata);
+
+	return devm_watchdog_register_device(dev, &drvdata->wdd);
+}
+
+static struct platform_driver vt8500_wdt_driver = {
+	.probe = vt8500_wdt_probe,
+	.driver = {
+		.name =	"vt8500-wdt",
+	},
+};
+module_platform_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] 11+ messages in thread

* Re: [PATCH v3 4/4] watchdog: Add support for VIA/WonderMedia SoC watchdog functionality
  2025-05-15 18:55 ` [PATCH v3 4/4] watchdog: Add support for VIA/WonderMedia SoC " Alexey Charkov
@ 2025-05-16  6:55   ` kernel test robot
  2025-05-16 13:02     ` Alexey Charkov
  0 siblings, 1 reply; 11+ messages in thread
From: kernel test robot @ 2025-05-16  6:55 UTC (permalink / raw)
  To: Alexey Charkov, Krzysztof Kozlowski, Daniel Lezcano,
	Thomas Gleixner, Rob Herring, Conor Dooley, Wim Van Sebroeck,
	Guenter Roeck
  Cc: oe-kbuild-all, linux-arm-kernel, linux-kernel, devicetree,
	linux-watchdog, Alexey Charkov

Hi Alexey,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 92a09c47464d040866cf2b4cd052bc60555185fb]

url:    https://github.com/intel-lab-lkp/linux/commits/Alexey-Charkov/dt-bindings-timer-via-vt8500-timer-Convert-to-YAML/20250516-025729
base:   92a09c47464d040866cf2b4cd052bc60555185fb
patch link:    https://lore.kernel.org/r/20250515-vt8500-timer-updates-v3-4-2197a1b062bd%40gmail.com
patch subject: [PATCH v3 4/4] watchdog: Add support for VIA/WonderMedia SoC watchdog functionality
config: sparc-allmodconfig (https://download.01.org/0day-ci/archive/20250516/202505161430.wdURc0TG-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250516/202505161430.wdURc0TG-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202505161430.wdURc0TG-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/watchdog/vt8500-wdt.c: In function 'vt8500_wdt_probe':
>> drivers/watchdog/vt8500-wdt.c:98:44: warning: conversion from 'long unsigned int' to 'unsigned int' changes value from '6148914691236517' to '3287081637' [-Woverflow]
      98 |         drvdata->wdd.max_hw_heartbeat_ms = -1UL / (VT8500_TIMER_HZ / 1000);
         |                                            ^


vim +98 drivers/watchdog/vt8500-wdt.c

    75	
    76	static int vt8500_wdt_probe(struct platform_device *pdev)
    77	{
    78		struct device *dev = &pdev->dev;
    79		struct vt8500_wdt *drvdata;
    80	
    81		drvdata = devm_kzalloc(dev, sizeof(struct vt8500_wdt), GFP_KERNEL);
    82		if (!drvdata)
    83			return -ENOMEM;
    84	
    85		/*
    86		 * The register area where the timer and watchdog reside is disarranged.
    87		 * Hence mapping individual register blocks for the timer and watchdog
    88		 * is not recommended as they would have access to each others
    89		 * registers. The timer driver creates the watchdog as a child device.
    90		 * During the watchdogs creation, the timer driver passes the base
    91		 * address to the watchdog over the private interface.
    92		 */
    93	
    94		drvdata->regbase = (void __iomem *)dev->platform_data;
    95	
    96		drvdata->wdd.info = &vt8500_watchdog_info;
    97		drvdata->wdd.ops = &vt8500_watchdog_ops;
  > 98		drvdata->wdd.max_hw_heartbeat_ms = -1UL / (VT8500_TIMER_HZ / 1000);
    99		drvdata->wdd.parent = dev;
   100	
   101		watchdog_set_drvdata(&drvdata->wdd, drvdata);
   102	
   103		return devm_watchdog_register_device(dev, &drvdata->wdd);
   104	}
   105	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 4/4] watchdog: Add support for VIA/WonderMedia SoC watchdog functionality
  2025-05-16  6:55   ` kernel test robot
@ 2025-05-16 13:02     ` Alexey Charkov
  0 siblings, 0 replies; 11+ messages in thread
From: Alexey Charkov @ 2025-05-16 13:02 UTC (permalink / raw)
  To: kernel test robot
  Cc: Krzysztof Kozlowski, Daniel Lezcano, Thomas Gleixner, Rob Herring,
	Conor Dooley, Wim Van Sebroeck, Guenter Roeck, oe-kbuild-all,
	linux-arm-kernel, linux-kernel, devicetree, linux-watchdog

On Fri, May 16, 2025 at 9:56 AM kernel test robot <lkp@intel.com> wrote:
>
> Hi Alexey,
>
> kernel test robot noticed the following build warnings:
>
> [auto build test WARNING on 92a09c47464d040866cf2b4cd052bc60555185fb]
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Alexey-Charkov/dt-bindings-timer-via-vt8500-timer-Convert-to-YAML/20250516-025729
> base:   92a09c47464d040866cf2b4cd052bc60555185fb
> patch link:    https://lore.kernel.org/r/20250515-vt8500-timer-updates-v3-4-2197a1b062bd%40gmail.com
> patch subject: [PATCH v3 4/4] watchdog: Add support for VIA/WonderMedia SoC watchdog functionality
> config: sparc-allmodconfig (https://download.01.org/0day-ci/archive/20250516/202505161430.wdURc0TG-lkp@intel.com/config)
> compiler: sparc64-linux-gcc (GCC) 14.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250516/202505161430.wdURc0TG-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202505161430.wdURc0TG-lkp@intel.com/
>
> All warnings (new ones prefixed by >>):
>
>    drivers/watchdog/vt8500-wdt.c: In function 'vt8500_wdt_probe':
> >> drivers/watchdog/vt8500-wdt.c:98:44: warning: conversion from 'long unsigned int' to 'unsigned int' changes value from '6148914691236517' to '3287081637' [-Woverflow]
>       98 |         drvdata->wdd.max_hw_heartbeat_ms = -1UL / (VT8500_TIMER_HZ / 1000);

Will change to U32_MAX in the next version.

Best regards,
Alexey


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 3/4] clocksource/drivers/timer-vt8500: Prepare for watchdog functionality
  2025-05-15 18:55 ` [PATCH v3 3/4] clocksource/drivers/timer-vt8500: Prepare for watchdog functionality Alexey Charkov
@ 2025-05-18  1:23   ` kernel test robot
  2025-05-19 11:34     ` Alexey Charkov
  0 siblings, 1 reply; 11+ messages in thread
From: kernel test robot @ 2025-05-18  1:23 UTC (permalink / raw)
  To: Alexey Charkov, Krzysztof Kozlowski, Daniel Lezcano,
	Thomas Gleixner, Rob Herring, Conor Dooley, Wim Van Sebroeck,
	Guenter Roeck
  Cc: oe-kbuild-all, linux-arm-kernel, linux-kernel, devicetree,
	linux-watchdog, Alexey Charkov

Hi Alexey,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 92a09c47464d040866cf2b4cd052bc60555185fb]

url:    https://github.com/intel-lab-lkp/linux/commits/Alexey-Charkov/dt-bindings-timer-via-vt8500-timer-Convert-to-YAML/20250516-025729
base:   92a09c47464d040866cf2b4cd052bc60555185fb
patch link:    https://lore.kernel.org/r/20250515-vt8500-timer-updates-v3-3-2197a1b062bd%40gmail.com
patch subject: [PATCH v3 3/4] clocksource/drivers/timer-vt8500: Prepare for watchdog functionality
config: loongarch-randconfig-r123-20250517 (https://download.01.org/0day-ci/archive/20250518/202505180911.hDevFA1N-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 14.2.0
reproduce: (https://download.01.org/0day-ci/archive/20250518/202505180911.hDevFA1N-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202505180911.hDevFA1N-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/clocksource/timer-vt8500.c:201:51: sparse: sparse: incorrect type in assignment (different address spaces) @@     expected void *platform_data @@     got void [noderef] __iomem *static [assigned] [toplevel] regbase @@
   drivers/clocksource/timer-vt8500.c:201:51: sparse:     expected void *platform_data
   drivers/clocksource/timer-vt8500.c:201:51: sparse:     got void [noderef] __iomem *static [assigned] [toplevel] regbase

vim +201 drivers/clocksource/timer-vt8500.c

   175	
   176	/*
   177	 * This probe gets called after the timer is already up and running. This will create
   178	 * the watchdog device as a child since the registers are shared.
   179	 */
   180	static int vt8500_timer_probe(struct platform_device *pdev)
   181	{
   182		struct platform_device *vt8500_watchdog_device;
   183		struct device *dev = &pdev->dev;
   184		int ret;
   185	
   186		if (!sys_timer_ch) {
   187			dev_info(dev, "Not enabling watchdog: only one irq was given");
   188			return 0;
   189		}
   190	
   191		if (!regbase)
   192			return dev_err_probe(dev, -ENOMEM,
   193				"Timer not initialized, cannot create watchdog");
   194	
   195		vt8500_watchdog_device = platform_device_alloc("vt8500-wdt", -1);
   196		if (!vt8500_watchdog_device)
   197			return dev_err_probe(dev, -ENOMEM,
   198				"Failed to allocate vt8500-wdt");
   199	
   200		/* Pass the base address as platform data and nothing else */
 > 201		vt8500_watchdog_device->dev.platform_data = regbase;
   202		vt8500_watchdog_device->dev.parent = dev;
   203	
   204		ret = platform_device_add(vt8500_watchdog_device);
   205		if (ret)
   206			platform_device_put(vt8500_watchdog_device);
   207	
   208		return ret;
   209	}
   210	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 3/4] clocksource/drivers/timer-vt8500: Prepare for watchdog functionality
  2025-05-18  1:23   ` kernel test robot
@ 2025-05-19 11:34     ` Alexey Charkov
  2025-05-19 13:34       ` Guenter Roeck
  0 siblings, 1 reply; 11+ messages in thread
From: Alexey Charkov @ 2025-05-19 11:34 UTC (permalink / raw)
  To: kernel test robot
  Cc: Krzysztof Kozlowski, Daniel Lezcano, Thomas Gleixner, Rob Herring,
	Conor Dooley, Wim Van Sebroeck, Guenter Roeck, oe-kbuild-all,
	linux-arm-kernel, linux-kernel, devicetree, linux-watchdog

On Sun, May 18, 2025 at 5:24 AM kernel test robot <lkp@intel.com> wrote:
>
> Hi Alexey,
>
> kernel test robot noticed the following build warnings:
>
> [auto build test WARNING on 92a09c47464d040866cf2b4cd052bc60555185fb]
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Alexey-Charkov/dt-bindings-timer-via-vt8500-timer-Convert-to-YAML/20250516-025729
> base:   92a09c47464d040866cf2b4cd052bc60555185fb
> patch link:    https://lore.kernel.org/r/20250515-vt8500-timer-updates-v3-3-2197a1b062bd%40gmail.com
> patch subject: [PATCH v3 3/4] clocksource/drivers/timer-vt8500: Prepare for watchdog functionality
> config: loongarch-randconfig-r123-20250517 (https://download.01.org/0day-ci/archive/20250518/202505180911.hDevFA1N-lkp@intel.com/config)
> compiler: loongarch64-linux-gcc (GCC) 14.2.0
> reproduce: (https://download.01.org/0day-ci/archive/20250518/202505180911.hDevFA1N-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202505180911.hDevFA1N-lkp@intel.com/
>
> sparse warnings: (new ones prefixed by >>)
> >> drivers/clocksource/timer-vt8500.c:201:51: sparse: sparse: incorrect type in assignment (different address spaces) @@     expected void *platform_data @@     got void [noderef] __iomem *static [assigned] [toplevel] regbase @@
>    drivers/clocksource/timer-vt8500.c:201:51: sparse:     expected void *platform_data
>    drivers/clocksource/timer-vt8500.c:201:51: sparse:     got void [noderef] __iomem *static [assigned] [toplevel] regbase
>
> vim +201 drivers/clocksource/timer-vt8500.c
>
>    175
>    176  /*
>    177   * This probe gets called after the timer is already up and running. This will create
>    178   * the watchdog device as a child since the registers are shared.
>    179   */
>    180  static int vt8500_timer_probe(struct platform_device *pdev)
>    181  {
>    182          struct platform_device *vt8500_watchdog_device;
>    183          struct device *dev = &pdev->dev;
>    184          int ret;
>    185
>    186          if (!sys_timer_ch) {
>    187                  dev_info(dev, "Not enabling watchdog: only one irq was given");
>    188                  return 0;
>    189          }
>    190
>    191          if (!regbase)
>    192                  return dev_err_probe(dev, -ENOMEM,
>    193                          "Timer not initialized, cannot create watchdog");
>    194
>    195          vt8500_watchdog_device = platform_device_alloc("vt8500-wdt", -1);
>    196          if (!vt8500_watchdog_device)
>    197                  return dev_err_probe(dev, -ENOMEM,
>    198                          "Failed to allocate vt8500-wdt");
>    199
>    200          /* Pass the base address as platform data and nothing else */
>  > 201          vt8500_watchdog_device->dev.platform_data = regbase;

Frankly, given that this driver only applies to VT8500 (which is ARM
based), the warning appears a bit overzealous. After all, on ARM MMIO
addresses are in the same physical address space as normal memory
addresses, and furthermore this platform_data is never dereferenced
directly anyway.

I could silence the warning either by more aggressive casting or by
wrapping the pointer into some struct, but both of those sound a bit
overreaching. Would appreciate guidance from the list on how to best
approach this.

Best regards,
Alexey


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 3/4] clocksource/drivers/timer-vt8500: Prepare for watchdog functionality
  2025-05-19 11:34     ` Alexey Charkov
@ 2025-05-19 13:34       ` Guenter Roeck
  2025-05-20 17:39         ` Alexey Charkov
  0 siblings, 1 reply; 11+ messages in thread
From: Guenter Roeck @ 2025-05-19 13:34 UTC (permalink / raw)
  To: Alexey Charkov, kernel test robot
  Cc: Krzysztof Kozlowski, Daniel Lezcano, Thomas Gleixner, Rob Herring,
	Conor Dooley, Wim Van Sebroeck, oe-kbuild-all, linux-arm-kernel,
	linux-kernel, devicetree, linux-watchdog

On 5/19/25 04:34, Alexey Charkov wrote:
> On Sun, May 18, 2025 at 5:24 AM kernel test robot <lkp@intel.com> wrote:
>>
>> Hi Alexey,
>>
>> kernel test robot noticed the following build warnings:
>>
>> [auto build test WARNING on 92a09c47464d040866cf2b4cd052bc60555185fb]
>>
>> url:    https://github.com/intel-lab-lkp/linux/commits/Alexey-Charkov/dt-bindings-timer-via-vt8500-timer-Convert-to-YAML/20250516-025729
>> base:   92a09c47464d040866cf2b4cd052bc60555185fb
>> patch link:    https://lore.kernel.org/r/20250515-vt8500-timer-updates-v3-3-2197a1b062bd%40gmail.com
>> patch subject: [PATCH v3 3/4] clocksource/drivers/timer-vt8500: Prepare for watchdog functionality
>> config: loongarch-randconfig-r123-20250517 (https://download.01.org/0day-ci/archive/20250518/202505180911.hDevFA1N-lkp@intel.com/config)
>> compiler: loongarch64-linux-gcc (GCC) 14.2.0
>> reproduce: (https://download.01.org/0day-ci/archive/20250518/202505180911.hDevFA1N-lkp@intel.com/reproduce)
>>
>> If you fix the issue in a separate patch/commit (i.e. not just a new version of
>> the same patch/commit), kindly add following tags
>> | Reported-by: kernel test robot <lkp@intel.com>
>> | Closes: https://lore.kernel.org/oe-kbuild-all/202505180911.hDevFA1N-lkp@intel.com/
>>
>> sparse warnings: (new ones prefixed by >>)
>>>> drivers/clocksource/timer-vt8500.c:201:51: sparse: sparse: incorrect type in assignment (different address spaces) @@     expected void *platform_data @@     got void [noderef] __iomem *static [assigned] [toplevel] regbase @@
>>     drivers/clocksource/timer-vt8500.c:201:51: sparse:     expected void *platform_data
>>     drivers/clocksource/timer-vt8500.c:201:51: sparse:     got void [noderef] __iomem *static [assigned] [toplevel] regbase
>>
>> vim +201 drivers/clocksource/timer-vt8500.c
>>
>>     175
>>     176  /*
>>     177   * This probe gets called after the timer is already up and running. This will create
>>     178   * the watchdog device as a child since the registers are shared.
>>     179   */
>>     180  static int vt8500_timer_probe(struct platform_device *pdev)
>>     181  {
>>     182          struct platform_device *vt8500_watchdog_device;
>>     183          struct device *dev = &pdev->dev;
>>     184          int ret;
>>     185
>>     186          if (!sys_timer_ch) {
>>     187                  dev_info(dev, "Not enabling watchdog: only one irq was given");
>>     188                  return 0;
>>     189          }
>>     190
>>     191          if (!regbase)
>>     192                  return dev_err_probe(dev, -ENOMEM,
>>     193                          "Timer not initialized, cannot create watchdog");
>>     194
>>     195          vt8500_watchdog_device = platform_device_alloc("vt8500-wdt", -1);
>>     196          if (!vt8500_watchdog_device)
>>     197                  return dev_err_probe(dev, -ENOMEM,
>>     198                          "Failed to allocate vt8500-wdt");
>>     199
>>     200          /* Pass the base address as platform data and nothing else */
>>   > 201          vt8500_watchdog_device->dev.platform_data = regbase;
> 
> Frankly, given that this driver only applies to VT8500 (which is ARM
> based), the warning appears a bit overzealous. After all, on ARM MMIO
> addresses are in the same physical address space as normal memory
> addresses, and furthermore this platform_data is never dereferenced
> directly anyway.

Guess we'll need AI compilers in the future to help them know that.
I for my part would argue that "this warning can be ignored" is the
source of many problems flying under the radar.

> 
> I could silence the warning either by more aggressive casting or by
> wrapping the pointer into some struct, but both of those sound a bit
> overreaching. Would appreciate guidance from the list on how to best
> approach this.
> 

First of all, I am quite sure that using platform drivers for this is the
wrong approach to start with. This seems to be a perfect candidate for
an auxiliary driver.

Second, I do consider passing an iomem pointer as platform data to be
inherently unsafe. I would very much prefer either passing a regmap
pointer or, if that doesn't work, a data structure.

Guenter



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 3/4] clocksource/drivers/timer-vt8500: Prepare for watchdog functionality
  2025-05-19 13:34       ` Guenter Roeck
@ 2025-05-20 17:39         ` Alexey Charkov
  0 siblings, 0 replies; 11+ messages in thread
From: Alexey Charkov @ 2025-05-20 17:39 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: kernel test robot, Krzysztof Kozlowski, Daniel Lezcano,
	Thomas Gleixner, Rob Herring, Conor Dooley, Wim Van Sebroeck,
	oe-kbuild-all, linux-arm-kernel, linux-kernel, devicetree,
	linux-watchdog

On Mon, May 19, 2025 at 5:34 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 5/19/25 04:34, Alexey Charkov wrote:
> > On Sun, May 18, 2025 at 5:24 AM kernel test robot <lkp@intel.com> wrote:
> >>
> >> Hi Alexey,
> >>
> >> kernel test robot noticed the following build warnings:
> >>
> >> [auto build test WARNING on 92a09c47464d040866cf2b4cd052bc60555185fb]
> >>
> >> url:    https://github.com/intel-lab-lkp/linux/commits/Alexey-Charkov/dt-bindings-timer-via-vt8500-timer-Convert-to-YAML/20250516-025729
> >> base:   92a09c47464d040866cf2b4cd052bc60555185fb
> >> patch link:    https://lore.kernel.org/r/20250515-vt8500-timer-updates-v3-3-2197a1b062bd%40gmail.com
> >> patch subject: [PATCH v3 3/4] clocksource/drivers/timer-vt8500: Prepare for watchdog functionality
> >> config: loongarch-randconfig-r123-20250517 (https://download.01.org/0day-ci/archive/20250518/202505180911.hDevFA1N-lkp@intel.com/config)
> >> compiler: loongarch64-linux-gcc (GCC) 14.2.0
> >> reproduce: (https://download.01.org/0day-ci/archive/20250518/202505180911.hDevFA1N-lkp@intel.com/reproduce)
> >>
> >> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> >> the same patch/commit), kindly add following tags
> >> | Reported-by: kernel test robot <lkp@intel.com>
> >> | Closes: https://lore.kernel.org/oe-kbuild-all/202505180911.hDevFA1N-lkp@intel.com/
> >>
> >> sparse warnings: (new ones prefixed by >>)
> >>>> drivers/clocksource/timer-vt8500.c:201:51: sparse: sparse: incorrect type in assignment (different address spaces) @@     expected void *platform_data @@     got void [noderef] __iomem *static [assigned] [toplevel] regbase @@
> >>     drivers/clocksource/timer-vt8500.c:201:51: sparse:     expected void *platform_data
> >>     drivers/clocksource/timer-vt8500.c:201:51: sparse:     got void [noderef] __iomem *static [assigned] [toplevel] regbase
> >>
> >> vim +201 drivers/clocksource/timer-vt8500.c
> >>
> >>     175
> >>     176  /*
> >>     177   * This probe gets called after the timer is already up and running. This will create
> >>     178   * the watchdog device as a child since the registers are shared.
> >>     179   */
> >>     180  static int vt8500_timer_probe(struct platform_device *pdev)
> >>     181  {
> >>     182          struct platform_device *vt8500_watchdog_device;
> >>     183          struct device *dev = &pdev->dev;
> >>     184          int ret;
> >>     185
> >>     186          if (!sys_timer_ch) {
> >>     187                  dev_info(dev, "Not enabling watchdog: only one irq was given");
> >>     188                  return 0;
> >>     189          }
> >>     190
> >>     191          if (!regbase)
> >>     192                  return dev_err_probe(dev, -ENOMEM,
> >>     193                          "Timer not initialized, cannot create watchdog");
> >>     194
> >>     195          vt8500_watchdog_device = platform_device_alloc("vt8500-wdt", -1);
> >>     196          if (!vt8500_watchdog_device)
> >>     197                  return dev_err_probe(dev, -ENOMEM,
> >>     198                          "Failed to allocate vt8500-wdt");
> >>     199
> >>     200          /* Pass the base address as platform data and nothing else */
> >>   > 201          vt8500_watchdog_device->dev.platform_data = regbase;
> >
> > Frankly, given that this driver only applies to VT8500 (which is ARM
> > based), the warning appears a bit overzealous. After all, on ARM MMIO
> > addresses are in the same physical address space as normal memory
> > addresses, and furthermore this platform_data is never dereferenced
> > directly anyway.
>
> Guess we'll need AI compilers in the future to help them know that.
> I for my part would argue that "this warning can be ignored" is the
> source of many problems flying under the radar.
>
> >
> > I could silence the warning either by more aggressive casting or by
> > wrapping the pointer into some struct, but both of those sound a bit
> > overreaching. Would appreciate guidance from the list on how to best
> > approach this.
> >
>
> First of all, I am quite sure that using platform drivers for this is the
> wrong approach to start with. This seems to be a perfect candidate for
> an auxiliary driver.

TIL: auxiliary bus :)

Thanks for the pointer Guenter, it does indeed look like a more
appropriate choice. I'll try and port the driver to use that instead,
and resubmit.

> Second, I do consider passing an iomem pointer as platform data to be
> inherently unsafe. I would very much prefer either passing a regmap
> pointer or, if that doesn't work, a data structure.

I guess it resolves itself with the auxiliary driver approach, as I
can then just upcast the auxiliary device pointer to the parent's
enclosing private struct, which can then contain both a timer read
function and the specific pointers to the two registers the watchdog
needs. No need then for the child to do its arbitrary offsets into the
parent's iomem region - just use what's given directly. It's still
going to be iomem pointer access, but on the other hand putting a
layer of indirection (i.e. regmap) into the system timer code sounds a
bit scary to me.

Best regards,
Alexey


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2025-05-20 17:44 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-05-15 18:55 [PATCH v3 0/4] clocksource/drivers/timer-vt8500: clean up and add watchdog function Alexey Charkov
2025-05-15 18:55 ` [PATCH v3 1/4] dt-bindings: timer: via,vt8500-timer: Convert to YAML Alexey Charkov
2025-05-15 18:55 ` [PATCH v3 2/4] clocksource/drivers/timer-vt8500: Add defines for magic constants Alexey Charkov
2025-05-15 18:55 ` [PATCH v3 3/4] clocksource/drivers/timer-vt8500: Prepare for watchdog functionality Alexey Charkov
2025-05-18  1:23   ` kernel test robot
2025-05-19 11:34     ` Alexey Charkov
2025-05-19 13:34       ` Guenter Roeck
2025-05-20 17:39         ` Alexey Charkov
2025-05-15 18:55 ` [PATCH v3 4/4] watchdog: Add support for VIA/WonderMedia SoC " Alexey Charkov
2025-05-16  6:55   ` kernel test robot
2025-05-16 13:02     ` Alexey Charkov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).