Re: [PATCH v3 04/12] of: add J-Core timer bindings

[Rich Felker]

On Wed, Jun 01, 2016 at 08:58:52AM -0500, Rob Herring wrote:
> On Wed, May 25, 2016 at 05:43:03AM +0000, Rich Felker wrote:
> > Signed-off-by: Rich Felker <dalias@xxxxxxxx>
> > ---
> >  .../devicetree/bindings/timer/jcore,pit.txt        | 28 ++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/timer/jcore,pit.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/timer/jcore,pit.txt b/Documentation/devicetree/bindings/timer/jcore,pit.txt
> > new file mode 100644
> > index 0000000..96c6815
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/timer/jcore,pit.txt
> > @@ -0,0 +1,28 @@
> > +J-Core Programmable Interval Timer and Clocksource
> > +
> > +Required properties:
> > +
> > +- compatible: Must be "jcore,pit".
> > +
> > +- reg: Memory region for timer/clocksource registers.
> > +
> > +- interrupts: An interrupt to assign for the timer. The actual pit
> > +  core is integrated with the aic and allows the timer interrupt
> > +  assignment to be programmed by software, but this property is
> > +  required in order to reserve an interrupt number that doesn't
> > +  conflict with other devices.
> > +
> > +Optional properties:
> > +
> > +- cpu-offset: For SMP, the per-cpu offset to the local timer
> > +  programming memory range.
> > +
> > +
> > +Example:
> > +
> > +timer@200 {
> > +	compatible = "jcore,pit";
> > +	reg = < 0x200 0x30 >;
> > +	cpu-offset = < 0x300 >;
> 
> This is outside the reg range. Perhaps reg should include each range of 
> per cpu registers.

In the hardware, each timer instance is mapped independently so
there's no fundamental reason they need to be mapped sufficiently
close that it would make sense for a single virtual mapping to cover
them all. This doesn't matter for nommu but it would with mmu in the
future. In the driver I've updated it to ioremap each percpu instance
separately (as its own memory range) using the cpu-offset applied to
the range obtained from "reg". Is this acceptable (in which case I
suppose the binding needs to be documented that "reg" just covers the
cpu0 instance's range)? Do you think it would be preferable to have
multiple "reg" ranges indexed by cpu instead of cpu-offset?

In theory it would even be possible to just require a DT node per
cpulocal timer, but I didn't see a good way to make the bindings
represent the relationship to cpus or to make the driver handle irqs
correctly for such a setup, so I'd need a viable proposal for how that
could be done to even consider such an approach.

Rich