Message-ID: <CABjd4YzQgjwYgTHCTz9_2T0Hr3oXu4SZjxjjbVPk4UjKoLC2TA@mail.gmail.com>
Date: Thu, 22 May 2025 09:34:09 +0400
From: Alexey Charkov <alchark@...il.com>
To: Daniel Lezcano <daniel.lezcano@...aro.org>
Cc: Krzysztof Kozlowski <krzk@...nel.org>, Thomas Gleixner <tglx@...utronix.de>, Rob Herring <robh@...nel.org>, 
	Conor Dooley <conor+dt@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, 
	Wim Van Sebroeck <wim@...ux-watchdog.org>, Guenter Roeck <linux@...ck-us.net>, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-watchdog@...r.kernel.org
Subject: Re: [PATCH v5 3/4] clocksource/drivers/timer-vt8500: Prepare for
 watchdog functionality

On Wed, May 21, 2025 at 9:25 PM Daniel Lezcano
<daniel.lezcano@...aro.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@...il.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@....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

Powered by blists - more mailing lists