diff mbox

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

Message ID 1363151142-32162-4-git-send-email-haojian.zhuang@linaro.org
State Superseded
Headers show

Commit Message

Haojian Zhuang March 13, 2013, 5:05 a.m. UTC
Parse clock & irq from device tree for sp804.

Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
---
 .../devicetree/bindings/timer/arm,sp804.txt        |   27 +++++
 drivers/clocksource/timer-sp.c                     |  105 ++++++++++++++++++++
 2 files changed, 132 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/timer/arm,sp804.txt

Comments

Pawel Moll March 13, 2013, 11:05 a.m. UTC | #1
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).

> +- 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):
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.

Regards

Paweł
Haojian Zhuang March 13, 2013, 11:37 a.m. UTC | #2
On 13 March 2013 19:05, Pawel Moll <pawel.moll@arm.com> 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).
>
I prefer to ignore the TIMINTC. It seems that nobody is using this interrupt.

>> +- 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):
> 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
>
Sure. I can append it.

>> +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.
>
Yes, I missed. I'll update the README.
Pawel Moll March 13, 2013, 11:41 a.m. UTC | #3
On Wed, 2013-03-13 at 11:37 +0000, Haojian Zhuang wrote:
> On 13 March 2013 19:05, Pawel Moll <pawel.moll@arm.com> 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).
> >
> I prefer to ignore the TIMINTC. It seems that nobody is using this interrupt.

VE is the nobody then ;-) As I mentioned above - both VE motherboard
SP804s have the TIMINTC wired up and TIMINT1/TIMINT2 unused.

Paweł
Rob Herring March 13, 2013, 2:17 p.m. UTC | #4
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
Pawel Moll March 13, 2013, 2:42 p.m. UTC | #5
On Wed, 2013-03-13 at 14:17 +0000, Rob Herring wrote:
> 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.

I was rather thinking about using the "interrupt-names" property and
naming them explicitly, eg:

	interrupt-names = "timint1", "timint2";
	interrupts = <1>, <2>;

	interrupt-names = "timint1";
	interrupts = <1>;

	interrupt-names = "timint2";
	interrupts = <2>;

	interrupt-names = "timintc";
	interrupts = <3>;

But now I see that of_amba_device_create() doesn't do anything about it
(platform device would use them as resource name so we could use
platform_get_resource_byname), so I'm not sure any more...

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

Sorry, wrong wording. I meant to say "even not mentioning the
APBCLK" (because it's required anyway by the primecell binding). VE
SP804 node obviously has it defined:

v2m_timer01: timer@110000 {                           
	compatible = "arm,sp804", "arm,primecell";    
	reg = <0x110000 0x1000>;
	interrupts = <2>;                
	clocks = <&v2m_sysctl 0>, <&v2m_sysctl 1>, <&smbclk>;
	clock-names = "timclken1", "timclken2", "apb_pclk";
};                                   

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

I merely pointed out that it's missing from the binding description.

I couldn't agree more that both should not be there in the first place.

Paweł
Rob Herring March 13, 2013, 2:51 p.m. UTC | #6
On 03/13/2013 09:42 AM, Pawel Moll wrote:
> On Wed, 2013-03-13 at 14:17 +0000, Rob Herring wrote:
>> 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.
> 
> I was rather thinking about using the "interrupt-names" property and
> naming them explicitly, eg:
> 
> 	interrupt-names = "timint1", "timint2";
> 	interrupts = <1>, <2>;
> 
> 	interrupt-names = "timint1";
> 	interrupts = <1>;
> 
> 	interrupt-names = "timint2";
> 	interrupts = <2>;
> 
> 	interrupt-names = "timintc";
> 	interrupts = <3>;
> 
> But now I see that of_amba_device_create() doesn't do anything about it
> (platform device would use them as resource name so we could use
> platform_get_resource_byname), so I'm not sure any more...

The interrupt-names property should not be required and without it here
you cannot determine the configuration.

