diff mbox

[v3,06/11] ARM: vexpress: use clocksource_of_init for sp804

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

Commit Message

Haojian Zhuang March 13, 2013, 5:05 a.m. UTC
Remove all code to parse sp804. Use clocksource_of_init() instead since
all these code are implemented in sp804 driver already.

Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
---
 arch/arm/boot/dts/vexpress-v2m-rs1.dtsi |    2 ++
 arch/arm/boot/dts/vexpress-v2m.dtsi     |    2 ++
 arch/arm/boot/dts/vexpress-v2p-ca9.dts  |    2 ++
 arch/arm/mach-vexpress/Kconfig          |    1 +
 arch/arm/mach-vexpress/v2m.c            |   13 ++-----------
 5 files changed, 9 insertions(+), 11 deletions(-)

Comments

Pawel Moll March 13, 2013, 11:10 a.m. UTC | #1
On Wed, 2013-03-13 at 05:05 +0000, Haojian Zhuang wrote:
> Remove all code to parse sp804. Use clocksource_of_init() instead since
> all these code are implemented in sp804 driver already.
> 
> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
> ---
>  arch/arm/boot/dts/vexpress-v2m-rs1.dtsi |    2 ++
>  arch/arm/boot/dts/vexpress-v2m.dtsi     |    2 ++
>  arch/arm/boot/dts/vexpress-v2p-ca9.dts  |    2 ++
>  arch/arm/mach-vexpress/Kconfig          |    1 +
>  arch/arm/mach-vexpress/v2m.c            |   13 ++-----------
>  5 files changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi b/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi
> index ac870fb..3fa798f 100644
> --- a/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi
> +++ b/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi
> @@ -183,6 +183,8 @@
>  				interrupts = <2>;
>  				clocks = <&v2m_sysctl 0>, <&v2m_sysctl 1>, <&smbclk>;
>  				clock-names = "timclken1", "timclken2", "apb_pclk";
> +				arm,sp804-clocksource = <0x20>;
> +				arm,sp804-clockevent = <0>;
>  			};
>  
>  			v2m_timer23: timer@120000 {
> diff --git a/arch/arm/boot/dts/vexpress-v2m.dtsi b/arch/arm/boot/dts/vexpress-v2m.dtsi
> index f142036..86e4046 100644
> --- a/arch/arm/boot/dts/vexpress-v2m.dtsi
> +++ b/arch/arm/boot/dts/vexpress-v2m.dtsi
> @@ -182,6 +182,8 @@
>  				interrupts = <2>;
>  				clocks = <&v2m_sysctl 0>, <&v2m_sysctl 1>, <&smbclk>;
>  				clock-names = "timclken1", "timclken2", "apb_pclk";
> +				arm,sp804-clocksource = <0x20>;
> +				arm,sp804-clockevent = <0>;
>  			};
>  
>  			v2m_timer23: timer@12000 {
> diff --git a/arch/arm/boot/dts/vexpress-v2p-ca9.dts b/arch/arm/boot/dts/vexpress-v2p-ca9.dts
> index 1420bb1..a2eba95 100644
> --- a/arch/arm/boot/dts/vexpress-v2p-ca9.dts
> +++ b/arch/arm/boot/dts/vexpress-v2p-ca9.dts
> @@ -98,6 +98,8 @@
>  			     <0 49 4>;
>  		clocks = <&oscclk2>, <&oscclk2>;
>  		clock-names = "timclk", "apb_pclk";
> +		arm,sp804-clocksource = <0x20>;
> +		arm,sp804-clockevent = <0>;
>  	};
>  
>  	watchdog@100e5000 {

I love the possibility of removing the SP804 code from the VE machine
(thanks for your work!), but I'd like to avoid it being replaced by
cryptic properties in the tree.

Now, to get this right: I'm not saying that there should be completly no
OS-specific (but neutral) properties for the clocking system.

I'm saying that in "my" (the VE) case, there are four, effectively
identical timers (each pair sharing an interrupt though). Every single
one of them is perfectly capable of serving as clocksource or
clockevent. Therefore I believe the driver/framework/OS should make its
own decision.

Cheers!

Paweł
Haojian Zhuang March 13, 2013, 11:42 a.m. UTC | #2
On 13 March 2013 19:10, Pawel Moll <pawel.moll@arm.com> wrote:
> On Wed, 2013-03-13 at 05:05 +0000, Haojian Zhuang wrote:
>> Remove all code to parse sp804. Use clocksource_of_init() instead since
>> all these code are implemented in sp804 driver already.
>>
>> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
>> ---
>>  arch/arm/boot/dts/vexpress-v2m-rs1.dtsi |    2 ++
>>  arch/arm/boot/dts/vexpress-v2m.dtsi     |    2 ++
>>  arch/arm/boot/dts/vexpress-v2p-ca9.dts  |    2 ++
>>  arch/arm/mach-vexpress/Kconfig          |    1 +
>>  arch/arm/mach-vexpress/v2m.c            |   13 ++-----------
>>  5 files changed, 9 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi b/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi
>> index ac870fb..3fa798f 100644
>> --- a/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi
>> +++ b/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi
>> @@ -183,6 +183,8 @@
>>                               interrupts = <2>;
>>                               clocks = <&v2m_sysctl 0>, <&v2m_sysctl 1>, <&smbclk>;
>>                               clock-names = "timclken1", "timclken2", "apb_pclk";
>> +                             arm,sp804-clocksource = <0x20>;
>> +                             arm,sp804-clockevent = <0>;
>>                       };
>>
>>                       v2m_timer23: timer@120000 {
>> diff --git a/arch/arm/boot/dts/vexpress-v2m.dtsi b/arch/arm/boot/dts/vexpress-v2m.dtsi
>> index f142036..86e4046 100644
>> --- a/arch/arm/boot/dts/vexpress-v2m.dtsi
>> +++ b/arch/arm/boot/dts/vexpress-v2m.dtsi
>> @@ -182,6 +182,8 @@
>>                               interrupts = <2>;
>>                               clocks = <&v2m_sysctl 0>, <&v2m_sysctl 1>, <&smbclk>;
>>                               clock-names = "timclken1", "timclken2", "apb_pclk";
>> +                             arm,sp804-clocksource = <0x20>;
>> +                             arm,sp804-clockevent = <0>;
>>                       };
>>
>>                       v2m_timer23: timer@12000 {
>> diff --git a/arch/arm/boot/dts/vexpress-v2p-ca9.dts b/arch/arm/boot/dts/vexpress-v2p-ca9.dts
>> index 1420bb1..a2eba95 100644
>> --- a/arch/arm/boot/dts/vexpress-v2p-ca9.dts
>> +++ b/arch/arm/boot/dts/vexpress-v2p-ca9.dts
>> @@ -98,6 +98,8 @@
>>                            <0 49 4>;
>>               clocks = <&oscclk2>, <&oscclk2>;
>>               clock-names = "timclk", "apb_pclk";
>> +             arm,sp804-clocksource = <0x20>;
>> +             arm,sp804-clockevent = <0>;
>>       };
>>
>>       watchdog@100e5000 {
>
> I love the possibility of removing the SP804 code from the VE machine
> (thanks for your work!), but I'd like to avoid it being replaced by
> cryptic properties in the tree.
>
> Now, to get this right: I'm not saying that there should be completly no
> OS-specific (but neutral) properties for the clocking system.
>
> I'm saying that in "my" (the VE) case, there are four, effectively
> identical timers (each pair sharing an interrupt though). Every single
> one of them is perfectly capable of serving as clocksource or
> clockevent. Therefore I believe the driver/framework/OS should make its
> own decision.
>
I can move those properties into dts file instead.

If clocksource uses alias to configure source or event. There's still a lot
of work. At least we need to split dual timer into two timer devices in DTS.
Pawel Moll March 13, 2013, 11:46 a.m. UTC | #3
On Wed, 2013-03-13 at 11:42 +0000, Haojian Zhuang wrote:
> > I love the possibility of removing the SP804 code from the VE machine
> > (thanks for your work!), but I'd like to avoid it being replaced by
> > cryptic properties in the tree.
> >
> > Now, to get this right: I'm not saying that there should be completly no
> > OS-specific (but neutral) properties for the clocking system.
> >
> > I'm saying that in "my" (the VE) case, there are four, effectively
> > identical timers (each pair sharing an interrupt though). Every single
> > one of them is perfectly capable of serving as clocksource or
> > clockevent. Therefore I believe the driver/framework/OS should make its
> > own decision.
> >
> I can move those properties into dts file instead.

I'm not sure I understand you here...

> If clocksource uses alias to configure source or event. There's still a lot
> of work. At least we need to split dual timer into two timer devices in DTS.

My point is: at least for VE there is _no need_ for any alias or special
property in the tree (DTS). All timers are equal. The system can (and
should) pick any one it wants.

Paweł
Haojian Zhuang March 13, 2013, 12:21 p.m. UTC | #4
On 13 March 2013 19:46, Pawel Moll <pawel.moll@arm.com> wrote:
> On Wed, 2013-03-13 at 11:42 +0000, Haojian Zhuang wrote:
>> > I love the possibility of removing the SP804 code from the VE machine
>> > (thanks for your work!), but I'd like to avoid it being replaced by
>> > cryptic properties in the tree.
>> >
>> > Now, to get this right: I'm not saying that there should be completly no
>> > OS-specific (but neutral) properties for the clocking system.
>> >
>> > I'm saying that in "my" (the VE) case, there are four, effectively
>> > identical timers (each pair sharing an interrupt though). Every single
>> > one of them is perfectly capable of serving as clocksource or
>> > clockevent. Therefore I believe the driver/framework/OS should make its
>> > own decision.
>> >
>> I can move those properties into dts file instead.
>
> I'm not sure I understand you here...
>
>> If clocksource uses alias to configure source or event. There's still a lot
>> of work. At least we need to split dual timer into two timer devices in DTS.
>
> My point is: at least for VE there is _no need_ for any alias or special
> property in the tree (DTS). All timers are equal. The system can (and
> should) pick any one it wants.
>
> Paweł
>
Do you mean that system wants to pick any one randomly? It's impossible.
Imagine that only TIMINT1 signal in sp804 is routed to interrupt controller.
Could system still pick any one it wants?

I think that clocksource or clockevent must be specified in DTS. Whatever
we're using alias or property.
Pawel Moll March 13, 2013, 2:48 p.m. UTC | #5
On Wed, 2013-03-13 at 12:21 +0000, Haojian Zhuang wrote:
> Do you mean that system wants to pick any one randomly? It's impossible.

It is possible in case of the VE platform. Therefore it is not
completely impossible. QED :-)

