lkml.org 
[lkml]   [2021]   [Jun]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 1/2] clocksource: arm_global_timer: implement rate compensation whenever source clock changes
From
Date

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(&gt_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(&gt_clocksource, gt_clk_rate);
> + return clocksource_register_hz(&gt_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, &gt_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, &gt_clk_rate_change_nb);
> out_clk:
> clk_disable_unprepare(gt_clk);
> out_unmap:
>


--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

\
 
 \ /
  Last update: 2021-06-10 15:03    [W:0.093 / U:0.736 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site