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

[Paul Osmialowski <pawelo@king.net.pl>]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3519 bytes --]

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
>

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: TEXT/x-diff; name=0004-arm-twr-k70f120m-timer-driver-for-Kinetis-SoC.patch, Size: 12866 bytes --]

From 562decc41dc7302c378bf6e4509a22561ff5a85e Mon Sep 17 00:00:00 2001
From: Paul Osmialowski <pawelo@king.net.pl>
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@emcraft.com>

Signed-off-by: Paul Osmialowski <pawelo@king.net.pl>
---
 .../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@emcraft.com>
+ *
+ * Copyright (C) 2015 Paul Osmialowski <pawelo@king.net.pl>
+ *
+ * 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