[PATCH v3 03/11] clocksource: sp804: add device tree support

[Rob Herring]

On 03/13/2013 06:05 AM, Pawel Moll wrote:
> On Wed, 2013-03-13 at 05:05 +0000, Haojian Zhuang wrote:
>> +++ b/Documentation/devicetree/bindings/timer/arm,sp804.txt
>> @@ -0,0 +1,27 @@
>> +ARM sp804 Dual Timers
>> +---------------------------------------
>> +
>> +Required properties:
>> +- compatible: Should be "arm,sp804" & "arm,primecell"
>> +- interrupts: Should contain the list of Dual Timer interrupts
>> +	interrupts = <0 0 4>, <0 1 4>;
> 
> SP804 has three interrupt outputs: TIMINT1 (interrupt generated by the
> first timer), TIMINT2 (the second timer) and TIMINTC (combined - logical
> sum of the two former ones). You may want to describe this somehow to
> make the choice possible later (eg. VE has the combined interrupt wired
> while - if I understand what Rob is saying correctly - Highbank uses
> only the TIMINT1).

How about:

1 irq - TIMINT1
2 irqs w/ same source # - TIMINTC
2 irqs w/ different source # - TIMINT1 and TIMINT2

I'm not completely sure if Linux and the irq domain code handles the
same interrupt source repeated. It should because that is basically a
shared irq line.

If we ever see only TIMINT2 connected we can add a property for that,
but I think that case is unlikely.

> 
>> +- reg: Should contain location and length for dual timer register.
>> +- clocks: clock driving the dual timer hardware
>> +	clocks = <&timclk0 &timclk1>;
> 
> Again, there are three "clock" inputs (even ignoring the APBCLK):

We should not ignore that clock as we may want this to be a proper
amba_device someday.

> TIMCLK, TIMCLKEN1, TIMCLKEN2. The real clocking rate for the timer
> depends on the TIMCLK and respective TIMCLKENx - see
> http://infocenter.arm.com/help/topic/com.arm.doc.ddi0271d/CJABHCIG.html 
> 
> The driver than could make educated choice based on this information.
> 
> You may choose to ignore this fact (and require only a clock
> representing the effective rate). I did it for the VE DTS, but it still
> doesn't seem completely right
> 
>> +Optional properties:
>> +- arm,sp804-clocksource: Should contain the register offset of TIMER1 or
>> +  TIMER2 in Dual Timer Controller.
>> +	arm,sp804-clocksource = <0x20>;
> 
> You seem to be missing the "arm,sp804-clockevent" one here.

These should stay as aliases or go away as I suggested.

Rob