Re: [PATCH 8/8] ARM: smp: Remove local timer API

[Stephen Boyd <sboyd@codeaurora.org>]

On 2/22/2013 3:15 AM, Mark Rutland wrote:
> Hi Stephen,
>
> One thing that struck me when I was fiddling with the broadcast mechanism was
> that it should be possible to have a generic dummy timer implementation. As
> long as the architecture calls notifiers at the appropriate times, it should
> look like any other timer driver (apart from not touching any hardware). It just
> needs to depend on ARCH_HAS_TICK_BROADCAST.
>
> I believe it shouldn't be too difficult to implement, though I may be blind to
> some problems.

I completely agree and I was thinking the same thing while writing this
patchset.

>
> Otherwise, I have a few comments on the patch below.
>
> On Fri, Feb 22, 2013 at 07:27:19AM +0000, Stephen Boyd wrote:
>> There are no more users of this API, remove it.
> As we're leaving the dummy timers, it'd be worth explaining why in the commit
> message.

No problem.

>
>> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
>> index dedf02b..7d4338d 100644
>> --- a/arch/arm/Kconfig
>> +++ b/arch/arm/Kconfig
>> @@ -1527,6 +1527,7 @@ config SMP
>>  	depends on HAVE_SMP
>>  	depends on MMU
>>  	select HAVE_ARM_SCU if !ARCH_MSM_SCORPIONMP
>> +	select HAVE_ARM_TWD if (!ARCH_MSM_SCORPIONMP && !EXYNOS4_MCT)
>>  	select USE_GENERIC_SMP_HELPERS
>>  	help
>>  	  This enables support for systems with more than one CPU. If you have
> Should this have been in an earlier patch?

It could be part of the smp_twd patch if you like.

> Why is it necessary?

It shouldn't be. In fact, I sent a patchset a few months ago that pushed
down the TWD and SCU selects to the respective machines that need them.
I should resend that.

>
> [...]
>
>> -static void percpu_timer_setup(void);
>> +static void broadcast_timer_setup(void);
>>  
>>  /*
>>   * This is the secondary CPU boot entry.  We're using this CPUs
>> @@ -325,9 +317,9 @@ asmlinkage void __cpuinit secondary_start_kernel(void)
>>  	complete(&cpu_running);
>>  
>>  	/*
>> -	 * Setup the percpu timer for this CPU.
>> +	 * Setup the dummy broadcast timer for this CPU.
> To me, calling something a broadcast timer makes it sound like it performs the
> broadcast. We use the term "broadcast timer" elsewhere here (e.g.
> broadcast_timer_setup), and I think it's any unnecessarily confusing term.
>
> Might it be better to say "dummy timer" consistently?

Sure. I wonder if we need the comment at all. I can rename the function
to dummy_timer_setup() and it pretty much sounds like what the comment
will say.

>
> [...]
>
>> diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
>> index 2bdd4cf..c00a8f8 100644
>> --- a/arch/arm/mach-omap2/timer.c
>> +++ b/arch/arm/mach-omap2/timer.c
>> @@ -587,7 +587,6 @@ OMAP_SYS_GP_TIMER_INIT(3_am33xx, 1, OMAP4_MPU_SOURCE, "ti,timer-alwon",
>>  #ifdef CONFIG_ARCH_OMAP4
>>  OMAP_SYS_32K_TIMER_INIT(4, 1, OMAP4_32K_SOURCE, "ti,timer-alwon",
>>  			2, OMAP4_MPU_SOURCE);
>> -#ifdef CONFIG_LOCAL_TIMERS
>>  static DEFINE_TWD_LOCAL_TIMER(twd_local_timer, OMAP44XX_LOCAL_TWD_BASE, 29);
>>  void __init omap4_local_timer_init(void)
>>  {
>> @@ -606,12 +605,6 @@ void __init omap4_local_timer_init(void)
>>  			pr_err("twd_local_timer_register failed %d\n", err);
>>  	}
>>  }
>> -#else /* CONFIG_LOCAL_TIMERS */
>> -void __init omap4_local_timer_init(void)
>> -{
>> -	omap4_sync32k_timer_init();
>> -}
>> -#endif /* CONFIG_LOCAL_TIMERS */
>>  #endif /* CONFIG_ARCH_OMAP4 */
>>  
>>  #ifdef CONFIG_SOC_OMAP5
> I believe the above OMAP changes should have been in an earlier patch?

There isn't an omap patch. I could make it part of the smp_twd patch?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation