| Message ID | 1568123236-767-5-git-send-email-claudiu.beznea@microchip.com |
|---|---|
| State | New |
| Headers | show |
| Series | add support for clocksource/clockevent DT selection | expand |
On Tue, Sep 10, 2019 at 04:47:13PM +0300, Claudiu Beznea wrote: > From: Alexandre Belloni <alexandre.belloni@bootlin.com> > > 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
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 <alexandre.belloni@bootlin.com> >> >> 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/1552580772-8499-1-git-send-email-claudiu.beznea@microchip.com/ > > -- > Regards, > Sudeep > >
On Tue, Sep 10, 2019 at 02:51:50PM +0000, Claudiu.Beznea@microchip.com 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 <alexandre.belloni@bootlin.com> > >> > >> 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
On 10/09/2019 16:08:26+0100, Sudeep Holla wrote: > On Tue, Sep 10, 2019 at 02:51:50PM +0000, Claudiu.Beznea@microchip.com 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 <alexandre.belloni@bootlin.com> > > >> > > >> 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?
On Tue, Sep 10, 2019 at 4:11 PM Alexandre Belloni <alexandre.belloni@bootlin.com> wrote: > On 10/09/2019 16:08:26+0100, Sudeep Holla wrote: > > On Tue, Sep 10, 2019 at 02:51:50PM +0000, Claudiu.Beznea@microchip.com 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
On 11.09.2019 03:03, Linus Walleij wrote: > External E-Mail > > > On Tue, Sep 10, 2019 at 4:11 PM Alexandre Belloni > <alexandre.belloni@bootlin.com> wrote: >> On 10/09/2019 16:08:26+0100, Sudeep Holla wrote: >>> On Tue, Sep 10, 2019 at 02:51:50PM +0000, Claudiu.Beznea@microchip.com 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/1552580772-8499-1-git-send-email-claudiu.beznea@microchip.com/ [2] https://lore.kernel.org/lkml/a738fce5-1108-34d7-d255-dfcb86f51c56@linaro.org/ [3] https://lore.kernel.org/lkml/2f831f1b-c87d-48bd-cf02-2ebb334b964c@linaro.org/ > 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 >
Hi, On 10/09/2019 15:47, Claudiu Beznea wrote: > From: Alexandre Belloni <alexandre.belloni@bootlin.com> > > 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 <alexandre.belloni@bootlin.com> > Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> > --- > 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
On 11/09/2019 09:34:27+0200, Neil Armstrong wrote: > Hi, > > On 10/09/2019 15:47, Claudiu Beznea wrote: > > From: Alexandre Belloni <alexandre.belloni@bootlin.com> > > > > 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 <alexandre.belloni@bootlin.com> > > Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> > > --- > > 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.
On Wed, Sep 11, 2019 at 8:18 AM <Claudiu.Beznea@microchip.com> 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
On Wed, Sep 11, 2019 at 07:18:07AM +0000, Claudiu.Beznea@microchip.com wrote: > > > On 11.09.2019 03:03, Linus Walleij wrote: > > External E-Mail > > > > > > On Tue, Sep 10, 2019 at 4:11 PM Alexandre Belloni > > <alexandre.belloni@bootlin.com> wrote: > >> On 10/09/2019 16:08:26+0100, Sudeep Holla wrote: > >>> On Tue, Sep 10, 2019 at 02:51:50PM +0000, Claudiu.Beznea@microchip.com 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
On 30.09.2019 17:32, Rob Herring wrote: > On Wed, Sep 11, 2019 at 07:18:07AM +0000, Claudiu.Beznea@microchip.com wrote: >> >> >> On 11.09.2019 03:03, Linus Walleij wrote: >>> External E-Mail >>> >>> >>> On Tue, Sep 10, 2019 at 4:11 PM Alexandre Belloni >>> <alexandre.belloni@bootlin.com> wrote: >>>> On 10/09/2019 16:08:26+0100, Sudeep Holla wrote: >>>>> On Tue, Sep 10, 2019 at 02:51:50PM +0000, Claudiu.Beznea@microchip.com 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 > >
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>; + }; + }; +};