Re: [PATCH v2 4/9] arm: twr-k70f120m: timer driver for Kinetis SoC

From: Paul Osmialowski
Date: Wed Jul 01 2015 - 07:48:30 EST

Hi Arnd,

Again, thanks for your remarks. I'm attaching timer patch candidate for the third iteration.

Following your advices, I've changed following things:

- not abusing aliases (same goes to pinctrl driver)
- ranges for addressing particular timers (no change in code though, it's
just up to the .dts implementor)
- *_RD and *_WR macros removed; whole this big-endian issue was a mistake,
I guess I overdid it a bit trying to make my drivers as universal as
fsl-edma driver...
- *_SET and *_RESET macros removed - they were giving false sense of
security hiding potential race.

Any comments are welcome.

On Tue, 30 Jun 2015, Arnd Bergmann wrote:

On Tuesday 30 June 2015 14:27:25 Paul Osmialowski wrote:

+Example:
+
+aliases {
+ pit0 = &pit0;
+ pit1 = &pit1;
+ pit2 = &pit2;
+ pit3 = &pit3;
+};
+
+pit@40037000 {
+ compatible = "fsl,kinetis-pit-timer";
+ reg = <0x40037000 0x100>;
+ clocks = <&mcg_pclk_gate 5 23>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;

All the subnodes seem to fall inside of the device's own register
area, so I think it would be nicer to use a specific 'ranges'
property that only translates the registers in question.

/ {
+ aliases {
+ pit0 = &pit0;
+ pit1 = &pit1;
+ pit2 = &pit2;
+ pit3 = &pit3;
+ };
+
soc {
+ pit@40037000 {
+ compatible = "fsl,kinetis-pit-timer";
+ reg = <0x40037000 0x100>;
+ clocks = <&mcg_pclk_gate 5 23>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;
+
+ pit0: timer@40037100 {
+ reg = <0x40037100 0x10>;
+ interrupts = <68>;
+ status = "disabled";
+ };

I don't think it's necessary to have both an alias
and a label here. What do you use the alias for?

+
+#define KINETIS_PITMCR_PTR(base, reg) \
+ (&(((struct kinetis_pit_mcr_regs *)(base))->reg))
+#define KINETIS_PITMCR_RD(be, base, reg) \
+ ((be) ? ioread32be(KINETIS_PITMCR_PTR(base, reg)) \
+ : ioread32(KINETIS_PITMCR_PTR(base, reg)))
+#define KINETIS_PITMCR_WR(be, base, reg, val) do { \
+ if (be) \
+ iowrite32be((val), KINETIS_PITMCR_PTR(base, reg)); \
+ else \
+ iowrite32((val), KINETIS_PITMCR_PTR(base, reg)); \
+ } while (0)

These should really be written as inline functions. Can you
explain why you need to deal with a big-endian version of this
hardware? Can you configure the endianess of this register block
and just set it to one of the two at boot time?

+#define KINETIS_PIT_PTR(base, reg) \
+ (&(((struct kinetis_pit_channel_regs *)(base))->reg))
+#define KINETIS_PIT_RD(be, base, reg) \
+ ((be) ? ioread32be(KINETIS_PIT_PTR(base, reg)) \
+ : ioread32(KINETIS_PIT_PTR(base, reg)))
+#define KINETIS_PIT_WR(be, base, reg, val) do { \
+ if (be) \
+ iowrite32be((val), KINETIS_PIT_PTR(base, reg)); \
+ else \
+ iowrite32((val), KINETIS_PIT_PTR(base, reg)); \
+ } while (0)
+#define KINETIS_PIT_SET(be, base, reg, mask) \
+ KINETIS_PIT_WR(be, base, reg, \
+ KINETIS_PIT_RD(be, base, reg) | (mask))
+#define KINETIS_PIT_RESET(be, base, reg, mask) \
+ KINETIS_PIT_WR(be, base, reg, \
+ KINETIS_PIT_RD(be, base, reg) & (~(mask)))


Functions again. Also, just pass a pointer to your own data structure
into the function, instead of the 'be' and 'base' values.

The 'set' and 'reset' functions look like they need a spinlock
to avoid races.

Arnd
From 562decc41dc7302c378bf6e4509a22561ff5a85e Mon Sep 17 00:00:00 2001
From: Paul Osmialowski <pawelo@xxxxxxxxxxx>
Date: Mon, 29 Jun 2015 21:32:41 +0200
Subject: [PATCH 4/9] arm: twr-k70f120m: timer driver for Kinetis SoC

Based on legacy pre-OF code by Alexander Potashev <aspotashev@xxxxxxxxxxx>

Signed-off-by: Paul Osmialowski <pawelo@xxxxxxxxxxx>
---
.../bindings/timer/fsl,kinetis-pit-timer.txt | 43 ++++
arch/arm/Kconfig | 1 +
arch/arm/boot/dts/kinetis-twr-k70f120m.dts | 4 +
arch/arm/boot/dts/kinetis.dtsi | 33 +++
drivers/clocksource/Kconfig | 5 +
drivers/clocksource/Makefile | 1 +
drivers/clocksource/timer-kinetis.c | 280 +++++++++++++++++++++
7 files changed, 367 insertions(+)
create mode 100644 Documentation/devicetree/bindings/timer/fsl,kinetis-pit-timer.txt
create mode 100644 drivers/clocksource/timer-kinetis.c

diff --git a/Documentation/devicetree/bindings/timer/fsl,kinetis-pit-timer.txt b/Documentation/devicetree/bindings/timer/fsl,kinetis-pit-timer.txt
new file mode 100644
index 0000000..f2930af
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/fsl,kinetis-pit-timer.txt
@@ -0,0 +1,43 @@
+Freescale Kinetis SoC Periodic Interrupt Timer (PIT)
+
+Required properties:
+
+- compatible: Should be "fsl,kinetis-pit-timer".
+- reg: Specifies base physical address and size of the register set for the
+ Periodic Interrupt Timer.
+- clocks: The clock provided by the SoC to drive the timer.
+- Set of timer devices: following properties are required for each:
+ - reg: Specifies base physical address and size of the register set
+ for given timer device.
+ - interrupts: Should be the clock event device interrupt.
+
+Example:
+
+pit@40037000 {
+ compatible = "fsl,kinetis-pit-timer";
+ reg = <0x40037000 0x200>;
+ clocks = <&mcg_pclk_gate 5 23>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0x0 0x40037100 0x100>;
+
+ pit0: timer@00 {
+ reg = <0x00 0x10>;
+ interrupts = <68>;
+ };
+
+ pit1: timer@10 {
+ reg = <0x10 0x10>;
+ interrupts = <69>;
+ };
+
+ pit2: timer@20 {
+ reg = <0x20 0x10>;
+ interrupts = <70>;
+ };
+
+ pit3: timer@30 {
+ reg = <0x30 0x10>;
+ interrupts = <71>;
+ };
+};
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 9c89bdc..96ddaae 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -968,6 +968,7 @@ config ARCH_KINETIS
bool "Freescale Kinetis MCU"
depends on ARM_SINGLE_ARMV7M
select ARMV7M_SYSTICK
+ select CLKSRC_KINETIS
help
This enables support for the Freescale Kinetis MCUs

diff --git a/arch/arm/boot/dts/kinetis-twr-k70f120m.dts b/arch/arm/boot/dts/kinetis-twr-k70f120m.dts
index edccf37..a6efc29 100644
--- a/arch/arm/boot/dts/kinetis-twr-k70f120m.dts
+++ b/arch/arm/boot/dts/kinetis-twr-k70f120m.dts
@@ -14,3 +14,7 @@
reg = <0x8000000 0x8000000>;
};
};
+
+&pit0 {
+ status = "ok";
+};
diff --git a/arch/arm/boot/dts/kinetis.dtsi b/arch/arm/boot/dts/kinetis.dtsi
index ae0cf00..74b58cb 100644
--- a/arch/arm/boot/dts/kinetis.dtsi
+++ b/arch/arm/boot/dts/kinetis.dtsi
@@ -6,6 +6,39 @@