Rob
Pawel Moll March 13, 2013, 2:55 p.m. UTC | #7
On Wed, 2013-03-13 at 14:51 +0000, Rob Herring wrote:
> On 03/13/2013 09:42 AM, Pawel Moll wrote:
> > On Wed, 2013-03-13 at 14:17 +0000, Rob Herring wrote:
> >> 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.
> > 
> > I was rather thinking about using the "interrupt-names" property and
> > naming them explicitly, eg:
> > 
> > 	interrupt-names = "timint1", "timint2";
> > 	interrupts = <1>, <2>;
> > 
> > 	interrupt-names = "timint1";
> > 	interrupts = <1>;
> > 
> > 	interrupt-names = "timint2";
> > 	interrupts = <2>;
> > 
> > 	interrupt-names = "timintc";
> > 	interrupts = <3>;
> > 
> > But now I see that of_amba_device_create() doesn't do anything about it
> > (platform device would use them as resource name so we could use
> > platform_get_resource_byname), so I'm not sure any more...
> 
> The interrupt-names property should not be required and without it here
> you cannot determine the configuration.

It is not required by the standard interrupt bindings. But it could be
required by the SP804 binding, couldn't it? (like any other property)
"Encoding" the information by specific organization of the field makes
it "invisible to the naked eye".

Not that I have any strong feelings about it.

Paweł
Haojian Zhuang March 13, 2013, 3:11 p.m. UTC | #8
On 13 March 2013 22:55, Pawel Moll <pawel.moll@arm.com> wrote:
> On Wed, 2013-03-13 at 14:51 +0000, Rob Herring wrote:
>> On 03/13/2013 09:42 AM, Pawel Moll wrote:
>> > On Wed, 2013-03-13 at 14:17 +0000, Rob Herring wrote:
>> >> 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.
>> >
>> > I was rather thinking about using the "interrupt-names" property and
>> > naming them explicitly, eg:
>> >
>> >     interrupt-names = "timint1", "timint2";
>> >     interrupts = <1>, <2>;
>> >
>> >     interrupt-names = "timint1";
>> >     interrupts = <1>;
>> >
>> >     interrupt-names = "timint2";
>> >     interrupts = <2>;
>> >
>> >     interrupt-names = "timintc";
>> >     interrupts = <3>;
>> >
>> > But now I see that of_amba_device_create() doesn't do anything about it
>> > (platform device would use them as resource name so we could use
>> > platform_get_resource_byname), so I'm not sure any more...
>>
>> The interrupt-names property should not be required and without it here
>> you cannot determine the configuration.
>
> It is not required by the standard interrupt bindings. But it could be
> required by the SP804 binding, couldn't it? (like any other property)
> "Encoding" the information by specific organization of the field makes
> it "invisible to the naked eye".
>
> Not that I have any strong feelings about it.
>
> Paweł
>
>

What's the scenario that we must use TIMINTC? TIMINT1 & TIMINT2 are
already enough on these two TIMERs. If we really needn't TIMINTC, we
need to support it. Since it's over-designed.

If TIMINT1 & TIMINT2 aren't routed, and only TIMINTC is routed. It's another
case. We can consider it as replacement of TIMINT1.

Regards
Haojian
Pawel Moll March 13, 2013, 3:23 p.m. UTC | #9
On Wed, 2013-03-13 at 14:17 +0000, Rob Herring wrote:
> How about:
> 
> 1 irq - TIMINT1
> 2 irqs w/ same source # - TIMINTC
> 2 irqs w/ different source # - TIMINT1 and TIMINT2

On Wed, 2013-03-13 at 15:11 +0000, Haojian Zhuang wrote:
> What's the scenario that we must use TIMINTC? TIMINT1 & TIMINT2 are
> already enough on these two TIMERs. If we really needn't TIMINTC, we
> need to support it. Since it's over-designed.
> 
> If TIMINT1 & TIMINT2 aren't routed, and only TIMINTC is routed. It's another
> case. We can consider it as replacement of TIMINT1.