> Imagine that only TIMINT1 signal in sp804 is routed to interrupt controller.
> Could system still pick any one it wants?

If you tell the system that only TIMINT1 is wired, it shouldn't even try
to use the second timer for anything that would require the interrupt.
Therefore, describing the hardware sufficiently, you're giving the
system enough information to make the right decisions.

> I think that clocksource or clockevent must be specified in DTS. Whatever
> we're using alias or property.

I'm convinced it does not *must* be there. I think it *can* be there,
for platforms where the available timers are not equal (for whatever
reason). My point is: why are you trying to force me to put some
information into my hardware description which doesn't have any meaning
for me?

Paweł
Haojian Zhuang March 13, 2013, 3:01 p.m. UTC | #6
On 13 March 2013 22:48, Pawel Moll <pawel.moll@arm.com> wrote:
> On Wed, 2013-03-13 at 12:21 +0000, Haojian Zhuang wrote:
>> Do you mean that system wants to pick any one randomly? It's impossible.
>
> It is possible in case of the VE platform. Therefore it is not
> completely impossible. QED :-)
>
Yes, it's possible. Since only clocksource / clockevent is using SP804 timer
on the VE platform. If user app or something else is using other SP804
timer, it would be mess because of resource conflict.

>> Imagine that only TIMINT1 signal in sp804 is routed to interrupt controller.
>> Could system still pick any one it wants?
>
Then we need another new property. I try to find using minim properties.

