From mboxrd@z Thu Jan 1 00:00:00 1970 From: Maxime Coquelin Subject: Re: [PATCH v3 04/15] clocksource: Add ARM System timer driver Date: Thu, 26 Mar 2015 21:19:58 +0100 Message-ID: References: <1426197361-19290-1-git-send-email-maxime.coquelin@st.com> <1426197361-19290-5-git-send-email-maxime.coquelin@st.com> <5513D64A.7060702@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: <5513D64A.7060702@linaro.org> Sender: linux-arch-owner@vger.kernel.org To: Daniel Lezcano Cc: =?UTF-8?Q?Uwe_Kleine=2DK=C3=B6nig?= , =?UTF-8?Q?Andreas_F=C3=A4rber?= , Geert Uytterhoeven , Rob Herring , Philipp Zabel , Linus Walleij , Arnd Bergmann , Stefan Agner , Peter Meerwald , Paul Bolle , Jonathan Corbet , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Russell King , Thomas Gleixner , Greg Kroah-Hartman , Jiri Slaby , Andrew Morton , "David S. Miller" , Mauro Carvalho Chehab List-Id: linux-gpio@vger.kernel.org Hi Daniel, Thanks for the review. Please find my answers below. 2015-03-26 10:50 GMT+01:00 Daniel Lezcano : > On 03/12/2015 10:55 PM, Maxime Coquelin wrote: >> >> From: Maxime Coquelin >> >> This patch adds clocksource support for ARMv7-M's System timer, >> also known as SysTick. >> >> Signed-off-by: Maxime Coquelin > > > Hi Maxime, > > the driver looks good. Three comments below. > > -- Daniel > > >> --- >> 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/Kconf= ig >> index 1c2506f..b82e58b 100644 >> --- a/drivers/clocksource/Kconfig >> +++ b/drivers/clocksource/Kconfig >> @@ -134,6 +134,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 u= nit >> + >> config ATMEL_PIT >> select CLKSRC_OF if OF >> def_bool SOC_AT91SAM9 || SOC_SAMA5 >> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Make= file >> index 752d5c7..1c9a643 100644 >> --- a/drivers/clocksource/Makefile >> +++ b/drivers/clocksource/Makefile >> @@ -44,6 +44,7 @@ obj-$(CONFIG_MTK_TIMER) +=3D mtk_tim= er.o >> >> obj-$(CONFIG_ARM_ARCH_TIMER) +=3D arm_arch_timer.o >> obj-$(CONFIG_ARM_GLOBAL_TIMER) +=3D arm_global_time= r.o >> +obj-$(CONFIG_ARMV7M_SYSTICK) +=3D armv7m_systick.o >> obj-$(CONFIG_CLKSRC_METAG_GENERIC) +=3D metag_generic.o >> obj-$(CONFIG_ARCH_HAS_TICK_BROADCAST) +=3D dummy_timer.o >> obj-$(CONFIG_ARCH_KEYSTONE) +=3D 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 >> + * License terms: GNU General Public License (GPL), version 2 >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#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 =3D 0; >> + int ret; >> + >> + base =3D of_iomap(np, 0); >> + if (!base) { >> + pr_warn("system-timer: invalid base address\n"); >> + return; >> + } >> + >> + clk =3D of_clk_get(np, 0); >> + if (!IS_ERR(clk)) { >> + ret =3D clk_prepare_enable(clk); >> + if (ret) { >> + clk_put(clk); >> + goto out_unmap; >> + } >> + >> + rate =3D clk_get_rate(clk); >> + } >> + >> + /* If no clock found, try to get clock-frequency property */ >> + if (!rate) { >> + ret =3D of_property_read_u32(np, "clock-frequency", = &rate); >> + if (ret) >> + goto out_unmap; > > > Shouldn't be 'goto out_clk_disable' ? No, because I assumed !rate means we failed to get the clock. Actually, clk_get_rate could return 0, so relying on rate value is not = safe. I propose to get clock-frequency property if IS_ERR(clk). Is it fine for you? > >> + } >> + >> + writel_relaxed(SYSTICK_LOAD_RELOAD_MASK, base + SYST_RVR); >> + writel_relaxed(SYST_CSR_ENABLE, base + SYST_CSR); >> + >> + ret =3D clocksource_mmio_init(base + SYST_CVR, "arm_system_t= imer", >> 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)) > > > Why do you need this check ? To handle the case were no clock was found, but a clk-frequency value was provided. > > It isn't missing a clk_put ? Right, thanks for spotting this. I wonder if it makes sense to implement the error path. If we fail to initialize the clocksource, the system will be unusable. Maybe I should just perform a BUG_ON() in the error cases, as most of the other clocksource drivers do. What is your view? Regards, Maxime > >> + 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); >> > > > -- > Linaro.org =E2=94=82 Open source software f= or ARM SoCs > > Follow Linaro: Facebook | > Twitter | > Blog > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752870AbbCZUUH (ORCPT ); Thu, 26 Mar 2015 16:20:07 -0400 Received: from mail-wg0-f47.google.com ([74.125.82.47]:35100 "EHLO mail-wg0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752544AbbCZUUA convert rfc822-to-8bit (ORCPT ); Thu, 26 Mar 2015 16:20:00 -0400 MIME-Version: 1.0 In-Reply-To: <5513D64A.7060702@linaro.org> References: <1426197361-19290-1-git-send-email-maxime.coquelin@st.com> <1426197361-19290-5-git-send-email-maxime.coquelin@st.com> <5513D64A.7060702@linaro.org> Date: Thu, 26 Mar 2015 21:19:58 +0100 Message-ID: Subject: Re: [PATCH v3 04/15] clocksource: Add ARM System timer driver From: Maxime Coquelin To: Daniel Lezcano Cc: =?UTF-8?Q?Uwe_Kleine=2DK=C3=B6nig?= , =?UTF-8?Q?Andreas_F=C3=A4rber?= , Geert Uytterhoeven , Rob Herring , Philipp Zabel , Linus Walleij , Arnd Bergmann , Stefan Agner , Peter Meerwald , Paul Bolle , Jonathan Corbet , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Russell King , Thomas Gleixner , Greg Kroah-Hartman , Jiri Slaby , Andrew Morton , "David S. Miller" , Mauro Carvalho Chehab , Joe Perches , Antti Palosaari , Tejun Heo , Will Deacon , Nikolay Borisov , Rusty Russell , Kees Cook , Michal Marek , "linux-doc@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" , "devicetree@vger.kernel.org" , "linux-gpio@vger.kernel.org" , "linux-serial@vger.kernel.org" , Linux-Arch , "linux-api@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Daniel, Thanks for the review. Please find my answers below. 2015-03-26 10:50 GMT+01:00 Daniel Lezcano : > On 03/12/2015 10:55 PM, Maxime Coquelin wrote: >> >> From: Maxime Coquelin >> >> This patch adds clocksource support for ARMv7-M's System timer, >> also known as SysTick. >> >> Signed-off-by: Maxime Coquelin > > > Hi Maxime, > > the driver looks good. Three comments below. > > -- Daniel > > >> --- >> 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 1c2506f..b82e58b 100644 >> --- a/drivers/clocksource/Kconfig >> +++ b/drivers/clocksource/Kconfig >> @@ -134,6 +134,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 752d5c7..1c9a643 100644 >> --- a/drivers/clocksource/Makefile >> +++ b/drivers/clocksource/Makefile >> @@ -44,6 +44,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 >> + * License terms: GNU General Public License (GPL), version 2 >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#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; > > > Shouldn't be 'goto out_clk_disable' ? No, because I assumed !rate means we failed to get the clock. Actually, clk_get_rate could return 0, so relying on rate value is not safe. I propose to get clock-frequency property if IS_ERR(clk). Is it fine for you? > >> + } >> + >> + 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)) > > > Why do you need this check ? To handle the case were no clock was found, but a clk-frequency value was provided. > > It isn't missing a clk_put ? Right, thanks for spotting this. I wonder if it makes sense to implement the error path. If we fail to initialize the clocksource, the system will be unusable. Maybe I should just perform a BUG_ON() in the error cases, as most of the other clocksource drivers do. What is your view? Regards, Maxime > >> + 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); >> > > > -- > Linaro.org │ Open source software for ARM SoCs > > Follow Linaro: Facebook | > Twitter | > Blog > From mboxrd@z Thu Jan 1 00:00:00 1970 From: mcoquelin.stm32@gmail.com (Maxime Coquelin) Date: Thu, 26 Mar 2015 21:19:58 +0100 Subject: [PATCH v3 04/15] clocksource: Add ARM System timer driver In-Reply-To: <5513D64A.7060702@linaro.org> References: <1426197361-19290-1-git-send-email-maxime.coquelin@st.com> <1426197361-19290-5-git-send-email-maxime.coquelin@st.com> <5513D64A.7060702@linaro.org> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Daniel, Thanks for the review. Please find my answers below. 2015-03-26 10:50 GMT+01:00 Daniel Lezcano : > On 03/12/2015 10:55 PM, Maxime Coquelin wrote: >> >> From: Maxime Coquelin >> >> This patch adds clocksource support for ARMv7-M's System timer, >> also known as SysTick. >> >> Signed-off-by: Maxime Coquelin > > > Hi Maxime, > > the driver looks good. Three comments below. > > -- Daniel > > >> --- >> 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 1c2506f..b82e58b 100644 >> --- a/drivers/clocksource/Kconfig >> +++ b/drivers/clocksource/Kconfig >> @@ -134,6 +134,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 752d5c7..1c9a643 100644 >> --- a/drivers/clocksource/Makefile >> +++ b/drivers/clocksource/Makefile >> @@ -44,6 +44,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 >> + * License terms: GNU General Public License (GPL), version 2 >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#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; > > > Shouldn't be 'goto out_clk_disable' ? No, because I assumed !rate means we failed to get the clock. Actually, clk_get_rate could return 0, so relying on rate value is not safe. I propose to get clock-frequency property if IS_ERR(clk). Is it fine for you? > >> + } >> + >> + 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)) > > > Why do you need this check ? To handle the case were no clock was found, but a clk-frequency value was provided. > > It isn't missing a clk_put ? Right, thanks for spotting this. I wonder if it makes sense to implement the error path. If we fail to initialize the clocksource, the system will be unusable. Maybe I should just perform a BUG_ON() in the error cases, as most of the other clocksource drivers do. What is your view? Regards, Maxime > >> + 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); >> > > > -- > Linaro.org ? Open source software for ARM SoCs > > Follow Linaro: Facebook | > Twitter | > Blog >