Patchwork [GIT,PULL] Timer clean-ups for 3.10, Part 2

login
register
mail settings
Submitter Rob Herring
Date April 11, 2013, 8:44 p.m.
Message ID <516720B4.3000703@gmail.com>
Download mbox
Permalink /patch/235929/
State New
Headers show

Pull-request

git://sources.calxeda.com/kernel/linux.git

Comments

Rob Herring - April 11, 2013, 8:44 p.m.
Olof, Arnd,

Please pull timer clean-ups for arch timer, sp804, and integrator-cp timers.

I've dropped the final patch which moved the sp804 and integrator timers
code to drivers/clocksource until the desired structure is worked out.

Simon, the fixes for shmobile were trivial. Please comment if you see
any issues.

Rob

The following changes since commit 3d5a96582303e28c48699f3faaf920ef7d43e6f2:

  clocksource: make CLOCKSOURCE_OF_DECLARE type safe (2013-03-28
12:01:06 +0100)

are available in the git repository at:

  git://sources.calxeda.com/kernel/linux.git
tags/clksrc-cleanup-for-3.10-part2

for you to fetch changes up to 69a517b2471bcd1c5a175aad82647c1e2c24f08b:

  devtree: add binding documentation for sp804 (2013-04-11 15:11:22 -0500)

----------------------------------------------------------------
This is the 2nd part of ARM timer clean-ups for 3.10. This series has
the following changes:

- Add sched_clock selection logic to select the highest frequency clock
- Use full 64-bit arch timer counter for sched_clock
- Convert arch timer, sp804 and integrator-cp timers to CLKSRC_OF and
adapt all users to use clocksource_of_init

----------------------------------------------------------------
Arnd Bergmann (1):
      ARM: make machine_desc->init_time default to clocksource_of_init

Haojian Zhuang (1):
      devtree: add binding documentation for sp804

Rob Herring (13):
      ARM: sched_clock: allow changing to higher frequency counter
      ARM: make sched_clock just call a function pointer
      ARM: arch_timer: use full 64-bit counter for sched_clock
      ARM: convert arm/arm64 arch timer to use CLKSRC_OF init
      OF: add empty of_device_is_available for !OF
      ARM: timer-sp: convert to use CLKSRC_OF init
      ARM: highbank: use OF init for sp804 timer
      ARM: vexpress: remove sp804 OF init
      ARM: dts: vexpress: disable CA9 core tile sp804 timer
      ARM: vexpress: remove extra timer-sp control register clearing
      ARM: versatile: add versatile dtbs to dtbs target
      ARM: versatile: use OF init for sp804 timer
      ARM: integrator-cp: convert use CLKSRC_OF for timer init

 .../devicetree/bindings/timer/arm,sp804.txt        |   29 ++++
 arch/arm/Kconfig                                   |    1 +
 arch/arm/boot/dts/Makefile                         |    2 +
 arch/arm/boot/dts/integratorcp.dts                 |    6 +-
 arch/arm/boot/dts/versatile-ab.dts                 |   12 ++
 arch/arm/boot/dts/vexpress-v2p-ca9.dts             |    1 +
 arch/arm/common/timer-sp.c                         |  140
+++++++++++++++++---
 arch/arm/include/asm/arch_timer.h                  |   13 +-
 arch/arm/include/asm/hardware/timer-sp.h           |   16 ++-
 arch/arm/include/asm/sched_clock.h                 |    2 +
 arch/arm/kernel/arch_timer.c                       |   29 ++--
 arch/arm/kernel/sched_clock.c                      |   15 ++-
 arch/arm/kernel/time.c                             |    7 +-
 arch/arm/mach-exynos/mach-exynos5-dt.c             |    1 -
 arch/arm/mach-exynos/mct.c                         |    6 -
 arch/arm/mach-highbank/highbank.c                  |   24 +---
 arch/arm/mach-integrator/integrator_cp.c           |   34 -----
 arch/arm/mach-omap2/timer.c                        |    5 +-
 arch/arm/mach-shmobile/board-kzm9d.c               |    1 -
 arch/arm/mach-shmobile/setup-emev2.c               |    1 -
 arch/arm/mach-shmobile/setup-r8a7740.c             |    1 -
 arch/arm/mach-shmobile/setup-sh7372.c              |    1 -
 arch/arm/mach-shmobile/setup-sh73a0.c              |    1 -
 arch/arm/mach-shmobile/timer.c                     |    7 +-
 arch/arm/mach-versatile/core.c                     |   26 ++--
 arch/arm/mach-versatile/versatile_dt.c             |    1 -
 arch/arm/mach-vexpress/v2m.c                       |   21 +--
 arch/arm/mach-virt/virt.c                          |    9 --
 arch/arm64/include/asm/arch_timer.h                |    5 +
 arch/arm64/kernel/time.c                           |    6 +-
 drivers/clocksource/Kconfig                        |    1 +
 drivers/clocksource/arm_arch_timer.c               |   23 ++--
 include/clocksource/arm_arch_timer.h               |    6 -
 include/linux/of.h                                 |    5 +
 34 files changed, 262 insertions(+), 196 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/timer/arm,sp804.txt