Just a thought... How to describe a SP804 with only TIMINT2 wired up?

Paweł
Haojian Zhuang March 13, 2013, 3:25 p.m. UTC | #10
On 13 March 2013 23:23, Pawel Moll <pawel.moll@arm.com> wrote:
> On Wed, 2013-03-13 at 14:17 +0000, Rob Herring wrote:
>> How about:
>>
>> 1 irq - TIMINT1
>> 2 irqs w/ same source # - TIMINTC
>> 2 irqs w/ different source # - TIMINT1 and TIMINT2
>
> On Wed, 2013-03-13 at 15:11 +0000, Haojian Zhuang wrote:
>> What's the scenario that we must use TIMINTC? TIMINT1 & TIMINT2 are
>> already enough on these two TIMERs. If we really needn't TIMINTC, we
>> need to support it. Since it's over-designed.
>>
>> If TIMINT1 & TIMINT2 aren't routed, and only TIMINTC is routed. It's another
>> case. We can consider it as replacement of TIMINT1.
>
> Just a thought... How to describe a SP804 with only TIMINT2 wired up?
>
> Paweł
>
>
There's no difference on TIMINTC is only used to replace TIMINT1 or TIMINT2.
We only need to tell sp804 driver which timer is using irq, and the irq number.
Pawel Moll March 13, 2013, 3:29 p.m. UTC | #11
On Wed, 2013-03-13 at 15:25 +0000, Haojian Zhuang wrote:
> On 13 March 2013 23:23, Pawel Moll <pawel.moll@arm.com> wrote:
> > On Wed, 2013-03-13 at 14:17 +0000, Rob Herring wrote:
> >> How about:
> >>
> >> 1 irq - TIMINT1
> >> 2 irqs w/ same source # - TIMINTC
> >> 2 irqs w/ different source # - TIMINT1 and TIMINT2
> >
> > On Wed, 2013-03-13 at 15:11 +0000, Haojian Zhuang wrote:
> >> What's the scenario that we must use TIMINTC? TIMINT1 & TIMINT2 are
> >> already enough on these two TIMERs. If we really needn't TIMINTC, we
> >> need to support it. Since it's over-designed.
> >>
> >> If TIMINT1 & TIMINT2 aren't routed, and only TIMINTC is routed. It's another
> >> case. We can consider it as replacement of TIMINT1.
> >
> > Just a thought... How to describe a SP804 with only TIMINT2 wired up?
> >
> > Paweł
> >
> >
> There's no difference on TIMINTC is only used to replace TIMINT1 or TIMINT2.
> We only need to tell sp804 driver which timer is using irq, and the irq number.

If I understand you well:

* Both TIMINT1 and TIMINT2 wired up - TIMINTC doesn't matter

	interrupts = <1>, <2>;

* TIMINT1 wired up, TIMINT2 not wired up

	interrupts = <1>;

* Only TIMINTC wired up - treat it as TIMINT1

	interrupts = <1>;

So far so good. My question is, how do you describe the following:

* _Only_ TIMINT2 wired up, TIMINT1 and TIMINTC _not_ wired up

	interrupts = ???

