[PATCH] ARM: ep93xx: Fix clocksource registration

[PATCH] ARM: ep93xx: Fix clocksource registration

[Alexander Sverdlin]

clocksource_mmio_init() explicitly checks for counter's width and refuses to
register anything with resolution higher than 32 bits. We need to use
clocksource_register_hz() directly to repair HIGH_RES_TIMERS.

=== Before ===

$ cat /sys/bus/clocksource/devices/clocksource0/available_clocksource
jiffies

$ cat /proc/timer_list
[...]
cpu: 0
 clock 0:
  .base:       c0388ae0
  .index:      0
  .resolution: 10000000 nsecs
  .get_time:   ktime_get
  .offset:     0 nsecs
active timers:
[...]
Tick Device: mode:     0
Per CPU device: 0
Clock Event Device: timer1
 max_delta_ns:   8446860587738
 min_delta_ns:   1967
 mult:           1091929
 shift:          31
 mode:           3
 next_event:     130610000000 nsecs
 set_next_event: ep93xx_clkevt_set_next_event
 shutdown: ep93xx_clkevt_shutdown
 oneshot:  ep93xx_clkevt_shutdown
 resume:   ep93xx_clkevt_shutdown
 event_handler:  tick_handle_periodic
 retries:        0

=== After ===

$ cat /sys/bus/clocksource/devices/clocksource0/available_clocksource
timer4

$ cat /proc/timer_list
[...]
cpu: 0
 clock 0:
  .base:       c0388b40
  .index:      0
  .resolution: 1 nsecs
  .get_time:   ktime_get
  .offset:     0 nsecs
active timers:
[...]
Tick Device: mode:     1
Per CPU device: 0
Clock Event Device: timer1
 max_delta_ns:   8446860587738
 min_delta_ns:   1967
 mult:           1091929
 shift:          31
 mode:           3
 next_event:     101000000000 nsecs
 set_next_event: ep93xx_clkevt_set_next_event
 shutdown: ep93xx_clkevt_shutdown
 oneshot:  ep93xx_clkevt_shutdown
 resume:   ep93xx_clkevt_shutdown
 event_handler:  hrtimer_interrupt
 retries:        1771

Signed-off-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>
---
 arch/arm/Kconfig                    |  1 -
 arch/arm/mach-ep93xx/timer-ep93xx.c | 13 ++++++++++---
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 0365cbb..c655597 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -422,7 +422,6 @@ config ARCH_EP93XX
 	select ARM_VIC
 	select AUTO_ZRELADDR
 	select CLKDEV_LOOKUP
-	select CLKSRC_MMIO
 	select CPU_ARM920T
 	select GENERIC_CLOCKEVENTS
 	help
diff --git a/arch/arm/mach-ep93xx/timer-ep93xx.c b/arch/arm/mach-ep93xx/timer-ep93xx.c
index e5f7911..893992b 100644
--- a/arch/arm/mach-ep93xx/timer-ep93xx.c
+++ b/arch/arm/mach-ep93xx/timer-ep93xx.c
@@ -68,6 +68,14 @@ cycle_t ep93xx_clocksource_read(struct clocksource *c)
 	return (cycle_t) ret;
 }