> If you tell the system that only TIMINT1 is wired, it shouldn't even try
> to use the second timer for anything that would require the interrupt.
> Therefore, describing the hardware sufficiently, you're giving the
> system enough information to make the right decisions.
>
>> I think that clocksource or clockevent must be specified in DTS. Whatever
>> we're using alias or property.
>
> I'm convinced it does not *must* be there. I think it *can* be there,
> for platforms where the available timers are not equal (for whatever
> reason). My point is: why are you trying to force me to put some
> information into my hardware description which doesn't have any meaning
> for me?
>
As I mentioned above, system must know the behavior first. The upper layer
software could know which timer is used in clocksource or clockevent.

For example, your Android system could be shared on multiple platforms. It
could parse hardware information from DTS. (Of course, it hasn't this feature
yet). Then it could arrange apps to use other timers.

It's like you define console on specified UART even you have multiples UART
port.
Pawel Moll March 13, 2013, 3:19 p.m. UTC | #7
On Wed, 2013-03-13 at 15:01 +0000, Haojian Zhuang wrote:
> Yes, it's possible. Since only clocksource / clockevent is using SP804 timer
> on the VE platform. If user app or something else is using other SP804
> timer, it would be mess because of resource conflict.

How would you expose this device to the app or something else? It's an
amba_bus device, so it must be bound with an amba_driver. Otherwise
you're abusing the device model and, well, you're on your own.

