[RESEND PATCH] ARM: dts: make arch-timer always on in rk3288 soc

[RESEND PATCH] ARM: dts: make arch-timer always on in rk3288 soc

[Kever Yang]

We need use the hrtimer, which need the arch-timer to be 'always-on'

Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
---

 arch/arm/boot/dts/rk3288.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
index 5950b0a..698e6ea 100644
--- a/arch/arm/boot/dts/rk3288.dtsi
+++ b/arch/arm/boot/dts/rk3288.dtsi
@@ -76,6 +76,7 @@
 			     <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>,
 			     <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
 		clock-frequency = <24000000>;
+		always-on;
 	};
 
 	i2c1: i2c@ff140000 {
-- 
1.9.1

Re: [RESEND PATCH] ARM: dts: make arch-timer always on in rk3288 soc

[Mark Rutland]

Hi Kever,

On Thu, Aug 28, 2014 at 02:40:17AM +0100, Kever Yang wrote:
> We need use the hrtimer, which need the arch-timer to be 'always-on'

I asked a question on the last posting [1]. Can you please confirm
either way?

Thanks,
Mark.

[1] lists.infradead.org/pipermail/linux-arm-kernel/2014-August/282327.html

> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> ---
> 
>  arch/arm/boot/dts/rk3288.dtsi | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
> index 5950b0a..698e6ea 100644
> --- a/arch/arm/boot/dts/rk3288.dtsi
> +++ b/arch/arm/boot/dts/rk3288.dtsi
> @@ -76,6 +76,7 @@
>  			     <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>,
>  			     <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
>  		clock-frequency = <24000000>;
> +		always-on;
>  	};
>  
>  	i2c1: i2c@ff140000 {
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [RESEND PATCH] ARM: dts: make arch-timer always on in rk3288 soc

[Mark Rutland]

On Thu, Aug 28, 2014 at 10:17:58AM +0100, Mark Rutland wrote:
> Hi Kever,
> 
> On Thu, Aug 28, 2014 at 02:40:17AM +0100, Kever Yang wrote:
> > We need use the hrtimer, which need the arch-timer to be 'always-on'
> 
> I asked a question on the last posting [1]. Can you please confirm
> either way?
> 
> Thanks,
> Mark.
> 
> [1] lists.infradead.org/pipermail/linux-arm-kernel/2014-August/282327.html

To clarify: if there are low power states that the CPU can enter where
we lose state, then this patch isn't correct.

A more general approach would be to enable the broadcast hrtimer for
arm, as has been done for arm64.

See commit 5d1638acb9f6 (tick: Introduce hrtimer based broadcast) which
introduced the broadcast hrtimer, and commit 9358d755bd5c (arm64:
kernel: initialize broadcast hrtimer based clock event device) which
added the requisite plumbing for arm64.

Mark.

> 
> > Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> > ---
> > 
> >  arch/arm/boot/dts/rk3288.dtsi | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
> > index 5950b0a..698e6ea 100644
> > --- a/arch/arm/boot/dts/rk3288.dtsi
> > +++ b/arch/arm/boot/dts/rk3288.dtsi
> > @@ -76,6 +76,7 @@
> >  			     <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>,
> >  			     <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
> >  		clock-frequency = <24000000>;
> > +		always-on;
> >  	};
> >  
> >  	i2c1: i2c@ff140000 {
> > -- 
> > 1.9.1
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe devicetree" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [RESEND PATCH] ARM: dts: make arch-timer always on in rk3288 soc

[Kever Yang]

Mark,

     Thanks for your reply and advice.

On 08/28/2014 11:11 PM, Mark Rutland wrote:
> On Thu, Aug 28, 2014 at 10:17:58AM +0100, Mark Rutland wrote:
>> Hi Kever,
>>
>> On Thu, Aug 28, 2014 at 02:40:17AM +0100, Kever Yang wrote:
>>> We need use the hrtimer, which need the arch-timer to be 'always-on'
>> I asked a question on the last posting [1]. Can you please confirm
>> either way?
>>
>> Thanks,
>> Mark.
>>
>> [1] lists.infradead.org/pipermail/linux-arm-kernel/2014-August/282327.html
> To clarify: if there are low power states that the CPU can enter where
> we lose state, then this patch isn't correct.
rk3288 has low power state and may turn off the cpu power domain
which will lost any logic state in cpu.
>
> A more general approach would be to enable the broadcast hrtimer for
> arm, as has been done for arm64.
>
> See commit 5d1638acb9f6 (tick: Introduce hrtimer based broadcast) which
> introduced the broadcast hrtimer, and commit 9358d755bd5c (arm64:
> kernel: initialize broadcast hrtimer based clock event device) which
> added the requisite plumbing for arm64.
I'll going to implement this and send another patch.

Thanks.

-Kever

Re: [RESEND PATCH] ARM: dts: make arch-timer always on in rk3288 soc

[Huang Tao]

Hi, Mark:

在 2014年08月28日 23:11, Mark Rutland 写道:
> To clarify: if there are low power states that the CPU can enter where
> we lose state, then this patch isn't correct.
Right now, the software of RK3288 SoC only support CPU hotplug
(cpu_on/off) and power off all CPUs on suspend.
We do not implement cpuidle to power off CPU. Do you think we should
introduce a broadcast timer?
On our early kernel, I never see any interrupt on a broadcast timer
(yes, we implement it with a external timer).
>
> A more general approach would be to enable the broadcast hrtimer for
> arm, as has been done for arm64.
Yes. I think it should be done by arm framework.
>
> See commit 5d1638acb9f6 (tick: Introduce hrtimer based broadcast) which
> introduced the broadcast hrtimer, and commit 9358d755bd5c (arm64:
> kernel: initialize broadcast hrtimer based clock event device) which
> added the requisite plumbing for arm64.

Re: [RESEND PATCH] ARM: dts: make arch-timer always on in rk3288 soc

[Mark Rutland]

On Fri, Aug 29, 2014 at 04:06:40AM +0100, Huang Tao wrote:
> Hi, Mark:

Hi,

> 在 2014年08月28日 23:11, Mark Rutland 写道:
> > To clarify: if there are low power states that the CPU can enter where
> > we lose state, then this patch isn't correct.
> Right now, the software of RK3288 SoC only support CPU hotplug
> (cpu_on/off) and power off all CPUs on suspend.

Sure, but that's a Linux implementation detail rather than a fixed
property of the hardware. Given those states exist, the "always-on"
property is not appropriate.

> We do not implement cpuidle to power off CPU. Do you think we should
> introduce a broadcast timer?

If one is present, yes. 

> On our early kernel, I never see any interrupt on a broadcast timer
> (yes, we implement it with a external timer).

That's fine; Linux doesn't need to use it just yet. However, when we
want to use low power states later, it will be necessary to enable
placing all CPUS into a low power state.

If your external timer is already supported by an existing driver, there
is no reason not to add it now.

> > A more general approach would be to enable the broadcast hrtimer for
> > arm, as has been done for arm64.
> Yes. I think it should be done by arm framework.

Patches welcome.

I also think it would make sense to enable this for arm.

Thanks,
Mark.

Re: [RESEND PATCH] ARM: dts: make arch-timer always on in rk3288 soc

[Huang Tao]

Hi,

在 2014年08月29日 19:22, Mark Rutland 写道:
>> 在 2014年08月28日 23:11, Mark Rutland 写道:
>>> To clarify: if there are low power states that the CPU can enter where
>>> we lose state, then this patch isn't correct.
>> Right now, the software of RK3288 SoC only support CPU hotplug
>> (cpu_on/off) and power off all CPUs on suspend.
> Sure, but that's a Linux implementation detail rather than a fixed
> property of the hardware. Given those states exist, the "always-on"
> property is not appropriate.
>
>> We do not implement cpuidle to power off CPU. Do you think we should
>> introduce a broadcast timer?
> If one is present, yes. 
>
>> On our early kernel, I never see any interrupt on a broadcast timer
>> (yes, we implement it with a external timer).
> That's fine; Linux doesn't need to use it just yet. However, when we
> want to use low power states later, it will be necessary to enable
> placing all CPUS into a low power state.
>
> If your external timer is already supported by an existing driver, there
> is no reason not to add it now.
>
>>> A more general approach would be to enable the broadcast hrtimer for
>>> arm, as has been done for arm64.
>> Yes. I think it should be done by arm framework.
> Patches welcome.
>
> I also think it would make sense to enable this for arm.
>
> Thanks,
> Mark.
>
>
Okay, so this patch is wrong as I expected.
Thank you!