Dear All,
This patchset is an attempt to submit the last piece of missing code
to have proper support for Exynos5433 SoCs based TM2(e) boards. It
performs a cleanup of timer configuration, which so far needed various
out-of-tree workarounds. The fixes provided by this patchset are also
needed for add proper support for system suspend/resume.
Best regards
Marek Szyprowski
Samsung R&D Institute Poland
Changelog:
v2:
- dropped arch timer patch, it will be discussed separately
- fixed issues pointed by Krzysztof Kozlowski
v1: https://patchwork.kernel.org/project/linux-samsung-soc/list/?series=27965&state=*&archive=both
- initial version
Patch summary:
Marek Szyprowski (6):
clocksource: exynos_mct: Remove dead code
clocksource: exynos_mct: Fix error path in timer resources
initialization
clocksource: exynos_mct: Add arch_timer cooperation mode for ARM64
clocksource: Change CPU hotplug priority of exynos_mct driver
arm64: dts: exynos: Move arch-timer node to right place
arm64: platform: Add enable Exynos Multi-Core Timer driver
arch/arm64/Kconfig.platforms | 1 +
arch/arm64/boot/dts/exynos/exynos5433.dtsi | 23 ++++----
drivers/clocksource/exynos_mct.c | 68 +++++++++++++++-------
include/linux/cpuhotplug.h | 2 +-
4 files changed, 61 insertions(+), 33 deletions(-)
--
2.17.1
On Exynos SoCs enabling MCT driver is required even if ARM Architected
Timer driver is used to for managing timer hardware and clock source
events.
Signed-off-by: Marek Szyprowski <[email protected]>
---
arch/arm64/Kconfig.platforms | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
index 51bc479334a4..7cc687580fad 100644
--- a/arch/arm64/Kconfig.platforms
+++ b/arch/arm64/Kconfig.platforms
@@ -62,6 +62,7 @@ config ARCH_BRCMSTB
config ARCH_EXYNOS
bool "ARMv8 based Samsung Exynos SoC family"
select COMMON_CLK_SAMSUNG
+ select CLKSRC_EXYNOS_MCT
select EXYNOS_PM_DOMAINS if PM_GENERIC_DOMAINS
select EXYNOS_PMU
select HAVE_S3C2410_WATCHDOG if WATCHDOG
--
2.17.1
Move ARM architected timer device-tree node to the beginning of 'soc'
node, to group it together with other ARM CPU core devices (like PMU).
Signed-off-by: Marek Szyprowski <[email protected]>
---
arch/arm64/boot/dts/exynos/exynos5433.dtsi | 23 +++++++++++-----------
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/arch/arm64/boot/dts/exynos/exynos5433.dtsi b/arch/arm64/boot/dts/exynos/exynos5433.dtsi
index 2131f12364cb..fa20eb3495b3 100644
--- a/arch/arm64/boot/dts/exynos/exynos5433.dtsi
+++ b/arch/arm64/boot/dts/exynos/exynos5433.dtsi
@@ -255,6 +255,18 @@
interrupt-affinity = <&cpu4>, <&cpu5>, <&cpu6>, <&cpu7>;
};
+ timer: timer {
+ compatible = "arm,armv8-timer";
+ interrupts = <GIC_PPI 13
+ (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_HIGH)>,
+ <GIC_PPI 14
+ (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_HIGH)>,
+ <GIC_PPI 11
+ (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_HIGH)>,
+ <GIC_PPI 10
+ (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_HIGH)>;
+ };
+
chipid@10000000 {
compatible = "samsung,exynos4210-chipid";
reg = <0x10000000 0x100>;
@@ -1750,17 +1762,6 @@
};
};
- timer: timer {
- compatible = "arm,armv8-timer";
- interrupts = <GIC_PPI 13
- (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_HIGH)>,
- <GIC_PPI 14
- (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_HIGH)>,
- <GIC_PPI 11
- (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_HIGH)>,
- <GIC_PPI 10
- (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_HIGH)>;
- };
};
#include "exynos5433-bus.dtsi"
--
2.17.1
While freeing interrupt handlers in error path, don't assume that all
requested interrupts are per-processor interrupts and properly release
standard interrupts too.
Suggested-by: Krzysztof Kozlowski <[email protected]>
Fixes: 56a94f13919c ("clocksource: exynos_mct: Avoid blocking calls in the cpu hotplug notifier")
Signed-off-by: Marek Szyprowski <[email protected]>
---
drivers/clocksource/exynos_mct.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
index 43b335ff4a96..a379f11fad2d 100644
--- a/drivers/clocksource/exynos_mct.c
+++ b/drivers/clocksource/exynos_mct.c
@@ -562,7 +562,19 @@ static int __init exynos4_timer_resources(struct device_node *np, void __iomem *
return 0;
out_irq:
- free_percpu_irq(mct_irqs[MCT_L0_IRQ], &percpu_mct_tick);
+ if (mct_int_type == MCT_INT_PPI) {
+ free_percpu_irq(mct_irqs[MCT_L0_IRQ], &percpu_mct_tick);
+ } else {
+ for_each_possible_cpu(cpu) {
+ struct mct_clock_event_device *pcpu_mevt =
+ per_cpu_ptr(&percpu_mct_tick, cpu);
+
+ if (pcpu_mevt->evt.irq != -1) {
+ free_irq(pcpu_mevt->evt.irq, pcpu_mevt);
+ pcpu_mevt->evt.irq = -1;
+ }
+ }
+ }
return err;
}
--
2.17.1
To get ARM Architected Timers working on Samsung Exynos SoCs, one has to
first configure and enable Exynos Multi-Core Timer, because they both
share some common hardware blocks. This patch adds a mode of cooperation
with arch_timer driver, so kernel can use CP15 based timer interface via
arch_timer driver, which is mandatory on ARM64. In such mode driver only
configures MCT registers and starts the timer but don't register any
clocksource or events in the system. Those are left to be handled by
arch_timer driver.
Signed-off-by: Marek Szyprowski <[email protected]>
---
drivers/clocksource/exynos_mct.c | 52 +++++++++++++++++++++-----------
1 file changed, 35 insertions(+), 17 deletions(-)
diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
index a379f11fad2d..06cd30a6d59a 100644
--- a/drivers/clocksource/exynos_mct.c
+++ b/drivers/clocksource/exynos_mct.c
@@ -57,6 +57,7 @@
#define TICK_BASE_CNT 1
enum {
+ MCT_INT_NONE = 0,
MCT_INT_SPI,
MCT_INT_PPI
};
@@ -238,6 +239,9 @@ static int __init exynos4_clocksource_init(void)
{
exynos4_mct_frc_start();
+ if (!mct_int_type)
+ return 0;
+
#if defined(CONFIG_ARM)
exynos4_delay_timer.read_current_timer = &exynos4_read_current_timer;
exynos4_delay_timer.freq = clk_rate;
@@ -343,6 +347,9 @@ static struct irqaction mct_comp_event_irq = {
static int exynos4_clockevent_init(void)
{
+ if (!mct_int_type)
+ return 0;
+
mct_comp_device.cpumask = cpumask_of(0);
clockevents_config_and_register(&mct_comp_device, clk_rate,
0xf, 0xffffffff);
@@ -476,12 +483,12 @@ static int exynos4_mct_starting_cpu(unsigned int cpu)
irq_force_affinity(evt->irq, cpumask_of(cpu));
enable_irq(evt->irq);
- } else {
+ } else if (mct_int_type == MCT_INT_PPI) {
enable_percpu_irq(mct_irqs[MCT_L0_IRQ], 0);
}
- clockevents_config_and_register(evt, clk_rate / (TICK_BASE_CNT + 1),
- 0xf, 0x7fffffff);
-
+ if (mct_int_type)
+ clockevents_config_and_register(evt,
+ clk_rate / (TICK_BASE_CNT + 1), 0xf, 0x7fffffff);
return 0;
}
@@ -496,7 +503,7 @@ static int exynos4_mct_dying_cpu(unsigned int cpu)
if (evt->irq != -1)
disable_irq_nosync(evt->irq);
exynos4_mct_write(0x1, mevt->base + MCT_L_INT_CSTAT_OFFSET);
- } else {
+ } else if (mct_int_type == MCT_INT_PPI) {
disable_percpu_irq(mct_irqs[MCT_L0_IRQ]);
}
return 0;
@@ -529,7 +536,7 @@ static int __init exynos4_timer_resources(struct device_node *np, void __iomem *
&percpu_mct_tick);
WARN(err, "MCT: can't request IRQ %d (%d)\n",
mct_irqs[MCT_L0_IRQ], err);
- } else {
+ } else if (mct_int_type == MCT_INT_SPI) {
for_each_possible_cpu(cpu) {
int mct_irq = mct_irqs[MCT_L0_IRQ + cpu];
struct mct_clock_event_device *pcpu_mevt =
@@ -564,7 +571,7 @@ static int __init exynos4_timer_resources(struct device_node *np, void __iomem *
out_irq:
if (mct_int_type == MCT_INT_PPI) {
free_percpu_irq(mct_irqs[MCT_L0_IRQ], &percpu_mct_tick);
- } else {
+ } else if (mct_int_type == MCT_INT_SPI) {
for_each_possible_cpu(cpu) {
struct mct_clock_event_device *pcpu_mevt =
per_cpu_ptr(&percpu_mct_tick, cpu);
@@ -585,17 +592,28 @@ static int __init mct_init_dt(struct device_node *np, unsigned int int_type)
mct_int_type = int_type;
- /* This driver uses only one global timer interrupt */
- mct_irqs[MCT_G0_IRQ] = irq_of_parse_and_map(np, MCT_G0_IRQ);
+ if (IS_ENABLED(CONFIG_ARM64) && IS_ENABLED(CONFIG_ARM_ARCH_TIMER)) {
+ struct device_node *np = of_find_compatible_node(NULL, NULL,
+ "arm,armv8-timer");
+ if (np) {
+ mct_int_type = MCT_INT_NONE;
+ of_node_put(np);
+ }
+ }
- /*
- * Find out the number of local irqs specified. The local
- * timer irqs are specified after the four global timer
- * irqs are specified.
- */
- nr_irqs = of_irq_count(np);
- for (i = MCT_L0_IRQ; i < nr_irqs; i++)
- mct_irqs[i] = irq_of_parse_and_map(np, i);
+ if (mct_int_type) {
+ /* This driver uses only one global timer interrupt */
+ mct_irqs[MCT_G0_IRQ] = irq_of_parse_and_map(np, MCT_G0_IRQ);
+
+ /*
+ * Find out the number of local irqs specified. The local
+ * timer irqs are specified after the four global timer
+ * irqs are specified.
+ */
+ nr_irqs = of_irq_count(np);
+ for (i = MCT_L0_IRQ; i < nr_irqs; i++)
+ mct_irqs[i] = irq_of_parse_and_map(np, i);
+ }
ret = exynos4_timer_resources(np, of_iomap(np, 0));
if (ret)
--
2.17.1
Exynos Multi-Core Timer driver is used only on device-tree based
systems, so remove non-dt related code. In case of !CONFIG_OF
the code is anyway equal because of_irq_count() has a stub
returning 0.
Signed-off-by: Marek Szyprowski <[email protected]>
Reviewed-by: Krzysztof Kozlowski <[email protected]>
---
drivers/clocksource/exynos_mct.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
index 7a244b681876..43b335ff4a96 100644
--- a/drivers/clocksource/exynos_mct.c
+++ b/drivers/clocksource/exynos_mct.c
@@ -581,11 +581,7 @@ static int __init mct_init_dt(struct device_node *np, unsigned int int_type)
* timer irqs are specified after the four global timer
* irqs are specified.
*/
-#ifdef CONFIG_OF
nr_irqs = of_irq_count(np);
-#else
- nr_irqs = 0;
-#endif
for (i = MCT_L0_IRQ; i < nr_irqs; i++)
mct_irqs[i] = irq_of_parse_and_map(np, i);
--
2.17.1
Exynos Multi-Core Timer driver (exynos_mct) must be started before
ARM Architected Timers (arch_timer), because both timers share common
hardware block and turning on MCT is needed to get ARM Architected
Timer working properly.
Signed-off-by: Marek Szyprowski <[email protected]>
Acked-by: Krzysztof Kozlowski <[email protected]>
---
include/linux/cpuhotplug.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index caf40ad0bbc6..5d9e4a6ea299 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -115,10 +115,10 @@ enum cpuhp_state {
CPUHP_AP_PERF_ARM_ACPI_STARTING,
CPUHP_AP_PERF_ARM_STARTING,
CPUHP_AP_ARM_L2X0_STARTING,
+ CPUHP_AP_EXYNOS4_MCT_TIMER_STARTING,
CPUHP_AP_ARM_ARCH_TIMER_STARTING,
CPUHP_AP_ARM_GLOBAL_TIMER_STARTING,
CPUHP_AP_JCORE_TIMER_STARTING,
- CPUHP_AP_EXYNOS4_MCT_TIMER_STARTING,
CPUHP_AP_ARM_TWD_STARTING,
CPUHP_AP_QCOM_TIMER_STARTING,
CPUHP_AP_ARMADA_TIMER_STARTING,
--
2.17.1
On Mon, Oct 15, 2018 at 02:31:09PM +0200, Marek Szyprowski wrote:
> To get ARM Architected Timers working on Samsung Exynos SoCs, one has to
> first configure and enable Exynos Multi-Core Timer, because they both
> share some common hardware blocks.
Could you please elaborate on what exactly is shared with the MCT?
Architecturally, the OS shouldn't have to do anything to put the timers
into a usable state. All instances should be fed (directly) from the
system counter, which FW is tasked with configuring and enabling, and
all other state is private to the instance.
If we have to poke things to make them usable, that means we can't
assume that it's always safe to use the timers or counters, as the
architecture lets us, and I'd like to understand what the impact is.
e.g. does this mean that there are windows where the counters don't
tick?
Are all the counters fed the same underlying counter value? ... or are
those independent?
Thanks,
Mark.
> This patch adds a mode of cooperation with arch_timer driver, so
> kernel can use CP15 based timer interface via arch_timer driver, which
> is mandatory on ARM64. In such mode driver only configures MCT
> registers and starts the timer but don't register any clocksource or
> events in the system. Those are left to be handled by arch_timer
> driver.
>
> Signed-off-by: Marek Szyprowski <[email protected]>
> ---
> drivers/clocksource/exynos_mct.c | 52 +++++++++++++++++++++-----------
> 1 file changed, 35 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
> index a379f11fad2d..06cd30a6d59a 100644
> --- a/drivers/clocksource/exynos_mct.c
> +++ b/drivers/clocksource/exynos_mct.c
> @@ -57,6 +57,7 @@
> #define TICK_BASE_CNT 1
>
> enum {
> + MCT_INT_NONE = 0,
> MCT_INT_SPI,
> MCT_INT_PPI
> };
> @@ -238,6 +239,9 @@ static int __init exynos4_clocksource_init(void)
> {
> exynos4_mct_frc_start();
>
> + if (!mct_int_type)
> + return 0;
> +
> #if defined(CONFIG_ARM)
> exynos4_delay_timer.read_current_timer = &exynos4_read_current_timer;
> exynos4_delay_timer.freq = clk_rate;
> @@ -343,6 +347,9 @@ static struct irqaction mct_comp_event_irq = {
>
> static int exynos4_clockevent_init(void)
> {
> + if (!mct_int_type)
> + return 0;
> +
> mct_comp_device.cpumask = cpumask_of(0);
> clockevents_config_and_register(&mct_comp_device, clk_rate,
> 0xf, 0xffffffff);
> @@ -476,12 +483,12 @@ static int exynos4_mct_starting_cpu(unsigned int cpu)
>
> irq_force_affinity(evt->irq, cpumask_of(cpu));
> enable_irq(evt->irq);
> - } else {
> + } else if (mct_int_type == MCT_INT_PPI) {
> enable_percpu_irq(mct_irqs[MCT_L0_IRQ], 0);
> }
> - clockevents_config_and_register(evt, clk_rate / (TICK_BASE_CNT + 1),
> - 0xf, 0x7fffffff);
> -
> + if (mct_int_type)
> + clockevents_config_and_register(evt,
> + clk_rate / (TICK_BASE_CNT + 1), 0xf, 0x7fffffff);
> return 0;
> }
>
> @@ -496,7 +503,7 @@ static int exynos4_mct_dying_cpu(unsigned int cpu)
> if (evt->irq != -1)
> disable_irq_nosync(evt->irq);
> exynos4_mct_write(0x1, mevt->base + MCT_L_INT_CSTAT_OFFSET);
> - } else {
> + } else if (mct_int_type == MCT_INT_PPI) {
> disable_percpu_irq(mct_irqs[MCT_L0_IRQ]);
> }
> return 0;
> @@ -529,7 +536,7 @@ static int __init exynos4_timer_resources(struct device_node *np, void __iomem *
> &percpu_mct_tick);
> WARN(err, "MCT: can't request IRQ %d (%d)\n",
> mct_irqs[MCT_L0_IRQ], err);
> - } else {
> + } else if (mct_int_type == MCT_INT_SPI) {
> for_each_possible_cpu(cpu) {
> int mct_irq = mct_irqs[MCT_L0_IRQ + cpu];
> struct mct_clock_event_device *pcpu_mevt =
> @@ -564,7 +571,7 @@ static int __init exynos4_timer_resources(struct device_node *np, void __iomem *
> out_irq:
> if (mct_int_type == MCT_INT_PPI) {
> free_percpu_irq(mct_irqs[MCT_L0_IRQ], &percpu_mct_tick);
> - } else {
> + } else if (mct_int_type == MCT_INT_SPI) {
> for_each_possible_cpu(cpu) {
> struct mct_clock_event_device *pcpu_mevt =
> per_cpu_ptr(&percpu_mct_tick, cpu);
> @@ -585,17 +592,28 @@ static int __init mct_init_dt(struct device_node *np, unsigned int int_type)
>
> mct_int_type = int_type;
>
> - /* This driver uses only one global timer interrupt */
> - mct_irqs[MCT_G0_IRQ] = irq_of_parse_and_map(np, MCT_G0_IRQ);
> + if (IS_ENABLED(CONFIG_ARM64) && IS_ENABLED(CONFIG_ARM_ARCH_TIMER)) {
> + struct device_node *np = of_find_compatible_node(NULL, NULL,
> + "arm,armv8-timer");
> + if (np) {
> + mct_int_type = MCT_INT_NONE;
> + of_node_put(np);
> + }
> + }
>
> - /*
> - * Find out the number of local irqs specified. The local
> - * timer irqs are specified after the four global timer
> - * irqs are specified.
> - */
> - nr_irqs = of_irq_count(np);
> - for (i = MCT_L0_IRQ; i < nr_irqs; i++)
> - mct_irqs[i] = irq_of_parse_and_map(np, i);
> + if (mct_int_type) {
> + /* This driver uses only one global timer interrupt */
> + mct_irqs[MCT_G0_IRQ] = irq_of_parse_and_map(np, MCT_G0_IRQ);
> +
> + /*
> + * Find out the number of local irqs specified. The local
> + * timer irqs are specified after the four global timer
> + * irqs are specified.
> + */
> + nr_irqs = of_irq_count(np);
> + for (i = MCT_L0_IRQ; i < nr_irqs; i++)
> + mct_irqs[i] = irq_of_parse_and_map(np, i);
> + }
>
> ret = exynos4_timer_resources(np, of_iomap(np, 0));
> if (ret)
> --
> 2.17.1
>
On Mon, 15 Oct 2018 at 14:31, Marek Szyprowski <[email protected]> wrote:
>
> While freeing interrupt handlers in error path, don't assume that all
> requested interrupts are per-processor interrupts and properly release
> standard interrupts too.
Thanks for fixing!
> Suggested-by: Krzysztof Kozlowski <[email protected]>
It is a bug so how about:
Reported-by: Krzysztof Kozlowski <[email protected]>
?
> Fixes: 56a94f13919c ("clocksource: exynos_mct: Avoid blocking calls in the cpu hotplug notifier")
> Signed-off-by: Marek Szyprowski <[email protected]>
Anyway I am fine so:
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
> ---
> drivers/clocksource/exynos_mct.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
> index 43b335ff4a96..a379f11fad2d 100644
> --- a/drivers/clocksource/exynos_mct.c
> +++ b/drivers/clocksource/exynos_mct.c
> @@ -562,7 +562,19 @@ static int __init exynos4_timer_resources(struct device_node *np, void __iomem *
> return 0;
>
> out_irq:
> - free_percpu_irq(mct_irqs[MCT_L0_IRQ], &percpu_mct_tick);
> + if (mct_int_type == MCT_INT_PPI) {
> + free_percpu_irq(mct_irqs[MCT_L0_IRQ], &percpu_mct_tick);
> + } else {
> + for_each_possible_cpu(cpu) {
> + struct mct_clock_event_device *pcpu_mevt =
> + per_cpu_ptr(&percpu_mct_tick, cpu);
> +
> + if (pcpu_mevt->evt.irq != -1) {
> + free_irq(pcpu_mevt->evt.irq, pcpu_mevt);
> + pcpu_mevt->evt.irq = -1;
> + }
> + }
> + }
> return err;
> }
>
> --
> 2.17.1
>
On 2018년 10월 15일 21:31, Marek Szyprowski wrote:
> Exynos Multi-Core Timer driver is used only on device-tree based
> systems, so remove non-dt related code. In case of !CONFIG_OF
> the code is anyway equal because of_irq_count() has a stub
> returning 0.
>
> Signed-off-by: Marek Szyprowski <[email protected]>
> Reviewed-by: Krzysztof Kozlowski <[email protected]>
> ---
> drivers/clocksource/exynos_mct.c | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
> index 7a244b681876..43b335ff4a96 100644
> --- a/drivers/clocksource/exynos_mct.c
> +++ b/drivers/clocksource/exynos_mct.c
> @@ -581,11 +581,7 @@ static int __init mct_init_dt(struct device_node *np, unsigned int int_type)
> * timer irqs are specified after the four global timer
> * irqs are specified.
> */
> -#ifdef CONFIG_OF
> nr_irqs = of_irq_count(np);
> -#else
> - nr_irqs = 0;
> -#endif
> for (i = MCT_L0_IRQ; i < nr_irqs; i++)
> mct_irqs[i] = irq_of_parse_and_map(np, i);
>
>
I agree that Exynos MCT timer is only used by device-tree.
Reviewed-by: Chanwoo Choi <[email protected]>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
On 2018년 10월 15일 21:31, Marek Szyprowski wrote:
> Exynos Multi-Core Timer driver (exynos_mct) must be started before
> ARM Architected Timers (arch_timer), because both timers share common
> hardware block and turning on MCT is needed to get ARM Architected
> Timer working properly.
>
> Signed-off-by: Marek Szyprowski <[email protected]>
> Acked-by: Krzysztof Kozlowski <[email protected]>
> ---
> include/linux/cpuhotplug.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
> index caf40ad0bbc6..5d9e4a6ea299 100644
> --- a/include/linux/cpuhotplug.h
> +++ b/include/linux/cpuhotplug.h
> @@ -115,10 +115,10 @@ enum cpuhp_state {
> CPUHP_AP_PERF_ARM_ACPI_STARTING,
> CPUHP_AP_PERF_ARM_STARTING,
> CPUHP_AP_ARM_L2X0_STARTING,
> + CPUHP_AP_EXYNOS4_MCT_TIMER_STARTING,
> CPUHP_AP_ARM_ARCH_TIMER_STARTING,
> CPUHP_AP_ARM_GLOBAL_TIMER_STARTING,
> CPUHP_AP_JCORE_TIMER_STARTING,
> - CPUHP_AP_EXYNOS4_MCT_TIMER_STARTING,
> CPUHP_AP_ARM_TWD_STARTING,
> CPUHP_AP_QCOM_TIMER_STARTING,
> CPUHP_AP_ARMADA_TIMER_STARTING,
>
On Exynos SoC, ARM architecture timer shares the block of Exynos MCT timer.
For using arch_timer, Exynos MCT timer should be initialized before arch_timer.
I agree about this.
Reviewed-by: Chanwoo Choi <[email protected]>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
Dear Marek,
On 2018년 10월 15일 21:31, Marek Szyprowski wrote:
> While freeing interrupt handlers in error path, don't assume that all
> requested interrupts are per-processor interrupts and properly release
> standard interrupts too.
>
> Suggested-by: Krzysztof Kozlowski <[email protected]>
> Fixes: 56a94f13919c ("clocksource: exynos_mct: Avoid blocking calls in the cpu hotplug notifier")
> Signed-off-by: Marek Szyprowski <[email protected]>
> ---
> drivers/clocksource/exynos_mct.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
> index 43b335ff4a96..a379f11fad2d 100644
> --- a/drivers/clocksource/exynos_mct.c
> +++ b/drivers/clocksource/exynos_mct.c
> @@ -562,7 +562,19 @@ static int __init exynos4_timer_resources(struct device_node *np, void __iomem *
> return 0;
>
> out_irq:
> - free_percpu_irq(mct_irqs[MCT_L0_IRQ], &percpu_mct_tick);
> + if (mct_int_type == MCT_INT_PPI) {
> + free_percpu_irq(mct_irqs[MCT_L0_IRQ], &percpu_mct_tick);
> + } else {
> + for_each_possible_cpu(cpu) {
> + struct mct_clock_event_device *pcpu_mevt =
> + per_cpu_ptr(&percpu_mct_tick, cpu);
> +
> + if (pcpu_mevt->evt.irq != -1) {
> + free_irq(pcpu_mevt->evt.irq, pcpu_mevt);
> + pcpu_mevt->evt.irq = -1;
> + }
> + }
> + }
> return err;
> }
>
>
Reviewed-by: Chanwoo Choi <[email protected]>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
On Mon, 15 Oct 2018 at 14:31, Marek Szyprowski <[email protected]> wrote:
>
> Move ARM architected timer device-tree node to the beginning of 'soc'
> node, to group it together with other ARM CPU core devices (like PMU).
>
> Signed-off-by: Marek Szyprowski <[email protected]>
> ---
> arch/arm64/boot/dts/exynos/exynos5433.dtsi | 23 +++++++++++-----------
> 1 file changed, 12 insertions(+), 11 deletions(-)
Looks okay, I'll take it after this merge window.
Best regards,
Krzysztof
On Mon, 15 Oct 2018 at 14:31, Marek Szyprowski <[email protected]> wrote:
>
> On Exynos SoCs enabling MCT driver is required even if ARM Architected
> Timer driver is used to for managing timer hardware and clock source
> events.
>
> Signed-off-by: Marek Szyprowski <[email protected]>
> ---
> arch/arm64/Kconfig.platforms | 1 +
> 1 file changed, 1 insertion(+)
Looks okay, I'll take it after this merge window.
In case this was applied directly to arm-soc:
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On Mon, 15 Oct 2018 at 14:31, Marek Szyprowski <[email protected]> wrote:
>
> To get ARM Architected Timers working on Samsung Exynos SoCs, one has to
> first configure and enable Exynos Multi-Core Timer, because they both
> share some common hardware blocks. This patch adds a mode of cooperation
> with arch_timer driver, so kernel can use CP15 based timer interface via
> arch_timer driver, which is mandatory on ARM64. In such mode driver only
> configures MCT registers and starts the timer but don't register any
> clocksource or events in the system. Those are left to be handled by
> arch_timer driver.
>
> Signed-off-by: Marek Szyprowski <[email protected]>
> ---
> drivers/clocksource/exynos_mct.c | 52 +++++++++++++++++++++-----------
> 1 file changed, 35 insertions(+), 17 deletions(-)
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
Hi Marek,
I tested it about kernel booting and CPU hotplug through sysfs path
on ARM64 Exynos5433-based TM2 board. It is well working.
Reviewed-by: Chanwoo Choi <[email protected]>
Tested-by: Chanwoo Choi <[email protected]>
But,
I have a minor comment for code clean-up.
On exynos4_mct_starting_cpu() function, the initialization of 'evt' instance
are not mandatory because 'evt' instance is not registered
by clockevents_config_and_register().
On 2018년 10월 15일 21:31, Marek Szyprowski wrote:
> To get ARM Architected Timers working on Samsung Exynos SoCs, one has to
> first configure and enable Exynos Multi-Core Timer, because they both
> share some common hardware blocks. This patch adds a mode of cooperation
> with arch_timer driver, so kernel can use CP15 based timer interface via
> arch_timer driver, which is mandatory on ARM64. In such mode driver only
> configures MCT registers and starts the timer but don't register any
> clocksource or events in the system. Those are left to be handled by
> arch_timer driver.
>
> Signed-off-by: Marek Szyprowski <[email protected]>
> ---
> drivers/clocksource/exynos_mct.c | 52 +++++++++++++++++++++-----------
> 1 file changed, 35 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
> index a379f11fad2d..06cd30a6d59a 100644
> --- a/drivers/clocksource/exynos_mct.c
> +++ b/drivers/clocksource/exynos_mct.c
> @@ -57,6 +57,7 @@
> #define TICK_BASE_CNT 1
>
> enum {
> + MCT_INT_NONE = 0,
> MCT_INT_SPI,
> MCT_INT_PPI
> };
> @@ -238,6 +239,9 @@ static int __init exynos4_clocksource_init(void)
> {
> exynos4_mct_frc_start();
>
> + if (!mct_int_type)
> + return 0;
> +
> #if defined(CONFIG_ARM)
> exynos4_delay_timer.read_current_timer = &exynos4_read_current_timer;
> exynos4_delay_timer.freq = clk_rate;
> @@ -343,6 +347,9 @@ static struct irqaction mct_comp_event_irq = {
>
> static int exynos4_clockevent_init(void)
> {
> + if (!mct_int_type)
> + return 0;
> +
> mct_comp_device.cpumask = cpumask_of(0);
> clockevents_config_and_register(&mct_comp_device, clk_rate,
> 0xf, 0xffffffff);
> @@ -476,12 +483,12 @@ static int exynos4_mct_starting_cpu(unsigned int cpu)
>
> irq_force_affinity(evt->irq, cpumask_of(cpu));
> enable_irq(evt->irq);
> - } else {
> + } else if (mct_int_type == MCT_INT_PPI) {
> enable_percpu_irq(mct_irqs[MCT_L0_IRQ], 0);
> }
> - clockevents_config_and_register(evt, clk_rate / (TICK_BASE_CNT + 1),
> - 0xf, 0x7fffffff);
> -
> + if (mct_int_type)
> + clockevents_config_and_register(evt,
> + clk_rate / (TICK_BASE_CNT + 1), 0xf, 0x7fffffff);
> return 0;
> }
>
> @@ -496,7 +503,7 @@ static int exynos4_mct_dying_cpu(unsigned int cpu)
> if (evt->irq != -1)
> disable_irq_nosync(evt->irq);
> exynos4_mct_write(0x1, mevt->base + MCT_L_INT_CSTAT_OFFSET);
> - } else {
> + } else if (mct_int_type == MCT_INT_PPI) {
> disable_percpu_irq(mct_irqs[MCT_L0_IRQ]);
> }
> return 0;
> @@ -529,7 +536,7 @@ static int __init exynos4_timer_resources(struct device_node *np, void __iomem *
> &percpu_mct_tick);
> WARN(err, "MCT: can't request IRQ %d (%d)\n",
> mct_irqs[MCT_L0_IRQ], err);
> - } else {
> + } else if (mct_int_type == MCT_INT_SPI) {
> for_each_possible_cpu(cpu) {
> int mct_irq = mct_irqs[MCT_L0_IRQ + cpu];
> struct mct_clock_event_device *pcpu_mevt =
> @@ -564,7 +571,7 @@ static int __init exynos4_timer_resources(struct device_node *np, void __iomem *
> out_irq:
> if (mct_int_type == MCT_INT_PPI) {
> free_percpu_irq(mct_irqs[MCT_L0_IRQ], &percpu_mct_tick);
> - } else {
> + } else if (mct_int_type == MCT_INT_SPI) {
> for_each_possible_cpu(cpu) {
> struct mct_clock_event_device *pcpu_mevt =
> per_cpu_ptr(&percpu_mct_tick, cpu);
> @@ -585,17 +592,28 @@ static int __init mct_init_dt(struct device_node *np, unsigned int int_type)
>
> mct_int_type = int_type;
>
> - /* This driver uses only one global timer interrupt */
> - mct_irqs[MCT_G0_IRQ] = irq_of_parse_and_map(np, MCT_G0_IRQ);
> + if (IS_ENABLED(CONFIG_ARM64) && IS_ENABLED(CONFIG_ARM_ARCH_TIMER)) {
> + struct device_node *np = of_find_compatible_node(NULL, NULL,
> + "arm,armv8-timer");
> + if (np) {
> + mct_int_type = MCT_INT_NONE;
> + of_node_put(np);
> + }
> + }
>
> - /*
> - * Find out the number of local irqs specified. The local
> - * timer irqs are specified after the four global timer
> - * irqs are specified.
> - */
> - nr_irqs = of_irq_count(np);
> - for (i = MCT_L0_IRQ; i < nr_irqs; i++)
> - mct_irqs[i] = irq_of_parse_and_map(np, i);
> + if (mct_int_type) {
> + /* This driver uses only one global timer interrupt */
> + mct_irqs[MCT_G0_IRQ] = irq_of_parse_and_map(np, MCT_G0_IRQ);
> +
> + /*
> + * Find out the number of local irqs specified. The local
> + * timer irqs are specified after the four global timer
> + * irqs are specified.
> + */
> + nr_irqs = of_irq_count(np);
> + for (i = MCT_L0_IRQ; i < nr_irqs; i++)
> + mct_irqs[i] = irq_of_parse_and_map(np, i);
> + }
>
> ret = exynos4_timer_resources(np, of_iomap(np, 0));
> if (ret)
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
Hi Marek,
On 2018년 10월 15일 21:31, Marek Szyprowski wrote:
> Move ARM architected timer device-tree node to the beginning of 'soc'
> node, to group it together with other ARM CPU core devices (like PMU).
>
> Signed-off-by: Marek Szyprowski <[email protected]>
> ---
> arch/arm64/boot/dts/exynos/exynos5433.dtsi | 23 +++++++++++-----------
> 1 file changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/exynos/exynos5433.dtsi b/arch/arm64/boot/dts/exynos/exynos5433.dtsi
> index 2131f12364cb..fa20eb3495b3 100644
> --- a/arch/arm64/boot/dts/exynos/exynos5433.dtsi
> +++ b/arch/arm64/boot/dts/exynos/exynos5433.dtsi
> @@ -255,6 +255,18 @@
> interrupt-affinity = <&cpu4>, <&cpu5>, <&cpu6>, <&cpu7>;
> };
>
> + timer: timer {
> + compatible = "arm,armv8-timer";
> + interrupts = <GIC_PPI 13
> + (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_HIGH)>,
> + <GIC_PPI 14
> + (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_HIGH)>,
> + <GIC_PPI 11
> + (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_HIGH)>,
> + <GIC_PPI 10
> + (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_HIGH)>;
> + };
> +
> chipid@10000000 {
> compatible = "samsung,exynos4210-chipid";
> reg = <0x10000000 0x100>;
> @@ -1750,17 +1762,6 @@
> };
> };
>
> - timer: timer {
> - compatible = "arm,armv8-timer";
> - interrupts = <GIC_PPI 13
> - (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_HIGH)>,
> - <GIC_PPI 14
> - (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_HIGH)>,
> - <GIC_PPI 11
> - (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_HIGH)>,
> - <GIC_PPI 10
> - (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_HIGH)>;
> - };
> };
>
> #include "exynos5433-bus.dtsi"
>
I tested it on Exynos5433-based TM2 board. It is well working.
Reviewed-by: Chanwoo Choi <[email protected]>
Tested-by: Chanwoo Choi <[email protected]>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
Hi Marek,
On 2018년 10월 15일 21:31, Marek Szyprowski wrote:
> On Exynos SoCs enabling MCT driver is required even if ARM Architected
> Timer driver is used to for managing timer hardware and clock source
> events.
>
> Signed-off-by: Marek Szyprowski <[email protected]>
> ---
> arch/arm64/Kconfig.platforms | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
> index 51bc479334a4..7cc687580fad 100644
> --- a/arch/arm64/Kconfig.platforms
> +++ b/arch/arm64/Kconfig.platforms
> @@ -62,6 +62,7 @@ config ARCH_BRCMSTB
> config ARCH_EXYNOS
> bool "ARMv8 based Samsung Exynos SoC family"
> select COMMON_CLK_SAMSUNG
> + select CLKSRC_EXYNOS_MCT
> select EXYNOS_PM_DOMAINS if PM_GENERIC_DOMAINS
> select EXYNOS_PMU
> select HAVE_S3C2410_WATCHDOG if WATCHDOG
>
I tested it on Exynos5433-based TM2 board.
Reviewed-by: Chanwoo Choi <[email protected]>
Tested-by: Chanwoo Choi <[email protected]>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
Hi Mark,
On 2018-10-15 15:26, Mark Rutland wrote:
> On Mon, Oct 15, 2018 at 02:31:09PM +0200, Marek Szyprowski wrote:
>> To get ARM Architected Timers working on Samsung Exynos SoCs, one has to
>> first configure and enable Exynos Multi-Core Timer, because they both
>> share some common hardware blocks.
> Could you please elaborate on what exactly is shared with the MCT?
>
> Architecturally, the OS shouldn't have to do anything to put the timers
> into a usable state. All instances should be fed (directly) from the
> system counter, which FW is tasked with configuring and enabling, and
> all other state is private to the instance.
>
> If we have to poke things to make them usable, that means we can't
> assume that it's always safe to use the timers or counters, as the
> architecture lets us, and I'd like to understand what the impact is.
>
> e.g. does this mean that there are windows where the counters don't
> tick?
Good question is always a helpful advice. I don't have such hardware
details and I've did my patches basing on what I've observed while
playing with the hardware.
I've checked again and I found that the only things that need to be
done to get ARM arch timer working are:
1. enable multi core timer clock,
2. set MCT_G_TCON_START (Timer enable) bit in MCT_G_TCON (Global timer
control) register.
Otherwise arch timer doesn't get any tick.
Changing CPUHP priorities nor any other MCT register writes are not
needed. It looks that they were leftovers from my older approaches
and I've kept them without really checking if they are needed in the
final version.
I will send a new simplified version of this patchset then.
> Are all the counters fed the same underlying counter value? ... or are
> those independent?
My summary above suggests that ARM architected timers are fed from the
physical counter, which is controlled by MCT registers.
> ...
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland