[PATCH] arm: dts: exynos5: Remove multi core timer

[PATCH] arm: dts: exynos5: Remove multi core timer

[Chirantan Ekbote]

The multi core timer and the ARM architected timer are two different
interfaces to the same underlying hardware timer.  This causes some
strange timing issues when they are both enabled at the same time so
remove the mct from the device tree and keep only the architected
timer.

Cc: Olof Johansson <olof@lixom.net>
Cc: Doug Anderson <dianders@chromium.org>
Cc: Kukjin Kim <kgene.kim@samsung.com>
Cc: linux-arm-kernel at lists.infradead.org
Cc: linux-samsung-soc at vger.kernel.org

Signed-off-by: Chirantan Ekbote <chirantan@chromium.org>
---
 arch/arm/boot/dts/exynos5250.dtsi | 24 ------------------------
 arch/arm/boot/dts/exynos5420.dtsi | 30 ------------------------------
 2 files changed, 54 deletions(-)

diff --git a/arch/arm/boot/dts/exynos5250.dtsi b/arch/arm/boot/dts/exynos5250.dtsi
index 3742331..60cd945 100644
--- a/arch/arm/boot/dts/exynos5250.dtsi
+++ b/arch/arm/boot/dts/exynos5250.dtsi
@@ -110,30 +110,6 @@
 		clock-frequency = <24000000>;
 	};
 
-	mct at 101C0000 {
-		compatible = "samsung,exynos4210-mct";
-		reg = <0x101C0000 0x800>;
-		interrupt-controller;
-		#interrups-cells = <2>;
-		interrupt-parent = <&mct_map>;
-		interrupts = <0 0>, <1 0>, <2 0>, <3 0>,
-			     <4 0>, <5 0>;
-		clocks = <&clock CLK_FIN_PLL>, <&clock CLK_MCT>;
-		clock-names = "fin_pll", "mct";
-
-		mct_map: mct-map {
-			#interrupt-cells = <2>;
-			#address-cells = <0>;
-			#size-cells = <0>;
-			interrupt-map = <0x0 0 &combiner 23 3>,
-					<0x1 0 &combiner 23 4>,
-					<0x2 0 &combiner 25 2>,
-					<0x3 0 &combiner 25 3>,
-					<0x4 0 &gic 0 120 0>,
-					<0x5 0 &gic 0 121 0>;
-		};
-	};
-
 	pmu {
 		compatible = "arm,cortex-a15-pmu";
 		interrupt-parent = <&combiner>;
diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
index c3a9a66..3c38c6d 100644
--- a/arch/arm/boot/dts/exynos5420.dtsi
+++ b/arch/arm/boot/dts/exynos5420.dtsi
@@ -169,36 +169,6 @@
 		status = "disabled";
 	};
 
-	mct at 101C0000 {
-		compatible = "samsung,exynos4210-mct";
-		reg = <0x101C0000 0x800>;
-		interrupt-controller;
-		#interrups-cells = <1>;
-		interrupt-parent = <&mct_map>;
-		interrupts = <0>, <1>, <2>, <3>, <4>, <5>, <6>, <7>,
-				<8>, <9>, <10>, <11>;
-		clocks = <&clock CLK_FIN_PLL>, <&clock CLK_MCT>;
-		clock-names = "fin_pll", "mct";
-
-		mct_map: mct-map {
-			#interrupt-cells = <1>;
-			#address-cells = <0>;
-			#size-cells = <0>;
-			interrupt-map = <0 &combiner 23 3>,
-					<1 &combiner 23 4>,
-					<2 &combiner 25 2>,
-					<3 &combiner 25 3>,
-					<4 &gic 0 120 0>,
-					<5 &gic 0 121 0>,
-					<6 &gic 0 122 0>,
-					<7 &gic 0 123 0>,
-					<8 &gic 0 128 0>,
-					<9 &gic 0 129 0>,
-					<10 &gic 0 130 0>,
-					<11 &gic 0 131 0>;
-		};
-	};
-
 	gsc_pd: power-domain at 10044000 {
 		compatible = "samsung,exynos4210-pd";
 		reg = <0x10044000 0x20>;
-- 
1.9.1.423.g4596e3a

[PATCH] arm: dts: exynos5: Remove multi core timer

[Tomasz Figa]

Hi Chirantan,

On 15.05.2014 23:07, Chirantan Ekbote wrote:
> The multi core timer and the ARM architected timer are two different
> interfaces to the same underlying hardware timer.  This causes some
> strange timing issues when they are both enabled at the same time so
> remove the mct from the device tree and keep only the architected
> timer.

Huh? I've always thought MCT is a completely separate hardware block
outside of ARM cores, while architected timers are embedded inside the
CPU block in which the ARM cores reside. Could you elaborate on this?

Best regards,
Tomasz

[PATCH] arm: dts: exynos5: Remove multi core timer

[Doug Anderson]

Tomasz,

On Thu, May 15, 2014 at 2:14 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> Hi Chirantan,
>
> On 15.05.2014 23:07, Chirantan Ekbote wrote:
>> The multi core timer and the ARM architected timer are two different
>> interfaces to the same underlying hardware timer.  This causes some
>> strange timing issues when they are both enabled at the same time so
>> remove the mct from the device tree and keep only the architected
>> timer.
>
> Huh? I've always thought MCT is a completely separate hardware block
> outside of ARM cores, while architected timers are embedded inside the
> CPU block in which the ARM cores reside. Could you elaborate on this?

Yup.  Our thoughts exactly.

...but it appears not to be the case.  Chirantan demonstrated this in
U-Boot just to prove that it's not some strange kernel interaction in
<https://chromium-review.googlesource.com/200035>.  I took his patch
and tweaked it a little more myself in
<https://chromium-review.googlesource.com/200098>.

Specifically:
* If you stop the MCT, the arch timer stops
* If you reset the MCT, the arch timer resets
* If you start the MCT again, the arch timer starts again
* If you read the MCT and the arch timer, they give the same value.


This is apparently the answer to my question at
<http://www.spinics.net/lists/linux-samsung-soc/msg29085.html>.
Specifically Chirantan found that the big jump in time happened when
MCT reset to 0.  That made the arch timer code think that there was a
wraparound and jump forward in time a lot.


Please confirm if you have a system that has MCT and arch timer in front of you.

-Doug

[PATCH] arm: dts: exynos5: Remove multi core timer

[Tomasz Figa]

On 15.05.2014 23:33, Doug Anderson wrote:
> Tomasz,
> 
> On Thu, May 15, 2014 at 2:14 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>> Hi Chirantan,
>>
>> On 15.05.2014 23:07, Chirantan Ekbote wrote:
>>> The multi core timer and the ARM architected timer are two different
>>> interfaces to the same underlying hardware timer.  This causes some
>>> strange timing issues when they are both enabled at the same time so
>>> remove the mct from the device tree and keep only the architected
>>> timer.
>>
>> Huh? I've always thought MCT is a completely separate hardware block
>> outside of ARM cores, while architected timers are embedded inside the
>> CPU block in which the ARM cores reside. Could you elaborate on this?
> 
> Yup.  Our thoughts exactly.
> 
> ...but it appears not to be the case.  Chirantan demonstrated this in
> U-Boot just to prove that it's not some strange kernel interaction in
> <https://chromium-review.googlesource.com/200035>.  I took his patch
> and tweaked it a little more myself in
> <https://chromium-review.googlesource.com/200098>.
> 
> Specifically:
> * If you stop the MCT, the arch timer stops
> * If you reset the MCT, the arch timer resets
> * If you start the MCT again, the arch timer starts again
> * If you read the MCT and the arch timer, they give the same value.
> 
> 
> This is apparently the answer to my question at
> <http://www.spinics.net/lists/linux-samsung-soc/msg29085.html>.
> Specifically Chirantan found that the big jump in time happened when
> MCT reset to 0.  That made the arch timer code think that there was a
> wraparound and jump forward in time a lot.
> 
> 
> Please confirm if you have a system that has MCT and arch timer in front of you.

Wow, this is so strange that I just can't believe it, but if you proved
it using such detailed test then I don't have any reasons to object anymore.

One more thing, though, is whether the architected timer "interface" can
wake the system from lighter power states, such as AFTR or LPA. Could
you check this?

Best regards,
Tomasz

[PATCH] arm: dts: exynos5: Remove multi core timer

[Doug Anderson]

Tomasz,



On Thu, May 15, 2014 at 2:40 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> On 15.05.2014 23:33, Doug Anderson wrote:
>> Tomasz,
>>
>> On Thu, May 15, 2014 at 2:14 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>>> Hi Chirantan,
>>>
>>> On 15.05.2014 23:07, Chirantan Ekbote wrote:
>>>> The multi core timer and the ARM architected timer are two different
>>>> interfaces to the same underlying hardware timer.  This causes some
>>>> strange timing issues when they are both enabled at the same time so
>>>> remove the mct from the device tree and keep only the architected
>>>> timer.
>>>
>>> Huh? I've always thought MCT is a completely separate hardware block
>>> outside of ARM cores, while architected timers are embedded inside the
>>> CPU block in which the ARM cores reside. Could you elaborate on this?
>>
>> Yup.  Our thoughts exactly.
>>
>> ...but it appears not to be the case.  Chirantan demonstrated this in
>> U-Boot just to prove that it's not some strange kernel interaction in
>> <https://chromium-review.googlesource.com/200035>.  I took his patch
>> and tweaked it a little more myself in
>> <https://chromium-review.googlesource.com/200098>.
>>
>> Specifically:
>> * If you stop the MCT, the arch timer stops
>> * If you reset the MCT, the arch timer resets
>> * If you start the MCT again, the arch timer starts again
>> * If you read the MCT and the arch timer, they give the same value.
>>
>>
>> This is apparently the answer to my question at
>> <http://www.spinics.net/lists/linux-samsung-soc/msg29085.html>.
>> Specifically Chirantan found that the big jump in time happened when
>> MCT reset to 0.  That made the arch timer code think that there was a
>> wraparound and jump forward in time a lot.
>>
>>
>> Please confirm if you have a system that has MCT and arch timer in front of you.
>
> Wow, this is so strange that I just can't believe it, but if you proved
> it using such detailed test then I don't have any reasons to object anymore.
>
> One more thing, though, is whether the architected timer "interface" can
> wake the system from lighter power states, such as AFTR or LPA. Could
> you check this?

I've never used AFTR or LPA and so can't really comment on it.
Hopefully someone on the list can?

NOTE: if for some reason we need to keep the MCT around, we're
definitely going to need to account for the fact that tweaking it
affects the arch timer.  ...and having the arch timer is really nice
since:
* it's faster to access.
* it implements the bits needed for udelay() to use it.
* it is accessible from userspace for really fast access.

-Doug

[PATCH] arm: dts: exynos5: Remove multi core timer

[Tomasz Figa]

On 15.05.2014 23:54, Doug Anderson wrote:
> Tomasz,
> 
> 
> 
> On Thu, May 15, 2014 at 2:40 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>> On 15.05.2014 23:33, Doug Anderson wrote:
>>> Tomasz,
>>>
>>> On Thu, May 15, 2014 at 2:14 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>>>> Hi Chirantan,
>>>>
>>>> On 15.05.2014 23:07, Chirantan Ekbote wrote:
>>>>> The multi core timer and the ARM architected timer are two different
>>>>> interfaces to the same underlying hardware timer.  This causes some
>>>>> strange timing issues when they are both enabled at the same time so
>>>>> remove the mct from the device tree and keep only the architected
>>>>> timer.
>>>>
>>>> Huh? I've always thought MCT is a completely separate hardware block
>>>> outside of ARM cores, while architected timers are embedded inside the
>>>> CPU block in which the ARM cores reside. Could you elaborate on this?
>>>
>>> Yup.  Our thoughts exactly.
>>>
>>> ...but it appears not to be the case.  Chirantan demonstrated this in
>>> U-Boot just to prove that it's not some strange kernel interaction in
>>> <https://chromium-review.googlesource.com/200035>.  I took his patch
>>> and tweaked it a little more myself in
>>> <https://chromium-review.googlesource.com/200098>.
>>>
>>> Specifically:
>>> * If you stop the MCT, the arch timer stops
>>> * If you reset the MCT, the arch timer resets
>>> * If you start the MCT again, the arch timer starts again
>>> * If you read the MCT and the arch timer, they give the same value.
>>>
>>>
>>> This is apparently the answer to my question at
>>> <http://www.spinics.net/lists/linux-samsung-soc/msg29085.html>.
>>> Specifically Chirantan found that the big jump in time happened when
>>> MCT reset to 0.  That made the arch timer code think that there was a
>>> wraparound and jump forward in time a lot.
>>>
>>>
>>> Please confirm if you have a system that has MCT and arch timer in front of you.
>>
>> Wow, this is so strange that I just can't believe it, but if you proved
>> it using such detailed test then I don't have any reasons to object anymore.
>>
>> One more thing, though, is whether the architected timer "interface" can
>> wake the system from lighter power states, such as AFTR or LPA. Could
>> you check this?
> 
> I've never used AFTR or LPA and so can't really comment on it.
> Hopefully someone on the list can?
> 
> NOTE: if for some reason we need to keep the MCT around, we're
> definitely going to need to account for the fact that tweaking it
> affects the arch timer.  ...and having the arch timer is really nice
> since:

[Let me reorder the points below to make it easier to comment:]

> * it's faster to access.
> * it is accessible from userspace for really fast access.

Do you have some data on whether it is a significant difference,
especially considering real use cases?

> * it implements the bits needed for udelay() to use it.

Hmm, do you know what bits are those? On Exynos4 MCT is the only option
and it would be nice to let udelay() use it.

Best regards,
Tomasz

[PATCH] arm: dts: exynos5: Remove multi core timer

[Doug Anderson]

Tomasz,

On Thu, May 15, 2014 at 3:13 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>> NOTE: if for some reason we need to keep the MCT around, we're
>> definitely going to need to account for the fact that tweaking it
>> affects the arch timer.  ...and having the arch timer is really nice
>> since:
>
> [Let me reorder the points below to make it easier to comment:]
>
>> * it's faster to access.
>> * it is accessible from userspace for really fast access.
>
> Do you have some data on whether it is a significant difference,
> especially considering real use cases?

I know that Chrome makes _a lot_ of calls to gettimeofday() for
profiling purposes, enough that it showed up on benchmarks.  In fact,
we made a change to the MCT to make accesses faster and there's a
small mention of the benchmarking that was done at:

https://chromium-review.googlesource.com/#/c/32190/

...that change probably should be sent upstream, actually.

I'll let Chirantan comment on how much faster arch timers were.
...and I think David Riley (also CCed now) may be able to comment on
the benefits of userspace timers.


>> * it implements the bits needed for udelay() to use it.
>
> Hmm, do you know what bits are those? On Exynos4 MCT is the only option
> and it would be nice to let udelay() use it.

Look for register_current_timer_delay().  It seems like we could do
this for MCT, but I've never done the investigation because we were
always planning to move to arch timers.  ;)

