diff mbox

[04/12] clocksource: Sched clock source for Versatile Express

Message ID 1392138636-29240-5-git-send-email-pawel.moll@arm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Pawel Moll Feb. 11, 2014, 5:10 p.m. UTC
This patch adds a trival sched clock source using free
running, 24MHz clocked counter present in the ARM Ltd.
Versatile Express platform's System Registers block.

This code replaces the call in the VE machine code.

Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Pawel Moll <pawel.moll@arm.com>
---
 arch/arm/mach-vexpress/v2m.c   |  2 --
 drivers/clocksource/Kconfig    |  9 +++++++++
 drivers/clocksource/Makefile   |  1 +
 drivers/clocksource/vexpress.c | 40 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 50 insertions(+), 2 deletions(-)
 create mode 100644 drivers/clocksource/vexpress.c

Comments

Rob Herring April 16, 2014, 1:56 p.m. UTC | #1
Adding Linus W...

On Tue, Feb 11, 2014 at 11:10 AM, Pawel Moll <pawel.moll@arm.com> wrote:
> This patch adds a trival sched clock source using free
> running, 24MHz clocked counter present in the ARM Ltd.
> Versatile Express platform's System Registers block.
>
> This code replaces the call in the VE machine code.
>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Pawel Moll <pawel.moll@arm.com>
> ---
>  arch/arm/mach-vexpress/v2m.c   |  2 --
>  drivers/clocksource/Kconfig    |  9 +++++++++
>  drivers/clocksource/Makefile   |  1 +
>  drivers/clocksource/vexpress.c | 40 ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 50 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/clocksource/vexpress.c
>
> diff --git a/arch/arm/mach-vexpress/v2m.c b/arch/arm/mach-vexpress/v2m.c
> index 90f04c9..d8a9fd7 100644
> --- a/arch/arm/mach-vexpress/v2m.c
> +++ b/arch/arm/mach-vexpress/v2m.c
> @@ -418,8 +418,6 @@ void __init v2m_dt_init_early(void)
>                         pr_warning("vexpress: DT HBI (%x) is not matching "
>                                         "hardware (%x)!\n", dt_hbi, hbi);
>         }
> -
> -       versatile_sched_clock_init(vexpress_get_24mhz_clock_base(), 24000000);
>  }
>
>
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index cd6950f..9799744 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -140,3 +140,12 @@ config VF_PIT_TIMER
>         bool
>         help
>           Support for Period Interrupt Timer on Freescale Vybrid Family SoCs.
> +
> +config CLKSRC_VEXPRESS
> +       bool
> +       depends on MFD_VEXPRESS_SYSREG

But you don't really depend on this...

> +       depends on GENERIC_SCHED_CLOCK

I think this should be a select, not a depends.

> +       select CLKSRC_OF
> +       default y
> +       help
> +         Simple provider of sched clock on ARM Versatile Express platform.
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index c7ca50a..1051a23 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -37,3 +37,4 @@ obj-$(CONFIG_ARM_ARCH_TIMER)          += arm_arch_timer.o
>  obj-$(CONFIG_ARM_GLOBAL_TIMER)         += arm_global_timer.o
>  obj-$(CONFIG_CLKSRC_METAG_GENERIC)     += metag_generic.o
>  obj-$(CONFIG_ARCH_HAS_TICK_BROADCAST)  += dummy_timer.o
> +obj-$(CONFIG_CLKSRC_VEXPRESS)  += vexpress.o
> diff --git a/drivers/clocksource/vexpress.c b/drivers/clocksource/vexpress.c
> new file mode 100644
> index 0000000..55b8ab4
> --- /dev/null
> +++ b/drivers/clocksource/vexpress.c
> @@ -0,0 +1,40 @@
> +/*
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * Copyright (C) 2013 ARM Limited

Did you write this last year?

> + */
> +
> +#include <linux/clocksource.h>
> +#include <linux/io.h>
> +#include <linux/of_address.h>
> +#include <linux/sched_clock.h>
> +
> +#define SYS_24MHZ 0x05c
> +
> +static void __iomem *vexpress_sys_24mhz;
> +
> +static u32 notrace vexpress_sys_24mhz_read(void)
> +{
> +       return readl(vexpress_sys_24mhz);
> +}
> +
> +static void __init vexpress_sched_clock_init(struct device_node *node)
> +{
> +       void __iomem *base = of_iomap(node, 0);
> +
> +       if (!base)
> +               return;
> +
> +       vexpress_sys_24mhz = base + SYS_24MHZ;
> +
> +       setup_sched_clock(vexpress_sys_24mhz_read, 32, 24000000);

This frequency should come from a DT clock binding. You will have to
fallback to 24MHz for backwards compatibility though.

> +}

