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@xxxxxxxx>
> ---
>
> 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
