[PATCH] ARM: dts: stm32: Enable high resolution timer

[PATCH] ARM: dts: stm32: Enable high resolution timer

[Benjamin Gaignard]

Adding always-on makes arm arch_timer claim to be an high resolution timer.
That is possible because power mode won't stop clocking the timer.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
---
 arch/arm/boot/dts/stm32mp157c.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/stm32mp157c.dtsi b/arch/arm/boot/dts/stm32mp157c.dtsi
index 9b11654a0a39..74f64745d60d 100644
--- a/arch/arm/boot/dts/stm32mp157c.dtsi
+++ b/arch/arm/boot/dts/stm32mp157c.dtsi
@@ -50,6 +50,7 @@
 			     <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
 			     <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
 		interrupt-parent = <&intc>;
+		always-on;
 	};
 
 	clocks {
-- 
2.15.0

Re: [PATCH] ARM: dts: stm32: Enable high resolution timer

[Marc Zyngier]

On 2019-09-27 09:48, Benjamin Gaignard wrote:
> Adding always-on makes arm arch_timer claim to be an high resolution 
> timer.
> That is possible because power mode won't stop clocking the timer.

The "always-on" is not about the clock. It is about the comparator.
The clock itself is *guaranteed* to always tick. If it didn't, that'd 
be
an integration bug, and a pretty bad one.

What you're claiming here is that your CPU never enters a low-power 
mode?
Ever? I find this very hard to believe.

Furthermore, claiming that always-on is the way to force the arch-timer
to be an hrtimer is factually wrong. This is what happens *if* this is
the only timer in the system. The only case this is true is for virtual
machines. Anything else has a global timer somewhere that will allow
the arch timers to be used as an hrtimer.

I'm pretty sure you too have a global timer somewhere in your system.
Enable it, and enjoy hrtimers without having to lie about the 
properties
of your system! ;-)

         M.

>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
> ---
>  arch/arm/boot/dts/stm32mp157c.dtsi | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm/boot/dts/stm32mp157c.dtsi
> b/arch/arm/boot/dts/stm32mp157c.dtsi
> index 9b11654a0a39..74f64745d60d 100644
> --- a/arch/arm/boot/dts/stm32mp157c.dtsi
> +++ b/arch/arm/boot/dts/stm32mp157c.dtsi
> @@ -50,6 +50,7 @@
>  			     <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
>  			     <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
>  		interrupt-parent = <&intc>;
> +		always-on;
>  	};
>
>  	clocks {

-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] ARM: dts: stm32: Enable high resolution timer

[Benjamin GAIGNARD]

On 9/27/19 1:22 PM, Marc Zyngier wrote:
> On 2019-09-27 09:48, Benjamin Gaignard wrote:
>> Adding always-on makes arm arch_timer claim to be an high resolution 
>> timer.
>> That is possible because power mode won't stop clocking the timer.
>
> The "always-on" is not about the clock. It is about the comparator.
> The clock itself is *guaranteed* to always tick. If it didn't, that'd be
> an integration bug, and a pretty bad one.
>
> What you're claiming here is that your CPU never enters a low-power mode?
> Ever? I find this very hard to believe.
>
> Furthermore, claiming that always-on is the way to force the arch-timer
> to be an hrtimer is factually wrong. This is what happens *if* this is
> the only timer in the system. The only case this is true is for virtual
> machines. Anything else has a global timer somewhere that will allow
> the arch timers to be used as an hrtimer.
>
> I'm pretty sure you too have a global timer somewhere in your system.
> Enable it, and enjoy hrtimers without having to lie about the properties
> of your system! ;-)

Hi Marc,

This SoC doesn't have any other global timer. Use arch_time is the only
we have to provide hrtimer on this system.

Benjamin

