ARM: ep93xx: Fix clocksource registration

Commit Message

Linus Walleij Dec. 1, 2015, 10:09 a.m. UTC

On Wed, Nov 25, 2015 at 2:50 AM, Alexander Sverdlin
<[e-mail]> 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:

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

Yours,
Linus Walleij

Comments

Alexander Sverdlin Dec. 1, 2015, 10:14 a.m. UTC | #1

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.

Linus Walleij Dec. 10, 2015, 5:23 p.m. UTC | #2

On Tue, Dec 1, 2015 at 11:14 AM, Alexander Sverdlin
<[e-mail]> 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

Russell King - ARM Linux Dec. 10, 2015, 5:32 p.m. UTC | #3

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

Linus Walleij Dec. 14, 2015, 1:37 p.m. UTC | #4

On Thu, Dec 10, 2015 at 6:32 PM, Russell King - ARM Linux
<[e-mail]> 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

Patch

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;