Re: [PATCH v2 2/4] clocksource/drivers: Add support for the Renesas RZ/N1 timers

[Geert Uytterhoeven]

Hi Hervé,

Thanks for your patch!

Just a few generic comments, as I am not a timer expert, and have no
access to the hardware.

On Wed, 29 Apr 2026 at 13:51, Herve Codina (Schneider Electric)
<herve.codina@xxxxxxxxxxx> wrote:
> The Renesas RZ/N1 timer block controller is the controller in charge of
> timers available in the Renesas RZ/N1 SoCs family.
>
> This controller handles 8 timers:
>   - 6 16-bit timers
>   - 2 32-bit timers
>
> Each timer has its own interrupt, its own prescaler that can be used to
> device the clock by 25 and all of them can work in either one-shot or

divide

> periodic mode.
>
> Signed-off-by: Herve Codina (Schneider Electric) <herve.codina@xxxxxxxxxxx>
> ---
>  drivers/clocksource/Kconfig      |  10 +
>  drivers/clocksource/Makefile     |   1 +
>  drivers/clocksource/timer-rzn1.c | 442 +++++++++++++++++++++++++++++++
>  3 files changed, 453 insertions(+)
>  create mode 100644 drivers/clocksource/timer-rzn1.c
>
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index d1a33a231a44..f8e49a4ac8f6 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -152,6 +152,16 @@ config REALTEK_OTTO_TIMER
>           RT8391, RTL8392, RTL8393 and RTL8396 and chips of the RTL930x series
>           such as RTL9301, RTL9302 or RTL9303.
>
> +config RZN1_TIMER
> +       bool "Renesas RZ/N1 Timer"

So this cannot be a module.

> +       depends on HAS_IOMEM && COMMON_CLK && (ARCH_RZN1 || COMPILE_TEST)

I would split this in two lines, to make it easier to read (and
maintain, if the same timer ends up in a different SoC series).

> +       help
> +         Enables support for RZ/N1 SoC timers.
> +         A timers block in RZ/N1 SoCs is composed of 8 timers
> +           - 6 16-bit timers
> +           - 2 32-bit timers
> +         Two timers blocks are available in RZ/N1 SoCs.
> +
>  config SUN4I_TIMER
>         bool "Sun4i timer driver" if COMPILE_TEST
>         depends on HAS_IOMEM

> --- /dev/null
> +++ b/drivers/clocksource/timer-rzn1.c

> +/*
> + * 8 timers are available. Among those 8 timers, the first 6 timers are 16-bit
> + * timers and the last two ones are 32-bit timers.
> + */
> +#define RZN1_TIMER_BASE_INDEX_16BIT_TIMERS 0
> +#define RZN1_TIMER_NB_16BIT_TIMERS 6
> +
> +#define RZN1_TIMER_BASE_INDEX_32BIT_TIMER 6
> +#define RZN1_TIMER_NB_32BIT_TIMERS 2

Align the number columns?

