[PATCH v3 0/4] clocksource: Add MCT support for ARTPEC-8

[PATCH v3 0/4] clocksource: Add MCT support for ARTPEC-8

[Vincent Whitchurch]

This series add supports for the timer block on ARTPEC-8.  The block itself is
fully compatible with the existing exynos4210-mct driver.  The ARTPEC-8 SoC
uses this block from two separate processors running Linux (AMP) so it needs
some extra code to allow this sharing.

v3:
- Split and rename devicetree properties
- Add vendor prefix to devicetree properties
- Change descriptions of properties to hopefully describe hardware
- Remove addition of more global variables to the driver

v2:
- The series is now rebased on top of Krzysztof's patch "dt-bindings: timer:
  exynos4210-mct: describe known hardware and its interrupts".
- Combine the Kconfig change and the local timer change into one series
- Use devicetree property rather than module parameter for the local timer handling
- Add specific compatible with the correct number of interrupts.

Cc: linux-kernel@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org

Cc: linux-samsung-soc@vger.kernel.org
Cc: alim.akhtar@samsung.com

Cc: devicetree@vger.kernel.org
Cc: robh+dt@kernel.org

Vincent Whitchurch (4):
  dt-bindings: timer: exynos4210-mct: Add ARTPEC-8 MCT support
  clocksource/drivers/exynos_mct: Support frc-shared property
  clocksource/drivers/exynos_mct: Support local-timers property
  clocksource/drivers/exynos_mct: Enable building on ARTPEC

 .../timer/samsung,exynos4210-mct.yaml         | 26 +++++++
 drivers/clocksource/Kconfig                   |  2 +-
 drivers/clocksource/exynos_mct.c              | 72 ++++++++++++++++---
 3 files changed, 90 insertions(+), 10 deletions(-)

-- 
2.34.1

[PATCH v3 1/4] dt-bindings: timer: exynos4210-mct: Add ARTPEC-8 MCT support

[Vincent Whitchurch]

The ARTPEC-8 has an MCT with 4 global and 8 local timer interrupts.

The SoC has a quad-core Cortex-A53 and a single-core Cortex-A5 which
share one MCT with one global and eight local timers.  The Cortex-A53
and Cortex-A5 do not have cache-coherency between them, and therefore
run two separate kernels.

The Cortex-A53 boots first and starts the global free-running counter
and also registers a clock events device using the global timer.  (This
global timer clock events is usually replaced by arch timer clock events
for each of the cores.)

When the A5 boots (via the A53), it should not use the global timer
interrupts or write to the global timer registers.  This is because even
if there are four global comparators, the control bits for all four are
in the same registers, and we would need to synchronize between the
cpus.  Instead, the global timer FRC (already started by the A53) should
be used as the clock source, and one of the local timers which are not
used by the A53 can be used for clock events on the A5.

To support this hardware, add a compatible for the MCT as well as two
new properties to describe the hardware-mandated sharing of the FRC and
dedicating local timers to specific processors.

Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
---

Notes:
    v3:
    - Add all required bindings for ARTPEC-8 in one patch
    - Rename and split local-timer-only to samsung,local-timers and
      samsung,frc-shared
    - Restrict above properties to the ARTPEC-8 compatible.
    - Rewrite descriptions of properties to hopefully describe hardware.
    
    v2:
    - Use devicetree property instead of module parameter.

 .../timer/samsung,exynos4210-mct.yaml         | 26 +++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/Documentation/devicetree/bindings/timer/samsung,exynos4210-mct.yaml b/Documentation/devicetree/bindings/timer/samsung,exynos4210-mct.yaml
index 1584944c7ac4..bcfc849ca087 100644
--- a/Documentation/devicetree/bindings/timer/samsung,exynos4210-mct.yaml
+++ b/Documentation/devicetree/bindings/timer/samsung,exynos4210-mct.yaml
@@ -25,6 +25,7 @@ properties:
           - samsung,exynos4412-mct
       - items:
           - enum:
