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

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(-)

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

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