[v2,04/18] clocksource: Add ARM System timer driver
diff mbox

Message ID 1424455277-29983-5-git-send-email-mcoquelin.stm32@gmail.com
State New
Headers show

Commit Message

Maxime Coquelin Feb. 20, 2015, 6:01 p.m. UTC
This patch adds clocksource support for ARMv7-M's System timer,
also known as SysTick.

Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
---
 drivers/clocksource/Kconfig          |  7 ++++
 drivers/clocksource/Makefile         |  1 +
 drivers/clocksource/armv7m_systick.c | 78 ++++++++++++++++++++++++++++++++++++
 3 files changed, 86 insertions(+)
 create mode 100644 drivers/clocksource/armv7m_systick.c

Comments

Uwe Kleine-König Feb. 20, 2015, 7:54 p.m. UTC | #1
Hello,

On Fri, Feb 20, 2015 at 07:01:03PM +0100, Maxime Coquelin wrote:
> This patch adds clocksource support for ARMv7-M's System timer,
> also known as SysTick.
> 
> Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
> ---
>  drivers/clocksource/Kconfig          |  7 ++++
>  drivers/clocksource/Makefile         |  1 +
>  drivers/clocksource/armv7m_systick.c | 78 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 86 insertions(+)
>  create mode 100644 drivers/clocksource/armv7m_systick.c
> 
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index fc01ec2..fb6011e 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -124,6 +124,13 @@ config CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK
>  	help
>  	 Use ARM global timer clock source as sched_clock
>  
> +config ARMV7M_SYSTICK
> +	bool
I assume this symbol is enabled later in the series. Would it make sense
to allow enabing the symbol for compile test coverage?

> +	select CLKSRC_OF if OF
What happens if ARMV7M_SYSTICK=y but OF=n? Doesn't the driver fail to
compile?

> +	select CLKSRC_MMIO
> +	help
> +	  This options enables support for the ARMv7M system timer unit
the right spelling is ARMv7-M.

Best regards
Uwe
Paul Bolle Feb. 20, 2015, 9:48 p.m. UTC | #2
On Fri, 2015-02-20 at 20:54 +0100, Uwe Kleine-König wrote:
> On Fri, Feb 20, 2015 at 07:01:03PM +0100, Maxime Coquelin wrote:
> > This patch adds clocksource support for ARMv7-M's System timer,
> > also known as SysTick.
> > 
> > Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
> > ---
> >  drivers/clocksource/Kconfig          |  7 ++++
> >  drivers/clocksource/Makefile         |  1 +
> >  drivers/clocksource/armv7m_systick.c | 78 ++++++++++++++++++++++++++++++++++++
> >  3 files changed, 86 insertions(+)
> >  create mode 100644 drivers/clocksource/armv7m_systick.c
> > 
> > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> > index fc01ec2..fb6011e 100644
> > --- a/drivers/clocksource/Kconfig
> > +++ b/drivers/clocksource/Kconfig
> > @@ -124,6 +124,13 @@ config CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK
> >  	help
> >  	 Use ARM global timer clock source as sched_clock
> >  
> > +config ARMV7M_SYSTICK
> > +	bool
> I assume this symbol is enabled later in the series.