Haojian Zhuang - April 12, 2013, 1:33 a.m.
On Fri, Apr 12, 2013 at 4:57 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thursday 11 April 2013, Rob Herring wrote:
>> Olof, Arnd,
>>
>> Please pull timer clean-ups for arch timer, sp804, and integrator-cp timers.
>>
>> I've dropped the final patch which moved the sp804 and integrator timers
>> code to drivers/clocksource until the desired structure is worked out.
>>
>> Simon, the fixes for shmobile were trivial. Please comment if you see
>> any issues.
>
> Ok, nice!
>
> There are various bits for which I'd like to see Russell's ACK before
> we send them Linuswards, but it all looks good to me.
>
> I would put it into arm-soc tomorrow, and hopefully Haojian Zhuang is
> able to send an updated version of his hisilicon platform code based
> on top of this in time for the merge window. The patches are all
> reviewed but just waiting for the sp804 changes now, and since
> it's a new platform, I'm not worried about regressions.
>
>         Arnd

I'll rebase code after it's merged into arm-soc git tree.

Regards
Haojian
Olof Johansson - April 13, 2013, 6:50 a.m.
On Thu, Apr 11, 2013 at 10:57:07PM +0200, Arnd Bergmann wrote:
> On Thursday 11 April 2013, Rob Herring wrote:
> > Olof, Arnd,
> > 
> > Please pull timer clean-ups for arch timer, sp804, and integrator-cp timers.
> > 
> > I've dropped the final patch which moved the sp804 and integrator timers
> > code to drivers/clocksource until the desired structure is worked out.
> > 
> > Simon, the fixes for shmobile were trivial. Please comment if you see
> > any issues.
> 
> Ok, nice!
> 
> There are various bits for which I'd like to see Russell's ACK before
> we send them Linuswards, but it all looks good to me.
> 
> I would put it into arm-soc tomorrow, and hopefully Haojian Zhuang is
> able to send an updated version of his hisilicon platform code based
> on top of this in time for the merge window. The patches are all
> reviewed but just waiting for the sp804 changes now, and since
> it's a new platform, I'm not worried about regressions.

I've pulled this in as a late/clksrc branch now, so that we keep it separate
from the other work in case Russell ends up having comments on it when he is
back online.

Also, I based the branch on samsung/mct and samsung/clk, since they had merge
conflicts. Kukjin, Rob, please confirm that I've resolved them properly.


-Olof
Rob Herring - April 15, 2013, 1:36 p.m.
On 04/13/2013 01:50 AM, Olof Johansson wrote:
> On Thu, Apr 11, 2013 at 10:57:07PM +0200, Arnd Bergmann wrote:
>> On Thursday 11 April 2013, Rob Herring wrote:
>>> Olof, Arnd,
>>>
>>> Please pull timer clean-ups for arch timer, sp804, and integrator-cp timers.
>>>
>>> I've dropped the final patch which moved the sp804 and integrator timers
>>> code to drivers/clocksource until the desired structure is worked out.
>>>
>>> Simon, the fixes for shmobile were trivial. Please comment if you see
>>> any issues.
>>
>> Ok, nice!
>>
>> There are various bits for which I'd like to see Russell's ACK before
>> we send them Linuswards, but it all looks good to me.
>>
>> I would put it into arm-soc tomorrow, and hopefully Haojian Zhuang is
>> able to send an updated version of his hisilicon platform code based
>> on top of this in time for the merge window. The patches are all
>> reviewed but just waiting for the sp804 changes now, and since
>> it's a new platform, I'm not worried about regressions.
> 
> I've pulled this in as a late/clksrc branch now, so that we keep it separate
> from the other work in case Russell ends up having comments on it when he is
> back online.
> 
> Also, I based the branch on samsung/mct and samsung/clk, since they had merge
> conflicts. Kukjin, Rob, please confirm that I've resolved them properly.

