[PATCH v2 0/6] Proper arch timer support for Exynos5433-based TM2(e) boards

[PATCH v2 0/6] Proper arch timer support for Exynos5433-based TM2(e) boards

[Marek Szyprowski]

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

[PATCH v2 1/6] clocksource: exynos_mct: Remove dead code

[Marek Szyprowski]

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 <m.szyprowski@samsung.com>
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 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

[PATCH v2 2/6] clocksource: exynos_mct: Fix error path in timer resources initialization

[Marek Szyprowski]

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 <krzk@kernel.org>
Fixes: 56a94f13919c ("clocksource: exynos_mct: Avoid blocking calls in the cpu hotplug notifier")
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 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

[PATCH v2 3/6] clocksource: exynos_mct: Add arch_timer cooperation mode for ARM64

[Marek Szyprowski]

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 <m.szyprowski@samsung.com>
---
 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

[PATCH v2 4/6] clocksource: Change CPU hotplug priority of exynos_mct driver

[Marek Szyprowski]

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 <m.szyprowski@samsung.com>
Acked-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 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

[PATCH v2 5/6] arm64: dts: exynos: Move arch-timer node to right place

[Marek Szyprowski]

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 <m.szyprowski@samsung.com>
---
 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

[PATCH v2 6/6] arm64: platform: Add enable Exynos Multi-Core Timer driver

[Marek Szyprowski]

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 <m.szyprowski@samsung.com>
---
 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

Re: [PATCH v2 3/6] clocksource: exynos_mct: Add arch_timer cooperation mode for ARM64

[Mark Rutland]

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 <m.szyprowski@samsung.com>
> ---
>  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
> 

Re: [PATCH v2 2/6] clocksource: exynos_mct: Fix error path in timer resources initialization

[Krzysztof Kozlowski]

On Mon, 15 Oct 2018 at 14:31, Marek Szyprowski <m.szyprowski@samsung.com> 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 <krzk@kernel.org>

It is a bug so how about:
Reported-by: Krzysztof Kozlowski <krzk@kernel.org>
?

> Fixes: 56a94f13919c ("clocksource: exynos_mct: Avoid blocking calls in the cpu hotplug notifier")
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

Anyway I am fine so:
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

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
>

Re: [PATCH v2 1/6] clocksource: exynos_mct: Remove dead code

[Chanwoo Choi]

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 <m.szyprowski@samsung.com>
> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
> ---
>  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 <cw00.choi@samsung.com>

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2 4/6] clocksource: Change CPU hotplug priority of exynos_mct driver

[Chanwoo Choi]

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 <m.szyprowski@samsung.com>
> Acked-by: Krzysztof Kozlowski <krzk@kernel.org>
> ---
>  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 <cw00.choi@samsung.com>

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2 2/6] clocksource: exynos_mct: Fix error path in timer resources initialization

[Chanwoo Choi]

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 <krzk@kernel.org>
> Fixes: 56a94f13919c ("clocksource: exynos_mct: Avoid blocking calls in the cpu hotplug notifier")
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  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 <cw00.choi@samsung.com>

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2 5/6] arm64: dts: exynos: Move arch-timer node to right place

[Krzysztof Kozlowski]

On Mon, 15 Oct 2018 at 14:31, Marek Szyprowski <m.szyprowski@samsung.com> 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 <m.szyprowski@samsung.com>
> ---
>  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

Re: [PATCH v2 6/6] arm64: platform: Add enable Exynos Multi-Core Timer driver

[Krzysztof Kozlowski]

On Mon, 15 Oct 2018 at 14:31, Marek Szyprowski <m.szyprowski@samsung.com> 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 <m.szyprowski@samsung.com>
> ---
>  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 <krzk@kernel.org>

Best regards,
Krzysztof

Re: [PATCH v2 3/6] clocksource: exynos_mct: Add arch_timer cooperation mode for ARM64

[Krzysztof Kozlowski]

On Mon, 15 Oct 2018 at 14:31, Marek Szyprowski <m.szyprowski@samsung.com> 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 <m.szyprowski@samsung.com>
> ---
>  drivers/clocksource/exynos_mct.c | 52 +++++++++++++++++++++-----------
>  1 file changed, 35 insertions(+), 17 deletions(-)

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

Re: [PATCH v2 3/6] clocksource: exynos_mct: Add arch_timer cooperation mode for ARM64

[Chanwoo Choi]

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 <cw00.choi@samsung.com>
Tested-by: Chanwoo Choi <cw00.choi@samsung.com>

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 <m.szyprowski@samsung.com>
> ---
>  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

Re: [PATCH v2 5/6] arm64: dts: exynos: Move arch-timer node to right place

[Chanwoo Choi]

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 <m.szyprowski@samsung.com>
> ---
>  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 <cw00.choi@samsung.com>
Tested-by: Chanwoo Choi <cw00.choi@samsung.com>

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2 6/6] arm64: platform: Add enable Exynos Multi-Core Timer driver

[Chanwoo Choi]

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 <m.szyprowski@samsung.com>
> ---
>  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 <cw00.choi@samsung.com>
Tested-by: Chanwoo Choi <cw00.choi@samsung.com>

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2 3/6] clocksource: exynos_mct: Add arch_timer cooperation mode for ARM64

[Marek Szyprowski]

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