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

Thomas Gleixner
2025-11-17T09:16:42Z

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