It is quite messed up because the mct timer changes are not based on
arm-soc/clksrc/cleanup. Any timer changes adding CLKSRC_OF support need
to be based on this. I've fixed it up in this branch (untested):

git://sources.calxeda.com/kernel/linux.git exynos-mct-merge

I merged with clksrc-cleanup-for-3.10-part2 and fixed it in the merge,
but really either the mct branch should be rebased or some of the
changes can be a preparatory commit before merging.

Also, I really don't like how intermingled the DT and non-DT init is in
the exynos code. We should remove of_have_populated_dt so we don't have
stuff like this:

        if (!of_have_populated_dt())
                gic_init_bases(0, IRQ_PPI(0), S5P_VA_GIC_DIST,
S5P_VA_GIC_CPU, gic_bank_offset, NULL);
#ifdef CONFIG_OF
        else
                irqchip_init();
#endif

        if (!of_have_populated_dt())
                combiner_init(S5P_VA_COMBINER_BASE, NULL);


I went thru arm-soc/for-next and all the other conversions not done by
me (mxs, sirf, zynq) are based on clksrc/cleanup.

Rob
Arnd Bergmann - April 15, 2013, 9:20 p.m.
On Monday 15 April 2013, Rob Herring wrote:
> It is quite messed up because the mct timer changes are not based on
> arm-soc/clksrc/cleanup. Any timer changes adding CLKSRC_OF support need
> to be based on this. I've fixed it up in this branch (untested):
> 
> git://sources.calxeda.com/kernel/linux.git exynos-mct-merge
> 
> I merged with clksrc-cleanup-for-3.10-part2 and fixed it in the merge,
> but really either the mct branch should be rebased or some of the
> changes can be a preparatory commit before merging.

Note that I also merged the branches when pulling in the samsung changes,
and did a complex merge there. We should ensure that we use the same
merge here.

	Arnd
Olof Johansson - April 16, 2013, 7:24 p.m.
On Mon, Apr 15, 2013 at 08:36:23AM -0500, Rob Herring wrote:
> On 04/13/2013 01:50 AM, Olof Johansson wrote:
> > On Thu, Apr 11, 2013 at 10:57:07PM +0200, Arnd Bergmann wrote:
> >> On Thursday 11 April 2013, Rob Herring wrote:
> >>> Olof, Arnd,
> >>>
> >>> Please pull timer clean-ups for arch timer, sp804, and integrator-cp timers.
> >>>
> >>> I've dropped the final patch which moved the sp804 and integrator timers
> >>> code to drivers/clocksource until the desired structure is worked out.
> >>>
> >>> Simon, the fixes for shmobile were trivial. Please comment if you see
> >>> any issues.
> >>
> >> Ok, nice!
> >>
> >> There are various bits for which I'd like to see Russell's ACK before
> >> we send them Linuswards, but it all looks good to me.
> >>
> >> I would put it into arm-soc tomorrow, and hopefully Haojian Zhuang is
> >> able to send an updated version of his hisilicon platform code based
> >> on top of this in time for the merge window. The patches are all
> >> reviewed but just waiting for the sp804 changes now, and since
> >> it's a new platform, I'm not worried about regressions.
> > 
> > I've pulled this in as a late/clksrc branch now, so that we keep it separate
> > from the other work in case Russell ends up having comments on it when he is
> > back online.
> > 
> > Also, I based the branch on samsung/mct and samsung/clk, since they had merge
> > conflicts. Kukjin, Rob, please confirm that I've resolved them properly.
> 
> It is quite messed up because the mct timer changes are not based on
> arm-soc/clksrc/cleanup. Any timer changes adding CLKSRC_OF support need
> to be based on this. I've fixed it up in this branch (untested):
> 
> git://sources.calxeda.com/kernel/linux.git exynos-mct-merge
> 
> I merged with clksrc-cleanup-for-3.10-part2 and fixed it in the merge,
> but really either the mct branch should be rebased or some of the
> changes can be a preparatory commit before merging.

