[4/7] dt-bindings: chosen: Add clocksource and clockevent selection

Commit Message

Claudiu Beznea Sept. 10, 2019, 1:47 p.m. UTC

From: Alexandre Belloni <[e-mail]>

Some timer drivers may behave either as clocksource or clockevent
or both. Until now, in case of platforms with multiple hardware
resources of the same type, the drivers were chosing the first
registered hardware resource as clocksource/clockevent and the
next one as clockevent/clocksource. Other were using different
compatibles (one for each functionality, although its about the
same hardware). Add DT bindings to be able to choose the
functionality of a timer.

Signed-off-by: Alexandre Belloni <[e-mail]>
Signed-off-by: Claudiu Beznea <[e-mail]>
---
 Documentation/devicetree/bindings/chosen.txt | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Comments

Sudeep Holla Sept. 10, 2019, 2:32 p.m. UTC | #1

On Tue, Sep 10, 2019 at 04:47:13PM +0300, Claudiu Beznea wrote:
> From: Alexandre Belloni <[e-mail]>
>
> Some timer drivers may behave either as clocksource or clockevent
> or both. Until now, in case of platforms with multiple hardware
> resources of the same type, the drivers were chosing the first
> registered hardware resource as clocksource/clockevent and the
> next one as clockevent/clocksource. Other were using different
> compatibles (one for each functionality, although its about the
> same hardware). Add DT bindings to be able to choose the
> functionality of a timer.
>

Is the piece of hardware not capable of serving as both clocksource
and clockevent or is it just the platform choice ?

--
Regards,
Sudeep

Claudiu Beznea Sept. 10, 2019, 2:51 p.m. UTC | #2

On 10.09.2019 17:32, Sudeep Holla wrote:
> External E-Mail
> 
> 
> On Tue, Sep 10, 2019 at 04:47:13PM +0300, Claudiu Beznea wrote:
>> From: Alexandre Belloni <[e-mail]>
>>
>> Some timer drivers may behave either as clocksource or clockevent
>> or both. Until now, in case of platforms with multiple hardware
>> resources of the same type, the drivers were chosing the first
>> registered hardware resource as clocksource/clockevent and the
>> next one as clockevent/clocksource. Other were using different
>> compatibles (one for each functionality, although its about the
>> same hardware). Add DT bindings to be able to choose the
>> functionality of a timer.
>>
> 
> Is the piece of hardware not capable of serving as both clocksource
> and clockevent or is it just the platform choice ?

In my case, the hardware is not capable of serving at the same time
a clocksource device and a clockevent device.

First, I published v1 for a hardware device having this behavior at
[1] requesting 1st probed hardware device to work as clocksource and
the 2nd one to work as clockevent. The discussion at [1] ended up with
the idea of having a mechanism to specify which hardware device behaves
as clocksource and which one behaves as clockevent.

Thank you,
Claudiu Beznea

[1] https://lore.kernel.org/lkml/[e-mail]/

> 
> --
> Regards,
> Sudeep
> 
>

Sudeep Holla Sept. 10, 2019, 3:08 p.m. UTC | #3

On Tue, Sep 10, 2019 at 02:51:50PM +0000, [e-mail] wrote:
> 
> 
> On 10.09.2019 17:32, Sudeep Holla wrote:
> > External E-Mail
> > 
> > 
> > On Tue, Sep 10, 2019 at 04:47:13PM +0300, Claudiu Beznea wrote:
> >> From: Alexandre Belloni <[e-mail]>
> >>
> >> Some timer drivers may behave either as clocksource or clockevent
> >> or both. Until now, in case of platforms with multiple hardware
> >> resources of the same type, the drivers were chosing the first
> >> registered hardware resource as clocksource/clockevent and the
> >> next one as clockevent/clocksource. Other were using different
> >> compatibles (one for each functionality, although its about the
> >> same hardware). Add DT bindings to be able to choose the
> >> functionality of a timer.
> >>
> > 
> > Is the piece of hardware not capable of serving as both clocksource
> > and clockevent or is it just the platform choice ?
> 
> In my case, the hardware is not capable of serving at the same time
> a clocksource device and a clockevent device.
> 
> First, I published v1 for a hardware device having this behavior at
> [1] requesting 1st probed hardware device to work as clocksource and
> the 2nd one to work as clockevent. The discussion at [1] ended up with
> the idea of having a mechanism to specify which hardware device behaves
> as clocksource and which one behaves as clockevent.
>