The devices are managed by the system. The device tree is merely a
description of the available hardware.

> >> Imagine that only TIMINT1 signal in sp804 is routed to interrupt controller.
> >> Could system still pick any one it wants?
> >
> Then we need another new property. I try to find using minim properties.

No, we don't. See the discussion with Rob. He has an idea (using the
interrupt property only), I have another one (using already defined
interrupt-names property). I see you've just came with another one
(TIMINTC treated as TIMINT1), which is fine with me. Either way you've
got what you need to make the right decision.

> As I mentioned above, system must know the behavior first. The upper layer
> software could know which timer is used in clocksource or clockevent.

I claim otherwise. The upper layer software should know what does the
hardware offer and based on this knowledge use it in a way it wants to.

> For example, your Android system could be shared on multiple platforms. It
> could parse hardware information from DTS. (Of course, it hasn't this feature
> yet). Then it could arrange apps to use other timers.

I honestly don't know what are you suggesting here other than it looks
like an abuse to the device model.

> It's like you define console on specified UART even you have multiples UART
> port.

Where do you do this? In the Device Tree? No. In the kernel command
line, which is a runtime argument (the fact it is actually being passed
through the tree is irrelevant here) You have the serialX aliases, which
- again - simply describe the hardware ("this node is connected to a
connector labeled as serial X"). The tree doesn't tell that the user
connected his serial cable to serial1 so it should be used as the
console.

Paweł
Haojian Zhuang March 13, 2013, 3:59 p.m. UTC | #8
On 13 March 2013 23:19, Pawel Moll <pawel.moll@arm.com> wrote:
> On Wed, 2013-03-13 at 15:01 +0000, Haojian Zhuang wrote:
>> Yes, it's possible. Since only clocksource / clockevent is using SP804 timer
>> on the VE platform. If user app or something else is using other SP804
>> timer, it would be mess because of resource conflict.
>
> How would you expose this device to the app or something else? It's an
> amba_bus device, so it must be bound with an amba_driver. Otherwise
> you're abusing the device model and, well, you're on your own.
>
> The devices are managed by the system. The device tree is merely a
> description of the available hardware.
>
Maybe I didn't express my idea well. My point is that timer could be
clock source
/clock event. It could also just be a timer that user app or other device driver
may want to use it directly. There're a lot of reason that user app or some
 driver wants to use hardware timer. For example, the specific timer may
keep running even system is in low power mode.

Although there's so much hardware timers available, system must choose
one for the usage of clock source / clock event / other hardware timer usage.

>> >> Imagine that only TIMINT1 signal in sp804 is routed to interrupt controller.
>> >> Could system still pick any one it wants?
>> >
>> Then we need another new property. I try to find using minim properties.
>
> No, we don't. See the discussion with Rob. He has an idea (using the
> interrupt property only), I have another one (using already defined
> interrupt-names property). I see you've just came with another one
> (TIMINTC treated as TIMINT1), which is fine with me. Either way you've
> got what you need to make the right decision.
>
After considered as second time, I think that we still need a property to meet
alias usage. Excuse me expand it in this topic.