Wouldn't this code work for Versatile and Realview ARM reference
boards? Even the register offset is the same.

> +CLOCKSOURCE_OF_DECLARE(vexpress, "arm,vexpress-sysreg",
> +               vexpress_sched_clock_init);
> --
> 1.8.3.2
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Pawel Moll April 16, 2014, 2:22 p.m. UTC | #2
Hi Rob,

Thanks for reminding me about this stuff. I'll get around and re-spin
the series this week.

On Wed, 2014-04-16 at 14:56 +0100, Rob Herring wrote:
> > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> > index cd6950f..9799744 100644
> > --- a/drivers/clocksource/Kconfig
> > +++ b/drivers/clocksource/Kconfig
> > @@ -140,3 +140,12 @@ config VF_PIT_TIMER
> >         bool
> >         help
> >           Support for Period Interrupt Timer on Freescale Vybrid Family SoCs.
> > +
> > +config CLKSRC_VEXPRESS
> > +       bool
> > +       depends on MFD_VEXPRESS_SYSREG
> 
> But you don't really depend on this...

Hm. Strictly speaking it's true (no code level dependency) but if one
doesn't build sysreg driver, one doesn't care 

> > +       depends on GENERIC_SCHED_CLOCK
> 
> I think this should be a select, not a depends.

Don't think so, no. It's being selected by an arch. 

> > +       select CLKSRC_OF
> > +       default y
> > +       help
> > +         Simple provider of sched clock on ARM Versatile Express platform.
> > diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> > index c7ca50a..1051a23 100644
> > --- a/drivers/clocksource/Makefile
> > +++ b/drivers/clocksource/Makefile
> > @@ -37,3 +37,4 @@ obj-$(CONFIG_ARM_ARCH_TIMER)          += arm_arch_timer.o
> >  obj-$(CONFIG_ARM_GLOBAL_TIMER)         += arm_global_timer.o
> >  obj-$(CONFIG_CLKSRC_METAG_GENERIC)     += metag_generic.o
> >  obj-$(CONFIG_ARCH_HAS_TICK_BROADCAST)  += dummy_timer.o
> > +obj-$(CONFIG_CLKSRC_VEXPRESS)  += vexpress.o
> > diff --git a/drivers/clocksource/vexpress.c b/drivers/clocksource/vexpress.c
> > new file mode 100644
> > index 0000000..55b8ab4
> > --- /dev/null
> > +++ b/drivers/clocksource/vexpress.c
> > @@ -0,0 +1,40 @@
> > +/*
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * Copyright (C) 2013 ARM Limited
> 
> Did you write this last year?

Yes. Can bump it up anyway.

> > + */
> > +
> > +#include <linux/clocksource.h>
> > +#include <linux/io.h>
> > +#include <linux/of_address.h>
> > +#include <linux/sched_clock.h>
> > +
> > +#define SYS_24MHZ 0x05c
> > +
> > +static void __iomem *vexpress_sys_24mhz;
> > +
> > +static u32 notrace vexpress_sys_24mhz_read(void)
> > +{
> > +       return readl(vexpress_sys_24mhz);
> > +}
> > +
> > +static void __init vexpress_sched_clock_init(struct device_node *node)
> > +{
> > +       void __iomem *base = of_iomap(node, 0);
> > +
> > +       if (!base)
> > +               return;
> > +
> > +       vexpress_sys_24mhz = base + SYS_24MHZ;
> > +
> > +       setup_sched_clock(vexpress_sys_24mhz_read, 32, 24000000);
> 
> This frequency should come from a DT clock binding. You will have to
> fallback to 24MHz for backwards compatibility though.

I don't see why would it go to the binding. You may have noticed the
register is called "SYS_24MHZ", not "SYS_RANDOMCLOCK". The driver
*knows* what the frequency is.

> > +}
> 
> Wouldn't this code work for Versatile and Realview ARM reference
> boards? Even the register offset is the same.
> 
> > +CLOCKSOURCE_OF_DECLARE(vexpress, "arm,vexpress-sysreg",
> > +               vexpress_sched_clock_init);

I guess it would, yes. The sysregs are annoyingly similar and different
at the same time.

One could of course try to come up with a "generic mmio clock source"
binding, taking the frequency as a property, but don't count on me doing
this... ;-)