In that case, why can't we identify capability that with the compatibles
for this timer IP ?

IOW, I don't like the proposal as it's hardware limitation.

--
Regards,
Sudeep

Alexandre Belloni Sept. 10, 2019, 3:10 p.m. UTC | #4

On 10/09/2019 16:08:26+0100, Sudeep Holla wrote:
> On Tue, Sep 10, 2019 at 02:51:50PM +0000, [e-mail] wrote:
> > 
> > 
> > On 10.09.2019 17:32, Sudeep Holla wrote:
> > > External E-Mail
> > > 
> > > 
> > > On Tue, Sep 10, 2019 at 04:47:13PM +0300, Claudiu Beznea wrote:
> > >> From: Alexandre Belloni <[e-mail]>
> > >>
> > >> Some timer drivers may behave either as clocksource or clockevent
> > >> or both. Until now, in case of platforms with multiple hardware
> > >> resources of the same type, the drivers were chosing the first
> > >> registered hardware resource as clocksource/clockevent and the
> > >> next one as clockevent/clocksource. Other were using different
> > >> compatibles (one for each functionality, although its about the
> > >> same hardware). Add DT bindings to be able to choose the
> > >> functionality of a timer.
> > >>
> > > 
> > > Is the piece of hardware not capable of serving as both clocksource
> > > and clockevent or is it just the platform choice ?
> > 
> > In my case, the hardware is not capable of serving at the same time
> > a clocksource device and a clockevent device.
> > 
> > First, I published v1 for a hardware device having this behavior at
> > [1] requesting 1st probed hardware device to work as clocksource and
> > the 2nd one to work as clockevent. The discussion at [1] ended up with
> > the idea of having a mechanism to specify which hardware device behaves
> > as clocksource and which one behaves as clockevent.
> >
> 
> In that case, why can't we identify capability that with the compatibles
> for this timer IP ?
> 
> IOW, I don't like the proposal as it's hardware limitation.
> 

To be clear, bot timers are exactly the same but can't be clocksource
and clockevent at the same time. Why would we have different compatibles
for the exact same IP?

Linus Walleij Sept. 11, 2019, 12:03 a.m. UTC | #5

On Tue, Sep 10, 2019 at 4:11 PM Alexandre Belloni
<[e-mail]> wrote:
> On 10/09/2019 16:08:26+0100, Sudeep Holla wrote:
> > On Tue, Sep 10, 2019 at 02:51:50PM +0000, [e-mail] wrote:

> > In that case, why can't we identify capability that with the compatibles
> > for this timer IP ?
> >
> > IOW, I don't like the proposal as it's hardware limitation.
>
> To be clear, bot timers are exactly the same but can't be clocksource
> and clockevent at the same time. Why would we have different compatibles
> for the exact same IP?

In that case why not just pick the first one you find as clocksource
and the second one as clock event? As they all come to the
same timer of init function two simple local state variables can
solve that:

static bool registered_clocksource;
static bool registered_clockevent;

probe(timer) {
   if (!registered_clocksource) {
       register_clocksource(timer);
       registrered_clocksource = true;
       return;
   }
   if (!registered_clockevent) {
       register_clockevent(timer);
       registered_clockevent = true;
       return;
   }
   pr_info("surplus timer %p\n", timer);
}

Clocksource and clockevent are natural singletons so there is
no need to handle more than one of each in a driver for identical
hardware.

With the Integrator AP timer there is a real reason to select one over
the other but as I replied to that patch it is pretty easy to just identify
which block has this limitation by simply commenting out the IRQ
line for it from the device tree.

Maybe there is something about this I don't understand.

Yours,
Linus Walleij

Claudiu Beznea Sept. 11, 2019, 7:18 a.m. UTC | #6