+static struct clocksource timer4_clocksource = {
+	.name	= "timer4",
+	.rating	= 200,
+	.read	= ep93xx_clocksource_read,
+	.mask	= CLOCKSOURCE_MASK(40),
+	.flags	= CLOCK_SOURCE_IS_CONTINUOUS,
+};
+
 static int ep93xx_clkevt_set_next_event(unsigned long next,
 					struct clock_event_device *evt)
 {
@@ -128,9 +136,8 @@ void __init ep93xx_timer_init(void)
 	/* Enable and register clocksource and sched_clock on timer 4 */
 	writel(EP93XX_TIMER4_VALUE_HIGH_ENABLE,
 	       EP93XX_TIMER4_VALUE_HIGH);
-	clocksource_mmio_init(NULL, "timer4",
-			      EP93XX_TIMER4_RATE, 200, 40,
-			      ep93xx_clocksource_read);
+	if (clocksource_register_hz(&timer4_clocksource, EP93XX_TIMER4_RATE))
+		pr_warn("Failed to register Timer4 as clocksource");
 	sched_clock_register(ep93xx_read_sched_clock, 40,
 			     EP93XX_TIMER4_RATE);

[PATCH] ARM: ep93xx: Fix clocksource registration

[Linus Walleij]

On Wed, Nov 25, 2015 at 2:50 AM, Alexander Sverdlin
<alexander.sverdlin@gmail.com> wrote:

> clocksource_mmio_init() explicitly checks for counter's width and refuses to
> register anything with resolution higher than 32 bits. We need to use
> clocksource_register_hz() directly to repair HIGH_RES_TIMERS.

Maybe tglx want to jump in on this.

Isn't it better to fix clocksource_mmio_init() to accept > 32 bits?

What is really prompting this in drivers/clocksource/mmio.c:

if (bits > 32 || bits < 16)
                return -EINVAL;


> +static struct clocksource timer4_clocksource = {
> +       .name   = "timer4",
> +       .rating = 200,
> +       .read   = ep93xx_clocksource_read,
> +       .mask   = CLOCKSOURCE_MASK(40),
> +       .flags  = CLOCK_SOURCE_IS_CONTINUOUS,
> +};
> +
>  static int ep93xx_clkevt_set_next_event(unsigned long next,
>                                         struct clock_event_device *evt)
>  {
> @@ -128,9 +136,8 @@ void __init ep93xx_timer_init(void)
>         /* Enable and register clocksource and sched_clock on timer 4 */
>         writel(EP93XX_TIMER4_VALUE_HIGH_ENABLE,
>                EP93XX_TIMER4_VALUE_HIGH);
> -       clocksource_mmio_init(NULL, "timer4",
> -                             EP93XX_TIMER4_RATE, 200, 40,
> -                             ep93xx_clocksource_read);
> +       if (clocksource_register_hz(&timer4_clocksource, EP93XX_TIMER4_RATE))
> +               pr_warn("Failed to register Timer4 as clocksource");
>         sched_clock_register(ep93xx_read_sched_clock, 40,
>                              EP93XX_TIMER4_RATE);

Isn't this a better fix:

diff --git a/drivers/clocksource/mmio.c b/drivers/clocksource/mmio.c
index 1593ade2a815..c4f7d7a9b689 100644
--- a/drivers/clocksource/mmio.c
+++ b/drivers/clocksource/mmio.c
@@ -55,7 +55,7 @@ int __init clocksource_mmio_init(void __iomem *base,
const char *name,
 {
        struct clocksource_mmio *cs;

-       if (bits > 32 || bits < 16)
+       if (bits > 64 || bits < 16)
                return -EINVAL;

        cs = kzalloc(sizeof(struct clocksource_mmio), GFP_KERNEL);

Yours,
Linus Walleij

[PATCH] ARM: ep93xx: Fix clocksource registration

[Alexander Sverdlin]

Hello Linus,

On 01/12/15 11:09, Linus Walleij wrote:
>> clocksource_mmio_init() explicitly checks for counter's width and refuses to
>> > register anything with resolution higher than 32 bits. We need to use
>> > clocksource_register_hz() directly to repair HIGH_RES_TIMERS.
> Maybe tglx want to jump in on this.
> 
> Isn't it better to fix clocksource_mmio_init() to accept > 32 bits?
> 
> What is really prompting this in drivers/clocksource/mmio.c:
> 
> if (bits > 32 || bits < 16)
>                 return -EINVAL;

probably lack of >32bit access functions in the driver. But I do not know
the original intentions of the author. In any case, if we modify the check here,
probably the access function for 64 bits should be moved to the mmio.c?

Alex.

[PATCH] ARM: ep93xx: Fix clocksource registration

[Linus Walleij]

On Tue, Dec 1, 2015 at 11:14 AM, Alexander Sverdlin
<alexander.sverdlin@gmail.com> wrote:
> On 01/12/15 11:09, Linus Walleij wrote:
>>> clocksource_mmio_init() explicitly checks for counter's width and refuses to
>>> > register anything with resolution higher than 32 bits. We need to use
>>> > clocksource_register_hz() directly to repair HIGH_RES_TIMERS.
>> Maybe tglx want to jump in on this.
>>
>> Isn't it better to fix clocksource_mmio_init() to accept > 32 bits?
>>
>> What is really prompting this in drivers/clocksource/mmio.c:
>>
>> if (bits > 32 || bits < 16)
>>                 return -EINVAL;
>
> probably lack of >32bit access functions in the driver. But I do not know
> the original intentions of the author. In any case, if we modify the check here,
> probably the access function for 64 bits should be moved to the mmio.c?

The clocksource returns a cycle_t which is 64 bits. I have sent a proper
patch to Daniel+tglx fixing it up.

Yours,
Linus Walleij

[PATCH] ARM: ep93xx: Fix clocksource registration

[Russell King - ARM Linux]

On Tue, Dec 01, 2015 at 11:14:02AM +0100, Alexander Sverdlin wrote:
> Hello Linus,
> 
> On 01/12/15 11:09, Linus Walleij wrote:
> >> clocksource_mmio_init() explicitly checks for counter's width and refuses to
> >> > register anything with resolution higher than 32 bits. We need to use
> >> > clocksource_register_hz() directly to repair HIGH_RES_TIMERS.
> > Maybe tglx want to jump in on this.
> > 
> > Isn't it better to fix clocksource_mmio_init() to accept > 32 bits?
> > 
> > What is really prompting this in drivers/clocksource/mmio.c:
> > 
> > if (bits > 32 || bits < 16)
> >                 return -EINVAL;
> 
> probably lack of >32bit access functions in the driver. But I do not know
> the original intentions of the author. In any case, if we modify the check here,
> probably the access function for 64 bits should be moved to the mmio.c?

If people want to use it with 64-bit accessors, then yes.  So says
the author of that file (me).

-- 
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.

[PATCH] ARM: ep93xx: Fix clocksource registration

[Linus Walleij]

On Thu, Dec 10, 2015 at 6:32 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Dec 01, 2015 at 11:14:02AM +0100, Alexander Sverdlin wrote:
>> Hello Linus,
>>
>> On 01/12/15 11:09, Linus Walleij wrote:
>> >> clocksource_mmio_init() explicitly checks for counter's width and refuses to
>> >> > register anything with resolution higher than 32 bits. We need to use
>> >> > clocksource_register_hz() directly to repair HIGH_RES_TIMERS.
>> > Maybe tglx want to jump in on this.
>> >
>> > Isn't it better to fix clocksource_mmio_init() to accept > 32 bits?
>> >
>> > What is really prompting this in drivers/clocksource/mmio.c:
>> >
>> > if (bits > 32 || bits < 16)
>> >                 return -EINVAL;
>>
>> probably lack of >32bit access functions in the driver. But I do not know
>> the original intentions of the author. In any case, if we modify the check here,
>> probably the access function for 64 bits should be moved to the mmio.c?
>
> If people want to use it with 64-bit accessors, then yes.  So says
> the author of that file (me).

I've sent a patch to tglx / Daniel Lezcano that I think does the proper
fix to the clksrc mmio core.

Yours,
Linus Walleij