| Message ID | 1554287101-16189-1-git-send-email-ping.bai@nxp.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [v3,1/2] dt-bindings: timer: Add binding doc for nxp system counter timer | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success |
On Wed, 3 Apr 2019, Jacky Bai wrote: > From: Bai Ping <ping.bai@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 == .... > +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
On Wed, 3 Apr 2019 10:20:25 +0000, Jacky Bai wrote: > From: Bai Ping <ping.bai@nxp.com> > > Add the binding doc for nxp system counter timer module. > > Signed-off-by: Bai Ping <ping.bai@nxp.com> > --- > change v1->v2 > - remove the blank line at EOF > change v2->v3 > - update the binding example based on the driver change > --- > .../devicetree/bindings/timer/nxp,sysctr-timer.txt | 25 ++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > create mode 100644 Documentation/devicetree/bindings/timer/nxp,sysctr-timer.txt > Reviewed-by: Rob Herring <robh@kernel.org>
diff --git a/Documentation/devicetree/bindings/timer/nxp,sysctr-timer.txt b/Documentation/devicetree/bindings/timer/nxp,sysctr-timer.txt new file mode 100644 index 0000000..d576599 --- /dev/null +++ b/Documentation/devicetree/bindings/timer/nxp,sysctr-timer.txt @@ -0,0 +1,25 @@ +NXP System Counter Module(sys_ctr) + +The system counter(sys_ctr) is a programmable system counter which provides +a shared time base to Cortex A15, A7, A53, A73, etc. it is intended for use in +applications where the counter is always powered and support multiple, +unrelated clocks. The compare frame inside can be used for timer purpose. + +Required properties: + +- compatible : should be "nxp,sysctr-timer" +- reg : Specifies the base physical address and size of the comapre + frame and the counter control, read & compare. +- interrupts : should be the first compare frames' interrupt +- clocks : Specifies the counter clock. +- clock-names: Specifies the clock's name of this module + +Example: + + system_counter: timer@306a0000 { + compatible = "nxp,sysctr-timer"; + reg = <0x306a0000 0x20000>;/* system-counter-rd & compare */ + clocks = <&clk_8m>; + clock-names = "per"; + interrupts = <GIC_SPI 47 IRQ_TYPE_LEVEL_HIGH>; + };