On 11.09.2019 03:03, Linus Walleij wrote:
> External E-Mail
> 
> 
> On Tue, Sep 10, 2019 at 4:11 PM Alexandre Belloni
> <[e-mail]> wrote:
>> On 10/09/2019 16:08:26+0100, Sudeep Holla wrote:
>>> On Tue, Sep 10, 2019 at 02:51:50PM +0000, [e-mail] wrote:
> 
>>> In that case, why can't we identify capability that with the compatibles
>>> for this timer IP ?
>>>
>>> IOW, I don't like the proposal as it's hardware limitation.
>>
>> To be clear, bot timers are exactly the same but can't be clocksource
>> and clockevent at the same time. Why would we have different compatibles
>> for the exact same IP?
> 
> In that case why not just pick the first one you find as clocksource
> and the second one as clock event? As they all come to the
> same timer of init function two simple local state variables can
> solve that:
> 
> static bool registered_clocksource;
> static bool registered_clockevent;
> 
> probe(timer) {
>    if (!registered_clocksource) {
>        register_clocksource(timer);
>        registrered_clocksource = true;
>        return;
>    }
>    if (!registered_clockevent) {
>        register_clockevent(timer);
>        registered_clockevent = true;
>        return;
>    }
>    pr_info("surplus timer %p\n", timer);
> }
> 

That was also my proposal for the driver I'm sending this series for (see
[1]) but it has been proposed to implement a mechanism similar to this one
in this series (see [2] and [3]).

Thank you,
Claudiu Beznea

[1]
https://lore.kernel.org/lkml/[e-mail]/
[2]
https://lore.kernel.org/lkml/[e-mail]/
[3]
https://lore.kernel.org/lkml/[e-mail]/

> Clocksource and clockevent are natural singletons so there is
> no need to handle more than one of each in a driver for identical
> hardware.
> 
> With the Integrator AP timer there is a real reason to select one over
> the other but as I replied to that patch it is pretty easy to just identify
> which block has this limitation by simply commenting out the IRQ
> line for it from the device tree.
> 
> Maybe there is something about this I don't understand.
> 
> Yours,
> Linus Walleij
>

Neil Armstrong Sept. 11, 2019, 7:34 a.m. UTC | #7

Hi,

On 10/09/2019 15:47, Claudiu Beznea wrote:
> From: Alexandre Belloni <[e-mail]>
> 
> Some timer drivers may behave either as clocksource or clockevent
> or both. Until now, in case of platforms with multiple hardware
> resources of the same type, the drivers were chosing the first
> registered hardware resource as clocksource/clockevent and the
> next one as clockevent/clocksource. Other were using different
> compatibles (one for each functionality, although its about the
> same hardware). Add DT bindings to be able to choose the
> functionality of a timer.
> 
> Signed-off-by: Alexandre Belloni <[e-mail]>
> Signed-off-by: Claudiu Beznea <[e-mail]>
> ---
>  Documentation/devicetree/bindings/chosen.txt | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/chosen.txt b/Documentation/devicetree/bindings/chosen.txt
> index 45e79172a646..aad3034cdbdf 100644
> --- a/Documentation/devicetree/bindings/chosen.txt
> +++ b/Documentation/devicetree/bindings/chosen.txt
> @@ -135,3 +135,23 @@ e.g.
>  		linux,initrd-end = <0x82800000>;
>  	};
>  };
> +
> +linux,clocksource and linux,clockevent
> +--------------------------------------
> +
> +Those nodes have a timer property. This property is a phandle to the timer to be
> +chosen as the clocksource or clockevent. This is only useful when the platform
> +has multiple identical timers and it is not possible to let linux make the
> +correct choice.
> +
> +/ {
> +	chosen {
> +		linux,clocksource {
> +			timer = <&timer0>;
> +		};
> +
> +		linux,clockevent {
> +			timer = <&timer1>;
> +		};
> +	};
> +};
> 

Why not in aliases ?

aliases {
    clocksource0 = &timer0;
    clockevent0 = &timer1;
};

since we can have multiple of each, we should not limit ourselves to 1 clkevent
and 1 clksource.

In the aliases case, each driver would expose both capabilities, and the core would select
what to enable.

Neil

Alexandre Belloni Sept. 11, 2019, 9:14 a.m. UTC | #8

