[PATCH v2 0/2] Fix missing entropy on Zynq arch due to get_cycles() not supported

[PATCH v2 0/2] Fix missing entropy on Zynq arch due to get_cycles() not supported

[Andrea Merello]

Changes wrt v1:
 - rebased on latest kernel tree
 - fix cover letter: does -> doesn't

A real-world problem has been seen with a Zynq-based board running
Debian 10, where ssh daemon takes a really long time to come up at boot.
This is due to lack of random numbers.

Since commit 50ee7529ec450 ("random: try to actively add entropy rather
than passively wait for it") we try to generate entropy whenever we are
in short of random numbers and someone needs them.

This trick works only when CPU cycle counter is available. On ARM this
means that get_cycles() works and in turn read_current_timer() works.

Zynq HW includes two "cadence TTC" timers and the "ARM global timer".
All these pieces of HW are fed by the CPU clock, which dynamically
changes whenever CPU frequency scaling is enabled.

In timer-cadence-ttc driver this scenario is handled by looking at parent
clock changes and adjusting things when required. This is the only usable
clocksource when CPU frequency scaling is in use. arm_global_timer driver
is disabled in Kconfig when CPU_FREQ is enabled for Zynq arch.

Unfortunately timer-cadence-ttc driver doesn't register itself via
register_current_timer_delay() and that ultimately ends up in get_cycles()
to always return zero, causing the aforementioned lack of entropy problem.
I believe that the reason for this is because Cadence TTC counter on Zynq
silicon is only 16-bit wide.

This patchset works around this by implementing in ARM global timer driver
a mechanism to compensate for parent clock variations, similarly to what
it's done in Cadence TTC timer driver, so that it can be used together
with CPU frequency scaling on Zynq arch.

This proved to finally fix the problem on my Zynq-based Z-turn board.

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>

Andrea Merello (2):
  clocksource: arm_global_timer: implement rate compensation whenever
    source clock changes
  arm: zynq: don't disable CONFIG_ARM_GLOBAL_TIMER due to
    CONFIG_CPU_FREQ anymore

 arch/arm/mach-zynq/Kconfig             |   2 +-
 drivers/clocksource/Kconfig            |  14 +++
 drivers/clocksource/arm_global_timer.c | 122 +++++++++++++++++++++++--
 3 files changed, 127 insertions(+), 11 deletions(-)

--
2.17.1

[PATCH v2 1/2] clocksource: arm_global_timer: implement rate compensation whenever source clock changes

[Andrea Merello]

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:
-- 
2.17.1

Re: [PATCH v2 1/2] clocksource: arm_global_timer: implement rate compensation whenever source clock changes

[Daniel Lezcano]

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

Re: [PATCH v2 1/2] clocksource: arm_global_timer: implement rate compensation whenever source clock changes

[Patrice CHOTARD]

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(&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:
>>
> 
> 
Reviewed-by: Patrice Chotard <patrice.chotard@foss.st.com>
Thanks
Patrice

[tip: timers/core] clocksource/drivers/arm_global_timer: Implement rate compensation whenever source clock changes

[tip-bot2 for Andrea Merello]

The following commit has been merged into the timers/core branch of tip:

Commit-ID:     171b45a4a70eef2fd36bb794ce4f5a48c440361e
Gitweb:        https://git.kernel.org/tip/171b45a4a70eef2fd36bb794ce4f5a48c440361e
Author:        Andrea Merello <andrea.merello@gmail.com>
AuthorDate:    Tue, 06 Apr 2021 15:00:44 +02:00
Committer:     Daniel Lezcano <daniel.lezcano@linaro.org>
CommitterDate: Wed, 16 Jun 2021 17:33:04 +02:00

clocksource/drivers/arm_global_timer: Implement rate compensation whenever source clock changes

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>
Reviewed-by: Patrice Chotard <patrice.chotard@foss.st.com>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Link: https://lore.kernel.org/r/20210406130045.15491-2-andrea.merello@gmail.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 39aa21d..19fc5f8 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 88b2d38..60a8047 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 @@ out_irq:
 	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:

[BUG] [PATCH v2 1/2] clocksource: arm_global_timer: implement rate compensation whenever source clock changes

[Johan Jonker]

[-- Attachment #1: Type: text/plain, Size: 17974 bytes --]

Hi Andrea,

Your patch contribution causes a kernel panic on MK808 with Rockchip rk3066a SoC.
Would you like to contribute to fix this issue?
The psv variable divides with 0 and more things might be wrong.
A revert makes it work again.


Johan

=====

[   13.087809] ------------[ cut here ]------------
[   13.110701] kernel BUG at drivers/cpufreq/cpufreq.c:1480!
[   13.134378] Internal error: Oops - BUG: 0 [#1] SMP ARM
[   13.157793] Modules linked in:
[   13.178760] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W          6.0.0-next-20221013 #1
[   13.205990] Hardware name: Rockchip (Device Tree)
[   13.228793] PC is at cpufreq_online+0x8fc/0xa1c
[   13.251421] LR is at __wake_up_common_lock+0x98/0xc4
[   13.274536] pc : [<c06883d8>]    lr : [<c0171c50>]    psr: a0000013
[   13.299046] sp : f0821c50  ip : f0821b70  fp : f0821c9c
[   13.322445] r10: 00000000  r9 : 00000000  r8 : c0f9b800
[   13.345887] r7 : 000493e0  r6 : c115ef18  r5 : c1005058  r4 : c2055e00
[   13.370714] r3 : b4aeb193  r2 : b4aeb193  r1 : 60000013  r0 : fffffff0
[   13.395373] Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
[   13.420948] Control: 10c5387d  Table: 6000404a  DAC: 00000051
[   13.444914] Register r0 information: non-paged memory
[   13.468230] Register r1 information: non-paged memory
[   13.491289] Register r2 information: non-paged memory
[   13.514135] Register r3 information: non-paged memory
[   13.536767] Register r4 information: slab kmalloc-512 start c2055e00 pointer offset 0 size 512
[   13.563712] Register r5 information: non-slab/vmalloc memory
[   13.587158] Register r6 information: non-slab/vmalloc memory
[   13.610385] Register r7 information: non-paged memory
[   13.632699] Register r8 information: non-slab/vmalloc memory
[   13.655529] Register r9 information: NULL pointer
[   13.677204] Register r10 information: NULL pointer
[   13.698865] Register r11 information: 2-page vmalloc region starting at 0xf0820000 allocated at kernel_clone+0xc0/0x3b0
[   13.727678] Register r12 information: 2-page vmalloc region starting at 0xf0820000 allocated at kernel_clone+0xc0/0x3b0
[   13.756320] Process swapper/0 (pid: 1, stack limit = 0x(ptrval))
[   13.779523] Stack: (0xf0821c50 to 0xf0822000)
[   13.800858] 1c40:                                     00000000 00000001 ef7c5050 00000001
[   13.826719] 1c60: c10da900 00000002 c115ef18 c2055f20 c0942224 00000000 ef7c5050 c1004ec8
[   13.852544] 1c80: c20d7b40 c10da884 c1005058 c1004f14 f0821cb4 f0821ca0 c0688590 c0687ae8
[   13.878330] 1ca0: c10da570 c10cf8a8 f0821ce4 f0821cb8 c0595080 c0688524 c14f7958 c1645434
[   13.904232] 1cc0: 00000000 b4aeb193 00000060 c10da900 c115ef18 c10da434 f0821d0c f0821ce8
[   13.930231] 1ce0: c06859e0 c0594fdc c10da8ec ef7c5050 c20d7b40 00000002 c10da8ec ef7c5050
[   13.956321] 1d00: f0821d8c f0821d10 c068b9a4 c068589c c0b161cc c1004ec8 00000000 c0b7b718
[   13.982392] 1d20: c14e3400 00000000 c20d7b48 00000001 c033f608 c20da5d8 c20da688 c160c688
[   14.008446] 1d40: c0b8bd28 00000001 c0c47d54 c111b000 f0821d84 00000000 00000000 b4aeb193
[   14.034637] 1d60: c14e3410 c14e3410 00000000 c10da898 00000000 c20d7ab8 c0c47d54 c111b000
[   14.060896] 1d80: f0821dac f0821d90 c0599424 c068b68c c14e3410 00000000 c10da898 00000000
[   14.087313] 1da0: f0821dcc f0821db0 c0596cd0 c05993c4 c14e3410 c10da898 c14e3410 00000000
[   14.113601] 1dc0: f0821de4 f0821dd0 c0596f8c c0596c04 c115dd68 c14e3410 f0821e04 f0821de8
[   14.139950] 1de0: c0596fe8 c0596ec0 00000000 c14e3410 c10da898 c059741c f0821e2c f0821e08
[   14.166351] 1e00: c0597490 c0596fb4 c1546ab4 c1004ec8 c10da898 c059741c c20d7ab8 c0c47d54
[   14.192790] 1e20: f0821e5c f0821e30 c0594c2c c0597428 f0821e68 c14f7858 c1546ab4 b4aeb193
[   14.219273] 1e40: c10da898 c10cf808 c20d7a80 00000000 f0821e6c f0821e60 c0596528 c0594bb4
[   14.245875] 1e60: f0821e9c f0821e70 c0595e60 c0596508 c0b7b7c8 00000000 f0821e9c c10da898
[   14.272550] 1e80: c1004ec8 c0c28e84 00000000 c1456854 f0821eb4 f0821ea0 c0597c80 c0595cc0
[   14.299270] 1ea0: c110cec0 c1004ec8 f0821ec4 f0821eb8 c0599130 c0597c08 f0821ed4 f0821ec8
[   14.326125] 1ec0: c0c28ea8 c0599110 f0821f4c f0821ed8 c0102180 c0c28e90 c03318e4 c0b14f30
[   14.352912] 1ee0: c0b14f10 c0b14f00 c0b2bc78 c1004ec8 00000000 c0b855d0 00000006 00000006
[   14.379805] 1f00: 00000000 c0b76eb0 c0c0055c c0bebf78 c0953a9c c154665b 00000000 b4aeb193
[   14.406665] 1f20: c0181cb8 b4aeb193 000000c6 c0c82160 00000007 c0c47d34 c1546600 000000c6
[   14.433661] 1f40: f0821f94 f0821f50 c0c01a48 c0102134 00000006 00000006 00000000 c0c0055c
[   14.460621] 1f60: c0c0055c c0bebf78 c1588000 00004ec0 c096ea48 00000000 00000000 00000000
[   14.487681] 1f80: 00000000 00000000 f0821fac f0821f98 c096ea6c c0c01884 00000000 c096ea48
[   14.514695] 1fa0: 00000000 f0821fb0 c0100148 c096ea54 00000000 00000000 00000000 00000000
[   14.541771] 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[   14.568615] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000
[   14.595194] Backtrace: 
[   14.615207]  cpufreq_online from cpufreq_add_dev+0x78/0x84
[   14.638819]  r10:c1004f14 r9:c1005058 r8:c10da884 r7:c20d7b40 r6:c1004ec8 r5:ef7c5050
[   14.665198]  r4:00000000
[   14.685514]  cpufreq_add_dev from subsys_interface_register+0xb0/0xfc
[   14.710558]  r5:c10cf8a8 r4:c10da570
[   14.732076]  subsys_interface_register from cpufreq_register_driver+0x150/0x2d8
[   14.758094]  r6:c10da434 r5:c115ef18 r4:c10da900
[   14.780943]  cpufreq_register_driver from dt_cpufreq_probe+0x324/0x49c
[   14.806283]  r6:ef7c5050 r5:c10da8ec r4:00000002
[   14.829299]  dt_cpufreq_probe from platform_probe+0x6c/0xc8
[   14.853499]  r10:c111b000 r9:c0c47d54 r8:c20d7ab8 r7:00000000 r6:c10da898 r5:00000000
[   14.880388]  r4:c14e3410
[   14.901252]  platform_probe from really_probe+0xd8/0x2bc
[   14.925473]  r7:00000000 r6:c10da898 r5:00000000 r4:c14e3410
[   14.949898]  really_probe from __driver_probe_device+0xd8/0xf4
[   14.974546]  r7:00000000 r6:c14e3410 r5:c10da898 r4:c14e3410
[   14.998855]  __driver_probe_device from driver_probe_device+0x40/0xd0
[   15.024426]  r5:c14e3410 r4:c115dd68
[   15.046857]  driver_probe_device from __driver_attach+0x74/0x120
[   15.072380]  r7:c059741c r6:c10da898 r5:c14e3410 r4:00000000
[   15.097388]  __driver_attach from bus_for_each_dev+0x84/0xc4
[   15.122444]  r9:c0c47d54 r8:c20d7ab8 r7:c059741c r6:c10da898 r5:c1004ec8 r4:c1546ab4
[   15.150131]  bus_for_each_dev from driver_attach+0x2c/0x30
[   15.175532]  r7:00000000 r6:c20d7a80 r5:c10cf808 r4:c10da898
[   15.201134]  driver_attach from bus_add_driver+0x1ac/0x1f8
[   15.226772]  bus_add_driver from driver_register+0x84/0x11c
[   15.252559]  r8:c1456854 r7:00000000 r6:c0c28e84 r5:c1004ec8 r4:c10da898
[   15.279483]  driver_register from __platform_driver_register+0x2c/0x34
[   15.306523]  r5:c1004ec8 r4:c110cec0
[   15.329801]  __platform_driver_register from dt_cpufreq_platdrv_init+0x24/0x28
[   15.357500]  dt_cpufreq_platdrv_init from do_one_initcall+0x58/0x20c
[   15.384159]  do_one_initcall from kernel_init_freeable+0x1d0/0x22c
[   15.410635]  r8:000000c6 r7:c1546600 r6:c0c47d34 r5:00000007 r4:c0c82160
[   15.437567]  kernel_init_freeable from kernel_init+0x24/0x144
[   15.463573]  r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c096ea48
[   15.491869]  r4:00004ec0
[   15.514022]  kernel_init from ret_from_fork+0x14/0x2c
[   15.539079] Exception stack(0xf0821fb0 to 0xf0821ff8)
[   15.563943] 1fa0:                                     00000000 00000000 00000000 00000000
[   15.592459] 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[   15.620836] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
[   15.647222]  r5:c096ea48 r4:00000000
[   15.670196] Code: e1a00004 ebfff74d e3500000 0a00001a (e7f001f2) 
[   15.696086] ---[ end trace 0000000000000000 ]---
[   15.720522] Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
[   15.748404] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b ]---


On 4/6/21 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;

This psv variable can become a 0 divider.

> +
> +		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:

[-- Attachment #2: v2-0001-Revert-clocksource-drivers-arm_global_timer-Remov.patch --]
[-- Type: text/x-patch, Size: 1084 bytes --]

From d283521df900be5205ab6d7f2e62ae11770e8f56 Mon Sep 17 00:00:00 2001
From: Johan Jonker <jbx6244@gmail.com>
Date: Thu, 13 Oct 2022 20:34:30 +0200
Subject: [PATCH v2 1/5] Revert "clocksource/drivers/arm_global_timer: Remove
 duplicated argument in arm_global_timer"

This reverts commit f94bc2667fb204d7c131ac39d9ea342bd16116dc.
---
 drivers/clocksource/arm_global_timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c
index 44a61dc6f932..68b1d144a412 100644
--- a/drivers/clocksource/arm_global_timer.c
+++ b/drivers/clocksource/arm_global_timer.c
@@ -104,7 +104,7 @@ static void gt_compare_set(unsigned long delta, int periodic)
 	counter += delta;
 	ctrl = readl(gt_base + GT_CONTROL);
 	ctrl &= ~(GT_CONTROL_COMP_ENABLE | GT_CONTROL_IRQ_ENABLE |
-		  GT_CONTROL_AUTO_INC);
+		  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);
-- 
2.20.1


[-- Attachment #3: v2-0002-Revert-clocksource-drivers-arm_global_timer-Make-.patch --]
[-- Type: text/x-patch, Size: 971 bytes --]

From 1c1ed65928038321fa92e88d67329e255ec77144 Mon Sep 17 00:00:00 2001
From: Johan Jonker <jbx6244@gmail.com>
Date: Thu, 13 Oct 2022 20:36:35 +0200
Subject: [PATCH v2 2/5] Revert "clocksource/drivers/arm_global_timer: Make
 symbol 'gt_clk_rate_change_nb' static"

This reverts commit be534f8ee137b95046d7c53c8200ffdcf05781a7.
---
 drivers/clocksource/arm_global_timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c
index 68b1d144a412..60a8047fd32e 100644
--- a/drivers/clocksource/arm_global_timer.c
+++ b/drivers/clocksource/arm_global_timer.c
@@ -51,7 +51,7 @@
  * the units for all operations.
  */
 static void __iomem *gt_base;
-static struct notifier_block gt_clk_rate_change_nb;
+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;
-- 
2.20.1


[-- Attachment #4: v2-0003-Revert-clocksource-drivers-arm_global_timer-Imple.patch --]
[-- Type: text/x-patch, Size: 7835 bytes --]

From 742c4cc1853e2529d7b0d020c0387038f979bf07 Mon Sep 17 00:00:00 2001
From: Johan Jonker <jbx6244@gmail.com>
Date: Thu, 13 Oct 2022 20:45:37 +0200
Subject: [PATCH v2 3/5] Revert "clocksource/drivers/arm_global_timer:
 Implement rate compensation whenever source clock changes"

This reverts commit 171b45a4a70eef2fd36bb794ce4f5a48c440361e.
---
 drivers/clocksource/Kconfig            |  14 ---
 drivers/clocksource/arm_global_timer.c | 122 ++-----------------------
 2 files changed, 10 insertions(+), 126 deletions(-)

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 4469e7f555e9..7b00efa59ffb 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -375,20 +375,6 @@ 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 2 if ARCH_ZYNQ
-	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 its 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 && HAVE_CLK
diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c
index 60a8047fd32e..88b2d38a7a61 100644
--- a/drivers/clocksource/arm_global_timer.c
+++ b/drivers/clocksource/arm_global_timer.c
@@ -31,10 +31,6 @@
 #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)
@@ -43,7 +39,6 @@
 #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.
  *
@@ -51,8 +46,7 @@
  * the units for all operations.
  */
 static void __iomem *gt_base;
-struct notifier_block gt_clk_rate_change_nb;
-static u32 gt_psv_new, gt_psv_bck, gt_target_rate;
+static unsigned long gt_clk_rate;
 static int gt_ppi;
 static struct clock_event_device __percpu *gt_evt;
 
@@ -102,10 +96,7 @@ static void gt_compare_set(unsigned long delta, int periodic)
 	unsigned long ctrl;
 
 	counter += delta;
-	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;
+	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);
@@ -132,7 +123,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_target_rate, HZ), 1);
+	gt_compare_set(DIV_ROUND_CLOSEST(gt_clk_rate, HZ), 1);
 	return 0;
 }
 
@@ -186,7 +177,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_target_rate,
+	clockevents_config_and_register(clk, gt_clk_rate,
 					1, 0xffffffff);
 	enable_percpu_irq(clk->irq, IRQ_TYPE_NONE);
 	return 0;
@@ -241,28 +232,9 @@ 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_target_rate;
+	gt_delay_timer.freq = gt_clk_rate;
 	register_current_timer_delay(&gt_delay_timer);
 }
 
