Re: [PATCH] clocksource: exynos_mct: fix exynos4_mct_write
Hi Tobias, On Wed, Oct 22, 2014 at 9:12 AM, Tobias Jakobi tjak...@math.uni-bielefeld.de wrote: Hello Doug, I didn't encounter any obvious problems with the MCT. I was made aware of the issue by kinsamanka on the Hardkernel forums, but didn't tell me through which means he/she found it. Looks like this came up back in February (https://lkml.org/lkml/2014/2/4/782) and the reason the code currently works is that these values are used for checking the success of the write operation. So in the current code we never check the success of the write but it hasn't been a problem because presumably the write is almost always successful. Regardless, I agree with Doug. We should take this for correctness. Concerning testing, I have this running since some weeks on my ODROID-X2 (Exynos4412), again, haven't encountered anything 'strange' yet. With best wishes, Tobias Chirantan -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] arm: dts: exynos5: Remove multi core timer
Hi Kukjin,
On Wed, May 21, 2014 at 5:47 AM, Kukjin Kim kgene@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
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] arm: mct: Don't reset the counter during boot and resume
Unfortunately on some exynos systems, resetting the mct counter also
resets the architected timer counter. This can cause problems if the
architected timer driver has already been initialized because the kernel
will think that the counter has wrapped around, causing a big jump in
printk timestamps and delaying any scheduled clock events until the
counter reaches the value it had before it was reset.
The kernel code makes no assumptions about the initial value of the mct
counter so there is no reason from a software perspective to clear the
counter before starting it. This also fixes the problems described in
the previous paragraph.
Cc: Olof Johansson o...@lixom.net
Cc: Doug Anderson diand...@chromium.org
Cc: Kukjin Kim kgene@samsung.com
Cc: Tomasz Figa tomasz.f...@gmail.com
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-samsung-soc@vger.kernel.org
Signed-off-by: Chirantan Ekbote chiran...@chromium.org
---
drivers/clocksource/exynos_mct.c | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
index acf5a32..9b4a0ae 100644
--- a/drivers/clocksource/exynos_mct.c
+++ b/drivers/clocksource/exynos_mct.c
@@ -152,13 +152,10 @@ static void exynos4_mct_write(unsigned int value,
unsigned long offset)
}
/* Clocksource handling */
-static void exynos4_mct_frc_start(u32 hi, u32 lo)
+static void exynos4_mct_frc_start(void)
{
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);
@@ -180,7 +177,7 @@ static cycle_t exynos4_frc_read(struct clocksource *cs)
static void exynos4_frc_resume(struct clocksource *cs)
{
- exynos4_mct_frc_start(0, 0);
+ exynos4_mct_frc_start();
}
struct clocksource mct_frc = {
@@ -194,7 +191,7 @@ struct clocksource mct_frc = {
static void __init exynos4_clocksource_init(void)
{
- exynos4_mct_frc_start(0, 0);
+ exynos4_mct_frc_start();
if (clocksource_register_hz(mct_frc, clk_rate))
panic(%s: can't register clocksource\n, mct_frc.name);
--
1.9.1.423.g4596e3a
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] arm: dts: exynos5: Remove multi core timer
On Wed, May 21, 2014 at 5:47 AM, Kukjin Kim kgene@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
From 468ab95fb57f1b72da838aa46346635a48a94af4 Mon Sep 17 00:00:00 2001
From: Doug Anderson diand...@chromium.org
Date: Fri, 16 May 2014 14:12:13 -0700
Subject: [PATCH] clocksource: mct: Don't clear the mct
On exynos5 SoCs, the mct and the arch timers appear to share the same
root timer. ...so clearing the mct actually ends up clearing up the
arch timer and that confuses the heck out of the arch timer code.
Let's allow the arch timer and mct driver to coexist by just not ever
clearing the timer. Just in case someone cares, we'll still pretend
that we cleared it by keeping track of our own offset.
BUG=chrome-os-partner:13805
TEST=Arch timer no longers goes all screwy
Change-Id: I2c4bd3cb0fede67c36a2734b2972763f079f2872
Signed-off-by: Doug Anderson diand...@chromium.org
---
drivers/clocksource/exynos_mct.c | 42 +++-
1 file changed, 24 insertions(+), 18 deletions(-)
diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
index b067219..32b8008 100644
--- a/drivers/clocksource/exynos_mct.c
+++ b/drivers/clocksource/exynos_mct.c
@@ -93,6 +93,7 @@ static unsigned long clk_rate;
static unsigned int mct_int_type;
static int mct_irqs[MCT_NR_IRQS];
static int nr_mct_irqs;
+static cycle_t exynos_mct_offset;
static const
Re: [PATCH] arm: dts: exynos5: Remove multi core timer
On Wed, May 21, 2014 at 9:20 AM, Tomasz Figa t.f...@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 -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] arm: dts: exynos5: Remove multi core timer
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 -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] arm: dts: exynos5: Remove multi core timer
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 o...@lixom.net
Cc: Doug Anderson diand...@chromium.org
Cc: Kukjin Kim kgene@samsung.com
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-samsung-soc@vger.kernel.org
Signed-off-by: Chirantan Ekbote chiran...@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 = 2400;
};
- mct@101C {
- compatible = samsung,exynos4210-mct;
- reg = 0x101C 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@101C {
- compatible = samsung,exynos4210-mct;
- reg = 0x101C 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@10044000 {
compatible = samsung,exynos4210-pd;
reg = 0x10044000 0x20;
--
1.9.1.423.g4596e3a
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] arm: dts: exynos5: Remove multi core timer
Hi Tomasz, On Thu, May 15, 2014 at 3:44 PM, Doug Anderson diand...@chromium.org wrote: Tomasz, On Thu, May 15, 2014 at 3:13 PM, Tomasz Figa tomasz.f...@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 -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
