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

Groups > linux.kernel > #1603585 > unrolled thread

Re: [PATCH v21 04/13] clocksource: arm_arch_timer: split arch_timer_rate for different types of timer

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 v21 04/13] clocksource: arm_arch_timer: split  arch_timer_rate for different types of timer Mark Rutland <mark.rutland@arm.com> - 2017-03-17 20:20 +0100
    Re: [PATCH v21 04/13] clocksource: arm_arch_timer: split  arch_timer_rate for different types of timer Fu Wei <fu.wei@linaro.org> - 2017-03-20 14:40 +0100

#1603585 — Re: [PATCH v21 04/13] clocksource: arm_arch_timer: split arch_timer_rate for different types of timer

On Tue, Feb 07, 2017 at 02:50:06AM +0800, fu.wei@linaro.org wrote:
> From: Fu Wei <fu.wei@linaro.org>
> 
> Currently, arch_timer_rate is used to store the frequency got from per-cpu
> arch-timer or the memory-mapped (MMIO) timers. But those values come from
> different registers which should all be initialized by firmware.
> 
> This patch remove arch_timer_rate, and use arch_timer_sysreg_freq and
> arch_timer_mmio_freq instead.
> 
> Signed-off-by: Fu Wei <fu.wei@linaro.org>

Thanks for attacking this. Generally, I do think this is the right thing
to do.

However...

> @@ -1070,10 +1077,9 @@ static int __init arch_timer_mem_init(struct device_node *np)
>  	 * Try to determine the frequency from the device tree,
>  	 * if fail, get the frequency from the CNTFRQ reg of MMIO timer.
>  	 */
> -	if (!arch_timer_rate &&
> -	    of_property_read_u32(np, "clock-frequency", &arch_timer_rate))
> -		arch_timer_rate = arch_timer_get_mmio_freq(base);
> -	if (!arch_timer_rate) {
> +	if (of_property_read_u32(np, "clock-frequency", &arch_timer_mmio_freq))
> +		arch_timer_mmio_freq = arch_timer_get_mmio_freq(base);
> +	if (!arch_timer_mmio_freq) {
>  		pr_err(FW_BUG "frequency not available for MMIO timer.\n");
>  		ret = -EINVAL;
>  		goto out;

... unfortunately, I believe that this will break some DT platforms that
have been (unintentionally) relying on the way currently allow the
frequency to be probed from either the MMIO timer or the sysreg timer.

So while the above was my suggestion, it was not my best.

For the timebeing, let's leave the single arch_timer_rate, but ensure
that it doesn't get in the way fo the ACPI code, by making the ACPI
probe path:

* Probe the sysreg timers first, using the sysreg cntfrq().

* Probe the MMIO timers second, verifying that each MMIO cntfrq matches
  the already-probed sysreg cntfrq.

... which is what I believe you suggested previously. 

Thanks,
Mark.

[toc] | [next] | [standalone]

#1604575

Hi Mark,

On 18 March 2017 at 03:05, Mark Rutland <mark.rutland@arm.com> wrote:
> On Tue, Feb 07, 2017 at 02:50:06AM +0800, fu.wei@linaro.org wrote:
>> From: Fu Wei <fu.wei@linaro.org>
>>
>> Currently, arch_timer_rate is used to store the frequency got from per-cpu
>> arch-timer or the memory-mapped (MMIO) timers. But those values come from
>> different registers which should all be initialized by firmware.
>>
>> This patch remove arch_timer_rate, and use arch_timer_sysreg_freq and
>> arch_timer_mmio_freq instead.
>>
>> Signed-off-by: Fu Wei <fu.wei@linaro.org>
>
> Thanks for attacking this. Generally, I do think this is the right thing
> to do.
>
> However...
>
>> @@ -1070,10 +1077,9 @@ static int __init arch_timer_mem_init(struct device_node *np)
>>        * Try to determine the frequency from the device tree,
>>        * if fail, get the frequency from the CNTFRQ reg of MMIO timer.
>>        */
>> -     if (!arch_timer_rate &&
>> -         of_property_read_u32(np, "clock-frequency", &arch_timer_rate))
>> -             arch_timer_rate = arch_timer_get_mmio_freq(base);
>> -     if (!arch_timer_rate) {
>> +     if (of_property_read_u32(np, "clock-frequency", &arch_timer_mmio_freq))
>> +             arch_timer_mmio_freq = arch_timer_get_mmio_freq(base);
>> +     if (!arch_timer_mmio_freq) {
>>               pr_err(FW_BUG "frequency not available for MMIO timer.\n");
>>               ret = -EINVAL;
>>               goto out;
>
> ... unfortunately, I believe that this will break some DT platforms that
> have been (unintentionally) relying on the way currently allow the
> frequency to be probed from either the MMIO timer or the sysreg timer.
>

Ah, I 'm really not aware of this. Thanks for pointing it out.

> So while the above was my suggestion, it was not my best.
>
> For the timebeing, let's leave the single arch_timer_rate, but ensure
> that it doesn't get in the way fo the ACPI code, by making the ACPI
> probe path:
>
> * Probe the sysreg timers first, using the sysreg cntfrq().
>
> * Probe the MMIO timers second, verifying that each MMIO cntfrq matches
>   the already-probed sysreg cntfrq.
>
> ... which is what I believe you suggested previously.

OK, NP, will do this way.

>
> Thanks,
> Mark.



-- 
Best regards,

Fu Wei
Software Engineer
Red Hat

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web