-Doug

[PATCH] arm: dts: exynos5: Remove multi core timer

[Chirantan Ekbote]

Hi Tomasz,

On Thu, May 15, 2014 at 3:44 PM, Doug Anderson <dianders@chromium.org> wrote:
> Tomasz,
>
> On Thu, May 15, 2014 at 3:13 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>>> NOTE: if for some reason we need to keep the MCT around, we're
>>> definitely going to need to account for the fact that tweaking it
>>> affects the arch timer.  ...and having the arch timer is really nice
>>> since:
>>
>> [Let me reorder the points below to make it easier to comment:]
>>
>>> * it's faster to access.
>>> * it is accessible from userspace for really fast access.
>>
>> Do you have some data on whether it is a significant difference,
>> especially considering real use cases?
>
> I know that Chrome makes _a lot_ of calls to gettimeofday() for
> profiling purposes, enough that it showed up on benchmarks.  In fact,
> we made a change to the MCT to make accesses faster and there's a
> small mention of the benchmarking that was done at:
>
> https://chromium-review.googlesource.com/#/c/32190/
>
> ...that change probably should be sent upstream, actually.
>
> I'll let Chirantan comment on how much faster arch timers were.
> ...and I think David Riley (also CCed now) may be able to comment on
> the benefits of userspace timers.
>

When I profiled gettimeofday() calls, they were about 50 - 60% faster
with the arch timers compared to the mct.

- Chirantan

[PATCH] arm: dts: exynos5: Remove multi core timer

[David Riley]

On Thu, May 15, 2014 at 4:03 PM, Chirantan Ekbote
<chirantan@chromium.org> wrote:
> Hi Tomasz,
>
> On Thu, May 15, 2014 at 3:44 PM, Doug Anderson <dianders@chromium.org> wrote:
>> Tomasz,
>>
>> On Thu, May 15, 2014 at 3:13 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>>>> NOTE: if for some reason we need to keep the MCT around, we're
>>>> definitely going to need to account for the fact that tweaking it
>>>> affects the arch timer.  ...and having the arch timer is really nice
>>>> since:
>>>
>>> [Let me reorder the points below to make it easier to comment:]
>>>
>>>> * it's faster to access.
>>>> * it is accessible from userspace for really fast access.
>>>
>>> Do you have some data on whether it is a significant difference,
>>> especially considering real use cases?
>>
>> I know that Chrome makes _a lot_ of calls to gettimeofday() for
>> profiling purposes, enough that it showed up on benchmarks.  In fact,
>> we made a change to the MCT to make accesses faster and there's a
>> small mention of the benchmarking that was done at:
>>
>> https://chromium-review.googlesource.com/#/c/32190/
>>
>> ...that change probably should be sent upstream, actually.
>>
>> I'll let Chirantan comment on how much faster arch timers were.
>> ...and I think David Riley (also CCed now) may be able to comment on
>> the benefits of userspace timers.
>>
>
> When I profiled gettimeofday() calls, they were about 50 - 60% faster
> with the arch timers compared to the mct.

When I profiled gettimeofday(), the standard systems call version took
about 2.5x longer than through a vDSO interface.

>
> - Chirantan

[PATCH] arm: dts: exynos5: Remove multi core timer

[Tomasz Figa]

On 16.05.2014 01:18, David Riley wrote:
> On Thu, May 15, 2014 at 4:03 PM, Chirantan Ekbote
> <chirantan@chromium.org> wrote:
>> Hi Tomasz,
>>
>> On Thu, May 15, 2014 at 3:44 PM, Doug Anderson <dianders@chromium.org> wrote:
>>> Tomasz,
>>>
>>> On Thu, May 15, 2014 at 3:13 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>>>>> NOTE: if for some reason we need to keep the MCT around, we're
>>>>> definitely going to need to account for the fact that tweaking it
>>>>> affects the arch timer.  ...and having the arch timer is really nice
>>>>> since:
>>>>
>>>> [Let me reorder the points below to make it easier to comment:]
>>>>
>>>>> * it's faster to access.
>>>>> * it is accessible from userspace for really fast access.
>>>>
>>>> Do you have some data on whether it is a significant difference,
>>>> especially considering real use cases?
>>>
>>> I know that Chrome makes _a lot_ of calls to gettimeofday() for
>>> profiling purposes, enough that it showed up on benchmarks.  In fact,
>>> we made a change to the MCT to make accesses faster and there's a
>>> small mention of the benchmarking that was done at:
>>>
>>> https://chromium-review.googlesource.com/#/c/32190/
>>>
>>> ...that change probably should be sent upstream, actually.
>>>
>>> I'll let Chirantan comment on how much faster arch timers were.
>>> ...and I think David Riley (also CCed now) may be able to comment on
>>> the benefits of userspace timers.
>>>
>>
>> When I profiled gettimeofday() calls, they were about 50 - 60% faster
>> with the arch timers compared to the mct.
> 
> When I profiled gettimeofday(), the standard systems call version took
> about 2.5x longer than through a vDSO interface.

Sounds like we should invent a new kind of jokes, starting with "When I
profiled gettimeofday()". ;)

Just kidding.

The raw improvement looks quite good, but what I'm more concerned about
is whether this has any significant effect on real use cases. As Doug
mentioned, Chrome makes a lot of calls to gettimeofday(), but he also
mentioned that this is for profiling purposes. Is performance of
gettimeofday() really that crucial in this case? Are there any other use
cases when this improvement is significant?

Anyway, I'm by no means opposed to switching to arch timers. They
provide a well designed, generic interface and drivers shared by
multiple platforms, which means more code sharing and possibly more eyes
looking at the code, which is always good. However if they don't support
low power states correctly, we can't just remove MCT.

Best regards,
Tomasz

[PATCH] arm: dts: exynos5: Remove multi core timer

[Olof Johansson]

