[4.4-rc][PATCH] ARM: dts: am4372: disable arm twd and global timer's nodes

[4.4-rc][PATCH] ARM: dts: am4372: disable arm twd and global timer's nodes

[Grygorii Strashko]

Keep ARM TWD and Global timer's nodes disabled by default - if someone
would like to use them then those nodes have to be enabled explicitly
in board file.

The reason for this change is:
- ARM TWD is not always-on timer on am437x and it will stop in low
  CPUIdle states and, therefore, broadcast timer has to configured
  properly if CPU_IDLE=y.
- ARM Global timer is not always-on timer on am437x and it can't be
  used as clocksource device if CPU_IDLE=y.
- ARM Global timer driver doesn't support CPUfreq now.

Cc: Felipe Balbi <balbi@ti.com>
Cc: Santosh Shilimkar <ssantosh@kernel.org>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 arch/arm/boot/dts/am4372.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/am4372.dtsi b/arch/arm/boot/dts/am4372.dtsi
index d83ff9c..11376e3 100644
--- a/arch/arm/boot/dts/am4372.dtsi
+++ b/arch/arm/boot/dts/am4372.dtsi
@@ -75,6 +75,7 @@
 		interrupts = <GIC_PPI 11 IRQ_TYPE_LEVEL_HIGH>;
 		interrupt-parent = <&gic>;
 		clocks = <&dpll_mpu_m2_ck>;