@@ -271,81 +243,18 @@ 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);
-	/* 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);
+	/* enables timer on all the cores */
+	writel(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_target_rate);
+	sched_clock_register(gt_sched_clock_read, 64, gt_clk_rate);
 #endif
-	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;
+	return clocksource_register_hz(&gt_clocksource, gt_clk_rate);
 }
 
 static int __init global_timer_of_register(struct device_node *np)
 {
 	struct clk *gt_clk;
-	static unsigned long gt_clk_rate;
 	int err = 0;
 
 	/*
@@ -383,20 +292,11 @@ 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_nb;
+		goto out_clk;
 	}
 
 	err = request_percpu_irq(gt_ppi, gt_clockevent_interrupt,
@@ -426,8 +326,6 @@ 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:
-- 
2.20.1

Re: [BUG] [PATCH v2 1/2] clocksource: arm_global_timer: implement rate compensation whenever source clock changes

[Andrea Merello]

Il giorno lun 17 ott 2022 alle ore 10:03 Johan Jonker
<jbx6244@gmail.com> ha scritto:
>
> Hi Andrea,
>
> Your patch contribution causes a kernel panic on MK808 with Rockchip rk3066a SoC.
> Would you like to contribute to fix this issue?

Hi! Thank you for reporting this; I'm definitely interested in
investigating this issue.

> The psv variable divides with 0 and more things might be wrong.
> A revert makes it work again.

Hum, good point: it's probably worth adding a check in the LOC you
pointed out and eventually returning NOTIFY_BAD.. Note however that
this would prevent the division by zeroed-psv (which is really
desirable), but it wouldn't make all magically work - so it's probably
worth understanding what's happening.. :

I'm not familiar with your SoC; is it like the Zynq i.e. when enabling
cpu frequency scaling then the clock source feeding the ARM global
timer keeps changing rate? If the rate-change callback has run (and it
calculated a zero psv) then it seems the case.

If this is really the case, then I'd say that the frequency scaling
needed to be disabled (i.e. locked out) when the ARM global timer was
in use without my patch (otherwise your clocksource would be also
unstable), while to take advantage of my patch a reasonable
CONFIG_ARM_GT_INITIAL_PRESCALER_VAL should be set (and hopefully this
prevents psv to become zero).

IOW, in this case, I would expect KConfig not letting enabling
frequency scaling and ARM global timer together (IIRC that was what
Zynq did) in case my patch is not there, or otherwise defining at
least a good default value for CONFIG_ARM_GT_INITIAL_PRESCALER_VAL,
that make it possible to swing the prescaler in a range wide enough.

How's CONFIG_ARM_GT_INITIAL_PRESCALER_VAL set in your kernel
configuration? Maybe tweaking with it prevents the problem?

On the contrary, in case your SoC doesn't vary the ARM global timer
clock source when scaling CPU frequency, then I don't understand why
the rate-change notification callback, where the psv is calculated, is
ever called (although I admit that I'm starting to wonder whether it
is maybe called one time anyway).. But even if it is, then the clock
rate should be the very same, so I would expect the psv to be
calculated as non-zero anyway.. Or am I missing some case in which
approximations can lead to zero?

BTW I'm not really sure about what's going on here: I cannot directly
correlate the Oops you attached below to the LOC you pointed out. How
did you track down the problem to that?

Thanks
Andrea

>
> Johan
>
> =====
>
> [   13.087809] ------------[ cut here ]------------
> [   13.110701] kernel BUG at drivers/cpufreq/cpufreq.c:1480!
> [   13.134378] Internal error: Oops - BUG: 0 [#1] SMP ARM
> [   13.157793] Modules linked in:
> [   13.178760] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W          6.0.0-next-20221013 #1
> [   13.205990] Hardware name: Rockchip (Device Tree)
> [   13.228793] PC is at cpufreq_online+0x8fc/0xa1c
> [   13.251421] LR is at __wake_up_common_lock+0x98/0xc4
> [   13.274536] pc : [<c06883d8>]    lr : [<c0171c50>]    psr: a0000013
> [   13.299046] sp : f0821c50  ip : f0821b70  fp : f0821c9c
> [   13.322445] r10: 00000000  r9 : 00000000  r8 : c0f9b800
> [   13.345887] r7 : 000493e0  r6 : c115ef18  r5 : c1005058  r4 : c2055e00
> [   13.370714] r3 : b4aeb193  r2 : b4aeb193  r1 : 60000013  r0 : fffffff0
> [   13.395373] Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
> [   13.420948] Control: 10c5387d  Table: 6000404a  DAC: 00000051
> [   13.444914] Register r0 information: non-paged memory
> [   13.468230] Register r1 information: non-paged memory
> [   13.491289] Register r2 information: non-paged memory
> [   13.514135] Register r3 information: non-paged memory
> [   13.536767] Register r4 information: slab kmalloc-512 start c2055e00 pointer offset 0 size 512
> [   13.563712] Register r5 information: non-slab/vmalloc memory
> [   13.587158] Register r6 information: non-slab/vmalloc memory
> [   13.610385] Register r7 information: non-paged memory
> [   13.632699] Register r8 information: non-slab/vmalloc memory
> [   13.655529] Register r9 information: NULL pointer
> [   13.677204] Register r10 information: NULL pointer
> [   13.698865] Register r11 information: 2-page vmalloc region starting at 0xf0820000 allocated at kernel_clone+0xc0/0x3b0
> [   13.727678] Register r12 information: 2-page vmalloc region starting at 0xf0820000 allocated at kernel_clone+0xc0/0x3b0
> [   13.756320] Process swapper/0 (pid: 1, stack limit = 0x(ptrval))
> [   13.779523] Stack: (0xf0821c50 to 0xf0822000)
> [   13.800858] 1c40:                                     00000000 00000001 ef7c5050 00000001
> [   13.826719] 1c60: c10da900 00000002 c115ef18 c2055f20 c0942224 00000000 ef7c5050 c1004ec8
> [   13.852544] 1c80: c20d7b40 c10da884 c1005058 c1004f14 f0821cb4 f0821ca0 c0688590 c0687ae8
> [   13.878330] 1ca0: c10da570 c10cf8a8 f0821ce4 f0821cb8 c0595080 c0688524 c14f7958 c1645434
> [   13.904232] 1cc0: 00000000 b4aeb193 00000060 c10da900 c115ef18 c10da434 f0821d0c f0821ce8
> [   13.930231] 1ce0: c06859e0 c0594fdc c10da8ec ef7c5050 c20d7b40 00000002 c10da8ec ef7c5050
> [   13.956321] 1d00: f0821d8c f0821d10 c068b9a4 c068589c c0b161cc c1004ec8 00000000 c0b7b718
> [   13.982392] 1d20: c14e3400 00000000 c20d7b48 00000001 c033f608 c20da5d8 c20da688 c160c688
> [   14.008446] 1d40: c0b8bd28 00000001 c0c47d54 c111b000 f0821d84 00000000 00000000 b4aeb193
> [   14.034637] 1d60: c14e3410 c14e3410 00000000 c10da898 00000000 c20d7ab8 c0c47d54 c111b000
> [   14.060896] 1d80: f0821dac f0821d90 c0599424 c068b68c c14e3410 00000000 c10da898 00000000
> [   14.087313] 1da0: f0821dcc f0821db0 c0596cd0 c05993c4 c14e3410 c10da898 c14e3410 00000000
> [   14.113601] 1dc0: f0821de4 f0821dd0 c0596f8c c0596c04 c115dd68 c14e3410 f0821e04 f0821de8
> [   14.139950] 1de0: c0596fe8 c0596ec0 00000000 c14e3410 c10da898 c059741c f0821e2c f0821e08
> [   14.166351] 1e00: c0597490 c0596fb4 c1546ab4 c1004ec8 c10da898 c059741c c20d7ab8 c0c47d54
> [   14.192790] 1e20: f0821e5c f0821e30 c0594c2c c0597428 f0821e68 c14f7858 c1546ab4 b4aeb193
> [   14.219273] 1e40: c10da898 c10cf808 c20d7a80 00000000 f0821e6c f0821e60 c0596528 c0594bb4
> [   14.245875] 1e60: f0821e9c f0821e70 c0595e60 c0596508 c0b7b7c8 00000000 f0821e9c c10da898
> [   14.272550] 1e80: c1004ec8 c0c28e84 00000000 c1456854 f0821eb4 f0821ea0 c0597c80 c0595cc0
> [   14.299270] 1ea0: c110cec0 c1004ec8 f0821ec4 f0821eb8 c0599130 c0597c08 f0821ed4 f0821ec8
> [   14.326125] 1ec0: c0c28ea8 c0599110 f0821f4c f0821ed8 c0102180 c0c28e90 c03318e4 c0b14f30
> [   14.352912] 1ee0: c0b14f10 c0b14f00 c0b2bc78 c1004ec8 00000000 c0b855d0 00000006 00000006
> [   14.379805] 1f00: 00000000 c0b76eb0 c0c0055c c0bebf78 c0953a9c c154665b 00000000 b4aeb193
> [   14.406665] 1f20: c0181cb8 b4aeb193 000000c6 c0c82160 00000007 c0c47d34 c1546600 000000c6
> [   14.433661] 1f40: f0821f94 f0821f50 c0c01a48 c0102134 00000006 00000006 00000000 c0c0055c
> [   14.460621] 1f60: c0c0055c c0bebf78 c1588000 00004ec0 c096ea48 00000000 00000000 00000000
> [   14.487681] 1f80: 00000000 00000000 f0821fac f0821f98 c096ea6c c0c01884 00000000 c096ea48
> [   14.514695] 1fa0: 00000000 f0821fb0 c0100148 c096ea54 00000000 00000000 00000000 00000000
> [   14.541771] 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [   14.568615] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000
> [   14.595194] Backtrace:
> [   14.615207]  cpufreq_online from cpufreq_add_dev+0x78/0x84
> [   14.638819]  r10:c1004f14 r9:c1005058 r8:c10da884 r7:c20d7b40 r6:c1004ec8 r5:ef7c5050
> [   14.665198]  r4:00000000
> [   14.685514]  cpufreq_add_dev from subsys_interface_register+0xb0/0xfc
> [   14.710558]  r5:c10cf8a8 r4:c10da570
> [   14.732076]  subsys_interface_register from cpufreq_register_driver+0x150/0x2d8
> [   14.758094]  r6:c10da434 r5:c115ef18 r4:c10da900
> [   14.780943]  cpufreq_register_driver from dt_cpufreq_probe+0x324/0x49c
> [   14.806283]  r6:ef7c5050 r5:c10da8ec r4:00000002
> [   14.829299]  dt_cpufreq_probe from platform_probe+0x6c/0xc8
> [   14.853499]  r10:c111b000 r9:c0c47d54 r8:c20d7ab8 r7:00000000 r6:c10da898 r5:00000000
> [   14.880388]  r4:c14e3410
> [   14.901252]  platform_probe from really_probe+0xd8/0x2bc
> [   14.925473]  r7:00000000 r6:c10da898 r5:00000000 r4:c14e3410
> [   14.949898]  really_probe from __driver_probe_device+0xd8/0xf4
> [   14.974546]  r7:00000000 r6:c14e3410 r5:c10da898 r4:c14e3410
> [   14.998855]  __driver_probe_device from driver_probe_device+0x40/0xd0
> [   15.024426]  r5:c14e3410 r4:c115dd68
> [   15.046857]  driver_probe_device from __driver_attach+0x74/0x120
> [   15.072380]  r7:c059741c r6:c10da898 r5:c14e3410 r4:00000000
> [   15.097388]  __driver_attach from bus_for_each_dev+0x84/0xc4
> [   15.122444]  r9:c0c47d54 r8:c20d7ab8 r7:c059741c r6:c10da898 r5:c1004ec8 r4:c1546ab4
> [   15.150131]  bus_for_each_dev from driver_attach+0x2c/0x30
> [   15.175532]  r7:00000000 r6:c20d7a80 r5:c10cf808 r4:c10da898
> [   15.201134]  driver_attach from bus_add_driver+0x1ac/0x1f8
> [   15.226772]  bus_add_driver from driver_register+0x84/0x11c
> [   15.252559]  r8:c1456854 r7:00000000 r6:c0c28e84 r5:c1004ec8 r4:c10da898
> [   15.279483]  driver_register from __platform_driver_register+0x2c/0x34
> [   15.306523]  r5:c1004ec8 r4:c110cec0
> [   15.329801]  __platform_driver_register from dt_cpufreq_platdrv_init+0x24/0x28
> [   15.357500]  dt_cpufreq_platdrv_init from do_one_initcall+0x58/0x20c
> [   15.384159]  do_one_initcall from kernel_init_freeable+0x1d0/0x22c
> [   15.410635]  r8:000000c6 r7:c1546600 r6:c0c47d34 r5:00000007 r4:c0c82160
> [   15.437567]  kernel_init_freeable from kernel_init+0x24/0x144
> [   15.463573]  r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c096ea48
> [   15.491869]  r4:00004ec0
> [   15.514022]  kernel_init from ret_from_fork+0x14/0x2c
> [   15.539079] Exception stack(0xf0821fb0 to 0xf0821ff8)
> [   15.563943] 1fa0:                                     00000000 00000000 00000000 00000000
> [   15.592459] 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [   15.620836] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> [   15.647222]  r5:c096ea48 r4:00000000
> [   15.670196] Code: e1a00004 ebfff74d e3500000 0a00001a (e7f001f2)
> [   15.696086] ---[ end trace 0000000000000000 ]---
> [   15.720522] Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
> [   15.748404] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b ]---
>
>
> On 4/6/21 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;
>
> This psv variable can become a 0 divider.
>
> > +
> > +             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:

Re: [BUG] [PATCH v2 1/2] clocksource: arm_global_timer: implement rate compensation whenever source clock changes

[Johan Jonker]

Hi Andrea,

Some MK80 rk3066a details:
From rk3xxx.dtsi:

	global_timer: global-timer@1013c200 {
		compatible = "arm,cortex-a9-global-timer";
		reg = <0x1013c200 0x20>;
		interrupts = <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_EDGE_RISING)>;
		clocks = <&cru CORE_PERI>;
	};

===

rk3066a clk_summery:
                                 enable  prepare  protect                                duty  hardware
   clock                          count    count    count        rate   accuracy phase  cycle    enable
-------------------------------------------------------------------------------------------------------
 xin24m                               8        8        0    24000000          0     0  50000         Y
    pll_apll                          1        1        0  1416000000          0     0  50000         Y
       apll                           1        1        0  1416000000          0     0  50000         Y
          armclk                      2        2        0  1416000000          0     0  50000         Y
             core_peri                2        2        0   177000000          0     0  50000         Y

===

Before your patch:

[    0.000000] >>>> GT gt_clk_rate:75000000
[    0.000000] >>>> GT gt_clocksource_init ctrl   :00000001

[    0.000105] >>>> GT gt_clockevent_shutdown ctrl:00000001
[    0.000128] >>>> GT gt_compare_set delta       :750000
[    0.000143] >>>> GT gt_compare_set ctrl        :0000000f
[    0.000412] >>>> GT gt_clockevent_shutdown ctrl:00000001
[    0.000444] >>>> GT gt_clockevent_shutdown ctrl:00000001

===

After your patch  with prescaler 1:

[    0.000000] >>>> GT CONFIG_ARM_GT_INITIAL_PRESCALER_VAL:2
[    0.000000] >>>> GT gt_clk_rate                        :75000000
[    0.000000] >>>> GT gt_target_rate                     :37500000
[    0.000000] >>>> GT gt_clocksource_init ctrl   :00000101
[    0.000002] sched_clock: 64 bits at 38MHz, resolution 26ns, wraps every 2199023255540ns
[    0.000049] clocksource: arm_global_timer: mask: 0xffffffffffffffff max_cycles: 0x8a60dd6a9, max_idle_ns: 440795204056 ns
[    0.000104] >>>> GT gt_clockevent_shutdown ctrl:00000101
[    0.000129] >>>> GT gt_compare_set delta       :375000
[    0.000143] >>>> GT gt_compare_set ctrl        :0000010f
[    0.000175] Switching to timer-based delay loop, resolution 26ns
[    0.000396] >>>> GT gt_clockevent_shutdown ctrl:00000101
[    0.000428] >>>> GT gt_clockevent_shutdown ctrl:00000101
[    0.001785] clocksource: timer: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 79635851949 ns
[    0.002136] Ignoring duplicate/late registration of read_current_timer delay

[   13.783289] calling  dt_cpufreq_platdrv_init+0x0/0x28 @ 1
[   13.809106] >>>> GT gt_clk_rate_change_cb ndata->new_rate:78000000
[   13.809148] >>>> GT gt_clk_rate_change_cb gt_target_rate :37500000
[   13.833691] >>>> GT gt_clk_rate_change_cb psv 1                                        :2
[   13.857882] >>>> GT gt_clk_rate_change_cb MAX_F_ERR                                    :50
[   13.884605] >>>> GT gt_clk_rate_change_cb abs(gt_target_rate - (ndata->new_rate / psv)):1500000
[   13.911207] >>>> GT NOTIFY_BAD 1
[   13.938409] >>>> GT gt_write_presc psv:0
[   13.959266] >>>> GT gt_write_presc reg:00000001
[   13.980873] cpu cpu0: _opp_config_clk_single: failed to set clock rate: -16
[   14.074809] cpufreq: __target_index: Failed to change cpu frequency: -16
[   14.124490] ------------[ cut here ]------------
[   14.168380] kernel BUG at drivers/cpufreq/cpufreq.c:1480!

===

After your patch with prescaler 2:

[    0.000000] >>>> GT CONFIG_ARM_GT_INITIAL_PRESCALER_VAL:2
[    0.000000] >>>> GT gt_clk_rate                        :75000000
[    0.000000] >>>> GT gt_target_rate                     :37500000
[    0.000000] >>>> GT gt_clocksource_init ctrl   :00000101
[    0.000002] sched_clock: 64 bits at 38MHz, resolution 26ns, wraps every 2199023255540ns
[    0.000049] clocksource: arm_global_timer: mask: 0xffffffffffffffff max_cycles: 0x8a60dd6a9, max_idle_ns: 440795204056 ns
[    0.000104] >>>> GT gt_clockevent_shutdown ctrl:00000101
[    0.000129] >>>> GT gt_compare_set delta       :375000
[    0.000143] >>>> GT gt_compare_set ctrl        :0000010f
[    0.000175] Switching to timer-based delay loop, resolution 26ns
[    0.000396] >>>> GT gt_clockevent_shutdown ctrl:00000101
[    0.000428] >>>> GT gt_clockevent_shutdown ctrl:00000101
[    0.001785] clocksource: timer: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 79635851949 ns
[    0.002136] Ignoring duplicate/late registration of read_current_timer delay

[   13.783289] calling  dt_cpufreq_platdrv_init+0x0/0x28 @ 1
[   13.809106] >>>> GT gt_clk_rate_change_cb ndata->new_rate:78000000
[   13.809148] >>>> GT gt_clk_rate_change_cb gt_target_rate :37500000
[   13.833691] >>>> GT gt_clk_rate_change_cb psv 1                                        :2
[   13.857882] >>>> GT gt_clk_rate_change_cb MAX_F_ERR                                    :50
[   13.884605] >>>> GT gt_clk_rate_change_cb abs(gt_target_rate - (ndata->new_rate / psv)):1500000
[   13.911207] >>>> GT NOTIFY_BAD 1
[   13.938409] >>>> GT gt_write_presc psv:0
[   13.959266] >>>> GT gt_write_presc reg:00000001
[   13.980873] cpu cpu0: _opp_config_clk_single: failed to set clock rate: -16
[   14.074809] cpufreq: __target_index: Failed to change cpu frequency: -16
[   14.124490] ------------[ cut here ]------------
[   14.168380] kernel BUG at drivers/cpufreq/cpufreq.c:1480!

On 10/27/22 15:37, Andrea Merello wrote:
> Il giorno lun 17 ott 2022 alle ore 10:03 Johan Jonker
> <jbx6244@gmail.com> ha scritto:
>>
>> Hi Andrea,
>>
>> Your patch contribution causes a kernel panic on MK808 with Rockchip rk3066a SoC.
>> Would you like to contribute to fix this issue?
> 
> Hi! Thank you for reporting this; I'm definitely interested in
> investigating this issue.
> 
>> The psv variable divides with 0 and more things might be wrong.
>> A revert makes it work again.
> 
> Hum, good point: it's probably worth adding a check in the LOC you
> pointed out and eventually returning NOTIFY_BAD.. Note however that
> this would prevent the division by zeroed-psv (which is really
> desirable), but it wouldn't make all magically work - so it's probably
> worth understanding what's happening.. :
> 
> I'm not familiar with your SoC; is it like the Zynq i.e. when enabling
> cpu frequency scaling then the clock source feeding the ARM global
> timer keeps changing rate? If the rate-change callback has run (and it
> calculated a zero psv) then it seems the case.
> 
> If this is really the case, then I'd say that the frequency scaling
> needed to be disabled (i.e. locked out) when the ARM global timer was
> in use without my patch (otherwise your clocksource would be also
> unstable), while to take advantage of my patch a reasonable
> CONFIG_ARM_GT_INITIAL_PRESCALER_VAL should be set (and hopefully this
> prevents psv to become zero).
> 
> IOW, in this case, I would expect KConfig not letting enabling
> frequency scaling and ARM global timer together (IIRC that was what
> Zynq did) in case my patch is not there, or otherwise defining at
> least a good default value for CONFIG_ARM_GT_INITIAL_PRESCALER_VAL,
> that make it possible to swing the prescaler in a range wide enough.
> 
> How's CONFIG_ARM_GT_INITIAL_PRESCALER_VAL set in your kernel
> configuration? Maybe tweaking with it prevents the problem?
> 
> On the contrary, in case your SoC doesn't vary the ARM global timer
> clock source when scaling CPU frequency, then I don't understand why
> the rate-change notification callback, where the psv is calculated, is
> ever called (although I admit that I'm starting to wonder whether it
> is maybe called one time anyway).. But even if it is, then the clock
> rate should be the very same, so I would expect the psv to be
> calculated as non-zero anyway.. Or am I missing some case in which
> approximations can lead to zero?
> 
> BTW I'm not really sure about what's going on here: I cannot directly
> correlate the Oops you attached below to the LOC you pointed out. How
> did you track down the problem to that?
> 
> Thanks
> Andrea
> 
>>
>> Johan
>>
>> =====
>>
>> [   13.087809] ------------[ cut here ]------------
>> [   13.110701] kernel BUG at drivers/cpufreq/cpufreq.c:1480!
>> [   13.134378] Internal error: Oops - BUG: 0 [#1] SMP ARM
>> [   13.157793] Modules linked in:
>> [   13.178760] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W          6.0.0-next-20221013 #1
>> [   13.205990] Hardware name: Rockchip (Device Tree)
>> [   13.228793] PC is at cpufreq_online+0x8fc/0xa1c
>> [   13.251421] LR is at __wake_up_common_lock+0x98/0xc4
>> [   13.274536] pc : [<c06883d8>]    lr : [<c0171c50>]    psr: a0000013
>> [   13.299046] sp : f0821c50  ip : f0821b70  fp : f0821c9c
>> [   13.322445] r10: 00000000  r9 : 00000000  r8 : c0f9b800
>> [   13.345887] r7 : 000493e0  r6 : c115ef18  r5 : c1005058  r4 : c2055e00
>> [   13.370714] r3 : b4aeb193  r2 : b4aeb193  r1 : 60000013  r0 : fffffff0
>> [   13.395373] Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
>> [   13.420948] Control: 10c5387d  Table: 6000404a  DAC: 00000051
>> [   13.444914] Register r0 information: non-paged memory
>> [   13.468230] Register r1 information: non-paged memory
>> [   13.491289] Register r2 information: non-paged memory
>> [   13.514135] Register r3 information: non-paged memory
>> [   13.536767] Register r4 information: slab kmalloc-512 start c2055e00 pointer offset 0 size 512
>> [   13.563712] Register r5 information: non-slab/vmalloc memory
>> [   13.587158] Register r6 information: non-slab/vmalloc memory
>> [   13.610385] Register r7 information: non-paged memory
>> [   13.632699] Register r8 information: non-slab/vmalloc memory
>> [   13.655529] Register r9 information: NULL pointer
>> [   13.677204] Register r10 information: NULL pointer
>> [   13.698865] Register r11 information: 2-page vmalloc region starting at 0xf0820000 allocated at kernel_clone+0xc0/0x3b0
>> [   13.727678] Register r12 information: 2-page vmalloc region starting at 0xf0820000 allocated at kernel_clone+0xc0/0x3b0
>> [   13.756320] Process swapper/0 (pid: 1, stack limit = 0x(ptrval))
>> [   13.779523] Stack: (0xf0821c50 to 0xf0822000)
>> [   13.800858] 1c40:                                     00000000 00000001 ef7c5050 00000001
>> [   13.826719] 1c60: c10da900 00000002 c115ef18 c2055f20 c0942224 00000000 ef7c5050 c1004ec8
>> [   13.852544] 1c80: c20d7b40 c10da884 c1005058 c1004f14 f0821cb4 f0821ca0 c0688590 c0687ae8
>> [   13.878330] 1ca0: c10da570 c10cf8a8 f0821ce4 f0821cb8 c0595080 c0688524 c14f7958 c1645434
>> [   13.904232] 1cc0: 00000000 b4aeb193 00000060 c10da900 c115ef18 c10da434 f0821d0c f0821ce8
>> [   13.930231] 1ce0: c06859e0 c0594fdc c10da8ec ef7c5050 c20d7b40 00000002 c10da8ec ef7c5050
>> [   13.956321] 1d00: f0821d8c f0821d10 c068b9a4 c068589c c0b161cc c1004ec8 00000000 c0b7b718
>> [   13.982392] 1d20: c14e3400 00000000 c20d7b48 00000001 c033f608 c20da5d8 c20da688 c160c688
>> [   14.008446] 1d40: c0b8bd28 00000001 c0c47d54 c111b000 f0821d84 00000000 00000000 b4aeb193
>> [   14.034637] 1d60: c14e3410 c14e3410 00000000 c10da898 00000000 c20d7ab8 c0c47d54 c111b000
>> [   14.060896] 1d80: f0821dac f0821d90 c0599424 c068b68c c14e3410 00000000 c10da898 00000000
>> [   14.087313] 1da0: f0821dcc f0821db0 c0596cd0 c05993c4 c14e3410 c10da898 c14e3410 00000000
>> [   14.113601] 1dc0: f0821de4 f0821dd0 c0596f8c c0596c04 c115dd68 c14e3410 f0821e04 f0821de8
>> [   14.139950] 1de0: c0596fe8 c0596ec0 00000000 c14e3410 c10da898 c059741c f0821e2c f0821e08
>> [   14.166351] 1e00: c0597490 c0596fb4 c1546ab4 c1004ec8 c10da898 c059741c c20d7ab8 c0c47d54
>> [   14.192790] 1e20: f0821e5c f0821e30 c0594c2c c0597428 f0821e68 c14f7858 c1546ab4 b4aeb193
>> [   14.219273] 1e40: c10da898 c10cf808 c20d7a80 00000000 f0821e6c f0821e60 c0596528 c0594bb4
>> [   14.245875] 1e60: f0821e9c f0821e70 c0595e60 c0596508 c0b7b7c8 00000000 f0821e9c c10da898
>> [   14.272550] 1e80: c1004ec8 c0c28e84 00000000 c1456854 f0821eb4 f0821ea0 c0597c80 c0595cc0
>> [   14.299270] 1ea0: c110cec0 c1004ec8 f0821ec4 f0821eb8 c0599130 c0597c08 f0821ed4 f0821ec8
>> [   14.326125] 1ec0: c0c28ea8 c0599110 f0821f4c f0821ed8 c0102180 c0c28e90 c03318e4 c0b14f30
>> [   14.352912] 1ee0: c0b14f10 c0b14f00 c0b2bc78 c1004ec8 00000000 c0b855d0 00000006 00000006
>> [   14.379805] 1f00: 00000000 c0b76eb0 c0c0055c c0bebf78 c0953a9c c154665b 00000000 b4aeb193
>> [   14.406665] 1f20: c0181cb8 b4aeb193 000000c6 c0c82160 00000007 c0c47d34 c1546600 000000c6
>> [   14.433661] 1f40: f0821f94 f0821f50 c0c01a48 c0102134 00000006 00000006 00000000 c0c0055c
>> [   14.460621] 1f60: c0c0055c c0bebf78 c1588000 00004ec0 c096ea48 00000000 00000000 00000000
>> [   14.487681] 1f80: 00000000 00000000 f0821fac f0821f98 c096ea6c c0c01884 00000000 c096ea48
>> [   14.514695] 1fa0: 00000000 f0821fb0 c0100148 c096ea54 00000000 00000000 00000000 00000000
>> [   14.541771] 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
>> [   14.568615] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000
>> [   14.595194] Backtrace:
>> [   14.615207]  cpufreq_online from cpufreq_add_dev+0x78/0x84
>> [   14.638819]  r10:c1004f14 r9:c1005058 r8:c10da884 r7:c20d7b40 r6:c1004ec8 r5:ef7c5050
>> [   14.665198]  r4:00000000
>> [   14.685514]  cpufreq_add_dev from subsys_interface_register+0xb0/0xfc
>> [   14.710558]  r5:c10cf8a8 r4:c10da570
>> [   14.732076]  subsys_interface_register from cpufreq_register_driver+0x150/0x2d8
>> [   14.758094]  r6:c10da434 r5:c115ef18 r4:c10da900
>> [   14.780943]  cpufreq_register_driver from dt_cpufreq_probe+0x324/0x49c
>> [   14.806283]  r6:ef7c5050 r5:c10da8ec r4:00000002
>> [   14.829299]  dt_cpufreq_probe from platform_probe+0x6c/0xc8
>> [   14.853499]  r10:c111b000 r9:c0c47d54 r8:c20d7ab8 r7:00000000 r6:c10da898 r5:00000000
>> [   14.880388]  r4:c14e3410
>> [   14.901252]  platform_probe from really_probe+0xd8/0x2bc
>> [   14.925473]  r7:00000000 r6:c10da898 r5:00000000 r4:c14e3410
>> [   14.949898]  really_probe from __driver_probe_device+0xd8/0xf4
>> [   14.974546]  r7:00000000 r6:c14e3410 r5:c10da898 r4:c14e3410
>> [   14.998855]  __driver_probe_device from driver_probe_device+0x40/0xd0
>> [   15.024426]  r5:c14e3410 r4:c115dd68
>> [   15.046857]  driver_probe_device from __driver_attach+0x74/0x120
>> [   15.072380]  r7:c059741c r6:c10da898 r5:c14e3410 r4:00000000
>> [   15.097388]  __driver_attach from bus_for_each_dev+0x84/0xc4
>> [   15.122444]  r9:c0c47d54 r8:c20d7ab8 r7:c059741c r6:c10da898 r5:c1004ec8 r4:c1546ab4
>> [   15.150131]  bus_for_each_dev from driver_attach+0x2c/0x30
>> [   15.175532]  r7:00000000 r6:c20d7a80 r5:c10cf808 r4:c10da898
>> [   15.201134]  driver_attach from bus_add_driver+0x1ac/0x1f8
>> [   15.226772]  bus_add_driver from driver_register+0x84/0x11c
>> [   15.252559]  r8:c1456854 r7:00000000 r6:c0c28e84 r5:c1004ec8 r4:c10da898
>> [   15.279483]  driver_register from __platform_driver_register+0x2c/0x34
>> [   15.306523]  r5:c1004ec8 r4:c110cec0
>> [   15.329801]  __platform_driver_register from dt_cpufreq_platdrv_init+0x24/0x28
>> [   15.357500]  dt_cpufreq_platdrv_init from do_one_initcall+0x58/0x20c
>> [   15.384159]  do_one_initcall from kernel_init_freeable+0x1d0/0x22c
>> [   15.410635]  r8:000000c6 r7:c1546600 r6:c0c47d34 r5:00000007 r4:c0c82160
>> [   15.437567]  kernel_init_freeable from kernel_init+0x24/0x144
>> [   15.463573]  r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c096ea48
>> [   15.491869]  r4:00004ec0
>> [   15.514022]  kernel_init from ret_from_fork+0x14/0x2c
>> [   15.539079] Exception stack(0xf0821fb0 to 0xf0821ff8)
>> [   15.563943] 1fa0:                                     00000000 00000000 00000000 00000000
>> [   15.592459] 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
>> [   15.620836] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
>> [   15.647222]  r5:c096ea48 r4:00000000
>> [   15.670196] Code: e1a00004 ebfff74d e3500000 0a00001a (e7f001f2)
>> [   15.696086] ---[ end trace 0000000000000000 ]---
>> [   15.720522] Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
>> [   15.748404] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b ]---
>>
>>
>> On 4/6/21 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;
>>
>> This psv variable can become a 0 divider.
>>
>>> +
>>> +             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:

Re: [BUG] [PATCH v2 1/2] clocksource: arm_global_timer: implement rate compensation whenever source clock changes

[Johan Jonker]

Hi,

Let me know what more data you like?
Function gt_clk_rate_change_cb() generates only errors.
 
Johan

On 10/30/22 15:50, Johan Jonker wrote:
> Hi Andrea,
> 
> Some MK80 rk3066a details:
> From rk3xxx.dtsi:
> 
> 	global_timer: global-timer@1013c200 {
> 		compatible = "arm,cortex-a9-global-timer";
> 		reg = <0x1013c200 0x20>;
> 		interrupts = <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_EDGE_RISING)>;
> 		clocks = <&cru CORE_PERI>;
> 	};
> 
> ===
> 
> rk3066a clk_summery:
>                                  enable  prepare  protect                                duty  hardware
>    clock                          count    count    count        rate   accuracy phase  cycle    enable
> -------------------------------------------------------------------------------------------------------
>  xin24m                               8        8        0    24000000          0     0  50000         Y
>     pll_apll                          1        1        0  1416000000          0     0  50000         Y
>        apll                           1        1        0  1416000000          0     0  50000         Y
>           armclk                      2        2        0  1416000000          0     0  50000         Y

divider 1:

DivFree
1~32

===

divider 2:

Divider can be Div
2,4,8,16
mux

Does that tree match with this prescaler patch?

>              core_peri                2        2        0   177000000          0     0  50000         Y
> 
> ===
> 
> Before your patch:
> 
> [    0.000000] >>>> GT gt_clk_rate:75000000
> [    0.000000] >>>> GT gt_clocksource_init ctrl   :00000001
> 
> [    0.000105] >>>> GT gt_clockevent_shutdown ctrl:00000001
> [    0.000128] >>>> GT gt_compare_set delta       :750000
> [    0.000143] >>>> GT gt_compare_set ctrl        :0000000f
> [    0.000412] >>>> GT gt_clockevent_shutdown ctrl:00000001
> [    0.000444] >>>> GT gt_clockevent_shutdown ctrl:00000001
> 
> ===
> 
> After your patch  with prescaler 1:

Copy went wrong. Should have been example for prescaler 1.
> 
> [    0.000000] >>>> GT CONFIG_ARM_GT_INITIAL_PRESCALER_VAL:2

> [    0.000000] >>>> GT gt_clk_rate                        :75000000
> [    0.000000] >>>> GT gt_target_rate                     :37500000

[    0.000000] >>>> GT gt_clk_rate                        :75000000
[    0.000000] >>>> GT gt_target_rate                     :75000000



> [    0.000000] >>>> GT gt_clocksource_init ctrl   :00000101
> [    0.000002] sched_clock: 64 bits at 38MHz, resolution 26ns, wraps every 2199023255540ns
> [    0.000049] clocksource: arm_global_timer: mask: 0xffffffffffffffff max_cycles: 0x8a60dd6a9, max_idle_ns: 440795204056 ns
> [    0.000104] >>>> GT gt_clockevent_shutdown ctrl:00000101
> [    0.000129] >>>> GT gt_compare_set delta       :375000
> [    0.000143] >>>> GT gt_compare_set ctrl        :0000010f
> [    0.000175] Switching to timer-based delay loop, resolution 26ns
> [    0.000396] >>>> GT gt_clockevent_shutdown ctrl:00000101
> [    0.000428] >>>> GT gt_clockevent_shutdown ctrl:00000101
> [    0.001785] clocksource: timer: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 79635851949 ns
> [    0.002136] Ignoring duplicate/late registration of read_current_timer delay
> 
> [   13.783289] calling  dt_cpufreq_platdrv_init+0x0/0x28 @ 1

> [   13.809106] >>>> GT gt_clk_rate_change_cb ndata->new_rate:78000000
> [   13.809148] >>>> GT gt_clk_rate_change_cb gt_target_rate :37500000


[   13.832554] >>>> GT gt_clk_rate_change_cb ndata->new_rate:78000000
[   13.832596] >>>> GT gt_clk_rate_change_cb gt_target_ratem:75000000

These rates don't match.

> [   13.833691] >>>> GT gt_clk_rate_change_cb psv 1                                        :2
> [   13.857882] >>>> GT gt_clk_rate_change_cb MAX_F_ERR                                    :50
> [   13.884605] >>>> GT gt_clk_rate_change_cb abs(gt_target_rate - (ndata->new_rate / psv)):1500000

The differance is larger then MAX_F_ERR.

> [   13.911207] >>>> GT NOTIFY_BAD 1

Result is NOTIFY_BAD.

> [   13.938409] >>>> GT gt_write_presc psv:0
> [   13.959266] >>>> GT gt_write_presc reg:00000001
> [   13.980873] cpu cpu0: _opp_config_clk_single: failed to set clock rate: -16
> [   14.074809] cpufreq: __target_index: Failed to change cpu frequency: -16
> [   14.124490] ------------[ cut here ]------------
> [   14.168380] kernel BUG at drivers/cpufreq/cpufreq.c:1480!
> 
> ===
> 
> After your patch with prescaler 2:
> 
> [    0.000000] >>>> GT CONFIG_ARM_GT_INITIAL_PRESCALER_VAL:2
> [    0.000000] >>>> GT gt_clk_rate                        :75000000
> [    0.000000] >>>> GT gt_target_rate                     :37500000
> [    0.000000] >>>> GT gt_clocksource_init ctrl   :00000101
> [    0.000002] sched_clock: 64 bits at 38MHz, resolution 26ns, wraps every 2199023255540ns
> [    0.000049] clocksource: arm_global_timer: mask: 0xffffffffffffffff max_cycles: 0x8a60dd6a9, max_idle_ns: 440795204056 ns
> [    0.000104] >>>> GT gt_clockevent_shutdown ctrl:00000101
> [    0.000129] >>>> GT gt_compare_set delta       :375000
> [    0.000143] >>>> GT gt_compare_set ctrl        :0000010f
> [    0.000175] Switching to timer-based delay loop, resolution 26ns
> [    0.000396] >>>> GT gt_clockevent_shutdown ctrl:00000101
> [    0.000428] >>>> GT gt_clockevent_shutdown ctrl:00000101
> [    0.001785] clocksource: timer: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 79635851949 ns
> [    0.002136] Ignoring duplicate/late registration of read_current_timer delay
> 
> [   13.783289] calling  dt_cpufreq_platdrv_init+0x0/0x28 @ 1
> [   13.809106] >>>> GT gt_clk_rate_change_cb ndata->new_rate:78000000
> [   13.809148] >>>> GT gt_clk_rate_change_cb gt_target_rate :37500000
> [   13.833691] >>>> GT gt_clk_rate_change_cb psv 1                                        :2
> [   13.857882] >>>> GT gt_clk_rate_change_cb MAX_F_ERR                                    :50
> [   13.884605] >>>> GT gt_clk_rate_change_cb abs(gt_target_rate - (ndata->new_rate / psv)):1500000
> [   13.911207] >>>> GT NOTIFY_BAD 1
> [   13.938409] >>>> GT gt_write_presc psv:0
> [   13.959266] >>>> GT gt_write_presc reg:00000001
> [   13.980873] cpu cpu0: _opp_config_clk_single: failed to set clock rate: -16
> [   14.074809] cpufreq: __target_index: Failed to change cpu frequency: -16
> [   14.124490] ------------[ cut here ]------------
> [   14.168380] kernel BUG at drivers/cpufreq/cpufreq.c:1480!
> 
> On 10/27/22 15:37, Andrea Merello wrote:
>> Il giorno lun 17 ott 2022 alle ore 10:03 Johan Jonker
>> <jbx6244@gmail.com> ha scritto:
>>>
>>> Hi Andrea,
>>>
>>> Your patch contribution causes a kernel panic on MK808 with Rockchip rk3066a SoC.
>>> Would you like to contribute to fix this issue?
>>
>> Hi! Thank you for reporting this; I'm definitely interested in
>> investigating this issue.
>>
>>> The psv variable divides with 0 and more things might be wrong.
>>> A revert makes it work again.
>>
>> Hum, good point: it's probably worth adding a check in the LOC you
>> pointed out and eventually returning NOTIFY_BAD.. Note however that
>> this would prevent the division by zeroed-psv (which is really
>> desirable), but it wouldn't make all magically work - so it's probably
>> worth understanding what's happening.. :
>>
>> I'm not familiar with your SoC; is it like the Zynq i.e. when enabling
>> cpu frequency scaling then the clock source feeding the ARM global
>> timer keeps changing rate? If the rate-change callback has run (and it
>> calculated a zero psv) then it seems the case.
>>
>> If this is really the case, then I'd say that the frequency scaling
>> needed to be disabled (i.e. locked out) when the ARM global timer was
>> in use without my patch (otherwise your clocksource would be also
>> unstable), while to take advantage of my patch a reasonable
>> CONFIG_ARM_GT_INITIAL_PRESCALER_VAL should be set (and hopefully this
>> prevents psv to become zero).
>>
>> IOW, in this case, I would expect KConfig not letting enabling
>> frequency scaling and ARM global timer together (IIRC that was what
>> Zynq did) in case my patch is not there, or otherwise defining at
>> least a good default value for CONFIG_ARM_GT_INITIAL_PRESCALER_VAL,
>> that make it possible to swing the prescaler in a range wide enough.
>>
>> How's CONFIG_ARM_GT_INITIAL_PRESCALER_VAL set in your kernel
>> configuration? Maybe tweaking with it prevents the problem?
>>
>> On the contrary, in case your SoC doesn't vary the ARM global timer
>> clock source when scaling CPU frequency, then I don't understand why
>> the rate-change notification callback, where the psv is calculated, is
>> ever called (although I admit that I'm starting to wonder whether it
>> is maybe called one time anyway).. But even if it is, then the clock
>> rate should be the very same, so I would expect the psv to be
>> calculated as non-zero anyway.. Or am I missing some case in which
>> approximations can lead to zero?
>>
>> BTW I'm not really sure about what's going on here: I cannot directly
>> correlate the Oops you attached below to the LOC you pointed out. How
>> did you track down the problem to that?
>>
>> Thanks
>> Andrea
>>
>>>
>>> Johan
>>>
>>> =====
>>>
>>> [   13.087809] ------------[ cut here ]------------
>>> [   13.110701] kernel BUG at drivers/cpufreq/cpufreq.c:1480!
>>> [   13.134378] Internal error: Oops - BUG: 0 [#1] SMP ARM
>>> [   13.157793] Modules linked in:
>>> [   13.178760] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W          6.0.0-next-20221013 #1
>>> [   13.205990] Hardware name: Rockchip (Device Tree)
>>> [   13.228793] PC is at cpufreq_online+0x8fc/0xa1c
>>> [   13.251421] LR is at __wake_up_common_lock+0x98/0xc4
>>> [   13.274536] pc : [<c06883d8>]    lr : [<c0171c50>]    psr: a0000013
>>> [   13.299046] sp : f0821c50  ip : f0821b70  fp : f0821c9c
>>> [   13.322445] r10: 00000000  r9 : 00000000  r8 : c0f9b800
>>> [   13.345887] r7 : 000493e0  r6 : c115ef18  r5 : c1005058  r4 : c2055e00
>>> [   13.370714] r3 : b4aeb193  r2 : b4aeb193  r1 : 60000013  r0 : fffffff0
>>> [   13.395373] Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
>>> [   13.420948] Control: 10c5387d  Table: 6000404a  DAC: 00000051
>>> [   13.444914] Register r0 information: non-paged memory
>>> [   13.468230] Register r1 information: non-paged memory
>>> [   13.491289] Register r2 information: non-paged memory
>>> [   13.514135] Register r3 information: non-paged memory
>>> [   13.536767] Register r4 information: slab kmalloc-512 start c2055e00 pointer offset 0 size 512
>>> [   13.563712] Register r5 information: non-slab/vmalloc memory
>>> [   13.587158] Register r6 information: non-slab/vmalloc memory
>>> [   13.610385] Register r7 information: non-paged memory
>>> [   13.632699] Register r8 information: non-slab/vmalloc memory
>>> [   13.655529] Register r9 information: NULL pointer
>>> [   13.677204] Register r10 information: NULL pointer
>>> [   13.698865] Register r11 information: 2-page vmalloc region starting at 0xf0820000 allocated at kernel_clone+0xc0/0x3b0
>>> [   13.727678] Register r12 information: 2-page vmalloc region starting at 0xf0820000 allocated at kernel_clone+0xc0/0x3b0
>>> [   13.756320] Process swapper/0 (pid: 1, stack limit = 0x(ptrval))
>>> [   13.779523] Stack: (0xf0821c50 to 0xf0822000)
>>> [   13.800858] 1c40:                                     00000000 00000001 ef7c5050 00000001
>>> [   13.826719] 1c60: c10da900 00000002 c115ef18 c2055f20 c0942224 00000000 ef7c5050 c1004ec8
>>> [   13.852544] 1c80: c20d7b40 c10da884 c1005058 c1004f14 f0821cb4 f0821ca0 c0688590 c0687ae8
>>> [   13.878330] 1ca0: c10da570 c10cf8a8 f0821ce4 f0821cb8 c0595080 c0688524 c14f7958 c1645434
>>> [   13.904232] 1cc0: 00000000 b4aeb193 00000060 c10da900 c115ef18 c10da434 f0821d0c f0821ce8
>>> [   13.930231] 1ce0: c06859e0 c0594fdc c10da8ec ef7c5050 c20d7b40 00000002 c10da8ec ef7c5050
>>> [   13.956321] 1d00: f0821d8c f0821d10 c068b9a4 c068589c c0b161cc c1004ec8 00000000 c0b7b718
>>> [   13.982392] 1d20: c14e3400 00000000 c20d7b48 00000001 c033f608 c20da5d8 c20da688 c160c688
>>> [   14.008446] 1d40: c0b8bd28 00000001 c0c47d54 c111b000 f0821d84 00000000 00000000 b4aeb193
>>> [   14.034637] 1d60: c14e3410 c14e3410 00000000 c10da898 00000000 c20d7ab8 c0c47d54 c111b000
>>> [   14.060896] 1d80: f0821dac f0821d90 c0599424 c068b68c c14e3410 00000000 c10da898 00000000
>>> [   14.087313] 1da0: f0821dcc f0821db0 c0596cd0 c05993c4 c14e3410 c10da898 c14e3410 00000000
>>> [   14.113601] 1dc0: f0821de4 f0821dd0 c0596f8c c0596c04 c115dd68 c14e3410 f0821e04 f0821de8
>>> [   14.139950] 1de0: c0596fe8 c0596ec0 00000000 c14e3410 c10da898 c059741c f0821e2c f0821e08
>>> [   14.166351] 1e00: c0597490 c0596fb4 c1546ab4 c1004ec8 c10da898 c059741c c20d7ab8 c0c47d54
>>> [   14.192790] 1e20: f0821e5c f0821e30 c0594c2c c0597428 f0821e68 c14f7858 c1546ab4 b4aeb193
>>> [   14.219273] 1e40: c10da898 c10cf808 c20d7a80 00000000 f0821e6c f0821e60 c0596528 c0594bb4
>>> [   14.245875] 1e60: f0821e9c f0821e70 c0595e60 c0596508 c0b7b7c8 00000000 f0821e9c c10da898
>>> [   14.272550] 1e80: c1004ec8 c0c28e84 00000000 c1456854 f0821eb4 f0821ea0 c0597c80 c0595cc0
>>> [   14.299270] 1ea0: c110cec0 c1004ec8 f0821ec4 f0821eb8 c0599130 c0597c08 f0821ed4 f0821ec8
>>> [   14.326125] 1ec0: c0c28ea8 c0599110 f0821f4c f0821ed8 c0102180 c0c28e90 c03318e4 c0b14f30
>>> [   14.352912] 1ee0: c0b14f10 c0b14f00 c0b2bc78 c1004ec8 00000000 c0b855d0 00000006 00000006
>>> [   14.379805] 1f00: 00000000 c0b76eb0 c0c0055c c0bebf78 c0953a9c c154665b 00000000 b4aeb193
>>> [   14.406665] 1f20: c0181cb8 b4aeb193 000000c6 c0c82160 00000007 c0c47d34 c1546600 000000c6
>>> [   14.433661] 1f40: f0821f94 f0821f50 c0c01a48 c0102134 00000006 00000006 00000000 c0c0055c
>>> [   14.460621] 1f60: c0c0055c c0bebf78 c1588000 00004ec0 c096ea48 00000000 00000000 00000000
>>> [   14.487681] 1f80: 00000000 00000000 f0821fac f0821f98 c096ea6c c0c01884 00000000 c096ea48
>>> [   14.514695] 1fa0: 00000000 f0821fb0 c0100148 c096ea54 00000000 00000000 00000000 00000000
>>> [   14.541771] 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
>>> [   14.568615] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000
>>> [   14.595194] Backtrace:
>>> [   14.615207]  cpufreq_online from cpufreq_add_dev+0x78/0x84
>>> [   14.638819]  r10:c1004f14 r9:c1005058 r8:c10da884 r7:c20d7b40 r6:c1004ec8 r5:ef7c5050
>>> [   14.665198]  r4:00000000
>>> [   14.685514]  cpufreq_add_dev from subsys_interface_register+0xb0/0xfc
>>> [   14.710558]  r5:c10cf8a8 r4:c10da570
>>> [   14.732076]  subsys_interface_register from cpufreq_register_driver+0x150/0x2d8
>>> [   14.758094]  r6:c10da434 r5:c115ef18 r4:c10da900
>>> [   14.780943]  cpufreq_register_driver from dt_cpufreq_probe+0x324/0x49c
>>> [   14.806283]  r6:ef7c5050 r5:c10da8ec r4:00000002
>>> [   14.829299]  dt_cpufreq_probe from platform_probe+0x6c/0xc8
>>> [   14.853499]  r10:c111b000 r9:c0c47d54 r8:c20d7ab8 r7:00000000 r6:c10da898 r5:00000000
>>> [   14.880388]  r4:c14e3410
>>> [   14.901252]  platform_probe from really_probe+0xd8/0x2bc
>>> [   14.925473]  r7:00000000 r6:c10da898 r5:00000000 r4:c14e3410
>>> [   14.949898]  really_probe from __driver_probe_device+0xd8/0xf4
>>> [   14.974546]  r7:00000000 r6:c14e3410 r5:c10da898 r4:c14e3410
>>> [   14.998855]  __driver_probe_device from driver_probe_device+0x40/0xd0
>>> [   15.024426]  r5:c14e3410 r4:c115dd68
>>> [   15.046857]  driver_probe_device from __driver_attach+0x74/0x120
>>> [   15.072380]  r7:c059741c r6:c10da898 r5:c14e3410 r4:00000000
>>> [   15.097388]  __driver_attach from bus_for_each_dev+0x84/0xc4
>>> [   15.122444]  r9:c0c47d54 r8:c20d7ab8 r7:c059741c r6:c10da898 r5:c1004ec8 r4:c1546ab4
>>> [   15.150131]  bus_for_each_dev from driver_attach+0x2c/0x30
>>> [   15.175532]  r7:00000000 r6:c20d7a80 r5:c10cf808 r4:c10da898
>>> [   15.201134]  driver_attach from bus_add_driver+0x1ac/0x1f8
>>> [   15.226772]  bus_add_driver from driver_register+0x84/0x11c
>>> [   15.252559]  r8:c1456854 r7:00000000 r6:c0c28e84 r5:c1004ec8 r4:c10da898
>>> [   15.279483]  driver_register from __platform_driver_register+0x2c/0x34
>>> [   15.306523]  r5:c1004ec8 r4:c110cec0
>>> [   15.329801]  __platform_driver_register from dt_cpufreq_platdrv_init+0x24/0x28
>>> [   15.357500]  dt_cpufreq_platdrv_init from do_one_initcall+0x58/0x20c
>>> [   15.384159]  do_one_initcall from kernel_init_freeable+0x1d0/0x22c
>>> [   15.410635]  r8:000000c6 r7:c1546600 r6:c0c47d34 r5:00000007 r4:c0c82160
>>> [   15.437567]  kernel_init_freeable from kernel_init+0x24/0x144
>>> [   15.463573]  r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c096ea48
>>> [   15.491869]  r4:00004ec0
>>> [   15.514022]  kernel_init from ret_from_fork+0x14/0x2c
>>> [   15.539079] Exception stack(0xf0821fb0 to 0xf0821ff8)
>>> [   15.563943] 1fa0:                                     00000000 00000000 00000000 00000000
>>> [   15.592459] 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
>>> [   15.620836] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
>>> [   15.647222]  r5:c096ea48 r4:00000000
>>> [   15.670196] Code: e1a00004 ebfff74d e3500000 0a00001a (e7f001f2)
>>> [   15.696086] ---[ end trace 0000000000000000 ]---
>>> [   15.720522] Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
>>> [   15.748404] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b ]---
>>>
>>>
>>> On 4/6/21 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;
>>>
>>> This psv variable can become a 0 divider.
>>>
>>>> +
>>>> +             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:

[PATCH v2 2/2] arm: zynq: don't disable CONFIG_ARM_GLOBAL_TIMER due to CONFIG_CPU_FREQ anymore

[Andrea Merello]

Now ARM global timer driver could work even if it's source clock rate
changes, so we don't need to disable that driver when cpu frequency scaling
is in use.

This cause Zynq arch to get support for timer delay and get_cycles().

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>
---
 arch/arm/mach-zynq/Kconfig  | 2 +-
 drivers/clocksource/Kconfig | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-zynq/Kconfig b/arch/arm/mach-zynq/Kconfig
index 43fb941dcd07..a56748d671c4 100644
--- a/arch/arm/mach-zynq/Kconfig
+++ b/arch/arm/mach-zynq/Kconfig
@@ -6,7 +6,7 @@ config ARCH_ZYNQ
 	select ARCH_SUPPORTS_BIG_ENDIAN
 	select ARM_AMBA
 	select ARM_GIC
-	select ARM_GLOBAL_TIMER if !CPU_FREQ
+	select ARM_GLOBAL_TIMER
 	select CADENCE_TTC_TIMER
 	select HAVE_ARM_SCU if SMP
 	select HAVE_ARM_TWD if SMP
diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 19fc5f8883e0..9fa28237715a 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -360,6 +360,7 @@ config ARM_GLOBAL_TIMER
 
 config ARM_GT_INITIAL_PRESCALER_VAL
 	int "ARM global timer initial prescaler value"
+	default 2 if ARCH_ZYNQ
 	default 1
 	depends on ARM_GLOBAL_TIMER
 	help
-- 
2.17.1

[tip: timers/core] arm: zynq: don't disable CONFIG_ARM_GLOBAL_TIMER due to CONFIG_CPU_FREQ anymore

[tip-bot2 for Andrea Merello]

The following commit has been merged into the timers/core branch of tip:

Commit-ID:     68e2215e9d5f5ec8e5ba0158683742932519cad9
Gitweb:        https://git.kernel.org/tip/68e2215e9d5f5ec8e5ba0158683742932519cad9
Author:        Andrea Merello <andrea.merello@gmail.com>
AuthorDate:    Tue, 06 Apr 2021 15:00:45 +02:00
Committer:     Daniel Lezcano <daniel.lezcano@linaro.org>
CommitterDate: Wed, 16 Jun 2021 17:33:04 +02:00

arm: zynq: don't disable CONFIG_ARM_GLOBAL_TIMER due to CONFIG_CPU_FREQ anymore

Now ARM global timer driver could work even if it's source clock rate
changes, so we don't need to disable that driver when cpu frequency scaling
is in use.

This cause Zynq arch to get support for timer delay and get_cycles().

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>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Link: https://lore.kernel.org/r/20210406130045.15491-3-andrea.merello@gmail.com
---
 arch/arm/mach-zynq/Kconfig  | 2 +-
 drivers/clocksource/Kconfig | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-zynq/Kconfig b/arch/arm/mach-zynq/Kconfig
index 43fb941..a56748d 100644
--- a/arch/arm/mach-zynq/Kconfig
+++ b/arch/arm/mach-zynq/Kconfig
@@ -6,7 +6,7 @@ config ARCH_ZYNQ
 	select ARCH_SUPPORTS_BIG_ENDIAN
 	select ARM_AMBA
 	select ARM_GIC
-	select ARM_GLOBAL_TIMER if !CPU_FREQ
+	select ARM_GLOBAL_TIMER
 	select CADENCE_TTC_TIMER
 	select HAVE_ARM_SCU if SMP
 	select HAVE_ARM_TWD if SMP
diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 19fc5f8..9fa2823 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -360,6 +360,7 @@ config ARM_GLOBAL_TIMER
 
 config ARM_GT_INITIAL_PRESCALER_VAL
 	int "ARM global timer initial prescaler value"
+	default 2 if ARCH_ZYNQ
 	default 1
 	depends on ARM_GLOBAL_TIMER
 	help

Re: [PATCH v2 0/2] Fix missing entropy on Zynq arch due to get_cycles() not supported

[Michal Simek]

Hi,

On 4/6/21 3:00 PM, Andrea Merello wrote:
> Changes wrt v1:
>  - rebased on latest kernel tree
>  - fix cover letter: does -> doesn't
> 
> A real-world problem has been seen with a Zynq-based board running
> Debian 10, where ssh daemon takes a really long time to come up at boot.
> This is due to lack of random numbers.
> 
> Since commit 50ee7529ec450 ("random: try to actively add entropy rather
> than passively wait for it") we try to generate entropy whenever we are
> in short of random numbers and someone needs them.
> 
> This trick works only when CPU cycle counter is available. On ARM this
> means that get_cycles() works and in turn read_current_timer() works.
> 
> Zynq HW includes two "cadence TTC" timers and the "ARM global timer".
> All these pieces of HW are fed by the CPU clock, which dynamically
> changes whenever CPU frequency scaling is enabled.
> 
> In timer-cadence-ttc driver this scenario is handled by looking at parent
> clock changes and adjusting things when required. This is the only usable
> clocksource when CPU frequency scaling is in use. arm_global_timer driver
> is disabled in Kconfig when CPU_FREQ is enabled for Zynq arch.
> 
> Unfortunately timer-cadence-ttc driver doesn't register itself via
> register_current_timer_delay() and that ultimately ends up in get_cycles()
> to always return zero, causing the aforementioned lack of entropy problem.
> I believe that the reason for this is because Cadence TTC counter on Zynq
> silicon is only 16-bit wide.
> 
> This patchset works around this by implementing in ARM global timer driver
> a mechanism to compensate for parent clock variations, similarly to what
> it's done in Cadence TTC timer driver, so that it can be used together
> with CPU frequency scaling on Zynq arch.
> 
> This proved to finally fix the problem on my Zynq-based Z-turn board.
> 
> 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>
> 
> Andrea Merello (2):
>   clocksource: arm_global_timer: implement rate compensation whenever
>     source clock changes
>   arm: zynq: don't disable CONFIG_ARM_GLOBAL_TIMER due to
>     CONFIG_CPU_FREQ anymore
> 
>  arch/arm/mach-zynq/Kconfig             |   2 +-
>  drivers/clocksource/Kconfig            |  14 +++
>  drivers/clocksource/arm_global_timer.c | 122 +++++++++++++++++++++++--
>  3 files changed, 127 insertions(+), 11 deletions(-)
> 
> --
> 2.17.1
> 

I have no problem with these patches. I have tested it on zc706 board
and I see performance improvement when I generate random numbers.

Only question what I have is if ARM_GT_INITIAL_PRESCALER_VAL=2 make
sense also for others SoC with multi_v7_defconfig.

Other than this feel free to add my
Acked-by: Michal Simek <michal.simek@xilinx.com>
Tested-by: Michal Simek <michal.simek@xilinx.com> (on Zynq)

Daniel: Can you please take a look at this series?

Thanks,
Michal