| Message ID | 1424455277-29983-5-git-send-email-mcoquelin.stm32@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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 >
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
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 >
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
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
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
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);
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