Re: [PATCH v3 7/9] clocksource: realtek: Add timer driver for rtl-otto platforms

[Sander Vanheule]

Hi Chris,

On Thu, 2024-06-27 at 16:33 +1200, Chris Packham wrote:
> The timer/counter block on the Realtek SoCs provides up to 5 timers. It
> also includes a watchdog timer but this isn't being used currently (it
> will be added as a separate wdt driver).

Do you mean the watchdog timer supported by drivers/watchdog/realtek_otto_wdt.c? Or are
you referring to another watchdog timer?

> One timer will be used per CPU as a local clock event generator. An
> additional timer will be used as an overal stable clocksource.
> 
> Signed-off-by: Markus Stockhausen <markus.stockhausen@xxxxxx>
> Signed-off-by: Sander Vanheule <sander@xxxxxxxxxxxxx>
> Signed-off-by: Chris Packham <chris.packham@xxxxxxxxxxxxxxxxxxx>
> ---

For reference, I submitted a driver for the same hardware back in 2022, but didn't manage
to follow up to finalize the submission:

https://lore.kernel.org/all/cover.1642369117.git.sander@xxxxxxxxxxxxx/


> +
> +/* Module initialization part. */
> +static DEFINE_PER_CPU(struct timer_of, rttm_to) = {
> +	.flags				= TIMER_OF_BASE | TIMER_OF_CLOCK |
> TIMER_OF_IRQ,
> +	.of_irq = {
> +		.flags			= IRQF_PERCPU | IRQF_TIMER,

In the original review of this code, I had some doubts about the use of IRQF_PERCPU. Maybe
the people in Cc can shed some light on this.

If I understand correctly, the SoC interrupts these timers use are not per-cpu interrupts.
(For comparison, AFAICT the MIPS CPU interrupts are)



> +		.handler		= rttm_timer_interrupt,
> +	},
> +	.clkevt = {
> +		.rating			= 400,
> +		.features		= CLOCK_EVT_FEAT_PERIODIC |
> CLOCK_EVT_FEAT_ONESHOT,

If the use of IRQF_PERCPU is appropriate, I wonder if the driver should also use
CLOCK_EVT_FEAT_PERCPU.


Best,
Sander