Dropping "depends on SMP" for HAVE_ARM_TWD -- take 2

Dropping "depends on SMP" for HAVE_ARM_TWD -- take 2

[Mason]

[ take 1 was sent on 2015-03-26 ]

Hello everyone,

In http://thread.gmane.org/gmane.linux.ports.arm.kernel/389931/focus=392348
Stephen Boyd wrote:

> I don't see any problem with the TWD dropping the dependency on SMP.
> The code should work the same on a UP configuration [...]

And Arnd recently said:

> I think this has come up before and should be fixed. Could you
> send a patch that allows using TWD in uniprocessor configurations?

Basically, this means reverting Shawn Guo's 904464b91eca patch.
and removing "depends on SMP" for HAVE_ARM_TWD.

However, Shawn's patch fixed an issue, therefore it seems likely
that simply reverting is not the proper solution?

What should I do?

Regards.

Dropping "depends on SMP" for HAVE_ARM_TWD -- take 2

[Mason]

[ Adding original reporter ]

On 02/10/2015 11:52, Mason wrote:

> [ take 1 was sent on 2015-03-26 ]
> 
> Hello everyone,
> 
> In http://thread.gmane.org/gmane.linux.ports.arm.kernel/389931/focus=392348
> Stephen Boyd wrote:
> 
>> I don't see any problem with the TWD dropping the dependency on SMP.
>> The code should work the same on a UP configuration [...]
> 
> And Arnd recently said:
> 
>> I think this has come up before and should be fixed. Could you
>> send a patch that allows using TWD in uniprocessor configurations?
> 
> Basically, this means reverting Shawn Guo's 904464b91eca patch.
> and removing "depends on SMP" for HAVE_ARM_TWD.
> 
> However, Shawn's patch fixed an issue, therefore it seems likely
> that simply reverting is not the proper solution?
> 
> What should I do?

For reference, the warning used to be:

    ------------[ cut here ]------------
    WARNING: at arch/arm/kernel/smp_twd.c:345
    twd_local_timer_of_register+0x7c/0x90()
    twd_local_timer_of_register failed (-6)
    Modules linked in:
    Backtrace:
    [<80011f14>] (dump_backtrace+0x0/0x10c) from [<8044dd30>]
    (dump_stack+0x18/0x1c)
     r7:805e9f58 r6:805ba84c r5:80539331 r4:00000159
    [<8044dd18>] (dump_stack+0x0/0x1c) from [<80020fbc>]
    (warn_slowpath_common+0x54/0x6c)
    [<80020f68>] (warn_slowpath_common+0x0/0x6c) from [<80021078>]
    (warn_slowpath_fmt+0x38/0x40)
     r9:412fc09a r8:8fffffff r7:ffffffff r6:00000001 r5:80633b8c
    r4:80b32da8
    [<80021040>] (warn_slowpath_fmt+0x0/0x40) from [<805ba84]
    (twd_local_timer_of_register+0x7c/0x90)
     r3:fffffffa r2:8053934b
    [<805ba7d0>] (twd_local_timer_of_register+0x0/0x90) from [<805c0bec>]
    (imx6q_timer_init+0x18/0x4c)
     r5:80633800 r4:8053b701
    [<805c0bd4>] (imx6q_timer_init+0x0/0x4c) from [<805ba4e8>]
    (time_init+0x28/0x38)
     r5:80633800 r4:805dc0f4
    [<805ba4c0>] (time_init+0x0/0x38) from [<805b6854>]
    (start_kernel+0x1a0/0x310)
    [<805b66b4>] (start_kernel+0x0/0x310) from [<10008044>] (0x10008044)
     r8:1000406a r7:805f3f8c r6:805dc0c4 r5:805f0518 r4:10c5387d
    ---[ end trace 1b75b31a2719ed1c ]---


I cannot reproduce on v4.2 + my platform...

Booting with the following command line:
ip=dhcp root=/dev/nfs rdinit=/none console=ttyS0,115200 mem=640M debug nosmp

produces the attached boot log (no run-time warning).

Perhaps CONFIG_SMP_ON_UP matters? (I have it enabled.)
Nope, I don't get any warning with CONFIG_SMP_ON_UP disabled.

Running out of ideas...

What does nosmp actually do?

static int __init nosmp(char *str)
{
	setup_max_cpus = 0;
	arch_disable_smp_support();

	return 0;
}

I don't think arch/arm overrides arch_disable_smp_support, so it's
just a NOP. So we're just setting setup_max_cpus to 0...

There have been several updates to arch/arm/kernel/smp_twd.c since
Shawn's patch, perhaps one of them unknowingly fixed the problem?

