| Message ID | 1383945677-29674-6-git-send-email-soren.brinkmann@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11/08/2013 10:21 PM, Soren Brinkmann wrote: > The currently used method adjusting the clocksource to a changing input > frequency does not work on kernels from 3.11 on. > The new approach is to keep the timer frequency as constant as possible. > I.e. > - due to the TTC's prescaler limitations, allow frequency changes > only if the frequency scales by a power of 2 > - adjust the counter's divider on the fly when a frequency change > occurs > > When suspending though, the driver should not prevent rate changes in > order to allow the system to enter its low power state. For that > reason a PM notifier is added so rate changes can be ignored during > suspend/resume. It sounds very weird you have to add a PM notifier in this driver. Have you been facing an issue or do you assume it could happen ? > This limits cpufreq to scale by certain factors only. > But we may keep the time base somewhat constant, so that sleep() & co > keep working as expected, while supporting cpufreq and suspend. > > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > --- > drivers/clocksource/cadence_ttc_timer.c | 142 +++++++++++++++++++++++++++----- > 1 file changed, 122 insertions(+), 20 deletions(-) > > diff --git a/drivers/clocksource/cadence_ttc_timer.c b/drivers/clocksource/cadence_ttc_timer.c > index 68a336038d8f..421b942dd707 100644 > --- a/drivers/clocksource/cadence_ttc_timer.c > +++ b/drivers/clocksource/cadence_ttc_timer.c > @@ -16,12 +16,14 @@ > */ > > #include <linux/clk.h> > +#include <linux/clk-provider.h> > #include <linux/interrupt.h> > #include <linux/clockchips.h> > #include <linux/of_address.h> > #include <linux/of_irq.h> > #include <linux/slab.h> > #include <linux/sched_clock.h> > +#include <linux/suspend.h> > > /* > * This driver configures the 2 16-bit count-up timers as follows: > @@ -52,6 +54,8 @@ > #define TTC_CNT_CNTRL_DISABLE_MASK 0x1 > > #define TTC_CLK_CNTRL_CSRC_MASK (1 << 5) /* clock source */ > +#define TTC_CLK_CNTRL_PSV_MASK 0x1e > +#define TTC_CLK_CNTRL_PSV_SHIFT 1 > > /* > * Setup the timers to use pre-scaling, using a fixed value for now that will > @@ -63,6 +67,8 @@ > #define CLK_CNTRL_PRESCALE_EN 1 > #define CNT_CNTRL_RESET (1 << 4) > > +#define MAX_F_ERR 50 > + > /** > * struct ttc_timer - This definition defines local timer structure > * > @@ -82,8 +88,13 @@ struct ttc_timer { > container_of(x, struct ttc_timer, clk_rate_change_nb) > > struct ttc_timer_clocksource { > + int scale_dir; > + u32 scale_clk_ctrl_reg_old; > + u32 scale_clk_ctrl_reg_new; > + int suspending; > struct ttc_timer ttc; > struct clocksource cs; > + struct notifier_block suspend_nb; > }; > > #define to_ttc_timer_clksrc(x) \ > @@ -228,33 +239,120 @@ static int ttc_rate_change_clocksource_cb(struct notifier_block *nb, > struct ttc_timer_clocksource *ttccs = container_of(ttc, > struct ttc_timer_clocksource, ttc); > > + if (ttccs->suspending) > + return NOTIFY_OK; I don't see how it couldn't be racy. What prevents suspend to occur right after this check ? > + > switch (event) { > - case POST_RATE_CHANGE: > + case PRE_RATE_CHANGE: > + { > + u32 psv; > + unsigned long factor, rate_low, rate_high; > + > + if (ndata->new_rate == ndata->old_rate) { > + ttccs->scale_dir = 0; > + return NOTIFY_OK; > + } > + > + if (ndata->new_rate > ndata->old_rate) { > + factor = DIV_ROUND_CLOSEST(ndata->new_rate, > + ndata->old_rate); > + ttccs->scale_dir = 1; > + rate_low = ndata->old_rate; > + rate_high = ndata->new_rate; > + } else { > + factor = DIV_ROUND_CLOSEST(ndata->old_rate, > + ndata->new_rate); > + ttccs->scale_dir = -1; > + rate_low = ndata->new_rate; > + rate_high = ndata->old_rate; > + } Hmm, it could be interesting if Viresh can give its opinion on this patch [cc'ed]. > + > + if (!is_power_of_2(factor)) > + return NOTIFY_BAD; > + > + if (abs(rate_high - (factor * rate_low)) > MAX_F_ERR) > + return NOTIFY_BAD; > + > + factor = __ilog2_u32(factor); > + > /* > - * Do whatever is necessary to maintain a proper time base > - * > - * I cannot find a way to adjust the currently used clocksource > - * to the new frequency. __clocksource_updatefreq_hz() sounds > - * good, but does not work. Not sure what's that missing. > - * > - * This approach works, but triggers two clocksource switches. > - * The first after unregister to clocksource jiffies. And > - * another one after the register to the newly registered timer. > - * > - * Alternatively we could 'waste' another HW timer to ping pong > - * between clock sources. That would also use one register and > - * one unregister call, but only trigger one clocksource switch > - * for the cost of another HW timer used by the OS. > + * store timer clock ctrl register so we can restore it in case > + * of an abort. > */ > - clocksource_unregister(&ttccs->cs); > - clocksource_register_hz(&ttccs->cs, > - ndata->new_rate / PRESCALE); > - /* fall through */ > - case PRE_RATE_CHANGE: > + ttccs->scale_clk_ctrl_reg_old = > + __raw_readl(ttccs->ttc.base_addr + > + TTC_CLK_CNTRL_OFFSET); > + > + psv = (ttccs->scale_clk_ctrl_reg_old & > + TTC_CLK_CNTRL_PSV_MASK) >> > + TTC_CLK_CNTRL_PSV_SHIFT; > + if (ttccs->scale_dir < 0) > + psv -= factor; > + else > + psv += factor; > + > + /* prescaler within legal range? */ > + if (psv & ~(TTC_CLK_CNTRL_PSV_MASK >> TTC_CLK_CNTRL_PSV_SHIFT)) > + return NOTIFY_BAD; > + > + ttccs->scale_clk_ctrl_reg_new = ttccs->scale_clk_ctrl_reg_old & > + ~TTC_CLK_CNTRL_PSV_MASK; > + ttccs->scale_clk_ctrl_reg_new |= psv << TTC_CLK_CNTRL_PSV_SHIFT; > + > + > + /* scale down: adjust divider in post-change notification */ > + if (ttccs->scale_dir < 0) > + return NOTIFY_DONE; > + > + /* scale up: adjust divider now - before frequency change */ > + __raw_writel(ttccs->scale_clk_ctrl_reg_new, > + ttccs->ttc.base_addr + TTC_CLK_CNTRL_OFFSET); > + break; > + } > + case POST_RATE_CHANGE: > + /* scale up: pre-change notification did the adjustment */ > + if (ttccs->scale_dir >= 0) > + return NOTIFY_OK; > + > + /* scale down: adjust divider now - after frequency change */ > + __raw_writel(ttccs->scale_clk_ctrl_reg_new, > + ttccs->ttc.base_addr + TTC_CLK_CNTRL_OFFSET); > + break; > + > case ABORT_RATE_CHANGE: > + /* we have to undo the adjustment in case we scale up */ > + if (ttccs->scale_dir <= 0) > + return NOTIFY_OK; > + > + /* restore original register value */ > + __raw_writel(ttccs->scale_clk_ctrl_reg_old, > + ttccs->ttc.base_addr + TTC_CLK_CNTRL_OFFSET); > + /* fall through */ > default: > return NOTIFY_DONE; > } > + > + return NOTIFY_DONE; > +} > + > +static int ttc_suspend_notifier_cb(struct notifier_block *nb, > + unsigned long event, void *data) > +{ > + struct ttc_timer_clocksource *ttccs = container_of(nb, > + struct ttc_timer_clocksource, suspend_nb); > + > + switch (event) { > + case PM_SUSPEND_PREPARE: > + ttccs->suspending = 1; > + break; > + case PM_POST_SUSPEND: > + ttccs->suspending = 0; > + break; > + default: > + break; > + } > + > + return NOTIFY_DONE; > } > > static void __init ttc_setup_clocksource(struct clk *clk, void __iomem *base) > @@ -283,6 +381,10 @@ static void __init ttc_setup_clocksource(struct clk *clk, void __iomem *base) > &ttccs->ttc.clk_rate_change_nb)) > pr_warn("Unable to register clock notifier.\n"); > > + ttccs->suspend_nb.notifier_call = ttc_suspend_notifier_cb; > + if (register_pm_notifier(&ttccs->suspend_nb)) > + pr_warn("Unable to register PM notifier.\n"); > + > ttccs->ttc.base_addr = base; > ttccs->cs.name = "ttc_clocksource"; > ttccs->cs.rating = 200; >
On Tue, Nov 12, 2013 at 08:01:37PM +0100, Daniel Lezcano wrote: > On 11/08/2013 10:21 PM, Soren Brinkmann wrote: > >The currently used method adjusting the clocksource to a changing input > >frequency does not work on kernels from 3.11 on. > >The new approach is to keep the timer frequency as constant as possible. > >I.e. > > - due to the TTC's prescaler limitations, allow frequency changes > > only if the frequency scales by a power of 2 > > - adjust the counter's divider on the fly when a frequency change > > occurs > > > >When suspending though, the driver should not prevent rate changes in > >order to allow the system to enter its low power state. For that > >reason a PM notifier is added so rate changes can be ignored during > >suspend/resume. > > It sounds very weird you have to add a PM notifier in this driver. > > Have you been facing an issue or do you assume it could happen ? Yes, I have problems without it, which are probably Zynq specific though. When we suspend on Zynq, we bypass and power down PLLs, which is a re-parent operation in the clock framework. This happens in early suspend and triggers the clock notifiers. Due to the restrictions the TTC's clock notifier applies to frequency changes, it would prevent the re-parent operation even though we do not have to care about it in suspend. The timer's own suspend/resume callbacks are called to late/early and the PM notifier seemed to be the solution. But I'm open to alternative approaches. > > >This limits cpufreq to scale by certain factors only. > >But we may keep the time base somewhat constant, so that sleep() & co > >keep working as expected, while supporting cpufreq and suspend. > > > >Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > >--- > > drivers/clocksource/cadence_ttc_timer.c | 142 +++++++++++++++++++++++++++----- > > 1 file changed, 122 insertions(+), 20 deletions(-) > > > >diff --git a/drivers/clocksource/cadence_ttc_timer.c b/drivers/clocksource/cadence_ttc_timer.c > >index 68a336038d8f..421b942dd707 100644 > >--- a/drivers/clocksource/cadence_ttc_timer.c > >+++ b/drivers/clocksource/cadence_ttc_timer.c > >@@ -16,12 +16,14 @@ > > */ > > > > #include <linux/clk.h> > >+#include <linux/clk-provider.h> > > #include <linux/interrupt.h> > > #include <linux/clockchips.h> > > #include <linux/of_address.h> > > #include <linux/of_irq.h> > > #include <linux/slab.h> > > #include <linux/sched_clock.h> > >+#include <linux/suspend.h> > > > > /* > > * This driver configures the 2 16-bit count-up timers as follows: > >@@ -52,6 +54,8 @@ > > #define TTC_CNT_CNTRL_DISABLE_MASK 0x1 > > > > #define TTC_CLK_CNTRL_CSRC_MASK (1 << 5) /* clock source */ > >+#define TTC_CLK_CNTRL_PSV_MASK 0x1e > >+#define TTC_CLK_CNTRL_PSV_SHIFT 1 > > > > /* > > * Setup the timers to use pre-scaling, using a fixed value for now that will > >@@ -63,6 +67,8 @@ > > #define CLK_CNTRL_PRESCALE_EN 1 > > #define CNT_CNTRL_RESET (1 << 4) > > > >+#define MAX_F_ERR 50 > >+ > > /** > > * struct ttc_timer - This definition defines local timer structure > > * > >@@ -82,8 +88,13 @@ struct ttc_timer { > > container_of(x, struct ttc_timer, clk_rate_change_nb) > > > > struct ttc_timer_clocksource { > >+ int scale_dir; > >+ u32 scale_clk_ctrl_reg_old; > >+ u32 scale_clk_ctrl_reg_new; > >+ int suspending; > > struct ttc_timer ttc; > > struct clocksource cs; > >+ struct notifier_block suspend_nb; > > }; > > > > #define to_ttc_timer_clksrc(x) \ > >@@ -228,33 +239,120 @@ static int ttc_rate_change_clocksource_cb(struct notifier_block *nb, > > struct ttc_timer_clocksource *ttccs = container_of(ttc, > > struct ttc_timer_clocksource, ttc); > > > >+ if (ttccs->suspending) > >+ return NOTIFY_OK; > > I don't see how it couldn't be racy. What prevents suspend to occur > right after this check ? Well, in our case there are two situations which could trigger this notifier. 1. cpufreq 2. suspend As long as cpufreq is doing its stuff things should be okay and fine. And suspend triggers the PM and clock notifier in the correct order, which solves the issue I described above. So, it is not really pretty, but works for me. Sören
On 13 November 2013 00:31, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > Hmm, it could be interesting if Viresh can give its opinion on this patch > [cc'ed]. I am not sure if I understood the problem completely, but if this is about not allowing frequency transitions from cpufreq when we have started to suspend our system, then you must have a look at this thread: https://lkml.org/lkml/2013/10/24/369
On 11/13/2013 09:03 AM, Viresh Kumar wrote: > On 13 November 2013 00:31, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: >> Hmm, it could be interesting if Viresh can give its opinion on this patch >> [cc'ed]. > > I am not sure if I understood the problem completely, but if this is about > not allowing frequency transitions from cpufreq when we have started to > suspend our system, then you must have a look at this thread: > > https://lkml.org/lkml/2013/10/24/369 Thanks Viresh for the pointer.
On Wed, Nov 13, 2013 at 01:33:52PM +0530, Viresh Kumar wrote: > On 13 November 2013 00:31, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > Hmm, it could be interesting if Viresh can give its opinion on this patch > > [cc'ed]. > > I am not sure if I understood the problem completely, but if this is about > not allowing frequency transitions from cpufreq when we have started to > suspend our system, then you must have a look at this thread: > > https://lkml.org/lkml/2013/10/24/369 Thanks for the link. I had problems in that direction at some point too. But the current issue, is not cpufreq changing the frequency, but our suspend code in prepare_late() causes a frequency change of this timer when the PLLs are powered down (it's done through a CCF clk_set_parent() operation). Sören
On Tue, Nov 12, 2013 at 08:01:37PM +0100, Daniel Lezcano wrote: > On 11/08/2013 10:21 PM, Soren Brinkmann wrote: > >The currently used method adjusting the clocksource to a changing input > >frequency does not work on kernels from 3.11 on. > >The new approach is to keep the timer frequency as constant as possible. > >I.e. > > - due to the TTC's prescaler limitations, allow frequency changes > > only if the frequency scales by a power of 2 > > - adjust the counter's divider on the fly when a frequency change > > occurs > > > >When suspending though, the driver should not prevent rate changes in > >order to allow the system to enter its low power state. For that > >reason a PM notifier is added so rate changes can be ignored during > >suspend/resume. > > It sounds very weird you have to add a PM notifier in this driver. > > Have you been facing an issue or do you assume it could happen ? As I said in my other email, we do have some issues related with suspend. The construct with the PM notifier seemed to work but since we upgraded our vendor tree to 3.12 I see issues with this code. So, for now, I'll remove the PM notifier and associated code from this patch. I have to revisit the suspend case later. Thanks, Sören
diff --git a/drivers/clocksource/cadence_ttc_timer.c b/drivers/clocksource/cadence_ttc_timer.c index 68a336038d8f..421b942dd707 100644 --- a/drivers/clocksource/cadence_ttc_timer.c +++ b/drivers/clocksource/cadence_ttc_timer.c @@ -16,12 +16,14 @@ */ #include <linux/clk.h> +#include <linux/clk-provider.h> #include <linux/interrupt.h> #include <linux/clockchips.h> #include <linux/of_address.h> #include <linux/of_irq.h> #include <linux/slab.h> #include <linux/sched_clock.h> +#include <linux/suspend.h> /* * This driver configures the 2 16-bit count-up timers as follows: @@ -52,6 +54,8 @@ #define TTC_CNT_CNTRL_DISABLE_MASK 0x1 #define TTC_CLK_CNTRL_CSRC_MASK (1 << 5) /* clock source */ +#define TTC_CLK_CNTRL_PSV_MASK 0x1e +#define TTC_CLK_CNTRL_PSV_SHIFT 1 /* * Setup the timers to use pre-scaling, using a fixed value for now that will @@ -63,6 +67,8 @@ #define CLK_CNTRL_PRESCALE_EN 1 #define CNT_CNTRL_RESET (1 << 4) +#define MAX_F_ERR 50 + /** * struct ttc_timer - This definition defines local timer structure * @@ -82,8 +88,13 @@ struct ttc_timer { container_of(x, struct ttc_timer, clk_rate_change_nb) struct ttc_timer_clocksource { + int scale_dir; + u32 scale_clk_ctrl_reg_old; + u32 scale_clk_ctrl_reg_new; + int suspending; struct ttc_timer ttc; struct clocksource cs; + struct notifier_block suspend_nb; }; #define to_ttc_timer_clksrc(x) \ @@ -228,33 +239,120 @@ static int ttc_rate_change_clocksource_cb(struct notifier_block *nb, struct ttc_timer_clocksource *ttccs = container_of(ttc, struct ttc_timer_clocksource, ttc); + if (ttccs->suspending) + return NOTIFY_OK; + switch (event) { - case POST_RATE_CHANGE: + case PRE_RATE_CHANGE: + { + u32 psv; + unsigned long factor, rate_low, rate_high; + + if (ndata->new_rate == ndata->old_rate) { + ttccs->scale_dir = 0; + return NOTIFY_OK; + } + + if (ndata->new_rate > ndata->old_rate) { + factor = DIV_ROUND_CLOSEST(ndata->new_rate, + ndata->old_rate); + ttccs->scale_dir = 1; + rate_low = ndata->old_rate; + rate_high = ndata->new_rate; + } else { + factor = DIV_ROUND_CLOSEST(ndata->old_rate, + ndata->new_rate); + ttccs->scale_dir = -1; + rate_low = ndata->new_rate; + rate_high = ndata->old_rate; + } + + if (!is_power_of_2(factor)) + return NOTIFY_BAD; + + if (abs(rate_high - (factor * rate_low)) > MAX_F_ERR) + return NOTIFY_BAD; + + factor = __ilog2_u32(factor); + /* - * Do whatever is necessary to maintain a proper time base - * - * I cannot find a way to adjust the currently used clocksource - * to the new frequency. __clocksource_updatefreq_hz() sounds - * good, but does not work. Not sure what's that missing. - * - * This approach works, but triggers two clocksource switches. - * The first after unregister to clocksource jiffies. And - * another one after the register to the newly registered timer. - * - * Alternatively we could 'waste' another HW timer to ping pong - * between clock sources. That would also use one register and - * one unregister call, but only trigger one clocksource switch - * for the cost of another HW timer used by the OS. + * store timer clock ctrl register so we can restore it in case + * of an abort. */ - clocksource_unregister(&ttccs->cs); - clocksource_register_hz(&ttccs->cs, - ndata->new_rate / PRESCALE); - /* fall through */ - case PRE_RATE_CHANGE: + ttccs->scale_clk_ctrl_reg_old = + __raw_readl(ttccs->ttc.base_addr + + TTC_CLK_CNTRL_OFFSET); + + psv = (ttccs->scale_clk_ctrl_reg_old & + TTC_CLK_CNTRL_PSV_MASK) >> + TTC_CLK_CNTRL_PSV_SHIFT; + if (ttccs->scale_dir < 0) + psv -= factor; + else + psv += factor; + + /* prescaler within legal range? */ + if (psv & ~(TTC_CLK_CNTRL_PSV_MASK >> TTC_CLK_CNTRL_PSV_SHIFT)) + return NOTIFY_BAD; + + ttccs->scale_clk_ctrl_reg_new = ttccs->scale_clk_ctrl_reg_old & + ~TTC_CLK_CNTRL_PSV_MASK; + ttccs->scale_clk_ctrl_reg_new |= psv << TTC_CLK_CNTRL_PSV_SHIFT; + + + /* scale down: adjust divider in post-change notification */ + if (ttccs->scale_dir < 0) + return NOTIFY_DONE; + + /* scale up: adjust divider now - before frequency change */ + __raw_writel(ttccs->scale_clk_ctrl_reg_new, + ttccs->ttc.base_addr + TTC_CLK_CNTRL_OFFSET); + break; + } + case POST_RATE_CHANGE: + /* scale up: pre-change notification did the adjustment */ + if (ttccs->scale_dir >= 0) + return NOTIFY_OK; + + /* scale down: adjust divider now - after frequency change */ + __raw_writel(ttccs->scale_clk_ctrl_reg_new, + ttccs->ttc.base_addr + TTC_CLK_CNTRL_OFFSET); + break; + case ABORT_RATE_CHANGE: + /* we have to undo the adjustment in case we scale up */ + if (ttccs->scale_dir <= 0) + return NOTIFY_OK; + + /* restore original register value */ + __raw_writel(ttccs->scale_clk_ctrl_reg_old, + ttccs->ttc.base_addr + TTC_CLK_CNTRL_OFFSET); + /* fall through */ default: return NOTIFY_DONE; } + + return NOTIFY_DONE; +} + +static int ttc_suspend_notifier_cb(struct notifier_block *nb, + unsigned long event, void *data) +{ + struct ttc_timer_clocksource *ttccs = container_of(nb, + struct ttc_timer_clocksource, suspend_nb); + + switch (event) { + case PM_SUSPEND_PREPARE: + ttccs->suspending = 1; + break; + case PM_POST_SUSPEND: + ttccs->suspending = 0; + break; + default: + break; + } + + return NOTIFY_DONE; } static void __init ttc_setup_clocksource(struct clk *clk, void __iomem *base) @@ -283,6 +381,10 @@ static void __init ttc_setup_clocksource(struct clk *clk, void __iomem *base) &ttccs->ttc.clk_rate_change_nb)) pr_warn("Unable to register clock notifier.\n"); + ttccs->suspend_nb.notifier_call = ttc_suspend_notifier_cb; + if (register_pm_notifier(&ttccs->suspend_nb)) + pr_warn("Unable to register PM notifier.\n"); + ttccs->ttc.base_addr = base; ttccs->cs.name = "ttc_clocksource"; ttccs->cs.rating = 200;
The currently used method adjusting the clocksource to a changing input frequency does not work on kernels from 3.11 on. The new approach is to keep the timer frequency as constant as possible. I.e. - due to the TTC's prescaler limitations, allow frequency changes only if the frequency scales by a power of 2 - adjust the counter's divider on the fly when a frequency change occurs When suspending though, the driver should not prevent rate changes in order to allow the system to enter its low power state. For that reason a PM notifier is added so rate changes can be ignored during suspend/resume. This limits cpufreq to scale by certain factors only. But we may keep the time base somewhat constant, so that sleep() & co keep working as expected, while supporting cpufreq and suspend. Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> --- drivers/clocksource/cadence_ttc_timer.c | 142 +++++++++++++++++++++++++++----- 1 file changed, 122 insertions(+), 20 deletions(-)