On 11/09/2019 09:34:27+0200, Neil Armstrong wrote:
> Hi,
> 
> On 10/09/2019 15:47, Claudiu Beznea wrote:
> > From: Alexandre Belloni <[e-mail]>
> > 
> > Some timer drivers may behave either as clocksource or clockevent
> > or both. Until now, in case of platforms with multiple hardware
> > resources of the same type, the drivers were chosing the first
> > registered hardware resource as clocksource/clockevent and the
> > next one as clockevent/clocksource. Other were using different
> > compatibles (one for each functionality, although its about the
> > same hardware). Add DT bindings to be able to choose the
> > functionality of a timer.
> > 
> > Signed-off-by: Alexandre Belloni <[e-mail]>
> > Signed-off-by: Claudiu Beznea <[e-mail]>
> > ---
> >  Documentation/devicetree/bindings/chosen.txt | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/chosen.txt b/Documentation/devicetree/bindings/chosen.txt
> > index 45e79172a646..aad3034cdbdf 100644
> > --- a/Documentation/devicetree/bindings/chosen.txt
> > +++ b/Documentation/devicetree/bindings/chosen.txt
> > @@ -135,3 +135,23 @@ e.g.
> >  		linux,initrd-end = <0x82800000>;
> >  	};
> >  };
> > +
> > +linux,clocksource and linux,clockevent
> > +--------------------------------------
> > +
> > +Those nodes have a timer property. This property is a phandle to the timer to be
> > +chosen as the clocksource or clockevent. This is only useful when the platform
> > +has multiple identical timers and it is not possible to let linux make the
> > +correct choice.
> > +
> > +/ {
> > +	chosen {
> > +		linux,clocksource {
> > +			timer = <&timer0>;
> > +		};
> > +
> > +		linux,clockevent {
> > +			timer = <&timer1>;
> > +		};
> > +	};
> > +};
> > 
> 
> Why not in aliases ?
> 
> aliases {
>     clocksource0 = &timer0;
>     clockevent0 = &timer1;
> };
> 
> since we can have multiple of each, we should not limit ourselves to 1 clkevent
> and 1 clksource.
> 
> In the aliases case, each driver would expose both capabilities, and the core would select
> what to enable.
> 

For extendability, you need nodes for that because at some point, you
may need to also be able to select the timer frequency. You can't do
that with an alias.

Linus Walleij Sept. 12, 2019, 2:18 p.m. UTC | #9

On Wed, Sep 11, 2019 at 8:18 AM <[e-mail]> wrote:
> [Me]
> > In that case why not just pick the first one you find as clocksource
> > and the second one as clock event?

> That was also my proposal for the driver I'm sending this series for (see
> [1]) but it has been proposed to implement a mechanism similar to this one
> in this series (see [2] and [3]).

OK I am not going to challenge the clock source maintainers on this,
so if that is what they want then that is what they should get.
It's fine to convert the Integrator driver too!

Yours,
Linus Walleij

Rob Herring Sept. 30, 2019, 2:32 p.m. UTC | #10

On Wed, Sep 11, 2019 at 07:18:07AM +0000, [e-mail] wrote:
> 
> 
> On 11.09.2019 03:03, Linus Walleij wrote:
> > External E-Mail
> > 
> > 
> > On Tue, Sep 10, 2019 at 4:11 PM Alexandre Belloni
> > <[e-mail]> wrote:
> >> On 10/09/2019 16:08:26+0100, Sudeep Holla wrote:
> >>> On Tue, Sep 10, 2019 at 02:51:50PM +0000, [e-mail] wrote:
> > 
> >>> In that case, why can't we identify capability that with the compatibles
> >>> for this timer IP ?
> >>>
> >>> IOW, I don't like the proposal as it's hardware limitation.
> >>
> >> To be clear, bot timers are exactly the same but can't be clocksource
> >> and clockevent at the same time. Why would we have different compatibles
> >> for the exact same IP?
> > 
> > In that case why not just pick the first one you find as clocksource
> > and the second one as clock event? As they all come to the
> > same timer of init function two simple local state variables can
> > solve that:
> > 
> > static bool registered_clocksource;
> > static bool registered_clockevent;
> > 
> > probe(timer) {
> >    if (!registered_clocksource) {
> >        register_clocksource(timer);
> >        registrered_clocksource = true;
> >        return;
> >    }
> >    if (!registered_clockevent) {
> >        register_clockevent(timer);
> >        registered_clockevent = true;
> >        return;
> >    }
> >    pr_info("surplus timer %p\n", timer);
> > }
> > 
> 
> That was also my proposal for the driver I'm sending this series for (see
> [1]) but it has been proposed to implement a mechanism similar to this one
> in this series (see [2] and [3]).

