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

[Herve Codina <herve.codina@bootlin.com>]

Hi,

On Fri, 11 Sep 2026 15:18:37 +0000
sashiko-bot@kernel.org wrote:

> 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.

Timers available in the hardware increases their counter.

These functions return increasing values.

> 
> [ ... ]
> 
> > +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.
> 

First the comment is not fully correct. Only 16-bit timers are registered
per-cpu. I will update the comment in the next series iteration.

We have one 32-bit timer shared between cpu (cpumask set to cpu_possible_mask),
two 16-bit timers per-cpu with cpumask set to the cpu registering the timer in
rzn1_local_timer_starting_cpu().

When timers are registered (calls to clockevents_config_and_register() function),
tick_check_preferred() [1] is called.

This function prefers a local cpu timer even if the local cpu timer has a rating
lower than a shared cpu timer.

With that pointed out, we cannot have asymmetrical per-CPU tick devices.

[1] https://elixir.bootlin.com/linux/v7.3-rc5/source/kernel/time/tick-common.c#L289

Best regards,
Hervé