| Message ID | 1372888216-10726-1-git-send-email-soren.brinkmann@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Soren, On Wed, Jul 03, 2013 at 02:50:16PM -0700, Soren Brinkmann wrote: > Reuse the TTC clocksource timer as sched clock, too. Since only a single > sched clock is supported in Linux, this feature optional and can be > selected through Kconfig. > > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > --- [...] > diff --git a/drivers/clocksource/cadence_ttc_timer.c > b/drivers/clocksource/cadence_ttc_timer.c > index 4cbe28c..ef0f52b 100644 > --- a/drivers/clocksource/cadence_ttc_timer.c > +++ b/drivers/clocksource/cadence_ttc_timer.c > @@ -22,6 +22,7 @@ > #include <linux/of_irq.h> > #include <linux/slab.h> > #include <linux/clk-provider.h> > +#include <asm/sched_clock.h> This has changed to linux/sched_clock.h in the tip tree. See https://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/commit/?h=timers/core&id=38ff87f77af0b5a93fc8581cff1d6e5692ab8970. baruch
On Wed, 3 Jul 2013, Soren Brinkmann wrote: > Reuse the TTC clocksource timer as sched clock, too. Since only a single > sched clock is supported in Linux, this feature optional and can be > selected through Kconfig. This changelog doesn't make sense. There can be only one active sched_clock, but that does no mean, that you cannot have different implementations compiled in. So if you disable this config which sched_clock is your kernel using? And if you enable it, how is guaranteed that you end up with the ttc sched_clock as the active one? Just due to initcall ordering? Thanks, tglx
On Fri, Jul 05, 2013 at 08:30:47AM +0200, Thomas Gleixner wrote: > On Wed, 3 Jul 2013, Soren Brinkmann wrote: > > > Reuse the TTC clocksource timer as sched clock, too. Since only a single > > sched clock is supported in Linux, this feature optional and can be > > selected through Kconfig. > > This changelog doesn't make sense. > > There can be only one active sched_clock, but that does no mean, that > you cannot have different implementations compiled in. > > So if you disable this config which sched_clock is your kernel using? Jiffies > And if you enable it, how is guaranteed that you end up with the ttc > sched_clock as the active one? Just due to initcall ordering? I assumed so. Is there a different mechanism? Sören
On Fri, 5 Jul 2013, Sören Brinkmann wrote: > On Fri, Jul 05, 2013 at 08:30:47AM +0200, Thomas Gleixner wrote: > > On Wed, 3 Jul 2013, Soren Brinkmann wrote: > > > > > Reuse the TTC clocksource timer as sched clock, too. Since only a single > > > sched clock is supported in Linux, this feature optional and can be > > > selected through Kconfig. > > > > This changelog doesn't make sense. > > > > There can be only one active sched_clock, but that does no mean, that > > you cannot have different implementations compiled in. > > > > So if you disable this config which sched_clock is your kernel using? > Jiffies > > > And if you enable it, how is guaranteed that you end up with the ttc > > sched_clock as the active one? Just due to initcall ordering? > I assumed so. Is there a different mechanism? jiffies is the default one. If you setup an explicit sched clock then this is used. initcall ordering only matters if you have two possible sched clocks which might replace jiffies. The one which gets registered last wins. So the question is, why you want to disable your sched clock at compile time. Thanks, tglx
On Fri, Jul 05, 2013 at 06:05:14PM +0200, Thomas Gleixner wrote: > On Fri, 5 Jul 2013, Sören Brinkmann wrote: > > On Fri, Jul 05, 2013 at 08:30:47AM +0200, Thomas Gleixner wrote: > > > On Wed, 3 Jul 2013, Soren Brinkmann wrote: > > > > > > > Reuse the TTC clocksource timer as sched clock, too. Since only a single > > > > sched clock is supported in Linux, this feature optional and can be > > > > selected through Kconfig. > > > > > > This changelog doesn't make sense. > > > > > > There can be only one active sched_clock, but that does no mean, that > > > you cannot have different implementations compiled in. > > > > > > So if you disable this config which sched_clock is your kernel using? > > Jiffies > > > > > And if you enable it, how is guaranteed that you end up with the ttc > > > sched_clock as the active one? Just due to initcall ordering? > > I assumed so. Is there a different mechanism? > > jiffies is the default one. If you setup an explicit sched clock then > this is used. initcall ordering only matters if you have two possible > sched clocks which might replace jiffies. The one which gets > registered last wins. > > So the question is, why you want to disable your sched clock at > compile time. The timer drivers I have seen unconditionally register themselves as sched_clock. There does not seem to be a runtime mechanism to choose the best one - I might miss it though. I was thinking about this due to the arm_global_timer driver which has been dicussed on lkml recently, which seemed to do it this way too. And since that timer would be an alternative sched_clock for Zynq too, I thought I follow the same approach for the TTC. Sören
On Fri, 5 Jul 2013, Sören Brinkmann wrote: > On Fri, Jul 05, 2013 at 06:05:14PM +0200, Thomas Gleixner wrote: > > On Fri, 5 Jul 2013, Sören Brinkmann wrote: > > > On Fri, Jul 05, 2013 at 08:30:47AM +0200, Thomas Gleixner wrote: > > > > On Wed, 3 Jul 2013, Soren Brinkmann wrote: > > > > > > > > > Reuse the TTC clocksource timer as sched clock, too. Since only a single > > > > > sched clock is supported in Linux, this feature optional and can be > > > > > selected through Kconfig. > > > > > > > > This changelog doesn't make sense. > > > > > > > > There can be only one active sched_clock, but that does no mean, that > > > > you cannot have different implementations compiled in. > > > > > > > > So if you disable this config which sched_clock is your kernel using? > > > Jiffies > > > > > > > And if you enable it, how is guaranteed that you end up with the ttc > > > > sched_clock as the active one? Just due to initcall ordering? > > > I assumed so. Is there a different mechanism? > > > > jiffies is the default one. If you setup an explicit sched clock then > > this is used. initcall ordering only matters if you have two possible > > sched clocks which might replace jiffies. The one which gets > > registered last wins. > > > > So the question is, why you want to disable your sched clock at > > compile time. > The timer drivers I have seen unconditionally register themselves as > sched_clock. There does not seem to be a runtime mechanism to choose the > best one - I might miss it though. > I was thinking about this due to the arm_global_timer driver which has > been dicussed on lkml recently, which seemed to do it this way too. And since > that timer would be an alternative sched_clock for Zynq too, I thought I > follow the same approach for the TTC. Hmm, ok. So there is a generic one as well. If we end up with multiple choices for that sched_clock, then there should be a mechanism to avoid that #ifdeffery and have it runtime selected. Having a setup call with a rating argument would be a first step. So the one with the highest rating wins. If people want to have it selectable from the kernel commandline, it would need some string matching as well. Thanks, tglx
On Thu, Jul 04, 2013 at 06:30:26AM +0300, Baruch Siach wrote: > Hi Soren, > > On Wed, Jul 03, 2013 at 02:50:16PM -0700, Soren Brinkmann wrote: > > Reuse the TTC clocksource timer as sched clock, too. Since only a single > > sched clock is supported in Linux, this feature optional and can be > > selected through Kconfig. > > > > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > > --- > > [...] > > > diff --git a/drivers/clocksource/cadence_ttc_timer.c > > b/drivers/clocksource/cadence_ttc_timer.c > > index 4cbe28c..ef0f52b 100644 > > --- a/drivers/clocksource/cadence_ttc_timer.c > > +++ b/drivers/clocksource/cadence_ttc_timer.c > > @@ -22,6 +22,7 @@ > > #include <linux/of_irq.h> > > #include <linux/slab.h> > > #include <linux/clk-provider.h> > > +#include <asm/sched_clock.h> > > This has changed to linux/sched_clock.h in the tip tree. See > https://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/commit/?h=timers/core&id=38ff87f77af0b5a93fc8581cff1d6e5692ab8970. Is that change going into Linus' tree during the 3.11 merge window? The tip tree is missing some changes to the TTC which went in through the armsoc tree. Sören
On Fri, 5 Jul 2013, Sören Brinkmann wrote: > On Thu, Jul 04, 2013 at 06:30:26AM +0300, Baruch Siach wrote: > > Hi Soren, > > > > On Wed, Jul 03, 2013 at 02:50:16PM -0700, Soren Brinkmann wrote: > > > Reuse the TTC clocksource timer as sched clock, too. Since only a single > > > sched clock is supported in Linux, this feature optional and can be > > > selected through Kconfig. > > > > > > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > > > --- > > > > [...] > > > > > diff --git a/drivers/clocksource/cadence_ttc_timer.c > > > b/drivers/clocksource/cadence_ttc_timer.c > > > index 4cbe28c..ef0f52b 100644 > > > --- a/drivers/clocksource/cadence_ttc_timer.c > > > +++ b/drivers/clocksource/cadence_ttc_timer.c > > > @@ -22,6 +22,7 @@ > > > #include <linux/of_irq.h> > > > #include <linux/slab.h> > > > #include <linux/clk-provider.h> > > > +#include <asm/sched_clock.h> > > > > This has changed to linux/sched_clock.h in the tip tree. See > > https://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/commit/?h=timers/core&id=38ff87f77af0b5a93fc8581cff1d6e5692ab8970. > Is that change going into Linus' tree during the 3.11 merge window? The > tip tree is missing some changes to the TTC which went in through the > armsoc tree. Yes, this is about to be sent Linuswards. Can we please coordinate that stuff. drivers/clocksource depend on tip timers/* branches and any modification in some other tree is going to be doomed. We have a mechanism for that in place, if stuff goes cross trees. One of the trees provides a set of commit for the other tree to pull, so we do not end up with merge dependencies and conflicts. Why is clocksource stuff going through armsoc unless it contains related modifications in arch/arm/ Grmbl. tglx
On Fri, Jul 05, 2013 at 10:42:03PM +0200, Thomas Gleixner wrote: > On Fri, 5 Jul 2013, Sören Brinkmann wrote: > > On Thu, Jul 04, 2013 at 06:30:26AM +0300, Baruch Siach wrote: > > > Hi Soren, > > > > > > On Wed, Jul 03, 2013 at 02:50:16PM -0700, Soren Brinkmann wrote: > > > > Reuse the TTC clocksource timer as sched clock, too. Since only a single > > > > sched clock is supported in Linux, this feature optional and can be > > > > selected through Kconfig. > > > > > > > > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > > > > --- > > > > > > [...] > > > > > > > diff --git a/drivers/clocksource/cadence_ttc_timer.c > > > > b/drivers/clocksource/cadence_ttc_timer.c > > > > index 4cbe28c..ef0f52b 100644 > > > > --- a/drivers/clocksource/cadence_ttc_timer.c > > > > +++ b/drivers/clocksource/cadence_ttc_timer.c > > > > @@ -22,6 +22,7 @@ > > > > #include <linux/of_irq.h> > > > > #include <linux/slab.h> > > > > #include <linux/clk-provider.h> > > > > +#include <asm/sched_clock.h> > > > > > > This has changed to linux/sched_clock.h in the tip tree. See > > > https://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/commit/?h=timers/core&id=38ff87f77af0b5a93fc8581cff1d6e5692ab8970. > > Is that change going into Linus' tree during the 3.11 merge window? The > > tip tree is missing some changes to the TTC which went in through the > > armsoc tree. > > Yes, this is about to be sent Linuswards. Can we please coordinate > that stuff. drivers/clocksource depend on tip timers/* branches and > any modification in some other tree is going to be doomed. > > We have a mechanism for that in place, if stuff goes cross trees. One > of the trees provides a set of commit for the other tree to pull, so > we do not end up with merge dependencies and conflicts. > > Why is clocksource stuff going through armsoc unless it contains > related modifications in arch/arm/ We overhauled Zynq's common clock code and migrated all Zynq drivers to it - the TTC being one. I'm pretty sure you were on the CC list for that change. https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=30e1e28598c2674c133148d8aec6d431d7acd314 Sören
On Fri, 5 Jul 2013, Sören Brinkmann wrote: > On Fri, Jul 05, 2013 at 10:42:03PM +0200, Thomas Gleixner wrote: > > We have a mechanism for that in place, if stuff goes cross trees. One > > of the trees provides a set of commit for the other tree to pull, so > > we do not end up with merge dependencies and conflicts. > > > > Why is clocksource stuff going through armsoc unless it contains > > related modifications in arch/arm/ > We overhauled Zynq's common clock code and migrated all Zynq drivers to > it - the TTC being one. I'm pretty sure you were on the CC list for that > change. > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=30e1e28598c2674c133148d8aec6d431d7acd314 > That's fine as it has no dependencies on any of the clocksource core changes AFAICT. Thanks, tglx
On Fri, Jul 05, 2013 at 10:59:53PM +0200, Thomas Gleixner wrote: > On Fri, 5 Jul 2013, Sören Brinkmann wrote: > > On Fri, Jul 05, 2013 at 10:42:03PM +0200, Thomas Gleixner wrote: > > > We have a mechanism for that in place, if stuff goes cross trees. One > > > of the trees provides a set of commit for the other tree to pull, so > > > we do not end up with merge dependencies and conflicts. > > > > > > Why is clocksource stuff going through armsoc unless it contains > > > related modifications in arch/arm/ > > We overhauled Zynq's common clock code and migrated all Zynq drivers to > > it - the TTC being one. I'm pretty sure you were on the CC list for that > > change. > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=30e1e28598c2674c133148d8aec6d431d7acd314 > > > > That's fine as it has no dependencies on any of the clocksource core > changes AFAICT. Right, shouldn't be a big deal. Just for this patch, I currently have the choice of working with the old TTC but the new clocksource core or the other way around. But since everything is going to meet in Linus' tree soon, I just wait a bit until tip/timers is merged. Sören
On Fri, Jul 05, 2013 at 06:23:19PM +0200, Thomas Gleixner wrote: > On Fri, 5 Jul 2013, Sören Brinkmann wrote: > > On Fri, Jul 05, 2013 at 06:05:14PM +0200, Thomas Gleixner wrote: > > > On Fri, 5 Jul 2013, Sören Brinkmann wrote: > > > > On Fri, Jul 05, 2013 at 08:30:47AM +0200, Thomas Gleixner wrote: > > > > > On Wed, 3 Jul 2013, Soren Brinkmann wrote: > > > > > > > > > > > Reuse the TTC clocksource timer as sched clock, too. Since only a single > > > > > > sched clock is supported in Linux, this feature optional and can be > > > > > > selected through Kconfig. > > > > > > > > > > This changelog doesn't make sense. > > > > > > > > > > There can be only one active sched_clock, but that does no mean, that > > > > > you cannot have different implementations compiled in. > > > > > > > > > > So if you disable this config which sched_clock is your kernel using? > > > > Jiffies > > > > > > > > > And if you enable it, how is guaranteed that you end up with the ttc > > > > > sched_clock as the active one? Just due to initcall ordering? > > > > I assumed so. Is there a different mechanism? > > > > > > jiffies is the default one. If you setup an explicit sched clock then > > > this is used. initcall ordering only matters if you have two possible > > > sched clocks which might replace jiffies. The one which gets > > > registered last wins. > > > > > > So the question is, why you want to disable your sched clock at > > > compile time. > > The timer drivers I have seen unconditionally register themselves as > > sched_clock. There does not seem to be a runtime mechanism to choose the > > best one - I might miss it though. > > I was thinking about this due to the arm_global_timer driver which has > > been dicussed on lkml recently, which seemed to do it this way too. And since > > that timer would be an alternative sched_clock for Zynq too, I thought I > > follow the same approach for the TTC. > > Hmm, ok. So there is a generic one as well. If we end up with multiple > choices for that sched_clock, then there should be a mechanism to > avoid that #ifdeffery and have it runtime selected. > > Having a setup call with a rating argument would be a first step. So > the one with the highest rating wins. If people want to have it > selectable from the kernel commandline, it would need some string > matching as well. I revisited the code: My original reason for adding the Kconfig option was that I hit this warning when I had multiple sched_clock providers: arch/arm/kernel/sched_clock.c:setup_sched_clock(): WARN_ON(read_sched_clock != jiffy_sched_clock_read); That code is gone in the lates Linux tree. Furthermore the now used setup_sched_clock routine, only updates the sched clock provider, if the new one is faster than the old one: if (cd.rate > rate) return; Which is good enough in terms of a rating mechanism, at least for this case. So, I'll remove all the #ifdefs and Kconfig stuff and send a v2. Sören
diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig index 5871933..8dda767 100644 --- a/drivers/clocksource/Kconfig +++ b/drivers/clocksource/Kconfig @@ -36,6 +36,13 @@ config VT8500_TIMER config CADENCE_TTC_TIMER bool +config CADENCE_TTC_TIMER_SCHED_CLOCK + bool "Cadence TTC Sched Clock" + depends on CADENCE_TTC_TIMER + help + Use the clocksource timer provided by the TTC as + sched clock, also. + config CLKSRC_NOMADIK_MTU bool depends on (ARCH_NOMADIK || ARCH_U8500) diff --git a/drivers/clocksource/cadence_ttc_timer.c b/drivers/clocksource/cadence_ttc_timer.c index 4cbe28c..ef0f52b 100644 --- a/drivers/clocksource/cadence_ttc_timer.c +++ b/drivers/clocksource/cadence_ttc_timer.c @@ -22,6 +22,7 @@ #include <linux/of_irq.h> #include <linux/slab.h> #include <linux/clk-provider.h> +#include <asm/sched_clock.h> /* * This driver configures the 2 16-bit count-up timers as follows: @@ -95,6 +96,10 @@ struct ttc_timer_clockevent { #define to_ttc_timer_clkevent(x) \ container_of(x, struct ttc_timer_clockevent, ce) +#ifdef CONFIG_CADENCE_TTC_TIMER_SCHED_CLOCK +static void __iomem *ttc_sched_clock_val_reg; +#endif + /** * ttc_set_interval - Set the timer interval value * @@ -156,6 +161,13 @@ static cycle_t __ttc_clocksource_read(struct clocksource *cs) TTC_COUNT_VAL_OFFSET); } +#ifdef CONFIG_CADENCE_TTC_TIMER_SCHED_CLOCK +static u32 notrace ttc_sched_clock_read(void) +{ + return __raw_readl(ttc_sched_clock_val_reg); +} +#endif + /** * ttc_set_next_event - Sets the time interval for next event * @@ -297,6 +309,12 @@ static void __init ttc_setup_clocksource(struct clk *clk, void __iomem *base) kfree(ttccs); return; } + +#ifdef CONFIG_CADENCE_TTC_TIMER_SCHED_CLOCK + ttc_sched_clock_val_reg = base + TTC_COUNT_VAL_OFFSET; + setup_sched_clock(ttc_sched_clock_read, 16, + clk_get_rate(ttccs->ttc.clk) / PRESCALE); +#endif } static int ttc_rate_change_clockevent_cb(struct notifier_block *nb,
Reuse the TTC clocksource timer as sched clock, too. Since only a single sched clock is supported in Linux, this feature optional and can be selected through Kconfig. Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> --- drivers/clocksource/Kconfig | 7 +++++++ drivers/clocksource/cadence_ttc_timer.c | 18 ++++++++++++++++++ 2 files changed, 25 insertions(+)