arm,sp804-has-irq = <offset>;
The offset means TIMER register offset. If it's not zero, TIMER2 is using
irq. And it could be configured as clock event. Otherwise, TIMER1 is clock
event.
We must make sure that only one of all sp804 timers is using this property.

alias:
linux,clocksource = <&sp804-A>
linux,clockevent = <&sp804-A>

Although both of them points to timerA, only TIMER1 or TIMER2 of sp804
is using irq. So one is clock source, and the other is clock event.

alias:
linux,clocksource = <&sp804-A>
linux,clockevent = <&sp804-B>
The issue comes since system don't know whether TIMER1 or TIMER2 could
be configured as clock source.

We still need a new property to claim it. It seems that we can't drop
"arm,sp804-clocksource", although everybody don't like it.

What's your opinion?

>
> Where do you do this? In the Device Tree? No. In the kernel command
> line
Yes, command line could also be filled in DTS.
Pawel Moll March 13, 2013, 4:28 p.m. UTC | #9
On Wed, 2013-03-13 at 15:59 +0000, Haojian Zhuang wrote:
> Maybe I didn't express my idea well. My point is that timer could be
> clock source
> /clock event. It could also just be a timer that user app or other device driver
> may want to use it directly. There're a lot of reason that user app or some
>  driver wants to use hardware timer. For example, the specific timer may
> keep running even system is in low power mode.

That's all fair enough. My point is in any case the *system* will take
care of setting this all up. Therefore it can manage the available
resources to avoid the conflict.

> Although there's so much hardware timers available, system must choose
> one for the usage of clock source / clock event / other hardware timer usage.

By all means. I'm just claiming that in normal situation it can choose
on its convenience. In other words - if there are no limitations in
hardware, it can pick any of them, being responsible for making the
right decision.

> >> >> Imagine that only TIMINT1 signal in sp804 is routed to interrupt controller.
> >> >> Could system still pick any one it wants?
> >> >
> >> Then we need another new property. I try to find using minim properties.
> >
> > No, we don't. See the discussion with Rob. He has an idea (using the
> > interrupt property only), I have another one (using already defined
> > interrupt-names property). I see you've just came with another one
> > (TIMINTC treated as TIMINT1), which is fine with me. Either way you've
> > got what you need to make the right decision.
> >
> After considered as second time, I think that we still need a property to meet
> alias usage. Excuse me expand it in this topic.
> 
> arm,sp804-has-irq = <offset>;
> The offset means TIMER register offset. If it's not zero, TIMER2 is using
> irq. And it could be configured as clock event. Otherwise, TIMER1 is clock
> event.
> We must make sure that only one of all sp804 timers is using this property.
> 
> alias:
> linux,clocksource = <&sp804-A>
> linux,clockevent = <&sp804-A>
> 
> Although both of them points to timerA, only TIMER1 or TIMER2 of sp804
> is using irq. So one is clock source, and the other is clock event.
> 
> alias:
> linux,clocksource = <&sp804-A>
> linux,clockevent = <&sp804-B>
> The issue comes since system don't know whether TIMER1 or TIMER2 could
> be configured as clock source.
> 
> We still need a new property to claim it. It seems that we can't drop
> "arm,sp804-clocksource", although everybody don't like it.
> 
> What's your opinion?

Let me repeat myself - I'm not saying that there must be no such thing
like the alias (or your property). I'm sure there will be a use case for
them.

What I am saying is that they should not be obligatory. Shortly speaking
- I don't want to have them in VE DTS files. For VE platform the driver
should pick one of the timers and use it as clocksource (I don't care
which one), another one as clockevent (again, it's up to you) and the
rest for whatever it wants. 

Could you please provide a solution for such a "generic" case, when
there are no aliases defined in the tree? I see no problem with checking
if the aliases exist and if they don't picking the first SP804 you can
get hold of. Just give me that and you won't hear me complaining
further :-)

> > Where do you do this? In the Device Tree? No. In the kernel command
> > line
> Yes, command line could also be filled in DTS.

Which is a very bad practice... There's a reason why "chosen" node is
named like this ;-)

