[PATCH] ARM: Kconfig: select GENERIC_CLOCKEVENTS_BROADCAST also on UP

[PATCH] ARM: Kconfig: select GENERIC_CLOCKEVENTS_BROADCAST also on UP

[Lucas Stach]

Quite a few systems have deeper idle states where the local timer tick
stops to fire, that rely on the GENERIC_CLOCKEVENTS_BROADCAST
infrastructure to switch to a global (non-stopping) clockevent device.
This is also true on UP systems, so select GENERIC_CLOCKEVENTS_BROADCAST
unconditionally.

Instead only select ARCH_HAS_TICK_BROADCAST if SMP is present, as this
is only implemented for SMP systems.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 arch/arm/Kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 90542db1220d..745843ea769c 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -4,7 +4,7 @@ config ARM
 	select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE
 	select ARCH_HAS_DEVMEM_IS_ALLOWED
 	select ARCH_HAS_ELF_RANDOMIZE
-	select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
+	select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST && SMP
 	select ARCH_HAVE_CUSTOM_GPIO_H
 	select ARCH_HAS_GCOV_PROFILE_ALL
 	select ARCH_MIGHT_HAVE_PC_PARPORT
@@ -20,7 +20,7 @@ config ARM
 	select EDAC_ATOMIC_SCRUB
 	select GENERIC_ALLOCATOR
 	select GENERIC_ATOMIC64 if (CPU_V7M || CPU_V6 || !CPU_32v6K || !AEABI)
-	select GENERIC_CLOCKEVENTS_BROADCAST if SMP
+	select GENERIC_CLOCKEVENTS_BROADCAST
 	select GENERIC_EARLY_IOREMAP
 	select GENERIC_IDLE_POLL_SETUP
 	select GENERIC_IRQ_PROBE
-- 
2.8.1

[PATCH] ARM: Kconfig: select GENERIC_CLOCKEVENTS_BROADCAST also on UP

[Lucas Stach]

Hi Russell,

can you please take a look at this and tell if there is something wrong
about it, or if you think it can go in?

Without this patch the kernel is unable to use the deeper CPU idle
states on i.MX6S if it isn't built with SMP support enabled.

Regards,
Lucas

Am Freitag, den 08.07.2016, 17:58 +0200 schrieb Lucas Stach:
> Quite a few systems have deeper idle states where the local timer tick
> stops to fire, that rely on the GENERIC_CLOCKEVENTS_BROADCAST
> infrastructure to switch to a global (non-stopping) clockevent device.
> This is also true on UP systems, so select GENERIC_CLOCKEVENTS_BROADCAST
> unconditionally.
> 
> Instead only select ARCH_HAS_TICK_BROADCAST if SMP is present, as this
> is only implemented for SMP systems.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  arch/arm/Kconfig | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 90542db1220d..745843ea769c 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -4,7 +4,7 @@ config ARM
>  	select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE
>  	select ARCH_HAS_DEVMEM_IS_ALLOWED
>  	select ARCH_HAS_ELF_RANDOMIZE
> -	select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
> +	select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST && SMP
>  	select ARCH_HAVE_CUSTOM_GPIO_H
>  	select ARCH_HAS_GCOV_PROFILE_ALL
>  	select ARCH_MIGHT_HAVE_PC_PARPORT
> @@ -20,7 +20,7 @@ config ARM
>  	select EDAC_ATOMIC_SCRUB
>  	select GENERIC_ALLOCATOR
>  	select GENERIC_ATOMIC64 if (CPU_V7M || CPU_V6 || !CPU_32v6K || !AEABI)
> -	select GENERIC_CLOCKEVENTS_BROADCAST if SMP
> +	select GENERIC_CLOCKEVENTS_BROADCAST
>  	select GENERIC_EARLY_IOREMAP
>  	select GENERIC_IDLE_POLL_SETUP
>  	select GENERIC_IRQ_PROBE

[PATCH] ARM: Kconfig: select GENERIC_CLOCKEVENTS_BROADCAST also on UP

[Russell King - ARM Linux]

On Fri, Sep 16, 2016 at 11:32:20AM +0200, Lucas Stach wrote:
> Hi Russell,
> 
> can you please take a look at this and tell if there is something wrong
> about it, or if you think it can go in?
> 
> Without this patch the kernel is unable to use the deeper CPU idle
> states on i.MX6S if it isn't built with SMP support enabled.

Without spending a lot of time digging into this code, working out what
these configuration symbols do, I frankly don't know if it's a sensible
thing to do or not.  I guess tglx knows this code like the back of his
hand, and would be in a better position to comment.

There's two factors there:
1. the amount of undocumented code in the kernel needing the code to be
   read and understood to understand various aspects of it from the
   architecture point of view.

2. your architecture maintainer has done very little actual platform
   development (not through his own choice) for the last 10 or so years,
   which means he's not had to dig into these areas.