Sigh. I'd like to just drop the Exynos MCT patches, but they're now a bit down
into the next/drivers branch with various dependencies into later branches too.

Given that things are already broken, I'll just fix this up in an incremental
patch on top of the late/clksrc branch for now.

> Also, I really don't like how intermingled the DT and non-DT init is in
> the exynos code. We should remove of_have_populated_dt so we don't have
> stuff like this:

Agreed. Hopefully we can remove the non-DT code when Exynos is going DT-only.

> I went thru arm-soc/for-next and all the other conversions not done by
> me (mxs, sirf, zynq) are based on clksrc/cleanup.

Good.


-Olof
Rob Herring - April 17, 2013, 1:40 p.m.
On 04/16/2013 07:27 PM, Stephen Boyd wrote:
> On 04/11/13 13:44, Rob Herring wrote:
>>
>> Rob Herring (13):
>>       ARM: sched_clock: allow changing to higher frequency counter
>>       ARM: make sched_clock just call a function pointer
>>       ARM: arch_timer: use full 64-bit counter for sched_clock
> 
> If I leave my system in the bootloader for a while this seems to cause
> my sched_clock timestamps to jump once the sched_clock is setup. It also
> sets up a sched_clock twice because read_sched_clock ==
> jiffy_sched_clock_read.
> 
> [    0.000000] Switching to timer-based delay loop
> [    0.000000] sched_clock: ARM arch timer >56 bits at 19200kHz,
> resolution 52ns
> [    0.000000] sched_clock: 32 bits at 100 Hz, resolution 10000000ns,
> wraps every 4294967286ms
> [    0.000000] Console: colour dummy device 80x30
> [16645.193054] Calibrating delay loop (skipped), value calculated using
> timer frequency.. 38.40 BogoMIPS (lpj=192000)
> 
> I suspect it's because we don't do any cyc_to_sched_clock() stuff in the
> arm architected timer case. Instead we just return the value from the
> counter when we really should do some sort of subtraction from the first
> value we read.
>
> I'm also curious how this is going to work for suspend/resume because it
> doesn't look like we're going to stop sched_clock on arm architected
> timer systems. See 6a4dae5e138a3 (ARM: 7565/1: sched: stop sched_clock()
> during suspend, 2012-10-23) for why we need to do this.


Well, I think arm64 is broken in both ways too. So we should fix this
for both.

I think this can be handled in a much more simple way than the 32-bit
code since we don't need to deal with wrapping.

Maintain a cycle offset that starts as the cycle count at init time.
This offset can be subtracted from the current count. On suspend and
resume, we need to calculate the cycle count delta while in suspend and
then add this to the cycle offset.

> 
> Finally, looks like this is unused now...

Yes, but that's unrelated to anything I did.

Rob

> 
> ---8<----
> 
> diff --git a/arch/arm/kernel/sched_clock.c b/arch/arm/kernel/sched_clock.c
> index 8805848..48ab64a 100644
> --- a/arch/arm/kernel/sched_clock.c
> +++ b/arch/arm/kernel/sched_clock.c
> @@ -24,7 +24,6 @@ struct clock_data {
>         u32 mult;
>         u32 shift;
>         bool suspended;
> -       bool needs_suspend;
>  };
>  
>  static void sched_clock_poll(unsigned long wrap_ticks);
>
Olof Johansson - April 17, 2013, 4:57 p.m.
On Mon, Apr 15, 2013 at 11:20:28PM +0200, Arnd Bergmann wrote:
> On Monday 15 April 2013, Rob Herring wrote:
> > It is quite messed up because the mct timer changes are not based on
> > arm-soc/clksrc/cleanup. Any timer changes adding CLKSRC_OF support need
> > to be based on this. I've fixed it up in this branch (untested):
> > 
> > git://sources.calxeda.com/kernel/linux.git exynos-mct-merge
> > 
> > I merged with clksrc-cleanup-for-3.10-part2 and fixed it in the merge,
> > but really either the mct branch should be rebased or some of the
> > changes can be a preparatory commit before merging.
> 
> Note that I also merged the branches when pulling in the samsung changes,
> and did a complex merge there. We should ensure that we use the same
> merge here.