$ git ls arch/arm/kernel/smp_twd.c 
4ed89f222806 ARM: convert printk(KERN_* to pr_*
06b96c8beb94 arm: Replace __this_cpu_ptr with raw_cpu_ptr
0b443ead714f cpufreq: remove unused notifier: CPUFREQ_{SUSPENDCHANGE|RESUMECHANGE}
2e874ea34214 ARM: twd: data endian fix
47dcd3563e45 Merge tag 'remove-local-timers' of git://git.kernel.org/pub/scm/linux/kernel/git/davidb/linux-msm into next/cleanup
8bd26e3a7e49 arm: delete __cpuinit/__CPUINIT usage from all ARM users
cbbe6f82b489 ARM: 7778/1: smp_twd: twd_update_frequency need be run on all online CPUs
a894fcc2d01a ARM: smp_twd: Divorce smp_twd from local timer API
da4a686a2cfb ARM: smp_twd: convert to use CLKSRC_OF init
904464b91eca ARM: 7655/1: smp_twd: make twd_local_timer_of_register() no-op for nosmp

"smp_twd: Divorce smp_twd from local timer API" might be a likely candidate.
(What do you think, Stephen?)

Regards.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: boot.log
Type: text/x-log
Size: 7666 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151002/85eb1a49/attachment-0001.bin>

Dropping "depends on SMP" for HAVE_ARM_TWD -- take 2

[Stephen Boyd]

On 10/02, Mason wrote:
> [ Adding original reporter ]
> 
> On 02/10/2015 11:52, Mason wrote:
> 
> > [ take 1 was sent on 2015-03-26 ]
> > 
> > Hello everyone,
> > 
> > In http://thread.gmane.org/gmane.linux.ports.arm.kernel/389931/focus=392348
> > Stephen Boyd wrote:
> > 
> >> I don't see any problem with the TWD dropping the dependency on SMP.
> >> The code should work the same on a UP configuration [...]
> > 
> > And Arnd recently said:
> > 
> >> I think this has come up before and should be fixed. Could you
> >> send a patch that allows using TWD in uniprocessor configurations?
> > 
> > Basically, this means reverting Shawn Guo's 904464b91eca patch.
> > and removing "depends on SMP" for HAVE_ARM_TWD.
> > 
> > However, Shawn's patch fixed an issue, therefore it seems likely
> > that simply reverting is not the proper solution?
> > 
> > What should I do?
> 
> For reference, the warning used to be:
> 
>     ------------[ cut here ]------------
>     WARNING: at arch/arm/kernel/smp_twd.c:345
>     twd_local_timer_of_register+0x7c/0x90()
>     twd_local_timer_of_register failed (-6)
>     Modules linked in:
>     Backtrace:
>     [<80011f14>] (dump_backtrace+0x0/0x10c) from [<8044dd30>]
>     (dump_stack+0x18/0x1c)
>      r7:805e9f58 r6:805ba84c r5:80539331 r4:00000159
>     [<8044dd18>] (dump_stack+0x0/0x1c) from [<80020fbc>]
>     (warn_slowpath_common+0x54/0x6c)
>     [<80020f68>] (warn_slowpath_common+0x0/0x6c) from [<80021078>]
>     (warn_slowpath_fmt+0x38/0x40)
>      r9:412fc09a r8:8fffffff r7:ffffffff r6:00000001 r5:80633b8c
>     r4:80b32da8
>     [<80021040>] (warn_slowpath_fmt+0x0/0x40) from [<805ba84]
>     (twd_local_timer_of_register+0x7c/0x90)
>      r3:fffffffa r2:8053934b
>     [<805ba7d0>] (twd_local_timer_of_register+0x0/0x90) from [<805c0bec>]
>     (imx6q_timer_init+0x18/0x4c)
>      r5:80633800 r4:8053b701
>     [<805c0bd4>] (imx6q_timer_init+0x0/0x4c) from [<805ba4e8>]
>     (time_init+0x28/0x38)
>      r5:80633800 r4:805dc0f4
>     [<805ba4c0>] (time_init+0x0/0x38) from [<805b6854>]
>     (start_kernel+0x1a0/0x310)
>     [<805b66b4>] (start_kernel+0x0/0x310) from [<10008044>] (0x10008044)
>      r8:1000406a r7:805f3f8c r6:805dc0c4 r5:805f0518 r4:10c5387d
>     ---[ end trace 1b75b31a2719ed1c ]---
> 
> 
> I cannot reproduce on v4.2 + my platform...

The warning has been removed in commit 5028090d1da1 (ARM: 8434/1:
Revert "7655/1: smp_twd: make twd_local_timer_of_register() no-op
for nosmp", 2015-09-14) sitting in linux-next. Oddly, that commit
doesn't remove the depends on SMP for the Kconfig. So it seems
that we can apply this patch and everyone is happy? We could also
drop the "if SMP" part of most of the platform selects if anyone
actually cares.

---8<----
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 369791fb619c..be64d9d604c3 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1389,7 +1389,6 @@ config HAVE_ARM_ARCH_TIMER
 
 config HAVE_ARM_TWD
 	bool
-	depends on SMP
 	select CLKSRC_OF if OF
 	help
 	  This options enables support for the ARM timer and watchdog unit

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Dropping "depends on SMP" for HAVE_ARM_TWD -- take 2

[Felipe Balbi]

Hi,

On Fri, Oct 02, 2015 at 11:02:55AM -0700, Stephen Boyd wrote:
> On 10/02, Mason wrote:
> > [ Adding original reporter ]
> > 
> > On 02/10/2015 11:52, Mason wrote:
> > 
> > > [ take 1 was sent on 2015-03-26 ]
> > > 
> > > Hello everyone,
> > > 
> > > In http://thread.gmane.org/gmane.linux.ports.arm.kernel/389931/focus=392348
> > > Stephen Boyd wrote:
> > > 
> > >> I don't see any problem with the TWD dropping the dependency on SMP.
> > >> The code should work the same on a UP configuration [...]
> > > 
> > > And Arnd recently said:
> > > 
> > >> I think this has come up before and should be fixed. Could you
> > >> send a patch that allows using TWD in uniprocessor configurations?
> > > 
> > > Basically, this means reverting Shawn Guo's 904464b91eca patch.
> > > and removing "depends on SMP" for HAVE_ARM_TWD.
> > > 
> > > However, Shawn's patch fixed an issue, therefore it seems likely
> > > that simply reverting is not the proper solution?
> > > 
> > > What should I do?
> > 
> > For reference, the warning used to be:
> > 
> >     ------------[ cut here ]------------
> >     WARNING: at arch/arm/kernel/smp_twd.c:345
> >     twd_local_timer_of_register+0x7c/0x90()
> >     twd_local_timer_of_register failed (-6)
> >     Modules linked in:
> >     Backtrace:
> >     [<80011f14>] (dump_backtrace+0x0/0x10c) from [<8044dd30>]
> >     (dump_stack+0x18/0x1c)
> >      r7:805e9f58 r6:805ba84c r5:80539331 r4:00000159
> >     [<8044dd18>] (dump_stack+0x0/0x1c) from [<80020fbc>]
> >     (warn_slowpath_common+0x54/0x6c)
> >     [<80020f68>] (warn_slowpath_common+0x0/0x6c) from [<80021078>]
> >     (warn_slowpath_fmt+0x38/0x40)
> >      r9:412fc09a r8:8fffffff r7:ffffffff r6:00000001 r5:80633b8c
> >     r4:80b32da8
> >     [<80021040>] (warn_slowpath_fmt+0x0/0x40) from [<805ba84]
> >     (twd_local_timer_of_register+0x7c/0x90)
> >      r3:fffffffa r2:8053934b
> >     [<805ba7d0>] (twd_local_timer_of_register+0x0/0x90) from [<805c0bec>]
> >     (imx6q_timer_init+0x18/0x4c)
> >      r5:80633800 r4:8053b701
> >     [<805c0bd4>] (imx6q_timer_init+0x0/0x4c) from [<805ba4e8>]
> >     (time_init+0x28/0x38)
> >      r5:80633800 r4:805dc0f4
> >     [<805ba4c0>] (time_init+0x0/0x38) from [<805b6854>]
> >     (start_kernel+0x1a0/0x310)
> >     [<805b66b4>] (start_kernel+0x0/0x310) from [<10008044>] (0x10008044)
> >      r8:1000406a r7:805f3f8c r6:805dc0c4 r5:805f0518 r4:10c5387d
> >     ---[ end trace 1b75b31a2719ed1c ]---
> > 
> > 
> > I cannot reproduce on v4.2 + my platform...
> 
> The warning has been removed in commit 5028090d1da1 (ARM: 8434/1:
> Revert "7655/1: smp_twd: make twd_local_timer_of_register() no-op
> for nosmp", 2015-09-14) sitting in linux-next. Oddly, that commit
> doesn't remove the depends on SMP for the Kconfig. So it seems
> that we can apply this patch and everyone is happy? We could also
> drop the "if SMP" part of most of the platform selects if anyone
> actually cares.

heh, sorry about that. I forgot omap2plus_defconfig enables everything. I could
refresh that patch if Russell is still okay taking a newer version of
it. Russell ?

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151002/5962cbea/attachment.sig>

Dropping "depends on SMP" for HAVE_ARM_TWD -- take 2

[Russell King - ARM Linux]

On Fri, Oct 02, 2015 at 01:48:27PM -0500, Felipe Balbi wrote:
> heh, sorry about that. I forgot omap2plus_defconfig enables everything.
> I could refresh that patch if Russell is still okay taking a newer
> version of it. Russell ?

Ok.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Dropping "depends on SMP" for HAVE_ARM_TWD -- take 2

[Felipe Balbi]

On Fri, Oct 02, 2015 at 07:55:33PM +0100, Russell King - ARM Linux wrote:
> On Fri, Oct 02, 2015 at 01:48:27PM -0500, Felipe Balbi wrote:
> > heh, sorry about that. I forgot omap2plus_defconfig enables everything.
> > I could refresh that patch if Russell is still okay taking a newer
> > version of it. Russell ?
> 
> Ok.

thanks, patch coming shortly. If anybody is against refreshing original patch,
let me know now :-)

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151002/e70ea9f0/attachment.sig>

Dropping "depends on SMP" for HAVE_ARM_TWD -- take 2

[Mason]

On 02/10/2015 20:02, Stephen Boyd wrote:
> On 10/02, Mason wrote:
>> [ Adding original reporter ]
>>
>> On 02/10/2015 11:52, Mason wrote:
>>
>>> [ take 1 was sent on 2015-03-26 ]
>>>
>>> Hello everyone,
>>>
>>> In http://thread.gmane.org/gmane.linux.ports.arm.kernel/389931/focus=392348
>>> Stephen Boyd wrote:
>>>
>>>> I don't see any problem with the TWD dropping the dependency on SMP.
>>>> The code should work the same on a UP configuration [...]
>>>
>>> And Arnd recently said:
>>>
>>>> I think this has come up before and should be fixed. Could you
>>>> send a patch that allows using TWD in uniprocessor configurations?
>>>
>>> Basically, this means reverting Shawn Guo's 904464b91eca patch.
>>> and removing "depends on SMP" for HAVE_ARM_TWD.
>>>
>>> However, Shawn's patch fixed an issue, therefore it seems likely
>>> that simply reverting is not the proper solution?
>>>
>>> What should I do?
>>
>> For reference, the warning used to be:
>>
>>     ------------[ cut here ]------------
>>     WARNING: at arch/arm/kernel/smp_twd.c:345
>>     twd_local_timer_of_register+0x7c/0x90()
>>     twd_local_timer_of_register failed (-6)
>>     Modules linked in:
>>     Backtrace:
>>     [<80011f14>] (dump_backtrace+0x0/0x10c) from [<8044dd30>]
>>     (dump_stack+0x18/0x1c)
>>      r7:805e9f58 r6:805ba84c r5:80539331 r4:00000159
>>     [<8044dd18>] (dump_stack+0x0/0x1c) from [<80020fbc>]
>>     (warn_slowpath_common+0x54/0x6c)
>>     [<80020f68>] (warn_slowpath_common+0x0/0x6c) from [<80021078>]
>>     (warn_slowpath_fmt+0x38/0x40)
>>      r9:412fc09a r8:8fffffff r7:ffffffff r6:00000001 r5:80633b8c
>>     r4:80b32da8
>>     [<80021040>] (warn_slowpath_fmt+0x0/0x40) from [<805ba84]
>>     (twd_local_timer_of_register+0x7c/0x90)
>>      r3:fffffffa r2:8053934b
>>     [<805ba7d0>] (twd_local_timer_of_register+0x0/0x90) from [<805c0bec>]
>>     (imx6q_timer_init+0x18/0x4c)
>>      r5:80633800 r4:8053b701
>>     [<805c0bd4>] (imx6q_timer_init+0x0/0x4c) from [<805ba4e8>]
>>     (time_init+0x28/0x38)
>>      r5:80633800 r4:805dc0f4
>>     [<805ba4c0>] (time_init+0x0/0x38) from [<805b6854>]
>>     (start_kernel+0x1a0/0x310)
>>     [<805b66b4>] (start_kernel+0x0/0x310) from [<10008044>] (0x10008044)
>>      r8:1000406a r7:805f3f8c r6:805dc0c4 r5:805f0518 r4:10c5387d
>>     ---[ end trace 1b75b31a2719ed1c ]---
>>
>>
>> I cannot reproduce on v4.2 + my platform...
> 
> The warning has been removed in commit 5028090d1da1 (ARM: 8434/1:
> Revert "7655/1: smp_twd: make twd_local_timer_of_register() no-op
> for nosmp", 2015-09-14) sitting in linux-next.

Does that mean it will automatically make it into v4.3?

> Oddly, that commit doesn't remove the depends on SMP for the Kconfig.

Perhaps because the commit is just a revert, and the depends
statement has been here since the beginning (f32f4ce257452)

> So it seems that we can apply this patch and everyone is happy?
> We could also drop the "if SMP" part of most of the platform
> selects if anyone actually cares.

My port requires the TWD to function. So I'm using
select HAVE_ARM_TWD (no "if SMP") to have it work
even on MULTIPLATFORM UP configs.

> ---8<----
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 369791fb619c..be64d9d604c3 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -1389,7 +1389,6 @@ config HAVE_ARM_ARCH_TIMER
>  
>  config HAVE_ARM_TWD
>  	bool
> -	depends on SMP
>  	select CLKSRC_OF if OF
>  	help
>  	  This options enables support for the ARM timer and watchdog unit


Regards.

Dropping "depends on SMP" for HAVE_ARM_TWD -- take 2

[Stephen Boyd]

On 10/02, Mason wrote:
> On 02/10/2015 20:02, Stephen Boyd wrote:
> > On 10/02, Mason wrote:
> >>
> >>
> >> I cannot reproduce on v4.2 + my platform...
> > 
> > The warning has been removed in commit 5028090d1da1 (ARM: 8434/1:
> > Revert "7655/1: smp_twd: make twd_local_timer_of_register() no-op
> > for nosmp", 2015-09-14) sitting in linux-next.
> 
> Does that mean it will automatically make it into v4.3?

No. It will most likely be in v4.4

> > So it seems that we can apply this patch and everyone is happy?
> > We could also drop the "if SMP" part of most of the platform
> > selects if anyone actually cares.
> 
> My port requires the TWD to function. So I'm using
> select HAVE_ARM_TWD (no "if SMP") to have it work
> even on MULTIPLATFORM UP configs.

So you probably get some Kconfig warning about unmet
dependencies.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Dropping "depends on SMP" for HAVE_ARM_TWD -- take 2

[Måns Rullgård]

Mason <slash.tmp@free.fr> writes:

> My port requires the TWD to function. So I'm using
> select HAVE_ARM_TWD (no "if SMP") to have it work
> even on MULTIPLATFORM UP configs.

You could just let it use another timer in the non-smp case if you
didn't have that weird aversion towards using my code.

-- 
M?ns Rullg?rd
mans at mansr.com

Dropping "depends on SMP" for HAVE_ARM_TWD -- take 2

[Marc Zyngier]

On Sat, 3 Oct 2015 00:34:02 +0100
M?ns Rullg?rd <mans@mansr.com> wrote:

> Mason <slash.tmp@free.fr> writes:
> 
> > My port requires the TWD to function. So I'm using
> > select HAVE_ARM_TWD (no "if SMP") to have it work
> > even on MULTIPLATFORM UP configs.
> 
> You could just let it use another timer in the non-smp case if you
> didn't have that weird aversion towards using my code.

I have no idea what your code does, but using the timer that is
integrated into the core (just like on any other A9 implementation)
feels like the right thing to do, unless the HW is too broken to use
TWD.

Another timer is a nice thing to have (it probably comes in handy for
suspend/resume, and it may have a much better resolution), but this
seems like icing on the cake at this point. Let's see the cake first.

Thanks,

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

Dropping "depends on SMP" for HAVE_ARM_TWD -- take 2

[Mason]

On 03/10/2015 11:32, Marc Zyngier wrote:
> On Sat, 3 Oct 2015 00:34:02 +0100
> M?ns Rullg?rd wrote:
>> Mason writes:
>>
>>> My port requires the TWD to function. So I'm using
>>> select HAVE_ARM_TWD (no "if SMP") to have it work
>>> even on MULTIPLATFORM UP configs.
>>
>> You could just let it use another timer in the non-smp case if you
>> didn't have that weird aversion towards using my code.
> 
> I have no idea what your code does, but using the timer that is
> integrated into the core (just like on any other A9 implementation)
> feels like the right thing to do, unless the HW is too broken to use
> TWD.
> 
> Another timer is a nice thing to have (it probably comes in handy for
> suspend/resume, and it may have a much better resolution), but this
> seems like icing on the cake at this point. Let's see the cake first.

Marc,

I have a quick question for you :-)

As I stated several months ago, my goal is to produce the simplest
port possible, which is why I chose the TWD instead of platform
timers. At one point, I was also considering the global timer.
(I have a Cortex A9 MPCore.) But as far as I could tell, the
code for the global timer does not handle CPU frequency changes
(while smp_twd.c does), is that correct?

AFAIU, TWD timers and global timer "tick" at the frequency of
PERIPHCLK (which is CPUCLK/2 in my implementation). Therefore,
code needs to be specifically written to handle cpufreq events.
Is that a correct understanding?

As for you comment about resolution, nominal PERIPHCLK is 600 MHz.
Hard to beat that kind of resolution, I think.

Have a nice week end.

Regards.

Dropping "depends on SMP" for HAVE_ARM_TWD -- take 2

[Marc Zyngier]

On Sat, 3 Oct 2015 11:49:41 +0200
Mason <slash.tmp@free.fr> wrote:

> On 03/10/2015 11:32, Marc Zyngier wrote:
> > On Sat, 3 Oct 2015 00:34:02 +0100
> > M?ns Rullg?rd wrote:
> >> Mason writes:
> >>
> >>> My port requires the TWD to function. So I'm using
> >>> select HAVE_ARM_TWD (no "if SMP") to have it work
> >>> even on MULTIPLATFORM UP configs.
> >>
> >> You could just let it use another timer in the non-smp case if you
> >> didn't have that weird aversion towards using my code.
> > 
> > I have no idea what your code does, but using the timer that is
> > integrated into the core (just like on any other A9 implementation)
> > feels like the right thing to do, unless the HW is too broken to use
> > TWD.
> > 
> > Another timer is a nice thing to have (it probably comes in handy for
> > suspend/resume, and it may have a much better resolution), but this
> > seems like icing on the cake at this point. Let's see the cake first.
> 
> Marc,
> 
> I have a quick question for you :-)
> 
> As I stated several months ago, my goal is to produce the simplest
> port possible, which is why I chose the TWD instead of platform
> timers. At one point, I was also considering the global timer.
> (I have a Cortex A9 MPCore.) But as far as I could tell, the
> code for the global timer does not handle CPU frequency changes
> (while smp_twd.c does), is that correct?

Indeed, I cannot see any code that does that in the GT driver. But if
you have an A9 MP, you probably want to stick to TWD, which gives you a
per-cpu timer instead of a global timer that will require IPIs to other
CPUs.

> AFAIU, TWD timers and global timer "tick" at the frequency of
> PERIPHCLK (which is CPUCLK/2 in my implementation). Therefore,
> code needs to be specifically written to handle cpufreq events.
> Is that a correct understanding?

I don't have the A9 TRM handy (nor the desire to read it on a
sunny Saturday morning), so I can't really tell. But yes, this is
likely to require some clock change handling.

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

Dropping "depends on SMP" for HAVE_ARM_TWD -- take 2

[Sören Brinkmann]

On Sat, 2015-10-03 at 11:12AM +0100, Marc Zyngier wrote:
> On Sat, 3 Oct 2015 11:49:41 +0200
> Mason <slash.tmp@free.fr> wrote:
> 
> > On 03/10/2015 11:32, Marc Zyngier wrote:
> > > On Sat, 3 Oct 2015 00:34:02 +0100
> > > M?ns Rullg?rd wrote:
> > >> Mason writes:
> > >>
> > >>> My port requires the TWD to function. So I'm using
> > >>> select HAVE_ARM_TWD (no "if SMP") to have it work
> > >>> even on MULTIPLATFORM UP configs.
> > >>
> > >> You could just let it use another timer in the non-smp case if you
> > >> didn't have that weird aversion towards using my code.
> > > 
> > > I have no idea what your code does, but using the timer that is
> > > integrated into the core (just like on any other A9 implementation)
> > > feels like the right thing to do, unless the HW is too broken to use
> > > TWD.
> > > 
> > > Another timer is a nice thing to have (it probably comes in handy for
> > > suspend/resume, and it may have a much better resolution), but this
> > > seems like icing on the cake at this point. Let's see the cake first.
> > 
> > Marc,
> > 
> > I have a quick question for you :-)
> > 
> > As I stated several months ago, my goal is to produce the simplest
> > port possible, which is why I chose the TWD instead of platform
> > timers. At one point, I was also considering the global timer.
> > (I have a Cortex A9 MPCore.) But as far as I could tell, the
> > code for the global timer does not handle CPU frequency changes
> > (while smp_twd.c does), is that correct?
> 
> Indeed, I cannot see any code that does that in the GT driver. But if
> you have an A9 MP, you probably want to stick to TWD, which gives you a
> per-cpu timer instead of a global timer that will require IPIs to other
> CPUs.

