On Wed, 3 Apr 2019, Jacky Bai wrote:
> From: Bai Ping <ping.bai@xxxxxxx>
>
> The system counter (sys_ctr) is a programmable system counter
> which provides a shared time base to the Cortex A15, A7, A53 etc cores.
> It is intended for use in applications where the counter is always
> powered on and supports multiple, unrelated clocks. The sys_ctr hardware
> supports:
> - 56-bit counter width (roll-over time greater than 40 years)
> - compare frame(64-bit compare value) contains programmable interrupt
> generation
I hope that's a <= compare and not a == ....
> +static void sysctr_timer_enable(bool enable)
> +{
> + u32 val;
> +
> + val = readl(sys_ctr_base + CMPCR);
> + val &= ~SYS_CTR_EN;
> + if (enable)
> + val |= SYS_CTR_EN;
> +
> + writel(val, sys_ctr_base + CMPCR);
This read is really just overhead. Why aren't you caching the control
register value? It's not a self modifying register and I don't see
concurrency here either.
> +}
> +
> +static void sysctr_irq_acknowledge(void)
> +{
> + /*
> + * clear the enable bit(EN =0) will clear
> + * the status bit(ISTAT = 0), then the interrupt
> + * signal will be negated(acknowledged).
> + */
> + sysctr_timer_enable(false);
> +}
> +
> +static inline u64 sysctr_read_counter(void)
> +{
> + u32 cnt_hi, tmp_hi, cnt_lo;
> +
> + do {
> + cnt_hi = readl_relaxed(sys_ctr_base + CNTCV_HI);
> + cnt_lo = readl_relaxed(sys_ctr_base + CNTCV_LO);
> + tmp_hi = readl_relaxed(sys_ctr_base + CNTCV_HI);
> + } while (tmp_hi != cnt_hi);
When will hardware people finally get it? Is it so damned hard to make the
readout do:
lo = read_lo() -> internally latches HI in hardware
hi = read_hi() -> reads the latched value
It's not rocket science, but it would spare these horrible read loops. But
sure, performance happens in whitepapers and marketing slides ....
> +
> + return ((u64) cnt_hi << 32) | cnt_lo;
> +}
> +
> +static int sysctr_set_next_event(unsigned long delta,
> + struct clock_event_device *evt)
> +{
> + u32 cmp_hi, cmp_lo;
> + u64 next;
> +
> + sysctr_timer_enable(false);
> +
> + next = sysctr_read_counter();
> +
> + next += delta;
> +
> + cmp_hi = (next >> 32) & 0x00fffff;
> + cmp_lo = next & 0xffffffff;
> +
> + writel_relaxed(cmp_hi, sys_ctr_base + CMPCV_HI);
> + writel_relaxed(cmp_lo, sys_ctr_base + CMPCV_LO);
Please document that this is a <= comparator. If that's not true then this
function is broken for small deltas and delays between read_counter() and
enable.
> +
> + sysctr_timer_enable(true);
> +
> + return 0;
> +}
> +
> +static int sysctr_set_state_oneshot(struct clock_event_device *evt)
> +{
> + sysctr_timer_enable(true);
That's wrong. Why do you want to enable the timer here? When the state is
set to one shot then the next operation is set_next_event() but before that
nothing should ever come out of the timer.
Thanks,
tglx