Pawel
Rob Herring April 16, 2014, 2:45 p.m. UTC | #3
On Wed, Apr 16, 2014 at 9:22 AM, Pawel Moll <pawel.moll@arm.com> wrote:
> Hi Rob,
>
> Thanks for reminding me about this stuff. I'll get around and re-spin
> the series this week.
>

[snip]

>> > +       setup_sched_clock(vexpress_sys_24mhz_read, 32, 24000000);
>>
>> This frequency should come from a DT clock binding. You will have to
>> fallback to 24MHz for backwards compatibility though.
>
> I don't see why would it go to the binding. You may have noticed the
> register is called "SYS_24MHZ", not "SYS_RANDOMCLOCK". The driver
> *knows* what the frequency is.

A 24MHz clock is fed to this h/w block to be used by the counter in
the block. The DT should describe that.

>
>> > +}
>>
>> Wouldn't this code work for Versatile and Realview ARM reference
>> boards? Even the register offset is the same.
>>
>> > +CLOCKSOURCE_OF_DECLARE(vexpress, "arm,vexpress-sysreg",
>> > +               vexpress_sched_clock_init);
>
> I guess it would, yes. The sysregs are annoyingly similar and different
> at the same time.
>
> One could of course try to come up with a "generic mmio clock source"
> binding, taking the frequency as a property, but don't count on me doing
> this... ;-)

I'm not asking for that. Just take care of all ARM Ltd boards which
have the exact same 24MHz counter at offset 0x5C.

Rob
Pawel Moll April 16, 2014, 3:05 p.m. UTC | #4
On Wed, 2014-04-16 at 15:45 +0100, Rob Herring wrote:
> >> > +       setup_sched_clock(vexpress_sys_24mhz_read, 32, 24000000);
> >>
> >> This frequency should come from a DT clock binding. You will have to
> >> fallback to 24MHz for backwards compatibility though.
> >
> > I don't see why would it go to the binding. You may have noticed the
> > register is called "SYS_24MHZ", not "SYS_RANDOMCLOCK". The driver
> > *knows* what the frequency is.
> 
> A 24MHz clock is fed to this h/w block to be used by the counter in
> the block. The DT should describe that.

I even have a clk24mhz in the motherboard's dtsi, so adding a phandle to
the node is trivial, can do it. But my point is that, whatever this
clock is (24MHz, 12MHz then multiplied by 2 inside sysreg, 48MHz divided
by 2 etc.), the value in this register is incremented every 1/24 uS by
definition. And the driver can rely on this. Otherwise we're really
talking about a generic mmio-counter-based clock source, with a
clock-frequency property or a clock phandle (and a multiplier/divider
then?).

> >> > +}
> >>
> >> Wouldn't this code work for Versatile and Realview ARM reference
> >> boards? Even the register offset is the same.
> >>
> >> > +CLOCKSOURCE_OF_DECLARE(vexpress, "arm,vexpress-sysreg",
> >> > +               vexpress_sched_clock_init);
> >
> > I guess it would, yes. The sysregs are annoyingly similar and different
> > at the same time.
> >
> > One could of course try to come up with a "generic mmio clock source"
> > binding, taking the frequency as a property, but don't count on me doing
> > this... ;-)
> 
> I'm not asking for that. Just take care of all ARM Ltd boards which
> have the exact same 24MHz counter at offset 0x5C.

Yes, this is doable by all means. I'll rename it to
"clocksource/versatile.c" so we can add relevant compatible values.

Pawe?
Linus Walleij May 2, 2014, 10:14 p.m. UTC | #5
On Tue, Feb 11, 2014 at 9:10 AM, Pawel Moll <pawel.moll@arm.com> wrote:

> This patch adds a trival sched clock source using free
> running, 24MHz clocked counter present in the ARM Ltd.
> Versatile Express platform's System Registers block.
>
> This code replaces the call in the VE machine code.
>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Pawel Moll <pawel.moll@arm.com>

Even the Integrator is very similar, albeit using register 0x28 instead
of register 0x5c. cd commit
a79528e9d849803457b6235ddb1a1cfd4e11c6cd

It's one of these things where writing a device driver results in
more code than just registering this counter with the sched_clock
guts :-/

Getting the clock from the DT gives this nice feeling of having it all
complete, and sched_clock cannot really change frequency anyway
so I'm happy with this thing.

I'll augment it for Integrator when/if I find time.

