Re: [PATCH 2/5] clocksource/drivers/spacemit: Add K1 timer support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] clocksource/drivers/spacemit: wrong MMIO register offsets for K1 timer
- [High] clocksource/drivers/spacemit: free-running mode in spacemit_timer_resume()
- [High] clocksource/drivers/spacemit: direct TMR_COUNT read in spacemit_timer_stop()
--

commit 4dc32afe42f41419e8802088abd5d90ef4a2a234
Author: Troy Mitchell <troy.mitchell@linux.spacemit.com>

clocksource/drivers/spacemit: Add K1 timer support

This commit adds a platform clockevent driver for the SpacemiT K1 and K3 APB
timers, using counter 0 and its first match comparator as a shared oneshot
broadcast timer device.

> diff --git a/drivers/clocksource/timer-spacemit.c b/drivers/clocksource/timer-spacemit.c
> new file mode 100644
> index 0000000000000..70076c17b5037
> --- /dev/null
> +++ b/drivers/clocksource/timer-spacemit.c
> @@ -0,0 +1,312 @@

[ ... ]

> +/* Use counter 0 and its first match comparator. */
> +#define TMR_CER			0x00
> +#define TMR_CMR			0x04
> +#define TMR_CCR			0x0c
> +#define TMR_MATCH		0x10
> +#define TMR_PRELOAD		0x40
> +#define TMR_PRELOAD_CTRL		0x50
> +#define TMR_IER			0x60
> +#define TMR_ICR			0x70
> +#define TMR_SR			0x80
> +#define TMR_COUNT		0x90

[Severity: High]
Are these register offsets matching the SpacemiT K1/K3 3-counter,
3-comparator APB timer hardware layout?

The K1/K3 timer block uses the MMP timer register map (as in
arch/arm/mach-mmp/regs-timers.h), where the counter 0 and shared control
registers are at:

  TMR_CCR          0x0000
  TMR_TN_MM(0, 0)  0x0004
  TMR_CR(0)        0x0028
  TMR_SR(0)        0x0034
  TMR_IER(0)       0x0040
  TMR_PLVR(0)      0x004c
  TMR_PLCR(0)      0x0058
  TMR_ICR(0)       0x0074
  TMR_CER          0x0084
  TMR_CMR          0x0088
  TMR_CVWR(0)      0x00a4

With the offsets defined here, TMR_CER (0x00) points to TMR_CCR, TMR_CMR
(0x04) points to counter 0 match 0, TMR_CCR (0x0c) points to counter 0 match
2, TMR_MATCH (0x10) points to counter 1 match 0, TMR_PRELOAD (0x40) points to
TMR_IER(0), TMR_PRELOAD_CTRL (0x50) points to TMR_PLVR(1), TMR_IER (0x60)
points to TMR_PLCR(2), TMR_ICR (0x70) points to TMR_WSR, TMR_SR (0x80) points
to TMR_WICR, and TMR_COUNT (0x90) points to TMR_ILR(1).

Won't this cause spacemit_timer_probe(), spacemit_timer_resume(),
spacemit_timer_next_event(), and spacemit_timer_interrupt() to read and write
the wrong hardware registers?

[ ... ]

> +static int spacemit_timer_stop(struct spacemit_timer *timer)
> +{
> +	u32 val, prev;
> +	int ret, i;
> +
> +	val = readl(timer->base + TMR_CER) & ~TMR_COUNTER_EN;
> +	ret = spacemit_timer_write(timer, TMR_CER, val, TMR_COUNTER_EN);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * CER crosses into the counter clock domain. Its readback alone does
> +	 * not show that counting has stopped. COUNT is synchronized back to
> +	 * APB, with up to three counter clocks of latency. Wait for a stable
> +	 * count before changing the match or preload registers.
> +	 */
> +	prev = readl(timer->base + TMR_COUNT);
> +	for (i = 0; i < 10; i++) {
> +		udelay(timer->sync_us);
> +		val = readl(timer->base + TMR_COUNT);
> +		if (val == prev)
> +			return 0;
> +		prev = val;
> +	}
> +
> +	return -ETIMEDOUT;
> +}