I think the TWD only provides a clock_event device. Clocksource and
schedclock would have to be provided by something else.

> 
> > AFAIU, TWD timers and global timer "tick" at the frequency of
> > PERIPHCLK (which is CPUCLK/2 in my implementation). Therefore,
> > code needs to be specifically written to handle cpufreq events.
> > Is that a correct understanding?
> 
> I don't have the A9 TRM handy (nor the desire to read it on a
> sunny Saturday morning), so I can't really tell. But yes, this is
> likely to require some clock change handling.

I looked at that once. IIRC, the problems are schedclock and clocksource.
Other than a clockevent device which can be adjusted for frequency
changes, there is (at least was) no such mechanism for clocksources and
schedclock. Those are required to run at a stable frequency at all times
(https://www.kernel.org/doc/Documentation/timers/timekeeping.txt).

On Zynq I had the same problem and created a construct of notifiers for
our clocksource to deal with this problem
(https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/clocksource/cadence_ttc_timer.c)
I never really liked that "solution" much, though.

That approach would probably not work with the GT because, last time I
checked, it runs at the maximum frequency possible, i.e. dividers at the
minimum (as a timer should), which means, if cpufreq throttles your CPU
speed down, you'd have to decrease the clock dividers which isn't
possible anymore (I could do that on the TTC because we have to run it
a at very low speeds with high dividers to avoid constantly overflowing
the 16-bit counter).

	S?ren

Dropping "depends on SMP" for HAVE_ARM_TWD -- take 2

[Mason]

On 05/10/2015 07:46, S?ren Brinkmann wrote:
> On Sat, 2015-10-03 at 11:12AM +0100, Marc Zyngier wrote:
>> On Sat, 3 Oct 2015 11:49:41 +0200
>> Mason <slash.tmp@free.fr> wrote:
>>
>>> On 03/10/2015 11:32, Marc Zyngier wrote:
>>>> On Sat, 3 Oct 2015 00:34:02 +0100
>>>> M?ns Rullg?rd wrote:
>>>>> Mason writes:
>>>>>
>>>>>> My port requires the TWD to function. So I'm using
>>>>>> select HAVE_ARM_TWD (no "if SMP") to have it work
>>>>>> even on MULTIPLATFORM UP configs.
>>>>>
>>>>> You could just let it use another timer in the non-smp case if you
>>>>> didn't have that weird aversion towards using my code.
>>>>
>>>> I have no idea what your code does, but using the timer that is
>>>> integrated into the core (just like on any other A9 implementation)
>>>> feels like the right thing to do, unless the HW is too broken to use
>>>> TWD.
>>>>
>>>> Another timer is a nice thing to have (it probably comes in handy for
>>>> suspend/resume, and it may have a much better resolution), but this
>>>> seems like icing on the cake at this point. Let's see the cake first.
>>>
>>> I have a quick question for you :-)
>>>
>>> As I stated several months ago, my goal is to produce the simplest
>>> port possible, which is why I chose the TWD instead of platform
>>> timers. At one point, I was also considering the global timer.
>>> (I have a Cortex A9 MPCore.) But as far as I could tell, the
>>> code for the global timer does not handle CPU frequency changes
>>> (while smp_twd.c does), is that correct?
>>
>> Indeed, I cannot see any code that does that in the GT driver. But if
>> you have an A9 MP, you probably want to stick to TWD, which gives you a
>> per-cpu timer instead of a global timer that will require IPIs to other
>> CPUs.
> 
> I think the TWD only provides a clock_event device. Clocksource and
> schedclock would have to be provided by something else.

