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.0Hi 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
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
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
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
© 2016 - 2025 Red Hat, Inc.