Hrm, this is now a total mess. I can pick up Rob's preferred resolution,
but merging that in on top of the exynos branches causes lots of conflicts
there as well.

I might just have to rebuild the next/ branches in question to resolve
this.  Either that or bring the late/clksrc stuff into the same branch
as the exynos timer patches.


-Olof
Stephen Boyd - April 17, 2013, 6:22 p.m.
On 04/17/13 06:40, Rob Herring wrote:
> On 04/16/2013 07:27 PM, Stephen Boyd wrote:
>> On 04/11/13 13:44, Rob Herring wrote:
>>> Rob Herring (13):
>>>       ARM: sched_clock: allow changing to higher frequency counter
>>>       ARM: make sched_clock just call a function pointer
>>>       ARM: arch_timer: use full 64-bit counter for sched_clock
>> If I leave my system in the bootloader for a while this seems to cause
>> my sched_clock timestamps to jump once the sched_clock is setup. It also
>> sets up a sched_clock twice because read_sched_clock ==
>> jiffy_sched_clock_read.
>>
>> [    0.000000] Switching to timer-based delay loop
>> [    0.000000] sched_clock: ARM arch timer >56 bits at 19200kHz,
>> resolution 52ns
>> [    0.000000] sched_clock: 32 bits at 100 Hz, resolution 10000000ns,
>> wraps every 4294967286ms
>> [    0.000000] Console: colour dummy device 80x30
>> [16645.193054] Calibrating delay loop (skipped), value calculated using
>> timer frequency.. 38.40 BogoMIPS (lpj=192000)
>>
>> I suspect it's because we don't do any cyc_to_sched_clock() stuff in the
>> arm architected timer case. Instead we just return the value from the
>> counter when we really should do some sort of subtraction from the first
>> value we read.
>>
>> I'm also curious how this is going to work for suspend/resume because it
>> doesn't look like we're going to stop sched_clock on arm architected
>> timer systems. See 6a4dae5e138a3 (ARM: 7565/1: sched: stop sched_clock()
>> during suspend, 2012-10-23) for why we need to do this.
>
> Well, I think arm64 is broken in both ways too. So we should fix this
> for both.
>
> I think this can be handled in a much more simple way than the 32-bit
> code since we don't need to deal with wrapping.
>
> Maintain a cycle offset that starts as the cycle count at init time.
> This offset can be subtracted from the current count. On suspend and
> resume, we need to calculate the cycle count delta while in suspend and
> then add this to the cycle offset.

Agreed. It looks like we're missing out on the irq time accounting stuff
because enable_sched_clock_irqtime() is never called too.

>
>> Finally, looks like this is unused now...
> Yes, but that's unrelated to anything I did.

Yes,  I didn't mean to say it was anything you did.
Olof Johansson - April 17, 2013, 6:48 p.m.
On Wed, Apr 17, 2013 at 09:57:42AM -0700, Olof Johansson wrote:
> On Mon, Apr 15, 2013 at 11:20:28PM +0200, Arnd Bergmann wrote:
> > On Monday 15 April 2013, Rob Herring wrote:
> > > It is quite messed up because the mct timer changes are not based on
> > > arm-soc/clksrc/cleanup. Any timer changes adding CLKSRC_OF support need
> > > to be based on this. I've fixed it up in this branch (untested):
> > > 
> > > git://sources.calxeda.com/kernel/linux.git exynos-mct-merge
> > > 
> > > I merged with clksrc-cleanup-for-3.10-part2 and fixed it in the merge,
> > > but really either the mct branch should be rebased or some of the
> > > changes can be a preparatory commit before merging.
> > 
> > Note that I also merged the branches when pulling in the samsung changes,
> > and did a complex merge there. We should ensure that we use the same
> > merge here.
> 
> Hrm, this is now a total mess. I can pick up Rob's preferred resolution,
> but merging that in on top of the exynos branches causes lots of conflicts
> there as well.
> 
> I might just have to rebuild the next/ branches in question to resolve
> this.  Either that or bring the late/clksrc stuff into the same branch
> as the exynos timer patches.