Paweł
Rob Herring March 13, 2013, 3:39 p.m. UTC | #12
On 03/13/2013 10:29 AM, Pawel Moll wrote:
> On Wed, 2013-03-13 at 15:25 +0000, Haojian Zhuang wrote:
>> On 13 March 2013 23:23, Pawel Moll <pawel.moll@arm.com> wrote:
>>> On Wed, 2013-03-13 at 14:17 +0000, Rob Herring wrote:
>>>> How about:
>>>>
>>>> 1 irq - TIMINT1
>>>> 2 irqs w/ same source # - TIMINTC
>>>> 2 irqs w/ different source # - TIMINT1 and TIMINT2
>>>
>>> On Wed, 2013-03-13 at 15:11 +0000, Haojian Zhuang wrote:
>>>> What's the scenario that we must use TIMINTC? TIMINT1 & TIMINT2 are
>>>> already enough on these two TIMERs. If we really needn't TIMINTC, we
>>>> need to support it. Since it's over-designed.
>>>>
>>>> If TIMINT1 & TIMINT2 aren't routed, and only TIMINTC is routed. It's another
>>>> case. We can consider it as replacement of TIMINT1.
>>>
>>> Just a thought... How to describe a SP804 with only TIMINT2 wired up?
>>>
>>> Paweł
>>>
>>>
>> There's no difference on TIMINTC is only used to replace TIMINT1 or TIMINT2.
>> We only need to tell sp804 driver which timer is using irq, and the irq number.
> 
> If I understand you well:
> 
> * Both TIMINT1 and TIMINT2 wired up - TIMINTC doesn't matter
> 
> 	interrupts = <1>, <2>;
> 
> * TIMINT1 wired up, TIMINT2 not wired up
> 
> 	interrupts = <1>;
> 
> * Only TIMINTC wired up - treat it as TIMINT1
> 
> 	interrupts = <1>;
> 
> So far so good. My question is, how do you describe the following:
> 
> * _Only_ TIMINT2 wired up, TIMINT1 and TIMINTC _not_ wired up
> 
> 	interrupts = ???

I previously described that to add a property in that case. That makes
INT1 and INT2 only handling asymetric though. An alternative would be an
optional property "arm,sp804-has-irq = <#>" in the case of only INT1 or
INT2 connected. The combined irq would be a single irq without the
"arm,sp804-has-irq" property and both connected would be 2 irqs.

Rob
Pawel Moll March 13, 2013, 3:41 p.m. UTC | #13
On Wed, 2013-03-13 at 15:39 +0000, Rob Herring wrote:
> I previously described that to add a property in that case. That makes
> INT1 and INT2 only handling asymetric though. An alternative would be an
> optional property "arm,sp804-has-irq = <#>" in the case of only INT1 or
> INT2 connected. The combined irq would be a single irq without the
> "arm,sp804-has-irq" property and both connected would be 2 irqs.

Fine with me.

Paweł
Haojian Zhuang March 13, 2013, 3:42 p.m. UTC | #14
On 13 March 2013 23:29, Pawel Moll <pawel.moll@arm.com> wrote:
> On Wed, 2013-03-13 at 15:25 +0000, Haojian Zhuang wrote:
>> On 13 March 2013 23:23, Pawel Moll <pawel.moll@arm.com> wrote:
>> > On Wed, 2013-03-13 at 14:17 +0000, Rob Herring wrote:
>> >> How about:
>> >>
>> >> 1 irq - TIMINT1
>> >> 2 irqs w/ same source # - TIMINTC
>> >> 2 irqs w/ different source # - TIMINT1 and TIMINT2
>> >
>> > On Wed, 2013-03-13 at 15:11 +0000, Haojian Zhuang wrote:
>> >> What's the scenario that we must use TIMINTC? TIMINT1 & TIMINT2 are
>> >> already enough on these two TIMERs. If we really needn't TIMINTC, we
>> >> need to support it. Since it's over-designed.
>> >>
>> >> If TIMINT1 & TIMINT2 aren't routed, and only TIMINTC is routed. It's another
>> >> case. We can consider it as replacement of TIMINT1.
>> >
>> > Just a thought... How to describe a SP804 with only TIMINT2 wired up?
>> >
>> > Paweł
>> >
>> >
>> There's no difference on TIMINTC is only used to replace TIMINT1 or TIMINT2.
>> We only need to tell sp804 driver which timer is using irq, and the irq number.
>
> If I understand you well:
>
> * Both TIMINT1 and TIMINT2 wired up - TIMINTC doesn't matter
>
>         interrupts = <1>, <2>;
>
> * TIMINT1 wired up, TIMINT2 not wired up
>
>         interrupts = <1>;
>
> * Only TIMINTC wired up - treat it as TIMINT1
>
>         interrupts = <1>;
>
> So far so good. My question is, how do you describe the following:
>
> * _Only_ TIMINT2 wired up, TIMINT1 and TIMINTC _not_ wired up
>
>         interrupts = ???
>
> Paweł
>
>
>
In my current implementation, I need two things to judge whether
irq is really necessary.

