* [PATCH 0/2] stm32 clocksource driver rework
@ 2017-09-14 7:56 Benjamin Gaignard
2017-09-14 7:56 ` [PATCH 1/2] clocksource: stm32: rework driver to use only one timer Benjamin Gaignard
2017-09-14 7:56 ` [PATCH 2/2] arm: dts: stm32: remove useless clocksource nodes Benjamin Gaignard
0 siblings, 2 replies; 9+ messages in thread
From: Benjamin Gaignard @ 2017-09-14 7:56 UTC (permalink / raw)
To: robh+dt, mark.rutland, linux, mcoquelin.stm32, alexandre.torgue,
daniel.lezcano, tglx, ludovic.barre
Cc: devicetree, linux-arm-kernel, linux-kernel, Benjamin Gaignard
These patch implements clocksource and clockevent by using only one hardware block.
It also limits usage of clocksource to 32 bits timers because 16 bits ones
aren't enough accurate.
Series includes minor clean up in structures, function prototypes and driver name.
Since 16 bits timers become useless it also removes them from stm32f4 and
stm32f7 devicetree.
Benjamin Gaignard (2):
clocksource: stm32: rework driver to use only one timer
arm: dts: stm32: remove useless clocksource nodes
arch/arm/boot/dts/stm32f429.dtsi | 32 -----
arch/arm/boot/dts/stm32f746.dtsi | 32 -----
drivers/clocksource/timer-stm32.c | 259 +++++++++++++++++++++++---------------
3 files changed, 155 insertions(+), 168 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] clocksource: stm32: rework driver to use only one timer
2017-09-14 7:56 [PATCH 0/2] stm32 clocksource driver rework Benjamin Gaignard
@ 2017-09-14 7:56 ` Benjamin Gaignard
2017-09-15 18:27 ` kbuild test robot
2017-09-18 21:30 ` Daniel Lezcano
2017-09-14 7:56 ` [PATCH 2/2] arm: dts: stm32: remove useless clocksource nodes Benjamin Gaignard
1 sibling, 2 replies; 9+ messages in thread
From: Benjamin Gaignard @ 2017-09-14 7:56 UTC (permalink / raw)
To: robh+dt, mark.rutland, linux, mcoquelin.stm32, alexandre.torgue,
daniel.lezcano, tglx, ludovic.barre
Cc: devicetree, linux-arm-kernel, linux-kernel, Benjamin Gaignard
Rework driver code to use only one timer for both clocksource
and clockevent.
This patch also forbids to use 16 bits timers because they are
not enough accurate.
Do some clean up in structures and functions names too.
Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
---
drivers/clocksource/timer-stm32.c | 259 +++++++++++++++++++++++---------------
1 file changed, 155 insertions(+), 104 deletions(-)
diff --git a/drivers/clocksource/timer-stm32.c b/drivers/clocksource/timer-stm32.c
index 8f24237..648c10a 100644
--- a/drivers/clocksource/timer-stm32.c
+++ b/drivers/clocksource/timer-stm32.c
@@ -16,175 +16,226 @@
#include <linux/of_irq.h>
#include <linux/clk.h>
#include <linux/reset.h>
+#include <linux/sched_clock.h>
+#include <linux/slab.h>
#define TIM_CR1 0x00
#define TIM_DIER 0x0c
#define TIM_SR 0x10
#define TIM_EGR 0x14
+#define TIM_CNT 0x24
#define TIM_PSC 0x28
#define TIM_ARR 0x2c
+#define TIM_CCR1 0x34
#define TIM_CR1_CEN BIT(0)
-#define TIM_CR1_OPM BIT(3)
+#define TIM_CR1_UDIS BIT(1)
#define TIM_CR1_ARPE BIT(7)
-#define TIM_DIER_UIE BIT(0)
-
-#define TIM_SR_UIF BIT(0)
+#define TIM_DIER_CC1IE BIT(1)
#define TIM_EGR_UG BIT(0)
-struct stm32_clock_event_ddata {
+struct stm32_clock_event {
struct clock_event_device evtdev;
unsigned periodic_top;
- void __iomem *base;
+ void __iomem *regs;
};
static int stm32_clock_event_shutdown(struct clock_event_device *evtdev)
{
- struct stm32_clock_event_ddata *data =
- container_of(evtdev, struct stm32_clock_event_ddata, evtdev);
- void *base = data->base;
+ struct stm32_clock_event *ce =
+ container_of(evtdev, struct stm32_clock_event, evtdev);
+
+ writel_relaxed(0, ce->regs + TIM_DIER);
- writel_relaxed(0, base + TIM_CR1);
return 0;
}
-static int stm32_clock_event_set_periodic(struct clock_event_device *evtdev)
+static int stm32_clock_event_set_next_event(unsigned long evt,
+ struct clock_event_device *evtdev)
{
- struct stm32_clock_event_ddata *data =
- container_of(evtdev, struct stm32_clock_event_ddata, evtdev);
- void *base = data->base;
+ struct stm32_clock_event *ce =
+ container_of(evtdev, struct stm32_clock_event, evtdev);
+ unsigned long cnt;
+
+ cnt = readl_relaxed(ce->regs + TIM_CNT);
+ writel_relaxed(cnt + evt, ce->regs + TIM_CCR1);
+ writel_relaxed(TIM_DIER_CC1IE, ce->regs + TIM_DIER);
- writel_relaxed(data->periodic_top, base + TIM_ARR);
- writel_relaxed(TIM_CR1_ARPE | TIM_CR1_CEN, base + TIM_CR1);
return 0;
}
-static int stm32_clock_event_set_next_event(unsigned long evt,
- struct clock_event_device *evtdev)
+static int stm32_clock_event_set_periodic(struct clock_event_device *evtdev)
{
- struct stm32_clock_event_ddata *data =
- container_of(evtdev, struct stm32_clock_event_ddata, evtdev);
+ struct stm32_clock_event *ce =
+ container_of(evtdev, struct stm32_clock_event, evtdev);
- writel_relaxed(evt, data->base + TIM_ARR);
- writel_relaxed(TIM_CR1_ARPE | TIM_CR1_OPM | TIM_CR1_CEN,
- data->base + TIM_CR1);
+ return stm32_clock_event_set_next_event(ce->periodic_top, evtdev);
+}
- return 0;
+static int stm32_clock_event_set_oneshot(struct clock_event_device *evtdev)
+{
+ return stm32_clock_event_set_next_event(0, evtdev);
}
static irqreturn_t stm32_clock_event_handler(int irq, void *dev_id)
{
- struct stm32_clock_event_ddata *data = dev_id;
+ struct stm32_clock_event *ce = dev_id;
+
+ writel_relaxed(0, ce->regs + TIM_SR);
- writel_relaxed(0, data->base + TIM_SR);
+ if (clockevent_state_periodic(&ce->evtdev))
+ stm32_clock_event_set_periodic(&ce->evtdev);
- data->evtdev.event_handler(&data->evtdev);
+ if (clockevent_state_oneshot(&ce->evtdev))
+ stm32_clock_event_shutdown(&ce->evtdev);
+
+ ce->evtdev.event_handler(&ce->evtdev);
return IRQ_HANDLED;
}
-static struct stm32_clock_event_ddata clock_event_ddata = {
- .evtdev = {
- .name = "stm32 clockevent",
- .features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_PERIODIC,
- .set_state_shutdown = stm32_clock_event_shutdown,
- .set_state_periodic = stm32_clock_event_set_periodic,
- .set_state_oneshot = stm32_clock_event_shutdown,
- .tick_resume = stm32_clock_event_shutdown,
- .set_next_event = stm32_clock_event_set_next_event,
- .rating = 200,
- },
-};
+static int __init stm32_clockevent_init(struct device_node *np,
+ void __iomem *base,
+ struct clk *clk, int irq)
+{
+ struct stm32_clock_event *ce;
+ unsigned long rate;
+ int err;
+
+ ce = kzalloc(sizeof(*ce), GFP_KERNEL);
+ if (!ce)
+ return -ENOMEM;
+
+ ce->regs = base;
+ ce->evtdev.name = "stm32_clockevent";
+ ce->evtdev.features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_PERIODIC;
+ ce->evtdev.set_state_shutdown = stm32_clock_event_shutdown;
+ ce->evtdev.set_state_periodic = stm32_clock_event_set_periodic;
+ ce->evtdev.set_state_oneshot = stm32_clock_event_set_oneshot;
+ ce->evtdev.tick_resume = stm32_clock_event_shutdown;
+ ce->evtdev.set_next_event = stm32_clock_event_set_next_event;
+ ce->evtdev.rating = 200;
-static int __init stm32_clockevent_init(struct device_node *np)
+ rate = clk_get_rate(clk);
+ ce->periodic_top = DIV_ROUND_CLOSEST(rate, HZ);
+
+ writel_relaxed(0, ce->regs + TIM_DIER);
+ writel_relaxed(0, ce->regs + TIM_SR);
+
+ err = request_irq(irq, stm32_clock_event_handler, IRQF_TIMER,
+ "stm32 clockevent", ce);
+ if (err) {
+ kfree(ce);
+ return err;
+ }
+
+ clockevents_config_and_register(&ce->evtdev, rate, 0x60, ~0U);
+
+ return 0;
+}
+
+static void __iomem *stm32_timer_cnt __read_mostly;
+static u64 notrace stm32_read_sched_clock(void)
+{
+ return readl_relaxed(stm32_timer_cnt);
+}
+
+static int __init stm32_clocksource_init(struct device_node *node,
+ void __iomem *regs,
+ struct clk *clk)
+{
+ unsigned long rate;
+
+ rate = clk_get_rate(clk);
+
+ writel_relaxed(~0U, regs + TIM_ARR);
+ writel_relaxed(0, regs + TIM_PSC);
+ writel_relaxed(0, regs + TIM_SR);
+ writel_relaxed(0, regs + TIM_DIER);
+ writel_relaxed(0, regs + TIM_SR);
+ writel_relaxed(TIM_CR1_ARPE | TIM_CR1_UDIS, regs + TIM_CR1);
+
+ /* Make sure that registers are updated */
+ writel_relaxed(TIM_EGR_UG, regs + TIM_EGR);
+
+ /* Enable controller */
+ writel_relaxed(TIM_CR1_ARPE | TIM_CR1_UDIS | TIM_CR1_CEN,
+ regs + TIM_CR1);
+
+ stm32_timer_cnt = regs + TIM_CNT;
+ sched_clock_register(stm32_read_sched_clock, 32, rate);
+
+ return clocksource_mmio_init(stm32_timer_cnt, "stm32_timer",
+ rate, 250, 32, clocksource_mmio_readl_up);
+}
+
+static int __init stm32_timer_init(struct device_node *node)
{
- struct stm32_clock_event_ddata *data = &clock_event_ddata;
- struct clk *clk;
struct reset_control *rstc;
- unsigned long rate, max_delta;
- int irq, ret, bits, prescaler = 1;
+ void __iomem *timer_base;
+ unsigned long max_arr;
+ struct clk *clk;
+ int irq, err;
- clk = of_clk_get(np, 0);
- if (IS_ERR(clk)) {
- ret = PTR_ERR(clk);
- pr_err("failed to get clock for clockevent (%d)\n", ret);
- goto err_clk_get;
+ timer_base = of_io_request_and_map(node, 0, of_node_full_name(node));
+ if (IS_ERR(timer_base)) {
+ pr_err("Can't map registers\n");
+ goto out;
}
- ret = clk_prepare_enable(clk);
- if (ret) {
- pr_err("failed to enable timer clock for clockevent (%d)\n",
- ret);
- goto err_clk_enable;
+ irq = irq_of_parse_and_map(node, 0);
+ if (irq <= 0) {
+ pr_err("Can't parse IRQ\n");
+ goto out_unmap;
}
- rate = clk_get_rate(clk);
+ clk = of_clk_get(node, 0);
+ if (IS_ERR(clk)) {
+ pr_err("Can't get timer clock\n");
+ goto out_unmap;
+ }
- rstc = of_reset_control_get(np, NULL);
+ rstc = of_reset_control_get(node, NULL);
if (!IS_ERR(rstc)) {
reset_control_assert(rstc);
reset_control_deassert(rstc);
}
- data->base = of_iomap(np, 0);
- if (!data->base) {
- ret = -ENXIO;
- pr_err("failed to map registers for clockevent\n");
- goto err_iomap;
- }
-
- irq = irq_of_parse_and_map(np, 0);
- if (!irq) {
- ret = -EINVAL;
- pr_err("%pOF: failed to get irq.\n", np);
- goto err_get_irq;
+ err = clk_prepare_enable(clk);
+ if (err) {
+ pr_err("Couldn't enable parent clock\n");
+ goto out_clk;
}
/* Detect whether the timer is 16 or 32 bits */
- writel_relaxed(~0U, data->base + TIM_ARR);
- max_delta = readl_relaxed(data->base + TIM_ARR);
- if (max_delta == ~0U) {
- prescaler = 1;
- bits = 32;
- } else {
- prescaler = 1024;
- bits = 16;
+ writel_relaxed(~0U, timer_base + TIM_ARR);
+ max_arr = readl_relaxed(timer_base + TIM_ARR);
+ if (max_arr != ~0U) {
+ err = -EINVAL;
+ pr_err("32 bits timer is needed\n");
+ goto out_unprepare;
}
- writel_relaxed(0, data->base + TIM_ARR);
-
- writel_relaxed(prescaler - 1, data->base + TIM_PSC);
- writel_relaxed(TIM_EGR_UG, data->base + TIM_EGR);
- writel_relaxed(TIM_DIER_UIE, data->base + TIM_DIER);
- writel_relaxed(0, data->base + TIM_SR);
-
- data->periodic_top = DIV_ROUND_CLOSEST(rate, prescaler * HZ);
- clockevents_config_and_register(&data->evtdev,
- DIV_ROUND_CLOSEST(rate, prescaler),
- 0x1, max_delta);
+ err = stm32_clocksource_init(node, timer_base, clk);
+ if (err)
+ goto out_unprepare;
- ret = request_irq(irq, stm32_clock_event_handler, IRQF_TIMER,
- "stm32 clockevent", data);
- if (ret) {
- pr_err("%pOF: failed to request irq.\n", np);
- goto err_get_irq;
- }
-
- pr_info("%pOF: STM32 clockevent driver initialized (%d bits)\n",
- np, bits);
+ err = stm32_clockevent_init(node, timer_base, clk, irq);
+ if (err)
+ goto out_unprepare;
- return ret;
+ return 0;
-err_get_irq:
- iounmap(data->base);
-err_iomap:
+out_unprepare:
clk_disable_unprepare(clk);
-err_clk_enable:
+out_clk:
clk_put(clk);
-err_clk_get:
- return ret;
+out_unmap:
+ iounmap(timer_base);
+out:
+ return err;
}
-TIMER_OF_DECLARE(stm32, "st,stm32-timer", stm32_clockevent_init);
+CLOCKSOURCE_OF_DECLARE(stm32, "st,stm32-timer", stm32_timer_init);
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] arm: dts: stm32: remove useless clocksource nodes
2017-09-14 7:56 [PATCH 0/2] stm32 clocksource driver rework Benjamin Gaignard
2017-09-14 7:56 ` [PATCH 1/2] clocksource: stm32: rework driver to use only one timer Benjamin Gaignard
@ 2017-09-14 7:56 ` Benjamin Gaignard
2017-09-15 21:03 ` Daniel Lezcano
1 sibling, 1 reply; 9+ messages in thread
From: Benjamin Gaignard @ 2017-09-14 7:56 UTC (permalink / raw)
To: robh+dt, mark.rutland, linux, mcoquelin.stm32, alexandre.torgue,
daniel.lezcano, tglx, ludovic.barre
Cc: devicetree, linux-arm-kernel, linux-kernel, Benjamin Gaignard
16 bits timers aren't accurate enough to be used as
clocksource, remove them from stm32f4 and stm32f7 devicetree.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
---
arch/arm/boot/dts/stm32f429.dtsi | 32 --------------------------------
arch/arm/boot/dts/stm32f746.dtsi | 32 --------------------------------
2 files changed, 64 deletions(-)
diff --git a/arch/arm/boot/dts/stm32f429.dtsi b/arch/arm/boot/dts/stm32f429.dtsi
index a8113dc..fd211cb 100644
--- a/arch/arm/boot/dts/stm32f429.dtsi
+++ b/arch/arm/boot/dts/stm32f429.dtsi
@@ -108,14 +108,6 @@
};
};
- timer3: timer@40000400 {
- compatible = "st,stm32-timer";
- reg = <0x40000400 0x400>;
- interrupts = <29>;
- clocks = <&rcc 0 STM32F4_APB1_CLOCK(TIM3)>;
- status = "disabled";
- };
-
timers3: timers@40000400 {
#address-cells = <1>;
#size-cells = <0>;
@@ -137,14 +129,6 @@
};
};
- timer4: timer@40000800 {
- compatible = "st,stm32-timer";
- reg = <0x40000800 0x400>;
- interrupts = <30>;
- clocks = <&rcc 0 STM32F4_APB1_CLOCK(TIM4)>;
- status = "disabled";
- };
-
timers4: timers@40000800 {
#address-cells = <1>;
#size-cells = <0>;
@@ -194,14 +178,6 @@
};
};
- timer6: timer@40001000 {
- compatible = "st,stm32-timer";
- reg = <0x40001000 0x400>;
- interrupts = <54>;
- clocks = <&rcc 0 STM32F4_APB1_CLOCK(TIM6)>;
- status = "disabled";
- };
-
timers6: timers@40001000 {
#address-cells = <1>;
#size-cells = <0>;
@@ -218,14 +194,6 @@
};
};
- timer7: timer@40001400 {
- compatible = "st,stm32-timer";
- reg = <0x40001400 0x400>;
- interrupts = <55>;
- clocks = <&rcc 0 STM32F4_APB1_CLOCK(TIM7)>;
- status = "disabled";
- };
-
timers7: timers@40001400 {
#address-cells = <1>;
#size-cells = <0>;
diff --git a/arch/arm/boot/dts/stm32f746.dtsi b/arch/arm/boot/dts/stm32f746.dtsi
index 4506eb9..c4d0273 100644
--- a/arch/arm/boot/dts/stm32f746.dtsi
+++ b/arch/arm/boot/dts/stm32f746.dtsi
@@ -82,22 +82,6 @@
status = "disabled";
};
- timer3: timer@40000400 {
- compatible = "st,stm32-timer";
- reg = <0x40000400 0x400>;
- interrupts = <29>;
- clocks = <&rcc 0 STM32F7_APB1_CLOCK(TIM3)>;
- status = "disabled";
- };
-
- timer4: timer@40000800 {
- compatible = "st,stm32-timer";
- reg = <0x40000800 0x400>;
- interrupts = <30>;
- clocks = <&rcc 0 STM32F7_APB1_CLOCK(TIM4)>;
- status = "disabled";
- };
-
timer5: timer@40000c00 {
compatible = "st,stm32-timer";
reg = <0x40000c00 0x400>;
@@ -105,22 +89,6 @@
clocks = <&rcc 0 STM32F7_APB1_CLOCK(TIM5)>;
};
- timer6: timer@40001000 {
- compatible = "st,stm32-timer";
- reg = <0x40001000 0x400>;
- interrupts = <54>;
- clocks = <&rcc 0 STM32F7_APB1_CLOCK(TIM6)>;
- status = "disabled";
- };
-
- timer7: timer@40001400 {
- compatible = "st,stm32-timer";
- reg = <0x40001400 0x400>;
- interrupts = <55>;
- clocks = <&rcc 0 STM32F7_APB1_CLOCK(TIM7)>;
- status = "disabled";
- };
-
rtc: rtc@40002800 {
compatible = "st,stm32-rtc";
reg = <0x40002800 0x400>;
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] clocksource: stm32: rework driver to use only one timer
2017-09-14 7:56 ` [PATCH 1/2] clocksource: stm32: rework driver to use only one timer Benjamin Gaignard
@ 2017-09-15 18:27 ` kbuild test robot
2017-09-18 21:30 ` Daniel Lezcano
1 sibling, 0 replies; 9+ messages in thread
From: kbuild test robot @ 2017-09-15 18:27 UTC (permalink / raw)
To: Benjamin Gaignard
Cc: kbuild-all, robh+dt, mark.rutland, linux, mcoquelin.stm32,
alexandre.torgue, daniel.lezcano, tglx, ludovic.barre,
devicetree, linux-arm-kernel, linux-kernel, Benjamin Gaignard
[-- Attachment #1: Type: text/plain, Size: 3136 bytes --]
Hi Benjamin,
[auto build test WARNING on tip/timers/core]
[also build test WARNING on next-20170915]
[cannot apply to robh/for-next v4.13]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Benjamin-Gaignard/stm32-clocksource-driver-rework/20170915-220617
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm
Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings
All warnings (new ones prefixed by >>):
drivers/clocksource/timer-stm32.c: In function 'stm32_timer_init':
>> drivers/clocksource/timer-stm32.c:180:11: warning: 'err' may be used uninitialized in this function [-Wmaybe-uninitialized]
int irq, err;
^~~
vim +/err +180 drivers/clocksource/timer-stm32.c
173
174 static int __init stm32_timer_init(struct device_node *node)
175 {
176 struct reset_control *rstc;
177 void __iomem *timer_base;
178 unsigned long max_arr;
179 struct clk *clk;
> 180 int irq, err;
181
182 timer_base = of_io_request_and_map(node, 0, of_node_full_name(node));
183 if (IS_ERR(timer_base)) {
184 pr_err("Can't map registers\n");
185 goto out;
186 }
187
188 irq = irq_of_parse_and_map(node, 0);
189 if (irq <= 0) {
190 pr_err("Can't parse IRQ\n");
191 goto out_unmap;
192 }
193
194 clk = of_clk_get(node, 0);
195 if (IS_ERR(clk)) {
196 pr_err("Can't get timer clock\n");
197 goto out_unmap;
198 }
199
200 rstc = of_reset_control_get(node, NULL);
201 if (!IS_ERR(rstc)) {
202 reset_control_assert(rstc);
203 reset_control_deassert(rstc);
204 }
205
206 err = clk_prepare_enable(clk);
207 if (err) {
208 pr_err("Couldn't enable parent clock\n");
209 goto out_clk;
210 }
211
212 /* Detect whether the timer is 16 or 32 bits */
213 writel_relaxed(~0U, timer_base + TIM_ARR);
214 max_arr = readl_relaxed(timer_base + TIM_ARR);
215 if (max_arr != ~0U) {
216 err = -EINVAL;
217 pr_err("32 bits timer is needed\n");
218 goto out_unprepare;
219 }
220
221 err = stm32_clocksource_init(node, timer_base, clk);
222 if (err)
223 goto out_unprepare;
224
225 err = stm32_clockevent_init(node, timer_base, clk, irq);
226 if (err)
227 goto out_unprepare;
228
229 return 0;
230
231 out_unprepare:
232 clk_disable_unprepare(clk);
233 out_clk:
234 clk_put(clk);
235 out_unmap:
236 iounmap(timer_base);
237 out:
238 return err;
239 }
240
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 62854 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] arm: dts: stm32: remove useless clocksource nodes
2017-09-14 7:56 ` [PATCH 2/2] arm: dts: stm32: remove useless clocksource nodes Benjamin Gaignard
@ 2017-09-15 21:03 ` Daniel Lezcano
2017-09-17 13:34 ` Benjamin Gaignard
0 siblings, 1 reply; 9+ messages in thread
From: Daniel Lezcano @ 2017-09-15 21:03 UTC (permalink / raw)
To: Benjamin Gaignard
Cc: robh+dt, mark.rutland, linux, mcoquelin.stm32, alexandre.torgue,
tglx, ludovic.barre, devicetree, linux-arm-kernel, linux-kernel
On Thu, Sep 14, 2017 at 09:56:52AM +0200, Benjamin Gaignard wrote:
> 16 bits timers aren't accurate enough to be used as
> clocksource, remove them from stm32f4 and stm32f7 devicetree.
Do you really want to remove the description? The timers are disabled, aren't they?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] arm: dts: stm32: remove useless clocksource nodes
2017-09-15 21:03 ` Daniel Lezcano
@ 2017-09-17 13:34 ` Benjamin Gaignard
0 siblings, 0 replies; 9+ messages in thread
From: Benjamin Gaignard @ 2017-09-17 13:34 UTC (permalink / raw)
To: Daniel Lezcano
Cc: Rob Herring, Mark Rutland, Russell King - ARM Linux,
Maxime Coquelin, Alexandre Torgue, Thomas Gleixner,
Ludovic Barre, devicetree, linux-arm-kernel,
Linux Kernel Mailing List
2017-09-15 23:03 GMT+02:00 Daniel Lezcano <daniel.lezcano@linaro.org>:
> On Thu, Sep 14, 2017 at 09:56:52AM +0200, Benjamin Gaignard wrote:
>> 16 bits timers aren't accurate enough to be used as
>> clocksource, remove them from stm32f4 and stm32f7 devicetree.
>
> Do you really want to remove the description? The timers are disabled, aren't they?
Yes because they are 16 bits timers and they won't be accepted anymore in
driver's probe function.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] clocksource: stm32: rework driver to use only one timer
2017-09-14 7:56 ` [PATCH 1/2] clocksource: stm32: rework driver to use only one timer Benjamin Gaignard
2017-09-15 18:27 ` kbuild test robot
@ 2017-09-18 21:30 ` Daniel Lezcano
2017-09-19 7:59 ` Benjamin Gaignard
1 sibling, 1 reply; 9+ messages in thread
From: Daniel Lezcano @ 2017-09-18 21:30 UTC (permalink / raw)
To: Benjamin Gaignard, robh+dt, mark.rutland, linux, mcoquelin.stm32,
alexandre.torgue, tglx, ludovic.barre
Cc: devicetree, linux-arm-kernel, linux-kernel
On 14/09/2017 09:56, Benjamin Gaignard wrote:
> Rework driver code to use only one timer for both clocksource
> and clockevent.
> This patch also forbids to use 16 bits timers because they are
> not enough accurate.
> Do some clean up in structures and functions names too.
>
> Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
Hi Benjamin,
I have a few comments below. Can you when reposting split this patch
into smaller changes ?
Also, can you consider using the timer-of API ?
Thanks.
-- Daniel
> ---
> drivers/clocksource/timer-stm32.c | 259 +++++++++++++++++++++++---------------
> 1 file changed, 155 insertions(+), 104 deletions(-)
>
> diff --git a/drivers/clocksource/timer-stm32.c b/drivers/clocksource/timer-stm32.c
> index 8f24237..648c10a 100644
> --- a/drivers/clocksource/timer-stm32.c
> +++ b/drivers/clocksource/timer-stm32.c
> @@ -16,175 +16,226 @@
> #include <linux/of_irq.h>
> #include <linux/clk.h>
> #include <linux/reset.h>
> +#include <linux/sched_clock.h>
> +#include <linux/slab.h>
>
> #define TIM_CR1 0x00
> #define TIM_DIER 0x0c
> #define TIM_SR 0x10
> #define TIM_EGR 0x14
> +#define TIM_CNT 0x24
> #define TIM_PSC 0x28
> #define TIM_ARR 0x2c
> +#define TIM_CCR1 0x34
>
> #define TIM_CR1_CEN BIT(0)
> -#define TIM_CR1_OPM BIT(3)
> +#define TIM_CR1_UDIS BIT(1)
> #define TIM_CR1_ARPE BIT(7)
>
> -#define TIM_DIER_UIE BIT(0)
> -
> -#define TIM_SR_UIF BIT(0)
> +#define TIM_DIER_CC1IE BIT(1)
>
> #define TIM_EGR_UG BIT(0)
>
> -struct stm32_clock_event_ddata {
> +struct stm32_clock_event {
> struct clock_event_device evtdev;
> unsigned periodic_top;
> - void __iomem *base;
> + void __iomem *regs;
> };
>
> static int stm32_clock_event_shutdown(struct clock_event_device *evtdev)
> {
> - struct stm32_clock_event_ddata *data =
> - container_of(evtdev, struct stm32_clock_event_ddata, evtdev);
> - void *base = data->base;
> + struct stm32_clock_event *ce =
> + container_of(evtdev, struct stm32_clock_event, evtdev);
> +
> + writel_relaxed(0, ce->regs + TIM_DIER);
>
> - writel_relaxed(0, base + TIM_CR1);
Why this change? TIM_CR1 -> TIM_DIER? A 16b to 32b change?
> return 0;
> }
>
> -static int stm32_clock_event_set_periodic(struct clock_event_device *evtdev)
> +static int stm32_clock_event_set_next_event(unsigned long evt,
> + struct clock_event_device *evtdev)
> {
> - struct stm32_clock_event_ddata *data =
> - container_of(evtdev, struct stm32_clock_event_ddata, evtdev);
> - void *base = data->base;
> + struct stm32_clock_event *ce =
> + container_of(evtdev, struct stm32_clock_event, evtdev);
> + unsigned long cnt;
> +
> + cnt = readl_relaxed(ce->regs + TIM_CNT);
> + writel_relaxed(cnt + evt, ce->regs + TIM_CCR1);
> + writel_relaxed(TIM_DIER_CC1IE, ce->regs + TIM_DIER);
>
> - writel_relaxed(data->periodic_top, base + TIM_ARR);
> - writel_relaxed(TIM_CR1_ARPE | TIM_CR1_CEN, base + TIM_CR1);
> return 0;
> }
>
> -static int stm32_clock_event_set_next_event(unsigned long evt,
> - struct clock_event_device *evtdev)
> +static int stm32_clock_event_set_periodic(struct clock_event_device *evtdev)
> {
> - struct stm32_clock_event_ddata *data =
> - container_of(evtdev, struct stm32_clock_event_ddata, evtdev);
> + struct stm32_clock_event *ce =
> + container_of(evtdev, struct stm32_clock_event, evtdev);
>
> - writel_relaxed(evt, data->base + TIM_ARR);
> - writel_relaxed(TIM_CR1_ARPE | TIM_CR1_OPM | TIM_CR1_CEN,
> - data->base + TIM_CR1);
> + return stm32_clock_event_set_next_event(ce->periodic_top, evtdev);
> +}
>
> - return 0;
> +static int stm32_clock_event_set_oneshot(struct clock_event_device *evtdev)
> +{
> + return stm32_clock_event_set_next_event(0, evtdev);
> }
>
> static irqreturn_t stm32_clock_event_handler(int irq, void *dev_id)
> {
> - struct stm32_clock_event_ddata *data = dev_id;
> + struct stm32_clock_event *ce = dev_id;
> +
> + writel_relaxed(0, ce->regs + TIM_SR);
>
> - writel_relaxed(0, data->base + TIM_SR);
> + if (clockevent_state_periodic(&ce->evtdev))
> + stm32_clock_event_set_periodic(&ce->evtdev);
nit: else condition to prevent an extra check
> - data->evtdev.event_handler(&data->evtdev);
> + if (clockevent_state_oneshot(&ce->evtdev))
> + stm32_clock_event_shutdown(&ce->evtdev);
> +
> + ce->evtdev.event_handler(&ce->evtdev);
>
> return IRQ_HANDLED;
> }
>
> -static struct stm32_clock_event_ddata clock_event_ddata = {
> - .evtdev = {
> - .name = "stm32 clockevent",
> - .features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_PERIODIC,
> - .set_state_shutdown = stm32_clock_event_shutdown,
> - .set_state_periodic = stm32_clock_event_set_periodic,
> - .set_state_oneshot = stm32_clock_event_shutdown,
> - .tick_resume = stm32_clock_event_shutdown,
> - .set_next_event = stm32_clock_event_set_next_event,
> - .rating = 200,
> - },
> -};
> +static int __init stm32_clockevent_init(struct device_node *np,
> + void __iomem *base,
> + struct clk *clk, int irq)
> +{
> + struct stm32_clock_event *ce;
> + unsigned long rate;
> + int err;
> +
> + ce = kzalloc(sizeof(*ce), GFP_KERNEL);
> + if (!ce)
> + return -ENOMEM;
> +
> + ce->regs = base;
> + ce->evtdev.name = "stm32_clockevent";
> + ce->evtdev.features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_PERIODIC;
> + ce->evtdev.set_state_shutdown = stm32_clock_event_shutdown;
> + ce->evtdev.set_state_periodic = stm32_clock_event_set_periodic;
> + ce->evtdev.set_state_oneshot = stm32_clock_event_set_oneshot;
> + ce->evtdev.tick_resume = stm32_clock_event_shutdown;
> + ce->evtdev.set_next_event = stm32_clock_event_set_next_event;
> + ce->evtdev.rating = 200;
>
> -static int __init stm32_clockevent_init(struct device_node *np)
> + rate = clk_get_rate(clk);
> + ce->periodic_top = DIV_ROUND_CLOSEST(rate, HZ);
> +
> + writel_relaxed(0, ce->regs + TIM_DIER);
> + writel_relaxed(0, ce->regs + TIM_SR);
> +
> + err = request_irq(irq, stm32_clock_event_handler, IRQF_TIMER,
> + "stm32 clockevent", ce);
> + if (err) {
> + kfree(ce);
> + return err;
> + }
> +
> + clockevents_config_and_register(&ce->evtdev, rate, 0x60, ~0U);
> +
> + return 0;
> +}
> +
> +static void __iomem *stm32_timer_cnt __read_mostly;
> +static u64 notrace stm32_read_sched_clock(void)
> +{
> + return readl_relaxed(stm32_timer_cnt);
> +}
> +
> +static int __init stm32_clocksource_init(struct device_node *node,
> + void __iomem *regs,
> + struct clk *clk)
> +{
> + unsigned long rate;
> +
> + rate = clk_get_rate(clk);
> +
> + writel_relaxed(~0U, regs + TIM_ARR);
> + writel_relaxed(0, regs + TIM_PSC);
> + writel_relaxed(0, regs + TIM_SR);
> + writel_relaxed(0, regs + TIM_DIER);
> + writel_relaxed(0, regs + TIM_SR);
> + writel_relaxed(TIM_CR1_ARPE | TIM_CR1_UDIS, regs + TIM_CR1);
> +
> + /* Make sure that registers are updated */
> + writel_relaxed(TIM_EGR_UG, regs + TIM_EGR);
> +
> + /* Enable controller */
> + writel_relaxed(TIM_CR1_ARPE | TIM_CR1_UDIS | TIM_CR1_CEN,
> + regs + TIM_CR1);
> +
> + stm32_timer_cnt = regs + TIM_CNT;
> + sched_clock_register(stm32_read_sched_clock, 32, rate);
> +
> + return clocksource_mmio_init(stm32_timer_cnt, "stm32_timer",
> + rate, 250, 32, clocksource_mmio_readl_up);
> +}
> +
> +static int __init stm32_timer_init(struct device_node *node)
> {
> - struct stm32_clock_event_ddata *data = &clock_event_ddata;
> - struct clk *clk;
> struct reset_control *rstc;
> - unsigned long rate, max_delta;
> - int irq, ret, bits, prescaler = 1;
> + void __iomem *timer_base;
> + unsigned long max_arr;
> + struct clk *clk;
> + int irq, err;
>
> - clk = of_clk_get(np, 0);
> - if (IS_ERR(clk)) {
> - ret = PTR_ERR(clk);
> - pr_err("failed to get clock for clockevent (%d)\n", ret);
> - goto err_clk_get;
> + timer_base = of_io_request_and_map(node, 0, of_node_full_name(node));
> + if (IS_ERR(timer_base)) {
> + pr_err("Can't map registers\n");
> + goto out;
> }
>
> - ret = clk_prepare_enable(clk);
> - if (ret) {
> - pr_err("failed to enable timer clock for clockevent (%d)\n",
> - ret);
> - goto err_clk_enable;
> + irq = irq_of_parse_and_map(node, 0);
> + if (irq <= 0) {
> + pr_err("Can't parse IRQ\n");
> + goto out_unmap;
> }
>
> - rate = clk_get_rate(clk);
Why not pass the rate to clkevt_init and clksrc_init instead of clk? So
clk_get_rate() is not called twice.
> + clk = of_clk_get(node, 0);
> + if (IS_ERR(clk)) {
> + pr_err("Can't get timer clock\n");
> + goto out_unmap;
> + }
>
> - rstc = of_reset_control_get(np, NULL);
> + rstc = of_reset_control_get(node, NULL);
> if (!IS_ERR(rstc)) {
> reset_control_assert(rstc);
> reset_control_deassert(rstc);
> }
>
> - data->base = of_iomap(np, 0);
> - if (!data->base) {
> - ret = -ENXIO;
> - pr_err("failed to map registers for clockevent\n");
> - goto err_iomap;
> - }
> -
> - irq = irq_of_parse_and_map(np, 0);
> - if (!irq) {
> - ret = -EINVAL;
> - pr_err("%pOF: failed to get irq.\n", np);
> - goto err_get_irq;
> + err = clk_prepare_enable(clk);
> + if (err) {
> + pr_err("Couldn't enable parent clock\n");
> + goto out_clk;
> }
>
> /* Detect whether the timer is 16 or 32 bits */
> - writel_relaxed(~0U, data->base + TIM_ARR);
> - max_delta = readl_relaxed(data->base + TIM_ARR);
> - if (max_delta == ~0U) {
> - prescaler = 1;
> - bits = 32;
> - } else {
> - prescaler = 1024;
> - bits = 16;
> + writel_relaxed(~0U, timer_base + TIM_ARR);
> + max_arr = readl_relaxed(timer_base + TIM_ARR);
> + if (max_arr != ~0U) {
> + err = -EINVAL;
> + pr_err("32 bits timer is needed\n");
> + goto out_unprepare;
> }
> - writel_relaxed(0, data->base + TIM_ARR);
> -
> - writel_relaxed(prescaler - 1, data->base + TIM_PSC);
> - writel_relaxed(TIM_EGR_UG, data->base + TIM_EGR);
> - writel_relaxed(TIM_DIER_UIE, data->base + TIM_DIER);
> - writel_relaxed(0, data->base + TIM_SR);
> -
> - data->periodic_top = DIV_ROUND_CLOSEST(rate, prescaler * HZ);
>
> - clockevents_config_and_register(&data->evtdev,
> - DIV_ROUND_CLOSEST(rate, prescaler),
> - 0x1, max_delta);
> + err = stm32_clocksource_init(node, timer_base, clk);
> + if (err)
> + goto out_unprepare;
>
> - ret = request_irq(irq, stm32_clock_event_handler, IRQF_TIMER,
> - "stm32 clockevent", data);
> - if (ret) {
> - pr_err("%pOF: failed to request irq.\n", np);
> - goto err_get_irq;
> - }
> -
> - pr_info("%pOF: STM32 clockevent driver initialized (%d bits)\n",
> - np, bits);
> + err = stm32_clockevent_init(node, timer_base, clk, irq);
> + if (err)
> + goto out_unprepare;
>
> - return ret;
> + return 0;
>
> -err_get_irq:
> - iounmap(data->base);
> -err_iomap:
> +out_unprepare:
> clk_disable_unprepare(clk);
> -err_clk_enable:
> +out_clk:
> clk_put(clk);
> -err_clk_get:
> - return ret;
> +out_unmap:
> + iounmap(timer_base);
> +out:
> + return err;
> }
>
> -TIMER_OF_DECLARE(stm32, "st,stm32-timer", stm32_clockevent_init);
> +CLOCKSOURCE_OF_DECLARE(stm32, "st,stm32-timer", stm32_timer_init);
CLOCKSOURCE_OF_DECLARE is deprecated, keep using TIMER_OF_DECLARE.
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] clocksource: stm32: rework driver to use only one timer
2017-09-18 21:30 ` Daniel Lezcano
@ 2017-09-19 7:59 ` Benjamin Gaignard
2017-09-19 15:54 ` Daniel Lezcano
0 siblings, 1 reply; 9+ messages in thread
From: Benjamin Gaignard @ 2017-09-19 7:59 UTC (permalink / raw)
To: Daniel Lezcano
Cc: Rob Herring, Mark Rutland, Russell King - ARM Linux,
Maxime Coquelin, Alexandre Torgue, Thomas Gleixner,
Ludovic Barre, devicetree, linux-arm-kernel,
Linux Kernel Mailing List
2017-09-18 23:30 GMT+02:00 Daniel Lezcano <daniel.lezcano@linaro.org>:
> On 14/09/2017 09:56, Benjamin Gaignard wrote:
>> Rework driver code to use only one timer for both clocksource
>> and clockevent.
>> This patch also forbids to use 16 bits timers because they are
>> not enough accurate.
>> Do some clean up in structures and functions names too.
>>
>> Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
>
> Hi Benjamin,
>
> I have a few comments below. Can you when reposting split this patch
> into smaller changes ?
Not so easy because I change the way of how the hardware is used to be able
to provide clocksource and clockevent on the same hardware block.
>
> Also, can you consider using the timer-of API ?
Is it just about using TIMER_OF_DECLARE ? or do you have something
else in mind ?
>
> Thanks.
>
> -- Daniel
>
>> ---
>> drivers/clocksource/timer-stm32.c | 259 +++++++++++++++++++++++---------------
>> 1 file changed, 155 insertions(+), 104 deletions(-)
>>
>> diff --git a/drivers/clocksource/timer-stm32.c b/drivers/clocksource/timer-stm32.c
>> index 8f24237..648c10a 100644
>> --- a/drivers/clocksource/timer-stm32.c
>> +++ b/drivers/clocksource/timer-stm32.c
>> @@ -16,175 +16,226 @@
>> #include <linux/of_irq.h>
>> #include <linux/clk.h>
>> #include <linux/reset.h>
>> +#include <linux/sched_clock.h>
>> +#include <linux/slab.h>
>>
>> #define TIM_CR1 0x00
>> #define TIM_DIER 0x0c
>> #define TIM_SR 0x10
>> #define TIM_EGR 0x14
>> +#define TIM_CNT 0x24
>> #define TIM_PSC 0x28
>> #define TIM_ARR 0x2c
>> +#define TIM_CCR1 0x34
>>
>> #define TIM_CR1_CEN BIT(0)
>> -#define TIM_CR1_OPM BIT(3)
>> +#define TIM_CR1_UDIS BIT(1)
>> #define TIM_CR1_ARPE BIT(7)
>>
>> -#define TIM_DIER_UIE BIT(0)
>> -
>> -#define TIM_SR_UIF BIT(0)
>> +#define TIM_DIER_CC1IE BIT(1)
>>
>> #define TIM_EGR_UG BIT(0)
>>
>> -struct stm32_clock_event_ddata {
>> +struct stm32_clock_event {
>> struct clock_event_device evtdev;
>> unsigned periodic_top;
>> - void __iomem *base;
>> + void __iomem *regs;
>> };
>>
>> static int stm32_clock_event_shutdown(struct clock_event_device *evtdev)
>> {
>> - struct stm32_clock_event_ddata *data =
>> - container_of(evtdev, struct stm32_clock_event_ddata, evtdev);
>> - void *base = data->base;
>> + struct stm32_clock_event *ce =
>> + container_of(evtdev, struct stm32_clock_event, evtdev);
>> +
>> + writel_relaxed(0, ce->regs + TIM_DIER);
>>
>> - writel_relaxed(0, base + TIM_CR1);
>
> Why this change? TIM_CR1 -> TIM_DIER? A 16b to 32b change?
No it is because I use the interrupt from the comparator instead of the counter.
With this patch clocksource will use the 32 bits counter of the hardware block
to provide the clock and comparator is generate interrupt for the event.
That change quite a lot the code, sorry.
>
>> return 0;
>> }
>>
>> -static int stm32_clock_event_set_periodic(struct clock_event_device *evtdev)
>> +static int stm32_clock_event_set_next_event(unsigned long evt,
>> + struct clock_event_device *evtdev)
>> {
>> - struct stm32_clock_event_ddata *data =
>> - container_of(evtdev, struct stm32_clock_event_ddata, evtdev);
>> - void *base = data->base;
>> + struct stm32_clock_event *ce =
>> + container_of(evtdev, struct stm32_clock_event, evtdev);
>> + unsigned long cnt;
>> +
>> + cnt = readl_relaxed(ce->regs + TIM_CNT);
>> + writel_relaxed(cnt + evt, ce->regs + TIM_CCR1);
>> + writel_relaxed(TIM_DIER_CC1IE, ce->regs + TIM_DIER);
>>
>> - writel_relaxed(data->periodic_top, base + TIM_ARR);
>> - writel_relaxed(TIM_CR1_ARPE | TIM_CR1_CEN, base + TIM_CR1);
>> return 0;
>> }
>>
>> -static int stm32_clock_event_set_next_event(unsigned long evt,
>> - struct clock_event_device *evtdev)
>> +static int stm32_clock_event_set_periodic(struct clock_event_device *evtdev)
>> {
>> - struct stm32_clock_event_ddata *data =
>> - container_of(evtdev, struct stm32_clock_event_ddata, evtdev);
>> + struct stm32_clock_event *ce =
>> + container_of(evtdev, struct stm32_clock_event, evtdev);
>>
>> - writel_relaxed(evt, data->base + TIM_ARR);
>> - writel_relaxed(TIM_CR1_ARPE | TIM_CR1_OPM | TIM_CR1_CEN,
>> - data->base + TIM_CR1);
>> + return stm32_clock_event_set_next_event(ce->periodic_top, evtdev);
>> +}
>>
>> - return 0;
>> +static int stm32_clock_event_set_oneshot(struct clock_event_device *evtdev)
>> +{
>> + return stm32_clock_event_set_next_event(0, evtdev);
>> }
>>
>> static irqreturn_t stm32_clock_event_handler(int irq, void *dev_id)
>> {
>> - struct stm32_clock_event_ddata *data = dev_id;
>> + struct stm32_clock_event *ce = dev_id;
>> +
>> + writel_relaxed(0, ce->regs + TIM_SR);
>>
>> - writel_relaxed(0, data->base + TIM_SR);
>> + if (clockevent_state_periodic(&ce->evtdev))
>> + stm32_clock_event_set_periodic(&ce->evtdev);
>
> nit: else condition to prevent an extra check
OK
>
>> - data->evtdev.event_handler(&data->evtdev);
>> + if (clockevent_state_oneshot(&ce->evtdev))
>> + stm32_clock_event_shutdown(&ce->evtdev);
>> +
>> + ce->evtdev.event_handler(&ce->evtdev);
>>
>> return IRQ_HANDLED;
>> }
>>
>> -static struct stm32_clock_event_ddata clock_event_ddata = {
>> - .evtdev = {
>> - .name = "stm32 clockevent",
>> - .features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_PERIODIC,
>> - .set_state_shutdown = stm32_clock_event_shutdown,
>> - .set_state_periodic = stm32_clock_event_set_periodic,
>> - .set_state_oneshot = stm32_clock_event_shutdown,
>> - .tick_resume = stm32_clock_event_shutdown,
>> - .set_next_event = stm32_clock_event_set_next_event,
>> - .rating = 200,
>> - },
>> -};
>> +static int __init stm32_clockevent_init(struct device_node *np,
>> + void __iomem *base,
>> + struct clk *clk, int irq)
>> +{
>> + struct stm32_clock_event *ce;
>> + unsigned long rate;
>> + int err;
>> +
>> + ce = kzalloc(sizeof(*ce), GFP_KERNEL);
>> + if (!ce)
>> + return -ENOMEM;
>> +
>> + ce->regs = base;
>> + ce->evtdev.name = "stm32_clockevent";
>> + ce->evtdev.features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_PERIODIC;
>> + ce->evtdev.set_state_shutdown = stm32_clock_event_shutdown;
>> + ce->evtdev.set_state_periodic = stm32_clock_event_set_periodic;
>> + ce->evtdev.set_state_oneshot = stm32_clock_event_set_oneshot;
>> + ce->evtdev.tick_resume = stm32_clock_event_shutdown;
>> + ce->evtdev.set_next_event = stm32_clock_event_set_next_event;
>> + ce->evtdev.rating = 200;
>>
>> -static int __init stm32_clockevent_init(struct device_node *np)
>> + rate = clk_get_rate(clk);
>> + ce->periodic_top = DIV_ROUND_CLOSEST(rate, HZ);
>> +
>> + writel_relaxed(0, ce->regs + TIM_DIER);
>> + writel_relaxed(0, ce->regs + TIM_SR);
>> +
>> + err = request_irq(irq, stm32_clock_event_handler, IRQF_TIMER,
>> + "stm32 clockevent", ce);
>> + if (err) {
>> + kfree(ce);
>> + return err;
>> + }
>> +
>> + clockevents_config_and_register(&ce->evtdev, rate, 0x60, ~0U);
>> +
>> + return 0;
>> +}
>> +
>> +static void __iomem *stm32_timer_cnt __read_mostly;
>> +static u64 notrace stm32_read_sched_clock(void)
>> +{
>> + return readl_relaxed(stm32_timer_cnt);
>> +}
>> +
>> +static int __init stm32_clocksource_init(struct device_node *node,
>> + void __iomem *regs,
>> + struct clk *clk)
>> +{
>> + unsigned long rate;
>> +
>> + rate = clk_get_rate(clk);
>> +
>> + writel_relaxed(~0U, regs + TIM_ARR);
>> + writel_relaxed(0, regs + TIM_PSC);
>> + writel_relaxed(0, regs + TIM_SR);
>> + writel_relaxed(0, regs + TIM_DIER);
>> + writel_relaxed(0, regs + TIM_SR);
>> + writel_relaxed(TIM_CR1_ARPE | TIM_CR1_UDIS, regs + TIM_CR1);
>> +
>> + /* Make sure that registers are updated */
>> + writel_relaxed(TIM_EGR_UG, regs + TIM_EGR);
>> +
>> + /* Enable controller */
>> + writel_relaxed(TIM_CR1_ARPE | TIM_CR1_UDIS | TIM_CR1_CEN,
>> + regs + TIM_CR1);
>> +
>> + stm32_timer_cnt = regs + TIM_CNT;
>> + sched_clock_register(stm32_read_sched_clock, 32, rate);
>> +
>> + return clocksource_mmio_init(stm32_timer_cnt, "stm32_timer",
>> + rate, 250, 32, clocksource_mmio_readl_up);
>> +}
>> +
>> +static int __init stm32_timer_init(struct device_node *node)
>> {
>> - struct stm32_clock_event_ddata *data = &clock_event_ddata;
>> - struct clk *clk;
>> struct reset_control *rstc;
>> - unsigned long rate, max_delta;
>> - int irq, ret, bits, prescaler = 1;
>> + void __iomem *timer_base;
>> + unsigned long max_arr;
>> + struct clk *clk;
>> + int irq, err;
>>
>> - clk = of_clk_get(np, 0);
>> - if (IS_ERR(clk)) {
>> - ret = PTR_ERR(clk);
>> - pr_err("failed to get clock for clockevent (%d)\n", ret);
>> - goto err_clk_get;
>> + timer_base = of_io_request_and_map(node, 0, of_node_full_name(node));
>> + if (IS_ERR(timer_base)) {
>> + pr_err("Can't map registers\n");
>> + goto out;
>> }
>>
>> - ret = clk_prepare_enable(clk);
>> - if (ret) {
>> - pr_err("failed to enable timer clock for clockevent (%d)\n",
>> - ret);
>> - goto err_clk_enable;
>> + irq = irq_of_parse_and_map(node, 0);
>> + if (irq <= 0) {
>> + pr_err("Can't parse IRQ\n");
>> + goto out_unmap;
>> }
>>
>> - rate = clk_get_rate(clk);
>
> Why not pass the rate to clkevt_init and clksrc_init instead of clk? So
> clk_get_rate() is not called twice.
I will change that in v3
>
>> + clk = of_clk_get(node, 0);
>> + if (IS_ERR(clk)) {
>> + pr_err("Can't get timer clock\n");
>> + goto out_unmap;
>> + }
>>
>> - rstc = of_reset_control_get(np, NULL);
>> + rstc = of_reset_control_get(node, NULL);
>> if (!IS_ERR(rstc)) {
>> reset_control_assert(rstc);
>> reset_control_deassert(rstc);
>> }
>>
>> - data->base = of_iomap(np, 0);
>> - if (!data->base) {
>> - ret = -ENXIO;
>> - pr_err("failed to map registers for clockevent\n");
>> - goto err_iomap;
>> - }
>> -
>> - irq = irq_of_parse_and_map(np, 0);
>> - if (!irq) {
>> - ret = -EINVAL;
>> - pr_err("%pOF: failed to get irq.\n", np);
>> - goto err_get_irq;
>> + err = clk_prepare_enable(clk);
>> + if (err) {
>> + pr_err("Couldn't enable parent clock\n");
>> + goto out_clk;
>> }
>>
>> /* Detect whether the timer is 16 or 32 bits */
>> - writel_relaxed(~0U, data->base + TIM_ARR);
>> - max_delta = readl_relaxed(data->base + TIM_ARR);
>> - if (max_delta == ~0U) {
>> - prescaler = 1;
>> - bits = 32;
>> - } else {
>> - prescaler = 1024;
>> - bits = 16;
>> + writel_relaxed(~0U, timer_base + TIM_ARR);
>> + max_arr = readl_relaxed(timer_base + TIM_ARR);
>> + if (max_arr != ~0U) {
>> + err = -EINVAL;
>> + pr_err("32 bits timer is needed\n");
>> + goto out_unprepare;
>> }
>> - writel_relaxed(0, data->base + TIM_ARR);
>> -
>> - writel_relaxed(prescaler - 1, data->base + TIM_PSC);
>> - writel_relaxed(TIM_EGR_UG, data->base + TIM_EGR);
>> - writel_relaxed(TIM_DIER_UIE, data->base + TIM_DIER);
>> - writel_relaxed(0, data->base + TIM_SR);
>> -
>> - data->periodic_top = DIV_ROUND_CLOSEST(rate, prescaler * HZ);
>>
>> - clockevents_config_and_register(&data->evtdev,
>> - DIV_ROUND_CLOSEST(rate, prescaler),
>> - 0x1, max_delta);
>> + err = stm32_clocksource_init(node, timer_base, clk);
>> + if (err)
>> + goto out_unprepare;
>>
>> - ret = request_irq(irq, stm32_clock_event_handler, IRQF_TIMER,
>> - "stm32 clockevent", data);
>> - if (ret) {
>> - pr_err("%pOF: failed to request irq.\n", np);
>> - goto err_get_irq;
>> - }
>> -
>> - pr_info("%pOF: STM32 clockevent driver initialized (%d bits)\n",
>> - np, bits);
>> + err = stm32_clockevent_init(node, timer_base, clk, irq);
>> + if (err)
>> + goto out_unprepare;
>>
>> - return ret;
>> + return 0;
>>
>> -err_get_irq:
>> - iounmap(data->base);
>> -err_iomap:
>> +out_unprepare:
>> clk_disable_unprepare(clk);
>> -err_clk_enable:
>> +out_clk:
>> clk_put(clk);
>> -err_clk_get:
>> - return ret;
>> +out_unmap:
>> + iounmap(timer_base);
>> +out:
>> + return err;
>> }
>>
>> -TIMER_OF_DECLARE(stm32, "st,stm32-timer", stm32_clockevent_init);
>> +CLOCKSOURCE_OF_DECLARE(stm32, "st,stm32-timer", stm32_timer_init);
>
> CLOCKSOURCE_OF_DECLARE is deprecated, keep using TIMER_OF_DECLARE.
OK
>
>
> --
> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] clocksource: stm32: rework driver to use only one timer
2017-09-19 7:59 ` Benjamin Gaignard
@ 2017-09-19 15:54 ` Daniel Lezcano
0 siblings, 0 replies; 9+ messages in thread
From: Daniel Lezcano @ 2017-09-19 15:54 UTC (permalink / raw)
To: Benjamin Gaignard
Cc: Rob Herring, Mark Rutland, Russell King - ARM Linux,
Maxime Coquelin, Alexandre Torgue, Thomas Gleixner,
Ludovic Barre, devicetree, linux-arm-kernel,
Linux Kernel Mailing List
On 19/09/2017 09:59, Benjamin Gaignard wrote:
> 2017-09-18 23:30 GMT+02:00 Daniel Lezcano <daniel.lezcano@linaro.org>:
>> On 14/09/2017 09:56, Benjamin Gaignard wrote:
>>> Rework driver code to use only one timer for both clocksource
>>> and clockevent.
>>> This patch also forbids to use 16 bits timers because they are
>>> not enough accurate.
>>> Do some clean up in structures and functions names too.
>>>
>>> Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
>>
>> Hi Benjamin,
>>
>> I have a few comments below. Can you when reposting split this patch
>> into smaller changes ?
>
> Not so easy because I change the way of how the hardware is used to be able
> to provide clocksource and clockevent on the same hardware block.
>
>>
>> Also, can you consider using the timer-of API ?
>
> Is it just about using TIMER_OF_DECLARE ? or do you have something
> else in mind ?
It is something else, see commit dc11bae785.
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-09-19 15:54 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-14 7:56 [PATCH 0/2] stm32 clocksource driver rework Benjamin Gaignard
2017-09-14 7:56 ` [PATCH 1/2] clocksource: stm32: rework driver to use only one timer Benjamin Gaignard
2017-09-15 18:27 ` kbuild test robot
2017-09-18 21:30 ` Daniel Lezcano
2017-09-19 7:59 ` Benjamin Gaignard
2017-09-19 15:54 ` Daniel Lezcano
2017-09-14 7:56 ` [PATCH 2/2] arm: dts: stm32: remove useless clocksource nodes Benjamin Gaignard
2017-09-15 21:03 ` Daniel Lezcano
2017-09-17 13:34 ` Benjamin Gaignard
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).