diff mbox

ARM: ep93xx: Fix clocksource registration

Message ID CACRpkdZy7TnrmkWrRerjm6dZW9m65Dsc_kp8TPHzbhTxz5Hbrw@mail.gmail.com
State New
Headers show

Commit Message

Linus Walleij Dec. 1, 2015, 10:09 a.m. UTC 
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:

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

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Comments

Linus Walleij Dec. 10, 2015, 5:23 p.m. UTC | #1 
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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Linus Walleij Dec. 14, 2015, 1:37 p.m. UTC | #2 
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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff mbox

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;