That is correct AFAIU. But my platform provides a simple 32-bit 27 MHz
tick counter, which makes it trivial to implement clock source, sched
clock and delay timer.

http://article.gmane.org/gmane.linux.kernel/2049558

I have identified two problems with using the TWD in Linux:

1) the TWD is only available on SMP kernels
=> that issue seems close to being resolved

2) I was told that some platforms "turn off" the TWD block in
low-power modes (like during WFI), and therefore must be marked
as unreliable in "C3" (FEAT_C3STOP).

I have a patch to selectively not mark the TWD as unreliable.
(I will make a formal submission today.)

>>> AFAIU, TWD timers and global timer "tick" at the frequency of
>>> PERIPHCLK (which is CPUCLK/2 in my implementation). Therefore,
>>> code needs to be specifically written to handle cpufreq events.
>>> Is that a correct understanding?
>>
>> I don't have the A9 TRM handy (nor the desire to read it on a
>> sunny Saturday morning), so I can't really tell. But yes, this is
>> likely to require some clock change handling.
> 
> I looked at that once. IIRC, the problems are schedclock and clocksource.
> Other than a clockevent device which can be adjusted for frequency
> changes, there is (at least was) no such mechanism for clocksources and
> schedclock. Those are required to run at a stable frequency at all times
> (https://www.kernel.org/doc/Documentation/timers/timekeeping.txt).
> 
> On Zynq I had the same problem and created a construct of notifiers for
> our clocksource to deal with this problem
> (https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/clocksource/cadence_ttc_timer.c)
> I never really liked that "solution" much, though.
> 
> That approach would probably not work with the GT because, last time I
> checked, it runs at the maximum frequency possible, i.e. dividers at the
> minimum (as a timer should), which means, if cpufreq throttles your CPU
> speed down, you'd have to decrease the clock dividers which isn't
> possible anymore (I could do that on the TTC because we have to run it
> a at very low speeds with high dividers to avoid constantly overflowing
> the 16-bit counter).

Thanks for sharing your analysis.

Regards.

Dropping "depends on SMP" for HAVE_ARM_TWD -- take 2

[Linus Walleij]

On Mon, Oct 5, 2015 at 9:38 AM, Mason <slash.tmp@free.fr> wrote:

> 2) I was told that some platforms "turn off" the TWD block in
> low-power modes (like during WFI), and therefore must be marked
> as unreliable in "C3" (FEAT_C3STOP).
>
> I have a patch to selectively not mark the TWD as unreliable.
> (I will make a formal submission today.)