On Thu, May 15, 2014 at 4:25 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> On 16.05.2014 01:18, David Riley wrote:
>> On Thu, May 15, 2014 at 4:03 PM, Chirantan Ekbote
>> <chirantan@chromium.org> wrote:
>>> Hi Tomasz,
>>>
>>> On Thu, May 15, 2014 at 3:44 PM, Doug Anderson <dianders@chromium.org> wrote:
>>>> Tomasz,
>>>>
>>>> On Thu, May 15, 2014 at 3:13 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>>>>>> NOTE: if for some reason we need to keep the MCT around, we're
>>>>>> definitely going to need to account for the fact that tweaking it
>>>>>> affects the arch timer.  ...and having the arch timer is really nice
>>>>>> since:
>>>>>
>>>>> [Let me reorder the points below to make it easier to comment:]
>>>>>
>>>>>> * it's faster to access.
>>>>>> * it is accessible from userspace for really fast access.
>>>>>
>>>>> Do you have some data on whether it is a significant difference,
>>>>> especially considering real use cases?
>>>>
>>>> I know that Chrome makes _a lot_ of calls to gettimeofday() for
>>>> profiling purposes, enough that it showed up on benchmarks.  In fact,
>>>> we made a change to the MCT to make accesses faster and there's a
>>>> small mention of the benchmarking that was done at:
>>>>
>>>> https://chromium-review.googlesource.com/#/c/32190/
>>>>
>>>> ...that change probably should be sent upstream, actually.
>>>>
>>>> I'll let Chirantan comment on how much faster arch timers were.
>>>> ...and I think David Riley (also CCed now) may be able to comment on
>>>> the benefits of userspace timers.
>>>>
>>>
>>> When I profiled gettimeofday() calls, they were about 50 - 60% faster
>>> with the arch timers compared to the mct.
>>
>> When I profiled gettimeofday(), the standard systems call version took
>> about 2.5x longer than through a vDSO interface.
>
> Sounds like we should invent a new kind of jokes, starting with "When I
> profiled gettimeofday()". ;)
>
> Just kidding.
>
> The raw improvement looks quite good, but what I'm more concerned about
> is whether this has any significant effect on real use cases. As Doug
> mentioned, Chrome makes a lot of calls to gettimeofday(), but he also
> mentioned that this is for profiling purposes. Is performance of
> gettimeofday() really that crucial in this case? Are there any other use
> cases when this improvement is significant?

In general, yes. gettimeofday() is normally the prime candidate for
vDSO implementaiton (see Nathan Lynch's patch set in the last couple
of months for enabling this). Speeding up gettimeofday() does have
real benefit for some workloads.

> Anyway, I'm by no means opposed to switching to arch timers. They
> provide a well designed, generic interface and drivers shared by
> multiple platforms, which means more code sharing and possibly more eyes
> looking at the code, which is always good. However if they don't support
> low power states correctly, we can't just remove MCT.

Yeah, this will definitely need to be tested. Do the low power states
work on exynos5 upstream such that they can be tested? The snow
chromebook has a u-boot bug that makes AFTR not work, so it's not a
suitable platform to test on, unfortunately.

And if we need MCT for low power states, we'll need MCT to co-exist
with arch timers. Maybe by checking for the presence of those dt nodes
+ CONFIG_ARM_ARCH_TIMER, or somesuch. But let's discuss that when we
know more.


-Olof

[PATCH] arm: dts: exynos5: Remove multi core timer

[Doug Anderson]

Olof,

On Thu, May 15, 2014 at 4:39 PM, Olof Johansson <olof@lixom.net> wrote:
> On Thu, May 15, 2014 at 4:25 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>> On 16.05.2014 01:18, David Riley wrote:
>>> On Thu, May 15, 2014 at 4:03 PM, Chirantan Ekbote
>>> <chirantan@chromium.org> wrote:
>>>> Hi Tomasz,
>>>>
>>>> On Thu, May 15, 2014 at 3:44 PM, Doug Anderson <dianders@chromium.org> wrote:
>>>>> Tomasz,
>>>>>
>>>>> On Thu, May 15, 2014 at 3:13 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>>>>>>> NOTE: if for some reason we need to keep the MCT around, we're
>>>>>>> definitely going to need to account for the fact that tweaking it
>>>>>>> affects the arch timer.  ...and having the arch timer is really nice
>>>>>>> since:
>>>>>>
>>>>>> [Let me reorder the points below to make it easier to comment:]
>>>>>>
>>>>>>> * it's faster to access.
>>>>>>> * it is accessible from userspace for really fast access.
>>>>>>
>>>>>> Do you have some data on whether it is a significant difference,
>>>>>> especially considering real use cases?
>>>>>
>>>>> I know that Chrome makes _a lot_ of calls to gettimeofday() for
>>>>> profiling purposes, enough that it showed up on benchmarks.  In fact,
>>>>> we made a change to the MCT to make accesses faster and there's a
>>>>> small mention of the benchmarking that was done at:
>>>>>
>>>>> https://chromium-review.googlesource.com/#/c/32190/
>>>>>
>>>>> ...that change probably should be sent upstream, actually.
>>>>>
>>>>> I'll let Chirantan comment on how much faster arch timers were.
>>>>> ...and I think David Riley (also CCed now) may be able to comment on
>>>>> the benefits of userspace timers.
>>>>>
>>>>
>>>> When I profiled gettimeofday() calls, they were about 50 - 60% faster
>>>> with the arch timers compared to the mct.
>>>
>>> When I profiled gettimeofday(), the standard systems call version took
>>> about 2.5x longer than through a vDSO interface.
>>
>> Sounds like we should invent a new kind of jokes, starting with "When I
>> profiled gettimeofday()". ;)
>>
>> Just kidding.
>>
>> The raw improvement looks quite good, but what I'm more concerned about
>> is whether this has any significant effect on real use cases. As Doug
>> mentioned, Chrome makes a lot of calls to gettimeofday(), but he also
>> mentioned that this is for profiling purposes. Is performance of
>> gettimeofday() really that crucial in this case? Are there any other use
>> cases when this improvement is significant?
>
> In general, yes. gettimeofday() is normally the prime candidate for
> vDSO implementaiton (see Nathan Lynch's patch set in the last couple
> of months for enabling this). Speeding up gettimeofday() does have
> real benefit for some workloads.
>
>> Anyway, I'm by no means opposed to switching to arch timers. They
>> provide a well designed, generic interface and drivers shared by
>> multiple platforms, which means more code sharing and possibly more eyes
>> looking at the code, which is always good. However if they don't support
>> low power states correctly, we can't just remove MCT.
>
> Yeah, this will definitely need to be tested. Do the low power states
> work on exynos5 upstream such that they can be tested? The snow
> chromebook has a u-boot bug that makes AFTR not work, so it's not a
> suitable platform to test on, unfortunately.

As long as you don't have read-only firmware 90.0 then the known bug
is fixed.  If you have 90.0 it's pretty easy to pull our your write
protect screw and update your read-only firmware if you're doing
development.

...but of course it's never been tested, so there might be other bugs.
 The known bug that was fixed was found solely by code inspection (a
missing break statement in a switch).

[PATCH] arm: dts: exynos5: Remove multi core timer

[Tomasz Figa]

On 16.05.2014 01:39, Olof Johansson wrote:
> On Thu, May 15, 2014 at 4:25 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>> On 16.05.2014 01:18, David Riley wrote:
>>> On Thu, May 15, 2014 at 4:03 PM, Chirantan Ekbote
>>> <chirantan@chromium.org> wrote:
>>>> Hi Tomasz,
>>>>
>>>> On Thu, May 15, 2014 at 3:44 PM, Doug Anderson <dianders@chromium.org> wrote:
>>>>> Tomasz,
>>>>>
>>>>> On Thu, May 15, 2014 at 3:13 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>>>>>>> NOTE: if for some reason we need to keep the MCT around, we're
>>>>>>> definitely going to need to account for the fact that tweaking it
>>>>>>> affects the arch timer.  ...and having the arch timer is really nice
>>>>>>> since:
>>>>>>
>>>>>> [Let me reorder the points below to make it easier to comment:]
>>>>>>
>>>>>>> * it's faster to access.
>>>>>>> * it is accessible from userspace for really fast access.
>>>>>>
>>>>>> Do you have some data on whether it is a significant difference,
>>>>>> especially considering real use cases?
>>>>>
>>>>> I know that Chrome makes _a lot_ of calls to gettimeofday() for
>>>>> profiling purposes, enough that it showed up on benchmarks.  In fact,
>>>>> we made a change to the MCT to make accesses faster and there's a
>>>>> small mention of the benchmarking that was done at:
>>>>>
>>>>> https://chromium-review.googlesource.com/#/c/32190/
>>>>>
>>>>> ...that change probably should be sent upstream, actually.
>>>>>
>>>>> I'll let Chirantan comment on how much faster arch timers were.
>>>>> ...and I think David Riley (also CCed now) may be able to comment on
>>>>> the benefits of userspace timers.
>>>>>
>>>>
>>>> When I profiled gettimeofday() calls, they were about 50 - 60% faster
>>>> with the arch timers compared to the mct.
>>>
>>> When I profiled gettimeofday(), the standard systems call version took
>>> about 2.5x longer than through a vDSO interface.
>>
>> Sounds like we should invent a new kind of jokes, starting with "When I
>> profiled gettimeofday()". ;)
>>
>> Just kidding.
>>
>> The raw improvement looks quite good, but what I'm more concerned about
>> is whether this has any significant effect on real use cases. As Doug
>> mentioned, Chrome makes a lot of calls to gettimeofday(), but he also
>> mentioned that this is for profiling purposes. Is performance of
>> gettimeofday() really that crucial in this case? Are there any other use
>> cases when this improvement is significant?
> 
> In general, yes. gettimeofday() is normally the prime candidate for
> vDSO implementaiton (see Nathan Lynch's patch set in the last couple
> of months for enabling this). Speeding up gettimeofday() does have
> real benefit for some workloads.

I'm just interested out of curiosity in an example of such workload, so
I could play with this a bit, also on Exynos4, which can use just MCT.

> 
>> Anyway, I'm by no means opposed to switching to arch timers. They
>> provide a well designed, generic interface and drivers shared by
>> multiple platforms, which means more code sharing and possibly more eyes
>> looking at the code, which is always good. However if they don't support
>> low power states correctly, we can't just remove MCT.
> 
> Yeah, this will definitely need to be tested. Do the low power states
> work on exynos5 upstream such that they can be tested? The snow
> chromebook has a u-boot bug that makes AFTR not work, so it's not a
> suitable platform to test on, unfortunately.

I think Exynos5250-based Arndale is supposed to have working AFTR state
in mainline, although it might depend on used bootloaders. To activate
it you need to enable CONFIG_CPU_IDLE and unplug CPU1.

> And if we need MCT for low power states, we'll need MCT to co-exist
> with arch timers. Maybe by checking for the presence of those dt nodes
> + CONFIG_ARM_ARCH_TIMER, or somesuch. But let's discuss that when we
> know more.

Agreed.

Best regards,
Tomasz

[PATCH] arm: dts: exynos5: Remove multi core timer

[Doug Anderson]

Tomasz,

On Thu, May 15, 2014 at 4:25 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> On 16.05.2014 01:18, David Riley wrote:
>> On Thu, May 15, 2014 at 4:03 PM, Chirantan Ekbote
>> <chirantan@chromium.org> wrote:
>>> Hi Tomasz,
>>>
>>> On Thu, May 15, 2014 at 3:44 PM, Doug Anderson <dianders@chromium.org> wrote:
>>>> Tomasz,
>>>>
>>>> On Thu, May 15, 2014 at 3:13 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>>>>>> NOTE: if for some reason we need to keep the MCT around, we're
>>>>>> definitely going to need to account for the fact that tweaking it
>>>>>> affects the arch timer.  ...and having the arch timer is really nice
>>>>>> since:
>>>>>
>>>>> [Let me reorder the points below to make it easier to comment:]
>>>>>
>>>>>> * it's faster to access.
>>>>>> * it is accessible from userspace for really fast access.
>>>>>
>>>>> Do you have some data on whether it is a significant difference,
>>>>> especially considering real use cases?
>>>>
>>>> I know that Chrome makes _a lot_ of calls to gettimeofday() for
>>>> profiling purposes, enough that it showed up on benchmarks.  In fact,
>>>> we made a change to the MCT to make accesses faster and there's a
>>>> small mention of the benchmarking that was done at:
>>>>
>>>> https://chromium-review.googlesource.com/#/c/32190/
>>>>
>>>> ...that change probably should be sent upstream, actually.
>>>>
>>>> I'll let Chirantan comment on how much faster arch timers were.
>>>> ...and I think David Riley (also CCed now) may be able to comment on
>>>> the benefits of userspace timers.
>>>>
>>>
>>> When I profiled gettimeofday() calls, they were about 50 - 60% faster
>>> with the arch timers compared to the mct.
>>
>> When I profiled gettimeofday(), the standard systems call version took
>> about 2.5x longer than through a vDSO interface.
>
> Sounds like we should invent a new kind of jokes, starting with "When I
> profiled gettimeofday()". ;)
>
> Just kidding.
>
> The raw improvement looks quite good, but what I'm more concerned about
> is whether this has any significant effect on real use cases. As Doug
> mentioned, Chrome makes a lot of calls to gettimeofday(), but he also
> mentioned that this is for profiling purposes. Is performance of
> gettimeofday() really that crucial in this case? Are there any other use
> cases when this improvement is significant?

I guess I should restate.  Chrome is always profiling to some extent.
I think that the Javascript engine, for instance, uses gettimeofday()
to figure out how much time it's spending in various places.

Sonny just sent me some recent benchmarks using perf.  On this
particular benchmark it shows 1.85% of the time was spent in
exynos_frc_read.  If we can half that then we'll get a ~1% speedup in
the system, which is nothing to sneeze at.  If getting rid of the
system call overead makes this several times faster again, then we
roughly eliminate the overhead totally.


> Anyway, I'm by no means opposed to switching to arch timers. They
> provide a well designed, generic interface and drivers shared by
> multiple platforms, which means more code sharing and possibly more eyes
> looking at the code, which is always good. However if they don't support
> low power states correctly, we can't just remove MCT.

I think low power states aren't in mainline (right?).

One solution that might work could be to leave the device tree entry
alone but change the MCT init code to simply act as a no-op if it sees
an arch timer is in the device tree and enabled.  Then when/if someone
got the low power states enabled we could just change source code
rather than dts files.

-Doug

[PATCH] arm: dts: exynos5: Remove multi core timer

[Sonny Rao]

On Thu, May 15, 2014 at 4:43 PM, Doug Anderson <dianders@chromium.org> wrote:
> Tomasz,
>
> On Thu, May 15, 2014 at 4:25 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>> On 16.05.2014 01:18, David Riley wrote:
>>> On Thu, May 15, 2014 at 4:03 PM, Chirantan Ekbote
>>> <chirantan@chromium.org> wrote:
>>>> Hi Tomasz,
>>>>
>>>> On Thu, May 15, 2014 at 3:44 PM, Doug Anderson <dianders@chromium.org> wrote:
>>>>> Tomasz,
>>>>>
>>>>> On Thu, May 15, 2014 at 3:13 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>>>>>>> NOTE: if for some reason we need to keep the MCT around, we're
>>>>>>> definitely going to need to account for the fact that tweaking it
>>>>>>> affects the arch timer.  ...and having the arch timer is really nice
>>>>>>> since:
>>>>>>
>>>>>> [Let me reorder the points below to make it easier to comment:]
>>>>>>
>>>>>>> * it's faster to access.
>>>>>>> * it is accessible from userspace for really fast access.
>>>>>>
>>>>>> Do you have some data on whether it is a significant difference,
>>>>>> especially considering real use cases?
>>>>>
>>>>> I know that Chrome makes _a lot_ of calls to gettimeofday() for
>>>>> profiling purposes, enough that it showed up on benchmarks.  In fact,
>>>>> we made a change to the MCT to make accesses faster and there's a
>>>>> small mention of the benchmarking that was done at:
>>>>>
>>>>> https://chromium-review.googlesource.com/#/c/32190/
>>>>>
>>>>> ...that change probably should be sent upstream, actually.
>>>>>
>>>>> I'll let Chirantan comment on how much faster arch timers were.
>>>>> ...and I think David Riley (also CCed now) may be able to comment on
>>>>> the benefits of userspace timers.
>>>>>
>>>>
>>>> When I profiled gettimeofday() calls, they were about 50 - 60% faster
>>>> with the arch timers compared to the mct.
>>>
>>> When I profiled gettimeofday(), the standard systems call version took
>>> about 2.5x longer than through a vDSO interface.
>>
>> Sounds like we should invent a new kind of jokes, starting with "When I
>> profiled gettimeofday()". ;)
>>
>> Just kidding.
>>
>> The raw improvement looks quite good, but what I'm more concerned about
>> is whether this has any significant effect on real use cases. As Doug
>> mentioned, Chrome makes a lot of calls to gettimeofday(), but he also
>> mentioned that this is for profiling purposes. Is performance of
>> gettimeofday() really that crucial in this case? Are there any other use
>> cases when this improvement is significant?
>
> I guess I should restate.  Chrome is always profiling to some extent.
> I think that the Javascript engine, for instance, uses gettimeofday()
> to figure out how much time it's spending in various places.
>
> Sonny just sent me some recent benchmarks using perf.  On this
> particular benchmark it shows 1.85% of the time was spent in
> exynos_frc_read.  If we can half that then we'll get a ~1% speedup in
> the system, which is nothing to sneeze at.  If getting rid of the
> system call overead makes this several times faster again, then we
> roughly eliminate the overhead totally.

To clarify, that data wasn't a benchmark -- It's field data, so
actually much better than a benchmark.

>
>
>> Anyway, I'm by no means opposed to switching to arch timers. They
>> provide a well designed, generic interface and drivers shared by
>> multiple platforms, which means more code sharing and possibly more eyes
>> looking at the code, which is always good. However if they don't support
>> low power states correctly, we can't just remove MCT.
>
> I think low power states aren't in mainline (right?).
>
> One solution that might work could be to leave the device tree entry
> alone but change the MCT init code to simply act as a no-op if it sees
> an arch timer is in the device tree and enabled.  Then when/if someone
> got the low power states enabled we could just change source code
> rather than dts files.
>
> -Doug

[PATCH] arm: dts: exynos5: Remove multi core timer

[Chirantan Ekbote]

>>> Anyway, I'm by no means opposed to switching to arch timers. They
>>> provide a well designed, generic interface and drivers shared by
>>> multiple platforms, which means more code sharing and possibly more eyes
>>> looking at the code, which is always good. However if they don't support
>>> low power states correctly, we can't just remove MCT.
>>
>> I think low power states aren't in mainline (right?).
>>
>> One solution that might work could be to leave the device tree entry
>> alone but change the MCT init code to simply act as a no-op if it sees
>> an arch timer is in the device tree and enabled.  Then when/if someone
>> got the low power states enabled we could just change source code
>> rather than dts files.
>>

Doug and I were talking about this and we think we may have a way to
have the mct and arch timers co-exist.  The main issue is that the mct
(and therefore arch timer) gets cleared once during boot and every
time we do a suspend / resume.  This happens in
exynos4_mct_frc_start() but it's not immediately clear to us why the
counter needs to be reset at all.  If we remove the lines that clear
the counter then there is no longer an issue with having both the mct
and the arch timers on at the same time.

Alternately, if there is some code that depends on the mct being reset
we could store an offset instead of clearing the counter and then
subtract that offset every time something reads it.  Doug has a patch
that does this at
https://chromium-review.googlesource.com/#/c/200298/.  Effectively the
visible behavior will not change.  Would either of these options work?

Chirantan

[PATCH] arm: dts: exynos5: Remove multi core timer

[Kukjin Kim]

On 05/17/14 07:56, Chirantan Ekbote wrote:
>>>> Anyway, I'm by no means opposed to switching to arch timers. They
>>>> provide a well designed, generic interface and drivers shared by
>>>> multiple platforms, which means more code sharing and possibly more eyes
>>>> looking at the code, which is always good. However if they don't support
>>>> low power states correctly, we can't just remove MCT.
>>>
>>> I think low power states aren't in mainline (right?).
>>>
>>> One solution that might work could be to leave the device tree entry
>>> alone but change the MCT init code to simply act as a no-op if it sees
>>> an arch timer is in the device tree and enabled.  Then when/if someone
>>> got the low power states enabled we could just change source code
>>> rather than dts files.
>>>
>
> Doug and I were talking about this and we think we may have a way to
> have the mct and arch timers co-exist.  The main issue is that the mct
> (and therefore arch timer) gets cleared once during boot and every
> time we do a suspend / resume.  This happens in
> exynos4_mct_frc_start() but it's not immediately clear to us why the
> counter needs to be reset at all.  If we remove the lines that clear
> the counter then there is no longer an issue with having both the mct
> and the arch timers on at the same time.
>
> Alternately, if there is some code that depends on the mct being reset
> we could store an offset instead of clearing the counter and then
> subtract that offset every time something reads it.  Doug has a patch
> that does this at
> https://chromium-review.googlesource.com/#/c/200298/.  Effectively the
> visible behavior will not change.  Would either of these options work?
>
Hi all,