Paweł
Rob Herring March 13, 2013, 4:32 p.m. UTC | #10
On 03/13/2013 10:59 AM, Haojian Zhuang wrote:
> On 13 March 2013 23:19, Pawel Moll <pawel.moll@arm.com> wrote:
>> On Wed, 2013-03-13 at 15:01 +0000, Haojian Zhuang wrote:
>>> Yes, it's possible. Since only clocksource / clockevent is using SP804 timer
>>> on the VE platform. If user app or something else is using other SP804
>>> timer, it would be mess because of resource conflict.
>>
>> How would you expose this device to the app or something else? It's an
>> amba_bus device, so it must be bound with an amba_driver. Otherwise
>> you're abusing the device model and, well, you're on your own.
>>
>> The devices are managed by the system. The device tree is merely a
>> description of the available hardware.
>>
> Maybe I didn't express my idea well. My point is that timer could be
> clock source
> /clock event. It could also just be a timer that user app or other device driver
> may want to use it directly. There're a lot of reason that user app or some
>  driver wants to use hardware timer. For example, the specific timer may
> keep running even system is in low power mode.
> 
> Although there's so much hardware timers available, system must choose
> one for the usage of clock source / clock event / other hardware timer usage.
> 
>>>>> Imagine that only TIMINT1 signal in sp804 is routed to interrupt controller.
>>>>> Could system still pick any one it wants?
>>>>
>>> Then we need another new property. I try to find using minim properties.
>>
>> No, we don't. See the discussion with Rob. He has an idea (using the
>> interrupt property only), I have another one (using already defined
>> interrupt-names property). I see you've just came with another one
>> (TIMINTC treated as TIMINT1), which is fine with me. Either way you've
>> got what you need to make the right decision.
>>
> After considered as second time, I think that we still need a property to meet
> alias usage. Excuse me expand it in this topic.
> 
> arm,sp804-has-irq = <offset>;

Interrupt/timer number not offset please. The offset is implied by the
fact that the h/w is an sp804. The Integrator AP is not actually an
sp804, so it's init should be handled differently with a different
compatible property.

> The offset means TIMER register offset. If it's not zero, TIMER2 is using
> irq. And it could be configured as clock event. Otherwise, TIMER1 is clock
> event.
> We must make sure that only one of all sp804 timers is using this property.
> 
> alias:
> linux,clocksource = <&sp804-A>
> linux,clockevent = <&sp804-A>
> 
> Although both of them points to timerA, only TIMER1 or TIMER2 of sp804
> is using irq. So one is clock source, and the other is clock event.
> 
> alias:
> linux,clocksource = <&sp804-A>
> linux,clockevent = <&sp804-B>
> The issue comes since system don't know whether TIMER1 or TIMER2 could
> be configured as clock source.

If you have 2 sp804's, why do you care which one is used by the OS? If
they are identical, then it doesn't matter. If they are not, then
describe how they are not identical.

The realview and versatile boards currently do this, but there is no
good reason that I see and I intend to change this.

> We still need a new property to claim it. It seems that we can't drop
> "arm,sp804-clocksource", although everybody don't like it.
> 
> What's your opinion?

As previously stated, I don't think we need this or the alias based on
current in tree users.

There is an issue that you don't know about all available timers so you
can select the one with the minimum required features leaving others
with more features (i.e. an irq) free. If you need more control like
this then you can do custom setup before or instead of calling
clocksource_of_init. None of the current platforms have this issue nor
do they use sp804 for anything beyond clksrc and clkevt. I would not
expect lots of new users of sp804 as I expect most new platforms will
use architected timers. So let's not solve imaginary problems now.

Rob
Russell King - ARM Linux March 15, 2013, 12:34 p.m. UTC | #11
On Wed, Mar 13, 2013 at 11:32:01AM -0500, Rob Herring wrote:
> If you have 2 sp804's, why do you care which one is used by the OS? If
> they are identical, then it doesn't matter. If they are not, then
> describe how they are not identical.
> 
> The realview and versatile boards currently do this, but there is no
> good reason that I see and I intend to change this.

It is highly likely that the timer choice on these boards was made by
independent people, who came up with different random choices.  Nothing
more than that - I believe it to be entirely arbitary.  I do think
discussion is getting too bogged down on this point.

So, we could make the decision about which timer to use for the clock
event based on whether an interrupt is specified, the known clock rate
and size of the timer register.