This sounds like something that should be done with a device
tree bool. Don't forget to patch
Documentation/devicetree/bindings/arm/twd.txt

Yours,
Linus Walleij

Dropping "depends on SMP" for HAVE_ARM_TWD -- take 2

[Afzal Mohammed]

Hi,

On Sun, Oct 04, 2015 at 10:46:52PM -0700, S?ren Brinkmann wrote:
> On Sat, 2015-10-03 at 11:12AM +0100, Marc Zyngier wrote:

> > Indeed, I cannot see any code that does that in the GT driver. But if
> > you have an A9 MP, you probably want to stick to TWD, which gives you a
> > per-cpu timer instead of a global timer that will require IPIs to other
> > CPUs.
> 
> I think the TWD only provides a clock_event device. Clocksource and
> schedclock would have to be provided by something else.

If no clocksource, sched clock is provided, default jiffies based ones
would be sufficient for single core, no ?, though not a preferred one.

Regards
afzal

> I looked at that once. IIRC, the problems are schedclock and clocksource.
> Other than a clockevent device which can be adjusted for frequency
> changes, there is (at least was) no such mechanism for clocksources and
> schedclock. Those are required to run at a stable frequency at all times

Dropping "depends on SMP" for HAVE_ARM_TWD -- take 2

[Sören Brinkmann]

