Re: [PATCH v3 2/2] driver: clocksource: Add nxp system counter timer driver support

[Jacky Bai]

Sorry for delayed response to you, my mail client did something wrong. :(

> 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 == ....
> 

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