Same can be done for the clock source, but preferring a timer without
an interrupt in preference to one with an interrupt.

We do have this nice rating system for clocksources and clockevents -
and it could be more flexible if it didn't separate clocksources from
clockevents - allowing just a single "timer" and marking it as being
suitable as a clocksource or a clockevent, or indeed both.

However, for SP804 selection, there are cases where it matters, such as
on Versatile Express where you have to select the right set of SP804 to
have a working system (because ARM didn't fully document the SP804 setup
there) and we have no way to switch some of those timers from their
32kHz clock source.  But then we could just omit the clock source for
the "bad" SP804s which would prevent them being used.
Pawel Moll March 15, 2013, 12:58 p.m. UTC | #12
On Fri, 2013-03-15 at 12:34 +0000, Russell King - ARM Linux wrote:
> However, for SP804 selection, there are cases where it matters, such as
> on Versatile Express where you have to select the right set of SP804 to
> have a working system (because ARM didn't fully document the SP804 setup
> there) and we have no way to switch some of those timers from their
> 32kHz clock source.  But then we could just omit the clock source for
> the "bad" SP804s which would prevent them being used.

Both VE motherboard's SP804s TIMCLKENx inputs are fed by the SP810's
outputs:

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

v2m_timer23: timer@12000 {
	compatible = "arm,sp804", "arm,primecell";
	reg = <0x12000 0x1000>;
	interrupts = <3>;
	clocks = <&v2m_sysctl 2>, <&v2m_sysctl 3>, <&smbclk>;
	clock-names = "timclken1", "timclken2", "apb_pclk";
};

TIMCLKs are wired to the SMB_CLK, so the effective rate depends on the
TIMCLKENx only.

The SP810 takes the 32kHz and 1MHz reference clocks:

v2m_sysctl: sysctl@01000 {
	compatible = "arm,sp810", "arm,primecell";
	reg = <0x01000 0x1000>;
	clocks = <&v2m_refclk32khz>, <&v2m_refclk1mhz>, <&smbclk>;
	clock-names = "refclk", "timclk", "apb_pclk";
	#clock-cells = <1>;
	clock-output-names = "timerclken0", "timerclken1", "timerclken2", "timerclken3";
};

and the code in drivers/clk/versatile/clk-vexpress.c picks up the faster
one as the parent.

The V2P-CA9 SP804's TIMCLK is wired to one of the main clock generators:

timer@100e4000 {
	compatible = "arm,sp804", "arm,primecell";
	reg = <0x100e4000 0x1000>;
	interrupts = <0 48 4>,
	             <0 49 4>;
	clocks = <&oscclk2>, <&oscclk2>;
	clock-names = "timclk", "apb_pclk";
};

Both TIMCLKEN0 & 1 are pulled up to 1 so they have no effect on the
timer rates.

Overall, the description of the VE clocking in the Device Tree is rather
complete.

Paweł
Russell King - ARM Linux March 15, 2013, 6:10 p.m. UTC | #13
On Fri, Mar 15, 2013 at 12:58:32PM +0000, Pawel Moll wrote:
> On Fri, 2013-03-15 at 12:34 +0000, Russell King - ARM Linux wrote:
> > However, for SP804 selection, there are cases where it matters, such as
> > on Versatile Express where you have to select the right set of SP804 to
> > have a working system (because ARM didn't fully document the SP804 setup
> > there) and we have no way to switch some of those timers from their
> > 32kHz clock source.  But then we could just omit the clock source for
> > the "bad" SP804s which would prevent them being used.
> 
> Both VE motherboard's SP804s TIMCLKENx inputs are fed by the SP810's
> outputs:

Sigh.

You do remember who (re)wrote the Versatile Express support that's in
mainline... I know that the motherboard SP804s are clocked from the
SP810 (which has no published documentation) and these are selectable
between 1MHz and 32kHz.

What I was referring to is the SP804s on the CT9x4 tile, which seem
to be clocked at 32kHz with no way to change them, and it's that which
has been totally and utterly undocumented (believe me, when I did that
work above, I searched the available documentation many times.)