Even though I've heard something about the behavior of mct and arch 
timer...but I couldn't finish the talk to h/w guys yet. I need to talk 
again in next week then I could provide some useful information. Sorry 
for late and can you please wait a minute before deciding whatever.

Thanks,
Kukjin

[PATCH] arm: dts: exynos5: Remove multi core timer

[Doug Anderson]

Kukjin,

On Fri, May 16, 2014 at 5:02 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> On 05/17/14 07:56, Chirantan Ekbote wrote:
>>>>>
>>>>> Anyway, I'm by no means opposed to switching to arch timers. They
>>>>> provide a well designed, generic interface and drivers shared by
>>>>> multiple platforms, which means more code sharing and possibly more
>>>>> eyes
>>>>> looking at the code, which is always good. However if they don't
>>>>> support
>>>>> low power states correctly, we can't just remove MCT.
>>>>
>>>>
>>>> I think low power states aren't in mainline (right?).
>>>>
>>>> One solution that might work could be to leave the device tree entry
>>>> alone but change the MCT init code to simply act as a no-op if it sees
>>>> an arch timer is in the device tree and enabled.  Then when/if someone
>>>> got the low power states enabled we could just change source code
>>>> rather than dts files.
>>>>
>>
>> Doug and I were talking about this and we think we may have a way to
>> have the mct and arch timers co-exist.  The main issue is that the mct
>> (and therefore arch timer) gets cleared once during boot and every
>> time we do a suspend / resume.  This happens in
>> exynos4_mct_frc_start() but it's not immediately clear to us why the
>> counter needs to be reset at all.  If we remove the lines that clear
>> the counter then there is no longer an issue with having both the mct
>> and the arch timers on at the same time.
>>
>> Alternately, if there is some code that depends on the mct being reset
>> we could store an offset instead of clearing the counter and then
>> subtract that offset every time something reads it.  Doug has a patch
>> that does this at
>> https://chromium-review.googlesource.com/#/c/200298/.  Effectively the
>> visible behavior will not change.  Would either of these options work?
>>
> Hi all,
>
> Even though I've heard something about the behavior of mct and arch
> timer...but I couldn't finish the talk to h/w guys yet. I need to talk again
> in next week then I could provide some useful information. Sorry for late
> and can you please wait a minute before deciding whatever.

I think we could wait a few days.  Note however, that Chirantan's
latest proposal keeps all existing functionality (can use both MCT and
arch timers).  It merely removes the unnecessary bit of the MCT init
code setting the timer.  No functionality is affected by that.

-Doug

[PATCH] arm: dts: exynos5: Remove multi core timer

[Kukjin Kim]

Doug Anderson wrote:
> 
> Kukjin,
> 
Hi Doug,

> On Fri, May 16, 2014 at 5:02 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> > On 05/17/14 07:56, Chirantan Ekbote wrote:
> >>>>>
> >>>>> Anyway, I'm by no means opposed to switching to arch timers. They
> >>>>> provide a well designed, generic interface and drivers shared by
> >>>>> multiple platforms, which means more code sharing and possibly more
> >>>>> eyes
> >>>>> looking at the code, which is always good. However if they don't
> >>>>> support
> >>>>> low power states correctly, we can't just remove MCT.
> >>>>
> >>>>
> >>>> I think low power states aren't in mainline (right?).
> >>>>
> >>>> One solution that might work could be to leave the device tree entry
> >>>> alone but change the MCT init code to simply act as a no-op if it sees
> >>>> an arch timer is in the device tree and enabled.  Then when/if someone
> >>>> got the low power states enabled we could just change source code
> >>>> rather than dts files.
> >>>>
> >>
> >> Doug and I were talking about this and we think we may have a way to
> >> have the mct and arch timers co-exist.  The main issue is that the mct
> >> (and therefore arch timer) gets cleared once during boot and every
> >> time we do a suspend / resume.  This happens in
> >> exynos4_mct_frc_start() but it's not immediately clear to us why the
> >> counter needs to be reset at all.  If we remove the lines that clear
> >> the counter then there is no longer an issue with having both the mct
> >> and the arch timers on at the same time.
> >>
> >> Alternately, if there is some code that depends on the mct being reset
> >> we could store an offset instead of clearing the counter and then
> >> subtract that offset every time something reads it.  Doug has a patch
> >> that does this at
> >> https://chromium-review.googlesource.com/#/c/200298/.  Effectively the
> >> visible behavior will not change.  Would either of these options work?
> >>
> > Hi all,
> >
> > Even though I've heard something about the behavior of mct and arch
> > timer...but I couldn't finish the talk to h/w guys yet. I need to talk
> again
> > in next week then I could provide some useful information. Sorry for late
> > and can you please wait a minute before deciding whatever.
> 
> I think we could wait a few days.  Note however, that Chirantan's
> latest proposal keeps all existing functionality (can use both MCT and
> arch timers).  It merely removes the unnecessary bit of the MCT init
> code setting the timer.  No functionality is affected by that.
> 
Let me explain the behavior of MCT and arch timer.

Basically the two blocks are connected and the arch timer uses the count value from MCT for reference on exynos5250 so following in this mail thread is expected and it's true.

* If you read the MCT and the arch timer, they give the same value.

And as you know, usually the access to arch timer is faster than MCT because of APB bus access...but using MCT has some benefits sometimes it depends on use case of power management though. BTW, since exynos5260, exynos5420 and exynos5800 doesn't support arch timer, we have been using MCT on exynos5 SoCs.

I hope this makes clear and my suggestion on previous this email thread would be helpful.

Thanks,
Kukjin

[PATCH] arm: dts: exynos5: Remove multi core timer

[Olof Johansson]

On Wed, May 21, 2014 at 6:24 AM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> Doug Anderson wrote:
>>
>> Kukjin,
>>
> Hi Doug,
>
>> On Fri, May 16, 2014 at 5:02 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
>> > On 05/17/14 07:56, Chirantan Ekbote wrote:
>> >>>>>
>> >>>>> Anyway, I'm by no means opposed to switching to arch timers. They
>> >>>>> provide a well designed, generic interface and drivers shared by
>> >>>>> multiple platforms, which means more code sharing and possibly more
>> >>>>> eyes
>> >>>>> looking at the code, which is always good. However if they don't
>> >>>>> support
>> >>>>> low power states correctly, we can't just remove MCT.
>> >>>>
>> >>>>
>> >>>> I think low power states aren't in mainline (right?).
>> >>>>
>> >>>> One solution that might work could be to leave the device tree entry
>> >>>> alone but change the MCT init code to simply act as a no-op if it sees
>> >>>> an arch timer is in the device tree and enabled.  Then when/if someone
>> >>>> got the low power states enabled we could just change source code
>> >>>> rather than dts files.
>> >>>>
>> >>
>> >> Doug and I were talking about this and we think we may have a way to
>> >> have the mct and arch timers co-exist.  The main issue is that the mct
>> >> (and therefore arch timer) gets cleared once during boot and every
>> >> time we do a suspend / resume.  This happens in
>> >> exynos4_mct_frc_start() but it's not immediately clear to us why the
>> >> counter needs to be reset at all.  If we remove the lines that clear
>> >> the counter then there is no longer an issue with having both the mct
>> >> and the arch timers on at the same time.
>> >>
>> >> Alternately, if there is some code that depends on the mct being reset
>> >> we could store an offset instead of clearing the counter and then
>> >> subtract that offset every time something reads it.  Doug has a patch
>> >> that does this at
>> >> https://chromium-review.googlesource.com/#/c/200298/.  Effectively the
>> >> visible behavior will not change.  Would either of these options work?
>> >>
>> > Hi all,
>> >
>> > Even though I've heard something about the behavior of mct and arch
>> > timer...but I couldn't finish the talk to h/w guys yet. I need to talk
>> again
>> > in next week then I could provide some useful information. Sorry for late
>> > and can you please wait a minute before deciding whatever.
>>
>> I think we could wait a few days.  Note however, that Chirantan's
>> latest proposal keeps all existing functionality (can use both MCT and
>> arch timers).  It merely removes the unnecessary bit of the MCT init
>> code setting the timer.  No functionality is affected by that.
>>
> Let me explain the behavior of MCT and arch timer.
>
> Basically the two blocks are connected and the arch timer uses the count value from MCT for reference on exynos5250 so following in this mail thread is expected and it's true.
>
> * If you read the MCT and the arch timer, they give the same value.
>
> And as you know, usually the access to arch timer is faster than MCT because of APB bus access...but using MCT has some benefits sometimes it depends on use case of power management though. BTW, since exynos5260, exynos5420 and exynos5800 doesn't support arch timer, we have been using MCT on exynos5 SoCs.

This I don't understand. What, _exactly_ are the benefits of MCT.
You've been vague on this several times now and it is not helping us
understand why Samsung (and you personally) prefer MCT. Details,
please.



-Olof

[PATCH] arm: dts: exynos5: Remove multi core timer

[Tomasz Figa]

On 21.05.2014 15:24, Kukjin Kim wrote:
> Doug Anderson wrote:
>> 
>> Kukjin,
>> 
> Hi Doug,
> 
>> On Fri, May 16, 2014 at 5:02 PM, Kukjin Kim <kgene.kim@samsung.com>
>> wrote:
>>> On 05/17/14 07:56, Chirantan Ekbote wrote:
>>>>>>> 
>>>>>>> Anyway, I'm by no means opposed to switching to arch
>>>>>>> timers. They provide a well designed, generic interface
>>>>>>> and drivers shared by multiple platforms, which means
>>>>>>> more code sharing and possibly more eyes looking at the
>>>>>>> code, which is always good. However if they don't 
>>>>>>> support low power states correctly, we can't just remove
>>>>>>> MCT.
>>>>>> 
>>>>>> 
>>>>>> I think low power states aren't in mainline (right?).
>>>>>> 
>>>>>> One solution that might work could be to leave the device
>>>>>> tree entry alone but change the MCT init code to simply act
>>>>>> as a no-op if it sees an arch timer is in the device tree
>>>>>> and enabled.  Then when/if someone got the low power states
>>>>>> enabled we could just change source code rather than dts
>>>>>> files.
>>>>>> 
>>>> 
>>>> Doug and I were talking about this and we think we may have a
>>>> way to have the mct and arch timers co-exist.  The main issue
>>>> is that the mct (and therefore arch timer) gets cleared once
>>>> during boot and every time we do a suspend / resume.  This
>>>> happens in exynos4_mct_frc_start() but it's not immediately
>>>> clear to us why the counter needs to be reset at all.  If we
>>>> remove the lines that clear the counter then there is no longer
>>>> an issue with having both the mct and the arch timers on at the
>>>> same time.
>>>> 
>>>> Alternately, if there is some code that depends on the mct
>>>> being reset we could store an offset instead of clearing the
>>>> counter and then subtract that offset every time something
>>>> reads it.  Doug has a patch that does this at 
>>>> https://chromium-review.googlesource.com/#/c/200298/.
>>>> Effectively the visible behavior will not change.  Would either
>>>> of these options work?
>>>> 
>>> Hi all,
>>> 
>>> Even though I've heard something about the behavior of mct and
>>> arch timer...but I couldn't finish the talk to h/w guys yet. I
>>> need to talk
>> again
>>> in next week then I could provide some useful information. Sorry
>>> for late and can you please wait a minute before deciding
>>> whatever.
>> 
>> I think we could wait a few days.  Note however, that Chirantan's 
>> latest proposal keeps all existing functionality (can use both MCT
>> and arch timers).  It merely removes the unnecessary bit of the MCT
>> init code setting the timer.  No functionality is affected by
>> that.
>> 
> Let me explain the behavior of MCT and arch timer.
> 
> Basically the two blocks are connected and the arch timer uses the
> count value from MCT for reference on exynos5250 so following in this
> mail thread is expected and it's true.

