Messages in this thread | | | Subject | Re: [PATCH v2 1/2] clocksource: arm_global_timer: implement rate compensation whenever source clock changes | From | Patrice CHOTARD <> | Date | Thu, 10 Jun 2021 15:49:32 +0200 |
| |
Hi Daniel
Thanks for pinging me, i forget this patch.
On 6/10/21 3:01 PM, Daniel Lezcano wrote: > > Hi Patrice, > > do you have any comment about these changes ? > > > On 06/04/2021 15:00, Andrea Merello wrote: >> This patch adds rate change notification support for the parent clock; >> should that clock change, then we try to adjust the our prescaler in order >> to compensate (i.e. we adjust to still get the same timer frequency). >> >> This is loosely based on what it's done in timer-cadence-ttc. timer-sun51, >> mips-gic-timer and smp_twd.c also seem to look at their parent clock rate >> and to perform some kind of adjustment whenever needed. >> >> In this particular case we have only one single counter and prescaler for >> all clocksource, clockevent and timer_delay, and we just update it for all >> (i.e. we don't let it go and call clockevents_update_freq() to notify to >> the kernel that our rate has changed). >> >> Note that, there is apparently no other way to fixup things, because once >> we call register_current_timer_delay(), specifying the timer rate, it seems >> that that rate is not supposed to change ever. >> >> In order for this mechanism to work, we have to make assumptions about how >> much the initial clock is supposed to eventually decrease from the initial >> one, and set our initial prescaler to a value that we can eventually >> decrease enough to compensate. We provide an option in KConfig for this. >> >> In case we end up in a situation in which we are not able to compensate the >> parent clock change, we fail returning NOTIFY_BAD. >> >> This fixes a real-world problem with Zynq arch not being able to use this >> driver and CPU_FREQ at the same time (because ARM global timer is fed by >> the CPU clock, which may keep changing when CPU_FREQ is enabled). >> >> Signed-off-by: Andrea Merello <andrea.merello@gmail.com> >> Cc: Patrice Chotard <patrice.chotard@st.com> >> Cc: linux-kernel@vger.kernel.org >> Cc: linux-arm-kernel@lists.infradead.org >> Cc: Michal Simek <michal.simek@xilinx.com> >> Cc: Sören Brinkmann <soren.brinkmann@xilinx.com> >> --- >> drivers/clocksource/Kconfig | 13 +++ >> drivers/clocksource/arm_global_timer.c | 122 +++++++++++++++++++++++-- >> 2 files changed, 125 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig >> index 39aa21d01e05..19fc5f8883e0 100644 >> --- a/drivers/clocksource/Kconfig >> +++ b/drivers/clocksource/Kconfig >> @@ -358,6 +358,19 @@ config ARM_GLOBAL_TIMER >> help >> This option enables support for the ARM global timer unit. >> >> +config ARM_GT_INITIAL_PRESCALER_VAL >> + int "ARM global timer initial prescaler value" >> + default 1 >> + depends on ARM_GLOBAL_TIMER >> + help >> + When the ARM global timer initializes, its current rate is declared >> + to the kernel and maintained forever. Should it's parent clock >> + change, the driver tries to fix the timer's internal prescaler. >> + On some machs (i.e. Zynq) the initial prescaler value thus poses >> + bounds about how much the parent clock is allowed to decrease or >> + increase wrt the initial clock value. >> + This affects CPU_FREQ max delta from the initial frequency. >> + >> config ARM_TIMER_SP804 >> bool "Support for Dual Timer SP804 module" if COMPILE_TEST >> depends on GENERIC_SCHED_CLOCK && CLKDEV_LOOKUP >> diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c >> index 88b2d38a7a61..60a8047fd32e 100644 >> --- a/drivers/clocksource/arm_global_timer.c >> +++ b/drivers/clocksource/arm_global_timer.c >> @@ -31,6 +31,10 @@ >> #define GT_CONTROL_COMP_ENABLE BIT(1) /* banked */ >> #define GT_CONTROL_IRQ_ENABLE BIT(2) /* banked */ >> #define GT_CONTROL_AUTO_INC BIT(3) /* banked */ >> +#define GT_CONTROL_PRESCALER_SHIFT 8 >> +#define GT_CONTROL_PRESCALER_MAX 0xF >> +#define GT_CONTROL_PRESCALER_MASK (GT_CONTROL_PRESCALER_MAX << \ >> + GT_CONTROL_PRESCALER_SHIFT) >> >> #define GT_INT_STATUS 0x0c >> #define GT_INT_STATUS_EVENT_FLAG BIT(0) >> @@ -39,6 +43,7 @@ >> #define GT_COMP1 0x14 >> #define GT_AUTO_INC 0x18 >> >> +#define MAX_F_ERR 50 >> /* >> * We are expecting to be clocked by the ARM peripheral clock. >> * >> @@ -46,7 +51,8 @@ >> * the units for all operations. >> */ >> static void __iomem *gt_base; >> -static unsigned long gt_clk_rate; >> +struct notifier_block gt_clk_rate_change_nb; >> +static u32 gt_psv_new, gt_psv_bck, gt_target_rate; >> static int gt_ppi; >> static struct clock_event_device __percpu *gt_evt; >> >> @@ -96,7 +102,10 @@ static void gt_compare_set(unsigned long delta, int periodic) >> unsigned long ctrl; >> >> counter += delta; >> - ctrl = GT_CONTROL_TIMER_ENABLE; >> + ctrl = readl(gt_base + GT_CONTROL); >> + ctrl &= ~(GT_CONTROL_COMP_ENABLE | GT_CONTROL_IRQ_ENABLE | >> + GT_CONTROL_AUTO_INC | GT_CONTROL_AUTO_INC); >> + ctrl |= GT_CONTROL_TIMER_ENABLE; >> writel_relaxed(ctrl, gt_base + GT_CONTROL); >> writel_relaxed(lower_32_bits(counter), gt_base + GT_COMP0); >> writel_relaxed(upper_32_bits(counter), gt_base + GT_COMP1); >> @@ -123,7 +132,7 @@ static int gt_clockevent_shutdown(struct clock_event_device *evt) >> >> static int gt_clockevent_set_periodic(struct clock_event_device *evt) >> { >> - gt_compare_set(DIV_ROUND_CLOSEST(gt_clk_rate, HZ), 1); >> + gt_compare_set(DIV_ROUND_CLOSEST(gt_target_rate, HZ), 1); >> return 0; >> } >> >> @@ -177,7 +186,7 @@ static int gt_starting_cpu(unsigned int cpu) >> clk->cpumask = cpumask_of(cpu); >> clk->rating = 300; >> clk->irq = gt_ppi; >> - clockevents_config_and_register(clk, gt_clk_rate, >> + clockevents_config_and_register(clk, gt_target_rate, >> 1, 0xffffffff); >> enable_percpu_irq(clk->irq, IRQ_TYPE_NONE); >> return 0; >> @@ -232,9 +241,28 @@ static struct delay_timer gt_delay_timer = { >> .read_current_timer = gt_read_long, >> }; >> >> +static void gt_write_presc(u32 psv) >> +{ >> + u32 reg; >> + >> + reg = readl(gt_base + GT_CONTROL); >> + reg &= ~GT_CONTROL_PRESCALER_MASK; >> + reg |= psv << GT_CONTROL_PRESCALER_SHIFT; >> + writel(reg, gt_base + GT_CONTROL); >> +} >> + >> +static u32 gt_read_presc(void) >> +{ >> + u32 reg; >> + >> + reg = readl(gt_base + GT_CONTROL); >> + reg &= GT_CONTROL_PRESCALER_MASK; >> + return reg >> GT_CONTROL_PRESCALER_SHIFT; >> +} >> + >> static void __init gt_delay_timer_init(void) >> { >> - gt_delay_timer.freq = gt_clk_rate; >> + gt_delay_timer.freq = gt_target_rate; >> register_current_timer_delay(>_delay_timer); >> } >> >> @@ -243,18 +271,81 @@ static int __init gt_clocksource_init(void) >> writel(0, gt_base + GT_CONTROL); >> writel(0, gt_base + GT_COUNTER0); >> writel(0, gt_base + GT_COUNTER1); >> - /* enables timer on all the cores */ >> - writel(GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); >> + /* set prescaler and enable timer on all the cores */ >> + writel(((CONFIG_ARM_GT_INITIAL_PRESCALER_VAL - 1) << >> + GT_CONTROL_PRESCALER_SHIFT) >> + | GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); >> >> #ifdef CONFIG_CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK >> - sched_clock_register(gt_sched_clock_read, 64, gt_clk_rate); >> + sched_clock_register(gt_sched_clock_read, 64, gt_target_rate); >> #endif >> - return clocksource_register_hz(>_clocksource, gt_clk_rate); >> + return clocksource_register_hz(>_clocksource, gt_target_rate); >> +} >> + >> +static int gt_clk_rate_change_cb(struct notifier_block *nb, >> + unsigned long event, void *data) >> +{ >> + struct clk_notifier_data *ndata = data; >> + >> + switch (event) { >> + case PRE_RATE_CHANGE: >> + { >> + int psv; >> + >> + psv = DIV_ROUND_CLOSEST(ndata->new_rate, >> + gt_target_rate); >> + >> + if (abs(gt_target_rate - (ndata->new_rate / psv)) > MAX_F_ERR) >> + return NOTIFY_BAD; >> + >> + psv--; >> + >> + /* prescaler within legal range? */ >> + if (psv < 0 || psv > GT_CONTROL_PRESCALER_MAX) >> + return NOTIFY_BAD; >> + >> + /* >> + * store timer clock ctrl register so we can restore it in case >> + * of an abort. >> + */ >> + gt_psv_bck = gt_read_presc(); >> + gt_psv_new = psv; >> + /* scale down: adjust divider in post-change notification */ >> + if (ndata->new_rate < ndata->old_rate) >> + return NOTIFY_DONE; >> + >> + /* scale up: adjust divider now - before frequency change */ >> + gt_write_presc(psv); >> + break; >> + } >> + case POST_RATE_CHANGE: >> + /* scale up: pre-change notification did the adjustment */ >> + if (ndata->new_rate > ndata->old_rate) >> + return NOTIFY_OK; >> + >> + /* scale down: adjust divider now - after frequency change */ >> + gt_write_presc(gt_psv_new); >> + break; >> + >> + case ABORT_RATE_CHANGE: >> + /* we have to undo the adjustment in case we scale up */ >> + if (ndata->new_rate < ndata->old_rate) >> + return NOTIFY_OK; >> + >> + /* restore original register value */ >> + gt_write_presc(gt_psv_bck); >> + break; >> + default: >> + return NOTIFY_DONE; >> + } >> + >> + return NOTIFY_DONE; >> } >> >> static int __init global_timer_of_register(struct device_node *np) >> { >> struct clk *gt_clk; >> + static unsigned long gt_clk_rate; >> int err = 0; >> >> /* >> @@ -292,11 +383,20 @@ static int __init global_timer_of_register(struct device_node *np) >> } >> >> gt_clk_rate = clk_get_rate(gt_clk); >> + gt_target_rate = gt_clk_rate / CONFIG_ARM_GT_INITIAL_PRESCALER_VAL; >> + gt_clk_rate_change_nb.notifier_call = >> + gt_clk_rate_change_cb; >> + err = clk_notifier_register(gt_clk, >_clk_rate_change_nb); >> + if (err) { >> + pr_warn("Unable to register clock notifier\n"); >> + goto out_clk; >> + } >> + >> gt_evt = alloc_percpu(struct clock_event_device); >> if (!gt_evt) { >> pr_warn("global-timer: can't allocate memory\n"); >> err = -ENOMEM; >> - goto out_clk; >> + goto out_clk_nb; >> } >> >> err = request_percpu_irq(gt_ppi, gt_clockevent_interrupt, >> @@ -326,6 +426,8 @@ static int __init global_timer_of_register(struct device_node *np) >> free_percpu_irq(gt_ppi, gt_evt); >> out_free: >> free_percpu(gt_evt); >> +out_clk_nb: >> + clk_notifier_unregister(gt_clk, >_clk_rate_change_nb); >> out_clk: >> clk_disable_unprepare(gt_clk); >> out_unmap: >> > > Reviewed-by: Patrice Chotard <patrice.chotard@foss.st.com> Thanks Patrice
| |