>
>         M.
>
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
>> ---
>>  arch/arm/boot/dts/stm32mp157c.dtsi | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/arm/boot/dts/stm32mp157c.dtsi
>> b/arch/arm/boot/dts/stm32mp157c.dtsi
>> index 9b11654a0a39..74f64745d60d 100644
>> --- a/arch/arm/boot/dts/stm32mp157c.dtsi
>> +++ b/arch/arm/boot/dts/stm32mp157c.dtsi
>> @@ -50,6 +50,7 @@
>>                   <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | 
>> IRQ_TYPE_LEVEL_LOW)>,
>>                   <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) | 
>> IRQ_TYPE_LEVEL_LOW)>;
>>          interrupt-parent = <&intc>;
>> +        always-on;
>>      };
>>
>>      clocks {
>

Re: [PATCH] ARM: dts: stm32: Enable high resolution timer

[Marc Zyngier]

On 2019-09-27 13:36, Benjamin GAIGNARD wrote:
> On 9/27/19 1:22 PM, Marc Zyngier wrote:
>> On 2019-09-27 09:48, Benjamin Gaignard wrote:
>>> Adding always-on makes arm arch_timer claim to be an high 
>>> resolution
>>> timer.
>>> That is possible because power mode won't stop clocking the timer.
>>
>> The "always-on" is not about the clock. It is about the comparator.
>> The clock itself is *guaranteed* to always tick. If it didn't, 
>> that'd be
>> an integration bug, and a pretty bad one.
>>
>> What you're claiming here is that your CPU never enters a low-power 
>> mode?
>> Ever? I find this very hard to believe.
>>
>> Furthermore, claiming that always-on is the way to force the 
>> arch-timer
>> to be an hrtimer is factually wrong. This is what happens *if* this 
>> is
>> the only timer in the system. The only case this is true is for 
>> virtual
>> machines. Anything else has a global timer somewhere that will allow
>> the arch timers to be used as an hrtimer.
>>
>> I'm pretty sure you too have a global timer somewhere in your 
>> system.
>> Enable it, and enjoy hrtimers without having to lie about the 
>> properties
>> of your system! ;-)
>
> Hi Marc,
>
> This SoC doesn't have any other global timer. Use arch_time is the 
> only
> we have to provide hrtimer on this system.

And you don't have any form of power management either? What happens 
when
your CPU goes into idle? If your system does any form of power 
management
*and* doesn't have a separate timer, it is remarkably broken.

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] ARM: dts: stm32: Enable high resolution timer

[Benjamin GAIGNARD]

On 9/27/19 2:41 PM, Marc Zyngier wrote:
> On 2019-09-27 13:36, Benjamin GAIGNARD wrote:
>> On 9/27/19 1:22 PM, Marc Zyngier wrote:
>>> On 2019-09-27 09:48, Benjamin Gaignard wrote:
>>>> Adding always-on makes arm arch_timer claim to be an high resolution
>>>> timer.
>>>> That is possible because power mode won't stop clocking the timer.
>>>
>>> The "always-on" is not about the clock. It is about the comparator.
>>> The clock itself is *guaranteed* to always tick. If it didn't, 
>>> that'd be
>>> an integration bug, and a pretty bad one.
>>>
>>> What you're claiming here is that your CPU never enters a low-power 
>>> mode?
>>> Ever? I find this very hard to believe.
>>>
>>> Furthermore, claiming that always-on is the way to force the arch-timer
>>> to be an hrtimer is factually wrong. This is what happens *if* this is
>>> the only timer in the system. The only case this is true is for virtual
>>> machines. Anything else has a global timer somewhere that will allow
>>> the arch timers to be used as an hrtimer.
>>>
>>> I'm pretty sure you too have a global timer somewhere in your system.
>>> Enable it, and enjoy hrtimers without having to lie about the 
>>> properties
>>> of your system! ;-)
>>
>> Hi Marc,
>>
>> This SoC doesn't have any other global timer. Use arch_time is the only
>> we have to provide hrtimer on this system.
>
> And you don't have any form of power management either? What happens when
> your CPU goes into idle? If your system does any form of power management
> *and* doesn't have a separate timer, it is remarkably broken.

Even in low-power modes this timer is always powered and clocked so it 
is working fine.

>
>         M.

Re: [PATCH] ARM: dts: stm32: Enable high resolution timer

[Marc Zyngier]