If the timer contains "arm,sp804-clockevent = <evtoffs>" property,
I'll check whether the timer irq is specified at the same time.

TIMINT1 & TIMINTC are not routed. TIMINT2 is used for clock event.

     interrupts = <any number> <1>;

        if (!retevt) {
                if (evtoffs) {
                        /* TIMER2 is clock event */
                        i = 1;
                        evtoffs = TIMER_2_BASE;
                } else {
                        /* TIMER1 is clock event */
                        i = 0;
                        evtoffs = TIMER_1_BASE;
                }
                irq = irq_of_parse_and_map(np, i);
                ...
        }

If the timer contains "arm,sp804-clocksource = <srcoffs>" property,
I won't check the timer irq. You even needn't define it.

TIMINT1 & TIMINTC are not routed. TIMER2 is used for clock source.
      interrupts = <any number> <any number>;
or
      don't define interrupts

        if (!retsrc) {
                if (srcoffs) {
                        /* TIMER2 is clock source */
                        i = 1;
                        srcoffs = TIMER_2_BASE;
                } else {
                        /* TIMER1 is clock source */
                        i = 0;
                        srcoffs = TIMER_1_BASE;
                }
                ...
        }
Haojian Zhuang March 13, 2013, 3:44 p.m. UTC | #15
On 13 March 2013 23:41, Pawel Moll <pawel.moll@arm.com> wrote:
> On Wed, 2013-03-13 at 15:39 +0000, Rob Herring wrote:
>> I previously described that to add a property in that case. That makes
>> INT1 and INT2 only handling asymetric though. An alternative would be an
>> optional property "arm,sp804-has-irq = <#>" in the case of only INT1 or
>> INT2 connected. The combined irq would be a single irq without the
>> "arm,sp804-has-irq" property and both connected would be 2 irqs.
>
> Fine with me.
>
> Paweł
>
>

It's OK to me. I can use this property instead.

Regards
Haojian
Pawel Moll March 13, 2013, 3:49 p.m. UTC | #16
On Wed, 2013-03-13 at 15:42 +0000, Haojian Zhuang wrote:
> In my current implementation, I need two things to judge whether
> irq is really necessary.
> 
> If the timer contains "arm,sp804-clockevent = <evtoffs>" property,
> I'll check whether the timer irq is specified at the same time.
> 
> TIMINT1 & TIMINTC are not routed. TIMINT2 is used for clock event.

Fine, so simply reverse the logic and drop the "arm,sp804-clock*"
properties. Something like:

if (has_timint1) {
	evtoffs = TIMER_1_BASE;
	irq = irq_of_parse_and_map(np, 1);
	srcoffs = TIMER_2_BASE;
} else (has_timint2) {
	evtoffs = TIMER_2_BASE;
	irq = irq_of_parse_and_map(np, 2);
	srcoffs = TIMER_1_BASE;
} else {
	can't be clockevent, sorry
}

Hope it makes clear what I'd expect from the driver.

Paweł
Arnd Bergmann March 13, 2013, 4:35 p.m. UTC | #17
On Wednesday 13 March 2013, Pawel Moll wrote:
> Fine, so simply reverse the logic and drop the "arm,sp804-clock*"
> properties. Something like:
> 
> if (has_timint1) {
>         evtoffs = TIMER_1_BASE;
>         irq = irq_of_parse_and_map(np, 1);
>         srcoffs = TIMER_2_BASE;
> } else (has_timint2) {
>         evtoffs = TIMER_2_BASE;
>         irq = irq_of_parse_and_map(np, 2);
>         srcoffs = TIMER_1_BASE;
> } else {
>         can't be clockevent, sorry
> }
> 
> Hope it makes clear what I'd expect from the driver.