[Severity: High]
Can the counter value register be read directly in spacemit_timer_stop()
without cross-domain metastability?

In this timer block, the counter register TMR_CR(n) runs in the functional
clock domain and is not synchronized to the APB bus on direct reads. As noted
in arch/arm/mach-mmp/time.c:timer_read():

  __raw_writel(1, mmp_timer_base + TMR_CVWR(1));

  while (delay--)
          val = __raw_readl(mmp_timer_base + TMR_CVWR(1));

Should spacemit_timer_stop() latch and read the synchronized counter snapshot
through TMR_CVWR(0) (offset 0xa4) instead of reading the counter register
directly?

[ ... ]

> +static int spacemit_timer_next_event(unsigned long delta,
> +				     struct clock_event_device *evt)
> +{
> +	struct spacemit_timer *timer = to_spacemit_timer(evt);
> +	unsigned long flags;
> +	u32 val;
> +	int ret;
> +
> +	raw_spin_lock_irqsave(&timer->lock, flags);
> +
> +	ret = spacemit_timer_quiesce(timer);
> +	if (ret)
> +		goto out;
> +
> +	ret = spacemit_timer_write(timer, TMR_MATCH, delta - 1, U32_MAX);
> +	if (ret)
> +		goto out;
> +
> +	ret = spacemit_timer_write(timer, TMR_IER, TMR_MATCH0, TMR_MATCH_MASK);
> +	if (ret)
> +		goto out;
> +
> +	/* Enabling the counter reloads the zero preload value. */
> +	val = readl(timer->base + TMR_CER) | TMR_COUNTER_EN;
> +	ret = spacemit_timer_write(timer, TMR_CER, val, TMR_COUNTER_EN);

[ ... ]

> +static int spacemit_timer_resume(struct clock_event_device *evt)
> +{
> +	struct spacemit_timer *timer = to_spacemit_timer(evt);
> +	unsigned long flags;
> +	u32 val;
> +	int ret;
> +
> +	raw_spin_lock_irqsave(&timer->lock, flags);
> +	ret = spacemit_timer_quiesce(timer);
> +	if (ret)
> +		goto out;
> +
> +	/* Select the functional clock and disable automatic match reload. */
> +	val = readl(timer->base + TMR_CCR) & ~TMR_CLOCK_MASK;
> +	ret = spacemit_timer_write(timer, TMR_CCR, val, TMR_CLOCK_MASK);
> +	if (ret)
> +		goto out;
> +
> +	val = readl(timer->base + TMR_CMR) | TMR_FREE_RUNNING;
> +	ret = spacemit_timer_write(timer, TMR_CMR, val, TMR_FREE_RUNNING);
> +	if (ret)
> +		goto out;
> +
> +	ret = spacemit_timer_write(timer, TMR_PRELOAD_CTRL, 0, GENMASK(2, 0));
> +	if (ret)
> +		goto out;
> +
> +	ret = spacemit_timer_write(timer, TMR_PRELOAD, 0, U32_MAX);

[Severity: High]
When TMR_FREE_RUNNING (BIT(0)) is set in TMR_CMR during
spacemit_timer_resume(), does enabling the counter in
spacemit_timer_next_event() reload TMR_PRELOAD?

In this timer block, setting bit 0 of TMR_CMR selects free-running mode where
the counter wraps across 32 bits and ignores the preload register, whereas
clearing bit 0 of TMR_CMR selects preload mode (as in
arch/arm/mach-mmp/time.c:timer_config(), which clears bit 0 in TMR_CMR for
counter 0).

Because spacemit_timer_next_event() programs TMR_MATCH with delta - 1
relative to zero rather than relative to the current counter value, won't
setting TMR_FREE_RUNNING in TMR_CMR leave the counter at its previous non-zero
value when re-enabled, causing oneshot events to fire at the wrong time or
wait for a full 32-bit wraparound?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-spacemit-soc-timer-v1-0-36722421db6e@linux.spacemit.com?part=2