| Message ID | 20250515-vt8500-timer-updates-v3-3-2197a1b062bd@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | clocksource/drivers/timer-vt8500: clean up and add watchdog function | expand |
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
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
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);
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(-)