On 07/03/2022 09:32, Vincent Whitchurch wrote:
> The ARTPEC-8 SoC has a quad-core Cortex-A53 and a single-core Cortex-A5
> which share one MCT with one global and eight local timers.
>
> The Cortex-A53 boots first and starts the global FRC and also registers
> a clock events device using the global timer. (This global timer clock
> events is usually replaced by arch timer clock events for each of the
> cores.)
>
> When the A5 boots, we should not use the global timer interrupts or
> write to the global timer registers. This is because even if there are
> four global comparators, the control bits for all four are in the same
> registers, and we would need to synchronize between the cpus. Instead,
> the global timer FRC (already started by the A53) should be used as the
> clock source, and one of the local timers which are not used by the A53
> can be used for clock events on the A5.
>
> To support this, add a module param to set the local timer starting
> index. If this parameter is non-zero, the global timer clock events
> device is not registered and we don't write to the global FRC if it is
> already started.
>
> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@xxxxxxxx>
> ---
> drivers/clocksource/exynos_mct.c | 29 ++++++++++++++++++++++++-----
> 1 file changed, 24 insertions(+), 5 deletions(-)
This should not be send separately from the previous patch.
>
> diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
> index f29c812b70c9..7ea2919b1808 100644
> --- a/drivers/clocksource/exynos_mct.c
> +++ b/drivers/clocksource/exynos_mct.c
> @@ -33,7 +33,7 @@
> #define EXYNOS4_MCT_G_INT_ENB EXYNOS4_MCTREG(0x248)
> #define EXYNOS4_MCT_G_WSTAT EXYNOS4_MCTREG(0x24C)
> #define _EXYNOS4_MCT_L_BASE EXYNOS4_MCTREG(0x300)
> -#define EXYNOS4_MCT_L_BASE(x) (_EXYNOS4_MCT_L_BASE + (0x100 * x))
> +#define EXYNOS4_MCT_L_BASE(x) (_EXYNOS4_MCT_L_BASE + (0x100 * (x)))
> #define EXYNOS4_MCT_L_MASK (0xffffff00)
>
> #define MCT_L_TCNTB_OFFSET (0x00)
> @@ -77,6 +77,13 @@ static unsigned long clk_rate;
> static unsigned int mct_int_type;
> static int mct_irqs[MCT_NR_IRQS];
>
> +/*
> + * First local timer index to use. If non-zero, global
> + * timer is not written to.
> + */
> +static unsigned int mct_local_idx;
> +module_param_named(local_idx, mct_local_idx, int, 0);
No, it's a no go. Please use dedicated compatible if you need specific
quirks.
> +
> struct mct_clock_event_device {
> struct clock_event_device evt;
> unsigned long base;
> @@ -157,6 +164,17 @@ static void exynos4_mct_frc_start(void)
> u32 reg;
>
> reg = readl_relaxed(reg_base + EXYNOS4_MCT_G_TCON);
> +
> + /*
> + * If the FRC is already running, we don't need to start it again. We
> + * could probably just do this on all systems, but, to avoid any risk
> + * for regressions, we only do it on systems where it's absolutely
> + * necessary (i.e., on systems where writes to the global registers
> + * need to be avoided).
> + */
> + if (mct_local_idx && (reg & MCT_G_TCON_START))
> + return;
I don't get it. exynos4_mct_frc_start() is called exactly only once in
the system - during init. Not once per every CPU or cluster (I
understood you have two clusters, right?).
Best regards,
Krzysztof