To clarify, is this true regarding only the free running counter or also
global interrupt timers and local interrupt timers?

> 
> * If you read the MCT and the arch timer, they give the same value.
> 
> And as you know, usually the access to arch timer is faster than MCT
> because of APB bus access...but using MCT has some benefits sometimes
> it depends on use case of power management though.

It would be nice to have a clear list of advantages of MCT, as at the
moment in this thread we had only the advantages of arch timer presented.

> BTW, since
> exynos5260, exynos5420 and exynos5800 doesn't support arch timer, we
> have been using MCT on exynos5 SoCs.

Hmm, I thought arch timer was a core feature of ARM Cortex A15 (and A7)
cores. Also you mention Exynos5420, while Chirantan's patch removing MCT
node from exynos5420.dtsi, would suggest that it worked for him fine. (I
assume it was tested.)

Best regards,
Tomasz

[PATCH] arm: dts: exynos5: Remove multi core timer

[Chirantan Ekbote]

On Wed, May 21, 2014 at 9:20 AM, Tomasz Figa <t.figa@samsung.com> wrote:
> On 21.05.2014 15:24, Kukjin Kim wrote:
>
>> BTW, since
>> exynos5260, exynos5420 and exynos5800 doesn't support arch timer, we
>> have been using MCT on exynos5 SoCs.
>
> Hmm, I thought arch timer was a core feature of ARM Cortex A15 (and A7)
> cores. Also you mention Exynos5420, while Chirantan's patch removing MCT
> node from exynos5420.dtsi, would suggest that it worked for him fine. (I
> assume it was tested.)
>

I was able to boot both 5420 and 5800 with just the arch timers on our
3.8 based franken-kernel.  The upstream kernel doesn't boot on those
systems with or without the mct so I wasn't able to test there.
Although looking at the upstream device tree now I see that there is
no entry for the arch timer in exynos5420.dtsi.  Maybe it should be
added in?

Chirantan

[PATCH] arm: dts: exynos5: Remove multi core timer

[Doug Anderson]

Kukjin,

On Wed, May 21, 2014 at 11:34 AM, Chirantan Ekbote
<chirantan@chromium.org> wrote:
> On Wed, May 21, 2014 at 9:20 AM, Tomasz Figa <t.figa@samsung.com> wrote:
>> On 21.05.2014 15:24, Kukjin Kim wrote:
>>
>>> BTW, since
>>> exynos5260, exynos5420 and exynos5800 doesn't support arch timer, we
>>> have been using MCT on exynos5 SoCs.
>>
>> Hmm, I thought arch timer was a core feature of ARM Cortex A15 (and A7)
>> cores. Also you mention Exynos5420, while Chirantan's patch removing MCT
>> node from exynos5420.dtsi, would suggest that it worked for him fine. (I
>> assume it was tested.)
>>
>
> I was able to boot both 5420 and 5800 with just the arch timers on our
> 3.8 based franken-kernel.  The upstream kernel doesn't boot on those
> systems with or without the mct so I wasn't able to test there.
> Although looking at the upstream device tree now I see that there is
> no entry for the arch timer in exynos5420.dtsi.  Maybe it should be
> added in?

A gentle reminder to respond here about the status of MCT on 5420 and 5800.

-Doug

[PATCH] arm: dts: exynos5: Remove multi core timer

[Doug Anderson]

Kukjin,

On Wed, May 28, 2014 at 10:38 AM, Doug Anderson <dianders@chromium.org> wrote:
> Kukjin,
>
> On Wed, May 21, 2014 at 11:34 AM, Chirantan Ekbote
> <chirantan@chromium.org> wrote:
>> On Wed, May 21, 2014 at 9:20 AM, Tomasz Figa <t.figa@samsung.com> wrote:
>>> On 21.05.2014 15:24, Kukjin Kim wrote:
>>>
>>>> BTW, since
>>>> exynos5260, exynos5420 and exynos5800 doesn't support arch timer, we
>>>> have been using MCT on exynos5 SoCs.
>>>
>>> Hmm, I thought arch timer was a core feature of ARM Cortex A15 (and A7)
>>> cores. Also you mention Exynos5420, while Chirantan's patch removing MCT
>>> node from exynos5420.dtsi, would suggest that it worked for him fine. (I
>>> assume it was tested.)
>>>
>>
>> I was able to boot both 5420 and 5800 with just the arch timers on our
>> 3.8 based franken-kernel.  The upstream kernel doesn't boot on those
>> systems with or without the mct so I wasn't able to test there.
>> Although looking at the upstream device tree now I see that there is
>> no entry for the arch timer in exynos5420.dtsi.  Maybe it should be
>> added in?
>
> A gentle reminder to respond here about the status of MCT on 5420 and 5800.

Another reminder to respond about the status of arch timers on 5420
and 5800 (sorry, meant arch timers here--we know MCT works).

-Doug

[PATCH] arm: dts: exynos5: Remove multi core timer

[Kukjin Kim]

Chirantan Ekbote wrote:
> 
> >>> Anyway, I'm by no means opposed to switching to arch timers. They
> >>> provide a well designed, generic interface and drivers shared by
> >>> multiple platforms, which means more code sharing and possibly more eyes
> >>> looking at the code, which is always good. However if they don't support
> >>> low power states correctly, we can't just remove MCT.
> >>
> >> I think low power states aren't in mainline (right?).
> >>
> >> One solution that might work could be to leave the device tree entry
> >> alone but change the MCT init code to simply act as a no-op if it sees
> >> an arch timer is in the device tree and enabled.  Then when/if someone
> >> got the low power states enabled we could just change source code
> >> rather than dts files.
> >>
> Doug and I were talking about this and we think we may have a way to
> have the mct and arch timers co-exist.  The main issue is that the mct
> (and therefore arch timer) gets cleared once during boot and every
> time we do a suspend / resume.  This happens in
> exynos4_mct_frc_start() but it's not immediately clear to us why the
> counter needs to be reset at all.  If we remove the lines that clear
> the counter then there is no longer an issue with having both the mct
> and the arch timers on at the same time.
> 
Yeah, actually we don't need to reset the count value after suspend/resume.
So, how about following? I think, it should be fine to you.

diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
index 8d64200..d24db6f 100644
--- a/drivers/clocksource/exynos_mct.c
+++ b/drivers/clocksource/exynos_mct.c
@@ -157,12 +157,15 @@ static void exynos4_mct_frc_start(u32 hi, u32 lo)
 {
 	u32 reg;
 
-	exynos4_mct_write(lo, EXYNOS4_MCT_G_CNT_L);
-	exynos4_mct_write(hi, EXYNOS4_MCT_G_CNT_U);
-
 	reg = __raw_readl(reg_base + EXYNOS4_MCT_G_TCON);
-	reg |= MCT_G_TCON_START;
-	exynos4_mct_write(reg, EXYNOS4_MCT_G_TCON);
+
+	if (!(reg & MCT_G_TCON_START)) {
+		exynos4_mct_write(lo, EXYNOS4_MCT_G_CNT_L);
+		exynos4_mct_write(hi, EXYNOS4_MCT_G_CNT_U);
+
+		reg |= MCT_G_TCON_START;
+		exynos4_mct_write(reg, EXYNOS4_MCT_G_TCON);
+	}
 }