On 2019-09-27 13:44, Benjamin GAIGNARD wrote:
> On 9/27/19 2:41 PM, Marc Zyngier wrote:
>> On 2019-09-27 13:36, Benjamin GAIGNARD wrote:
>>> On 9/27/19 1:22 PM, Marc Zyngier wrote:
>>>> On 2019-09-27 09:48, Benjamin Gaignard wrote:
>>>>> Adding always-on makes arm arch_timer claim to be an high 
>>>>> resolution
>>>>> timer.
>>>>> That is possible because power mode won't stop clocking the 
>>>>> timer.
>>>>
>>>> The "always-on" is not about the clock. It is about the 
>>>> comparator.
>>>> The clock itself is *guaranteed* to always tick. If it didn't,
>>>> that'd be
>>>> an integration bug, and a pretty bad one.
>>>>
>>>> What you're claiming here is that your CPU never enters a 
>>>> low-power
>>>> mode?
>>>> Ever? I find this very hard to believe.
>>>>
>>>> Furthermore, claiming that always-on is the way to force the 
>>>> arch-timer
>>>> to be an hrtimer is factually wrong. This is what happens *if* 
>>>> this is
>>>> the only timer in the system. The only case this is true is for 
>>>> virtual
>>>> machines. Anything else has a global timer somewhere that will 
>>>> allow
>>>> the arch timers to be used as an hrtimer.
>>>>
>>>> I'm pretty sure you too have a global timer somewhere in your 
>>>> system.
>>>> Enable it, and enjoy hrtimers without having to lie about the
>>>> properties
>>>> of your system! ;-)
>>>
>>> Hi Marc,
>>>
>>> This SoC doesn't have any other global timer. Use arch_time is the 
>>> only
>>> we have to provide hrtimer on this system.
>>
>> And you don't have any form of power management either? What happens 
>> when
>> your CPU goes into idle? If your system does any form of power 
>> management
>> *and* doesn't have a separate timer, it is remarkably broken.
>
> Even in low-power modes this timer is always powered and clocked so 
> it
> is working fine.

You're missing the point again. It is not about the clock, but the 
comparator
that is internal to the CPU, and not functional when the CPU is in its 
lowest
power mode. See also the verbiage in [1] (44.3 STGEN functional 
description),
which indicates that the clock source actually dies in low-power mode 
(going
against the architecture which mandates it to be always-on).

Also, coming back to your earlier assertion ("This SoC doesn't have any 
other
global timer"): The documentation at [1] shows at least 17 timers that 
could
be used and avoid this dirty hack.

So for what it is worth, NAK to this patch.

         M.

[1] https://www.st.com/resource/en/reference_manual/dm00366355.pdf
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] ARM: dts: stm32: Enable high resolution timer

[Sudeep Holla]

On Fri, Sep 27, 2019 at 12:44:55PM +0000, Benjamin GAIGNARD wrote:

[...]
>
> Even in low-power modes this timer is always powered and clocked so it
> is working fine.
>

Is that tested ? I see only cpu_{on,off} available on this platform with
PSCI v0.1. Did you add cpu_suspend, idle-states and then gave it a spin ?
Or do you have some other idle driver with which this is tested ?

Also I don't understand how "always-on" is linked to hrtimer. Always on
timers are just selected to be broadcast timer without sacrificing(simply
keeping) a cpu to be always active for broadcast purposes.

--
Regards,
Sudeep

Re: [PATCH] ARM: dts: stm32: Enable high resolution timer

[Benjamin GAIGNARD]