+		status = "disabled";
 	};
 
 	local_timer: timer at 48240600 {
@@ -83,6 +84,7 @@
 		interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_HIGH>;
 		interrupt-parent = <&gic>;
 		clocks = <&dpll_mpu_m2_ck>;
+		status = "disabled";
 	};
 
 	l2-cache-controller at 48242000 {
-- 
2.6.3

[4.4-rc][PATCH] ARM: dts: am4372: disable arm twd and global timer's nodes

[Mark Rutland]

On Wed, Nov 18, 2015 at 04:01:55PM +0200, Grygorii Strashko wrote:
> Keep ARM TWD and Global timer's nodes disabled by default - if someone
> would like to use them then those nodes have to be enabled explicitly
> in board file.
> 
> The reason for this change is:
> - ARM TWD is not always-on timer on am437x and it will stop in low
>   CPUIdle states and, therefore, broadcast timer has to configured
>   properly if CPU_IDLE=y.
> - ARM Global timer is not always-on timer on am437x and it can't be
>   used as clocksource device if CPU_IDLE=y.

I don't understand. What timer do you use in the absence of a TWD, and
if it is better why is it not used even if TWD is present?

> - ARM Global timer driver doesn't support CPUfreq now.

Surely that should be fixed in the driver (e.g. make it fail to probe if
CPUfreq is present)? It's broken for any other users too...

Mark.

> Cc: Felipe Balbi <balbi@ti.com>
> Cc: Santosh Shilimkar <ssantosh@kernel.org>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> ---
>  arch/arm/boot/dts/am4372.dtsi | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/am4372.dtsi b/arch/arm/boot/dts/am4372.dtsi
> index d83ff9c..11376e3 100644
> --- a/arch/arm/boot/dts/am4372.dtsi
> +++ b/arch/arm/boot/dts/am4372.dtsi
> @@ -75,6 +75,7 @@
>  		interrupts = <GIC_PPI 11 IRQ_TYPE_LEVEL_HIGH>;
>  		interrupt-parent = <&gic>;
>  		clocks = <&dpll_mpu_m2_ck>;
> +		status = "disabled";
>  	};
>  
>  	local_timer: timer at 48240600 {
> @@ -83,6 +84,7 @@
>  		interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_HIGH>;
>  		interrupt-parent = <&gic>;
>  		clocks = <&dpll_mpu_m2_ck>;
> +		status = "disabled";
>  	};
>  
>  	l2-cache-controller at 48242000 {
> -- 
> 2.6.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[4.4-rc][PATCH] ARM: dts: am4372: disable arm twd and global timer's nodes

[Grygorii Strashko]

Hi Mark,

On 11/18/2015 04:15 PM, Mark Rutland wrote:
> On Wed, Nov 18, 2015 at 04:01:55PM +0200, Grygorii Strashko wrote:
>> Keep ARM TWD and Global timer's nodes disabled by default - if someone
>> would like to use them then those nodes have to be enabled explicitly
>> in board file.
>>
>> The reason for this change is:
>> - ARM TWD is not always-on timer on am437x and it will stop in low
>>    CPUIdle states and, therefore, broadcast timer has to configured
>>    properly if CPU_IDLE=y.
>> - ARM Global timer is not always-on timer on am437x and it can't be
>>    used as clocksource device if CPU_IDLE=y.
> 
> I don't understand. What timer do you use in the absence of a TWD, and
> if it is better why is it not used even if TWD is present?

OMAP GP timer as clockevent device
"ti,omap-counter32k" as clocksource
both are in wakeup (always-on) PM domain.

I see some problems with selecting clocksource/event device (but may be I'm mistaken):
- Current clock event will be selected using "rating" field in clock_event_device
  structure and now ARM TWD has higer value (350) vs OMAP GP timer (300),
  so ARM TWD will be always selected if it's present. 
- we have to force all am437x user to use kernel cmdline parameter "clocksource="
  if ARM Global timer is present, but shouldn't be used. But even in this case,
  it will be selected as sched_clock.

We can see benefits from using these timers when Power mangement is not the case,
for example on -RT kernel where CPUIdle is not the target.

By this change, and taking into account discovered issues, I want to make people,
who would like to use these timers on AM437x based boards, responsible for such
decision with assumption that they know what they are doing.
And this is simplest way I found to disable those timers without modifying kernel.

> 
>> - ARM Global timer driver doesn't support CPUfreq now.
> 
> Surely that should be fixed in the driver (e.g. make it fail to probe if
> CPUfreq is present)? It's broken for any other users too...

May be.

Also, there is additional issue with ARM global timer which I've not
mentioned here, because I sent the fix for it already (in progress):
https://lkml.org/lkml/2015/11/13/456

and there is ongoing discussion:
http://www.spinics.net/lists/arm-kernel/msg459649.html

-- 
regards,
-grygorii

[4.4-rc][PATCH] ARM: dts: am4372: disable arm twd and global timer's nodes

[Tony Lindgren]

* Grygorii Strashko <grygorii.strashko@ti.com> [151118 07:36]:
> Hi Mark,
> 
> On 11/18/2015 04:15 PM, Mark Rutland wrote:
> > On Wed, Nov 18, 2015 at 04:01:55PM +0200, Grygorii Strashko wrote:
> >> Keep ARM TWD and Global timer's nodes disabled by default - if someone
> >> would like to use them then those nodes have to be enabled explicitly
> >> in board file.
> >>
> >> The reason for this change is:
> >> - ARM TWD is not always-on timer on am437x and it will stop in low
> >>    CPUIdle states and, therefore, broadcast timer has to configured
> >>    properly if CPU_IDLE=y.
> >> - ARM Global timer is not always-on timer on am437x and it can't be
> >>    used as clocksource device if CPU_IDLE=y.
> > 
> > I don't understand. What timer do you use in the absence of a TWD, and
> > if it is better why is it not used even if TWD is present?
> 
> OMAP GP timer as clockevent device
> "ti,omap-counter32k" as clocksource
> both are in wakeup (always-on) PM domain.
> 
> I see some problems with selecting clocksource/event device (but may be I'm mistaken):
> - Current clock event will be selected using "rating" field in clock_event_device
>   structure and now ARM TWD has higer value (350) vs OMAP GP timer (300),
>   so ARM TWD will be always selected if it's present. 
> - we have to force all am437x user to use kernel cmdline parameter "clocksource="
>   if ARM Global timer is present, but shouldn't be used. But even in this case,
>   it will be selected as sched_clock.

Yeah makes sense to me. Eventually when I get my clocksource switch patches
updated this problem will go away and we can use a local timer clocksource
during runtime and slower always on timer during idle like we already do
for clockevents.

> We can see benefits from using these timers when Power mangement is not the case,
> for example on -RT kernel where CPUIdle is not the target.

Yes we really want to use local timers for RT during runtime.

> By this change, and taking into account discovered issues, I want to make people,
> who would like to use these timers on AM437x based boards, responsible for such
> decision with assumption that they know what they are doing.
> And this is simplest way I found to disable those timers without modifying kernel.
> 
> > 
> >> - ARM Global timer driver doesn't support CPUfreq now.
> > 
> > Surely that should be fixed in the driver (e.g. make it fail to probe if
> > CPUfreq is present)? It's broken for any other users too...
> 
> May be.

I think it's only broken for users who have PM entering deeper idle states
where the ARM core is powered off.

The selection of the timer needs to be done in the board specific dts file as we
already have cases where RT works but PM does not and should use the local timer,
and we have boards where PM works and we must use the always on timer.

> Also, there is additional issue with ARM global timer which I've not
> mentioned here, because I sent the fix for it already (in progress):
> https://lkml.org/lkml/2015/11/13/456
> 
> and there is ongoing discussion:
> http://www.spinics.net/lists/arm-kernel/msg459649.html

It seems we should apply this as a fix unless somebody has better ideas.

Regards,

Tony

[4.4-rc][PATCH] ARM: dts: am4372: disable arm twd and global timer's nodes

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [151203 08:34]:
>
> It seems we should apply this as a fix unless somebody has better ideas.

Actually I think the fix for now is "[4.4-rc][PATCH v2] ARM: dts: am4372: fix
clock source for arm twd and global timers" until PM starts working?

Regards,

Tony

[4.4-rc][PATCH] ARM: dts: am4372: disable arm twd and global timer's nodes

[Grygorii Strashko]

On 12/03/2015 06:37 PM, Tony Lindgren wrote:
> * Tony Lindgren <tony@atomide.com> [151203 08:34]:
>>
>> It seems we should apply this as a fix unless somebody has better ideas.
> 
> Actually I think the fix for now is "[4.4-rc][PATCH v2] ARM: dts: am4372: fix
> clock source for arm twd and global timers" until PM starts working?
> 

Yes. if no PM it should work, but PM patches are on the fly for Am43.

But, again, main reason I've posted this patch is that I do not see the
way how to enable/disable or limit functionality of these timer by using 
Kconfig options in case of Muliplatform builds :( (or by boot parameters).

Example 1 (this is how 4.4 kernel is working):
1) OMAP4 has ARM TWD timer and properly supports it, so in Kconfig we have
"select HAVE_ARM_TWD" and corresponding "local-timer" is defined in DT.
AM437x has ARM TWD timer, but does not support it yet, so corresponding "local-timer"
defined in DT and in Kconfig HAVE_ARM_TWD is not selected.
(patch http://www.spinics.net/lists/arm-kernel/msg459649.html
 is not merged yet)

Result: ARM TWD timer will be enabled on AM437x !? :(

Example 2:
ARM Global timer can't be used as clocksource device if CPUFreq is enabled
(this is not supported even by Timekeeping core now).

So, there are two options:
1) update selection for config SOC_AM43XX
+ select ARM_GLOBAL_TIMER if !CPU_FREQ
2) updated ARM GT driver and don't register clocksource device if CPUFreq is enabled

Both options will not work:
1) ARM_GLOBAL_TIMER will be selected if any other SoC in multiplatform build
will select ARM_GLOBAL_TIMER
2) clocksource device will never be registered if at least one SoC will
select CPU_FREQ. And this might break SoC which expect it to be registered,
because CPU_FREQ is not enabled on these SoCs (no CPUFreq drivers registered).