So, I'm afraid I feel less than qualified on this at the moment.  I'll
try to look into this and talk to tglx to work out whether it's a
sensible change.  As I never had a reply to my last email to tglx about
threaded interrupts vs tasklets (for a reported performance regression
in my crypto patches already queued for the next merge window) I'm not
hoping for much help.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH] ARM: Kconfig: select GENERIC_CLOCKEVENTS_BROADCAST also on UP

[Thomas Gleixner]

On Fri, 16 Sep 2016, Russell King - ARM Linux wrote:
> On Fri, Sep 16, 2016 at 11:32:20AM +0200, Lucas Stach wrote:
> > Hi Russell,
> > 
> > can you please take a look at this and tell if there is something wrong
> > about it, or if you think it can go in?
> > 
> > Without this patch the kernel is unable to use the deeper CPU idle
> > states on i.MX6S if it isn't built with SMP support enabled.
> 
> Without spending a lot of time digging into this code, working out what
> these configuration symbols do, I frankly don't know if it's a sensible
> thing to do or not.  I guess tglx knows this code like the back of his
> hand, and would be in a better position to comment.
> 
> There's two factors there:
> 1. the amount of undocumented code in the kernel needing the code to be
>    read and understood to understand various aspects of it from the
>    architecture point of view.

Yeah. I know that this stuff lacks documentation.
 
> 2. your architecture maintainer has done very little actual platform
>    development (not through his own choice) for the last 10 or so years,
>    which means he's not had to dig into these areas.
> 
> So, I'm afraid I feel less than qualified on this at the moment.  I'll
> try to look into this and talk to tglx to work out whether it's a
> sensible change. 

The broadcast feature has the following functionality:

    It lets you use fast accessible (cpu local) timers for normal operation
    and in case of deep idle sleeps where the cpu local timer stops switch
    to broadcast operation, which arms a slower to access (global) timer
    device which does not stop in deep idle.

    On UP we usually just use the global device, but on SMP we need the cpu
    local timers for normal (non idle) operation. Though there is no reason
    to restrict this UP from a technical POV especially when the local
    timer is way cheaper to access than the other device.

Hope that helps.

> As I never had a reply to my last email to tglx about threaded interrupts
> vs tasklets (for a reported performance regression in my crypto patches
> already queued for the next merge window) I'm not hoping for much help.

Sorry, got lost in my mail backlog. Looking at it next.

Thanks,

	tglx

[PATCH] ARM: Kconfig: select GENERIC_CLOCKEVENTS_BROADCAST also on UP

[Lucas Stach]

Am Freitag, den 16.09.2016, 12:30 +0200 schrieb Thomas Gleixner:
> On Fri, 16 Sep 2016, Russell King - ARM Linux wrote:
> > On Fri, Sep 16, 2016 at 11:32:20AM +0200, Lucas Stach wrote:
> > > Hi Russell,
> > > 
> > > can you please take a look at this and tell if there is something wrong
> > > about it, or if you think it can go in?
> > > 
> > > Without this patch the kernel is unable to use the deeper CPU idle
> > > states on i.MX6S if it isn't built with SMP support enabled.
> > 
> > Without spending a lot of time digging into this code, working out what
> > these configuration symbols do, I frankly don't know if it's a sensible
> > thing to do or not.  I guess tglx knows this code like the back of his
> > hand, and would be in a better position to comment.
> > 
> > There's two factors there:
> > 1. the amount of undocumented code in the kernel needing the code to be
> >    read and understood to understand various aspects of it from the
> >    architecture point of view.
> 
> Yeah. I know that this stuff lacks documentation.
>  
> > 2. your architecture maintainer has done very little actual platform
> >    development (not through his own choice) for the last 10 or so years,
> >    which means he's not had to dig into these areas.
> > 
> > So, I'm afraid I feel less than qualified on this at the moment.  I'll
> > try to look into this and talk to tglx to work out whether it's a
> > sensible change. 
> 
> The broadcast feature has the following functionality:
> 
>     It lets you use fast accessible (cpu local) timers for normal operation
>     and in case of deep idle sleeps where the cpu local timer stops switch
>     to broadcast operation, which arms a slower to access (global) timer
>     device which does not stop in deep idle.
> 
>     On UP we usually just use the global device, but on SMP we need the cpu
>     local timers for normal (non idle) operation. Though there is no reason
>     to restrict this UP from a technical POV especially when the local
>     timer is way cheaper to access than the other device.
> 
> Hope that helps.

What I take from this is that the i.MX6 decision to use the local timer
for normal operation on UP is sane, as it's both cheaper to access, as
well as providing higher accuracy.

In fact this behavior has been explicitly enabled by a patch "ARM: imx:
always use TWD on IMX6Q" from Sebastian Andrzej Siewior to get better
responsiveness in cyclictest.

So the subject patch is actually a regression fix, as we now need the
timer broadcast infrastructure to be able to use the deeper idle states,
instead of having them enabled by always using the global timer on UP.

