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

From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
To: Vincent Whitchurch <vincent.whitchurch@axis.com>,
	tglx@linutronix.de, daniel.lezcano@linaro.org
Cc: kernel@axis.com, linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-samsung-soc@vger.kernel.org, alim.akhtar@samsung.com,
	devicetree@vger.kernel.org, robh+dt@kernel.org
Subject: Re: [PATCH v3 3/4] clocksource/drivers/exynos_mct: Support local-timers property
Date: Fri, 8 Apr 2022 10:02:48 +0200	[thread overview]
Message-ID: <d3cd6d0c-26b7-c870-ee30-361ef4e11f35@linaro.org> (raw)
In-Reply-To: <20220407074432.424578-4-vincent.whitchurch@axis.com>

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