Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1611059 > unrolled thread

Re: [PATCH v22 03/11] clocksource: arm_arch_timer: refactor arch_timer_needs_probing

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v22 03/11] clocksource: arm_arch_timer: refactor  arch_timer_needs_probing Daniel Lezcano <daniel.lezcano@linaro.org> - 2017-03-28 17:10 +0200
    Re: [PATCH v22 03/11] clocksource: arm_arch_timer: refactor  arch_timer_needs_probing Mark Rutland <mark.rutland@arm.com> - 2017-03-29 17:30 +0200
      Re: [PATCH v22 03/11] clocksource: arm_arch_timer: refactor  arch_timer_needs_probing Daniel Lezcano <daniel.lezcano@linaro.org> - 2017-03-29 17:40 +0200

#1611059 — Re: [PATCH v22 03/11] clocksource: arm_arch_timer: refactor arch_timer_needs_probing

On Wed, Mar 22, 2017 at 12:31:14AM +0800, fu.wei@linaro.org wrote:
> From: Fu Wei <fu.wei@linaro.org>
> 
> When system init with device-tree, we don't know which node will be
> initialized first. And the code in arch_timer_common_init should wait
> until per-cpu timer and MMIO timer are both initialized. So we need
> arch_timer_needs_probing to detect the init status of system.
> 
> But currently the code is dispersed in arch_timer_needs_probing and
> arch_timer_common_init. And the function name doesn't specify that
> it's only for device-tree. This is somewhat confusing.

Can the following patch help you to solve nicely the situation ?

https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1360007.html
 
> This patch move all related code from arch_timer_common_init to
> arch_timer_needs_probing, refactor it, and rename it to
> arch_timer_needs_of_probing. And make sure that it will be called
> only if acpi is disabled.
> 
> Signed-off-by: Fu Wei <fu.wei@linaro.org>
> Reviewed-by: Hanjun Guo <hanjun.guo@linaro.org>
> ---
>  drivers/clocksource/arm_arch_timer.c | 34 +++++++++++++++++++---------------
>  1 file changed, 19 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 29ca7d6..d1a05d9 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -839,15 +839,28 @@ static const struct of_device_id arch_timer_mem_of_match[] __initconst = {
>  	{},
>  };
>  
> -static bool __init
> -arch_timer_needs_probing(int type, const struct of_device_id *matches)
> +static bool __init arch_timer_needs_of_probing(void)
>  {
>  	struct device_node *dn;
>  	bool needs_probing = false;
> +	unsigned int mask = ARCH_TIMER_TYPE_CP15 | ARCH_TIMER_TYPE_MEM;
>  
> -	dn = of_find_matching_node(NULL, matches);
> -	if (dn && of_device_is_available(dn) && !(arch_timers_present & type))
> +	/* We have two timers, and both device-tree nodes are probed. */
> +	if ((arch_timers_present & mask) == mask)
> +		return false;
> +
> +	/*
> +	 * Only one type of timer is probed,
> +	 * check if we have another type of timer node in device-tree.
> +	 */
> +	if (arch_timers_present & ARCH_TIMER_TYPE_CP15)
> +		dn = of_find_matching_node(NULL, arch_timer_mem_of_match);
> +	else
> +		dn = of_find_matching_node(NULL, arch_timer_of_match);
> +
> +	if (dn && of_device_is_available(dn))
>  		needs_probing = true;
> +
>  	of_node_put(dn);
>  
>  	return needs_probing;
> @@ -855,17 +868,8 @@ arch_timer_needs_probing(int type, const struct of_device_id *matches)
>  
>  static int __init arch_timer_common_init(void)
>  {
> -	unsigned mask = ARCH_TIMER_TYPE_CP15 | ARCH_TIMER_TYPE_MEM;
> -
> -	/* Wait until both nodes are probed if we have two timers */
> -	if ((arch_timers_present & mask) != mask) {
> -		if (arch_timer_needs_probing(ARCH_TIMER_TYPE_MEM,
> -					     arch_timer_mem_of_match))
> -			return 0;
> -		if (arch_timer_needs_probing(ARCH_TIMER_TYPE_CP15,
> -					     arch_timer_of_match))
> -			return 0;
> -	}
> +	if (acpi_disabled && arch_timer_needs_of_probing())
> +		return 0;
>  
>  	arch_timer_banner(arch_timers_present);
>  	arch_counter_register(arch_timers_present);
> -- 
> 2.9.3
> 

-- 

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[toc] | [next] | [standalone]

#1612051

On Tue, Mar 28, 2017 at 05:02:20PM +0200, Daniel Lezcano wrote:
> On Wed, Mar 22, 2017 at 12:31:14AM +0800, fu.wei@linaro.org wrote:
> > From: Fu Wei <fu.wei@linaro.org>
> > 
> > When system init with device-tree, we don't know which node will be
> > initialized first. And the code in arch_timer_common_init should wait
> > until per-cpu timer and MMIO timer are both initialized. So we need
> > arch_timer_needs_probing to detect the init status of system.
> > 
> > But currently the code is dispersed in arch_timer_needs_probing and
> > arch_timer_common_init. And the function name doesn't specify that
> > it's only for device-tree. This is somewhat confusing.
> 
> Can the following patch help you to solve nicely the situation ?
> 
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1360007.html

This does not help.

The needs_probing logic is all there to bodge around a problem with
registering sched_clock, when you have two sources of the same
frequency, but one is otherwise better.

The sysreg clocksource has much lower latency than the MMIO clocksource,
so we always want to use that as the sched_clock if we have it.
Currently, the code ensures this by deferring registration of
sched_clock.

Ideally, we'd figure that out dynamically, or we'd have a rating
argument.

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

#1612062

On Wed, Mar 29, 2017 at 04:24:08PM +0100, Mark Rutland wrote:
> On Tue, Mar 28, 2017 at 05:02:20PM +0200, Daniel Lezcano wrote:
> > On Wed, Mar 22, 2017 at 12:31:14AM +0800, fu.wei@linaro.org wrote:
> > > From: Fu Wei <fu.wei@linaro.org>
> > > 
> > > When system init with device-tree, we don't know which node will be
> > > initialized first. And the code in arch_timer_common_init should wait
> > > until per-cpu timer and MMIO timer are both initialized. So we need
> > > arch_timer_needs_probing to detect the init status of system.
> > > 
> > > But currently the code is dispersed in arch_timer_needs_probing and
> > > arch_timer_common_init. And the function name doesn't specify that
> > > it's only for device-tree. This is somewhat confusing.
> > 
> > Can the following patch help you to solve nicely the situation ?
> > 
> > https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1360007.html
> 
> This does not help.
> 
> The needs_probing logic is all there to bodge around a problem with
> registering sched_clock, when you have two sources of the same
> frequency, but one is otherwise better.
> 
> The sysreg clocksource has much lower latency than the MMIO clocksource,
> so we always want to use that as the sched_clock if we have it.
> Currently, the code ensures this by deferring registration of
> sched_clock.
> 
> Ideally, we'd figure that out dynamically, or we'd have a rating
> argument.
> 

Ok, I see. Thanks.

-- 

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web