Russell, okay to put it in the patch system?

Regards,
Lucas

[PATCH] ARM: Kconfig: select GENERIC_CLOCKEVENTS_BROADCAST also on UP

[Lucas Stach]

Quite a few systems have deeper idle states where the local timer tick
stops to fire, that rely on the GENERIC_CLOCKEVENTS_BROADCAST
infrastructure to switch to a global (non-stopping) clockevent device.
This is also true on UP systems, so select GENERIC_CLOCKEVENTS_BROADCAST
unconditionally.

Instead only select ARCH_HAS_TICK_BROADCAST if SMP is present, as this
is only implemented for SMP systems.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
v2: Only select GENERIC_CLOCKEVENTS_BROADCAST if the architecture
    supports GENERIC_CLOCKEVENTS.
---
 arch/arm/Kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 5fab553fd03a..f4b7432554e2 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -4,7 +4,7 @@ config ARM
 	select ARCH_CLOCKSOURCE_DATA
 	select ARCH_HAS_DEVMEM_IS_ALLOWED
 	select ARCH_HAS_ELF_RANDOMIZE
-	select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
+	select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST && SMP
 	select ARCH_HAVE_CUSTOM_GPIO_H
 	select ARCH_HAS_GCOV_PROFILE_ALL
 	select ARCH_MIGHT_HAVE_PC_PARPORT
@@ -20,7 +20,7 @@ config ARM
 	select EDAC_ATOMIC_SCRUB
 	select GENERIC_ALLOCATOR
 	select GENERIC_ATOMIC64 if (CPU_V7M || CPU_V6 || !CPU_32v6K || !AEABI)
-	select GENERIC_CLOCKEVENTS_BROADCAST if SMP
+	select GENERIC_CLOCKEVENTS_BROADCAST if GENERIC_CLOCKEVENTS
 	select GENERIC_EARLY_IOREMAP
 	select GENERIC_IDLE_POLL_SETUP
 	select GENERIC_IRQ_PROBE
-- 
2.11.0

[PATCH] ARM: Kconfig: select GENERIC_CLOCKEVENTS_BROADCAST also on UP

[Russell King - ARM Linux]

On Fri, Jan 20, 2017 at 05:32:30PM +0100, Lucas Stach wrote:
> Quite a few systems have deeper idle states where the local timer tick
> stops to fire, that rely on the GENERIC_CLOCKEVENTS_BROADCAST
> infrastructure to switch to a global (non-stopping) clockevent device.
> This is also true on UP systems, so select GENERIC_CLOCKEVENTS_BROADCAST
> unconditionally.
> 
> Instead only select ARCH_HAS_TICK_BROADCAST if SMP is present, as this
> is only implemented for SMP systems.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
> v2: Only select GENERIC_CLOCKEVENTS_BROADCAST if the architecture
>     supports GENERIC_CLOCKEVENTS.

Provided this solves the problems identified by the 0-day builder with
your previous patch, please submit it to the patch system, thanks.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH] ARM: Kconfig: select GENERIC_CLOCKEVENTS_BROADCAST also on UP

[Russell King - ARM Linux]

On Mon, Jan 30, 2017 at 01:53:48PM +0000, Russell King - ARM Linux wrote:
> On Fri, Jan 20, 2017 at 05:32:30PM +0100, Lucas Stach wrote:
> > Quite a few systems have deeper idle states where the local timer tick
> > stops to fire, that rely on the GENERIC_CLOCKEVENTS_BROADCAST
> > infrastructure to switch to a global (non-stopping) clockevent device.
> > This is also true on UP systems, so select GENERIC_CLOCKEVENTS_BROADCAST
> > unconditionally.
> > 
> > Instead only select ARCH_HAS_TICK_BROADCAST if SMP is present, as this
> > is only implemented for SMP systems.
> > 
> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > ---
> > v2: Only select GENERIC_CLOCKEVENTS_BROADCAST if the architecture
> >     supports GENERIC_CLOCKEVENTS.
> 
> Provided this solves the problems identified by the 0-day builder with
> your previous patch, please submit it to the patch system, thanks.

I think this causes OMAP builds to fail:

arch/arm/mach-omap2/timer.c:325:8: error: expected identifier or '(' before 'void'
arch/arm/mach-omap2/timer.c:325:15: error: expected ')' before numeric constant

     #if !defined(CONFIG_SMP) && defined(CONFIG_GENERIC_CLOCKEVENTS_BROADCAST)
>>>> void tick_broadcast(const struct cpumask *mask)
     {
     }
     #endif

vs

     # ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
     #  ifdef CONFIG_ARCH_HAS_TICK_BROADCAST
     extern void tick_broadcast(const struct cpumask *mask);
     #  else
>>>> #   define tick_broadcast       NULL
     #  endif
     extern int tick_receive_broadcast(void);
     # endif

So, I'm dropping this change.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.