+              - axis,artpec8-mct
               - samsung,exynos3250-mct
               - samsung,exynos5250-mct
               - samsung,exynos5260-mct
@@ -46,6 +47,19 @@ properties:
   reg:
     maxItems: 1
 
+  samsung,frc-shared:
+    type: boolean
+    description: |
+      Indicates that the hardware requires that this processor share the
+      free-running counter with a different (main) processor.
+
+  samsung,local-timers:
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    minItems: 1
+    maxItems: 16
+    description: |
+      List of indices of local timers usable from this processor.
+
   interrupts:
     description: |
       Interrupts should be put in specific order. This is, the local timer
@@ -75,6 +89,17 @@ required:
   - reg
 
 allOf:
+  - if:
+      not:
+        properties:
+          compatible:
+            contains:
+              enum:
+                - axis,artpec8-mct
+    then:
+      properties:
+        samsung,local-timers: false
+        samsung,frc-shared: false
   - if:
       properties:
         compatible:
@@ -102,6 +127,7 @@ allOf:
         compatible:
           contains:
             enum:
+              - axis,artpec8-mct
               - samsung,exynos5260-mct
               - samsung,exynos5420-mct
               - samsung,exynos5433-mct
-- 
2.34.1

Re: [PATCH v3 1/4] dt-bindings: timer: exynos4210-mct: Add ARTPEC-8 MCT support

[Rob Herring]

On Thu, Apr 07, 2022 at 09:44:29AM +0200, Vincent Whitchurch wrote:
> The ARTPEC-8 has an MCT with 4 global and 8 local timer interrupts.
> 
> The SoC has a quad-core Cortex-A53 and a single-core Cortex-A5 which
> share one MCT with one global and eight local timers.  The Cortex-A53
> and Cortex-A5 do not have cache-coherency between them, and therefore
> run two separate kernels.
> 
> The Cortex-A53 boots first and starts the global free-running counter
> and also registers a clock events device using the global timer.  (This
> global timer clock events is usually replaced by arch timer clock events
> for each of the cores.)
> 
> When the A5 boots (via the A53), it should not use the global timer
> interrupts or write to the global timer registers.  This is because even
> if there are four global comparators, the control bits for all four are
> in the same registers, and we would need to synchronize between the
> cpus.  Instead, the global timer FRC (already started by the A53) should
> be used as the clock source, and one of the local timers which are not
> used by the A53 can be used for clock events on the A5.
> 
> To support this hardware, add a compatible for the MCT as well as two
> new properties to describe the hardware-mandated sharing of the FRC and
> dedicating local timers to specific processors.
> 
> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
> ---
> 
> Notes:
>     v3:
>     - Add all required bindings for ARTPEC-8 in one patch
>     - Rename and split local-timer-only to samsung,local-timers and
>       samsung,frc-shared
>     - Restrict above properties to the ARTPEC-8 compatible.
>     - Rewrite descriptions of properties to hopefully describe hardware.
>     
>     v2:
>     - Use devicetree property instead of module parameter.
> 
>  .../timer/samsung,exynos4210-mct.yaml         | 26 +++++++++++++++++++
>  1 file changed, 26 insertions(+)

What's this based on? Doesn't apply on v5.18-rc1.

Rob

Re: [PATCH v3 1/4] dt-bindings: timer: exynos4210-mct: Add ARTPEC-8 MCT support

[Vincent Whitchurch]

On Thu, Apr 07, 2022 at 05:04:09PM +0200, Rob Herring wrote:
> On Thu, Apr 07, 2022 at 09:44:29AM +0200, Vincent Whitchurch wrote:
> >  .../timer/samsung,exynos4210-mct.yaml         | 26 +++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> 
> What's this based on? Doesn't apply on v5.18-rc1.

This series is still based on Krzysztof's "dt-bindings: timer:
exynos4210-mct: describe known hardware and its interrupts".  The cover
letter mentions this but not very prominently, sorry.

That patch was reviewed a while ago but doesn't seem to have made it to
v5.18-rc1, but I see that Krzysztof reposted it yesterday: 

 https://lore.kernel.org/lkml/20220407194127.19004-1-krzysztof.kozlowski@linaro.org/

Re: [PATCH v3 1/4] dt-bindings: timer: exynos4210-mct: Add ARTPEC-8 MCT support

[Krzysztof Kozlowski]

On 07/04/2022 09:44, Vincent Whitchurch wrote:
> The ARTPEC-8 has an MCT with 4 global and 8 local timer interrupts.
> 
> The SoC has a quad-core Cortex-A53 and a single-core Cortex-A5 which
> share one MCT with one global and eight local timers.  The Cortex-A53
> and Cortex-A5 do not have cache-coherency between them, and therefore
> run two separate kernels.
> 
> The Cortex-A53 boots first and starts the global free-running counter
> and also registers a clock events device using the global timer.  (This
> global timer clock events is usually replaced by arch timer clock events
> for each of the cores.)
> 
> When the A5 boots (via the A53), it should not use the global timer
> interrupts or write to the global timer registers.  This is because even
> if there are four global comparators, the control bits for all four are
> in the same registers, and we would need to synchronize between the
> cpus.  Instead, the global timer FRC (already started by the A53) should
> be used as the clock source, and one of the local timers which are not
> used by the A53 can be used for clock events on the A5.
> 
> To support this hardware, add a compatible for the MCT as well as two
> new properties to describe the hardware-mandated sharing of the FRC and
> dedicating local timers to specific processors.
> 
> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
> ---
> 

This is rebased on my patch:
https://lore.kernel.org/lkml/20220407194127.19004-1-krzysztof.kozlowski@linaro.org/


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

[PATCH v3 2/4] clocksource/drivers/exynos_mct: Support frc-shared property

[Vincent Whitchurch]

When the FRC is shared with another main processor, the other processor
is assumed to have started it and this processor should not write to the
global registers.

Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
---

Notes:
    v3:
    - Split FRC sharing handling from local timer indices handling
    - Remove addition of global variable.

 drivers/clocksource/exynos_mct.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
index f29c812b70c9..12023831dedf 100644
--- a/drivers/clocksource/exynos_mct.c
+++ b/drivers/clocksource/exynos_mct.c
@@ -233,9 +233,16 @@ static cycles_t exynos4_read_current_timer(void)
 }
 #endif
 
