Re: [PATCH v3 2/2] clocksource: Add Realtek systimer as tick broadcast driver

[Thomas Gleixner <tglx@linutronix.de>]

On Mon, Nov 17 2025 at 15:34, Hao-Wen Ting wrote:
> +static u64 rtk_ts64_read(void)
> +{
> +	u64 ts;
> +	u32 low, high;

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#variable-declarations

> +	/* Caution: Read LSB word (TS_LW_OFST) first then MSB (TS_HW_OFST) */
> +	low = readl(systimer_base + TS_LW_OFST);
> +	high = readl(systimer_base + TS_HW_OFST);
> +
> +	pr_debug("64bit-TS:HW=0x%08x,LW=0x%08x\n", high, low);

Please get rid of these debug prints.

> +	ts = ((u64)high << 32) | low;
> +
> +	return ts;
> +}
> +
> +
> +static int rtk_syst_clkevt_next_ktime(ktime_t expires,
> +				      struct clock_event_device *clkevt)

Pointless line break. You have 100 characters

> +{
> +	u64 cmp_val;
> +	unsigned long flags;
> +	ktime_t now = ktime_get();
> +	s64 delta_ns = ktime_to_ns(ktime_sub(expires, now));
> +	u64 delta_us = delta_ns / 1000;
> +
> +	pr_debug("delta_ns = %lld, clkevt.min_delta_ns = %llu\n",
> +		 delta_ns, clkevt->min_delta_ns);
> +
> +	if (delta_ns <= (s64)clkevt->min_delta_ns) {
> +		delta_ns = clkevt->min_delta_ns;
> +		delta_us = delta_ns / 1000;
> +		pr_debug("Clamping delta_ns to min_delta_ns\n");
> +	}

Why are you using the set_next_ktime() callback instead of set_next(),
where the core code does the conversion _and_ the clamping?

> +	rtk_cmp_en_write(DSBL);
> +	local_irq_save(flags);

Pointless exercise. set_next*() is always invoked with interrupts disabled.

> +	cmp_val = rtk_ts64_read();
> +
> +	/* Set CMP value to current timestamp plus delta_us */
> +	rtk_cmp_value_write(cmp_val + delta_us);
> +	rtk_cmp_en_write(ENBL);
> +	local_irq_restore(flags);
> +	return 0;
> +}

> +static struct timer_of _to = {

What's wrong with a proper variable name instead of this made up '_to'?

> +	.flags = TIMER_OF_IRQ | TIMER_OF_BASE,
> +
> +	.clkevt = {
> +		.name = "rtk-clkevt",
> +		.rating = 300,
> +		.cpumask = cpu_possible_mask,
> +		.features = CLOCK_EVT_FEAT_DYNIRQ |
> +			    CLOCK_EVT_FEAT_ONESHOT |
> +			    CLOCK_EVT_FEAT_KTIME,
> +		.set_next_ktime = rtk_syst_clkevt_next_ktime,
> +		.set_state_oneshot = rtk_syst_shutdown,
> +		.set_state_shutdown = rtk_syst_shutdown
> +	},
> +
> +	.of_irq = {
> +		.flags = IRQF_TIMER | IRQF_IRQPOLL,
> +		.handler = rtk_ts_match_intr_handler
> +	},

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#struct-declarations-and-initializers

Thanks,

        tglx