Skip to contentSkip to conversation

ARM: ep93xx: Fix clocksource registration

PATCH New Alexander Sverdlin 3 participants 6 messages Nov 25, 2015 – Dec 14, 2015

Subsystems (4)
Arm (32-bit) Arm sub-architectures Clocksource, clockevent drivers Arm/Cirrus logic EP93XX Arm architecture
lore
Share & tools
lore

Alexander Sverdlin

author maintainer
#0 lore raw

PATCH 1/1 ARM: ep93xx: Fix clocksource registration

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 <[email protected]>
---
 arch/arm/Kconfig                    |  1 -
 arch/arm/mach-ep93xx/timer-ep93xx.c | 13 ++++++++++---
 2 files changed, 10 insertions(+), 4 deletions(-)
arch/arm/Kconfig +0 -1
index 0365cbb..c655597 100644
@@ -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
arch/arm/mach-ep93xx/timer-ep93xx.c +10 -3
index e5f7911..893992b 100644
@@ -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);
#1 lore raw

PATCH 1/1 ARM: ep93xx: Fix clocksource registration

Quoted 3 lines · 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.

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;
Quoted 12 lines
(quoted diff) +8 -0
+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)
 {

Review void __init ep93xx_timer_init(void) · line 142

Quoted 10 lines
(quoted diff) +2 -3
@@ -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:

drivers/clocksource/mmio.c +0 -0
index 1593ade2a815..c4f7d7a9b689 100644
@@ -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

author maintainer
#2 lore raw

PATCH 1/1 ARM: ep93xx: Fix clocksource registration

Hello Linus,

Quoted 11 lines · Linus Walleij
Quoted 3 lines

clocksource_mmio_init() explicitly checks for counter's width and refuses to

Quoted 2 lines

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.

#3 lore raw

PATCH 1/1 ARM: ep93xx: Fix clocksource registration

Quoted 8 lines · Alexander Sverdlin
Quoted 7 lines · Linus Walleij
Quoted 3 lines

clocksource_mmio_init() explicitly checks for counter's width and refuses to

Quoted 2 lines

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?

Quoted 8 lines
Quoted 4 lines

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

#4 lore raw

PATCH 1/1 ARM: ep93xx: Fix clocksource registration

In reply to Alexander Sverdlin
Quoted 10 lines · Alexander Sverdlin

Hello Linus,

Quoted 7 lines · Linus Walleij
Quoted 3 lines

clocksource_mmio_init() explicitly checks for counter's width and refuses to

Quoted 2 lines

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?

Quoted 8 lines
Quoted 4 lines

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

Signature · 3 lines
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.
#5 lore raw

PATCH 1/1 ARM: ep93xx: Fix clocksource registration

Quoted 16 lines · Russell King - ARM Linux
Quoted 15 lines · Alexander Sverdlin

Hello Linus,

Quoted 11 lines · Linus Walleij
Quoted 3 lines

clocksource_mmio_init() explicitly checks for counter's width and refuses to

Quoted 2 lines

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;
Quoted 6 lines
Quoted 3 lines

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