This comes up over and over, and the answer is still no. Either each 
block is identical and doesn't matter which one is used for what or 
there is some h/w difference that you should describe. 

If you want something that would even be considered to put into DT, 
then define something BSD or other OS's could use too. (That's not a 
suggestion to respin this with generalized names.)

Rob

Claudiu Beznea Oct. 2, 2019, 1:32 p.m. UTC | #11

On 30.09.2019 17:32, Rob Herring wrote:
> On Wed, Sep 11, 2019 at 07:18:07AM +0000, [e-mail] wrote:
>>
>>
>> On 11.09.2019 03:03, Linus Walleij wrote:
>>> External E-Mail
>>>
>>>
>>> On Tue, Sep 10, 2019 at 4:11 PM Alexandre Belloni
>>> <[e-mail]> wrote:
>>>> On 10/09/2019 16:08:26+0100, Sudeep Holla wrote:
>>>>> On Tue, Sep 10, 2019 at 02:51:50PM +0000, [e-mail] wrote:
>>>
>>>>> In that case, why can't we identify capability that with the compatibles
>>>>> for this timer IP ?
>>>>>
>>>>> IOW, I don't like the proposal as it's hardware limitation.
>>>>
>>>> To be clear, bot timers are exactly the same but can't be clocksource
>>>> and clockevent at the same time. Why would we have different compatibles
>>>> for the exact same IP?
>>>
>>> In that case why not just pick the first one you find as clocksource
>>> and the second one as clock event? As they all come to the
>>> same timer of init function two simple local state variables can
>>> solve that:
>>>
>>> static bool registered_clocksource;
>>> static bool registered_clockevent;
>>>
>>> probe(timer) {
>>>    if (!registered_clocksource) {
>>>        register_clocksource(timer);
>>>        registrered_clocksource = true;
>>>        return;
>>>    }
>>>    if (!registered_clockevent) {
>>>        register_clockevent(timer);
>>>        registered_clockevent = true;
>>>        return;
>>>    }
>>>    pr_info("surplus timer %p\n", timer);
>>> }
>>>
>>
>> That was also my proposal for the driver I'm sending this series for (see
>> [1]) but it has been proposed to implement a mechanism similar to this one
>> in this series (see [2] and [3]).
> 
> This comes up over and over, and the answer is still no. Either each 
> block is identical and doesn't matter which one is used for what or 
> there is some h/w difference that you should describe. 

There are no hardware differences in my case. The block just cannot work at
the same time  as clocksource and clockevent. And on SAM9X60's we want to
use it as clockevent for high resolution timers support.

> 
> If you want something that would even be considered to put into DT, 
> then define something BSD or other OS's could use too. (That's not a 
> suggestion to respin this with generalized names.)
> 
> Rob
> 
>

Patch

diff --git a/Documentation/devicetree/bindings/chosen.txt b/Documentation/devicetree/bindings/chosen.txt
index 45e79172a646..aad3034cdbdf 100644
--- a/Documentation/devicetree/bindings/chosen.txt
+++ b/Documentation/devicetree/bindings/chosen.txt
@@ -135,3 +135,23 @@  e.g.
 		linux,initrd-end = <0x82800000>;
 	};
 };
+
+linux,clocksource and linux,clockevent
+--------------------------------------
+
+Those nodes have a timer property. This property is a phandle to the timer to be
+chosen as the clocksource or clockevent. This is only useful when the platform
+has multiple identical timers and it is not possible to let linux make the
+correct choice.
+
+/ {
+	chosen {
+		linux,clocksource {
+			timer = <&timer0>;
+		};
+
+		linux,clockevent {
+			timer = <&timer1>;
+		};
+	};
+};