Yes, I noticed it was selected in 14/18 ("ARM: Add STM32 family
machine").

> Would it make sense
> to allow enabing the symbol for compile test coverage?
> 
> > +	select CLKSRC_OF if OF
> What happens if ARMV7M_SYSTICK=y but OF=n? Doesn't the driver fail to
> compile?
> 
> > +	select CLKSRC_MMIO
> > +	help
> > +	  This options enables support for the ARMv7M system timer unit
> the right spelling is ARMv7-M.

This Kconfig entry has no prompt, so no one is going to see this text
during make *config. Perhaps this should be made a comment. In that case
the right spelling should still be used.

Thanks,


Paul Bolle

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Maxime Coquelin March 2, 2015, 4:53 p.m. UTC | #3
Hi Paul, Uwe,

2015-02-20 22:48 GMT+01:00 Paul Bolle <pebolle@tiscali.nl>:
> On Fri, 2015-02-20 at 20:54 +0100, Uwe Kleine-König wrote:
>> On Fri, Feb 20, 2015 at 07:01:03PM +0100, Maxime Coquelin wrote:
>> > This patch adds clocksource support for ARMv7-M's System timer,
>> > also known as SysTick.
>> >
>> > Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
>> > ---
>> >  drivers/clocksource/Kconfig          |  7 ++++
>> >  drivers/clocksource/Makefile         |  1 +
>> >  drivers/clocksource/armv7m_systick.c | 78 ++++++++++++++++++++++++++++++++++++
>> >  3 files changed, 86 insertions(+)
>> >  create mode 100644 drivers/clocksource/armv7m_systick.c
>> >
>> > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
>> > index fc01ec2..fb6011e 100644
>> > --- a/drivers/clocksource/Kconfig
>> > +++ b/drivers/clocksource/Kconfig
>> > @@ -124,6 +124,13 @@ config CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK
>> >     help
>> >      Use ARM global timer clock source as sched_clock
>> >
>> > +config ARMV7M_SYSTICK
>> > +   bool
>> I assume this symbol is enabled later in the series.
>
> Yes, I noticed it was selected in 14/18 ("ARM: Add STM32 family
> machine").
>
>> Would it make sense
>> to allow enabing the symbol for compile test coverage?
>>
>> > +   select CLKSRC_OF if OF
>> What happens if ARMV7M_SYSTICK=y but OF=n? Doesn't the driver fail to
>> compile?
>>
>> > +   select CLKSRC_MMIO
>> > +   help
>> > +     This options enables support for the ARMv7M system timer unit
>> the right spelling is ARMv7-M.
>
> This Kconfig entry has no prompt, so no one is going to see this text
> during make *config. Perhaps this should be made a comment. In that case
> the right spelling should still be used.

Yes, you are right.
Do you agree if I define it like this:

config ARMV7M_SYSTICK
    bool "Clocksource driver for ARMv7-M System timer"
    depends on OF && (CPU_V7M || COMPILE_TEST)
    select CLKSRC_OF
    select CLKSRC_MMIO
    help
      This options enables clocksource support for the ARMv7-M system
      timer unit.

Thanks,
Maxime
>
> Thanks,
>
>
> Paul Bolle
>
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paul Bolle March 3, 2015, 7:43 p.m. UTC | #4
Maxime Coquelin schreef op ma 02-03-2015 om 17:53 [+0100]:
> Do you agree if I define it like this:
> 
> config ARMV7M_SYSTICK
>     bool "Clocksource driver for ARMv7-M System timer"
>     depends on OF && (CPU_V7M || COMPILE_TEST)
>     select CLKSRC_OF
>     select CLKSRC_MMIO
>     help
>       This options enables clocksource support for the ARMv7-M system
>       timer unit.

I don't really have strong feelings on whatever way you choose to fix
the, well, minor problem I pointed out.

Having said that, if a Kconfig entry without a prompt (and therefor,
without help) actually does what you want it to do, why bother adding a
prompt and a one line help text?


Paul Bolle

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Maxime Coquelin March 4, 2015, 12:08 p.m. UTC | #5
2015-03-03 20:43 GMT+01:00 Paul Bolle <pebolle@tiscali.nl>:
> Maxime Coquelin schreef op ma 02-03-2015 om 17:53 [+0100]:
>> Do you agree if I define it like this:
>>
>> config ARMV7M_SYSTICK
>>     bool "Clocksource driver for ARMv7-M System timer"
>>     depends on OF && (CPU_V7M || COMPILE_TEST)
>>     select CLKSRC_OF
>>     select CLKSRC_MMIO
>>     help
>>       This options enables clocksource support for the ARMv7-M system
>>       timer unit.
>
> I don't really have strong feelings on whatever way you choose to fix
> the, well, minor problem I pointed out.
>
> Having said that, if a Kconfig entry without a prompt (and therefor,
> without help) actually does what you want it to do, why bother adding a
> prompt and a one line help text?

This is because I added also support for COMPILE_TEST coverage as per
Uwe advice,
and thought it was necessary to have an entry for this.
Maybe I'm just wrong?

Thanks,
Maxime

>
>
> Paul Bolle
>
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij March 9, 2015, 3:50 p.m. UTC | #6
On Fri, Feb 20, 2015 at 7:01 PM, Maxime Coquelin
<mcoquelin.stm32@gmail.com> wrote:

> This patch adds clocksource support for ARMv7-M's System timer,
> also known as SysTick.
>
> Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
(...)
> +       /* If no clock found, try to get clock-frequency property */
> +       if (!rate) {
> +               ret = of_property_read_u32(np, "clock-frequency", &rate);
> +               if (ret)
> +                       goto out_unmap;
> +       }

If this driver is only used for this one system, and if on this one system
the clk subsystem will provide the clock rate, then there is no need
to include this hackaround property.

Alternatively there is no point including reading the frequency from
the clk subsystem for this one system.

So which one is it?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Maxime Coquelin March 9, 2015, 5 p.m. UTC | #7
2015-03-09 16:50 GMT+01:00 Linus Walleij <linus.walleij@linaro.org>:
> On Fri, Feb 20, 2015 at 7:01 PM, Maxime Coquelin
> <mcoquelin.stm32@gmail.com> wrote:
>
>> This patch adds clocksource support for ARMv7-M's System timer,
>> also known as SysTick.
>>
>> Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
> (...)
>> +       /* If no clock found, try to get clock-frequency property */
>> +       if (!rate) {
>> +               ret = of_property_read_u32(np, "clock-frequency", &rate);
>> +               if (ret)
>> +                       goto out_unmap;
>> +       }
>
> If this driver is only used for this one system, and if on this one system
> the clk subsystem will provide the clock rate, then there is no need
> to include this hackaround property.
>
> Alternatively there is no point including reading the frequency from
> the clk subsystem for this one system.
>
> So which one is it?

In the first version, the "clock-frequency" property handling was not here.
Rob Herring advised me to add its support, as it could be used by
simple systems not selecting CCF.

So, I don't have the name of a system where it could be useful, but I
think Rob's request make sense.

Note that I tested using the clock-frequency property before sending.

Best regards,
Maxime
>
> Yours,
> Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paul Bolle March 9, 2015, 9:12 p.m. UTC | #8
On Wed, 2015-03-04 at 13:08 +0100, Maxime Coquelin wrote:
> This is because I added also support for COMPILE_TEST coverage as per
> Uwe advice,
> and thought it was necessary to have an entry for this.
> Maybe I'm just wrong?

I missed that you added COMPILE_TEST.

A quick scan of your idea doesn't show any obvious issues. (Note that I
don't really know how people actually use COMPILE_TEST. I guess things
like "make allyesconfig" are involved.)


Paul Bolle

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index fc01ec2..fb6011e 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -124,6 +124,13 @@  config CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK
 	help
 	 Use ARM global timer clock source as sched_clock
 
+config ARMV7M_SYSTICK
+	bool
+	select CLKSRC_OF if OF
+	select CLKSRC_MMIO
+	help
+	  This options enables support for the ARMv7M system timer unit
+
 config ATMEL_PIT
 	select CLKSRC_OF if OF
 	def_bool SOC_AT91SAM9 || SOC_SAMA5
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 94d90b2..eeea736 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -42,6 +42,7 @@  obj-$(CONFIG_MTK_TIMER)		+= mtk_timer.o
 
 obj-$(CONFIG_ARM_ARCH_TIMER)		+= arm_arch_timer.o
 obj-$(CONFIG_ARM_GLOBAL_TIMER)		+= arm_global_timer.o
+obj-$(CONFIG_ARMV7M_SYSTICK)		+= armv7m_systick.o
 obj-$(CONFIG_CLKSRC_METAG_GENERIC)	+= metag_generic.o
 obj-$(CONFIG_ARCH_HAS_TICK_BROADCAST)	+= dummy_timer.o
 obj-$(CONFIG_ARCH_KEYSTONE)		+= timer-keystone.o
diff --git a/drivers/clocksource/armv7m_systick.c b/drivers/clocksource/armv7m_systick.c
new file mode 100644
index 0000000..23d8249
--- /dev/null
+++ b/drivers/clocksource/armv7m_systick.c
@@ -0,0 +1,78 @@ 
+/*
+ * Copyright (C) Maxime Coquelin 2015
+ * Author:  Maxime Coquelin <mcoquelin.stm32@gmail.com>
+ * License terms:  GNU General Public License (GPL), version 2
+ */
+
+#include <linux/kernel.h>
+#include <linux/clocksource.h>
+#include <linux/clockchips.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/clk.h>
+#include <linux/bitops.h>
+
+#define SYST_CSR	0x00
+#define SYST_RVR	0x04
+#define SYST_CVR	0x08
+#define SYST_CALIB	0x0c
+
+#define SYST_CSR_ENABLE BIT(0)
+
+#define SYSTICK_LOAD_RELOAD_MASK 0x00FFFFFF
+
+static void __init system_timer_of_register(struct device_node *np)
+{
+	struct clk *clk;
+	void __iomem *base;
+	u32 rate = 0;
+	int ret;
+
+	base = of_iomap(np, 0);
+	if (!base) {
+		pr_warn("system-timer: invalid base address\n");
+		return;
+	}
+
+	clk = of_clk_get(np, 0);
+	if (!IS_ERR(clk)) {
+		ret = clk_prepare_enable(clk);
+		if (ret) {
+			clk_put(clk);
+			goto out_unmap;
+		}
+
+		rate = clk_get_rate(clk);
+	}
+
+	/* If no clock found, try to get clock-frequency property */
+	if (!rate) {
+		ret = of_property_read_u32(np, "clock-frequency", &rate);
+		if (ret)
+			goto out_unmap;
+	}
+
+	writel_relaxed(SYSTICK_LOAD_RELOAD_MASK, base + SYST_RVR);
+	writel_relaxed(SYST_CSR_ENABLE, base + SYST_CSR);
+
+	ret = clocksource_mmio_init(base + SYST_CVR, "arm_system_timer", rate,
+			200, 24, clocksource_mmio_readl_down);
+	if (ret) {
+		pr_err("failed to init clocksource (%d)\n", ret);
+		goto out_clk_disable;
+	}
+
+	pr_info("ARM System timer initialized as clocksource\n");
+
+	return;
+
+out_clk_disable:
+	if (!IS_ERR(clk))
+		clk_disable_unprepare(clk);
+out_unmap:
+	iounmap(base);
+	WARN(ret, "ARM System timer register failed (%d)\n", ret);
+}
+
+CLOCKSOURCE_OF_DECLARE(arm_systick, "arm,armv7m-systick",
+			system_timer_of_register);