Yours,
Linus Walleij
Pawel Moll May 7, 2014, 9:57 a.m. UTC | #6
On Fri, 2014-05-02 at 23:14 +0100, Linus Walleij wrote:
> On Tue, Feb 11, 2014 at 9:10 AM, Pawel Moll <pawel.moll@arm.com> wrote:
> 
> > This patch adds a trival sched clock source using free
> > running, 24MHz clocked counter present in the ARM Ltd.
> > Versatile Express platform's System Registers block.
> >
> > This code replaces the call in the VE machine code.
> >
> > Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Signed-off-by: Pawel Moll <pawel.moll@arm.com>
> 
> Even the Integrator is very similar, albeit using register 0x28 instead
> of register 0x5c. cd commit
> a79528e9d849803457b6235ddb1a1cfd4e11c6cd
> 
> It's one of these things where writing a device driver results in
> more code than just registering this counter with the sched_clock
> guts :-/
> 
> Getting the clock from the DT gives this nice feeling of having it all
> complete, and sched_clock cannot really change frequency anyway
> so I'm happy with this thing.
> 
> I'll augment it for Integrator when/if I find time.

Can I consider this as Reviewed-by? :-)

Pawel
Linus Walleij May 13, 2014, 8:47 a.m. UTC | #7
On Wed, May 7, 2014 at 11:57 AM, Pawel Moll <pawel.moll@arm.com> wrote:
> On Fri, 2014-05-02 at 23:14 +0100, Linus Walleij wrote:
(Me: blah blah)

> Can I consider this as Reviewed-by? :-)

Yep.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij
diff mbox

Patch

diff --git a/arch/arm/mach-vexpress/v2m.c b/arch/arm/mach-vexpress/v2m.c
index 90f04c9..d8a9fd7 100644
--- a/arch/arm/mach-vexpress/v2m.c
+++ b/arch/arm/mach-vexpress/v2m.c
@@ -418,8 +418,6 @@  void __init v2m_dt_init_early(void)
 			pr_warning("vexpress: DT HBI (%x) is not matching "
 					"hardware (%x)!\n", dt_hbi, hbi);
 	}
-
-	versatile_sched_clock_init(vexpress_get_24mhz_clock_base(), 24000000);
 }
 
 
diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index cd6950f..9799744 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -140,3 +140,12 @@  config VF_PIT_TIMER
 	bool
 	help
 	  Support for Period Interrupt Timer on Freescale Vybrid Family SoCs.
+
+config CLKSRC_VEXPRESS
+	bool
+	depends on MFD_VEXPRESS_SYSREG
+	depends on GENERIC_SCHED_CLOCK
+	select CLKSRC_OF
+	default y
+	help
+	  Simple provider of sched clock on ARM Versatile Express platform.
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index c7ca50a..1051a23 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -37,3 +37,4 @@  obj-$(CONFIG_ARM_ARCH_TIMER)		+= arm_arch_timer.o
 obj-$(CONFIG_ARM_GLOBAL_TIMER)		+= arm_global_timer.o
 obj-$(CONFIG_CLKSRC_METAG_GENERIC)	+= metag_generic.o
 obj-$(CONFIG_ARCH_HAS_TICK_BROADCAST)	+= dummy_timer.o
+obj-$(CONFIG_CLKSRC_VEXPRESS)	+= vexpress.o
diff --git a/drivers/clocksource/vexpress.c b/drivers/clocksource/vexpress.c
new file mode 100644
index 0000000..55b8ab4
--- /dev/null
+++ b/drivers/clocksource/vexpress.c
@@ -0,0 +1,40 @@ 
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * Copyright (C) 2013 ARM Limited
+ */
+
+#include <linux/clocksource.h>
+#include <linux/io.h>
+#include <linux/of_address.h>
+#include <linux/sched_clock.h>
+
+#define SYS_24MHZ 0x05c
+
+static void __iomem *vexpress_sys_24mhz;
+
+static u32 notrace vexpress_sys_24mhz_read(void)
+{
+	return readl(vexpress_sys_24mhz);
+}
+
+static void __init vexpress_sched_clock_init(struct device_node *node)
+{
+	void __iomem *base = of_iomap(node, 0);
+
+	if (!base)
+		return;
+
+	vexpress_sys_24mhz = base + SYS_24MHZ;
+
+	setup_sched_clock(vexpress_sys_24mhz_read, 32, 24000000);
+}
+CLOCKSOURCE_OF_DECLARE(vexpress, "arm,vexpress-sysreg",
+		vexpress_sched_clock_init);