Re: [PATCH v7 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] time: rzn1: missing bitwise inversion of down-counter breaks timekeeping
- [Medium] time: rzn1: low clocksource rating prevents selection of high-res timer
--

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

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

This commit introduces a new clocksource driver for the Renesas RZ/N1 SoCs
timer block. It provides support for the 8 available timers (6 16-bit and
2 32-bit timers) to be used as clock events, clocksource, and sched_clock.

> diff --git a/drivers/clocksource/timer-rzn1.c b/drivers/clocksource/timer-rzn1.c
> new file mode 100644
> index 0000000000000..25ebebff3c639
> --- /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: High]
The RZ/N1 timer is a down-counter (it uses a LOAD_COUNT register and counts
down to 0), but the sched_clock and clocksource subsystems require
monotonically increasing counter values.

Will returning the raw down-counter value cause the delta between consecutive
reads to be negative, underflowing into a massive positive time jump? This
breaks core timekeeping, scheduling, and RCU.

Does this require bitwise inversion of the read value (e.g.,
~readl_relaxed(...)) to return an increasing counter?

> +static struct clocksource rzn1_clocksource = {
> +	.name = "rzn1_timer_clocksource",
> +	.rating = 200,
> +	.flags = CLOCK_SOURCE_IS_CONTINUOUS,
> +	.read = rzn1_clksrc_read,
> +};

[Severity: Medium]
The 32-bit RZ/N1 timer has a higher resolution (25 MHz) than the ARM arch
timer (6.25 MHz), but the standard ARM arch timer has a rating of 400.

Will statically defining this clocksource's rating as 200 prevent it from
being selected over the lower-resolution ARM arch timer?

(Note that the clock event device correctly receives a rating of 460 in
rzn1_timer_clkevt_init_ced() for this exact reason.)

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