It may be that as a result of my complaints to ARM about that, the
documentation has since been fixed.  Whether it makes sense to use
the CT9x4 SP804s over the motherboards, I've no idea because I've been
forced out of the loop on Versatile Express.
diff mbox

Patch

diff --git a/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi b/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi
index ac870fb..3fa798f 100644
--- a/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi
+++ b/arch/arm/boot/dts/vexpress-v2m-rs1.dtsi
@@ -183,6 +183,8 @@ 
 				interrupts = <2>;
 				clocks = <&v2m_sysctl 0>, <&v2m_sysctl 1>, <&smbclk>;
 				clock-names = "timclken1", "timclken2", "apb_pclk";
+				arm,sp804-clocksource = <0x20>;
+				arm,sp804-clockevent = <0>;
 			};
 
 			v2m_timer23: timer@120000 {
diff --git a/arch/arm/boot/dts/vexpress-v2m.dtsi b/arch/arm/boot/dts/vexpress-v2m.dtsi
index f142036..86e4046 100644
--- a/arch/arm/boot/dts/vexpress-v2m.dtsi
+++ b/arch/arm/boot/dts/vexpress-v2m.dtsi
@@ -182,6 +182,8 @@ 
 				interrupts = <2>;
 				clocks = <&v2m_sysctl 0>, <&v2m_sysctl 1>, <&smbclk>;
 				clock-names = "timclken1", "timclken2", "apb_pclk";
+				arm,sp804-clocksource = <0x20>;
+				arm,sp804-clockevent = <0>;
 			};
 
 			v2m_timer23: timer@12000 {
diff --git a/arch/arm/boot/dts/vexpress-v2p-ca9.dts b/arch/arm/boot/dts/vexpress-v2p-ca9.dts
index 1420bb1..a2eba95 100644
--- a/arch/arm/boot/dts/vexpress-v2p-ca9.dts
+++ b/arch/arm/boot/dts/vexpress-v2p-ca9.dts
@@ -98,6 +98,8 @@ 
 			     <0 49 4>;
 		clocks = <&oscclk2>, <&oscclk2>;
 		clock-names = "timclk", "apb_pclk";
+		arm,sp804-clocksource = <0x20>;
+		arm,sp804-clockevent = <0>;
 	};
 
 	watchdog@100e5000 {
diff --git a/arch/arm/mach-vexpress/Kconfig b/arch/arm/mach-vexpress/Kconfig
index 52d315b..ff78c15 100644
--- a/arch/arm/mach-vexpress/Kconfig
+++ b/arch/arm/mach-vexpress/Kconfig
@@ -5,6 +5,7 @@  config ARCH_VEXPRESS
 	select ARM_GIC
 	select ARM_TIMER_SP804
 	select CLKDEV_LOOKUP
+	select CLKSRC_OF
 	select COMMON_CLK
 	select COMMON_CLK_VERSATILE
 	select CPU_V7
diff --git a/arch/arm/mach-vexpress/v2m.c b/arch/arm/mach-vexpress/v2m.c
index 51e8701..bb599d3 100644
--- a/arch/arm/mach-vexpress/v2m.c
+++ b/arch/arm/mach-vexpress/v2m.c
@@ -21,6 +21,7 @@ 
 #include <linux/regulator/fixed.h>
 #include <linux/regulator/machine.h>
 #include <linux/vexpress.h>
+#include <linux/clocksource.h>
 #include <clocksource/arm_timer.h>
 #include <clocksource/timer-sp.h>
 
@@ -431,19 +432,9 @@  void __init v2m_dt_init_early(void)
 
 static void __init v2m_dt_timer_init(void)
 {
-	struct device_node *node = NULL;
-
 	vexpress_clk_of_init();
 
-	do {
-		node = of_find_compatible_node(node, NULL, "arm,sp804");
-	} while (node && vexpress_get_site_by_node(node) != VEXPRESS_SITE_MB);
-	if (node) {
-		pr_info("Using SP804 '%s' as a clock & events source\n",
-				node->full_name);
-		v2m_sp804_init(of_iomap(node, 0),
-				irq_of_parse_and_map(node, 0));
-	}
+	clocksource_of_init();
 
 	if (arch_timer_of_register() != 0)
 		twd_local_timer_of_register();