So, as alternative to this patch, ARM timers nodes can be disabled in
platform DTS files, but then all platform DTS files will need to be updated.

-- 
regards,
-grygorii

[4.4-rc][PATCH] ARM: dts: am4372: disable arm twd and global timer's nodes

[Tony Lindgren]

* Grygorii Strashko <grygorii.strashko@ti.com> [151203 10:21]:
> On 12/03/2015 06:37 PM, Tony Lindgren wrote:
> > * Tony Lindgren <tony@atomide.com> [151203 08:34]:
> >>
> >> It seems we should apply this as a fix unless somebody has better ideas.
> > 
> > Actually I think the fix for now is "[4.4-rc][PATCH v2] ARM: dts: am4372: fix
> > clock source for arm twd and global timers" until PM starts working?
> > 
> 
> Yes. if no PM it should work, but PM patches are on the fly for Am43.

OK let's not merge this one then.

> But, again, main reason I've posted this patch is that I do not see the
> way how to enable/disable or limit functionality of these timer by using 
> Kconfig options in case of Muliplatform builds :( (or by boot parameters).
> 
> Example 1 (this is how 4.4 kernel is working):
> 1) OMAP4 has ARM TWD timer and properly supports it, so in Kconfig we have
> "select HAVE_ARM_TWD" and corresponding "local-timer" is defined in DT.
> AM437x has ARM TWD timer, but does not support it yet, so corresponding "local-timer"
> defined in DT and in Kconfig HAVE_ARM_TWD is not selected.
> (patch http://www.spinics.net/lists/arm-kernel/msg459649.html
>  is not merged yet)
> 
> Result: ARM TWD timer will be enabled on AM437x !? :(
> 
> Example 2:
> ARM Global timer can't be used as clocksource device if CPUFreq is enabled
> (this is not supported even by Timekeeping core now).
> 
> So, there are two options:
> 1) update selection for config SOC_AM43XX
> + select ARM_GLOBAL_TIMER if !CPU_FREQ
> 2) updated ARM GT driver and don't register clocksource device if CPUFreq is enabled
> 
> Both options will not work:
> 1) ARM_GLOBAL_TIMER will be selected if any other SoC in multiplatform build
> will select ARM_GLOBAL_TIMER
> 2) clocksource device will never be registered if at least one SoC will
> select CPU_FREQ. And this might break SoC which expect it to be registered,
> because CPU_FREQ is not enabled on these SoCs (no CPUFreq drivers registered).
> 
> So, as alternative to this patch, ARM timers nodes can be disabled in
> platform DTS files, but then all platform DTS files will need to be updated.

Yes I don't see any other option right now until we have the clocksource
switching happening. Only then will the scoring work properly.

Regards,

Tony