Makes sense. I guess you can actually derive "has_timint1"
from the presence of the irq and just try to parse it:

	irq = irq_of_parse_and_map(np, 0);
	if (irq) {
		evtoffs = TIMER_1_BASE;
		srcoffs = TIMER_2_BASE;
	} else {
		irq = irq_of_parse_and_map(np, 1);
		if (!irq)
			return -ENODEV;
		srcoffs = TIMER_1_BASE;
		evtoffs = TIMER_2_BASE;
	}


I wonder if we can ever get into the second case though: If the
first interrupt in not valid, can there be a second one in the
interrupts property? I don't know of a way to put an "invalid"
interrupt in there.

	Arnd
Pawel Moll March 13, 2013, 4:41 p.m. UTC | #18
On Wed, 2013-03-13 at 16:35 +0000, Arnd Bergmann wrote:
> Makes sense. I guess you can actually derive "has_timint1"
> from the presence of the irq and just try to parse it:
> 
> 	irq = irq_of_parse_and_map(np, 0);
> 	if (irq) {
> 		evtoffs = TIMER_1_BASE;
> 		srcoffs = TIMER_2_BASE;
> 	} else {
> 		irq = irq_of_parse_and_map(np, 1);
> 		if (!irq)
> 			return -ENODEV;
> 		srcoffs = TIMER_1_BASE;
> 		evtoffs = TIMER_2_BASE;
> 	}
> 
> 
> I wonder if we can ever get into the second case though: If the
> first interrupt in not valid, can there be a second one in the
> interrupts property? I don't know of a way to put an "invalid"
> interrupt in there.

The discussion in the "[PATCH v3 03/11] clocksource: sp804: add device
tree support" thread resulted in some ideas how to describe such
situation. I suggested using interrupt-names property to explicitly
describe the numbers, Rob wanted to simply define one or two interrupts
and in the former case have a special property describing which one is
it. Either way you can get this information from the tree. The main
argument is about the clockevent/source aliases - whether they should be
obligatory, optional or banned.

Paweł
Russell King - ARM Linux March 15, 2013, 12:20 p.m. UTC | #19
On Wed, Mar 13, 2013 at 09:17:21AM -0500, Rob Herring wrote:
> 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.

I think you're missing something here.  Any particular platform will
generally only wire either the common interrupt, or the individual
interrupts.  Very few, if any, will wire both.

It's just like stuff such as MMCI and the UARTs.  They have a combined
interrupt out, and individual interrupt outs.  However, they're all
written for the combined interrupt only, because practically no one
uses the individal interrupts.
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/timer/arm,sp804.txt b/Documentation/devicetree/bindings/timer/arm,sp804.txt
new file mode 100644
index 0000000..ec1ae45
--- /dev/null
+++ 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>;
+- reg: Should contain location and length for dual timer register.
+- clocks: clock driving the dual timer hardware
+	clocks = <&timclk0 &timclk1>;
+
+Optional properties:
+- arm,sp804-clocksource: Should contain the register offset of TIMER1 or
+  TIMER2 in Dual Timer Controller.
+	arm,sp804-clocksource = <0x20>;
+
+Example:
+
+	timer0: timer@fc800000 {
+		compatible = "arm,sp804", "arm,primecell";
+		reg = <0xfc800000 0x1000>;
+		/* timer00 & timer01 */
+		interrupts = <0 0 4>, <0 1 4>;
+		clocks = <&timclk0 &timclk1>;
+		clock-names = "apb_pclk";
+		status = "disabled";
+	};
diff --git a/drivers/clocksource/timer-sp.c b/drivers/clocksource/timer-sp.c
index a7f2510..63f757d 100644
--- a/drivers/clocksource/timer-sp.c
+++ b/drivers/clocksource/timer-sp.c
@@ -19,16 +19,23 @@ 
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */
 #include <linux/clk.h>
