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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The 16-bit per-CPU timers will never be chosen over the arch timer due to their low rating, resulting in asymmetrical per-CPU tick devices.
- [Medium] Clocksource and sched_clock return raw decreasing values if the timer is a down-counter.
--

commit 0389aacef8150cde2d480116d05b7b23c573015b
Author: Herve Codina (Schneider Electric) <herve.codina@bootlin.com>

clocksource/drivers: Add support for the Renesas RZ/N1 timers

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, 2 32-bit). Each timer has its own interrupt and prescaler.

> diff --git a/drivers/clocksource/timer-rzn1.c b/drivers/clocksource/timer-rzn1.c
> new file mode 100644
> index 0000000000000..a66a045309f63
> --- /dev/null
> +++ b/drivers/clocksource/timer-rzn1.c

[ ... ]

> +static u64 notrace rzn1_sched_read(void)
> +{
> +	return readl_relaxed(rzn1_sched_clock->base + RZN1_TIMER_CURRENT_COUNT_REG);
> +}
> +
> +static u64 rzn1_clksrc_read(struct clocksource *cs)
> +{
> +	return readl_relaxed(rzn1_sched_clock->base + RZN1_TIMER_CURRENT_COUNT_REG);
> +}

[Severity: Medium]
If the timer is a down-counter, will these functions return a decreasing value?

The clocksource and sched_clock cores require monotonically increasing values.
If the timer counts down, returning the raw value could cause the
(now - last) & mask calculation to yield large forward time jumps, breaking
system timekeeping and scheduling.

[ ... ]

> +static int rzn1_local_timer_starting_cpu(unsigned int cpu)
> +{
> +	struct rzn1_timer *timer;
> +
> +	/* We assign the first and second 16-bit timer as per-cpu timers. */
> +	if (cpu >= 2)
> +		return 0;
> +
> +	timer = &rzn1_tab_timers[cpu + RZN1_TIMER_BASE_INDEX_16BIT_TIMERS];

[ ... ]

> +static void rzn1_timer_clkevt_init_ced(struct rzn1_timer *timer, const char *name,
> +				       int irq)
> +{

[ ... ]

> +	/*
> +	 * 32-bit timers are fed with a 25 MHz clock directly derived from the
> +	 * main PLL (1 GHz). The arch timer, ARMv7 CP15 in RZ/N1 SoCs, is fed
> +	 * with to a 6.25 MHz clock.
> +	 *
> +	 * The clock of the RZ/N1 timers block is a stable one and the RZ/N1
> +	 * timers have a higher resolution than the ARM CP15 one.
> +	 *
> +	 * Also those 32-bit RZ/N1 timers are assigned per-cpu.
> +	 *
> +	 * Use a high rating for the 32-bit RZ/N1 timers in order to have them
> +	 * chosen by the system.
> +	 */
> +	timer->ced.rating = timer->width == 16 ? 200 : 460;

[Severity: High]
Is it intentional that the 16-bit per-CPU timers are assigned a rating of 200?

In rzn1_local_timer_starting_cpu(), the 16-bit timers are explicitly assigned
as the per-CPU tick devices. However, because the ARM arch timer typically has
a higher rating (400-450), won't the 16-bit timers always be outranked and
ignored by the clockevents core?

This appears to contradict the comment stating that 32-bit timers are assigned
per-cpu, and could leave CPU 1 falling back to the arch timer while CPU 0
might use the global 32-bit timer, resulting in asymmetrical per-CPU tick
devices.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911150456.893761-1-herve.codina@bootlin.com?part=2