Re: [PATCH v2 1/2] devicetree: add binding for generic mmio clocksource

[Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>]

On Wed, Oct 07, 2015 at 05:47:12PM +0100, Måns Rullgård wrote:
> Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> writes:
> 
> > On Wed, Oct 07, 2015 at 04:37:13PM +0100, Mans Rullgard wrote:
> >> This adds a DT binding for a generic mmio clocksource as implemented
> >> by clocksource_mmio_init().
> >> 
> >> Signed-off-by: Mans Rullgard <mans-2StjZFpD7GcAvxtiuMwx3w@public.gmane.org>
> >> ---
> >> Changed in v2:
> >> - added sched_clock support
> >> ---
> >>  .../devicetree/bindings/timer/clocksource-mmio.txt | 28 ++++++++++++++++++++++
> >>  1 file changed, 28 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/timer/clocksource-mmio.txt
> >> 
> >> diff --git a/Documentation/devicetree/bindings/timer/clocksource-mmio.txt b/Documentation/devicetree/bindings/timer/clocksource-mmio.txt
> >> new file mode 100644
> >> index 0000000..cfb3601
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/timer/clocksource-mmio.txt
> >> @@ -0,0 +1,28 @@
> >> +Generic MMIO clocksource
> >> +
> >> +Required properties:
> >> +
> >> +- compatible: should be "clocksource-mmio"
> >> +- reg: the physical address of the counter register
> >> +- reg-io-width: size of counter register in bytes, should be 2 or 4
> >
> > Can this not be inferred from the reg?
> 
> You're right, it can.
> 
> > What about 8 byte counters?
> 
> The existing code only supports 2 or 4, but that of course doesn't
> matter here.
> 
> >> +- clocks: phandle to the source clock
> >
> > Is the frequency expected to be exactly the source clock frequency? I
> > imagine it's possible for there to be a divisor.
> 
> There could of course be, though there isn't in the hardware I'm dealing
> with.  Is specifying it here preferable using a fixed-factor-clock?

I'm not actually sure; I guess it would be ok to do so.

For now we should just explicitly state that the clocksource is assumed
to tick at the rate of the clock.

> > We can add properties for that later, but we should be explcit as to
> > what we currently expect the relationship between the clock and the
> > clocksource to be.
> >
> >> +- clocksource-bits: number of valid bits
> >> +- clocksource-rating: rating of the clocksource
> >
> > NAK. This has no meaning w.r.t. the hardware. This should not be in the
> > DT. If there are criteria that bias this (e.g. frequency, latency), they
> > should either be described in the DT or determined dynamically.
> 
> I had a bad feeling about this.  How would you suggest determining a
> suitable value from actual hardware parameters?

I don't have a good answer to that given the rating is semi-arbitrary,
but that's also the reason I don't want to see it in the DT.

Can we not just choose a fixed number in the driver for now? Likely
something lower than the architected timers (400 currently).

> >> +- linux,sched-clock: boolean, register clocksource as sched_clock
> >
> > Likewise, this property doesn't belong in the DT for the same reasons as
> > clocksource-rating.
> 
> What would be a proper way to select a sched_clock source?  I realise
> it's a Linux-specific thing and DT is supposed to be generic, but the
> information must be provided somehow.

I think that any source above a certain rate and below a certain
latency, which does not lose state, should be registered (and the best
gets chosen dynamically).

Come to think of it, what's the expected behaviour of this source w.r.t.
power management? I expect that the kernel needs to leave the clock
enabled at all times for this to possibly work.

Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html