Sorry for delayed response to you, my mail client did something wrong. :( > On Wed, 3 Apr 2019, Jacky Bai wrote: > > > From: Bai Ping <ping....@nxp.com> > > > > 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 == .... >
Yes, it is <= compare, when the free running counter value >= the compare value, then interrupt is triggered. > > +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. > Thanks, I will use a cached value for it. > > +} > > + > > +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 .... > Sadly, hardware people don't implement such internal latch logic, so we need to use such read loop. > > + > > + 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. It is <= comparator, so it is safe. > > > + > > + 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, I will remove it in V4. BR Jacky Bai > Thanks, > > tglx