| Message ID | 1480421716-30782-2-git-send-email-al.kochet@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 29 Nov 2016, Alexander Kochetkov wrote: > After a cpufreq transition, update the clockevent's frequency > by fetching the new clock rate from the clock framework and > reprogram the next clock event. The frequency change would not only affect the clockevent device, it also would affect the clocksource. So the patch is incomplete, but see below. > The clock supplying the arm-global-timer on the rk3188 is coming > from the the cpu clock itself and thus changes its rate everytime > cpufreq adjusts the cpu frequency. That's broken and the clk framework should keep the CORE_PERI clock at a constant rate by reprogramming the divider of the CPU clock. > Found by code review, real impact not known. Assume what actual > HZ value will be different from expected on platforms using > arm-global-timer as clockevent. Assumptions w/o real impact are a perfect reason not to apply that patch. This want's a proper proof that the global timer really changes and this hackery is required, which I seriously doubt. Thanks, tglx
Hello Thomas! > 29 нояб. 2016 г., в 16:42, Thomas Gleixner <tglx@linutronix.de> написал(а): > > The frequency change would not only affect the clockevent device, it also > would affect the clocksource. So the patch is incomplete, but see below. Looks like kernel disallow change clocksource and shed_clock rate at runtime. I haven’t found any function for that. > 29 нояб. 2016 г., в 16:42, Thomas Gleixner <tglx@linutronix.de> написал(а): > > That's broken and the clk framework should keep the CORE_PERI clock at a > constant rate by reprogramming the divider of the CPU clock. Thank you for the hint. > Assumptions w/o real impact are a perfect reason not to apply that > patch. This want's a proper proof that the global timer really changes and > this hackery is required, which I seriously doubt. It really changes. It changes like it changes for smp-twd. I guess they are driven by same clock CORE_PERI. Thank you for help. Alexander.
On Tue, 29 Nov 2016, Alexander Kochetkov wrote: > > 29 нояб. 2016 г., в 16:42, Thomas Gleixner <tglx@linutronix.de> написал(а): > > > > The frequency change would not only affect the clockevent device, it also > > would affect the clocksource. So the patch is incomplete, but see below. > Looks like kernel disallow change clocksource and shed_clock rate at > runtime. I haven’t found any function for that. > > > 29 нояб. 2016 г., в 16:42, Thomas Gleixner <tglx@linutronix.de> написал(а): > > > > That's broken and the clk framework should keep the CORE_PERI clock at a > > constant rate by reprogramming the divider of the CPU clock. > > Thank you for the hint. > > > Assumptions w/o real impact are a perfect reason not to apply that > > patch. This want's a proper proof that the global timer really changes and > > this hackery is required, which I seriously doubt. > > It really changes. It changes like it changes for smp-twd. I guess they are > driven by same clock CORE_PERI. If that happens then the whole thing wants to be disabled on those misconfigured systems and not just hacked for nothing. Thanks, tglx
On 29/11/16 13:42, Thomas Gleixner wrote: > On Tue, 29 Nov 2016, Alexander Kochetkov wrote: > >> After a cpufreq transition, update the clockevent's frequency >> by fetching the new clock rate from the clock framework and >> reprogram the next clock event. > > The frequency change would not only affect the clockevent device, it also > would affect the clocksource. So the patch is incomplete, but see below. > >> The clock supplying the arm-global-timer on the rk3188 is coming >> from the the cpu clock itself and thus changes its rate everytime >> cpufreq adjusts the cpu frequency. > > That's broken and the clk framework should keep the CORE_PERI clock at a > constant rate by reprogramming the divider of the CPU clock. > >> Found by code review, real impact not known. Assume what actual >> HZ value will be different from expected on platforms using >> arm-global-timer as clockevent. > > Assumptions w/o real impact are a perfect reason not to apply that > patch. This want's a proper proof that the global timer really changes and > this hackery is required, which I seriously doubt. Well, let's not underestimate the "creativity" [1] of A5/A9 when it comes to the timer clocks, and it is a very sad fact that both the global timer and the local timers are clocked by PERIPHCLK, which is ticking at a fixed ratio N (N >= 2) of the main CPU clock (CLK): http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0407f/CIHGECHJ.html I'm not sure how feasible it is to change this ratio (the TRM seems to be very silent on the subject). So short of being able to reconfigure it on the fly, this will probably need some surgery similar to what we already do for the TWD (which this patch mimics). Thankfully, we don't see that anymore on moderately recent HW (anything since A15) and the advent of the arch timer, which is guaranteed to have a fixed frequency. Thanks, M. [1] I wanted to write something else, but common decency prevents me from being more explicit...
On Tue, 29 Nov 2016, Marc Zyngier wrote: > On 29/11/16 13:42, Thomas Gleixner wrote: > > Assumptions w/o real impact are a perfect reason not to apply that > > patch. This want's a proper proof that the global timer really changes and > > this hackery is required, which I seriously doubt. > > Well, let's not underestimate the "creativity" [1] of A5/A9 when it > comes to the timer clocks, and it is a very sad fact that both the > global timer and the local timers are clocked by PERIPHCLK, which is > ticking at a fixed ratio N (N >= 2) of the main CPU clock (CLK): > > http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0407f/CIHGECHJ.html > > I'm not sure how feasible it is to change this ratio (the TRM seems to > be very silent on the subject). The CRU documentation of the RK3188 suggests that you can adjust it as it has a seperate divider, but who knows. > So short of being able to reconfigure it on the fly, this will probably > need some surgery similar to what we already do for the TWD (which this > patch mimics). > > Thankfully, we don't see that anymore on moderately recent HW (anything > since A15) and the advent of the arch timer, which is guaranteed to have > a fixed frequency. Can we just disable that global timer on affected SoCs and use something else instead? Thanks, tglx
On 29/11/16 14:32, Thomas Gleixner wrote: > On Tue, 29 Nov 2016, Marc Zyngier wrote: >> On 29/11/16 13:42, Thomas Gleixner wrote: >>> Assumptions w/o real impact are a perfect reason not to apply that >>> patch. This want's a proper proof that the global timer really changes and >>> this hackery is required, which I seriously doubt. >> >> Well, let's not underestimate the "creativity" [1] of A5/A9 when it >> comes to the timer clocks, and it is a very sad fact that both the >> global timer and the local timers are clocked by PERIPHCLK, which is >> ticking at a fixed ratio N (N >= 2) of the main CPU clock (CLK): >> >> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0407f/CIHGECHJ.html >> >> I'm not sure how feasible it is to change this ratio (the TRM seems to >> be very silent on the subject). > > The CRU documentation of the RK3188 suggests that you can adjust it as it > has a seperate divider, but who knows. > >> So short of being able to reconfigure it on the fly, this will probably >> need some surgery similar to what we already do for the TWD (which this >> patch mimics). >> >> Thankfully, we don't see that anymore on moderately recent HW (anything >> since A15) and the advent of the arch timer, which is guaranteed to have >> a fixed frequency. > > Can we just disable that global timer on affected SoCs and use something > else instead? That'd be my preferred course of action. I've located some documentation over there [1], and page 1126 seems to indicate a profusion of additional timers, some of which are in an always-on domain. Seems like a much better use of someone's time... Thanks, M. [1] http://rockchip.fr/Rockchip%20RK3188%20TRM%20V1.3.pdf
> 29 нояб. 2016 г., в 17:32, Thomas Gleixner <tglx@linutronix.de> написал(а): > > Can we just disable that global timer on affected SoCs and use something > else instead? I’ve sent patch series for fixing that on rockchip SoC. http://lists.infradead.org/pipermail/linux-rockchip/2016-November/013217.html But the series contain fix only for rk3188, because I don’t have another rockchip SoC. rk3288 and other could be easy fixed with dts files. There are a lot of other platforms what probably use shed_clock and clocksource form global-timer. alexander@ubuntu:dts$ grep arm,cortex-a9-global-timer * am4372.dtsi: compatible = "arm,cortex-a9-global-timer"; artpec6.dtsi: compatible = "arm,cortex-a9-global-timer"; bcm5301x.dtsi: compatible = "arm,cortex-a9-global-timer"; bcm63138.dtsi: compatible = "arm,cortex-a9-global-timer"; bcm-cygnus.dtsi: compatible = "arm,cortex-a9-global-timer"; bcm-nsp.dtsi: compatible = "arm,cortex-a9-global-timer"; hip01.dtsi: compatible = "arm,cortex-a9-global-timer"; rk3xxx.dtsi: compatible = "arm,cortex-a9-global-timer"; stih407-family.dtsi: compatible = "arm,cortex-a9-global-timer"; stih41x.dtsi: compatible = "arm,cortex-a9-global-timer"; uniphier-common32.dtsi: compatible = "arm,cortex-a9-global-timer"; uniphier-ph1-sld3.dtsi: compatible = "arm,cortex-a9-global-timer"; vexpress-v2p-ca5s.dts: "arm,cortex-a9-global-timer"; vf500.dtsi: compatible = "arm,cortex-a9-global-timer"; zx296702.dtsi: compatible = "arm,cortex-a9-global-timer"; zynq-7000.dtsi: compatible = "arm,cortex-a9-global-timer»; Regards, Alexander.
> 29 нояб. 2016 г., в 17:51, Marc Zyngier <marc.zyngier@arm.com> написал(а): > > That'd be my preferred course of action. I've located some documentation > over there [1], and page 1126 seems to indicate a profusion of > additional timers, some of which are in an always-on domain. Seems like > a much better use of someone's time... Thank you very much :) And thank you for great explanation of core problem. In general, I no longer insist on the inclusion the patch in the kernel. Thank you guys for the help. Alexander.
On 29/11/16 14:51, Alexander Kochetkov wrote: > >> 29 нояб. 2016 г., в 17:32, Thomas Gleixner <tglx@linutronix.de> написал(а): >> >> Can we just disable that global timer on affected SoCs and use something >> else instead? > > I’ve sent patch series for fixing that on rockchip SoC. > http://lists.infradead.org/pipermail/linux-rockchip/2016-November/013217.html > > But the series contain fix only for rk3188, because I don’t have another rockchip > SoC. rk3288 and other could be easy fixed with dts files. 3288 (and probably anything newer) is irrelevant to this discussion, as it has the arch timer interface - that may be busted in other ways (such as not being correctly set up by firmware and not being always-on as it should), but frequency is not one of them. This only affects Cortex-A9/A5 based parts. > There are a lot of other platforms what probably use shed_clock and > clocksource form global-timer. Presumably it's only an issue if they also have cpufreq? > alexander@ubuntu:dts$ grep arm,cortex-a9-global-timer * > am4372.dtsi: compatible = "arm,cortex-a9-global-timer"; > artpec6.dtsi: compatible = "arm,cortex-a9-global-timer"; > bcm5301x.dtsi: compatible = "arm,cortex-a9-global-timer"; > bcm63138.dtsi: compatible = "arm,cortex-a9-global-timer"; > bcm-cygnus.dtsi: compatible = "arm,cortex-a9-global-timer"; > bcm-nsp.dtsi: compatible = "arm,cortex-a9-global-timer"; > hip01.dtsi: compatible = "arm,cortex-a9-global-timer"; > rk3xxx.dtsi: compatible = "arm,cortex-a9-global-timer"; > stih407-family.dtsi: compatible = "arm,cortex-a9-global-timer"; > stih41x.dtsi: compatible = "arm,cortex-a9-global-timer"; > uniphier-common32.dtsi: compatible = "arm,cortex-a9-global-timer"; > uniphier-ph1-sld3.dtsi: compatible = "arm,cortex-a9-global-timer"; > vexpress-v2p-ca5s.dts: "arm,cortex-a9-global-timer"; I can tell you that one, for one, is never used, because it depends on an input clock provided by the vexpress-osc driver which cannot be probed sufficiently early. Robin. > vf500.dtsi: compatible = "arm,cortex-a9-global-timer"; > zx296702.dtsi: compatible = "arm,cortex-a9-global-timer"; > zynq-7000.dtsi: compatible = "arm,cortex-a9-global-timer»; > > Regards, > Alexander. > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c index 8da0329..55addeb 100644 --- a/drivers/clocksource/arm_global_timer.c +++ b/drivers/clocksource/arm_global_timer.c @@ -49,6 +49,7 @@ * the units for all operations. */ static void __iomem *gt_base; +static struct clk *gt_clk; static unsigned long gt_clk_rate; static int gt_ppi; static struct clock_event_device __percpu *gt_evt; @@ -137,6 +138,97 @@ static int gt_clockevent_set_next_event(unsigned long evt, return 0; } +#ifdef CONFIG_COMMON_CLK + +/* + * Updates clockevent frequency when the cpu frequency changes. + * Called on the cpu that is changing frequency with interrupts disabled. + */ +static void gt_update_frequency(void *new_rate) +{ + gt_clk_rate = *((unsigned long *) new_rate); + + clockevents_update_freq(raw_cpu_ptr(gt_evt), gt_clk_rate); +} + +static int gt_rate_change(struct notifier_block *nb, + unsigned long flags, void *data) +{ + struct clk_notifier_data *cnd = data; + + /* + * The gt clock events must be reprogrammed to account for the new + * frequency. The timer is local to a cpu, so cross-call to the + * changing cpu. + */ + if (flags == POST_RATE_CHANGE) + on_each_cpu(gt_update_frequency, + (void *)&cnd->new_rate, 1); + + return NOTIFY_OK; +} + +static struct notifier_block gt_clk_nb = { + .notifier_call = gt_rate_change, +}; + +static int gt_clk_init(void) +{ + if (gt_evt && raw_cpu_ptr(gt_evt) && !IS_ERR(gt_clk)) + return clk_notifier_register(gt_clk, >_clk_nb); + + return 0; +} +core_initcall(gt_clk_init); + +#elif defined (CONFIG_CPU_FREQ) + +#include <linux/cpufreq.h> + +/* + * Updates clockevent frequency when the cpu frequency changes. + * Called on the cpu that is changing frequency with interrupts disabled. + */ +static void gt_update_frequency(void *data) +{ + gt_clk_rate = clk_get_rate(gt_clk); + + clockevents_update_freq(raw_cpu_ptr(gt_evt), gt_clk_rate); +} + +static int gt_cpufreq_transition(struct notifier_block *nb, + unsigned long state, void *data) +{ + struct cpufreq_freqs *freqs = data; + + /* + * The gt clock events must be reprogrammed to account for the new + * frequency. The timer is local to a cpu, so cross-call to the + * changing cpu. + */ + if (state == CPUFREQ_POSTCHANGE) + smp_call_function_single(freqs->cpu, gt_update_frequency, + NULL, 1); + + return NOTIFY_OK; +} + +static struct notifier_block gt_cpufreq_nb = { + .notifier_call = gt_cpufreq_transition, +}; + +static int gt_cpufreq_init(void) +{ + if (gt_evt && raw_cpu_ptr(gt_evt) && !IS_ERR(gt_clk)) + return cpufreq_register_notifier(>_cpufreq_nb, + CPUFREQ_TRANSITION_NOTIFIER); + + return 0; +} +core_initcall(gt_cpufreq_init); + +#endif + static irqreturn_t gt_clockevent_interrupt(int irq, void *dev_id) { struct clock_event_device *evt = dev_id; @@ -257,7 +349,6 @@ static int __init gt_clocksource_init(void) static int __init global_timer_of_register(struct device_node *np) { - struct clk *gt_clk; int err = 0; /*
After a cpufreq transition, update the clockevent's frequency by fetching the new clock rate from the clock framework and reprogram the next clock event. The clock supplying the arm-global-timer on the rk3188 is coming from the the cpu clock itself and thus changes its rate everytime cpufreq adjusts the cpu frequency. Found by code review, real impact not known. Assume what actual HZ value will be different from expected on platforms using arm-global-timer as clockevent. The patch is port of commit 4fd7f9b12810 ("ARM: 7212/1: smp_twd: reconfigure clockevents after cpufreq change") and commit 2b25d9f64b54 ("ARM: 7535/1: Reprogram smp_twd based on new common clk framework notifiers"). Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com> --- drivers/clocksource/arm_global_timer.c | 93 +++++++++++++++++++++++++++++++- 1 file changed, 92 insertions(+), 1 deletion(-)