Ok, given the complex merge fixup by Arnd (I'm not sure it's a great
idea to add the new code in the merge commit like that, by the way),
I've chosen to go with that as a base for late/clksrc instead, and I've
pushed a new rebased late/clksrc that contains Arnd's version of the
resolutions instead of Rob's. Let me know if further fixups are needed,
in particular on exynos4 since I lack hardware to test that side.


-Olof
Rob Herring - April 17, 2013, 11:18 p.m.
On 04/17/2013 01:22 PM, Stephen Boyd wrote:
> On 04/17/13 06:40, Rob Herring wrote:
>> On 04/16/2013 07:27 PM, Stephen Boyd wrote:
>>> On 04/11/13 13:44, Rob Herring wrote:
>>>> Rob Herring (13):
>>>>       ARM: sched_clock: allow changing to higher frequency counter
>>>>       ARM: make sched_clock just call a function pointer
>>>>       ARM: arch_timer: use full 64-bit counter for sched_clock
>>> If I leave my system in the bootloader for a while this seems to cause
>>> my sched_clock timestamps to jump once the sched_clock is setup. It also
>>> sets up a sched_clock twice because read_sched_clock ==
>>> jiffy_sched_clock_read.
>>>
>>> [    0.000000] Switching to timer-based delay loop
>>> [    0.000000] sched_clock: ARM arch timer >56 bits at 19200kHz,
>>> resolution 52ns
>>> [    0.000000] sched_clock: 32 bits at 100 Hz, resolution 10000000ns,
>>> wraps every 4294967286ms
>>> [    0.000000] Console: colour dummy device 80x30
>>> [16645.193054] Calibrating delay loop (skipped), value calculated using
>>> timer frequency.. 38.40 BogoMIPS (lpj=192000)
>>>
>>> I suspect it's because we don't do any cyc_to_sched_clock() stuff in the
>>> arm architected timer case. Instead we just return the value from the
>>> counter when we really should do some sort of subtraction from the first
>>> value we read.
>>>
>>> I'm also curious how this is going to work for suspend/resume because it
>>> doesn't look like we're going to stop sched_clock on arm architected
>>> timer systems. See 6a4dae5e138a3 (ARM: 7565/1: sched: stop sched_clock()
>>> during suspend, 2012-10-23) for why we need to do this.
>>
>> Well, I think arm64 is broken in both ways too. So we should fix this
>> for both.
>>
>> I think this can be handled in a much more simple way than the 32-bit
>> code since we don't need to deal with wrapping.
>>
>> Maintain a cycle offset that starts as the cycle count at init time.
>> This offset can be subtracted from the current count. On suspend and
>> resume, we need to calculate the cycle count delta while in suspend and
>> then add this to the cycle offset.
> 
> Agreed. It looks like we're missing out on the irq time accounting stuff
> because enable_sched_clock_irqtime() is never called too.
> 

Yes, but until ARM selects HAVE_IRQ_TIME_ACCOUNTING that doesn't matter.
Looks like adding that was forgotten.

Rob
Kukjin Kim - April 19, 2013, 10:52 a.m.
Olof Johansson wrote:
> 

[...]

> > Hrm, this is now a total mess. I can pick up Rob's preferred resolution,
> > but merging that in on top of the exynos branches causes lots of
> conflicts
> > there as well.
> >
> > I might just have to rebuild the next/ branches in question to resolve
> > this.  Either that or bring the late/clksrc stuff into the same branch
> > as the exynos timer patches.
> 
> Ok, given the complex merge fixup by Arnd (I'm not sure it's a great
> idea to add the new code in the merge commit like that, by the way),
> I've chosen to go with that as a base for late/clksrc instead, and I've
> pushed a new rebased late/clksrc that contains Arnd's version of the
> resolutions instead of Rob's. Let me know if further fixups are needed,
> in particular on exynos4 since I lack hardware to test that side.
> 
Let me test it on exynos4 in this weekend then I'll let you know.

I hope it will not be late for you.

Thanks.

- Kukjin