Messages in this thread | | | Date | Mon, 27 Jun 2022 16:17:43 +0200 | Subject | Re: [PATCH v2 07/12] clocksource: mips-gic-timer: Always use cluster 0 counter as clocksource | From | Dragan Mladjenovic <> |
| |
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
| |