lkml.org 
[lkml]   [2022]   [Jun]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v2 07/12] clocksource: mips-gic-timer: Always use cluster 0 counter as clocksource
From
On 25-May-22 14:10, Dragan Mladjenovic wrote:
> From: Paul Burton <paulburton@kernel.org>
>
> In a multi-cluster MIPS system we have multiple GICs - one in each
> cluster - each of which has its own independent counter. The counters in
> each GIC are not synchronised in any way, so they can drift relative to
> one another through the lifetime of the system. This is problematic for
> a clocksource which ought to be global.
>
> Avoid problems by always accessing cluster 0's counter, using
> cross-cluster register access. This adds overhead so we only do so on
> systems where we actually have CPUs present in multiple clusters.
> For now, be extra conservative and don't use gic counter for vdso or
> sched_clock in this case.
>
> Signed-off-by: Paul Burton <paulburton@kernel.org>
> Signed-off-by: Chao-ying Fu <cfu@wavecomp.com>
> Signed-off-by: Dragan Mladjenovic <dragan.mladjenovic@syrmia.com>
>
> diff --git a/drivers/clocksource/mips-gic-timer.c b/drivers/clocksource/mips-gic-timer.c
> index be4175f415ba..6632d314a2c0 100644
> --- a/drivers/clocksource/mips-gic-timer.c
> +++ b/drivers/clocksource/mips-gic-timer.c
> @@ -170,6 +170,37 @@ static u64 gic_hpt_read(struct clocksource *cs)
> return gic_read_count();
> }
>
> +static u64 gic_hpt_read_multicluster(struct clocksource *cs)
> +{
> + unsigned int hi, hi2, lo;
> + u64 count;
> +
> + mips_cm_lock_other(0, 0, 0, CM_GCR_Cx_OTHER_BLOCK_GLOBAL);
> +
> + if (mips_cm_is64) {
> + count = read_gic_redir_counter();
> + goto out;
> + }
> +
> + hi = read_gic_redir_counter_32h();
> + while (true) {
> + lo = read_gic_redir_counter_32l();
> +
> + /* If hi didn't change then lo didn't wrap & we're done */
> + hi2 = read_gic_redir_counter_32h();
> + if (hi2 == hi)
> + break;
> +
> + /* Otherwise, repeat with the latest hi value */
> + hi = hi2;
> + }
> +
> + count = (((u64)hi) << 32) + lo;
> +out:
> + mips_cm_unlock_other();
> + return count;
> +}
> +
> static struct clocksource gic_clocksource = {
> .name = "GIC",
> .read = gic_hpt_read,
> @@ -204,6 +235,11 @@ static int __init __gic_clocksource_init(void)
> /* Calculate a somewhat reasonable rating value. */
> gic_clocksource.rating = 200 + gic_frequency / 10000000;
>
> + if (mips_cps_multicluster_cpus()) {
> + gic_clocksource.read = &gic_hpt_read_multicluster;
> + gic_clocksource.vdso_clock_mode = VDSO_CLOCKMODE_NONE;
> + }
> +
> ret = clocksource_register_hz(&gic_clocksource, gic_frequency);
> if (ret < 0)
> pr_warn("Unable to register clocksource\n");
> @@ -262,7 +298,8 @@ static int __init gic_clocksource_of_init(struct device_node *node)
> * stable CPU frequency or on the platforms with CM3 and CPU frequency
> * change performed by the CPC core clocks divider.
> */
> - if (mips_cm_revision() >= CM_REV_CM3 || !IS_ENABLED(CONFIG_CPU_FREQ)) {
> + if ((mips_cm_revision() >= CM_REV_CM3 || !IS_ENABLED(CONFIG_CPU_FREQ)) &&
> + !mips_cps_multicluster_cpus()) {
> sched_clock_register(mips_cm_is64 ?
> gic_read_count_64 : gic_read_count_2x32,
> 64, gic_frequency);

Hi,

I was expecting some comments on this, but I'll ask first. We now taking
a conservative approach of not using gic as sched_clock in multicluster
case. Is this necessary or can sched_clock tolerate a fixed delta
between clocks on different cpu clusters?

Best regards,

Dragan

\
 
 \ /
  Last update: 2022-06-27 16:18    [W:0.195 / U:0.212 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site