-static int __init exynos4_clocksource_init(void)
+static int __init exynos4_clocksource_init(bool frc_shared)
 {
-	exynos4_mct_frc_start();
+	/*
+	 * When the frc is shared, the main processer should have already
+	 * turned it on and we shouldn't be writing to TCON.
+	 */
+	if (frc_shared)
+		mct_frc.resume = NULL;
+	else
+		exynos4_mct_frc_start();
 
 #if defined(CONFIG_ARM)
 	exynos4_delay_timer.read_current_timer = &exynos4_read_current_timer;
@@ -605,6 +612,7 @@ static int __init exynos4_timer_interrupts(struct device_node *np,
 
 static int __init mct_init_dt(struct device_node *np, unsigned int int_type)
 {
+	bool frc_shared = of_property_read_bool(np, "samsung,frc-shared");
 	int ret;
 
 	ret = exynos4_timer_resources(np);
@@ -615,10 +623,17 @@ static int __init mct_init_dt(struct device_node *np, unsigned int int_type)
 	if (ret)
 		return ret;
 
-	ret = exynos4_clocksource_init();
+	ret = exynos4_clocksource_init(frc_shared);
 	if (ret)
 		return ret;
 
+	/*
+	 * When the FRC is shared with a main processor, this secondary
+	 * processor cannot use the global comparator.
+	 */
+	if (frc_shared)
+		return ret;
+
 	return exynos4_clockevent_init();
 }
 
-- 
2.34.1

Re: [PATCH v3 2/4] clocksource/drivers/exynos_mct: Support frc-shared property

[Krzysztof Kozlowski]

On 07/04/2022 09:44, Vincent Whitchurch wrote:
> When the FRC is shared with another main processor, the other processor
> is assumed to have started it and this processor should not write to the
> global registers.
> 
> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
> ---
> 
> Notes:
>     v3:
>     - Split FRC sharing handling from local timer indices handling
>     - Remove addition of global variable.
> 
>  drivers/clocksource/exynos_mct.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 

Looks correct and I hope it works correct. :) I did not test it though.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof

[PATCH v3 3/4] clocksource/drivers/exynos_mct: Support local-timers property

[Vincent Whitchurch]

If the device tree indicates that the hardware requires that the
processor only use certain local timers, respect that.

Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
---

Notes:
    v3:
    - Use array in devicetree
    - Remove addition of global variable
    - Split out FRC sharing changes

 drivers/clocksource/exynos_mct.c | 51 ++++++++++++++++++++++++++++----
 1 file changed, 45 insertions(+), 6 deletions(-)

diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
index 12023831dedf..4093a71ff618 100644
--- a/drivers/clocksource/exynos_mct.c
+++ b/drivers/clocksource/exynos_mct.c
@@ -33,7 +33,7 @@
 #define EXYNOS4_MCT_G_INT_ENB		EXYNOS4_MCTREG(0x248)
 #define EXYNOS4_MCT_G_WSTAT		EXYNOS4_MCTREG(0x24C)
 #define _EXYNOS4_MCT_L_BASE		EXYNOS4_MCTREG(0x300)
-#define EXYNOS4_MCT_L_BASE(x)		(_EXYNOS4_MCT_L_BASE + (0x100 * x))
+#define EXYNOS4_MCT_L_BASE(x)		(_EXYNOS4_MCT_L_BASE + (0x100 * (x)))
 #define EXYNOS4_MCT_L_MASK		(0xffffff00)
 
 #define MCT_L_TCNTB_OFFSET		(0x00)
@@ -66,6 +66,8 @@
 #define MCT_L0_IRQ	4
 /* Max number of IRQ as per DT binding document */
 #define MCT_NR_IRQS	20
+/* Max number of local timers */
+#define MCT_NR_LOCAL	(MCT_NR_IRQS - MCT_L0_IRQ)
 
 enum {
 	MCT_INT_SPI,
@@ -456,7 +458,6 @@ static int exynos4_mct_starting_cpu(unsigned int cpu)
 		per_cpu_ptr(&percpu_mct_tick, cpu);
 	struct clock_event_device *evt = &mevt->evt;
 
-	mevt->base = EXYNOS4_MCT_L_BASE(cpu);
 	snprintf(mevt->name, sizeof(mevt->name), "mct_tick%d", cpu);
 
 	evt->name = mevt->name;
@@ -528,7 +529,9 @@ static int __init exynos4_timer_resources(struct device_node *np)
 }
 
 static int __init exynos4_timer_interrupts(struct device_node *np,
-					   unsigned int int_type)
+					   unsigned int int_type,
+					   u32 *local_idx,
+					   size_t nr_local)
 {
 	int nr_irqs, i, err, cpu;
 
@@ -561,13 +564,19 @@ static int __init exynos4_timer_interrupts(struct device_node *np,
 	} else {
 		for_each_possible_cpu(cpu) {
 			int mct_irq;
+			unsigned int irqidx;
 			struct mct_clock_event_device *pcpu_mevt =
 				per_cpu_ptr(&percpu_mct_tick, cpu);
 
+			if (cpu >= nr_local)
+				break;
+
+			irqidx = MCT_L0_IRQ + local_idx[cpu];
+
 			pcpu_mevt->evt.irq = -1;
-			if (MCT_L0_IRQ + cpu >= ARRAY_SIZE(mct_irqs))
+			if (irqidx >= ARRAY_SIZE(mct_irqs))
 				break;
-			mct_irq = mct_irqs[MCT_L0_IRQ + cpu];
+			mct_irq = mct_irqs[irqidx];
 
 			irq_set_status_flags(mct_irq, IRQ_NOAUTOEN);
 			if (request_irq(mct_irq,
@@ -583,6 +592,15 @@ static int __init exynos4_timer_interrupts(struct device_node *np,
 		}
 	}
 
+	for_each_possible_cpu(cpu) {
+		struct mct_clock_event_device *mevt = per_cpu_ptr(&percpu_mct_tick, cpu);
+
+		if (cpu >= nr_local)
+			break;
+
+		mevt->base = EXYNOS4_MCT_L_BASE(local_idx[cpu]);
+	}
+
 	/* Install hotplug callbacks which configure the timer on this CPU */
 	err = cpuhp_setup_state(CPUHP_AP_EXYNOS4_MCT_TIMER_STARTING,
 				"clockevents/exynos4/mct_timer:starting",
@@ -613,13 +631,34 @@ static int __init exynos4_timer_interrupts(struct device_node *np,
 static int __init mct_init_dt(struct device_node *np, unsigned int int_type)
 {
 	bool frc_shared = of_property_read_bool(np, "samsung,frc-shared");
+	u32 local_idx[MCT_NR_LOCAL] = {0};
+	int nr_local;
 	int ret;
 
+	nr_local = of_property_count_u32_elems(np, "samsung,local-timers");
+	if (nr_local == 0)
+		return -EINVAL;
+	if (nr_local > 0) {
+		if (nr_local > ARRAY_SIZE(local_idx))
+			return -EINVAL;
+
+		ret = of_property_read_u32_array(np, "samsung,local-timers",
+						 local_idx, nr_local);
+		if (ret)
+			return ret;
+	} else {
+		int i;
+
+		nr_local = ARRAY_SIZE(local_idx);
+		for (i = 0; i < nr_local; i++)
+			local_idx[i] = i;
+	}
+
 	ret = exynos4_timer_resources(np);
 	if (ret)
 		return ret;
 
-	ret = exynos4_timer_interrupts(np, int_type);
+	ret = exynos4_timer_interrupts(np, int_type, local_idx, nr_local);
 	if (ret)
 		return ret;
 
-- 
2.34.1

Re: [PATCH v3 3/4] clocksource/drivers/exynos_mct: Support local-timers property

[Krzysztof Kozlowski]

On 07/04/2022 09:44, Vincent Whitchurch wrote:
> If the device tree indicates that the hardware requires that the
> processor only use certain local timers, respect that.
> 
> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
> ---
> 
> Notes:
>     v3:
>     - Use array in devicetree
>     - Remove addition of global variable
>     - Split out FRC sharing changes
> 
>  drivers/clocksource/exynos_mct.c | 51 ++++++++++++++++++++++++++++----
>  1 file changed, 45 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
> index 12023831dedf..4093a71ff618 100644
> --- a/drivers/clocksource/exynos_mct.c
> +++ b/drivers/clocksource/exynos_mct.c
> @@ -33,7 +33,7 @@
>  #define EXYNOS4_MCT_G_INT_ENB		EXYNOS4_MCTREG(0x248)
>  #define EXYNOS4_MCT_G_WSTAT		EXYNOS4_MCTREG(0x24C)
>  #define _EXYNOS4_MCT_L_BASE		EXYNOS4_MCTREG(0x300)
> -#define EXYNOS4_MCT_L_BASE(x)		(_EXYNOS4_MCT_L_BASE + (0x100 * x))
> +#define EXYNOS4_MCT_L_BASE(x)		(_EXYNOS4_MCT_L_BASE + (0x100 * (x)))
>  #define EXYNOS4_MCT_L_MASK		(0xffffff00)
>  
>  #define MCT_L_TCNTB_OFFSET		(0x00)
> @@ -66,6 +66,8 @@
>  #define MCT_L0_IRQ	4
>  /* Max number of IRQ as per DT binding document */
>  #define MCT_NR_IRQS	20
> +/* Max number of local timers */
> +#define MCT_NR_LOCAL	(MCT_NR_IRQS - MCT_L0_IRQ)
>  
>  enum {
>  	MCT_INT_SPI,
> @@ -456,7 +458,6 @@ static int exynos4_mct_starting_cpu(unsigned int cpu)
>  		per_cpu_ptr(&percpu_mct_tick, cpu);
>  	struct clock_event_device *evt = &mevt->evt;
>  
> -	mevt->base = EXYNOS4_MCT_L_BASE(cpu);
>  	snprintf(mevt->name, sizeof(mevt->name), "mct_tick%d", cpu);
>  
>  	evt->name = mevt->name;
> @@ -528,7 +529,9 @@ static int __init exynos4_timer_resources(struct device_node *np)
>  }
>  

Document the arguments, especially focusing on the keys and the contents
of local_idx. The code is getting to a state with 3 or 4 variables
having similar meaning (IRQ number, local IRQ number, local IRQ index).

>  static int __init exynos4_timer_interrupts(struct device_node *np,
> -					   unsigned int int_type)
> +					   unsigned int int_type,
> +					   u32 *local_idx,

const u32 *

> +					   size_t nr_local)
>  {
>  	int nr_irqs, i, err, cpu;
>  
> @@ -561,13 +564,19 @@ static int __init exynos4_timer_interrupts(struct device_node *np,
>  	} else {
>  		for_each_possible_cpu(cpu) {
>  			int mct_irq;
> +			unsigned int irqidx;

irq_idx

>  			struct mct_clock_event_device *pcpu_mevt =
>  				per_cpu_ptr(&percpu_mct_tick, cpu);
>  
> +			if (cpu >= nr_local)
> +				break;
> +
> +			irqidx = MCT_L0_IRQ + local_idx[cpu];
> +
>  			pcpu_mevt->evt.irq = -1;
> -			if (MCT_L0_IRQ + cpu >= ARRAY_SIZE(mct_irqs))
> +			if (irqidx >= ARRAY_SIZE(mct_irqs))
>  				break;
> -			mct_irq = mct_irqs[MCT_L0_IRQ + cpu];
> +			mct_irq = mct_irqs[irqidx];
>  
>  			irq_set_status_flags(mct_irq, IRQ_NOAUTOEN);
>  			if (request_irq(mct_irq,
> @@ -583,6 +592,15 @@ static int __init exynos4_timer_interrupts(struct device_node *np,
>  		}
>  	}
>  
> +	for_each_possible_cpu(cpu) {
> +		struct mct_clock_event_device *mevt = per_cpu_ptr(&percpu_mct_tick, cpu);
> +
> +		if (cpu >= nr_local)

It looks like an error condition, so this should not be handled silently
because later base==0 will be used. Probably old code has similar problem...


Best regards,
Krzysztof

[PATCH v3 4/4] clocksource/drivers/exynos_mct: Enable building on ARTPEC

[Vincent Whitchurch]

This timer block is used on ARTPEC-8.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
---

Notes:
    v3:
    - Add Krzysztof's Reviewed-by.

 drivers/clocksource/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index ae95d06a4a8f..2ea981ef23af 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -419,7 +419,7 @@ config ATMEL_TCB_CLKSRC
 config CLKSRC_EXYNOS_MCT
 	bool "Exynos multi core timer driver" if COMPILE_TEST
 	depends on ARM || ARM64
-	depends on ARCH_EXYNOS || COMPILE_TEST
+	depends on ARCH_ARTPEC || ARCH_EXYNOS || COMPILE_TEST
 	help
 	  Support for Multi Core Timer controller on Exynos SoCs.
 
-- 
2.34.1