/ {
soc {
+ pit@40037000 {
+ compatible = "fsl,kinetis-pit-timer";
+ reg = <0x40037000 0x200>;
+ clocks = <&mcg_pclk_gate 5 23>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0x0 0x40037100 0x100>;
+
+ pit0: timer@00 {
+ reg = <0x00 0x10>;
+ interrupts = <68>;
+ status = "disabled";
+ };
+
+ pit1: timer@10 {
+ reg = <0x10 0x10>;
+ interrupts = <69>;
+ status = "disabled";
+ };
+
+ pit2: timer@20 {
+ reg = <0x20 0x10>;
+ interrupts = <70>;
+ status = "disabled";
+ };
+
+ pit3: timer@30 {
+ reg = <0x30 0x10>;
+ interrupts = <71>;
+ status = "disabled";
+ };
+ };
+
cmu@40064000 {
compatible = "fsl,kinetis-cmu";
reg = <0x40064000 0x14>, <0x40047000 0x1100>;
diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 0f1c77e..d377395 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -106,6 +106,11 @@ config CLKSRC_EFM32
Support to use the timers of EFM32 SoCs as clock source and clock
event device.

+config CLKSRC_KINETIS
+ bool "Clocksource for Kinetis SoCs"
+ depends on OF && ARCH_KINETIS
+ select CLKSRC_OF
+
config CLKSRC_LPC32XX
bool
select CLKSRC_MMIO
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index f1ae0e7..6da77a8 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -36,6 +36,7 @@ obj-$(CONFIG_ARCH_NSPIRE) += zevio-timer.o
obj-$(CONFIG_ARCH_BCM_MOBILE) += bcm_kona_timer.o
obj-$(CONFIG_CADENCE_TTC_TIMER) += cadence_ttc_timer.o
obj-$(CONFIG_CLKSRC_EFM32) += time-efm32.o
+obj-$(CONFIG_CLKSRC_KINETIS) += timer-kinetis.o
obj-$(CONFIG_CLKSRC_STM32) += timer-stm32.o
obj-$(CONFIG_CLKSRC_EXYNOS_MCT) += exynos_mct.o
obj-$(CONFIG_CLKSRC_LPC32XX) += time-lpc32xx.o
diff --git a/drivers/clocksource/timer-kinetis.c b/drivers/clocksource/timer-kinetis.c
new file mode 100644
index 0000000..1424308
--- /dev/null
+++ b/drivers/clocksource/timer-kinetis.c
@@ -0,0 +1,280 @@
+/*
+ * timer-kinetis.c - Timer driver for Kinetis K70
+ *
+ * Based on legacy pre-OF code by Alexander Potashev <aspotashev@xxxxxxxxxxx>
+ *
+ * Copyright (C) 2015 Paul Osmialowski <pawelo@xxxxxxxxxxx>
+ *
+ * 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.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/kernel.h>
+#include <linux/io.h>
+#include <linux/clocksource.h>
+#include <linux/clockchips.h>
+#include <linux/irq.h>
+#include <linux/interrupt.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>
+#include <linux/clk.h>
+#include <linux/sched.h>
+#include <linux/slab.h>
+
+/*
+ * PIT Timer Control Register
+ */
+/* Timer Interrupt Enable Bit */
+#define KINETIS_PIT_TCTRL_TIE_MSK (1 << 1)
+/* Timer Enable Bit */
+#define KINETIS_PIT_TCTRL_TEN_MSK (1 << 0)
+/*
+ * PIT Timer Flag Register
+ */
+/* Timer Interrupt Flag */
+#define KINETIS_PIT_TFLG_TIF_MSK (1 << 0)
+
+struct kinetis_pit_mcr_regs {
+ u32 mcr;
+};
+#define KINETIS_PITMCR_PTR(base, reg) \
+ (&(((struct kinetis_pit_mcr_regs *)(base))->reg))
+
+/*
+ * Periodic Interrupt Timer (PIT) registers
+ */
+struct kinetis_pit_channel_regs {
+ u32 ldval; /* Timer Load Value Register */
+ u32 cval; /* Current Timer Value Register */
+ u32 tctrl; /* Timer Control Register */
+ u32 tflg; /* Timer Flag Register */
+};
+#define KINETIS_PIT_PTR(base, reg) \
+ (&(((struct kinetis_pit_channel_regs *)(base))->reg))
+
+struct kinetis_clock_event_ddata {
+ struct clock_event_device evtdev;
+ void __iomem *base;
+ void __iomem *mcr;
+ spinlock_t lock;
+};
+
+/*
+ * Enable or disable a PIT channel
+ */
+static void kinetis_pit_enable(struct kinetis_clock_event_ddata *tmr,
+ int enable)
+{
+ u32 tctrl_val;
+ unsigned long flags;
+
+ spin_lock_irqsave(&tmr->lock, flags);
+
+ tctrl_val = ioread32(KINETIS_PIT_PTR(tmr->base, tctrl));
+ if (enable)
+ iowrite32(tctrl_val | KINETIS_PIT_TCTRL_TEN_MSK,
+ KINETIS_PIT_PTR(tmr->base, tctrl));
+ else
+ iowrite32(tctrl_val & ~KINETIS_PIT_TCTRL_TEN_MSK,
+ KINETIS_PIT_PTR(tmr->base, tctrl));
+
+ spin_unlock_irqrestore(&tmr->lock, flags);
+}
+
+/*
+ * Initialize a PIT channel, but do not enable it
+ */
+static void kinetis_pit_init(struct kinetis_clock_event_ddata *tmr, u32 ticks)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&tmr->lock, flags);
+
+ /*
+ * Enable the PIT module clock
+ */
+ iowrite32(0, KINETIS_PITMCR_PTR(tmr->mcr, mcr));
+
+ iowrite32(0, KINETIS_PIT_PTR(tmr->base, tctrl));
+ iowrite32(KINETIS_PIT_TFLG_TIF_MSK, KINETIS_PIT_PTR(tmr->base, tflg));
+ iowrite32(ticks, KINETIS_PIT_PTR(tmr->base, ldval));
+ iowrite32(0, KINETIS_PIT_PTR(tmr->base, cval));
+ iowrite32(KINETIS_PIT_TCTRL_TIE_MSK, KINETIS_PIT_PTR(tmr->base, tctrl));
+
+ spin_unlock_irqrestore(&tmr->lock, flags);
+}
+
+static int kinetis_clockevent_tmr_set_state_periodic(
+ struct clock_event_device *evt)
+{
+ struct kinetis_clock_event_ddata *pit =
+ container_of(evt, struct kinetis_clock_event_ddata, evtdev);
+
+ kinetis_pit_enable(pit, 1);
+
+ return 0;
+}
+
+static int kinetis_clockevent_tmr_set_state_oneshot(
+ struct clock_event_device *evt)
+{
+ struct kinetis_clock_event_ddata *pit =
+ container_of(evt, struct kinetis_clock_event_ddata, evtdev);
+
+ kinetis_pit_enable(pit, 0);
+
+ return 0;
+}
+
+/*
+ * Configure the timer to generate an interrupt in the specified amount of ticks
+ */
+static int kinetis_clockevent_tmr_set_next_event(
+ unsigned long delta, struct clock_event_device *c)
+{
+ struct kinetis_clock_event_ddata *pit =
+ container_of(c, struct kinetis_clock_event_ddata, evtdev);
+
+ kinetis_pit_init(pit, delta);
+ kinetis_pit_enable(pit, 1);
+
+ return 0;
+}
+
+/*
+ * Timer IRQ handler
+ */
+static irqreturn_t kinetis_clockevent_tmr_irq_handler(int irq, void *dev_id)
+{
+ struct kinetis_clock_event_ddata *tmr = dev_id;
+
+ iowrite32(KINETIS_PIT_TFLG_TIF_MSK, KINETIS_PIT_PTR(tmr->base, tflg));
+
+ tmr->evtdev.event_handler(&tmr->evtdev);
+
+ return IRQ_HANDLED;
+}
+
+static void __init kinetis_clockevent_init(struct device_node *np)
+{
+ const u64 max_delay_in_sec = 5;
+ struct kinetis_clock_event_ddata *kinetis_tmr;
+ struct device_node *child;
+ struct clk *clk;
+ void __iomem *base;
+ void __iomem *mcr;
+ unsigned long rate;
+ int irq;
+
+ clk = of_clk_get(np, 0);
+ if (IS_ERR(clk)) {
+ pr_err("failed to get clock for clockevent\n");
+ return;
+ }
+
+ if (clk_prepare_enable(clk)) {
+ pr_err("failed to enable timer clock for clockevent\n");
+ goto err_clk_enable;
+ }
+
+ rate = clk_get_rate(clk);
+ if (!(rate / HZ)) {
+ pr_err("failed to get proper clock rate for clockevent\n");
+ goto err_get_rate;
+ }
+
+ mcr = of_iomap(np, 0);
+ if (!mcr) {
+ pr_err("failed to get mcr for clockevent\n");
+ goto err_iomap_mcr;
+ }
+
+ for_each_child_of_node(np, child) {
+ if (!of_device_is_available(child))
+ continue;
+
+ kinetis_tmr = kzalloc(sizeof(struct kinetis_clock_event_ddata),
+ GFP_KERNEL);
+ if (!kinetis_tmr)
+ continue;
+
+
+ base = of_iomap(child, 0);
+ if (!base) {
+ pr_err("failed to get registers for pit channel\n");
+ kfree(kinetis_tmr);
+ continue;
+ }
+
+ irq = irq_of_parse_and_map(child, 0);
+ if (irq <= 0) {
+ pr_err("failed to get irq for pit channel\n");
+ iounmap(base);
+ kfree(kinetis_tmr);
+ continue;
+ }
+
+ kinetis_tmr->evtdev.name = child->full_name;
+ kinetis_tmr->evtdev.rating = 200;
+ kinetis_tmr->evtdev.features = CLOCK_EVT_FEAT_PERIODIC |
+ CLOCK_EVT_FEAT_ONESHOT;
+ kinetis_tmr->evtdev.set_next_event =
+ kinetis_clockevent_tmr_set_next_event;
+ kinetis_tmr->evtdev.set_state_periodic =
+ kinetis_clockevent_tmr_set_state_periodic;
+ kinetis_tmr->evtdev.set_state_oneshot =
+ kinetis_clockevent_tmr_set_state_oneshot;
+ kinetis_tmr->evtdev.set_state_oneshot_stopped =
+ kinetis_clockevent_tmr_set_state_oneshot;
+ kinetis_tmr->evtdev.set_state_shutdown =
+ kinetis_clockevent_tmr_set_state_oneshot;
+ kinetis_tmr->base = base;
+ kinetis_tmr->mcr = mcr;
+ spin_lock_init(&kinetis_tmr->lock);
+
+ /*
+ * Set the fields required for the set_next_event method
+ * (tickless kernel support)
+ */
+ clockevents_calc_mult_shift(&kinetis_tmr->evtdev, rate,
+ max_delay_in_sec);
+ kinetis_tmr->evtdev.max_delta_ns =
+ max_delay_in_sec * NSEC_PER_SEC;
+ kinetis_tmr->evtdev.min_delta_ns = clockevent_delta2ns(0xf,
+ &kinetis_tmr->evtdev);
+
+ clockevents_register_device(&kinetis_tmr->evtdev);
+
+ kinetis_pit_init(kinetis_tmr, (rate / HZ) - 1);
+ kinetis_pit_enable(kinetis_tmr, 1);
+
+ if (request_irq(irq, kinetis_clockevent_tmr_irq_handler,
+ IRQF_TIMER | IRQF_IRQPOLL,
+ "kinetis-timer",
+ kinetis_tmr)) {
+ pr_err("failed to request irq for pit channel\n");
+ kinetis_pit_enable(kinetis_tmr, 0);
+ continue;
+ }
+
+ pr_info("prepared pit channel at MMIO %#x\n", (unsigned)base);
+ }
+
+ return;
+
+err_iomap_mcr:
+err_get_rate:
+
+ clk_disable_unprepare(clk);
+err_clk_enable:
+
+ clk_put(clk);
+}
+
+CLOCKSOURCE_OF_DECLARE(kinetis_pit_timer, "fsl,kinetis-pit-timer",
+ kinetis_clockevent_init);
--
2.3.6