On Mon, 2015-10-05 at 05:19PM +0530, Afzal Mohammed wrote:
> Hi,
> 
> On Sun, Oct 04, 2015 at 10:46:52PM -0700, S?ren Brinkmann wrote:
> > On Sat, 2015-10-03 at 11:12AM +0100, Marc Zyngier wrote:
> 
> > > Indeed, I cannot see any code that does that in the GT driver. But if
> > > you have an A9 MP, you probably want to stick to TWD, which gives you a
> > > per-cpu timer instead of a global timer that will require IPIs to other
> > > CPUs.
> > 
> > I think the TWD only provides a clock_event device. Clocksource and
> > schedclock would have to be provided by something else.
> 
> If no clocksource, sched clock is provided, default jiffies based ones
> would be sufficient for single core, no ?, though not a preferred one.

Yes that is correct. My point was more, that twd+cpufreq is a rather
easy case since it doesn't provide a clocksource/schedclock. A driver
that provides those needs quite some more work if it depends on the CPU
frequency and needs to be usable with cpufreq.

	S?ren

Dropping "depends on SMP" for HAVE_ARM_TWD -- take 2

[Russell King - ARM Linux]

On Mon, Oct 05, 2015 at 08:13:03AM -0700, S?ren Brinkmann wrote:
> On Mon, 2015-10-05 at 05:19PM +0530, Afzal Mohammed wrote:
> > Hi,
> > 
> > On Sun, Oct 04, 2015 at 10:46:52PM -0700, S?ren Brinkmann wrote:
> > > On Sat, 2015-10-03 at 11:12AM +0100, Marc Zyngier wrote:
> > 
> > > > Indeed, I cannot see any code that does that in the GT driver. But if
> > > > you have an A9 MP, you probably want to stick to TWD, which gives you a
> > > > per-cpu timer instead of a global timer that will require IPIs to other
> > > > CPUs.
> > > 
> > > I think the TWD only provides a clock_event device. Clocksource and
> > > schedclock would have to be provided by something else.
> > 
> > If no clocksource, sched clock is provided, default jiffies based ones
> > would be sufficient for single core, no ?, though not a preferred one.
> 
> Yes that is correct. My point was more, that twd+cpufreq is a rather
> easy case since it doesn't provide a clocksource/schedclock. A driver
> that provides those needs quite some more work if it depends on the CPU
> frequency and needs to be usable with cpufreq.