On 9/27/19 2:59 PM, Marc Zyngier wrote:
> On 2019-09-27 13:44, Benjamin GAIGNARD wrote:
>> On 9/27/19 2:41 PM, Marc Zyngier wrote:
>>> On 2019-09-27 13:36, Benjamin GAIGNARD wrote:
>>>> On 9/27/19 1:22 PM, Marc Zyngier wrote:
>>>>> On 2019-09-27 09:48, Benjamin Gaignard wrote:
>>>>>> Adding always-on makes arm arch_timer claim to be an high resolution
>>>>>> timer.
>>>>>> That is possible because power mode won't stop clocking the timer.
>>>>>
>>>>> The "always-on" is not about the clock. It is about the comparator.
>>>>> The clock itself is *guaranteed* to always tick. If it didn't,
>>>>> that'd be
>>>>> an integration bug, and a pretty bad one.
>>>>>
>>>>> What you're claiming here is that your CPU never enters a low-power
>>>>> mode?
>>>>> Ever? I find this very hard to believe.
>>>>>
>>>>> Furthermore, claiming that always-on is the way to force the 
>>>>> arch-timer
>>>>> to be an hrtimer is factually wrong. This is what happens *if* 
>>>>> this is
>>>>> the only timer in the system. The only case this is true is for 
>>>>> virtual
>>>>> machines. Anything else has a global timer somewhere that will allow
>>>>> the arch timers to be used as an hrtimer.
>>>>>
>>>>> I'm pretty sure you too have a global timer somewhere in your system.
>>>>> Enable it, and enjoy hrtimers without having to lie about the
>>>>> properties
>>>>> of your system! ;-)
>>>>
>>>> Hi Marc,
>>>>
>>>> This SoC doesn't have any other global timer. Use arch_time is the 
>>>> only
>>>> we have to provide hrtimer on this system.
>>>
>>> And you don't have any form of power management either? What happens 
>>> when
>>> your CPU goes into idle? If your system does any form of power 
>>> management
>>> *and* doesn't have a separate timer, it is remarkably broken.
>>
>> Even in low-power modes this timer is always powered and clocked so it
>> is working fine.
>
> You're missing the point again. It is not about the clock, but the 
> comparator
> that is internal to the CPU, and not functional when the CPU is in its 
> lowest
> power mode. See also the verbiage in [1] (44.3 STGEN functional 
> description),
> which indicates that the clock source actually dies in low-power mode 
> (going
> against the architecture which mandates it to be always-on).
>
> Also, coming back to your earlier assertion ("This SoC doesn't have 
> any other
> global timer"): The documentation at [1] shows at least 17 timers that 
> could
> be used and avoid this dirty hack.
>
> So for what it is worth, NAK to this patch.


Hi Marc,

I have listen your remarks and propose another way to solve this issue:

https://lkml.org/lkml/2019/10/9/690

Thanks,

Benjamin


>
>         M.
>
> [1] https://www.st.com/resource/en/reference_manual/dm00366355.pdf

Re: [PATCH] ARM: dts: stm32: Enable high resolution timer

[Marc Zyngier]

On 2019-10-14 10:31, Benjamin GAIGNARD wrote:
> On 9/27/19 2:59 PM, Marc Zyngier wrote:
>> On 2019-09-27 13:44, Benjamin GAIGNARD wrote:
>>> On 9/27/19 2:41 PM, Marc Zyngier wrote:
>>>> On 2019-09-27 13:36, Benjamin GAIGNARD wrote:
>>>>> On 9/27/19 1:22 PM, Marc Zyngier wrote:
>>>>>> On 2019-09-27 09:48, Benjamin Gaignard wrote:
>>>>>>> Adding always-on makes arm arch_timer claim to be an high 
>>>>>>> resolution
>>>>>>> timer.
>>>>>>> That is possible because power mode won't stop clocking the 
>>>>>>> timer.
>>>>>>
>>>>>> The "always-on" is not about the clock. It is about the 
>>>>>> comparator.
>>>>>> The clock itself is *guaranteed* to always tick. If it didn't,
>>>>>> that'd be
>>>>>> an integration bug, and a pretty bad one.
>>>>>>
>>>>>> What you're claiming here is that your CPU never enters a 
>>>>>> low-power
>>>>>> mode?
>>>>>> Ever? I find this very hard to believe.
>>>>>>
>>>>>> Furthermore, claiming that always-on is the way to force the
>>>>>> arch-timer
>>>>>> to be an hrtimer is factually wrong. This is what happens *if*
>>>>>> this is
>>>>>> the only timer in the system. The only case this is true is for
>>>>>> virtual
>>>>>> machines. Anything else has a global timer somewhere that will 
>>>>>> allow
>>>>>> the arch timers to be used as an hrtimer.
>>>>>>
>>>>>> I'm pretty sure you too have a global timer somewhere in your 
>>>>>> system.
>>>>>> Enable it, and enjoy hrtimers without having to lie about the
>>>>>> properties
>>>>>> of your system! ;-)
>>>>>
>>>>> Hi Marc,
>>>>>
>>>>> This SoC doesn't have any other global timer. Use arch_time is 
>>>>> the
>>>>> only
>>>>> we have to provide hrtimer on this system.
>>>>
>>>> And you don't have any form of power management either? What 
>>>> happens
>>>> when
>>>> your CPU goes into idle? If your system does any form of power
>>>> management
>>>> *and* doesn't have a separate timer, it is remarkably broken.
>>>
>>> Even in low-power modes this timer is always powered and clocked so 
>>> it
>>> is working fine.
>>
>> You're missing the point again. It is not about the clock, but the
>> comparator
>> that is internal to the CPU, and not functional when the CPU is in 
>> its
>> lowest
>> power mode. See also the verbiage in [1] (44.3 STGEN functional
>> description),
>> which indicates that the clock source actually dies in low-power 
>> mode
>> (going
>> against the architecture which mandates it to be always-on).
>>
>> Also, coming back to your earlier assertion ("This SoC doesn't have
>> any other
>> global timer"): The documentation at [1] shows at least 17 timers 
>> that
>> could
>> be used and avoid this dirty hack.
>>
>> So for what it is worth, NAK to this patch.
>
>
> Hi Marc,
>
> I have listen your remarks and propose another way to solve this 
> issue:
>
> https://lkml.org/lkml/2019/10/9/690

I don't think you have. You're just trying to move the same dirty hack 
to
another place instead of properly describing your hardware, and Thomas
has pointed you in the same direction.

         M.
-- 
Jazz is not dead. It just smells funny...