> Alternately, if there is some code that depends on the mct being reset
> we could store an offset instead of clearing the counter and then
> subtract that offset every time something reads it.  Doug has a patch
> that does this at
> https://chromium-review.googlesource.com/#/c/200298/.  Effectively the
> visible behavior will not change.  Would either of these options work?
> 
Hmm...I cannot open the webpage :(

- Kukjin

[PATCH] arm: dts: exynos5: Remove multi core timer

[Chirantan Ekbote]

On Wed, May 21, 2014 at 5:47 AM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> Chirantan Ekbote wrote:
>>
>> >>> Anyway, I'm by no means opposed to switching to arch timers. They
>> >>> provide a well designed, generic interface and drivers shared by
>> >>> multiple platforms, which means more code sharing and possibly more eyes
>> >>> looking at the code, which is always good. However if they don't support
>> >>> low power states correctly, we can't just remove MCT.
>> >>
>> >> I think low power states aren't in mainline (right?).
>> >>
>> >> One solution that might work could be to leave the device tree entry
>> >> alone but change the MCT init code to simply act as a no-op if it sees
>> >> an arch timer is in the device tree and enabled.  Then when/if someone
>> >> got the low power states enabled we could just change source code
>> >> rather than dts files.
>> >>
>> Doug and I were talking about this and we think we may have a way to
>> have the mct and arch timers co-exist.  The main issue is that the mct
>> (and therefore arch timer) gets cleared once during boot and every
>> time we do a suspend / resume.  This happens in
>> exynos4_mct_frc_start() but it's not immediately clear to us why the
>> counter needs to be reset at all.  If we remove the lines that clear
>> the counter then there is no longer an issue with having both the mct
>> and the arch timers on at the same time.
>>
> Yeah, actually we don't need to reset the count value after suspend/resume.
> So, how about following? I think, it should be fine to you.
>
> diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
> index 8d64200..d24db6f 100644
> --- a/drivers/clocksource/exynos_mct.c
> +++ b/drivers/clocksource/exynos_mct.c
> @@ -157,12 +157,15 @@ static void exynos4_mct_frc_start(u32 hi, u32 lo)
>  {
>         u32 reg;
>
> -       exynos4_mct_write(lo, EXYNOS4_MCT_G_CNT_L);
> -       exynos4_mct_write(hi, EXYNOS4_MCT_G_CNT_U);
> -
>         reg = __raw_readl(reg_base + EXYNOS4_MCT_G_TCON);
> -       reg |= MCT_G_TCON_START;
> -       exynos4_mct_write(reg, EXYNOS4_MCT_G_TCON);
> +
> +       if (!(reg & MCT_G_TCON_START)) {
> +               exynos4_mct_write(lo, EXYNOS4_MCT_G_CNT_L);
> +               exynos4_mct_write(hi, EXYNOS4_MCT_G_CNT_U);
> +
> +               reg |= MCT_G_TCON_START;
> +               exynos4_mct_write(reg, EXYNOS4_MCT_G_TCON);
> +       }
>  }
>

So if I understand correctly, we still need to reset the counter
during boot?  This is still a problem because there would be a big
jump in time since the sched_clock code would think that the timer had
wrapped around.  Additionally, any clock events that were scheduled
before the reset would be delayed until the cycle counter caught back
up to the value it had previously.  This manifests itself in our tree
as an extra long jiffy when the xor code is measuring the software
checksum speed and it delays the entire boot process.

There is a hacky solution to this, which is to ensure that the mct is
always initialized before the arch timer.  This should be possible by
reordering the entries in the device tree.  We would also need to
change the clocksource rating for one of the two (either increase the
arch timer rating or decrease the mct rating) to ensure that the
kernel still picked the arch timer as its clocksource.

>
>> Alternately, if there is some code that depends on the mct being reset
>> we could store an offset instead of clearing the counter and then
>> subtract that offset every time something reads it.  Doug has a patch
>> that does this at
>> https://chromium-review.googlesource.com/#/c/200298/.  Effectively the
>> visible behavior will not change.  Would either of these options work?
>>
> Hmm...I cannot open the webpage :(
>

I've attached the patch to this email.

Chirantan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-clocksource-mct-Don-t-clear-the-mct.patch
Type: text/x-patch
Size: 3636 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140521/5f5ddfae/attachment-0001.bin>

[PATCH] arm: dts: exynos5: Remove multi core timer

[Doug Anderson]

Kukjin,

Sorry for the delay in responding--this thread started just as I was
walking out the door for vacation...

On Wed, May 21, 2014 at 5:47 AM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> Chirantan Ekbote wrote:
>>
>> >>> Anyway, I'm by no means opposed to switching to arch timers. They
>> >>> provide a well designed, generic interface and drivers shared by
>> >>> multiple platforms, which means more code sharing and possibly more eyes
>> >>> looking at the code, which is always good. However if they don't support
>> >>> low power states correctly, we can't just remove MCT.
>> >>
>> >> I think low power states aren't in mainline (right?).
>> >>
>> >> One solution that might work could be to leave the device tree entry
>> >> alone but change the MCT init code to simply act as a no-op if it sees
>> >> an arch timer is in the device tree and enabled.  Then when/if someone
>> >> got the low power states enabled we could just change source code
>> >> rather than dts files.
>> >>
>> Doug and I were talking about this and we think we may have a way to
>> have the mct and arch timers co-exist.  The main issue is that the mct
>> (and therefore arch timer) gets cleared once during boot and every
>> time we do a suspend / resume.  This happens in
>> exynos4_mct_frc_start() but it's not immediately clear to us why the
>> counter needs to be reset at all.  If we remove the lines that clear
>> the counter then there is no longer an issue with having both the mct
>> and the arch timers on at the same time.
>>
> Yeah, actually we don't need to reset the count value after suspend/resume.
> So, how about following? I think, it should be fine to you.
>
> diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
> index 8d64200..d24db6f 100644
> --- a/drivers/clocksource/exynos_mct.c
> +++ b/drivers/clocksource/exynos_mct.c
> @@ -157,12 +157,15 @@ static void exynos4_mct_frc_start(u32 hi, u32 lo)
>  {
>         u32 reg;
>
> -       exynos4_mct_write(lo, EXYNOS4_MCT_G_CNT_L);
> -       exynos4_mct_write(hi, EXYNOS4_MCT_G_CNT_U);
> -
>         reg = __raw_readl(reg_base + EXYNOS4_MCT_G_TCON);
> -       reg |= MCT_G_TCON_START;
> -       exynos4_mct_write(reg, EXYNOS4_MCT_G_TCON);
> +
> +       if (!(reg & MCT_G_TCON_START)) {
> +               exynos4_mct_write(lo, EXYNOS4_MCT_G_CNT_L);
> +               exynos4_mct_write(hi, EXYNOS4_MCT_G_CNT_U);
> +
> +               reg |= MCT_G_TCON_START;
> +               exynos4_mct_write(reg, EXYNOS4_MCT_G_TCON);
> +       }

I guess this is OK, but personally I'd vote to remove the init
altogether unless there is a hardware bug on some versions of exynos
that requires that we init this.  The kernel couldn't care less about
what value the timer starts at, so even on systems where the MCT isn't
started by the bootloader this is just a waste of code.

Chirantan: can you post your patch
<https://chromium-review.googlesource.com/#/c/201143/> up?


>> Alternately, if there is some code that depends on the mct being reset
>> we could store an offset instead of clearing the counter and then
>> subtract that offset every time something reads it.  Doug has a patch
>> that does this at
>> https://chromium-review.googlesource.com/#/c/200298/.  Effectively the
>> visible behavior will not change.  Would either of these options work?
>>
> Hmm...I cannot open the webpage :(

Can you try again?  I think there may have been a glitch.  It's also
possible that your browser grabbed the "." at the end of the URL,
which would confuse things.

-Doug

[PATCH] arm: dts: exynos5: Remove multi core timer

[Chirantan Ekbote]

Hi Kukjin,

On Wed, May 21, 2014 at 5:47 AM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> Yeah, actually we don't need to reset the count value after suspend/resume.
> So, how about following? I think, it should be fine to you.
>
> diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
> index 8d64200..d24db6f 100644
> --- a/drivers/clocksource/exynos_mct.c
> +++ b/drivers/clocksource/exynos_mct.c
> @@ -157,12 +157,15 @@ static void exynos4_mct_frc_start(u32 hi, u32 lo)
>  {
>         u32 reg;
>
> -       exynos4_mct_write(lo, EXYNOS4_MCT_G_CNT_L);
> -       exynos4_mct_write(hi, EXYNOS4_MCT_G_CNT_U);
> -
>         reg = __raw_readl(reg_base + EXYNOS4_MCT_G_TCON);
> -       reg |= MCT_G_TCON_START;
> -       exynos4_mct_write(reg, EXYNOS4_MCT_G_TCON);
> +
> +       if (!(reg & MCT_G_TCON_START)) {
> +               exynos4_mct_write(lo, EXYNOS4_MCT_G_CNT_L);
> +               exynos4_mct_write(hi, EXYNOS4_MCT_G_CNT_U);
> +
> +               reg |= MCT_G_TCON_START;
> +               exynos4_mct_write(reg, EXYNOS4_MCT_G_TCON);
> +       }
>  }
>
>

As Doug mentioned, this seems more complicated than necessary since
the kernel doesn't care about the initial value of the mct counter at
all.  Is there some reason from a hardware standpoint that the counter
needs to be cleared?  If not, I would rather just delete the two
offending lines.  I am sending a patch that does this instead.

Cheers,
Chirantan

[PATCH] arm: dts: exynos5: Remove multi core timer

[Kukjin Kim]

Chirantan Ekbote wrote:
> 
> Hi Kukjin,
> 
Hi,

> On Wed, May 21, 2014 at 5:47 AM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> > Yeah, actually we don't need to reset the count value after suspend/resume.
> > So, how about following? I think, it should be fine to you.
> >
> > diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
> > index 8d64200..d24db6f 100644
> > --- a/drivers/clocksource/exynos_mct.c
> > +++ b/drivers/clocksource/exynos_mct.c
> > @@ -157,12 +157,15 @@ static void exynos4_mct_frc_start(u32 hi, u32 lo)
> >  {
> >         u32 reg;
> >
> > -       exynos4_mct_write(lo, EXYNOS4_MCT_G_CNT_L);
> > -       exynos4_mct_write(hi, EXYNOS4_MCT_G_CNT_U);
> > -
> >         reg = __raw_readl(reg_base + EXYNOS4_MCT_G_TCON);
> > -       reg |= MCT_G_TCON_START;
> > -       exynos4_mct_write(reg, EXYNOS4_MCT_G_TCON);
> > +
> > +       if (!(reg & MCT_G_TCON_START)) {
> > +               exynos4_mct_write(lo, EXYNOS4_MCT_G_CNT_L);
> > +               exynos4_mct_write(hi, EXYNOS4_MCT_G_CNT_U);
> > +
> > +               reg |= MCT_G_TCON_START;
> > +               exynos4_mct_write(reg, EXYNOS4_MCT_G_TCON);
> > +       }
> >  }
> >
> >
> 
> As Doug mentioned, this seems more complicated than necessary since
> the kernel doesn't care about the initial value of the mct counter at
> all.  Is there some reason from a hardware standpoint that the counter
> needs to be cleared?  If not, I would rather just delete the two
> offending lines.  I am sending a patch that does this instead.
> 
So decision point is that the initialization of MCT counter is required or not
when kernel begins. Yes it doesn't matter, basically MCT start has no problem
with any initial value and additionally its hardware reset value is 0x0.

OK, your suggestion is fair enough.

Thanks,
Kukjin

[PATCH] arm: dts: exynos5: Remove multi core timer

[Doug Anderson]

Tomasz,

On Thu, May 15, 2014 at 3:44 PM, Doug Anderson <dianders@chromium.org> wrote:
> Tomasz,
>
> On Thu, May 15, 2014 at 3:13 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>>> NOTE: if for some reason we need to keep the MCT around, we're
>>> definitely going to need to account for the fact that tweaking it
>>> affects the arch timer.  ...and having the arch timer is really nice
>>> since:
>>
>> [Let me reorder the points below to make it easier to comment:]
>>
>>> * it's faster to access.
>>> * it is accessible from userspace for really fast access.
>>
>> Do you have some data on whether it is a significant difference,
>> especially considering real use cases?
>
> I know that Chrome makes _a lot_ of calls to gettimeofday() for
> profiling purposes, enough that it showed up on benchmarks.  In fact,
> we made a change to the MCT to make accesses faster and there's a
> small mention of the benchmarking that was done at:
>
> https://chromium-review.googlesource.com/#/c/32190/
>
> ...that change probably should be sent upstream, actually.
>
> I'll let Chirantan comment on how much faster arch timers were.
> ...and I think David Riley (also CCed now) may be able to comment on
> the benefits of userspace timers.
>
>
>>> * it implements the bits needed for udelay() to use it.
>>
>> Hmm, do you know what bits are those? On Exynos4 MCT is the only option
>> and it would be nice to let udelay() use it.
>
> Look for register_current_timer_delay().  It seems like we could do
> this for MCT, but I've never done the investigation because we were
> always planning to move to arch timers.  ;)

If you're planning to add support for MCT as a source for udelay, let
me know.  It sounds like there's a chance that we won't be able to use
the ARCH timers on 5420 so we may be interested in these patches as
well.

Also note that it appears that MCT upstream is also not used as a
scheduler clock.  Perhaps you would want those patches, too?  I think
Chirantan said that it will cause problems on systems that have both
MCT and arch timers though, since the system didn't like two scheduler
clocks to be registered (?).


In summary, we've got the following MCT patches proposed to go upstream:

1. MCT scheduler clock: <http://crosreview.com/56363> and
<http://crosreview.com/56364>
2. Speed MCT access: <http://crosreview.com/56365>.  I wonder if we
could also speed it up further with a 64-bit read.
3. Use MCT for udelay: yet to be written.

...does someone want to claim the task of sending those things up?


Oh, actually it looks like (93bfb76 clocksource: exynos_mct: register
sched_clock callback) in linuxnext adds a partial version of the first
patch but isn't as complete as what's in our tree (it's missing the
KConfig change we have locally as well as the notrace so it probably
breaks ftrace?).  Adding Vincent.


-Doug

[PATCH] arm: dts: exynos5: Remove multi core timer

[Vincent Guittot]

On 28 May 2014 19:37, Doug Anderson <dianders@chromium.org> wrote:
> Tomasz,
>
> On Thu, May 15, 2014 at 3:44 PM, Doug Anderson <dianders@chromium.org> wrote:
>> Tomasz,
>>
>> On Thu, May 15, 2014 at 3:13 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>>>> NOTE: if for some reason we need to keep the MCT around, we're
>>>> definitely going to need to account for the fact that tweaking it
>>>> affects the arch timer.  ...and having the arch timer is really nice
>>>> since:
>>>
>>> [Let me reorder the points below to make it easier to comment:]
>>>
>>>> * it's faster to access.
>>>> * it is accessible from userspace for really fast access.
>>>
>>> Do you have some data on whether it is a significant difference,
>>> especially considering real use cases?
>>
>> I know that Chrome makes _a lot_ of calls to gettimeofday() for
>> profiling purposes, enough that it showed up on benchmarks.  In fact,
>> we made a change to the MCT to make accesses faster and there's a
>> small mention of the benchmarking that was done at:
>>
>> https://chromium-review.googlesource.com/#/c/32190/
>>
>> ...that change probably should be sent upstream, actually.
>>
>> I'll let Chirantan comment on how much faster arch timers were.
>> ...and I think David Riley (also CCed now) may be able to comment on
>> the benefits of userspace timers.
>>
>>
>>>> * it implements the bits needed for udelay() to use it.
>>>
>>> Hmm, do you know what bits are those? On Exynos4 MCT is the only option
>>> and it would be nice to let udelay() use it.
>>
>> Look for register_current_timer_delay().  It seems like we could do
>> this for MCT, but I've never done the investigation because we were
>> always planning to move to arch timers.  ;)
>
> If you're planning to add support for MCT as a source for udelay, let
> me know.  It sounds like there's a chance that we won't be able to use
> the ARCH timers on 5420 so we may be interested in these patches as
> well.
>
> Also note that it appears that MCT upstream is also not used as a
> scheduler clock.  Perhaps you would want those patches, too?  I think
> Chirantan said that it will cause problems on systems that have both
> MCT and arch timers though, since the system didn't like two scheduler
> clocks to be registered (?).
>
>
> In summary, we've got the following MCT patches proposed to go upstream:
>
> 1. MCT scheduler clock: <http://crosreview.com/56363> and
> <http://crosreview.com/56364>
> 2. Speed MCT access: <http://crosreview.com/56365>.  I wonder if we
> could also speed it up further with a 64-bit read.
> 3. Use MCT for udelay: yet to be written.
>
> ...does someone want to claim the task of sending those things up?
>
>
> Oh, actually it looks like (93bfb76 clocksource: exynos_mct: register
> sched_clock callback) in linuxnext adds a partial version of the first
> patch but isn't as complete as what's in our tree (it's missing the
> KConfig change we have locally as well as the notrace so it probably
> breaks ftrace?).  Adding Vincent.

Hi Doug,

Thanks for adding me in the loop.

The only difference i see are:
 -HAVE_SCHED_CLOCK which is no more needed
 -and the use of 32bit vs 64bit in the for-next
 -notrace is present in the for-next  AFAICT

Regards
Vincent

>
>
> -Doug

[PATCH] arm: dts: exynos5: Remove multi core timer

[Doug Anderson]

Vincent,

On Thu, May 29, 2014 at 1:42 PM, Vincent Guittot
<vincent.guittot@linaro.org> wrote:
>> In summary, we've got the following MCT patches proposed to go upstream:
>>
>> 1. MCT scheduler clock: <http://crosreview.com/56363> and
>> <http://crosreview.com/56364>
>> 2. Speed MCT access: <http://crosreview.com/56365>.  I wonder if we
>> could also speed it up further with a 64-bit read.
>> 3. Use MCT for udelay: yet to be written.
>>
>> ...does someone want to claim the task of sending those things up?
>>
>>
>> Oh, actually it looks like (93bfb76 clocksource: exynos_mct: register
>> sched_clock callback) in linuxnext adds a partial version of the first
>> patch but isn't as complete as what's in our tree (it's missing the
>> KConfig change we have locally as well as the notrace so it probably
>> breaks ftrace?).  Adding Vincent.
>
> Hi Doug,
>
> Thanks for adding me in the loop.
>
> The only difference i see are:
>  -HAVE_SCHED_CLOCK which is no more needed
>  -and the use of 32bit vs 64bit in the for-next
>  -notrace is present in the for-next  AFAICT

Ah, my bad!  Yes, you're right that things look OK.  I looked too
quickly and didn't see the notrace.  ...and I wasn't aware that
HAVE_SCHED_CLOCK was no longer needed.

One thing that might be interesting is to consider using 32-bit
instead of 64-bit.  We know that this clock is slow to access, so
reducing 3 reads down to 1 would be worth it.  A 32-bit clock should
be sufficient for the scheduler anyway.

-Doug

[PATCH] arm: dts: exynos5: Remove multi core timer

[Kukjin Kim]

On 05/16/14 06:33, Doug Anderson wrote:
> Tomasz,
>
> On Thu, May 15, 2014 at 2:14 PM, Tomasz Figa<tomasz.figa@gmail.com>  wrote:
>> Hi Chirantan,
>>
>> On 15.05.2014 23:07, Chirantan Ekbote wrote:
>>> The multi core timer and the ARM architected timer are two different
>>> interfaces to the same underlying hardware timer.  This causes some
>>> strange timing issues when they are both enabled at the same time so
>>> remove the mct from the device tree and keep only the architected
>>> timer.
>>
>> Huh? I've always thought MCT is a completely separate hardware block
>> outside of ARM cores, while architected timers are embedded inside the
>> CPU block in which the ARM cores reside. Could you elaborate on this?
>
> Yup.  Our thoughts exactly.
>
> ...but it appears not to be the case.  Chirantan demonstrated this in
> U-Boot just to prove that it's not some strange kernel interaction in
> <https://chromium-review.googlesource.com/200035>.  I took his patch
> and tweaked it a little more myself in
> <https://chromium-review.googlesource.com/200098>.
>
> Specifically:
> * If you stop the MCT, the arch timer stops
> * If you reset the MCT, the arch timer resets
> * If you start the MCT again, the arch timer starts again
> * If you read the MCT and the arch timer, they give the same value.
>
>
> This is apparently the answer to my question at
> <http://www.spinics.net/lists/linux-samsung-soc/msg29085.html>.
> Specifically Chirantan found that the big jump in time happened when
> MCT reset to 0.  That made the arch timer code think that there was a
> wraparound and jump forward in time a lot.
>
>
> Please confirm if you have a system that has MCT and arch timer in front of you.
>
Hi all,

I need to talk to hardware guy to clarify the issue then I'll let you know.

Thanks,
Kukjin

[PATCH] arm: dts: exynos5: Remove multi core timer

[Tomasz Figa]

On 15.05.2014 23:44, Kukjin Kim wrote:
> On 05/16/14 06:33, Doug Anderson wrote:
>> Tomasz,
>>
>> On Thu, May 15, 2014 at 2:14 PM, Tomasz Figa<tomasz.figa@gmail.com> 
>> wrote:
>>> Hi Chirantan,
>>>
>>> On 15.05.2014 23:07, Chirantan Ekbote wrote:
>>>> The multi core timer and the ARM architected timer are two different
>>>> interfaces to the same underlying hardware timer.  This causes some
>>>> strange timing issues when they are both enabled at the same time so
>>>> remove the mct from the device tree and keep only the architected
>>>> timer.
>>>
>>> Huh? I've always thought MCT is a completely separate hardware block
>>> outside of ARM cores, while architected timers are embedded inside the
>>> CPU block in which the ARM cores reside. Could you elaborate on this?
>>
>> Yup.  Our thoughts exactly.
>>
>> ...but it appears not to be the case.  Chirantan demonstrated this in
>> U-Boot just to prove that it's not some strange kernel interaction in
>> <https://chromium-review.googlesource.com/200035>.  I took his patch
>> and tweaked it a little more myself in
>> <https://chromium-review.googlesource.com/200098>.
>>
>> Specifically:
>> * If you stop the MCT, the arch timer stops
>> * If you reset the MCT, the arch timer resets
>> * If you start the MCT again, the arch timer starts again
>> * If you read the MCT and the arch timer, they give the same value.
>>
>>
>> This is apparently the answer to my question at
>> <http://www.spinics.net/lists/linux-samsung-soc/msg29085.html>.
>> Specifically Chirantan found that the big jump in time happened when
>> MCT reset to 0.  That made the arch timer code think that there was a
>> wraparound and jump forward in time a lot.
>>
>>
>> Please confirm if you have a system that has MCT and arch timer in
>> front of you.
>>
> Hi all,
> 
> I need to talk to hardware guy to clarify the issue then I'll let you know.

Great, thanks. Having a confirmation from hardware team would be
definitely nice.

Best regards,
Tomasz