[PATCH v4 07/14] clocksource: mips-gic-timer: Always use cluster 0 counter as clocksource

Aleksandar Rikalo posted 14 patches 1 year, 7 months ago
There is a newer version of this series
[PATCH v4 07/14] clocksource: mips-gic-timer: Always use cluster 0 counter as clocksource
Posted by Aleksandar Rikalo 1 year, 7 months ago
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>
Signed-off-by: Aleksandar Rikalo <aleksandar.rikalo@syrmia.com>
---
 drivers/clocksource/mips-gic-timer.c | 39 +++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/clocksource/mips-gic-timer.c b/drivers/clocksource/mips-gic-timer.c
index b3ae38f36720..ebf308916fb1 100644
--- a/drivers/clocksource/mips-gic-timer.c
+++ b/drivers/clocksource/mips-gic-timer.c
@@ -165,6 +165,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,
@@ -199,6 +230,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");
@@ -257,7 +293,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);
-- 
2.25.1
Re: [PATCH v4 07/14] clocksource: mips-gic-timer: Always use cluster 0 counter as clocksource
Posted by Daniel Lezcano 1 year, 5 months ago
On 11/05/2024 12:43, Aleksandar Rikalo 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>
> Signed-off-by: Aleksandar Rikalo <aleksandar.rikalo@syrmia.com>
> ---

Applied patch 7 and 8

Thanks

-- 
<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 v4 07/14] clocksource: mips-gic-timer: Always use cluster 0 counter as clocksource
Posted by Jiaxun Yang 1 year, 5 months ago

在2024年7月9日七月 上午12:36,Daniel Lezcano写道:
> On 11/05/2024 12:43, Aleksandar Rikalo 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>
>> Signed-off-by: Aleksandar Rikalo <aleksandar.rikalo@syrmia.com>
>> ---
>
> Applied patch 7 and 8

I think it won't compile without patch 1 being applid.

Thomas, do you mind to apply patch 1 for now? Given that it's just some extra
function definitions.

Thanks
- Jiaxun

>
> Thanks
>
> -- 
> <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

-- 
- Jiaxun
Re: [PATCH v4 07/14] clocksource: mips-gic-timer: Always use cluster 0 counter as clocksource
Posted by Thomas Bogendoerfer 1 year, 5 months ago
On Tue, Jul 09, 2024 at 09:47:52AM +0800, Jiaxun Yang wrote:
> 
> 
> 在2024年7月9日七月 上午12:36,Daniel Lezcano写道:
> > On 11/05/2024 12:43, Aleksandar Rikalo 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>
> >> Signed-off-by: Aleksandar Rikalo <aleksandar.rikalo@syrmia.com>
> >> ---
> >
> > Applied patch 7 and 8
> 
> I think it won't compile without patch 1 being applid.
> 
> Thomas, do you mind to apply patch 1 for now? Given that it's just some extra
> function definitions.

no problem, I've applied patch 1 und 2 to mips-next.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]
Re: [PATCH v4 07/14] clocksource: mips-gic-timer: Always use cluster 0 counter as clocksource
Posted by Daniel Lezcano 1 year, 5 months ago
On 09/07/2024 10:53, Thomas Bogendoerfer wrote:
> On Tue, Jul 09, 2024 at 09:47:52AM +0800, Jiaxun Yang wrote:
>>
>>
>> 在2024年7月9日七月 上午12:36,Daniel Lezcano写道:
>>> On 11/05/2024 12:43, Aleksandar Rikalo 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>
>>>> Signed-off-by: Aleksandar Rikalo <aleksandar.rikalo@syrmia.com>
>>>> ---
>>>
>>> Applied patch 7 and 8
>>
>> I think it won't compile without patch 1 being applid.
>>
>> Thomas, do you mind to apply patch 1 for now? Given that it's just some extra
>> function definitions.
> 
> no problem, I've applied patch 1 und 2 to mips-next.

Usually we create a shared immutable branch, but as we are close to the 
PR, I propose I ack these two patches and let them go through the mips 
tree this time

-- 
<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 v4 07/14] clocksource: mips-gic-timer: Always use cluster 0 counter as clocksource
Posted by Thomas Bogendoerfer 1 year, 5 months ago
On Tue, Jul 09, 2024 at 04:44:30PM +0200, Daniel Lezcano wrote:
> On 09/07/2024 10:53, Thomas Bogendoerfer wrote:
> > On Tue, Jul 09, 2024 at 09:47:52AM +0800, Jiaxun Yang wrote:
> > > 
> > > 
> > > 在2024年7月9日七月 上午12:36,Daniel Lezcano写道:
> > > > On 11/05/2024 12:43, Aleksandar Rikalo 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>
> > > > > Signed-off-by: Aleksandar Rikalo <aleksandar.rikalo@syrmia.com>
> > > > > ---
> > > > 
> > > > Applied patch 7 and 8
> > > 
> > > I think it won't compile without patch 1 being applid.
> > > 
> > > Thomas, do you mind to apply patch 1 for now? Given that it's just some extra
> > > function definitions.
> > 
> > no problem, I've applied patch 1 und 2 to mips-next.
> 
> Usually we create a shared immutable branch, but as we are close to the PR,
> I propose I ack these two patches and let them go through the mips tree this
> time

works for me, as well.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]