* Re: [PATCH v1] ARM: clocksource: make ARM_GLOBAL_TIMER selectable
2016-04-26 19:35 ` Grygorii Strashko
@ 2016-04-27 10:12 ` Liviu Dudau
2016-04-27 13:46 ` Grygorii Strashko
2016-04-27 10:15 ` Robin Murphy
2016-04-27 10:41 ` Russell King - ARM Linux
2 siblings, 1 reply; 11+ messages in thread
From: Liviu Dudau @ 2016-04-27 10:12 UTC (permalink / raw)
To: Grygorii Strashko
Cc: kernel, Tony Lindgren, Sekhar Nori, Masahiro Yamada, Wei Xu,
Jesper Nilsson, Florian Fainelli, Russell King, Daniel Lezcano,
Michal Simek, linux-arm-kernel, linux-rockchip,
bcm-kernel-feedback-list, Arnd Bergmann, Mike Looijmans,
Lars Persson, linux-omap, linux-arm-kernel, Maxime Coquelin,
Srinivas Kandagatla, linux-kernel, Sascha Hauer, Sudeep Holla,
Olof Johansson, Jun Nie, Shawn Guo
On Tue, Apr 26, 2016 at 10:35:08PM +0300, Grygorii Strashko wrote:
> On 04/26/2016 07:02 PM, Liviu Dudau wrote:
> > On Tue, Apr 26, 2016 at 06:28:52PM +0300, Grygorii Strashko wrote:
> >
> > Hi Grygorii,
> >
> > First time I'm seeing this patch, so I have a few questions, mostly
> > related to the commit message:
>
> Hm. You are in cc for RFC.
> Sry, forgot to add link [1].
Hmm, possibly fell through some cracks.
>
> >
> >> This patch intended to fix following cases:
> >> - SoC-A has ARM GT, defines DT node for ARM GT and selects
> >> ARM_GLOBAL_TIMER statically in Kconfig file. SoC-B has ARM GT and
> >> defines DT node for ARM GT, but do not selects ARM_GLOBAL_TIMER
> >> statically in Kconfig file. In case of multiplatform build ARM GT will
> >> be implicitly enabled for SoC-B.
> >
> > Well, SoC-B has the GT *and* the DT node, so what is the problem with
> > enabling it for SoC-B? If there are reasons not to use the Global Timer
> > on SoC-B, surely a better option would be to mark it in DT with status = "disabled";
>
> This was rejected [2]. DT describes HW and if it is functional the status = "disabled"
> is not good choice.
They way I read that thread is that you are trying to disable the Global Timer
for the wrong reasons, not that having status = "disabled" is not a good choice.
DT *does* describe the HW, but there is nothing from stopping someone to say in the DT
that a specific HW is OFF. Removing the node description from DT would be wrong, because
the HW is there, we just want it disabled. Having status = "disabled" is similar to having
a software OFF button.
Now, the only issue one might have is when the HW is enabled at power on and active. In
that case, marking it with status = "disabled" is indeed wrong, because there's going
to be a disconnect between the state of the HW and what the DT says is being active. But
the timers are not really active until setup, so you should be able to play with the
status property.
> ARM GT can't be used as clocksource/sched_clock/clockevent when CPUFreq or
> CPUIdle are enabled :(, and this is Linux specific functionality and
> not HW description.
First of all you probably need to separate the issues you have with TWD versus
the GT timer, they are two different pieces of IP and have individual drivers.
When it comes to CPUidle, the TWD timer does the right thing: if there is no
"always-on" property in the DT then it will set the CLOCK_EVT_FEAT_C3STOP bit in
the features and so the clk framework will know that the clock will be disabled
when the core goes idle and will look for another clock source for wakeups. What
it does seem to be missing is the CLOCK_EVT_FEAT_PERCPU flag though, but that seems
to be a very underused flag in kernel/time/tick-broadcast.c.
As for the GT timer and CPUidle, something similar to the TWD timer needs done (i.e
set C3STOP feature if the clock is not marked as always on).
For the issues that you are seeing with CPUfreq, I'm afraid I need more info on that.
>
> >
> >>
> >> - There is no way to disable ARM GT without modifying Kconfig file,
> >> once ARM_GLOBAL_TIMER is selected statically in Kconfig file.
> >
> > What about disabling the DT node?
> >
> > Not sure I properly understand the problem you are trying to solve here.
>
> I'd like to have way to enable/disable ARM GT without modifying Kernel sources
> (Kconfig specifically) which is now impossible.
>
> >
> >>
> >> Hence, fix above case by defining both HAVE_ARM_GLOBAL_TIMER and
> >> ARM_GLOBAL_TIMER as recommended by 'Adding common features and make
> >> the usage configurable' section in kconfig-language.txt. All places in
> >> ARM folder where ARM_GLOBAL_TIMER was used now replaced on
> >> HAVE_ARM_GLOBAL_TIMER.
> >
> > I'm OK with the way you have changed ARM_GLOBAL_TIMER from a sticky config
> > option to a selectable one, but I would like more details on the problem
> > this was causing you.
> >
>
> The same HW (board) could be used with PM features enabled (power saving)
> and disabled (-RT). Without this change it will require to have
> and maintain two branches, but with it - just separate defconfig.
I'm not NAK-ing the patch for the moment until we understand if there are better
options (and patches) for your problem, but I still fail to see how you are going
to use this patch in the scenario that your commit message describes. If you compile
a kernel to work on both SoC-A and SoC-B you still end up having GT timer enabled for
SoC-B.
Maybe commit message needs changed to say something like "Make ARM_GLOBAL_TIMER a
selectable feature for builds that want to disable the functionality".
Best regards,
Liviu
>
> [1] http://lists.infradead.org/pipermail/linux-rockchip/2016-February/007159.html
> [2] http://www.spinics.net/lists/devicetree/msg102918.html
>
> >
> >>
> >> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> >> Cc: Florian Fainelli <f.fainelli@gmail.com>
> >> Cc: Russell King <linux@arm.linux.org.uk>
> >> Cc: Wei Xu <xuwei5@hisilicon.com>
> >> Cc: Shawn Guo <shawnguo@kernel.org>
> >> Cc: Sascha Hauer <kernel@pengutronix.de>
> >> Cc: Srinivas Kandagatla <srinivas.kandagatla@gmail.com>
> >> Cc: Maxime Coquelin <maxime.coquelin@st.com>
> >> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> >> Cc: Liviu Dudau <liviu.dudau@arm.com>
> >> Cc: Sudeep Holla <sudeep.holla@arm.com>
> >> Cc: Jun Nie <jun.nie@linaro.org>
> >> Cc: Michal Simek <michal.simek@xilinx.com>
> >> Cc: Jesper Nilsson <jesper.nilsson@axis.com>
> >> Cc: Lars Persson <lars.persson@axis.com>
> >> Cc: Mike Looijmans <mike.looijmans@topic.nl>
> >> Acked-by: Sören Brinkmann <soren.brinkmann@xilinx.com>
> >> Acked-by: Moritz Fischer <moritz.fischer@ettus.com>
> >> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> >> ---
> >> Changes is v1:
> >> - updated mach-artpec
> >> - rebased on top of tip: timers/core
> >> commit: 86d3473 time: Introduce do_sys_settimeofday64()
> >>
> >> arch/arm/mach-artpec/Kconfig | 2 +-
> >> arch/arm/mach-bcm/Kconfig | 4 ++--
> >> arch/arm/mach-hisi/Kconfig | 2 +-
> >> arch/arm/mach-imx/Kconfig | 2 +-
> >> arch/arm/mach-rockchip/Kconfig | 2 +-
> >> arch/arm/mach-sti/Kconfig | 2 +-
> >> arch/arm/mach-uniphier/Kconfig | 2 +-
> >> arch/arm/mach-vexpress/Kconfig | 2 +-
> >> arch/arm/mach-zx/Kconfig | 2 +-
> >> arch/arm/mach-zynq/Kconfig | 2 +-
> >> drivers/clocksource/Kconfig | 7 ++++++-
> >> 11 files changed, 17 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/arch/arm/mach-artpec/Kconfig b/arch/arm/mach-artpec/Kconfig
> >> index 6cbe5a2..6cbca77 100644
> >> --- a/arch/arm/mach-artpec/Kconfig
> >> +++ b/arch/arm/mach-artpec/Kconfig
> >> @@ -9,7 +9,7 @@ config MACH_ARTPEC6
> >> depends on ARCH_MULTI_V7
> >> select ARM_AMBA
> >> select ARM_GIC
> >> - select ARM_GLOBAL_TIMER
>
> [...]
>
> --
> regards,
> -grygorii
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v1] ARM: clocksource: make ARM_GLOBAL_TIMER selectable
2016-04-27 10:12 ` Liviu Dudau
@ 2016-04-27 13:46 ` Grygorii Strashko
0 siblings, 0 replies; 11+ messages in thread
From: Grygorii Strashko @ 2016-04-27 13:46 UTC (permalink / raw)
To: Liviu Dudau
Cc: kernel, Tony Lindgren, Sekhar Nori, Masahiro Yamada, Wei Xu,
Jesper Nilsson, Florian Fainelli, Russell King, Daniel Lezcano,
Michal Simek, linux-arm-kernel, linux-rockchip,
bcm-kernel-feedback-list, Arnd Bergmann, Mike Looijmans,
Lars Persson, linux-omap, linux-arm-kernel, Maxime Coquelin,
Srinivas Kandagatla, linux-kernel, Sascha Hauer, Sudeep Holla,
Olof Johansson, Jun Nie, Shawn Guo
On 04/27/2016 01:12 PM, Liviu Dudau wrote:
> On Tue, Apr 26, 2016 at 10:35:08PM +0300, Grygorii Strashko wrote:
>> On 04/26/2016 07:02 PM, Liviu Dudau wrote:
>>> On Tue, Apr 26, 2016 at 06:28:52PM +0300, Grygorii Strashko wrote:
>>>
>>> Hi Grygorii,
>>>
>>> First time I'm seeing this patch, so I have a few questions, mostly
>>> related to the commit message:
>>
>> Hm. You are in cc for RFC.
>> Sry, forgot to add link [1].
>
> Hmm, possibly fell through some cracks.
>
>>
>>>
>>>> This patch intended to fix following cases:
>>>> - SoC-A has ARM GT, defines DT node for ARM GT and selects
>>>> ARM_GLOBAL_TIMER statically in Kconfig file. SoC-B has ARM GT and
>>>> defines DT node for ARM GT, but do not selects ARM_GLOBAL_TIMER
>>>> statically in Kconfig file. In case of multiplatform build ARM GT will
>>>> be implicitly enabled for SoC-B.
>>>
>>> Well, SoC-B has the GT *and* the DT node, so what is the problem with
>>> enabling it for SoC-B? If there are reasons not to use the Global Timer
>>> on SoC-B, surely a better option would be to mark it in DT with status = "disabled";
>>
>> This was rejected [2]. DT describes HW and if it is functional the status = "disabled"
>> is not good choice.
>
> They way I read that thread is that you are trying to disable the Global Timer
> for the wrong reasons, not that having status = "disabled" is not a good choice.
> DT *does* describe the HW, but there is nothing from stopping someone to say in the DT
> that a specific HW is OFF. Removing the node description from DT would be wrong, because
> the HW is there, we just want it disabled. Having status = "disabled" is similar to having
> a software OFF button.
>
> Now, the only issue one might have is when the HW is enabled at power on and active. In
> that case, marking it with status = "disabled" is indeed wrong, because there's going
> to be a disconnect between the state of the HW and what the DT says is being active. But
> the timers are not really active until setup, so you should be able to play with the
> status property.
As I mentioned, I have one HW which can be used in two different configurations
and targeted to perform two different function (RT vs non-RT).
So, is it expected to burn/replace DT in the above case?
Do you replace DT (or u-boot) each time you load new kernel?
>
>> ARM GT can't be used as clocksource/sched_clock/clockevent when CPUFreq or
>> CPUIdle are enabled :(, and this is Linux specific functionality and
>> not HW description.
>
> First of all you probably need to separate the issues you have with TWD versus
> the GT timer, they are two different pieces of IP and have individual drivers.
TWD? I did not mention TWD anywhere here.
>
> When it comes to CPUidle, the TWD timer does the right thing: if there is no
> "always-on" property in the DT then it will set the CLOCK_EVT_FEAT_C3STOP bit in
> the features and so the clk framework will know that the clock will be disabled
> when the core goes idle and will look for another clock source for wakeups. What
> it does seem to be missing is the CLOCK_EVT_FEAT_PERCPU flag though, but that seems
> to be a very underused flag in kernel/time/tick-broadcast.c.
>
> As for the GT timer and CPUidle, something similar to the TWD timer needs done (i.e
> set C3STOP feature if the clock is not marked as always on).
Oh :) There are big difference between clocksource and clockevent devices :(
I assume you are talking about clockevent devices then yes, for these kind
of timers the broadcast timer is used as backup timer in CPUIdle states.
(and TWD is always selected now in favor of ARM GT clockevent device on my HW)
But, I need and use ARM GT as clocksource/sched_clock - for these type of timers
no backup timers can be registered.
Last try to add such functionality was rejected [1]
>
> For the issues that you are seeing with CPUfreq, I'm afraid I need more info on that.
>
>>
>>>
>>>>
>>>> - There is no way to disable ARM GT without modifying Kconfig file,
>>>> once ARM_GLOBAL_TIMER is selected statically in Kconfig file.
>>>
>>> What about disabling the DT node?
>>>
>>> Not sure I properly understand the problem you are trying to solve here.
>>
>> I'd like to have way to enable/disable ARM GT without modifying Kernel sources
>> (Kconfig specifically) which is now impossible.
>>
>>>
>>>>
>>>> Hence, fix above case by defining both HAVE_ARM_GLOBAL_TIMER and
>>>> ARM_GLOBAL_TIMER as recommended by 'Adding common features and make
>>>> the usage configurable' section in kconfig-language.txt. All places in
>>>> ARM folder where ARM_GLOBAL_TIMER was used now replaced on
>>>> HAVE_ARM_GLOBAL_TIMER.
>>>
>>> I'm OK with the way you have changed ARM_GLOBAL_TIMER from a sticky config
>>> option to a selectable one, but I would like more details on the problem
>>> this was causing you.
>>>
>>
>> The same HW (board) could be used with PM features enabled (power saving)
>> and disabled (-RT). Without this change it will require to have
>> and maintain two branches, but with it - just separate defconfig.
>
> I'm not NAK-ing the patch for the moment until we understand if there are better
> options (and patches) for your problem, but I still fail to see how you are going
> to use this patch in the scenario that your commit message describes. If you compile
> a kernel to work on both SoC-A and SoC-B you still end up having GT timer enabled for
> SoC-B.
ok. That fair. I'd need to drop first paragraph. But problem exist anyway
./arch/arm/mach-vexpress/Kconfig: select ARM_GLOBAL_TIMER
vs
./arch/arm/mach-zynq/Kconfig: select ARM_GLOBAL_TIMER if !CPU_FREQ
as example
>
> Maybe commit message needs changed to say something like "Make ARM_GLOBAL_TIMER a
> selectable feature for builds that want to disable the functionality".
I can do that. How about:
---
ARM: clocksource: Make ARM_GLOBAL_TIMER a selectable
There is no way now to disable ARM GT without modifying Kconfig file,
once ARM_GLOBAL_TIMER is selected statically in Kconfig file.
Hence, make ARM_GLOBAL_TIMER a selectable feature for builds that want
to disable that functionality by defining both HAVE_ARM_GLOBAL_TIMER and
ARM_GLOBAL_TIMER as recommended by 'Adding common features and make
the usage configurable' section in kconfig-language.txt. All places in
ARM folder where ARM_GLOBAL_TIMER was used now replaced on
HAVE_ARM_GLOBAL_TIMER.
---
[1] https://lkml.org/lkml/2015/7/6/65
[PATCH] clocksource: Allow toggling between runtime and persistent clocksource for idle
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v1] ARM: clocksource: make ARM_GLOBAL_TIMER selectable
2016-04-26 19:35 ` Grygorii Strashko
2016-04-27 10:12 ` Liviu Dudau
@ 2016-04-27 10:15 ` Robin Murphy
2016-04-27 13:25 ` Grygorii Strashko
2016-04-27 10:41 ` Russell King - ARM Linux
2 siblings, 1 reply; 11+ messages in thread
From: Robin Murphy @ 2016-04-27 10:15 UTC (permalink / raw)
To: Grygorii Strashko, Liviu Dudau
Cc: kernel, Tony Lindgren, Sekhar Nori, Masahiro Yamada, Wei Xu,
Jesper Nilsson, Florian Fainelli, Russell King, Daniel Lezcano,
Michal Simek, linux-arm-kernel, linux-rockchip,
bcm-kernel-feedback-list, Arnd Bergmann, Mike Looijmans,
Lars Persson, linux-omap, linux-arm-kernel, Maxime Coquelin,
Srinivas Kandagatla, linux-kernel, Sascha Hauer, Sudeep Holla,
Olof Johansson, Jun Nie, Shawn Guo
On 26/04/16 20:35, Grygorii Strashko wrote:
> On 04/26/2016 07:02 PM, Liviu Dudau wrote:
>> On Tue, Apr 26, 2016 at 06:28:52PM +0300, Grygorii Strashko wrote:
>>
>> Hi Grygorii,
>>
>> First time I'm seeing this patch, so I have a few questions, mostly
>> related to the commit message:
>
> Hm. You are in cc for RFC.
> Sry, forgot to add link [1].
>
>>
>>> This patch intended to fix following cases:
>>> - SoC-A has ARM GT, defines DT node for ARM GT and selects
>>> ARM_GLOBAL_TIMER statically in Kconfig file. SoC-B has ARM GT and
>>> defines DT node for ARM GT, but do not selects ARM_GLOBAL_TIMER
>>> statically in Kconfig file. In case of multiplatform build ARM GT will
>>> be implicitly enabled for SoC-B.
>>
>> Well, SoC-B has the GT *and* the DT node, so what is the problem with
>> enabling it for SoC-B? If there are reasons not to use the Global Timer
>> on SoC-B, surely a better option would be to mark it in DT with status = "disabled";
>
> This was rejected [2]. DT describes HW and if it is functional the status = "disabled"
> is not good choice.
> ARM GT can't be used as clocksource/sched_clock/clockevent when CPUFreq or
> CPUIdle are enabled :(, and this is Linux specific functionality and
> not HW description.
It sounds to me like the cleanest option might then be to address it
within the Linux-specific driver itself. How feasible would it be for
the GT driver to detect at runtime whether CPUfreq/idle will be used and
simply refuse to register as a clocksource if so? (I guess there are
probably horrible initialisation order issues, at least...)
Robin.
>>
>>>
>>> - There is no way to disable ARM GT without modifying Kconfig file,
>>> once ARM_GLOBAL_TIMER is selected statically in Kconfig file.
>>
>> What about disabling the DT node?
>>
>> Not sure I properly understand the problem you are trying to solve here.
>
> I'd like to have way to enable/disable ARM GT without modifying Kernel sources
> (Kconfig specifically) which is now impossible.
>
>>
>>>
>>> Hence, fix above case by defining both HAVE_ARM_GLOBAL_TIMER and
>>> ARM_GLOBAL_TIMER as recommended by 'Adding common features and make
>>> the usage configurable' section in kconfig-language.txt. All places in
>>> ARM folder where ARM_GLOBAL_TIMER was used now replaced on
>>> HAVE_ARM_GLOBAL_TIMER.
>>
>> I'm OK with the way you have changed ARM_GLOBAL_TIMER from a sticky config
>> option to a selectable one, but I would like more details on the problem
>> this was causing you.
>>
>
> The same HW (board) could be used with PM features enabled (power saving)
> and disabled (-RT). Without this change it will require to have
> and maintain two branches, but with it - just separate defconfig.
>
> [1] http://lists.infradead.org/pipermail/linux-rockchip/2016-February/007159.html
> [2] http://www.spinics.net/lists/devicetree/msg102918.html
>
>>
>>>
>>> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
>>> Cc: Florian Fainelli <f.fainelli@gmail.com>
>>> Cc: Russell King <linux@arm.linux.org.uk>
>>> Cc: Wei Xu <xuwei5@hisilicon.com>
>>> Cc: Shawn Guo <shawnguo@kernel.org>
>>> Cc: Sascha Hauer <kernel@pengutronix.de>
>>> Cc: Srinivas Kandagatla <srinivas.kandagatla@gmail.com>
>>> Cc: Maxime Coquelin <maxime.coquelin@st.com>
>>> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>>> Cc: Liviu Dudau <liviu.dudau@arm.com>
>>> Cc: Sudeep Holla <sudeep.holla@arm.com>
>>> Cc: Jun Nie <jun.nie@linaro.org>
>>> Cc: Michal Simek <michal.simek@xilinx.com>
>>> Cc: Jesper Nilsson <jesper.nilsson@axis.com>
>>> Cc: Lars Persson <lars.persson@axis.com>
>>> Cc: Mike Looijmans <mike.looijmans@topic.nl>
>>> Acked-by: Sören Brinkmann <soren.brinkmann@xilinx.com>
>>> Acked-by: Moritz Fischer <moritz.fischer@ettus.com>
>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>>> ---
>>> Changes is v1:
>>> - updated mach-artpec
>>> - rebased on top of tip: timers/core
>>> commit: 86d3473 time: Introduce do_sys_settimeofday64()
>>>
>>> arch/arm/mach-artpec/Kconfig | 2 +-
>>> arch/arm/mach-bcm/Kconfig | 4 ++--
>>> arch/arm/mach-hisi/Kconfig | 2 +-
>>> arch/arm/mach-imx/Kconfig | 2 +-
>>> arch/arm/mach-rockchip/Kconfig | 2 +-
>>> arch/arm/mach-sti/Kconfig | 2 +-
>>> arch/arm/mach-uniphier/Kconfig | 2 +-
>>> arch/arm/mach-vexpress/Kconfig | 2 +-
>>> arch/arm/mach-zx/Kconfig | 2 +-
>>> arch/arm/mach-zynq/Kconfig | 2 +-
>>> drivers/clocksource/Kconfig | 7 ++++++-
>>> 11 files changed, 17 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-artpec/Kconfig b/arch/arm/mach-artpec/Kconfig
>>> index 6cbe5a2..6cbca77 100644
>>> --- a/arch/arm/mach-artpec/Kconfig
>>> +++ b/arch/arm/mach-artpec/Kconfig
>>> @@ -9,7 +9,7 @@ config MACH_ARTPEC6
>>> depends on ARCH_MULTI_V7
>>> select ARM_AMBA
>>> select ARM_GIC
>>> - select ARM_GLOBAL_TIMER
>
> [...]
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v1] ARM: clocksource: make ARM_GLOBAL_TIMER selectable
2016-04-27 10:15 ` Robin Murphy
@ 2016-04-27 13:25 ` Grygorii Strashko
0 siblings, 0 replies; 11+ messages in thread
From: Grygorii Strashko @ 2016-04-27 13:25 UTC (permalink / raw)
To: Robin Murphy, Liviu Dudau
Cc: kernel, Tony Lindgren, Sekhar Nori, Masahiro Yamada, Wei Xu,
Jesper Nilsson, Florian Fainelli, Russell King, Daniel Lezcano,
Michal Simek, linux-arm-kernel, linux-rockchip,
bcm-kernel-feedback-list, Arnd Bergmann, Mike Looijmans,
Lars Persson, linux-omap, linux-arm-kernel, Maxime Coquelin,
Srinivas Kandagatla, linux-kernel, Sascha Hauer, Sudeep Holla,
Olof Johansson, Jun Nie, Shawn Guo
On 04/27/2016 01:15 PM, Robin Murphy wrote:
> On 26/04/16 20:35, Grygorii Strashko wrote:
>> On 04/26/2016 07:02 PM, Liviu Dudau wrote:
>>> On Tue, Apr 26, 2016 at 06:28:52PM +0300, Grygorii Strashko wrote:
>>>
>>> Hi Grygorii,
>>>
>>> First time I'm seeing this patch, so I have a few questions, mostly
>>> related to the commit message:
>>
>> Hm. You are in cc for RFC.
>> Sry, forgot to add link [1].
>>
>>>
>>>> This patch intended to fix following cases:
>>>> - SoC-A has ARM GT, defines DT node for ARM GT and selects
>>>> ARM_GLOBAL_TIMER statically in Kconfig file. SoC-B has ARM GT and
>>>> defines DT node for ARM GT, but do not selects ARM_GLOBAL_TIMER
>>>> statically in Kconfig file. In case of multiplatform build ARM GT will
>>>> be implicitly enabled for SoC-B.
>>>
>>> Well, SoC-B has the GT *and* the DT node, so what is the problem with
>>> enabling it for SoC-B? If there are reasons not to use the Global Timer
>>> on SoC-B, surely a better option would be to mark it in DT with
>>> status = "disabled";
>>
>> This was rejected [2]. DT describes HW and if it is functional the
>> status = "disabled"
>> is not good choice.
>> ARM GT can't be used as clocksource/sched_clock/clockevent when
>> CPUFreq or
>> CPUIdle are enabled :(, and this is Linux specific functionality and
>> not HW description.
>
> It sounds to me like the cleanest option might then be to address it
> within the Linux-specific driver itself. How feasible would it be for
> the GT driver to detect at runtime whether CPUfreq/idle will be used and
> simply refuse to register as a clocksource if so? (I guess there are
> probably horrible initialisation order issues, at least...)
>
I do not thinK this is really possible, taking into account that
clockevent device might need to be registered very early and
PM features can be enabled/disabled dynamically.
For example, PM is enabled by loading module for TI am437x and am335x SoCs,
but ARM GT is initialized much earlier. Before loading PM module,
it is (theoretically) possible to change clocksource and
clockevent (unbind) devices, but there is no way to change sched_clock
(or timer_delay).
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v1] ARM: clocksource: make ARM_GLOBAL_TIMER selectable
2016-04-26 19:35 ` Grygorii Strashko
2016-04-27 10:12 ` Liviu Dudau
2016-04-27 10:15 ` Robin Murphy
@ 2016-04-27 10:41 ` Russell King - ARM Linux
2016-04-27 13:31 ` Grygorii Strashko
2 siblings, 1 reply; 11+ messages in thread
From: Russell King - ARM Linux @ 2016-04-27 10:41 UTC (permalink / raw)
To: Grygorii Strashko
Cc: Liviu Dudau, Daniel Lezcano, Arnd Bergmann, Olof Johansson,
linux-arm-kernel, Sekhar Nori, linux-omap, Tony Lindgren,
bcm-kernel-feedback-list, linux-kernel, linux-rockchip, kernel,
linux-arm-kernel, Florian Fainelli, Wei Xu, Shawn Guo,
Sascha Hauer, Srinivas Kandagatla, Maxime Coquelin,
Masahiro Yamada, Sudeep Holla, Jun Nie, Michal Simek,
Jesper Nilsson, Lars Persson, Mike Looijmans
On Tue, Apr 26, 2016 at 10:35:08PM +0300, Grygorii Strashko wrote:
> On 04/26/2016 07:02 PM, Liviu Dudau wrote:
> > Well, SoC-B has the GT *and* the DT node, so what is the problem with
> > enabling it for SoC-B? If there are reasons not to use the Global Timer
> > on SoC-B, surely a better option would be to mark it in DT with status = "disabled";
>
> This was rejected [2]. DT describes HW and if it is functional the status = "disabled"
> is not good choice.
> ARM GT can't be used as clocksource/sched_clock/clockevent when CPUFreq or
> CPUIdle are enabled :(, and this is Linux specific functionality and
> not HW description.
Sorry, but we don't want to have to disable drivers in the kernel just
because one platform has a problem (consider the single zImage case
where it may be required that the global timer is enabled for some
platform to boot - it becomes mandatory in single zImage at that point.)
Maybe a linux-specific property is needed here - "linux,low-power-unstable"
or something like that?
--
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v1] ARM: clocksource: make ARM_GLOBAL_TIMER selectable
2016-04-27 10:41 ` Russell King - ARM Linux
@ 2016-04-27 13:31 ` Grygorii Strashko
2016-04-27 16:24 ` Tony Lindgren
2016-04-27 21:06 ` Russell King - ARM Linux
0 siblings, 2 replies; 11+ messages in thread
From: Grygorii Strashko @ 2016-04-27 13:31 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Liviu Dudau, Daniel Lezcano, Arnd Bergmann, Olof Johansson,
linux-arm-kernel, Sekhar Nori, linux-omap, Tony Lindgren,
bcm-kernel-feedback-list, linux-kernel, linux-rockchip, kernel,
linux-arm-kernel, Florian Fainelli, Wei Xu, Shawn Guo,
Sascha Hauer, Srinivas Kandagatla, Maxime Coquelin,
Masahiro Yamada, Sudeep Holla, Jun Nie, Michal Simek,
Jesper Nilsson, Lars Persson, Mike Looijmans
Hi Russell,
On 04/27/2016 01:41 PM, Russell King - ARM Linux wrote:
> On Tue, Apr 26, 2016 at 10:35:08PM +0300, Grygorii Strashko wrote:
>> On 04/26/2016 07:02 PM, Liviu Dudau wrote:
>>> Well, SoC-B has the GT *and* the DT node, so what is the problem with
>>> enabling it for SoC-B? If there are reasons not to use the Global Timer
>>> on SoC-B, surely a better option would be to mark it in DT with status = "disabled";
>>
>> This was rejected [2]. DT describes HW and if it is functional the status = "disabled"
>> is not good choice.
>> ARM GT can't be used as clocksource/sched_clock/clockevent when CPUFreq or
>> CPUIdle are enabled :(, and this is Linux specific functionality and
>> not HW description.
>
> Sorry, but we don't want to have to disable drivers in the kernel just
> because one platform has a problem (consider the single zImage case
> where it may be required that the global timer is enabled for some
> platform to boot - it becomes mandatory in single zImage at that point.)
Sorry, but this patch doesn't disable anything. It provides possibility
to do a custom build with disabled ARM GT driver without Kernel code modification -
in my case RT-kernel and non-RT Kernel should run on same HW and
RT kernel should use ARM GT as clocksource/sched_clock, but
non-RT Kernel shouldn't.
I think RT can't be part of single zImage - it make no sense.
>
> Maybe a linux-specific property is needed here - "linux,low-power-unstable"
> or something like that?
>
I've tried smth. like this [1]
And it is not "unstable", it's will just stop in C3 (CPUIdle) and there no possibility
(at least I've not found how to do this) to adjust Freq in case of CPUFreq.
[above is about clocksource/sched_clock]
[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-November/386858.html
Oh :(. And it's really hard (I even would say - crazy task) to select
proper clocksource/clockevent/sched_clock and timer_delay (opt, arm)
when SoC provides few choices.
Last discussion:
https://lkml.org/lkml/2016/3/17/38
Prev discussions:
http://www.spinics.net/lists/arm-kernel/msg459803.html
http://www.spinics.net/lists/linux-omap/msg123658.html
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v1] ARM: clocksource: make ARM_GLOBAL_TIMER selectable
2016-04-27 13:31 ` Grygorii Strashko
@ 2016-04-27 16:24 ` Tony Lindgren
2016-04-27 21:06 ` Russell King - ARM Linux
1 sibling, 0 replies; 11+ messages in thread
From: Tony Lindgren @ 2016-04-27 16:24 UTC (permalink / raw)
To: Grygorii Strashko
Cc: Russell King - ARM Linux, Liviu Dudau, Daniel Lezcano,
Arnd Bergmann, Olof Johansson, linux-arm-kernel, Sekhar Nori,
linux-omap, bcm-kernel-feedback-list, linux-kernel,
linux-rockchip, kernel, linux-arm-kernel, Florian Fainelli,
Wei Xu, Shawn Guo, Sascha Hauer, Srinivas Kandagatla,
Maxime Coquelin, Masahiro Yamada, Sudeep Holla, Jun Nie,
Michal Simek, Jesper Nilsson, Lars Persson, Mike Looijmans
* Grygorii Strashko <grygorii.strashko@ti.com> [160427 06:32]:
> Hi Russell,
>
> On 04/27/2016 01:41 PM, Russell King - ARM Linux wrote:
> > On Tue, Apr 26, 2016 at 10:35:08PM +0300, Grygorii Strashko wrote:
> >> On 04/26/2016 07:02 PM, Liviu Dudau wrote:
> >>> Well, SoC-B has the GT *and* the DT node, so what is the problem with
> >>> enabling it for SoC-B? If there are reasons not to use the Global Timer
> >>> on SoC-B, surely a better option would be to mark it in DT with status = "disabled";
> >>
> >> This was rejected [2]. DT describes HW and if it is functional the status = "disabled"
> >> is not good choice.
> >> ARM GT can't be used as clocksource/sched_clock/clockevent when CPUFreq or
> >> CPUIdle are enabled :(, and this is Linux specific functionality and
> >> not HW description.
> >
> > Sorry, but we don't want to have to disable drivers in the kernel just
> > because one platform has a problem (consider the single zImage case
> > where it may be required that the global timer is enabled for some
> > platform to boot - it becomes mandatory in single zImage at that point.)
>
> Sorry, but this patch doesn't disable anything. It provides possibility
> to do a custom build with disabled ARM GT driver without Kernel code modification -
> in my case RT-kernel and non-RT Kernel should run on same HW and
> RT kernel should use ARM GT as clocksource/sched_clock, but
> non-RT Kernel shouldn't.
How about a kernel cmdline option for both local timers to disable
the selected timer(s)?
Tony
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v1] ARM: clocksource: make ARM_GLOBAL_TIMER selectable
2016-04-27 13:31 ` Grygorii Strashko
2016-04-27 16:24 ` Tony Lindgren
@ 2016-04-27 21:06 ` Russell King - ARM Linux
1 sibling, 0 replies; 11+ messages in thread
From: Russell King - ARM Linux @ 2016-04-27 21:06 UTC (permalink / raw)
To: Grygorii Strashko
Cc: Liviu Dudau, Daniel Lezcano, Arnd Bergmann, Olof Johansson,
linux-arm-kernel, Sekhar Nori, linux-omap, Tony Lindgren,
bcm-kernel-feedback-list, linux-kernel, linux-rockchip, kernel,
linux-arm-kernel, Florian Fainelli, Wei Xu, Shawn Guo,
Sascha Hauer, Srinivas Kandagatla, Maxime Coquelin,
Masahiro Yamada, Sudeep Holla, Jun Nie, Michal Simek,
Jesper Nilsson, Lars Persson, Mike Looijmans
On Wed, Apr 27, 2016 at 04:31:05PM +0300, Grygorii Strashko wrote:
> Sorry, but this patch doesn't disable anything. It provides possibility
> to do a custom build with disabled ARM GT driver without Kernel code
> modification - in my case RT-kernel and non-RT Kernel should run on same
> HW and RT kernel should use ARM GT as clocksource/sched_clock, but
> non-RT Kernel shouldn't.
>
> I think RT can't be part of single zImage - it make no sense.
>
> >
> > Maybe a linux-specific property is needed here - "linux,low-power-unstable"
> > or something like that?
> >
>
> I've tried smth. like this [1]
>
> And it is not "unstable", it's will just stop in C3 (CPUIdle) and there no possibility
> (at least I've not found how to do this) to adjust Freq in case of CPUFreq.
> [above is about clocksource/sched_clock]
>
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-November/386858.html
Don't think I'm going to follow URLs - I don't use a desktop mail
client, and as I'm several days behind with email, I don't have time
to go looking through a crappy pipermail web archive at the moment
either.
Anyway, why can't we use the priority system for clocksources? If
ARM GT is problematical, we need a way to register it with a lower
priority than the preferred time keeping source. That's exactly
what the clocksource priority field is for.
--
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 11+ messages in thread