+#include <linux/clkdev.h>
 #include <linux/clocksource.h>
 #include <linux/clockchips.h>
 #include <linux/err.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/io.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
 #include <clocksource/arm_timer.h>
+#include <clocksource/timer-sp.h>
 
 #include <asm/sched_clock.h>
 
+#define SP804_CLKSRC	"sp804 source"
+#define SP804_CLKEVT	"sp804 event"
+
 static long __init sp804_get_clock_rate(const char *name)
 {
 	struct clk *clk;
@@ -189,3 +196,101 @@  void __init sp804_clockevents_init(void __iomem *base, unsigned int irq,
 	setup_irq(irq, &sp804_timer_irq);
 	clockevents_config_and_register(evt, rate, 0xf, 0xffffffff);
 }
+
+static struct device_node *from = NULL;
+
+static struct of_device_id sp804_timer_match[] __initdata = {
+	{ .compatible = "arm,sp804", },
+	{}
+};
+
+static struct clk __init *sp804_dt_init_clk(struct device_node *np, int i,
+					    const char *name)
+{
+	struct clk_lookup *lookup = NULL;
+	struct clk *clk;
+
+	clk = of_clk_get(np, i);
+	if (IS_ERR(clk))
+		return clk;
+	lookup = clkdev_alloc(clk, name, "sp804");
+	if (!lookup) {
+		clk_put(clk);
+		return ERR_PTR(-EINVAL);
+	}
+	clkdev_add(lookup);
+	return clk;
+}
+
+static void __attribute__((__unused__)) __init sp804_dt_init(void)
+{
+	struct device_node *np = NULL;
+	struct clk *clksrc = NULL, *clkevt = NULL;
+	void __iomem *base;
+	int retsrc, retevt, i = 0, irq = 0;
+	u32 srcoffs = 0, evtoffs = 0;
+
+	np = of_find_matching_node(from, sp804_timer_match);
+	WARN_ON(!np);
+	if (!np) {
+		pr_err("Failed to find sp804 timer\n");
+		return;
+	}
+	from = np;
+	/* check whether timer node is available */
+	if (!of_device_is_available(np))
+		return;
+
+	base = of_iomap(np, 0);
+	WARN_ON(!base);
+	if (!base)
+		return;
+
+	retsrc = of_property_read_u32(np, "arm,sp804-clocksource", &srcoffs);
+	retevt = of_property_read_u32(np, "arm,sp804-clockevent", &evtoffs);
+	if (retsrc < 0 && retevt < 0)
+		goto err;
+
+	if (!retsrc) {
+		if (srcoffs) {
+			/* TIMER2 is clock source */
+			i = 1;
+			srcoffs = TIMER_2_BASE;
+		} else {
+			/* TIMER1 is clock source */
+			i = 0;
+			srcoffs = TIMER_1_BASE;
+		}
+		clksrc = sp804_dt_init_clk(np, i, SP804_CLKSRC);
+		if (IS_ERR(clksrc))
+			goto err;
+		sp804_clocksource_and_sched_clock_init(base + srcoffs,
+						       SP804_CLKSRC);
+	}
+	if (!retevt) {
+		if (evtoffs) {
+			/* TIMER2 is clock event */
+			i = 1;
+			evtoffs = TIMER_2_BASE;
+		} else {
+			/* TIMER1 is clock event */
+			i = 0;
+			evtoffs = TIMER_1_BASE;
+		}
+		irq = irq_of_parse_and_map(np, i);
+		if (irq < 0)
+			goto err_evt;
+		clkevt = sp804_dt_init_clk(np, i, SP804_CLKEVT);
+		if (IS_ERR(clkevt))
+			goto err_evt;
+		sp804_clockevents_init(base + evtoffs, irq, SP804_CLKEVT);
+	}
+
+	return;
+err_evt:
+	if (clksrc)
+		clk_put(clksrc);
+err:
+	iounmap(base);
+}
+CLOCKSOURCE_OF_DECLARE(sp804, "arm,sp804", sp804_dt_init)