_And_ you need to take the decision that you don't care about time
keeping accuracy - because each and every time you change the
clocksource's frequency, you will never be able to know exactly when
the frequency change took effect in comparison with the counter value.

You could do a very tight "read counter before, write clock rate, read
counter after" and use that to calculate the number of timer ticks at
the old rate, and re-set the timing parameters with the new initial
counter value after the change, but even that's not perfect as you don't
actually know when the write hits the hardware compared to the reads.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Dropping "depends on SMP" for HAVE_ARM_TWD -- take 2

[Mason]

On 02/10/2015 20:02, Stephen Boyd wrote:

> The warning has been removed in commit 5028090d1da1 (ARM: 8434/1:
> Revert "7655/1: smp_twd: make twd_local_timer_of_register() no-op
> for nosmp", 2015-09-14) sitting in linux-next. Oddly, that commit
> doesn't remove the depends on SMP for the Kconfig. So it seems
> that we can apply this patch and everyone is happy? We could also
> drop the "if SMP" part of most of the platform selects if anyone
> actually cares.
> 
> ---8<----
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 369791fb619c..be64d9d604c3 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -1389,7 +1389,6 @@ config HAVE_ARM_ARCH_TIMER
>  
>  config HAVE_ARM_TWD
>  	bool
> -	depends on SMP
>  	select CLKSRC_OF if OF
>  	help
>  	  This options enables support for the ARM timer and watchdog unit

Hello Stephen,

Should I make a formal submission for the above patch to be accepted?

(Did you see my "twd: Don't set CLOCK_EVT_FEAT_C3STOP unconditionally"
patch? What should I do now for it to be accepted?)

Regards.

Dropping "depends on SMP" for HAVE_ARM_TWD -- take 2

[Stephen Boyd]

On 10/06, Mason wrote:
> On 02/10/2015 20:02, Stephen Boyd wrote:
> 
> > The warning has been removed in commit 5028090d1da1 (ARM: 8434/1:
> > Revert "7655/1: smp_twd: make twd_local_timer_of_register() no-op
> > for nosmp", 2015-09-14) sitting in linux-next. Oddly, that commit
> > doesn't remove the depends on SMP for the Kconfig. So it seems
> > that we can apply this patch and everyone is happy? We could also
> > drop the "if SMP" part of most of the platform selects if anyone
> > actually cares.
> > 
> > ---8<----
> > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > index 369791fb619c..be64d9d604c3 100644
> > --- a/arch/arm/Kconfig
> > +++ b/arch/arm/Kconfig
> > @@ -1389,7 +1389,6 @@ config HAVE_ARM_ARCH_TIMER
> >  
> >  config HAVE_ARM_TWD
> >  	bool
> > -	depends on SMP
> >  	select CLKSRC_OF if OF
> >  	help
> >  	  This options enables support for the ARM timer and watchdog unit
> 
> Hello Stephen,
> 
> Should I make a formal submission for the above patch to be accepted?

No. I believe Felipe is going to update commit 5028090d1da1 to
remove the depends on.

> 
> (Did you see my "twd: Don't set CLOCK_EVT_FEAT_C3STOP unconditionally"
> patch? What should I do now for it to be accepted?)
> 

Assuming you sent it to the correct maintainer from the
MAINTAINERS file, I would expect them to apply it directly.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Dropping "depends on SMP" for HAVE_ARM_TWD -- take 2

[Felipe Balbi]

Stephen Boyd <sboyd@codeaurora.org> writes:

> On 10/06, Mason wrote:
>> On 02/10/2015 20:02, Stephen Boyd wrote:
>> 
>> > The warning has been removed in commit 5028090d1da1 (ARM: 8434/1:
>> > Revert "7655/1: smp_twd: make twd_local_timer_of_register() no-op
>> > for nosmp", 2015-09-14) sitting in linux-next. Oddly, that commit
>> > doesn't remove the depends on SMP for the Kconfig. So it seems
>> > that we can apply this patch and everyone is happy? We could also
>> > drop the "if SMP" part of most of the platform selects if anyone
>> > actually cares.
>> > 
>> > ---8<----
>> > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
>> > index 369791fb619c..be64d9d604c3 100644
>> > --- a/arch/arm/Kconfig
>> > +++ b/arch/arm/Kconfig
>> > @@ -1389,7 +1389,6 @@ config HAVE_ARM_ARCH_TIMER
>> >  
>> >  config HAVE_ARM_TWD
>> >  	bool
>> > -	depends on SMP
>> >  	select CLKSRC_OF if OF
>> >  	help
>> >  	  This options enables support for the ARM timer and watchdog unit
>> 
>> Hello Stephen,
>> 
>> Should I make a formal submission for the above patch to be accepted?
>
> No. I believe Felipe is going to update commit 5028090d1da1 to
> remove the depends on.

Already did it. Should be in next shortly.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151006/5823d60b/attachment.sig>

Dropping "depends on SMP" for HAVE_ARM_TWD -- take 2

[Mason]

Stephen Boyd wrote:

> Mason wrote:
>
>> Did you see my "twd: Don't set CLOCK_EVT_FEAT_C3STOP unconditionally"
>> patch? What should I do now for it to be accepted?
> 
> Assuming you sent it to the correct maintainer from the
> MAINTAINERS file, I would expect them to apply it directly.

<confused>

The patch touches arch/arm/kernel/smp_twd.c
I do not see any entry for arch/arm/kernel in MAINTAINERS.
This means I should fall back to the generic arch/arm entry, right?

ARM PORT
M:	Russell King <linux@arm.linux.org.uk>
L:	linux-arm-kernel at lists.infradead.org (moderated for non-subscribers)
W:	http://www.arm.linux.org.uk/
S:	Maintained
F:	arch/arm/

AFAIU, there is a separate process for submitting patches for arch/arm:
http://www.arm.linux.org.uk/developer/patches/

Russell, Mark Rutland wrote: "Otherwise this looks ok."
Does that mean I should now submit via the web form?

Regards.

Dropping "depends on SMP" for HAVE_ARM_TWD -- take 2

[Felipe Balbi]

Hi,

Mason <slash.tmp@free.fr> writes:
> Stephen Boyd wrote:
>
>> Mason wrote:
>>
>>> Did you see my "twd: Don't set CLOCK_EVT_FEAT_C3STOP unconditionally"
>>> patch? What should I do now for it to be accepted?
>> 
>> Assuming you sent it to the correct maintainer from the
>> MAINTAINERS file, I would expect them to apply it directly.
>
> <confused>
>
> The patch touches arch/arm/kernel/smp_twd.c
> I do not see any entry for arch/arm/kernel in MAINTAINERS.
> This means I should fall back to the generic arch/arm entry, right?
>
> ARM PORT
> M:	Russell King <linux@arm.linux.org.uk>
> L:	linux-arm-kernel at lists.infradead.org (moderated for non-subscribers)
> W:	http://www.arm.linux.org.uk/
> S:	Maintained
> F:	arch/arm/
>
> AFAIU, there is a separate process for submitting patches for arch/arm:
> http://www.arm.linux.org.uk/developer/patches/
>
> Russell, Mark Rutland wrote: "Otherwise this looks ok."
> Does that mean I should now submit via the web form?

Mason, please have a read at Russell's patch info [1]. You don't even
need to use the web form.

[1] http://www.arm.linux.org.uk/developer/patches/info.php

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151006/0580d2ad/attachment.sig>