> +static int rzn1_timer_probe_first(struct platform_device *pdev, struct rzn1_timer *tab_timers,
> +                                 void __iomem *base, unsigned long clock_rate)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct rzn1_timer *timer;
> +       unsigned int i;
> +       char *name;
> +       int irq;
> +       int ret;
> +
> +       /*
> +        * Probe the first instance. In that case, timers are assigned as
> +        * follow:
> +        *   - First 16-bit timer: clocksource and sched_clock
> +        *   - Other 16-bit timers: clock events for all possible CPUs
> +        *   - 32-bit timers: clock events per CPU
> +        *
> +        * First step, perform all operation that could fail without calling
> +        * clockevents_config_and_register(), sched_clock_register() nor
> +        * cpuhp_setup_state(). Those operation don't have unregister nor
> +        * teardown counterparts and so, once called, we cannot remove the
> +        * related resource.
> +        */
> +
> +       /*
> +        * First step for 16-bit timers except the first one and all 32-bit
> +        * timers.
> +        */
> +       for (i = RZN1_TIMER_BASE_INDEX_16BIT_TIMERS + 1; i < RZN1_TIMER_NB_TIMERS; i++) {
> +               timer = &tab_timers[i];
> +
> +               rzn1_timer_init(timer, i, base, clock_rate);
> +
> +               irq = platform_get_irq(pdev, i);
> +               if (irq < 0)
> +                       return irq;
> +
> +               name = devm_kasprintf(dev, GFP_KERNEL, "%s-%u", dev_name(dev), i);
> +               if (!name)
> +                       return -ENOMEM;
> +
> +               rzn1_timer_clkevt_init_ced(timer, name, irq);
> +
> +               ret = devm_request_irq(dev, timer->ced.irq, rzn1_timer_interrupt,
> +                                      IRQF_TIMER, timer->ced.name, timer);
> +               if (ret < 0)
> +                       return dev_err_probe(dev, irq, "timer%d: Failed to request IRQ\n", i);

%u

> +
> +               rzn1_timer_int_enable(timer);
> +       }
> +
> +       /*
> +        * Second step, almost all operations that can fail have been called.
> +        * Timers are ready to work. Start with the last operation that can fail,
> +        * installing and invoking hotplug callbacks
> +        */
> +       rzn1_tab_timers = tab_timers;
> +       ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
> +                               "clockevents/rzn1/global_timer:starting",
> +                               rzn1_local_timer_starting_cpu, NULL);
> +       if (ret < 0)
> +               return dev_err_probe(dev, ret, "Setup CPU hotplug state failed\n");
> +
> +       /*
> +        * At this point, no more operations can fail. Perform remaining
> +        * operations. Starting by handling the first 16-bit timer
> +        */
> +       timer = &tab_timers[RZN1_TIMER_BASE_INDEX_16BIT_TIMERS];
> +
> +       rzn1_timer_init(timer, RZN1_TIMER_BASE_INDEX_16BIT_TIMERS, base, clock_rate);
> +       rzn1_timer_config(timer, GENMASK(timer->width - 1, 0), true);
> +
> +       rzn1_sched_clock = timer;
> +
> +       sched_clock_register(rzn1_sched_read, rzn1_sched_clock->width, rzn1_sched_clock->rate);
> +
> +       rzn1_clocksource.mask = CLOCKSOURCE_MASK(rzn1_sched_clock->width);
> +       clocksource_register_hz(&rzn1_clocksource, rzn1_sched_clock->rate);
> +
> +       /*
> +        * Register clockevents only for 16-bit timers. 32-bit timers clockevents
> +        * are registered by CPU hotplug startup function set previously by the
> +        * cpuhp_setup_state() call.
> +        */
> +       for (i = RZN1_TIMER_BASE_INDEX_16BIT_TIMERS + 1; i < RZN1_TIMER_NB_16BIT_TIMERS; i++) {
> +               timer = &tab_timers[i];
> +               clockevents_config_and_register(&timer->ced, timer->rate,
> +                                               1, GENMASK(timer->width - 1, 0));
> +       }
> +
> +       return 0;
> +}
> +
> +static int rzn1_timer_probe_other(struct platform_device *pdev, struct rzn1_timer *tab_timers,
> +                                 void __iomem *base, unsigned long clock_rate)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct rzn1_timer *timer;
> +       unsigned int i;
> +       char *name;
> +       int irq;
> +       int ret;
> +
> +       /*
> +        * Probe other instance(s), i.e. not the first one. In that case,
> +        * all timers are used as clock events and available for all possible
> +        * CPUs
> +        *
> +        * First step, perform all operation that could fail without calling
> +        * clockevents_config_and_register(). Unregister counterpart does not
> +        * exist and so, once called, we cannot remove the related resource.
> +        */
> +       for (i = 0; i < RZN1_TIMER_NB_TIMERS; i++) {

Technically "0" is RZN1_TIMER_BASE_INDEX_16BIT_TIMERS.

> +               timer = &tab_timers[i];
> +
> +               rzn1_timer_init(timer, i, base, clock_rate);
> +
> +               irq = platform_get_irq(pdev, i);
> +               if (irq < 0)
> +                       return irq;
> +
> +               name = devm_kasprintf(dev, GFP_KERNEL, "%s-%u", dev_name(dev), i);
> +               if (!name)
> +                       return -ENOMEM;
> +
> +               rzn1_timer_clkevt_init_ced(timer, name, irq);
> +
> +               ret = devm_request_irq(dev, timer->ced.irq, rzn1_timer_interrupt,
> +                                      IRQF_TIMER, timer->ced.name, timer);
> +               if (ret < 0)
> +                       return dev_err_probe(dev, irq, "timer%d: Failed to request IRQ\n", i);

%u

> +
> +               rzn1_timer_int_enable(timer);
> +       }

This loop is identical to the first loop in rzn1_timer_probe_first(),
except for the lower bound. Perhaps it can be factored out?

> +
> +       /*
> +        * Second step, all operation that can fail have been called. We can
> +        * register our timers
> +        */
> +
> +       for (i = 0; i < RZN1_TIMER_NB_TIMERS; i++) {

Technically "0" is RZN1_TIMER_BASE_INDEX_16BIT_TIMERS.

> +               timer = &tab_timers[i];
> +               clockevents_config_and_register(&timer->ced, timer->rate,
> +                                               1, GENMASK(timer->width - 1, 0));
> +       }
> +
> +       return 0;
> +}

This loop is identical to the second loop in rzn1_timer_probe_first(),
except for the lower and upper bound. Perhaps it can be factored out?

Alternatively, you can
  1. Unify rzn1_timer_probe_{first,other}(), and pass the different
     bounds as parameters, OR
  2. Split rzn1_timer_probe_first() in three sub-functions, and call
     these from rzn1_timer_probe() directly.

> +static struct platform_driver rzn1_timer_driver = {
> +       .driver = {
> +               .name = "rzn1_timer",
> +               .of_match_table = rzn1_timer_of_match,

This driver can't be modular, and can't be unbound:

    .suppress_bind_attrs = true

> +       },
> +};
> +builtin_platform_driver_probe(rzn1_timer_driver, rzn1_timer_probe);

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds